[PATCH v3 0/4] net: stmmac: dwc-qos: Add FSD EQoS support

[PATCH v3 0/4] net: stmmac: dwc-qos: Add FSD EQoS support

[Sriranjani P]

FSD platform has two instances of EQoS IP, one is in FSYS0 block and
another one is in PERIC block. This patch series add required DT binding,
DT file modifications and platform driver specific changes for the same.

Changes since v2:
1. Addressed all the review comments suggested by Krzysztof with respect to
DT binding and DTS files.
2. Added SOB Swathi for her contributions in this patch.

Sriranjani P (4):
  dt-bindings: net: Add FSD EQoS device tree bindings
  net: stmmac: dwc-qos: Add FSD EQoS support
  arm64: dts: fsd: Add Ethernet support for FSYS0 Block of FSD SoC
  arm64: dts: fsd: Add Ethernet support for PERIC Block of FSD SoC

 .../devicetree/bindings/net/snps,dwmac.yaml   |   5 +-
 .../devicetree/bindings/net/tesla,ethqos.yaml | 114 ++++++++++++
 arch/arm64/boot/dts/tesla/fsd-evb.dts         |  18 ++
 arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi    | 112 ++++++++++++
 arch/arm64/boot/dts/tesla/fsd.dtsi            |  51 ++++++
 .../stmicro/stmmac/dwmac-dwc-qos-eth.c        | 172 ++++++++++++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  28 ++-
 include/linux/stmmac.h                        |   1 +
 8 files changed, 497 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/tesla,ethqos.yaml

-- 
2.17.1

[PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Sriranjani P]

Add FSD Ethernet compatible in Synopsys dt-bindings document. Add FSD
Ethernet YAML schema to enable the DT validation.

Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Ravi Patel <ravi.patel@samsung.com>
Signed-off-by: Swathi K S <swathi.ks@samsung.com>
Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
---
 .../devicetree/bindings/net/snps,dwmac.yaml   |   5 +-
 .../devicetree/bindings/net/tesla,ethqos.yaml | 114 ++++++++++++++++++
 2 files changed, 117 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/tesla,ethqos.yaml

diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index ddf9522a5dc2..0ced7901e644 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -96,6 +96,7 @@ properties:
         - snps,dwxgmac
         - snps,dwxgmac-2.10
         - starfive,jh7110-dwmac
+        - tesla,fsd-ethqos-4.21
 
   reg:
     minItems: 1
@@ -117,7 +118,7 @@ properties:
 
   clocks:
     minItems: 1
-    maxItems: 8
+    maxItems: 10
     additionalItems: true
     items:
       - description: GMAC main clock
@@ -129,7 +130,7 @@ properties:
 
   clock-names:
     minItems: 1
-    maxItems: 8
+    maxItems: 10
     additionalItems: true
     contains:
       enum:
diff --git a/Documentation/devicetree/bindings/net/tesla,ethqos.yaml b/Documentation/devicetree/bindings/net/tesla,ethqos.yaml
new file mode 100644
index 000000000000..b78829246364
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/tesla,ethqos.yaml
@@ -0,0 +1,114 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/tesla,ethqos.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: FSD Ethernet Quality of Service
+
+maintainers:
+  - Sriranjani P <sriranjani.p@samsung.com>
+  - Swathi K S <swathi.ks@samsung.com>
+
+description:
+  dwmmac based tesla ethernet devices which support Gigabit
+  ethernet.
+
+allOf:
+  - $ref: snps,dwmac.yaml#
+
+properties:
+  compatible:
+    const: tesla,fsd-ethqos-4.21.yaml
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    minItems: 5
+    maxItems: 10
+
+  clock-names:
+    minItems: 5
+    maxItems: 10
+    items:
+      - const: ptp_ref
+      - const: master_bus
+      - const: slave_bus
+      - const: tx
+      - const: rx
+      - const: master2_bus
+      - const: slave2_bus
+      - const: eqos_rxclk_mux
+      - const: eqos_phyrxclk
+      - const: dout_peric_rgmii_clk
+
+  fsd-rx-clock-skew:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: phandle to the syscon node
+          - description: offset of the control register
+    description:
+      Should be phandle/offset pair. The phandle to the syscon node.
+
+  iommus:
+    maxItems: 1
+
+  phy-mode:
+    $ref: ethernet-controller.yaml#/properties/phy-connection-type
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - rx-clock-skew
+  - iommus
+  - phy-mode
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/fsd-clk.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    ethernet_1: ethernet@14300000 {
+              compatible = "tesla,dwc-qos-ethernet-4.21";
+              reg = <0x0 0x14300000 0x0 0x10000>;
+              interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+              clocks = <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_CLK_PTP_REF_I>,
+                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_ACLK_I>,
+                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_HCLK_I>,
+                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_RGMII_CLK_I>,
+                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_CLK_RX_I>,
+                       <&clock_peric PERIC_BUS_D_PERIC_IPCLKPORT_EQOSCLK>,
+                       <&clock_peric PERIC_BUS_P_PERIC_IPCLKPORT_EQOSCLK>,
+                       <&clock_peric PERIC_EQOS_PHYRXCLK_MUX>,
+                       <&clock_peric PERIC_EQOS_PHYRXCLK>,
+                       <&clock_peric PERIC_DOUT_RGMII_CLK>;
+              clock-names = "ptp_ref",
+                            "master_bus",
+                            "slave_bus",
+                            "tx",
+                            "rx",
+                            "master2_bus",
+                            "slave2_bus",
+                            "eqos_rxclk_mux",
+                            "eqos_phyrxclk",
+                            "dout_peric_rgmii_clk";
+              pinctrl-names = "default";
+              pinctrl-0 = <&eth1_tx_clk>, <&eth1_tx_data>, <&eth1_tx_ctrl>,
+                          <&eth1_phy_intr>, <&eth1_rx_clk>, <&eth1_rx_data>,
+                          <&eth1_rx_ctrl>, <&eth1_mdio>;
+              fsd-rx-clock-skew = <&sysreg_peric 0x10>;
+              iommus = <&smmu_peric 0x0 0x1>;
+              phy-mode = "rgmii";
+    };
+
+...
-- 
2.17.1

Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Rob Herring]

On Mon, 14 Aug 2023 16:55:36 +0530, Sriranjani P wrote:
> Add FSD Ethernet compatible in Synopsys dt-bindings document. Add FSD
> Ethernet YAML schema to enable the DT validation.
> 
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Signed-off-by: Ravi Patel <ravi.patel@samsung.com>
> Signed-off-by: Swathi K S <swathi.ks@samsung.com>
> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> ---
>  .../devicetree/bindings/net/snps,dwmac.yaml   |   5 +-
>  .../devicetree/bindings/net/tesla,ethqos.yaml | 114 ++++++++++++++++++
>  2 files changed, 117 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/tesla,ethqos.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:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/tesla,ethqos.yaml: properties:clock-names: {'minItems': 5, 'maxItems': 10, 'items': [{'const': 'ptp_ref'}, {'const': 'master_bus'}, {'const': 'slave_bus'}, {'const': 'tx'}, {'const': 'rx'}, {'const': 'master2_bus'}, {'const': 'slave2_bus'}, {'const': 'eqos_rxclk_mux'}, {'const': 'eqos_phyrxclk'}, {'const': 'dout_peric_rgmii_clk'}]} should not be valid under {'required': ['maxItems']}
	hint: "maxItems" is not needed with an "items" list
	from schema $id: http://devicetree.org/meta-schemas/items.yaml#
Documentation/devicetree/bindings/net/tesla,ethqos.example.dtb: /example-0/ethernet@14300000: failed to match any schema with compatible: ['tesla,dwc-qos-ethernet-4.21']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230814112539.70453-2-sriranjani.p@samsung.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 v3 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Sriranjani P]

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: 14 August 2023 19:03
> To: Sriranjani P <sriranjani.p@samsung.com>
> Cc: edumazet@google.com; linux-kernel@vger.kernel.org;
> alexandre.torgue@foss.st.com; ravi.patel@samsung.com;
> alim.akhtar@samsung.com; linux-samsung-soc@vger.kernel.org; linux-
> fsd@tesla.com; conor+dt@kernel.org; mcoquelin.stm32@gmail.com;
> kuba@kernel.org; netdev@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; pabeni@redhat.com; robh+dt@kernel.org;
> pankaj.dubey@samsung.com; richardcochran@gmail.com;
> krzysztof.kozlowski+dt@linaro.org; joabreu@synopsys.com;
> devicetree@vger.kernel.org; davem@davemloft.net;
> swathi.ks@samsung.com
> Subject: Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree
> bindings
> 
> 
> On Mon, 14 Aug 2023 16:55:36 +0530, Sriranjani P wrote:
> > Add FSD Ethernet compatible in Synopsys dt-bindings document. Add FSD
> > Ethernet YAML schema to enable the DT validation.
> >
> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> > Signed-off-by: Ravi Patel <ravi.patel@samsung.com>
> > Signed-off-by: Swathi K S <swathi.ks@samsung.com>
> > Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> > ---
> >  .../devicetree/bindings/net/snps,dwmac.yaml   |   5 +-
> >  .../devicetree/bindings/net/tesla,ethqos.yaml | 114
> > ++++++++++++++++++
> >  2 files changed, 117 insertions(+), 2 deletions(-)  create mode
> > 100644 Documentation/devicetree/bindings/net/tesla,ethqos.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:
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/net/tesla,ethqos.yaml:
> properties:clock-names: {'minItems': 5, 'maxItems': 10, 'items': [{'const':
> 'ptp_ref'}, {'const': 'master_bus'}, {'const': 'slave_bus'}, {'const': 'tx'}, {'const':
> 'rx'}, {'const': 'master2_bus'}, {'const': 'slave2_bus'}, {'const':
> 'eqos_rxclk_mux'}, {'const': 'eqos_phyrxclk'}, {'const':
> 'dout_peric_rgmii_clk'}]} should not be valid under {'required': ['maxItems']}
> 	hint: "maxItems" is not needed with an "items" list
> 	from schema $id: https://protect2.fireeye.com/v1/url?k=f50e335d-
> aa950a44-f50fb812-000babff3793-de26ea17ef025418&q=1&e=897786e4-
> 5f9b-40d8-8a7f-399cb69c7ee8&u=http%3A%2F%2Fdevicetree.org%2Fmeta-
> schemas%2Fitems.yaml%23
> Documentation/devicetree/bindings/net/tesla,ethqos.example.dtb:
> /example-0/ethernet@14300000: failed to match any schema with
> compatible: ['tesla,dwc-qos-ethernet-4.21']
> 

Thanks for review. Will fix this in v4.

> doc reference errors (make refcheckdocs):
> 
> See https://protect2.fireeye.com/v1/url?k=ccb7f6d0-932ccfc9-ccb67d9f-
> 000babff3793-2137ac63fe6ddef8&q=1&e=897786e4-5f9b-40d8-8a7f-
> 399cb69c7ee8&u=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fdev
> icetree-bindings%2Fpatch%2F20230814112539.70453-2-
> sriranjani.p%40samsung.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 

Sorry, I could not get this comment, can you elaborate this. 

> 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
> 
Sure will cross check.

> 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 v3 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Krzysztof Kozlowski]

On 16/08/2023 07:36, Sriranjani P wrote:
> 
> 
>> -----Original Message-----
>> From: Rob Herring [mailto:robh@kernel.org]
>> Sent: 14 August 2023 19:03
>> To: Sriranjani P <sriranjani.p@samsung.com>
>> Cc: edumazet@google.com; linux-kernel@vger.kernel.org;
>> alexandre.torgue@foss.st.com; ravi.patel@samsung.com;
>> alim.akhtar@samsung.com; linux-samsung-soc@vger.kernel.org; linux-
>> fsd@tesla.com; conor+dt@kernel.org; mcoquelin.stm32@gmail.com;
>> kuba@kernel.org; netdev@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; pabeni@redhat.com; robh+dt@kernel.org;
>> pankaj.dubey@samsung.com; richardcochran@gmail.com;
>> krzysztof.kozlowski+dt@linaro.org; joabreu@synopsys.com;
>> devicetree@vger.kernel.org; davem@davemloft.net;
>> swathi.ks@samsung.com
>> Subject: Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree
>> bindings
>>
>>
>> On Mon, 14 Aug 2023 16:55:36 +0530, Sriranjani P wrote:
>>> Add FSD Ethernet compatible in Synopsys dt-bindings document. Add FSD
>>> Ethernet YAML schema to enable the DT validation.
>>>
>>> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
>>> Signed-off-by: Ravi Patel <ravi.patel@samsung.com>
>>> Signed-off-by: Swathi K S <swathi.ks@samsung.com>
>>> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
>>> ---
>>>  .../devicetree/bindings/net/snps,dwmac.yaml   |   5 +-
>>>  .../devicetree/bindings/net/tesla,ethqos.yaml | 114
>>> ++++++++++++++++++
>>>  2 files changed, 117 insertions(+), 2 deletions(-)  create mode
>>> 100644 Documentation/devicetree/bindings/net/tesla,ethqos.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:
>> /builds/robherring/dt-review-
>> ci/linux/Documentation/devicetree/bindings/net/tesla,ethqos.yaml:
>> properties:clock-names: {'minItems': 5, 'maxItems': 10, 'items': [{'const':
>> 'ptp_ref'}, {'const': 'master_bus'}, {'const': 'slave_bus'}, {'const': 'tx'}, {'const':
>> 'rx'}, {'const': 'master2_bus'}, {'const': 'slave2_bus'}, {'const':
>> 'eqos_rxclk_mux'}, {'const': 'eqos_phyrxclk'}, {'const':
>> 'dout_peric_rgmii_clk'}]} should not be valid under {'required': ['maxItems']}
>> 	hint: "maxItems" is not needed with an "items" list
>> 	from schema $id: https://protect2.fireeye.com/v1/url?k=f50e335d-
>> aa950a44-f50fb812-000babff3793-de26ea17ef025418&q=1&e=897786e4-
>> 5f9b-40d8-8a7f-399cb69c7ee8&u=http%3A%2F%2Fdevicetree.org%2Fmeta-
>> schemas%2Fitems.yaml%23
>> Documentation/devicetree/bindings/net/tesla,ethqos.example.dtb:
>> /example-0/ethernet@14300000: failed to match any schema with
>> compatible: ['tesla,dwc-qos-ethernet-4.21']
>>
> 
> Thanks for review. Will fix this in v4.

