diff mbox series

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

Message ID 20221128054920.2113-3-clin@suse.com
State New
Headers
Series Add GMAC support for S32 SoC family |

Commit Message

Chester Lin Nov. 28, 2022, 5:49 a.m. UTC 
  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>
---

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

Comments

Krzysztof Kozlowski Nov. 30, 2022, 3:51 p.m. UTC | #1 
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
> @@ -0,0 +1,135 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2021-2022 NXP
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/net/nxp,s32cc-dwmac.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

Drop quotes from both.

> +
> +title: NXP S32CC DWMAC Ethernet controller
> +
> +maintainers:
> +  - Jan Petrous <jan.petrous@nxp.com>
> +  - Chester Lin <clin@suse.com>
> +
> +allOf:
> +  - $ref: "snps,dwmac.yaml#"

Drop quotes.

> +
> +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.

> +
> +    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
Andreas Färber Nov. 30, 2022, 5:33 p.m. UTC | #2 
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
>
Andrew Lunn Nov. 30, 2022, 6:14 p.m. UTC | #3 
> 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.

Just be aware, you are setting an ABI here. So your fluid SCMI
firmware interface must forever support this.

	 Andrew
Krzysztof Kozlowski Dec. 1, 2022, 10:18 a.m. UTC | #4 
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@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?

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
Chester Lin Dec. 5, 2022, 7:54 a.m. UTC | #5 
Hi Krzysztof,

On Thu, Dec 01, 2022 at 11:18:57AM +0100, Krzysztof Kozlowski wrote:
> 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@nxp.com>
> >>> Signed-off-by: Chester Lin <clin@suse.com>
> >>
> >> Thank you for your patch. There is something to discuss/improve.
> >>

Thanks for taking time to review this patch!

> >>> ---
> >>>
> >>> 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?
> 

Actually these GMAC-related SCMI clock IDs changed once in NXP's downstream TF-A,
some redundant TS clock IDs were removed and the rest of clock IDs were all moved
forward. Apart from GMAC-related IDs, some other clock IDs were also appended
in both base-clock IDs and platform-specific clock IDs [The first plat ID =
The last base ID + 1]. Due to the current design of the clk-scmi driver and the
SCMI clock protocol, IIUC, it's better to keep all clock IDs in sequence without
a blank in order to avoid query miss, which could affect the probe speed.

> > 
> > 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
>
Krzysztof Kozlowski Dec. 5, 2022, 8:55 a.m. UTC | #6 
On 05/12/2022 08:54, Chester Lin wrote:
>>>>> +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?
>>
> 
> Actually these GMAC-related SCMI clock IDs changed once in NXP's downstream TF-A,
> some redundant TS clock IDs were removed and the rest of clock IDs were all moved
> forward. 

This is not accepted. Your downstream TF-A cannot change bindings. As an
upstream contributor you should push this back and definitely not try to
upstream such approach.

> Apart from GMAC-related IDs, some other clock IDs were also appended
> in both base-clock IDs and platform-specific clock IDs [The first plat ID =
> The last base ID + 1]. Due to the current design of the clk-scmi driver and the
> SCMI clock protocol, IIUC, it's better to keep all clock IDs in sequence without
> a blank in order to avoid query miss, which could affect the probe speed.

You miss here broken ABI! Any change in IDs causes all DTBs to be
broken. Downstream, upstream, other projects, everywhere.

Therefore thank you for clarifying that we need to be more careful about
stuff coming from (or for) NXP. Here you need to drop all defines and
all your patches must assume the ID is fixed. Once there, it's
unchangeable without breaking the ABI.

Best regards,
Krzysztof
Chester Lin Dec. 13, 2022, 2:46 a.m. UTC | #7 
Hi Krzysztof,

Sorry for the late reply.

On Mon, Dec 05, 2022 at 09:55:40AM +0100, Krzysztof Kozlowski wrote:
> On 05/12/2022 08:54, Chester Lin wrote:
> >>>>> +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?
> >>
> > 
> > Actually these GMAC-related SCMI clock IDs changed once in NXP's downstream TF-A,
> > some redundant TS clock IDs were removed and the rest of clock IDs were all moved
> > forward. 
> 
> This is not accepted. Your downstream TF-A cannot change bindings. As an
> upstream contributor you should push this back and definitely not try to
> upstream such approach.
> 
> > Apart from GMAC-related IDs, some other clock IDs were also appended
> > in both base-clock IDs and platform-specific clock IDs [The first plat ID =
> > The last base ID + 1]. Due to the current design of the clk-scmi driver and the
> > SCMI clock protocol, IIUC, it's better to keep all clock IDs in sequence without
> > a blank in order to avoid query miss, which could affect the probe speed.
> 
> You miss here broken ABI! Any change in IDs causes all DTBs to be
> broken. Downstream, upstream, other projects, everywhere.
> 
> Therefore thank you for clarifying that we need to be more careful about
> stuff coming from (or for) NXP. Here you need to drop all defines and
> all your patches must assume the ID is fixed. Once there, it's
> unchangeable without breaking the ABI.
> 

