Date: Tue, 21 Nov 2023 13:10:24 +0100
From: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	devicetree@...r.kernel.org, netdev@...r.kernel.org,
	linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: net: renesas,ethertsn: Add bindings for
 Ethernet TSN

Hi Krzysztof,

Thanks for your review.

On 2023-11-21 08:45:29 +0100, Krzysztof Kozlowski wrote:
> On 20/11/2023 17:07, Niklas Söderlund wrote:
> > Add bindings for Renesas R-Car Ethernet TSN End-station IP. The RTSN
> > device provides Ethernet network.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.
> 
> Why do you decide to skip some maintainers?

Sorry will do for v2.

> 
> > ---
> >  .../bindings/net/renesas,ethertsn.yaml        | 133 ++++++++++++++++++
> >  1 file changed, 133 insertions(+)
> 
> A nit, subject: drop second/last, redundant "bindings for". The
> "dt-bindings" prefix is already stating that these are bindings.

Ack.

> 
> >  create mode 100644 Documentation/devicetree/bindings/net/renesas,ethertsn.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml b/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml
> > new file mode 100644
> > index 000000000000..255c8f3a5a3b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/renesas,ethertsn.yaml
> > @@ -0,0 +1,133 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/renesas,ethertsn.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas Ethernet TSN End-station
> > +
> > +maintainers:
> > +  - Niklas Söderlund <niklas.soderlund@...natech.se>
> > +
> > +description:
> > +  The RTSN device provides Ethernet network using a 10 Mbps, 100 Mbps, or 1
> > +  Gbps full-duplex link via MII/GMII/RMII/RGMII. Depending on the connected PHY.
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> 
> Drop items.

Ack.

> 
> I assume you have oneOf above because you predict this will grow with
> entries with fallbacks? If not, drop.

As Geert points out I will add a fallback here and somewhen add support 
for R-Car S4 could be added.

> 
> > +          - enum:
> > +              - renesas,ethertsn-r8a779g0      # R-Car V4H
> > +
> > +  reg:
> > +    items:
> > +      - description: TSN End Station target
> > +      - description: generalized Precision Time Protocol target
> > +
> > +  reg-names:
> > +    items:
> > +      - const: tsnes
> > +      - const: gptp
> > +
> > +  interrupts:
> > +    items:
> > +      - description: TX data interrupt
> > +      - description: RX data interrupt
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: tx_data
> 
> tx
> 
> > +      - const: rx_data
> 
> rx

Ack.

> 
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  phy-mode:
> > +    contains:
> > +      enum:
> > +        - mii
> > +        - rgmii
> > +
> > +  phy-handle:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Specifies a reference to a node representing a PHY device.
> 
> You miss top-level $ref to ethernet controller

Sorry I don't fully understand what you are asking here, there are a few 
things about bindings I still need to learn. Looking at other bindings 
some have a

maintainers:
  ..

allOf:
  - $ref: ethernet-controller.yaml#

properties:
 ..

Is it this all ollOff node I'm missing?

> 
> > +
> > +  renesas,rx-internal-delay:
> > +    type: boolean
> > +    description:
> > +      Enable internal Rx clock delay, typically 1.8ns.
> 
> Why this is bool, not delay in ns?

The TSN is only capable of enabling or disable internal delays, not set 
how long the delay is. The documentation states that the delay depends 
on the electronic characteristics of the particular board, but states 
that they typically are 1.8ns for Rx and 2.0ns for Tx.

I looked at the generic properties {rx,tx}-internal-delay-ps but they 
are of int type. So I opted for a vendor specific bool property. Do you 
think a better route is to use the generic property and force the value 
to be either 0 or the typical delay?

> Why this is property of a board (not SoC)?

I'm sorry I don't understand this question.

> 
> > +
> > +  renesas,tx-internal-delay:
> > +    type: boolean
> > +    description:
> > +      Enable internal Tx clock delay, typically 2.0ns.
> 
> Same questions.
> 
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^ethernet-phy@[0-9a-f]$":
> > +    type: object
> > +    $ref: ethernet-phy.yaml#
> 
> Missing unevaluatedProperties. Open existing bindings and look how it is
> done there. Don't create something different.

Ack.

> 
> 
> 
> Best regards,
> Krzysztof
> 

-- 
Kind Regards,
Niklas Söderlund

Powered by blists - more mailing lists