serial: 8250_platform: Enable generic 16550A platform devices

[PATCH] serial: 8250_platform: Enable generic 16550A platform devices

Posted by Sunil V L 1 year, 4 months ago

Currently, 8250_platform driver is used only for devices with fixed
serial ports (plat_serial8250_port). Extend this driver for any generic
16550A platform devices which can be probed using standard hardware
discovery mechanisms like ACPI.

This is required in particular for RISC-V which has non-PNP generic
16550A compatible UART that needs to be enumerated as ACPI platform
device.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 drivers/tty/serial/8250/8250_platform.c | 77 +++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index d5c8d851348d..bdfb16bed4f2 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -6,7 +6,9 @@
  *	      PNP 8250/16550 ports
  *	      "serial8250" platform devices
  */
+#include <linux/acpi.h>
 #include <linux/array_size.h>
+#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/once.h>
@@ -100,6 +102,65 @@ void __init serial8250_isa_init_ports(void)
 	DO_ONCE(__serial8250_isa_init_ports);
 }
 
+/*
+ * Generic 16550A platform devices
+ */
+static int serial8250_platform_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct uart_8250_port uart = { 0 };
+	struct resource *regs;
+	unsigned char iotype;
+	int ret, line;
+
+	regs = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (regs) {
+		uart.port.iobase = regs->start;
+		iotype = UPIO_PORT;
+	} else {
+		regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!regs) {
+			dev_err(dev, "no registers defined\n");
+			return -EINVAL;
+		}
+
+		uart.port.mapbase = regs->start;
+		uart.port.mapsize = resource_size(regs);
+		uart.port.flags = UPF_IOREMAP;
+		iotype = UPIO_MEM;
+	}
+
+	/* Default clock frequency*/
+	uart.port.uartclk = 1843200;
+	uart.port.type = PORT_16550A;
+	uart.port.dev = &pdev->dev;
+	uart.port.flags |= UPF_SKIP_TEST | UPF_BOOT_AUTOCONF;
+	ret = uart_read_and_validate_port_properties(&uart.port);
+	/* no interrupt -> fall back to polling */
+	if (ret == -ENXIO)
+		ret = 0;
+	if (ret)
+		return ret;
+
+	if (uart.port.mapbase) {
+		uart.port.membase = devm_ioremap(dev, uart.port.mapbase, uart.port.mapsize);
+		if (!uart.port.membase)
+			return -ENOMEM;
+	}
+
+	/*
+	 * The previous call may not set iotype correctly when reg-io-width
+	 * property is absent and it doesn't support IO port resource.
+	 */
+	uart.port.iotype = iotype;
+
+	line = serial8250_register_8250_port(&uart);
+	if (line < 0)
+		return -ENODEV;
+
+	return 0;
+}
+
 /*
  * Register a set of serial devices attached to a platform device.  The
  * list is terminated with a zero flags entry, which means we expect
@@ -110,6 +171,15 @@ static int serial8250_probe(struct platform_device *dev)
 	struct plat_serial8250_port *p = dev_get_platdata(&dev->dev);
 	struct uart_8250_port uart;
 	int ret, i, irqflag = 0;
+	struct fwnode_handle *fwnode = dev_fwnode(&dev->dev);
+
+	/*
+	 * Probe platform UART devices defined using standard hardware
+	 * discovery mechanism like ACPI or DT. Support only ACPI based
+	 * serial device for now.
+	 */
+	if (!p && is_acpi_node(fwnode))
+		return serial8250_platform_probe(dev);
 
 	memset(&uart, 0, sizeof(uart));
 
@@ -198,6 +268,12 @@ static int serial8250_resume(struct platform_device *dev)
 	return 0;
 }
 
+static const struct acpi_device_id acpi_platform_serial_table[] = {
+	{ "RSCV0003", 0 }, // RISC-V Generic 16550A UART
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, acpi_platform_serial_table);
+
 static struct platform_driver serial8250_isa_driver = {
 	.probe		= serial8250_probe,
 	.remove_new	= serial8250_remove,
@@ -205,6 +281,7 @@ static struct platform_driver serial8250_isa_driver = {
 	.resume		= serial8250_resume,
 	.driver		= {
 		.name	= "serial8250",
+		.acpi_match_table = ACPI_PTR(acpi_platform_serial_table),
 	},
 };
 
-- 
2.43.0

Re: [PATCH] serial: 8250_platform: Enable generic 16550A platform devices

Posted by Palmer Dabbelt 1 year, 3 months ago

On Mon, 29 Jul 2024 22:12:18 PDT (-0700), Sunil V L wrote:
> Currently, 8250_platform driver is used only for devices with fixed
> serial ports (plat_serial8250_port). Extend this driver for any generic
> 16550A platform devices which can be probed using standard hardware
> discovery mechanisms like ACPI.
>
> This is required in particular for RISC-V which has non-PNP generic
> 16550A compatible UART that needs to be enumerated as ACPI platform
> device.
>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  drivers/tty/serial/8250/8250_platform.c | 77 +++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)


It seems a bit awkward that we need something RISC-V specific here.  I 
guess the idea is that we don't want to depend on PNP because we'd end 
up with a bunch of PCI dependencies as well?  That seems like kind of an 
odd way to do things, but I'm frequently suprised by ACPI stuff so

Acked-by: Palmer Dabbelt <palmer@rivosinc.com>

if someone was waiting for me...

> diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
> index d5c8d851348d..bdfb16bed4f2 100644
> --- a/drivers/tty/serial/8250/8250_platform.c
> +++ b/drivers/tty/serial/8250/8250_platform.c
> @@ -6,7 +6,9 @@
>   *	      PNP 8250/16550 ports
>   *	      "serial8250" platform devices
>   */
> +#include <linux/acpi.h>
>  #include <linux/array_size.h>
> +#include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
>  #include <linux/once.h>
> @@ -100,6 +102,65 @@ void __init serial8250_isa_init_ports(void)
>  	DO_ONCE(__serial8250_isa_init_ports);
>  }
>
> +/*
> + * Generic 16550A platform devices
> + */
> +static int serial8250_platform_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct uart_8250_port uart = { 0 };
> +	struct resource *regs;
> +	unsigned char iotype;
> +	int ret, line;
> +
> +	regs = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +	if (regs) {
> +		uart.port.iobase = regs->start;
> +		iotype = UPIO_PORT;
> +	} else {
> +		regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!regs) {
> +			dev_err(dev, "no registers defined\n");
> +			return -EINVAL;
> +		}
> +
> +		uart.port.mapbase = regs->start;
> +		uart.port.mapsize = resource_size(regs);
> +		uart.port.flags = UPF_IOREMAP;
> +		iotype = UPIO_MEM;
> +	}
> +
> +	/* Default clock frequency*/
> +	uart.port.uartclk = 1843200;
> +	uart.port.type = PORT_16550A;
> +	uart.port.dev = &pdev->dev;
> +	uart.port.flags |= UPF_SKIP_TEST | UPF_BOOT_AUTOCONF;
> +	ret = uart_read_and_validate_port_properties(&uart.port);
> +	/* no interrupt -> fall back to polling */
> +	if (ret == -ENXIO)
> +		ret = 0;
> +	if (ret)
> +		return ret;
> +
> +	if (uart.port.mapbase) {
> +		uart.port.membase = devm_ioremap(dev, uart.port.mapbase, uart.port.mapsize);
> +		if (!uart.port.membase)
> +			return -ENOMEM;
> +	}
> +
> +	/*
> +	 * The previous call may not set iotype correctly when reg-io-width
> +	 * property is absent and it doesn't support IO port resource.
> +	 */
> +	uart.port.iotype = iotype;
> +
> +	line = serial8250_register_8250_port(&uart);
> +	if (line < 0)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
>  /*
>   * Register a set of serial devices attached to a platform device.  The
>   * list is terminated with a zero flags entry, which means we expect
> @@ -110,6 +171,15 @@ static int serial8250_probe(struct platform_device *dev)
>  	struct plat_serial8250_port *p = dev_get_platdata(&dev->dev);
>  	struct uart_8250_port uart;
>  	int ret, i, irqflag = 0;
> +	struct fwnode_handle *fwnode = dev_fwnode(&dev->dev);
> +
> +	/*
> +	 * Probe platform UART devices defined using standard hardware
> +	 * discovery mechanism like ACPI or DT. Support only ACPI based
> +	 * serial device for now.
> +	 */
> +	if (!p && is_acpi_node(fwnode))
> +		return serial8250_platform_probe(dev);
>
>  	memset(&uart, 0, sizeof(uart));
>
> @@ -198,6 +268,12 @@ static int serial8250_resume(struct platform_device *dev)
>  	return 0;
>  }
>
> +static const struct acpi_device_id acpi_platform_serial_table[] = {
> +	{ "RSCV0003", 0 }, // RISC-V Generic 16550A UART
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, acpi_platform_serial_table);
> +
>  static struct platform_driver serial8250_isa_driver = {
>  	.probe		= serial8250_probe,
>  	.remove_new	= serial8250_remove,
> @@ -205,6 +281,7 @@ static struct platform_driver serial8250_isa_driver = {
>  	.resume		= serial8250_resume,
>  	.driver		= {
>  		.name	= "serial8250",
> +		.acpi_match_table = ACPI_PTR(acpi_platform_serial_table),
>  	},
>  };

Re: [PATCH] serial: 8250_platform: Enable generic 16550A platform devices

Posted by Andy Shevchenko 1 year, 3 months ago

On Tue, Sep 17, 2024 at 06:10:25AM -0700, Palmer Dabbelt wrote:
> On Mon, 29 Jul 2024 22:12:18 PDT (-0700), Sunil V L wrote:
> > Currently, 8250_platform driver is used only for devices with fixed
> > serial ports (plat_serial8250_port). Extend this driver for any generic
> > 16550A platform devices which can be probed using standard hardware
> > discovery mechanisms like ACPI.
> >
> > This is required in particular for RISC-V which has non-PNP generic
> > 16550A compatible UART that needs to be enumerated as ACPI platform
> > device.

> It seems a bit awkward that we need something RISC-V specific here.  I 
> guess the idea is that we don't want to depend on PNP because we'd end 
> up with a bunch of PCI dependencies as well?  That seems like kind of an 
> odd way to do things, but I'm frequently suprised by ACPI stuff so

We have the same for HSUART on Intel (see 8250_dw.c). The ACPI devices use
different approach than PNP. PNP has the exact requirements for compatibility
with the legacy UARTs (to some extend). It puts some limitations. I believe
having specifics for RISC-V is fine. But I haven't checked the real IP used
in the SoC. If it's one that we have the existing driver for (like again
mentioned 8250_dw.c), then it should be added there.

TL;DR: so, what is the IP is in hardware for UART? Custom?

-- 
With Best Regards,
Andy Shevchenko