[PATCH v4 0/2] Introduce ICSSG based ethernet Driver

[PATCH v4 0/2] Introduce ICSSG based ethernet Driver

[MD Danish Anwar]

The Programmable Real-time Unit and Industrial Communication Subsystem
Gigabit (PRU_ICSSG) is a low-latency microcontroller subsystem in the TI
SoCs. This subsystem is provided for the use cases like the implementation
of custom peripheral interfaces, offloading of tasks from the other
processor cores of the SoC, etc.

The subsystem includes many accelerators for data processing like
multiplier and multiplier-accumulator. It also has peripherals like
UART, MII/RGMII, MDIO, etc. Every ICSSG core includes two 32-bit
load/store RISC CPU cores called PRUs.

The above features allow it to be used for implementing custom firmware
based peripherals like ethernet.

This series adds the YAML documentation and the driver with basic EMAC
support for TI AM654 Silicon Rev 2 SoC with the PRU_ICSSG Sub-system.
running dual-EMAC firmware.
This currently supports basic EMAC with 1Gbps and 100Mbps link. 10M and
half-duplex modes are not yet supported because they require the support
of an IEP, which will be added later.
Advanced features like switch-dev and timestamping will be added later.

This series depends on two patch series that are not yet merged, one in
the remoteproc tree and another in the soc tree. the first one is titled
Introduce PRU remoteproc consumer API and the second one is titled
Introduce PRU platform consumer API.
Both of these are required for this driver.

To explain this dependency and to get reviews, I had earlier posted all
three of these as an RFC[1], this can be seen for understanding the
dependencies.

The two series remoteproc[2] and soc[3] have been posted separately to 
their respective trees.

This is the v3 of the patch series [v1]. This version of the patchset 
addresses the comments made on [v2] of the series. 

Changes from v3 to v4 :
*) Addressed Krzysztof's comments and fixed dt_binding_check errors in 
   patch 1/2.
*) Added interrupt-extended property in ethernet-ports properties section.
*) Fixed comments in file icssg_switch_map.h according to the Linux coding
   style in patch 2/2. Added Documentation of structures in patch 2/2.

Changes from v2 to v3 :
*) Addressed Rob and Krzysztof's comments on patch 1 of this series.
   Fixed indentation. Removed description and pinctrl section from 
   ti,icssg-prueth.yaml file.
*) Addressed Krzysztof, Paolo, Randy, Andrew and Christophe's comments on 
   patch 2 of this seires.
*) Fixed blanklines in Kconfig and Makefile. Changed structures to const 
   as suggested by Krzysztof.
*) Fixed while loop logic in emac_tx_complete_packets() API as suggested 
   by Paolo. Previously in the loop's last iteration 'budget' was 0 and 
   napi_consume_skb would wrongly assume the caller is not in NAPI context
   Now, budget won't be zero in last iteration of loop. 
*) Removed inline functions addr_to_da1() and addr_to_da0() as asked by 
   Andrew.
*) Added dev_err_probe() instead of dev_err() as suggested by Christophe.
*) In ti,icssg-prueth.yaml file, in the patternProperties section of 
   ethernet-ports, kept the port name as "port" instead of "ethernet-port" 
   as all other drivers were using "port". Will change it if is compulsory 
   to use "ethernet-port".

[1] https://lore.kernel.org/all/20220406094358.7895-1-p-mohan@ti.com/
[2] https://patchwork.kernel.org/project/linux-remoteproc/cover/20220418104118.12878-1-p-mohan@ti.com/
[3] https://patchwork.kernel.org/project/linux-remoteproc/cover/20220418123004.9332-1-p-mohan@ti.com/

[v1] https://lore.kernel.org/all/20220506052433.28087-1-p-mohan@ti.com/
[v2] https://lore.kernel.org/all/20220531095108.21757-1-p-mohan@ti.com/
[v3] https://lore.kernel.org/all/20221223110930.1337536-1-danishanwar@ti.com/

Thanks and Regards,
Md Danish Anwar

Puranjay Mohan (1):
  dt-bindings: net: Add ICSSG Ethernet Driver bindings

Roger Quadros (1):
  net: ti: icssg-prueth: Add ICSSG ethernet driver

 .../bindings/net/ti,icssg-prueth.yaml         |  179 ++
 drivers/net/ethernet/ti/Kconfig               |   13 +
 drivers/net/ethernet/ti/Makefile              |    2 +
 drivers/net/ethernet/ti/icssg_classifier.c    |  369 ++++
 drivers/net/ethernet/ti/icssg_config.c        |  449 ++++
 drivers/net/ethernet/ti/icssg_config.h        |  200 ++
 drivers/net/ethernet/ti/icssg_ethtool.c       |  326 +++
 drivers/net/ethernet/ti/icssg_mii_cfg.c       |  104 +
 drivers/net/ethernet/ti/icssg_mii_rt.h        |  151 ++
 drivers/net/ethernet/ti/icssg_prueth.c        | 1880 +++++++++++++++++
 drivers/net/ethernet/ti/icssg_prueth.h        |  246 +++
 drivers/net/ethernet/ti/icssg_switch_map.h    |  234 ++
 include/linux/pruss.h                         |    1 +
 13 files changed, 4154 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
 create mode 100644 drivers/net/ethernet/ti/icssg_classifier.c
 create mode 100644 drivers/net/ethernet/ti/icssg_config.c
 create mode 100644 drivers/net/ethernet/ti/icssg_config.h
 create mode 100644 drivers/net/ethernet/ti/icssg_ethtool.c
 create mode 100644 drivers/net/ethernet/ti/icssg_mii_cfg.c
 create mode 100644 drivers/net/ethernet/ti/icssg_mii_rt.h
 create mode 100644 drivers/net/ethernet/ti/icssg_prueth.c
 create mode 100644 drivers/net/ethernet/ti/icssg_prueth.h
 create mode 100644 drivers/net/ethernet/ti/icssg_switch_map.h

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings

[MD Danish Anwar]

From: Puranjay Mohan <p-mohan@ti.com>

Add a YAML binding document for the ICSSG Programmable real time unit
based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs
to interface the PRUs and load/run the firmware for supporting ethernet
functionality.

Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
Signed-off-by: Md Danish Anwar <danishanwar@ti.com>
---
 .../bindings/net/ti,icssg-prueth.yaml         | 179 ++++++++++++++++++
 1 file changed, 179 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml

diff --git a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
new file mode 100644
index 000000000000..e4dee01a272a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
@@ -0,0 +1,179 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/ti,icssg-prueth.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments ICSSG PRUSS Ethernet
+
+maintainers:
+  - Md Danish Anwar <danishanwar@ti.com>
+
+description:
+  Ethernet based on the Programmable Real-Time
+  Unit and Industrial Communication Subsystem.
+
+allOf:
+  - $ref: /schemas/remoteproc/ti,pru-consumer.yaml#
+
+properties:
+  compatible:
+    enum:
+      - ti,am654-icssg-prueth  # for AM65x SoC family
+
+  ti,sram:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandle to MSMC SRAM node
+
+  dmas:
+    maxItems: 10
+
+  dma-names:
+    items:
+      - const: tx0-0
+      - const: tx0-1
+      - const: tx0-2
+      - const: tx0-3
+      - const: tx1-0
+      - const: tx1-1
+      - const: tx1-2
+      - const: tx1-3
+      - const: rx0
+      - const: rx1
+
+  ethernet-ports:
+    type: object
+    additionalProperties: false
+    properties:
+      '#address-cells':
+        const: 1
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      ^port@[0-1]$:
+        type: object
+        description: ICSSG PRUETH external ports
+
+        $ref: ethernet-controller.yaml#
+
+        unevaluatedProperties: false
+
+        properties:
+          reg:
+            items:
+              - enum: [0, 1]
+            description: ICSSG PRUETH port number
+
+          interrupts-extended:
+            maxItems: 1
+
+          ti,syscon-rgmii-delay:
+            items:
+              - items:
+                  - description: phandle to system controller node
+                  - description: The offset to ICSSG control register
+            $ref: /schemas/types.yaml#/definitions/phandle-array
+            description:
+              phandle to system controller node and register offset
+              to ICSSG control register for RGMII transmit delay
+
+        required:
+          - reg
+
+  ti,mii-g-rt:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: |
+      phandle to MII_G_RT module's syscon regmap.
+
+  ti,mii-rt:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: |
+      phandle to MII_RT module's syscon regmap
+
+  interrupts:
+    maxItems: 2
+    description: |
+      Interrupt specifiers to TX timestamp IRQ.
+
+  interrupt-names:
+    items:
+      - const: tx_ts0
+      - const: tx_ts1
+
+required:
+  - compatible
+  - ti,sram
+  - dmas
+  - dma-names
+  - ethernet-ports
+  - ti,mii-g-rt
+  - interrupts
+  - interrupt-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    /* Example k3-am654 base board SR2.0, dual-emac */
+    pruss2_eth: ethernet {
+        compatible = "ti,am654-icssg-prueth";
+        pinctrl-names = "default";
+        pinctrl-0 = <&icssg2_rgmii_pins_default>;
+        ti,sram = <&msmc_ram>;
+
+        ti,prus = <&pru2_0>, <&rtu2_0>, <&tx_pru2_0>,
+                  <&pru2_1>, <&rtu2_1>, <&tx_pru2_1>;
+        firmware-name = "ti-pruss/am65x-pru0-prueth-fw.elf",
+                        "ti-pruss/am65x-rtu0-prueth-fw.elf",
+                        "ti-pruss/am65x-txpru0-prueth-fw.elf",
+                        "ti-pruss/am65x-pru1-prueth-fw.elf",
+                        "ti-pruss/am65x-rtu1-prueth-fw.elf",
+                        "ti-pruss/am65x-txpru1-prueth-fw.elf";
+        ti,pruss-gp-mux-sel = <2>,      /* MII mode */
+                              <2>,
+                              <2>,
+                              <2>,      /* MII mode */
+                              <2>,
+                              <2>;
+        dmas = <&main_udmap 0xc300>, /* egress slice 0 */
+               <&main_udmap 0xc301>, /* egress slice 0 */
+               <&main_udmap 0xc302>, /* egress slice 0 */
+               <&main_udmap 0xc303>, /* egress slice 0 */
+               <&main_udmap 0xc304>, /* egress slice 1 */
+               <&main_udmap 0xc305>, /* egress slice 1 */
+               <&main_udmap 0xc306>, /* egress slice 1 */
+               <&main_udmap 0xc307>, /* egress slice 1 */
+               <&main_udmap 0x4300>, /* ingress slice 0 */
+               <&main_udmap 0x4301>; /* ingress slice 1 */
+        dma-names = "tx0-0", "tx0-1", "tx0-2", "tx0-3",
+                    "tx1-0", "tx1-1", "tx1-2", "tx1-3",
+                    "rx0", "rx1";
+        ti,mii-g-rt = <&icssg2_mii_g_rt>;
+        interrupts = <24 0 2>, <25 1 3>;
+        interrupt-names = "tx_ts0", "tx_ts1";
+        ethernet-ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            pruss2_emac0: port@0 {
+                reg = <0>;
+                phy-handle = <&pruss2_eth0_phy>;
+                phy-mode = "rgmii-id";
+                interrupts-extended = <&icssg2_intc 24>;
+                ti,syscon-rgmii-delay = <&scm_conf 0x4120>;
+                /* Filled in by bootloader */
+                local-mac-address = [00 00 00 00 00 00];
+            };
+
+            pruss2_emac1: port@1 {
+                reg = <1>;
+                phy-handle = <&pruss2_eth1_phy>;
+                phy-mode = "rgmii-id";
+                interrupts-extended = <&icssg2_intc 25>;
+                ti,syscon-rgmii-delay = <&scm_conf 0x4124>;
+                /* Filled in by bootloader */
+                local-mac-address = [00 00 00 00 00 00];
+            };
+        };
+    };
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings

