[PATCH 1/2] net: dsa: microchip: add KSZ9896 switch support

[PATCH 1/2] net: dsa: microchip: add KSZ9896 switch support

[Romain Naour]

From: Romain Naour <romain.naour@skf.com>

Add support for the KSZ9896 6-port Gigabit Ethernet Switch to the
ksz9477 driver.

Although the KSZ9896 is already listed in the device tree binding
documentation since a1c0ed24fe9b (dt-bindings: net: dsa: document
additional Microchip KSZ9477 family switches) the chip id
(0x00989600) is not recognized by ksz_switch_detect() and rejected
by the driver.

The KSZ9896 is similar to KSZ9897 but has only one configurable
MII/RMII/RGMII/GMII cpu port.

Signed-off-by: Romain Naour <romain.naour@skf.com>
Signed-off-by: Romain Naour <romain.naour@smile.fr>
---
It seems that the KSZ9896 support has been sent to the kernel netdev
mailing list a while ago but got lost after initial review:
https://www.spinics.net/lists/netdev/msg554771.html

The initial testing with the ksz9896 was done on a 5.10 kernel
but due to recent changes in dsa microchip driver it was required
to rework this initial version for 6.0-rc2 kernel.
---
 drivers/net/dsa/microchip/ksz_common.c | 30 ++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |  2 ++
 drivers/net/dsa/microchip/ksz_spi.c    |  6 ++++++
 3 files changed, 38 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 6bd69a7e6809..759d98f4e317 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -547,6 +547,34 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 				   true, false, false},
 	},
 
