[PATCH net-next 1/2] net: phy: aquantia: validate PHY mode on AQR107

[PATCH net-next 1/2] net: phy: aquantia: validate PHY mode on AQR107

[Robert Marko]

The Aquantia driver is not setting the PHY mode itself, but it does however
still check if the PHY mode set in DTS is one of the supported modes.

However, the set PHY mode does not have to match the actual one, so lets
add update the PHY mode during .config_init and warn if they differ.

Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 drivers/net/phy/aquantia/aquantia_main.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index cc4a97741c4a..7711e052e737 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -505,6 +505,21 @@ static void aqr107_chip_info(struct phy_device *phydev)
 		   fw_major, fw_minor, build_id, prov_id);
 }
 
+static void aqr107_validate_mode(struct phy_device *phydev,
+				 phy_interface_t dts_mode)
+{
+	int ret;
+
+	/* Get the actual PHY mode */
+	ret = aqr107_read_status(phydev);
+	if (ret)
+		return;
+
+	if (dts_mode != phydev->interface)
+		phydev_info(phydev, "%s mode is set in DTS while %s mode is actual. Please update your devicetree.\n",
+			    phy_modes(dts_mode), phy_modes(phydev->interface));
+}
+
 static int aqr107_config_init(struct phy_device *phydev)
 {
 	int ret;
@@ -528,6 +543,8 @@ static int aqr107_config_init(struct phy_device *phydev)
 	if (!ret)
 		aqr107_chip_info(phydev);
 
+	aqr107_validate_mode(phydev, phydev->interface);
+
 	return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT);
 }
 
-- 
2.42.0

[PATCH net-next 2/2] net: phy: aquantia: enable USXGMII autoneg on AQR107

[Robert Marko]

In case USXGMII is being used as the PHY interface mode then USXGMII
autoneg must be enabled as well.

HW defaults to USXGMII autoneg being disabled which then results in
autoneg timeout, so enable it in case USXGMII is used.

Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 drivers/net/phy/aquantia/aquantia_main.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index 7711e052e737..c602873052a0 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -26,6 +26,9 @@
 #define PHY_ID_AQR412	0x03a1b712
 #define PHY_ID_AQR113C	0x31c31c12
 
+#define MDIO_PHYXS_XAUI_RX_VEND2		0xc441
+#define MDIO_PHYXS_XAUI_RX_VEND2_USX_AUTONEG_EN	BIT(3)
+
 #define MDIO_PHYXS_VEND_IF_STATUS		0xe812
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK	GENMASK(7, 3)
 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR	0
@@ -545,6 +548,15 @@ static int aqr107_config_init(struct phy_device *phydev)
 
 	aqr107_validate_mode(phydev, phydev->interface);
 
+	if (phydev->interface == PHY_INTERFACE_MODE_USXGMII) {
+		ret = phy_modify_mmd(phydev, MDIO_MMD_PHYXS,
+				     MDIO_PHYXS_XAUI_RX_VEND2,
+				     MDIO_PHYXS_XAUI_RX_VEND2_USX_AUTONEG_EN,
+				     MDIO_PHYXS_XAUI_RX_VEND2_USX_AUTONEG_EN);
+		if (ret)
+			return ret;
+	}
+
 	return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT);
 }
 
-- 
2.42.0

Re: [PATCH net-next 1/2] net: phy: aquantia: validate PHY mode on AQR107

[Russell King (Oracle)]

On Fri, Nov 17, 2023 at 11:09:48AM +0100, Robert Marko wrote:
> The Aquantia driver is not setting the PHY mode itself, but it does however
> still check if the PHY mode set in DTS is one of the supported modes.
> 
> However, the set PHY mode does not have to match the actual one, so lets
> add update the PHY mode during .config_init and warn if they differ.

This looks completely wrong to me. These PHYs can be configured to
change their MAC-facing interface mode according to the media negotiated
speed, but you are only checking that _if_ the media is up, then the
interface that has resulted from that negotiation matches what is in
DTS. That could be dependent on the link partner, so what works for a
platform when connected to one link partner may issue your "info"-level
warning when connected to a different link partner.

So no, this to me looks completely wrong.