[Krzysztof Kozlowski]

On 06/02/2023 07:07, MD Danish Anwar wrote:
> From: Puranjay Mohan <p-mohan@ti.com>
> 
> Add a YAML binding document for the ICSSG Programmable real time unit
> based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs

You add a binding for the hardware, not for driver.

> to interface the PRUs and load/run the firmware for supporting ethernet
> functionality.

Subject: drop second/last, redundant "driver bindings". The
"dt-bindings" prefix is already stating that these are bindings.

> 
> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> Signed-off-by: Md Danish Anwar <danishanwar@ti.com>
> ---
>  .../bindings/net/ti,icssg-prueth.yaml         | 179 ++++++++++++++++++
>  1 file changed, 179 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
> new file mode 100644
> index 000000000000..e4dee01a272a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
> @@ -0,0 +1,179 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/ti,icssg-prueth.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments ICSSG PRUSS Ethernet
> +
> +maintainers:
> +  - Md Danish Anwar <danishanwar@ti.com>
> +
> +description:
> +  Ethernet based on the Programmable Real-Time
> +  Unit and Industrial Communication Subsystem.
> +
> +allOf:
> +  - $ref: /schemas/remoteproc/ti,pru-consumer.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,am654-icssg-prueth  # for AM65x SoC family
> +
> +  ti,sram:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to MSMC SRAM node
> +
> +  dmas:
> +    maxItems: 10
> +
> +  dma-names:
> +    items:
> +      - const: tx0-0
> +      - const: tx0-1
> +      - const: tx0-2
> +      - const: tx0-3
> +      - const: tx1-0
> +      - const: tx1-1
> +      - const: tx1-2
> +      - const: tx1-3
> +      - const: rx0
> +      - const: rx1
> +
> +  ethernet-ports:

Bring some order or logic in the order of the properties. Keep the
ethernet-ports as last property.

> +    type: object
> +    additionalProperties: false

Blank line

> +    properties:
> +      '#address-cells':
> +        const: 1
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +      ^port@[0-1]$:
> +        type: object
> +        description: ICSSG PRUETH external ports
> +

Drop blank line

> +        $ref: ethernet-controller.yaml#
> +

Drop blank line

> +        unevaluatedProperties: false
> +
> +        properties:
> +          reg:
> +            items:
> +              - enum: [0, 1]
> +            description: ICSSG PRUETH port number
> +
> +          interrupts-extended:

Just "interrupts"
> +            maxItems: 1
> +
> +          ti,syscon-rgmii-delay:
> +            items:
> +              - items:
> +                  - description: phandle to system controller node
> +                  - description: The offset to ICSSG control register
> +            $ref: /schemas/types.yaml#/definitions/phandle-array
> +            description:
> +              phandle to system controller node and register offset
> +              to ICSSG control register for RGMII transmit delay
> +
> +        required:
> +          - reg

