Messages in this thread | | | From | Christian Herber <> | Subject | Re: [EXT] Re: [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem | Date | Fri, 16 Aug 2019 12:05:33 +0000 |
| |
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@nxp.com> >>> --- >>> 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
| |