Messages in this thread | | | Date | Wed, 30 Nov 2022 18:33:11 +0100 | Subject | Re: [PATCH v2 2/5] dt-bindings: net: add schema for NXP S32CC dwmac glue driver | From | Andreas Färber <> |
| |
Hi Krysztof,
Am 30.11.22 um 16:51 schrieb Krzysztof Kozlowski: > On 28/11/2022 06:49, Chester Lin wrote: >> Add the DT schema for the DWMAC Ethernet controller on NXP S32 Common >> Chassis. >> >> Signed-off-by: Jan Petrous <jan.petrous@nxp.com> >> Signed-off-by: Chester Lin <clin@suse.com> > > Thank you for your patch. There is something to discuss/improve. > >> --- >> >> 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 [...] >> +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? > >> + 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
>> + >> + 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. > >> + 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)
| |