[PATCH RFC 0/2] Introduce PRU UART driver

[PATCH RFC 0/2] Introduce PRU UART driver

[Judith Mendez]

This patch series is sent as an RFC to get some initial comments
on the PRU UART driver.

The ICSSM modules on am64x SoC and the PRUSS module on am62 SoC or am335x
SoCs have a UART sub-module. This patch series introduces the driver and the
corresponding binding documentation for this sub-module.

The DTS patches for adding PRU nodes and enabling PRU UART will be added
in a later v1 version of the series if accepted.

This driver has been previously tested on the following boards:
am64x SK, am62x SK, and am335x SK boards.

Bin Liu (2):
  dt-bindings: serial: add binding documentation for TI PRUSS UART
  serial: 8250: Add PRUSS UART driver

 .../bindings/serial/ti,pruss-uart.yaml        |  54 +++++
 drivers/tty/serial/8250/8250_pruss.c          | 213 ++++++++++++++++++
 drivers/tty/serial/8250/Kconfig               |  10 +
 drivers/tty/serial/8250/Makefile              |   1 +
 4 files changed, 278 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/ti,pruss-uart.yaml
 create mode 100644 drivers/tty/serial/8250/8250_pruss.c

-- 
2.49.0

[PATCH RFC 1/2] dt-bindings: serial: add binding documentation for TI PRUSS UART

[Judith Mendez]

From: Bin Liu <b-liu@ti.com>

This adds the YAML DT binding for PRUSS UART on TI SoCs.

Signed-off-by: Bin Liu <b-liu@ti.com>
Signed-off-by: Judith Mendez <jm@ti.com>
---
 .../bindings/serial/ti,pruss-uart.yaml        | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/ti,pruss-uart.yaml

diff --git a/Documentation/devicetree/bindings/serial/ti,pruss-uart.yaml b/Documentation/devicetree/bindings/serial/ti,pruss-uart.yaml
new file mode 100644
index 000000000000..f7f660688f7e
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/ti,pruss-uart.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/serial/ti,pruss-uart.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI PRUSS serial UART
+
+maintainers:
+  - Bin Liu <b-liu@ti.com>
+
+description: |
+  The PRU-ICSS module has a serial UART peripheral, which is based on
+  industry standard TL16C550, with 16-bytes TX/RX FIFOs.
+
+allOf:
+  - $ref: /schemas/serial.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: ti,pruss-uart
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+    description: |
+      PRU UART interrupt mappings, containing an entry of 3 cell-values.
+      The first is the PRU System Event ID for PRU UART Interrupt Request.
+      The second is the PRU interrupt channel ID.
+      The third is the PRU host interrupt ID.
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - interrupts
+
+additionalProperties: true
+
+examples:
+  - |
+    pruss_uart: serial@28000 {
+        compatible = "ti,pruss-uart";
+        reg = <0x28000 0x40>;
+        clocks = <&k3_clks 81 13>;
+        interrupt-parent = <&pruss_intc>;
+        interrupts = <6 4 4>;
+    };
-- 
2.49.0

[PATCH RFC 2/2] serial: 8250: Add PRUSS UART driver

[Judith Mendez]

From: Bin Liu <b-liu@ti.com>

This adds a new serial 8250 driver that supports the UART in PRUSS
module.

The PRUSS has a UART sub-module which is based on the industry standard
TL16C550 UART controller, which has 16-bytes FIFO and supports 16x and
13x over samplings.

Signed-off-by: Bin Liu <b-liu@ti.com>
Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/tty/serial/8250/8250_pruss.c | 213 +++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig      |  10 ++
 drivers/tty/serial/8250/Makefile     |   1 +
 3 files changed, 224 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_pruss.c

diff --git a/drivers/tty/serial/8250/8250_pruss.c b/drivers/tty/serial/8250/8250_pruss.c
new file mode 100644
index 000000000000..2943bf7d6645
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_pruss.c
@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ *  Serial Port driver for PRUSS UART on TI platforms
+ *
+ *  Copyright (C) 2020-2021 by Texas Instruments Incorporated - http://www.ti.com/
+ *  Author: Bin Liu <b-liu@ti.com>
+ */
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/serial_reg.h>
+#include <linux/serial_core.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/remoteproc.h>
+#include "8250.h"
+
+#define DEFAULT_CLK_SPEED	192000000
+
+/* extra registers */
+#define PRUSS_UART_PEREMU_MGMT	12
+#define PRUSS_UART_TX_EN	BIT(14)
+#define PRUSS_UART_RX_EN	BIT(13)
+#define PRUSS_UART_FREE_RUN	BIT(0)
+
+#define PRUSS_UART_MDR			13
+#define PRUSS_UART_MDR_OSM_SEL_MASK	BIT(0)
+#define PRUSS_UART_MDR_16X_MODE		0
+#define PRUSS_UART_MDR_13X_MODE		1
+
+struct pruss8250_info {
+	int type;
+	int line;
+};
+
+static inline void uart_writel(struct uart_port *p, u32 offset, int value)
+{
+	writel(value, p->membase + (offset << p->regshift));
+}
+
+static int pruss8250_startup(struct uart_port *port)
+{
+	int ret;
+
+	uart_writel(port, PRUSS_UART_PEREMU_MGMT, 0);
+
+	ret = serial8250_do_startup(port);
+	if (!ret)
+		uart_writel(port, PRUSS_UART_PEREMU_MGMT, PRUSS_UART_TX_EN |
+							  PRUSS_UART_RX_EN |
+							  PRUSS_UART_FREE_RUN);
+	return ret;
+}
+
+static unsigned int pruss8250_get_divisor(struct uart_port *port,
+					  unsigned int baud,
+					  unsigned int *frac)
+{
+	unsigned int uartclk = port->uartclk;
+	unsigned int div_13, div_16;
+	unsigned int abs_d13, abs_d16;
+	u16 quot;
+
+	/* Old custom speed handling */
+	if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
+		quot = port->custom_divisor & UART_DIV_MAX;
+		if (port->custom_divisor & (1 << 16))
+			*frac = PRUSS_UART_MDR_13X_MODE;
+		else
+			*frac = PRUSS_UART_MDR_16X_MODE;
+
+		return quot;
+	}
+
+	div_13 = DIV_ROUND_CLOSEST(uartclk, 13 * baud);
+	div_16 = DIV_ROUND_CLOSEST(uartclk, 16 * baud);
+	div_13 = div_13 ? : 1;
+	div_16 = div_16 ? : 1;
+
+	abs_d13 = abs(baud - uartclk / 13 / div_13);
+	abs_d16 = abs(baud - uartclk / 16 / div_16);
+
+	if (abs_d13 >= abs_d16) {
+		*frac = PRUSS_UART_MDR_16X_MODE;
+		quot = div_16;
+	} else {
+		*frac = PRUSS_UART_MDR_13X_MODE;
+		quot = div_13;
+	}
+
+	return quot;
+}
+
+static void pruss8250_set_divisor(struct uart_port *port, unsigned int baud,
+				  unsigned int quot, unsigned int quot_frac)
+{
+	serial8250_do_set_divisor(port, baud, quot);
+	/*
+	 * quot_frac holds the MDR over-sampling mode
+	 * which is set in pruss8250_get_divisor()
+	 */
+	quot_frac &= PRUSS_UART_MDR_OSM_SEL_MASK;
+	serial_port_out(port, PRUSS_UART_MDR, quot_frac);
+}
+
+static int pruss8250_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct uart_8250_port port8250;
+	struct uart_port *up = &port8250.port;
+	struct pruss8250_info *info;
+	struct resource resource;
+	unsigned int port_type;
+	struct clk *clk;
+	int ret;
+
+	port_type = (unsigned long)of_device_get_match_data(&pdev->dev);
+	if (port_type == PORT_UNKNOWN)
+		return -EINVAL;
+
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	memset(&port8250, 0, sizeof(port8250));
+
+	ret = of_address_to_resource(np, 0, &resource);
+	if (ret) {
+		dev_err(&pdev->dev, "invalid address\n");
+		return ret;
+	}
+
+	ret = of_alias_get_id(np, "serial");
+	if (ret > 0)
+		up->line = ret;
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk)) {
+		if (PTR_ERR(clk) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		up->uartclk = DEFAULT_CLK_SPEED;
+	} else {
+		up->uartclk = clk_get_rate(clk);
+		devm_clk_put(&pdev->dev, clk);
+	}
+
+	up->dev = &pdev->dev;
+	up->mapbase = resource.start;
+	up->mapsize = resource_size(&resource);
+	up->type = port_type;
+	up->iotype = UPIO_MEM;
+	up->regshift = 2;
+	up->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_FIXED_PORT |
+		    UPF_FIXED_TYPE | UPF_IOREMAP;
+	up->irqflags |= IRQF_SHARED;
+	up->startup = pruss8250_startup;
+	up->rs485_config = serial8250_em485_config;
+	up->get_divisor = pruss8250_get_divisor;
+	up->set_divisor = pruss8250_set_divisor;
+
+	ret = of_irq_get(np, 0);
+	if (ret < 0) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "missing irq\n");
+		return ret;
+	}
+
+	up->irq = ret;
+	spin_lock_init(&port8250.port.lock);
+	port8250.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
+
+	ret = serial8250_register_8250_port(&port8250);
+	if (ret < 0)
+		goto err_dispose;
+
+	info->type = port_type;
+	info->line = ret;
+	platform_set_drvdata(pdev, info);
+
+	return 0;
+
+err_dispose:
+	irq_dispose_mapping(port8250.port.irq);
+	return ret;
+}
+
+static void pruss8250_remove(struct platform_device *pdev)
+{
+	struct pruss8250_info *info = platform_get_drvdata(pdev);
+
+	serial8250_unregister_port(info->line);
+}
+
+static const struct of_device_id pruss8250_table[] = {
+	{ .compatible = "ti,pruss-uart", .data = (void *)PORT_16550A, },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, pruss8250_table);
+
+static struct platform_driver pruss8250_driver = {
+	.driver = {
+		.name = "pruss8250",
+		.of_match_table = pruss8250_table,
+	},
+	.probe = pruss8250_probe,
+	.remove = pruss8250_remove,
+};
+
+module_platform_driver(pruss8250_driver);
+
+MODULE_AUTHOR("Bin Liu <b-liu@ti.com");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Serial Port driver for PRUSS UART on TI platforms");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index f64ef0819cd4..cd4346609c55 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -582,6 +582,16 @@ config SERIAL_8250_NI
 	  To compile this driver as a module, choose M here: the module
 	  will be called 8250_ni.
 
