Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Tue, 19 Apr 2022 17:00:44 +0200
From:   =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <clement.leger@bootlin.com>
To:     Rob Herring <robh@kernel.org>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Magnus Damm <magnus.damm@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        Herve Codina <herve.codina@bootlin.com>,
        =?UTF-8?B?TWlxdcOobA==?= Raynal <miquel.raynal@bootlin.com>,
        Milan Stevanovic <milan.stevanovic@se.com>,
        Jimmy Lalande <jimmy.lalande@se.com>,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-renesas-soc@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH net-next 03/12] dt-bindings: net: pcs: add bindings for
 Renesas RZ/N1 MII converter
Message-ID: <20220419170044.450050ca@fixe.home>
In-Reply-To: <Yl68k22fUw7uBgV9@robh.at.kernel.org>
References: <20220414122250.158113-1-clement.leger@bootlin.com>
        <20220414122250.158113-4-clement.leger@bootlin.com>
        <Yl68k22fUw7uBgV9@robh.at.kernel.org>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Le Tue, 19 Apr 2022 08:43:47 -0500,
Rob Herring <robh@kernel.org> a =C3=A9crit :

> > +  clocks:
> > +    items:
> > +      - description: MII reference clock
> > +      - description: RGMII reference clock
> > +      - description: RMII reference clock
> > +      - description: AHB clock used for the MII converter register int=
erface
> > +
> > +  renesas,miic-cfg-mode:
> > +    description: MII mux configuration mode. This value should use one=
 of the
> > +                 value defined in dt-bindings/net/pcs-rzn1-miic.h. =20
>=20
> Describe possible values here as constraints. At present, I don't see=20
> the point of this property if there is only 1 possible value and it is=20
> required.

The ethernet subsystem contains a number of internal muxes that allows
to configure ethernet routing. This configuration option allows to set
the register that configure these muxes.

After talking with Andrew, I considered moving to something like this:

eth-miic@44030000 {
  compatible =3D "renesas,rzn1-miic";

  mii_conv1: mii-conv-1 {
    renesas,miic-input =3D <MIIC_GMAC1_PORT>;
    port =3D <1>;
  };
  mii_conv2: mii-conv-2 {
    renesas,miic-input =3D <MIIC_SWITCHD_PORT>;
    port =3D <2>;
  };
   ...
};

Which would allow embedding the configuration inside the port
sub-nodes. Moreover, it allows a better validation of the values using
the schema validation directly since only a limited number of values
are allowed for each port.

>=20
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > + =20
> > +patternProperties:
> > +  "^mii-conv@[0-4]$":
> > +    type: object =20
>=20
>        additionalProperties: false
>=20
> > +    description: MII converter port
> > +
> > +    properties:
> > +      reg:
> > +        maxItems: 1 =20
>=20
> Why do you need sub-nodes? They don't have any properties. A simple mask=
=20
> property could tell you which ports are present/active/enabled if that's=
=20
> what you are tracking. Or the SoC specific compatibles you need to add=20
> can imply the ports if they are SoC specific.

The MACs are using phandles to these sub-nodes to query a specific MII
converter port PCS:

switch@44050000 {
    compatible =3D "renesas,rzn1-a5psw";

    ports {
        port@0 {
            reg =3D <0>;
            label =3D "lan0";
            phy-handle =3D <&switch0phy3>;
            pcs-handle =3D <&mii_conv4>;
        };
    };
};

According to Andrew, this is not a good idea to represent the PCS as a
bus since it is indeed not a bus. I could also switch to something like
pcs-handle =3D <&eth_mii 4> but i'm not sure what you'd prefer. We could
also remove this from the device-tree and consider each driver to
request the MII ouput to be configured using something like this for
instance:

 miic_request_pcs(pcs_np, miic_port_nr, MIIC_SWITCHD_PORT);

But I'm not really fan of this because it requires the drivers to
assume some specificities of the MII converter (port number are not in
the same order of the switch for instance) and thus I would prefer this
to be in the device-tree.

Let me know if you can think of something that would suit you
better but  keep in mind that I need to correctly match a switch/MAC
port with a PCS port and that I also need to configure MII internal
muxes.=20

For more information, you can look at section 8 of the manual at [1].

Thanks,

[1]
https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-gr=
oup-users-manual-system-introduction-multiplexing-electrical-and
--=20
Cl=C3=A9ment L=C3=A9ger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com