Re: [PATCH 3/5] dt-bindings: net: Add Loongson-1 DWMAC glue layer

[Serge Semin]

On Fri, Aug 18, 2023 at 06:42:42PM +0800, Keguang Zhang wrote:
> On Wed, Aug 16, 2023 at 8:54 PM Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> >
> > Hi Keguang
> >
> > On Sat, Aug 12, 2023 at 11:11:33PM +0800, Keguang Zhang wrote:
> > > Add devicetree binding document for Loongson-1 DWMAC glue layer.
> > >
> > > Signed-off-by: Keguang Zhang <keguang.zhang@xxxxxxxxx>
> > > ---
> > >  .../bindings/net/loongson,ls1x-dwmac.yaml     | 98 +++++++++++++++++++
> > >  .../devicetree/bindings/net/snps,dwmac.yaml   |  2 +
> > >  2 files changed, 100 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/net/loongson,ls1x-dwmac.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/net/loongson,ls1x-dwmac.yaml b/Documentation/devicetree/bindings/net/loongson,ls1x-dwmac.yaml
> > > new file mode 100644
> > > index 000000000000..150799460599
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/loongson,ls1x-dwmac.yaml
> > > @@ -0,0 +1,98 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/net/loongson,ls1x-dwmac.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> >
> > > +title: Loongson-1 DWMAC glue layer
> >
> > DT-schemas describe a device. It has nothing to do with the glue
> > driver/layer/whatever.
> >

> OK. But what about the MODULE_DESCRIPTION in dwmac-loongson1.c
> MODULE_DESCRIPTION("Loongson1 DWMAC glue layer");
> Should the two parts be aligned with each other?

No they shouldn't. MODULE_DESCRIPTION() describes the driver module.
"Loongson1 (G)MAC glue layer" is a correct description of the kernel
driver module.

> If not, what's your suggestion then?

Something like "Loongson-1 Ethernet controller" or "Loongson-1 (G)MAC
controller". A name which would refer to the device itself
irrespective to the driver name, driver design, etc.

* Note the already available DW (XG)MAC vendor-specific DT-bindings
* referring to the glue layer/driver in the title property are wrong
* in doing that.

> 
> > Also I suggest to add a brief device description in the
> > "description:" property and add there a brief info regarding the SoCs
> > the controllers can be found on, the DW (G)MAC IP-core version the
> > ethernet controllers are based on and if possible some data about the
> > synthesize parameters: SMA (MDIO-bus), Tx/Rx COE, DMA FIFOs size,
> > perfect and hash MAC-filters size, L3L4 frame filters availability,
> > PHY interfaces (MII, RMII, RGMII, etc), EEE support, IEEE 1588(-2008)
> > Timestamping support, PMT and Wake-up frame support, MAC Management
> > counters (MMC).
> >
> > Note DMA FIFO sizes can be also constrained in the properties
> > "rx-fifo-depth" and "tx-fifo-depth"; perfect and hash MAC-filter sizes -
> > in "snps,perfect-filter-entries" and "snps,multicast-filter-bins".
> >
> OK. The description could be added in next version.
> 
> > > +
> > > +maintainers:
> > > +  - Keguang Zhang <keguang.zhang@xxxxxxxxx>
> > > +
> > > +select:
> > > +  properties:
> > > +    compatible:
> > > +      contains:
> > > +        enum:
> > > +          - loongson,ls1b-dwmac
> > > +          - loongson,ls1c-dwmac
> > > +  required:
> > > +    - compatible
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - enum:

> > > +          - loongson,ls1b-dwmac
> > > +          - loongson,ls1c-dwmac

BTW referring to the DW IP-core in the compatible string isn't very
much useful especially seeing you have a generic fallback compatible.

The next names would be more descriptive:
loongson,ls1b-gmac - seeing MAC supports 10/100/1000 speed modes
loongson,ls1c-mac - seeing MAC support 10/100 speed modes only


> > > +      - const: snps,dwmac-3.50a
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  clock-names:
> > > +    const: stmmaceth
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  interrupt-names:
> > > +    const: macirq
> > > +
> >
> > > +  syscon:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description:
> > > +      Phandle to the syscon containing some extra configurations
> > > +      including PHY interface mode.
> >
> > I believe the property is supposed to have a vendor-specific name like
> > "loongson,ls1-syscon" or similar.
> 

