diff mbox series

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

Message ID 20250501003113.1609342-3-jm@ti.com
State New
Headers show
Series Introduce PRU UART driver | expand

Commit Message

Judith Mendez May 1, 2025, 12:31 a.m. UTC 
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

Comments

Andy Shevchenko May 2, 2025, 9:50 a.m. UTC | #1 
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).
Judith Mendez May 8, 2025, 10:09 p.m. UTC | #2 
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
Judith Mendez May 8, 2025, 10:11 p.m. UTC | #3 
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
Andy Shevchenko May 9, 2025, 11:43 a.m. UTC | #4 
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.
diff mbox series

Patch

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