On 30/11/2022 18:33, Andreas Färber wrote: > 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@xxxxxxx> >>> Signed-off-by: Chester Lin <clin@xxxxxxxx> >> >> 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? Any comments here? If not, drop minItems. >> >>> + 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. This does not explain to me why you added defines in the example. Are you saying these can change any moment? > > 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. You should rather fix firmware then create some incorrect bindings as a workaround... Best regards, Krzysztof