+config SERIAL_8250_PRUSS
+	tristate "TI PRU-ICSS UART support"
+	depends on SERIAL_8250
+	depends on PRU_REMOTEPROC && TI_PRUSS_INTC
+	help
+	  This driver is to support the UART module in PRU-ICSS which is
+	  available in some TI platforms.
+	  Say 'Y' here if you wish to use PRU-ICSS UART.
+	  Otherwise, say 'N'.
+
 config SERIAL_OF_PLATFORM
 	tristate "Devicetree based probing for 8250 ports"
 	depends on SERIAL_8250 && OF
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index b04eeda03b23..3132b4f40a34 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_SERIAL_8250_PARISC)	+= 8250_parisc.o
 obj-$(CONFIG_SERIAL_8250_PCI)		+= 8250_pci.o
 obj-$(CONFIG_SERIAL_8250_PCI1XXXX)	+= 8250_pci1xxxx.o
 obj-$(CONFIG_SERIAL_8250_PERICOM)	+= 8250_pericom.o
+obj-$(CONFIG_SERIAL_8250_PRUSS)		+= 8250_pruss.o
 obj-$(CONFIG_SERIAL_8250_PXA)		+= 8250_pxa.o
 obj-$(CONFIG_SERIAL_8250_RT288X)	+= 8250_rt288x.o
 obj-$(CONFIG_SERIAL_8250_CS)		+= serial_cs.o
-- 
2.49.0

Re: [PATCH RFC 0/2] Introduce PRU UART driver

[Greg Kroah-Hartman]

On Wed, Apr 30, 2025 at 07:31:11PM -0500, Judith Mendez wrote:
> This patch series is sent as an RFC to get some initial comments
> on the PRU UART driver.
> 
> The ICSSM modules on am64x SoC and the PRUSS module on am62 SoC or am335x
> SoCs have a UART sub-module. This patch series introduces the driver and the
> corresponding binding documentation for this sub-module.
> 
> The DTS patches for adding PRU nodes and enabling PRU UART will be added
> in a later v1 version of the series if accepted.
> 
> This driver has been previously tested on the following boards:
> am64x SK, am62x SK, and am335x SK boards.

Why is this "RFC"?  What needs to be done to make it something that you
actually feel works properly and should be merged?

thanks,

greg k-h

Re: [PATCH RFC 0/2] Introduce PRU UART driver

[Judith Mendez]

Hi Greg,

On 5/1/25 12:18 AM, Greg Kroah-Hartman wrote:
> On Wed, Apr 30, 2025 at 07:31:11PM -0500, Judith Mendez wrote:
>> This patch series is sent as an RFC to get some initial comments
>> on the PRU UART driver.
>>
>> The ICSSM modules on am64x SoC and the PRUSS module on am62 SoC or am335x
>> SoCs have a UART sub-module. This patch series introduces the driver and the
>> corresponding binding documentation for this sub-module.
>>
>> The DTS patches for adding PRU nodes and enabling PRU UART will be added
>> in a later v1 version of the series if accepted.
>>
>> This driver has been previously tested on the following boards:
>> am64x SK, am62x SK, and am335x SK boards.
> 
> Why is this "RFC"?  What needs to be done to make it something that you
> actually feel works properly and should be merged?

Nothing needs to be done IMO, the only reason it was sent as an RFC is
to get initial thoughts/issues that anyone might have with the driver
before sending v1.

If none, I will go ahead and send v1. Thanks for your attention Greg.

~ Judith

Re: [PATCH RFC 2/2] serial: 8250: Add PRUSS UART driver

[Andrew Davis]