required for ethernet-ports - at least one port is required, isn't it?
> +
> +  ti,mii-g-rt:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: |
> +      phandle to MII_G_RT module's syscon regmap.
> +
> +  ti,mii-rt:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: |
> +      phandle to MII_RT module's syscon regmap
> +
> +  interrupts:
> +    maxItems: 2
> +    description: |
> +      Interrupt specifiers to TX timestamp IRQ.
> +
> +  interrupt-names:
> +    items:
> +      - const: tx_ts0
> +      - const: tx_ts1
> +
> +required:
> +  - compatible
> +  - ti,sram
> +  - dmas
> +  - dma-names
> +  - ethernet-ports
> +  - ti,mii-g-rt
> +  - interrupts
> +  - interrupt-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    /* Example k3-am654 base board SR2.0, dual-emac */
> +    pruss2_eth: ethernet {
> +        compatible = "ti,am654-icssg-prueth";
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&icssg2_rgmii_pins_default>;
> +        ti,sram = <&msmc_ram>;
> +
> +        ti,prus = <&pru2_0>, <&rtu2_0>, <&tx_pru2_0>,
> +                  <&pru2_1>, <&rtu2_1>, <&tx_pru2_1>;
> +        firmware-name = "ti-pruss/am65x-pru0-prueth-fw.elf",
> +                        "ti-pruss/am65x-rtu0-prueth-fw.elf",
> +                        "ti-pruss/am65x-txpru0-prueth-fw.elf",
> +                        "ti-pruss/am65x-pru1-prueth-fw.elf",
> +                        "ti-pruss/am65x-rtu1-prueth-fw.elf",
> +                        "ti-pruss/am65x-txpru1-prueth-fw.elf";
> +        ti,pruss-gp-mux-sel = <2>,      /* MII mode */
> +                              <2>,
> +                              <2>,
> +                              <2>,      /* MII mode */
> +                              <2>,
> +                              <2>;
> +        dmas = <&main_udmap 0xc300>, /* egress slice 0 */
> +               <&main_udmap 0xc301>, /* egress slice 0 */
> +               <&main_udmap 0xc302>, /* egress slice 0 */
> +               <&main_udmap 0xc303>, /* egress slice 0 */
> +               <&main_udmap 0xc304>, /* egress slice 1 */
> +               <&main_udmap 0xc305>, /* egress slice 1 */
> +               <&main_udmap 0xc306>, /* egress slice 1 */
> +               <&main_udmap 0xc307>, /* egress slice 1 */
> +               <&main_udmap 0x4300>, /* ingress slice 0 */
> +               <&main_udmap 0x4301>; /* ingress slice 1 */
> +        dma-names = "tx0-0", "tx0-1", "tx0-2", "tx0-3",
> +                    "tx1-0", "tx1-1", "tx1-2", "tx1-3",
> +                    "rx0", "rx1";
> +        ti,mii-g-rt = <&icssg2_mii_g_rt>;
> +        interrupts = <24 0 2>, <25 1 3>;

Aren't you open-coding some IRQ flags?

> +        interrupt-names = "tx_ts0", "tx_ts1";
> +        ethernet-ports {


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [PATCH v4 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings

[Md Danish Anwar]

Hi Krzysztof,

Thanks for the comments.

On 06/02/23 13:20, Krzysztof Kozlowski wrote:
> On 06/02/2023 07:07, MD Danish Anwar wrote:
>> From: Puranjay Mohan <p-mohan@ti.com>
>>
>> Add a YAML binding document for the ICSSG Programmable real time unit
>> based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs
> 
> You add a binding for the hardware, not for driver.
> 
>> to interface the PRUs and load/run the firmware for supporting ethernet
>> functionality.
> 
> Subject: drop second/last, redundant "driver bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> 

Sure I will modify the subject and commit description.

>>
>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>> Signed-off-by: Md Danish Anwar <danishanwar@ti.com>
>> ---
>>  .../bindings/net/ti,icssg-prueth.yaml         | 179 ++++++++++++++++++
>>  1 file changed, 179 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
>> new file mode 100644
>> index 000000000000..e4dee01a272a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
>> @@ -0,0 +1,179 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/ti,icssg-prueth.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments ICSSG PRUSS Ethernet
>> +
>> +maintainers:
>> +  - Md Danish Anwar <danishanwar@ti.com>
>> +
>> +description:
>> +  Ethernet based on the Programmable Real-Time
>> +  Unit and Industrial Communication Subsystem.
>> +
>> +allOf:
>> +  - $ref: /schemas/remoteproc/ti,pru-consumer.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ti,am654-icssg-prueth  # for AM65x SoC family
>> +
>> +  ti,sram:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      phandle to MSMC SRAM node
>> +
>> +  dmas:
>> +    maxItems: 10
>> +
>> +  dma-names:
>> +    items:
>> +      - const: tx0-0
>> +      - const: tx0-1
>> +      - const: tx0-2
>> +      - const: tx0-3
>> +      - const: tx1-0
>> +      - const: tx1-1
>> +      - const: tx1-2
>> +      - const: tx1-3
>> +      - const: rx0
>> +      - const: rx1
>> +
>> +  ethernet-ports:
> 
> Bring some order or logic in the order of the properties. Keep the
> ethernet-ports as last property.
> 

Sure, I will re-arrange them.

>> +    type: object
>> +    additionalProperties: false
> 
> Blank line
> 
>> +    properties:
>> +      '#address-cells':
>> +        const: 1
>> +      '#size-cells':
>> +        const: 0
>> +
>> +    patternProperties:
>> +      ^port@[0-1]$:
>> +        type: object
>> +        description: ICSSG PRUETH external ports
>> +

At least one ethernet port is required. Should I add the below line here for this?

   minItems: 1

>> Drop blank line
> 
>> +        $ref: ethernet-controller.yaml#
>> +
> 
> Drop blank line
> 
>> +        unevaluatedProperties: false
>> +
>> +        properties:
>> +          reg:
>> +            items:
>> +              - enum: [0, 1]
>> +            description: ICSSG PRUETH port number
>> +
>> +          interrupts-extended:
> 
> Just "interrupts"

I will drop / add blank lines and change this to just "interrupts".

>> +            maxItems: 1
>> +
>> +          ti,syscon-rgmii-delay:
>> +            items:
>> +              - items:
>> +                  - description: phandle to system controller node
>> +                  - description: The offset to ICSSG control register
>> +            $ref: /schemas/types.yaml#/definitions/phandle-array
>> +            description:
>> +              phandle to system controller node and register offset
>> +              to ICSSG control register for RGMII transmit delay
>> +
>> +        required:
>> +          - reg
> 
> required for ethernet-ports - at least one port is required, isn't it?

Yes, at least one ethernet port is required. Should I add "minItems: 1" in
patternProperties section or should I add a new required section in
patternProperties and mention something like port@0 as required?

>> +
>> +  ti,mii-g-rt:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: |
>> +      phandle to MII_G_RT module's syscon regmap.
>> +
>> +  ti,mii-rt:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: |
>> +      phandle to MII_RT module's syscon regmap
>> +
>> +  interrupts:
>> +    maxItems: 2
>> +    description: |
>> +      Interrupt specifiers to TX timestamp IRQ.
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: tx_ts0
>> +      - const: tx_ts1
>> +
>> +required:
>> +  - compatible
>> +  - ti,sram
>> +  - dmas
>> +  - dma-names
>> +  - ethernet-ports
>> +  - ti,mii-g-rt
>> +  - interrupts
>> +  - interrupt-names
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    /* Example k3-am654 base board SR2.0, dual-emac */
>> +    pruss2_eth: ethernet {
>> +        compatible = "ti,am654-icssg-prueth";
>> +        pinctrl-names = "default";
>> +        pinctrl-0 = <&icssg2_rgmii_pins_default>;
>> +        ti,sram = <&msmc_ram>;
>> +
>> +        ti,prus = <&pru2_0>, <&rtu2_0>, <&tx_pru2_0>,
>> +                  <&pru2_1>, <&rtu2_1>, <&tx_pru2_1>;
>> +        firmware-name = "ti-pruss/am65x-pru0-prueth-fw.elf",
>> +                        "ti-pruss/am65x-rtu0-prueth-fw.elf",
>> +                        "ti-pruss/am65x-txpru0-prueth-fw.elf",
>> +                        "ti-pruss/am65x-pru1-prueth-fw.elf",
>> +                        "ti-pruss/am65x-rtu1-prueth-fw.elf",
>> +                        "ti-pruss/am65x-txpru1-prueth-fw.elf";
>> +        ti,pruss-gp-mux-sel = <2>,      /* MII mode */
>> +                              <2>,
>> +                              <2>,
>> +                              <2>,      /* MII mode */
>> +                              <2>,
>> +                              <2>;
>> +        dmas = <&main_udmap 0xc300>, /* egress slice 0 */
>> +               <&main_udmap 0xc301>, /* egress slice 0 */
>> +               <&main_udmap 0xc302>, /* egress slice 0 */
>> +               <&main_udmap 0xc303>, /* egress slice 0 */
>> +               <&main_udmap 0xc304>, /* egress slice 1 */
>> +               <&main_udmap 0xc305>, /* egress slice 1 */
>> +               <&main_udmap 0xc306>, /* egress slice 1 */
>> +               <&main_udmap 0xc307>, /* egress slice 1 */
>> +               <&main_udmap 0x4300>, /* ingress slice 0 */
>> +               <&main_udmap 0x4301>; /* ingress slice 1 */
>> +        dma-names = "tx0-0", "tx0-1", "tx0-2", "tx0-3",
>> +                    "tx1-0", "tx1-1", "tx1-2", "tx1-3",
>> +                    "rx0", "rx1";
>> +        ti,mii-g-rt = <&icssg2_mii_g_rt>;
>> +        interrupts = <24 0 2>, <25 1 3>;
> 
> Aren't you open-coding some IRQ flags?
> 

These values are not open coded here. These values are the expected values for
interrupt node.

Here,

Cell 1 -> PRU System event number
Cell 2 -> PRU channel
Cell 3 -> PRU host_event (target)

This is documented in detail in
Documentation/devicetree/bindings/interrupt-controller/ti,pruss-intc.yaml

I also missed mentioning "interrupt-parent" in example section. I will add
interrupt-parent in next revision.

interrupt-parent = <&icssg2_intc>;

>> +        interrupt-names = "tx_ts0", "tx_ts1";
>> +        ethernet-ports {
> 
> 
> Best regards,
> Krzysztof
> 

-- 
Thanks and Regards,
Danish.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [PATCH v4 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings

[Krzysztof Kozlowski]

On 06/02/2023 11:39, Md Danish Anwar wrote:
>>> +    properties:
>>> +      '#address-cells':
>>> +        const: 1
>>> +      '#size-cells':
>>> +        const: 0
>>> +
>>> +    patternProperties:
>>> +      ^port@[0-1]$:
>>> +        type: object
>>> +        description: ICSSG PRUETH external ports
>>> +
> 
> At least one ethernet port is required. Should I add the below line here for this?
> 
>    minItems: 1

You need after the patternProperties:
    anyOf:
      - required:
          - port@0
      - required:
          - port@1

> 
> 

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings

[Rob Herring]

On Mon, 06 Feb 2023 11:37:07 +0530, MD Danish Anwar wrote:
> From: Puranjay Mohan <p-mohan@ti.com>
> 
> Add a YAML binding document for the ICSSG Programmable real time unit
> based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs
> to interface the PRUs and load/run the firmware for supporting ethernet
> functionality.
> 
> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> Signed-off-by: Md Danish Anwar <danishanwar@ti.com>
> ---
>  .../bindings/net/ti,icssg-prueth.yaml         | 179 ++++++++++++++++++
>  1 file changed, 179 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
> 

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:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/remoteproc/ti,pru-consumer.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.example.dtb: ethernet: False schema does not allow {'compatible': ['ti,am654-icssg-prueth'], 'pinctrl-names': ['default'], 'pinctrl-0': [[4294967295]], 'ti,sram': [[4294967295]], 'ti,prus': [[4294967295, 4294967295, 4294967295, 4294967295, 4294967295, 4294967295]], 'firmware-name': ['ti-pruss/am65x-pru0-prueth-fw.elf', 'ti-pruss/am65x-rtu0-prueth-fw.elf', 'ti-pruss/am65x-txpru0-prueth-fw.elf', 'ti-pruss/am65x-pru1-prueth-fw.elf', 'ti-pruss/am65x-rtu1-prueth-fw.elf', 'ti-pruss/am65x-txpru1-prueth-fw.elf'], 'ti,pruss-gp-mux-sel': [[2, 2, 2, 2, 2, 2]], 'dmas': [[4294967295, 49920], [4294967295, 49921], [4294967295, 49922], [4294967295, 49923], [4294967295, 49924], [4294967295, 49925], [4294967295, 49926], [4294967295, 49927], [4294967295, 17152], [4294967295, 17153]], 'dma-names': ['tx0-0', 'tx0-1', 'tx0-2', 'tx0-3', 'tx1-0', 'tx1-1', 'tx1-2', 'tx1-3', 'rx0', 'rx1'], 'ti,mii-g-rt': [[4294967295]], 'interrupts': [[24, 0, 2], [25, 1, 3]], 'interrupt-names': ['tx_ts0', 'tx_ts1'], 'ethernet-ports': {'#address-cells': [[1]], '#size-cells': [[0]], 'port@0': {'reg': [[0]], 'phy-handle': [[4294967295]], 'phy-mode': ['rgmii-id'], 'interrupts-extended': [[4294967295, 24]], 'ti,syscon-rgmii-delay': [[4294967295, 16672]], 'local-mac-address': [[0, 0, 0, 0, 0, 0]]}, 'port@1': {'reg': [[1]], 'phy-handle': [[4294967295]], 'phy-mode': ['rgmii-id'], 'interrupts-extended': [[4294967295, 25]], 'ti,syscon-rgmii-delay': [[4294967295, 16676]], 'local-mac-address': [[0, 0, 0, 0, 0, 0]]}}, '$nodename': ['ethernet']}
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.example.dtb: ethernet: Unevaluated properties are not allowed ('firmware-name', 'ti,prus', 'ti,pruss-gp-mux-sel' were unexpected)
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230206060708.3574472-2-danishanwar@ti.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.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Andrew Lunn]

> +enum mii_mode {
> +	MII_MODE_MII = 0,
> +	MII_MODE_RGMII,
> +	MII_MODE_SGMII

There is no mention of SGMII anywhere else. And in a couple of places,
the code makes the assumption that if it is not RGMII it is MII.

Does the hardware really support SGMII?

> +static int prueth_config_rgmiidelay(struct prueth *prueth,
> +				    struct device_node *eth_np,
> +				    phy_interface_t phy_if)
> +{

...

> +	if (phy_if == PHY_INTERFACE_MODE_RGMII_ID ||
> +	    phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
> +		rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE;
> +
> +	regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id);

Here you are adding the TX delay if the phy-mode indicates it should
be added.

> +static int prueth_netdev_init(struct prueth *prueth,
> +			      struct device_node *eth_node)
> +{

> +	ret = of_get_phy_mode(eth_node, &emac->phy_if);
> +	if (ret) {
> +		dev_err(prueth->dev, "could not get phy-mode property\n");
> +		goto free;
> +	}

> +	ret = prueth_config_rgmiidelay(prueth, eth_node, emac->phy_if);
> +	if (ret)
> +		goto free;
> +

Reading it from DT and calling the delay function.

> +static int prueth_probe(struct platform_device *pdev)
> +{


> +	/* register the network devices */
> +	if (eth0_node) {
> +		ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev);
> +		if (ret) {
> +			dev_err(dev, "can't register netdev for port MII0");
> +			goto netdev_exit;
> +		}
> +
> +		prueth->registered_netdevs[PRUETH_MAC0] = prueth->emac[PRUETH_MAC0]->ndev;
> +
> +		emac_phy_connect(prueth->emac[PRUETH_MAC0]);

And this is connecting the MAC and the PHY, where emac_phy_connect()
passes emac->phy_if to phylib.

What i don't see anywhere is you changing emac->phy_if to indicate the
MAC has inserted the TX delay, and so the PHY should not.

    Andrew


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [PATCH v4 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings

[Md Danish Anwar]

Hi Rob,

On 06/02/23 19:16, Rob Herring wrote:
> 
> On Mon, 06 Feb 2023 11:37:07 +0530, MD Danish Anwar wrote:
>> From: Puranjay Mohan <p-mohan@ti.com>
>>
>> Add a YAML binding document for the ICSSG Programmable real time unit
>> based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs
>> to interface the PRUs and load/run the firmware for supporting ethernet
>> functionality.
>>
>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>> Signed-off-by: Md Danish Anwar <danishanwar@ti.com>
>> ---
>>  .../bindings/net/ti,icssg-prueth.yaml         | 179 ++++++++++++++++++
>>  1 file changed, 179 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
>>
> 
> 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:
> 
> dtschema/dtc warnings/errors:
> ./Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/remoteproc/ti,pru-consumer.yaml

This error is coming because this series depends on series [1]. The
ti,pru-consumer.yaml is upstreamed through series [1]. The series is approved
and will hopefully be merged soon.

[1] https://lore.kernel.org/all/20230106121046.886863-1-danishanwar@ti.com/

> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.example.dtb: ethernet: False schema does not allow {'compatible': ['ti,am654-icssg-prueth'], 'pinctrl-names': ['default'], 'pinctrl-0': [[4294967295]], 'ti,sram': [[4294967295]], 'ti,prus': [[4294967295, 4294967295, 4294967295, 4294967295, 4294967295, 4294967295]], 'firmware-name': ['ti-pruss/am65x-pru0-prueth-fw.elf', 'ti-pruss/am65x-rtu0-prueth-fw.elf', 'ti-pruss/am65x-txpru0-prueth-fw.elf', 'ti-pruss/am65x-pru1-prueth-fw.elf', 'ti-pruss/am65x-rtu1-prueth-fw.elf', 'ti-pruss/am65x-txpru1-prueth-fw.elf'], 'ti,pruss-gp-mux-sel': [[2, 2, 2, 2, 2, 2]], 'dmas': [[4294967295, 49920], [4294967295, 49921], [4294967295, 49922], [4294967295, 49923], [4294967295, 49924], [4294967295, 49925], [4294967295, 49926], [4294967295, 49927], [4294967295, 17152], [4294967295, 17153]], 'dma-names': ['tx0-0', 'tx0-1', 'tx0-2', 'tx0-3', 'tx1-0', 'tx1-1', 'tx1-2', 'tx1-3', 'rx0', 'rx1'], 'ti,mii-g-rt': [[429!
>  4967295]], 'interrupts': [[24, 0, 2], [25, 1, 3]], 'interrupt-names': ['tx_ts0', 'tx_ts1'], 'ethernet-ports': {'#address-cells': [[1]], '#size-cells': [[0]], 'port@0': {'reg': [[0]], 'phy-handle': [[4294967295]], 'phy-mode': ['rgmii-id'], 'interrupts-extended': [[4294967295, 24]], 'ti,syscon-rgmii-delay': [[4294967295, 16672]], 'local-mac-address': [[0, 0, 0, 0, 0, 0]]}, 'port@1': {'reg': [[1]], 'phy-handle': [[4294967295]], 'phy-mode': ['rgmii-id'], 'interrupts-extended': [[4294967295, 25]], 'ti,syscon-rgmii-delay': [[4294967295, 16676]], 'local-mac-address': [[0, 0, 0, 0, 0, 0]]}}, '$nodename': ['ethernet']}
> 	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.example.dtb: ethernet: Unevaluated properties are not allowed ('firmware-name', 'ti,prus', 'ti,pruss-gp-mux-sel' were unexpected)
> 	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230206060708.3574472-2-danishanwar@ti.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.
> 

-- 
Thanks and Regards,
Danish.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [PATCH v4 1/2] dt-bindings: net: Add ICSSG Ethernet Driver bindings

[Md Danish Anwar]

Hi Krzysztof,

On 06/02/23 16:11, Krzysztof Kozlowski wrote:
> On 06/02/2023 11:39, Md Danish Anwar wrote:
>>>> +    properties:
>>>> +      '#address-cells':
>>>> +        const: 1
>>>> +      '#size-cells':
>>>> +        const: 0
>>>> +
>>>> +    patternProperties:
>>>> +      ^port@[0-1]$:
>>>> +        type: object
>>>> +        description: ICSSG PRUETH external ports
>>>> +
>>
>> At least one ethernet port is required. Should I add the below line here for this?
>>
>>    minItems: 1
> 
> You need after the patternProperties:
>     anyOf:
>       - required:
>           - port@0
>       - required:
>           - port@1
> 


Is this correct?

  ethernet-ports:
    type: object
    additionalProperties: false

    properties:
      '#address-cells':
        const: 1
      '#size-cells':
        const: 0

    patternProperties:
      ^port@[0-1]$:
        type: object
        description: ICSSG PRUETH external ports
        $ref: ethernet-controller.yaml#
        unevaluatedProperties: false

        properties:
          reg:
            items:
              - enum: [0, 1]
            description: ICSSG PRUETH port number

          interrupts:
            maxItems: 1

          ti,syscon-rgmii-delay:
            items:
              - items:
                  - description: phandle to system controller node
                  - description: The offset to ICSSG control register
            $ref: /schemas/types.yaml#/definitions/phandle-array
            description:
              phandle to system controller node and register offset
              to ICSSG control register for RGMII transmit delay

        required:
          - reg
    anyOf:
      - required:
          - port@0
      - required:
          - port@1

Adding anyOf just below patternProperties was throwing error, so I added anyOf
after the end of patternProperties. Please let me know if this looks OK.

>>
>>
> 
> Best regards,
> Krzysztof
> 

-- 
Thanks and Regards,
Danish.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [PATCH v4 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Md Danish Anwar]

Hi Andrew,

On 06/02/23 19:45, Andrew Lunn wrote:
>> +enum mii_mode {
>> +	MII_MODE_MII = 0,
>> +	MII_MODE_RGMII,
>> +	MII_MODE_SGMII
> 
> There is no mention of SGMII anywhere else. And in a couple of places,
> the code makes the assumption that if it is not RGMII it is MII.
> 
> Does the hardware really support SGMII?
> 

As far as I know, the hardware does support SGMII but it's not yet supported by
the driver. I will drop the SGMII because it's not needed as of now. If in
future support for SGMII is there, I'll add it.

>> +static int prueth_config_rgmiidelay(struct prueth *prueth,
>> +				    struct device_node *eth_np,
>> +				    phy_interface_t phy_if)
>> +{
> 
> ...
> 
>> +	if (phy_if == PHY_INTERFACE_MODE_RGMII_ID ||
>> +	    phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
>> +		rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE;
>> +
>> +	regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id);
> 
> Here you are adding the TX delay if the phy-mode indicates it should
> be added.
> 
>> +static int prueth_netdev_init(struct prueth *prueth,
>> +			      struct device_node *eth_node)
>> +{
> 
>> +	ret = of_get_phy_mode(eth_node, &emac->phy_if);
>> +	if (ret) {
>> +		dev_err(prueth->dev, "could not get phy-mode property\n");
>> +		goto free;
>> +	}
> 
>> +	ret = prueth_config_rgmiidelay(prueth, eth_node, emac->phy_if);
>> +	if (ret)
>> +		goto free;
>> +
> 
> Reading it from DT and calling the delay function.
> 
>> +static int prueth_probe(struct platform_device *pdev)
>> +{
> 
> 
>> +	/* register the network devices */
>> +	if (eth0_node) {
>> +		ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev);
>> +		if (ret) {
>> +			dev_err(dev, "can't register netdev for port MII0");
>> +			goto netdev_exit;
>> +		}
>> +
>> +		prueth->registered_netdevs[PRUETH_MAC0] = prueth->emac[PRUETH_MAC0]->ndev;
>> +
>> +		emac_phy_connect(prueth->emac[PRUETH_MAC0]);
> 
> And this is connecting the MAC and the PHY, where emac_phy_connect()
> passes emac->phy_if to phylib.
> 
> What i don't see anywhere is you changing emac->phy_if to indicate the
> MAC has inserted the TX delay, and so the PHY should not.
> 

Yes, there is no indication whether MAC has enabled TX delay or not. I have
changed the phy-mode in DT from "rgmii-rxid" to "rgmii-id" as per your
suggestion in previous revision. I will keep Tx Internal delay as it is(getting
configured in MAC) and inside emac_phy_connect() API, while calling
of_phy_connect() instead of passing emac->phy_if (which is rgmii-id as per DT),
I will pass PHY_INTERFACE_MODE_RGMII_RXID. This will make sure that phy only
enables Rx delay and keep the existing approach of keepping Tx delay in MAC.

Currently, in emac_phy_connect() API,

	/* connect PHY */
	ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node,
				      &emac_adjust_link, 0,
				      emac->phy_if);
I will change it to,

	/* connect PHY */
	ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node,
				      &emac_adjust_link, 0,
				      PHY_INTERFACE_MODE_RGMII_RXID);

Let me know if this looks OK.

>     Andrew
> 

-- 
Thanks and Regards,
Danish.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [PATCH v4 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Roger Quadros]

Danish,

On 07/02/2023 17:29, Md Danish Anwar wrote:
> Hi Andrew,
> 
> On 06/02/23 19:45, Andrew Lunn wrote:
>>> +enum mii_mode {
>>> +	MII_MODE_MII = 0,
>>> +	MII_MODE_RGMII,
>>> +	MII_MODE_SGMII
>>
>> There is no mention of SGMII anywhere else. And in a couple of places,
>> the code makes the assumption that if it is not RGMII it is MII.
>>
>> Does the hardware really support SGMII?
>>
> 
> As far as I know, the hardware does support SGMII but it's not yet supported by
> the driver. I will drop the SGMII because it's not needed as of now. If in
> future support for SGMII is there, I'll add it.
> 
>>> +static int prueth_config_rgmiidelay(struct prueth *prueth,
>>> +				    struct device_node *eth_np,
>>> +				    phy_interface_t phy_if)
>>> +{
>>
>> ...
>>
>>> +	if (phy_if == PHY_INTERFACE_MODE_RGMII_ID ||
>>> +	    phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
>>> +		rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE;
>>> +
>>> +	regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id);

This is only applicable to some devices so you need to restrict this only
to those devices.

And only when you enable MAC TX delay you need to change emac->phy_if to PHY_INTERFACE_MODE_RGMII_RXID
if it was PHY_INTERFACE_MODE_RGMII_ID.

>>
>> Here you are adding the TX delay if the phy-mode indicates it should
>> be added.
>>
>>> +static int prueth_netdev_init(struct prueth *prueth,
>>> +			      struct device_node *eth_node)
>>> +{
>>
>>> +	ret = of_get_phy_mode(eth_node, &emac->phy_if);
>>> +	if (ret) {
>>> +		dev_err(prueth->dev, "could not get phy-mode property\n");
>>> +		goto free;
>>> +	}
>>
>>> +	ret = prueth_config_rgmiidelay(prueth, eth_node, emac->phy_if);
>>> +	if (ret)
>>> +		goto free;
>>> +
>>
>> Reading it from DT and calling the delay function.
>>
>>> +static int prueth_probe(struct platform_device *pdev)
>>> +{
>>
>>
>>> +	/* register the network devices */
>>> +	if (eth0_node) {
>>> +		ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev);
>>> +		if (ret) {
>>> +			dev_err(dev, "can't register netdev for port MII0");
>>> +			goto netdev_exit;
>>> +		}
>>> +
>>> +		prueth->registered_netdevs[PRUETH_MAC0] = prueth->emac[PRUETH_MAC0]->ndev;
>>> +
>>> +		emac_phy_connect(prueth->emac[PRUETH_MAC0]);
>>
>> And this is connecting the MAC and the PHY, where emac_phy_connect()
>> passes emac->phy_if to phylib.
>>
>> What i don't see anywhere is you changing emac->phy_if to indicate the
>> MAC has inserted the TX delay, and so the PHY should not.
>>
> 
> Yes, there is no indication whether MAC has enabled TX delay or not. I have
> changed the phy-mode in DT from "rgmii-rxid" to "rgmii-id" as per your
> suggestion in previous revision. I will keep Tx Internal delay as it is(getting
> configured in MAC) and inside emac_phy_connect() API, while calling
> of_phy_connect() instead of passing emac->phy_if (which is rgmii-id as per DT),
> I will pass PHY_INTERFACE_MODE_RGMII_RXID. This will make sure that phy only
> enables Rx delay and keep the existing approach of keepping Tx delay in MAC.
> 
> Currently, in emac_phy_connect() API,
> 
> 	/* connect PHY */
> 	ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node,
> 				      &emac_adjust_link, 0,
> 				      emac->phy_if);
> I will change it to,
> 
> 	/* connect PHY */
> 	ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node,
> 				      &emac_adjust_link, 0,
> 				      PHY_INTERFACE_MODE_RGMII_RXID);
> 
> Let me know if this looks OK.

No, this is not OK.

Please keep this as emac->phy_if.

In prueth_config_rgmiidelay(), you can change emac->phy_if to
PHY_INTERFACE_MODE_RGMII_RXID only if it was RGMII mode
*and* MAC TX delay was enabled.

cheers,
-roger

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Md Danish Anwar]

Hi Roger and Andrew,

On 08/02/23 01:26, Roger Quadros wrote:
> Danish,
> 
> On 07/02/2023 17:29, Md Danish Anwar wrote:
>> Hi Andrew,
>>
>> On 06/02/23 19:45, Andrew Lunn wrote:
>>>> +enum mii_mode {
>>>> +	MII_MODE_MII = 0,
>>>> +	MII_MODE_RGMII,
>>>> +	MII_MODE_SGMII
>>>
>>> There is no mention of SGMII anywhere else. And in a couple of places,
>>> the code makes the assumption that if it is not RGMII it is MII.
>>>
>>> Does the hardware really support SGMII?
>>>
>>
>> As far as I know, the hardware does support SGMII but it's not yet supported by
>> the driver. I will drop the SGMII because it's not needed as of now. If in
>> future support for SGMII is there, I'll add it.
>>
>>>> +static int prueth_config_rgmiidelay(struct prueth *prueth,
>>>> +				    struct device_node *eth_np,
>>>> +				    phy_interface_t phy_if)
>>>> +{
>>>
>>> ...
>>>
>>>> +	if (phy_if == PHY_INTERFACE_MODE_RGMII_ID ||
>>>> +	    phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
>>>> +		rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE;
>>>> +
>>>> +	regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id);
> 
> This is only applicable to some devices so you need to restrict this only
> to those devices.
> 

Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't
think there is any need for any device related restriction. Once support for
other devices are enabled for upstream, we can modify this accordingly.

> And only when you enable MAC TX delay you need to change emac->phy_if to PHY_INTERFACE_MODE_RGMII_RXID
> if it was PHY_INTERFACE_MODE_RGMII_ID.
> 
>>>
>>> Here you are adding the TX delay if the phy-mode indicates it should
>>> be added.
>>>
>>>> +static int prueth_netdev_init(struct prueth *prueth,
>>>> +			      struct device_node *eth_node)
>>>> +{
>>>
>>>> +	ret = of_get_phy_mode(eth_node, &emac->phy_if);
>>>> +	if (ret) {
>>>> +		dev_err(prueth->dev, "could not get phy-mode property\n");
>>>> +		goto free;
>>>> +	}
>>>
>>>> +	ret = prueth_config_rgmiidelay(prueth, eth_node, emac->phy_if);
>>>> +	if (ret)
>>>> +		goto free;
>>>> +
>>>
>>> Reading it from DT and calling the delay function.
>>>
>>>> +static int prueth_probe(struct platform_device *pdev)
>>>> +{
>>>
>>>
>>>> +	/* register the network devices */
>>>> +	if (eth0_node) {
>>>> +		ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev);
>>>> +		if (ret) {
>>>> +			dev_err(dev, "can't register netdev for port MII0");
>>>> +			goto netdev_exit;
>>>> +		}
>>>> +
>>>> +		prueth->registered_netdevs[PRUETH_MAC0] = prueth->emac[PRUETH_MAC0]->ndev;
>>>> +
>>>> +		emac_phy_connect(prueth->emac[PRUETH_MAC0]);
>>>
>>> And this is connecting the MAC and the PHY, where emac_phy_connect()
>>> passes emac->phy_if to phylib.
>>>
>>> What i don't see anywhere is you changing emac->phy_if to indicate the
>>> MAC has inserted the TX delay, and so the PHY should not.
>>>
>>
>> Yes, there is no indication whether MAC has enabled TX delay or not. I have
>> changed the phy-mode in DT from "rgmii-rxid" to "rgmii-id" as per your
>> suggestion in previous revision. I will keep Tx Internal delay as it is(getting
>> configured in MAC) and inside emac_phy_connect() API, while calling
>> of_phy_connect() instead of passing emac->phy_if (which is rgmii-id as per DT),
>> I will pass PHY_INTERFACE_MODE_RGMII_RXID. This will make sure that phy only
>> enables Rx delay and keep the existing approach of keepping Tx delay in MAC.
>>
>> Currently, in emac_phy_connect() API,
>>
>> 	/* connect PHY */
>> 	ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node,
>> 				      &emac_adjust_link, 0,
>> 				      emac->phy_if);
>> I will change it to,
>>
>> 	/* connect PHY */
>> 	ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node,
>> 				      &emac_adjust_link, 0,
>> 				      PHY_INTERFACE_MODE_RGMII_RXID);
>>
>> Let me know if this looks OK.
> 
> No, this is not OK.
> 
> Please keep this as emac->phy_if.
> 
> In prueth_config_rgmiidelay(), you can change emac->phy_if to
> PHY_INTERFACE_MODE_RGMII_RXID only if it was RGMII mode
> *and* MAC TX delay was enabled.
> 

I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table
5-624) for AM65 Silicon Revision 2.0.

Below is the description in Table 5-624

BIT	    : 24
Field	    : RGMII0_ID_MODE
Type	    : R/W
Reset	    : 0h
Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay
	      0h - Internal transmit delay is enabled
	      1h - Reserved

The TX internal delay is always enabled and couldn't be disabled as 1h is
reserved. So hardware support for disabling TX internal delay is not there.

As, TX internal delay is always there, there is no need to enable it in MAC or
PHY. So no need of API prueth_config_rgmiidelay().

My approach to handle delay would be as below.

*) Keep phy-mode = "rgmii-id" in DT as asked by Andrew.
*) Let TX internal delay enabled in Hardware.
*) Let PHY configure RX internal delay.
*) Remove prueth_config_rgmiidelay() API is there is no use of this. TX
Internal delay is always enabled.
*) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init()
API, add below if condition.

	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID)
		emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID

This will check if phy-mode is "rgmii-id", then change the phy-mode to
"rgmii-rxid". The same emac->phy_if is passed to emac_phy_connect which passes
emac->phy_if to phylib. So that PHY will only enable RX internal delay and TX
internal delay is always enabled by default(in Hardware)

As of now ICSSG driver is getting upstreamed with support for only AM65 SR2.0,
In future when ICSSG driver starts supporting other devices, we can modify this
condition to something like below

	if(device_is_AM65 && emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID)
		emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID

So that, for other devices, phy-mode should remain as it is, as other devices
don't have hardware restriction.

Please let me know if this is the right approach.

[1]
https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf?ts=1675841023830&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FAM6548#%5B%7B%22num%22%3A1706%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2C0%2C717.9%2C0%5D

> cheers,
> -roger
-- 
Thanks and Regards,
Danish.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Roger Quadros]

Danish,

On 08/02/2023 09:46, Md Danish Anwar wrote:
> Hi Roger and Andrew,
> 
> On 08/02/23 01:26, Roger Quadros wrote:
>> Danish,
>>
>> On 07/02/2023 17:29, Md Danish Anwar wrote:
>>> Hi Andrew,
>>>
>>> On 06/02/23 19:45, Andrew Lunn wrote:
>>>>> +enum mii_mode {
>>>>> +	MII_MODE_MII = 0,
>>>>> +	MII_MODE_RGMII,
>>>>> +	MII_MODE_SGMII
>>>>
>>>> There is no mention of SGMII anywhere else. And in a couple of places,
>>>> the code makes the assumption that if it is not RGMII it is MII.
>>>>
>>>> Does the hardware really support SGMII?
>>>>
>>>
>>> As far as I know, the hardware does support SGMII but it's not yet supported by
>>> the driver. I will drop the SGMII because it's not needed as of now. If in
>>> future support for SGMII is there, I'll add it.
>>>
>>>>> +static int prueth_config_rgmiidelay(struct prueth *prueth,
>>>>> +				    struct device_node *eth_np,
>>>>> +				    phy_interface_t phy_if)
>>>>> +{
>>>>
>>>> ...
>>>>
>>>>> +	if (phy_if == PHY_INTERFACE_MODE_RGMII_ID ||
>>>>> +	    phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
>>>>> +		rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE;
>>>>> +
>>>>> +	regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id);
>>
>> This is only applicable to some devices so you need to restrict this only
>> to those devices.
>>
> 
> Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't
> think there is any need for any device related restriction. Once support for
> other devices are enabled for upstream, we can modify this accordingly.

OK then please get rid of prueth_config_rgmiidelay() entirely.

> 
>> And only when you enable MAC TX delay you need to change emac->phy_if to PHY_INTERFACE_MODE_RGMII_RXID
>> if it was PHY_INTERFACE_MODE_RGMII_ID.
>>
>>>>
>>>> Here you are adding the TX delay if the phy-mode indicates it should
>>>> be added.
>>>>
>>>>> +static int prueth_netdev_init(struct prueth *prueth,
>>>>> +			      struct device_node *eth_node)
>>>>> +{
>>>>
>>>>> +	ret = of_get_phy_mode(eth_node, &emac->phy_if);
>>>>> +	if (ret) {
>>>>> +		dev_err(prueth->dev, "could not get phy-mode property\n");
>>>>> +		goto free;
>>>>> +	}
>>>>
>>>>> +	ret = prueth_config_rgmiidelay(prueth, eth_node, emac->phy_if);
>>>>> +	if (ret)
>>>>> +		goto free;
>>>>> +
>>>>
>>>> Reading it from DT and calling the delay function.
>>>>
>>>>> +static int prueth_probe(struct platform_device *pdev)
>>>>> +{
>>>>
>>>>
>>>>> +	/* register the network devices */
>>>>> +	if (eth0_node) {
>>>>> +		ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev);
>>>>> +		if (ret) {
>>>>> +			dev_err(dev, "can't register netdev for port MII0");
>>>>> +			goto netdev_exit;
>>>>> +		}
>>>>> +
>>>>> +		prueth->registered_netdevs[PRUETH_MAC0] = prueth->emac[PRUETH_MAC0]->ndev;
>>>>> +
>>>>> +		emac_phy_connect(prueth->emac[PRUETH_MAC0]);
>>>>
>>>> And this is connecting the MAC and the PHY, where emac_phy_connect()
>>>> passes emac->phy_if to phylib.
>>>>
>>>> What i don't see anywhere is you changing emac->phy_if to indicate the
>>>> MAC has inserted the TX delay, and so the PHY should not.
>>>>
>>>
>>> Yes, there is no indication whether MAC has enabled TX delay or not. I have
>>> changed the phy-mode in DT from "rgmii-rxid" to "rgmii-id" as per your
>>> suggestion in previous revision. I will keep Tx Internal delay as it is(getting
>>> configured in MAC) and inside emac_phy_connect() API, while calling
>>> of_phy_connect() instead of passing emac->phy_if (which is rgmii-id as per DT),
>>> I will pass PHY_INTERFACE_MODE_RGMII_RXID. This will make sure that phy only
>>> enables Rx delay and keep the existing approach of keepping Tx delay in MAC.
>>>
>>> Currently, in emac_phy_connect() API,
>>>
>>> 	/* connect PHY */
>>> 	ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node,
>>> 				      &emac_adjust_link, 0,
>>> 				      emac->phy_if);
>>> I will change it to,
>>>
>>> 	/* connect PHY */
>>> 	ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node,
>>> 				      &emac_adjust_link, 0,
>>> 				      PHY_INTERFACE_MODE_RGMII_RXID);
>>>
>>> Let me know if this looks OK.
>>
>> No, this is not OK.
>>
>> Please keep this as emac->phy_if.
>>
>> In prueth_config_rgmiidelay(), you can change emac->phy_if to
>> PHY_INTERFACE_MODE_RGMII_RXID only if it was RGMII mode
>> *and* MAC TX delay was enabled.
>>
> 
> I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table
> 5-624) for AM65 Silicon Revision 2.0.
> 
> Below is the description in Table 5-624
> 
> BIT	    : 24
> Field	    : RGMII0_ID_MODE
> Type	    : R/W
> Reset	    : 0h
> Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay
> 	      0h - Internal transmit delay is enabled
> 	      1h - Reserved
> 
> The TX internal delay is always enabled and couldn't be disabled as 1h is
> reserved. So hardware support for disabling TX internal delay is not there.
> 
> As, TX internal delay is always there, there is no need to enable it in MAC or
> PHY. So no need of API prueth_config_rgmiidelay().
> 
> My approach to handle delay would be as below.
> 
> *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew.
> *) Let TX internal delay enabled in Hardware.

You mean in TX delay in MAC? Why? If Silicon does not have an issue
then it should be enabled in PHY not in MAC.

> *) Let PHY configure RX internal delay.

No. PHY should do whatever is asked in the DT. In this case both TX and RX delay.

> *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX

Agreed.

> Internal delay is always enabled.

no.

> *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init()
> API, add below if condition.
> 
> 	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID)
> 		emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID

No. This will remain emac->phy_if.

> 
> This will check if phy-mode is "rgmii-id", then change the phy-mode to
> "rgmii-rxid". The same emac->phy_if is passed to emac_phy_connect which passes
> emac->phy_if to phylib. So that PHY will only enable RX internal delay and TX
> internal delay is always enabled by default(in Hardware)
> 

No need of all this complexity.

> As of now ICSSG driver is getting upstreamed with support for only AM65 SR2.0,

OK. Then let's just deal with SR2.0 for now. 
Pre-production devices are never up-streamed.

> In future when ICSSG driver starts supporting other devices, we can modify this
> condition to something like below
> 
> 	if(device_is_AM65 && emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID)
> 		emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID
> 
> So that, for other devices, phy-mode should remain as it is, as other devices
> don't have hardware restriction.
> 
> Please let me know if this is the right approach.
> 
> [1]
> https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf?ts=1675841023830&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FAM6548#%5B%7B%22num%22%3A1706%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2C0%2C717.9%2C0%5D
> 
>> cheers,
>> -roger

cheers,
-roger.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Andrew Lunn]

> >>>> +static int prueth_config_rgmiidelay(struct prueth *prueth,
> >>>> +				    struct device_node *eth_np,
> >>>> +				    phy_interface_t phy_if)
> >>>> +{
> >>>
> >>> ...
> >>>
> >>>> +	if (phy_if == PHY_INTERFACE_MODE_RGMII_ID ||
> >>>> +	    phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
> >>>> +		rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE;
> >>>> +
> >>>> +	regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id);
> > 
> > This is only applicable to some devices so you need to restrict this only
> > to those devices.
> > 
> 
> Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't
> think there is any need for any device related restriction. Once support for
> other devices are enabled for upstream, we can modify this accordingly.

The problem is, this is a board property, not a SoC property. What if
somebody designs a board with extra long clock lines in order to add
the delay?

> I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table
> 5-624) for AM65 Silicon Revision 2.0.
> 
> Below is the description in Table 5-624
> 
> BIT	    : 24
> Field	    : RGMII0_ID_MODE
> Type	    : R/W
> Reset	    : 0h
> Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay
> 	      0h - Internal transmit delay is enabled
> 	      1h - Reserved
> 
> The TX internal delay is always enabled and couldn't be disabled as 1h is
> reserved. So hardware support for disabling TX internal delay is not there.

So if somebody passes a phy-mode which requires it disabled, you need
to return -EINVAL, to indicate the hardware cannot actually do it.

> As, TX internal delay is always there, there is no need to enable it in MAC or
> PHY. So no need of API prueth_config_rgmiidelay().
> 
> My approach to handle delay would be as below.
> 
> *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew.

As i said this depends on the board, not the SoC. In theory, you could
design a board with an extra long RX clock line, and then use phy-mode
rgmii-txid, meaning the MAC/PHY combination needs to add the TX delay.

> *) Let TX internal delay enabled in Hardware.
> *) Let PHY configure RX internal delay.
> *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX
> Internal delay is always enabled.
> *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init()
> API, add below if condition.
> 
> 	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID)
> 		emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID

You should handle all cases where a TX delay is requested, not just
ID.

	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Md Danish Anwar]

