[PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Luiz Angelo Daros de Luca]

RTL8367RB-VB was not mentioned in the compatible table, nor in the
Kconfig help text.

The driver still detects the variant by itself and ignores which
compatible string was used to select it. So, any compatible string will
work for any compatible model.

Reported-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/Kconfig        | 3 ++-
 drivers/net/dsa/realtek/realtek-mdio.c | 1 +
 drivers/net/dsa/realtek/realtek-smi.c  | 4 ++++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
index b7427a8292b2..8eb5148bcc00 100644
--- a/drivers/net/dsa/realtek/Kconfig
+++ b/drivers/net/dsa/realtek/Kconfig
@@ -29,7 +29,8 @@ config NET_DSA_REALTEK_RTL8365MB
 	depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO
 	select NET_DSA_TAG_RTL8_4
 	help
-	  Select to enable support for Realtek RTL8365MB-VC and RTL8367S.
+	  Select to enable support for Realtek RTL8365MB-VC, RTL8367RB-VB
+	  and RTL8367S.
 
 config NET_DSA_REALTEK_RTL8366RB
 	tristate "Realtek RTL8366RB switch subdriver"
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 31e1f100e48e..a36b0d8f17ff 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -267,6 +267,7 @@ 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,rtl8367rb", .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 2243d3da55b2..c2200bd23448 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -556,6 +556,10 @@ static const struct of_device_id realtek_smi_of_match[] = {
 		.compatible = "realtek,rtl8365mb",
 		.data = &rtl8365mb_variant,
 	},
+	{
+		.compatible = "realtek,rtl8367rb",
+		.data = &rtl8365mb_variant,
+	},
 	{
 		.compatible = "realtek,rtl8367s",
 		.data = &rtl8365mb_variant,
-- 
2.35.1

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Andrew Lunn]

On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote:
> RTL8367RB-VB was not mentioned in the compatible table, nor in the
> Kconfig help text.
> 
> The driver still detects the variant by itself and ignores which
> compatible string was used to select it. So, any compatible string will
> work for any compatible model.

Meaning the compatible string is pointless, and cannot be trusted. So
yes, you can add it, but don't actually try to use it for anything,
like quirks.

     Andrew

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Luiz Angelo Daros de Luca]

> On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote:
> > RTL8367RB-VB was not mentioned in the compatible table, nor in the
> > Kconfig help text.
> >
> > The driver still detects the variant by itself and ignores which
> > compatible string was used to select it. So, any compatible string will
> > work for any compatible model.
>
> Meaning the compatible string is pointless, and cannot be trusted. So
> yes, you can add it, but don't actually try to use it for anything,
> like quirks.


Thanks, Andrew. Those compatible strings are indeed useless for now.
The driver probes the chip variant. Maybe in the future, if required,
we could provide a way to either force a model or let it autodetect as
it does today.

There is no "family name" for those devices. The best we had was
rtl8367c (with "c" probably meaning 3rd family). I suggested renaming
the driver to rtl8367c but, in the end, we kept it as the first
supported device name. My plan is, at least, to allow the user to
specify the correct model without knowing which model it is equivalent
to.

Regards,

Luiz

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Alvin Šipraga]

On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote:
> RTL8367RB-VB was not mentioned in the compatible table, nor in the
> Kconfig help text.
> 
> The driver still detects the variant by itself and ignores which
> compatible string was used to select it. So, any compatible string will
> work for any compatible model.

This is not quite true: a compatible string of realtek,rtl8366rb will select the
other subdriver, assuming it is available.

Besides that small inaccuracy, I think your description is missing one crucial
bit of information, which is that the chip ID/version of the '67RB is already
included in the driver. Otherwise it reads as though the '67RB has the same ID
as one of the already-supported chips ('65MB or '67S).

With the above clarifications:

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

Kind regards,
Alvin

