[PATCH 0/2] Add support for Tegra UART Trace Controller (UTC) client

[PATCH 0/2] Add support for Tegra UART Trace Controller (UTC) client

[Kartik Rajput]

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.

The Tegra UTC uses 8-N-1 configuration and operates on a pre-configured
baudrate, which is configured by the bootloader.

Kartik Rajput (2):
  dt-bindings: serial: Add bindings for nvidia,tegra264-utc
  serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

 .../bindings/serial/nvidia,tegra264-utc.yaml  |  83 +++
 drivers/tty/serial/Kconfig                    |  23 +
 drivers/tty/serial/Makefile                   |   1 +
 drivers/tty/serial/tegra-utc.c                | 641 ++++++++++++++++++
 4 files changed, 748 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/nvidia,tegra264-utc.yaml
 create mode 100644 drivers/tty/serial/tegra-utc.c

-- 
2.43.0

[PATCH 1/2] dt-bindings: serial: Add bindings for nvidia,tegra264-utc

[Kartik Rajput]

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@nvidia.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
+
+maintainers:
+  - Kartik Rajput <kkartik@nvidia.com>
+  - Thierry Reding <thierry.reding@gmail.com>
+  - Jonathan Hunter <jonathanh@nvidia.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.
+
+  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(@.*)?$"
+
+  compatible:
+    const: nvidia,tegra264-utc
+
+  reg:
+    items:
+      - description: Register region for TX client.
+      - description: Register region for RX client.
+    minItems: 2
+
+  reg-names:
+    items:
+      - const: tx
+      - const: rx
+    minItems: 2
+
+  interrupts:
+    maxItems: 1
+
+  current-speed:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      This property specifies the baudrate at which the Tegra UTC is
+      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.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - current-speed
+  - nvidia,utc-fifo-threshold
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    tegra_utc: serial@c4e0000 {
+        compatible = "nvidia,tegra264-utc";
+        reg = <0xc4e0000 0x8000>, <0xc4e8000 0x8000>;
+        reg-names = "tx", "rx";
+        interrupts = <GIC_SPI 514 IRQ_TYPE_LEVEL_HIGH>;
+        current-speed = <115200>;
+        nvidia,utc-fifo-threshold = <4>;
+    };
-- 
2.43.0

Re: [PATCH 1/2] dt-bindings: serial: Add bindings for nvidia,tegra264-utc

[Krzysztof Kozlowski]

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@nvidia.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?

> +
> +maintainers:
> +  - Kartik Rajput <kkartik@nvidia.com>
> +  - Thierry Reding <thierry.reding@gmail.com>
> +  - Jonathan Hunter <jonathanh@nvidia.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?

> +
> +  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.


> +
> +  compatible:
> +    const: nvidia,tegra264-utc
> +
> +  reg:
> +    items:
> +      - description: Register region for TX client.

Drop redundant parts, so just "TX region".

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

Drop

> +
> +  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.

> +
> +  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.

> +      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?

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

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: serial: Add bindings for nvidia,tegra264-utc

[Kartik Rajput]

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@nvidia.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@nvidia.com>
> > +  - Thierry Reding <thierry.reding@gmail.com>
> > +  - Jonathan Hunter <jonathanh@nvidia.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

Re: [PATCH 1/2] dt-bindings: serial: Add bindings for nvidia,tegra264-utc

[Thierry Reding]

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

On Wed, Jan 29, 2025 at 07:30:55AM +0000, Kartik Rajput wrote:
> 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:
[...]
> > > +  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.

I suppose "rx-threshold" and "tx-threshold" could be used instead of the
custom "nvidia,utc-fifo-threshold" property. It looks like the hardware
has separate values for the threshold in both directions, so this would
give us a more accurate description (though from the current state of
affairs it looks like both are always going to be the same).

I'm not so sure about "current-speed", though. There's no concept of
speed for the UTC, right? It's effectively backed by a physical UART
that will run at a certain speed, but given that it will multiplex data
from a variety of sources, "current-speed" will not be accurate in many
cases.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[Kartik Rajput]

The Tegra264 SoC supports the UART Trace Controller (UTC), which allows
multiple firmware clients (up to 16) to share a single physical UART.
Each client is provided with its own interrupt and has access to a
128-character wide FIFO for both transmit (TX) and receive (RX)
operations.

Add tegra-utc driver to support Tegra UART Trace Controller (UTC)
client.

Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
---
 drivers/tty/serial/Kconfig     |  23 ++
 drivers/tty/serial/Makefile    |   1 +
 drivers/tty/serial/tegra-utc.c | 641 +++++++++++++++++++++++++++++++++
 3 files changed, 665 insertions(+)
 create mode 100644 drivers/tty/serial/tegra-utc.c

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 976dae3bb1bb..edc56a3c0ace 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -306,6 +306,29 @@ config SERIAL_TEGRA_TCU_CONSOLE
 
 	  If unsure, say Y.
 
