Hi @Rob Herring, Thanks for the review. > -----Original Message----- > From: Thierry Reding <thierry.reding@xxxxxxxxx> > Sent: 30 June 2022 08:25 PM > To: Rob Herring <robh@xxxxxxxxxx>; Bhadram Varka > <vbhadram@xxxxxxxxxx> > Cc: netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- > tegra@xxxxxxxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; Jonathan Hunter > <jonathanh@xxxxxxxxxx>; kuba@xxxxxxxxxx; catalin.marinas@xxxxxxx; > will@xxxxxxxxxx > 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: > > On Thu, Jun 23, 2022 at 01:16:11PM +0530, Bhadram Varka wrote: > > > Add device-tree binding documentation for the Tegra234 MGBE ethernet > > > controller. > > > > > > Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx> > > > Signed-off-by: Bhadram Varka <vbhadram@xxxxxxxxxx> > > > --- > > > .../bindings/net/nvidia,tegra234-mgbe.yaml | 163 > ++++++++++++++++++ > > > 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 > > > +++ l > > > @@ -0,0 +1,163 @@ > > > +# SPDX-License-Identifier: GPL-2.0 > > > > Dual license. checkpatch.pl will tell you this. Addressed this comment. > > > > > +%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 > > > > > + > > > +maintainers: > > > + - Thierry Reding <treding@xxxxxxxxxx> > > > + - Jon Hunter <jonathanh@xxxxxxxxxx> > > > + > > > +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. > > > > + > > > + 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. > > > > > > + > > > + 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. > > > > + > > > + 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 > > > > > + > > > + 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