> This has been fixed in v2.

The name "loongson,dwmac-syscon" doesn't look correct because "dwmac-"
prefix refer to some DWMAC system controller meanwhile the phandle
passed to the device is a generic Loongson1 SoC system controller. So
"loongson,ls1-syscon" looks more suitable.

> Could you please review v2?
> Thanks!

I'll have a look at v3 since v2 doesn't have my comments taken into
account. BTW don't rush with resubmitting your series. Give it at
least one week or so to hang out in the reviewers mail boxes as the
Linux kernel patches review process suggests.

> >
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - clock-names
> > > +  - interrupts
> > > +  - interrupt-names
> > > +  - phy-handle
> >
> > > +  - phy-mode
> >
> > You may want to specify the enum-constraints with the value permitted
> > for the particular Loongson (G)MAC controller. Seeing ls1b and ls1c
> > imply different sets of the PHY-modes the constraints are better to be
> > defined in the allOf sub-schemas. Alternatively you can split the
> > DT-schema file into two: one for ls1b-dwmac, another one for
> > ls1c-dwmac. IMO the later option seems better.
> >

> The "phy-mode", as pointed by Krzysztof, is defined in
> ethernet-controller and already required by snps,dwmac.
> So I have dropped it in v2.

My point was in specifying a particular constraints on the "phy-mode"
property. Krzysztof correctly suggested to drop the property from the
"required" list since it's already required by the snps,dwmac.yaml
schema. One doesn't contradict to another.

> For allOf sub-schemas, do you mean something below?
> allOf:
>  - $ref: snps,dwmac.yaml#
> 
>  - if:
>      properties:
>        compatible:
>          contains:
>            const: loongson,ls1b-dwmac
>    then:
>      properties:
>        phy-mode:
>          enum:
>            - mii
>            - rgmii
> 
>  - if:
>      properties:
>        compatible:
>          contains:
>            const: loongson,ls1c-dwmac
>    then:
>      properties:
>        phy-mode:
>          enum:
>            - mii
>            - rmii

Yes. But IMO in order to prevent having such complicated multi-level
schemas you can just split up your bindings into two:
loongson,ls1b-dwmac.yaml
and
loongson,ls1c-dwmac.yaml

Thus you'll be able to have a device-specific generic "title" and
"description" in each of them (especially seeing LS1-C MAC lacks of
1000Mbps mode support which you said you would add to the bindings
description), simpler "compatible" and "phy-mode" property
constraints.

-Serge(y)

> 
> > -Serge(y)
> >
> > > +  - syscon
> > > +
> > > +allOf:
> > > +  - $ref: snps,dwmac.yaml#
> > > +
> > > +unevaluatedProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/clock/loongson,ls1x-clk.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +
> > > +    gmac0: ethernet@1fe10000 {
> > > +        compatible = "loongson,ls1b-dwmac", "snps,dwmac-3.50a";
> > > +        reg = <0x1fe10000 0x10000>;
> > > +
> > > +        clocks = <&clkc LS1X_CLKID_AHB>;
> > > +        clock-names = "stmmaceth";
> > > +
> > > +        interrupt-parent = <&intc1>;
> > > +        interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
> > > +        interrupt-names = "macirq";
> > > +
> > > +        phy-handle = <&phy0>;
> > > +        phy-mode = "mii";
> > > +
> > > +        snps,pbl = <1>;
> > > +        syscon = <&syscon>;
> > > +
> > > +        mdio {
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +            compatible = "snps,dwmac-mdio";
> > > +
> > > +            phy0: ethernet-phy@0 {
> > > +                reg = <0x0>;
> > > +            };
> > > +        };
> > > +    };
> > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > > index ddf9522a5dc2..e1a956cf171e 100644
> > > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > > @@ -66,6 +66,8 @@ properties:
> > >          - ingenic,x2000-mac
> > >          - loongson,ls2k-dwmac
> > >          - loongson,ls7a-dwmac
> > > +        - loongson,ls1b-dwmac
> > > +        - loongson,ls1c-dwmac
> > >          - qcom,qcs404-ethqos
> > >          - qcom,sa8775p-ethqos
> > >          - qcom,sc8280xp-ethqos
> > > --
> > > 2.39.2
> > >
> 
> 
> 
> --
> Best regards,
> 
> Keguang Zhang