Test the patches before sending them.

> 
>> doc reference errors (make refcheckdocs):
>>
>> See https://protect2.fireeye.com/v1/url?k=ccb7f6d0-932ccfc9-ccb67d9f-
>> 000babff3793-2137ac63fe6ddef8&q=1&e=897786e4-5f9b-40d8-8a7f-
>> 399cb69c7ee8&u=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fdev
>> icetree-bindings%2Fpatch%2F20230814112539.70453-2-
>> sriranjani.p%40samsung.com
>>
>> The base for the series is generally the latest rc1. A different dependency
>> should be noted in *this* patch.
>>
> 
> Sorry, I could not get this comment, can you elaborate this. 

What else to say? You did no stated any dependency here. The base is
explained.


> 
>> 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
>>
> Sure will cross check.

Why do you ask/comment to bot?

Best regards,
Krzysztof

Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Rob Herring]

On Wed, Aug 16, 2023 at 11:06:51AM +0530, Sriranjani P wrote:
> 
> 
> > -----Original Message-----
> > From: Rob Herring [mailto:robh@kernel.org]
> > Sent: 14 August 2023 19:03
> > To: Sriranjani P <sriranjani.p@samsung.com>
> > Cc: edumazet@google.com; linux-kernel@vger.kernel.org;
> > alexandre.torgue@foss.st.com; ravi.patel@samsung.com;
> > alim.akhtar@samsung.com; linux-samsung-soc@vger.kernel.org; linux-
> > fsd@tesla.com; conor+dt@kernel.org; mcoquelin.stm32@gmail.com;
> > kuba@kernel.org; netdev@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; pabeni@redhat.com; robh+dt@kernel.org;
> > pankaj.dubey@samsung.com; richardcochran@gmail.com;
> > krzysztof.kozlowski+dt@linaro.org; joabreu@synopsys.com;
> > devicetree@vger.kernel.org; davem@davemloft.net;
> > swathi.ks@samsung.com
> > Subject: Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree
> > bindings
> > 
> > 
> > On Mon, 14 Aug 2023 16:55:36 +0530, Sriranjani P wrote:
> > > Add FSD Ethernet compatible in Synopsys dt-bindings document. Add FSD
> > > Ethernet YAML schema to enable the DT validation.
> > >
> > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> > > Signed-off-by: Ravi Patel <ravi.patel@samsung.com>
> > > Signed-off-by: Swathi K S <swathi.ks@samsung.com>
> > > Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> > > ---
> > >  .../devicetree/bindings/net/snps,dwmac.yaml   |   5 +-
> > >  .../devicetree/bindings/net/tesla,ethqos.yaml | 114
> > > ++++++++++++++++++
> > >  2 files changed, 117 insertions(+), 2 deletions(-)  create mode
> > > 100644 Documentation/devicetree/bindings/net/tesla,ethqos.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:
> > /builds/robherring/dt-review-
> > ci/linux/Documentation/devicetree/bindings/net/tesla,ethqos.yaml:
> > properties:clock-names: {'minItems': 5, 'maxItems': 10, 'items': [{'const':
> > 'ptp_ref'}, {'const': 'master_bus'}, {'const': 'slave_bus'}, {'const': 'tx'}, {'const':
> > 'rx'}, {'const': 'master2_bus'}, {'const': 'slave2_bus'}, {'const':
> > 'eqos_rxclk_mux'}, {'const': 'eqos_phyrxclk'}, {'const':
> > 'dout_peric_rgmii_clk'}]} should not be valid under {'required': ['maxItems']}
> > 	hint: "maxItems" is not needed with an "items" list
> > 	from schema $id: https://protect2.fireeye.com/v1/url?k=f50e335d-
> > aa950a44-f50fb812-000babff3793-de26ea17ef025418&q=1&e=897786e4-
> > 5f9b-40d8-8a7f-399cb69c7ee8&u=http%3A%2F%2Fdevicetree.org%2Fmeta-
> > schemas%2Fitems.yaml%23
> > Documentation/devicetree/bindings/net/tesla,ethqos.example.dtb:
> > /example-0/ethernet@14300000: failed to match any schema with
> > compatible: ['tesla,dwc-qos-ethernet-4.21']
> > 
> 
> Thanks for review. Will fix this in v4.

It's not a review. It's an automated reply running what you should have 
run yourself...

> 
> > doc reference errors (make refcheckdocs):
> > 
> > See https://protect2.fireeye.com/v1/url?k=ccb7f6d0-932ccfc9-ccb67d9f-
> > 000babff3793-2137ac63fe6ddef8&q=1&e=897786e4-5f9b-40d8-8a7f-
> > 399cb69c7ee8&u=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fdev
> > icetree-bindings%2Fpatch%2F20230814112539.70453-2-
> > sriranjani.p%40samsung.com
> > 
> > The base for the series is generally the latest rc1. A different dependency
> > should be noted in *this* patch.
> > 
> 
> Sorry, I could not get this comment, can you elaborate this. 

The automated tests apply patches to the latest rc1 tag. Patches which 
apply, but have some other dependency may have warnings. If you have 
such a dependency, you should note it in the patch (below the '---').

Rob

Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Krzysztof Kozlowski]

On 14/08/2023 13:25, Sriranjani P wrote:
> Add FSD Ethernet compatible in Synopsys dt-bindings document. Add FSD
> Ethernet YAML schema to enable the DT validation.
> 
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Signed-off-by: Ravi Patel <ravi.patel@samsung.com>
> Signed-off-by: Swathi K S <swathi.ks@samsung.com>
> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> ---
>  .../devicetree/bindings/net/snps,dwmac.yaml   |   5 +-
>  .../devicetree/bindings/net/tesla,ethqos.yaml | 114 ++++++++++++++++++
>  2 files changed, 117 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/tesla,ethqos.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> index ddf9522a5dc2..0ced7901e644 100644
> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> @@ -96,6 +96,7 @@ properties:
>          - snps,dwxgmac
>          - snps,dwxgmac-2.10
>          - starfive,jh7110-dwmac
> +        - tesla,fsd-ethqos-4.21

I don't think one given SoC - and I was told FSD is strictly defined one
specific SoC - can have different versions of the same block, so drop
the block versioning.

>  
>    reg:
>      minItems: 1
> @@ -117,7 +118,7 @@ properties:
>  
>    clocks:
>      minItems: 1
> -    maxItems: 8
> +    maxItems: 10
>      additionalItems: true
>      items:
>        - description: GMAC main clock
> @@ -129,7 +130,7 @@ properties:
>  
>    clock-names:
>      minItems: 1
> -    maxItems: 8
> +    maxItems: 10
>      additionalItems: true
>      contains:
>        enum:
> diff --git a/Documentation/devicetree/bindings/net/tesla,ethqos.yaml b/Documentation/devicetree/bindings/net/tesla,ethqos.yaml
> new file mode 100644
> index 000000000000..b78829246364
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/tesla,ethqos.yaml
> @@ -0,0 +1,114 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/tesla,ethqos.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: FSD Ethernet Quality of Service
> +
> +maintainers:
> +  - Sriranjani P <sriranjani.p@samsung.com>
> +  - Swathi K S <swathi.ks@samsung.com>
> +
> +description:
> +  dwmmac based tesla ethernet devices which support Gigabit
> +  ethernet.
> +
> +allOf:
> +  - $ref: snps,dwmac.yaml#
> +
> +properties:
> +  compatible:
> +    const: tesla,fsd-ethqos-4.21.yaml

?

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 5

Why? I expect it to be specific.

> +    maxItems: 10
> +
> +  clock-names:
> +    minItems: 5
> +    maxItems: 10
> +    items:
> +      - const: ptp_ref
> +      - const: master_bus
> +      - const: slave_bus
> +      - const: tx
> +      - const: rx
> +      - const: master2_bus
> +      - const: slave2_bus
> +      - const: eqos_rxclk_mux
> +      - const: eqos_phyrxclk
> +      - const: dout_peric_rgmii_clk
> +
> +  fsd-rx-clock-skew:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle to the syscon node
> +          - description: offset of the control register
> +    description:
> +      Should be phandle/offset pair. The phandle to the syscon node.
> +
> +  iommus:
> +    maxItems: 1
> +
> +  phy-mode:
> +    $ref: ethernet-controller.yaml#/properties/phy-connection-type
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - rx-clock-skew

Eee? Isn't it fsd-rx-clock-skew which anyway is not correct?

> +  - iommus
> +  - phy-mode
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/fsd-clk.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    ethernet_1: ethernet@14300000 {
> +              compatible = "tesla,dwc-qos-ethernet-4.21";

Three different compatibles for the same.

No, please test your patches before sending.

I am not even checking if previous feedback was applied... Did you
really go through it?

Best regards,
Krzysztof

RE: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Sriranjani P]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 15 August 2023 01:10
> To: Sriranjani P <sriranjani.p@samsung.com>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; richardcochran@gmail.com;
> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
> mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
> fsd@tesla.com; pankaj.dubey@samsung.com; swathi.ks@samsung.com;
> ravi.patel@samsung.com
> Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree
> bindings
> 
> On 14/08/2023 13:25, Sriranjani P wrote:
> > Add FSD Ethernet compatible in Synopsys dt-bindings document. Add FSD
> > Ethernet YAML schema to enable the DT validation.
> >
> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> > Signed-off-by: Ravi Patel <ravi.patel@samsung.com>
> > Signed-off-by: Swathi K S <swathi.ks@samsung.com>
> > Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> > ---
> >  .../devicetree/bindings/net/snps,dwmac.yaml   |   5 +-
> >  .../devicetree/bindings/net/tesla,ethqos.yaml | 114
> > ++++++++++++++++++
> >  2 files changed, 117 insertions(+), 2 deletions(-)  create mode
> > 100644 Documentation/devicetree/bindings/net/tesla,ethqos.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > index ddf9522a5dc2..0ced7901e644 100644
> > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > @@ -96,6 +96,7 @@ properties:
> >          - snps,dwxgmac
> >          - snps,dwxgmac-2.10
> >          - starfive,jh7110-dwmac
> > +        - tesla,fsd-ethqos-4.21
> 
> I don't think one given SoC - and I was told FSD is strictly defined one specific
> SoC - can have different versions of the same block, so drop the block
> versioning.
> 

Ok, will remove versioning.

> >
> >    reg:
> >      minItems: 1
> > @@ -117,7 +118,7 @@ properties:
> >
> >    clocks:
> >      minItems: 1
> > -    maxItems: 8
> > +    maxItems: 10
> >      additionalItems: true
> >      items:
> >        - description: GMAC main clock
> > @@ -129,7 +130,7 @@ properties:
> >
> >    clock-names:
> >      minItems: 1
> > -    maxItems: 8
> > +    maxItems: 10
> >      additionalItems: true
> >      contains:
> >        enum:
> > diff --git a/Documentation/devicetree/bindings/net/tesla,ethqos.yaml
> > b/Documentation/devicetree/bindings/net/tesla,ethqos.yaml
> > new file mode 100644
> > index 000000000000..b78829246364
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/tesla,ethqos.yaml
> > @@ -0,0 +1,114 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause %YAML 1.2
> > +---
> > +$id:
> > +https://protect2.fireeye.com/v1/url?k=0d5f9085-6cd485b3-0d5e1bca-
> 74fe
> > +485cbff1-9835d59b137d73e5&q=1&e=93f28da2-6d86-4cc2-a07a-
> 9be1380616cc&
> >
> +u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fnet%2Ftesla%2Cethqos.
> yaml%2
> > +3
> > +$schema:
> > +https://protect2.fireeye.com/v1/url?k=67d3522a-0658471c-67d2d965-
> 74fe
> > +485cbff1-b9570dbbedf33f81&q=1&e=93f28da2-6d86-4cc2-a07a-
> 9be1380616cc&
> > +u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
> > +
> > +title: FSD Ethernet Quality of Service
> > +
> > +maintainers:
> > +  - Sriranjani P <sriranjani.p@samsung.com>
> > +  - Swathi K S <swathi.ks@samsung.com>
> > +
> > +description:
> > +  dwmmac based tesla ethernet devices which support Gigabit
> > +  ethernet.
> > +
> > +allOf:
> > +  - $ref: snps,dwmac.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: tesla,fsd-ethqos-4.21.yaml
> 
> ?

Will fix this to tesla,fsd-ethqos.yaml 

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 5
> 
> Why? I expect it to be specific.

Sorry, I could not understood this comment. In FSD we have two instances of EQoS IP, one in PERIC block, which requires total 10 clocks  to be configured and another instance exist in FSYS0 block which needs 5 clocks to be configured, so we kept minItems as 5 and maxItems as 10, but looks like latest items schema do not need maxItems entry so we will drop maxItems entry. In my understanding minItems still required so it should be kept with minimum number of clock requirements.

