| Message ID | 20230812151135.1028780-4-keguang.zhang@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Move Loongson1 MAC arch-code to the driver dir | expand |
On 12/08/2023 17:11, Keguang Zhang wrote: > Add devicetree binding document for Loongson-1 DWMAC glue layer. > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> > --- > .../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 > + > +maintainers: > + - Keguang Zhang <keguang.zhang@gmail.com> > + > +select: > + properties: > + compatible: > + contains: > + enum: > + - loongson,ls1b-dwmac > + - loongson,ls1c-dwmac > + required: > + - compatible > + > +properties: > + compatible: > + items: > + - enum: > + - loongson,ls1b-dwmac > + - loongson,ls1c-dwmac > + - const: snps,dwmac-3.50a > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + clock-names: > + const: stmmaceth This should be list (items) with one const item. > + > + interrupts: > + maxItems: 1 > + > + interrupt-names: > + const: macirq Ditto > + > + syscon: Let me quote: "Phandle to syscon device requires a vendor, descriptive name and a description" You only got description right. > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + Phandle to the syscon containing some extra configurations > + including PHY interface mode. > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - interrupts > + - interrupt-names > + - phy-handle > + - phy-mode Drop, it is not defined here and already required by snps,dwmac. > + - 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 You should not need it. Isn't snps,dwmac-3.50a already there? Anyway, not alphabetically ordered... > - qcom,qcs404-ethqos > - qcom,sa8775p-ethqos > - qcom,sc8280xp-ethqos Best regards, Krzysztof
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@gmail.com> > --- > .../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. 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". > + > +maintainers: > + - Keguang Zhang <keguang.zhang@gmail.com> > + > +select: > + properties: > + compatible: > + contains: > + enum: > + - loongson,ls1b-dwmac > + - loongson,ls1c-dwmac > + required: > + - compatible > + > +properties: > + compatible: > + items: > + - enum: > + - loongson,ls1b-dwmac > + - loongson,ls1c-dwmac > + - 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. > + > +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. -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 >
On Wed, Aug 16, 2023 at 8:54 PM Serge Semin <fancer.lancer@gmail.com> 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@gmail.com> > > --- > > .../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? If not, what's your suggestion then? > 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@gmail.com> > > + > > +select: > > + properties: > > + compatible: > > + contains: > > + enum: > > + - loongson,ls1b-dwmac > > + - loongson,ls1c-dwmac > > + required: > > + - compatible > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - loongson,ls1b-dwmac > > + - loongson,ls1c-dwmac > > + - 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. Could you please review v2? Thanks! > > > + > > +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. 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 > -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
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@gmail.com> 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@gmail.com> > > > --- > > > .../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@gmail.com> > > > + > > > +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
On Fri, Aug 18, 2023 at 9:48 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > 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@gmail.com> 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@gmail.com> > > > > --- > > > > .../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@gmail.com> > > > > + > > > > +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. > Will do. > > 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. > OK. I will split this file into loongson,ls1b-gmac.yaml and loongson,ls1c-mac.yaml. Thanks for your review! > -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 -- Best regards, Keguang Zhang
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 + +maintainers: + - Keguang Zhang <keguang.zhang@gmail.com> + +select: + properties: + compatible: + contains: + enum: + - loongson,ls1b-dwmac + - loongson,ls1c-dwmac + required: + - compatible + +properties: + compatible: + items: + - enum: + - loongson,ls1b-dwmac + - loongson,ls1c-dwmac + - 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. + +required: + - compatible + - reg + - clocks + - clock-names + - interrupts + - interrupt-names + - phy-handle + - phy-mode + - 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
Add devicetree binding document for Loongson-1 DWMAC glue layer. Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> --- .../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