[PATCH net-next v1 1/2] dt-bindings: net: dsa: Add KSZ8567 switch support

[PATCH net-next v1 1/2] dt-bindings: net: dsa: Add KSZ8567 switch support

[Philippe Schenker]

From: Philippe Schenker <philippe.schenker@impulsing.ch>

This commit adds the dt-binding for KSZ8567, a robust 7-port
Ethernet switch. The KSZ8567 features two RGMII/MII/RMII interfaces,
each capable of gigabit speeds, complemented by five 10/100 Mbps
MAC/PHYs.

Signed-off-by: Philippe Schenker <philippe.schenker@impulsing.ch>
---

 Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index c963dc09e8e1..52acc15ebcbf 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -31,6 +31,7 @@ properties:
       - microchip,ksz9893
       - microchip,ksz9563
       - microchip,ksz8563
+      - microchip,ksz8567
 
   reset-gpios:
     description:
-- 
2.34.1

[PATCH net-next v1 2/2] net: dsa: Add KSZ8567 switch support

[Philippe Schenker]

From: Philippe Schenker <philippe.schenker@impulsing.ch>

This commit introduces support for the KSZ8567, a robust 7-port
Ethernet switch. The KSZ8567 features two RGMII/MII/RMII interfaces,
each capable of gigabit speeds, complemented by five 10/100 Mbps
MAC/PHYs.

Signed-off-by: Philippe Schenker <philippe.schenker@impulsing.ch>

---

 drivers/net/dsa/microchip/ksz9477_i2c.c     |  4 ++
 drivers/net/dsa/microchip/ksz_common.c      | 42 ++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.h      |  1 +
 drivers/net/dsa/microchip/ksz_spi.c         |  5 +++
 include/linux/platform_data/microchip-ksz.h |  1 +
 5 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
index cac4a607e54a..82bebee4615c 100644
--- a/drivers/net/dsa/microchip/ksz9477_i2c.c
+++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
@@ -103,6 +103,10 @@ static const struct of_device_id ksz9477_dt_ids[] = {
 		.compatible = "microchip,ksz8563",
 		.data = &ksz_switch_chips[KSZ8563]
 	},
+	{
+		.compatible = "microchip,ksz8567",
+		.data = &ksz_switch_chips[KSZ8567]
+	},
 	{
 		.compatible = "microchip,ksz9567",
 		.data = &ksz_switch_chips[KSZ9567]
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 245dfb7a7a31..9b96de86dfc8 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1214,6 +1214,38 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.rd_table = &ksz8563_register_set,
 	},
 
