Re: [PATCH v3] ns16550: add support for polling mode when device tree is used

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2023-07-25 at 10:18 +0200, Jan Beulich wrote:
> On 25.07.2023 10:15, Oleksii wrote:
> > On Mon, 2023-07-24 at 16:43 +0200, Jan Beulich wrote:
> > > On 24.07.2023 16:27, Oleksii Kurochko wrote:
> > > > @@ -1330,9 +1341,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->intr_works = polling;
> > > > +                }
> > > >  #endif
> > > > -                if ( !uart->irq )
> > > > +                if ( !uart->irq || (uart->intr_works ==
> > > > polling) )
> > > >                      printk(XENLOG_INFO
> > > >                             "ns16550: %pp: no legacy IRQ, using
> > > > poll mode\n",
> > > >                             &PCI_SBDF(0, b, d, f));
> > > 
> > > Message and code (after your addition) continue to be out of
> > > sync.
> > > As said before, any condition that leads to polling mode wants to
> > > find itself expressed by uart->intr_works set to "polling".
> > Well. It looks like it would be better to set 'uart->intr_works =
> > polling' inside if ( !uart->irq ):
> >   if ( !uart->irq || (uart->intr_works == polling) )
> >   {
> >       uart->intr_works = polling;
> >       printk(XENLOG_INFO
> >              "ns16550: %pp: using poll mode\n",
> >              &PCI_SBDF(0, b, d, f));
> >   }
> > Then "uart->intr_works = polling;" can be removed from "if ( uart-
> > >irq
> > == 0xff )" as in that case 'uart->irq = 0' means polling mode for
> > X86.
> 
> I'm not fully convinced. Care needs to be taken that both the x86
> case
> and the general case continue to function as they did (unless proven
> to
> be buggy).
But it continues to work as it worked before ( when uart->intr_works !=
polling ) otherwise, if uart->intr_works = polling, then polling mode
is forced.

The only thing that I would probably add it is to force polling mode in
case of X86 when polling mode is forced by command line:
    if ( ( uart->irq == 0xff ) || (uart->intr_works == polling) )
    {
        uart->irq = 0;
        uart->intr_works = polling;
    }
But this check looks a little bit unnecessary because if the polling
mode is forced or not will be checked later in the next if condition.

~ Oleksii