> 
> Reported-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  drivers/net/dsa/realtek/Kconfig        | 3 ++-
>  drivers/net/dsa/realtek/realtek-mdio.c | 1 +
>  drivers/net/dsa/realtek/realtek-smi.c  | 4 ++++
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
> index b7427a8292b2..8eb5148bcc00 100644
> --- a/drivers/net/dsa/realtek/Kconfig
> +++ b/drivers/net/dsa/realtek/Kconfig
> @@ -29,7 +29,8 @@ config NET_DSA_REALTEK_RTL8365MB
>  	depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO
>  	select NET_DSA_TAG_RTL8_4
>  	help
> -	  Select to enable support for Realtek RTL8365MB-VC and RTL8367S.
> +	  Select to enable support for Realtek RTL8365MB-VC, RTL8367RB-VB
> +	  and RTL8367S.
>  
>  config NET_DSA_REALTEK_RTL8366RB
>  	tristate "Realtek RTL8366RB switch subdriver"
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 31e1f100e48e..a36b0d8f17ff 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -267,6 +267,7 @@ 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,rtl8367rb", .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 2243d3da55b2..c2200bd23448 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -556,6 +556,10 @@ static const struct of_device_id realtek_smi_of_match[] = {
>  		.compatible = "realtek,rtl8365mb",
>  		.data = &rtl8365mb_variant,
>  	},
> +	{
> +		.compatible = "realtek,rtl8367rb",
> +		.data = &rtl8365mb_variant,
> +	},
>  	{
>  		.compatible = "realtek,rtl8367s",
>  		.data = &rtl8365mb_variant,
> -- 
> 2.35.1
>

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Andrew Lunn]

On Tue, Apr 12, 2022 at 01:12:51AM -0300, Luiz Angelo Daros de Luca wrote:
> > On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote:
> > > RTL8367RB-VB was not mentioned in the compatible table, nor in the
> > > Kconfig help text.
> > >
> > > The driver still detects the variant by itself and ignores which
> > > compatible string was used to select it. So, any compatible string will
> > > work for any compatible model.
> >
> > Meaning the compatible string is pointless, and cannot be trusted. So
> > yes, you can add it, but don't actually try to use it for anything,
> > like quirks.
> 
> 
> Thanks, Andrew. Those compatible strings are indeed useless for now.
> The driver probes the chip variant. Maybe in the future, if required,
> we could provide a way to either force a model or let it autodetect as
> it does today.

The problem is, you have to assume some percentage of shipped DT blobs
have the wrong compatible string, but work because it is not actually
used in a meaningful way. This is why the couple of dozen Marvell
switches have just 3 compatible strings, which is enough to find the
ID registers to identify the actual switch. The three compatibles are
the name of the lowest chip in the family which introduced to location
of the ID register.

> There is no "family name" for those devices. The best we had was
> rtl8367c (with "c" probably meaning 3rd family). I suggested renaming
> the driver to rtl8367c but, in the end, we kept it as the first
> supported device name. My plan is, at least, to allow the user to
> specify the correct model without knowing which model it is equivalent
> to.

In order words, you are quite happy to allow the DT author to get is
wrong, and do not care it is wrong. So the percentage of DT blobs with
the wrong compatible will go up, making it even more useless.

It is also something you cannot retrospectively make useful, because
of all those broken DT blobs.

    Andrew

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Alvin Šipraga]

On Tue, Apr 12, 2022 at 02:43:06PM +0200, Andrew Lunn wrote:
> On Tue, Apr 12, 2022 at 01:12:51AM -0300, Luiz Angelo Daros de Luca wrote:
> > > On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote:
> > > > RTL8367RB-VB was not mentioned in the compatible table, nor in the
> > > > Kconfig help text.
> > > >
> > > > The driver still detects the variant by itself and ignores which
> > > > compatible string was used to select it. So, any compatible string will
> > > > work for any compatible model.
> > >
> > > Meaning the compatible string is pointless, and cannot be trusted. So
> > > yes, you can add it, but don't actually try to use it for anything,
> > > like quirks.
> > 
> > 
> > Thanks, Andrew. Those compatible strings are indeed useless for now.
> > The driver probes the chip variant. Maybe in the future, if required,
> > we could provide a way to either force a model or let it autodetect as
> > it does today.
> 
> The problem is, you have to assume some percentage of shipped DT blobs
> have the wrong compatible string, but work because it is not actually
> used in a meaningful way. This is why the couple of dozen Marvell
> switches have just 3 compatible strings, which is enough to find the
> ID registers to identify the actual switch. The three compatibles are
> the name of the lowest chip in the family which introduced to location
> of the ID register.

Right, this was basically the original behaviour:

- realtek,rtl8265mb -> use rtl8365mb.c subdriver
- realtek,rtl8366rb -> use rtl8366rb.c subdriver (different family with different register layout)