Hi Andrew,

On 08/02/23 18:26, Andrew Lunn wrote:
>>>>>> +static int prueth_config_rgmiidelay(struct prueth *prueth,
>>>>>> +				    struct device_node *eth_np,
>>>>>> +				    phy_interface_t phy_if)
>>>>>> +{
>>>>>
>>>>> ...
>>>>>
>>>>>> +	if (phy_if == PHY_INTERFACE_MODE_RGMII_ID ||
>>>>>> +	    phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
>>>>>> +		rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE;
>>>>>> +
>>>>>> +	regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id);
>>>
>>> This is only applicable to some devices so you need to restrict this only
>>> to those devices.
>>>
>>
>> Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't
>> think there is any need for any device related restriction. Once support for
>> other devices are enabled for upstream, we can modify this accordingly.
> 
> The problem is, this is a board property, not a SoC property. What if
> somebody designs a board with extra long clock lines in order to add
> the delay?
> 
>> I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table
>> 5-624) for AM65 Silicon Revision 2.0.
>>
>> Below is the description in Table 5-624
>>
>> BIT	    : 24
>> Field	    : RGMII0_ID_MODE
>> Type	    : R/W
>> Reset	    : 0h
>> Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay
>> 	      0h - Internal transmit delay is enabled
>> 	      1h - Reserved
>>
>> The TX internal delay is always enabled and couldn't be disabled as 1h is
>> reserved. So hardware support for disabling TX internal delay is not there.
> 
> So if somebody passes a phy-mode which requires it disabled, you need
> to return -EINVAL, to indicate the hardware cannot actually do it.
> 

