| Message ID | 20230827091710.1483-3-jszhang@kernel.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | add the dwmac driver for T-HEAD TH1520 SoC | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 8 warnings, 84 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Sun, Aug 27, 2023 at 05:17:09PM +0800, Jisheng Zhang wrote: > Add documentation to describe T-HEAD dwmac. > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > --- > .../devicetree/bindings/net/snps,dwmac.yaml | 1 + > .../devicetree/bindings/net/thead,dwmac.yaml | 77 +++++++++++++++++++ > 2 files changed, 78 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/thead,dwmac.yaml > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > index b196c5de2061..73821f86a609 100644 > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > @@ -96,6 +96,7 @@ properties: > - snps,dwxgmac > - snps,dwxgmac-2.10 > - starfive,jh7110-dwmac > + - thead,th1520-dwmac > > reg: > minItems: 1 > diff --git a/Documentation/devicetree/bindings/net/thead,dwmac.yaml b/Documentation/devicetree/bindings/net/thead,dwmac.yaml > new file mode 100644 > index 000000000000..bf8ec8ca2753 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/thead,dwmac.yaml see further regarding using dwmac in the names here. > @@ -0,0 +1,77 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/thead,dwmac.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: T-HEAD DWMAC Ethernet controller Additionally would be nice to have a brief controller "description:" having the next info: the SoCs the controllers can be found on, the DW (G)MAC IP-core version the ethernet controller is based on and 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, VLAN hash filter, SA/VLAN-tag insertion, ARP offload engine, PHY interfaces (MII, RMII, RGMII, etc), EEE support, IEEE 1588(-2008) Timestamping support, PMT and Wake-up frame support, MAC Management counters (MMC). In addition to that for DW QoS ETH/XGMAC/XLGMAC the next info would be useful: number of MTL Queues and DMA channels, MTL queues capabilities (QoS-related), TSO availability, SPO availability. 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: > + - Jisheng Zhang <jszhang@kernel.org> > + > +select: > + properties: > + compatible: > + contains: > + enum: > + - thead,th1520-dwmac Referring to the DW IP-core in the compatible string isn't very much useful especially seeing you have a generic fallback compatible. Name like "thead,th1520-gmac" looks more informative indicating its speed capability. > + required: > + - compatible > + > +properties: > + compatible: > + items: > + - enum: > + - thead,th1520-dwmac ditto. -Serge(y) > + - const: snps,dwmac-3.70a > + > + reg: > + maxItems: 1 > + > + thead,gmacapb: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + The phandle to the syscon node that control ethernet > + interface and timing delay. > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - interrupts > + - interrupt-names > + - phy-mode > + - thead,gmacapb > + > +allOf: > + - $ref: snps,dwmac.yaml# > + > +unevaluatedProperties: false > + > +examples: > + - | > + gmac0: ethernet@e7070000 { > + compatible = "thead,th1520-dwmac", "snps,dwmac-3.70a"; > + reg = <0xe7070000 0x2000>; > + clocks = <&clk 1>, <&clk 2>; > + clock-names = "stmmaceth", "pclk"; > + interrupts = <66>; > + interrupt-names = "macirq"; > + phy-mode = "rgmii-id"; > + snps,fixed-burst; > + snps,axi-config = <&stmmac_axi_setup>; > + snps,pbl = <32>; > + thead,gmacapb = <&gmacapb_syscon>; > + phy-handle = <&phy0>; > + > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "snps,dwmac-mdio"; > + > + phy0: ethernet-phy@0 { > + reg = <0>; > + }; > + }; > + }; > -- > 2.40.1 > >
On Sun, Aug 27, 2023 at 05:17:09PM +0800, Jisheng Zhang wrote: > Add documentation to describe T-HEAD dwmac. > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > --- > .../devicetree/bindings/net/snps,dwmac.yaml | 1 + > .../devicetree/bindings/net/thead,dwmac.yaml | 77 +++++++++++++++++++ > 2 files changed, 78 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/thead,dwmac.yaml > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > index b196c5de2061..73821f86a609 100644 > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > @@ -96,6 +96,7 @@ properties: > - snps,dwxgmac > - snps,dwxgmac-2.10 > - starfive,jh7110-dwmac > + - thead,th1520-dwmac > > reg: > minItems: 1 > diff --git a/Documentation/devicetree/bindings/net/thead,dwmac.yaml b/Documentation/devicetree/bindings/net/thead,dwmac.yaml > new file mode 100644 > index 000000000000..bf8ec8ca2753 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/thead,dwmac.yaml > @@ -0,0 +1,77 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/thead,dwmac.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: T-HEAD DWMAC Ethernet controller > + > +maintainers: > + - Jisheng Zhang <jszhang@kernel.org> > + > +select: > + properties: > + compatible: > + contains: > + enum: > + - thead,th1520-dwmac > + required: > + - compatible > + > +properties: > + compatible: > + items: > + - enum: > + - thead,th1520-dwmac > + - const: snps,dwmac-3.70a > + > + reg: > + maxItems: 1 > + > + thead,gmacapb: BTW what is a point in having the "apb" prefix in the name? The property name like "thead,gmac-syscon" looks much more suitable since it refers to the actual property content. -Serge(y) > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + The phandle to the syscon node that control ethernet > + interface and timing delay. > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - interrupts > + - interrupt-names > + - phy-mode > + - thead,gmacapb > + > +allOf: > + - $ref: snps,dwmac.yaml# > + > +unevaluatedProperties: false > + > +examples: > + - | > + gmac0: ethernet@e7070000 { > + compatible = "thead,th1520-dwmac", "snps,dwmac-3.70a"; > + reg = <0xe7070000 0x2000>; > + clocks = <&clk 1>, <&clk 2>; > + clock-names = "stmmaceth", "pclk"; > + interrupts = <66>; > + interrupt-names = "macirq"; > + phy-mode = "rgmii-id"; > + snps,fixed-burst; > + snps,axi-config = <&stmmac_axi_setup>; > + snps,pbl = <32>; > + thead,gmacapb = <&gmacapb_syscon>; > + phy-handle = <&phy0>; > + > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "snps,dwmac-mdio"; > + > + phy0: ethernet-phy@0 { > + reg = <0>; > + }; > + }; > + }; > -- > 2.40.1 > >
On Mon, Aug 28, 2023 at 04:13:00PM +0300, Serge Semin wrote: > On Sun, Aug 27, 2023 at 05:17:09PM +0800, Jisheng Zhang wrote: > > Add documentation to describe T-HEAD dwmac. > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > --- > > .../devicetree/bindings/net/snps,dwmac.yaml | 1 + > > .../devicetree/bindings/net/thead,dwmac.yaml | 77 +++++++++++++++++++ > > 2 files changed, 78 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/net/thead,dwmac.yaml > > > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > index b196c5de2061..73821f86a609 100644 > > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > @@ -96,6 +96,7 @@ properties: > > - snps,dwxgmac > > - snps,dwxgmac-2.10 > > - starfive,jh7110-dwmac > > + - thead,th1520-dwmac > > > > reg: > > minItems: 1 > > diff --git a/Documentation/devicetree/bindings/net/thead,dwmac.yaml b/Documentation/devicetree/bindings/net/thead,dwmac.yaml > > new file mode 100644 > > index 000000000000..bf8ec8ca2753 > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/net/thead,dwmac.yaml > > see further regarding using dwmac in the names here. > > > @@ -0,0 +1,77 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/net/thead,dwmac.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > > +title: T-HEAD DWMAC Ethernet controller > > Additionally would be nice to have a brief controller "description:" > having the next info: the SoCs the controllers can be found on, the DW > (G)MAC IP-core version the ethernet controller is based on and 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, VLAN hash filter, SA/VLAN-tag insertion, ARP offload > engine, PHY interfaces (MII, RMII, RGMII, etc), EEE support, IEEE > 1588(-2008) Timestamping support, PMT and Wake-up frame support, MAC > Management counters (MMC). In addition to that for DW QoS > ETH/XGMAC/XLGMAC the next info would be useful: number of MTL Queues > and DMA channels, MTL queues capabilities (QoS-related), TSO > availability, SPO availability. > > 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". Hi Serge, Thank you for your code review. I have different views here: If we only support the gmac controller in one specific SoC, these detailed information is nice to have, but what about if the driver/dt-binding supports the gmac controller in different SoCs? These detailed information will be outdated. what's more, I think the purpose of dt-binding is different from the one of documentation. So I prefer to put these GMAC IP related detailed information into the SoC's dtsi commit msg rather than polluting the dt-binding. > > > + > > +maintainers: > > + - Jisheng Zhang <jszhang@kernel.org> > > + > > +select: > > + properties: > > + compatible: > > + contains: > > + enum: > > > + - thead,th1520-dwmac > > Referring to the DW IP-core in the compatible string isn't very > much useful especially seeing you have a generic fallback compatible. > Name like "thead,th1520-gmac" looks more informative indicating its > speed capability. This is just to follow the common style as those dwmac-* does. I'm not sure which is better, but personally, I'd like to keep current common style.
On Mon, Aug 28, 2023 at 11:17:36PM +0800, Jisheng Zhang wrote: > On Mon, Aug 28, 2023 at 04:13:00PM +0300, Serge Semin wrote: > > On Sun, Aug 27, 2023 at 05:17:09PM +0800, Jisheng Zhang wrote: > > > Add documentation to describe T-HEAD dwmac. > > > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > > --- > > > .../devicetree/bindings/net/snps,dwmac.yaml | 1 + > > > .../devicetree/bindings/net/thead,dwmac.yaml | 77 +++++++++++++++++++ > > > 2 files changed, 78 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/net/thead,dwmac.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > index b196c5de2061..73821f86a609 100644 > > > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > @@ -96,6 +96,7 @@ properties: > > > - snps,dwxgmac > > > - snps,dwxgmac-2.10 > > > - starfive,jh7110-dwmac > > > + - thead,th1520-dwmac > > > > > > reg: > > > minItems: 1 > > > diff --git a/Documentation/devicetree/bindings/net/thead,dwmac.yaml b/Documentation/devicetree/bindings/net/thead,dwmac.yaml > > > new file mode 100644 > > > index 000000000000..bf8ec8ca2753 > > > --- /dev/null > > > > > +++ b/Documentation/devicetree/bindings/net/thead,dwmac.yaml > > > > see further regarding using dwmac in the names here. > > > > > @@ -0,0 +1,77 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/net/thead,dwmac.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > > > +title: T-HEAD DWMAC Ethernet controller > > > > Additionally would be nice to have a brief controller "description:" > > having the next info: the SoCs the controllers can be found on, the DW > > (G)MAC IP-core version the ethernet controller is based on and 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, VLAN hash filter, SA/VLAN-tag insertion, ARP offload > > engine, PHY interfaces (MII, RMII, RGMII, etc), EEE support, IEEE > > 1588(-2008) Timestamping support, PMT and Wake-up frame support, MAC > > Management counters (MMC). In addition to that for DW QoS > > ETH/XGMAC/XLGMAC the next info would be useful: number of MTL Queues > > and DMA channels, MTL queues capabilities (QoS-related), TSO > > availability, SPO availability. > > > > 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". BTW plus to this you may wish to add the "rx-internal-delay-ps" and "tx-internal-delay-ps" properties constraints seeing they device supports internal Tx/Rx delays. > > Hi Serge, > > Thank you for your code review. I have different views here: If we > only support the gmac controller in one specific SoC, these detailed > information is nice to have, but what about if the driver/dt-binding > supports the gmac controller in different SoCs? These detailed > information will be outdated. First they won't. Second then you can either add more info to the description for instance in a separate paragraph or create a dedicated DT-bindings. Such information would be very much useful for the generic STMMAC driver code maintenance. > > what's more, I think the purpose of dt-binding is different from > the one of documentation. The purpose of the DT-bindings is a hardware "description". The info I listed describes your hardware. > > So I prefer to put these GMAC IP related detailed information into > the SoC's dtsi commit msg rather than polluting the dt-binding. > > > > > + > > > +maintainers: > > > + - Jisheng Zhang <jszhang@kernel.org> > > > + > > > +select: > > > + properties: > > > + compatible: > > > + contains: > > > + enum: > > > > > + - thead,th1520-dwmac > > > > Referring to the DW IP-core in the compatible string isn't very > > much useful especially seeing you have a generic fallback compatible. > > Name like "thead,th1520-gmac" looks more informative indicating its > > speed capability. > > This is just to follow the common style as those dwmac-* does. > I'm not sure which is better, but personally, I'd like to keep current > common style. It's not that common. Half the compatible strings use the notation suggested by me and it has more sense then a dwmac suffix. It's ok to use the suffix in the STMMAC driver-related things because the glue code is supposed to work with the DW *MAC generic code. Using it in the compatible string especially together with the generic fallback compatible just useless. -Serge(y)
On Mon, Aug 28, 2023 at 06:51:49PM +0300, Serge Semin wrote: > On Mon, Aug 28, 2023 at 11:17:36PM +0800, Jisheng Zhang wrote: > > On Mon, Aug 28, 2023 at 04:13:00PM +0300, Serge Semin wrote: > > > On Sun, Aug 27, 2023 at 05:17:09PM +0800, Jisheng Zhang wrote: > > > > Add documentation to describe T-HEAD dwmac. > > > > > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > > > --- > > > > .../devicetree/bindings/net/snps,dwmac.yaml | 1 + > > > > .../devicetree/bindings/net/thead,dwmac.yaml | 77 +++++++++++++++++++ > > > > 2 files changed, 78 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/net/thead,dwmac.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > > index b196c5de2061..73821f86a609 100644 > > > > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > > @@ -96,6 +96,7 @@ properties: > > > > - snps,dwxgmac > > > > - snps,dwxgmac-2.10 > > > > - starfive,jh7110-dwmac > > > > + - thead,th1520-dwmac > > > > > > > > reg: > > > > minItems: 1 > > > > diff --git a/Documentation/devicetree/bindings/net/thead,dwmac.yaml b/Documentation/devicetree/bindings/net/thead,dwmac.yaml > > > > new file mode 100644 > > > > index 000000000000..bf8ec8ca2753 > > > > --- /dev/null > > > > > > > +++ b/Documentation/devicetree/bindings/net/thead,dwmac.yaml > > > > > > see further regarding using dwmac in the names here. > > > > > > > @@ -0,0 +1,77 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/net/thead,dwmac.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > > > > +title: T-HEAD DWMAC Ethernet controller > > > > > > Additionally would be nice to have a brief controller "description:" > > > having the next info: the SoCs the controllers can be found on, the DW > > > (G)MAC IP-core version the ethernet controller is based on and 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, VLAN hash filter, SA/VLAN-tag insertion, ARP offload > > > engine, PHY interfaces (MII, RMII, RGMII, etc), EEE support, IEEE > > > 1588(-2008) Timestamping support, PMT and Wake-up frame support, MAC > > > Management counters (MMC). In addition to that for DW QoS > > > ETH/XGMAC/XLGMAC the next info would be useful: number of MTL Queues > > > and DMA channels, MTL queues capabilities (QoS-related), TSO > > > availability, SPO availability. > > > > > > > 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". > > BTW plus to this you may wish to add the "rx-internal-delay-ps" and > "tx-internal-delay-ps" properties constraints seeing they device > supports internal Tx/Rx delays. > > > > > Hi Serge, > > > > > Thank you for your code review. I have different views here: If we > > only support the gmac controller in one specific SoC, these detailed > > information is nice to have, but what about if the driver/dt-binding > > supports the gmac controller in different SoCs? These detailed > > information will be outdated. > > First they won't. Second then you can either add more info to the > description for instance in a separate paragraph or create a dedicated > DT-bindings. Such information would be very much useful for the > generic STMMAC driver code maintenance. > > > > > what's more, I think the purpose of dt-binding is different from > > the one of documentation. > > The purpose of the DT-bindings is a hardware "description". The info I > listed describes your hardware. dt-binding VS. dts(i), they are different things. Part of what you listed belong dts(i), that's the reason why I prefer to put those into dts(i) commit msg. The HW description is in dts(i) itself rather than dt-binding. Anyway I will add generic decriptions to the dt-binding. > > > > > So I prefer to put these GMAC IP related detailed information into > > the SoC's dtsi commit msg rather than polluting the dt-binding. > > > > > > > + > > > > +maintainers: > > > > + - Jisheng Zhang <jszhang@kernel.org> > > > > + > > > > +select: > > > > + properties: > > > > + compatible: > > > > + contains: > > > > + enum: > > > > > > > + - thead,th1520-dwmac > > > > > > Referring to the DW IP-core in the compatible string isn't very > > > much useful especially seeing you have a generic fallback compatible. > > > Name like "thead,th1520-gmac" looks more informative indicating its > > > speed capability. > > > > > This is just to follow the common style as those dwmac-* does. > > I'm not sure which is better, but personally, I'd like to keep current > > common style. > > It's not that common. Half the compatible strings use the notation > suggested by me and it has more sense then a dwmac suffix. It's ok to > use the suffix in the STMMAC driver-related things because the glue > code is supposed to work with the DW *MAC generic code. Using it in > the compatible string especially together with the generic fallback > compatible just useless. > > -Serge(y) >
On 28/08/2023 17:51, Serge Semin wrote: > On Mon, Aug 28, 2023 at 11:17:36PM +0800, Jisheng Zhang wrote: >> On Mon, Aug 28, 2023 at 04:13:00PM +0300, Serge Semin wrote: >>> On Sun, Aug 27, 2023 at 05:17:09PM +0800, Jisheng Zhang wrote: >>>> Add documentation to describe T-HEAD dwmac. >>>> >>>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org> >>>> --- >>>> .../devicetree/bindings/net/snps,dwmac.yaml | 1 + >>>> .../devicetree/bindings/net/thead,dwmac.yaml | 77 +++++++++++++++++++ >>>> 2 files changed, 78 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/net/thead,dwmac.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>> index b196c5de2061..73821f86a609 100644 >>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>> @@ -96,6 +96,7 @@ properties: >>>> - snps,dwxgmac >>>> - snps,dwxgmac-2.10 >>>> - starfive,jh7110-dwmac >>>> + - thead,th1520-dwmac >>>> >>>> reg: >>>> minItems: 1 >>>> diff --git a/Documentation/devicetree/bindings/net/thead,dwmac.yaml b/Documentation/devicetree/bindings/net/thead,dwmac.yaml >>>> new file mode 100644 >>>> index 000000000000..bf8ec8ca2753 >>>> --- /dev/null >>> >>>> +++ b/Documentation/devicetree/bindings/net/thead,dwmac.yaml >>> >>> see further regarding using dwmac in the names here. >>> >>>> @@ -0,0 +1,77 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/net/thead,dwmac.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>> >>>> +title: T-HEAD DWMAC Ethernet controller >>> >>> Additionally would be nice to have a brief controller "description:" >>> having the next info: the SoCs the controllers can be found on, the DW >>> (G)MAC IP-core version the ethernet controller is based on and 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, VLAN hash filter, SA/VLAN-tag insertion, ARP offload >>> engine, PHY interfaces (MII, RMII, RGMII, etc), EEE support, IEEE >>> 1588(-2008) Timestamping support, PMT and Wake-up frame support, MAC >>> Management counters (MMC). In addition to that for DW QoS >>> ETH/XGMAC/XLGMAC the next info would be useful: number of MTL Queues >>> and DMA channels, MTL queues capabilities (QoS-related), TSO >>> availability, SPO availability. >>> > >>> 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". > > BTW plus to this you may wish to add the "rx-internal-delay-ps" and > "tx-internal-delay-ps" properties constraints seeing they device > supports internal Tx/Rx delays. > >> >> Hi Serge, >> > >> Thank you for your code review. I have different views here: If we >> only support the gmac controller in one specific SoC, these detailed >> information is nice to have, but what about if the driver/dt-binding >> supports the gmac controller in different SoCs? These detailed >> information will be outdated. > > First they won't. Second then you can either add more info to the > description for instance in a separate paragraph or create a dedicated > DT-bindings. Such information would be very much useful for the > generic STMMAC driver code maintenance. > >> >> what's more, I think the purpose of dt-binding is different from >> the one of documentation. > > The purpose of the DT-bindings is a hardware "description". The info I > listed describes your hardware. > >> >> So I prefer to put these GMAC IP related detailed information into >> the SoC's dtsi commit msg rather than polluting the dt-binding. >>> >>>> + >>>> +maintainers: >>>> + - Jisheng Zhang <jszhang@kernel.org> >>>> + >>>> +select: >>>> + properties: >>>> + compatible: >>>> + contains: >>>> + enum: >>> >>>> + - thead,th1520-dwmac >>> >>> Referring to the DW IP-core in the compatible string isn't very >>> much useful especially seeing you have a generic fallback compatible. >>> Name like "thead,th1520-gmac" looks more informative indicating its >>> speed capability. >> > >> This is just to follow the common style as those dwmac-* does. >> I'm not sure which is better, but personally, I'd like to keep current >> common style. > > It's not that common. Half the compatible strings use the notation > suggested by me and it has more sense then a dwmac suffix. It's ok to > use the suffix in the STMMAC driver-related things because the glue > code is supposed to work with the DW *MAC generic code. Using it in > the compatible string especially together with the generic fallback > compatible just useless. THEAD did not make dwmac here, but a gmac. dwmac does not exist in the context of Thead and Th1520, so the naming suggested by Serge makes sense. Best regards, Krzysztof
On Mon, Aug 28, 2023 at 07:55:44PM +0200, Krzysztof Kozlowski wrote: > On 28/08/2023 17:51, Serge Semin wrote: > > On Mon, Aug 28, 2023 at 11:17:36PM +0800, Jisheng Zhang wrote: > >> On Mon, Aug 28, 2023 at 04:13:00PM +0300, Serge Semin wrote: > >>> On Sun, Aug 27, 2023 at 05:17:09PM +0800, Jisheng Zhang wrote: > >>>> Add documentation to describe T-HEAD dwmac. > >>>> > >>>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > >>>> --- > >>>> .../devicetree/bindings/net/snps,dwmac.yaml | 1 + > >>>> .../devicetree/bindings/net/thead,dwmac.yaml | 77 +++++++++++++++++++ > >>>> 2 files changed, 78 insertions(+) > >>>> create mode 100644 Documentation/devicetree/bindings/net/thead,dwmac.yaml > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > >>>> index b196c5de2061..73821f86a609 100644 > >>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > >>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > >>>> @@ -96,6 +96,7 @@ properties: > >>>> - snps,dwxgmac > >>>> - snps,dwxgmac-2.10 > >>>> - starfive,jh7110-dwmac > >>>> + - thead,th1520-dwmac > >>>> > >>>> reg: > >>>> minItems: 1 > >>>> diff --git a/Documentation/devicetree/bindings/net/thead,dwmac.yaml b/Documentation/devicetree/bindings/net/thead,dwmac.yaml > >>>> new file mode 100644 > >>>> index 000000000000..bf8ec8ca2753 > >>>> --- /dev/null > >>> > >>>> +++ b/Documentation/devicetree/bindings/net/thead,dwmac.yaml > >>> > >>> see further regarding using dwmac in the names here. > >>> > >>>> @@ -0,0 +1,77 @@ > >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>>> +%YAML 1.2 > >>>> +--- > >>>> +$id: http://devicetree.org/schemas/net/thead,dwmac.yaml# > >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>>> + > >>> > >>>> +title: T-HEAD DWMAC Ethernet controller > >>> > >>> Additionally would be nice to have a brief controller "description:" > >>> having the next info: the SoCs the controllers can be found on, the DW > >>> (G)MAC IP-core version the ethernet controller is based on and 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, VLAN hash filter, SA/VLAN-tag insertion, ARP offload > >>> engine, PHY interfaces (MII, RMII, RGMII, etc), EEE support, IEEE > >>> 1588(-2008) Timestamping support, PMT and Wake-up frame support, MAC > >>> Management counters (MMC). In addition to that for DW QoS > >>> ETH/XGMAC/XLGMAC the next info would be useful: number of MTL Queues > >>> and DMA channels, MTL queues capabilities (QoS-related), TSO > >>> availability, SPO availability. > >>> > > > >>> 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". > > > > BTW plus to this you may wish to add the "rx-internal-delay-ps" and > > "tx-internal-delay-ps" properties constraints seeing they device > > supports internal Tx/Rx delays. > > > >> > >> Hi Serge, > >> > > > >> Thank you for your code review. I have different views here: If we > >> only support the gmac controller in one specific SoC, these detailed > >> information is nice to have, but what about if the driver/dt-binding > >> supports the gmac controller in different SoCs? These detailed > >> information will be outdated. > > > > First they won't. Second then you can either add more info to the > > description for instance in a separate paragraph or create a dedicated > > DT-bindings. Such information would be very much useful for the > > generic STMMAC driver code maintenance. > > > >> > >> what's more, I think the purpose of dt-binding is different from > >> the one of documentation. > > > > The purpose of the DT-bindings is a hardware "description". The info I > > listed describes your hardware. > > > >> > >> So I prefer to put these GMAC IP related detailed information into > >> the SoC's dtsi commit msg rather than polluting the dt-binding. > >>> > >>>> + > >>>> +maintainers: > >>>> + - Jisheng Zhang <jszhang@kernel.org> > >>>> + > >>>> +select: > >>>> + properties: > >>>> + compatible: > >>>> + contains: > >>>> + enum: > >>> > >>>> + - thead,th1520-dwmac > >>> > >>> Referring to the DW IP-core in the compatible string isn't very > >>> much useful especially seeing you have a generic fallback compatible. > >>> Name like "thead,th1520-gmac" looks more informative indicating its > >>> speed capability. > >> > > > >> This is just to follow the common style as those dwmac-* does. > >> I'm not sure which is better, but personally, I'd like to keep current > >> common style. > > > > It's not that common. Half the compatible strings use the notation > > suggested by me and it has more sense then a dwmac suffix. It's ok to > > use the suffix in the STMMAC driver-related things because the glue > > code is supposed to work with the DW *MAC generic code. Using it in > > the compatible string especially together with the generic fallback > > compatible just useless. > > THEAD did not make dwmac here, but a gmac. dwmac does not exist in the > context of Thead and Th1520, so the naming suggested by Serge makes sense. > I have no preference. But just want to confirm: the th1520 ethernet controller doesn't always function as GMAC, but can act as MII, so "thead,th1520-gmac" is still OK?
diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml index b196c5de2061..73821f86a609 100644 --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml @@ -96,6 +96,7 @@ properties: - snps,dwxgmac - snps,dwxgmac-2.10 - starfive,jh7110-dwmac + - thead,th1520-dwmac reg: minItems: 1 diff --git a/Documentation/devicetree/bindings/net/thead,dwmac.yaml b/Documentation/devicetree/bindings/net/thead,dwmac.yaml new file mode 100644 index 000000000000..bf8ec8ca2753 --- /dev/null +++ b/Documentation/devicetree/bindings/net/thead,dwmac.yaml @@ -0,0 +1,77 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/thead,dwmac.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: T-HEAD DWMAC Ethernet controller + +maintainers: + - Jisheng Zhang <jszhang@kernel.org> + +select: + properties: + compatible: + contains: + enum: + - thead,th1520-dwmac + required: + - compatible + +properties: + compatible: + items: + - enum: + - thead,th1520-dwmac + - const: snps,dwmac-3.70a + + reg: + maxItems: 1 + + thead,gmacapb: + $ref: /schemas/types.yaml#/definitions/phandle + description: + The phandle to the syscon node that control ethernet + interface and timing delay. + +required: + - compatible + - reg + - clocks + - clock-names + - interrupts + - interrupt-names + - phy-mode + - thead,gmacapb + +allOf: + - $ref: snps,dwmac.yaml# + +unevaluatedProperties: false + +examples: + - | + gmac0: ethernet@e7070000 { + compatible = "thead,th1520-dwmac", "snps,dwmac-3.70a"; + reg = <0xe7070000 0x2000>; + clocks = <&clk 1>, <&clk 2>; + clock-names = "stmmaceth", "pclk"; + interrupts = <66>; + interrupt-names = "macirq"; + phy-mode = "rgmii-id"; + snps,fixed-burst; + snps,axi-config = <&stmmac_axi_setup>; + snps,pbl = <32>; + thead,gmacapb = <&gmacapb_syscon>; + phy-handle = <&phy0>; + + mdio { + #address-cells = <1>; + #size-cells = <0>; + compatible = "snps,dwmac-mdio"; + + phy0: ethernet-phy@0 { + reg = <0>; + }; + }; + };
Add documentation to describe T-HEAD dwmac. Signed-off-by: Jisheng Zhang <jszhang@kernel.org> --- .../devicetree/bindings/net/snps,dwmac.yaml | 1 + .../devicetree/bindings/net/thead,dwmac.yaml | 77 +++++++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/thead,dwmac.yaml