On 4/30/25 7:31 PM, Judith Mendez wrote:
> From: Bin Liu <b-liu@ti.com>
> 
> This adds a new serial 8250 driver that supports the UART in PRUSS
> module.
> 
> The PRUSS has a UART sub-module which is based on the industry standard
> TL16C550 UART controller, which has 16-bytes FIFO and supports 16x and
> 13x over samplings.
> 
> Signed-off-by: Bin Liu <b-liu@ti.com>
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>   drivers/tty/serial/8250/8250_pruss.c | 213 +++++++++++++++++++++++++++
>   drivers/tty/serial/8250/Kconfig      |  10 ++
>   drivers/tty/serial/8250/Makefile     |   1 +
>   3 files changed, 224 insertions(+)
>   create mode 100644 drivers/tty/serial/8250/8250_pruss.c
> 
> diff --git a/drivers/tty/serial/8250/8250_pruss.c b/drivers/tty/serial/8250/8250_pruss.c
> new file mode 100644
> index 000000000000..2943bf7d6645
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_pruss.c
> @@ -0,0 +1,213 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *  Serial Port driver for PRUSS UART on TI platforms
> + *
> + *  Copyright (C) 2020-2021 by Texas Instruments Incorporated - http://www.ti.com/
> + *  Author: Bin Liu <b-liu@ti.com>
> + */
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/serial_reg.h>
> +#include <linux/serial_core.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/remoteproc.h>
> +#include "8250.h"
> +
> +#define DEFAULT_CLK_SPEED	192000000
> +
> +/* extra registers */
> +#define PRUSS_UART_PEREMU_MGMT	12
> +#define PRUSS_UART_TX_EN	BIT(14)
> +#define PRUSS_UART_RX_EN	BIT(13)
> +#define PRUSS_UART_FREE_RUN	BIT(0)
> +
> +#define PRUSS_UART_MDR			13
> +#define PRUSS_UART_MDR_OSM_SEL_MASK	BIT(0)
> +#define PRUSS_UART_MDR_16X_MODE		0
> +#define PRUSS_UART_MDR_13X_MODE		1
> +
> +struct pruss8250_info {
> +	int type;

You never use type, why store it here?

> +	int line;
> +};
> +
> +static inline void uart_writel(struct uart_port *p, u32 offset, int value)
> +{
> +	writel(value, p->membase + (offset << p->regshift));
> +}
> +
> +static int pruss8250_startup(struct uart_port *port)
> +{
> +	int ret;
> +
> +	uart_writel(port, PRUSS_UART_PEREMU_MGMT, 0);
> +
> +	ret = serial8250_do_startup(port);
> +	if (!ret)
> +		uart_writel(port, PRUSS_UART_PEREMU_MGMT, PRUSS_UART_TX_EN |
> +							  PRUSS_UART_RX_EN |
> +							  PRUSS_UART_FREE_RUN);
> +	return ret;
> +}
> +
> +static unsigned int pruss8250_get_divisor(struct uart_port *port,
> +					  unsigned int baud,
> +					  unsigned int *frac)
> +{
> +	unsigned int uartclk = port->uartclk;
> +	unsigned int div_13, div_16;
> +	unsigned int abs_d13, abs_d16;
> +	u16 quot;
> +
> +	/* Old custom speed handling */
> +	if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
> +		quot = port->custom_divisor & UART_DIV_MAX;
> +		if (port->custom_divisor & (1 << 16))
> +			*frac = PRUSS_UART_MDR_13X_MODE;
> +		else
> +			*frac = PRUSS_UART_MDR_16X_MODE;
> +
> +		return quot;
> +	}
> +
> +	div_13 = DIV_ROUND_CLOSEST(uartclk, 13 * baud);
> +	div_16 = DIV_ROUND_CLOSEST(uartclk, 16 * baud);
> +	div_13 = div_13 ? : 1;
> +	div_16 = div_16 ? : 1;
> +
> +	abs_d13 = abs(baud - uartclk / 13 / div_13);
> +	abs_d16 = abs(baud - uartclk / 16 / div_16);
> +
> +	if (abs_d13 >= abs_d16) {
> +		*frac = PRUSS_UART_MDR_16X_MODE;
> +		quot = div_16;
> +	} else {
> +		*frac = PRUSS_UART_MDR_13X_MODE;
> +		quot = div_13;
> +	}
> +
> +	return quot;
> +}
> +
> +static void pruss8250_set_divisor(struct uart_port *port, unsigned int baud,
> +				  unsigned int quot, unsigned int quot_frac)
> +{
> +	serial8250_do_set_divisor(port, baud, quot);
> +	/*
> +	 * quot_frac holds the MDR over-sampling mode
> +	 * which is set in pruss8250_get_divisor()
> +	 */
> +	quot_frac &= PRUSS_UART_MDR_OSM_SEL_MASK;
> +	serial_port_out(port, PRUSS_UART_MDR, quot_frac);
> +}
> +
> +static int pruss8250_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct uart_8250_port port8250;
> +	struct uart_port *up = &port8250.port;
> +	struct pruss8250_info *info;
> +	struct resource resource;
> +	unsigned int port_type;
> +	struct clk *clk;
> +	int ret;
> +
> +	port_type = (unsigned long)of_device_get_match_data(&pdev->dev);

Will the port type ever not be PORT_16550A?

> +	if (port_type == PORT_UNKNOWN)
> +		return -EINVAL;
> +
> +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	memset(&port8250, 0, sizeof(port8250));
> +
> +	ret = of_address_to_resource(np, 0, &resource);

platform_get_resource()

> +	if (ret) {
> +		dev_err(&pdev->dev, "invalid address\n");
> +		return ret;
> +	}
> +
> +	ret = of_alias_get_id(np, "serial");

Can you make use of the uart_read_port_properties() helper here?

Andrew

> +	if (ret > 0)
> +		up->line = ret;
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk)) {
> +		if (PTR_ERR(clk) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +		up->uartclk = DEFAULT_CLK_SPEED;
> +	} else {
> +		up->uartclk = clk_get_rate(clk);
> +		devm_clk_put(&pdev->dev, clk);
> +	}
> +
> +	up->dev = &pdev->dev;
> +	up->mapbase = resource.start;
> +	up->mapsize = resource_size(&resource);
> +	up->type = port_type;
> +	up->iotype = UPIO_MEM;
> +	up->regshift = 2;
> +	up->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_FIXED_PORT |
> +		    UPF_FIXED_TYPE | UPF_IOREMAP;
> +	up->irqflags |= IRQF_SHARED;
> +	up->startup = pruss8250_startup;
> +	up->rs485_config = serial8250_em485_config;
> +	up->get_divisor = pruss8250_get_divisor;
> +	up->set_divisor = pruss8250_set_divisor;
> +
> +	ret = of_irq_get(np, 0);
> +	if (ret < 0) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "missing irq\n");
> +		return ret;
> +	}
> +
> +	up->irq = ret;
> +	spin_lock_init(&port8250.port.lock);
> +	port8250.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
> +
> +	ret = serial8250_register_8250_port(&port8250);
> +	if (ret < 0)
> +		goto err_dispose;
> +
> +	info->type = port_type;
> +	info->line = ret;
> +	platform_set_drvdata(pdev, info);
> +
> +	return 0;
> +
> +err_dispose:
> +	irq_dispose_mapping(port8250.port.irq);
> +	return ret;
> +}
> +
> +static void pruss8250_remove(struct platform_device *pdev)
> +{
> +	struct pruss8250_info *info = platform_get_drvdata(pdev);
> +
> +	serial8250_unregister_port(info->line);
> +}
> +
> +static const struct of_device_id pruss8250_table[] = {
> +	{ .compatible = "ti,pruss-uart", .data = (void *)PORT_16550A, },
> +	{ /* end of list */ },
> +};
> +MODULE_DEVICE_TABLE(of, pruss8250_table);
> +
> +static struct platform_driver pruss8250_driver = {
> +	.driver = {
> +		.name = "pruss8250",
> +		.of_match_table = pruss8250_table,
> +	},
> +	.probe = pruss8250_probe,
> +	.remove = pruss8250_remove,
> +};
> +
> +module_platform_driver(pruss8250_driver);
> +
> +MODULE_AUTHOR("Bin Liu <b-liu@ti.com");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Serial Port driver for PRUSS UART on TI platforms");
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index f64ef0819cd4..cd4346609c55 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -582,6 +582,16 @@ config SERIAL_8250_NI
>   	  To compile this driver as a module, choose M here: the module
>   	  will be called 8250_ni.
>   
> +config SERIAL_8250_PRUSS
> +	tristate "TI PRU-ICSS UART support"
> +	depends on SERIAL_8250
> +	depends on PRU_REMOTEPROC && TI_PRUSS_INTC
> +	help
> +	  This driver is to support the UART module in PRU-ICSS which is
> +	  available in some TI platforms.
> +	  Say 'Y' here if you wish to use PRU-ICSS UART.
> +	  Otherwise, say 'N'.
> +
>   config SERIAL_OF_PLATFORM
>   	tristate "Devicetree based probing for 8250 ports"
>   	depends on SERIAL_8250 && OF
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index b04eeda03b23..3132b4f40a34 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -47,6 +47,7 @@ obj-$(CONFIG_SERIAL_8250_PARISC)	+= 8250_parisc.o
>   obj-$(CONFIG_SERIAL_8250_PCI)		+= 8250_pci.o
>   obj-$(CONFIG_SERIAL_8250_PCI1XXXX)	+= 8250_pci1xxxx.o
>   obj-$(CONFIG_SERIAL_8250_PERICOM)	+= 8250_pericom.o
> +obj-$(CONFIG_SERIAL_8250_PRUSS)		+= 8250_pruss.o
>   obj-$(CONFIG_SERIAL_8250_PXA)		+= 8250_pxa.o
>   obj-$(CONFIG_SERIAL_8250_RT288X)	+= 8250_rt288x.o
>   obj-$(CONFIG_SERIAL_8250_CS)		+= serial_cs.o