Sure, I'll do that. In the list of all phy modes described in [1], I can only
see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other
phy-mode that requires enabling/disabling TX internal delays? Please let me
know if any other phy-mode also needs this. I will add check for that as well.

>> As, TX internal delay is always there, there is no need to enable it in MAC or
>> PHY. So no need of API prueth_config_rgmiidelay().
>>
>> My approach to handle delay would be as below.
>>
>> *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew.
> 
> As i said this depends on the board, not the SoC. In theory, you could
> design a board with an extra long RX clock line, and then use phy-mode
> rgmii-txid, meaning the MAC/PHY combination needs to add the TX delay.
> 

Yes I understand that board can have any phy-mode in it's DTS. We need to be
able to handle all different phy modes.

>> *) Let TX internal delay enabled in Hardware.
>> *) Let PHY configure RX internal delay.
>> *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX
>> Internal delay is always enabled.
>> *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init()
>> API, add below if condition.
>>
>> 	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID)
>> 		emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID
> 
> You should handle all cases where a TX delay is requested, not just
> ID.
> 

So there could be four different RGMII phy modes as described in [1]. Below is
the handling mechanism for different phy modes.

1)    # RGMII with internal RX and TX delays provided by the PHY,
      # the MAC should not add the RX or TX delays in this case
      - rgmii-id