Please accept my apologies for submitting these bad patches. We were just
confused since we thought that this approach might be acceptable because
there were some similar examples got merged in the kernel tree:

https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/net/intel,dwmac-plat.yaml#L57
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/usb/intel,keembay-dwc3.yaml#L55
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/sound/intel,keembay-i2s.yaml#L73
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml#L39
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/watchdog/intel,keembay-wdt.yaml#L46
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml#L75
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml#L282

The defines in these yaml files are not actually referred by kernel DTs or
drivers so I assume that they should be provided by firmware as pure integer
numbers and the clk-scmi driver should just take them to ask firmware for doing
clk stuff.

Regards,
Chester

> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Dec. 13, 2022, 7:50 a.m. UTC | #8 
On 13/12/2022 03:46, Chester Lin wrote:
> Hi Krzysztof,
> 
> Sorry for the late reply.
> 
> On Mon, Dec 05, 2022 at 09:55:40AM +0100, Krzysztof Kozlowski wrote:
>> On 05/12/2022 08:54, Chester Lin wrote:
>>>>>>> +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?
>>>>
>>>
>>> Actually these GMAC-related SCMI clock IDs changed once in NXP's downstream TF-A,
>>> some redundant TS clock IDs were removed and the rest of clock IDs were all moved
>>> forward. 
>>
>> This is not accepted. Your downstream TF-A cannot change bindings. As an
>> upstream contributor you should push this back and definitely not try to
>> upstream such approach.
>>
>>> Apart from GMAC-related IDs, some other clock IDs were also appended
>>> in both base-clock IDs and platform-specific clock IDs [The first plat ID =
>>> The last base ID + 1]. Due to the current design of the clk-scmi driver and the
>>> SCMI clock protocol, IIUC, it's better to keep all clock IDs in sequence without
>>> a blank in order to avoid query miss, which could affect the probe speed.
>>
>> You miss here broken ABI! Any change in IDs causes all DTBs to be
>> broken. Downstream, upstream, other projects, everywhere.
>>
>> Therefore thank you for clarifying that we need to be more careful about
>> stuff coming from (or for) NXP. Here you need to drop all defines and
>> all your patches must assume the ID is fixed. Once there, it's
>> unchangeable without breaking the ABI.
>>
> 
> Please accept my apologies for submitting these bad patches. We were just
> confused since we thought that this approach might be acceptable because
> there were some similar examples got merged in the kernel tree:

How are these related to the talk of ABI break?

> 
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/net/intel,dwmac-plat.yaml#L57
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/usb/intel,keembay-dwc3.yaml#L55
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/sound/intel,keembay-i2s.yaml#L73
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml#L39
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/watchdog/intel,keembay-wdt.yaml#L46
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml#L75
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml#L282

How are these even relevant here? The support for this platform is
incomplete, right? These are just few scattered pieces - only bindings -
without drivers and DTS. It proves nothing. You cannot take some
incomplete platform and build on top of it theory that you can keep
changing ABI!

And anyway it is entirely independent problem. You just said you want to
change defines which is not allowed. ABI break. No one changes the
Keembay defines, whatever they are (you even do not know what they are...).

> 
> The defines in these yaml files are not actually referred by kernel DTs or
> drivers so I assume that they should be provided by firmware as pure integer
> numbers and the clk-scmi driver should just take them to ask firmware for doing
> clk stuff.


Best regards,
Krzysztof
diff mbox series

Patch

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
@@ -0,0 +1,135 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2021-2022 NXP
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/net/nxp,s32cc-dwmac.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: NXP S32CC DWMAC Ethernet controller
+
+maintainers:
+  - Jan Petrous <jan.petrous@nxp.com>
+  - Chester Lin <clin@suse.com>
+
+allOf:
+  - $ref: "snps,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
+    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
+
+    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 */
+        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>;
+        clock-names = "stmmaceth", "pclk",
+                      "tx_pcs", "tx_rgmii", "tx_rmii", "tx_mii",
+                      "rx_pcs", "rx_rgmii", "rx_rmii", "rx_mii",
+                      "ptp_ref";
+
+        gmac0_mdio: mdio {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          compatible = "snps,dwmac-mdio";
+
+          ethernet-phy@4 {
+            reg = <0x04>;
+          };
+        };
+      };
+    };