Re: [PATCH RFC 0/2] Introduce PRU UART driver

[Andy Shevchenko]

On Wed, Apr 30, 2025 at 07:31:11PM -0500, Judith Mendez wrote:
> This patch series is sent as an RFC to get some initial comments
> on the PRU UART driver.
> 
> The ICSSM modules on am64x SoC and the PRUSS module on am62 SoC or am335x
> SoCs have a UART sub-module. This patch series introduces the driver and the
> corresponding binding documentation for this sub-module.
> 
> The DTS patches for adding PRU nodes and enabling PRU UART will be added
> in a later v1 version of the series if accepted.
> 
> This driver has been previously tested on the following boards:
> am64x SK, am62x SK, and am335x SK boards.

The first and main question here: Have you checked the existing zillion of
drivers and become with an idea that none of them not even close to this one
(based on RTL)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 2/2] serial: 8250: Add PRUSS UART driver

[Andy Shevchenko]

On Wed, Apr 30, 2025 at 07:31:13PM -0500, Judith Mendez wrote:
> From: Bin Liu <b-liu@ti.com>
> 
> This adds a new serial 8250 driver that supports the UART in PRUSS
> module.
> 
> The PRUSS has a UART sub-module which is based on the industry standard
> TL16C550 UART controller, which has 16-bytes FIFO and supports 16x and
> 13x over samplings.

> Signed-off-by: Bin Liu <b-liu@ti.com>
> Signed-off-by: Judith Mendez <jm@ti.com>

Are you just a committer and not a co-developer of this code?

...

> +/*
> + *  Serial Port driver for PRUSS UART on TI platforms
> + *
> + *  Copyright (C) 2020-2021 by Texas Instruments Incorporated - http://www.ti.com/

My calendar shows 2025...

> + *  Author: Bin Liu <b-liu@ti.com>
> + */

...

The list of the inclusions is semi-random. Please, follow the IWYU principle.

> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/serial_reg.h>
> +#include <linux/serial_core.h>

> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>

Please, no of*.h in a new code.

> +#include <linux/remoteproc.h>

Keep them ordered as well.

+ blank line here.

> +#include "8250.h"

...

> +#define DEFAULT_CLK_SPEED	192000000

Units as a suffix? HZ_PER_MHZ?
You can also fix 8250_omap in the same way.

...

> +static inline void uart_writel(struct uart_port *p, u32 offset, int value)
> +{
> +	writel(value, p->membase + (offset << p->regshift));
> +}

Why? Or how does it differ from the ones that serial core provides?

...

> +static int pruss8250_startup(struct uart_port *port)
> +{
> +	int ret;
> +
> +	uart_writel(port, PRUSS_UART_PEREMU_MGMT, 0);
> +
> +	ret = serial8250_do_startup(port);
> +	if (!ret)

Please, use usual pattern, check for errors first

	if (ret)
		return ret;
	...
	return 0;

> +		uart_writel(port, PRUSS_UART_PEREMU_MGMT, PRUSS_UART_TX_EN |
> +							  PRUSS_UART_RX_EN |
> +							  PRUSS_UART_FREE_RUN);
> +	return ret;
> +}

...

> +static unsigned int pruss8250_get_divisor(struct uart_port *port,
> +					  unsigned int baud,
> +					  unsigned int *frac)
> +{
> +	unsigned int uartclk = port->uartclk;
> +	unsigned int div_13, div_16;
> +	unsigned int abs_d13, abs_d16;
> +	u16 quot;

> +	/* Old custom speed handling */
> +	if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
> +		quot = port->custom_divisor & UART_DIV_MAX;
> +		if (port->custom_divisor & (1 << 16))
> +			*frac = PRUSS_UART_MDR_13X_MODE;
> +		else
> +			*frac = PRUSS_UART_MDR_16X_MODE;
> +
> +		return quot;
> +	}

Why?! Please, try to avoid adding more drivers with this ugly hack.

> +	div_13 = DIV_ROUND_CLOSEST(uartclk, 13 * baud);
> +	div_16 = DIV_ROUND_CLOSEST(uartclk, 16 * baud);
> +	div_13 = div_13 ? : 1;
> +	div_16 = div_16 ? : 1;
> +
> +	abs_d13 = abs(baud - uartclk / 13 / div_13);
> +	abs_d16 = abs(baud - uartclk / 16 / div_16);
> +
> +	if (abs_d13 >= abs_d16) {
> +		*frac = PRUSS_UART_MDR_16X_MODE;
> +		quot = div_16;
> +	} else {
> +		*frac = PRUSS_UART_MDR_13X_MODE;
> +		quot = div_13;
> +	}
> +
> +	return quot;

Are you sure it doesn't repeat existing things from other driver(s)?

> +}

...

> +static int pruss8250_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;

You don't need this.

> +	struct uart_8250_port port8250;
> +	struct uart_port *up = &port8250.port;
> +	struct pruss8250_info *info;
> +	struct resource resource;
> +	unsigned int port_type;
> +	struct clk *clk;
> +	int ret;
> +
> +	port_type = (unsigned long)of_device_get_match_data(&pdev->dev);
> +	if (port_type == PORT_UNKNOWN)
> +		return -EINVAL;
> +
> +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	memset(&port8250, 0, sizeof(port8250));
> +
> +	ret = of_address_to_resource(np, 0, &resource);

Yeah, no modifications from 2021?

> +	if (ret) {
> +		dev_err(&pdev->dev, "invalid address\n");
> +		return ret;
> +	}

> +	ret = of_alias_get_id(np, "serial");
> +	if (ret > 0)
> +		up->line = ret;

Use uart_read_port_properties() instead of this and other related code.

> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk)) {
> +		if (PTR_ERR(clk) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;

We have other errors which are effectively being ignored here, why?

> +		up->uartclk = DEFAULT_CLK_SPEED;
> +	} else {
> +		up->uartclk = clk_get_rate(clk);

> +		devm_clk_put(&pdev->dev, clk);

Why? Maybe you should not to try devm_ to begin with?

> +	}
> +
> +	up->dev = &pdev->dev;
> +	up->mapbase = resource.start;
> +	up->mapsize = resource_size(&resource);
> +	up->type = port_type;
> +	up->iotype = UPIO_MEM;
> +	up->regshift = 2;
> +	up->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_FIXED_PORT |
> +		    UPF_FIXED_TYPE | UPF_IOREMAP;
> +	up->irqflags |= IRQF_SHARED;
> +	up->startup = pruss8250_startup;
> +	up->rs485_config = serial8250_em485_config;
> +	up->get_divisor = pruss8250_get_divisor;
> +	up->set_divisor = pruss8250_set_divisor;
> +
> +	ret = of_irq_get(np, 0);
> +	if (ret < 0) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "missing irq\n");
> +		return ret;
> +	}

