Re: [PATCH v3 1/3] dt-bindings: net: Add T-HEAD dwmac support

[Krzysztof Kozlowski <krzk@kernel.org>]

On 02/10/2024 21:08, Drew Fustini wrote:
> On Tue, Oct 01, 2024 at 08:58:34AM +0200, Krzysztof Kozlowski wrote:
>> On Mon, Sep 30, 2024 at 11:23:24PM -0700, Drew Fustini wrote:
>>> From: Jisheng Zhang <jszhang@kernel.org>
>>>
>>> Add documentation to describe the DesginWare-based GMAC controllers in
>>> the T-HEAD TH1520 SoC.
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>>> Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
>>> [drew: rename compatible, add apb registers as second reg of gmac node]
>>> Signed-off-by: Drew Fustini <dfustini@tenstorrent.com>
>>> ---
>>>  .../devicetree/bindings/net/snps,dwmac.yaml        |  1 +
>>>  .../devicetree/bindings/net/thead,th1520-gmac.yaml | 97 ++++++++++++++++++++++
>>>  MAINTAINERS                                        |  1 +
>>>  3 files changed, 99 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> index 4e2ba1bf788c..474ade185033 100644
>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> @@ -99,6 +99,7 @@ properties:
>>>          - snps,dwxgmac-2.10
>>>          - starfive,jh7100-dwmac
>>>          - starfive,jh7110-dwmac
>>> +        - thead,th1520-gmac
>>>  
>>>    reg:
>>>      minItems: 1
>>> diff --git a/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
>>> new file mode 100644
>>> index 000000000000..fef1810b10c4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
>>> @@ -0,0 +1,97 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/net/thead,th1520-gmac.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: T-HEAD TH1520 GMAC Ethernet controller
>>> +
>>> +maintainers:
>>> +  - Drew Fustini <dfustini@tenstorrent.com>
>>> +
>>> +description: |
>>> +  The TH1520 GMAC is described in the TH1520 Peripheral Interface User Manual
>>> +  https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/tree/main/docs
>>> +
>>> +  Features include
>>> +    - Compliant with IEEE802.3 Specification
>>> +    - IEEE 1588-2008 standard for precision networked clock synchronization
>>> +    - Supports 10/100/1000Mbps data transfer rate
>>> +    - Supports RGMII/MII interface
>>> +    - Preamble and start of frame data (SFD) insertion in Transmit path
>>> +    - Preamble and SFD deletion in the Receive path
>>> +    - Automatic CRC and pad generation options for receive frames
>>> +    - MDIO master interface for PHY device configuration and management
>>> +
>>> +  The GMAC Registers consists of two parts
>>> +    - APB registers are used to configure clock frequency/clock enable/clock
>>> +      direction/PHY interface type.
>>> +    - AHB registers are use to configure GMAC core (DesignWare Core part).
>>> +      GMAC core register consists of DMA registers and GMAC registers.
>>> +
>>> +select:
>>> +  properties:
>>> +    compatible:
>>> +      contains:
>>> +        enum:
>>> +          - thead,th1520-gmac
>>> +  required:
>>> +    - compatible
>>> +
>>> +allOf:
>>> +  - $ref: snps,dwmac.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - thead,th1520-gmac
>>> +      - const: snps,dwmac-3.70a
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: DesignWare GMAC IP core registers
>>> +      - description: GMAC APB registers
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: dwmac
>>> +      - const: apb
>>
>> I don't get why none of snps,dwmac properties are restricted. How many
>> interrupts do you have here? How many clocks? resets?
> 
> Thanks for pointing this out. Yes, I forgot to document the clocks,
> interrupts and resets.
> 
> There needs to be 2 clocks (stmmaceth and pclk). There also needs to be
> 2 resets: each GMAC has a reset plus a seperate reset for the GMAC AXI
> interface. There is 1 interrupt (macirq) but it is optional. The BeagleV
> Ahead uses it but the LicheePi 4A does not use the interrupt.
> 
> However, I'm uncertain about how to restrict the snps,dwmac properties.
> 
> I see that starfive,jh7110-dwmac.yaml has the following logic. Should I
> be adding something like this to restrict the snps,dwmac properties?
> 
> ---------------------------------------------------------------------
> allOf:
>   - $ref: snps,dwmac.yaml#
> 
>   - if:
>       properties:
>         compatible:
>           contains:
>             const: starfive,jh7100-dwmac
>     then:
>       properties:
>         interrupts:
>           minItems: 2
>           maxItems: 2
> 
>         interrupt-names:
>           minItems: 2
>           maxItems: 2
> 
>         resets:
>           maxItems: 1
> 
>         reset-names:
>           const: ahb

No, that's not necessary. You have one device in this binding, so in
top-level should define how each such property looks like.

Best regards,
Krzysztof