+	[KSZ8567] = {
+		.chip_id = KSZ8567_CHIP_ID,
+		.dev_name = "KSZ8567",
+		.num_vlans = 4096,
+		.num_alus = 4096,
+		.num_statics = 16,
+		.cpu_ports = 0x7F,	/* can be configured as cpu port */
+		.port_cnt = 7,		/* total port count */
+		.port_nirqs = 3,
+		.num_tx_queues = 4,
+		.tc_cbs_supported = true,
+		.tc_ets_supported = true,
+		.ops = &ksz9477_dev_ops,
+		.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, true},
+		.supports_rmii	= {false, false, false, false,
+				   false, true, true},
+		.supports_rgmii = {false, false, false, false,
+				   false, true, true},
+		.internal_phy	= {true, true, true, true,
+				   true, false, false},
+		.gbit_capable	= {false, false, false, false, false, true, true},
+	},
+
 	[KSZ8795] = {
 		.chip_id = KSZ8795_CHIP_ID,
 		.dev_name = "KSZ8795",
@@ -2649,6 +2681,7 @@ static void ksz_port_teardown(struct dsa_switch *ds, int port)
 
 	switch (dev->chip_id) {
 	case KSZ8563_CHIP_ID:
+	case KSZ8567_CHIP_ID:
 	case KSZ9477_CHIP_ID:
 	case KSZ9563_CHIP_ID:
 	case KSZ9567_CHIP_ID:
@@ -2705,7 +2738,8 @@ static enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds,
 	    dev->chip_id == KSZ9563_CHIP_ID)
 		proto = DSA_TAG_PROTO_KSZ9893;
 
-	if (dev->chip_id == KSZ9477_CHIP_ID ||
+	if (dev->chip_id == KSZ8567_CHIP_ID ||
+	    dev->chip_id == KSZ9477_CHIP_ID ||
 	    dev->chip_id == KSZ9896_CHIP_ID ||
 	    dev->chip_id == KSZ9897_CHIP_ID ||
 	    dev->chip_id == KSZ9567_CHIP_ID)
@@ -2813,6 +2847,7 @@ static int ksz_max_mtu(struct dsa_switch *ds, int port)
 	case KSZ8830_CHIP_ID:
 		return KSZ8863_HUGE_PACKET_SIZE - VLAN_ETH_HLEN - ETH_FCS_LEN;
 	case KSZ8563_CHIP_ID:
+	case KSZ8567_CHIP_ID:
 	case KSZ9477_CHIP_ID:
 	case KSZ9563_CHIP_ID:
 	case KSZ9567_CHIP_ID:
@@ -2839,6 +2874,7 @@ static int ksz_validate_eee(struct dsa_switch *ds, int port)
 
 	switch (dev->chip_id) {
 	case KSZ8563_CHIP_ID:
+	case KSZ8567_CHIP_ID:
 	case KSZ9477_CHIP_ID:
 	case KSZ9563_CHIP_ID:
 	case KSZ9567_CHIP_ID:
@@ -3183,6 +3219,7 @@ static int ksz_switch_detect(struct ksz_device *dev)
 		case KSZ9896_CHIP_ID:
 		case KSZ9897_CHIP_ID:
 		case KSZ9567_CHIP_ID:
+		case KSZ8567_CHIP_ID:
 		case LAN9370_CHIP_ID:
 		case LAN9371_CHIP_ID:
 		case LAN9372_CHIP_ID:
@@ -3220,6 +3257,7 @@ static int ksz_cls_flower_add(struct dsa_switch *ds, int port,
 
 	switch (dev->chip_id) {
 	case KSZ8563_CHIP_ID:
+	case KSZ8567_CHIP_ID:
 	case KSZ9477_CHIP_ID:
 	case KSZ9563_CHIP_ID:
 	case KSZ9567_CHIP_ID:
@@ -3239,6 +3277,7 @@ static int ksz_cls_flower_del(struct dsa_switch *ds, int port,
 
 	switch (dev->chip_id) {
 	case KSZ8563_CHIP_ID:
+	case KSZ8567_CHIP_ID:
 	case KSZ9477_CHIP_ID:
 	case KSZ9563_CHIP_ID:
 	case KSZ9567_CHIP_ID:
@@ -4142,6 +4181,7 @@ static int ksz_parse_drive_strength(struct ksz_device *dev)
 	case KSZ8794_CHIP_ID:
 	case KSZ8765_CHIP_ID:
 	case KSZ8563_CHIP_ID:
+	case KSZ8567_CHIP_ID:
 	case KSZ9477_CHIP_ID:
 	case KSZ9563_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 15612101a155..060c5de9aa05 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -187,6 +187,7 @@ struct ksz_device {
 /* List of supported models */
 enum ksz_model {
 	KSZ8563,
+	KSZ8567,
 	KSZ8795,
 	KSZ8794,
 	KSZ8765,
diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c
index 6f6d878e742c..c8166fb440ab 100644
--- a/drivers/net/dsa/microchip/ksz_spi.c
+++ b/drivers/net/dsa/microchip/ksz_spi.c
@@ -164,6 +164,10 @@ static const struct of_device_id ksz_dt_ids[] = {
 		.compatible = "microchip,ksz8563",
 		.data = &ksz_switch_chips[KSZ8563]
 	},
+	{
+		.compatible = "microchip,ksz8567",
+		.data = &ksz_switch_chips[KSZ8567]
+	},
 	{
 		.compatible = "microchip,ksz9567",
 		.data = &ksz_switch_chips[KSZ9567]
@@ -204,6 +208,7 @@ static const struct spi_device_id ksz_spi_ids[] = {
 	{ "ksz9893" },
 	{ "ksz9563" },
 	{ "ksz8563" },
+	{ "ksz8567" },
 	{ "ksz9567" },
 	{ "lan9370" },
 	{ "lan9371" },
diff --git a/include/linux/platform_data/microchip-ksz.h b/include/linux/platform_data/microchip-ksz.h
index f177416635a2..c4466e56d9d7 100644
--- a/include/linux/platform_data/microchip-ksz.h
+++ b/include/linux/platform_data/microchip-ksz.h
@@ -24,6 +24,7 @@
 
 enum ksz_chip_id {
 	KSZ8563_CHIP_ID = 0x8563,
+	KSZ8567_CHIP_ID = 0x00856700,
 	KSZ8795_CHIP_ID = 0x8795,
 	KSZ8794_CHIP_ID = 0x8794,
 	KSZ8765_CHIP_ID = 0x8765,
-- 
2.34.1

Re: [PATCH net-next v1 2/2] net: dsa: Add KSZ8567 switch support

[Arun.Ramadoss]

Hi Philippe,

On Tue, 2024-01-23 at 14:50 +0100, Philippe Schenker wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> From: Philippe Schenker <philippe.schenker@impulsing.ch>
> 
> This commit introduces support for the KSZ8567, a robust 7-port
> Ethernet switch. The KSZ8567 features two RGMII/MII/RMII interfaces,
> each capable of gigabit speeds, complemented by five 10/100 Mbps
> MAC/PHYs.
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@impulsing.ch>

KSZ8567 switch is similar to KSZ9897/KSZ9567 except that internal phys
are without gigabit capability.
So add KSZ8567 related updates adjacent to KSZ9567. So it will be
grouped together and easier to update in case new features are added to
this switch family.

Re: [PATCH net-next v1 2/2] net: dsa: Add KSZ8567 switch support

[Philippe Schenker]

Hi Arun and thanks for your message!

On Tue, 2024-01-23 at 15:58 +0000, Arun.Ramadoss@microchip.com wrote:
> Hi Philippe,
> 
> On Tue, 2024-01-23 at 14:50 +0100, Philippe Schenker wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > From: Philippe Schenker <philippe.schenker@impulsing.ch>
> > 
> > This commit introduces support for the KSZ8567, a robust 7-port
> > Ethernet switch. The KSZ8567 features two RGMII/MII/RMII
> > interfaces,
> > each capable of gigabit speeds, complemented by five 10/100 Mbps
> > MAC/PHYs.
> > 
> > Signed-off-by: Philippe Schenker <philippe.schenker@impulsing.ch>
> 
> KSZ8567 switch is similar to KSZ9897/KSZ9567 except that internal
> phys
> are without gigabit capability.
> So add KSZ8567 related updates adjacent to KSZ9567. So it will be
> grouped together and easier to update in case new features are added
> to
> this switch family.


I actually tried sorting it kind of alphanumerically but I can put it
next to KSZ9567, will do in a v2 but wait a bit more for some feedback
to arrive.

Philippe

> 
> 

Re: [PATCH net-next v1 1/2] dt-bindings: net: dsa: Add KSZ8567 switch support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

On Tue, Jan 23, 2024 at 02:50:13PM +0100, Philippe Schenker wrote:
> From: Philippe Schenker <philippe.schenker@impulsing.ch>
> 
> This commit adds the dt-binding for KSZ8567, a robust 7-port
> Ethernet switch. The KSZ8567 features two RGMII/MII/RMII interfaces,
> each capable of gigabit speeds, complemented by five 10/100 Mbps
> MAC/PHYs.
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@impulsing.ch>

This device has all the same constraints as the other ones in this
binding, why is it not compatible with any of them? If it isn't, the
compatible should mention why it is not.

Cheers,
Conor.

> ---
> 
>  Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> index c963dc09e8e1..52acc15ebcbf 100644
> --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> @@ -31,6 +31,7 @@ properties:
>        - microchip,ksz9893
>        - microchip,ksz9563
>        - microchip,ksz8563
> +      - microchip,ksz8567
>  
>    reset-gpios:
>      description:
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v1 1/2] dt-bindings: net: dsa: Add KSZ8567 switch support

[Philippe Schenker]

On Tue, 2024-01-23 at 16:06 +0000, Conor Dooley wrote:
> On Tue, Jan 23, 2024 at 02:50:13PM +0100, Philippe Schenker wrote:
> > From: Philippe Schenker <philippe.schenker@impulsing.ch>
> > 
> > This commit adds the dt-binding for KSZ8567, a robust 7-port
> > Ethernet switch. The KSZ8567 features two RGMII/MII/RMII
> > interfaces,
> > each capable of gigabit speeds, complemented by five 10/100 Mbps
> > MAC/PHYs.
> > 
> > Signed-off-by: Philippe Schenker <philippe.schenker@impulsing.ch>
> 
> This device has all the same constraints as the other ones in this
> binding, why is it not compatible with any of them? If it isn't, the
> compatible should mention why it is not.

Hi Conor, Thanks for your message!

I need the compatible to make sure the correct ID of the switch is
being set in the driver as well as its features.

You mean I shall mention the reason in the commit-message, or where?

Philippe

> 
> Cheers,
> Conor.
> 
> > ---
> > 
> >  Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > index c963dc09e8e1..52acc15ebcbf 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > @@ -31,6 +31,7 @@ properties:
> >        - microchip,ksz9893
> >        - microchip,ksz9563
> >        - microchip,ksz8563
> > +      - microchip,ksz8567
> >  
> >    reset-gpios:
> >      description:
> > -- 
> > 2.34.1
> > 

Re: [PATCH net-next v1 1/2] dt-bindings: net: dsa: Add KSZ8567 switch support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2049 bytes --]

On Tue, Jan 23, 2024 at 05:17:53PM +0100, Philippe Schenker wrote:
> 
> 
> On Tue, 2024-01-23 at 16:06 +0000, Conor Dooley wrote:
> > On Tue, Jan 23, 2024 at 02:50:13PM +0100, Philippe Schenker wrote:
> > > From: Philippe Schenker <philippe.schenker@impulsing.ch>
> > > 
> > > This commit adds the dt-binding for KSZ8567, a robust 7-port
> > > Ethernet switch. The KSZ8567 features two RGMII/MII/RMII
> > > interfaces,
> > > each capable of gigabit speeds, complemented by five 10/100 Mbps
> > > MAC/PHYs.
> > > 
> > > Signed-off-by: Philippe Schenker <philippe.schenker@impulsing.ch>
> > 
> > This device has all the same constraints as the other ones in this
> > binding, why is it not compatible with any of them? If it isn't, the
> > compatible should mention why it is not.
> 
> Hi Conor, Thanks for your message!
> 
> I need the compatible to make sure the correct ID of the switch is
> being set in the driver as well as its features.

Are the features of this switch such that a driver for another ksz
switch would not work (even in a limited capacity) with the 8567?
Things like the register map changing or some feature being removed are
examples of why it may not work.

> You mean I shall mention the reason in the commit-message, or where?

Yes.

Thanks,
Conor

> > >  Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > index c963dc09e8e1..52acc15ebcbf 100644
> > > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > @@ -31,6 +31,7 @@ properties:
> > >        - microchip,ksz9893
> > >        - microchip,ksz9563
> > >        - microchip,ksz8563
> > > +      - microchip,ksz8567
> > >  
> > >    reset-gpios:
> > >      description:
> > > -- 
> > > 2.34.1
> > > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v1 1/2] dt-bindings: net: dsa: Add KSZ8567 switch support

[Philippe Schenker]

On Tue, 2024-01-23 at 17:23 +0000, Conor Dooley wrote:
> On Tue, Jan 23, 2024 at 05:17:53PM +0100, Philippe Schenker wrote:
> > 
> > 
> > On Tue, 2024-01-23 at 16:06 +0000, Conor Dooley wrote:
> > > On Tue, Jan 23, 2024 at 02:50:13PM +0100, Philippe Schenker
> > > wrote:
> > > > From: Philippe Schenker <philippe.schenker@impulsing.ch>
> > > > 
> > > > This commit adds the dt-binding for KSZ8567, a robust 7-port
> > > > Ethernet switch. The KSZ8567 features two RGMII/MII/RMII
> > > > interfaces,
> > > > each capable of gigabit speeds, complemented by five 10/100
> > > > Mbps
> > > > MAC/PHYs.
> > > > 
> > > > Signed-off-by: Philippe Schenker
> > > > <philippe.schenker@impulsing.ch>
> > > 
> > > This device has all the same constraints as the other ones in
> > > this
> > > binding, why is it not compatible with any of them? If it isn't,
> > > the
> > > compatible should mention why it is not.
> > 
> > Hi Conor, Thanks for your message!
> > 
> > I need the compatible to make sure the correct ID of the switch is
> > being set in the driver as well as its features.
> 
> Are the features of this switch such that a driver for another ksz
> switch would not work (even in a limited capacity) with the 8567?
> Things like the register map changing or some feature being removed
> are
> examples of why it may not work.

Yes the ksz dsa driver is made so that it checks the ID of the attached
chip and refuses to work if it doesn't match. [1]
It is a very similar chip and uses the same regmap as KSZ9567 but with
lower phy-speeds on its 5 switch ports. The two upstream CPU ports are
gigabit capable. All this information is set-up in the second patch of
this series. [2]

I will include a description to the second series. Thanks for your
feedback.

Philippe


[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/microchip/ksz_common.c?h=v6.8-rc1#n3181
[2]
https://patchwork.kernel.org/project/netdevbpf/patch/20240123135014.614858-2-dev@pschenker.ch/

> 
> > You mean I shall mention the reason in the commit-message, or
> > where?
> 
> Yes.
> 
> Thanks,
> Conor
> 
> > > >  Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml |
> > > > 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > > b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > > index c963dc09e8e1..52acc15ebcbf 100644
> > > > ---
> > > > a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > > +++
> > > > b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > > @@ -31,6 +31,7 @@ properties:
> > > >        - microchip,ksz9893
> > > >        - microchip,ksz9563
> > > >        - microchip,ksz8563
> > > > +      - microchip,ksz8567
> > > >  
> > > >    reset-gpios:
> > > >      description:
> > > > -- 
> > > > 2.34.1
> > > > 

Re: [PATCH net-next v1 1/2] dt-bindings: net: dsa: Add KSZ8567 switch support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3816 bytes --]

On Tue, Jan 23, 2024 at 06:30:16PM +0100, Philippe Schenker wrote:
> 
> 
> On Tue, 2024-01-23 at 17:23 +0000, Conor Dooley wrote:
> > On Tue, Jan 23, 2024 at 05:17:53PM +0100, Philippe Schenker wrote:
> > > 
> > > 
> > > On Tue, 2024-01-23 at 16:06 +0000, Conor Dooley wrote:
> > > > On Tue, Jan 23, 2024 at 02:50:13PM +0100, Philippe Schenker
> > > > wrote:
> > > > > From: Philippe Schenker <philippe.schenker@impulsing.ch>
> > > > > 
> > > > > This commit adds the dt-binding for KSZ8567, a robust 7-port
> > > > > Ethernet switch. The KSZ8567 features two RGMII/MII/RMII
> > > > > interfaces,
> > > > > each capable of gigabit speeds, complemented by five 10/100
> > > > > Mbps
> > > > > MAC/PHYs.
> > > > > 
> > > > > Signed-off-by: Philippe Schenker
> > > > > <philippe.schenker@impulsing.ch>
> > > > 
> > > > This device has all the same constraints as the other ones in
> > > > this
> > > > binding, why is it not compatible with any of them? If it isn't,
> > > > the
> > > > compatible should mention why it is not.
> > > 
> > > Hi Conor, Thanks for your message!
> > > 
> > > I need the compatible to make sure the correct ID of the switch is
> > > being set in the driver as well as its features.
> > 
> > Are the features of this switch such that a driver for another ksz
> > switch would not work (even in a limited capacity) with the 8567?
> > Things like the register map changing or some feature being removed
> > are
> > examples of why it may not work.
> 
> Yes the ksz dsa driver is made so that it checks the ID of the attached
> chip and refuses to work if it doesn't match. [1]

That sounds counter productive to be honest. Why does the driver not
trust that the dt is correct? I saw this recently in some IIO drivers,
but it was shot down for this sort of reason.

> It is a very similar chip and uses the same regmap as KSZ9567 but with
> lower phy-speeds on its 5 switch ports. The two upstream CPU ports are
> gigabit capable. All this information is set-up in the second patch of
> this series. [2]

That, to me, means the lack of a fallback is justified. If it were the
other way around, then a fallback sounds like it would be suitable.

> 
> I will include a description to the second series. Thanks for your
> feedback.

Okay, thanks. You can add
Acked-by: Conor Dooley <conor.dooley@microchip.com>
when you do.

And despite the email, I have nothing to do with these switches, I am
just a sucker that signed up to review dt-bindings...

Thanks,
Conor.

> 
> Philippe
> 
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/microchip/ksz_common.c?h=v6.8-rc1#n3181
> [2]
> https://patchwork.kernel.org/project/netdevbpf/patch/20240123135014.614858-2-dev@pschenker.ch/
> 
> > 
> > > You mean I shall mention the reason in the commit-message, or
> > > where?
> > 
> > Yes.
> > 
> > Thanks,
> > Conor
> > 
> > > > >  Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml |
> > > > > 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > > > b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > > > index c963dc09e8e1..52acc15ebcbf 100644
> > > > > ---
> > > > > a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > > > +++
> > > > > b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > > > @@ -31,6 +31,7 @@ properties:
> > > > >        - microchip,ksz9893
> > > > >        - microchip,ksz9563
> > > > >        - microchip,ksz8563
> > > > > +      - microchip,ksz8567
> > > > >  
> > > > >    reset-gpios:
> > > > >      description:
> > > > > -- 
> > > > > 2.34.1
> > > > > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v1 1/2] dt-bindings: net: dsa: Add KSZ8567 switch support

[Philippe Schenker]

On Tue, 2024-01-23 at 18:37 +0000, Conor Dooley wrote:
> On Tue, Jan 23, 2024 at 06:30:16PM +0100, Philippe Schenker wrote:
> > 
> > 
> > On Tue, 2024-01-23 at 17:23 +0000, Conor Dooley wrote:
> > > On Tue, Jan 23, 2024 at 05:17:53PM +0100, Philippe Schenker wrote:
> > > > 
> > > > 
> > > > On Tue, 2024-01-23 at 16:06 +0000, Conor Dooley wrote:
> > > > > On Tue, Jan 23, 2024 at 02:50:13PM +0100, Philippe Schenker
> > > > > wrote:
> > > > > > From: Philippe Schenker <philippe.schenker@impulsing.ch>
> > > > > > 
> > > > > > This commit adds the dt-binding for KSZ8567, a robust 7-port
> > > > > > Ethernet switch. The KSZ8567 features two RGMII/MII/RMII
> > > > > > interfaces,
> > > > > > each capable of gigabit speeds, complemented by five 10/100
> > > > > > Mbps
> > > > > > MAC/PHYs.
> > > > > > 
> > > > > > Signed-off-by: Philippe Schenker
> > > > > > <philippe.schenker@impulsing.ch>
> > > > > 
> > > > > This device has all the same constraints as the other ones in
> > > > > this
> > > > > binding, why is it not compatible with any of them? If it
> > > > > isn't,
> > > > > the
> > > > > compatible should mention why it is not.
> > > > 
> > > > Hi Conor, Thanks for your message!
> > > > 
> > > > I need the compatible to make sure the correct ID of the switch
> > > > is
> > > > being set in the driver as well as its features.
> > > 
> > > Are the features of this switch such that a driver for another ksz
> > > switch would not work (even in a limited capacity) with the 8567?
> > > Things like the register map changing or some feature being
> > > removed
> > > are
> > > examples of why it may not work.
> > 
> > Yes the ksz dsa driver is made so that it checks the ID of the
> > attached
> > chip and refuses to work if it doesn't match. [1]
> 
> That sounds counter productive to be honest. Why does the driver not
> trust that the dt is correct? I saw this recently in some IIO drivers,
> but it was shot down for this sort of reason.
> 
> > It is a very similar chip and uses the same regmap as KSZ9567 but
> > with
> > lower phy-speeds on its 5 switch ports. The two upstream CPU ports
> > are
> > gigabit capable. All this information is set-up in the second patch
> > of
> > this series. [2]
> 
> That, to me, means the lack of a fallback is justified. If it were the
> other way around, then a fallback sounds like it would be suitable.

Yes, I believe a generic fallback would anyway mostly end up with errors
or misbehavior. But I also might be wrong. What I checked is that these
per-switch settings even increased from 5.15..6.7 which to me it seems
those are really needed.
I am no expert in this driver at all I just try to make my hardware work
and with these patches it now seems to switch really well.

> 
> > 
> > I will include a description to the second series. Thanks for your
> > feedback.
> 
> Okay, thanks. You can add
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> when you do.
> 
> And despite the email, I have nothing to do with these switches, I am
> just a sucker that signed up to review dt-bindings...

Well, I'm grateful for your review and feedback. Thanks for your work
around Linux kernel! And thanks for the Acked-by.

Philippe

> 
> Thanks,
> Conor.
> 
> > 
> > Philippe
> > 
> > 
> > [1]
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/microchip/ksz_common.c?h=v6.8-rc1#n3181
> > [2]
> > https://patchwork.kernel.org/project/netdevbpf/patch/20240123135014.614858-2-dev@pschenker.ch/
> > 
> > > 
> > > > You mean I shall mention the reason in the commit-message, or
> > > > where?
> > > 
> > > Yes.
> > > 
> > > Thanks,
> > > Conor
> > > 
> > > > > >  Documentation/devicetree/bindings/net/dsa/microchip,ksz.yam
> > > > > > l |
> > > > > > 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > > 
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.ya
> > > > > > ml
> > > > > > b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.ya
> > > > > > ml
> > > > > > index c963dc09e8e1..52acc15ebcbf 100644
> > > > > > ---
> > > > > > a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.ya
> > > > > > ml
> > > > > > +++
> > > > > > b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.ya
> > > > > > ml
> > > > > > @@ -31,6 +31,7 @@ properties:
> > > > > >        - microchip,ksz9893
> > > > > >        - microchip,ksz9563
> > > > > >        - microchip,ksz8563
> > > > > > +      - microchip,ksz8567
> > > > > >  
> > > > > >    reset-gpios:
> > > > > >      description:
> > > > > > -- 
> > > > > > 2.34.1
> > > > > > 

Re: [PATCH net-next v1 1/2] dt-bindings: net: dsa: Add KSZ8567 switch support

[Andrew Lunn]

> That sounds counter productive to be honest. Why does the driver not
> trust that the dt is correct? I saw this recently in some IIO drivers,
> but it was shot down for this sort of reason.

DT is software, therefore it contains bugs.

Say we ignore that the compatible does not match the hardware on the
board and just accept the DT has a bug in it and keep going.

That then makes the compatible pointless, and unusable for anything,
since there are boards out in the wild with incorrect compatibles. If
we later actually use the compatible for something, it might cause
regressions for those buggy DT blobs.

By erroring out then the compatible does not match the hardware avoids
such bugs.

The marvell mv88e6xxx driver takes a different approach. All the
compatible does is tell the driver where to find the ID
register. Marvell keeps moving it around, so there are three different
compatibles for the three different locations. If you use the wrong
compatible, its not going to find a device is knows about and errors
out. So this also avoids bugs in the compatible.

     Andrew

Re: [PATCH net-next v1 1/2] dt-bindings: net: dsa: Add KSZ8567 switch support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1776 bytes --]

On Wed, Jan 24, 2024 at 07:08:29PM +0100, Andrew Lunn wrote:
> > That sounds counter productive to be honest. Why does the driver not
> > trust that the dt is correct? I saw this recently in some IIO drivers,
> > but it was shot down for this sort of reason.
> 
> DT is software, therefore it contains bugs.
> 
> Say we ignore that the compatible does not match the hardware on the
> board and just accept the DT has a bug in it and keep going.
> 
> That then makes the compatible pointless, and unusable for anything,
> since there are boards out in the wild with incorrect compatibles. If
> we later actually use the compatible for something, it might cause
> regressions for those buggy DT blobs.
> 
> By erroring out then the compatible does not match the hardware avoids
> such bugs.

It also makes fallback compatibles useless, which is what I see as being
counter productive, since you'll have to add support to the driver even
if (other than the id) the change is imperceptible to software.
If you have your reasons why you do not trust the compatibles for these
devices, then it is your prerogative as a driver author to cross check it
and fail if they don't match.

That said, it does not prevent the fallback being accurately described
in the binding itself, which at the end of the day is what I am more
interested it.

> The marvell mv88e6xxx driver takes a different approach. All the
> compatible does is tell the driver where to find the ID
> register. Marvell keeps moving it around, so there are three different
> compatibles for the three different locations. If you use the wrong
> compatible, its not going to find a device is knows about and errors
> out. So this also avoids bugs in the compatible.
> 
>      Andrew

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v1 1/2] dt-bindings: net: dsa: Add KSZ8567 switch support

[Vladimir Oltean]

Hi Conor,

On Tue, Jan 23, 2024 at 06:37:55PM +0000, Conor Dooley wrote:
> On Tue, Jan 23, 2024 at 06:30:16PM +0100, Philippe Schenker wrote:
> > > > Hi Conor, Thanks for your message!
> > > > 
> > > > I need the compatible to make sure the correct ID of the switch is
> > > > being set in the driver as well as its features.
> > > 
> > > Are the features of this switch such that a driver for another ksz
> > > switch would not work (even in a limited capacity) with the 8567?
> > > Things like the register map changing or some feature being removed
> > > are
> > > examples of why it may not work.
> > 
> > Yes the ksz dsa driver is made so that it checks the ID of the attached
> > chip and refuses to work if it doesn't match. [1]
> 
> That sounds counter productive to be honest. Why does the driver not
> trust that the dt is correct? I saw this recently in some IIO drivers,
> but it was shot down for this sort of reason.

If the hardware provides device ID registers, what is the best practice
in reconciling them with the compatible string?

I see 2 extreme cases. Ethernet PHY devices seem to blindly trust the
PHY ID from the "ethernet-phy-idXXXX.XXXX" compatible string, and phylib
won't read the PHY ID from the standard MDIO registers when this is present.
Whereas PCI seems to completely disregard the vendor ID and device ID
from the "pciXXXX,XXXX" compatible string of function OF nodes. Both
these subsystems have the "compatible" string optional.

I could see an advantage in using a precise device ID in the compatible
string, rather than just one from which the _location_ of the device ID
can be deduced, because the precise compatible string allows for much
finer grained static analysis in the schema for the device. In case of a
switch, that is the number of ports, supported interface modes for each
port, etc. With an imprecise device ID, you don't know what you really
get until you boot the board.

> And despite the email, I have nothing to do with these switches, I am
> just a sucker that signed up to review dt-bindings...

Same here, just a sucker with some switches and not many clues about
other things :)

Re: [PATCH net-next v1 1/2] dt-bindings: net: dsa: Add KSZ8567 switch support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4373 bytes --]

On Thu, Jan 25, 2024 at 11:57:19AM +0200, Vladimir Oltean wrote:
> Hi Conor,
> 
> On Tue, Jan 23, 2024 at 06:37:55PM +0000, Conor Dooley wrote:
> > On Tue, Jan 23, 2024 at 06:30:16PM +0100, Philippe Schenker wrote:
> > > > > Hi Conor, Thanks for your message!
> > > > > 
> > > > > I need the compatible to make sure the correct ID of the switch is
> > > > > being set in the driver as well as its features.
> > > > 
> > > > Are the features of this switch such that a driver for another ksz
> > > > switch would not work (even in a limited capacity) with the 8567?
> > > > Things like the register map changing or some feature being removed
> > > > are
> > > > examples of why it may not work.
> > > 
> > > Yes the ksz dsa driver is made so that it checks the ID of the attached
> > > chip and refuses to work if it doesn't match. [1]
> > 
> > That sounds counter productive to be honest. Why does the driver not
> > trust that the dt is correct? I saw this recently in some IIO drivers,
> > but it was shot down for this sort of reason.
> 
> If the hardware provides device ID registers, what is the best practice
> in reconciling them with the compatible string?
> 
> I see 2 extreme cases. Ethernet PHY devices seem to blindly trust the
> PHY ID from the "ethernet-phy-idXXXX.XXXX" compatible string, and phylib
> won't read the PHY ID from the standard MDIO registers when this is present.
> Whereas PCI seems to completely disregard the vendor ID and device ID
> from the "pciXXXX,XXXX" compatible string of function OF nodes. Both
> these subsystems have the "compatible" string optional.

The software is free to do either, even if the compatible is a mandatory
property. Which is the right thing to do varies though. In your first
example, the compatible might be used because the standard MDIO
registers do not provide that function for whatever reason and therefore
cannot be read. Or there might be some shenzen market ripoff of another
device that it claims to be in its ID registers but has some
incompatible difference in the programming model.

In general though, if a device can self identify, I would be inclined to
value the self identification over that provided by the devicetree.

My comments here were because of the default in the switch statement
that rejects probe if the ID is unknown as it excludes the use of
fallback compatibles.

In this particular case, and maybe some historical reasons prevent this
that I am unaware of, I would read the ID of the device and if that is
an ID that the driver is aware of, treat that as the truth. As far as I
can tell from my quick look, the driver does this.

The first potential mismatch then is where the ID is, but does not match
what the DT claims the device is. As I said, I'd be inclined to trust
the self identification here, but I can at least understand rejection of
probe. Again from a quick look, the driver seems to trust the self
identification.

If the driver don't recognise the ID then I would treat the device as if
it is what the match data says it is, so that fallback compatibles would
work. This is the case that this driver does not allow.

> I could see an advantage in using a precise device ID in the compatible
> string, rather than just one from which the _location_ of the device ID
> can be deduced, because the precise compatible string allows for much
> finer grained static analysis in the schema for the device. In case of a
> switch, that is the number of ports, supported interface modes for each
> port, etc. With an imprecise device ID, you don't know what you really
> get until you boot the board.

That is true, I think it is pretty desirable (although admittedly I am
rather biased) to add specific compatibles for validation reasons even
if the drivers make no use of them. Generally new bindings for such
cases require a fair bit of justification, but its harder to stop the
proliferation of existing cases, without retrofitting specific
compatibles to the dts users (in addition to the generic one), and
disallowing the generic compatible in isolation.

> > And despite the email, I have nothing to do with these switches, I am
> > just a sucker that signed up to review dt-bindings...
> 
> Same here, just a sucker with some switches and not many clues about
> other things :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]