[PATCH net-next v3 1/2] dt-bindings: net: marvell,aquantia: add property to override MDI_CFG

[PATCH net-next v3 1/2] dt-bindings: net: marvell,aquantia: add property to override MDI_CFG

[Daniel Golle]

Usually the MDI pair order reversal configuration is defined by
bootstrap pin MDI_CFG. Some designs, however, require overriding the MDI
pair order and force either normal or reverse order.

Add property 'marvell,mdi-cfg-order' to allow forcing either normal or
reverse order of the MDI pairs.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v3: use integer enum instead of two properties as suggested
v2: enforce mutually exclusive relationship of the two new properties in
    dt-schema.

 Documentation/devicetree/bindings/net/marvell,aquantia.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/marvell,aquantia.yaml b/Documentation/devicetree/bindings/net/marvell,aquantia.yaml
index 9854fab4c4db..f269615126d8 100644
--- a/Documentation/devicetree/bindings/net/marvell,aquantia.yaml
+++ b/Documentation/devicetree/bindings/net/marvell,aquantia.yaml
@@ -48,6 +48,12 @@ properties:
   firmware-name:
     description: specify the name of PHY firmware to load
 
+  marvell,mdi-cfg-order:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]
+    description:
+      force normal (0) or reverse (1) order of MDI pairs, overriding MDI_CFG bootstrap pin.
+
   nvmem-cells:
     description: phandle to the firmware nvmem cell
     maxItems: 1
-- 
2.46.2

[PATCH net-next v3 2/2] net: phy: aquantia: allow forcing order of MDI pairs

[Daniel Golle]

Despite supporting Auto MDI-X, it looks like Aquantia only supports
swapping pair (1,2) with pair (3,6) like it used to be for MDI-X on
100MBit/s networks.

When all 4 pairs are in use (for 1000MBit/s or faster) the link does not
come up with pair order is not configured correctly, either using
MDI_CFG pin or using the "PMA Receive Reserved Vendor Provisioning 1"
register.

Normally, the order of MDI pairs being either ABCD or DCBA is configured
by pulling the MDI_CFG pin.

However, some hardware designs require overriding the value configured
by that bootstrap pin. The PHY allows doing that by setting a bit in
"PMA Receive Reserved Vendor Provisioning 1" register which allows
ignoring the state of the MDI_CFG pin and another bit configuring
whether the order of MDI pairs should be normal (ABCD) or reverse
(DCBA). Pair polarity is not affected and remains identical in both
settings.

Introduce property "marvell,mdi-cfg-order" which allows forcing either
normal or reverse order of the MDI pairs from DT.

If the property isn't present, the behavior is unchanged and MDI pair
order configuration is untouched (ie. either the result of MDI_CFG pin
pull-up/pull-down, or pair order override already configured by the
bootloader before Linux is started).

Forcing normal pair order is required on the Adtran SDG-8733A Wi-Fi 7
residential gateway.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v3: use u32 'marvell,mdi-cfg-order' instead of two mutually exclusive
    properties as suggested
v2: add missing 'static' keyword, improve commit description

 drivers/net/phy/aquantia/aquantia_main.c | 33 ++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index 4d156d406bab..dcad3fa1ddc3 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/delay.h>
 #include <linux/bitfield.h>
+#include <linux/of.h>
 #include <linux/phy.h>
 
 #include "aquantia.h"
@@ -71,6 +72,11 @@
 #define MDIO_AN_TX_VEND_INT_MASK2		0xd401
 #define MDIO_AN_TX_VEND_INT_MASK2_LINK		BIT(0)
 
+#define PMAPMD_RSVD_VEND_PROV			0xe400
+#define PMAPMD_RSVD_VEND_PROV_MDI_CONF		GENMASK(1, 0)
+#define PMAPMD_RSVD_VEND_PROV_MDI_REVERSE	BIT(0)
+#define PMAPMD_RSVD_VEND_PROV_MDI_FORCE		BIT(1)
+
 #define MDIO_AN_RX_LP_STAT1			0xe820
 #define MDIO_AN_RX_LP_STAT1_1000BASET_FULL	BIT(15)
 #define MDIO_AN_RX_LP_STAT1_1000BASET_HALF	BIT(14)
