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

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:

quoted

On Sun, Aug 27, 2023 at 05:17:09PM +0800, Jisheng Zhang wrote:

quoted

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

quoted

+++ b/Documentation/devicetree/bindings/net/thead,dwmac.yaml

see further regarding using dwmac in the names here.

quoted

@@ -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#
+

quoted

+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.

quoted

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.

quoted

quoted

+
+maintainers:
+  - Jisheng Zhang [off-list ref]
+
+select:
+  properties:
+    compatible:
+      contains:
+        enum:

quoted

+          - 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)