For phy-mode="rgmii-id", phy needs to add both TX and RX internal delays. But
in our SoC TX internal delay is always enabled. So to handle this, we'll change
the phy-mode in driver to "rgmii-rxid" and then pass it ti PHY, so that PHY
will enable RX internal delay only.

2)    # RGMII with internal RX delay provided by the PHY, the MAC
      # should not add an RX delay in this case
      - rgmii-rxid

For phy-mode="rgmii-rxid", phy needs to add only RX internal delay. We will do
nothing in the driver and just pass the same mode to phy, so that PHY will
enable RX internal delay only.

3)    # RGMII with internal TX delay provided by the PHY, the MAC
      # should not add an TX delay in this case
      - rgmii-txid

For phy-mode="rgmii-txid", phy needs to add only TX internal delay,the MAC
should not add an TX delay in this case. But in our SoC TX internal delay is
always enabled. So this scenario can not be handled. We will return -EINVAL in
this case.


4)    # RX and TX delays are added by the MAC when required
      - rgmii

For phy-mode="rgmii", MAC needs to add both TX and RX delays. But in our SoC TX
internal delay is always enabled so no need to add TX delay. For RX I am not
sure what should we do as there is no provision of adding RX delay in MAC
currently. Should we ask PHY to add RX delay?

Andrew, Roger, Can you please comment on this.

