Messages in this thread | | | From | Vladimir Oltean <> | Date | Sun, 4 Aug 2019 22:22:01 +0300 | Subject | Re: [PATCH net-next v3] net: phy: broadcom: add 1000Base-X support for BCM54616S |
| |
On Sun, 4 Aug 2019 at 19:07, Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 04.08.2019 17:59, Vladimir Oltean wrote: > > On Sun, 4 Aug 2019 at 17:52, Andrew Lunn <andrew@lunn.ch> wrote: > >> > >>>> The patchset looks better now. But is it ok, I wonder, to keep > >>>> PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that > >>>> phy_attach_direct is overwriting it? > >>> > >> > >>> I checked ftgmac100 driver (used on my machine) and it calls > >>> phy_connect_direct which passes phydev->dev_flags when calling > >>> phy_attach_direct: that explains why the flag is not cleared in my > >>> case. > >> > >> Yes, that is the way it is intended to be used. The MAC driver can > >> pass flags to the PHY. It is a fragile API, since the MAC needs to > >> know what PHY is being used, since the flags are driver specific. > >> > >> One option would be to modify the assignment in phy_attach_direct() to > >> OR in the flags passed to it with flags which are already in > >> phydev->dev_flags. > >> > >> Andrew > > > > Even if that were the case (patching phy_attach_direct to apply a > > logical-or to dev_flags), it sounds fishy to me that the genphy code > > is unable to determine that this PHY is running in 1000Base-X mode. > > > > In my opinion it all boils down to this warning: > > > > "PHY advertising (0,00000200,000062c0) more modes than genphy > > supports, some modes not advertised". > > > The genphy code deals with Clause 22 + Gigabit BaseT only. > Question is whether you want aneg at all in 1000Base-X mode and > what you want the config_aneg callback to do. > There may be some inspiration in the Marvel PHY drivers. >
AN for 1000Base-X still gives you duplex and pause frame settings. I thought the base page format for exchanging that info is standardized in clause 37. Does genphy cover only copper media by design, or is it desirable to augment genphy_read_status?
> > You see, the 0x200 in the above advertising mask corresponds exactly > > to this definition from ethtool.h: > > ETHTOOL_LINK_MODE_1000baseX_Full_BIT = 41, > > > > But it gets truncated and hence lost. > > > > Regards, > > -Vladimir > > > Heiner
| |