@@ -485,6 +491,29 @@ static void aqr107_chip_info(struct phy_device *phydev)
 		   fw_major, fw_minor, build_id, prov_id);
 }
 
+static int aqr107_config_mdi(struct phy_device *phydev)
+{
+	struct device_node *np = phydev->mdio.dev.of_node;
+	u32 mdi_conf;
+	int ret;
+
+	ret = of_property_read_u32(np, "marvell,mdi-cfg-order", &mdi_conf);
+
+	/* Do nothing in case property "marvell,mdi-cfg-order" is not present */
+	if (ret == -ENOENT)
+		return 0;
+
+	if (ret)
+		return ret;
+
+	if (mdi_conf & ~PMAPMD_RSVD_VEND_PROV_MDI_REVERSE)
+		return -EINVAL;
+
+	return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, PMAPMD_RSVD_VEND_PROV,
+			      PMAPMD_RSVD_VEND_PROV_MDI_CONF,
+			      mdi_conf | PMAPMD_RSVD_VEND_PROV_MDI_FORCE);
+}
+
 static int aqr107_config_init(struct phy_device *phydev)
 {
 	struct aqr107_priv *priv = phydev->priv;
@@ -514,6 +543,10 @@ static int aqr107_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	ret = aqr107_config_mdi(phydev);
+	if (ret)
+		return ret;
+
 	/* Restore LED polarity state after reset */
 	for_each_set_bit(led_active_low, &priv->leds_active_low, AQR_MAX_LEDS) {
 		ret = aqr_phy_led_active_low_set(phydev, led_active_low, true);
-- 
2.46.2

Re: [PATCH net-next v3 1/2] dt-bindings: net: marvell,aquantia: add property to override MDI_CFG

[Rob Herring (Arm)]

On Fri, 04 Oct 2024 17:18:05 +0100, Daniel Golle wrote:
> Usually the MDI pair order reversal configuration is defined by
> bootstrap pin MDI_CFG. Some designs, however, require overriding the MDI
> pair order and force either normal or reverse order.
> 
> Add property 'marvell,mdi-cfg-order' to allow forcing either normal or
> reverse order of the MDI pairs.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> v3: use integer enum instead of two properties as suggested
> v2: enforce mutually exclusive relationship of the two new properties in
>     dt-schema.
> 
>  Documentation/devicetree/bindings/net/marvell,aquantia.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH net-next v3 2/2] net: phy: aquantia: allow forcing order of MDI pairs

[Andrew Lunn]

On Fri, Oct 04, 2024 at 05:18:16PM +0100, Daniel Golle wrote:
> Despite supporting Auto MDI-X, it looks like Aquantia only supports
> swapping pair (1,2) with pair (3,6) like it used to be for MDI-X on
> 100MBit/s networks.
> 
> When all 4 pairs are in use (for 1000MBit/s or faster) the link does not
> come up with pair order is not configured correctly, either using
> MDI_CFG pin or using the "PMA Receive Reserved Vendor Provisioning 1"
> register.
> 
> Normally, the order of MDI pairs being either ABCD or DCBA is configured
> by pulling the MDI_CFG pin.
> 
> However, some hardware designs require overriding the value configured
> by that bootstrap pin. The PHY allows doing that by setting a bit in
> "PMA Receive Reserved Vendor Provisioning 1" register which allows
> ignoring the state of the MDI_CFG pin and another bit configuring
> whether the order of MDI pairs should be normal (ABCD) or reverse
> (DCBA). Pair polarity is not affected and remains identical in both
> settings.
> 
> Introduce property "marvell,mdi-cfg-order" which allows forcing either
> normal or reverse order of the MDI pairs from DT.
> 
> If the property isn't present, the behavior is unchanged and MDI pair
> order configuration is untouched (ie. either the result of MDI_CFG pin
> pull-up/pull-down, or pair order override already configured by the
> bootloader before Linux is started).
> 
> Forcing normal pair order is required on the Adtran SDG-8733A Wi-Fi 7
> residential gateway.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v3 1/2] dt-bindings: net: marvell,aquantia: add property to override MDI_CFG

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 4 Oct 2024 17:18:05 +0100 you wrote:
> Usually the MDI pair order reversal configuration is defined by
> bootstrap pin MDI_CFG. Some designs, however, require overriding the MDI
> pair order and force either normal or reverse order.
> 
> Add property 'marvell,mdi-cfg-order' to allow forcing either normal or
> reverse order of the MDI pairs.
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/2] dt-bindings: net: marvell,aquantia: add property to override MDI_CFG
    https://git.kernel.org/netdev/net-next/c/1432965bf5ce
  - [net-next,v3,2/2] net: phy: aquantia: allow forcing order of MDI pairs
    https://git.kernel.org/netdev/net-next/c/a2e1ba275eae

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v3 2/2] net: phy: aquantia: allow forcing order of MDI pairs

