[PATCH net 1/2] dt-bindings: net: dsa: realtek: cleanup compatible strings

[PATCH net 1/2] dt-bindings: net: dsa: realtek: cleanup compatible strings

[Luiz Angelo Daros de Luca]

Compatible strings are used to help the driver find the chip ID/version
register for each chip family. After that, the driver can setup the
switch accordingly. Keep only the first supported model for each family
as a compatible string and reference other chip models in the
description.

CC: devicetree@vger.kernel.org
Link: https://lore.kernel.org/netdev/20220414014055.m4wbmr7tdz6hsa3m@bang-olufsen.dk/
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 .../devicetree/bindings/net/dsa/realtek.yaml  | 33 +++++++------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index 8756060895a8..9bf862abb496 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -27,32 +27,23 @@ description:
   The realtek-mdio driver is an MDIO driver and it must be inserted inside
   an MDIO node.
 
+  The compatibility string is used only to find an identification register,
+  (chip ID and version) which is at a different MDIO base address in different
+  switch families. The driver then uses the chip ID/version to device how to
+  drive the switch.
+
 properties:
   compatible:
     enum:
       - realtek,rtl8365mb
-      - realtek,rtl8366
       - realtek,rtl8366rb
-      - realtek,rtl8366s
-      - realtek,rtl8367
-      - realtek,rtl8367b
-      - realtek,rtl8367rb
-      - realtek,rtl8367s
-      - realtek,rtl8368s
-      - realtek,rtl8369
-      - realtek,rtl8370
     description: |
-      realtek,rtl8365mb: 4+1 ports
-      realtek,rtl8366: 5+1 ports
-      realtek,rtl8366rb: 5+1 ports
-      realtek,rtl8366s: 5+1 ports
-      realtek,rtl8367:
-      realtek,rtl8367b:
-      realtek,rtl8367rb: 5+2 ports
-      realtek,rtl8367s: 5+2 ports
-      realtek,rtl8368s: 8 ports
-      realtek,rtl8369: 8+1 ports
-      realtek,rtl8370: 8+2 ports
+      realtek,rtl8365mb:
+        Use with models RTL8363NB, RTL8363NB-VB, RTL8363SC, RTL8363SC-VB,
+        RTL8364NB, RTL8364NB-VB, RTL8365MB, RTL8366SC, RTL8367RB-VB, RTL8367S,
+        RTL8367SB, RTL8370MB, RTL8310SR
+      realtek,rtl8367rb:
+        Use with models RTL8366RB, RTL8366S
 
   mdc-gpios:
     description: GPIO line for the MDC clock line.
@@ -335,7 +326,7 @@ examples:
             #size-cells = <0>;
 
             switch@29 {
-                    compatible = "realtek,rtl8367s";
+                    compatible = "realtek,rtl8365mb";
                     reg = <29>;
 
                     reset-gpios = <&gpio2 20 GPIO_ACTIVE_LOW>;
-- 
2.35.1

[PATCH net 2/2] net: dsa: realtek: remove realtek,rtl8367s string

[Luiz Angelo Daros de Luca]

There is no need to add new compatible strings for each new supported
chip version. The compatible string is used only to select the subdriver
(rtl8365mb.c or rtl8366rb). Once in the subdriver, it will detect the
chip model by itself, ignoring which compatible string was used.

Link: https://lore.kernel.org/netdev/20220414014055.m4wbmr7tdz6hsa3m@bang-olufsen.dk/
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/realtek-mdio.c | 1 -
 drivers/net/dsa/realtek/realtek-smi.c  | 4 ----
 2 files changed, 5 deletions(-)

diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 31e1f100e48e..c58f49d558d2 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -267,7 +267,6 @@ static const struct of_device_id realtek_mdio_of_match[] = {
 #endif
 #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB)
 	{ .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, },
-	{ .compatible = "realtek,rtl8367s", .data = &rtl8365mb_variant, },
 #endif
 	{ /* sentinel */ },
 };
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
index 6cec559c90ce..45992f79ec8d 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -551,10 +551,6 @@ static const struct of_device_id realtek_smi_of_match[] = {
 		.compatible = "realtek,rtl8365mb",
 		.data = &rtl8365mb_variant,
 	},
-	{
-		.compatible = "realtek,rtl8367s",
-		.data = &rtl8365mb_variant,
-	},
 #endif
 	{ /* sentinel */ },
 };
-- 
2.35.1

