Message-ID: <7c2f6af3-5686-452a-8d8a-191899b3d225@gmail.com>
Date: Thu, 7 Nov 2024 18:15:51 +0800
From: Joey Lu <a0987203069@...il.com>
To: Conor Dooley <conor@...nel.org>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, mcoquelin.stm32@...il.com, richardcochran@...il.com,
 alexandre.torgue@...s.st.com, joabreu@...opsys.com, ychuang3@...oton.com,
 schung@...oton.com, yclu4@...oton.com, linux-arm-kernel@...ts.infradead.org,
 netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, openbmc@...ts.ozlabs.org,
 linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH 1/3] dt-bindings: net: nuvoton: Add schema for Nuvoton
 MA35 family GMAC

Dear Conor,

Thank you for your reply.

Conor Dooley 於 11/6/2024 11:44 PM 寫道:
> On Wed, Nov 06, 2024 at 07:19:28PM +0800, Joey Lu wrote:
>> Create initial schema for Nuvoton MA35 family Gigabit MAC.
>>
>> Signed-off-by: Joey Lu <a0987203069@...il.com>
>> ---
>>   .../bindings/net/nuvoton,ma35xx-dwmac.yaml    | 163 ++++++++++++++++++
>>   1 file changed, 163 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/net/nuvoton,ma35xx-dwmac.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/nuvoton,ma35xx-dwmac.yaml b/Documentation/devicetree/bindings/net/nuvoton,ma35xx-dwmac.yaml
>> new file mode 100644
>> index 000000000000..f4d24ca872b2
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/nuvoton,ma35xx-dwmac.yaml
>> @@ -0,0 +1,163 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/nuvoton,ma35xx-dwmac.yaml#
> The filename needs to match the compatible.
I will fix it.
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Nuvoton DWMAC glue layer controller
>> +
>> +maintainers:
>> +  - Joey Lu <yclu4@...oton.com>
>> +
>> +description:
>> +  Nuvoton 10/100/1000Mbps Gigabit Ethernet MAC Controller is based on
>> +  Synopsys DesignWare MAC (version 3.73a).
>> +
>> +# We need a select here so we don't match all nodes with 'snps,dwmac'
>> +select:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        enum:
>> +          - nuvoton,ma35d1-dwmac
>> +  required:
>> +    - compatible
>> +
>> +allOf:
>> +  - $ref: snps,dwmac.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    - items:
>> +        - enum:
>> +            - nuvoton,ma35d1-dwmac
>> +        - const: snps,dwmac-3.70a
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    minItems: 2
>> +    items:
>> +      - description: MAC clock
>> +      - description: PTP clock
>> +
>> +  clock-names:
>> +    minItems: 2
>> +    contains:
>> +      - enum:
>> +          - stmmaceth
>> +          - ptp_ref
> This can just be an items list like interrupt-names, since the clocks
> property has a fixed order.
I will fix it.
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: macirq
> This name carries no information, this is an interrupt for a mac after
> all. You don't need a name since you only have one interrupt.
This interrupt name is an argument required by the stmmac driver to 
obtain interrupt information.
>> +  nuvoton,sys:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: phandle to access GCR (Global Control Register) registers.
> Why do you need a phandle to this? You appear to have multiple dwmacs on
> your device if the example is anything to go by, how come you don't need
> to access different portions of this depending on which dwmac instance
> you are?
On our platform, a system register is required to specify the TX/RX 
clock path delay control, switch modes between RMII and RGMII, and 
configure other related settings.
>> +  resets:
>> +    maxItems: 1
>> +
>> +  reset-names:
>> +    items:
>> +      - const: stmmaceth
>> +
>> +  mac-id:
>> +    maxItems: 1
>> +    description:
>> +      The interface of MAC.
> A vendor prefix is required for custom properties, but I don't think you
> need this and actually it is a bandaid for some other information you're
> missing. Probably related to your nuvoton,sys property only being a
> phandle with no arguments.
This property will be removed.
>> +
>> +  phy-mode:
>> +    enum:
>> +      - rmii
>> +      - rgmii-id
>> +
>> +  tx_delay:
> Needs constraints, a type, a vendor prefix and a unit suffix. No
> underscores in property names either. See the amlogic dwmac binding for
> an example.
I will fix it.
>> +    maxItems: 1
>> +    description:
>> +      Control transmit clock path delay in nanoseconds.
>> +
>> +  rx_delay:
> Ditto here.
I will fix it.
>
>> +    maxItems: 1
>> +    description:
>> +      Control receive clock path delay in nanoseconds.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - interrupt-names
>> +  - clocks
>> +  - clock-names
>> +  - nuvoton,sys
>> +  - resets
>> +  - reset-names
>> +  - mac-id
>> +  - phy-mode
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
>> +    #include <dt-bindings/reset/nuvoton,ma35d1-reset.h>
>> +    //Example 1
>> +    eth0: ethernet@...20000 {
> The eth0 label is not used, drop it.
The label is used in dtsi and dts.
>> +        compatible = "nuvoton,ma35d1-dwmac";
>> +        reg = <0x0 0x40120000 0x0 0x10000>;
>> +        interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
>> +        interrupt-names = "macirq";
>> +        clocks = <&clk EMAC0_GATE>, <&clk EPLL_DIV8>;
>> +        clock-names = "stmmaceth", "ptp_ref";
>> +
>> +        nuvoton,sys = <&sys>;
>> +        resets = <&sys MA35D1_RESET_GMAC0>;
>> +        reset-names = "stmmaceth";
>> +        mac-id = <0>;
>> +
>> +        clk_csr = <4>;
> This property is not documented.
This unused property will be removed.
>
> Cheers,
> Conor.
>
>> +        phy-mode = "rgmii-id";
>> +        phy-handle = <&eth_phy0>;
>> +        mdio0 {
>> +            compatible = "snps,dwmac-mdio";
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            eth_phy0: ethernet-phy@0 {
>> +                reg = <0>;
>> +            };
>> +        };
>> +    };
>> +
>> +  - |
>> +    //Example 2
>> +    eth1: ethernet@...30000 {
>> +        compatible = "nuvoton,ma35d1-dwmac";
>> +        reg = <0x0 0x40130000 0x0 0x10000>;
>> +        interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
>> +        interrupt-names = "macirq";
>> +        clocks = <&clk EMAC1_GATE>, <&clk EPLL_DIV8>;
>> +        clock-names = "stmmaceth", "ptp_ref";
>> +
>> +        nuvoton,sys = <&sys>;
>> +        resets = <&sys MA35D1_RESET_GMAC1>;
>> +        reset-names = "stmmaceth";
>> +        mac-id = <1>;
>> +
>> +        clk_csr = <4>;
>> +        phy-mode = "rmii";
>> +        phy-handle = <&eth_phy1>;
>> +        mdio1 {
>> +            compatible = "snps,dwmac-mdio";
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            eth_phy1: ethernet-phy@1 {
>> +                reg = <1>;
>> +            };
>> +        };
>> +    };
>> -- 
>> 2.34.1

Thanks!

BR,

Joey

Powered by blists - more mailing lists