We then check a chip ID/version register and store that in the driver-private
data, in case of quirks or different behaviours between chips in the same
family.

I think Andrew has a point that adding more compatible strings is not really
going to add any tangible benefit, due to the above bahviour. People can equally
well just put one of the above two compatible strings.

> 
> > There is no "family name" for those devices. The best we had was
> > rtl8367c (with "c" probably meaning 3rd family). I suggested renaming
> > the driver to rtl8367c but, in the end, we kept it as the first
> > supported device name. My plan is, at least, to allow the user to
> > specify the correct model without knowing which model it is equivalent
> > to.
> 
> In order words, you are quite happy to allow the DT author to get is
> wrong, and do not care it is wrong. So the percentage of DT blobs with
> the wrong compatible will go up, making it even more useless.
> 
> It is also something you cannot retrospectively make useful, because
> of all those broken DT blobs.

I think Luiz is saying he wants to allow device tree authors to write
"realtek,rtl8367rb" if their hardware really does have an RTL8367RB switch and
not an RTL8365MB, rather than writing "realtek,rtl8365mb". But an enterprising
device tree author could just as well write:

	 compatible = "realtek,rtl8367rb", "realtek,rtl8365mb";

... which would work without us having to continually add more (arguably
useless) compatible strings to the driver, including this one.

Kind regards,
Alvin

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Luiz Angelo Daros de Luca]

> On Tue, Apr 12, 2022 at 02:43:06PM +0200, Andrew Lunn wrote:
> > On Tue, Apr 12, 2022 at 01:12:51AM -0300, Luiz Angelo Daros de Luca wrote:
> > > > On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote:
> > > > > RTL8367RB-VB was not mentioned in the compatible table, nor in the
> > > > > Kconfig help text.
> > > > >
> > > > > The driver still detects the variant by itself and ignores which
> > > > > compatible string was used to select it. So, any compatible string will
> > > > > work for any compatible model.
> > > >
> > > > Meaning the compatible string is pointless, and cannot be trusted. So
> > > > yes, you can add it, but don't actually try to use it for anything,
> > > > like quirks.
> > >
> > >
> > > Thanks, Andrew. Those compatible strings are indeed useless for now.
> > > The driver probes the chip variant. Maybe in the future, if required,
> > > we could provide a way to either force a model or let it autodetect as
> > > it does today.
> >
> > The problem is, you have to assume some percentage of shipped DT blobs
> > have the wrong compatible string, but work because it is not actually
> > used in a meaningful way. This is why the couple of dozen Marvell
> > switches have just 3 compatible strings, which is enough to find the
> > ID registers to identify the actual switch. The three compatibles are
> > the name of the lowest chip in the family which introduced to location
> > of the ID register.
>
> Right, this was basically the original behaviour:
>
> - realtek,rtl8265mb -> use rtl8365mb.c subdriver
> - realtek,rtl8366rb -> use rtl8366rb.c subdriver (different family with different register layout)
>
> We then check a chip ID/version register and store that in the driver-private
> data, in case of quirks or different behaviours between chips in the same
> family.
>
> I think Andrew has a point that adding more compatible strings is not really
> going to add any tangible benefit, due to the above bahviour. People can equally
> well just put one of the above two compatible strings.

The Realtek driver (rtl8367c) does provide a way to skip the probe and
force a specific model detection. Maybe that is a requirement for some
kind of device we might see in the future. If needed, we could add a
new property ("autodetect-{model,variant} = false") to force the model
based only on the compatible string. It would also allow a compatible
device to use the driver even if its ids are not known by the driver.

> > > There is no "family name" for those devices. The best we had was
> > > rtl8367c (with "c" probably meaning 3rd family). I suggested renaming
> > > the driver to rtl8367c but, in the end, we kept it as the first
> > > supported device name. My plan is, at least, to allow the user to
> > > specify the correct model without knowing which model it is equivalent
> > > to.
> >
> > In order words, you are quite happy to allow the DT author to get is
> > wrong, and do not care it is wrong. So the percentage of DT blobs with
> > the wrong compatible will go up, making it even more useless.

I wouldn't say it is wrong but not as specific as possible.
"compatible" seems to indicate that a driver from modelA can be used
for modelB.
We could start to warn the user when the compatible string does not
match the model (at least from what we already know).