A lot of this will be simplified by using the above mentioned API.

> +	up->irq = ret;
> +	spin_lock_init(&port8250.port.lock);
> +	port8250.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
> +
> +	ret = serial8250_register_8250_port(&port8250);
> +	if (ret < 0)
> +		goto err_dispose;
> +
> +	info->type = port_type;
> +	info->line = ret;
> +	platform_set_drvdata(pdev, info);
> +
> +	return 0;

> +err_dispose:
> +	irq_dispose_mapping(port8250.port.irq);

Why this is needed?

> +	return ret;
> +}

...

> +static const struct of_device_id pruss8250_table[] = {
> +	{ .compatible = "ti,pruss-uart", .data = (void *)PORT_16550A, },

Inner comma is redundant.

> +	{ /* end of list */ },

No trailing comma in the terminator.

> +};

...

> +config SERIAL_8250_PRUSS
> +	tristate "TI PRU-ICSS UART support"
> +	depends on SERIAL_8250

> +	depends on PRU_REMOTEPROC && TI_PRUSS_INTC

No COMPILE_TEST?

> +	help
> +	  This driver is to support the UART module in PRU-ICSS which is
> +	  available in some TI platforms.
> +	  Say 'Y' here if you wish to use PRU-ICSS UART.
> +	  Otherwise, say 'N'.

If m, how would the module be called? See many new driver examples in Kconfigs
(in particular IIO follows this pattern).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 0/2] Introduce PRU UART driver

[Andy Shevchenko]

On Thu, May 01, 2025 at 09:47:34AM -0500, Judith Mendez wrote:
> On 5/1/25 12:18 AM, Greg Kroah-Hartman wrote:
> > On Wed, Apr 30, 2025 at 07:31:11PM -0500, Judith Mendez wrote:
> > > This patch series is sent as an RFC to get some initial comments
> > > on the PRU UART driver.
> > > 
> > > The ICSSM modules on am64x SoC and the PRUSS module on am62 SoC or am335x
> > > SoCs have a UART sub-module. This patch series introduces the driver and the
> > > corresponding binding documentation for this sub-module.
> > > 
> > > The DTS patches for adding PRU nodes and enabling PRU UART will be added
> > > in a later v1 version of the series if accepted.
> > > 
> > > This driver has been previously tested on the following boards:
> > > am64x SK, am62x SK, and am335x SK boards.
> > 
> > Why is this "RFC"?  What needs to be done to make it something that you
> > actually feel works properly and should be merged?
> 
> Nothing needs to be done IMO, the only reason it was sent as an RFC is
> to get initial thoughts/issues that anyone might have with the driver
> before sending v1.
> 
> If none, I will go ahead and send v1. Thanks for your attention Greg.

I have tons of comments, please read my replies before sending a v1.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 0/2] Introduce PRU UART driver

[Judith Mendez]

Hi Andy,

On 5/2/25 4:51 AM, Andy Shevchenko wrote:
> On Thu, May 01, 2025 at 09:47:34AM -0500, Judith Mendez wrote:
>> On 5/1/25 12:18 AM, Greg Kroah-Hartman wrote:
>>> On Wed, Apr 30, 2025 at 07:31:11PM -0500, Judith Mendez wrote:
>>>> This patch series is sent as an RFC to get some initial comments
>>>> on the PRU UART driver.
>>>>
>>>> The ICSSM modules on am64x SoC and the PRUSS module on am62 SoC or am335x
>>>> SoCs have a UART sub-module. This patch series introduces the driver and the
>>>> corresponding binding documentation for this sub-module.
>>>>
>>>> The DTS patches for adding PRU nodes and enabling PRU UART will be added
>>>> in a later v1 version of the series if accepted.
>>>>
>>>> This driver has been previously tested on the following boards:
>>>> am64x SK, am62x SK, and am335x SK boards.
>>>
>>> Why is this "RFC"?  What needs to be done to make it something that you
>>> actually feel works properly and should be merged?
>>
>> Nothing needs to be done IMO, the only reason it was sent as an RFC is
>> to get initial thoughts/issues that anyone might have with the driver
>> before sending v1.
>>
>> If none, I will go ahead and send v1. Thanks for your attention Greg.
> 
> I have tons of comments, please read my replies before sending a v1.

Thanks for your comments, I need a day or so to review and then respond
to your comments and Andrews.

Judith

> 

Re: [PATCH RFC 0/2] Introduce PRU UART driver

[Judith Mendez]

Hi Andy,

On 5/2/25 4:38 AM, Andy Shevchenko wrote:
> On Wed, Apr 30, 2025 at 07:31:11PM -0500, Judith Mendez wrote:
>> This patch series is sent as an RFC to get some initial comments
>> on the PRU UART driver.
>>
>> The ICSSM modules on am64x SoC and the PRUSS module on am62 SoC or am335x
>> SoCs have a UART sub-module. This patch series introduces the driver and the
>> corresponding binding documentation for this sub-module.
>>
>> The DTS patches for adding PRU nodes and enabling PRU UART will be added
>> in a later v1 version of the series if accepted.
>>
>> This driver has been previously tested on the following boards:
>> am64x SK, am62x SK, and am335x SK boards.
> 
> The first and main question here: Have you checked the existing zillion of
> drivers and become with an idea that none of them not even close to this one
> (based on RTL)?
> 

Thanks for reviewing.

I have looked through the drivers in tty/serial/8250 and I believe there
are no drivers we could leverage, especially since this driver has a
dependency on the PRU drivers for clock and power. This PORT_16550A UART
is in PRU module which is specific to TI, so besides simplifying the
driver a bit further, I don't think we can use another driver.

~ Judith

Re: [PATCH RFC 2/2] serial: 8250: Add PRUSS UART driver

[Judith Mendez]

Hi Andy

On 5/2/25 4:50 AM, Andy Shevchenko wrote:
> On Wed, Apr 30, 2025 at 07:31:13PM -0500, Judith Mendez wrote:
>> From: Bin Liu <b-liu@ti.com>
>>
>> This adds a new serial 8250 driver that supports the UART in PRUSS
>> module.
>>
>> The PRUSS has a UART sub-module which is based on the industry standard
>> TL16C550 UART controller, which has 16-bytes FIFO and supports 16x and
>> 13x over samplings.
> 
>> Signed-off-by: Bin Liu <b-liu@ti.com>
>> Signed-off-by: Judith Mendez <jm@ti.com>
> 
> Are you just a committer and not a co-developer of this code?

Commiter only. I am only carrying the driver across kernel version,
fixing merge conflicts, testing the driver, and now up-streaming it.

> 
> ...
> 
>> +/*
>> + *  Serial Port driver for PRUSS UART on TI platforms
>> + *
>> + *  Copyright (C) 2020-2021 by Texas Instruments Incorporated - http://www.ti.com/
> 
> My calendar shows 2025...
> 
>> + *  Author: Bin Liu <b-liu@ti.com>
>> + */
> 
> ...
> 
> The list of the inclusions is semi-random. Please, follow the IWYU principle.
> 
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/serial_reg.h>
>> +#include <linux/serial_core.h>
> 
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
> 
> Please, no of*.h in a new code.

Will only keep linux/of_platform.h for of_device_id struct.