Apart from Case 4, below code change will be able to handle all other cases.

	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID)
		emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID;
	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
		return -EINVAL;

Please let me know if I am missing any other phy modes.

[1] Documentation/devicetree/bindings/net/ethernet-controller.yaml

> 	Andrew

-- 
Thanks and Regards,
Danish.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Roger Quadros]

On 09/02/2023 12:29, Md Danish Anwar wrote:
> Hi Andrew,
> 
> On 08/02/23 18:26, Andrew Lunn wrote:
>>>>>>> +static int prueth_config_rgmiidelay(struct prueth *prueth,
>>>>>>> +				    struct device_node *eth_np,
>>>>>>> +				    phy_interface_t phy_if)
>>>>>>> +{
>>>>>>
>>>>>> ...
>>>>>>
>>>>>>> +	if (phy_if == PHY_INTERFACE_MODE_RGMII_ID ||
>>>>>>> +	    phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
>>>>>>> +		rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE;
>>>>>>> +
>>>>>>> +	regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id);
>>>>
>>>> This is only applicable to some devices so you need to restrict this only
>>>> to those devices.
>>>>
>>>
>>> Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't
>>> think there is any need for any device related restriction. Once support for
>>> other devices are enabled for upstream, we can modify this accordingly.
>>
>> The problem is, this is a board property, not a SoC property. What if
>> somebody designs a board with extra long clock lines in order to add
>> the delay?
>>
>>> I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table
>>> 5-624) for AM65 Silicon Revision 2.0.
>>>
>>> Below is the description in Table 5-624
>>>
>>> BIT	    : 24
>>> Field	    : RGMII0_ID_MODE
>>> Type	    : R/W
>>> Reset	    : 0h
>>> Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay
>>> 	      0h - Internal transmit delay is enabled
>>> 	      1h - Reserved
>>>
>>> The TX internal delay is always enabled and couldn't be disabled as 1h is
>>> reserved. So hardware support for disabling TX internal delay is not there.
>>
>> So if somebody passes a phy-mode which requires it disabled, you need
>> to return -EINVAL, to indicate the hardware cannot actually do it.
>>
> 
> Sure, I'll do that. In the list of all phy modes described in [1], I can only
> see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other
> phy-mode that requires enabling/disabling TX internal delays? Please let me
> know if any other phy-mode also needs this. I will add check for that as well.
> 
>>> As, TX internal delay is always there, there is no need to enable it in MAC or
>>> PHY. So no need of API prueth_config_rgmiidelay().
>>>
>>> My approach to handle delay would be as below.
>>>
>>> *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew.
>>
>> As i said this depends on the board, not the SoC. In theory, you could
>> design a board with an extra long RX clock line, and then use phy-mode
>> rgmii-txid, meaning the MAC/PHY combination needs to add the TX delay.
>>
> 
> Yes I understand that board can have any phy-mode in it's DTS. We need to be
> able to handle all different phy modes.
> 
>>> *) Let TX internal delay enabled in Hardware.
>>> *) Let PHY configure RX internal delay.
>>> *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX
>>> Internal delay is always enabled.
>>> *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init()
>>> API, add below if condition.
>>>
>>> 	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID)
>>> 		emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID
>>
>> You should handle all cases where a TX delay is requested, not just
>> ID.
>>
> 
> So there could be four different RGMII phy modes as described in [1]. Below is
> the handling mechanism for different phy modes.
> 
> 1)    # RGMII with internal RX and TX delays provided by the PHY,
>       # the MAC should not add the RX or TX delays in this case
>       - rgmii-id
> 
> For phy-mode="rgmii-id", phy needs to add both TX and RX internal delays. But
> in our SoC TX internal delay is always enabled. So to handle this, we'll change

OK. I thought that this MAC forced TX delay issue was fixed in Later Silicon Revisions.
But it looks like it hasn't been fixed yet.


> the phy-mode in driver to "rgmii-rxid" and then pass it ti PHY, so that PHY
> will enable RX internal delay only.

OK.

> 
> 2)    # RGMII with internal RX delay provided by the PHY, the MAC
>       # should not add an RX delay in this case
>       - rgmii-rxid
> 
> For phy-mode="rgmii-rxid", phy needs to add only RX internal delay. We will do
> nothing in the driver and just pass the same mode to phy, so that PHY will
> enable RX internal delay only.

But the MAC is forcing TX-delay right? So this case can't be implemented.
you have to return error.

