From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Guanbing Huang <albanhuang@outlook.com>
Cc: gregkh@linuxfoundation.org, jirislaby@kernel.org,
linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
lvjianmin@loongson.cn, albanhuang@tencent.com,
tombinfan@tencent.com
Subject: Re: [PATCH v3] serial: 8250_pnp: Support configurable reg shift property
Date: Thu, 28 Mar 2024 18:22:05 +0200 [thread overview]
Message-ID: <ZgWZLfKp9fqAHjb8@smile.fi.intel.com> (raw)
In-Reply-To: <SG2PR06MB4953D0E0A40FDAC34FF130C8C93B2@SG2PR06MB4953.apcprd06.prod.outlook.com>
On Thu, Mar 28, 2024 at 11:45:29AM +0800, Guanbing Huang wrote:
> From: Guanbing Huang <albanhuang@tencent.com>
>
> 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
prev parent reply other threads:[~2024-03-28 16:22 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-28 3:45 [PATCH v3] serial: 8250_pnp: Support configurable reg shift property Guanbing Huang
2024-03-28 16:22 ` Andy Shevchenko [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZgWZLfKp9fqAHjb8@smile.fi.intel.com \
--to=andriy.shevchenko@intel.com \
--cc=albanhuang@outlook.com \
--cc=albanhuang@tencent.com \
--cc=gregkh@linuxfoundation.org \
--cc=jirislaby@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=lvjianmin@loongson.cn \
--cc=tombinfan@tencent.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).