Re: [PATCH v5 8/8] board: gw_ventana: enable MV88E61XX DSA support

On Tue, Oct 04, 2022 at 09:49:18AM -0700, Tim Harvey wrote:

Add MV88E61XX DSA support:

• update dt: U-Boot dsa driver requires different device-tree syntax than the linux driver in order to link the dsa ports to the mdio bus.

Not really correct. Better said, U-Boot requires a more restrictive subset of what DT bindings Linux supports, for sake of simplicity of the code. It is equally valid to link ports to their internal PHY by phy-handle in the Linux mv88e6xxx driver.

• update defconfig
• replace mv88e61xx_hw_reset weak override with board_phy_config support for mv88e61xx configuration that is outside the scope of the DSA driver

Note that the dt changes were not placed in a u-boot.dtsi as it is intended that these changes get merged with the Linux dt's.

Signed-off-by: Tim Harvey tharvey@gateworks.com Reviewed-by: Fabio Estevam festevam@denx.de

---

v5:

• fix typo in defconfig s/MV88E61XX/MV88E6XXX
• added Fabio's rb tag
• added note to commit log

v4:

• use standard Linux internal MDIO dt structure
• use PHY_FIXED_ID define

v3:

• move mdio's mdio@0 subnode

v2: no changes

arch/arm/dts/imx6qdl-gw5904.dtsi | 31 +++++++++++++++ board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- configs/gwventana_gw5904_defconfig | 7 ++-- drivers/net/mv88e6xxx.c | 29 ++++++-------- 4 files changed, 64 insertions(+), 53 deletions(-)

diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi index 612b6e068e28..487dd7648274 100644 --- a/arch/arm/dts/imx6qdl-gw5904.dtsi +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi @@ -212,6 +212,27 @@ compatible = "marvell,mv88e6085"; reg = <0>;

• 	mdio {
  

• 		#address-cells = <1>;
  

• 		#size-cells = <0>;
  

• 		sw_phy0: ethernet-phy@0 {
  

• 			reg = <0x0>;
  

• 		};
  

• 		sw_phy1: ethernet-phy@1 {
  

• 			reg = <0x1>;
  

• 		};
  

• 		sw_phy2: ethernet-phy@2 {
  

• 			reg = <0x2>;
  

• 		};
  

• 		sw_phy3: ethernet-phy@3 {
  

• 			reg = <0x3>;
  

• 		};
  

• 	};
  

• ports {
  	#address-cells = <1>;
  	#size-cells = <0>;
  

@@ -219,27 +240,37 @@ port@0 { reg = <0>; label = "lan4";

• 			phy-handle = <&sw_phy0>;
  	};
  
  	port@1 {
  		reg = <1>;
  		label = "lan3";
  

• 			phy-handle = <&sw_phy1>;
  	};
  
  	port@2 {
  		reg = <2>;
  		label = "lan2";
  

• 			phy-handle = <&sw_phy2>;
  	};
  
  	port@3 {
  		reg = <3>;
  		label = "lan1";
  

• 			phy-handle = <&sw_phy3>;
  	};
  
  	port@5 {
  		reg = <5>;
  		label = "cpu";
  

This is not used, please remove it.

			ethernet = <&fec>;

• 			phy-mode = "rgmii-id";
  

• 			fixed-link {
  

• 				speed = <1000>;
  

• 				full-duplex;
  

• 			};
  	};
  };
  

  };

Binding looks good, thanks.

diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c index 99f52b9953e2..1c3da2c82707 100644 --- a/board/gateworks/gw_ventana/gw_ventana.c +++ b/board/gateworks/gw_ventana/gw_ventana.c @@ -83,44 +83,30 @@ int board_phy_config(struct phy_device *phydev) break; }

• /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */
• if (phydev->phy_id == PHY_FIXED_ID &&

•    (board_type == GW5904 || board_type == GW5909)) {
  

• struct mii_dev *bus = miiphy_get_dev_by_name("mdio");
  

• puts("MV88E61XX ");
  

• /* GPIO[0] output CLK125 for RGMII_REFCLK */
  

• bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe);
  

• bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7);
  

• /* Port 0-3 LED configuration: Table 80/82 */
  

• /* LED configuration: 7:4-green (8=Activity)  3:0 amber (8=Link) */
  

• bus->write(bus, 0x10, 0, 0x16, 0x8088);
  

• bus->write(bus, 0x11, 0, 0x16, 0x8088);
  

• bus->write(bus, 0x12, 0, 0x16, 0x8088);
  

• bus->write(bus, 0x13, 0, 0x16, 0x8088);
  

