| Message ID | cc5a08056abacdbb6d6509b56716eb45467307bb.1689240611.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ns1650: refactor interrupt handling in ns16550_uart_dt_init() | expand |
Hi Oleksii, Title: IMO, Your patch doesn't do any refactor. Instead, it add support for polling when using the DT. On 13/07/2023 10:30, Oleksii Kurochko wrote: > In ns16550_init_postirq() there is the following check: > if ( uart->irq > 0 ) > { > uart->irqaction.handler = ns16550_interrupt; > uart->irqaction.name = "ns16550"; > uart->irqaction.dev_id = port; > if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) > printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq); > } > > Thereby to have ns16550 work in polling mode uart->irq, should be equal to 0. > > So it is needed to relax the following check in ns16550_uart_dt_init(): > res = platform_get_irq(dev, 0); > if ( ! res ) > return -EINVAL; > uart->irq = res; > If 'res' equals to -1 then polling mode should be used instead of return > -EINVAL. This commit message has a bit too much code in it for me taste. I don't think it is necessary to quote the code. Instead, you can explain the following: * Why you want to support polling * Why this is valid to have a node without interrupts (add a reference to the bindings) * That polling is indicated by using 'irq = 0'. I would consider to provide a define (e.g NO_IRQ_POLL) to make it more clearer. > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > xen/drivers/char/ns16550.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c > index 2aed6ec707..f30f10d175 100644 > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -1791,8 +1791,16 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev, > } > > res = platform_get_irq(dev, 0); > - if ( ! res ) > - return -EINVAL; > + if ( res == -1 ) Why do you check explicitely for -1 instead of < 0? Also, the behavior is somewhat change now. Before, we would return -EINVAL when res equals 0. Can you explain in the commit message why this is done? > + { > + printk("ns1650: polling will be used\n"); > + /* > + * There is the check 'if ( uart->irq > 0 )' in ns16550_init_postirq(). > + * If the check is true then interrupt mode will be used otherwise > + * ( when irq = 0 )polling. > + */ Similar remark to the commit message. You could write: "If the node doesn't have any interrupt, then it means the driver will want to using polling." > + res = 0; > + } > uart->irq = res; > > uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart"); Cheers,
On 13.07.2023 11:30, Oleksii Kurochko wrote: > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -1791,8 +1791,16 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev, > } > > res = platform_get_irq(dev, 0); > - if ( ! res ) > - return -EINVAL; > + if ( res == -1 ) > + { > + printk("ns1650: polling will be used\n"); Nit: Please don't omit one of the two 5-s here. > + /* > + * There is the check 'if ( uart->irq > 0 )' in ns16550_init_postirq(). > + * If the check is true then interrupt mode will be used otherwise > + * ( when irq = 0 )polling. > + */ I wonder in how far that's actually correct outside of x86. On x86 IRQ0 is always the timer interrupt, but I'm not convinced something similar can be used as kind of a heuristic on Arm, RISC-V, or basically any other architecture. Jan
Hi Jan, On 13/07/2023 11:08, Jan Beulich wrote: > On 13.07.2023 11:30, Oleksii Kurochko wrote: >> --- a/xen/drivers/char/ns16550.c >> +++ b/xen/drivers/char/ns16550.c >> @@ -1791,8 +1791,16 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev, >> } >> >> res = platform_get_irq(dev, 0); >> - if ( ! res ) >> - return -EINVAL; >> + if ( res == -1 ) >> + { >> + printk("ns1650: polling will be used\n"); > > Nit: Please don't omit one of the two 5-s here. > >> + /* >> + * There is the check 'if ( uart->irq > 0 )' in ns16550_init_postirq(). >> + * If the check is true then interrupt mode will be used otherwise >> + * ( when irq = 0 )polling. >> + */ > > I wonder in how far that's actually correct outside of x86. On x86 IRQ0 is > always the timer interrupt, but I'm not convinced something similar can be > used as kind of a heuristic on Arm, RISC-V, or basically any other > architecture. I wondered the same. On Arm we are fine because the UART will be an SPI which starts at 32. That's part why I was suggesting to use a define. Because we don't have to hardcode the poll value everywhere. Cheers,
Hi Julien, On Thu, 2023-07-13 at 10:43 +0100, Julien Grall wrote: > Hi Oleksii, > > Title: IMO, Your patch doesn't do any refactor. Instead, it add > support > for polling when using the DT. Agree. It would be better to rephrase the title. > > On 13/07/2023 10:30, Oleksii Kurochko wrote: > > In ns16550_init_postirq() there is the following check: > > if ( uart->irq > 0 ) > > { > > uart->irqaction.handler = ns16550_interrupt; > > uart->irqaction.name = "ns16550"; > > uart->irqaction.dev_id = port; > > if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 > > ) > > printk("ERROR: Failed to allocate ns16550 IRQ %d\n", > > uart->irq); > > } > > > > Thereby to have ns16550 work in polling mode uart->irq, should be > > equal to 0. > > > > So it is needed to relax the following check in > > ns16550_uart_dt_init(): > > res = platform_get_irq(dev, 0); > > if ( ! res ) > > return -EINVAL; > > uart->irq = res; > > If 'res' equals to -1 then polling mode should be used instead of > > return > > -EINVAL. > > This commit message has a bit too much code in it for me taste. I > don't > think it is necessary to quote the code. Instead, you can explain the > following: > > * Why you want to support polling > * Why this is valid to have a node without interrupts (add a > reference > to the bindings) > * That polling is indicated by using 'irq = 0'. I would consider to > provide a define (e.g NO_IRQ_POLL) to make it more clearer. Thanks. I'll update the commit message. > > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > xen/drivers/char/ns16550.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/xen/drivers/char/ns16550.c > > b/xen/drivers/char/ns16550.c > > index 2aed6ec707..f30f10d175 100644 > > --- a/xen/drivers/char/ns16550.c > > +++ b/xen/drivers/char/ns16550.c > > @@ -1791,8 +1791,16 @@ static int __init > > ns16550_uart_dt_init(struct dt_device_node *dev, > > } > > > > res = platform_get_irq(dev, 0); > > - if ( ! res ) > > - return -EINVAL; > > + if ( res == -1 ) > > Why do you check explicitely for -1 instead of < 0? Also, the > behavior > is somewhat change now. I checked it for -1 as I missed that platform_get_irq() returns 'int' and uart->irq is also 'int'. 'irq' variable inside plaform_get_irq is declared as 'unsigned int', so I thought that in case of 'interrupt' property is processed successfully we will have some positive value otherwise platform_get_irq() returns -1 ( in current implementation ). So it would be better to check for " res < 0 ". > Before, we would return -EINVAL when res equals > 0. Can you explain in the commit message why this is done? This is not clear for me. It was done during replacing of dt_device_get_irq by platform_get_irq ( https://gitlab.com/xen-project/xen/-/commit/554cbe32381fa4482e1a47cd31afb054e97d986d ) and for other similar cases it was changed to "res < 0" except ns16550 driver. > > > + { > > + printk("ns1650: polling will be used\n"); > > + /* > > + * There is the check 'if ( uart->irq > 0 )' in > > ns16550_init_postirq(). > > + * If the check is true then interrupt mode will be used > > otherwise > > + * ( when irq = 0 )polling. > > + */ > > Similar remark to the commit message. You could write: > > "If the node doesn't have any interrupt, then it means the driver > will > want to using polling." Thanks. I'll take into account. > > > + res = 0; > > + } > > uart->irq = res; > > > > uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb- > > uart"); > > Cheers, > ~ Oleksii
Hi, On 13/07/2023 12:36, Oleksii wrote: > On Thu, 2023-07-13 at 10:43 +0100, Julien Grall wrote: >> Hi Oleksii, >> >> Title: IMO, Your patch doesn't do any refactor. Instead, it add >> support >> for polling when using the DT. > Agree. It would be better to rephrase the title. > >> >> On 13/07/2023 10:30, Oleksii Kurochko wrote: >>> In ns16550_init_postirq() there is the following check: >>> if ( uart->irq > 0 ) >>> { >>> uart->irqaction.handler = ns16550_interrupt; >>> uart->irqaction.name = "ns16550"; >>> uart->irqaction.dev_id = port; >>> if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 >>> ) >>> printk("ERROR: Failed to allocate ns16550 IRQ %d\n", >>> uart->irq); >>> } >>> >>> Thereby to have ns16550 work in polling mode uart->irq, should be >>> equal to 0. >>> >>> So it is needed to relax the following check in >>> ns16550_uart_dt_init(): >>> res = platform_get_irq(dev, 0); >>> if ( ! res ) >>> return -EINVAL; >>> uart->irq = res; >>> If 'res' equals to -1 then polling mode should be used instead of >>> return >>> -EINVAL. >> >> This commit message has a bit too much code in it for me taste. I >> don't >> think it is necessary to quote the code. Instead, you can explain the >> following: >> >> * Why you want to support polling >> * Why this is valid to have a node without interrupts (add a >> reference >> to the bindings) >> * That polling is indicated by using 'irq = 0'. I would consider to >> provide a define (e.g NO_IRQ_POLL) to make it more clearer. > Thanks. I'll update the commit message. > >> >>> >>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >>> --- >>> xen/drivers/char/ns16550.c | 12 ++++++++++-- >>> 1 file changed, 10 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/drivers/char/ns16550.c >>> b/xen/drivers/char/ns16550.c >>> index 2aed6ec707..f30f10d175 100644 >>> --- a/xen/drivers/char/ns16550.c >>> +++ b/xen/drivers/char/ns16550.c >>> @@ -1791,8 +1791,16 @@ static int __init >>> ns16550_uart_dt_init(struct dt_device_node *dev, >>> } >>> >>> res = platform_get_irq(dev, 0); >>> - if ( ! res ) >>> - return -EINVAL; >>> + if ( res == -1 ) >> >> Why do you check explicitely for -1 instead of < 0? Also, the >> behavior >> is somewhat change now. > I checked it for -1 as I missed that platform_get_irq() returns 'int' > and uart->irq is also 'int'. 'irq' variable inside plaform_get_irq is > declared as 'unsigned int', so I thought that in case of 'interrupt' > property is processed successfully we will have some positive value > otherwise platform_get_irq() returns -1 ( in current implementation ). > So it would be better to check for " res < 0 ". > >> Before, we would return -EINVAL when res equals >> 0. Can you explain in the commit message why this is done? > This is not clear for me. > It was done during replacing of dt_device_get_irq by platform_get_irq > ( https://gitlab.com/xen-project/xen/-/commit/554cbe32381fa4482e1a47cd31afb054e97d986d > ) and for other similar cases it was changed to "res < 0" except > ns16550 driver. Hmmm... I think I made a mistake back then. This check should have been 'res <= 0' because '0' is used for polling. Cheers,
On Thu, 2023-07-13 at 11:13 +0100, Julien Grall wrote: > Hi Jan, > > On 13/07/2023 11:08, Jan Beulich wrote: > > On 13.07.2023 11:30, Oleksii Kurochko wrote: > > > --- a/xen/drivers/char/ns16550.c > > > +++ b/xen/drivers/char/ns16550.c > > > @@ -1791,8 +1791,16 @@ static int __init > > > ns16550_uart_dt_init(struct dt_device_node *dev, > > > } > > > > > > res = platform_get_irq(dev, 0); > > > - if ( ! res ) > > > - return -EINVAL; > > > + if ( res == -1 ) > > > + { > > > + printk("ns1650: polling will be used\n"); > > > > Nit: Please don't omit one of the two 5-s here. > > > > > + /* > > > + * There is the check 'if ( uart->irq > 0 )' in > > > ns16550_init_postirq(). > > > + * If the check is true then interrupt mode will be used > > > otherwise > > > + * ( when irq = 0 )polling. > > > + */ > > > > I wonder in how far that's actually correct outside of x86. On x86 > > IRQ0 is > > always the timer interrupt, but I'm not convinced something similar > > can be > > used as kind of a heuristic on Arm, RISC-V, or basically any other > > architecture. > > I wondered the same. On Arm we are fine because the UART will be an > SPI > which starts at 32. > > That's part why I was suggesting to use a define. Because we don't > have > to hardcode the poll value everywhere. Probably then it would be better to introduce 'bool is_polling_mode' inside struct ns16550? The same thing ( with uart->irq = 0 ) is used for detecting if polling mode should be used in case of x86 and PCI: https://gitlab.com/xen-project/xen/-/blame/staging/xen/drivers/char/ns16550.c?page=2#L1332 ~ Oleksii
Hi, On 13/07/2023 12:55, Oleksii wrote: > On Thu, 2023-07-13 at 11:13 +0100, Julien Grall wrote: >> Hi Jan, >> >> On 13/07/2023 11:08, Jan Beulich wrote: >>> On 13.07.2023 11:30, Oleksii Kurochko wrote: >>>> --- a/xen/drivers/char/ns16550.c >>>> +++ b/xen/drivers/char/ns16550.c >>>> @@ -1791,8 +1791,16 @@ static int __init >>>> ns16550_uart_dt_init(struct dt_device_node *dev, >>>> } >>>> >>>> res = platform_get_irq(dev, 0); >>>> - if ( ! res ) >>>> - return -EINVAL; >>>> + if ( res == -1 ) >>>> + { >>>> + printk("ns1650: polling will be used\n"); >>> >>> Nit: Please don't omit one of the two 5-s here. >>> >>>> + /* >>>> + * There is the check 'if ( uart->irq > 0 )' in >>>> ns16550_init_postirq(). >>>> + * If the check is true then interrupt mode will be used >>>> otherwise >>>> + * ( when irq = 0 )polling. >>>> + */ >>> >>> I wonder in how far that's actually correct outside of x86. On x86 >>> IRQ0 is >>> always the timer interrupt, but I'm not convinced something similar >>> can be >>> used as kind of a heuristic on Arm, RISC-V, or basically any other >>> architecture. >> >> I wondered the same. On Arm we are fine because the UART will be an >> SPI >> which starts at 32. >> >> That's part why I was suggesting to use a define. Because we don't >> have >> to hardcode the poll value everywhere. > Probably then it would be better to introduce 'bool is_polling_mode' > inside struct ns16550? I would be OK with that. Cheers,
On 13.07.2023 13:55, Oleksii wrote: > On Thu, 2023-07-13 at 11:13 +0100, Julien Grall wrote: >> Hi Jan, >> >> On 13/07/2023 11:08, Jan Beulich wrote: >>> On 13.07.2023 11:30, Oleksii Kurochko wrote: >>>> --- a/xen/drivers/char/ns16550.c >>>> +++ b/xen/drivers/char/ns16550.c >>>> @@ -1791,8 +1791,16 @@ static int __init >>>> ns16550_uart_dt_init(struct dt_device_node *dev, >>>> } >>>> >>>> res = platform_get_irq(dev, 0); >>>> - if ( ! res ) >>>> - return -EINVAL; >>>> + if ( res == -1 ) >>>> + { >>>> + printk("ns1650: polling will be used\n"); >>> >>> Nit: Please don't omit one of the two 5-s here. >>> >>>> + /* >>>> + * There is the check 'if ( uart->irq > 0 )' in >>>> ns16550_init_postirq(). >>>> + * If the check is true then interrupt mode will be used >>>> otherwise >>>> + * ( when irq = 0 )polling. >>>> + */ >>> >>> I wonder in how far that's actually correct outside of x86. On x86 >>> IRQ0 is >>> always the timer interrupt, but I'm not convinced something similar >>> can be >>> used as kind of a heuristic on Arm, RISC-V, or basically any other >>> architecture. >> >> I wondered the same. On Arm we are fine because the UART will be an >> SPI >> which starts at 32. >> >> That's part why I was suggesting to use a define. Because we don't >> have >> to hardcode the poll value everywhere. > Probably then it would be better to introduce 'bool is_polling_mode' > inside struct ns16550? Perhaps. If I was to make such a change, I'd probably convert intr_works to a tristate. But a boolean will be okay; if I may ask, name it just "polling" though. Jan
On Thu, 2023-07-13 at 12:08 +0200, Jan Beulich wrote: > On 13.07.2023 11:30, Oleksii Kurochko wrote: > > --- a/xen/drivers/char/ns16550.c > > +++ b/xen/drivers/char/ns16550.c > > @@ -1791,8 +1791,16 @@ static int __init > > ns16550_uart_dt_init(struct dt_device_node *dev, > > } > > > > res = platform_get_irq(dev, 0); > > - if ( ! res ) > > - return -EINVAL; > > + if ( res == -1 ) > > + { > > + printk("ns1650: polling will be used\n"); > > Nit: Please don't omit one of the two 5-s here. > > > + /* > > + * There is the check 'if ( uart->irq > 0 )' in > > ns16550_init_postirq(). > > + * If the check is true then interrupt mode will be used > > otherwise > > + * ( when irq = 0 )polling. > > + */ > > I wonder in how far that's actually correct outside of x86. On x86 > IRQ0 is > always the timer interrupt, but I'm not convinced something similar > can be > used as kind of a heuristic on Arm, RISC-V, or basically any other > architecture. uart->irq is used as an interrupt number for ns16550 and according to the code in ns16550_init_postirq() uart->irq should be > 0. So there is safe to use 0 as a detector of polling as it won't be used as an interrupt number for ns16550 itself. ~ Oleksii
On 13.07.2023 15:19, Oleksii wrote: > On Thu, 2023-07-13 at 12:08 +0200, Jan Beulich wrote: >> On 13.07.2023 11:30, Oleksii Kurochko wrote: >>> --- a/xen/drivers/char/ns16550.c >>> +++ b/xen/drivers/char/ns16550.c >>> @@ -1791,8 +1791,16 @@ static int __init >>> ns16550_uart_dt_init(struct dt_device_node *dev, >>> } >>> >>> res = platform_get_irq(dev, 0); >>> - if ( ! res ) >>> - return -EINVAL; >>> + if ( res == -1 ) >>> + { >>> + printk("ns1650: polling will be used\n"); >> >> Nit: Please don't omit one of the two 5-s here. >> >>> + /* >>> + * There is the check 'if ( uart->irq > 0 )' in >>> ns16550_init_postirq(). >>> + * If the check is true then interrupt mode will be used >>> otherwise >>> + * ( when irq = 0 )polling. >>> + */ >> >> I wonder in how far that's actually correct outside of x86. On x86 >> IRQ0 is >> always the timer interrupt, but I'm not convinced something similar >> can be >> used as kind of a heuristic on Arm, RISC-V, or basically any other >> architecture. > uart->irq is used as an interrupt number for ns16550 and according to > the code in ns16550_init_postirq() uart->irq should be > 0. > So there is safe to use 0 as a detector of polling as it won't be used > as an interrupt number for ns16550 itself. I don't understand. My earlier comment was affecting all checks of uart->irq alike, as I'm unconvinced IRQ0 may not possibly be usable on some architecture / platform. IOW I don't see why the check in ns16550_init_postirq() would allow us any leeway. Jan
On Thu, 2023-07-13 at 15:27 +0200, Jan Beulich wrote: > I don't understand. My earlier comment was affecting all checks of > uart->irq alike, as I'm unconvinced IRQ0 may not possibly be usable > on some architecture / platform. IOW I don't see why the check in > ns16550_init_postirq() would allow us any leeway. It looks like I misunderstood you. Do you mean that on some architecture IRQ0 may be used for ns16550? If yes, the code inside ns16550_init_postirq() will ignore that fact and use polling mode. ~ Oleksii
On 13.07.2023 15:36, Oleksii wrote: > On Thu, 2023-07-13 at 15:27 +0200, Jan Beulich wrote: >> I don't understand. My earlier comment was affecting all checks of >> uart->irq alike, as I'm unconvinced IRQ0 may not possibly be usable >> on some architecture / platform. IOW I don't see why the check in >> ns16550_init_postirq() would allow us any leeway. > It looks like I misunderstood you. > > Do you mean that on some architecture IRQ0 may be used for ns16550? Yes, I don't see why this shouldn't be possible in principle. As Julien said it can't happen on Arm, so if it also can't happen on RISC-V and PPC, we could elect to continue to ignore that aspect. Jan
On Thu, 2023-07-13 at 16:26 +0200, Jan Beulich wrote: > On 13.07.2023 15:36, Oleksii wrote: > > On Thu, 2023-07-13 at 15:27 +0200, Jan Beulich wrote: > > > I don't understand. My earlier comment was affecting all checks > > > of > > > uart->irq alike, as I'm unconvinced IRQ0 may not possibly be > > > usable > > > on some architecture / platform. IOW I don't see why the check in > > > ns16550_init_postirq() would allow us any leeway. > > It looks like I misunderstood you. > > > > Do you mean that on some architecture IRQ0 may be used for ns16550? > > Yes, I don't see why this shouldn't be possible in principle. As > Julien > said it can't happen on Arm, so if it also can't happen on RISC-V and > PPC, we could elect to continue to ignore that aspect. > Then for RISC-V ( at least, for PLIC interrupt controller ) it is reserved: https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc#interrupt-identifiers-ids What about to have 'define NO_IRQ_POLL 0' ( mentioned by Julien )+ assert(irq_from_device_tree != NO_IRQ_POLL) ? ~ Oleksii
On 13.07.2023 19:49, Oleksii wrote: > On Thu, 2023-07-13 at 16:26 +0200, Jan Beulich wrote: >> On 13.07.2023 15:36, Oleksii wrote: >>> On Thu, 2023-07-13 at 15:27 +0200, Jan Beulich wrote: >>>> I don't understand. My earlier comment was affecting all checks >>>> of >>>> uart->irq alike, as I'm unconvinced IRQ0 may not possibly be >>>> usable >>>> on some architecture / platform. IOW I don't see why the check in >>>> ns16550_init_postirq() would allow us any leeway. >>> It looks like I misunderstood you. >>> >>> Do you mean that on some architecture IRQ0 may be used for ns16550? >> >> Yes, I don't see why this shouldn't be possible in principle. As >> Julien >> said it can't happen on Arm, so if it also can't happen on RISC-V and >> PPC, we could elect to continue to ignore that aspect. >> > Then for RISC-V ( at least, for PLIC interrupt controller ) it is > reserved: > https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc#interrupt-identifiers-ids > > What about to have 'define NO_IRQ_POLL 0' ( mentioned by Julien )+ > assert(irq_from_device_tree != NO_IRQ_POLL) ? Such a #define may be okay as long as indeed used consistently in all places where it belongs (which may mean making some code less simple, which is a downside), but I can't judge at all the validity of the assertion you propose. Jan
Hi, On 14/07/2023 07:56, Jan Beulich wrote: > On 13.07.2023 19:49, Oleksii wrote: >> On Thu, 2023-07-13 at 16:26 +0200, Jan Beulich wrote: >>> On 13.07.2023 15:36, Oleksii wrote: >>>> On Thu, 2023-07-13 at 15:27 +0200, Jan Beulich wrote: >>>>> I don't understand. My earlier comment was affecting all checks >>>>> of >>>>> uart->irq alike, as I'm unconvinced IRQ0 may not possibly be >>>>> usable >>>>> on some architecture / platform. IOW I don't see why the check in >>>>> ns16550_init_postirq() would allow us any leeway. >>>> It looks like I misunderstood you. >>>> >>>> Do you mean that on some architecture IRQ0 may be used for ns16550? >>> >>> Yes, I don't see why this shouldn't be possible in principle. As >>> Julien >>> said it can't happen on Arm, so if it also can't happen on RISC-V and >>> PPC, we could elect to continue to ignore that aspect. >>> >> Then for RISC-V ( at least, for PLIC interrupt controller ) it is >> reserved: >> https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc#interrupt-identifiers-ids >> >> What about to have 'define NO_IRQ_POLL 0' ( mentioned by Julien )+ >> assert(irq_from_device_tree != NO_IRQ_POLL) ? > > Such a #define may be okay as long as indeed used consistently in all > places where it belongs (which may mean making some code less simple, > which is a downside), but I can't judge at all the validity of the > assertion you propose. Is the assert() indented to check the return of platform_get_irq()? If so, the return value is considered as user input because it is coming from the device-tree. assert()s must not be used for checking external input. Instead, you need to add proper check (i.e. if (...)). Cheers,
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index 2aed6ec707..f30f10d175 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -1791,8 +1791,16 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev, } res = platform_get_irq(dev, 0); - if ( ! res ) - return -EINVAL; + if ( res == -1 ) + { + printk("ns1650: polling will be used\n"); + /* + * There is the check 'if ( uart->irq > 0 )' in ns16550_init_postirq(). + * If the check is true then interrupt mode will be used otherwise + * ( when irq = 0 )polling. + */ + res = 0; + } uart->irq = res; uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
In ns16550_init_postirq() there is the following check: if ( uart->irq > 0 ) { uart->irqaction.handler = ns16550_interrupt; uart->irqaction.name = "ns16550"; uart->irqaction.dev_id = port; if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq); } Thereby to have ns16550 work in polling mode uart->irq, should be equal to 0. So it is needed to relax the following check in ns16550_uart_dt_init(): res = platform_get_irq(dev, 0); if ( ! res ) return -EINVAL; uart->irq = res; If 'res' equals to -1 then polling mode should be used instead of return -EINVAL. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/drivers/char/ns16550.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)