Re: [PATCH net 2/2] net: dsa: realtek: remove realtek,rtl8367s string

[Arınç ÜNAL]

On 16/04/2022 09:25, Luiz Angelo Daros de Luca wrote:
> There is no need to add new compatible strings for each new supported
> chip version. The compatible string is used only to select the subdriver
> (rtl8365mb.c or rtl8366rb). Once in the subdriver, it will detect the

Might as well call the subdriver rtl8365mb like rtl8366rb since you're 
going to send a v2 anyway.

Arınç

Re: [PATCH net 2/2] net: dsa: realtek: remove realtek,rtl8367s string

[Alvin Šipraga]

On Sat, Apr 16, 2022 at 03:25:04AM -0300, Luiz Angelo Daros de Luca wrote:
> There is no need to add new compatible strings for each new supported
> chip version. The compatible string is used only to select the subdriver
> (rtl8365mb.c or rtl8366rb). Once in the subdriver, it will detect the
> chip model by itself, ignoring which compatible string was used.
> 
> Link: https://lore.kernel.org/netdev/20220414014055.m4wbmr7tdz6hsa3m@bang-olufsen.dk/
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---

Hi Luiz,

I think it's great that you identified this early enough to remove the
redundant compatible string before it hits an official release. Now we
have well-defined semantics regarding the compatible strings, which is
great :)

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

>  drivers/net/dsa/realtek/realtek-mdio.c | 1 -
>  drivers/net/dsa/realtek/realtek-smi.c  | 4 ----
>  2 files changed, 5 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 31e1f100e48e..c58f49d558d2 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -267,7 +267,6 @@ static const struct of_device_id realtek_mdio_of_match[] = {
>  #endif
>  #if IS_ENABLED(CONFIG_NET_DSA_REALTEK_RTL8365MB)
>  	{ .compatible = "realtek,rtl8365mb", .data = &rtl8365mb_variant, },
> -	{ .compatible = "realtek,rtl8367s", .data = &rtl8365mb_variant, },
>  #endif
>  	{ /* sentinel */ },
>  };
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index 6cec559c90ce..45992f79ec8d 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -551,10 +551,6 @@ static const struct of_device_id realtek_smi_of_match[] = {
>  		.compatible = "realtek,rtl8365mb",
>  		.data = &rtl8365mb_variant,
>  	},
> -	{
> -		.compatible = "realtek,rtl8367s",
> -		.data = &rtl8365mb_variant,
> -	},
>  #endif
>  	{ /* sentinel */ },
>  };
> -- 
> 2.35.1
>

Re: [PATCH net 2/2] net: dsa: realtek: remove realtek,rtl8367s string

[Alvin Šipraga]

On Sat, Apr 16, 2022 at 11:07:24AM +0000, Alvin Šipraga wrote:
> Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Yikes, let me try that again...

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Re: [PATCH net 2/2] net: dsa: realtek: remove realtek,rtl8367s string

[Florian Fainelli]

On 4/15/2022 11:25 PM, Luiz Angelo Daros de Luca wrote:
> There is no need to add new compatible strings for each new supported
> chip version. The compatible string is used only to select the subdriver
> (rtl8365mb.c or rtl8366rb). Once in the subdriver, it will detect the
> chip model by itself, ignoring which compatible string was used.
> 
> Link: https://lore.kernel.org/netdev/20220414014055.m4wbmr7tdz6hsa3m@bang-olufsen.dk/
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net 1/2] dt-bindings: net: dsa: realtek: cleanup compatible strings

[Arınç ÜNAL]

On 16/04/2022 09:25, Luiz Angelo Daros de Luca wrote:
> Compatible strings are used to help the driver find the chip ID/version
> register for each chip family. After that, the driver can setup the
> switch accordingly. Keep only the first supported model for each family
> as a compatible string and reference other chip models in the
> description.
> 
> CC: devicetree@vger.kernel.org
> Link: https://lore.kernel.org/netdev/20220414014055.m4wbmr7tdz6hsa3m@bang-olufsen.dk/
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>   .../devicetree/bindings/net/dsa/realtek.yaml  | 33 +++++++------------
>   1 file changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index 8756060895a8..9bf862abb496 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -27,32 +27,23 @@ description:
>     The realtek-mdio driver is an MDIO driver and it must be inserted inside
>     an MDIO node.
>   
> +  The compatibility string is used only to find an identification register,
> +  (chip ID and version) which is at a different MDIO base address in different
> +  switch families. The driver then uses the chip ID/version to device how to
> +  drive the switch.