+	[KSZ9896] = {
+		.chip_id = KSZ9896_CHIP_ID,
+		.dev_name = "KSZ9896",
+		.num_vlans = 4096,
+		.num_alus = 4096,
+		.num_statics = 16,
+		.cpu_ports = 0x3F,	/* can be configured as cpu port */
+		.port_cnt = 6,		/* total physical port count */
+		.ops = &ksz9477_dev_ops,
+		.phy_errata_9477 = true,
+		.mib_names = ksz9477_mib_names,
+		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
+		.reg_mib_cnt = MIB_COUNTER_NUM,
+		.regs = ksz9477_regs,
+		.masks = ksz9477_masks,
+		.shifts = ksz9477_shifts,
+		.xmii_ctrl0 = ksz9477_xmii_ctrl0,
+		.xmii_ctrl1 = ksz9477_xmii_ctrl1,
+		.supports_mii	= {false, false, false, false,
+				   false, true},
+		.supports_rmii	= {false, false, false, false,
+				   false, true},
+		.supports_rgmii = {false, false, false, false,
+				   false, true},
+		.internal_phy	= {true, true, true, true,
+				   true, false},
+	},
+
 	[KSZ9897] = {
 		.chip_id = KSZ9897_CHIP_ID,
 		.dev_name = "KSZ9897",
@@ -1370,6 +1398,7 @@ static enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds,
 		proto = DSA_TAG_PROTO_KSZ9893;
 
 	if (dev->chip_id == KSZ9477_CHIP_ID ||
+	    dev->chip_id == KSZ9896_CHIP_ID ||
 	    dev->chip_id == KSZ9897_CHIP_ID ||
 	    dev->chip_id == KSZ9567_CHIP_ID)
 		proto = DSA_TAG_PROTO_KSZ9477;
@@ -1730,6 +1759,7 @@ static int ksz_switch_detect(struct ksz_device *dev)
 
 		switch (id32) {
 		case KSZ9477_CHIP_ID:
+		case KSZ9896_CHIP_ID:
 		case KSZ9897_CHIP_ID:
 		case KSZ9893_CHIP_ID:
 		case KSZ9567_CHIP_ID:
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 0d9520dc6d2d..65980da61b54 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -129,6 +129,7 @@ enum ksz_model {
 	KSZ8765,
 	KSZ8830,
 	KSZ9477,
+	KSZ9896,
 	KSZ9897,
 	KSZ9893,
 	KSZ9567,
@@ -145,6 +146,7 @@ enum ksz_chip_id {
 	KSZ8765_CHIP_ID = 0x8765,
 	KSZ8830_CHIP_ID = 0x8830,
 	KSZ9477_CHIP_ID = 0x00947700,
+	KSZ9896_CHIP_ID = 0x00989600,
 	KSZ9897_CHIP_ID = 0x00989700,
 	KSZ9893_CHIP_ID = 0x00989300,
 	KSZ9567_CHIP_ID = 0x00956700,
diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c
index 05bd089795f8..1b6ff5e3d575 100644
--- a/drivers/net/dsa/microchip/ksz_spi.c
+++ b/drivers/net/dsa/microchip/ksz_spi.c
@@ -146,6 +146,10 @@ static const struct of_device_id ksz_dt_ids[] = {
 		.compatible = "microchip,ksz9477",
 		.data = &ksz_switch_chips[KSZ9477]
 	},
+	{
+		.compatible = "microchip,ksz9896",
+		.data = &ksz_switch_chips[KSZ9896]
+	},
 	{
 		.compatible = "microchip,ksz9897",
 		.data = &ksz_switch_chips[KSZ9897]
@@ -197,6 +201,7 @@ static const struct spi_device_id ksz_spi_ids[] = {
 	{ "ksz8863" },
 	{ "ksz8873" },
 	{ "ksz9477" },
+	{ "ksz9896" },
 	{ "ksz9897" },
 	{ "ksz9893" },
 	{ "ksz9563" },
@@ -226,6 +231,7 @@ static struct spi_driver ksz_spi_driver = {
 module_spi_driver(ksz_spi_driver);
 
 MODULE_ALIAS("spi:ksz9477");
+MODULE_ALIAS("spi:ksz9896");
 MODULE_ALIAS("spi:ksz9897");
 MODULE_ALIAS("spi:ksz9893");
 MODULE_ALIAS("spi:ksz9563");
-- 
2.34.3

[PATCH 2/2] net: dsa: microchip: add KSZ9896 to KSZ9477 I2C driver

[Romain Naour]

From: Romain Naour <romain.naour@skf.com>

Add support for the KSZ9896 6-port Gigabit Ethernet Switch to the
ksz9477 driver. The KSZ9896 supports both SPI (already in) and I2C.

Signed-off-by: Romain Naour <romain.naour@skf.com>
Signed-off-by: Romain Naour <romain.naour@smile.fr>
---
The KSZ9896 support i2c interface, it seems safe to enable as is but
runtime testing is really needed (my KSZ9896 is wired with spi).
---
 drivers/net/dsa/microchip/ksz9477_i2c.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
index 99966514d444..8fbc122e3384 100644
--- a/drivers/net/dsa/microchip/ksz9477_i2c.c
+++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
@@ -91,6 +91,10 @@ static const struct of_device_id ksz9477_dt_ids[] = {
 		.compatible = "microchip,ksz9477",
 		.data = &ksz_switch_chips[KSZ9477]
 	},
+	{
+		.compatible = "microchip,ksz9896",
+		.data = &ksz_switch_chips[KSZ9896]
+	},
 	{
 		.compatible = "microchip,ksz9897",
 		.data = &ksz_switch_chips[KSZ9897]
-- 
2.34.3

Re: [PATCH 1/2] net: dsa: microchip: add KSZ9896 switch support

[Andrew Lunn]

On Thu, Aug 25, 2022 at 11:39:42PM +0200, Romain Naour wrote:
> From: Romain Naour <romain.naour@skf.com>
> 
> Add support for the KSZ9896 6-port Gigabit Ethernet Switch to the
> ksz9477 driver.
> 
> Although the KSZ9896 is already listed in the device tree binding
> documentation since a1c0ed24fe9b (dt-bindings: net: dsa: document
> additional Microchip KSZ9477 family switches) the chip id
> (0x00989600) is not recognized by ksz_switch_detect() and rejected
> by the driver.
> 
> The KSZ9896 is similar to KSZ9897 but has only one configurable
> MII/RMII/RGMII/GMII cpu port.
> 
> Signed-off-by: Romain Naour <romain.naour@skf.com>
> Signed-off-by: Romain Naour <romain.naour@smile.fr>

Two signed-off-by from the same person is unusual :-)

> ---
> It seems that the KSZ9896 support has been sent to the kernel netdev
> mailing list a while ago but got lost after initial review:
> https://www.spinics.net/lists/netdev/msg554771.html

I'm not sure saying it got lost is true. It looks more like the issues
pointed out were never addressed.

> The initial testing with the ksz9896 was done on a 5.10 kernel
> but due to recent changes in dsa microchip driver it was required
> to rework this initial version for 6.0-rc2 kernel.

This looks sufficiently different that i don't think we need
Tristram's Signed-off-by as well.

I don't know these chips well enough to do a detailed review.

  Andrew

Re: [PATCH 1/2] net: dsa: microchip: add KSZ9896 switch support

[Romain Naour]

Hi,

Le 26/08/2022 à 00:04, Andrew Lunn a écrit :
> On Thu, Aug 25, 2022 at 11:39:42PM +0200, Romain Naour wrote:
>> From: Romain Naour <romain.naour@skf.com>
>>
>> Add support for the KSZ9896 6-port Gigabit Ethernet Switch to the
>> ksz9477 driver.
>>
>> Although the KSZ9896 is already listed in the device tree binding
>> documentation since a1c0ed24fe9b (dt-bindings: net: dsa: document
>> additional Microchip KSZ9477 family switches) the chip id
>> (0x00989600) is not recognized by ksz_switch_detect() and rejected
>> by the driver.
>>
>> The KSZ9896 is similar to KSZ9897 but has only one configurable
>> MII/RMII/RGMII/GMII cpu port.
>>
>> Signed-off-by: Romain Naour <romain.naour@skf.com>
>> Signed-off-by: Romain Naour <romain.naour@smile.fr>
> 
> Two signed-off-by from the same person is unusual :-)

Indeed, but my customer (skf) asked me to use the skf.com address for the patch
but I use my smile.fr (my employer) git/email setup for mailing list.

> 
>> ---
>> It seems that the KSZ9896 support has been sent to the kernel netdev
>> mailing list a while ago but got lost after initial review:
>> https://www.spinics.net/lists/netdev/msg554771.html
> 
> I'm not sure saying it got lost is true. It looks more like the issues
> pointed out were never addressed.

It seems the initial KSZ9896 support was in the same patch with other changes
that were addressed later by followup patches.

> 
>> The initial testing with the ksz9896 was done on a 5.10 kernel
>> but due to recent changes in dsa microchip driver it was required
>> to rework this initial version for 6.0-rc2 kernel.
> 
> This looks sufficiently different that i don't think we need
> Tristram's Signed-off-by as well.
> 
> I don't know these chips well enough to do a detailed review.

Thanks for your feedback!

Best regards,
Romain


> 
>   Andrew

Re: [PATCH 1/2] net: dsa: microchip: add KSZ9896 switch support

[Jakub Kicinski]

On Fri, 26 Aug 2022 09:31:00 +0200 Romain Naour wrote:
> >> Signed-off-by: Romain Naour <romain.naour@skf.com>
> >> Signed-off-by: Romain Naour <romain.naour@smile.fr>  
> > 
> > Two signed-off-by from the same person is unusual :-)  
> 
> Indeed, but my customer (skf) asked me to use the skf.com address for the patch
> but I use my smile.fr (my employer) git/email setup for mailing list.

It's pretty common to use a different email server than what's
on the From and S-o-b lines. You can drop the second S-o-b.

Re: [PATCH 1/2] net: dsa: microchip: add KSZ9896 switch support

[Romain Naour]

Hi,

Le 27/08/2022 à 02:49, Jakub Kicinski a écrit :
> On Fri, 26 Aug 2022 09:31:00 +0200 Romain Naour wrote:
>>>> Signed-off-by: Romain Naour <romain.naour@skf.com>
>>>> Signed-off-by: Romain Naour <romain.naour@smile.fr>  
>>>
>>> Two signed-off-by from the same person is unusual :-)  
>>
>> Indeed, but my customer (skf) asked me to use the skf.com address for the patch
>> but I use my smile.fr (my employer) git/email setup for mailing list.
> 
> It's pretty common to use a different email server than what's
> on the From and S-o-b lines. You can drop the second S-o-b.

Thanks for clarification.

Should resend the series now or wait for a review?

Best regards,
Romain

Re: [PATCH 1/2] net: dsa: microchip: add KSZ9896 switch support

[Jakub Kicinski]

On Mon, 29 Aug 2022 09:20:06 +0200 Romain Naour wrote:
> > It's pretty common to use a different email server than what's
> > on the From and S-o-b lines. You can drop the second S-o-b.  
> 
> Thanks for clarification.
> 
> Should resend the series now or wait for a review?

Resend, please.

Looking quickly at the code it's interesting to see the compatible
already listed in the DT schema but the driver not supporting it. 
Is this common in the embedded world?

That's orthogonal to your patch, I'm just curious.

Re: [PATCH 1/2] net: dsa: microchip: add KSZ9896 switch support

[Romain Naour]

Hi,

Le 30/08/2022 à 07:07, Jakub Kicinski a écrit :
> On Mon, 29 Aug 2022 09:20:06 +0200 Romain Naour wrote:
>>> It's pretty common to use a different email server than what's
>>> on the From and S-o-b lines. You can drop the second S-o-b.  
>>
>> Thanks for clarification.
>>
>> Should resend the series now or wait for a review?
> 
> Resend, please.

ok.

> 
> Looking quickly at the code it's interesting to see the compatible
> already listed in the DT schema but the driver not supporting it. 
> Is this common in the embedded world?
> 
> That's orthogonal to your patch, I'm just curious.

I was surprised too about the compatible being listed in the DT schema but being
missing in the driver.

AFAIK, the DT schema was merged in 2019 (kernel 5.1) but the corresponding
driver patch didn't pass the review and no further patch was sent on the mailing
list for the KSZ9896.

It's true that the KSZ9896 is really close to the KSZ9897 (only one CPU port is
missing) but the driver reject this device due to a mismatch in the check
between the device id and the compatible. I tried to workaround this check but
the driver really need to know how many port are really present on the ethernet
switch otherwise there is a kernel oops.

Best regards,
Romain