xen/drivers/char/ns16550.c | 51 ++++++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 13 deletions(-)
RISC-V doesn't support interrupts for the time being, so it would be nice to
have polling mode.
The patch assumes that polling mode will be used if there is no interrupt
property or the interrupt is equal to some unused UART interrupt number ( look
at the definition of NO_IRQ_POLL in ns16550.c ).
Also, the patch updates other places where '0' ( use NO_IRQ_POLL instead of
'0' ) was used to set that polling mode should be used.
It is possible that interrupt '0' can be used for some architectures as an
legal UART interrupt number ( according to dts files in Linux kernel ).
For example:
https://github.com/torvalds/linux/blob/master/arch/powerpc/boot/dts/ebony.dts#L197
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
xen/drivers/char/ns16550.c | 51 ++++++++++++++++++++++++++++----------
1 file changed, 38 insertions(+), 13 deletions(-)
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 2aed6ec707..2547f53f5a 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -40,6 +40,8 @@
#include <asm/fixmap.h>
#endif
+#define NO_IRQ_POLL 0
+
static struct ns16550 {
int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
u64 io_base; /* I/O port or memory-mapped I/O address. */
@@ -58,7 +60,11 @@ static struct ns16550 {
struct timer timer;
struct timer resume_timer;
unsigned int timeout_ms;
- bool_t intr_works;
+ enum {
+ intr_off,
+ intr_on,
+ polling,
+ } intr_works;
bool_t dw_usr_bsy;
#ifdef NS16550_PCI
/* PCI card parameters. */
@@ -181,7 +187,7 @@ static void cf_check ns16550_interrupt(
struct serial_port *port = dev_id;
struct ns16550 *uart = port->uart;
- uart->intr_works = 1;
+ uart->intr_works = intr_on;
while ( !(ns_read_reg(uart, UART_IIR) & UART_IIR_NOINT) )
{
@@ -212,7 +218,7 @@ static void cf_check __ns16550_poll(struct cpu_user_regs *regs)
struct serial_port *port = this_cpu(poll_port);
struct ns16550 *uart = port->uart;
- if ( uart->intr_works )
+ if ( uart->intr_works == intr_on )
return; /* Interrupts work - no more polling */
while ( ns_read_reg(uart, UART_LSR) & UART_LSR_DR )
@@ -305,7 +311,8 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
unsigned char lcr;
unsigned int divisor;
- uart->intr_works = 0;
+ if ( uart->intr_works != polling )
+ uart->intr_works = intr_off;
pci_serial_early_init(uart);
@@ -394,7 +401,7 @@ static void __init cf_check ns16550_init_irq(struct serial_port *port)
static void ns16550_setup_postirq(struct ns16550 *uart)
{
- if ( uart->irq > 0 )
+ if ( uart->intr_works != polling )
{
/* Master interrupt enable; also keep DTR/RTS asserted. */
ns_write_reg(uart,
@@ -472,7 +479,8 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
if ( rc )
{
- uart->irq = 0;
+ uart->irq = NO_IRQ_POLL;
+ uart->intr_works = polling;
if ( msi_desc )
msi_free_irq(msi_desc);
else
@@ -488,7 +496,7 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
}
#endif
- if ( uart->irq > 0 )
+ if ( uart->intr_works != polling )
{
uart->irqaction.handler = ns16550_interrupt;
uart->irqaction.name = "ns16550";
@@ -595,7 +603,9 @@ static void __init cf_check ns16550_endboot(struct serial_port *port)
static int __init cf_check ns16550_irq(struct serial_port *port)
{
struct ns16550 *uart = port->uart;
- return ((uart->irq > 0) ? uart->irq : -1);
+
+ return (((uart->intr_works != polling) && (uart->irq >= 0)) ?
+ uart->irq : -1);
}
static void cf_check ns16550_start_tx(struct serial_port *port)
@@ -1330,9 +1340,12 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
* as special only for X86.
*/
if ( uart->irq == 0xff )
- uart->irq = 0;
+ {
+ uart->irq = NO_IRQ_POLL;
+ uart->intr_works = polling;
+ }
#endif
- if ( !uart->irq )
+ if ( uart->intr_works == polling )
printk(XENLOG_INFO
"ns16550: %pp: no legacy IRQ, using poll mode\n",
&PCI_SBDF(0, b, d, f));
@@ -1551,7 +1564,8 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
{
conf += 3;
uart->msi = true;
- uart->irq = 0;
+ uart->irq = NO_IRQ_POLL;
+ uart->intr_works = polling;
}
else
#endif
@@ -1791,8 +1805,19 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
}
res = platform_get_irq(dev, 0);
- if ( ! res )
- return -EINVAL;
+ if ( (res < 0 ) || (res == NO_IRQ_POLL) )
+ {
+ printk("ns1650: polling will be used\n");
+
+ /*
+ * If the node doesn't have any interrupt or an interrupt number is
+ * equal to reserved NO_IRQ_POLL, then it means the driver
+ * will want to using polling.
+ */
+ res = NO_IRQ_POLL;
+
+ uart->intr_works = polling;
+ }
uart->irq = res;
uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
--
2.41.0On 18.07.2023 16:13, Oleksii Kurochko wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -40,6 +40,8 @@
> #include <asm/fixmap.h>
> #endif
>
> +#define NO_IRQ_POLL 0
Do you really need this? I ask because ...
> @@ -595,7 +603,9 @@ static void __init cf_check ns16550_endboot(struct serial_port *port)
> static int __init cf_check ns16550_irq(struct serial_port *port)
> {
> struct ns16550 *uart = port->uart;
> - return ((uart->irq > 0) ? uart->irq : -1);
> +
> + return (((uart->intr_works != polling) && (uart->irq >= 0)) ?
... you now use >= here, which includes that special value. As long
as intr_works is always set to "polling", the particular value in
uart->irq shouldn't matter (and hence you wouldn't need to store
anywhere that or any other special value).
> @@ -1330,9 +1340,12 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
> * as special only for X86.
> */
> if ( uart->irq == 0xff )
> - uart->irq = 0;
> + {
> + uart->irq = NO_IRQ_POLL;
> + uart->intr_works = polling;
> + }
> #endif
> - if ( !uart->irq )
> + if ( uart->intr_works == polling )
Careful here - we may also have read 0 from PCI_INTERRUPT_LINE, or
forced 0 because we read 0 from PCI_INTERRUPT_PIN. All these cases,
unless provably broken, need to continue to function as they were.
Further you alter parse_positional(), but you leave alone
parse_namevalue_pairs(). I think you're changing the admin (command
line) interface that way, because so far "irq=0" was the way to
request polling. While it may be unavoidable to change that interface
(which will then need noting in ./CHANGELOG.md), you still need to
offer a way to forcibly set polling mode.
Jan
On Tue, 2023-07-18 at 16:40 +0200, Jan Beulich wrote:
> On 18.07.2023 16:13, Oleksii Kurochko wrote:
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -40,6 +40,8 @@
> > #include <asm/fixmap.h>
> > #endif
> >
> > +#define NO_IRQ_POLL 0
>
> Do you really need this? I ask because ...
>
> > @@ -595,7 +603,9 @@ static void __init cf_check
> > ns16550_endboot(struct serial_port *port)
> > static int __init cf_check ns16550_irq(struct serial_port *port)
> > {
> > struct ns16550 *uart = port->uart;
> > - return ((uart->irq > 0) ? uart->irq : -1);
> > +
> > + return (((uart->intr_works != polling) && (uart->irq >= 0)) ?
>
> ... you now use >= here, which includes that special value. As long
> as intr_works is always set to "polling", the particular value in
> uart->irq shouldn't matter (and hence you wouldn't need to store
> anywhere that or any other special value).
You are right it should matter what is the value of uart->irq in case
when polling mode is set.
>
> > @@ -1330,9 +1340,12 @@ pci_uart_config(struct ns16550 *uart, bool_t
> > skip_amt, unsigned int idx)
> > * as special only for X86.
> > */
> > if ( uart->irq == 0xff )
> > - uart->irq = 0;
> > + {
> > + uart->irq = NO_IRQ_POLL;
> > + uart->intr_works = polling;
> > + }
> > #endif
> > - if ( !uart->irq )
> > + if ( uart->intr_works == polling )
>
> Careful here - we may also have read 0 from PCI_INTERRUPT_LINE, or
> forced 0 because we read 0 from PCI_INTERRUPT_PIN. All these cases,
> unless provably broken, need to continue to function as they were.
Missed that it should be if (( uart->intr_works == polling ) || !uart-
>irq).
>
> Further you alter parse_positional(), but you leave alone
> parse_namevalue_pairs(). I think you're changing the admin (command
> line) interface that way, because so far "irq=0" was the way to
> request polling. While it may be unavoidable to change that interface
> (which will then need noting in ./CHANGELOG.md), you still need to
> offer a way to forcibly set polling mode.
It think it would be better to pass 'uart_force_polling' ( or kind of )
via command line.
~ Oleksii
On 18/07/2023 15:13, Oleksii Kurochko wrote: > RISC-V doesn't support interrupts for the time being, so it would be nice to > have polling mode. > > The patch assumes that polling mode will be used if there is no interrupt > property As I asked in v1, please explain that this is allowed by the binding and provide a link for others to verify. > or the interrupt is equal to some unused UART interrupt number ( look > at the definition of NO_IRQ_POLL in ns16550.c ). Nack. If you want to use polling mode and yet have an interrupts property then you should provide the information differently. This would either be via the command line or an extra property in the DT node. If the latter, it would need to be standardized first. Cheers, -- Julien Grall
On Tue, 2023-07-18 at 15:23 +0100, Julien Grall wrote:
>
>
> On 18/07/2023 15:13, Oleksii Kurochko wrote:
> > RISC-V doesn't support interrupts for the time being, so it would
> > be nice to
> > have polling mode.
> >
> > The patch assumes that polling mode will be used if there is no
> > interrupt
> > property
>
> As I asked in v1, please explain that this is allowed by the binding
> and
> provide a link for others to verify.
According to 4.2.2 National Semiconductor 16450/16550 Compatible UART
Requirements from The Devicetree Specification v0.4
( https://www.devicetree.org/specifications/ ) interrupts property
should always present.
So if interrupt property doesn't present in serial node then it should
return -EINVAL:
res = platform_get_irq(dev, 0);
if ( res < 0 )
{
printk("ns16550: Unable to retrieve the IRQ\n");
return -EINVAL;
}
>
> > or the interrupt is equal to some unused UART interrupt number (
> > look
> > at the definition of NO_IRQ_POLL in ns16550.c ).
>
> Nack. If you want to use polling mode and yet have an interrupts
> property then you should provide the information differently. This
> would
> either be via the command line or an extra property in the DT node.
>
> If the latter, it would need to be standardized first.
What does it mean 'standardized'? Do you mean that it should updated
The Devicetree Specification?
I am not sure that I know the process of standardization of such stuff
could you please give me any pointers?
It looks like it will be faster to pass it via the command line as
standardization can consume some time...
~ Oleksii
Hi, On 18/07/2023 16:00, Oleksii wrote: > On Tue, 2023-07-18 at 15:23 +0100, Julien Grall wrote: >> >> >> On 18/07/2023 15:13, Oleksii Kurochko wrote: >>> RISC-V doesn't support interrupts for the time being, so it would >>> be nice to >>> have polling mode. >>> >>> The patch assumes that polling mode will be used if there is no >>> interrupt >>> property >> >> As I asked in v1, please explain that this is allowed by the binding >> and >> provide a link for others to verify. > According to 4.2.2 National Semiconductor 16450/16550 Compatible UART > Requirements from The Devicetree Specification v0.4 > ( https://www.devicetree.org/specifications/ ) interrupts property > should always present. I don't read the spec the same way as you. The property is marked as 'OR' which means the property is optional but recommended. Therefore, what you just wrote is enough to justify why we can relax the check. >> >>> or the interrupt is equal to some unused UART interrupt number ( >>> look >>> at the definition of NO_IRQ_POLL in ns16550.c ). >> >> Nack. If you want to use polling mode and yet have an interrupts >> property then you should provide the information differently. This >> would >> either be via the command line or an extra property in the DT node. >> >> If the latter, it would need to be standardized first. > What does it mean 'standardized'? Do you mean that it should updated > The Devicetree Specification? I mean that the binding for the ns16550 in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings should be updated. I will not accept any code in Xen which use properties that have not been accepted by the Linux/Device-Tree community. Unless this is a binding "owned" by Xen (e.g. the nodes for dom0less). > > I am not sure that I know the process of standardization of such stuff > could you please give me any pointers? In general, this is sending an e-mail to the device-tree mailing with your proposal in the form of a patch. > > It looks like it will be faster to pass it via the command line as > standardization can consume some time... Well yes. But as usual, it depends on your end goal. For instance, we would not want to describe the HW on the command line. Thinking a bit more, in this case, the command line option is probably best because you want to override what's describe in the firmware table. Cheers, -- Julien Grall
© 2016 - 2024 Red Hat, Inc.