[RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
diff mbox series

Message ID 93AF473E2DA327428DE3D46B72B1E9FD41121A5B@CHN-SV-EXMX02.mchp-main.com
State New, archived
Headers show
Series
  • [RFC] Update documentation for KSZ DSA drivers so that new drivers can be added
Related show

Commit Message

Tristram.Ha@microchip.com Sept. 7, 2017, 9:11 p.m. UTC
From: Tristram Ha <Tristram.Ha@microchip.com>

Add other KSZ switches support so that patch check does not complain.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 Documentation/devicetree/bindings/net/dsa/ksz.txt | 117 ++++++++++++----------
 1 file changed, 62 insertions(+), 55 deletions(-)

 
 Ethernet switch connected via SPI to the host, CPU port wired to eth0:
 
-                             eth0: ethernet@10001000 {
-                                             fixed-link {
-                                                             speed = <1000>;
-                                                             full-duplex;
-                                             };
-                             };
+	eth0: ethernet@10001000 {
+		fixed-link {
+			speed = <1000>;
+			full-duplex;
+		};
+	};
 
-                             spi1: spi@f8008000 {
-                                             pinctrl-0 = <&pinctrl_spi_ksz>;
-                                             cs-gpios = <&pioC 25 0>;
-                                             id = <1>;
-                                             status = "okay";
+	spi1: spi@f8008000 {
+		cs-gpios = <&pioC 25 0>;
+		id = <1>;
+		status = "okay";
 
-                                             ksz9477: ksz9477@0 {
-                                                             compatible = "microchip,ksz9477";
-                                                             reg = <0>;
+		ksz9477: ksz9477@0 {
+			compatible = "microchip,ksz9477";
+			reg = <0>;
 
-                                                             spi-max-frequency = <44000000>;
-                                                             spi-cpha;
-                                                             spi-cpol;
+			spi-max-frequency = <44000000>;
+			spi-cpha;
+			spi-cpol;
+
+			status = "okay";
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				port@0 {
+					reg = <0>;
+					label = "lan1";
+				};
+				port@1 {
+					reg = <1>;
+					label = "lan2";
+				};
+				port@2 {
+					reg = <2>;
+					label = "lan3";
+				};
+				port@3 {
+					reg = <3>;
+					label = "lan4";
+				};
+				port@4 {
+					reg = <4>;
+					label = "lan5";
+				};
+				port@5 {
+					reg = <5>;
+					label = "cpu";
+					ethernet = <&eth0>;
+					fixed-link {
+						speed = <1000>;
+						full-duplex;
+					};
+				};
+			};
+		};
+	};
 
-                                                             status = "okay";
-                                                             ports {
-                                                                             #address-cells = <1>;
-                                                                             #size-cells = <0>;
-                                                                             port@0 {
-                                                                                             reg = <0>;
-                                                                                             label = "lan1";
-                                                                             };
-                                                                             port@1 {
-                                                                                             reg = <1>;
-                                                                                             label = "lan2";
-                                                                             };
-                                                                             port@2 {
-                                                                                             reg = <2>;
-                                                                                             label = "lan3";
-                                                                             };
-                                                                             port@3 {
-                                                                                             reg = <3>;
-                                                                                             label = "lan4";
-                                                                             };
-                                                                             port@4 {
-                                                                                             reg = <4>;
-                                                                                             label = "lan5";
-                                                                             };
-                                                                             port@5 {
-                                                                                             reg = <5>;
-                                                                                             label = "cpu";
-                                                                                             ethernet = <&eth0>;
-                                                                                             fixed-link {
-                                                                                                             speed = <1000>;
-                                                                                                             full-duplex;
-                                                                                             };
-                                                                             };
-                                                             };
-                                             };
-                             };

Comments

Andrew Lunn Sept. 7, 2017, 9:54 p.m. UTC | #1
> -- compatible: For external switch chips, compatible string must be exactly one
> -  of: "microchip,ksz9477"
> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> +	      "microchip,ksz8795" for KSZ8795 chip,
> +	      "microchip,ksz8794" for KSZ8794 chip,
> +	      "microchip,ksz8765" for KSZ8765 chip,
> +	      "microchip,ksz8895" for KSZ8895 chip,
> +	      "microchip,ksz8864" for KSZ8864 chip,
> +	      "microchip,ksz8873" for KSZ8873 chip,
> +	      "microchip,ksz8863" for KSZ8863 chip,
> +	      "microchip,ksz8463" for KSZ8463 chip

This part of this patch should be in a patch of the series that
actually adds support for these chips. Don't document chips until you
actually support them.

>  See Documentation/devicetree/bindings/dsa/dsa.txt for a list of additional  required and optional properties.
> @@ -13,60 +20,60 @@ Examples:
>  
>  Ethernet switch connected via SPI to the host, CPU port wired to eth0:
>  
> -                             eth0: ethernet@10001000 {
> -                                             fixed-link {
> -                                                             speed = <1000>;
> -                                                             full-duplex;
> -                                             };
> -                             };
> +	eth0: ethernet@10001000 {
> +		fixed-link {
> +			speed = <1000>;
> +			full-duplex;
> +		};
> +	};
>  
> -                             spi1: spi@f8008000 {
> -                                             pinctrl-0 = <&pinctrl_spi_ksz>;
> -                                             cs-gpios = <&pioC 25 0>;
> -                                             id = <1>;
> -                                             status = "okay";
> +	spi1: spi@f8008000 {
> +		cs-gpios = <&pioC 25 0>;
> +		id = <1>;
> +		status = "okay";
>  
> -                                             ksz9477: ksz9477@0 {
> -                                                             compatible = "microchip,ksz9477";
> -                                                             reg = <0>;
> +		ksz9477: ksz9477@0 {
> +			compatible = "microchip,ksz9477";
> +			reg = <0>;
>  
> -                                                             spi-max-frequency = <44000000>;
> -                                                             spi-cpha;
> -                                                             spi-cpol;
> +			spi-max-frequency = <44000000>;
> +			spi-cpha;
> +			spi-cpol;
> +
> +			status = "okay";
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				port@0 {
> +					reg = <0>;
> +					label = "lan1";
> +				};
> +				port@1 {
> +					reg = <1>;
> +					label = "lan2";
> +				};
> +				port@2 {
> +					reg = <2>;
> +					label = "lan3";
> +				};
> +				port@3 {
> +					reg = <3>;
> +					label = "lan4";
> +				};
> +				port@4 {
> +					reg = <4>;
> +					label = "lan5";
> +				};
> +				port@5 {
> +					reg = <5>;
> +					label = "cpu";
> +					ethernet = <&eth0>;
> +					fixed-link {
> +						speed = <1000>;
> +						full-duplex;
> +					};
> +				};
> +			};
> +		};
> +	};
>  
> -                                                             status = "okay";
> -                                                             ports {
> -                                                                             #address-cells = <1>;
> -                                                                             #size-cells = <0>;
> -                                                                             port@0 {
> -                                                                                             reg = <0>;
> -                                                                                             label = "lan1";
> -                                                                             };
> -                                                                             port@1 {
> -                                                                                             reg = <1>;
> -                                                                                             label = "lan2";
> -                                                                             };
> -                                                                             port@2 {
> -                                                                                             reg = <2>;
> -                                                                                             label = "lan3";
> -                                                                             };
> -                                                                             port@3 {
> -                                                                                             reg = <3>;
> -                                                                                             label = "lan4";
> -                                                                             };
> -                                                                             port@4 {
> -                                                                                             reg = <4>;
> -                                                                                             label = "lan5";
> -                                                                             };
> -                                                                             port@5 {
> -                                                                                             reg = <5>;
> -                                                                                             label = "cpu";
> -                                                                                             ethernet = <&eth0>;
> -                                                                                             fixed-link {
> -                                                                                                             speed = <1000>;
> -                                                                                                             full-duplex;
> -                                                                                             };
> -                                                                             };
> -                                                             };
> -                                             };
> -                             };

