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

[Chen Baozi <baozich@xxxxxxxxx>]

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

> On Thu, 2013-08-08 at 18:52 +0800, Chen Baozi wrote:
> 
>> +#define omap_read(uart, off)       ioreadl((uart)->regs + (off<<REG_SHIFT))
>> +#define omap_write(uart, off, val) iowritel((uart)->regs + 
>> (off<<REG_SHIFT), (val))
> 
> Other than my comment made on #2 about either using DT or hardcoing
> REG_SHIFT in this driver I think this patch is good.
>> diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
>> index 33daa6d..7498e71 100644
>> --- a/xen/include/xen/8250-uart.h
>> +++ b/xen/include/xen/8250-uart.h
> 
> To what extent are these changes specific to the TI part?
> 
> I guess at least OMAP_* could be in its own header or in the .c file I
> guess? Not sure what others think about that?

Hmmm, in Linux, they are in one header (see comments below).

> 
>> @@ -63,14 +63,45 @@
>> #define UART_FCR_TRG8     0x80    /* Rx FIFO trig lev 8   */
>> #define UART_FCR_TRG14    0xc0    /* Rx FIFO trig lev 14  */
>> 
>> +/*
>> + * Note: The FIFO trigger levels are chip specific:
>> + *  RX:76 = 00  01  10  11  TX:54 = 00  01  10  11
>> + * PC16550D:         1   4   8  14          xx  xx  xx  xx
>> + * TI16C550A:        1   4   8  14          xx  xx  xx  xx
>> + * TI16C550C:        1   4   8  14          xx  xx  xx  xx
>> + * ST16C550:         1   4   8  14          xx  xx  xx  xx
>> + * ST16C650:         8  16  24  28          16   8  24  30  PORT_16650V2
>> + * NS16C552:         1   4   8  14          xx  xx  xx  xx
>> + * ST16C654:         8  16  56  60           8  16  32  56  PORT_16654
>> + * TI16C750:         1  16  32  56          xx  xx  xx  xx  PORT_16750
>> + * TI16C752:         8  16  56  60           8  16  32  56
>> + * Tegra:    1   4   8  14          16   8   4   1  PORT_TEGRA
>> + */
>> +#define UART_FCR_R_TRIG_00 0x00
>> +#define UART_FCR_R_TRIG_01 0x40
>> +#define UART_FCR_R_TRIG_10 0x80
>> +#define UART_FCR_R_TRIG_11 0xc0
>> +#define UART_FCR_T_TRIG_00 0x00
>> +#define UART_FCR_T_TRIG_01 0x10
>> +#define UART_FCR_T_TRIG_10 0x20
>> +#define UART_FCR_T_TRIG_11 0x30
> 
> This is inherently chipset specific but covers many so I this is OK here
> IMO.
> 
>> +
>> /* Line Control Register */
>> #define UART_LCR_DLAB     0x80    /* Divisor Latch Access */
>> 
>> +/*
>> + * Access to some registers depends on register access / configuration
>> + * mode.
> 
> This mode is a TI thing?

I took this defines from linux/include/linux/serial_reg.h. Though it is used
by OMAP5 board only in xen, it seems it is also used by other platform in
Linux (See drivers/tty/serial/8250/8250_core.c).

> 
>> + */
>> +#define UART_LCR_CONF_MODE_A        UART_LCR_DLAB   /* Configuration mode A 
>> */
>> +#define UART_LCR_CONF_MODE_B        0xBF            /* Configuration mode B 
>> */
>> +
>> /* Modem Control Register */
>> #define UART_MCR_DTR      0x01    /* Data Terminal Ready  */
>> #define UART_MCR_RTS      0x02    /* Request to Send      */
>> #define UART_MCR_OUT2     0x08    /* OUT2: interrupt mask */
>> #define UART_MCR_LOOP     0x10    /* Enable loopback test mode */
>> +#define UART_MCR_TCRTLR   0x40    /* Access TCR/TLR (TI16C752, EFR[4]=1) */
> 
> Also a TI thing? But I can see why you would want it here for
> consistency.
> 
>> 
>> /* Line Status Register */
>> #define UART_LSR_DR       0x01    /* Data ready           */
>> @@ -96,6 +127,26 @@
>> #define RESUME_DELAY      MILLISECS(10)
>> #define RESUME_RETRIES    100
>> 
>> +/* Enhanced feature register */
>> +#define UART_OMAP_EFR     0x02
>> +
>> +#define UART_OMAP_EFR_ECB 0x10 /* Enhanced control bit */
>> +
>> +/* Mode definition register 1 */
>> +#define UART_OMAP_MDR1    0x08
>> +
>> +/*
>> + * These are the definitions for the MDR1 register
>> + */
>> +#define UART_OMAP_MDR1_16X_MODE 0x00 /* UART 16x mode           */
>> +#define UART_OMAP_MDR1_DISABLE  0x07 /* Disable (default state) */
>> +
>> +/* Supplementary control register */
>> +#define UART_OMAP_SCR     0x10
>> +
>> +/* SCR register bitmasks */
>> +#define OMAP_UART_SCR_RX_TRIG_GRANU1_MASK (1 << 7)
>> +
>> #endif /* __XEN_8250_UART_H__ */
>> 
>> /*
> 
> 


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