[PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support
Tim Harvey
tharvey at gateworks.com
Sat Jun 25 01:16:34 CEST 2022
On Fri, Jun 24, 2022 at 3:25 AM Vladimir Oltean <vladimir.oltean at 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 <tharvey at gateworks.com>
> > ---
> > v3:
> > - move mdio's mdio at 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 at 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 at 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 at 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 at 0 {
reg = <0>;
};
switch1phy1: switch1phy1 at 1 {
reg = <1>;
};
switch1phy2: switch1phy2 at 2 {
reg = <2>;
};
switch1phy3: switch1phy3 at 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 at 0 {
> > + reg = <0x0>;
> > + };
> > +
> > + sw_phy1: ethernet-phy at 1 {
> > + reg = <0x1>;
> > + };
> > +
> > + sw_phy2: ethernet-phy at 2 {
> > + reg = <0x2>;
> > + };
> > +
> > + sw_phy3: ethernet-phy at 3 {
> > + reg = <0x3>;
> > + };
> > + };
> > + };
> > +
> > ports {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > @@ -226,27 +253,41 @@
> > port at 0 {
> > reg = <0>;
> > label = "lan4";
> > + phy-mode = "internal";
> > + phy-handle = <&sw_phy0>;
> > };
> >
> > port at 1 {
> > reg = <1>;
> > label = "lan3";
> > + phy-mode = "internal";
> > + phy-handle = <&sw_phy1>;
> > };
> >
> > port at 2 {
> > reg = <2>;
> > label = "lan2";
> > + phy-mode = "internal";
> > + phy-handle = <&sw_phy2>;
> > };
> >
> > port at 3 {
> > reg = <3>;
> > label = "lan1";
> > + phy-mode = "internal";
> > + phy-handle = <&sw_phy3>;
> > };
> >
> > port at 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
> >
More information about the U-Boot
mailing list