On Fri, Jun 24, 2022 at 3:25 AM Vladimir Oltean <vladimir.olt...@nxp.com> wrote: > > On Mon, May 23, 2022 at 11:25:49AM -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. > > - 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 > > > > Signed-off-by: Tim Harvey <thar...@gateworks.com> > > --- > > v3: > > - move mdio's mdio@0 subnode > > v2: no changes > > --- > > arch/arm/dts/imx6qdl-gw5904.dtsi | 41 ++++++++++++++++++++ > > board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- > > configs/gwventana_gw5904_defconfig | 7 ++-- > > 3 files changed, 62 insertions(+), 36 deletions(-) > > > > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi > > b/arch/arm/dts/imx6qdl-gw5904.dtsi > > index 286c7a9924c2..1b2f70d1ccb2 100644 > > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi > > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi > > @@ -219,6 +219,33 @@ > > compatible = "marvell,mv88e6085"; > > reg = <0>; > > > > + mdios { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + mdio@0 { > > If you are going to follow this new model with a dedicated "mdios" subnode, > I've consulted with Andrew Lunn and Florian Fainelli and there shouldn't > be a problem to later make Linux accept this alternate binding format. > But in that case, please match this OF node by a dedicated compatible > string, like "marvell,mv88e6xxx-mdio-internal", so that there will be a > way to differentiate this from the existing "marvell,mv88e6xxx-mdio-external" > when support for that is added in U-Boot. > > Alternatively, to repeat myself, you can always follow the de-facto > bindings for Linux mv88e6xxx which have: > > switch0: switch0@0 { > compatible = "marvell,mv88e6190"; > > ports { > #address-cells = <1>; > #size-cells = <0>; > > ... > }; > > mdio { // internal > #address-cells = <1>; > #size-cells = <0>; > > ... > }; > > mdio1 { > compatible = > "marvell,mv88e6xxx-mdio-external"; > #address-cells = <1>; > #size-cells = <0>; > > ... > }; > }; >
Documentation/devicetree/bindings/net/dsa/marvell.txt shows en example with just one child node under the internal mdio node: mdio { #address-cells = <1>; #size-cells = <0>; switch1phy0: switch1phy0@0 { reg = <0>; interrupt-parent = <&switch0>; interrupts = <0 IRQ_TYPE_LEVEL_HIGH>; }; }; Am I to assume I can add additional nodes there for the other ports such as the following? mdio { #address-cells = <1>; #size-cells = <0>; switch1phy0: switch1phy0@0 { reg = <0>; }; switch1phy1: switch1phy1@1 { reg = <1>; }; switch1phy2: switch1phy2@2 { reg = <2>; }; switch1phy3: switch1phy3@3 { reg = <3>; }; ... }; Best Regards, Tim > and the associated parsing code: > > static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip, > struct device_node *np) > { > struct device_node *child; > int err; > > /* Always register one mdio bus for the internal/default mdio > * bus. This maybe represented in the device tree, but is > * optional. > */ > child = of_get_child_by_name(np, "mdio"); > err = mv88e6xxx_mdio_register(chip, child, false); > of_node_put(child); > if (err) > return err; > > /* Walk the device tree, and see if there are any other nodes > * which say they are compatible with the external mdio > * bus. > */ > for_each_available_child_of_node(np, child) { > if (of_device_is_compatible( > child, "marvell,mv88e6xxx-mdio-external")) { > err = mv88e6xxx_mdio_register(chip, child, true); > if (err) { > mv88e6xxx_mdios_unregister(chip); > of_node_put(child); > return err; > } > } > } > > return 0; > } > > Personally I believe that if you have limited amount of time to spend on > U-Boot DM_DSA and DT bindings in general, you should just follow the > format already accepted by Linux ("mdio" node is for internal MDIO, > doesn't have compatible string, is placed directly under "switch" node", > while external MDIO is matched by compatible string and its node can > have any name). > > What we should try to accomplish is that the DT blob that U-Boot creates > for itself here to be coherent enough that Linux is able to understand > and use it, in case we decide to pass it to the operating system. With > your approach you'd have work to do in Linux as well to accept the > "mdios" subnode and parse controllers by compatible string inside, and > I'm not exactly sure you're willing to do that. > > > + reg = <0>; > > + #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>; > > @@ -226,27 +253,41 @@ > > port@0 { > > reg = <0>; > > label = "lan4"; > > + phy-mode = "internal"; > > + phy-handle = <&sw_phy0>; > > }; > > > > port@1 { > > reg = <1>; > > label = "lan3"; > > + phy-mode = "internal"; > > + phy-handle = <&sw_phy1>; > > }; > > > > port@2 { > > reg = <2>; > > label = "lan2"; > > + phy-mode = "internal"; > > + phy-handle = <&sw_phy2>; > > }; > > > > port@3 { > > reg = <3>; > > label = "lan1"; > > + phy-mode = "internal"; > > + phy-handle = <&sw_phy3>; > > }; > > > > port@5 { > > reg = <5>; > > label = "cpu"; > > ethernet = <&fec>; > > + phy-mode = "rgmii-id"; > > + > > + fixed-link { > > + speed = <1000>; > > + full-duplex; > > + }; > > }; > > }; > > }; > > diff --git a/board/gateworks/gw_ventana/gw_ventana.c > > b/board/gateworks/gw_ventana/gw_ventana.c > > index c06630a66b66..bef3f7ef0d2b 100644 > > --- a/board/gateworks/gw_ventana/gw_ventana.c > > +++ b/board/gateworks/gw_ventana/gw_ventana.c > > @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev) > > phy_write(phydev, MDIO_DEVAD_NONE, 14, val); > > } > > > > + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */ > > + else if (phydev->phy_id == 0xa5a55a5a && > > + ((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); > > + } > > + > > 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 d25a8324b1df..9809bc691508 100644 > > --- a/configs/gwventana_gw5904_defconfig > > +++ b/configs/gwventana_gw5904_defconfig > > @@ -103,12 +103,11 @@ 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_MV88E61XX=y > > +CONFIG_PHY_FIXED=y > > CONFIG_DM_ETH=y > > CONFIG_DM_MDIO=y > > +CONFIG_DM_DSA=y > > CONFIG_E1000=y > > CONFIG_FEC_MXC=y > > CONFIG_MII=y > > -- > > 2.17.1 > >