> 
> > +    maxItems: 10
> > +
> > +  clock-names:
> > +    minItems: 5
> > +    maxItems: 10
> > +    items:
> > +      - const: ptp_ref
> > +      - const: master_bus
> > +      - const: slave_bus
> > +      - const: tx
> > +      - const: rx
> > +      - const: master2_bus
> > +      - const: slave2_bus
> > +      - const: eqos_rxclk_mux
> > +      - const: eqos_phyrxclk
> > +      - const: dout_peric_rgmii_clk
> > +
> > +  fsd-rx-clock-skew:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    items:
> > +      - items:
> > +          - description: phandle to the syscon node
> > +          - description: offset of the control register
> > +    description:
> > +      Should be phandle/offset pair. The phandle to the syscon node.
> > +
> > +  iommus:
> > +    maxItems: 1
> > +
> > +  phy-mode:
> > +    $ref: ethernet-controller.yaml#/properties/phy-connection-type
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +  - rx-clock-skew
> 
> Eee? Isn't it fsd-rx-clock-skew which anyway is not correct?

Sorry, I missed to change this in DT schema before posting, I will make this to fsd-rx-clock-skew. 

> 
> > +  - iommus
> > +  - phy-mode
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/fsd-clk.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    ethernet_1: ethernet@14300000 {
> > +              compatible = "tesla,dwc-qos-ethernet-4.21";
> 
> Three different compatibles for the same.
> 
> No, please test your patches before sending.
> 
> I am not even checking if previous feedback was applied... Did you really go
> through it?
> 

Sorry, I missed this will take care. 

> Best regards,
> Krzysztof

Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Krzysztof Kozlowski]

On 16/08/2023 07:58, Sriranjani P wrote:
>>> +
>>> +allOf:
>>> +  - $ref: snps,dwmac.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: tesla,fsd-ethqos-4.21.yaml
>>
>> ?
> 
> Will fix this to tesla,fsd-ethqos.yaml 

Test your patches before sending. REALLY TEST.

> 
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    minItems: 5
>>
>> Why? I expect it to be specific.
> 
> Sorry, I could not understood this comment. In FSD we have two instances of EQoS IP, one in PERIC block, which requires total 10 clocks  to be configured and another instance exist in FSYS0 block which needs 5 clocks to be configured, so we kept minItems as 5 and maxItems as 10, but looks like latest items schema do not need maxItems entry so we will drop maxItems entry. In my understanding minItems still required so it should be kept with minimum number of clock requirements.

No, the code is fine then.

> 
>>
>>> +    maxItems: 10
>>> +
>>> +  clock-names:
>>> +    minItems: 5
>>> +    maxItems: 10
>>> +    items:
>>> +      - const: ptp_ref
>>> +      - const: master_bus
>>> +      - const: slave_bus
>>> +      - const: tx
>>> +      - const: rx
>>> +      - const: master2_bus
>>> +      - const: slave2_bus
>>> +      - const: eqos_rxclk_mux
>>> +      - const: eqos_phyrxclk
>>> +      - const: dout_peric_rgmii_clk
>>> +
>>> +  fsd-rx-clock-skew:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    items:
>>> +      - items:
>>> +          - description: phandle to the syscon node
>>> +          - description: offset of the control register
>>> +    description:
>>> +      Should be phandle/offset pair. The phandle to the syscon node.
>>> +
>>> +  iommus:
>>> +    maxItems: 1
>>> +
>>> +  phy-mode:
>>> +    $ref: ethernet-controller.yaml#/properties/phy-connection-type
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - clocks
>>> +  - clock-names
>>> +  - rx-clock-skew
>>
>> Eee? Isn't it fsd-rx-clock-skew which anyway is not correct?
> 
> Sorry, I missed to change this in DT schema before posting, I will make this to fsd-rx-clock-skew. 

Remember about vendor prefixes for every custom property.


Best regards,
Krzysztof

RE: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Swathi K S]

Hi Krzysztof, 

Sorry for the delay in response.
Starting now, I will be taking over this task.
I have gone through your comments and feedback and will be implementing them in v4 of this patch.

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 16 August 2023 11:48
> To: Sriranjani P <sriranjani.p@samsung.com>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; richardcochran@gmail.com;
> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
> mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
> fsd@tesla.com; pankaj.dubey@samsung.com; swathi.ks@samsung.com;
> ravi.patel@samsung.com
> Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree
> bindings
> 
> On 16/08/2023 07:58, Sriranjani P wrote:
> >>> +
> >>> +allOf:
> >>> +  - $ref: snps,dwmac.yaml#
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: tesla,fsd-ethqos-4.21.yaml
> >>
> >> ?
> >
> > Will fix this to tesla,fsd-ethqos.yaml
> 
> Test your patches before sending. REALLY TEST.

Sure. Will fix this to tesla,fsd-ethqos.yaml and test the same.

> 
> >
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 1
> >>> +
> >>> +  clocks:
> >>> +    minItems: 5
> >>
> >> Why? I expect it to be specific.
> >
> > Sorry, I could not understood this comment. In FSD we have two instances
> of EQoS IP, one in PERIC block, which requires total 10 clocks  to be
> configured and another instance exist in FSYS0 block which needs 5 clocks to
> be configured, so we kept minItems as 5 and maxItems as 10, but looks like
> latest items schema do not need maxItems entry so we will drop maxItems
> entry. In my understanding minItems still required so it should be kept with
> minimum number of clock requirements.
> 
> No, the code is fine then.
> 
> >
> >>
> >>> +    maxItems: 10
> >>> +
> >>> +  clock-names:
> >>> +    minItems: 5
> >>> +    maxItems: 10
> >>> +    items:
> >>> +      - const: ptp_ref
> >>> +      - const: master_bus
> >>> +      - const: slave_bus
> >>> +      - const: tx
> >>> +      - const: rx
> >>> +      - const: master2_bus
> >>> +      - const: slave2_bus
> >>> +      - const: eqos_rxclk_mux
> >>> +      - const: eqos_phyrxclk
> >>> +      - const: dout_peric_rgmii_clk
> >>> +
> >>> +  fsd-rx-clock-skew:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> >>> +    items:
> >>> +      - items:
> >>> +          - description: phandle to the syscon node
> >>> +          - description: offset of the control register
> >>> +    description:
> >>> +      Should be phandle/offset pair. The phandle to the syscon node.
> >>> +
> >>> +  iommus:
> >>> +    maxItems: 1
> >>> +
> >>> +  phy-mode:
> >>> +    $ref: ethernet-controller.yaml#/properties/phy-connection-type
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - reg
> >>> +  - interrupts
> >>> +  - clocks
> >>> +  - clock-names
> >>> +  - rx-clock-skew
> >>
> >> Eee? Isn't it fsd-rx-clock-skew which anyway is not correct?
> >
> > Sorry, I missed to change this in DT schema before posting, I will make this
> to fsd-rx-clock-skew.
> 
> Remember about vendor prefixes for every custom property.

Sure, will fix this in v4.

> 
> 
> Best regards,
> Krzysztof

Regards, 
Swathi

Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Andrew Lunn]

> +  fsd-rx-clock-skew:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle to the syscon node
> +          - description: offset of the control register
> +    description:
> +      Should be phandle/offset pair. The phandle to the syscon node.

What clock are you skew-ing here? And why?

> +    ethernet_1: ethernet@14300000 {
> +              compatible = "tesla,dwc-qos-ethernet-4.21";
> +              reg = <0x0 0x14300000 0x0 0x10000>;
> +              interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> +              clocks = <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_CLK_PTP_REF_I>,
> +                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_ACLK_I>,
> +                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_HCLK_I>,
> +                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_RGMII_CLK_I>,
> +                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_CLK_RX_I>,
> +                       <&clock_peric PERIC_BUS_D_PERIC_IPCLKPORT_EQOSCLK>,
> +                       <&clock_peric PERIC_BUS_P_PERIC_IPCLKPORT_EQOSCLK>,
> +                       <&clock_peric PERIC_EQOS_PHYRXCLK_MUX>,
> +                       <&clock_peric PERIC_EQOS_PHYRXCLK>,
> +                       <&clock_peric PERIC_DOUT_RGMII_CLK>;
> +              clock-names = "ptp_ref",
> +                            "master_bus",
> +                            "slave_bus",
> +                            "tx",
> +                            "rx",
> +                            "master2_bus",
> +                            "slave2_bus",
> +                            "eqos_rxclk_mux",
> +                            "eqos_phyrxclk",
> +                            "dout_peric_rgmii_clk";
> +              pinctrl-names = "default";
> +              pinctrl-0 = <&eth1_tx_clk>, <&eth1_tx_data>, <&eth1_tx_ctrl>,
> +                          <&eth1_phy_intr>, <&eth1_rx_clk>, <&eth1_rx_data>,
> +                          <&eth1_rx_ctrl>, <&eth1_mdio>;
> +              fsd-rx-clock-skew = <&sysreg_peric 0x10>;
> +              iommus = <&smmu_peric 0x0 0x1>;
> +              phy-mode = "rgmii";

I know it is just an example, but "rgmii" is generally
wrong. "rgmii-id" is generally what you need. And when i do see
"rgmii", it starts ringing alarm bells for me, it could mean your
RGMII delays are being handled wrongly.

       Andrew

RE: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Swathi K S]

Hi Andrew, 

Sorry for the delay in response.
Starting now, I will be taking over this task.
I have gone through your comments and feedback and will be implementing them
in v4 of this patch.

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: 15 August 2023 02:10
> To: Sriranjani P <sriranjani.p@samsung.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
> richardcochran@gmail.com; alexandre.torgue@foss.st.com;
> joabreu@synopsys.com; mcoquelin.stm32@gmail.com;
> alim.akhtar@samsung.com; linux-fsd@tesla.com;
> pankaj.dubey@samsung.com; swathi.ks@samsung.com;
> ravi.patel@samsung.com; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-
> soc@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree
> bindings
> 
> > +  fsd-rx-clock-skew:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    items:
> > +      - items:
> > +          - description: phandle to the syscon node
> > +          - description: offset of the control register
> > +    description:
> > +      Should be phandle/offset pair. The phandle to the syscon node.
> 
> What clock are you skew-ing here? And why?

As per customer's requirement, we need 2ns delay in fsys block both in TX
and RX path.

> 
> > +    ethernet_1: ethernet@14300000 {
> > +              compatible = "tesla,dwc-qos-ethernet-4.21";
> > +              reg = <0x0 0x14300000 0x0 0x10000>;
> > +              interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
> > +              clocks = <&clock_peric
> PERIC_EQOS_TOP_IPCLKPORT_CLK_PTP_REF_I>,
> > +                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_ACLK_I>,
> > +                       <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_HCLK_I>,
> > +                       <&clock_peric
PERIC_EQOS_TOP_IPCLKPORT_RGMII_CLK_I>,
> > +                       <&clock_peric
PERIC_EQOS_TOP_IPCLKPORT_CLK_RX_I>,
> > +                       <&clock_peric
PERIC_BUS_D_PERIC_IPCLKPORT_EQOSCLK>,
> > +                       <&clock_peric
PERIC_BUS_P_PERIC_IPCLKPORT_EQOSCLK>,
> > +                       <&clock_peric PERIC_EQOS_PHYRXCLK_MUX>,
> > +                       <&clock_peric PERIC_EQOS_PHYRXCLK>,
> > +                       <&clock_peric PERIC_DOUT_RGMII_CLK>;
> > +              clock-names = "ptp_ref",
> > +                            "master_bus",
> > +                            "slave_bus",
> > +                            "tx",
> > +                            "rx",
> > +                            "master2_bus",
> > +                            "slave2_bus",
> > +                            "eqos_rxclk_mux",
> > +                            "eqos_phyrxclk",
> > +                            "dout_peric_rgmii_clk";
> > +              pinctrl-names = "default";
> > +              pinctrl-0 = <&eth1_tx_clk>, <&eth1_tx_data>,
<&eth1_tx_ctrl>,
> > +                          <&eth1_phy_intr>, <&eth1_rx_clk>,
<&eth1_rx_data>,
> > +                          <&eth1_rx_ctrl>, <&eth1_mdio>;
> > +              fsd-rx-clock-skew = <&sysreg_peric 0x10>;
> > +              iommus = <&smmu_peric 0x0 0x1>;
> > +              phy-mode = "rgmii";
> 
> I know it is just an example, but "rgmii" is generally wrong. "rgmii-id"
is
> generally what you need. And when i do see "rgmii", it starts ringing
alarm
> bells for me, it could mean your RGMII delays are being handled wrongly.

Thanks for bringing this to our notice. Will correct this in v4 as rgmii-id.

> 
>        Andrew

Regards,
Swathi

Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Andrew Lunn]

> > > +  fsd-rx-clock-skew:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > > +    items:
> > > +      - items:
> > > +          - description: phandle to the syscon node
> > > +          - description: offset of the control register
> > > +    description:
> > > +      Should be phandle/offset pair. The phandle to the syscon node.
> > 
> > What clock are you skew-ing here? And why?
> 
> As per customer's requirement, we need 2ns delay in fsys block both in TX
> and RX path.

Lots of people get RGMII delays wrong. Please look back at the mailing
list where there is plenty of discussion about this. I don't want to
have to repeat myself yet again...

     Andrew

RE: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree bindings

[Swathi K S]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 06 June 2024 18:56
> To: Swathi K S <swathi.ks@samsung.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; richardcochran@gmail.com;
> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
> mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
> fsd@tesla.com; pankaj.dubey@samsung.com; ravi.patel@samsung.com;
> netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH v3 1/4] dt-bindings: net: Add FSD EQoS device tree
bindings
> 
> > > > +  fsd-rx-clock-skew:
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > > > +    items:
> > > > +      - items:
> > > > +          - description: phandle to the syscon node
> > > > +          - description: offset of the control register
> > > > +    description:
> > > > +      Should be phandle/offset pair. The phandle to the syscon
node.
> > >
> > > What clock are you skew-ing here? And why?
> >
> > As per customer's requirement, we need 2ns delay in fsys block both in
> > TX and RX path.
> 
> Lots of people get RGMII delays wrong. Please look back at the mailing
list
> where there is plenty of discussion about this. I don't want to have to
repeat
> myself yet again...

