Hi Krysztof,

Am 30.11.22 um 16:51 schrieb Krzysztof Kozlowski:

On 28/11/2022 06:49, Chester Lin wrote:

quoted

Add the DT schema for the DWMAC Ethernet controller on NXP S32 Common
Chassis.

Signed-off-by: Jan Petrous <redacted>
Signed-off-by: Chester Lin <redacted>

Thank you for your patch. There is something to discuss/improve.

quoted

---

Changes in v2:
   - Fix schema issues.
   - Add minItems to clocks & clock-names.
   - Replace all sgmii/SGMII terms with pcs/PCS.

  .../bindings/net/nxp,s32cc-dwmac.yaml         | 135 ++++++++++++++++++
  1 file changed, 135 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/net/nxp,s32cc-dwmac.yaml

diff --git a/Documentation/devicetree/bindings/net/nxp,s32cc-dwmac.yaml b/Documentation/devicetree/bindings/net/nxp,s32cc-dwmac.yaml
new file mode 100644
index 000000000000..c6839fd3df40
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/nxp,s32cc-dwmac.yaml

[...]

quoted

+properties:
+  compatible:
+    enum:
+      - nxp,s32cc-dwmac
+
+  reg:
+    items:
+      - description: Main GMAC registers
+      - description: S32 MAC control registers
+
+  dma-coherent: true
+
+  clocks:
+    minItems: 5

Why only 5 clocks are required? Receive clocks don't have to be there?
Is such system - only with clocks for transmit - usable?

quoted

+    items:
+      - description: Main GMAC clock
+      - description: Peripheral registers clock
+      - description: Transmit PCS clock
+      - description: Transmit RGMII clock
+      - description: Transmit RMII clock
+      - description: Transmit MII clock
+      - description: Receive PCS clock
+      - description: Receive RGMII clock
+      - description: Receive RMII clock
+      - description: Receive MII clock
+      - description:
+          PTP reference clock. This clock is used for programming the
+          Timestamp Addend Register. If not passed then the system
+          clock will be used.
+
+  clock-names:
+    minItems: 5
+    items:
+      - const: stmmaceth
+      - const: pclk
+      - const: tx_pcs
+      - const: tx_rgmii
+      - const: tx_rmii
+      - const: tx_mii
+      - const: rx_pcs
+      - const: rx_rgmii
+      - const: rx_rmii
+      - const: rx_mii
+      - const: ptp_ref
+
+  tx-fifo-depth:
+    const: 20480
+
+  rx-fifo-depth:
+    const: 20480
+
+required:
+  - compatible
+  - reg
+  - tx-fifo-depth
+  - rx-fifo-depth
+  - clocks
+  - clock-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    #define S32GEN1_SCMI_CLK_GMAC0_AXI
+    #define S32GEN1_SCMI_CLK_GMAC0_TX_PCS
+    #define S32GEN1_SCMI_CLK_GMAC0_TX_RGMII
+    #define S32GEN1_SCMI_CLK_GMAC0_TX_RMII
+    #define S32GEN1_SCMI_CLK_GMAC0_TX_MII
+    #define S32GEN1_SCMI_CLK_GMAC0_RX_PCS
+    #define S32GEN1_SCMI_CLK_GMAC0_RX_RGMII
+    #define S32GEN1_SCMI_CLK_GMAC0_RX_RMII
+    #define S32GEN1_SCMI_CLK_GMAC0_RX_MII
+    #define S32GEN1_SCMI_CLK_GMAC0_TS

Why defines? Your clock controller is not ready? If so, just use raw
numbers.

Please compare v1: There is no Linux-driven clock controller here but 
rather a fluid SCMI firmware interface. Work towards getting clocks into 
a kernel-hosted .dtsi was halted in favor of (downstream) TF-A, which 
also explains the ugly examples here and for pinctrl.

Logically there are only 5 input clocks; however due to SCMI not 
supporting re-parenting today, some clocks got duplicated at SCMI level. 
Andrew appeared to approve of that approach. I still dislike it but 
don't have a better proposal that would work today. So the two values 
above indeed seem wrong and should be 11 rather than 5.

Cheers,
Andreas

quoted

+
+    soc {
+      #address-cells = <1>;
+      #size-cells = <1>;
+
+      gmac0: ethernet@4033c000 {
+        compatible = "nxp,s32cc-dwmac";
+        reg = <0x4033c000 0x2000>, /* gmac IP */
+              <0x4007C004 0x4>;    /* S32 CTRL_STS reg */

Lowercase hex.

quoted

+        interrupt-parent = <&gic>;
+        interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "macirq";
+        phy-mode = "rgmii-id";
+        tx-fifo-depth = <20480>;
+        rx-fifo-depth = <20480>;
+        dma-coherent;
+        clocks = <&clks S32GEN1_SCMI_CLK_GMAC0_AXI>,
+                 <&clks S32GEN1_SCMI_CLK_GMAC0_AXI>,
+                 <&clks S32GEN1_SCMI_CLK_GMAC0_TX_PCS>,
+                 <&clks S32GEN1_SCMI_CLK_GMAC0_TX_RGMII>,
+                 <&clks S32GEN1_SCMI_CLK_GMAC0_TX_RMII>,
+                 <&clks S32GEN1_SCMI_CLK_GMAC0_TX_MII>,
+                 <&clks S32GEN1_SCMI_CLK_GMAC0_RX_PCS>,
+                 <&clks S32GEN1_SCMI_CLK_GMAC0_RX_RGMII>,
+                 <&clks S32GEN1_SCMI_CLK_GMAC0_RX_RMII>,
+                 <&clks S32GEN1_SCMI_CLK_GMAC0_RX_MII>,
+                 <&clks S32GEN1_SCMI_CLK_GMAC0_TS>;

Best regards,
Krzysztof

-- 
SUSE Software Solutions Germany GmbH
Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nürnberg)