[PATCH v5 8/8] board: gw_ventana: enable MV88E61XX DSA support
Tim Harvey
tharvey at gateworks.com
Wed Oct 5 19:16:55 CEST 2022
On Tue, Oct 4, 2022 at 4:58 PM Vladimir Oltean <vladimir.oltean at nxp.com> wrote:
>
> 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.
Agreed - I will update the commit message now that I understand the
MDIO bindings correctly.
>
> > - 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 at gateworks.com>
> > Reviewed-by: Fabio Estevam <festevam at 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 at 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 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>;
> > @@ -219,27 +240,37 @@
> > port at 0 {
> > reg = <0>;
> > label = "lan4";
> > + phy-handle = <&sw_phy0>;
> > };
> >
> > port at 1 {
> > reg = <1>;
> > label = "lan3";
> > + phy-handle = <&sw_phy1>;
> > };
> >
> > port at 2 {
> > reg = <2>;
> > label = "lan2";
> > + phy-handle = <&sw_phy2>;
> > };
> >
> > port at 3 {
> > reg = <3>;
> > label = "lan1";
> > + phy-handle = <&sw_phy3>;
> > };
> >
> > port at 5 {
> > reg = <5>;
> > label = "cpu";
>
> This is not used, please remove it.
ok - will remove
>
> > ethernet = <&fec>;
> > + phy-mode = "rgmii-id";
> > +
> > + fixed-link {
> > + speed = <1000>;
> > + full-duplex;
> > + };
> > };
> > };
> > };
>
> Binding looks good, thanks.
Great! Thanks for working through this with me. I will submit that
hunk to the Linux dt as well.
>
> > 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?
>
Yes, I know you don't like this code and I do plan on submitting a
follow-up patch that adds some defines and/or functions that can be
used to make it at least more readable. There are currently around 70
board files that use board_phy_config() to perform direct PHY register
writes for gpio/led/pin config for things that are currently not
defined well enough via bindings to do in drivers and they all read
more or less like the above.
In all the NIC drivers I've looked at the driver probe function
performs a phy-reset followed by binding internal mdios, then calls
phy_config() at the end of the probe which hooks to the
board_phy_config() function that has always been there for per-board
obscure register config like this.
> > if (phydev->drv->config)
> > phydev->drv->config(phydev);
> >
> > return 0;
> > }
> >
<snip>
> > 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.
Yes, I apologize for this - I squashed the above into this
board-specific patch instead of the mv88e6xxx driver patch which
caused you to waste some time while reviwing the driver patch.
Best Regards,
Tim
More information about the U-Boot
mailing list