Hi @Rob Herring,
Thanks for the review.

-----Original Message-----
From: Thierry Reding <redacted>
Sent: 30 June 2022 08:25 PM
To: Rob Herring <robh@kernel.org>; Bhadram Varka
[off-list ref]
Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
tegra@vger.kernel.org; krzysztof.kozlowski+dt@linaro.org; Jonathan Hunter
[off-list ref]; kuba@kernel.org; catalin.marinas@arm.com;
will@kernel.org
Subject: Re: [PATCH net-next v1 5/9] dt-bindings: net: Add Tegra234 MGBE

On Tue, Jun 28, 2022 at 01:55:34PM -0600, Rob Herring wrote:

quoted

On Thu, Jun 23, 2022 at 01:16:11PM +0530, Bhadram Varka wrote:

quoted

Add device-tree binding documentation for the Tegra234 MGBE ethernet
controller.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Bhadram Varka <redacted>
---
 .../bindings/net/nvidia,tegra234-mgbe.yaml    | 163

++++++++++++++++++

quoted

quoted

 1 file changed, 163 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/net/nvidia,tegra234-mgbe.yaml

diff --git
a/Documentation/devicetree/bindings/net/nvidia,tegra234-mgbe.yaml
b/Documentation/devicetree/bindings/net/nvidia,tegra234-mgbe.yaml
new file mode 100644
index 000000000000..d6db43e60ab8

--- /dev/null
+++ b/Documentation/devicetree/bindings/net/nvidia,tegra234-

mgbe.yam

quoted

quoted

+++ l

@@ -0,0 +1,163 @@
+# SPDX-License-Identifier: GPL-2.0

Dual license. checkpatch.pl will tell you this.

Addressed this comment.

quoted

quoted

+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/nvidia,tegra234-mgbe.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Tegra234 MGBE Device Tree Bindings

s/Device Tree Bindings/???bit Ethernet Controller/

Addressed this comment

quoted

quoted

+
+maintainers:
+  - Thierry Reding [off-list ref]
+  - Jon Hunter [off-list ref]
+
+properties:
+
+  compatible:
+    const: nvidia,tegra234-mgbe
+
+  reg:
+    minItems: 3
+    maxItems: 3
+
+  reg-names:
+    items:
+      - const: hypervisor
+      - const: mac
+      - const: xpcs

Is this really part of the same block? You don't have a PHY (the one
in front of the ethernet PHY) and PCS is sometimes part of the PHY.

Yes, these are all part of the same block. As an example, the MGBE0
instantiation of this block on Tegra234 is assigned an address space from
0x06800000 to 0x068fffff. Within that there are three main sections of
registers:

	MAC 0x06800000-0x0689ffff
	PCS 0x068a0000-0x068bffff
	SEC 0x068c0000-0x068effff

Each of these are further subdivided (hypervisor and mac are within that
MAC section, while XPCS is in the PCS section) and we don't have reg entries
for all of them because things like SEC and virtualization aren't supported
upstream yet.

quoted

quoted

+
+  interrupts:
+    minItems: 1
+
+  interrupt-names:
+    items:
+      - const: common

Just drop interrupt-names. Not a useful name really.

There will eventually be other interrupts that could be used here. For
example there are five additional interrupts that are used for virtualization
and another two for the MACSEC module. Neither of which are supported in
upstream at the moment, so we didn't want to define these yet. However,
specifying the interrupt-names property from the start, it will allow these
other interrupts to be added in a backwards- compatible and easy way later
on.

quoted

quoted

+
+  clocks:
+    minItems: 12
+    maxItems: 12
+
+  clock-names:
+    minItems: 12
+    maxItems: 12
+    contains:
+      enum:
+        - mgbe
+        - mac
+        - mac-divider
+        - ptp-ref
+        - rx-input-m
+        - rx-input
+        - tx
+        - eee-pcs
+        - rx-pcs-input
+        - rx-pcs-m
+        - rx-pcs
+        - tx-pcs
+
+  resets:
+    minItems: 2
+    maxItems: 2
+
+  reset-names:
+    contains:
+      enum:
+        - mac
+        - pcs
+
+  interconnects:
+    items:
+      - description: memory read client
+      - description: memory write client
+
+  interconnect-names:
+    items:
+      - const: dma-mem # read
+      - const: write
+
+  iommus:
+    maxItems: 1
+
+  power-domains:
+    items:
+      - description: MGBE power-domain

What else would it be? Just 'maxItems: 1'.

It's possible that we have some generic descriptions like this in other
bindings, but I agree that this doesn't add anything useful. I can look into
other bindings and remove these generic descriptions so that they don't set
a bad example.

quoted

quoted

+
+  phy-handle: true
+
+  phy-mode: true

All possible modes are supported by this h/w? Not likely.

Updated the comments to reflect usxgmii and xfi

quoted

quoted

+
+  mdio:
+    $ref: mdio.yaml#
+    unevaluatedProperties: false
+    description:
+      Creates and registers an MDIO bus.

That's OS behavior...

I suppose we can just leave out the description here because this is fairly
standard.

Bhadram, can you address the comments in this and send out a v2 of the
whole series? As suggested by Jakub, let's either leave out the driver bits at
this point so as not to confuse maintainers any further, or at least make sure
that the driver patch is the last one in the series to make it a bit more obvious
what needs to be applied to net/next.

Okay.

Also, keep in mind that if we want to get this into v5.20, we need to get the
bindings finalized in the next couple of days.

Thierry