Sorry for the delay.
We took time to confirm with the board designer regarding this delay.
This is not a mandatory one to have and hence we are dropping clock-skewing
here.

> 
>      Andrew

Thanks,
Swathi

[PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support

[Sriranjani P]

The FSD SoC contains two instance of the Synopsys DWC ethernet QOS IP core.
The binding that it uses is slightly different from existing ones because
of the integration (clocks, resets).

For FSD SoC, a mux switch is needed between internal and external clocks.
By default after reset internal clock is used but for receiving packets
properly, external clock is needed. Mux switch to external clock happens
only when the external clock is present.

Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
Signed-off-by: Suresh Siddha <ssiddha@tesla.com>
Signed-off-by: Swathi K S <swathi.ks@samsung.com>
Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
---
 .../stmicro/stmmac/dwmac-dwc-qos-eth.c        | 172 ++++++++++++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  28 ++-
 include/linux/stmmac.h                        |   1 +
 3 files changed, 199 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
index 61ebf36da13d..651a41e0dab9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
@@ -20,6 +20,7 @@
 #include <linux/platform_device.h>
 #include <linux/reset.h>
 #include <linux/stmmac.h>
+#include <linux/regmap.h>
 
 #include "stmmac_platform.h"
 #include "dwmac4.h"
@@ -37,6 +38,45 @@ struct tegra_eqos {
 	struct gpio_desc *reset;
 };
 
+enum fsd_rxmux_clk {
+	FSD_RXCLK_MUX = 7,
+	FSD_RXCLK_EXTERNAL,
+	FSD_RXCLK_INTERNAL
+};
+
+struct fsd_eqos_plat_data {
+	const struct fsd_eqos_variant *fsd_eqos_inst_var;
+	struct clk_bulk_data *clks;
+	struct device *dev;
+};
+
+struct fsd_eqos_variant {
+	const char * const *clk_list;
+	int num_clks;
+};
+
+static const char * const fsd_eqos_instance_0_clk[] = {
+	"ptp_ref", "master_bus", "slave_bus", "tx", "rx"
+};
+
+static const char * const fsd_eqos_instance_1_clk[] = {
+	"ptp_ref", "master_bus", "slave_bus", "tx", "rx", "master2_bus",
+	"slave2_bus", "eqos_rxclk_mux", "eqos_phyrxclk", "dout_peric_rgmii_clk"
+};
+
+static const int rx_clock_skew_val[] = {0x2, 0x0};
+
+static const struct fsd_eqos_variant fsd_eqos_clk_info[] = {
+	{
+		.clk_list = fsd_eqos_instance_0_clk,
+		.num_clks = ARRAY_SIZE(fsd_eqos_instance_0_clk)
+	},
+	{
+		.clk_list = fsd_eqos_instance_1_clk,
+		.num_clks = ARRAY_SIZE(fsd_eqos_instance_1_clk)
+	},
+};
+
 static int dwc_eth_dwmac_config_dt(struct platform_device *pdev,
 				   struct plat_stmmacenet_data *plat_dat)
 {
@@ -265,6 +305,132 @@ static int tegra_eqos_init(struct platform_device *pdev, void *priv)
 	return 0;
 }
 
+static int dwc_eqos_rxmux_setup(void *priv, bool external)
+{
+	struct fsd_eqos_plat_data *plat = priv;
+
+	/* doesn't support RX clock mux */
+	if (!plat->clks[FSD_RXCLK_MUX].clk)
+		return 0;
+
+	if (external)
+		return clk_set_parent(plat->clks[FSD_RXCLK_MUX].clk,
+				      plat->clks[FSD_RXCLK_EXTERNAL].clk);
+	else
+		return clk_set_parent(plat->clks[FSD_RXCLK_MUX].clk,
+				      plat->clks[FSD_RXCLK_INTERNAL].clk);
+}
+
+static int dwc_eqos_setup_rxclock(struct platform_device *pdev, int ins_num)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct regmap *syscon;
+	unsigned int reg;
+
+	if (np && of_property_read_bool(np, "fsd-rx-clock-skew")) {
+		syscon = syscon_regmap_lookup_by_phandle_args(np,
+							      "fsd-rx-clock-skew",
+							      1, &reg);
+		if (IS_ERR(syscon)) {
+			dev_err(&pdev->dev,
+				"couldn't get the rx-clock-skew syscon!\n");
+			return PTR_ERR(syscon);
+		}
+
+		regmap_write(syscon, reg, rx_clock_skew_val[ins_num]);
+	}
+
+	return 0;
+}
+
+static int fsd_eqos_clk_init(struct fsd_eqos_plat_data *plat,
+			     struct plat_stmmacenet_data *data)
+{
+	int ret = 0, i;
+
+	const struct fsd_eqos_variant *fsd_eqos_v_data =
+						plat->fsd_eqos_inst_var;
+
+	plat->clks = devm_kcalloc(plat->dev, fsd_eqos_v_data->num_clks,
+				  sizeof(*plat->clks), GFP_KERNEL);
+	if (!plat->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < fsd_eqos_v_data->num_clks; i++)
+		plat->clks[i].id = fsd_eqos_v_data->clk_list[i];
+
+	ret = devm_clk_bulk_get(plat->dev, fsd_eqos_v_data->num_clks,
+				plat->clks);
+
+	return ret;
+}
+
+static int fsd_clks_endisable(void *priv, bool enabled)
+{
+	int ret, num_clks;
+	struct fsd_eqos_plat_data *plat = priv;
+
+	num_clks = plat->fsd_eqos_inst_var->num_clks;
+
+	if (enabled) {
+		ret = clk_bulk_prepare_enable(num_clks, plat->clks);
+		if (ret) {
+			dev_err(plat->dev, "Clock enable failed, err = %d\n", ret);
+			return ret;
+		}
+	} else {
+		clk_bulk_disable_unprepare(num_clks, plat->clks);
+	}
+
+	return 0;
+}
+
+static int fsd_eqos_probe(struct platform_device *pdev,
+			  struct plat_stmmacenet_data *data,
+			  struct stmmac_resources *res)
+{
+	struct fsd_eqos_plat_data *priv_plat;
+	struct device_node *np = pdev->dev.of_node;
+	int ret = 0;
+
+	priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat), GFP_KERNEL);
+	if (!priv_plat) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	priv_plat->dev = &pdev->dev;
+	data->bus_id = of_alias_get_id(np, "eth");
+
+	priv_plat->fsd_eqos_inst_var = &fsd_eqos_clk_info[data->bus_id];
+
+	ret = fsd_eqos_clk_init(priv_plat, data);
+
+	data->bsp_priv = priv_plat;
+	data->clks_config = fsd_clks_endisable;
+	data->rxmux_setup = dwc_eqos_rxmux_setup;
+
+	ret = fsd_clks_endisable(priv_plat, true);
+	if (ret)
+		goto error;
+
+	ret = dwc_eqos_setup_rxclock(pdev, data->bus_id);
+	if (ret) {
+		fsd_clks_endisable(priv_plat, false);
+		dev_err_probe(&pdev->dev, ret, "Unable to setup rxclock\n");
+	}
+
+error:
+	return ret;
+}
+
+static void fsd_eqos_remove(struct platform_device *pdev)
+{
+	struct fsd_eqos_plat_data *priv_plat = get_stmmac_bsp_priv(&pdev->dev);
+
+	fsd_clks_endisable(priv_plat, false);
+}
+
 static int tegra_eqos_probe(struct platform_device *pdev,
 			    struct plat_stmmacenet_data *data,
 			    struct stmmac_resources *res)
@@ -411,6 +577,11 @@ static const struct dwc_eth_dwmac_data tegra_eqos_data = {
 	.remove = tegra_eqos_remove,
 };
 
