Re: [PATCH 1/7] dt-bindings: net: add STM32MP13 compatible in documentation for stm32

From: Conor Dooley
Date: Fri Sep 22 2023 - 07:43:17 EST

Yo,

On Thu, Sep 21, 2023 at 05:06:16PM +0200, Christophe Roullier wrote:
> New STM32 SOC have 2 GMACs instances.
> GMAC IP version is SNPS 4.20.
>
> Signed-off-by: Christophe Roullier <christophe.roullier@xxxxxxxxxxx>
> ---
> .../devicetree/bindings/net/stm32-dwmac.yaml | 140 +++++++++++++++---
> 1 file changed, 118 insertions(+), 22 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
> index fc8c96b08d7d..75836916c38c 100644
> --- a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
> @@ -22,15 +22,17 @@ select:
> enum:
> - st,stm32-dwmac
> - st,stm32mp1-dwmac
> + - st,stm32mp13-dwmac
> required:
> - compatible
>
> -allOf:
> - - $ref: snps,dwmac.yaml#
> -
> properties:
> compatible:
> oneOf:
> + - items:
> + - enum:
> + - st,stm32mp13-dwmac
> + - const: snps,dwmac-4.20a

The enum just below this is also for the 4.20a, no? Why not just put
this mp13 compatible into that enum?

> - items:
> - enum:
> - st,stm32mp1-dwmac
> @@ -72,27 +74,69 @@ properties:
> - eth-ck
> - ptp_ref
>
> - st,syscon:

Please try to avoid defining properties inside if/then/else sections and
only move the variable bits if possible.

> - $ref: /schemas/types.yaml#/definitions/phandle-array
> - items:
> - - items:
> - - description: phandle to the syscon node which encompases the glue register
> - - description: offset of the control register
> + phy-supply:
> + description: PHY regulator
> +
> + st,ext-phyclk:
> description:
> - Should be phandle/offset pair. The phandle to the syscon node which
> - encompases the glue register, and the offset of the control register
> + set this property in RMII mode when you have PHY without crystal 50MHz and want to
> + select RCC clock instead of ETH_REF_CLK. or in RGMII mode when you want to select
> + RCC clock instead of ETH_CLK125.
> + type: boolean
>
> st,eth-clk-sel:
> + deprecated: true

Why have these been marked as deprecated? That doesn't appear to be
mention in the commit message & sounds like it should be a different
commit.

> description:
> set this property in RGMII PHY when you want to select RCC clock instead of ETH_CLK125.
> type: boolean
>
> st,eth-ref-clk-sel:
> + deprecated: true

Ditto.

> description:
> set this property in RMII mode when you have PHY without crystal 50MHz and want to
> select RCC clock instead of ETH_REF_CLK.
> type: boolean
>
> +allOf:
> + - $ref: snps,dwmac.yaml#
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - st,stm32mp1-dwmac
> + - st,stm32-dwmac
> + then:
> + properties:
> + st,syscon:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + items:
> + - items:
> + - description: phandle to the syscon node which encompases the glue register
> + - description: offset of the control register
> + description:
> + Should be phandle/offset pair. The phandle to the syscon node which
> + encompases the glue register, and the offset of the control register
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - st,stm32mp13-dwmac

You've got 2 if/then sections containing tests for 3 compatibles. There
are only 2 compatibles total right now & 3 with the patch, so it looks
like you'd get away with if/then/else instead.

> + then:
> + properties:
> + st,syscon:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + items:
> + - items:
> + - description: phandle to the syscon node which encompases the glue register
> + - description: offset of the control register
> + - description: field to set mask in register
> + description:
> + Should be phandle/offset pair. The phandle to the syscon node which
> + encompases the glue register, the offset of the control register and
> + the mask to set bitfield in control register
> +
> required:
> - compatible
> - clocks
> @@ -112,24 +156,36 @@ examples:
> compatible = "st,stm32mp1-dwmac", "snps,dwmac-4.20a";

I don't understand why this existing example is changing.

Thanks,
Conor.

> reg = <0x5800a000 0x2000>;
> reg-names = "stmmaceth";
> - interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
> - interrupt-names = "macirq";
> + interrupts-extended = <&intc GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>,
> + <&exti 70 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "macirq",
> + "eth_wake_irq";
> clock-names = "stmmaceth",
> - "mac-clk-tx",
> - "mac-clk-rx",
> - "ethstp",
> - "eth-ck";
> + "mac-clk-tx",
> + "mac-clk-rx",
> + "eth-ck",
> + "ptp_ref",
> + "ethstp";
> clocks = <&rcc ETHMAC>,
> - <&rcc ETHTX>,
> - <&rcc ETHRX>,
> - <&rcc ETHSTP>,
> - <&rcc ETHCK_K>;
> + <&rcc ETHTX>,
> + <&rcc ETHRX>,
> + <&rcc ETHCK_K>,
> + <&rcc ETHPTP_K>,
> + <&rcc ETHSTP>;
> st,syscon = <&syscfg 0x4>;
> + snps,mixed-burst;
> snps,pbl = <2>;
> + snps,en-tx-lpi-clockgating;
> snps,axi-config = <&stmmac_axi_config_0>;
> snps,tso;
> phy-mode = "rgmii";
> - };
> +
> + stmmac_axi_config_0: stmmac-axi-config {
> + snps,wr_osr_lmt = <0x7>;
> + snps,rd_osr_lmt = <0x7>;
> + snps,blen = <0 0 0 0 16 8 4>;
> + };
> + };
>
> - |
> //Example 2 (MCU example)
> @@ -161,3 +217,43 @@ examples:
> snps,pbl = <8>;
> phy-mode = "mii";
> };
> +
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/clock/stm32mp1-clks.h>
> + #include <dt-bindings/reset/stm32mp1-resets.h>
> + #include <dt-bindings/mfd/stm32h7-rcc.h>
> + //Example 4
> + ethernet3: ethernet@5800a000 {
> + compatible = "st,stm32mp13-dwmac", "snps,dwmac-4.20a";
> + reg = <0x5800a000 0x2000>;
> + reg-names = "stmmaceth";
> + interrupts-extended = <&intc GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>,
> + <&exti 68 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "macirq",
> + "eth_wake_irq";
> + clock-names = "stmmaceth",
> + "mac-clk-tx",
> + "mac-clk-rx",
> + "eth-ck",
> + "ptp_ref",
> + "ethstp";
> + clocks = <&rcc ETHMAC>,
> + <&rcc ETHTX>,
> + <&rcc ETHRX>,
> + <&rcc ETHCK_K>,
> + <&rcc ETHPTP_K>,
> + <&rcc ETHSTP>;
> + st,syscon = <&syscfg 0x4 0xff0000>;
> + snps,mixed-burst;
> + snps,pbl = <2>;
> + snps,axi-config = <&stmmac_axi_config_1>;
> + snps,tso;
> + phy-mode = "rmii";
> +
> + stmmac_axi_config_1: stmmac-axi-config {
> + snps,wr_osr_lmt = <0x7>;
> + snps,rd_osr_lmt = <0x7>;
> + snps,blen = <0 0 0 0 16 8 4>;
> + };
> + };
> --
> 2.25.1
>

Attachment: signature.asc
Description: PGP signature