This part however is a nice cleanup. You can submit this patch as a
separate patch, once netdev has opened again in about 10 days time.

	 Andrew
Pavel Machek Sept. 8, 2017, 9:04 a.m. UTC | #2
On Thu 2017-09-07 21:11:45, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Add other KSZ switches support so that patch check does not complain.

Nice; unlike Andrew I believe that adding compatible strings is ok
before the support is added. Device tree description is
Linux-independend.

But... you need to Cc: device tree maintainers on this one. They
usually take some time to respond, so it is nice to do this early.

> index 0ab8b39..34af0e0 100644
> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
>  
>  Required properties:
>  
> -- compatible: For external switch chips, compatible string must be exactly one
> -  of: "microchip,ksz9477"
> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> +	      "microchip,ksz8795" for KSZ8795 chip,
> +	      "microchip,ksz8794" for KSZ8794 chip,
> +	      "microchip,ksz8765" for KSZ8765 chip,
> +	      "microchip,ksz8895" for KSZ8895 chip,
> +	      "microchip,ksz8864" for KSZ8864 chip,
> +	      "microchip,ksz8873" for KSZ8873 chip,
> +	      "microchip,ksz8863" for KSZ8863 chip,
> +	      "microchip,ksz8463" for KSZ8463 chip
>  
>  See Documentation/devicetree/bindings/dsa/dsa.txt for a list of additional  required and optional properties.

And you may want to wrap long line here, unless that's email client
playing another trick.
								Pavel
Maxim Uvarov Sept. 8, 2017, 1:32 p.m. UTC | #3
2017-09-08 0:54 GMT+03:00 Andrew Lunn <andrew@lunn.ch>:
>> -- compatible: For external switch chips, compatible string must be exactly one
>> -  of: "microchip,ksz9477"
>> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
>> +           "microchip,ksz8795" for KSZ8795 chip,
>> +           "microchip,ksz8794" for KSZ8794 chip,
>> +           "microchip,ksz8765" for KSZ8765 chip,
>> +           "microchip,ksz8895" for KSZ8895 chip,
>> +           "microchip,ksz8864" for KSZ8864 chip,
>> +           "microchip,ksz8873" for KSZ8873 chip,
>> +           "microchip,ksz8863" for KSZ8863 chip,
>> +           "microchip,ksz8463" for KSZ8463 chip
>

all that chips have the same spi access to get chip id on probe(). I
prefer common microship,ksz-spi rather than somebody will always
maintain that list.

Maxim.

> This part of this patch should be in a patch of the series that
> actually adds support for these chips. Don't document chips until you
> actually support them.
>
>>  See Documentation/devicetree/bindings/dsa/dsa.txt for a list of additional  required and optional properties.
>> @@ -13,60 +20,60 @@ Examples:
>>
>>  Ethernet switch connected via SPI to the host, CPU port wired to eth0:
>>
>> -                             eth0: ethernet@10001000 {
>> -                                             fixed-link {
>> -                                                             speed = <1000>;
>> -                                                             full-duplex;
>> -                                             };
>> -                             };
>> +     eth0: ethernet@10001000 {
>> +             fixed-link {
>> +                     speed = <1000>;
>> +                     full-duplex;
>> +             };
>> +     };
>>
>> -                             spi1: spi@f8008000 {
>> -                                             pinctrl-0 = <&pinctrl_spi_ksz>;
>> -                                             cs-gpios = <&pioC 25 0>;
>> -                                             id = <1>;
>> -                                             status = "okay";
>> +     spi1: spi@f8008000 {
>> +             cs-gpios = <&pioC 25 0>;
>> +             id = <1>;
>> +             status = "okay";
>>
>> -                                             ksz9477: ksz9477@0 {
>> -                                                             compatible = "microchip,ksz9477";
>> -                                                             reg = <0>;
>> +             ksz9477: ksz9477@0 {
>> +                     compatible = "microchip,ksz9477";
>> +                     reg = <0>;
>>
>> -                                                             spi-max-frequency = <44000000>;
>> -                                                             spi-cpha;
>> -                                                             spi-cpol;
>> +                     spi-max-frequency = <44000000>;
>> +                     spi-cpha;
>> +                     spi-cpol;
>> +
>> +                     status = "okay";
>> +                     ports {
>> +                             #address-cells = <1>;
>> +                             #size-cells = <0>;
>> +                             port@0 {
>> +                                     reg = <0>;
>> +                                     label = "lan1";
>> +                             };
>> +                             port@1 {
>> +                                     reg = <1>;
>> +                                     label = "lan2";
>> +                             };
>> +                             port@2 {
>> +                                     reg = <2>;
>> +                                     label = "lan3";
>> +                             };
>> +                             port@3 {
>> +                                     reg = <3>;
>> +                                     label = "lan4";
>> +                             };
>> +                             port@4 {
>> +                                     reg = <4>;
>> +                                     label = "lan5";
>> +                             };
>> +                             port@5 {
>> +                                     reg = <5>;
>> +                                     label = "cpu";
>> +                                     ethernet = <&eth0>;
>> +                                     fixed-link {
>> +                                             speed = <1000>;
>> +                                             full-duplex;
>> +                                     };
>> +                             };
>> +                     };
>> +             };
>> +     };
>>
>> -                                                             status = "okay";
>> -                                                             ports {
>> -                                                                             #address-cells = <1>;
>> -                                                                             #size-cells = <0>;
>> -                                                                             port@0 {
>> -                                                                                             reg = <0>;
>> -                                                                                             label = "lan1";
>> -                                                                             };
>> -                                                                             port@1 {
>> -                                                                                             reg = <1>;
>> -                                                                                             label = "lan2";
>> -                                                                             };
>> -                                                                             port@2 {
>> -                                                                                             reg = <2>;
>> -                                                                                             label = "lan3";
>> -                                                                             };
>> -                                                                             port@3 {
>> -                                                                                             reg = <3>;
>> -                                                                                             label = "lan4";
>> -                                                                             };
>> -                                                                             port@4 {
>> -                                                                                             reg = <4>;
>> -                                                                                             label = "lan5";
>> -                                                                             };
>> -                                                                             port@5 {
>> -                                                                                             reg = <5>;
>> -                                                                                             label = "cpu";
>> -                                                                                             ethernet = <&eth0>;
>> -                                                                                             fixed-link {
>> -                                                                                                             speed = <1000>;
>> -                                                                                                             full-duplex;
>> -                                                                                             };
>> -                                                                             };
>> -                                                             };
>> -                                             };
>> -                             };
>
> This part however is a nice cleanup. You can submit this patch as a
> separate patch, once netdev has opened again in about 10 days time.
>
>          Andrew
Andrew Lunn Sept. 8, 2017, 2:12 p.m. UTC | #4
On Fri, Sep 08, 2017 at 04:32:35PM +0300, Maxim Uvarov wrote:
> 2017-09-08 0:54 GMT+03:00 Andrew Lunn <andrew@lunn.ch>:
> >> -- compatible: For external switch chips, compatible string must be exactly one
> >> -  of: "microchip,ksz9477"
> >> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> >> +           "microchip,ksz8795" for KSZ8795 chip,
> >> +           "microchip,ksz8794" for KSZ8794 chip,
> >> +           "microchip,ksz8765" for KSZ8765 chip,
> >> +           "microchip,ksz8895" for KSZ8895 chip,
> >> +           "microchip,ksz8864" for KSZ8864 chip,
> >> +           "microchip,ksz8873" for KSZ8873 chip,
> >> +           "microchip,ksz8863" for KSZ8863 chip,
> >> +           "microchip,ksz8463" for KSZ8463 chip
> >
> 
> all that chips have the same spi access to get chip id on probe(). I
> prefer common microship,ksz-spi rather than somebody will always
> maintain that list.

