[PATCH net-next] net: phy: let genphy_c45_read_link manage the devices to check

[PATCH net-next] net: phy: let genphy_c45_read_link manage the devices to check

[Heiner Kallweit]

Let genphy_c45_read_link manage the devices to check, this removes
overhead from callers. Devices VEND1 and VEND2 will never be checked,
for now adopt the logic of the Marvell driver to also exclude PHY XS.

At the moment we have very few clause 45 PHY drivers, so we are
lacking experience whether other drivers will have to exclude further
devices, or may need to check PHY XS. If we should figure out that
list of devices to check needs to be configurable, I think best will
be to add a device list member to struct phy_driver.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/marvell10g.c | 10 +---------
 drivers/net/phy/phy-c45.c    | 17 +++++++++--------
 include/linux/phy.h          |  2 +-
 include/uapi/linux/mdio.h    |  2 ++
 4 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 296a537cd..96a79c6c7 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -428,16 +428,8 @@ static int mv3310_read_10gbr_status(struct phy_device *phydev)
 
 static int mv3310_read_status(struct phy_device *phydev)
 {
-	u32 mmd_mask = phydev->c45_ids.devices_in_package;
 	int val;
 
-	/* The vendor devads do not report link status.  Avoid the PHYXS
-	 * instance as there are three, and its status depends on the MAC
-	 * being appropriately configured for the negotiated speed.
-	 */
-	mmd_mask &= ~(BIT(MDIO_MMD_VEND1) | BIT(MDIO_MMD_VEND2) |
-		      BIT(MDIO_MMD_PHYXS));
-
 	phydev->speed = SPEED_UNKNOWN;
 	phydev->duplex = DUPLEX_UNKNOWN;
 	linkmode_zero(phydev->lp_advertising);
@@ -453,7 +445,7 @@ static int mv3310_read_status(struct phy_device *phydev)
 	if (val & MDIO_STAT1_LSTATUS)
 		return mv3310_read_10gbr_status(phydev);
 
-	val = genphy_c45_read_link(phydev, mmd_mask);
+	val = genphy_c45_read_link(phydev);
 	if (val < 0)
 		return val;
 
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 449f0f986..d429c6a3e 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -118,17 +118,23 @@ EXPORT_SYMBOL_GPL(genphy_c45_aneg_done);
 /**
  * genphy_c45_read_link - read the overall link status from the MMDs
  * @phydev: target phy_device struct
- * @mmd_mask: MMDs to read status from
  *
  * Read the link status from the specified MMDs, and if they all indicate
  * that the link is up, set phydev->link to 1.  If an error is encountered,
  * a negative errno will be returned, otherwise zero.
  */
-int genphy_c45_read_link(struct phy_device *phydev, u32 mmd_mask)
+int genphy_c45_read_link(struct phy_device *phydev)
 {
+	u32 mmd_mask = phydev->c45_ids.devices_in_package;
 	int val, devad;
 	bool link = true;
 
+	/* The vendor devads do not report link status.  Avoid the PHYXS
+	 * instance as its status may depend on the MAC being appropriately
+	 * configured for the negotiated speed.
+	 */
+	mmd_mask &= ~(MDIO_DEVS_VEND1 | MDIO_DEVS_VEND2 | MDIO_DEVS_PHYXS);
+
 	while (mmd_mask && link) {
 		devad = __ffs(mmd_mask);
 		mmd_mask &= ~BIT(devad);
@@ -272,16 +278,11 @@ EXPORT_SYMBOL_GPL(gen10g_config_aneg);
 
 int gen10g_read_status(struct phy_device *phydev)
 {
-	u32 mmd_mask = phydev->c45_ids.devices_in_package;
-
 	/* For now just lie and say it's 10G all the time */
 	phydev->speed = SPEED_10000;
 	phydev->duplex = DUPLEX_FULL;
 
-	/* Avoid reading the vendor MMDs */
-	mmd_mask &= ~(BIT(MDIO_MMD_VEND1) | BIT(MDIO_MMD_VEND2));
-
-	return genphy_c45_read_link(phydev, mmd_mask);
+	return genphy_c45_read_link(phydev);
 }
 EXPORT_SYMBOL_GPL(gen10g_read_status);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 237dd0358..f41bf651f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1094,7 +1094,7 @@ int genphy_write_mmd_unsupported(struct phy_device *phdev, int devnum,
 /* Clause 45 PHY */
 int genphy_c45_restart_aneg(struct phy_device *phydev);
 int genphy_c45_aneg_done(struct phy_device *phydev);
-int genphy_c45_read_link(struct phy_device *phydev, u32 mmd_mask);
+int genphy_c45_read_link(struct phy_device *phydev);
 int genphy_c45_read_lpa(struct phy_device *phydev);
 int genphy_c45_read_pma(struct phy_device *phydev);
 int genphy_c45_pma_setup_forced(struct phy_device *phydev);
diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index d435b00d6..2e6e309f0 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -123,6 +123,8 @@
 #define MDIO_DEVS_TC			MDIO_DEVS_PRESENT(MDIO_MMD_TC)
 #define MDIO_DEVS_AN			MDIO_DEVS_PRESENT(MDIO_MMD_AN)
 #define MDIO_DEVS_C22EXT		MDIO_DEVS_PRESENT(MDIO_MMD_C22EXT)
+#define MDIO_DEVS_VEND1			MDIO_DEVS_PRESENT(MDIO_MMD_VEND1)
+#define MDIO_DEVS_VEND2			MDIO_DEVS_PRESENT(MDIO_MMD_VEND2)
 
 /* Control register 2. */
 #define MDIO_PMA_CTRL2_TYPE		0x000f	/* PMA/PMD type selection */
-- 
2.20.1

Re: [PATCH net-next] net: phy: let genphy_c45_read_link manage the devices to check

[Andrew Lunn]

On Thu, Feb 07, 2019 at 08:05:55PM +0100, Heiner Kallweit wrote:
> Let genphy_c45_read_link manage the devices to check, this removes
> overhead from callers. Devices VEND1 and VEND2 will never be checked,
> for now adopt the logic of the Marvell driver to also exclude PHY XS.
> 
> At the moment we have very few clause 45 PHY drivers, so we are
> lacking experience whether other drivers will have to exclude further
> devices, or may need to check PHY XS. If we should figure out that
> list of devices to check needs to be configurable, I think best will
> be to add a device list member to struct phy_driver.

Hi Heiner

For the Aquantia PHY you probably need to exclude MDIO_MMD_C22EXT.

There is no register 1D:1 listed in the datasheet.

PHY XS is the interface towards the MAC. You cannot configure the MAC
to use the correct interface mode until the PHY has link to its peer
and the link mode has been determined. So you need the PHY to signal
link up independent of the MAC-PHY link, to kick off the configuration
of the MAC. When using phylink, the MAC can indicate it has a link to
the PHY using phylink_mac_change().

      Andrew
 

Re: [PATCH net-next] net: phy: let genphy_c45_read_link manage the devices to check

[Heiner Kallweit]

On 07.02.2019 20:57, Andrew Lunn wrote:
> On Thu, Feb 07, 2019 at 08:05:55PM +0100, Heiner Kallweit wrote:
>> Let genphy_c45_read_link manage the devices to check, this removes
>> overhead from callers. Devices VEND1 and VEND2 will never be checked,
>> for now adopt the logic of the Marvell driver to also exclude PHY XS.
>>
>> At the moment we have very few clause 45 PHY drivers, so we are
>> lacking experience whether other drivers will have to exclude further
>> devices, or may need to check PHY XS. If we should figure out that
>> list of devices to check needs to be configurable, I think best will
>> be to add a device list member to struct phy_driver.
> 
> Hi Heiner
> 
> For the Aquantia PHY you probably need to exclude MDIO_MMD_C22EXT.
> 
> There is no register 1D:1 listed in the datasheet.
> 
Thanks, Andrew. Right, the Aquantia PHY doesn't seem to have the C22EXT
MMD. Because the Aquantia PHY has no device 29 in its package the code
should work.
I also checked the 802.3 spec and it says that registers 29.0 to
29.4 are reserved. At another location the 802.3 spec states that read
access to non-existing registers should return a zero value.
Therefore current code (when reading a 0 from 29.1) may come to the
false conclusion that device 29 reports link down.
So it seems we have to exclude device C22EXT in general. I'll add that
and submit a v2.

> PHY XS is the interface towards the MAC. You cannot configure the MAC
> to use the correct interface mode until the PHY has link to its peer
> and the link mode has been determined. So you need the PHY to signal
> link up independent of the MAC-PHY link, to kick off the configuration
> of the MAC. When using phylink, the MAC can indicate it has a link to
> the PHY using phylink_mac_change().
> 
OK, so excluding PHY XS is right.

>       Andrew
>  
> 
Heiner

Re: [PATCH net-next] net: phy: let genphy_c45_read_link manage the devices to check

[Andrew Lunn]

> Thanks, Andrew. Right, the Aquantia PHY doesn't seem to have the C22EXT
> MMD. Because the Aquantia PHY has no device 29 in its package the code
> should work.

It lists device 29 in its devices in package. So the current code does
look there.

> So it seems we have to exclude device C22EXT in general. I'll add that
> and submit a v2.

Yes.

	Andrew

Re: [PATCH net-next] net: phy: let genphy_c45_read_link manage the devices to check

[Heiner Kallweit]

On 07.02.2019 21:50, Andrew Lunn wrote:
>> Thanks, Andrew. Right, the Aquantia PHY doesn't seem to have the C22EXT
>> MMD. Because the Aquantia PHY has no device 29 in its package the code
>> should work.
> 
> It lists device 29 in its devices in package. So the current code does
> look there.
> 
I just looked for a description of a device 29. Strange that the device
list states it's there and then it's not there.

When checking that I was scratching my head because of the following code
in genphy_c45_read_link:

devad = __ffs(mmd_mask);
mmd_mask &= ~BIT(devad);

AFAIK __ffs() returns the posix bit number, means it returns 1 for bit 0.
Then this code piece seems to be wrong because I think the intention is
to clear the bit we just found. Instead we clear the next bit.

And device 0 isn't really a device but a flag "Clause 22 registers present".
So far we may have been lucky because to supported 10G PHY has this flag set.
But if this flag is set and we try to access a register 0.1 then we may be
in trouble again. Therefore I think we need to exclude also device 0.
Or do I miss something?

>> So it seems we have to exclude device C22EXT in general. I'll add that
>> and submit a v2.
> 
> Yes.
> 
> 	Andrew
> 
Heiner

Re: [PATCH net-next] net: phy: let genphy_c45_read_link manage the devices to check

[Heiner Kallweit]

On 07.02.2019 22:54, Heiner Kallweit wrote:
> On 07.02.2019 21:50, Andrew Lunn wrote:
>>> Thanks, Andrew. Right, the Aquantia PHY doesn't seem to have the C22EXT
>>> MMD. Because the Aquantia PHY has no device 29 in its package the code
>>> should work.
>>
>> It lists device 29 in its devices in package. So the current code does
>> look there.
>>
> I just looked for a description of a device 29. Strange that the device
> list states it's there and then it's not there.
> 
> When checking that I was scratching my head because of the following code
> in genphy_c45_read_link:
> 
> devad = __ffs(mmd_mask);
> mmd_mask &= ~BIT(devad);
> 
> AFAIK __ffs() returns the posix bit number, means it returns 1 for bit 0.
> Then this code piece seems to be wrong because I think the intention is
> to clear the bit we just found. Instead we clear the next bit.
> 
Nope, it doesn't seem to be the posix version .. Just checked the code of __ffs().

> And device 0 isn't really a device but a flag "Clause 22 registers present".
> So far we may have been lucky because to supported 10G PHY has this flag set.
> But if this flag is set and we try to access a register 0.1 then we may be
> in trouble again. Therefore I think we need to exclude also device 0.
> Or do I miss something?
> 
>>> So it seems we have to exclude device C22EXT in general. I'll add that
>>> and submit a v2.
>>
>> Yes.
>>
>> 	Andrew
>>
> Heiner
> 

Re: [PATCH net-next] net: phy: let genphy_c45_read_link manage the devices to check

[Russell King - ARM Linux admin]

On Thu, Feb 07, 2019 at 10:54:51PM +0100, Heiner Kallweit wrote:
> On 07.02.2019 21:50, Andrew Lunn wrote:
> >> Thanks, Andrew. Right, the Aquantia PHY doesn't seem to have the C22EXT
> >> MMD. Because the Aquantia PHY has no device 29 in its package the code
> >> should work.
> > 
> > It lists device 29 in its devices in package. So the current code does
> > look there.
> > 
> I just looked for a description of a device 29. Strange that the device
> list states it's there and then it's not there.
> 
> When checking that I was scratching my head because of the following code
> in genphy_c45_read_link:
> 
> devad = __ffs(mmd_mask);
> mmd_mask &= ~BIT(devad);
> 
> AFAIK __ffs() returns the posix bit number, means it returns 1 for bit 0.

I think you're wrong.  Look at include/asm-generic/bitops/__ffs.h.  If
'word' is 0x00000001, then the function returns zero.  If 'word' is
0x00000002, it returns 1, etc.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up