I think you meant to say "decide how to drive the switch"?

> +
>   properties:
>     compatible:
>       enum:
>         - realtek,rtl8365mb
> -      - realtek,rtl8366
>         - realtek,rtl8366rb
> -      - realtek,rtl8366s
> -      - realtek,rtl8367
> -      - realtek,rtl8367b
> -      - realtek,rtl8367rb
> -      - realtek,rtl8367s
> -      - realtek,rtl8368s
> -      - realtek,rtl8369
> -      - realtek,rtl8370
>       description: |
> -      realtek,rtl8365mb: 4+1 ports
> -      realtek,rtl8366: 5+1 ports
> -      realtek,rtl8366rb: 5+1 ports
> -      realtek,rtl8366s: 5+1 ports
> -      realtek,rtl8367:
> -      realtek,rtl8367b:
> -      realtek,rtl8367rb: 5+2 ports
> -      realtek,rtl8367s: 5+2 ports
> -      realtek,rtl8368s: 8 ports
> -      realtek,rtl8369: 8+1 ports
> -      realtek,rtl8370: 8+2 ports
> +      realtek,rtl8365mb:
> +        Use with models RTL8363NB, RTL8363NB-VB, RTL8363SC, RTL8363SC-VB,
> +        RTL8364NB, RTL8364NB-VB, RTL8365MB, RTL8366SC, RTL8367RB-VB, RTL8367S,
> +        RTL8367SB, RTL8370MB, RTL8310SR
> +      realtek,rtl8367rb:
> +        Use with models RTL8366RB, RTL8366S

You kept rtl8366rb string but defined description for rtl8367rb.

Arınç

Re: [PATCH net 1/2] dt-bindings: net: dsa: realtek: cleanup compatible strings

[Alvin Šipraga]

On Sat, Apr 16, 2022 at 03:25:03AM -0300, Luiz Angelo Daros de Luca wrote:
> Compatible strings are used to help the driver find the chip ID/version
> register for each chip family. After that, the driver can setup the
> switch accordingly. Keep only the first supported model for each family
> as a compatible string and reference other chip models in the
> description.
> 
> CC: devicetree@vger.kernel.org
> Link: https://lore.kernel.org/netdev/20220414014055.m4wbmr7tdz6hsa3m@bang-olufsen.dk/
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  .../devicetree/bindings/net/dsa/realtek.yaml  | 33 +++++++------------
>  1 file changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index 8756060895a8..9bf862abb496 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -27,32 +27,23 @@ description:
>    The realtek-mdio driver is an MDIO driver and it must be inserted inside
>    an MDIO node.
>  
> +  The compatibility string is used only to find an identification register,

s/compatibility/compatible/

> +  (chip ID and version) which is at a different MDIO base address in different
> +  switch families. The driver then uses the chip ID/version to device how to
> +  drive the switch.