The Marvell DSA driver is similar. The compatibility string tells you
enough to go find the switch ID in the switch itself.

I suppose this comes down to, is there going to be one SPI driver for
all the devices, or lots of drivers? In general, DSA has one driver
for lots of devices. The mv88e6xxx supports around 25 devices. The b53
has around 17, etc.

So i would suggest one driver supporting all the different devices.

   Andrew
Tristram.Ha@microchip.com Sept. 8, 2017, 6:40 p.m. UTC | #5
> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Friday, September 08, 2017 7:12 AM
> To: Maxim Uvarov
> Cc: Tristram Ha - C24268; Pavel Machek; Nathan Conrad; Vivien Didelot; Florian
> Fainelli; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
> drivers can be added
> 
> On Fri, Sep 08, 2017 at 04:32:35PM +0300, Maxim Uvarov wrote:
> > 2017-09-08 0:54 GMT+03:00 Andrew Lunn <andrew@lunn.ch>:
> > >> -- compatible: For external switch chips, compatible string must be exactly
> one
> > >> -  of: "microchip,ksz9477"
> > >> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> > >> +           "microchip,ksz8795" for KSZ8795 chip,
> > >> +           "microchip,ksz8794" for KSZ8794 chip,
> > >> +           "microchip,ksz8765" for KSZ8765 chip,
> > >> +           "microchip,ksz8895" for KSZ8895 chip,
> > >> +           "microchip,ksz8864" for KSZ8864 chip,
> > >> +           "microchip,ksz8873" for KSZ8873 chip,
> > >> +           "microchip,ksz8863" for KSZ8863 chip,
> > >> +           "microchip,ksz8463" for KSZ8463 chip
> > >
> >
> > all that chips have the same spi access to get chip id on probe(). I
> > prefer common microship,ksz-spi rather than somebody will always
> > maintain that list.
> 
> The Marvell DSA driver is similar. The compatibility string tells you
> enough to go find the switch ID in the switch itself.
> 
> I suppose this comes down to, is there going to be one SPI driver for
> all the devices, or lots of drivers? In general, DSA has one driver
> for lots of devices. The mv88e6xxx supports around 25 devices. The b53
> has around 17, etc.
> 
> So i would suggest one driver supporting all the different devices.
> 

There will be 5 drivers to support these devices:

ksz9477.c - KSZ9893/KSZ9897/KSZ9567/KSZ9566/KSZ9477
ksz8795.c - KSZ8795/KSZ8795/KSZ8765
ksz8895.c - KSZ8895/KSZ8864
ksz8863.c - KSZ8863/KSZ8873
ksz8463.c - KSZ8463

These chips have different SPI access mechanisms, MIB counter reading,
and register set.  These can be combined into one single driver using
function pointers, at least for ksz8795/ksz8895/ksz8863/ksz8463.  My
only concern is the memory footprint.  The customer may not want a
big driver to cover all the switches while only one is used.

Out of topic I have a question to ask the community regarding the DSA
port creation:

Port 1 can be specified using the reg parameter specifying 0; port 2, 1;
and so on.  The KSZ8794 is a variant of KSZ8795 with port 4 disabled.
So
lan1 = 0, lan2 = 1, lan3 = 2, cpu = 4.
But our company Marketing does not want to promote that fact but treat
KSZ8794 as a distinct product.
So
lan1 = 0, lan2 = 1, lan3 = 2, cpu = 3.
Is this okay to hide this information inside the driver?  This is manageable
for KSZ8794 but for KSZ8864 the first port is disabled:
lan1 = 1, lan2 = 2, lan3 = 3, cpu = 4.

I am not sure whether DSA has or will have a way to display the port
mapping to regular users.
Florian Fainelli Sept. 8, 2017, 6:45 p.m. UTC | #6
On 09/08/2017 11:40 AM, Tristram.Ha@microchip.com wrote:
>> -----Original Message-----
>> From: Andrew Lunn [mailto:andrew@lunn.ch]
>> Sent: Friday, September 08, 2017 7:12 AM
>> To: Maxim Uvarov
>> Cc: Tristram Ha - C24268; Pavel Machek; Nathan Conrad; Vivien Didelot; Florian
>> Fainelli; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
>> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
>> drivers can be added
>>
>> On Fri, Sep 08, 2017 at 04:32:35PM +0300, Maxim Uvarov wrote:
>>> 2017-09-08 0:54 GMT+03:00 Andrew Lunn <andrew@lunn.ch>:
>>>>> -- compatible: For external switch chips, compatible string must be exactly
>> one
>>>>> -  of: "microchip,ksz9477"
>>>>> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
>>>>> +           "microchip,ksz8795" for KSZ8795 chip,
>>>>> +           "microchip,ksz8794" for KSZ8794 chip,
>>>>> +           "microchip,ksz8765" for KSZ8765 chip,
>>>>> +           "microchip,ksz8895" for KSZ8895 chip,
>>>>> +           "microchip,ksz8864" for KSZ8864 chip,
>>>>> +           "microchip,ksz8873" for KSZ8873 chip,
>>>>> +           "microchip,ksz8863" for KSZ8863 chip,
>>>>> +           "microchip,ksz8463" for KSZ8463 chip
>>>>
>>>
>>> all that chips have the same spi access to get chip id on probe(). I
>>> prefer common microship,ksz-spi rather than somebody will always
>>> maintain that list.
>>
>> The Marvell DSA driver is similar. The compatibility string tells you
>> enough to go find the switch ID in the switch itself.
>>
>> I suppose this comes down to, is there going to be one SPI driver for
>> all the devices, or lots of drivers? In general, DSA has one driver
>> for lots of devices. The mv88e6xxx supports around 25 devices. The b53
>> has around 17, etc.
>>
>> So i would suggest one driver supporting all the different devices.
>>
> 
> There will be 5 drivers to support these devices:
> 
> ksz9477.c - KSZ9893/KSZ9897/KSZ9567/KSZ9566/KSZ9477
> ksz8795.c - KSZ8795/KSZ8795/KSZ8765
> ksz8895.c - KSZ8895/KSZ8864
> ksz8863.c - KSZ8863/KSZ8873
> ksz8463.c - KSZ8463
> 
> These chips have different SPI access mechanisms, MIB counter reading,
> and register set.  These can be combined into one single driver using
> function pointers, at least for ksz8795/ksz8895/ksz8863/ksz8463.  My
> only concern is the memory footprint.  The customer may not want a
> big driver to cover all the switches while only one is used.
> 
> Out of topic I have a question to ask the community regarding the DSA
> port creation:
> 
> Port 1 can be specified using the reg parameter specifying 0; port 2, 1;
> and so on.  The KSZ8794 is a variant of KSZ8795 with port 4 disabled.
> So
> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 4.
> But our company Marketing does not want to promote that fact but treat
> KSZ8794 as a distinct product.
> So
> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 3.
> Is this okay to hide this information inside the driver?  This is manageable
> for KSZ8794 but for KSZ8864 the first port is disabled:
> lan1 = 1, lan2 = 2, lan3 = 3, cpu = 4.