+static const struct dwc_eth_dwmac_data fsd_eqos_data = {
+	.probe = fsd_eqos_probe,
+	.remove = fsd_eqos_remove,
+};
+
 static int dwc_eth_dwmac_probe(struct platform_device *pdev)
 {
 	const struct dwc_eth_dwmac_data *data;
@@ -482,6 +653,7 @@ static void dwc_eth_dwmac_remove(struct platform_device *pdev)
 static const struct of_device_id dwc_eth_dwmac_match[] = {
 	{ .compatible = "snps,dwc-qos-ethernet-4.10", .data = &dwc_qos_data },
 	{ .compatible = "nvidia,tegra186-eqos", .data = &tegra_eqos_data },
+	{ .compatible = "tesla,dwc-qos-ethernet-4.21", .data = &fsd_eqos_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, dwc_eth_dwmac_match);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 733b5e900817..3c7d55786aaa 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3883,6 +3883,12 @@ static int __stmmac_open(struct net_device *dev,
 	netif_tx_start_all_queues(priv->dev);
 	stmmac_enable_all_dma_irq(priv);
 
+	if (priv->plat->rxmux_setup) {
+		ret = priv->plat->rxmux_setup(priv->plat->bsp_priv, true);
+		if (ret)
+			netdev_err(priv->dev, "Rxmux setup failed\n");
+	}
+
 	return 0;
 
 irq_error:
@@ -3936,7 +3942,13 @@ static void stmmac_fpe_stop_wq(struct stmmac_priv *priv)
 static int stmmac_release(struct net_device *dev)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
-	u32 chan;
+	u32 chan, ret;
+
+	if (priv->plat->rxmux_setup) {
+		ret = priv->plat->rxmux_setup(priv->plat->bsp_priv, false);
+		if (ret)
+			netdev_err(priv->dev, "Rxmux setup failed\n");
+	}
 
 	if (device_may_wakeup(priv->device))
 		phylink_speed_down(priv->phylink, false);
@@ -7630,11 +7642,17 @@ int stmmac_suspend(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct stmmac_priv *priv = netdev_priv(ndev);
-	u32 chan;
+	u32 chan, ret;
 
 	if (!ndev || !netif_running(ndev))
 		return 0;
 
+	if (priv->plat->rxmux_setup) {
+		ret = priv->plat->rxmux_setup(priv->plat->bsp_priv, false);
+		if (ret)
+			netdev_err(priv->dev, "Rxmux setup failed\n");
+	}
+
 	mutex_lock(&priv->lock);
 
 	netif_device_detach(ndev);
@@ -7799,6 +7817,12 @@ int stmmac_resume(struct device *dev)
 	mutex_unlock(&priv->lock);
 	rtnl_unlock();
 
+	if (priv->plat->rxmux_setup) {
+		ret = priv->plat->rxmux_setup(priv->plat->bsp_priv, true);
+		if (ret)
+			netdev_err(priv->dev, "Rxmux setup failed\n");
+	}
+
 	netif_device_attach(ndev);
 
 	return 0;
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 784277d666eb..69150c8c8df7 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -264,6 +264,7 @@ struct plat_stmmacenet_data {
 	void (*ptp_clk_freq_config)(struct stmmac_priv *priv);
 	int (*init)(struct platform_device *pdev, void *priv);
 	void (*exit)(struct platform_device *pdev, void *priv);
+	int (*rxmux_setup)(void *priv, bool external);
 	struct mac_device_info *(*setup)(void *priv);
 	int (*clks_config)(void *priv, bool enabled);
 	int (*crosststamp)(ktime_t *device, struct system_counterval_t *system,
-- 
2.17.1

Re: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support

[Krzysztof Kozlowski]

On 14/08/2023 13:25, Sriranjani P wrote:
> The FSD SoC contains two instance of the Synopsys DWC ethernet QOS IP core.
> The binding that it uses is slightly different from existing ones because
> of the integration (clocks, resets).
> 
> For FSD SoC, a mux switch is needed between internal and external clocks.
> By default after reset internal clock is used but for receiving packets
> properly, external clock is needed. Mux switch to external clock happens
> only when the external clock is present.
> 
> Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
> Signed-off-by: Suresh Siddha <ssiddha@tesla.com>
> Signed-off-by: Swathi K S <swathi.ks@samsung.com>
> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> ---


> +static int dwc_eqos_setup_rxclock(struct platform_device *pdev, int ins_num)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct regmap *syscon;
> +	unsigned int reg;
> +
> +	if (np && of_property_read_bool(np, "fsd-rx-clock-skew")) {
> +		syscon = syscon_regmap_lookup_by_phandle_args(np,
> +							      "fsd-rx-clock-skew",
> +							      1, &reg);
> +		if (IS_ERR(syscon)) {
> +			dev_err(&pdev->dev,
> +				"couldn't get the rx-clock-skew syscon!\n");
> +			return PTR_ERR(syscon);
> +		}
> +
> +		regmap_write(syscon, reg, rx_clock_skew_val[ins_num]);
> +	}
> +
> +	return 0;
> +}
> +
> +static int fsd_eqos_clk_init(struct fsd_eqos_plat_data *plat,
> +			     struct plat_stmmacenet_data *data)
> +{
> +	int ret = 0, i;
> +
> +	const struct fsd_eqos_variant *fsd_eqos_v_data =
> +						plat->fsd_eqos_inst_var;
> +
> +	plat->clks = devm_kcalloc(plat->dev, fsd_eqos_v_data->num_clks,
> +				  sizeof(*plat->clks), GFP_KERNEL);
> +	if (!plat->clks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < fsd_eqos_v_data->num_clks; i++)
> +		plat->clks[i].id = fsd_eqos_v_data->clk_list[i];
> +
> +	ret = devm_clk_bulk_get(plat->dev, fsd_eqos_v_data->num_clks,
> +				plat->clks);

Instead of duplicating entire clock management with existing code, you
should extend/rework existing one.

This code is unfortunately great example how not to stuff vendor code
into upstream project. :(

> +
> +	return ret;
> +}
> +
> +static int fsd_clks_endisable(void *priv, bool enabled)
> +{
> +	int ret, num_clks;
> +	struct fsd_eqos_plat_data *plat = priv;
> +
> +	num_clks = plat->fsd_eqos_inst_var->num_clks;
> +
> +	if (enabled) {
> +		ret = clk_bulk_prepare_enable(num_clks, plat->clks);
> +		if (ret) {
> +			dev_err(plat->dev, "Clock enable failed, err = %d\n", ret);
> +			return ret;
> +		}
> +	} else {
> +		clk_bulk_disable_unprepare(num_clks, plat->clks);
> +	}
> +
> +	return 0;
> +}
> +
> +static int fsd_eqos_probe(struct platform_device *pdev,
> +			  struct plat_stmmacenet_data *data,
> +			  struct stmmac_resources *res)
> +{
> +	struct fsd_eqos_plat_data *priv_plat;
> +	struct device_node *np = pdev->dev.of_node;
> +	int ret = 0;
> +
> +	priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat), GFP_KERNEL);
> +	if (!priv_plat) {
> +		ret = -ENOMEM;

return -ENOMEM

> +		goto error;
> +	}
> +
> +	priv_plat->dev = &pdev->dev;
> +	data->bus_id = of_alias_get_id(np, "eth");

No, you cannot do like this. Aliases are board specific and are based on
labeling on the board.

> +
> +	priv_plat->fsd_eqos_inst_var = &fsd_eqos_clk_info[data->bus_id];
> +
> +	ret = fsd_eqos_clk_init(priv_plat, data);
> +
> +	data->bsp_priv = priv_plat;
> +	data->clks_config = fsd_clks_endisable;
> +	data->rxmux_setup = dwc_eqos_rxmux_setup;
> +
> +	ret = fsd_clks_endisable(priv_plat, true);
> +	if (ret)
> +		goto error;
> +
> +	ret = dwc_eqos_setup_rxclock(pdev, data->bus_id);
> +	if (ret) {
> +		fsd_clks_endisable(priv_plat, false);
> +		dev_err_probe(&pdev->dev, ret, "Unable to setup rxclock\n");

The syntax is: return dev_err_probe().

> +	}
> +
> +error:
> +	return ret;
> +}

....


Best regards,
Krzysztof

RE: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support

[Sriranjani P]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 15 August 2023 01:21
> To: Sriranjani P <sriranjani.p@samsung.com>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; richardcochran@gmail.com;
> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
> mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
> fsd@tesla.com; pankaj.dubey@samsung.com; swathi.ks@samsung.com;
> ravi.patel@samsung.com
> Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Chandrasekar R <rcsekar@samsung.com>;
> Suresh Siddha <ssiddha@tesla.com>
> Subject: Re: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support
> 
> On 14/08/2023 13:25, Sriranjani P wrote:
> > The FSD SoC contains two instance of the Synopsys DWC ethernet QOS IP
> core.
> > The binding that it uses is slightly different from existing ones
> > because of the integration (clocks, resets).
> >
> > For FSD SoC, a mux switch is needed between internal and external clocks.
> > By default after reset internal clock is used but for receiving
> > packets properly, external clock is needed. Mux switch to external
> > clock happens only when the external clock is present.
> >
> > Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
> > Signed-off-by: Suresh Siddha <ssiddha@tesla.com>
> > Signed-off-by: Swathi K S <swathi.ks@samsung.com>
> > Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> > ---
> 
> 
> > +static int dwc_eqos_setup_rxclock(struct platform_device *pdev, int
> > +ins_num) {
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct regmap *syscon;
> > +	unsigned int reg;
> > +
> > +	if (np && of_property_read_bool(np, "fsd-rx-clock-skew")) {
> > +		syscon = syscon_regmap_lookup_by_phandle_args(np,
> > +							      "fsd-rx-clock-
> skew",
> > +							      1, &reg);
> > +		if (IS_ERR(syscon)) {
> > +			dev_err(&pdev->dev,
> > +				"couldn't get the rx-clock-skew syscon!\n");
> > +			return PTR_ERR(syscon);
> > +		}
> > +
> > +		regmap_write(syscon, reg, rx_clock_skew_val[ins_num]);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int fsd_eqos_clk_init(struct fsd_eqos_plat_data *plat,
> > +			     struct plat_stmmacenet_data *data) {
> > +	int ret = 0, i;
> > +
> > +	const struct fsd_eqos_variant *fsd_eqos_v_data =
> > +						plat->fsd_eqos_inst_var;
> > +
> > +	plat->clks = devm_kcalloc(plat->dev, fsd_eqos_v_data->num_clks,
> > +				  sizeof(*plat->clks), GFP_KERNEL);
> > +	if (!plat->clks)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < fsd_eqos_v_data->num_clks; i++)
> > +		plat->clks[i].id = fsd_eqos_v_data->clk_list[i];
> > +
> > +	ret = devm_clk_bulk_get(plat->dev, fsd_eqos_v_data->num_clks,
> > +				plat->clks);
> 
> Instead of duplicating entire clock management with existing code, you
> should extend/rework existing one.
> 
> This code is unfortunately great example how not to stuff vendor code into
> upstream project. :(

I will check again if I can extend existing one to support FSD platform specific requirement.

> 
> > +
> > +	return ret;
> > +}
> > +
> > +static int fsd_clks_endisable(void *priv, bool enabled) {
> > +	int ret, num_clks;
> > +	struct fsd_eqos_plat_data *plat = priv;
> > +
> > +	num_clks = plat->fsd_eqos_inst_var->num_clks;
> > +
> > +	if (enabled) {
> > +		ret = clk_bulk_prepare_enable(num_clks, plat->clks);
> > +		if (ret) {
> > +			dev_err(plat->dev, "Clock enable failed, err = %d\n",
> ret);
> > +			return ret;
> > +		}
> > +	} else {
> > +		clk_bulk_disable_unprepare(num_clks, plat->clks);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int fsd_eqos_probe(struct platform_device *pdev,
> > +			  struct plat_stmmacenet_data *data,
> > +			  struct stmmac_resources *res)
> > +{
> > +	struct fsd_eqos_plat_data *priv_plat;
> > +	struct device_node *np = pdev->dev.of_node;
> > +	int ret = 0;
> > +
> > +	priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat),
> GFP_KERNEL);
> > +	if (!priv_plat) {
> > +		ret = -ENOMEM;
> 
> return -ENOMEM

Will fix this in v4.

> 
> > +		goto error;
> > +	}
> > +
> > +	priv_plat->dev = &pdev->dev;
> > +	data->bus_id = of_alias_get_id(np, "eth");
> 
> No, you cannot do like this. Aliases are board specific and are based on
> labeling on the board.

So if I understood this correctly, I need to move alias in the board specific DTS file and I can use this, because we have to handle rx-clock-skew differently for the two instances in the FSD platform. Another approach we took in v1, by specifying the value to be programmed in rx-clock-skew property itself, but it seems it is not a preferred approach. 
I can see that in drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c +436 common code is already using this API and getting alias id, so I can probably use the same rather getting same again here, but I have to specify alias in DTS file.

> 
> > +
> > +	priv_plat->fsd_eqos_inst_var = &fsd_eqos_clk_info[data->bus_id];
> > +
> > +	ret = fsd_eqos_clk_init(priv_plat, data);
> > +
> > +	data->bsp_priv = priv_plat;
> > +	data->clks_config = fsd_clks_endisable;
> > +	data->rxmux_setup = dwc_eqos_rxmux_setup;
> > +
> > +	ret = fsd_clks_endisable(priv_plat, true);
> > +	if (ret)
> > +		goto error;
> > +
> > +	ret = dwc_eqos_setup_rxclock(pdev, data->bus_id);
> > +	if (ret) {
> > +		fsd_clks_endisable(priv_plat, false);
> > +		dev_err_probe(&pdev->dev, ret, "Unable to setup
> rxclock\n");
> 
> The syntax is: return dev_err_probe().

Will fix it in v4.

> 
> > +	}
> > +
> > +error:
> > +	return ret;
> > +}
> 
> ....
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support

[Krzysztof Kozlowski]

On 16/08/2023 08:38, Sriranjani P wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
>> Sent: 15 August 2023 01:21
>> To: Sriranjani P <sriranjani.p@samsung.com>; davem@davemloft.net;
>> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
>> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
>> conor+dt@kernel.org; richardcochran@gmail.com;
>> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
>> mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
>> fsd@tesla.com; pankaj.dubey@samsung.com; swathi.ks@samsung.com;
>> ravi.patel@samsung.com
>> Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; Chandrasekar R <rcsekar@samsung.com>;
>> Suresh Siddha <ssiddha@tesla.com>
>> Subject: Re: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support
>>
>> On 14/08/2023 13:25, Sriranjani P wrote:
>>> The FSD SoC contains two instance of the Synopsys DWC ethernet QOS IP
>> core.
>>> The binding that it uses is slightly different from existing ones
>>> because of the integration (clocks, resets).
>>>
>>> For FSD SoC, a mux switch is needed between internal and external clocks.
>>> By default after reset internal clock is used but for receiving
>>> packets properly, external clock is needed. Mux switch to external
>>> clock happens only when the external clock is present.
>>>
>>> Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
>>> Signed-off-by: Suresh Siddha <ssiddha@tesla.com>
>>> Signed-off-by: Swathi K S <swathi.ks@samsung.com>
>>> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
>>> ---
>>
>>
>>> +static int dwc_eqos_setup_rxclock(struct platform_device *pdev, int
>>> +ins_num) {
>>> +	struct device_node *np = pdev->dev.of_node;
>>> +	struct regmap *syscon;
>>> +	unsigned int reg;
>>> +
>>> +	if (np && of_property_read_bool(np, "fsd-rx-clock-skew")) {
>>> +		syscon = syscon_regmap_lookup_by_phandle_args(np,
>>> +							      "fsd-rx-clock-
>> skew",
>>> +							      1, &reg);
>>> +		if (IS_ERR(syscon)) {
>>> +			dev_err(&pdev->dev,
>>> +				"couldn't get the rx-clock-skew syscon!\n");
>>> +			return PTR_ERR(syscon);
>>> +		}
>>> +
>>> +		regmap_write(syscon, reg, rx_clock_skew_val[ins_num]);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int fsd_eqos_clk_init(struct fsd_eqos_plat_data *plat,
>>> +			     struct plat_stmmacenet_data *data) {
>>> +	int ret = 0, i;
>>> +
>>> +	const struct fsd_eqos_variant *fsd_eqos_v_data =
>>> +						plat->fsd_eqos_inst_var;
>>> +
>>> +	plat->clks = devm_kcalloc(plat->dev, fsd_eqos_v_data->num_clks,
>>> +				  sizeof(*plat->clks), GFP_KERNEL);
>>> +	if (!plat->clks)
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < fsd_eqos_v_data->num_clks; i++)
>>> +		plat->clks[i].id = fsd_eqos_v_data->clk_list[i];
>>> +
>>> +	ret = devm_clk_bulk_get(plat->dev, fsd_eqos_v_data->num_clks,
>>> +				plat->clks);
>>
>> Instead of duplicating entire clock management with existing code, you
>> should extend/rework existing one.
>>
>> This code is unfortunately great example how not to stuff vendor code into
>> upstream project. :(
> 
> I will check again if I can extend existing one to support FSD platform specific requirement.
> 
>>
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int fsd_clks_endisable(void *priv, bool enabled) {
>>> +	int ret, num_clks;
>>> +	struct fsd_eqos_plat_data *plat = priv;
>>> +
>>> +	num_clks = plat->fsd_eqos_inst_var->num_clks;
>>> +
>>> +	if (enabled) {
>>> +		ret = clk_bulk_prepare_enable(num_clks, plat->clks);
>>> +		if (ret) {
>>> +			dev_err(plat->dev, "Clock enable failed, err = %d\n",
>> ret);
>>> +			return ret;
>>> +		}
>>> +	} else {
>>> +		clk_bulk_disable_unprepare(num_clks, plat->clks);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int fsd_eqos_probe(struct platform_device *pdev,
>>> +			  struct plat_stmmacenet_data *data,
>>> +			  struct stmmac_resources *res)
>>> +{
>>> +	struct fsd_eqos_plat_data *priv_plat;
>>> +	struct device_node *np = pdev->dev.of_node;
>>> +	int ret = 0;
>>> +
>>> +	priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat),
>> GFP_KERNEL);
>>> +	if (!priv_plat) {
>>> +		ret = -ENOMEM;
>>
>> return -ENOMEM
> 
> Will fix this in v4.
> 
>>
>>> +		goto error;
>>> +	}
>>> +
>>> +	priv_plat->dev = &pdev->dev;
>>> +	data->bus_id = of_alias_get_id(np, "eth");
>>
>> No, you cannot do like this. Aliases are board specific and are based on
>> labeling on the board.
> 
> So if I understood this correctly, I need to move alias in the board specific DTS file 

This part: yes

> and I can use this, because we have to handle rx-clock-skew differently for the two instances in the FSD platform.

Not really. Do you expect it to work correctly if given EQoS instance
receives different alias, e.g. 5?

> Another approach we took in v1, by specifying the value to be programmed in rx-clock-skew property itself, but it seems it is not a preferred approach. 
> I can see that in drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c +436 common code is already using this API and getting alias id, so I can probably use the same rather getting same again here, but I have to specify alias in DTS file.

Getting alias ID is okay in general. It is used to provide user-visible
ID of the devices (see mmc). Using such alias to configure hardware is
abuse of the alias, because of the reasons I said - how is it supposed
to work if alias is 5 (this is perfectly valid alias)?

I suspect that all this is to substitute missing abstractions, like
clocks, phys or resets...

Best regards,
Krzysztof

RE: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support

[Swathi K S]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 18 August 2023 14:57
> To: Sriranjani P <sriranjani.p@samsung.com>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; richardcochran@gmail.com;
> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
> mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
> fsd@tesla.com; pankaj.dubey@samsung.com; swathi.ks@samsung.com;
> ravi.patel@samsung.com
> Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; 'Chandrasekar R' <rcsekar@samsung.com>;
> 'Suresh Siddha' <ssiddha@tesla.com>
> Subject: Re: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support
> 
> On 16/08/2023 08:38, Sriranjani P wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> >> Sent: 15 August 2023 01:21
> >> To: Sriranjani P <sriranjani.p@samsung.com>; davem@davemloft.net;
> >> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> >> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> >> conor+dt@kernel.org; richardcochran@gmail.com;
> >> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
> >> mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
> >> fsd@tesla.com; pankaj.dubey@samsung.com; swathi.ks@samsung.com;
> >> ravi.patel@samsung.com
> >> Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
> >> kernel@lists.infradead.org; Chandrasekar R <rcsekar@samsung.com>;
> >> Suresh Siddha <ssiddha@tesla.com>
> >> Subject: Re: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS
> >> support
> >>
> >> On 14/08/2023 13:25, Sriranjani P wrote:
> >>> The FSD SoC contains two instance of the Synopsys DWC ethernet QOS
> >>> IP
> >> core.
> >>> The binding that it uses is slightly different from existing ones
> >>> because of the integration (clocks, resets).
> >>>
> >>> For FSD SoC, a mux switch is needed between internal and external
> clocks.
> >>> By default after reset internal clock is used but for receiving
> >>> packets properly, external clock is needed. Mux switch to external
> >>> clock happens only when the external clock is present.
> >>>
> >>> Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
> >>> Signed-off-by: Suresh Siddha <ssiddha@tesla.com>
> >>> Signed-off-by: Swathi K S <swathi.ks@samsung.com>
> >>> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> >>> ---
> >>
> >>
> >>> +static int dwc_eqos_setup_rxclock(struct platform_device *pdev, int
> >>> +ins_num) {
> >>> +	struct device_node *np = pdev->dev.of_node;
> >>> +	struct regmap *syscon;
> >>> +	unsigned int reg;
> >>> +
> >>> +	if (np && of_property_read_bool(np, "fsd-rx-clock-skew")) {
> >>> +		syscon = syscon_regmap_lookup_by_phandle_args(np,
> >>> +							      "fsd-rx-clock-
> >> skew",
> >>> +							      1, &reg);
> >>> +		if (IS_ERR(syscon)) {
> >>> +			dev_err(&pdev->dev,
> >>> +				"couldn't get the rx-clock-skew syscon!\n");
> >>> +			return PTR_ERR(syscon);
> >>> +		}
> >>> +
> >>> +		regmap_write(syscon, reg, rx_clock_skew_val[ins_num]);
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int fsd_eqos_clk_init(struct fsd_eqos_plat_data *plat,
> >>> +			     struct plat_stmmacenet_data *data) {
> >>> +	int ret = 0, i;
> >>> +
> >>> +	const struct fsd_eqos_variant *fsd_eqos_v_data =
> >>> +						plat->fsd_eqos_inst_var;
> >>> +
> >>> +	plat->clks = devm_kcalloc(plat->dev, fsd_eqos_v_data->num_clks,
> >>> +				  sizeof(*plat->clks), GFP_KERNEL);
> >>> +	if (!plat->clks)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	for (i = 0; i < fsd_eqos_v_data->num_clks; i++)
> >>> +		plat->clks[i].id = fsd_eqos_v_data->clk_list[i];
> >>> +
> >>> +	ret = devm_clk_bulk_get(plat->dev, fsd_eqos_v_data->num_clks,
> >>> +				plat->clks);
> >>
> >> Instead of duplicating entire clock management with existing code,
> >> you should extend/rework existing one.
> >>
> >> This code is unfortunately great example how not to stuff vendor code
> >> into upstream project. :(
> >
> > I will check again if I can extend existing one to support FSD platform
> specific requirement.
> >
> >>
> >>> +
> >>> +	return ret;
> >>> +}
> >>> +
> >>> +static int fsd_clks_endisable(void *priv, bool enabled) {
> >>> +	int ret, num_clks;
> >>> +	struct fsd_eqos_plat_data *plat = priv;
> >>> +
> >>> +	num_clks = plat->fsd_eqos_inst_var->num_clks;
> >>> +
> >>> +	if (enabled) {
> >>> +		ret = clk_bulk_prepare_enable(num_clks, plat->clks);
> >>> +		if (ret) {
> >>> +			dev_err(plat->dev, "Clock enable failed, err = %d\n",
> >> ret);
> >>> +			return ret;
> >>> +		}
> >>> +	} else {
> >>> +		clk_bulk_disable_unprepare(num_clks, plat->clks);
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int fsd_eqos_probe(struct platform_device *pdev,
> >>> +			  struct plat_stmmacenet_data *data,
> >>> +			  struct stmmac_resources *res)
> >>> +{
> >>> +	struct fsd_eqos_plat_data *priv_plat;
> >>> +	struct device_node *np = pdev->dev.of_node;
> >>> +	int ret = 0;
> >>> +
> >>> +	priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat),
> >> GFP_KERNEL);
> >>> +	if (!priv_plat) {
> >>> +		ret = -ENOMEM;
> >>
> >> return -ENOMEM
> >
> > Will fix this in v4.
> >
> >>
> >>> +		goto error;
> >>> +	}
> >>> +
> >>> +	priv_plat->dev = &pdev->dev;
> >>> +	data->bus_id = of_alias_get_id(np, "eth");
> >>
> >> No, you cannot do like this. Aliases are board specific and are based
> >> on labeling on the board.
> >
> > So if I understood this correctly, I need to move alias in the board
> > specific DTS file
> 
> This part: yes
> 
> > and I can use this, because we have to handle rx-clock-skew differently for
> the two instances in the FSD platform.
> 
> Not really. Do you expect it to work correctly if given EQoS instance receives
> different alias, e.g. 5?
> 
> > Another approach we took in v1, by specifying the value to be programmed
> in rx-clock-skew property itself, but it seems it is not a preferred approach.
> > I can see that in drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +436 common code is already using this API and getting alias id, so I can
> probably use the same rather getting same again here, but I have to specify
> alias in DTS file.
> 
> Getting alias ID is okay in general. It is used to provide user-visible ID of the
> devices (see mmc). Using such alias to configure hardware is abuse of the
> alias, because of the reasons I said - how is it supposed to work if alias is 5
> (this is perfectly valid alias)?
> 
> I suspect that all this is to substitute missing abstractions, like clocks, phys or
> resets...

Will avoid using the API to get alias id to configure the HW. Will share the new implementation in v4.

> 
> Best regards,
> Krzysztof


Regards,
Swathi

Re: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support

[Krzysztof Kozlowski]

On 06/06/2024 11:14, Swathi K S wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
>> Sent: 18 August 2023 14:57
>> To: Sriranjani P <sriranjani.p@samsung.com>; davem@davemloft.net;
>> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
>> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
>> conor+dt@kernel.org; richardcochran@gmail.com;
>> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
>> mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
>> fsd@tesla.com; pankaj.dubey@samsung.com; swathi.ks@samsung.com;
>> ravi.patel@samsung.com
>> Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; 'Chandrasekar R' <rcsekar@samsung.com>;
>> 'Suresh Siddha' <ssiddha@tesla.com>
>> Subject: Re: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support
>>
>> On 16/08/2023 08:38, Sriranjani P wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
>>>> Sent: 15 August 2023 01:21
>>>> To: Sriranjani P <sriranjani.p@samsung.com>; davem@davemloft.net;
>>>> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
>>>> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
>>>> conor+dt@kernel.org; richardcochran@gmail.com;
>>>> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
>>>> mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
>>>> fsd@tesla.com; pankaj.dubey@samsung.com; swathi.ks@samsung.com;
>>>> ravi.patel@samsung.com
>>>> Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
>>>> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
>>>> kernel@lists.infradead.org; Chandrasekar R <rcsekar@samsung.com>;
>>>> Suresh Siddha <ssiddha@tesla.com>
>>>> Subject: Re: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS
>>>> support
>>>>
>>>> On 14/08/2023 13:25, Sriranjani P wrote:
>>>>> The FSD SoC contains two instance of the Synopsys DWC ethernet QOS
>>>>> IP
>>>> core.
>>>>> The binding that it uses is slightly different from existing ones
>>>>> because of the integration (clocks, resets).
>>>>>
>>>>> For FSD SoC, a mux switch is needed between internal and external
>> clocks.
>>>>> By default after reset internal clock is used but for receiving
>>>>> packets properly, external clock is needed. Mux switch to external
>>>>> clock happens only when the external clock is present.
>>>>>
>>>>> Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
>>>>> Signed-off-by: Suresh Siddha <ssiddha@tesla.com>
>>>>> Signed-off-by: Swathi K S <swathi.ks@samsung.com>
>>>>> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
>>>>> ---
>>>>
>>>>
>>>>> +static int dwc_eqos_setup_rxclock(struct platform_device *pdev, int
>>>>> +ins_num) {
>>>>> +	struct device_node *np = pdev->dev.of_node;
>>>>> +	struct regmap *syscon;
>>>>> +	unsigned int reg;
>>>>> +
>>>>> +	if (np && of_property_read_bool(np, "fsd-rx-clock-skew")) {
>>>>> +		syscon = syscon_regmap_lookup_by_phandle_args(np,
>>>>> +							      "fsd-rx-clock-
>>>> skew",
>>>>> +							      1, &reg);
>>>>> +		if (IS_ERR(syscon)) {
>>>>> +			dev_err(&pdev->dev,
>>>>> +				"couldn't get the rx-clock-skew syscon!\n");
>>>>> +			return PTR_ERR(syscon);
>>>>> +		}
>>>>> +
>>>>> +		regmap_write(syscon, reg, rx_clock_skew_val[ins_num]);
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int fsd_eqos_clk_init(struct fsd_eqos_plat_data *plat,
>>>>> +			     struct plat_stmmacenet_data *data) {
>>>>> +	int ret = 0, i;
>>>>> +
>>>>> +	const struct fsd_eqos_variant *fsd_eqos_v_data =
>>>>> +						plat->fsd_eqos_inst_var;
>>>>> +
>>>>> +	plat->clks = devm_kcalloc(plat->dev, fsd_eqos_v_data->num_clks,
>>>>> +				  sizeof(*plat->clks), GFP_KERNEL);
>>>>> +	if (!plat->clks)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	for (i = 0; i < fsd_eqos_v_data->num_clks; i++)
>>>>> +		plat->clks[i].id = fsd_eqos_v_data->clk_list[i];
>>>>> +
>>>>> +	ret = devm_clk_bulk_get(plat->dev, fsd_eqos_v_data->num_clks,
>>>>> +				plat->clks);
>>>>
>>>> Instead of duplicating entire clock management with existing code,
>>>> you should extend/rework existing one.
>>>>
>>>> This code is unfortunately great example how not to stuff vendor code
>>>> into upstream project. :(
>>>
>>> I will check again if I can extend existing one to support FSD platform
>> specific requirement.
>>>
>>>>
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static int fsd_clks_endisable(void *priv, bool enabled) {
>>>>> +	int ret, num_clks;
>>>>> +	struct fsd_eqos_plat_data *plat = priv;
>>>>> +
>>>>> +	num_clks = plat->fsd_eqos_inst_var->num_clks;
>>>>> +
>>>>> +	if (enabled) {
>>>>> +		ret = clk_bulk_prepare_enable(num_clks, plat->clks);
>>>>> +		if (ret) {
>>>>> +			dev_err(plat->dev, "Clock enable failed, err = %d\n",
>>>> ret);
>>>>> +			return ret;
>>>>> +		}
>>>>> +	} else {
>>>>> +		clk_bulk_disable_unprepare(num_clks, plat->clks);
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int fsd_eqos_probe(struct platform_device *pdev,
>>>>> +			  struct plat_stmmacenet_data *data,
>>>>> +			  struct stmmac_resources *res)
>>>>> +{
>>>>> +	struct fsd_eqos_plat_data *priv_plat;
>>>>> +	struct device_node *np = pdev->dev.of_node;
>>>>> +	int ret = 0;
>>>>> +
>>>>> +	priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat),
>>>> GFP_KERNEL);
>>>>> +	if (!priv_plat) {
>>>>> +		ret = -ENOMEM;
>>>>
>>>> return -ENOMEM
>>>
>>> Will fix this in v4.
>>>
>>>>
>>>>> +		goto error;
>>>>> +	}
>>>>> +
>>>>> +	priv_plat->dev = &pdev->dev;
>>>>> +	data->bus_id = of_alias_get_id(np, "eth");
>>>>
>>>> No, you cannot do like this. Aliases are board specific and are based
>>>> on labeling on the board.
>>>
>>> So if I understood this correctly, I need to move alias in the board
>>> specific DTS file
>>
>> This part: yes
>>
>>> and I can use this, because we have to handle rx-clock-skew differently for
>> the two instances in the FSD platform.
>>
>> Not really. Do you expect it to work correctly if given EQoS instance receives
>> different alias, e.g. 5?
>>
>>> Another approach we took in v1, by specifying the value to be programmed
>> in rx-clock-skew property itself, but it seems it is not a preferred approach.
>>> I can see that in drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> +436 common code is already using this API and getting alias id, so I can
>> probably use the same rather getting same again here, but I have to specify
>> alias in DTS file.
>>
>> Getting alias ID is okay in general. It is used to provide user-visible ID of the
>> devices (see mmc). Using such alias to configure hardware is abuse of the
>> alias, because of the reasons I said - how is it supposed to work if alias is 5
>> (this is perfectly valid alias)?
>>
>> I suspect that all this is to substitute missing abstractions, like clocks, phys or
>> resets...
> 
> Will avoid using the API to get alias id to configure the HW. Will share the new implementation in v4.

That was August 2023, almost year ago.

Whatever you plan, expect having same questions in the discussion
because we forgot everything said that year ago...

Best regards,
Krzysztof

Re: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support

[Andrew Lunn]

> +static const int rx_clock_skew_val[] = {0x2, 0x0};

> +static int dwc_eqos_setup_rxclock(struct platform_device *pdev, int ins_num)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct regmap *syscon;
> +	unsigned int reg;
> +
> +	if (np && of_property_read_bool(np, "fsd-rx-clock-skew")) {
> +		syscon = syscon_regmap_lookup_by_phandle_args(np,
> +							      "fsd-rx-clock-skew",
> +							      1, &reg);
> +		if (IS_ERR(syscon)) {
> +			dev_err(&pdev->dev,
> +				"couldn't get the rx-clock-skew syscon!\n");
> +			return PTR_ERR(syscon);
> +		}
> +
> +		regmap_write(syscon, reg, rx_clock_skew_val[ins_num]);

Please could you explain what this is doing.

       Andrew

RE: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support

[Swathi K S]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: 15 August 2023 02:17
> To: Sriranjani P <sriranjani.p@samsung.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
> richardcochran@gmail.com; alexandre.torgue@foss.st.com;
> joabreu@synopsys.com; mcoquelin.stm32@gmail.com;
> alim.akhtar@samsung.com; linux-fsd@tesla.com;
> pankaj.dubey@samsung.com; swathi.ks@samsung.com;
> ravi.patel@samsung.com; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-
> soc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Chandrasekar R
> <rcsekar@samsung.com>; Suresh Siddha <ssiddha@tesla.com>
> Subject: Re: [PATCH v3 2/4] net: stmmac: dwc-qos: Add FSD EQoS support
> 
> > +static const int rx_clock_skew_val[] = {0x2, 0x0};
> 
> > +static int dwc_eqos_setup_rxclock(struct platform_device *pdev, int
> > +ins_num) {
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct regmap *syscon;
> > +	unsigned int reg;
> > +
> > +	if (np && of_property_read_bool(np, "fsd-rx-clock-skew")) {
> > +		syscon = syscon_regmap_lookup_by_phandle_args(np,
> > +							      "fsd-rx-clock-
> skew",
> > +							      1, &reg);
> > +		if (IS_ERR(syscon)) {
> > +			dev_err(&pdev->dev,
> > +				"couldn't get the rx-clock-skew syscon!\n");
> > +			return PTR_ERR(syscon);
> > +		}
> > +
> > +		regmap_write(syscon, reg, rx_clock_skew_val[ins_num]);
> 
> Please could you explain what this is doing.

As per customer requirement, we need to provide a delay of 2ns in FSYS in
both TX and RX path and no delay in peric block

> 
>        Andrew

Regards,
Swathi

[PATCH v3 3/4] arm64: dts: fsd: Add Ethernet support for FSYS0 Block of FSD SoC

[Sriranjani P]

The FSD SoC contains two instances of Synopsys DWC QoS Ethernet IP, one
in FSYS0 block and other in PERIC block.

Adds device tree node for Ethernet in FSYS0 Block and enables the same for
FSD platform.

Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Jayati Sahu <jayati.sahu@samsung.com>
Signed-off-by: Swathi K S <swathi.ks@samsung.com>
Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
---
 arch/arm64/boot/dts/tesla/fsd-evb.dts      |  9 ++++
 arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 56 ++++++++++++++++++++++
 arch/arm64/boot/dts/tesla/fsd.dtsi         | 22 +++++++++
 3 files changed, 87 insertions(+)

diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts b/arch/arm64/boot/dts/tesla/fsd-evb.dts
index 8d7794642900..2c37097c709a 100644
--- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
+++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
@@ -64,6 +64,15 @@
 	};
 };
 
+&ethernet_0 {
+	status = "okay";
+
+	fixed-link {
+		speed = <1000>;
+		full-duplex;
+	};
+};
+
 &fin_pll {
 	clock-frequency = <24000000>;
 };
diff --git a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
index 3f898cf4874c..cb437483ff6e 100644
--- a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
+++ b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
@@ -64,6 +64,62 @@
 		samsung,pin-pud = <FSD_PIN_PULL_UP>;
 		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
 	};
+
+	eth0_tx_clk: eth0-tx-clk-pins {
+		samsung,pins = "gpf0-0";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV6>;
+	};
+
+	eth0_tx_data: eth0-tx-data-pins {
+		samsung,pins = "gpf0-1", "gpf0-2", "gpf0-3", "gpf0-4";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV6>;
+	};
+
+	eth0_tx_ctrl: eth0-tx-ctrl-pins {
+		samsung,pins = "gpf0-5";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV6>;
+	};
+
+	eth0_phy_intr: eth0-phy-intr-pins {
+		samsung,pins = "gpf0-6";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_NONE>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
+	};
+
+	eth0_rx_clk: eth0-rx-clk-pins {
+		samsung,pins = "gpf1-0";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV6>;
+	};
+
+	eth0_rx_data: eth0-rx-data-pins {
+		samsung,pins = "gpf1-1", "gpf1-2", "gpf1-3", "gpf1-4";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV6>;
+	};
+
+	eth0_rx_ctrl: eth0-rx-ctrl-pins {
+		samsung,pins = "gpf1-5";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV6>;
+	};
+
+	eth0_mdio: eth0-mdio-pins {
+		samsung,pins = "gpf1-6", "gpf1-7";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_NONE>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
+	};
 };
 
 &pinctrl_peric {
diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi b/arch/arm64/boot/dts/tesla/fsd.dtsi
index 1c53c68efd53..9a991f021711 100644
--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
@@ -32,6 +32,7 @@
 		spi0 = &spi_0;
 		spi1 = &spi_1;
 		spi2 = &spi_2;
+		eth0 = &ethernet_0;
 	};
 
 	cpus {
@@ -984,6 +985,27 @@
 			clocks = <&clock_fsys0 UFS0_MPHY_REFCLK_IXTAL26>;
 			clock-names = "ref_clk";
 		};
+
+		ethernet_0: ethernet@15300000 {
+			compatible = "tesla,dwc-qos-ethernet-4.21";
+			reg = <0x0 0x15300000 0x0 0x10000>;
+			interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clock_fsys0 FSYS0_EQOS_TOP0_IPCLKPORT_CLK_PTP_REF_I>,
+				 <&clock_fsys0 FSYS0_EQOS_TOP0_IPCLKPORT_ACLK_I>,
+				 <&clock_fsys0 FSYS0_EQOS_TOP0_IPCLKPORT_HCLK_I>,
+				 <&clock_fsys0 FSYS0_EQOS_TOP0_IPCLKPORT_RGMII_CLK_I>,
+				 <&clock_fsys0 FSYS0_EQOS_TOP0_IPCLKPORT_CLK_RX_I>;
+			clock-names = "ptp_ref", "master_bus", "slave_bus", "tx", "rx";
+			pinctrl-names = "default";
+			pinctrl-0 = <&eth0_tx_clk>, <&eth0_tx_data>, <&eth0_tx_ctrl>,
+				    <&eth0_phy_intr>, <&eth0_rx_clk>, <&eth0_rx_data>,
+				    <&eth0_rx_ctrl>, <&eth0_mdio>;
+			local-mac-address = [00 00 00 00 00 00];
+			fsd-rx-clock-skew = <&sysreg_fsys0 0x0>;
+			iommus = <&smmu_fsys0 0x0 0x1>;
+			phy-mode = "rgmii";
+			status = "disabled";
+		};
 	};
 };
 
-- 
2.17.1

Re: [PATCH v3 3/4] arm64: dts: fsd: Add Ethernet support for FSYS0 Block of FSD SoC

[Krzysztof Kozlowski]

On 14/08/2023 13:25, Sriranjani P wrote:
> The FSD SoC contains two instances of Synopsys DWC QoS Ethernet IP, one
> in FSYS0 block and other in PERIC block.


...

>  
>  	cpus {
> @@ -984,6 +985,27 @@
>  			clocks = <&clock_fsys0 UFS0_MPHY_REFCLK_IXTAL26>;
>  			clock-names = "ref_clk";
>  		};
> +
> +		ethernet_0: ethernet@15300000 {
> +			compatible = "tesla,dwc-qos-ethernet-4.21";

The requirement for entire Samsung and its flavors is to pass
dtbs_check. Since some months.

Does it pass?

Best regards,
Krzysztof

Re: [PATCH v3 3/4] arm64: dts: fsd: Add Ethernet support for FSYS0 Block of FSD SoC

[Krzysztof Kozlowski]

On 14/08/2023 13:25, Sriranjani P wrote:
> The FSD SoC contains two instances of Synopsys DWC QoS Ethernet IP, one
> in FSYS0 block and other in PERIC block.
> 
> Adds device tree node for Ethernet in FSYS0 Block and enables the same for
> FSD platform.
> 

...

>  &pinctrl_peric {
> diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi b/arch/arm64/boot/dts/tesla/fsd.dtsi
> index 1c53c68efd53..9a991f021711 100644
> --- a/arch/arm64/boot/dts/tesla/fsd.dtsi
> +++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
> @@ -32,6 +32,7 @@
>  		spi0 = &spi_0;
>  		spi1 = &spi_1;
>  		spi2 = &spi_2;
> +		eth0 = &ethernet_0;

One more thing - I said it two times already. Patch v1 and then in v2.
You responded now without waiting for my further feedback and
immediately sent the same stuff.

Let's be clear:

NAK for the reasons I said multiple times.

Best regards,
Krzysztof

RE: [PATCH v3 3/4] arm64: dts: fsd: Add Ethernet support for FSYS0 Block of FSD SoC

[Sriranjani P]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 15 August 2023 01:27
> To: Sriranjani P <sriranjani.p@samsung.com>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; richardcochran@gmail.com;
> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
> mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
> fsd@tesla.com; pankaj.dubey@samsung.com; swathi.ks@samsung.com;
> ravi.patel@samsung.com
> Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Jayati Sahu <jayati.sahu@samsung.com>
> Subject: Re: [PATCH v3 3/4] arm64: dts: fsd: Add Ethernet support for FSYS0
> Block of FSD SoC
> 
> On 14/08/2023 13:25, Sriranjani P wrote:
> > The FSD SoC contains two instances of Synopsys DWC QoS Ethernet IP,
> > one in FSYS0 block and other in PERIC block.
> >
> > Adds device tree node for Ethernet in FSYS0 Block and enables the same
> > for FSD platform.
> >
> 
> ...
> 
> >  &pinctrl_peric {
> > diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi
> > b/arch/arm64/boot/dts/tesla/fsd.dtsi
> > index 1c53c68efd53..9a991f021711 100644
> > --- a/arch/arm64/boot/dts/tesla/fsd.dtsi
> > +++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
> > @@ -32,6 +32,7 @@
> >  		spi0 = &spi_0;
> >  		spi1 = &spi_1;
> >  		spi2 = &spi_2;
> > +		eth0 = &ethernet_0;
> 
> One more thing - I said it two times already. Patch v1 and then in v2.
> You responded now without waiting for my further feedback and
> immediately sent the same stuff.
> 
> Let's be clear:
> 
> NAK for the reasons I said multiple times.

Got it, will move this alias in board specific dts file.

> 
> Best regards,
> Krzysztof

Re: [PATCH v3 3/4] arm64: dts: fsd: Add Ethernet support for FSYS0 Block of FSD SoC

[Andrew Lunn]

> +&ethernet_0 {
> +	status = "okay";
> +
> +	fixed-link {
> +		speed = <1000>;
> +		full-duplex;
> +	};
> +};

A fixed link on its own is pretty unusual. Normally it is combined
with an Ethernet switch. What is the link peer here?

     Andrew

RE: [PATCH v3 3/4] arm64: dts: fsd: Add Ethernet support for FSYS0 Block of FSD SoC

[Swathi K S]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: 15 August 2023 02:20
> To: Sriranjani P <sriranjani.p@samsung.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
> richardcochran@gmail.com; alexandre.torgue@foss.st.com;
> joabreu@synopsys.com; mcoquelin.stm32@gmail.com;
> alim.akhtar@samsung.com; linux-fsd@tesla.com;
> pankaj.dubey@samsung.com; swathi.ks@samsung.com;
> ravi.patel@samsung.com; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-
> soc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Jayati Sahu
> <jayati.sahu@samsung.com>
> Subject: Re: [PATCH v3 3/4] arm64: dts: fsd: Add Ethernet support for
FSYS0
> Block of FSD SoC
> 
> > +&ethernet_0 {
> > +	status = "okay";
> > +
> > +	fixed-link {
> > +		speed = <1000>;
> > +		full-duplex;
> > +	};
> > +};
> 
> A fixed link on its own is pretty unusual. Normally it is combined with an
> Ethernet switch. What is the link peer here?

It is a direct connection to the Ethernet switch managed by an external
management unit.

> 
>      Andrew

Regards,
Swathi

Re: [PATCH v3 3/4] arm64: dts: fsd: Add Ethernet support for FSYS0 Block of FSD SoC

[Andrew Lunn]

> > > +&ethernet_0 {
> > > +	status = "okay";
> > > +
> > > +	fixed-link {
> > > +		speed = <1000>;
> > > +		full-duplex;
> > > +	};
> > > +};
> > 
> > A fixed link on its own is pretty unusual. Normally it is combined with an
> > Ethernet switch. What is the link peer here?
> 
> It is a direct connection to the Ethernet switch managed by an external
> management unit.

Ah, interesting. This is the third example of this in about a
month. Take a look at the Realtek and TI work in this area.

So, i will ask the same questions i put to Realtek and TI. Does Linux
know about the switch in any way? Can it manage the switch, other than
SNMP, HTTP from user space? Does it know about the state of the ports,
etc?

If you say this is just a colocated management switch, which Linux is
not managing in any way, that is O.K. If you have Linux involved in
some way, please join the discussion with TI about adding a new model
for semi-autonomous switches.

   Andrew

RE: [PATCH v3 3/4] arm64: dts: fsd: Add Ethernet support for FSYS0 Block of FSD SoC

[Swathi K S]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 06 June 2024 18:53
> To: Swathi K S <swathi.ks@samsung.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; richardcochran@gmail.com;
> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
> mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
> fsd@tesla.com; pankaj.dubey@samsung.com; ravi.patel@samsung.com;
> netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; 'Jayati Sahu' <jayati.sahu@samsung.com>;
Siddharth
> Vadapalli <s-vadapalli@ti.com>
> Subject: Re: [PATCH v3 3/4] arm64: dts: fsd: Add Ethernet support for
FSYS0
> Block of FSD SoC
> 
> > > > +&ethernet_0 {
> > > > +	status = "okay";
> > > > +
> > > > +	fixed-link {
> > > > +		speed = <1000>;
> > > > +		full-duplex;
> > > > +	};
> > > > +};
> > >
> > > A fixed link on its own is pretty unusual. Normally it is combined
> > > with an Ethernet switch. What is the link peer here?
> >
> > It is a direct connection to the Ethernet switch managed by an
> > external management unit.
> 
> Ah, interesting. This is the third example of this in about a month. Take
a look at
> the Realtek and TI work in this area.
> 
> So, i will ask the same questions i put to Realtek and TI. Does Linux know
about
> the switch in any way? Can it manage the switch, other than SNMP, HTTP
from
> user space? Does it know about the state of the ports, etc?
> 
> If you say this is just a colocated management switch, which Linux is not
> managing in any way, that is O.K. If you have Linux involved in some way,
please
> join the discussion with TI about adding a new model for semi-autonomous
> switches.

Thanks for letting us know about the ongoing discussion.
But in this case, the switch is not managed by Linux.

> 
>    Andrew

Thanks,
Swathi

Re: [PATCH v3 3/4] arm64: dts: fsd: Add Ethernet support for FSYS0 Block of FSD SoC

[Andrew Lunn]

> diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi b/arch/arm64/boot/dts/tesla/fsd.dtsi
> index 1c53c68efd53..9a991f021711 100644
> --- a/arch/arm64/boot/dts/tesla/fsd.dtsi
> +++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
> @@ -32,6 +32,7 @@
>  		spi0 = &spi_0;
>  		spi1 = &spi_1;
>  		spi2 = &spi_2;
> +		eth0 = &ethernet_0;
>  	};
>  
>  	cpus {
> @@ -984,6 +985,27 @@
>  			clocks = <&clock_fsys0 UFS0_MPHY_REFCLK_IXTAL26>;
>  			clock-names = "ref_clk";
>  		};
> +
> +		ethernet_0: ethernet@15300000 {
> +			compatible = "tesla,dwc-qos-ethernet-4.21";
> +			reg = <0x0 0x15300000 0x0 0x10000>;
> +			interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&clock_fsys0 FSYS0_EQOS_TOP0_IPCLKPORT_CLK_PTP_REF_I>,
> +				 <&clock_fsys0 FSYS0_EQOS_TOP0_IPCLKPORT_ACLK_I>,
> +				 <&clock_fsys0 FSYS0_EQOS_TOP0_IPCLKPORT_HCLK_I>,
> +				 <&clock_fsys0 FSYS0_EQOS_TOP0_IPCLKPORT_RGMII_CLK_I>,
> +				 <&clock_fsys0 FSYS0_EQOS_TOP0_IPCLKPORT_CLK_RX_I>;
> +			clock-names = "ptp_ref", "master_bus", "slave_bus", "tx", "rx";
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&eth0_tx_clk>, <&eth0_tx_data>, <&eth0_tx_ctrl>,
> +				    <&eth0_phy_intr>, <&eth0_rx_clk>, <&eth0_rx_data>,
> +				    <&eth0_rx_ctrl>, <&eth0_mdio>;
> +			local-mac-address = [00 00 00 00 00 00];
> +			fsd-rx-clock-skew = <&sysreg_fsys0 0x0>;
> +			iommus = <&smmu_fsys0 0x0 0x1>;
> +			phy-mode = "rgmii";

What is inserting the RGMII delays?

      Andrew

[PATCH v3 4/4] arm64: dts: fsd: Add Ethernet support for PERIC Block of FSD SoC

[Sriranjani P]

The FSD SoC contains two instances of Synopsys DWC QoS Ethernet IP, one in
FSYS0 block and other in PERIC block.

Adds device tree node for Ethernet in PERIC Block and enables the same for
FSD platform.

Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Jayati Sahu <jayati.sahu@samsung.com>
Signed-off-by: Swathi K S <swathi.ks@samsung.com>
Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
---
 arch/arm64/boot/dts/tesla/fsd-evb.dts      |  9 ++++
 arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 56 ++++++++++++++++++++++
 arch/arm64/boot/dts/tesla/fsd.dtsi         | 29 +++++++++++
 3 files changed, 94 insertions(+)

diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts b/arch/arm64/boot/dts/tesla/fsd-evb.dts
index 2c37097c709a..80ca120b3d7f 100644
--- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
+++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
@@ -73,6 +73,15 @@
 	};
 };
 
+&ethernet_1 {
+	status = "okay";
+
+	fixed-link {
+		speed = <1000>;
+		full-duplex;
+	};
+};
+
 &fin_pll {
 	clock-frequency = <24000000>;
 };
diff --git a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
index cb437483ff6e..6f4658f57453 100644
--- a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
+++ b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
@@ -437,6 +437,62 @@
 		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
 		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
 	};
+
+	eth1_tx_clk: eth1-tx-clk-pins {
+		samsung,pins = "gpf2-0";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV6>;
+	};
+
+	eth1_tx_data: eth1-tx-data-pins {
+		samsung,pins = "gpf2-1", "gpf2-2", "gpf2-3", "gpf2-4";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV6>;
+	};
+
+	eth1_tx_ctrl: eth1-tx-ctrl-pins {
+		samsung,pins = "gpf2-5";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV6>;
+	};
+
+	eth1_phy_intr: eth1-phy-intr-pins {
+		samsung,pins = "gpf2-6";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
+	};
+
+	eth1_rx_clk: eth1-rx-clk-pins {
+		samsung,pins = "gpf3-0";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV6>;
+	};
+
+	eth1_rx_data: eth1-rx-data-pins {
+		samsung,pins = "gpf3-1", "gpf3-2", "gpf3-3", "gpf3-4";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV6>;
+	};
+
+	eth1_rx_ctrl: eth1-rx-ctrl-pins {
+		samsung,pins = "gpf3-5";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV6>;
+	};
+
+	eth1_mdio: eth1-mdio-pins {
+		samsung,pins = "gpf3-6", "gpf3-7";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_UP>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
+	};
 };
 
 &pinctrl_pmu {
diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi b/arch/arm64/boot/dts/tesla/fsd.dtsi
index 9a991f021711..ce5d5f8546b9 100644
--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
@@ -33,6 +33,7 @@
 		spi1 = &spi_1;
 		spi2 = &spi_2;
 		eth0 = &ethernet_0;
+		eth1 = &ethernet_1;
 	};
 
 	cpus {
@@ -1006,6 +1007,34 @@
 			phy-mode = "rgmii";
 			status = "disabled";
 		};
+
+		ethernet_1: ethernet@14300000 {
+			compatible = "tesla,dwc-qos-ethernet-4.21";
+			reg = <0x0 0x14300000 0x0 0x10000>;
+			interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_CLK_PTP_REF_I>,
+				 <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_ACLK_I>,
+				 <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_HCLK_I>,
+				 <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_RGMII_CLK_I>,
+				 <&clock_peric PERIC_EQOS_TOP_IPCLKPORT_CLK_RX_I>,
+				 <&clock_peric PERIC_BUS_D_PERIC_IPCLKPORT_EQOSCLK>,
+				 <&clock_peric PERIC_BUS_P_PERIC_IPCLKPORT_EQOSCLK>,
+				 <&clock_peric PERIC_EQOS_PHYRXCLK_MUX>,
+				 <&clock_peric PERIC_EQOS_PHYRXCLK>,
+				 <&clock_peric PERIC_DOUT_RGMII_CLK>;
+			clock-names = "ptp_ref", "master_bus", "slave_bus", "tx", "rx",
+				      "master2_bus", "slave2_bus", "eqos_rxclk_mux",
+				      "eqos_phyrxclk", "dout_peric_rgmii_clk";
+			pinctrl-names = "default";
+			pinctrl-0 = <&eth1_tx_clk>, <&eth1_tx_data>, <&eth1_tx_ctrl>,
+				    <&eth1_phy_intr>, <&eth1_rx_clk>, <&eth1_rx_data>,
+				    <&eth1_rx_ctrl>, <&eth1_mdio>;
+			local-mac-address = [00 00 00 00 00 00];
+			fsd-rx-clock-skew = <&sysreg_peric 0x10>;
+			iommus = <&smmu_peric 0x0 0x1>;
+			phy-mode = "rgmii";
+			status = "disabled";
+		};
 	};
 };
 
-- 
2.17.1

Re: [PATCH v3 4/4] arm64: dts: fsd: Add Ethernet support for PERIC Block of FSD SoC

[Krzysztof Kozlowski]

On 14/08/2023 13:25, Sriranjani P wrote:
> The FSD SoC contains two instances of Synopsys DWC QoS Ethernet IP, one in
> FSYS0 block and other in PERIC block.
> 
> Adds device tree node for Ethernet in PERIC Block and enables the same for
> FSD platform.
> 
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Signed-off-by: Jayati Sahu <jayati.sahu@samsung.com>
> Signed-off-by: Swathi K S <swathi.ks@samsung.com>
> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> ---
>  arch/arm64/boot/dts/tesla/fsd-evb.dts      |  9 ++++
>  arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 56 ++++++++++++++++++++++
>  arch/arm64/boot/dts/tesla/fsd.dtsi         | 29 +++++++++++
>  3 files changed, 94 insertions(+)

Looks duplicated.

Best regards,
Krzysztof

Re: [PATCH v3 4/4] arm64: dts: fsd: Add Ethernet support for PERIC Block of FSD SoC

[Krzysztof Kozlowski]

On 14/08/2023 21:41, Krzysztof Kozlowski wrote:
> On 14/08/2023 13:25, Sriranjani P wrote:
>> The FSD SoC contains two instances of Synopsys DWC QoS Ethernet IP, one in
>> FSYS0 block and other in PERIC block.
>>
>> Adds device tree node for Ethernet in PERIC Block and enables the same for
>> FSD platform.
>>
>> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
>> Signed-off-by: Jayati Sahu <jayati.sahu@samsung.com>
>> Signed-off-by: Swathi K S <swathi.ks@samsung.com>
>> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
>> ---
>>  arch/arm64/boot/dts/tesla/fsd-evb.dts      |  9 ++++
>>  arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 56 ++++++++++++++++++++++
>>  arch/arm64/boot/dts/tesla/fsd.dtsi         | 29 +++++++++++
>>  3 files changed, 94 insertions(+)
> 
> Looks duplicated.

Ah, not, it's another block.

My question whether this was tested remains...

Best regards,
Krzysztof

RE: [PATCH v3 4/4] arm64: dts: fsd: Add Ethernet support for PERIC Block of FSD SoC

[Sriranjani P]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 15 August 2023 01:12
> To: Sriranjani P <sriranjani.p@samsung.com>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; richardcochran@gmail.com;
> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
> mcoquelin.stm32@gmail.com; alim.akhtar@samsung.com; linux-
> fsd@tesla.com; pankaj.dubey@samsung.com; swathi.ks@samsung.com;
> ravi.patel@samsung.com
> Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Jayati Sahu <jayati.sahu@samsung.com>
> Subject: Re: [PATCH v3 4/4] arm64: dts: fsd: Add Ethernet support for PERIC
> Block of FSD SoC
> 
> On 14/08/2023 21:41, Krzysztof Kozlowski wrote:
> > On 14/08/2023 13:25, Sriranjani P wrote:
> >> The FSD SoC contains two instances of Synopsys DWC QoS Ethernet IP,
> >> one in
> >> FSYS0 block and other in PERIC block.
> >>
> >> Adds device tree node for Ethernet in PERIC Block and enables the
> >> same for FSD platform.
> >>
> >> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> >> Signed-off-by: Jayati Sahu <jayati.sahu@samsung.com>
> >> Signed-off-by: Swathi K S <swathi.ks@samsung.com>
> >> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> >> ---
> >>  arch/arm64/boot/dts/tesla/fsd-evb.dts      |  9 ++++
> >>  arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 56
> ++++++++++++++++++++++
> >>  arch/arm64/boot/dts/tesla/fsd.dtsi         | 29 +++++++++++
> >>  3 files changed, 94 insertions(+)
> >
> > Looks duplicated.
> 
> Ah, not, it's another block.
> 
> My question whether this was tested remains...

I understand your concern. It was tested but just before posting I addressed one of earlier review comment of compatible and I missed to reflect it in all the patches. I will take care. 

> 
> Best regards,
> Krzysztof

Re: [PATCH v3 4/4] arm64: dts: fsd: Add Ethernet support for PERIC Block of FSD SoC

[Andrew Lunn]

> +&ethernet_1 {
> +	status = "okay";
> +
> +	fixed-link {
> +		speed = <1000>;
> +		full-duplex;
> +	};
> +};

So the exact same comments i made for patch 3/4 apply here.

   Andrew