Re: [PATCH net-next v3] net: phy: broadcom: add 1000Base-X support for BCM54616S

[Heiner Kallweit]

On 04.08.2019 21:22, Vladimir Oltean wrote:
> On Sun, 4 Aug 2019 at 19:07, Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
>>
>> On 04.08.2019 17:59, Vladimir Oltean wrote:
>>> On Sun, 4 Aug 2019 at 17:52, Andrew Lunn <andrew@xxxxxxx> 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