From: Guanbing Huang <[email protected]>
The 16550a serial port based on the ACPI table requires obtaining the
reg-shift attribute. In the ACPI scenario, If the reg-shift property
is not configured like in DTS, the 16550a serial driver cannot read or
write controller registers properly during initialization.
Signed-off-by: Guanbing Huang <[email protected]>
Reviewed-by: Bing Fan <[email protected]>
Tested-by: Linheng Du <[email protected]>
---
v2 -> v3: switch to use uart_read_port_properties(), change "Signed-off-by" to "Reviewed-by" and "Tested-by".
v1 -> v2: change the names after "Signed off by" to the real names
drivers/tty/serial/8250/8250_pnp.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_pnp.c b/drivers/tty/serial/8250/8250_pnp.c
index 1974bbadc975..aafddede783a 100644
--- a/drivers/tty/serial/8250/8250_pnp.c
+++ b/drivers/tty/serial/8250/8250_pnp.c
@@ -443,25 +443,22 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
}
memset(&uart, 0, sizeof(uart));
- if (pnp_irq_valid(dev, 0))
- uart.port.irq = pnp_irq(dev, 0);
if ((flags & CIR_PORT) && pnp_port_valid(dev, 2)) {
uart.port.iobase = pnp_port_start(dev, 2);
- uart.port.iotype = UPIO_PORT;
} else if (pnp_port_valid(dev, 0)) {
uart.port.iobase = pnp_port_start(dev, 0);
- uart.port.iotype = UPIO_PORT;
} else if (pnp_mem_valid(dev, 0)) {
uart.port.mapbase = pnp_mem_start(dev, 0);
- uart.port.iotype = UPIO_MEM;
uart.port.flags = UPF_IOREMAP;
} else
return -ENODEV;
- dev_dbg(&dev->dev,
- "Setup PNP port: port %#lx, mem %#llx, irq %u, type %u\n",
- uart.port.iobase, (unsigned long long)uart.port.mapbase,
- uart.port.irq, uart.port.iotype);
+ uart.port.uartclk = 1843200;
+ uart.port.dev = &dev->dev;
+
+ ret = uart_read_port_properties(&uart.port);
+ if (ret < 0)
+ return ret;
if (flags & CIR_PORT) {
uart.port.flags |= UPF_FIXED_PORT | UPF_FIXED_TYPE;
@@ -469,11 +466,11 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
}
uart.port.flags |= UPF_SKIP_TEST | UPF_BOOT_AUTOCONF;
- if (pnp_irq_flags(dev, 0) & IORESOURCE_IRQ_SHAREABLE)
- uart.port.flags |= UPF_SHARE_IRQ;
- uart.port.uartclk = 1843200;
- device_property_read_u32(&dev->dev, "clock-frequency", &uart.port.uartclk);
- uart.port.dev = &dev->dev;
+
+ dev_dbg(&dev->dev,
+ "Setup PNP port: port %#lx, mem %#llx, irq %u, type %u\n",
+ uart.port.iobase, (unsigned long long)uart.port.mapbase,
+ uart.port.irq, uart.port.iotype);
line = serial8250_register_8250_port(&uart);
if (line < 0 || (flags & CIR_PORT))
--
2.17.1
On Thu, Mar 28, 2024 at 11:45:29AM +0800, Guanbing Huang wrote:
> From: Guanbing Huang <[email protected]>
>
> The 16550a serial port based on the ACPI table requires obtaining the
> reg-shift attribute. In the ACPI scenario, If the reg-shift property
> is not configured like in DTS, the 16550a serial driver cannot read or
> write controller registers properly during initialization.
Thank you for the update!
Basically we have now a few issues with this (easy to fix though).
Note, below I described _two_ variants of what needs to be done,
but if you think it's too much for you, let's go in v4 to the code
of v2 (i.o.w. just adding what you need in 8250_pnp).
..
> memset(&uart, 0, sizeof(uart));
> - if (pnp_irq_valid(dev, 0))
> - uart.port.irq = pnp_irq(dev, 0);
This has checked for IRQ validness and left it 0 if not...
> if ((flags & CIR_PORT) && pnp_port_valid(dev, 2)) {
> uart.port.iobase = pnp_port_start(dev, 2);
> - uart.port.iotype = UPIO_PORT;
> } else if (pnp_port_valid(dev, 0)) {
> uart.port.iobase = pnp_port_start(dev, 0);
> - uart.port.iotype = UPIO_PORT;
> } else if (pnp_mem_valid(dev, 0)) {
> uart.port.mapbase = pnp_mem_start(dev, 0);
(The mapsize is also needed now to be initialized correctly)
> - uart.port.iotype = UPIO_MEM;
> uart.port.flags = UPF_IOREMAP;
> } else
> return -ENODEV;
>
> - dev_dbg(&dev->dev,
> - "Setup PNP port: port %#lx, mem %#llx, irq %u, type %u\n",
> - uart.port.iobase, (unsigned long long)uart.port.mapbase,
> - uart.port.irq, uart.port.iotype);
> + uart.port.uartclk = 1843200;
> + uart.port.dev = &dev->dev;
> +
> + ret = uart_read_port_properties(&uart.port);
..which means here you need to check for IRQ being absent. The example
is 8250_dw.c, where similar check is done.
> + if (ret < 0)
> + return ret;
Then the iotype should be preserved, most likely it will become UPIO_UNKNOWN
for those who do not provide reg-io-width property. That means you need an
additional local variable that you assign instead of port->iotype in the above
if-else-if chain and (re)assign port->iotype with it here.
..
Next point is that above API doesn't cover PNP devices, what you need is
to add a patch to that API in drivers/tty/serial/serial_port.c
else if (dev_is_pnp(dev))
ret = pnp_irq(to_pnp_dev(dev), 0);
if (ret < 0)
ret = -ENXIO;
} else
You may try to fix pnp to return -ENXIO from pnp_irq(), but this might need
more careful check of users that none of them relies on the exact returned
value. (If you choose this direction, the mentioned change has to be done
separately.)
And also update include/linux/pnp.h to provide dev_is_pnp() macro
(somewhere before module_pnp_driver() I think)
#define dev_is_pnp(d) ((d)->bus == &pnp_bus_type)
(This should be done in a separate patch)
That said you should have the series out of 3+ patches:
1) (optional) Fix pnp_irq() to return -ENXIO.
2) Add dev_is_pnp() macro to include/linux/pnp.h.
3) Add support of PNP IRQ to __uart_read_properties().
4) Update 8250_pnp to support the additional properties.
Alternatively to the above you may simply try pnp_irq() after the call
ret = uart_read_port_properties(&uart.port);
if (ret == -ENXIO) {
if (pnp_irq_valid(dev, 0))
uart.port.irq = pnp_irq(dev, 0);
if (pnp_irq_flags(dev, 0) & IORESOURCE_IRQ_SHAREABLE)
uart.port.flags |= UPF_SHARE_IRQ;
} else if (ret) {
return ret;
}
--
With Best Regards,
Andy Shevchenko