+config SERIAL_TEGRA_UTC
+	tristate "NVIDIA Tegra UART Trace Controller"
+	depends on ARCH_TEGRA || COMPILE_TEST
+	select SERIAL_CORE
+	help
+	  Support for Tegra UTC (UART Trace controller) client serial port.
+
+	  UTC is a HW based serial port that allows multiplexing multiple data
+	  streams of up to 16 UTC clients into a single hardware serial port.
+
+config SERIAL_TEGRA_UTC_CONSOLE
+	bool "Support for console on a Tegra UTC serial port"
+	depends on SERIAL_TEGRA_UTC
+	select SERIAL_CORE_CONSOLE
+	default SERIAL_TEGRA_UTC
+	help
+	  If you say Y here, it will be possible to use a Tegra UTC client as
+	  the system console (the system console is the device which receives
+	  all kernel messages and warnings and which allows logins in single
+	  user mode).
+
+	  If unsure, say Y.
+
 config SERIAL_MAX3100
 	tristate "MAX3100/3110/3111/3222 support"
 	depends on SPI
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index 6ff74f0a9530..7190914ba707 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -86,6 +86,7 @@ obj-$(CONFIG_SERIAL_STM32)		+= stm32-usart.o
 obj-$(CONFIG_SERIAL_SUNPLUS)		+= sunplus-uart.o
 obj-$(CONFIG_SERIAL_TEGRA)		+= serial-tegra.o
 obj-$(CONFIG_SERIAL_TEGRA_TCU)		+= tegra-tcu.o
+obj-$(CONFIG_SERIAL_TEGRA_UTC)		+= tegra-utc.o
 obj-$(CONFIG_SERIAL_TIMBERDALE)		+= timbuart.o
 obj-$(CONFIG_SERIAL_TXX9)		+= serial_txx9.o
 obj-$(CONFIG_SERIAL_UARTLITE)		+= uartlite.o
