Re: [EXT] Re: [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem

[Christian Herber]

On 15.08.2019 18:34, Heiner Kallweit wrote:
> Caution: EXT Email
> 
> On 15.08.2019 17:56, Andrew Lunn wrote:
>> On Thu, Aug 15, 2019 at 03:32:29PM +0000, Christian Herber wrote:
>>> BASE-T1 is a category of Ethernet PHYs.
>>> They use a single copper pair for transmission.
>>> This patch add basic support for this category of PHYs.
>>> It coveres the discovery of abilities and basic configuration.
>>> It includes setting fixed speed and enabling auto-negotiation.
>>> BASE-T1 devices should always Clause-45 managed.
>>> Therefore, this patch extends phy-c45.c.
>>> While for some functions like auto-neogtiation different registers are
>>> used, the layout of these registers is the same for the used fields.
>>> Thus, much of the logic of basic Clause-45 devices can be reused.
>>>
>>> Signed-off-by: Christian Herber <christian.herber@xxxxxxx>
>>> ---
>>>   drivers/net/phy/phy-c45.c    | 113 +++++++++++++++++++++++++++++++----
>>>   drivers/net/phy/phy-core.c   |   4 +-
>>>   include/uapi/linux/ethtool.h |   2 +
>>>   include/uapi/linux/mdio.h    |  21 +++++++
>>>   4 files changed, 129 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
>>> index b9d4145781ca..9ff0b8c785de 100644
>>> --- a/drivers/net/phy/phy-c45.c
>>> +++ b/drivers/net/phy/phy-c45.c
>>> @@ -8,13 +8,23 @@
>>>   #include <linux/mii.h>
>>>   #include <linux/phy.h>
>>>
>>> +#define IS_100BASET1(phy) (linkmode_test_bit( \
>>> +                       ETHTOOL_LINK_MODE_100baseT1_Full_BIT, \
>>> +                       (phy)->supported))
>>> +#define IS_1000BASET1(phy) (linkmode_test_bit( \
>>> +                        ETHTOOL_LINK_MODE_1000baseT1_Full_BIT, \
>>> +                        (phy)->supported))
>>
>> Hi Christian
>>
>> We already have the flag phydev->is_gigabit_capable. Maybe add a flag
>> phydev->is_t1_capable
>>
>>> +
>>> +static u32 get_aneg_ctrl(struct phy_device *phydev);
>>> +static u32 get_aneg_stat(struct phy_device *phydev);
>>
>> No forward declarations please. Put the code in the right order so
>> they are not needed.
>>
>> Thanks
>>
>>       Andrew
>>
> 
> For whatever reason I don't have the original mail in my netdev inbox (yet).
> 
> +       if (IS_100BASET1(phydev) || IS_1000BASET1(phydev))
> +               ctrl = MDIO_AN_BT1_CTRL;
> 
> Code like this could be problematic once a PHY supports one of the T1 modes
> AND normal modes. Then normal modes would be unusable.
> 
> I think this scenario isn't completely hypothetical. See the Aquantia
> AQCS109 that supports normal modes and (proprietary) 1000Base-T2.
> 
> Maybe we need separate versions of the generic functions for T1.
> Then it would be up to the PHY driver to decide when to use which
> version.
> 
> Heiner
> 

Integrating this with the existing driver or creating a new on is an 
interesting question. I came to the conclusion that it is most efficient 
to integrate to avoid all to much copy paste code.

So far, I am not aware of any device that supports T1 and something else 
at the same time. From a HW perspective, I also consider this quite 
unlikely. In the unlikely case that such a device comes up, support from 
the genphy driver would be limited to the BASE-T1 modes. But i would 
rather create the special case for the special device and cater the 
current mainstream support to the mainstream devices.

I think this boils down to a general strategy for the PHY framework, as 
this can happen for other classes of devices also like NGBASE-T1, 
MultiGBASE-T and future unknown devices. For now, I think the 
architecture is sufficiently scalable with a single c45 genphy driver

Christian