> 
>> +#include <linux/remoteproc.h>
> 
> Keep them ordered as well.
> 
> + blank line here.
> 
>> +#include "8250.h"
> 
> ...
> 
>> +#define DEFAULT_CLK_SPEED	192000000
> 
> Units as a suffix? HZ_PER_MHZ?
> You can also fix 8250_omap in the same way.
> 
> ...
> 
>> +static inline void uart_writel(struct uart_port *p, u32 offset, int value)
>> +{
>> +	writel(value, p->membase + (offset << p->regshift));
>> +}
> 
> Why? Or how does it differ from the ones that serial core provides?
> 
> ...
> 
>> +static int pruss8250_startup(struct uart_port *port)
>> +{
>> +	int ret;
>> +
>> +	uart_writel(port, PRUSS_UART_PEREMU_MGMT, 0);
>> +
>> +	ret = serial8250_do_startup(port);
>> +	if (!ret)
> 
> Please, use usual pattern, check for errors first
> 
> 	if (ret)
> 		return ret;
> 	...
> 	return 0;
> 
>> +		uart_writel(port, PRUSS_UART_PEREMU_MGMT, PRUSS_UART_TX_EN |
>> +							  PRUSS_UART_RX_EN |
>> +							  PRUSS_UART_FREE_RUN);
>> +	return ret;
>> +}
> 
> ...
> 
>> +static unsigned int pruss8250_get_divisor(struct uart_port *port,
>> +					  unsigned int baud,
>> +					  unsigned int *frac)
>> +{
>> +	unsigned int uartclk = port->uartclk;
>> +	unsigned int div_13, div_16;
>> +	unsigned int abs_d13, abs_d16;
>> +	u16 quot;
> 
>> +	/* Old custom speed handling */
>> +	if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
>> +		quot = port->custom_divisor & UART_DIV_MAX;
>> +		if (port->custom_divisor & (1 << 16))
>> +			*frac = PRUSS_UART_MDR_13X_MODE;
>> +		else
>> +			*frac = PRUSS_UART_MDR_16X_MODE;
>> +
>> +		return quot;
>> +	}
> 
> Why?! Please, try to avoid adding more drivers with this ugly hack.

My understanding is that this is not a hack, for 38400 we need to pass
as custom baud. What is the alternative here?

I see other drivers are doing this as well, will look into this further
but not sure if there is a better solution for this.

> 
>> +	div_13 = DIV_ROUND_CLOSEST(uartclk, 13 * baud);
>> +	div_16 = DIV_ROUND_CLOSEST(uartclk, 16 * baud);
>> +	div_13 = div_13 ? : 1;
>> +	div_16 = div_16 ? : 1;
>> +
>> +	abs_d13 = abs(baud - uartclk / 13 / div_13);
>> +	abs_d16 = abs(baud - uartclk / 16 / div_16);
>> +
>> +	if (abs_d13 >= abs_d16) {
>> +		*frac = PRUSS_UART_MDR_16X_MODE;
>> +		quot = div_16;
>> +	} else {
>> +		*frac = PRUSS_UART_MDR_13X_MODE;
>> +		quot = div_13;
>> +	}
>> +
>> +	return quot;
> 
> Are you sure it doesn't repeat existing things from other driver(s)?

core driver does this differently so we cant use that.

> 
>> +}
> 
> ...
> 
>> +static int pruss8250_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
> 
> You don't need this.
> 
>> +	struct uart_8250_port port8250;
>> +	struct uart_port *up = &port8250.port;
>> +	struct pruss8250_info *info;
>> +	struct resource resource;
>> +	unsigned int port_type;
>> +	struct clk *clk;
>> +	int ret;
>> +
>> +	port_type = (unsigned long)of_device_get_match_data(&pdev->dev);
>> +	if (port_type == PORT_UNKNOWN)
>> +		return -EINVAL;
>> +
>> +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
>> +	if (!info)
>> +		return -ENOMEM;
>> +
>> +	memset(&port8250, 0, sizeof(port8250));
>> +
>> +	ret = of_address_to_resource(np, 0, &resource);
> 
> Yeah, no modifications from 2021?
> 
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "invalid address\n");
>> +		return ret;
>> +	}
> 
>> +	ret = of_alias_get_id(np, "serial");
>> +	if (ret > 0)
>> +		up->line = ret;
> 
> Use uart_read_port_properties() instead of this and other related code.
> 
>> +	clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(clk)) {
>> +		if (PTR_ERR(clk) == -EPROBE_DEFER)
>> +			return -EPROBE_DEFER;
> 
> We have other errors which are effectively being ignored here, why?
> 
>> +		up->uartclk = DEFAULT_CLK_SPEED;
>> +	} else {
>> +		up->uartclk = clk_get_rate(clk);
> 
>> +		devm_clk_put(&pdev->dev, clk);
> 
> Why? Maybe you should not to try devm_ to begin with?
> 
>> +	}
>> +
>> +	up->dev = &pdev->dev;
>> +	up->mapbase = resource.start;
>> +	up->mapsize = resource_size(&resource);
>> +	up->type = port_type;
>> +	up->iotype = UPIO_MEM;
>> +	up->regshift = 2;
>> +	up->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_FIXED_PORT |
>> +		    UPF_FIXED_TYPE | UPF_IOREMAP;
>> +	up->irqflags |= IRQF_SHARED;
>> +	up->startup = pruss8250_startup;
>> +	up->rs485_config = serial8250_em485_config;
>> +	up->get_divisor = pruss8250_get_divisor;
>> +	up->set_divisor = pruss8250_set_divisor;
>> +
>> +	ret = of_irq_get(np, 0);
>> +	if (ret < 0) {
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(&pdev->dev, "missing irq\n");
>> +		return ret;
>> +	}
> 
> A lot of this will be simplified by using the above mentioned API.
> 
>> +	up->irq = ret;
>> +	spin_lock_init(&port8250.port.lock);
>> +	port8250.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
>> +
>> +	ret = serial8250_register_8250_port(&port8250);
>> +	if (ret < 0)
>> +		goto err_dispose;
>> +
>> +	info->type = port_type;
>> +	info->line = ret;
>> +	platform_set_drvdata(pdev, info);
>> +
>> +	return 0;
> 
>> +err_dispose:
>> +	irq_dispose_mapping(port8250.port.irq);
> 
> Why this is needed?

No longer needed will remove

> 
>> +	return ret;
>> +}
> 
> ...
> 
>> +static const struct of_device_id pruss8250_table[] = {
>> +	{ .compatible = "ti,pruss-uart", .data = (void *)PORT_16550A, },
> 
> Inner comma is redundant.
> 
>> +	{ /* end of list */ },
> 
> No trailing comma in the terminator.
> 
>> +};
> 
> ...
> 
>> +config SERIAL_8250_PRUSS
>> +	tristate "TI PRU-ICSS UART support"
>> +	depends on SERIAL_8250
> 
>> +	depends on PRU_REMOTEPROC && TI_PRUSS_INTC
> 
> No COMPILE_TEST?
> 
>> +	help
>> +	  This driver is to support the UART module in PRU-ICSS which is
>> +	  available in some TI platforms.
>> +	  Say 'Y' here if you wish to use PRU-ICSS UART.
>> +	  Otherwise, say 'N'.
> 
> If m, how would the module be called? See many new driver examples in Kconfigs
> (in particular IIO follows this pattern).
> 

All other comments I will fix for v1, thanks for reviewing.

~ Judith

Re: [PATCH RFC 2/2] serial: 8250: Add PRUSS UART driver

[Judith Mendez]

Hi Andrew,

