Re: [PATCH v2] xen/serial: setup UART idle mode for OMAP

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Dec 09, 2014 at 05:25:08PM +0200, Oleksandr Dmytryshyn wrote:
> On Tue, Dec 9, 2014 at 4:47 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-12-08 at 15:51 +0200, Oleksandr Dmytryshyn wrote:
> >> UART is not able to receive bytes when idle mode is not
> >> configured properly. When we use Xen with old Linux
> >> Kernel (for example 3.8) this kernel configures hwmods
> >> for all devices even if the device tree nodes for those
> >> devices is absent in device tree. Thus UART idle mode
> >> is configured too. The fake node for the UART should
> >> be added to the device tree because the MMIO range
> >> is not mapped by Xen. So UART works normally in this
> >> case. But new Linux Kernel (3.12 and upper) doesn't
> >> configure idle mode for UART and UART can not work
> >> normally in this case.
> >
> > I think the focus is too much on the hack done with 3.8 to make this
> > work rather than on the fix being made here itself. The hack is only
> > really of peripheral/historic interest.
> >
> > How about instead:
> >
> >         The UART is not able to receive bytes when idle mode is not
> >         configured properly, therefore setup the UART with autoidle and
> >         wakeup enabled.
> >
> > You could stop here or if you really want to cover the old hack you
> > could go on to say:
> >
> >         Older Linux kernels (for example 3.8) configures hwmods for all
> >         devices even if the device tree nodes for those devices is
> >         absent in device tree, thus UART idle mode is configured too.
> >         With such kernels we can workaround the issue by adding a fake
> >         node in the UART containing this MMIO range, which is therefore
> >         mapped by Xen to dom0, which reconfigures the UART, causing
> >         things to work normally.
> >
> >         Newer Linux Kernels (3.12 and beyond) do not configure idle mode
> >         for UART and so this hack no longer works.
> >
> > If you are happy with the proposed wording (and indicate whether you
> > want both bits or just the first) then, subject to Konrad giving a
> > release Ack I'd be happy with this for 4.5 and I'll change the commit
> > log as I go.
> I'm fully happy with proposed wording (and want the both bits to be used)
> 
> > Konrad, this is a bug fix for a particular piece of hardware, it can't
> > affect anything other than the OMAP ARM platform.

OK, RElease-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >
> >
> >> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> >
> >> ---
> >> Changed since v1:
> >>  * corrected commit message
> >>
> >>  xen/drivers/char/omap-uart.c | 3 +++
> >>  xen/include/xen/8250-uart.h  | 4 ++++
> >>  2 files changed, 7 insertions(+)
> >>
> >> diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
> >> index a798b8d..16d1454 100644
> >> --- a/xen/drivers/char/omap-uart.c
> >> +++ b/xen/drivers/char/omap-uart.c
> >> @@ -195,6 +195,9 @@ static void __init omap_uart_init_preirq(struct serial_port *port)
> >>      omap_write(uart, UART_MCR, UART_MCR_DTR|UART_MCR_RTS);
> >>
> >>      omap_write(uart, UART_OMAP_MDR1, UART_OMAP_MDR1_16X_MODE);
> >> +
> >> +    /* setup iddle mode */
> >> +    omap_write(uart, UART_SYSC, OMAP_UART_SYSC_DEF_CONF);
> >>  }
> >>
> >>  static void __init omap_uart_init_postirq(struct serial_port *port)
> >> diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
> >> index a682bae..304b9dd 100644
> >> --- a/xen/include/xen/8250-uart.h
> >> +++ b/xen/include/xen/8250-uart.h
> >> @@ -32,6 +32,7 @@
> >>  #define UART_MCR          0x04    /* Modem control        */
> >>  #define UART_LSR          0x05    /* line status          */
> >>  #define UART_MSR          0x06    /* Modem status         */
> >> +#define UART_SYSC         0x15    /* System configuration register */
> >>  #define UART_USR          0x1f    /* Status register (DW) */
> >>  #define UART_DLL          0x00    /* divisor latch (ls) (DLAB=1) */
> >>  #define UART_DLM          0x01    /* divisor latch (ms) (DLAB=1) */
> >> @@ -145,6 +146,9 @@
> >>  /* SCR register bitmasks */
> >>  #define OMAP_UART_SCR_RX_TRIG_GRANU1_MASK (1 << 7)
> >>
> >> +/* System configuration register */
> >> +#define OMAP_UART_SYSC_DEF_CONF 0x0d /* autoidle mode, wakeup is enabled */
> >> +
> >>  #endif /* __XEN_8250_UART_H__ */
> >>
> >>  /*
> >
> >