[PATCH v2 0/8] Add support for ICSSG-based Ethernet on SR1.0 devices

[PATCH v2 0/8] Add support for ICSSG-based Ethernet on SR1.0 devices

[Diogo Ivo]

Hello,

This series extends the current ICSSG-based Ethernet driver to support
Silicon Revision 1.0 devices.

Notable differences between the Silicon Revisions are that there is
no TX core in SR1.0 with this being handled by the firmware, requiring
extra DMA channels to communicate commands to the firmware (with the
firmware being different as well) and in the packet classifier.

The motivation behind it is that a significant number of Siemens
devices containing SR1.0 silicon have been deployed in the field
and need to be supported and updated to newer kernel versions
without losing functionality.

This series is based on TI's 5.10 SDK [1].

The first version of this patch series can be found in [2].

[1]: https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/tree/?h=ti-linux-5.10.y
[2]: https://lore.kernel.org/all/20231219174548.3481-1-diogo.ivo@siemens.com/

Changes in v2:
 - Addressed Krzysztof's comments on the dt-binding
 - Removed explicit references to SR2.0
 - Added static keyword as indicated by the kernel test robot

Diogo Ivo (8):
  dt-bindings: net: Add support for AM65x SR1.0 in ICSSG
  net: ti: icssg-config: add SR1.0-specific configuration bits
  net: ti: icssg-prueth: add SR1.0-specific configuration bits
  net: ti: icssg-classifier: Add support for SR1.0
  net: ti: icssg-config: Add SR1.0 configuration functions
  net: ti: icssg-ethtool: Adjust channel count for SR1.0
  net: ti: iccsg-prueth: Add necessary functions for SR1.0 support
  net: ti: icssg-prueth: Wire up support for SR1.0

 .../bindings/net/ti,icssg-prueth.yaml         |  29 +-
 .../net/ethernet/ti/icssg/icssg_classifier.c  | 113 +++-
 drivers/net/ethernet/ti/icssg/icssg_config.c  |  86 ++-
 drivers/net/ethernet/ti/icssg/icssg_config.h  |  55 ++
 drivers/net/ethernet/ti/icssg/icssg_ethtool.c |  10 +-
 drivers/net/ethernet/ti/icssg/icssg_prueth.c  | 556 ++++++++++++++++--
 drivers/net/ethernet/ti/icssg/icssg_prueth.h  |  21 +-
 7 files changed, 788 insertions(+), 82 deletions(-)

-- 
2.43.0

[PATCH v2 1/8] dt-bindings: net: Add support for AM65x SR1.0 in ICSSG

[Diogo Ivo]

Silicon Revision 1.0 of the AM65x came with a slightly different ICSSG
support: Only 2 PRUs per slice are available and instead 2 additional
DMA channels are used for management purposes. We have no restrictions
on specified PRUs, but the DMA channels need to be adjusted.

Co-developed-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>
---
Changes in v2:
 - Removed explicit reference to SR2.0
 - Moved sr1 to the SoC name
 - Expand dma-names list and adjust min/maxItems depending on SR1.0/2.0

 .../bindings/net/ti,icssg-prueth.yaml         | 29 ++++++++++++++++---
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
index 229c8f32019f..59a3292191d9 100644
--- a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
+++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
@@ -19,8 +19,9 @@ allOf:
 properties:
   compatible:
     enum:
-      - ti,am642-icssg-prueth  # for AM64x SoC family
-      - ti,am654-icssg-prueth  # for AM65x SoC family
+      - ti,am642-icssg-prueth      # for AM64x SoC family
+      - ti,am654-icssg-prueth      # for AM65x SoC family
+      - ti,am654-sr1-icssg-prueth  # for AM65x SoC family, SR1.0
 
   sram:
     $ref: /schemas/types.yaml#/definitions/phandle
@@ -28,8 +29,7 @@ properties:
       phandle to MSMC SRAM node
 
   dmas:
-    maxItems: 10
-
+    minItems: 10
   dma-names:
     items:
       - const: tx0-0
@@ -42,6 +42,8 @@ properties:
       - const: tx1-3
       - const: rx0
       - const: rx1
+      - const: rxmgm0
+      - const: rxmgm1
 
   ti,mii-g-rt:
     $ref: /schemas/types.yaml#/definitions/phandle
@@ -132,6 +134,25 @@ required:
   - interrupts
   - interrupt-names
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: ti,am654-sr1-icssg-prueth
+    then:
+      properties:
+        dmas:
+          minItems: 12
+        dma-names:
+          minItems: 12
+    else:
+      properties:
+        dmas:
+          maxItems: 10
+        dma-names:
+          maxItems: 10
+
 unevaluatedProperties: false
 
 examples:
-- 
2.43.0

Re: [PATCH v2 1/8] dt-bindings: net: Add support for AM65x SR1.0 in ICSSG

[Rob Herring]

On Wed, 17 Jan 2024 16:14:55 +0000, Diogo Ivo wrote:
> Silicon Revision 1.0 of the AM65x came with a slightly different ICSSG
> support: Only 2 PRUs per slice are available and instead 2 additional
> DMA channels are used for management purposes. We have no restrictions
> on specified PRUs, but the DMA channels need to be adjusted.
> 
> Co-developed-by: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>
> ---
> Changes in v2:
>  - Removed explicit reference to SR2.0
>  - Moved sr1 to the SoC name
>  - Expand dma-names list and adjust min/maxItems depending on SR1.0/2.0
> 
>  .../bindings/net/ti,icssg-prueth.yaml         | 29 ++++++++++++++++---
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml:137:1: [error] duplication of key "allOf" in mapping (key-duplicates)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/net/ti,icssg-prueth.example.dts'
Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml:137:1: found duplicate key "allOf" with value "[]" (original value: "[]")
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/net/ti,icssg-prueth.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml:137:1: found duplicate key "allOf" with value "[]" (original value: "[]")
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1424: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240117161602.153233-2-diogo.ivo@siemens.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 0/8] Add support for ICSSG-based Ethernet on SR1.0 devices

