On 8/5/19 1:45 PM, Heiner Kallweit wrote: > On 04.08.2019 21:22, Vladimir Oltean wrote: >> On Sun, 4 Aug 2019 at 19:07, Heiner Kallweit <hkallwe...@gmail.com> wrote: >>> >>> On 04.08.2019 17:59, Vladimir Oltean wrote: >>>> On Sun, 4 Aug 2019 at 17:52, Andrew Lunn <and...@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? >> > 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.
Thank you all for the inputs on this patch. If I understand correctly, we are going to create a set of genphy_c37_* functions for 1000x support so it can be used by phy drivers? Or are we considering other options? What's your recommendation on this specific patch? Thanks, Tao