On 5/1/25 11:28 AM, Andrew Davis wrote:
> On 4/30/25 7:31 PM, Judith Mendez wrote:
>> From: Bin Liu <b-liu@ti.com>
>>
>> This adds a new serial 8250 driver that supports the UART in PRUSS
>> module.
>>
>> The PRUSS has a UART sub-module which is based on the industry standard
>> TL16C550 UART controller, which has 16-bytes FIFO and supports 16x and
>> 13x over samplings.
>>
>> Signed-off-by: Bin Liu <b-liu@ti.com>
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/tty/serial/8250/8250_pruss.c | 213 +++++++++++++++++++++++++++
>>   drivers/tty/serial/8250/Kconfig      |  10 ++
>>   drivers/tty/serial/8250/Makefile     |   1 +
>>   3 files changed, 224 insertions(+)
>>   create mode 100644 drivers/tty/serial/8250/8250_pruss.c
>>
>> diff --git a/drivers/tty/serial/8250/8250_pruss.c 
>> b/drivers/tty/serial/8250/8250_pruss.c
>> new file mode 100644
>> index 000000000000..2943bf7d6645
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_pruss.c
>> @@ -0,0 +1,213 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + *  Serial Port driver for PRUSS UART on TI platforms
>> + *
>> + *  Copyright (C) 2020-2021 by Texas Instruments Incorporated - 
>> http://www.ti.com/
>> + *  Author: Bin Liu <b-liu@ti.com>
>> + */
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/serial_reg.h>
>> +#include <linux/serial_core.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/remoteproc.h>
>> +#include "8250.h"
>> +
>> +#define DEFAULT_CLK_SPEED    192000000
>> +
>> +/* extra registers */
>> +#define PRUSS_UART_PEREMU_MGMT    12
>> +#define PRUSS_UART_TX_EN    BIT(14)
>> +#define PRUSS_UART_RX_EN    BIT(13)
>> +#define PRUSS_UART_FREE_RUN    BIT(0)
>> +
>> +#define PRUSS_UART_MDR            13
>> +#define PRUSS_UART_MDR_OSM_SEL_MASK    BIT(0)
>> +#define PRUSS_UART_MDR_16X_MODE        0
>> +#define PRUSS_UART_MDR_13X_MODE        1
>> +
>> +struct pruss8250_info {
>> +    int type;
> 
> You never use type, why store it here?

removed

> 
>> +    int line;
>> +};
>> +
>> +static inline void uart_writel(struct uart_port *p, u32 offset, int 
>> value)
>> +{
>> +    writel(value, p->membase + (offset << p->regshift));
>> +}
>> +
>> +static int pruss8250_startup(struct uart_port *port)
>> +{
>> +    int ret;
>> +
>> +    uart_writel(port, PRUSS_UART_PEREMU_MGMT, 0);
>> +
>> +    ret = serial8250_do_startup(port);
>> +    if (!ret)
>> +        uart_writel(port, PRUSS_UART_PEREMU_MGMT, PRUSS_UART_TX_EN |
>> +                              PRUSS_UART_RX_EN |
>> +                              PRUSS_UART_FREE_RUN);
>> +    return ret;
>> +}
>> +
>> +static unsigned int pruss8250_get_divisor(struct uart_port *port,
>> +                      unsigned int baud,
>> +                      unsigned int *frac)
>> +{
>> +    unsigned int uartclk = port->uartclk;
>> +    unsigned int div_13, div_16;
>> +    unsigned int abs_d13, abs_d16;
>> +    u16 quot;
>> +
>> +    /* Old custom speed handling */
>> +    if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
>> +        quot = port->custom_divisor & UART_DIV_MAX;
>> +        if (port->custom_divisor & (1 << 16))
>> +            *frac = PRUSS_UART_MDR_13X_MODE;
>> +        else
>> +            *frac = PRUSS_UART_MDR_16X_MODE;
>> +
>> +        return quot;
>> +    }
>> +
>> +    div_13 = DIV_ROUND_CLOSEST(uartclk, 13 * baud);
>> +    div_16 = DIV_ROUND_CLOSEST(uartclk, 16 * baud);
>> +    div_13 = div_13 ? : 1;
>> +    div_16 = div_16 ? : 1;
>> +
>> +    abs_d13 = abs(baud - uartclk / 13 / div_13);
>> +    abs_d16 = abs(baud - uartclk / 16 / div_16);
>> +
>> +    if (abs_d13 >= abs_d16) {
>> +        *frac = PRUSS_UART_MDR_16X_MODE;
>> +        quot = div_16;
>> +    } else {
>> +        *frac = PRUSS_UART_MDR_13X_MODE;
>> +        quot = div_13;
>> +    }
>> +
>> +    return quot;
>> +}
>> +
>> +static void pruss8250_set_divisor(struct uart_port *port, unsigned 
>> int baud,
>> +                  unsigned int quot, unsigned int quot_frac)
>> +{
>> +    serial8250_do_set_divisor(port, baud, quot);
>> +    /*
>> +     * quot_frac holds the MDR over-sampling mode
>> +     * which is set in pruss8250_get_divisor()
>> +     */
>> +    quot_frac &= PRUSS_UART_MDR_OSM_SEL_MASK;
>> +    serial_port_out(port, PRUSS_UART_MDR, quot_frac);
>> +}
>> +
>> +static int pruss8250_probe(struct platform_device *pdev)
>> +{
>> +    struct device_node *np = pdev->dev.of_node;
>> +    struct uart_8250_port port8250;
>> +    struct uart_port *up = &port8250.port;
>> +    struct pruss8250_info *info;
>> +    struct resource resource;
>> +    unsigned int port_type;
>> +    struct clk *clk;
>> +    int ret;
>> +
>> +    port_type = (unsigned long)of_device_get_match_data(&pdev->dev);
> 
> Will the port type ever not be PORT_16550A?

So far each UART module is has been the same, I think it might be safe
to hardcode:

port->type = PORT_16550A;


> 
>> +    if (port_type == PORT_UNKNOWN)
>> +        return -EINVAL;
>> +
>> +    info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
>> +    if (!info)
>> +        return -ENOMEM;
>> +
>> +    memset(&port8250, 0, sizeof(port8250));
>> +
>> +    ret = of_address_to_resource(np, 0, &resource);
> 
> platform_get_resource()

Will add

> 
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "invalid address\n");
>> +        return ret;
>> +    }
>> +
>> +    ret = of_alias_get_id(np, "serial");
> 
> Can you make use of the uart_read_port_properties() helper here?

yes we can,
thanks for reviewing.

~ Judith