Today, the driver only uses the model to tell the CPU and user ports
(and chip version is enough for that usage). However, the vendor
driver does an independent probe when it is setting the external port
and it does check additional registers. Today the Linux driver only
supports RGMII without actually checking if the model does support it.
When we expand that support to other port modes, we might need to
revisit the chip probe.

> > It is also something you cannot retrospectively make useful, because
> > of all those broken DT blobs.

We cannot tell if there are other models that share the same chip
version (from reg 0x1302). We can only tell from the devices we have
access to. Yes, there might already be a device using a different
compatible string from the real device and I don't intend to break it.

> I think Luiz is saying he wants to allow device tree authors to write
> "realtek,rtl8367rb" if their hardware really does have an RTL8367RB switch and
> not an RTL8365MB, rather than writing "realtek,rtl8365mb". But an enterprising
> device tree author could just as well write:
>
>          compatible = "realtek,rtl8367rb", "realtek,rtl8365mb";
>
> ... which would work without us having to continually add more (arguably
> useless) compatible strings to the driver, including this one.

Yes, Alvin. Thanks.

It feels strange to force the user to use "realtek,rtl8365mb" or any
other different string that does not match the chip's real name. I
would not expect the one writing the DT to know that rtl8367s shares
the same family with rtl8365mb and rtl8365mb driver does support
rtl8367s. Before writing the rtl8367s driver, I also didn't know the
relation between those chips. The common was only to relate rtl8367s
(or any other chip model) with the vendor driver rtl8367c. As we don't
have a generic family string, I think it is better to add every model
variant.

Regards,

Luiz

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Luiz Angelo Daros de Luca]

> On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote:
> > RTL8367RB-VB was not mentioned in the compatible table, nor in the
> > Kconfig help text.
> >
> > The driver still detects the variant by itself and ignores which
> > compatible string was used to select it. So, any compatible string will
> > work for any compatible model.
>
> This is not quite true: a compatible string of realtek,rtl8366rb will select the
> other subdriver, assuming it is available.

Yes, how about:

The string (no matter which one) is currently only used to select the
subdriver. Then, the subdriver
will ignore which compatible string was used and it will detect the
variant by itself using the
chip id/version returned by the device.

rtl8367rb chip ID/version of the '67RB is already included in the
driver and in the dt-bindings.

> Besides that small inaccuracy, I think your description is missing one crucial
> bit of information, which is that the chip ID/version of the '67RB is already
> included in the driver. Otherwise it reads as though the '67RB has the same ID
> as one of the already-supported chips ('65MB or '67S).
> With the above clarifications:
>
> Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Andrew Lunn]

> It feels strange to force the user to use "realtek,rtl8365mb" or any
> other different string that does not match the chip's real name. I
> would not expect the one writing the DT to know that rtl8367s shares
> the same family with rtl8365mb and rtl8365mb driver does support
> rtl8367s. Before writing the rtl8367s driver, I also didn't know the
> relation between those chips. The common was only to relate rtl8367s
> (or any other chip model) with the vendor driver rtl8367c. As we don't
> have a generic family string, I think it is better to add every model
> variant.

I will just quote the Marvell mv88e6xxx binding:

The compatibility string is used only to find an identification register,
which is at a different MDIO base address in different switch families.
- "marvell,mv88e6085"   : Switch has base address 0x10. Use with models:
                          6085, 6095, 6097, 6123, 6131, 6141, 6161, 6165,
                          6171, 6172, 6175, 6176, 6185, 6240, 6320, 6321,
                          6341, 6350, 6351, 6352
- "marvell,mv88e6190"   : Switch has base address 0x00. Use with models:
                          6190, 6190X, 6191, 6290, 6390, 6390X
- "marvell,mv88e6250"   : Switch has base address 0x08 or 0x18. Use with model:
                          6220, 6250

This has worked well for that driver.

     Andrew

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Alvin Šipraga]

