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

[Vladimir Oltean]

Hi Tim,

On Wed, May 18, 2022 at 09:15:26AM -0700, Tim Harvey wrote:
> 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?

My considerations are the following.

The Linux Documentation/devicetree/bindings/net/mdio.yaml file says that
the following are valid node names for an MDIO controller:

properties:
  $nodename:
    pattern: "^mdio(@.*)?"

In simple terms, "mdio" or "mdio at something".

When you put an "mdio" controller on the same level as the "ports" node,
you cannot use the "mdio at something" syntax, because the #address-cells
and #size-cells must be 0 (otherwise said, if you say "mdio at something",
you are also forced to say "ports at something").

This is not a problem if there is a single MDIO controller, because you
can just use "mdio" and call it a day.

But when you have multiple MDIO controllers, naming the second one
becomes problematic.

The Linux mv88e6xxx driver expects secondary (external) MDIO controllers
to be named whatever (mdio1 for example) and just looks at their compatible
strings. This is fine, but the "mdio1" name is neither "mdio" nor
"mdio at something". So mdio.yaml won't really validate these nodes.

This is why some other drivers have an "mdios" container node on the
same level as "ports", and make "mdios" have #address-cells of 1, so
that children of "mdios" can be "mdio at something", and all children will
be validated by mdio.yaml.

The mv88e6xxx driver does not support parsing an "mdios" container node,
but code for that could be added, if we ever wanted to improve the
bindings for this switch.

All that I'm saying is that your approach is the weird mix where you are
neither benefitting from having a container node for MDIO controllers,
nor complying to mdio.yaml, nor following what the Linux driver expects.
You are free to make a choice whether to simply follow the non-compliant
Linux way of having "mdio", "mdio1", etc, or to introduce a proper
"mdios" container node here _and_ in Linux. But what you've proposed
currently doesn't really make sense.