linux-kernel - Re: [PATCH 3/7] serial: 8250: Add PRUSS UART driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aCSKzFHGl5ua3rrP@smile.fi.intel.com>
Date: Wed, 14 May 2025 15:21:32 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Judith Mendez <jm@...com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Nishanth Menon <nm@...com>,
	Santosh Shilimkar <ssantosh@...nel.org>,
	Vignesh Raghavendra <vigneshr@...com>,
	Tero Kristo <kristo@...nel.org>, Bin Liu <b-liu@...com>,
	Andrew Davis <afd@...com>, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 3/7] serial: 8250: Add PRUSS UART driver

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

...

+ bits.h

> +#include <linux/clk.h>

+ math.h

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

Can you keep them sorted?



...

> +static int pruss8250_startup(struct uart_port *port)
> +{
> +	int ret;
> +
> +	port->serial_out(port, PRUSS_UART_PEREMU_MGMT, 0);
> +
> +	ret = serial8250_do_startup(port);

Please, use standard pattern, i.e.

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

I believe I have told this previously. Can you double check that you read and
addressed all of the comments?

> +	if (!ret)
> +		port->serial_out(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;
> +
> +	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) {

Isn't this something like abs_diff() ?

> +		*frac = PRUSS_UART_MDR_16X_MODE;
> +		quot = div_16;
> +	} else {
> +		*frac = PRUSS_UART_MDR_13X_MODE;
> +		quot = div_13;
> +	}
> +
> +	return quot;
> +}

> +static int pruss8250_probe(struct platform_device *pdev)
> +{
> +	struct uart_8250_port port8250;
> +	struct uart_port *port = &port8250.port;
> +	struct device *dev = &pdev->dev;
> +	struct pruss8250_data *data;
> +	struct resource *res;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);

Needs device/devres.h.

> +	if (!data)
> +		return -ENOMEM;

Needs err.h (actually errno.h, but that's not enough for the following IS_ERR()
et al.)

> +	memset(&port8250, 0, sizeof(port8250));

Instead of having dependency on string.h (which is missed) just assign it to {}
in the definition.

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get resource");
> +		return -EINVAL;

	return dev_err_probe();

> +	}

> +	if (!port->uartclk) {
> +		data->clk = devm_clk_get(dev, NULL);
> +		if (IS_ERR(data->clk)) {
> +			dev_err(dev, "Failed to get clock!\n");
> +			return -ENODEV;
> +		} else {
> +			port->uartclk = clk_get_rate(data->clk);
> +			devm_clk_put(dev, data->clk);

I think you completely ignored my review I have done in previous version...

> +		}
> +	}

Should be done after uart_read_properties().

> +	port->dev = dev;
> +	port->mapbase = res->start;
> +	port->mapsize = resource_size(res);
> +	port->type = PORT_16550A;
> +	port->flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_FIXED_TYPE |
> +		      UPF_IOREMAP;
> +	port->startup = pruss8250_startup;
> +	port->rs485_config = serial8250_em485_config;
> +	port->get_divisor = pruss8250_get_divisor;
> +	port->set_divisor = pruss8250_set_divisor;
> +
> +	ret = uart_read_port_properties(port);
> +	if (ret)
> +		return ret;
> +
> +	port->iotype = UPIO_MEM32;
> +	port->regshift = 2;
> +
> +	spin_lock_init(&port8250.port.lock);
> +	port8250.capabilities = UART_CAP_FIFO | UART_CAP_AFE;
> +
> +	ret = serial8250_register_8250_port(&port8250);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Unable to register 8250 port.\n");
> +
> +	data->line = ret;
> +	platform_set_drvdata(pdev, data);
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko