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(-)
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");
--
2.41.0On 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
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, -- Julien Grall
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,
--
Julien Grall
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
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
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,
--
Julien Grall
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,
--
Julien Grall
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,
--
Julien Grall
© 2016 - 2024 Red Hat, Inc.