Re: [PATCH v2 2/5] dt-bindings: net: add schema for NXP S32CC dwmac glue driver

[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@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?
>
> > +    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)