On Wed, Apr 13, 2022 at 08:40:54PM +0200, Andrew Lunn wrote:
> > It feels strange to force the user to use "realtek,rtl8365mb" or any
> > other different string that does not match the chip's real name. I
> > would not expect the one writing the DT to know that rtl8367s shares
> > the same family with rtl8365mb and rtl8365mb driver does support
> > rtl8367s. Before writing the rtl8367s driver, I also didn't know the
> > relation between those chips. The common was only to relate rtl8367s
> > (or any other chip model) with the vendor driver rtl8367c. As we don't
> > have a generic family string, I think it is better to add every model
> > variant.
> 
> I will just quote the Marvell mv88e6xxx binding:
> 
> The compatibility string is used only to find an identification register,
> which is at a different MDIO base address in different switch families.
> - "marvell,mv88e6085"   : Switch has base address 0x10. Use with models:
>                           6085, 6095, 6097, 6123, 6131, 6141, 6161, 6165,
>                           6171, 6172, 6175, 6176, 6185, 6240, 6320, 6321,
>                           6341, 6350, 6351, 6352
> - "marvell,mv88e6190"   : Switch has base address 0x00. Use with models:
>                           6190, 6190X, 6191, 6290, 6390, 6390X
> - "marvell,mv88e6250"   : Switch has base address 0x08 or 0x18. Use with model:
>                           6220, 6250
> 
> This has worked well for that driver.

Right, so there's no real reason to add more compatible strings as long as the
DT bindings are clear about it. Luiz, if you think the DT bindings are somehow
unclear, feel free to update them. But I think we are probably better off not
adding any more compatible strings unless there is a real technical need for it.

In the same way, I guess the recently added compatible string "realtek,rtl8367s"
was actually unnecessary.

Kind regards,
Alvin

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Alvin Šipraga]

On Wed, Apr 13, 2022 at 03:38:31PM -0300, Luiz Angelo Daros de Luca wrote:
> > On Mon, Apr 11, 2022 at 06:04:07PM -0300, Luiz Angelo Daros de Luca wrote:
> > > RTL8367RB-VB was not mentioned in the compatible table, nor in the
> > > Kconfig help text.
> > >
> > > The driver still detects the variant by itself and ignores which
> > > compatible string was used to select it. So, any compatible string will
> > > work for any compatible model.
> >
> > This is not quite true: a compatible string of realtek,rtl8366rb will select the
> > other subdriver, assuming it is available.
> 
> Yes, how about:
> 
> The string (no matter which one) is currently only used to select the
> subdriver. Then, the subdriver
> will ignore which compatible string was used and it will detect the
> variant by itself using the
> chip id/version returned by the device.
> 
> rtl8367rb chip ID/version of the '67RB is already included in the
> driver and in the dt-bindings.
> 
> > Besides that small inaccuracy, I think your description is missing one crucial
> > bit of information, which is that the chip ID/version of the '67RB is already
> > included in the driver. Otherwise it reads as though the '67RB has the same ID
> > as one of the already-supported chips ('65MB or '67S).
> > With the above clarifications:
> >
> > Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

While the code is OK, on second thought I think based on Andrew's points in the
other subthread that we are better off without this patch.

Kind regards,
Alvin

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Luiz Angelo Daros de Luca]

> While the code is OK, on second thought I think based on Andrew's points in the
> other subthread that we are better off without this patch.

I agree, although the rtl8365mb name will make it harder for a
newcomer to find the driver.

Is it too late to get rid of all those compatible strings from
dt-bindings? And rtl8367s from the driver?
We must add all supported devices to the doc as well, similar to mv88e6085.

Regards,

Luiz

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Alvin Šipraga]

On Wed, Apr 13, 2022 at 11:32:42PM -0300, Luiz Angelo Daros de Luca wrote:
> > While the code is OK, on second thought I think based on Andrew's points in the
> > other subthread that we are better off without this patch.
> 
> I agree, although the rtl8365mb name will make it harder for a
> newcomer to find the driver.

If it is made clear in the DT bindings, I think it's fine. If it works for
Marvell switches, it will work for Realtek switches too.

> 
> Is it too late to get rid of all those compatible strings from
> dt-bindings? And rtl8367s from the driver?
> We must add all supported devices to the doc as well, similar to mv88e6085.

You can always try! I'm OK with those things in principle, but others might
object due to ABI reasons.

Don't get hung up on the vestigial "realtek,rtl8367s" compatible string
though... while it's probably harmless to remove it, it's also relatively
harmless to leave it there. Linux is full of such examples.

Kind regards,
Alvin

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Andrew Lunn]

> > Is it too late to get rid of all those compatible strings from
> > dt-bindings? And rtl8367s from the driver?
> > We must add all supported devices to the doc as well, similar to mv88e6085.
> 
> You can always try! I'm OK with those things in principle, but others might
> object due to ABI reasons.

