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

[Tim Harvey]

On Wed, May 18, 2022 at 7:41 AM Vladimir Oltean <vladimir.oltean at nxp.com> wrote:
>
> On Wed, May 11, 2022 at 05:20:03PM -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>
> > ---
> > v2: no changes
> > ---
> >  arch/arm/dts/imx6qdl-gw5904.dtsi        | 35 +++++++++++++++++
> >  board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++----------------
> >  configs/gwventana_gw5904_defconfig      |  7 ++--
> >  3 files changed, 56 insertions(+), 36 deletions(-)
> >
> > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi
> > index 286c7a9924c2..63590a2debc7 100644
> > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi
> > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi
> > @@ -219,6 +219,27 @@
> >                       compatible = "marvell,mv88e6085";
> >                       reg = <0>;
> >
> > +                     mdios {
>
> I'm thinking either "mdios" is a container node for other "mdio" nodes,
> or you have the "mdio" node directly. Are you sure you want this
> structure? We might have problems if we pass this DT as-is up to Linux.
> Whereas the support for an "mdios" subnode could be upstreamed, I think.
> The current DT bindings document says:
>
> - mdio                  : Container of PHY and devices on the switches MDIO
>                           bus.
> - mdio?         : Container of PHYs and devices on the external MDIO
>                           bus. The node must contains a compatible string of
>                           "marvell,mv88e6xxx-mdio-external"
>
> You have an "mdios" which theoretically matches "mdio?" but only
> unintendedly so, since it is the internal MDIO bus and not the external
> one.
>
> Easiest way out right now is to not introduce something which isn't
> parsed by the current Linux driver. So the internal MDIO bus would be
> named "mdio", and the external one would be named whatever, and would be
> identified by compatible string.
>
> > +                             #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>;

Vladimir,

I'm not sure I follow your suggestion correctly. I agree this is not
ideal and not at all what I wanted. The dt prior to this patch is what
exists and works in Linux but U-Boot dsa (more correctly DM_MDIO
dm_eth_phy_connect()) requires the phy-mode and phy-handle in the
ports and I'm not clear how to define the mdio's for those to point to
here.

Recall I did the same thing for gw7901 in commit 1cb87b929e1e ("arm:
dts: imx8mm-venice-gw7901.dts: fix dsa switch configuration") which I
didn't feel was correct either as the previous dt there is what is in
Linux and works.

Can you give me an example?

Best Regards,

Tim