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

[Tao Ren <taoren@fb.com>]

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 <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?
>>
> 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