Anything which is in a released Linus kernel is ABI and cannot be
removed. Anything in net-next, or an -rcX kernel can still be changed.

	 Andrew

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Luiz Angelo Daros de Luca]

> > > Is it too late to get rid of all those compatible strings from
> > > dt-bindings? And rtl8367s from the driver?
> > > We must add all supported devices to the doc as well, similar to mv88e6085.
> >
> > You can always try! I'm OK with those things in principle, but others might
> > object due to ABI reasons.
>
> Anything which is in a released Linus kernel is ABI and cannot be
> removed. Anything in net-next, or an -rcX kernel can still be changed.

rtl8367s was added for 5.18. I'll prepare a patch for net to remove it.

Now, about dt-bindings, I don't know what is the best approach. As
device-tree should not focus on Linux, it is strange to use a
compatible "rtl8365mb" just because it is the Linux subdriver name and
that name was just because it was the first device supported. Also,
once published, it is not good to break it.

Just to illustrate it better, these are the chip versions rtl8365mb.c
already supports or could support:
- rtl8363nb (not in dt)
- rtl8363nb-vb (not in dt)
- rtl8363sc (not in dt)
- rtl8363sc-vb (not in dt)
- rtl8364nb (not in dt)
- rtl8364nb-vb (not in dt)
- rtl8365mb (the first supported by rtl8365mb.c, maybe it should be
realtek,rtl8365mb-vc as rtl8365mb and rtl8365mb-vb seems to exist and
they might be incompatible)
- rtl8366sc (not in dt)
- rtl8367rb-vb (not in dt)
- rtl8367s (in dt and referenced in the code. The one I'll remove from code)
- rtl8367sb (new)
- rtl8370mb (new)
- rtl8310sr (new)

and these are the ones referenced in rtl8366rb.c code:
- rtl8366rb (rtl8366rb.c)
- rtl8366s (removed from rtl8366rb.c recently)

but I know nothing about unsupported chip versions. These "models" are
referenced in the bindings but some of them are not really a chip,
like rtl8367 and rtl8367b:
- rtl8366 (??)
- rtl8367 (??)
- rtl8367b (??)
- rtl8367rb (??)
- rtl8368s (??)
- rtl8369 (??)
- rtl8370 (??)

Anyway, I'm planning on submitting something like this:

--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -31,28 +31,14 @@ 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.

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Andrew Lunn]

> Now, about dt-bindings, I don't know what is the best approach. As
> device-tree should not focus on Linux, it is strange to use a
> compatible "rtl8365mb" just because it is the Linux subdriver name and
> that name was just because it was the first device supported.

What you are trying to express is, how do you access the ID
register. There is no obvious One True Compatible string for that. So
just picking one switch name for that is O.K. There is nothing Linux
specific in that, FreeBSD or whatever can use the label as a clue
where to find the ID register.

> +      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

So to me, this is fine. But i might add a bit more detail, that the
compatible is used by the driver to find the ID register, and the
driver then uses to ID register to decide how to drive the switch. The
problem i had with the mv88e6xxx binding was until i spelt this out in
the binding, people kept submitting patches adding new compatible
strings, rather than extend the documented list of switches supported
by a compatible.

       Andrew

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Luiz Angelo Daros de Luca]

> So to me, this is fine. But i might add a bit more detail, that the
> compatible is used by the driver to find the ID register, and the
> driver then uses to ID register to decide how to drive the switch. The
> problem i had with the mv88e6xxx binding was until i spelt this out in
> the binding, people kept submitting patches adding new compatible
> strings, rather than extend the documented list of switches supported
> by a compatible.

Thanks, Andrew.

I just sent two patches to deal with the cleanup.

Regards,

Luiz

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Luiz Angelo Daros de Luca]

>
> > RTL8367RB-VB was not mentioned in the compatible table, nor in the
> > Kconfig help text.
> >
> > The driver still detects the variant by itself and ignores which
> > compatible string was used to select it. So, any compatible string will
> > work for any compatible model.
> >
> > Reported-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Yours,
> Linus Walleij

Is there anything else pending I need to do? Docs are already updated
(during yaml conversion)

Regards,

Luiz

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Luiz Angelo Daros de Luca]

> Please also update the binding documentation:
> Documentation/devicetree/bindings/net/dsa/realtek-smi.txt