diff --git a/drivers/tty/serial/tegra-utc.c b/drivers/tty/serial/tegra-utc.c
new file mode 100644
index 000000000000..3d44e83f9302
--- /dev/null
+++ b/drivers/tty/serial/tegra-utc.c
@@ -0,0 +1,641 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+/*
+ * NVIDIA Tegra UTC (UART Trace Controller) driver.
+ */
+
+#include <linux/console.h>
+#include <linux/kthread.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/serial.h>
+#include <linux/serial_core.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+
+#define TEGRA_UTC_ENABLE			0x0
+#define TEGRA_UTC_ENABLE_CLIENT_ENABLE		BIT(0)
+
+#define TEGRA_UTC_FIFO_THRESHOLD		0x8
+
+#define TEGRA_UTC_COMMAND			0xc
+#define TEGRA_UTC_COMMAND_FLUSH			BIT(1)
+#define TEGRA_UTC_COMMAND_RESET			BIT(0)
+
+#define TEGRA_UTC_DATA				0x20
+
+#define TEGRA_UTC_FIFO_STATUS			0x100
+#define TEGRA_UTC_FIFO_TIMEOUT			BIT(4)
+#define TEGRA_UTC_FIFO_OVERFLOW			BIT(3)
+#define TEGRA_UTC_FIFO_REQ			BIT(2)
+#define TEGRA_UTC_FIFO_FULL			BIT(1)
+#define TEGRA_UTC_FIFO_EMPTY			BIT(0)
+
+#define TEGRA_UTC_FIFO_OCCUPANCY		0x104
+
+#define TEGRA_UTC_INTR_STATUS			0x108
+#define TEGRA_UTC_INTR_SET			0x10c
+#define TEGRA_UTC_INTR_MASK			0x110
+#define TEGRA_UTC_INTR_CLEAR			0x114
+#define TEGRA_UTC_INTR_TIMEOUT			BIT(4)
+#define TEGRA_UTC_INTR_OVERFLOW			BIT(3)
+#define TEGRA_UTC_INTR_REQ			BIT(2)
+#define TEGRA_UTC_INTR_FULL			BIT(1)
+#define TEGRA_UTC_INTR_EMPTY			BIT(0)
+
+#define UART_NR					16
+
+struct tegra_utc_soc {
+	unsigned int fifosize;
+};
+
+struct tegra_utc_port {
+	const struct tegra_utc_soc *soc;
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA_UTC_CONSOLE)
+	struct console console;
+#endif
+	struct uart_port port;
+
+	void __iomem *rx_base;
+	void __iomem *tx_base;
+
+	u32 tx_irqmask;
+	u32 rx_irqmask;
+
+	u32 fifo_threshold;
+	u32 baudrate;
+	int irq;
+};
+
+static u32 tegra_utc_rx_readl(struct tegra_utc_port *tup, unsigned int offset)
+{
+	void __iomem *addr = tup->rx_base + offset;
+
+	return readl_relaxed(addr);
+}
+
+static void tegra_utc_rx_writel(struct tegra_utc_port *tup, u32 val, unsigned int offset)
+{
+	void __iomem *addr = tup->rx_base + offset;
+
+	writel_relaxed(val, addr);
+}
+
+static u32 tegra_utc_tx_readl(struct tegra_utc_port *tup, unsigned int offset)
+{
+	void __iomem *addr = tup->tx_base + offset;
+
+	return readl_relaxed(addr);
+}
+
+static void tegra_utc_tx_writel(struct tegra_utc_port *tup, u32 val, unsigned int offset)
+{
+	void __iomem *addr = tup->tx_base + offset;
+
+	writel_relaxed(val, addr);
+}
+
+static void tegra_utc_enable_tx_irq(struct tegra_utc_port *tup)
+{
+	tup->tx_irqmask = TEGRA_UTC_INTR_REQ;
+
+	tegra_utc_tx_writel(tup, tup->tx_irqmask, TEGRA_UTC_INTR_MASK);
+	tegra_utc_tx_writel(tup, tup->tx_irqmask, TEGRA_UTC_INTR_SET);
+}
+
+static void tegra_utc_disable_tx_irq(struct tegra_utc_port *tup)
+{
+	tup->tx_irqmask = 0x0;
+
+	tegra_utc_tx_writel(tup, tup->tx_irqmask, TEGRA_UTC_INTR_MASK);
+	tegra_utc_tx_writel(tup, tup->tx_irqmask, TEGRA_UTC_INTR_SET);
+}
+
+static void tegra_utc_stop_tx(struct uart_port *port)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	tegra_utc_disable_tx_irq(tup);
+}
+
+static void tegra_utc_init_tx(struct tegra_utc_port *tup)
+{
+	/* Disable TX. */
+	tegra_utc_tx_writel(tup, 0x0, TEGRA_UTC_ENABLE);
+
+	/* Update the FIFO Threshold. */
+	tegra_utc_tx_writel(tup, tup->fifo_threshold, TEGRA_UTC_FIFO_THRESHOLD);
+
+	/* Clear and mask all the interrupts. */
+	tegra_utc_tx_writel(tup, TEGRA_UTC_INTR_REQ | TEGRA_UTC_INTR_FULL | TEGRA_UTC_INTR_EMPTY,
+			    TEGRA_UTC_INTR_CLEAR);
+	tegra_utc_disable_tx_irq(tup);
+
+	/* Enable TX. */
+	tegra_utc_tx_writel(tup, TEGRA_UTC_ENABLE_CLIENT_ENABLE, TEGRA_UTC_ENABLE);
+}
+
+static void tegra_utc_init_rx(struct tegra_utc_port *tup)
+{
+	tup->rx_irqmask = TEGRA_UTC_INTR_REQ | TEGRA_UTC_INTR_TIMEOUT;
+
+	tegra_utc_rx_writel(tup, TEGRA_UTC_COMMAND_RESET, TEGRA_UTC_COMMAND);
+	tegra_utc_rx_writel(tup, tup->fifo_threshold, TEGRA_UTC_FIFO_THRESHOLD);
+
+	/* Clear all the pending interrupts. */
+	tegra_utc_rx_writel(tup, TEGRA_UTC_INTR_TIMEOUT | TEGRA_UTC_INTR_OVERFLOW |
+			    TEGRA_UTC_INTR_REQ | TEGRA_UTC_INTR_FULL |
+			    TEGRA_UTC_INTR_EMPTY, TEGRA_UTC_INTR_CLEAR);
+	tegra_utc_rx_writel(tup, tup->rx_irqmask, TEGRA_UTC_INTR_MASK);
+	tegra_utc_rx_writel(tup, tup->rx_irqmask, TEGRA_UTC_INTR_SET);
+
+	/* Enable RX. */
+	tegra_utc_rx_writel(tup, TEGRA_UTC_ENABLE_CLIENT_ENABLE, TEGRA_UTC_ENABLE);
+}
+
+static bool tegra_utc_tx_char(struct tegra_utc_port *tup, u8 c)
+{
+	if (tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_STATUS) & TEGRA_UTC_FIFO_FULL)
+		return false;
+
+	tegra_utc_tx_writel(tup, c, TEGRA_UTC_DATA);
+
+	return true;
+}
+
+static bool tegra_utc_tx_chars(struct tegra_utc_port *tup)
+{
+	struct tty_port *tport = &tup->port.state->port;
+	u8 c;
+
+	if (tup->port.x_char) {
+		if (!tegra_utc_tx_char(tup, tup->port.x_char))
+			return true;
+
+		tup->port.x_char = 0;
+	}
+
+	if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(&tup->port)) {
+		tegra_utc_stop_tx(&tup->port);
+		return false;
+	}
+
+	while (kfifo_peek(&tport->xmit_fifo, &c)) {
+		if (!tegra_utc_tx_char(tup, c))
+			break;
+
+		kfifo_skip(&tport->xmit_fifo);
+	}
+
+	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
+		uart_write_wakeup(&tup->port);
+
+	if (kfifo_is_empty(&tport->xmit_fifo)) {
+		tegra_utc_stop_tx(&tup->port);
+		return false;
+	}
+
+	return true;
+}
+
+static void tegra_utc_rx_chars(struct tegra_utc_port *tup)
+{
+	struct tty_port *port = &tup->port.state->port;
+	unsigned int max_chars = 256;
+	unsigned int flag;
+	u32 status;
+	int sysrq;
+	u32 ch;
+
+	while (--max_chars) {
+		status = tegra_utc_rx_readl(tup, TEGRA_UTC_FIFO_STATUS);
+		if (status & TEGRA_UTC_FIFO_EMPTY)
+			break;
+
+		ch = tegra_utc_rx_readl(tup, TEGRA_UTC_DATA);
+		flag = TTY_NORMAL;
+		tup->port.icount.rx++;
+
+		if (status & TEGRA_UTC_FIFO_OVERFLOW)
+			tup->port.icount.overrun++;
+
+		uart_port_unlock(&tup->port);
+		sysrq = uart_handle_sysrq_char(&tup->port, ch & 0xff);
+		uart_port_lock(&tup->port);
+
+		if (!sysrq)
+			tty_insert_flip_char(port, ch, flag);
+	}
+
+	tty_flip_buffer_push(port);
+}
+
+static irqreturn_t tegra_utc_isr(int irq, void *dev_id)
+{
+	struct tegra_utc_port *tup = dev_id;
+	unsigned long flags;
+	u32 status;
+
+	uart_port_lock_irqsave(&tup->port, &flags);
+
+	/* Process RX_REQ and RX_TIMEOUT interrupts. */
+	do {
+		status = tegra_utc_rx_readl(tup, TEGRA_UTC_INTR_STATUS) & tup->rx_irqmask;
+		if (status) {
+			tegra_utc_rx_writel(tup, tup->rx_irqmask, TEGRA_UTC_INTR_CLEAR);
+			tegra_utc_rx_chars(tup);
+		}
+	} while (status);
+
+	/* Process TX_REQ interrupt. */
+	do {
+		status = tegra_utc_tx_readl(tup, TEGRA_UTC_INTR_STATUS) & tup->tx_irqmask;
+		if (status) {
+			tegra_utc_tx_writel(tup, tup->tx_irqmask, TEGRA_UTC_INTR_CLEAR);
+			tegra_utc_tx_chars(tup);
+		}
+	} while (status);
+
+	uart_port_unlock_irqrestore(&tup->port, flags);
+
+	return IRQ_HANDLED;
+}
+
+static unsigned int tegra_utc_tx_empty(struct uart_port *port)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	return tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_OCCUPANCY) ? 0 : TIOCSER_TEMT;
+}
+
+static void tegra_utc_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+}
+
+static unsigned int tegra_utc_get_mctrl(struct uart_port *port)
+{
+	return 0;
+}
+
+static void tegra_utc_start_tx(struct uart_port *port)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	if (tegra_utc_tx_chars(tup))
+		tegra_utc_enable_tx_irq(tup);
+}
+
+static void tegra_utc_stop_rx(struct uart_port *port)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	tup->rx_irqmask = 0x0;
+	tegra_utc_rx_writel(tup, tup->rx_irqmask, TEGRA_UTC_INTR_MASK);
+	tegra_utc_rx_writel(tup, tup->rx_irqmask, TEGRA_UTC_INTR_SET);
+}
+
+static void tegra_utc_hw_init(struct tegra_utc_port *tup)
+{
+	tegra_utc_init_tx(tup);
+	tegra_utc_init_rx(tup);
+}
+
+static int tegra_utc_startup(struct uart_port *port)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+	int ret;
+
+	tegra_utc_hw_init(tup);
+
+	ret = request_irq(tup->irq, tegra_utc_isr, 0, dev_name(port->dev), tup);
+	if (ret < 0) {
+		dev_err(port->dev, "failed to register interrupt handler\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void tegra_utc_shutdown(struct uart_port *port)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	tegra_utc_rx_writel(tup, 0x0, TEGRA_UTC_ENABLE);
+	free_irq(tup->irq, tup);
+}
+
+static void tegra_utc_set_termios(struct uart_port *port, struct ktermios *termios,
+				  const struct ktermios *old)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+	unsigned long flags;
+
+	tty_termios_encode_baud_rate(termios, tup->baudrate, tup->baudrate);
+	termios->c_cflag &= ~(CSIZE | CSTOPB | PARENB | PARODD);
+	termios->c_cflag &= ~(CMSPAR | CRTSCTS);
+	termios->c_cflag |= CS8 | CLOCAL;
+
+	uart_port_lock_irqsave(port, &flags);
+	uart_update_timeout(port, CS8, tup->baudrate);
+	uart_port_unlock_irqrestore(port, flags);
+}
+
+#ifdef CONFIG_CONSOLE_POLL
+
+static int tegra_utc_poll_init(struct uart_port *port)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	tegra_utc_hw_init(tup);
+	return 0;
+}
+
+static int tegra_utc_get_poll_char(struct uart_port *port)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	while (tegra_utc_rx_readl(tup, TEGRA_UTC_FIFO_STATUS) & TEGRA_UTC_FIFO_EMPTY)
+		cpu_relax();
+
+	return tegra_utc_rx_readl(tup, TEGRA_UTC_DATA);
+}
+
+static void tegra_utc_put_poll_char(struct uart_port *port, unsigned char ch)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	while (tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_STATUS) & TEGRA_UTC_FIFO_FULL)
+		cpu_relax();
+
+	tegra_utc_tx_writel(tup, ch, TEGRA_UTC_DATA);
+}
+
+#endif
+
+static const struct uart_ops tegra_utc_uart_ops = {
+	.tx_empty = tegra_utc_tx_empty,
+	.set_mctrl = tegra_utc_set_mctrl,
+	.get_mctrl = tegra_utc_get_mctrl,
+	.stop_tx = tegra_utc_stop_tx,
+	.start_tx = tegra_utc_start_tx,
+	.stop_rx = tegra_utc_stop_rx,
+	.startup = tegra_utc_startup,
+	.shutdown = tegra_utc_shutdown,
+	.set_termios = tegra_utc_set_termios,
+#ifdef CONFIG_CONSOLE_POLL
+	.poll_init = tegra_utc_poll_init,
+	.poll_get_char = tegra_utc_get_poll_char,
+	.poll_put_char = tegra_utc_put_poll_char,
+#endif
+};
+
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA_UTC_CONSOLE)
+#define TEGRA_UTC_DEFAULT_FIFO_THRESHOLD	0x4
+#define TEGRA_UTC_EARLYCON_MAX_BURST_SIZE	128
+
+static void tegra_utc_putc(struct uart_port *port, unsigned char c)
+{
+	writel(c, port->membase + TEGRA_UTC_DATA);
+}
+
+static void tegra_utc_early_write(struct console *con, const char *s, unsigned int n)
+{
+	struct earlycon_device *dev = con->data;
+
+	while (n) {
+		u32 burst_size = TEGRA_UTC_EARLYCON_MAX_BURST_SIZE;
+
+		burst_size -= readl(dev->port.membase + TEGRA_UTC_FIFO_OCCUPANCY);
+		if (n < burst_size)
+			burst_size = n;
+
+		uart_console_write(&dev->port, s, burst_size, tegra_utc_putc);
+
+		n -= burst_size;
+		s += burst_size;
+	}
+}
+
+static int __init tegra_utc_early_console_setup(struct earlycon_device *device, const char *opt)
+{
+	if (!device->port.membase)
+		return -ENODEV;
+
+	/* Configure TX */
+	writel(TEGRA_UTC_COMMAND_FLUSH | TEGRA_UTC_COMMAND_RESET,
+		device->port.membase + TEGRA_UTC_COMMAND);
+	writel(TEGRA_UTC_DEFAULT_FIFO_THRESHOLD, device->port.membase + TEGRA_UTC_FIFO_THRESHOLD);
+
+	/* Clear and mask all the interrupts. */
+	writel(TEGRA_UTC_INTR_REQ | TEGRA_UTC_INTR_FULL | TEGRA_UTC_INTR_EMPTY,
+		device->port.membase + TEGRA_UTC_INTR_CLEAR);
+
+	writel(0x0, device->port.membase + TEGRA_UTC_INTR_MASK);
+	writel(0x0, device->port.membase + TEGRA_UTC_INTR_SET);
+
+	/* Enable TX. */
+	writel(TEGRA_UTC_ENABLE_CLIENT_ENABLE, device->port.membase + TEGRA_UTC_ENABLE);
+
+	device->con->write = tegra_utc_early_write;
+
+	return 0;
+}
+OF_EARLYCON_DECLARE(tegra_utc, "nvidia,tegra264-utc", tegra_utc_early_console_setup);
+
+static void tegra_utc_console_putchar(struct uart_port *port, unsigned char ch)
+{
+	struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+	tegra_utc_tx_writel(tup, ch, TEGRA_UTC_DATA);
+}
+
+static void tegra_utc_console_write(struct console *cons, const char *s, unsigned int count)
+{
+	struct tegra_utc_port *tup = container_of(cons, struct tegra_utc_port, console);
+	unsigned long flags;
+	int locked = 1;
+
+	if (tup->port.sysrq || oops_in_progress)
+		locked = uart_port_trylock_irqsave(&tup->port, &flags);
+	else
+		uart_port_lock_irqsave(&tup->port, &flags);
+
+	while (count) {
+		u32 burst_size = tup->soc->fifosize;
+
+		burst_size -= tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_OCCUPANCY);
+		if (count < burst_size)
+			burst_size = count;
+
+		uart_console_write(&tup->port, s, burst_size, tegra_utc_console_putchar);
+
+		count -= burst_size;
+		s += burst_size;
+	};
+
+	if (locked)
+		uart_port_unlock_irqrestore(&tup->port, flags);
+}
+
+static int tegra_utc_console_setup(struct console *cons, char *options)
+{
+	struct tegra_utc_port *tup = container_of(cons, struct tegra_utc_port, console);
+
+	tegra_utc_init_tx(tup);
+
+	return 0;
+}
+#endif
+
+static struct uart_driver tegra_utc_driver = {
+	.driver_name	= "tegra-utc",
+	.dev_name	= "ttyUTC",
+	.nr		= UART_NR
+};
+
+static void tegra_utc_setup_port(struct device *dev, struct tegra_utc_port *tup,
+				 unsigned int index)
+{
+	tup->port.dev		= dev;
+	tup->port.fifosize	= tup->soc->fifosize;
+	tup->port.flags		= UPF_BOOT_AUTOCONF;
+	tup->port.iotype	= UPIO_MEM;
+	tup->port.ops		= &tegra_utc_uart_ops;
+	tup->port.type		= PORT_TEGRA_TCU;
+	tup->port.line		= index;
+	tup->port.private_data	= tup;
+
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA_UTC_CONSOLE)
+	strscpy(tup->console.name, "ttyUTC", sizeof(tup->console.name));
+	tup->console.write	= tegra_utc_console_write;
+	tup->console.device	= uart_console_device;
+	tup->console.setup	= tegra_utc_console_setup;
+	tup->console.flags	= CON_PRINTBUFFER | CON_CONSDEV | CON_ANYTIME;
+	tup->console.data	= &tegra_utc_driver;
+#endif
+}
+
+static int tegra_utc_register_port(struct tegra_utc_port *tup)
+{
+	int ret;
+
+	ret = uart_add_one_port(&tegra_utc_driver, &tup->port);
+	if (ret)
+		return ret;
+
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA_UTC_CONSOLE)
+	register_console(&tup->console);
+#endif
+
+	return 0;
+}
+
+static int tegra_utc_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct tegra_utc_port *tup;
+	int index;
+	int ret;
+
+	index = of_alias_get_id(np, "serial");
+	if (index < 0) {
+		dev_err(&pdev->dev, "failed to get alias id, err %d\n", index);
+		return index;
+	}
+
+	tup = devm_kzalloc(&pdev->dev, sizeof(struct tegra_utc_port), GFP_KERNEL);
+	if (!tup)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(np, "current-speed", &tup->baudrate);
+	if (ret) {
+		dev_err(&pdev->dev, "missing current-speed device-tree property\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "nvidia,utc-fifo-threshold", &tup->fifo_threshold);
+	if (ret) {
+		dev_err(&pdev->dev, "missing nvidia,fifo-threshold device-tree property\n");
+		return ret;
+	}
+
+	tup->irq = platform_get_irq(pdev, 0);
+	if (tup->irq < 0) {
+		dev_err(&pdev->dev, "failed to get interrupt\n");
+		return tup->irq;
+	}
+
+	tup->soc = of_device_get_match_data(&pdev->dev);
+
+	tup->tx_base = devm_platform_ioremap_resource_byname(pdev, "tx");
+	if (IS_ERR(tup->tx_base))
+		return PTR_ERR(tup->tx_base);
+
+	tup->rx_base = devm_platform_ioremap_resource_byname(pdev, "rx");
+	if (IS_ERR(tup->rx_base))
+		return PTR_ERR(tup->rx_base);
+
+	tegra_utc_setup_port(&pdev->dev, tup, index);
+	platform_set_drvdata(pdev, tup);
+
+	return tegra_utc_register_port(tup);
+}
+
+static void tegra_utc_remove(struct platform_device *pdev)
+{
+	struct tegra_utc_port *tup = platform_get_drvdata(pdev);
+
+	uart_remove_one_port(&tegra_utc_driver, &tup->port);
+}
+
+static const struct tegra_utc_soc tegra264_utc_soc = {
+	.fifosize = 128,
+};
+
+static const struct of_device_id tegra_utc_of_match[] = {
+	{ .compatible = "nvidia,tegra264-utc", .data = &tegra264_utc_soc },
+	{},
+};
+MODULE_DEVICE_TABLE(of, tegra_utc_of_match);
+
+static struct platform_driver tegra_utc_platform_driver = {
+	.probe = tegra_utc_probe,
+	.remove = tegra_utc_remove,
+	.driver = {
+		.name = "tegra-utc",
+		.of_match_table = tegra_utc_of_match,
+	},
+};
+
+static int __init tegra_utc_init(void)
+{
+	int ret;
+
+	ret = uart_register_driver(&tegra_utc_driver);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&tegra_utc_platform_driver);
+	if (ret) {
+		uart_unregister_driver(&tegra_utc_driver);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(tegra_utc_init);
+
+static void __exit tegra_utc_exit(void)
+{
+	platform_driver_unregister(&tegra_utc_platform_driver);
+	uart_unregister_driver(&tegra_utc_driver);
+}
+module_exit(tegra_utc_exit);
+
+MODULE_AUTHOR("Kartik Rajput <kkartik@nvidia.com>");
+MODULE_DESCRIPTION("Tegra UART Trace Controller");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[Andy Shevchenko]

On Tue, Jan 28, 2025 at 12:16:33PM +0530, Kartik Rajput wrote:
> The Tegra264 SoC supports the UART Trace Controller (UTC), which allows
> multiple firmware clients (up to 16) to share a single physical UART.
> Each client is provided with its own interrupt and has access to a
> 128-character wide FIFO for both transmit (TX) and receive (RX)
> operations.
> 
> Add tegra-utc driver to support Tegra UART Trace Controller (UTC)
> client.

...

> +static int tegra_utc_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct tegra_utc_port *tup;
> +	int index;
> +	int ret;
> +
> +	index = of_alias_get_id(np, "serial");
> +	if (index < 0) {
> +		dev_err(&pdev->dev, "failed to get alias id, err %d\n", index);
> +		return index;
> +	}

Can we please use uart_read_port_properties() instead of open-coding everything
again and again?

> +	tup = devm_kzalloc(&pdev->dev, sizeof(struct tegra_utc_port), GFP_KERNEL);
> +	if (!tup)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32(np, "current-speed", &tup->baudrate);

Why not clock-frequency? But if needed, add this to the above mentioned API as
I know more than one driver may utilise this.


> +	if (ret) {
> +		dev_err(&pdev->dev, "missing current-speed device-tree property\n");

With

	struct device *dev = &pdev-dev;

this and other lines will be neater.

> +		return ret;

		return dev_err_probe(...);

> +	}
> +
> +	ret = of_property_read_u32(np, "nvidia,utc-fifo-threshold", &tup->fifo_threshold);
> +	if (ret) {
> +		dev_err(&pdev->dev, "missing nvidia,fifo-threshold device-tree property\n");
> +		return ret;
> +	}
> +
> +	tup->irq = platform_get_irq(pdev, 0);
> +	if (tup->irq < 0) {

> +		dev_err(&pdev->dev, "failed to get interrupt\n");

Dup. This error report is done by the above API.

> +		return tup->irq;
> +	}

> +	tup->soc = of_device_get_match_data(&pdev->dev);

> +	tup->tx_base = devm_platform_ioremap_resource_byname(pdev, "tx");
> +	if (IS_ERR(tup->tx_base))
> +		return PTR_ERR(tup->tx_base);
> +
> +	tup->rx_base = devm_platform_ioremap_resource_byname(pdev, "rx");
> +	if (IS_ERR(tup->rx_base))
> +		return PTR_ERR(tup->rx_base);
> +
> +	tegra_utc_setup_port(&pdev->dev, tup, index);
> +	platform_set_drvdata(pdev, tup);
> +
> +	return tegra_utc_register_port(tup);
> +}

...

> +static const struct of_device_id tegra_utc_of_match[] = {
> +	{ .compatible = "nvidia,tegra264-utc", .data = &tegra264_utc_soc },
> +	{},

No comma for the terminator line.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[Kartik Rajput]

Thanks for reviewing the patch Andy!

On Wed, 2025-01-29 at 09:07 +0200, Andy Shevchenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Jan 28, 2025 at 12:16:33PM +0530, Kartik Rajput wrote:
> > The Tegra264 SoC supports the UART Trace Controller (UTC), which
> > allows
> > multiple firmware clients (up to 16) to share a single physical
> > UART.
> > Each client is provided with its own interrupt and has access to a
> > 128-character wide FIFO for both transmit (TX) and receive (RX)
> > operations.
> > 
> > Add tegra-utc driver to support Tegra UART Trace Controller (UTC)
> > client.
> 
> ...
> 
> > +static int tegra_utc_probe(struct platform_device *pdev)
> > +{
> > +     struct device_node *np = pdev->dev.of_node;
> > +     struct tegra_utc_port *tup;
> > +     int index;
> > +     int ret;
> > +
> > +     index = of_alias_get_id(np, "serial");
> > +     if (index < 0) {
> > +             dev_err(&pdev->dev, "failed to get alias id, err
> > %d\n", index);
> > +             return index;
> > +     }
> 
> Can we please use uart_read_port_properties() instead of open-coding
> everything
> again and again?
> 

Ack. I will use uart_read_port_properties() in v2.

> > +     tup = devm_kzalloc(&pdev->dev, sizeof(struct tegra_utc_port),
> > GFP_KERNEL);
> > +     if (!tup)
> > +             return -ENOMEM;
> > +
> > +     ret = of_property_read_u32(np, "current-speed", &tup-
> > >baudrate);
> 
> Why not clock-frequency? But if needed, add this to the above
> mentioned API as
> I know more than one driver may utilise this.
> 
> 

"current-speed" is to specify the baudrate at which the UTC is
operating. Not sure if "clock-frequency" is the ideal property for this
as we are not configuring any clocks in the driver.

Also, to add this in uart_read_port_properties() would require updating
uart_port stucture to store the baudrate value. Would this be fine?
Asking because I do not see termios related configurations stored in
uart_port structure.

> > +     if (ret) {
> > +             dev_err(&pdev->dev, "missing current-speed device-
> > tree property\n");
> 
> With
> 
>         struct device *dev = &pdev-dev;
> 
> this and other lines will be neater.
> 
> > +             return ret;
> 
>                 return dev_err_probe(...);
> 

Ack. I will update this in v2.

> > +     }
> > +
> > +     ret = of_property_read_u32(np, "nvidia,utc-fifo-threshold",
> > &tup->fifo_threshold);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "missing nvidia,fifo-threshold
> > device-tree property\n");
> > +             return ret;
> > +     }
> > +
> > +     tup->irq = platform_get_irq(pdev, 0);
> > +     if (tup->irq < 0) {
> 
> > +             dev_err(&pdev->dev, "failed to get interrupt\n");
> 
> Dup. This error report is done by the above API.
> 

Ack. I will fix this in v2.

> > +             return tup->irq;
> > +     }
> 
> > +     tup->soc = of_device_get_match_data(&pdev->dev);
> 
> > +     tup->tx_base = devm_platform_ioremap_resource_byname(pdev,
> > "tx");
> > +     if (IS_ERR(tup->tx_base))
> > +             return PTR_ERR(tup->tx_base);
> > +
> > +     tup->rx_base = devm_platform_ioremap_resource_byname(pdev,
> > "rx");
> > +     if (IS_ERR(tup->rx_base))
> > +             return PTR_ERR(tup->rx_base);
> > +
> > +     tegra_utc_setup_port(&pdev->dev, tup, index);
> > +     platform_set_drvdata(pdev, tup);
> > +
> > +     return tegra_utc_register_port(tup);
> > +}
> 
> ...
> 
> > +static const struct of_device_id tegra_utc_of_match[] = {
> > +     { .compatible = "nvidia,tegra264-utc", .data =
> > &tegra264_utc_soc },
> > +     {},
> 
> No comma for the terminator line.
> 
> > +};
> 

Ack. I will fix this in v2.

> --
> With Best Regards,
> Andy Shevchenko
> 
> 

Thanks & Regards,
Kartik

Re: [PATCH 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)

[andriy.shevchenko]

On Wed, Jan 29, 2025 at 08:04:27AM +0000, Kartik Rajput wrote:
> On Wed, 2025-01-29 at 09:07 +0200, Andy Shevchenko wrote:
> > On Tue, Jan 28, 2025 at 12:16:33PM +0530, Kartik Rajput wrote:

...

> > > +     ret = of_property_read_u32(np, "current-speed", &tup-
> > > >baudrate);
> > 
> > Why not clock-frequency? But if needed, add this to the above
> > mentioned API as
> > I know more than one driver may utilise this.
> 
> "current-speed" is to specify the baudrate at which the UTC is
> operating. Not sure if "clock-frequency" is the ideal property for this
> as we are not configuring any clocks in the driver.

I didn't say anything about configuring clocks. The clock-frequency property is
standard way to provide a functional frequency of the UART (usually some MHz
crystal) which you can divide by the known HW coefficients and get the baud rate
(but yes, I agree that this is HW-dependent and needs to be thought through).

> Also, to add this in uart_read_port_properties() would require updating
> uart_port stucture to store the baudrate value. Would this be fine?

Sure, it requires some members to be defined beforehand.

> Asking because I do not see termios related configurations stored in
> uart_port structure.

Yeah, the only one is uartclk. But again, if you need current-speed to be
parsed, consider doing this in the uart_read_port_properties() and saving
as uartclk in the known way.
Technically it's possible to add a new member to uart_port, but it has
a lot of downsides as I can see: additional memory, added ambiguity to
how parse uartclk and current speed if they both are defined.

-- 
With Best Regards,
Andy Shevchenko