• }

Ugh. I'll have to get over this.

BTW, does this run before or after the switch driver getting probed? Don't you lose the config once you reset the switch? How do you ensure ordering?

if (phydev->drv->config) phydev->drv->config(phydev);

return 0; }

-#ifdef CONFIG_MV88E61XX_SWITCH -int mv88e61xx_hw_reset(struct phy_device *phydev) -{

• struct mii_dev *bus = phydev->bus;
• /* GPIO[0] output, CLK125 */
• debug("enabling RGMII_REFCLK\n");
• bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0,

•    0x1a /*MV_SCRATCH_MISC*/,
  

•    (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe);
  

• bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0,

•    0x1a /*MV_SCRATCH_MISC*/,
  

•    (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7);
  

• /* RGMII delay - Physical Control register bit[15:14] */
• debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT);
• /* forced 1000mbps full-duplex link */
• bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe);
• phydev->autoneg = AUTONEG_DISABLE;
• phydev->speed = SPEED_1000;
• phydev->duplex = DUPLEX_FULL;
• /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */
• bus->write(bus, 0x10, 0, 0x16, 0x8088);
• bus->write(bus, 0x11, 0, 0x16, 0x8088);
• bus->write(bus, 0x12, 0, 0x16, 0x8088);
• bus->write(bus, 0x13, 0, 0x16, 0x8088);
• return 0;

-} -#endif // CONFIG_MV88E61XX_SWITCH

#if defined(CONFIG_VIDEO_IPUV3) static void enable_hdmi(struct display_info_t const *dev) { diff --git a/configs/gwventana_gw5904_defconfig b/configs/gwventana_gw5904_defconfig index fb5870fa5803..50115b21e263 100644 --- a/configs/gwventana_gw5904_defconfig +++ b/configs/gwventana_gw5904_defconfig @@ -109,13 +109,12 @@ CONFIG_SUPPORT_EMMC_BOOT=y CONFIG_FSL_USDHC=y CONFIG_MTD=y CONFIG_PHYLIB=y -CONFIG_MV88E61XX_SWITCH=y -CONFIG_MV88E61XX_CPU_PORT=5 -CONFIG_MV88E61XX_PHY_PORTS=0xf -CONFIG_MV88E61XX_FIXED_PORTS=0x0 +CONFIG_PHY_FIXED=y CONFIG_DM_MDIO=y +CONFIG_DM_DSA=y CONFIG_E1000=y CONFIG_FEC_MXC=y +CONFIG_MV88E6XXX=y CONFIG_MII=y CONFIG_PCI=y CONFIG_PCIE_IMX=y diff --git a/drivers/net/mv88e6xxx.c b/drivers/net/mv88e6xxx.c index e59e2464c641..09ce6dd1142d 100644 --- a/drivers/net/mv88e6xxx.c +++ b/drivers/net/mv88e6xxx.c @@ -728,31 +728,26 @@ static const struct dsa_ops mv88e6xxx_dsa_ops = { static int mv88e6xxx_probe_mdio(struct udevice *dev) { struct udevice *mdev;

• ofnode node, mdios; const char *name;

• ofnode node; int ret;

• /* bind phy ports of mdios child node to mv88e6xxx_mdio device */
• mdios = dev_read_subnode(dev, "mdios");
• if (!ofnode_valid(mdios))

• /* bind phy ports of mdio child node to mv88e6xxx_mdio device */
• node = dev_read_subnode(dev, "mdio");
• if (!ofnode_valid(node)) return 0;

• ofnode_for_each_subnode(node, mdios) {

• name = ofnode_get_name(node);
  

• ret = device_bind_driver_to_node(dev,
  

• 				 "mv88e6xxx_mdio",
  

• 				 name, node, NULL);
  

• if (ret) {
  

• 	dev_err(dev, "failed to bind %s: %d\n", name, ret);
  

• 	continue;
  

• }
  

• name = ofnode_get_name(node);
• ret = device_bind_driver_to_node(dev,

• 			 "mv88e6xxx_mdio",
  

• 			 name, node, NULL);
  

• if (ret) {

• dev_err(dev, "failed to bind %s: %d\n", name, ret);
  

• } else { /* need to probe it as there is no compatible to do so */ ret = uclass_get_device_by_ofnode(UCLASS_MDIO, node, &mdev);

• if (ret) {
  

• if (ret)
  dev_err(dev, "failed to probe %s: %d\n", name, ret);
  

• 	continue;
  

• }
  

  }

  return ret;

Please move this hunk into patch 7/8.

-- 2.25.1