Date: Fri, 16 Feb 2024 17:36:57 +0800
From: Yang Xiwen <forbidden405@...look.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 Yisen Zhuang <yisen.zhuang@...wei.com>, Salil Mehta
 <salil.mehta@...wei.com>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Yang Xiwen <forbidden405@...mail.com>,
 Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
 Russell King <linux@...linux.org.uk>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH 4/6] dt-bindings: net: add hisilicon-femac

On 2/16/2024 3:24 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 00:48, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@...look.com>
>>
>> This binding gets rewritten. Compared to previous txt based binding doc,
>> the following changes are made:
>>
>> - No "hisi-femac-v1/2" binding anymore
>> - Remove unused Hi3516 SoC, add Hi3798MV200
>> - add MDIO subnode
>> - add phy clock and reset
> I don't understand why conversion you make in two commits. Please
> perform proper conversion and then propose changes to the binding.
>
>> Signed-off-by: Yang Xiwen <forbidden405@...look.com>
>> ---
>>   .../devicetree/bindings/net/hisilicon-femac.yaml   | 125 +++++++++++++++++++++
>>   1 file changed, 125 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/hisilicon-femac.yaml b/Documentation/devicetree/bindings/net/hisilicon-femac.yaml
>> new file mode 100644
>> index 000000000000..008127e148aa
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/hisilicon-femac.yaml
> Use compatible as filename.
>
>> @@ -0,0 +1,125 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/hisilicon-femac.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Hisilicon Fast Ethernet MAC controller
>> +
>> +maintainers:
>> +  - Yang Xiwen <forbidden405@...mail.com>
>> +
>> +allOf:
>> +  - $ref: ethernet-controller.yaml
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - hisilicon,hi3798mv200-femac
>> +
>> +  reg:
>> +    minItems: 2
>> +    maxItems: 2
>> +    description: |
>> +      The first region is the MAC core register base and size.
>> +      The second region is the global MAC control register.
> Just items: - description: instead of all this.
>
>> +
>> +  ranges:
>> +    maxItems: 1
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    minItems: 3
> Drop
>
>> +    maxItems: 3
>> +
>> +  clock-names:
>> +    items:
>> +      - const: mac
>> +      - const: macif
>> +      - const: phy
>> +
>> +  resets:
>> +    minItems: 2
> Drop
>
>> +    maxItems: 2
>> +
>> +  reset-names:
>> +    items:
>> +      - const: mac
>> +      - const: phy
>> +
>> +  hisilicon,phy-reset-delays-us:
>> +    minItems: 3
>> +    maxItems: 3
>> +    description: |
>> +      The 1st cell is reset pre-delay in micro seconds.
>> +      The 2nd cell is reset pulse in micro seconds.
>> +      The 3rd cell is reset post-delay in micro seconds.
> items:
>   - description:
>
> Anyway, isn't this property of the phy?
It ought to be. But it seems a bit hard to implement it like this.
>
>
>> +
>> +patternProperties:
>> +  '^mdio@[0-9a-f]+$':
>> +    type: object
>> +    description: See ./hisi-femac-mdio.txt
> No, please first convert other file and then reference it here.
>
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +  - resets
>> +  - reset-names
>> +  - phy-connection-type
>> +  - phy-handle
>> +  - hisilicon,phy-reset-delays-us
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/histb-clock.h>
>> +
>> +    #ifndef HISTB_ETH0_PHY_CLK
>> +    #define HISTB_ETH0_PHY_CLK 0
>> +    #endif
> Drop these defines.

It is depending on a local patch which update 
include/dt-bindings/clock/histb-clock.h. But currently i'm not going to 
sent it since i'm still tweaking clock drivers frequently.

Removing these defines now would probably leads to a compile error.

Or I can just change them to some magic numbers.

>
>> +    femac: ethernet@...0000 {
> Drop label.
>
>> +        compatible = "hisilicon,hi3798mv200-femac";
>> +        reg = <0x9c30000 0x1000>, <0x9c31300 0x200>;
>> +        ranges = <0x0 0x9c30000 0x10000>;
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +        interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
>> +        clocks = <&crg HISTB_ETH0_MAC_CLK>,
>> +                 <&crg HISTB_ETH0_MACIF_CLK>,
>> +                 <&crg HISTB_ETH0_PHY_CLK>;
>> +        clock-names = "mac", "macif", "phy";
>> +        resets = <&crg 0xd0 3>, <&crg 0x388 4>;
>> +        reset-names = "mac", "phy";
>> +        phy-handle = <&fephy>;
>> +        phy-connection-type = "mii";
>> +        // To be filled by bootloader
>> +        mac-address = [00 00 00 00 00 00];
>> +        hisilicon,phy-reset-delays-us = <10000 10000 500000>;
>> +        status = "okay";
> Drop
>
>> +
>> +        mdio: mdio@...0 {
> Drop label
>
>> +            compatible = "hisilicon,hisi-femac-mdio";
>> +            reg = <0x1100 0x20>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +            status = "okay";
> Drop
>
>> +
>> +            fephy: ethernet-phy@1 {
> Drop label
>
>
>> +                reg = <1>;
>> +                #phy-cells = <0>;
>> +            };
>> +        };
>> +    };
>>
> Best regards,
> Krzysztof
>

-- 
Regards,
Yang Xiwen

Powered by blists - more mailing lists