Re: [PATCH net-next v1 5/9] dt-bindings: net: Add Tegra234 MGBE

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5254 bytes --]

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@nvidia.com>
> > Signed-off-by: Bhadram Varka <vbhadram@nvidia.com>
> > ---
> >  .../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@nvidia.com>
> > +  - Jon Hunter <jonathanh@nvidia.com>
> > +
> > +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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]