Re: [PATCH net-next 03/12] dt-bindings: net: pcs: add bindings for Renesas RZ/N1 MII converter

["Clément Léger" <clement.leger@bootlin.com>]

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

> > +  clocks:
> > +    items:
> > +      - description: MII reference clock
> > +      - description: RGMII reference clock
> > +      - description: RMII reference clock
> > +      - description: AHB clock used for the MII converter register interface
> > +
> > +  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.  
> 
> Describe possible values here as constraints. At present, I don't see 
> the point of this property if there is only 1 possible value and it is 
> 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 = "renesas,rzn1-miic";

  mii_conv1: mii-conv-1 {
    renesas,miic-input = <MIIC_GMAC1_PORT>;
    port = <1>;
  };
  mii_conv2: mii-conv-2 {
    renesas,miic-input = <MIIC_SWITCHD_PORT>;
    port = <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.

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

    ports {
        port@0 {
            reg = <0>;
            label = "lan0";
            phy-handle = <&switch0phy3>;
            pcs-handle = <&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 = <&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. 

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-group-users-manual-system-introduction-multiplexing-electrical-and
-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com