Messages in this thread | | | Subject | Re: [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem | From | Heiner Kallweit <> | Date | Thu, 15 Aug 2019 18:34:11 +0200 |
| |
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
| |