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

From: Christian Herber
Date: Fri Aug 16 2019 - 08:05:39 EST


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