[Jon Hunter]

Hi Daniel,

On 04/10/2024 17:18, Daniel Golle wrote:
> Despite supporting Auto MDI-X, it looks like Aquantia only supports
> swapping pair (1,2) with pair (3,6) like it used to be for MDI-X on
> 100MBit/s networks.
> 
> When all 4 pairs are in use (for 1000MBit/s or faster) the link does not
> come up with pair order is not configured correctly, either using
> MDI_CFG pin or using the "PMA Receive Reserved Vendor Provisioning 1"
> register.
> 
> Normally, the order of MDI pairs being either ABCD or DCBA is configured
> by pulling the MDI_CFG pin.
> 
> However, some hardware designs require overriding the value configured
> by that bootstrap pin. The PHY allows doing that by setting a bit in
> "PMA Receive Reserved Vendor Provisioning 1" register which allows
> ignoring the state of the MDI_CFG pin and another bit configuring
> whether the order of MDI pairs should be normal (ABCD) or reverse
> (DCBA). Pair polarity is not affected and remains identical in both
> settings.
> 
> Introduce property "marvell,mdi-cfg-order" which allows forcing either
> normal or reverse order of the MDI pairs from DT.
> 
> If the property isn't present, the behavior is unchanged and MDI pair
> order configuration is untouched (ie. either the result of MDI_CFG pin
> pull-up/pull-down, or pair order override already configured by the
> bootloader before Linux is started).
> 
> Forcing normal pair order is required on the Adtran SDG-8733A Wi-Fi 7
> residential gateway.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> v3: use u32 'marvell,mdi-cfg-order' instead of two mutually exclusive
>      properties as suggested
> v2: add missing 'static' keyword, improve commit description
> 
>   drivers/net/phy/aquantia/aquantia_main.c | 33 ++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> index 4d156d406bab..dcad3fa1ddc3 100644
> --- a/drivers/net/phy/aquantia/aquantia_main.c
> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> @@ -11,6 +11,7 @@
>   #include <linux/module.h>
>   #include <linux/delay.h>
>   #include <linux/bitfield.h>
> +#include <linux/of.h>
>   #include <linux/phy.h>
>   
>   #include "aquantia.h"
> @@ -71,6 +72,11 @@
>   #define MDIO_AN_TX_VEND_INT_MASK2		0xd401
>   #define MDIO_AN_TX_VEND_INT_MASK2_LINK		BIT(0)
>   
> +#define PMAPMD_RSVD_VEND_PROV			0xe400
> +#define PMAPMD_RSVD_VEND_PROV_MDI_CONF		GENMASK(1, 0)
> +#define PMAPMD_RSVD_VEND_PROV_MDI_REVERSE	BIT(0)
> +#define PMAPMD_RSVD_VEND_PROV_MDI_FORCE		BIT(1)
> +
>   #define MDIO_AN_RX_LP_STAT1			0xe820
>   #define MDIO_AN_RX_LP_STAT1_1000BASET_FULL	BIT(15)
>   #define MDIO_AN_RX_LP_STAT1_1000BASET_HALF	BIT(14)
> @@ -485,6 +491,29 @@ static void aqr107_chip_info(struct phy_device *phydev)
>   		   fw_major, fw_minor, build_id, prov_id);
>   }
>   
> +static int aqr107_config_mdi(struct phy_device *phydev)
> +{
> +	struct device_node *np = phydev->mdio.dev.of_node;
> +	u32 mdi_conf;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "marvell,mdi-cfg-order", &mdi_conf);
> +
> +	/* Do nothing in case property "marvell,mdi-cfg-order" is not present */
> +	if (ret == -ENOENT)
> +		return 0;


This change is breaking networking for one of our Tegra boards and on 
boot I am seeing ...

  tegra-mgbe 6800000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
  tegra-mgbe 6800000.ethernet eth0: __stmmac_open: Cannot attach to PHY
  (error: -22)

The issue is that of_property_read_u32() does not return -ENOENT if the 
property is missing, it actually returns -EINVAL. See the description of 
of_property_read_variable_u32_array() which is called by 
of_property_read_u32().

Andrew, can we drop this change from -next until this is fixed?

Thanks!
Jon

-- 
nvpublic

Re: [PATCH net-next v3 2/2] net: phy: aquantia: allow forcing order of MDI pairs

[Jon Hunter]

On 11/10/2024 18:37, Jon Hunter wrote:
> Hi Daniel,
> 
> On 04/10/2024 17:18, Daniel Golle wrote:
>> Despite supporting Auto MDI-X, it looks like Aquantia only supports
>> swapping pair (1,2) with pair (3,6) like it used to be for MDI-X on
>> 100MBit/s networks.
>>
>> When all 4 pairs are in use (for 1000MBit/s or faster) the link does not
>> come up with pair order is not configured correctly, either using
>> MDI_CFG pin or using the "PMA Receive Reserved Vendor Provisioning 1"
>> register.
>>
>> Normally, the order of MDI pairs being either ABCD or DCBA is configured
>> by pulling the MDI_CFG pin.
>>
>> However, some hardware designs require overriding the value configured
>> by that bootstrap pin. The PHY allows doing that by setting a bit in
>> "PMA Receive Reserved Vendor Provisioning 1" register which allows
>> ignoring the state of the MDI_CFG pin and another bit configuring
>> whether the order of MDI pairs should be normal (ABCD) or reverse
>> (DCBA). Pair polarity is not affected and remains identical in both
>> settings.
>>
>> Introduce property "marvell,mdi-cfg-order" which allows forcing either
>> normal or reverse order of the MDI pairs from DT.
>>
>> If the property isn't present, the behavior is unchanged and MDI pair
>> order configuration is untouched (ie. either the result of MDI_CFG pin
>> pull-up/pull-down, or pair order override already configured by the
>> bootloader before Linux is started).
>>
>> Forcing normal pair order is required on the Adtran SDG-8733A Wi-Fi 7
>> residential gateway.
>>
>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>> ---
>> v3: use u32 'marvell,mdi-cfg-order' instead of two mutually exclusive
>>      properties as suggested
>> v2: add missing 'static' keyword, improve commit description
>>
>>   drivers/net/phy/aquantia/aquantia_main.c | 33 ++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/net/phy/aquantia/aquantia_main.c 
>> b/drivers/net/phy/aquantia/aquantia_main.c
>> index 4d156d406bab..dcad3fa1ddc3 100644
>> --- a/drivers/net/phy/aquantia/aquantia_main.c
>> +++ b/drivers/net/phy/aquantia/aquantia_main.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/module.h>
>>   #include <linux/delay.h>
>>   #include <linux/bitfield.h>
>> +#include <linux/of.h>
>>   #include <linux/phy.h>
>>   #include "aquantia.h"
>> @@ -71,6 +72,11 @@
>>   #define MDIO_AN_TX_VEND_INT_MASK2        0xd401
>>   #define MDIO_AN_TX_VEND_INT_MASK2_LINK        BIT(0)
>> +#define PMAPMD_RSVD_VEND_PROV            0xe400
>> +#define PMAPMD_RSVD_VEND_PROV_MDI_CONF        GENMASK(1, 0)
>> +#define PMAPMD_RSVD_VEND_PROV_MDI_REVERSE    BIT(0)
>> +#define PMAPMD_RSVD_VEND_PROV_MDI_FORCE        BIT(1)
>> +
>>   #define MDIO_AN_RX_LP_STAT1            0xe820
>>   #define MDIO_AN_RX_LP_STAT1_1000BASET_FULL    BIT(15)
>>   #define MDIO_AN_RX_LP_STAT1_1000BASET_HALF    BIT(14)
>> @@ -485,6 +491,29 @@ static void aqr107_chip_info(struct phy_device 
>> *phydev)
>>              fw_major, fw_minor, build_id, prov_id);
>>   }
>> +static int aqr107_config_mdi(struct phy_device *phydev)
>> +{
>> +    struct device_node *np = phydev->mdio.dev.of_node;
>> +    u32 mdi_conf;
>> +    int ret;
>> +
>> +    ret = of_property_read_u32(np, "marvell,mdi-cfg-order", &mdi_conf);
>> +
>> +    /* Do nothing in case property "marvell,mdi-cfg-order" is not 
>> present */
>> +    if (ret == -ENOENT)
>> +        return 0;
> 
> 
> This change is breaking networking for one of our Tegra boards and on 
> boot I am seeing ...
> 
>   tegra-mgbe 6800000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
>   tegra-mgbe 6800000.ethernet eth0: __stmmac_open: Cannot attach to PHY
>   (error: -22)
> 
> The issue is that of_property_read_u32() does not return -ENOENT if the 
> property is missing, it actually returns -EINVAL. See the description of 
> of_property_read_variable_u32_array() which is called by 
> of_property_read_u32().
> 
> Andrew, can we drop this change from -next until this is fixed?

Sorry, I believe Jakub applied and not Andrew.

Jon

-- 
nvpublic

Re: [PATCH net-next v3 2/2] net: phy: aquantia: allow forcing order of MDI pairs

[Andrew Lunn]

> This change is breaking networking for one of our Tegra boards and on boot I
> am seeing ...
> 
>  tegra-mgbe 6800000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
>  tegra-mgbe 6800000.ethernet eth0: __stmmac_open: Cannot attach to PHY
>  (error: -22)
> 
> The issue is that of_property_read_u32() does not return -ENOENT if the
> property is missing, it actually returns -EINVAL. See the description of
> of_property_read_variable_u32_array() which is called by
> of_property_read_u32().
> 
> Andrew, can we drop this change from -next until this is fixed?

If it is as simple as s/ENOENT/EINVAL we should just fix it, rather
than revert it.

	Andrew

Re: [PATCH net-next v3 2/2] net: phy: aquantia: allow forcing order of MDI pairs

[Jon Hunter]

On 11/10/2024 19:12, Andrew Lunn wrote:
>> This change is breaking networking for one of our Tegra boards and on boot I
>> am seeing ...
>>
>>   tegra-mgbe 6800000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
>>   tegra-mgbe 6800000.ethernet eth0: __stmmac_open: Cannot attach to PHY
>>   (error: -22)
>>
>> The issue is that of_property_read_u32() does not return -ENOENT if the
>> property is missing, it actually returns -EINVAL. See the description of
>> of_property_read_variable_u32_array() which is called by
>> of_property_read_u32().
>>
>> Andrew, can we drop this change from -next until this is fixed?
> 
> If it is as simple as s/ENOENT/EINVAL we should just fix it, rather
> than revert it.

That also works for me. And yes it is that simple.

Thanks
Jon

-- 
nvpublic