[Jakub Kicinski]

On Wed, 17 Jan 2024 16:14:54 +0000 Diogo Ivo wrote:
>   net: ti: icssg-config: add SR1.0-specific configuration bits

FWIW looks like this patch didn't make it to the list.

Re: [PATCH v2 0/8] Add support for ICSSG-based Ethernet on SR1.0 devices

[Roger Quadros]

Hello Diogo,

On 17/01/2024 18:14, Diogo Ivo wrote:
> Hello,
> 
> This series extends the current ICSSG-based Ethernet driver to support
> Silicon Revision 1.0 devices.
> 
> Notable differences between the Silicon Revisions are that there is
> no TX core in SR1.0 with this being handled by the firmware, requiring
> extra DMA channels to communicate commands to the firmware (with the
> firmware being different as well) and in the packet classifier.
> 
> The motivation behind it is that a significant number of Siemens
> devices containing SR1.0 silicon have been deployed in the field
> and need to be supported and updated to newer kernel versions
> without losing functionality.

Adding SR1.0 support with all its ifdefs makes the driver more complicated
than it should be.

I think we need to treat SR1.0 and SR2.0 as different devices with their
own independent drivers. While the data path is pretty much the same, 
also like in am65-cpsw-nuss.c, the initialization, firmware and other
runtime logic is significantly different.

How about introducing a new icssg_prueth_sr1.c and putting all the SR1 stuff
there? You could still re-use the other helper files in net/ti/icssg/.
It also warrants for it's own Kconfig symbol so it can be built only
if required.
Any common logic could still be moved to icssg_common.c and re-used in both drivers.

> 
> This series is based on TI's 5.10 SDK [1].
> 
> The first version of this patch series can be found in [2].
> 
> [1]: https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/tree/?h=ti-linux-5.10.y
> [2]: https://lore.kernel.org/all/20231219174548.3481-1-diogo.ivo@siemens.com/
> 
> Changes in v2:
>  - Addressed Krzysztof's comments on the dt-binding
>  - Removed explicit references to SR2.0
>  - Added static keyword as indicated by the kernel test robot
> 
> Diogo Ivo (8):
>   dt-bindings: net: Add support for AM65x SR1.0 in ICSSG
>   net: ti: icssg-config: add SR1.0-specific configuration bits
>   net: ti: icssg-prueth: add SR1.0-specific configuration bits
>   net: ti: icssg-classifier: Add support for SR1.0
>   net: ti: icssg-config: Add SR1.0 configuration functions
>   net: ti: icssg-ethtool: Adjust channel count for SR1.0
>   net: ti: iccsg-prueth: Add necessary functions for SR1.0 support
>   net: ti: icssg-prueth: Wire up support for SR1.0
> 
>  .../bindings/net/ti,icssg-prueth.yaml         |  29 +-
>  .../net/ethernet/ti/icssg/icssg_classifier.c  | 113 +++-
>  drivers/net/ethernet/ti/icssg/icssg_config.c  |  86 ++-
>  drivers/net/ethernet/ti/icssg/icssg_config.h  |  55 ++
>  drivers/net/ethernet/ti/icssg/icssg_ethtool.c |  10 +-
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c  | 556 ++++++++++++++++--
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h  |  21 +-
>  7 files changed, 788 insertions(+), 82 deletions(-)
> 

-- 
cheers,
-roger

Re: [PATCH v2 0/8] Add support for ICSSG-based Ethernet on SR1.0 devices

[Diogo Ivo]

Hi all, thank you for your input so far.

On 1/23/24 12:15, Roger Quadros wrote:
> Hello Diogo,
>
> On 17/01/2024 18:14, Diogo Ivo wrote:
>> Hello,
>>
>> This series extends the current ICSSG-based Ethernet driver to support
>> Silicon Revision 1.0 devices.
>>
>> Notable differences between the Silicon Revisions are that there is
>> no TX core in SR1.0 with this being handled by the firmware, requiring
>> extra DMA channels to communicate commands to the firmware (with the
>> firmware being different as well) and in the packet classifier.
>>
>> The motivation behind it is that a significant number of Siemens
>> devices containing SR1.0 silicon have been deployed in the field
>> and need to be supported and updated to newer kernel versions
>> without losing functionality.
> Adding SR1.0 support with all its ifdefs makes the driver more complicated
> than it should be.
>
> I think we need to treat SR1.0 and SR2.0 as different devices with their
> own independent drivers. While the data path is pretty much the same,
> also like in am65-cpsw-nuss.c, the initialization, firmware and other
> runtime logic is significantly different.
>
> How about introducing a new icssg_prueth_sr1.c and putting all the SR1 stuff
> there? You could still re-use the other helper files in net/ti/icssg/.
> It also warrants for it's own Kconfig symbol so it can be built only
> if required.
> Any common logic could still be moved to icssg_common.c and re-used in both drivers.

Yes, that sounds like a more reasonable approach. I will refactor the code

and come back with a v3, hopefully with all patches getting sent out :)


Best regards,

Diogo