> 
> Andrew
> 
>> +    if (ret > 0)
>> +        up->line = ret;
>> +
>> +    clk = devm_clk_get(&pdev->dev, NULL);
>> +    if (IS_ERR(clk)) {
>> +        if (PTR_ERR(clk) == -EPROBE_DEFER)
>> +            return -EPROBE_DEFER;
>> +        up->uartclk = DEFAULT_CLK_SPEED;
>> +    } else {
>> +        up->uartclk = clk_get_rate(clk);
>> +        devm_clk_put(&pdev->dev, clk);
>> +    }
>> +
>> +    up->dev = &pdev->dev;
>> +    up->mapbase = resource.start;
>> +    up->mapsize = resource_size(&resource);
>> +    up->type = port_type;
>> +    up->iotype = UPIO_MEM;
>> +    up->regshift = 2;
>> +    up->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_FIXED_PORT |
>> +            UPF_FIXED_TYPE | UPF_IOREMAP;
>> +    up->irqflags |= IRQF_SHARED;
>> +    up->startup = pruss8250_startup;
>> +    up->rs485_config = serial8250_em485_config;
>> +    up->get_divisor = pruss8250_get_divisor;
>> +    up->set_divisor = pruss8250_set_divisor;
>> +
>> +    ret = of_irq_get(np, 0);
>> +    if (ret < 0) {
>> +        if (ret != -EPROBE_DEFER)
>> +            dev_err(&pdev->dev, "missing irq\n");
>> +        return ret;
>> +    }
>> +
>> +    up->irq = ret;
>> +    spin_lock_init(&port8250.port.lock);
>> +    port8250.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
>> +
>> +    ret = serial8250_register_8250_port(&port8250);
>> +    if (ret < 0)
>> +        goto err_dispose;
>> +
>> +    info->type = port_type;
>> +    info->line = ret;
>> +    platform_set_drvdata(pdev, info);
>> +
>> +    return 0;
>> +
>> +err_dispose:
>> +    irq_dispose_mapping(port8250.port.irq);
>> +    return ret;
>> +}
>> +
>> +static void pruss8250_remove(struct platform_device *pdev)
>> +{
>> +    struct pruss8250_info *info = platform_get_drvdata(pdev);
>> +
>> +    serial8250_unregister_port(info->line);
>> +}
>> +
>> +static const struct of_device_id pruss8250_table[] = {
>> +    { .compatible = "ti,pruss-uart", .data = (void *)PORT_16550A, },
>> +    { /* end of list */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, pruss8250_table);
>> +
>> +static struct platform_driver pruss8250_driver = {
>> +    .driver = {
>> +        .name = "pruss8250",
>> +        .of_match_table = pruss8250_table,
>> +    },
>> +    .probe = pruss8250_probe,
>> +    .remove = pruss8250_remove,
>> +};
>> +
>> +module_platform_driver(pruss8250_driver);
>> +
>> +MODULE_AUTHOR("Bin Liu <b-liu@ti.com");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("Serial Port driver for PRUSS UART on TI platforms");
>> diff --git a/drivers/tty/serial/8250/Kconfig 
>> b/drivers/tty/serial/8250/Kconfig
>> index f64ef0819cd4..cd4346609c55 100644
>> --- a/drivers/tty/serial/8250/Kconfig
>> +++ b/drivers/tty/serial/8250/Kconfig
>> @@ -582,6 +582,16 @@ config SERIAL_8250_NI
>>         To compile this driver as a module, choose M here: the module
>>         will be called 8250_ni.
>> +config SERIAL_8250_PRUSS
>> +    tristate "TI PRU-ICSS UART support"
>> +    depends on SERIAL_8250
>> +    depends on PRU_REMOTEPROC && TI_PRUSS_INTC
>> +    help
>> +      This driver is to support the UART module in PRU-ICSS which is
>> +      available in some TI platforms.
>> +      Say 'Y' here if you wish to use PRU-ICSS UART.
>> +      Otherwise, say 'N'.
>> +
>>   config SERIAL_OF_PLATFORM
>>       tristate "Devicetree based probing for 8250 ports"
>>       depends on SERIAL_8250 && OF
>> diff --git a/drivers/tty/serial/8250/Makefile 
>> b/drivers/tty/serial/8250/Makefile
>> index b04eeda03b23..3132b4f40a34 100644
>> --- a/drivers/tty/serial/8250/Makefile
>> +++ b/drivers/tty/serial/8250/Makefile
>> @@ -47,6 +47,7 @@ obj-$(CONFIG_SERIAL_8250_PARISC)    += 8250_parisc.o
>>   obj-$(CONFIG_SERIAL_8250_PCI)        += 8250_pci.o
>>   obj-$(CONFIG_SERIAL_8250_PCI1XXXX)    += 8250_pci1xxxx.o
>>   obj-$(CONFIG_SERIAL_8250_PERICOM)    += 8250_pericom.o
>> +obj-$(CONFIG_SERIAL_8250_PRUSS)        += 8250_pruss.o
>>   obj-$(CONFIG_SERIAL_8250_PXA)        += 8250_pxa.o
>>   obj-$(CONFIG_SERIAL_8250_RT288X)    += 8250_rt288x.o
>>   obj-$(CONFIG_SERIAL_8250_CS)        += serial_cs.o

Re: [PATCH RFC 2/2] serial: 8250: Add PRUSS UART driver

[Andy Shevchenko]

On Thu, May 08, 2025 at 05:09:03PM -0500, Judith Mendez wrote:
> On 5/2/25 4:50 AM, Andy Shevchenko wrote:
> > On Wed, Apr 30, 2025 at 07:31:13PM -0500, Judith Mendez wrote:

...

> > The list of the inclusions is semi-random. Please, follow the IWYU principle.

This and other comments left unanswered, why? What does this mean? Usual way is
to remove the context with all what you are agree on and discuss only stuff
that needs more elaboration.

Ah, I see now the P.S., but please also remove the context you agree with next
time.

> > > +#include <linux/clk.h>
> > > +#include <linux/module.h>
> > > +#include <linux/serial_reg.h>
> > > +#include <linux/serial_core.h>
> > 
> > > +#include <linux/of_irq.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_platform.h>
> > 
> > Please, no of*.h in a new code.
> 
> Will only keep linux/of_platform.h for of_device_id struct.

Hmm... Is it really the header where it's defined? (I know the answer,
but want you to perform some research.)

> > > +#include <linux/remoteproc.h>
> > 
> > Keep them ordered as well.
> > 
> > + blank line here.
> > 
> > > +#include "8250.h"

...

> > > +	/* Old custom speed handling */
> > > +	if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
> > > +		quot = port->custom_divisor & UART_DIV_MAX;
> > > +		if (port->custom_divisor & (1 << 16))
> > > +			*frac = PRUSS_UART_MDR_13X_MODE;
> > > +		else
> > > +			*frac = PRUSS_UART_MDR_16X_MODE;
> > > +
> > > +		return quot;
> > > +	}
> > 
> > Why?! Please, try to avoid adding more drivers with this ugly hack.
> 
> My understanding is that this is not a hack, for 38400 we need to pass
> as custom baud. What is the alternative here?

BOTHER. The 38400 is a hack, you lie to the stakeholders that you are at 38.4,
while in real life it means anything.

> I see other drivers are doing this as well, will look into this further
> but not sure if there is a better solution for this.

BOTHER is the solution. Not perfect, but the existing one.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 2/2] serial: 8250: Add PRUSS UART driver

[Judith Mendez]

Hi Andy,

On 5/9/25 6:43 AM, Andy Shevchenko wrote:
> On Thu, May 08, 2025 at 05:09:03PM -0500, Judith Mendez wrote:
>> On 5/2/25 4:50 AM, Andy Shevchenko wrote:
>>> On Wed, Apr 30, 2025 at 07:31:13PM -0500, Judith Mendez wrote:

...
>>>> +	/* Old custom speed handling */
>>>> +	if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
>>>> +		quot = port->custom_divisor & UART_DIV_MAX;
>>>> +		if (port->custom_divisor & (1 << 16))
>>>> +			*frac = PRUSS_UART_MDR_13X_MODE;
>>>> +		else
>>>> +			*frac = PRUSS_UART_MDR_16X_MODE;
>>>> +
>>>> +		return quot;
>>>> +	}
>>>
>>> Why?! Please, try to avoid adding more drivers with this ugly hack.
>>
>> My understanding is that this is not a hack, for 38400 we need to pass
>> as custom baud. What is the alternative here?
> 
> BOTHER. The 38400 is a hack, you lie to the stakeholders that you are at 38.4,
> while in real life it means anything.
> 
>> I see other drivers are doing this as well, will look into this further
>> but not sure if there is a better solution for this.
> 
> BOTHER is the solution. Not perfect, but the existing one.


Thanks for the hint, I have removed the hack from the driver and will be
sending v1. thanks.

~ Judith