You need to check the VEND1_GLOBAL_CFG_* registers, and determine from
those what interface mode(s) will be used, and then use that to validate
the mode.

It just so happens that...

http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=f7b531ee8855f81d267a8a42c44da51576f48daf
http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=f55389aa5d11da8a32dfd65a1b98049878ce09f0

builds a bitmap that can then be tested to check this. Whether the
above is entirely correct or not, I can't really say, I don't have
enough information on this PHY.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/2] net: phy: aquantia: enable USXGMII autoneg on AQR107

[Russell King (Oracle)]

On Fri, Nov 17, 2023 at 11:09:49AM +0100, Robert Marko wrote:
> In case USXGMII is being used as the PHY interface mode then USXGMII
> autoneg must be enabled as well.
> 
> HW defaults to USXGMII autoneg being disabled which then results in
> autoneg timeout, so enable it in case USXGMII is used.

I was led to believe that the bitfield in bits 8:7 of the
VEND1_GLOBAL_CFG_* registers, when set to value '1' is something
to do with selecting USXGMII mode as opposed to 10GBASE-R. Could
you look in to that and whether that is the more correct way of
configuring the PHY for USXGMII mode?

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/2] net: phy: aquantia: validate PHY mode on AQR107

[Robert Marko]

On Fri, 17 Nov 2023 at 13:46, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Fri, Nov 17, 2023 at 11:09:48AM +0100, Robert Marko wrote:
> > The Aquantia driver is not setting the PHY mode itself, but it does however
> > still check if the PHY mode set in DTS is one of the supported modes.
> >
> > However, the set PHY mode does not have to match the actual one, so lets
> > add update the PHY mode during .config_init and warn if they differ.
>
> This looks completely wrong to me. These PHYs can be configured to
> change their MAC-facing interface mode according to the media negotiated
> speed, but you are only checking that _if_ the media is up, then the
> interface that has resulted from that negotiation matches what is in
> DTS. That could be dependent on the link partner, so what works for a
> platform when connected to one link partner may issue your "info"-level
> warning when connected to a different link partner.
>
> So no, this to me looks completely wrong.
>
> You need to check the VEND1_GLOBAL_CFG_* registers, and determine from
> those what interface mode(s) will be used, and then use that to validate
> the mode.
>
> It just so happens that...
>
> http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=f7b531ee8855f81d267a8a42c44da51576f48daf
> http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=f55389aa5d11da8a32dfd65a1b98049878ce09f0
>
> builds a bitmap that can then be tested to check this. Whether the
> above is entirely correct or not, I can't really say, I don't have
> enough information on this PHY.

Hi,
Yeah, I get the issue now.

Nice, those got merged into net-next, so I will iterate by using those.

Regards,
Robert
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/2] net: phy: aquantia: enable USXGMII autoneg on AQR107

[Robert Marko]

On Fri, 17 Nov 2023 at 13:47, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Fri, Nov 17, 2023 at 11:09:49AM +0100, Robert Marko wrote:
> > In case USXGMII is being used as the PHY interface mode then USXGMII
> > autoneg must be enabled as well.
> >
> > HW defaults to USXGMII autoneg being disabled which then results in
> > autoneg timeout, so enable it in case USXGMII is used.
>
> I was led to believe that the bitfield in bits 8:7 of the
> VEND1_GLOBAL_CFG_* registers, when set to value '1' is something
> to do with selecting USXGMII mode as opposed to 10GBASE-R. Could
> you look in to that and whether that is the more correct way of
> configuring the PHY for USXGMII mode?

Hi,

bits 8:7 in the VEND1_GLOBAL_CFG_* are used to configure the rate
adaptation method.
With the following allowed values:
0 (Default) = No rate adaptation
1 = USX rate adaptation
2 = Pause rate adaptation

I dont think that is related to the issue I am facing here which is
that by default
Bit 3 in the PHY XS Transmit (XAUI Rx) Reserved Vendor Provisioning 2 register
is set to 0.

This means that USX Autoneg Control For MAC is disabled and in USXGMII mode
auto-negotiation between the PHY and MAC will fail/timeout.

I have checked various vendor drivers and they all enable this bit in
case USXGMII
is used.

Regards,
Robert

>
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!