Message-ID: <ae6132b93ac30a1f7b2721066a0e0eddc01745d5.camel@nvidia.com>
Date: Wed, 29 Jan 2025 07:30:55 +0000
From: Kartik Rajput <kkartik@...dia.com>
To: "krzk@...nel.org" <krzk@...nel.org>
CC: Jon Hunter <jonathanh@...dia.com>, "robh@...nel.org" <robh@...nel.org>,
	"robert.marko@...tura.hr" <robert.marko@...tura.hr>, "arnd@...nel.org"
	<arnd@...nel.org>, "thierry.reding@...il.com" <thierry.reding@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>, "geert+renesas@...der.be"
	<geert+renesas@...der.be>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "jirislaby@...nel.org" <jirislaby@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "hvilleneuve@...onoff.com"
	<hvilleneuve@...onoff.com>, "schnelle@...ux.ibm.com"
	<schnelle@...ux.ibm.com>, "gregkh@...uxfoundation.org"
	<gregkh@...uxfoundation.org>, "linux-serial@...r.kernel.org"
	<linux-serial@...r.kernel.org>, "andriy.shevchenko@...ux.intel.com"
	<andriy.shevchenko@...ux.intel.com>, "linux-tegra@...r.kernel.org"
	<linux-tegra@...r.kernel.org>
Subject: Re: [PATCH 1/2] dt-bindings: serial: Add bindings for
 nvidia,tegra264-utc

Thanks for reviewing the patch Krzysztof!

On Tue, 2025-01-28 at 08:52 +0100, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Jan 28, 2025 at 12:16:32PM +0530, Kartik Rajput wrote:
> > The Tegra UTC (UART Trace Controller) is a HW based serial port
> > that
> > allows multiplexing multiple data streams of up to 16 UTC clients
> > into
> > a single hardware serial port.
> > 
> > Add bindings for the Tegra UTC client device.
> > 
> > Signed-off-by: Kartik Rajput <kkartik@...dia.com>
> > ---
> >  .../bindings/serial/nvidia,tegra264-utc.yaml  | 83
> > +++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/serial/nvidia,tegra264-utc.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/serial/nvidia,tegra264-utc.yaml
> > b/Documentation/devicetree/bindings/serial/nvidia,tegra264-utc.yaml
> > new file mode 100644
> > index 000000000000..63ba3655451f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/serial/nvidia,tegra264-
> > utc.yaml
> > @@ -0,0 +1,83 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> > http://devicetree.org/schemas/serial/nvidia,tegra264-utc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NVIDIA Tegra UART Trace Controller (UTC) client
> 
> Controller and client (Client?) sound conflicting. What is this
> client
> of?
> 

This DT binding document is for the Tegra UTC client.

The Tegra UTC (UART Trace Controller) is a controller that allows
sharing a single physical UART across multiple firmwares/OS. It
supports up-to 16 client interfaces, each client have its own RX and TX
FIFO and an interrupt. The Tegra UTC controller is managed by the
bootloader and its clients are managed by firmwares/OS.

The firmwares/OS can use these client interfaces to send/receive data
over the UART by reading/writing from/to the client FIFOs. The Tegra
UTC multiplexes and de-multiplexes the data from/to each client FIFOs
and sends/receive that over a physical UART.

> > +
> > +maintainers:
> > +  - Kartik Rajput <kkartik@...dia.com>
> > +  - Thierry Reding <thierry.reding@...il.com>
> > +  - Jonathan Hunter <jonathanh@...dia.com>
> > +
> > +description:
> > +  The Tegra UTC (UART Trace Controller) is a hardware controller
> > that
> > +  allows multiple systems within the Tegra SoC to share a hardware
> > UART
> > +  interface. It supports up to 16 clients, with each client having
> > its own
> > +  interrupt and a FIFO buffer for both RX (receive) and TX
> > (transmit), each
> > +  capable of holding 128 characters.
> 
> So is this client or the controller?
> 

This is for the The Tegra UTC client interface. The Tegra UTC
controller is managed by the bootloader.

> > +
> > +  The Tegra UTC uses 8-N-1 configuration and operates on a pre-
> > configured
> > +  baudrate, which is configured by the bootloader.
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "^serial(@.*)?$"
> 
> Drop, not needed. But you miss proper $ref, see other bindings.
> 
> 

Ack. I'll fix this in v2.

> > +
> > +  compatible:
> > +    const: nvidia,tegra264-utc
> > +
> > +  reg:
> > +    items:
> > +      - description: Register region for TX client.
> 
> Drop redundant parts, so just "TX region".
> 

Ack. I'll fix this in v2.

> > +      - description: Register region for RX client.
> > +    minItems: 2
> 
> Drop
> 

Ack. I'll fix this in v2.

> > +
> > +  reg-names:
> > +    items:
> > +      - const: tx
> > +      - const: rx
> > +    minItems: 2
> 
> Drop. Please take a look at other bindings how they do things. There
> is
> no such code anywhere in the kernel.
> 

Ack. I'll fix this in v2.

> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  current-speed:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      This property specifies the baudrate at which the Tegra UTC
> > is
> 
> Drop "This property specifies the". Do not say what Devicetree syntax
> is. We all know. This is a description of hardware, not the DTS
> langauge.
> 

Ack. I'll fix this in v2.

> > +      operating.
> > +
> > +  nvidia,utc-fifo-threshold:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      This property specifies the UTC TX and RX client FIFO
> > threshold in
> > +      terms of occupancy.
> > +
> > +      This property should have the same value as the burst size
> > (number
> > +      of characters read by the Tegra UTC hardware at a time from
> > each
> > +      client) which is configured by the bootloader.
> 
> Title says this is a client, so quite confusing. Anyway, why is this
> board specific?

The client FIFO threshold should match the burst size configured in the
UTC controller by bootloader. This value could change depending on what
bootloader has programmed. Hence, this is moved to the device-tree.

> 
> Also, missing constraints, missing units. Why common serial
> properties
> are not applicable?
> 

I do see current-speed defined in serial-peripheral-props.yaml, that
can be used here. I also see "rx-threshold" and "tx-threshold"
properties defined in serial.yaml, maybe those can be utilized here. I
will update this in v2.

> Best regards,
> Krzysztof
> 

Thanks & Regards,
Kartik

Powered by blists - more mailing lists