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

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

[Oleksandr Dmytryshyn]

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.

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__ */
 
 /*
-- 
1.9.1

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

[Ian Campbell]

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.

Konrad, this is a bug fix for a particular piece of hardware, it can't
affect anything other than the OMAP ARM platform.


> 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__ */
>  
>  /*

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

[Oleksandr Dmytryshyn]

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.
>
>
>> 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__ */
>>
>>  /*
>
>

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

[Konrad Rzeszutek Wilk]

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__ */
> >>
> >>  /*
> >
> >

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

[Ian Campbell]

On Tue, 2014-12-09 at 11:26 -0500, Konrad Rzeszutek Wilk wrote:

> > I'm fully happy with proposed wording (and want the both bits to be used)

Done and committed.

> > 
> > > 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>

Ian.