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

[Bhadram Varka <vbhadram@nvidia.com>]

Hi @Rob Herring,
Thanks for the review.

> -----Original Message-----
> From: Thierry Reding <thierry.reding@gmail.com>
> Sent: 30 June 2022 08:25 PM
> To: Rob Herring <robh@kernel.org>; Bhadram Varka
> <vbhadram@nvidia.com>
> Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
> tegra@vger.kernel.org; krzysztof.kozlowski+dt@linaro.org; Jonathan Hunter
> <jonathanh@nvidia.com>; 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:
> > 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.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@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.
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