This applies to Marvell nicely but it is not accurate for Realtek, so
either make it less precise (don't mention MDIO base address), or be
more precise. Here is my suggestion:

| The compatible string is only used to identify which (silicon) family
| the switch belongs to. Roughly speaking, a family is any set of Realtek
| switches whose chip identification register(s) have a common location
| and semantics. The different models in a given family can be
| automatically disambiguated by parsing the chip identification
| register(s) according to the given family, obviating the need for a
| unique compatible string for each model.

This also makes it clear when and why a new compatible string should be
added.

> +
>  properties:
>    compatible:
>      enum:
>        - realtek,rtl8365mb
> -      - realtek,rtl8366
>        - realtek,rtl8366rb
> -      - realtek,rtl8366s
> -      - realtek,rtl8367
> -      - realtek,rtl8367b
> -      - realtek,rtl8367rb
> -      - realtek,rtl8367s
> -      - realtek,rtl8368s
> -      - realtek,rtl8369
> -      - realtek,rtl8370
>      description: |
> -      realtek,rtl8365mb: 4+1 ports
> -      realtek,rtl8366: 5+1 ports
> -      realtek,rtl8366rb: 5+1 ports
> -      realtek,rtl8366s: 5+1 ports
> -      realtek,rtl8367:
> -      realtek,rtl8367b:
> -      realtek,rtl8367rb: 5+2 ports
> -      realtek,rtl8367s: 5+2 ports
> -      realtek,rtl8368s: 8 ports
> -      realtek,rtl8369: 8+1 ports
> -      realtek,rtl8370: 8+2 ports
> +      realtek,rtl8365mb:
> +        Use with models RTL8363NB, RTL8363NB-VB, RTL8363SC, RTL8363SC-VB,
> +        RTL8364NB, RTL8364NB-VB, RTL8365MB, RTL8366SC, RTL8367RB-VB, RTL8367S,
> +        RTL8367SB, RTL8370MB, RTL8310SR
> +      realtek,rtl8367rb:

s/67rb/66rb/?

> +        Use with models RTL8366RB, RTL8366S
>  
>    mdc-gpios:
>      description: GPIO line for the MDC clock line.
> @@ -335,7 +326,7 @@ examples:
>              #size-cells = <0>;
>  
>              switch@29 {
> -                    compatible = "realtek,rtl8367s";
> +                    compatible = "realtek,rtl8365mb";
>                      reg = <29>;
>  
>                      reset-gpios = <&gpio2 20 GPIO_ACTIVE_LOW>;
> -- 
> 2.35.1
>

Re: [PATCH net 1/2] dt-bindings: net: dsa: realtek: cleanup compatible strings

[Andrew Lunn]

On Sat, Apr 16, 2022 at 03:25:03AM -0300, Luiz Angelo Daros de Luca wrote:
> Compatible strings are used to help the driver find the chip ID/version
> register for each chip family. After that, the driver can setup the
> switch accordingly. Keep only the first supported model for each family
> as a compatible string and reference other chip models in the
> description.

It would be good to mention here that these compatible strings have
never been used in a released kernel, so it is safe to remove them.

That is the sort of information which makes the job of reviewing
patches simpler. It is great to have the answers to questions you
cannot see directly from the code in the commit message.

       Andrew

Re: [PATCH net 1/2] dt-bindings: net: dsa: realtek: cleanup compatible strings

[Linus Walleij]

On Sat, Apr 16, 2022 at 8:25 AM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> Compatible strings are used to help the driver find the chip ID/version
> register for each chip family. After that, the driver can setup the
> switch accordingly. Keep only the first supported model for each family
> as a compatible string and reference other chip models in the
> description.
>
> CC: devicetree@vger.kernel.org
> Link: https://lore.kernel.org/netdev/20220414014055.m4wbmr7tdz6hsa3m@bang-olufsen.dk/
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

OK, I suppose we know that Realtek has always maintained the
ID numbers in the hardware? Otherwise we will end up where
bindings/arm/primecell.yaml is: hardware ID numbers that were
supposed to be updated but weren't, so now both DT and the
kernel has to go through all kinds of loops and hoops to make it
work by encoding the number that should have been in the
hardware is instead in the device tree...

Yours,
Linus Walleij

Re: [PATCH net 1/2] dt-bindings: net: dsa: realtek: cleanup compatible strings

[Luiz Angelo Daros de Luca]

> On Sat, Apr 16, 2022 at 8:25 AM Luiz Angelo Daros de Luca
> <luizluca@gmail.com> wrote:
>
> > Compatible strings are used to help the driver find the chip ID/version
> > register for each chip family. After that, the driver can setup the
> > switch accordingly. Keep only the first supported model for each family
> > as a compatible string and reference other chip models in the
> > description.
> >
> > CC: devicetree@vger.kernel.org
> > Link: https://lore.kernel.org/netdev/20220414014055.m4wbmr7tdz6hsa3m@bang-olufsen.dk/
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
>
> OK, I suppose we know that Realtek has always maintained the
> ID numbers in the hardware? Otherwise we will end up where
> bindings/arm/primecell.yaml is: hardware ID numbers that were
> supposed to be updated but weren't, so now both DT and the
> kernel has to go through all kinds of loops and hoops to make it
> work by encoding the number that should have been in the
> hardware is instead in the device tree...

Thanks, Linus. The rtl8367c driver seems to depend on information
retrieved from registers, mainly chip id/ver. If they forget to update
a chip id/version, it might be the case that it does not really matter
from the driver's point of view.
Anyway, if deemed to be necessary, adding a compatible string is much
easier than removing one after a kernel is released.

Regards,

Luiz

Re: [PATCH net 1/2] dt-bindings: net: dsa: realtek: cleanup compatible strings

[Luiz Angelo Daros de Luca]

Thanks Arınç, Alvin, Andrew and Linus for the reviews. I just sent v2.

Regards,

Luiz