Re: [Xen-devel] [PATCH 3/5] xen/arm: Add the new OMAP UART driver.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-08-07 at 11:14 +0800, Chen Baozi wrote:
> On Aug 7, 2013, at 3:14 AM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 
> > On 5 August 2013 12:49, Chen Baozi <baozich@xxxxxxxxx> wrote:
> >> TI OMAP UART introduces some features such as register access modes, which
> >> makes its configuration and interrupt handling differs from 8250 compatible
> >> UART. Thus, we seperate this driver from ns16550's implementation.
> > 
> > On your previous version of this patch series you use a modified ns16550,
> > why didn't you continue in the same way?
> 
> I used to think that OMAP UART is 8250 compatible with just a few
> additional registers. However, since I read the manual carefully last
> weekend, I found it rather different than a common 8250 UART, 
> especially switching different register access modes to do
> do initialization



>  and disable THRE interrupt after finishing tx.

This sounds like something which would perhaps be harmless on all other
systems too, or it could be implemented as a quirk within the existing
driver.

> Those features would make the modified ns16550 share less common
> codes between X86 and ARM. What's more, OMAP's UART codes cannot be
> reused on other ARM platform such as suni6. So I think it would
> be better to separate it into a new driver and leave ns16550 as the
> driver for a more common 8250 UART such as suni6. 

It looks like Linux has gone down the same path? The commit in Linux
says the reason has to do with DMA setup, and we likely wouldn't do DMA
console in the hypervisor.

Anyway, it's a shame if this code cannot be common but I suppose we can
live with it.

> > 
> >> +        lsr = uart->regs[UART_LSR] & 0xff;
> > 
> > Please use ioread{l,w,...} instead of uart->regs[...]. Same for
> > everywhere in you driver.
> 
> No problem.
> 
> Any potential hazard using uart->regs[...]?

It doesn't have any barriers (compiler or processor) and it isn't
guaranteed (although it is highly likely) that the compiler will
implement this as a simple store.

pl011 has switched too by the way.
> >> 
> >> +/*
> >> + * Access to some registers depends on register access / configuration
> >> + * mode.
> >> + */
> >> +#define UART_LCR_CONF_MODE_A   UART_LCR_DLAB   /* Configutation mode A */
> >> +#define UART_LCR_CONF_MODE_B   0xBF            /* Configutation mode B */

"Configuration" twice

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel