On 04.08.2019 21:22, Vladimir Oltean wrote: > On Sun, 4 Aug 2019 at 19:07, Heiner Kallweit <[email protected]> wrote: >> >> On 04.08.2019 17:59, Vladimir Oltean wrote: >>> On Sun, 4 Aug 2019 at 17:52, Andrew Lunn <[email protected]> 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? > So far we care about copper only in phylib. Some constants needed for Clause 37 support are defined, but used by few drivers only.
ADVERTISE_1000XHALF ADVERTISE_1000XFULL ADVERTISE_1000XPAUSE ADVERTISE_1000XPSE_ASYM I think it would make sense to have something like genphy_c37_config_aneg. Similar for 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 > Heiner