> 
> 3)    # RGMII with internal TX delay provided by the PHY, the MAC
>       # should not add an TX delay in this case
>       - rgmii-txid
> 
> For phy-mode="rgmii-txid", phy needs to add only TX internal delay,the MAC
> should not add an TX delay in this case. But in our SoC TX internal delay is
> always enabled. So this scenario can not be handled. We will return -EINVAL in
> this case.

As you didn't return error for 1st case "rgmii-id" even though TX delay was requested
for PHY but you added it in the MAC I see no reason to return error here.

You just do the delay in MAC and pass "rgmii" to the PHY.

> 
> 
> 4)    # RX and TX delays are added by the MAC when required
>       - rgmii
> 
> For phy-mode="rgmii", MAC needs to add both TX and RX delays. But in our SoC TX
> internal delay is always enabled so no need to add TX delay. For RX I am not
> sure what should we do as there is no provision of adding RX delay in MAC
> currently. Should we ask PHY to add RX delay?
>

I don't think it will work so you can error out in this case.
 
> Andrew, Roger, Can you please comment on this.
> 
> Apart from Case 4, below code change will be able to handle all other cases.
> 
> 	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID)
> 		emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID;
> 	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
> 		return -EINVAL;
> 
> Please let me know if I am missing any other phy modes.
> 
> [1] Documentation/devicetree/bindings/net/ethernet-controller.yaml
> 
>> 	Andrew
> 

cheers,
-roger

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Md Danish Anwar]

On 09/02/23 18:28, Roger Quadros wrote:
> 
> 
> On 09/02/2023 12:29, Md Danish Anwar wrote:
>> Hi Andrew,
>>
>> On 08/02/23 18:26, Andrew Lunn wrote:
>>>>>>>> +static int prueth_config_rgmiidelay(struct prueth *prueth,
>>>>>>>> +				    struct device_node *eth_np,
>>>>>>>> +				    phy_interface_t phy_if)
>>>>>>>> +{
>>>>>>>
>>>>>>> ...
>>>>>>>
>>>>>>>> +	if (phy_if == PHY_INTERFACE_MODE_RGMII_ID ||
>>>>>>>> +	    phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
>>>>>>>> +		rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE;
>>>>>>>> +
>>>>>>>> +	regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id);
>>>>>
>>>>> This is only applicable to some devices so you need to restrict this only
>>>>> to those devices.
>>>>>
>>>>
>>>> Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't
>>>> think there is any need for any device related restriction. Once support for
>>>> other devices are enabled for upstream, we can modify this accordingly.
>>>
>>> The problem is, this is a board property, not a SoC property. What if
>>> somebody designs a board with extra long clock lines in order to add
>>> the delay?
>>>
>>>> I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table
>>>> 5-624) for AM65 Silicon Revision 2.0.
>>>>
>>>> Below is the description in Table 5-624
>>>>
>>>> BIT	    : 24
>>>> Field	    : RGMII0_ID_MODE
>>>> Type	    : R/W
>>>> Reset	    : 0h
>>>> Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay
>>>> 	      0h - Internal transmit delay is enabled
>>>> 	      1h - Reserved
>>>>
>>>> The TX internal delay is always enabled and couldn't be disabled as 1h is
>>>> reserved. So hardware support for disabling TX internal delay is not there.
>>>
>>> So if somebody passes a phy-mode which requires it disabled, you need
>>> to return -EINVAL, to indicate the hardware cannot actually do it.
>>>
>>
>> Sure, I'll do that. In the list of all phy modes described in [1], I can only
>> see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other
>> phy-mode that requires enabling/disabling TX internal delays? Please let me
>> know if any other phy-mode also needs this. I will add check for that as well.
>>
>>>> As, TX internal delay is always there, there is no need to enable it in MAC or
>>>> PHY. So no need of API prueth_config_rgmiidelay().
>>>>
>>>> My approach to handle delay would be as below.
>>>>
>>>> *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew.
>>>
>>> As i said this depends on the board, not the SoC. In theory, you could
>>> design a board with an extra long RX clock line, and then use phy-mode
>>> rgmii-txid, meaning the MAC/PHY combination needs to add the TX delay.
>>>
>>
>> Yes I understand that board can have any phy-mode in it's DTS. We need to be
>> able to handle all different phy modes.
>>
>>>> *) Let TX internal delay enabled in Hardware.
>>>> *) Let PHY configure RX internal delay.
>>>> *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX
>>>> Internal delay is always enabled.
>>>> *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init()
>>>> API, add below if condition.
>>>>
>>>> 	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID)
>>>> 		emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID
>>>
>>> You should handle all cases where a TX delay is requested, not just
>>> ID.
>>>
>>
>> So there could be four different RGMII phy modes as described in [1]. Below is
>> the handling mechanism for different phy modes.
>>
>> 1)    # RGMII with internal RX and TX delays provided by the PHY,
>>       # the MAC should not add the RX or TX delays in this case
>>       - rgmii-id
>>
>> For phy-mode="rgmii-id", phy needs to add both TX and RX internal delays. But
>> in our SoC TX internal delay is always enabled. So to handle this, we'll change
> 
> OK. I thought that this MAC forced TX delay issue was fixed in Later Silicon Revisions.
> But it looks like it hasn't been fixed yet.
> 

Yes, I confirmed with the Hardware folks, the forced TX delay issue is still
there. It's not fixed as of now.

> 
>> the phy-mode in driver to "rgmii-rxid" and then pass it ti PHY, so that PHY
>> will enable RX internal delay only.
> 
> OK.
> 

Noted.

>>
>> 2)    # RGMII with internal RX delay provided by the PHY, the MAC
>>       # should not add an RX delay in this case
>>       - rgmii-rxid
>>
>> For phy-mode="rgmii-rxid", phy needs to add only RX internal delay. We will do
>> nothing in the driver and just pass the same mode to phy, so that PHY will
>> enable RX internal delay only.
> 
> But the MAC is forcing TX-delay right? So this case can't be implemented.
> you have to return error.
> 

Yes, My bad. "rgmii-rxid" is not possible. I'll return -EINVAL here.

>>
>> 3)    # RGMII with internal TX delay provided by the PHY, the MAC
>>       # should not add an TX delay in this case
>>       - rgmii-txid
>>
>> For phy-mode="rgmii-txid", phy needs to add only TX internal delay,the MAC
>> should not add an TX delay in this case. But in our SoC TX internal delay is
>> always enabled. So this scenario can not be handled. We will return -EINVAL in
>> this case.
> 
> As you didn't return error for 1st case "rgmii-id" even though TX delay was requested
> for PHY but you added it in the MAC I see no reason to return error here.
> 
> You just do the delay in MAC and pass "rgmii" to the PHY.
> 

Yes Noted, TX delay will be forced and phy-mode "rgmii" will be passed to PHY.

>>
>>
>> 4)    # RX and TX delays are added by the MAC when required
>>       - rgmii
>>
>> For phy-mode="rgmii", MAC needs to add both TX and RX delays. But in our SoC TX
>> internal delay is always enabled so no need to add TX delay. For RX I am not
>> sure what should we do as there is no provision of adding RX delay in MAC
>> currently. Should we ask PHY to add RX delay?
>>
> 
> I don't think it will work so you can error out in this case.
>  

Sure, Noted.

So just to summarize,

For "rgmii-id" phy mode, pass "rgmii-rxid" to phy.
For "rgmii-rxid", return error.
For "rgmii-txid", TX delay is forced, pass "rgmii" to PHY.
For "rgmii", return error.


>> Andrew, Roger, Can you please comment on this.
>>
>> Apart from Case 4, below code change will be able to handle all other cases.
>>
>> 	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID)
>> 		emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID;
>> 	if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_TXID)
>> 		return -EINVAL;
>>
>> Please let me know if I am missing any other phy modes.
>>
>> [1] Documentation/devicetree/bindings/net/ethernet-controller.yaml
>>
>>> 	Andrew
>>
> 
> cheers,
> -roger

-- 
Thanks and Regards,
Danish.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Andrew Lunn]

> Sure, I'll do that. In the list of all phy modes described in [1], I can only
> see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other
> phy-mode that requires enabling/disabling TX internal delays? Please let me
> know if any other phy-mode also needs this. I will add check for that as well.

There are 4 phy-modes for RGMII.

rgmii, rgmii-id, rmgii-rxid, rgmii-txid.

rgmii-id, rgmii-txid both require TX delays. If you do that in the MAC
you then need to pass rgmii-rxid and rgmii to the PHY respectively.

rmii and rgmii-rxid requires no TX delays, which your SoC cannot do,
so you need to return -EINVAl,

The interpretation of these properties is all really messy and
historically not very uniformly done. Which is why i recommend the MAC
does nothing, leaving it to the PHY. That generally works since the
PHYs have a pretty uniform implementation. But in your case, you don't
have that option. So i suggest you do what is described above. 

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v4 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver

[Md Danish Anwar]

On 09/02/23 19:24, Andrew Lunn wrote:
>> Sure, I'll do that. In the list of all phy modes described in [1], I can only
>> see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other
>> phy-mode that requires enabling/disabling TX internal delays? Please let me
>> know if any other phy-mode also needs this. I will add check for that as well.
> 
> There are 4 phy-modes for RGMII.
> 
> rgmii, rgmii-id, rmgii-rxid, rgmii-txid.
> 
> rgmii-id, rgmii-txid both require TX delays. If you do that in the MAC
> you then need to pass rgmii-rxid and rgmii to the PHY respectively.
> 
> rmii and rgmii-rxid requires no TX delays, which your SoC cannot do,
> so you need to return -EINVAl,
> 
> The interpretation of these properties is all really messy and
> historically not very uniformly done. Which is why i recommend the MAC
> does nothing, leaving it to the PHY. That generally works since the
> PHYs have a pretty uniform implementation. But in your case, you don't
> have that option. So i suggest you do what is described above. 
> 
>     Andrew

Sure Andrew, I will post new revision with the changes described above.

-- 
Thanks and Regards,
Danish.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel