| Message ID | ed46220cc4c52d630fc481c8148fc749242c368d.1724244281.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,1/2] dt-bindings: net: marvell,aquantia: add properties to override MDI_CFG | expand |
On Wed, Aug 21, 2024 at 01:46:50PM +0100, Daniel Golle wrote: > Normally, the MDI reversal configuration is taken from 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 which > allows ignoring the state of the MDI_CFG pin and configuring whether > the order of MDI pairs should be normal (ABCD) or reverse (DCBA). > > Introduce two boolean properties which allow forcing either normal or > reverse order of the MDI pairs from DT. How does this interact with ethtool -s eth42 [mdix auto|on|off] In general, you want mdix auto, so the two ends figure out how the cable is wired and so it just works. Andrew
On Wed, Aug 21, 2024 at 06:07:06PM +0200, Andrew Lunn wrote: > On Wed, Aug 21, 2024 at 01:46:50PM +0100, Daniel Golle wrote: > > Normally, the MDI reversal configuration is taken from 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 which > > allows ignoring the state of the MDI_CFG pin and configuring whether > > the order of MDI pairs should be normal (ABCD) or reverse (DCBA). > > > > Introduce two boolean properties which allow forcing either normal or > > reverse order of the MDI pairs from DT. > > How does this interact with ethtool -s eth42 [mdix auto|on|off] > > In general, you want mdix auto, so the two ends figure out how the > cable is wired and so it just works. 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. And yes, I did verify that Auto MDI-X is enabled in the "Autonegotiation Reserved Vendor Provisioning 1" register.
Hi Daniel, kernel test robot noticed the following build warnings: [auto build test WARNING on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Daniel-Golle/net-phy-aquantia-allow-forcing-order-of-MDI-pairs/20240821-210717 base: net-next/main patch link: https://lore.kernel.org/r/ed46220cc4c52d630fc481c8148fc749242c368d.1724244281.git.daniel%40makrotopia.org patch subject: [PATCH net-next 2/2] net: phy: aquantia: allow forcing order of MDI pairs config: x86_64-randconfig-123-20240822 (https://download.01.org/0day-ci/archive/20240822/202408221406.WtGcNGxX-lkp@intel.com/config) compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240822/202408221406.WtGcNGxX-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408221406.WtGcNGxX-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/net/phy/aquantia/aquantia_main.c:483:5: sparse: sparse: symbol 'aqr107_config_mdi' was not declared. Should it be static? drivers/net/phy/aquantia/aquantia_main.c: note: in included file (through include/linux/mmzone.h, include/linux/gfp.h, include/linux/umh.h, include/linux/kmod.h, ...): include/linux/page-flags.h:235:46: sparse: sparse: self-comparison always evaluates to false include/linux/page-flags.h:235:46: sparse: sparse: self-comparison always evaluates to false vim +/aqr107_config_mdi +483 drivers/net/phy/aquantia/aquantia_main.c 482 > 483 int aqr107_config_mdi(struct phy_device *phydev) 484 { 485 struct device_node *np = phydev->mdio.dev.of_node; 486 bool force_normal, force_reverse; 487 488 force_normal = of_property_read_bool(np, "marvell,force-mdi-order-normal"); 489 force_reverse = of_property_read_bool(np, "marvell,force-mdi-order-reverse"); 490 491 if (force_normal && force_reverse) 492 return -EINVAL; 493 494 if (force_normal) 495 return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, PMAPMD_RSVD_VEND_PROV, 496 PMAPMD_RSVD_VEND_PROV_MDI_CONF, 497 PMAPMD_RSVD_VEND_PROV_MDI_FORCE); 498 499 if (force_reverse) 500 return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, PMAPMD_RSVD_VEND_PROV, 501 PMAPMD_RSVD_VEND_PROV_MDI_CONF, 502 PMAPMD_RSVD_VEND_PROV_MDI_REVERSE | 503 PMAPMD_RSVD_VEND_PROV_MDI_FORCE); 504 505 return 0; 506 } 507
Hi Daniel, kernel test robot noticed the following build warnings: [auto build test WARNING on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Daniel-Golle/net-phy-aquantia-allow-forcing-order-of-MDI-pairs/20240821-210717 base: net-next/main patch link: https://lore.kernel.org/r/ed46220cc4c52d630fc481c8148fc749242c368d.1724244281.git.daniel%40makrotopia.org patch subject: [PATCH net-next 2/2] net: phy: aquantia: allow forcing order of MDI pairs config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240822/202408221537.gmrL3l3n-lkp@intel.com/config) compiler: alpha-linux-gcc (GCC) 13.3.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240822/202408221537.gmrL3l3n-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408221537.gmrL3l3n-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/net/phy/aquantia/aquantia_main.c:483:5: warning: no previous prototype for 'aqr107_config_mdi' [-Wmissing-prototypes] 483 | int aqr107_config_mdi(struct phy_device *phydev) | ^~~~~~~~~~~~~~~~~ vim +/aqr107_config_mdi +483 drivers/net/phy/aquantia/aquantia_main.c 482 > 483 int aqr107_config_mdi(struct phy_device *phydev) 484 { 485 struct device_node *np = phydev->mdio.dev.of_node; 486 bool force_normal, force_reverse; 487 488 force_normal = of_property_read_bool(np, "marvell,force-mdi-order-normal"); 489 force_reverse = of_property_read_bool(np, "marvell,force-mdi-order-reverse"); 490 491 if (force_normal && force_reverse) 492 return -EINVAL; 493 494 if (force_normal) 495 return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, PMAPMD_RSVD_VEND_PROV, 496 PMAPMD_RSVD_VEND_PROV_MDI_CONF, 497 PMAPMD_RSVD_VEND_PROV_MDI_FORCE); 498 499 if (force_reverse) 500 return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, PMAPMD_RSVD_VEND_PROV, 501 PMAPMD_RSVD_VEND_PROV_MDI_CONF, 502 PMAPMD_RSVD_VEND_PROV_MDI_REVERSE | 503 PMAPMD_RSVD_VEND_PROV_MDI_FORCE); 504 505 return 0; 506 } 507
Hi Daniel,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Daniel-Golle/net-phy-aquantia-allow-forcing-order-of-MDI-pairs/20240821-210717
base: net-next/main
patch link: https://lore.kernel.org/r/ed46220cc4c52d630fc481c8148fc749242c368d.1724244281.git.daniel%40makrotopia.org
patch subject: [PATCH net-next 2/2] net: phy: aquantia: allow forcing order of MDI pairs
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240822/202408221523.6Cc0ADf9-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 26670e7fa4f032a019d23d56c6a02926e854e8af)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240822/202408221523.6Cc0ADf9-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408221523.6Cc0ADf9-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/net/phy/aquantia/aquantia_main.c:15:
In file included from include/linux/phy.h:16:
In file included from include/linux/ethtool.h:18:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:10:
In file included from include/linux/mm.h:2228:
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
In file included from drivers/net/phy/aquantia/aquantia_main.c:15:
In file included from include/linux/phy.h:16:
In file included from include/linux/ethtool.h:18:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/net/phy/aquantia/aquantia_main.c:15:
In file included from include/linux/phy.h:16:
In file included from include/linux/ethtool.h:18:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/net/phy/aquantia/aquantia_main.c:15:
In file included from include/linux/phy.h:16:
In file included from include/linux/ethtool.h:18:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
>> drivers/net/phy/aquantia/aquantia_main.c:483:5: warning: no previous prototype for function 'aqr107_config_mdi' [-Wmissing-prototypes]
483 | int aqr107_config_mdi(struct phy_device *phydev)
| ^
drivers/net/phy/aquantia/aquantia_main.c:483:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
483 | int aqr107_config_mdi(struct phy_device *phydev)
| ^
| static
8 warnings generated.
vim +/aqr107_config_mdi +483 drivers/net/phy/aquantia/aquantia_main.c
482
> 483 int aqr107_config_mdi(struct phy_device *phydev)
484 {
485 struct device_node *np = phydev->mdio.dev.of_node;
486 bool force_normal, force_reverse;
487
488 force_normal = of_property_read_bool(np, "marvell,force-mdi-order-normal");
489 force_reverse = of_property_read_bool(np, "marvell,force-mdi-order-reverse");
490
491 if (force_normal && force_reverse)
492 return -EINVAL;
493
494 if (force_normal)
495 return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, PMAPMD_RSVD_VEND_PROV,
496 PMAPMD_RSVD_VEND_PROV_MDI_CONF,
497 PMAPMD_RSVD_VEND_PROV_MDI_FORCE);
498
499 if (force_reverse)
500 return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, PMAPMD_RSVD_VEND_PROV,
501 PMAPMD_RSVD_VEND_PROV_MDI_CONF,
502 PMAPMD_RSVD_VEND_PROV_MDI_REVERSE |
503 PMAPMD_RSVD_VEND_PROV_MDI_FORCE);
504
505 return 0;
506 }
507
On Wed, Aug 21, 2024 at 05:18:44PM +0100, Daniel Golle wrote: > On Wed, Aug 21, 2024 at 06:07:06PM +0200, Andrew Lunn wrote: > > On Wed, Aug 21, 2024 at 01:46:50PM +0100, Daniel Golle wrote: > > > Normally, the MDI reversal configuration is taken from 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 which > > > allows ignoring the state of the MDI_CFG pin and configuring whether > > > the order of MDI pairs should be normal (ABCD) or reverse (DCBA). > > > > > > Introduce two boolean properties which allow forcing either normal or > > > reverse order of the MDI pairs from DT. > > > > How does this interact with ethtool -s eth42 [mdix auto|on|off] > > > > In general, you want mdix auto, so the two ends figure out how the > > cable is wired and so it just works. > > 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. > > And yes, I did verify that Auto MDI-X is enabled in the > "Autonegotiation Reserved Vendor Provisioning 1" register. Is it possible to read the strap configuration? All DT needs to indicate is that the strap is inverted. Andrew
On Fri, Aug 23, 2024 at 03:28:39AM +0200, Andrew Lunn wrote: > On Wed, Aug 21, 2024 at 05:18:44PM +0100, Daniel Golle wrote: > > On Wed, Aug 21, 2024 at 06:07:06PM +0200, Andrew Lunn wrote: > > > On Wed, Aug 21, 2024 at 01:46:50PM +0100, Daniel Golle wrote: > > > > Normally, the MDI reversal configuration is taken from 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 which > > > > allows ignoring the state of the MDI_CFG pin and configuring whether > > > > the order of MDI pairs should be normal (ABCD) or reverse (DCBA). > > > > > > > > Introduce two boolean properties which allow forcing either normal or > > > > reverse order of the MDI pairs from DT. > > > > > > How does this interact with ethtool -s eth42 [mdix auto|on|off] > > > > > > In general, you want mdix auto, so the two ends figure out how the > > > cable is wired and so it just works. > > > > 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. > > > > And yes, I did verify that Auto MDI-X is enabled in the > > "Autonegotiation Reserved Vendor Provisioning 1" register. > > Is it possible to read the strap configuration? All DT needs to > indicate is that the strap is inverted. Sadly no, because there is only a single r/w bit which could already have been changed by the bootloader as well. We risk that vendor bootloader updates then require modifying DT again once they "fix it" there. I'd rather have two properties to force either ABCD or DCBA pair order. If it got to be a single property, then we can either use a string with pre-defined values "abcd" and "dcba", or use macro defined integer values in include/dt-bindings such as #define AQUANTIA_MII_PAIR_ORDER_ABCD 0 #define AQUANTIA_MII_PAIR_ORDER_DCBA 1 In both when using a single property for overriding the bootstrap value, absence of that property would mean to not touch what ever has been setup by bootstrap pin (or force-mode configured by the bootloader, which I've also seen already, and that also seems to survive PHY resets).
diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c index e982e9ce44a59..33a6eb55d99d4 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) @@ -474,6 +480,31 @@ static void aqr107_chip_info(struct phy_device *phydev) fw_major, fw_minor, build_id, prov_id); } +int aqr107_config_mdi(struct phy_device *phydev) +{ + struct device_node *np = phydev->mdio.dev.of_node; + bool force_normal, force_reverse; + + force_normal = of_property_read_bool(np, "marvell,force-mdi-order-normal"); + force_reverse = of_property_read_bool(np, "marvell,force-mdi-order-reverse"); + + if (force_normal && force_reverse) + return -EINVAL; + + if (force_normal) + return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, PMAPMD_RSVD_VEND_PROV, + PMAPMD_RSVD_VEND_PROV_MDI_CONF, + PMAPMD_RSVD_VEND_PROV_MDI_FORCE); + + if (force_reverse) + return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, PMAPMD_RSVD_VEND_PROV, + PMAPMD_RSVD_VEND_PROV_MDI_CONF, + PMAPMD_RSVD_VEND_PROV_MDI_REVERSE | + PMAPMD_RSVD_VEND_PROV_MDI_FORCE); + + return 0; +} + static int aqr107_config_init(struct phy_device *phydev) { struct aqr107_priv *priv = phydev->priv; @@ -503,6 +534,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, index, led_active_low);
Normally, the MDI reversal configuration is taken from 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 which allows ignoring the state of the MDI_CFG pin and configuring whether the order of MDI pairs should be normal (ABCD) or reverse (DCBA). Introduce two boolean properties which allow forcing either normal or reverse order of the MDI pairs from DT. Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- drivers/net/phy/aquantia/aquantia_main.c | 35 ++++++++++++++++++++++++ 1 file changed, 35 insertions(+)