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.yaml > > @@ -0,0 +1,163 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > Dual license. checkpatch.pl will tell you this. > > > +%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/ > > > + > > +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. > > > + > > + 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. 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
Attachment:
signature.asc
Description: PGP signature