What dictates all of that is ultimately Device Tree because it defines
the port mapping, including where the CPU port is. Once your driver
knows which chip it is "talking to" you could always have the driver
issue warnings and indicate that the Device Tree is malformed if the
user-specified port mapping does not match what the HW supports.

> 
> I am not sure whether DSA has or will have a way to display the port
> mapping to regular users.

DSA does display the port mapping of user facing ports under the
standard sysfs attribute /sys/class/net/*/phys_port_name. CPU port
mapping is not displayed because there is no CPU-port network device
(intentionally).
Florian Fainelli Sept. 8, 2017, 6:48 p.m. UTC | #7
On 09/07/2017 02:11 PM, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Add other KSZ switches support so that patch check does not complain.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> ---
>  Documentation/devicetree/bindings/net/dsa/ksz.txt | 117 ++++++++++++----------
>  1 file changed, 62 insertions(+), 55 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> index 0ab8b39..34af0e0 100644
> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
>  
>  Required properties:
>  
> -- compatible: For external switch chips, compatible string must be exactly one
> -  of: "microchip,ksz9477"
> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> +	      "microchip,ksz8795" for KSZ8795 chip,
> +	      "microchip,ksz8794" for KSZ8794 chip,
> +	      "microchip,ksz8765" for KSZ8765 chip,
> +	      "microchip,ksz8895" for KSZ8895 chip,
> +	      "microchip,ksz8864" for KSZ8864 chip,
> +	      "microchip,ksz8873" for KSZ8873 chip,
> +	      "microchip,ksz8863" for KSZ8863 chip,
> +	      "microchip,ksz8463" for KSZ8463 chip

It becomes pretty obvious there is a 1 to 1 mapping between the
compatible name and what you should be using it for so specifying
ksz8795 for KSZ8795 chip is really redundant.

You could just list all compatible strings that you support and change
the verbiage to be:

compatible: Should be one of:
		"microchip,ksz9477"
		...
		"microchip,ksz8463"
>  
>  See Documentation/devicetree/bindings/dsa/dsa.txt for a list of additional  required and optional properties.
> @@ -13,60 +20,60 @@ Examples:
>  
>  Ethernet switch connected via SPI to the host, CPU port wired to eth0:
>  
> -                             eth0: ethernet@10001000 {
> -                                             fixed-link {
> -                                                             speed = <1000>;
> -                                                             full-duplex;
> -                                             };
> -                             };
> +	eth0: ethernet@10001000 {
> +		fixed-link {
> +			speed = <1000>;
> +			full-duplex;
> +		};
> +	};

This is a good clean up, but it would probably belong in a separate
patch that you would do before adding compatible strings for instance.

>  
> -                             spi1: spi@f8008000 {
> -                                             pinctrl-0 = <&pinctrl_spi_ksz>;
> -                                             cs-gpios = <&pioC 25 0>;
> -                                             id = <1>;
> -                                             status = "okay";
> +	spi1: spi@f8008000 {
> +		cs-gpios = <&pioC 25 0>;
> +		id = <1>;
> +		status = "okay";
>  
> -                                             ksz9477: ksz9477@0 {
> -                                                             compatible = "microchip,ksz9477";
> -                                                             reg = <0>;
> +		ksz9477: ksz9477@0 {
> +			compatible = "microchip,ksz9477";
> +			reg = <0>;
>  
> -                                                             spi-max-frequency = <44000000>;
> -                                                             spi-cpha;
> -                                                             spi-cpol;
> +			spi-max-frequency = <44000000>;
> +			spi-cpha;
> +			spi-cpol;
> +
> +			status = "okay";
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				port@0 {
> +					reg = <0>;
> +					label = "lan1";
> +				};
> +				port@1 {
> +					reg = <1>;
> +					label = "lan2";
> +				};
> +				port@2 {
> +					reg = <2>;
> +					label = "lan3";
> +				};
> +				port@3 {
> +					reg = <3>;
> +					label = "lan4";
> +				};
> +				port@4 {
> +					reg = <4>;
> +					label = "lan5";
> +				};
> +				port@5 {
> +					reg = <5>;
> +					label = "cpu";
> +					ethernet = <&eth0>;
> +					fixed-link {
> +						speed = <1000>;
> +						full-duplex;
> +					};
> +				};
> +			};
> +		};
> +	};
>  
> -                                                             status = "okay";
> -                                                             ports {
> -                                                                             #address-cells = <1>;
> -                                                                             #size-cells = <0>;
> -                                                                             port@0 {
> -                                                                                             reg = <0>;
> -                                                                                             label = "lan1";
> -                                                                             };
> -                                                                             port@1 {
> -                                                                                             reg = <1>;
> -                                                                                             label = "lan2";
> -                                                                             };
> -                                                                             port@2 {
> -                                                                                             reg = <2>;
> -                                                                                             label = "lan3";
> -                                                                             };
> -                                                                             port@3 {
> -                                                                                             reg = <3>;
> -                                                                                             label = "lan4";
> -                                                                             };
> -                                                                             port@4 {
> -                                                                                             reg = <4>;
> -                                                                                             label = "lan5";
> -                                                                             };
> -                                                                             port@5 {
> -                                                                                             reg = <5>;
> -                                                                                             label = "cpu";
> -                                                                                             ethernet = <&eth0>;
> -                                                                                             fixed-link {
> -                                                                                                             speed = <1000>;
> -                                                                                                             full-duplex;
> -                                                                                             };
> -                                                                             };
> -                                                             };
> -                                             };
> -                             };
>
Maxim Uvarov Sept. 8, 2017, 7 p.m. UTC | #8
2017-09-08 21:48 GMT+03:00 Florian Fainelli <f.fainelli@gmail.com>:
> On 09/07/2017 02:11 PM, Tristram.Ha@microchip.com wrote:
>> From: Tristram Ha <Tristram.Ha@microchip.com>
>>
>> Add other KSZ switches support so that patch check does not complain.
>>
>> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
>> ---
>>  Documentation/devicetree/bindings/net/dsa/ksz.txt | 117 ++++++++++++----------
>>  1 file changed, 62 insertions(+), 55 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>> index 0ab8b39..34af0e0 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
>> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
>>
>>  Required properties:
>>
>> -- compatible: For external switch chips, compatible string must be exactly one
>> -  of: "microchip,ksz9477"
>> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
>> +           "microchip,ksz8795" for KSZ8795 chip,
>> +           "microchip,ksz8794" for KSZ8794 chip,
>> +           "microchip,ksz8765" for KSZ8765 chip,
>> +           "microchip,ksz8895" for KSZ8895 chip,
>> +           "microchip,ksz8864" for KSZ8864 chip,
>> +           "microchip,ksz8873" for KSZ8873 chip,
>> +           "microchip,ksz8863" for KSZ8863 chip,
>> +           "microchip,ksz8463" for KSZ8463 chip
>


Tristram, does any of this devices support chaining?

Maxim.

> It becomes pretty obvious there is a 1 to 1 mapping between the
> compatible name and what you should be using it for so specifying
> ksz8795 for KSZ8795 chip is really redundant.
>
> You could just list all compatible strings that you support and change
> the verbiage to be:
>
> compatible: Should be one of:
>                 "microchip,ksz9477"
>                 ...
>                 "microchip,ksz8463"
>>
>>  See Documentation/devicetree/bindings/dsa/dsa.txt for a list of additional  required and optional properties.
>> @@ -13,60 +20,60 @@ Examples:
>>
>>  Ethernet switch connected via SPI to the host, CPU port wired to eth0:
>>
>> -                             eth0: ethernet@10001000 {
>> -                                             fixed-link {
>> -                                                             speed = <1000>;
>> -                                                             full-duplex;
>> -                                             };
>> -                             };
>> +     eth0: ethernet@10001000 {
>> +             fixed-link {
>> +                     speed = <1000>;
>> +                     full-duplex;
>> +             };
>> +     };
>
> This is a good clean up, but it would probably belong in a separate
> patch that you would do before adding compatible strings for instance.
>
>>
>> -                             spi1: spi@f8008000 {
>> -                                             pinctrl-0 = <&pinctrl_spi_ksz>;
>> -                                             cs-gpios = <&pioC 25 0>;
>> -                                             id = <1>;
>> -                                             status = "okay";
>> +     spi1: spi@f8008000 {
>> +             cs-gpios = <&pioC 25 0>;
>> +             id = <1>;
>> +             status = "okay";
>>
>> -                                             ksz9477: ksz9477@0 {
>> -                                                             compatible = "microchip,ksz9477";
>> -                                                             reg = <0>;
>> +             ksz9477: ksz9477@0 {
>> +                     compatible = "microchip,ksz9477";
>> +                     reg = <0>;
>>
>> -                                                             spi-max-frequency = <44000000>;
>> -                                                             spi-cpha;
>> -                                                             spi-cpol;
>> +                     spi-max-frequency = <44000000>;
>> +                     spi-cpha;
>> +                     spi-cpol;
>> +
>> +                     status = "okay";
>> +                     ports {
>> +                             #address-cells = <1>;
>> +                             #size-cells = <0>;
>> +                             port@0 {
>> +                                     reg = <0>;
>> +                                     label = "lan1";
>> +                             };
>> +                             port@1 {
>> +                                     reg = <1>;
>> +                                     label = "lan2";
>> +                             };
>> +                             port@2 {
>> +                                     reg = <2>;
>> +                                     label = "lan3";
>> +                             };
>> +                             port@3 {
>> +                                     reg = <3>;
>> +                                     label = "lan4";
>> +                             };
>> +                             port@4 {
>> +                                     reg = <4>;
>> +                                     label = "lan5";
>> +                             };
>> +                             port@5 {
>> +                                     reg = <5>;
>> +                                     label = "cpu";
>> +                                     ethernet = <&eth0>;
>> +                                     fixed-link {
>> +                                             speed = <1000>;
>> +                                             full-duplex;
>> +                                     };
>> +                             };
>> +                     };
>> +             };
>> +     };
>>
>> -                                                             status = "okay";
>> -                                                             ports {
>> -                                                                             #address-cells = <1>;
>> -                                                                             #size-cells = <0>;
>> -                                                                             port@0 {
>> -                                                                                             reg = <0>;
>> -                                                                                             label = "lan1";
>> -                                                                             };
>> -                                                                             port@1 {
>> -                                                                                             reg = <1>;
>> -                                                                                             label = "lan2";
>> -                                                                             };
>> -                                                                             port@2 {
>> -                                                                                             reg = <2>;
>> -                                                                                             label = "lan3";
>> -                                                                             };
>> -                                                                             port@3 {
>> -                                                                                             reg = <3>;
>> -                                                                                             label = "lan4";
>> -                                                                             };
>> -                                                                             port@4 {
>> -                                                                                             reg = <4>;
>> -                                                                                             label = "lan5";
>> -                                                                             };
>> -                                                                             port@5 {
>> -                                                                                             reg = <5>;
>> -                                                                                             label = "cpu";
>> -                                                                                             ethernet = <&eth0>;
>> -                                                                                             fixed-link {
>> -                                                                                                             speed = <1000>;
>> -                                                                                                             full-duplex;
>> -                                                                                             };
>> -                                                                             };
>> -                                                             };
>> -                                             };
>> -                             };
>>
>
>
> --
> Florian
Andrew Lunn Sept. 8, 2017, 7:01 p.m. UTC | #9
> > So i would suggest one driver supporting all the different devices.
> 
> There will be 5 drivers to support these devices:
> 
> ksz9477.c - KSZ9893/KSZ9897/KSZ9567/KSZ9566/KSZ9477
> ksz8795.c - KSZ8795/KSZ8795/KSZ8765
> ksz8895.c - KSZ8895/KSZ8864
> ksz8863.c - KSZ8863/KSZ8873
> ksz8463.c - KSZ8463
> 
> These chips have different SPI access mechanisms, MIB counter reading,
> and register set.  These can be combined into one single driver using
> function pointers, at least for ksz8795/ksz8895/ksz8863/ksz8463.  My
> only concern is the memory footprint.  The customer may not want a
> big driver to cover all the switches while only one is used.

If memory footprint is your problem, make it a compile time choice
which devices are supported within the one driver. In practice, you
will find most distributions just enable them all.
 
> Out of topic I have a question to ask the community regarding the DSA
> port creation:
> 
> Port 1 can be specified using the reg parameter specifying 0; port 2, 1;
> and so on.  The KSZ8794 is a variant of KSZ8795 with port 4 disabled.
> So
> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 4.
> But our company Marketing does not want to promote that fact but treat
> KSZ8794 as a distinct product.
> So
> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 3.
> Is this okay to hide this information inside the driver?  This is manageable
> for KSZ8794 but for KSZ8864 the first port is disabled:
> lan1 = 1, lan2 = 2, lan3 = 3, cpu = 4.
> 
> I am not sure whether DSA has or will have a way to display the port
> mapping to regular users.

So the port number to name is determined in the device tree. It
depends on the board how the ports are named. I always suggest going
by the label on the box. clearfog is a good example of this, 0 - lan5,
1 - lan4, etc.

So for the KSZ8864, ensure reg = 0 causes an error.

Does the hardware force which port is used for the CPU? I've boards
with Marvell devices where the CPU is port 0, or port 6, or port
7. The hardware does not care. So we don't force anything.

As for KSZ8794 vs KSZ8795, is there different IDs in the silicon?  It
is these IDs you are using, not the compatible string, to determine
how the drive the silicon. You can trust the ID in the
silicon. Anything else can be wrong.

	 Andrew
Florian Fainelli Sept. 8, 2017, 7:05 p.m. UTC | #10
On 09/08/2017 12:01 PM, Andrew Lunn wrote:
>>> So i would suggest one driver supporting all the different devices.
>>
>> There will be 5 drivers to support these devices:
>>
>> ksz9477.c - KSZ9893/KSZ9897/KSZ9567/KSZ9566/KSZ9477
>> ksz8795.c - KSZ8795/KSZ8795/KSZ8765
>> ksz8895.c - KSZ8895/KSZ8864
>> ksz8863.c - KSZ8863/KSZ8873
>> ksz8463.c - KSZ8463
>>
>> These chips have different SPI access mechanisms, MIB counter reading,
>> and register set.  These can be combined into one single driver using
>> function pointers, at least for ksz8795/ksz8895/ksz8863/ksz8463.  My
>> only concern is the memory footprint.  The customer may not want a
>> big driver to cover all the switches while only one is used.
> 
> If memory footprint is your problem, make it a compile time choice
> which devices are supported within the one driver. In practice, you
> will find most distributions just enable them all.
>  
>> Out of topic I have a question to ask the community regarding the DSA
>> port creation:
>>
>> Port 1 can be specified using the reg parameter specifying 0; port 2, 1;
>> and so on.  The KSZ8794 is a variant of KSZ8795 with port 4 disabled.
>> So
>> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 4.
>> But our company Marketing does not want to promote that fact but treat
>> KSZ8794 as a distinct product.
>> So
>> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 3.
>> Is this okay to hide this information inside the driver?  This is manageable
>> for KSZ8794 but for KSZ8864 the first port is disabled:
>> lan1 = 1, lan2 = 2, lan3 = 3, cpu = 4.
>>
>> I am not sure whether DSA has or will have a way to display the port
>> mapping to regular users.
> 
> So the port number to name is determined in the device tree. It
> depends on the board how the ports are named. I always suggest going
> by the label on the box. clearfog is a good example of this, 0 - lan5,
> 1 - lan4, etc.
> 
> So for the KSZ8864, ensure reg = 0 causes an error.
> 
> Does the hardware force which port is used for the CPU? I've boards
> with Marvell devices where the CPU is port 0, or port 6, or port
> 7. The hardware does not care. So we don't force anything.
> 
> As for KSZ8794 vs KSZ8795, is there different IDs in the silicon?  It
> is these IDs you are using, not the compatible string, to determine
> how the drive the silicon. You can trust the ID in the
> silicon. Anything else can be wrong.

You can't always trust the silicon to report the right revision, just
like marketing people in semiconductors tend to like to play tricks and
pretend that the same chip is the same, but it is not quite, and a new
tape-out was not possible, so it has the same ID, including the
revision. This is of course not what should be done, but it happens all
the time.

Describe in Device Tree with a proper compatible string, which may
end-up being treated the same way by the driver, and if it happens that
the silicon is not wrong, and can be differentiated, then you can always
resort to that to warn the user the wrong compatible was specified, or
that this is not going to work, or anything really.
Tristram.Ha@microchip.com Sept. 8, 2017, 7:48 p.m. UTC | #11
> -----Original Message-----
> From: Maxim Uvarov [mailto:muvarov@gmail.com]
> Sent: Friday, September 08, 2017 12:00 PM
> To: Florian Fainelli
> Cc: Tristram Ha - C24268; Andrew Lunn; Pavel Machek; Nathan Conrad; Vivien
> Didelot; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
> drivers can be added
> 
> 2017-09-08 21:48 GMT+03:00 Florian Fainelli <f.fainelli@gmail.com>:
> > On 09/07/2017 02:11 PM, Tristram.Ha@microchip.com wrote:
> >> From: Tristram Ha <Tristram.Ha@microchip.com>
> >>
> >> Add other KSZ switches support so that patch check does not complain.
> >>
> >> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> >> ---
> >>  Documentation/devicetree/bindings/net/dsa/ksz.txt | 117
> >> ++++++++++++----------
> >>  1 file changed, 62 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >> b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >> index 0ab8b39..34af0e0 100644
> >> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
> >>
> >>  Required properties:
> >>
> >> -- compatible: For external switch chips, compatible string must be
> >> exactly one
> >> -  of: "microchip,ksz9477"
> >> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> >> +           "microchip,ksz8795" for KSZ8795 chip,
> >> +           "microchip,ksz8794" for KSZ8794 chip,
> >> +           "microchip,ksz8765" for KSZ8765 chip,
> >> +           "microchip,ksz8895" for KSZ8895 chip,
> >> +           "microchip,ksz8864" for KSZ8864 chip,
> >> +           "microchip,ksz8873" for KSZ8873 chip,
> >> +           "microchip,ksz8863" for KSZ8863 chip,
> >> +           "microchip,ksz8463" for KSZ8463 chip
> >
> 
> 
> Tristram, does any of this devices support chaining?
> 
> Maxim.

They do not if you mean daisy-chaining the switches together.

There is always a problem that once tail tagging mode is enabled
sending a frame through the MAC without going through the DSA
layer will cause the frame to be dropped.
Florian Fainelli Sept. 8, 2017, 7:54 p.m. UTC | #12
On 09/08/2017 12:48 PM, Tristram.Ha@microchip.com wrote:
>> -----Original Message-----
>> From: Maxim Uvarov [mailto:muvarov@gmail.com]
>> Sent: Friday, September 08, 2017 12:00 PM
>> To: Florian Fainelli
>> Cc: Tristram Ha - C24268; Andrew Lunn; Pavel Machek; Nathan Conrad; Vivien
>> Didelot; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
>> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
>> drivers can be added
>>
>> 2017-09-08 21:48 GMT+03:00 Florian Fainelli <f.fainelli@gmail.com>:
>>> On 09/07/2017 02:11 PM, Tristram.Ha@microchip.com wrote:
>>>> From: Tristram Ha <Tristram.Ha@microchip.com>
>>>>
>>>> Add other KSZ switches support so that patch check does not complain.
>>>>
>>>> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/net/dsa/ksz.txt | 117
>>>> ++++++++++++----------
>>>>  1 file changed, 62 insertions(+), 55 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>> b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>> index 0ab8b39..34af0e0 100644
>>>> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
>>>>
>>>>  Required properties:
>>>>
>>>> -- compatible: For external switch chips, compatible string must be
>>>> exactly one
>>>> -  of: "microchip,ksz9477"
>>>> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
>>>> +           "microchip,ksz8795" for KSZ8795 chip,
>>>> +           "microchip,ksz8794" for KSZ8794 chip,
>>>> +           "microchip,ksz8765" for KSZ8765 chip,
>>>> +           "microchip,ksz8895" for KSZ8895 chip,
>>>> +           "microchip,ksz8864" for KSZ8864 chip,
>>>> +           "microchip,ksz8873" for KSZ8873 chip,
>>>> +           "microchip,ksz8863" for KSZ8863 chip,
>>>> +           "microchip,ksz8463" for KSZ8463 chip
>>>
>>
>>
>> Tristram, does any of this devices support chaining?
>>
>> Maxim.
> 
> They do not if you mean daisy-chaining the switches together.

Marvell tags allow you to specify both a port and switch index
destination after setting up an appropriate routing table, I am assuming
this is not supported.

What happens though if I connect two KSZ switches ones to another say:

eth0
	-> KSZ8463
		-> KSZ8463

Will the first switch terminates KSZ tag if it sees two tags
encapsulated in another, something like this:

| MAC DA | MAC SA | .... payload | Inner KSZ tag | Inner KSZ tag | FCS |

> 
> There is always a problem that once tail tagging mode is enabled
> sending a frame through the MAC without going through the DSA
> layer will cause the frame to be dropped.

Yes, once the master network device is used for DSA, it is still usable
directly by e.g: applications, but it won't do anything if the switch is
configured such that it drops ingressing frames not having the proper
tag. We documented that here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/dsa/dsa.txt#n275
Tristram.Ha@microchip.com Sept. 8, 2017, 8:07 p.m. UTC | #13
> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Friday, September 08, 2017 12:54 PM
> To: Tristram Ha - C24268; muvarov@gmail.com
> Cc: andrew@lunn.ch; pavel@ucw.cz; nathan.leigh.conrad@gmail.com;
> vivien.didelot@savoirfairelinux.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Woojung Huh - C21699
> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
> drivers can be added
> 
> On 09/08/2017 12:48 PM, Tristram.Ha@microchip.com wrote:
> >> -----Original Message-----
> >> From: Maxim Uvarov [mailto:muvarov@gmail.com]
> >> Sent: Friday, September 08, 2017 12:00 PM
> >> To: Florian Fainelli
> >> Cc: Tristram Ha - C24268; Andrew Lunn; Pavel Machek; Nathan Conrad; Vivien
> >> Didelot; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
> >> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that
> new
> >> drivers can be added
> >>
> >> 2017-09-08 21:48 GMT+03:00 Florian Fainelli <f.fainelli@gmail.com>:
> >>> On 09/07/2017 02:11 PM, Tristram.Ha@microchip.com wrote:
> >>>> From: Tristram Ha <Tristram.Ha@microchip.com>
> >>>>
> >>>> Add other KSZ switches support so that patch check does not complain.
> >>>>
> >>>> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> >>>> ---
> >>>>  Documentation/devicetree/bindings/net/dsa/ksz.txt | 117
> >>>> ++++++++++++----------
> >>>>  1 file changed, 62 insertions(+), 55 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >>>> b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >>>> index 0ab8b39..34af0e0 100644
> >>>> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >>>> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> >>>> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
> >>>>
> >>>>  Required properties:
> >>>>
> >>>> -- compatible: For external switch chips, compatible string must be
> >>>> exactly one
> >>>> -  of: "microchip,ksz9477"
> >>>> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
> >>>> +           "microchip,ksz8795" for KSZ8795 chip,
> >>>> +           "microchip,ksz8794" for KSZ8794 chip,
> >>>> +           "microchip,ksz8765" for KSZ8765 chip,
> >>>> +           "microchip,ksz8895" for KSZ8895 chip,
> >>>> +           "microchip,ksz8864" for KSZ8864 chip,
> >>>> +           "microchip,ksz8873" for KSZ8873 chip,
> >>>> +           "microchip,ksz8863" for KSZ8863 chip,
> >>>> +           "microchip,ksz8463" for KSZ8463 chip
> >>>
> >>
> >>
> >> Tristram, does any of this devices support chaining?
> >>
> >> Maxim.
> >
> > They do not if you mean daisy-chaining the switches together.
> 
> Marvell tags allow you to specify both a port and switch index
> destination after setting up an appropriate routing table, I am assuming
> this is not supported.
> 
> What happens though if I connect two KSZ switches ones to another say:
> 
> eth0
> 	-> KSZ8463
> 		-> KSZ8463
> 
> Will the first switch terminates KSZ tag if it sees two tags
> encapsulated in another, something like this:
> 
> | MAC DA | MAC SA | .... payload | Inner KSZ tag | Inner KSZ tag | FCS |
> 

In theory it is doable by adding more tags and remember which port
is connected to the cpu port of another switch, but there is no switch
forwarding and everything is handled by software.

> >
> > There is always a problem that once tail tagging mode is enabled
> > sending a frame through the MAC without going through the DSA
> > layer will cause the frame to be dropped.
> 
> Yes, once the master network device is used for DSA, it is still usable
> directly by e.g: applications, but it won't do anything if the switch is
> configured such that it drops ingressing frames not having the proper
> tag. We documented that here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docume
> ntation/networking/dsa/dsa.txt#n275
> --

As the DSA was developed for the Marvell switches I assumed they can
forward frame without a tag.
Florian Fainelli Sept. 8, 2017, 8:58 p.m. UTC | #14
On 09/08/2017 01:07 PM, Tristram.Ha@microchip.com wrote:
>> -----Original Message-----
>> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
>> Sent: Friday, September 08, 2017 12:54 PM
>> To: Tristram Ha - C24268; muvarov@gmail.com
>> Cc: andrew@lunn.ch; pavel@ucw.cz; nathan.leigh.conrad@gmail.com;
>> vivien.didelot@savoirfairelinux.com; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Woojung Huh - C21699
>> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
>> drivers can be added
>>
>> On 09/08/2017 12:48 PM, Tristram.Ha@microchip.com wrote:
>>>> -----Original Message-----
>>>> From: Maxim Uvarov [mailto:muvarov@gmail.com]
>>>> Sent: Friday, September 08, 2017 12:00 PM
>>>> To: Florian Fainelli
>>>> Cc: Tristram Ha - C24268; Andrew Lunn; Pavel Machek; Nathan Conrad; Vivien
>>>> Didelot; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
>>>> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that
>> new
>>>> drivers can be added
>>>>
>>>> 2017-09-08 21:48 GMT+03:00 Florian Fainelli <f.fainelli@gmail.com>:
>>>>> On 09/07/2017 02:11 PM, Tristram.Ha@microchip.com wrote:
>>>>>> From: Tristram Ha <Tristram.Ha@microchip.com>
>>>>>>
>>>>>> Add other KSZ switches support so that patch check does not complain.
>>>>>>
>>>>>> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/net/dsa/ksz.txt | 117
>>>>>> ++++++++++++----------
>>>>>>  1 file changed, 62 insertions(+), 55 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>>>> b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>>>> index 0ab8b39..34af0e0 100644
>>>>>> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>>>> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>>>>>> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
>>>>>>
>>>>>>  Required properties:
>>>>>>
>>>>>> -- compatible: For external switch chips, compatible string must be
>>>>>> exactly one
>>>>>> -  of: "microchip,ksz9477"
>>>>>> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
>>>>>> +           "microchip,ksz8795" for KSZ8795 chip,
>>>>>> +           "microchip,ksz8794" for KSZ8794 chip,
>>>>>> +           "microchip,ksz8765" for KSZ8765 chip,
>>>>>> +           "microchip,ksz8895" for KSZ8895 chip,
>>>>>> +           "microchip,ksz8864" for KSZ8864 chip,
>>>>>> +           "microchip,ksz8873" for KSZ8873 chip,
>>>>>> +           "microchip,ksz8863" for KSZ8863 chip,
>>>>>> +           "microchip,ksz8463" for KSZ8463 chip
>>>>>
>>>>
>>>>
>>>> Tristram, does any of this devices support chaining?
>>>>
>>>> Maxim.
>>>
>>> They do not if you mean daisy-chaining the switches together.
>>
>> Marvell tags allow you to specify both a port and switch index
>> destination after setting up an appropriate routing table, I am assuming
>> this is not supported.
>>
>> What happens though if I connect two KSZ switches ones to another say:
>>
>> eth0
>> 	-> KSZ8463
>> 		-> KSZ8463
>>
>> Will the first switch terminates KSZ tag if it sees two tags
>> encapsulated in another, something like this:
>>
>> | MAC DA | MAC SA | .... payload | Inner KSZ tag | Inner KSZ tag | FCS |
>>
> 
> In theory it is doable by adding more tags and remember which port
> is connected to the cpu port of another switch, but there is no switch
> forwarding and everything is handled by software.

Fair enough.

> 
>>>
>>> There is always a problem that once tail tagging mode is enabled
>>> sending a frame through the MAC without going through the DSA
>>> layer will cause the frame to be dropped.
>>
>> Yes, once the master network device is used for DSA, it is still usable
>> directly by e.g: applications, but it won't do anything if the switch is
>> configured such that it drops ingressing frames not having the proper
>> tag. We documented that here:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docume
>> ntation/networking/dsa/dsa.txt#n275
>> --
> 
> As the DSA was developed for the Marvell switches I assumed they can
> forward frame without a tag.

Once you use tags with your switch, eth0/master netdev/conduit interface
no longer has a purpose as a normal interface because we create per-port
network devices. That means if you want to send packets towards Port 0
you use the proper network interface. If you create a bridge, you will
use brX as the network device to send/receive packets from, in all
cases, the packets originate from the CPU and the frame ingresses the
switch with the proper information within the tag to target the vector
of ports.

If you have tags enabled and you use eth0 to send packets towards the
switch, there are not that many options, either you have the proper tag,
and the switch will forward to the right port which can be the CPU port
itself if you want, but why do that?

When tags are not enabled (e.g: b53) that is slightly different,
eth0/master/conduit remains usable as a normal interface would, but
unless you start adding VLAN tags, you cannot quite differentiate where
the traffic from LAN to CPU is coming from, so this has limited usefulness.

Hope this clears things.
Pavel Machek Sept. 8, 2017, 9:50 p.m. UTC | #15
On Fri 2017-09-08 21:01:22, Andrew Lunn wrote:
> > > So i would suggest one driver supporting all the different devices.
> > 
> > There will be 5 drivers to support these devices:
> > 
> > ksz9477.c - KSZ9893/KSZ9897/KSZ9567/KSZ9566/KSZ9477
> > ksz8795.c - KSZ8795/KSZ8795/KSZ8765
> > ksz8895.c - KSZ8895/KSZ8864
> > ksz8863.c - KSZ8863/KSZ8873
> > ksz8463.c - KSZ8463
> > 
> > These chips have different SPI access mechanisms, MIB counter reading,
> > and register set.  These can be combined into one single driver using
> > function pointers, at least for ksz8795/ksz8895/ksz8863/ksz8463.  My
> > only concern is the memory footprint.  The customer may not want a
> > big driver to cover all the switches while only one is used.
> 
> If memory footprint is your problem, make it a compile time choice
> which devices are supported within the one driver. In practice, you
> will find most distributions just enable them all.

I have to side with Tristram here. The register layouts are so
different that single driver does not make sense.

What could make sense is single function, compiled 5 times, based on
different includes; same source code but 5 different binaries.

									Pavel
Pavel Machek Sept. 8, 2017, 9:53 p.m. UTC | #16
Hi!

> There will be 5 drivers to support these devices:
> 
> ksz9477.c - KSZ9893/KSZ9897/KSZ9567/KSZ9566/KSZ9477
> ksz8795.c - KSZ8795/KSZ8795/KSZ8765
> ksz8895.c - KSZ8895/KSZ8864

Could we see the 8895 driver, please?

> Out of topic I have a question to ask the community regarding the DSA
> port creation:
> 
> Port 1 can be specified using the reg parameter specifying 0; port 2, 1;
> and so on.  The KSZ8794 is a variant of KSZ8795 with port 4 disabled.
> So
> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 4.
> But our company Marketing does not want to promote that fact but treat
> KSZ8794 as a distinct product.
> So
> lan1 = 0, lan2 = 1, lan3 = 2, cpu = 3.
> Is this okay to hide this information inside the driver?  This is manageable
> for KSZ8794 but for KSZ8864 the first port is disabled:
> lan1 = 1, lan2 = 2, lan3 = 3, cpu = 4.
> 
> I am not sure whether DSA has or will have a way to display the port
> mapping to regular users.

Kernel is not a place to play marketing games, and people reading the
kernel sources are not targets of your marketing department.

Please let us just see the underlying hardware, as is.
									Pavel
Maxim Uvarov Sept. 11, 2017, 7:53 a.m. UTC | #17
2017-09-08 22:48 GMT+03:00  <Tristram.Ha@microchip.com>:
>> -----Original Message-----
>> From: Maxim Uvarov [mailto:muvarov@gmail.com]
>> Sent: Friday, September 08, 2017 12:00 PM
>> To: Florian Fainelli
>> Cc: Tristram Ha - C24268; Andrew Lunn; Pavel Machek; Nathan Conrad; Vivien
>> Didelot; netdev; linux-kernel@vger.kernel.org; Woojung Huh - C21699
>> Subject: Re: [PATCH RFC] Update documentation for KSZ DSA drivers so that new
>> drivers can be added
>>
>> 2017-09-08 21:48 GMT+03:00 Florian Fainelli <f.fainelli@gmail.com>:
>> > On 09/07/2017 02:11 PM, Tristram.Ha@microchip.com wrote:
>> >> From: Tristram Ha <Tristram.Ha@microchip.com>
>> >>
>> >> Add other KSZ switches support so that patch check does not complain.
>> >>
>> >> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
>> >> ---
>> >>  Documentation/devicetree/bindings/net/dsa/ksz.txt | 117
>> >> ++++++++++++----------
>> >>  1 file changed, 62 insertions(+), 55 deletions(-)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt
>> >> b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>> >> index 0ab8b39..34af0e0 100644
>> >> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
>> >> +++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
>> >> @@ -3,8 +3,15 @@ Microchip KSZ Series Ethernet switches
>> >>
>> >>  Required properties:
>> >>
>> >> -- compatible: For external switch chips, compatible string must be
>> >> exactly one
>> >> -  of: "microchip,ksz9477"
>> >> +- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
>> >> +           "microchip,ksz8795" for KSZ8795 chip,
>> >> +           "microchip,ksz8794" for KSZ8794 chip,
>> >> +           "microchip,ksz8765" for KSZ8765 chip,
>> >> +           "microchip,ksz8895" for KSZ8895 chip,
>> >> +           "microchip,ksz8864" for KSZ8864 chip,
>> >> +           "microchip,ksz8873" for KSZ8873 chip,
>> >> +           "microchip,ksz8863" for KSZ8863 chip,
>> >> +           "microchip,ksz8463" for KSZ8463 chip
>> >
>>
>>
>> Tristram, does any of this devices support chaining?
>>
>> Maxim.
>
> They do not if you mean daisy-chaining the switches together.
>
> There is always a problem that once tail tagging mode is enabled
> sending a frame through the MAC without going through the DSA
> layer will cause the frame to be dropped.
>

Tistram, as Florian answered before  by "chaining"  in my question I
meant milti chip DSA.
I.e. when several chips represent one DSA instance and all interfaces
joined to  the same bridge.
Bridge code take care about fdb, mdb, vlan configuration for all
chips. If packet supposed to be
forward across chips it will not go to cpu, Only cpus related traffic
goes to cpu (like broad casts,
fdb mac entry, unknown packet, stp/BPDU). I.e. milti chip DSA allows
you to work with several chips
as one big virtual chip. How to make it happen depends on hardware
possibilities.  Might be several
tail tags, but in that case other chips have to know how to work with
it. Or smart vlans configuration,
but it that case cross chip vlan should not overlap with system vlans.
In marvel it's done that each tag
has chip id which can be programmed from mdio, and also each chip
knows how to work with that tag
and their place in dsa chain - "up-link" and "down-link". For ksz* I
did not find any notes that such configuration
is supported. How ever it might be doable with some smart software settings.

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt b/Documentation/devicetree/bindings/net/dsa/ksz.txt
index 0ab8b39..34af0e0 100644
--- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
+++ b/Documentation/devicetree/bindings/net/dsa/ksz.txt
@@ -3,8 +3,15 @@  Microchip KSZ Series Ethernet switches
 
 Required properties:
 
-- compatible: For external switch chips, compatible string must be exactly one
-  of: "microchip,ksz9477"
+- compatible: Should be "microchip,ksz9477" for KSZ9477 chip,
+	      "microchip,ksz8795" for KSZ8795 chip,
+	      "microchip,ksz8794" for KSZ8794 chip,
+	      "microchip,ksz8765" for KSZ8765 chip,
+	      "microchip,ksz8895" for KSZ8895 chip,
+	      "microchip,ksz8864" for KSZ8864 chip,
+	      "microchip,ksz8873" for KSZ8873 chip,
+	      "microchip,ksz8863" for KSZ8863 chip,
+	      "microchip,ksz8463" for KSZ8463 chip
 
 See Documentation/devicetree/bindings/dsa/dsa.txt for a list of additional  required and optional properties.
@@ -13,60 +20,60 @@  Examples: