diff mbox series

[net-next,v2,2/3] dt-bindings: net: add T-HEAD dwmac support

Message ID 20230827091710.1483-3-jszhang@kernel.org (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series add the dwmac driver for T-HEAD TH1520 SoC | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/cc_maintainers warning 3 maintainers not CCed: palmer@dabbelt.com paul.walmsley@sifive.com aou@eecs.berkeley.edu
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 9 this patch: 9
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jisheng Zhang Aug. 27, 2023, 9:17 a.m. UTC
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

Comments

Serge Semin Aug. 28, 2023, 1:13 p.m. UTC | #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

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
> 
>
Serge Semin Aug. 28, 2023, 1:16 p.m. UTC | #2
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
> 
>
Jisheng Zhang Aug. 28, 2023, 3:17 p.m. UTC | #3
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.
Serge Semin Aug. 28, 2023, 3:51 p.m. UTC | #4
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)
Jisheng Zhang Aug. 28, 2023, 4:06 p.m. UTC | #5
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)
>
Krzysztof Kozlowski Aug. 28, 2023, 5:55 p.m. UTC | #6
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
Jisheng Zhang Aug. 29, 2023, 3:02 a.m. UTC | #7
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 mbox series

Patch

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>;
+            };
+        };
+    };