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

[Chen Baozi <baozich@xxxxxxxxx>]

On Aug 7, 2013, at 4:46 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:

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

After reading the Linux driver, I think the commit log here actually
means the DMA feature of OMAP change the setup flow.

Cheers,

Baozi

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