Documentation/devicetree/bindings/net/dsa/realtek.yaml was merged

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Linus Walleij]

On Tue, Feb 8, 2022 at 6:16 AM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> RTL8367RB-VB was not mentioned in the compatible table, nor in the
> Kconfig help text.
>
> The driver still detects the variant by itself and ignores which
> compatible string was used to select it. So, any compatible string will
> work for any compatible model.
>
> Reported-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Luiz Angelo Daros de Luca]

>
> On Tue, Feb 08, 2022 at 02:15:52AM -0300, Luiz Angelo Daros de Luca wrote:
> > RTL8367RB-VB was not mentioned in the compatible table, nor in the
> > Kconfig help text.
> >
> > The driver still detects the variant by itself and ignores which
> > compatible string was used to select it. So, any compatible string will
> > work for any compatible model.
>
> Please also update the binding documentation:
> Documentation/devicetree/bindings/net/dsa/realtek-smi.txt

Hello Andrew,

I just sent the doc update.
https://patchwork.kernel.org/project/netdevbpf/list/?series=612748

Thanks,

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Luiz Angelo Daros de Luca]

> On Tue, Feb 08, 2022 at 02:15:52AM -0300, Luiz Angelo Daros de Luca wrote:
> > RTL8367RB-VB was not mentioned in the compatible table, nor in the
> > Kconfig help text.
> >
> > The driver still detects the variant by itself and ignores which
> > compatible string was used to select it. So, any compatible string will
> > work for any compatible model.
>
> Please also update the binding documentation:
> Documentation/devicetree/bindings/net/dsa/realtek-smi.txt

No, I'll do better. I'll send a new yaml version covering both
realtek-smi and realtek-mdio.

>
> Thanks
>         Andrew

Re: [PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Andrew Lunn]

On Tue, Feb 08, 2022 at 02:15:52AM -0300, Luiz Angelo Daros de Luca wrote:
> RTL8367RB-VB was not mentioned in the compatible table, nor in the
> Kconfig help text.
> 
> The driver still detects the variant by itself and ignores which
> compatible string was used to select it. So, any compatible string will
> work for any compatible model.

Please also update the binding documentation:
Documentation/devicetree/bindings/net/dsa/realtek-smi.txt

Thanks
	Andrew

[PATCH net-next] net: dsa: realtek: add compatible strings for RTL8367RB-VB

[Luiz Angelo Daros de Luca]

RTL8367RB-VB was not mentioned in the compatible table, nor in the
Kconfig help text.

The driver still detects the variant by itself and ignores which
compatible string was used to select it. So, any compatible string will
work for any compatible model.

Reported-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/Kconfig        | 3 ++-
 drivers/net/dsa/realtek/realtek-mdio.c | 1 +
 drivers/net/dsa/realtek/realtek-smi.c  | 4 ++++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
index b7427a8292b2..8eb5148bcc00 100644
--- a/drivers/net/dsa/realtek/Kconfig
+++ b/drivers/net/dsa/realtek/Kconfig
@@ -29,7 +29,8 @@ config NET_DSA_REALTEK_RTL8365MB
 	depends on NET_DSA_REALTEK_SMI || NET_DSA_REALTEK_MDIO
 	select NET_DSA_TAG_RTL8_4
 	help
-	  Select to enable support for Realtek RTL8365MB-VC and RTL8367S.
+	  Select to enable support for Realtek RTL8365MB-VC, RTL8367RB-VB
+	  and RTL8367S.
 
 config NET_DSA_REALTEK_RTL8366RB
 	tristate "Realtek RTL8366RB switch subdriver"
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 0c5f2bdced9d..e6e3c1769166 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -206,6 +206,7 @@ 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,rtl8367rb", .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 946fbbd70153..a849b5cbb4e4 100644
--- a/drivers/net/dsa/realtek/realtek-smi.c
+++ b/drivers/net/dsa/realtek/realtek-smi.c
@@ -510,6 +510,10 @@ static const struct of_device_id realtek_smi_of_match[] = {
 		.compatible = "realtek,rtl8365mb",
 		.data = &rtl8365mb_variant,
 	},
+	{
+		.compatible = "realtek,rtl8367rb",
+		.data = &rtl8365mb_variant,
+	},
 	{
 		.compatible = "realtek,rtl8367s",
 		.data = &rtl8365mb_variant,
-- 
2.35.1