[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Thu, 7 Jul 2022 04:10:46 +0000
From: Bhadram Varka <vbhadram@...dia.com>
To: Thierry Reding <thierry.reding@...il.com>,
Rob Herring <robh@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>,
Jonathan Hunter <jonathanh@...dia.com>,
"kuba@...nel.org" <kuba@...nel.org>,
"catalin.marinas@....com" <catalin.marinas@....com>,
"will@...nel.org" <will@...nel.org>
Subject: RE: [PATCH net-next v1 5/9] dt-bindings: net: Add Tegra234 MGBE
Hi @Rob Herring,
Thanks for the review.
> -----Original Message-----
> From: Thierry Reding <thierry.reding@...il.com>
> Sent: 30 June 2022 08:25 PM
> To: Rob Herring <robh@...nel.org>; Bhadram Varka
> <vbhadram@...dia.com>
> Cc: netdev@...r.kernel.org; devicetree@...r.kernel.org; linux-
> tegra@...r.kernel.org; krzysztof.kozlowski+dt@...aro.org; Jonathan Hunter
> <jonathanh@...dia.com>; kuba@...nel.org; catalin.marinas@....com;
> will@...nel.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@...dia.com>
> > > Signed-off-by: Bhadram Varka <vbhadram@...dia.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@...dia.com>
> > > + - Jon Hunter <jonathanh@...dia.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
Powered by blists - more mailing lists