Re: [PATCH 1/3] dt-bindings: net: nuvoton: Add schema for Nuvoton MA35 family GMAC

Re: [PATCH 1/3] dt-bindings: net: nuvoton: Add schema for Nuvoton MA35 family GMAC

[Joey Lu]

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@gmail.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@nuvoton.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@40120000 {
> 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@40130000 {
>> +        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

Re: [PATCH 1/3] dt-bindings: net: nuvoton: Add schema for Nuvoton MA35 family GMAC

[Krzysztof Kozlowski]

On 07/11/2024 11:15, Joey Lu wrote:
>>> +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@40120000 {
>> The eth0 label is not used, drop it.
> The label is used in dtsi and dts.

But we do not talk about DTSI or DTS here. Comments appear in specific
places in specific patches. We do not discuss here other patches :/

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: net: nuvoton: Add schema for Nuvoton MA35 family GMAC

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1576 bytes --]

On Thu, Nov 07, 2024 at 06:15:51PM +0800, Joey Lu wrote:
> Conor Dooley 於 11/6/2024 11:44 PM 寫道:
> > On Wed, Nov 06, 2024 at 07:19:28PM +0800, Joey Lu wrote:
> > > +  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.

I'm almost certain you can't just remove this property, because you need
it to tell which portion of the GCR is applicable to the dwmac instance
in question. Instead, you need to ad an argument to your phandle. The
starfive dwmac binding/driver has an example of what you can do.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] dt-bindings: net: nuvoton: Add schema for Nuvoton MA35 family GMAC

[Joey Lu]

Conor Dooley 於 11/8/2024 1:09 AM 寫道:
> On Thu, Nov 07, 2024 at 06:15:51PM +0800, Joey Lu wrote:
>> Conor Dooley 於 11/6/2024 11:44 PM 寫道:
>>> On Wed, Nov 06, 2024 at 07:19:28PM +0800, Joey Lu wrote:
>>>> +  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.
> I'm almost certain you can't just remove this property, because you need
> it to tell which portion of the GCR is applicable to the dwmac instance
> in question. Instead, you need to ad an argument to your phandle. The
> starfive dwmac binding/driver has an example of what you can do.

Yes, I will use this method instead.🙂

mac-id and tx/rx-delay will be arguments of syscon.

Thanks!

Re: [PATCH 1/3] dt-bindings: net: nuvoton: Add schema for Nuvoton MA35 family GMAC

[Joey Lu]

Dear Andrew,

Thank you for your reply.

Andrew Lunn 於 11/7/2024 2:13 AM 寫道:
>> +  phy-mode:
>> +    enum:
>> +      - rmii
>> +      - rgmii-id
> The phy-mode deepened on the board design. All four rgmii values are
> valid.
I will add them.
>> +
>> +  tx_delay:
>> +    maxItems: 1
>> +    description:
>> +      Control transmit clock path delay in nanoseconds.
>> +
>> +  rx_delay:
>> +    maxItems: 1
>> +    description:
>> +      Control receive clock path delay in nanoseconds.
> If you absolutely really need these, keep them, but i suggest you drop
> them. They just cause confusion, when ideally we want the PHY to be
> adding RGMII delays, not the MAC.
>
> If you do need them, then they should be in pS.

I will fix it.

We have customers who use a fixed link instead of a PHY, so these 
properties may be necessary.

> 	Andrew

Thanks!

BR,

Joey

Re: [PATCH 1/3] dt-bindings: net: nuvoton: Add schema for Nuvoton MA35 family GMAC

[Andrew Lunn]

On Thu, Nov 07, 2024 at 06:31:26PM +0800, Joey Lu wrote:
> Dear Andrew,
> 
> Thank you for your reply.
> 
> Andrew Lunn 於 11/7/2024 2:13 AM 寫道:
> > > +  phy-mode:
> > > +    enum:
> > > +      - rmii
> > > +      - rgmii-id
> > The phy-mode deepened on the board design. All four rgmii values are
> > valid.
> I will add them.
> > > +
> > > +  tx_delay:
> > > +    maxItems: 1
> > > +    description:
> > > +      Control transmit clock path delay in nanoseconds.
> > > +
> > > +  rx_delay:
> > > +    maxItems: 1
> > > +    description:
> > > +      Control receive clock path delay in nanoseconds.
> > If you absolutely really need these, keep them, but i suggest you drop
> > them. They just cause confusion, when ideally we want the PHY to be
> > adding RGMII delays, not the MAC.
> > 
> > If you do need them, then they should be in pS.
> 
> I will fix it.
> 
> We have customers who use a fixed link instead of a PHY, so these properties
> may be necessary.

That is a legitimate use case which can require the MAC to add delays,
but i generally try to get the switch on the other end to add the
delays, just to keep with the uniform setup.

Also, please take a look at ethernet-controller.yaml, these should be
called rx-internal-delay-ps & tx-internal-delay-ps.

	Andrew

Re: [PATCH 1/3] dt-bindings: net: nuvoton: Add schema for Nuvoton MA35 family GMAC

[Joey Lu]

On 11/7/24 21:18, Andrew Lunn wrote:
> On Thu, Nov 07, 2024 at 06:31:26PM +0800, Joey Lu wrote:
>> Dear Andrew,
>>
>> Thank you for your reply.
>>
>> Andrew Lunn 於 11/7/2024 2:13 AM 寫道:
>>>> +  phy-mode:
>>>> +    enum:
>>>> +      - rmii
>>>> +      - rgmii-id
>>> The phy-mode deepened on the board design. All four rgmii values are
>>> valid.
>> I will add them.
>>>> +
>>>> +  tx_delay:
>>>> +    maxItems: 1
>>>> +    description:
>>>> +      Control transmit clock path delay in nanoseconds.
>>>> +
>>>> +  rx_delay:
>>>> +    maxItems: 1
>>>> +    description:
>>>> +      Control receive clock path delay in nanoseconds.
>>> If you absolutely really need these, keep them, but i suggest you drop
>>> them. They just cause confusion, when ideally we want the PHY to be
>>> adding RGMII delays, not the MAC.
>>>
>>> If you do need them, then they should be in pS.
>> I will fix it.
>>
>> We have customers who use a fixed link instead of a PHY, so these properties
>> may be necessary.
> That is a legitimate use case which can require the MAC to add delays,
> but i generally try to get the switch on the other end to add the
> delays, just to keep with the uniform setup.
>
> Also, please take a look at ethernet-controller.yaml, these should be
> called rx-internal-delay-ps & tx-internal-delay-ps.
>
> 	Andrew

Thank you, sir. I will use rx-internal-delay-ps and tx-internal-delay-ps 
instead.

                 Joey

Re: [PATCH 1/3] dt-bindings: net: nuvoton: Add schema for Nuvoton MA35 family GMAC

[Joey Lu]

Andrew Lunn 於 11/7/2024 2:13 AM 寫道:
>> +  mac-id:
>> +    maxItems: 1
>> +    description:
>> +      The interface of MAC.
> Please could you expand on what this is?
This property will be removed. Thanks.
> 	Andrew

Re: [PATCH 3/3] net: stmmac: dwmac-nuvoton: Add dwmac glue for Nuvoton MA35 family

[Joey Lu]

Dear Andrew,

Thank you for your reply.

Andrew Lunn 於 11/7/2024 2:23 AM 寫道:
>> +struct nvt_priv_data {
>> +	struct platform_device *pdev;
>> +	int id;
>> +	struct regmap *regmap;
>> +	phy_interface_t phy_mode;
> phy_mode does not seem to be used outside of nuvoton_gmac_setup(). In
> fact nothing in nvt_priv_data is used outside of
> nuvoton_gmac_setup. So it looks like you can remove it.
I will remove it.
>> +	if (of_property_read_u32(dev->of_node, "tx_delay", &tx_delay)) {
>> +		dev_info(dev, "Set TX delay(0x0).\n");
>> +		tx_delay = 0x0;
>> +	} else {
>> +		dev_info(dev, "Set TX delay(0x%x).\n", tx_delay);
> Please don't spam the logs. dev_dbg(), or no message at all.
>
> 	Andrew
I will fix it.

Thanks!

BR,

Joey