uart: pl011: Introduce register accessor
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v13 2/5 uart: pl011: Introduce register accessor
Hi Jun,
(CC:ing linux-arm-kernel to catch a broader audience)
I assume this is the latest version on the list? If I got the outcome of the discussion right, we do more review on this and merge an improved version later on.
Quoted 2 lines · Jun Nie
Introduce register accessor to ease loop up table access in later patch.
Wouldn't it make sense to make this the first patch? Then you could get rid of patching your own previous patch, possibly also joining patch 1/5 and 3/5 into a new 2/4.
More comments inline ...
Quoted 58 lines
Signed-off-by Jun Nie <[email protected]>Reviewed-by Peter Hurley <[email protected]>--- drivers/tty/serial/amba-pl011.c | 263 +++++++++++++++++++++------------------- 1 file changed, 141 insertions(+), 122 deletions(-)drivers/tty/serial/amba-pl011.c +20
-16@@ -85,23 +85,26 @@ struct vendor_data {unsigned int (*get_fifosize)(struct amba_device *dev); }; +/* Max address offset of register in use is 0x48 */ +#define REG_NR (0x48 >> 2) +#define IDX (x) (x >> 2)enum reg_idx { -REG_DR = UART01x_DR, -REG_RSR = UART01x_RSR, -REG_ST_DMAWM =ST_UART011_DMAWM , -REG_FR = UART01x_FR, -REG_ST_LCRH_RX =ST_UART011_LCRH_RX , -REG_ILPR = UART01x_ILPR, -REG_IBRD =UART011_IBRD , -REG_FBRD =UART011_FBRD , -REG_LCRH =UART011_LCRH , -REG_CR =UART011_CR , -REG_IFLS =UART011_IFLS , -REG_IMSC =UART011_IMSC , -REG_RIS =UART011_RIS , -REG_MIS =UART011_MIS , -REG_ICR =UART011_ICR , -REG_DMACR =UART011_DMACR , +REG_DR =IDX (UART01x_DR), +REG_RSR =IDX (UART01x_RSR), +REG_ST_DMAWM =IDX (ST_UART011_DMAWM ), +REG_FR =IDX (UART01x_FR), +REG_ST_LCRH_RX =IDX (ST_UART011_LCRH_RX ), +REG_ILPR =IDX (UART01x_ILPR), +REG_IBRD =IDX (UART011_IBRD ), +REG_FBRD =IDX (UART011_FBRD ), +REG_LCRH =IDX (UART011_LCRH ), +REG_CR =IDX (UART011_CR ), +REG_IFLS =IDX (UART011_IFLS ), +REG_IMSC =IDX (UART011_IMSC ), +REG_RIS =IDX (UART011_RIS ), +REG_MIS =IDX (UART011_MIS ), +REG_ICR =IDX (UART011_ICR ), +REG_DMACR =IDX (UART011_DMACR ), };static unsigned int get_fifosize_arm (struct amba_device *dev)@@ -203,6 +206,24 @@ struct uart_amba_port {#endif }; +static unsigned int pl011_readw (struct uart_amba_port *uap,int index)
drivers/tty/serial/amba-pl011.c struct uart_amba_port { · line 219
Quoted 10 lines
drivers/tty/serial/amba-pl011.c +10
-0+{ +WARN_ON (index >REG_NR ); +return readw_relaxed (uap->port.membase + (index <<2 )); +} + +static void pl011_writew (struct uart_amba_port *uap,int val,int index) +{ +WARN_ON (index >REG_NR ); +writew_relaxed (val, uap->port.membase + (index <<2 )); +}
I wonder if you could rename those to pl011_{read,write}, respectively (loosing the "w" suffix).
The SBSA UART spec reads as the registers are actually accessible via 32-bit accesses and rumour has it that there are implementations which rely on that and don't work with ldrh/strh.
I am still waiting for reports about actual hardware to fail, but we might be forced to change the access width to 32-bit for the SBSA subset in the future. So having wrapper functions would make that change much easier, but having them without a suffix from the beginning would even be better, as I wouldn't be bothered to rename them later on.
drivers/tty/serial/amba-pl011.c struct uart_amba_port { · line 226
Quoted 7 lines
drivers/tty/serial/amba-pl011.c +7
-0+ +static void pl011_writeb (struct uart_amba_port *uap,u8 val,int index) +{ +WARN_ON (index >REG_NR ); +writeb_relaxed (val, uap->port.membase + (index <<2 )); +} +
As you already know, this one can go, as we don't need it.
Quoted 674 lines
drivers/tty/serial/amba-pl011.c +97
-102/* * Reads up to 256 characters from the FIFO or until it's empty and * inserts them into the TTY layer. Returns the number of characters @@ -215,12 +236,12 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap)int fifotaken =0 ;while (max_count--) { - status =readw (uap->port.membase +REG_FR ); + status =pl011_readw (uap,REG_FR );if (status & UART01x_FR_RXFE)break ;/* Take chars from the FIFO and update status */ - ch =readw (uap->port.membase +REG_DR ) | + ch =pl011_readw (uap,REG_DR ) |UART_DUMMY_DR_RX ; flag =TTY_NORMAL ; uap->port.icount.rx++;@@ -457,7 +478,7 @@ static void pl011_dma_tx_callback(void *data) dmacr = uap->dmacr; uap->dmacr = dmacr & ~UART011_TXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR );/* * If TX DMA was disabled, it means that we've stopped the DMA for @@ -571,7 +592,7 @@ static int pl011_dma_tx_refill(struct uart_amba_port *uap) dma_dev->device_issue_pending (chan); uap->dmacr |=UART011_TXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); uap->dmatx.queued = true;/* @@ -607,9 +628,9 @@ static bool pl011_dma_tx_irq(struct uart_amba_port *uap)*/ if (uap->dmatx.queued) { uap->dmacr |=UART011_TXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); uap->im &= ~UART011_TXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );return true; }@@ -619,7 +640,7 @@ static bool pl011_dma_tx_irq(struct uart_amba_port *uap)*/ if (pl011_dma_tx_refill (uap) >0 ) { uap->im &= ~UART011_TXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );return true; }return false;@@ -633,7 +654,7 @@ static inline void pl011_dma_tx_stop(struct uart_amba_port *uap) {if (uap->dmatx.queued) { uap->dmacr &= ~UART011_TXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); } }@@ -659,14 +680,12 @@ static inline bool pl011_dma_tx_start(struct uart_amba_port *uap)if (!uap->dmatx.queued) {if (pl011_dma_tx_refill (uap) >0 ) { uap->im &= ~UART011_TXIM ; -writew (uap->im, uap->port.membase + -REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC ); }else ret = false; }else if (!(uap->dmacr &UART011_TXDMAE )) { uap->dmacr |=UART011_TXDMAE ; -writew (uap->dmacr, - uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); }return ret; }@@ -677,9 +696,9 @@ static inline bool pl011_dma_tx_start(struct uart_amba_port *uap)*/ dmacr = uap->dmacr; uap->dmacr &= ~UART011_TXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); -if (readw (uap->port.membase +REG_FR ) & UART01x_FR_TXFF) { +if (pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF) {/* * No space in the FIFO, so enable the transmit interrupt * so we know when there is space. Note that once we've @@ -688,13 +707,13 @@ static inline bool pl011_dma_tx_start(struct uart_amba_port *uap)return false; } -writew (uap->port.x_char, uap->port.membase +REG_DR ); +pl011_writew (uap, uap->port.x_char,REG_DR ); uap->port.icount.tx++; uap->port.x_char =0 ;/* Success - restore the DMA state */ uap->dmacr = dmacr; -writew (dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, dmacr,REG_DMACR );return true; }@@ -722,7 +741,7 @@ __acquires(&uap->port.lock) DMA_TO_DEVICE); uap->dmatx.queued = false; uap->dmacr &= ~UART011_TXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); } }@@ -762,11 +781,11 @@ static int pl011_dma_rx_trigger_dma(struct uart_amba_port *uap)dma_async_issue_pending (rxchan); uap->dmacr |=UART011_RXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); uap->dmarx.running = true; uap->im &= ~UART011_RXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );return 0 ; }@@ -824,8 +843,9 @@ static void pl011_dma_rx_chars(struct uart_amba_port *uap,*/ if (dma_count == pending && readfifo) {/* Clear any error flags */ -writew (UART011_OEIS |UART011_BEIS |UART011_PEIS |UART011_FEIS , - uap->port.membase +REG_ICR ); +pl011_writew (uap, +UART011_OEIS |UART011_BEIS |UART011_PEIS + |UART011_FEIS ,REG_ICR );/* * If we read all the DMA'd characters, and we had an @@ -873,7 +893,7 @@ static void pl011_dma_rx_irq(struct uart_amba_port *uap)/* Disable RX DMA - incoming data will wait in the FIFO */ uap->dmacr &= ~UART011_RXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); uap->dmarx.running = false; pending = sgbuf->sg.length - state.residue;@@ -893,7 +913,7 @@ static void pl011_dma_rx_irq(struct uart_amba_port *uap)dev_dbg (uap->port.dev,"could not retrigger RX DMA job " "fall back to interrupt mode\n" ); uap->im |=UART011_RXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC ); } }@@ -941,7 +961,7 @@ static void pl011_dma_rx_callback(void *data)dev_dbg (uap->port.dev,"could not retrigger RX DMA job " "fall back to interrupt mode\n" ); uap->im |=UART011_RXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC ); } }@@ -954,7 +974,7 @@ static inline void pl011_dma_rx_stop(struct uart_amba_port *uap) {/* FIXME. Just disable the DMA enable */ uap->dmacr &= ~UART011_RXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); }/* @@ -998,7 +1018,7 @@ static void pl011_dma_rx_poll(unsigned long args)spin_lock_irqsave (&uap->port.lock, flags);pl011_dma_rx_stop (uap); uap->im |=UART011_RXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );spin_unlock_irqrestore (&uap->port.lock, flags); uap->dmarx.running = false;@@ -1060,7 +1080,7 @@ static void pl011_dma_startup(struct uart_amba_port *uap) skip_rx:/* Turn on DMA error (RX/TX will be enabled on demand) */ uap->dmacr |=UART011_DMAONERR ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR );/* * ST Micro variants has some specific dma burst threshold @@ -1068,9 +1088,9 @@ skip_rx:* be issued above/below 16 bytes. */ if (uap->vendor->dma_threshold) -writew (ST_UART011_DMAWM_RX_16 |ST_UART011_DMAWM_TX_16 , - uap->port.membase +REG_ST_DMAWM ); - +pl011_writew (uap, +ST_UART011_DMAWM_RX_16 |ST_UART011_DMAWM_TX_16 , +REG_ST_DMAWM );if (uap->using_rx_dma) {if (pl011_dma_rx_trigger_dma (uap))@@ -1095,12 +1115,12 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap)return ;/* Disable RX and TX DMA */ -while (readw (uap->port.membase +REG_FR ) & UART01x_FR_BUSY) +while (pl011_readw (uap,REG_FR ) & UART01x_FR_BUSY)barrier ();spin_lock_irq (&uap->port.lock); uap->dmacr &= ~(UART011_DMAONERR |UART011_RXDMAE |UART011_TXDMAE ); -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR );spin_unlock_irq (&uap->port.lock);if (uap->using_tx_dma) {@@ -1201,7 +1221,7 @@ static void pl011_stop_tx(struct uart_port *port) container_of(port, struct uart_amba_port, port); uap->im &= ~UART011_TXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );pl011_dma_tx_stop (uap); }@@ -1211,7 +1231,7 @@ static void pl011_tx_chars(struct uart_amba_port *uap, bool from_irq);static void pl011_start_tx_pio (struct uart_amba_port *uap) { uap->im |=UART011_TXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );pl011_tx_chars (uap, false); }@@ -1231,7 +1251,7 @@ static void pl011_stop_rx(struct uart_port *port) uap->im &= ~(UART011_RXIM |UART011_RTIM |UART011_FEIM |UART011_PEIM |UART011_BEIM |UART011_OEIM ); -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );pl011_dma_rx_stop (uap); }@@ -1242,7 +1262,7 @@ static void pl011_enable_ms(struct uart_port *port) container_of(port, struct uart_amba_port, port); uap->im |=UART011_RIMIM |UART011_CTSMIM |UART011_DCDMIM |UART011_DSRMIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC ); }static void pl011_rx_chars (struct uart_amba_port *uap)@@ -1262,7 +1282,7 @@ __acquires(&uap->port.lock)dev_dbg (uap->port.dev,"could not trigger RX DMA job " "fall back to interrupt mode again\n" ); uap->im |=UART011_RXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC ); }else {#ifdef CONFIG_DMA_ENGINE /* Start Rx DMA poll */ @@ -1283,10 +1303,10 @@ static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c, bool from_irq) {if (unlikely (!from_irq) && -readw (uap->port.membase +REG_FR ) & UART01x_FR_TXFF) +pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF)return false;/* unable to transmit character */ -writew (c, uap->port.membase +REG_DR ); +pl011_writew (uap, c,REG_DR ); uap->port.icount.tx++;return true;@@ -1333,7 +1353,7 @@ static void pl011_modem_status(struct uart_amba_port *uap) {unsigned int status, delta; - status =readw (uap->port.membase +REG_FR ) & UART01x_FR_MODEM_ANY; + status =pl011_readw (uap,REG_FR ) & UART01x_FR_MODEM_ANY; delta = status ^ uap->old_status; uap->old_status = status;@@ -1361,15 +1381,15 @@ static void check_apply_cts_event_workaround(struct uart_amba_port *uap)return ;/* workaround to make sure that all bits are unlocked.. */ -writew (0x00 , uap->port.membase +REG_ICR ); +pl011_writew (uap,0x00 ,REG_ICR );/* * WA: introduce 26ns(1 uart clk) delay before W1C; * single apb access will incur 2 pclk(133.12Mhz) delay, * so add 2 dummy reads */ - dummy_read =readw (uap->port.membase +REG_ICR ); - dummy_read =readw (uap->port.membase +REG_ICR ); + dummy_read =pl011_readw (uap,REG_ICR ); + dummy_read =pl011_readw (uap,REG_ICR ); }static irqreturn_t pl011_int (int irq,void *dev_id)@@ -1381,15 +1401,13 @@ static irqreturn_t pl011_int(int irq, void *dev_id)int handled =0 ;spin_lock_irqsave (&uap->port.lock, flags); - imsc =readw (uap->port.membase +REG_IMSC ); - status =readw (uap->port.membase +REG_RIS ) & imsc; + imsc =pl011_readw (uap,REG_IMSC ); + status =pl011_readw (uap,REG_RIS ) & imsc;if (status) { do { check_apply_cts_event_workaround(uap); - - writew(status & ~(UART011_TXIS|UART011_RTIS| - UART011_RXIS), - uap->port.membase + REG_ICR); + pl011_writew(uap, status & ~(UART011_TXIS|UART011_RTIS| + UART011_RXIS), REG_ICR); if (status & (UART011_RTIS|UART011_RXIS)) { if (pl011_dma_rx_running(uap))@@ -1406,7 +1424,7 @@ static irqreturn_t pl011_int(int irq, void *dev_id)if (pass_counter-- ==0 )break ; - status =readw (uap->port.membase +REG_RIS ) & imsc; + status =pl011_readw (uap,REG_RIS ) & imsc; }while (status !=0 ); handled =1 ; }@@ -1420,7 +1438,7 @@ static unsigned int pl011_tx_empty(struct uart_port *port) {struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); -unsigned int status =readw (uap->port.membase +REG_FR ); +unsigned int status =pl011_readw (uap,REG_FR );return status & (UART01x_FR_BUSY|UART01x_FR_TXFF) ?0 :TIOCSER_TEMT ; }@@ -1429,7 +1447,7 @@ static unsigned int pl011_get_mctrl(struct uart_port *port)struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port);unsigned int result =0 ; -unsigned int status =readw (uap->port.membase +REG_FR ); +unsigned int status =pl011_readw (uap,REG_FR );#define TIOCMBIT (uartbit, tiocmbit) \ if (status & uartbit) \@@ -1449,7 +1467,7 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl) container_of(port, struct uart_amba_port, port);unsigned int cr; - cr =readw (uap->port.membase +REG_CR ); + cr =pl011_readw (uap,REG_CR );#define TIOCMBIT (tiocmbit, uartbit) \ if (mctrl & tiocmbit) \@@ -1469,7 +1487,7 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl) }#undef TIOCMBIT -writew (cr, uap->port.membase +REG_CR ); +pl011_writew (uap, cr,REG_CR ); }static void pl011_break_ctl (struct uart_port *port,int break_state)@@ -1480,12 +1498,12 @@ static void pl011_break_ctl(struct uart_port *port, int break_state)unsigned int lcr_h;spin_lock_irqsave (&uap->port.lock, flags); - lcr_h =readw (uap->port.membase + uap->lcrh_tx); + lcr_h =pl011_readw (uap, uap->lcrh_tx);if (break_state ==-1 ) lcr_h |= UART01x_LCRH_BRK;else lcr_h &= ~UART01x_LCRH_BRK; -writew (lcr_h, uap->port.membase + uap->lcrh_tx); +pl011_writew (uap, lcr_h, uap->lcrh_tx);spin_unlock_irqrestore (&uap->port.lock, flags); }@@ -1495,9 +1513,8 @@ static void pl011_quiesce_irqs(struct uart_port *port) {struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); -unsigned char __iomem *regs = uap->port.membase; -writew (readw (regs +REG_MIS ), regs +REG_ICR ); +pl011_writew (uap,pl011_readw (uap,REG_MIS ),REG_ICR );/* * There is no way to clear TXIM as this is "ready to transmit IRQ", so * we simply mask it. start_tx() will unmask it. @@ -1511,7 +1528,7 @@ static void pl011_quiesce_irqs(struct uart_port *port)* (including tx queue), so we're also fine with start_tx()'s caller * side. */ -writew (readw (regs +REG_IMSC ) & ~UART011_TXIM , regs +REG_IMSC ); +pl011_writew (uap,pl011_readw (uap,REG_IMSC ) & ~UART011_TXIM ,REG_IMSC ); }static int pl011_get_poll_char (struct uart_port *port)@@ -1526,11 +1543,11 @@ static int pl011_get_poll_char(struct uart_port *port)*/ pl011_quiesce_irqs (port); - status =readw (uap->port.membase +REG_FR ); + status =pl011_readw (uap,REG_FR );if (status & UART01x_FR_RXFE)return NO_POLL_CHAR ; -return readw (uap->port.membase +REG_DR ); +return pl011_readw (uap,REG_DR ); } static void pl011_put_poll_char(struct uart_port *port,@@ -1539,10 +1556,10 @@ static void pl011_put_poll_char(struct uart_port *port,struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); -while (readw (uap->port.membase +REG_FR ) & UART01x_FR_TXFF) +while (pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF)barrier (); - writew(ch, uap->port.membase + REG_DR); + pl011_writew(uap, ch, REG_DR); }#endif /* CONFIG_CONSOLE_POLL */ @@ -1566,15 +1583,15 @@ static int pl011_hwinit(struct uart_port *port) uap->port.uartclk =clk_get_rate (uap->clk);/* Clear pending error and receive interrupts */ -writew (UART011_OEIS |UART011_BEIS |UART011_PEIS |UART011_FEIS | -UART011_RTIS |UART011_RXIS , uap->port.membase +REG_ICR ); +pl011_writew (uap,UART011_OEIS |UART011_BEIS |UART011_PEIS | +UART011_FEIS |UART011_RTIS |UART011_RXIS ,REG_ICR );/* * Save interrupts enable mask, and enable RX interrupts in case if * the interrupt is used for NMI entry. */ - uap->im =readw (uap->port.membase +REG_IMSC ); -writew (UART011_RTIM |UART011_RXIM , uap->port.membase +REG_IMSC ); + uap->im =pl011_readw (uap,REG_IMSC ); +pl011_writew (uap,UART011_RTIM |UART011_RXIM ,REG_IMSC );if (dev_get_platdata (uap->port.dev)) {struct amba_pl011_data *plat;@@ -1588,7 +1605,7 @@ static int pl011_hwinit(struct uart_port *port)static void pl011_write_lcr_h (struct uart_amba_port *uap,unsigned int lcr_h) { -writew (lcr_h, uap->port.membase + uap->lcrh_rx); +pl011_writew (uap, lcr_h, uap->lcrh_rx);if (uap->lcrh_rx != uap->lcrh_tx) {int i;/* @@ -1596,14 +1613,14 @@ static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h)* to get this delay write read only register 10 times */ for (i =0 ; i <10 ; ++i) -writew (0xff , uap->port.membase +REG_MIS ); -writew (lcr_h, uap->port.membase + uap->lcrh_tx); +pl011_writew (uap,0xff ,REG_MIS ); +pl011_writew (uap, lcr_h, uap->lcrh_tx); } }static int pl011_allocate_irq (struct uart_amba_port *uap) { -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );return request_irq (uap->port.irq, pl011_int,0 ,"uart-pl011" , uap); }@@ -1618,12 +1635,11 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap)spin_lock_irq (&uap->port.lock);/* Clear out any spuriously appearing RX interrupts */ -writew (UART011_RTIS |UART011_RXIS , - uap->port.membase +REG_ICR ); +pl011_writew (uap,UART011_RTIS |UART011_RXIS ,REG_ICR ); uap->im =UART011_RTIM ;if (!pl011_dma_rx_running (uap)) uap->im |=UART011_RXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );spin_unlock_irq (&uap->port.lock); }@@ -1642,21 +1658,21 @@ static int pl011_startup(struct uart_port *port)if (retval) goto clk_dis; -writew (uap->vendor->ifls, uap->port.membase +REG_IFLS ); +pl011_writew (uap, uap->vendor->ifls,REG_IFLS );spin_lock_irq (&uap->port.lock);/* restore RTS and DTR */ cr = uap->old_cr & (UART011_CR_RTS |UART011_CR_DTR ); cr |= UART01x_CR_UARTEN |UART011_CR_RXE |UART011_CR_TXE ; -writew (cr, uap->port.membase +REG_CR ); +pl011_writew (uap, cr,REG_CR );spin_unlock_irq (&uap->port.lock);/* * initialise the old status of the modem signals */ - uap->old_status =readw (uap->port.membase +REG_FR ) & + uap->old_status =pl011_readw (uap,REG_FR ) & UART01x_FR_MODEM_ANY;/* Startup DMA */ @@ -1696,11 +1712,11 @@ static int sbsa_uart_startup(struct uart_port *port)static void pl011_shutdown_channel (struct uart_amba_port *uap,unsigned int lcrh) { -unsigned long val; +unsigned long val; - val =readw (uap->port.membase + lcrh); - val &= ~(UART01x_LCRH_BRK | UART01x_LCRH_FEN); -writew (val, uap->port.membase + lcrh); + val =pl011_readw (uap, lcrh); + val &= ~(UART01x_LCRH_BRK | UART01x_LCRH_FEN); +pl011_writew (uap, val, lcrh); }/* @@ -1714,11 +1730,11 @@ static void pl011_disable_uart(struct uart_amba_port *uap) uap->autorts = false;spin_lock_irq (&uap->port.lock); - cr =readw (uap->port.membase +REG_CR ); + cr =pl011_readw (uap,REG_CR ); uap->old_cr = cr; cr &=UART011_CR_RTS |UART011_CR_DTR ; cr |= UART01x_CR_UARTEN |UART011_CR_TXE ; -writew (cr, uap->port.membase +REG_CR ); +pl011_writew (uap, cr,REG_CR );spin_unlock_irq (&uap->port.lock);/* @@ -1735,8 +1751,8 @@ static void pl011_disable_interrupts(struct uart_amba_port *uap)/* mask all interrupts and clear all pending ones */ uap->im =0 ; -writew (uap->im, uap->port.membase +REG_IMSC ); -writew (0xffff , uap->port.membase +REG_ICR ); +pl011_writew (uap, uap->im,REG_IMSC ); +pl011_writew (0xffff ,REG_ICR );spin_unlock_irq (&uap->port.lock); }@@ -1888,8 +1904,8 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,pl011_enable_ms (port);/* first, disable everything */ - old_cr =readw (port->membase +REG_CR ); -writew (0 , port->membase +REG_CR ); + old_cr =pl011_readw (uap,REG_CR ); +pl011_writew (uap,0 ,REG_CR );if (termios->c_cflag &CRTSCTS ) {if (old_cr &UART011_CR_RTS )@@ -1922,8 +1938,8 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios, quot -=2 ; }/* Set baud rate */ -writew (quot &0x3f , port->membase +REG_FBRD ); -writew (quot >>6 , port->membase +REG_IBRD ); +pl011_writew (uap, quot &0x3f ,REG_FBRD ); +pl011_writew (uap, quot >>6 ,REG_IBRD );/* * ----------v----------v----------v----------v----- @@ -1932,7 +1948,7 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,* ----------^----------^----------^----------^----- */ pl011_write_lcr_h (uap, lcr_h); -writew (old_cr, port->membase +REG_CR ); +pl011_writew (uap, old_cr,REG_CR );spin_unlock_irqrestore (&port->lock, flags); }@@ -2073,9 +2089,9 @@ static void pl011_console_putchar(struct uart_port *port, int ch)struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); -while (readw (uap->port.membase +REG_FR ) & UART01x_FR_TXFF) +while (pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF)barrier (); -writew (ch, uap->port.membase +REG_DR ); +pl011_writew (uap, ch,REG_DR ); } static void@@ -2100,10 +2116,10 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)* First save the CR then disable the interrupts */ if (!uap->vendor->always_enabled) { - old_cr =readw (uap->port.membase +REG_CR ); + old_cr =pl011_readw (uap,REG_CR ); new_cr = old_cr & ~UART011_CR_CTSEN ; new_cr |= UART01x_CR_UARTEN |UART011_CR_TXE ; -writew (new_cr, uap->port.membase +REG_CR ); +pl011_writew (uap, new_cr,REG_CR ); }uart_console_write (&uap->port, s, count, pl011_console_putchar);@@ -2113,10 +2129,10 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)* and restore the TCR */ do { - status =readw (uap->port.membase +REG_FR ); + status =pl011_readw (uap,REG_FR ); }while (status & UART01x_FR_BUSY);if (!uap->vendor->always_enabled) -writew (old_cr, uap->port.membase +REG_CR ); +pl011_writew (uap, old_cr,REG_CR );if (locked)spin_unlock (&uap->port.lock);@@ -2129,10 +2145,10 @@ static void __init pl011_console_get_options(struct uart_amba_port *uap, int *baud, int *parity, int *bits) { - if (readw(uap->port.membase + REG_CR) & UART01x_CR_UARTEN) { + if (pl011_readw(uap, REG_CR) & UART01x_CR_UARTEN) { unsigned int lcr_h, ibrd, fbrd; - lcr_h =readw (uap->port.membase + uap->lcrh_tx); + lcr_h =pl011_readw (uap, uap->lcrh_tx); *parity ='n' ;if (lcr_h & UART01x_LCRH_PEN) {@@ -2147,13 +2163,13 @@ pl011_console_get_options(struct uart_amba_port *uap, int *baud,else *bits =8 ; - ibrd =readw (uap->port.membase +REG_IBRD ); - fbrd =readw (uap->port.membase +REG_FBRD ); + ibrd =pl011_readw (uap,REG_IBRD ); + fbrd =pl011_readw (uap,REG_FBRD ); *baud = uap->port.uartclk *4 / (64 * ibrd + fbrd);if (uap->vendor->oversampling) { -if (readw (uap->port.membase +REG_CR ) +if (pl011_readw (uap,REG_CR ) &ST_UART011_CR_OVSFACT ) *baud *=2 ; }@@ -2225,10 +2241,13 @@ static struct console amba_console = {static void pl011_putc (struct uart_port *port,int c) { -while (readl (port->membase +REG_FR ) & UART01x_FR_TXFF) + struct uart_amba_port *uap =
drivers/tty/serial/amba-pl011.c static struct console amba_console = { · line 2252
Quoted 10 lines
drivers/tty/serial/amba-pl011.c +5
-2+ container_of(port, struct uart_amba_port, port); + +while (pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF) ; -writeb (c, port->membase +REG_DR ); -while (readl (port->membase +REG_FR ) & UART01x_FR_BUSY) +pl011_writeb (uap, c,REG_DR ); +while (pl011_readw (uap,REG_FR ) & UART01x_FR_BUSY) ; }
Just for the records, as this has been discussed before: pl011_putc() is called by the earlycon code, before the uart_port is actually initialized. So we cannot rely on the accessors, but have to use the old-fashioned director accessors for this.
Which means you cannot use that approach to get earlycon support for the ZTE UART, if I get this correctly. It shouldn't be to hard to introduce another earlycon type specificly for that one, copying pl011_early_write and pl011_early_console_setup and changing pl011_putc into zte_uart_putc. But of course this belongs into the final patch (or a separate one), not in this. So I guess you just leave that function unchanged in this patch.
As a side note: I wonder why those accessors here are actually 32-bit wide here, and the data register 8-bits only?
This is in contrast to the rest of the driver, which uses 16-bit accesses all of the time (in line with the spec).
Cheers,
Andre.
Quoted 15 lines
drivers/tty/serial/amba-pl011.c +2
-3@@ -2355,8 +2374,8 @@ static int pl011_register_port(struct uart_amba_port *uap)int ret;/* Ensure interrupts from this UART are masked and cleared */ -writew (0 , uap->port.membase +REG_IMSC ); -writew (0xffff , uap->port.membase +REG_ICR ); +pl011_writew (uap,0 ,REG_IMSC ); +pl011_writew (uap,0xffff ,REG_ICR );if (!amba_reg.state) { ret =uart_register_driver (&amba_reg); --1.9.1
PATCH v13 2/5 uart: pl011: Introduce register accessor
2015-09-18 18:51 GMT+08:00 Andre Przywara <[email protected]>:
Quoted 7 lines
Hi Jun,
(CC:ing linux-arm-kernel to catch a broader audience)
I assume this is the latest version on the list? If I got the outcome of the discussion right, we do more review on this and merge an improved version later on.
Yes, version 13 is the latest version patch. Thanks for you comments and I add reply inline.
Quoted 8 lines · Jun Nie
Quoted 2 lines
Introduce register accessor to ease loop up table access in later patch.
Wouldn't it make sense to make this the first patch? Then you could get rid of patching your own previous patch, possibly also joining patch 1/5 and 3/5 into a new 2/4.
The previous patch rename register to general name so that register table can hide the vendor specific offset related name difference. If get rid of it, you want to use current name and just change specific names and join it into later patch?
Quoted 74 lines
More comments inline ...
Quoted 58 lines
Signed-off-by Jun Nie <[email protected]>Reviewed-by Peter Hurley <[email protected]>--- drivers/tty/serial/amba-pl011.c | 263 +++++++++++++++++++++------------------- 1 file changed, 141 insertions(+), 122 deletions(-)drivers/tty/serial/amba-pl011.c +20
-16@@ -85,23 +85,26 @@ struct vendor_data {unsigned int (*get_fifosize)(struct amba_device *dev); }; +/* Max address offset of register in use is 0x48 */ +#define REG_NR (0x48 >> 2) +#define IDX (x) (x >> 2)enum reg_idx { -REG_DR = UART01x_DR, -REG_RSR = UART01x_RSR, -REG_ST_DMAWM =ST_UART011_DMAWM , -REG_FR = UART01x_FR, -REG_ST_LCRH_RX =ST_UART011_LCRH_RX , -REG_ILPR = UART01x_ILPR, -REG_IBRD =UART011_IBRD , -REG_FBRD =UART011_FBRD , -REG_LCRH =UART011_LCRH , -REG_CR =UART011_CR , -REG_IFLS =UART011_IFLS , -REG_IMSC =UART011_IMSC , -REG_RIS =UART011_RIS , -REG_MIS =UART011_MIS , -REG_ICR =UART011_ICR , -REG_DMACR =UART011_DMACR , +REG_DR =IDX (UART01x_DR), +REG_RSR =IDX (UART01x_RSR), +REG_ST_DMAWM =IDX (ST_UART011_DMAWM ), +REG_FR =IDX (UART01x_FR), +REG_ST_LCRH_RX =IDX (ST_UART011_LCRH_RX ), +REG_ILPR =IDX (UART01x_ILPR), +REG_IBRD =IDX (UART011_IBRD ), +REG_FBRD =IDX (UART011_FBRD ), +REG_LCRH =IDX (UART011_LCRH ), +REG_CR =IDX (UART011_CR ), +REG_IFLS =IDX (UART011_IFLS ), +REG_IMSC =IDX (UART011_IMSC ), +REG_RIS =IDX (UART011_RIS ), +REG_MIS =IDX (UART011_MIS ), +REG_ICR =IDX (UART011_ICR ), +REG_DMACR =IDX (UART011_DMACR ), };static unsigned int get_fifosize_arm (struct amba_device *dev)@@ -203,6 +206,24 @@ struct uart_amba_port {#endif }; +static unsigned int pl011_readw (struct uart_amba_port *uap,int index)drivers/tty/serial/amba-pl011.c struct uart_amba_port { · line 219
Quoted 10 lines
drivers/tty/serial/amba-pl011.c +10
-0+{ +WARN_ON (index >REG_NR ); +return readw_relaxed (uap->port.membase + (index <<2 )); +} + +static void pl011_writew (struct uart_amba_port *uap,int val,int index) +{ +WARN_ON (index >REG_NR ); +writew_relaxed (val, uap->port.membase + (index <<2 )); +}I wonder if you could rename those to pl011_{read,write}, respectively (loosing the "w" suffix).
Quoted 8 lines
The SBSA UART spec reads as the registers are actually accessible via 32-bit accesses and rumour has it that there are implementations which rely on that and don't work with ldrh/strh.
I am still waiting for reports about actual hardware to fail, but we might be forced to change the access width to 32-bit for the SBSA subset in the future. So having wrapper functions would make that change much easier, but having them without a suffix from the beginning would even be better, as I wouldn't be bothered to rename them later on.
OK, will change to 32-bit access in future version.
Quoted 10 lines
drivers/tty/serial/amba-pl011.c struct uart_amba_port { · line 226
Quoted 8 lines
drivers/tty/serial/amba-pl011.c +7
-0+ +static void pl011_writeb (struct uart_amba_port *uap,u8 val,int index) +{ +WARN_ON (index >REG_NR ); +writeb_relaxed (val, uap->port.membase + (index <<2 )); +} +As you already know, this one can go, as we don't need it.
Right.
Quoted 691 lines
Quoted 675 lines
drivers/tty/serial/amba-pl011.c +97
-102/* * Reads up to 256 characters from the FIFO or until it's empty and * inserts them into the TTY layer. Returns the number of characters @@ -215,12 +236,12 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap)int fifotaken =0 ;while (max_count--) { - status =readw (uap->port.membase +REG_FR ); + status =pl011_readw (uap,REG_FR );if (status & UART01x_FR_RXFE)break ;/* Take chars from the FIFO and update status */ - ch =readw (uap->port.membase +REG_DR ) | + ch =pl011_readw (uap,REG_DR ) |UART_DUMMY_DR_RX ; flag =TTY_NORMAL ; uap->port.icount.rx++;@@ -457,7 +478,7 @@ static void pl011_dma_tx_callback(void *data) dmacr = uap->dmacr; uap->dmacr = dmacr & ~UART011_TXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR );/* * If TX DMA was disabled, it means that we've stopped the DMA for @@ -571,7 +592,7 @@ static int pl011_dma_tx_refill(struct uart_amba_port *uap) dma_dev->device_issue_pending (chan); uap->dmacr |=UART011_TXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); uap->dmatx.queued = true;/* @@ -607,9 +628,9 @@ static bool pl011_dma_tx_irq(struct uart_amba_port *uap)*/ if (uap->dmatx.queued) { uap->dmacr |=UART011_TXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); uap->im &= ~UART011_TXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );return true; }@@ -619,7 +640,7 @@ static bool pl011_dma_tx_irq(struct uart_amba_port *uap)*/ if (pl011_dma_tx_refill (uap) >0 ) { uap->im &= ~UART011_TXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );return true; }return false;@@ -633,7 +654,7 @@ static inline void pl011_dma_tx_stop(struct uart_amba_port *uap) {if (uap->dmatx.queued) { uap->dmacr &= ~UART011_TXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); } }@@ -659,14 +680,12 @@ static inline bool pl011_dma_tx_start(struct uart_amba_port *uap)if (!uap->dmatx.queued) {if (pl011_dma_tx_refill (uap) >0 ) { uap->im &= ~UART011_TXIM ; -writew (uap->im, uap->port.membase + -REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC ); }else ret = false; }else if (!(uap->dmacr &UART011_TXDMAE )) { uap->dmacr |=UART011_TXDMAE ; -writew (uap->dmacr, - uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); }return ret; }@@ -677,9 +696,9 @@ static inline bool pl011_dma_tx_start(struct uart_amba_port *uap)*/ dmacr = uap->dmacr; uap->dmacr &= ~UART011_TXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); -if (readw (uap->port.membase +REG_FR ) & UART01x_FR_TXFF) { +if (pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF) {/* * No space in the FIFO, so enable the transmit interrupt * so we know when there is space. Note that once we've @@ -688,13 +707,13 @@ static inline bool pl011_dma_tx_start(struct uart_amba_port *uap)return false; } -writew (uap->port.x_char, uap->port.membase +REG_DR ); +pl011_writew (uap, uap->port.x_char,REG_DR ); uap->port.icount.tx++; uap->port.x_char =0 ;/* Success - restore the DMA state */ uap->dmacr = dmacr; -writew (dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, dmacr,REG_DMACR );return true; }@@ -722,7 +741,7 @@ __acquires(&uap->port.lock) DMA_TO_DEVICE); uap->dmatx.queued = false; uap->dmacr &= ~UART011_TXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); } }@@ -762,11 +781,11 @@ static int pl011_dma_rx_trigger_dma(struct uart_amba_port *uap)dma_async_issue_pending (rxchan); uap->dmacr |=UART011_RXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); uap->dmarx.running = true; uap->im &= ~UART011_RXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );return 0 ; }@@ -824,8 +843,9 @@ static void pl011_dma_rx_chars(struct uart_amba_port *uap,*/ if (dma_count == pending && readfifo) {/* Clear any error flags */ -writew (UART011_OEIS |UART011_BEIS |UART011_PEIS |UART011_FEIS , - uap->port.membase +REG_ICR ); +pl011_writew (uap, +UART011_OEIS |UART011_BEIS |UART011_PEIS + |UART011_FEIS ,REG_ICR );/* * If we read all the DMA'd characters, and we had an @@ -873,7 +893,7 @@ static void pl011_dma_rx_irq(struct uart_amba_port *uap)/* Disable RX DMA - incoming data will wait in the FIFO */ uap->dmacr &= ~UART011_RXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); uap->dmarx.running = false; pending = sgbuf->sg.length - state.residue;@@ -893,7 +913,7 @@ static void pl011_dma_rx_irq(struct uart_amba_port *uap)dev_dbg (uap->port.dev,"could not retrigger RX DMA job " "fall back to interrupt mode\n" ); uap->im |=UART011_RXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC ); } }@@ -941,7 +961,7 @@ static void pl011_dma_rx_callback(void *data)dev_dbg (uap->port.dev,"could not retrigger RX DMA job " "fall back to interrupt mode\n" ); uap->im |=UART011_RXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC ); } }@@ -954,7 +974,7 @@ static inline void pl011_dma_rx_stop(struct uart_amba_port *uap) {/* FIXME. Just disable the DMA enable */ uap->dmacr &= ~UART011_RXDMAE ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR ); }/* @@ -998,7 +1018,7 @@ static void pl011_dma_rx_poll(unsigned long args)spin_lock_irqsave (&uap->port.lock, flags);pl011_dma_rx_stop (uap); uap->im |=UART011_RXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );spin_unlock_irqrestore (&uap->port.lock, flags); uap->dmarx.running = false;@@ -1060,7 +1080,7 @@ static void pl011_dma_startup(struct uart_amba_port *uap) skip_rx:/* Turn on DMA error (RX/TX will be enabled on demand) */ uap->dmacr |=UART011_DMAONERR ; -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR );/* * ST Micro variants has some specific dma burst threshold @@ -1068,9 +1088,9 @@ skip_rx:* be issued above/below 16 bytes. */ if (uap->vendor->dma_threshold) -writew (ST_UART011_DMAWM_RX_16 |ST_UART011_DMAWM_TX_16 , - uap->port.membase +REG_ST_DMAWM ); - +pl011_writew (uap, +ST_UART011_DMAWM_RX_16 |ST_UART011_DMAWM_TX_16 , +REG_ST_DMAWM );if (uap->using_rx_dma) {if (pl011_dma_rx_trigger_dma (uap))@@ -1095,12 +1115,12 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap)return ;/* Disable RX and TX DMA */ -while (readw (uap->port.membase +REG_FR ) & UART01x_FR_BUSY) +while (pl011_readw (uap,REG_FR ) & UART01x_FR_BUSY)barrier ();spin_lock_irq (&uap->port.lock); uap->dmacr &= ~(UART011_DMAONERR |UART011_RXDMAE |UART011_TXDMAE ); -writew (uap->dmacr, uap->port.membase +REG_DMACR ); +pl011_writew (uap, uap->dmacr,REG_DMACR );spin_unlock_irq (&uap->port.lock);if (uap->using_tx_dma) {@@ -1201,7 +1221,7 @@ static void pl011_stop_tx(struct uart_port *port) container_of(port, struct uart_amba_port, port); uap->im &= ~UART011_TXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );pl011_dma_tx_stop (uap); }@@ -1211,7 +1231,7 @@ static void pl011_tx_chars(struct uart_amba_port *uap, bool from_irq);static void pl011_start_tx_pio (struct uart_amba_port *uap) { uap->im |=UART011_TXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );pl011_tx_chars (uap, false); }@@ -1231,7 +1251,7 @@ static void pl011_stop_rx(struct uart_port *port) uap->im &= ~(UART011_RXIM |UART011_RTIM |UART011_FEIM |UART011_PEIM |UART011_BEIM |UART011_OEIM ); -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );pl011_dma_rx_stop (uap); }@@ -1242,7 +1262,7 @@ static void pl011_enable_ms(struct uart_port *port) container_of(port, struct uart_amba_port, port); uap->im |=UART011_RIMIM |UART011_CTSMIM |UART011_DCDMIM |UART011_DSRMIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC ); }static void pl011_rx_chars (struct uart_amba_port *uap)@@ -1262,7 +1282,7 @@ __acquires(&uap->port.lock)dev_dbg (uap->port.dev,"could not trigger RX DMA job " "fall back to interrupt mode again\n" ); uap->im |=UART011_RXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC ); }else {#ifdef CONFIG_DMA_ENGINE /* Start Rx DMA poll */ @@ -1283,10 +1303,10 @@ static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c, bool from_irq) {if (unlikely (!from_irq) && -readw (uap->port.membase +REG_FR ) & UART01x_FR_TXFF) +pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF)return false;/* unable to transmit character */ -writew (c, uap->port.membase +REG_DR ); +pl011_writew (uap, c,REG_DR ); uap->port.icount.tx++;return true;@@ -1333,7 +1353,7 @@ static void pl011_modem_status(struct uart_amba_port *uap) {unsigned int status, delta; - status =readw (uap->port.membase +REG_FR ) & UART01x_FR_MODEM_ANY; + status =pl011_readw (uap,REG_FR ) & UART01x_FR_MODEM_ANY; delta = status ^ uap->old_status; uap->old_status = status;@@ -1361,15 +1381,15 @@ static void check_apply_cts_event_workaround(struct uart_amba_port *uap)return ;/* workaround to make sure that all bits are unlocked.. */ -writew (0x00 , uap->port.membase +REG_ICR ); +pl011_writew (uap,0x00 ,REG_ICR );/* * WA: introduce 26ns(1 uart clk) delay before W1C; * single apb access will incur 2 pclk(133.12Mhz) delay, * so add 2 dummy reads */ - dummy_read =readw (uap->port.membase +REG_ICR ); - dummy_read =readw (uap->port.membase +REG_ICR ); + dummy_read =pl011_readw (uap,REG_ICR ); + dummy_read =pl011_readw (uap,REG_ICR ); }static irqreturn_t pl011_int (int irq,void *dev_id)@@ -1381,15 +1401,13 @@ static irqreturn_t pl011_int(int irq, void *dev_id)int handled =0 ;spin_lock_irqsave (&uap->port.lock, flags); - imsc =readw (uap->port.membase +REG_IMSC ); - status =readw (uap->port.membase +REG_RIS ) & imsc; + imsc =pl011_readw (uap,REG_IMSC ); + status =pl011_readw (uap,REG_RIS ) & imsc;if (status) { do { check_apply_cts_event_workaround(uap); - - writew(status & ~(UART011_TXIS|UART011_RTIS| - UART011_RXIS), - uap->port.membase + REG_ICR); + pl011_writew(uap, status & ~(UART011_TXIS|UART011_RTIS| + UART011_RXIS), REG_ICR); if (status & (UART011_RTIS|UART011_RXIS)) { if (pl011_dma_rx_running(uap))@@ -1406,7 +1424,7 @@ static irqreturn_t pl011_int(int irq, void *dev_id)if (pass_counter-- ==0 )break ; - status =readw (uap->port.membase +REG_RIS ) & imsc; + status =pl011_readw (uap,REG_RIS ) & imsc; }while (status !=0 ); handled =1 ; }@@ -1420,7 +1438,7 @@ static unsigned int pl011_tx_empty(struct uart_port *port) {struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); -unsigned int status =readw (uap->port.membase +REG_FR ); +unsigned int status =pl011_readw (uap,REG_FR );return status & (UART01x_FR_BUSY|UART01x_FR_TXFF) ?0 :TIOCSER_TEMT ; }@@ -1429,7 +1447,7 @@ static unsigned int pl011_get_mctrl(struct uart_port *port)struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port);unsigned int result =0 ; -unsigned int status =readw (uap->port.membase +REG_FR ); +unsigned int status =pl011_readw (uap,REG_FR );#define TIOCMBIT (uartbit, tiocmbit) \ if (status & uartbit) \@@ -1449,7 +1467,7 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl) container_of(port, struct uart_amba_port, port);unsigned int cr; - cr =readw (uap->port.membase +REG_CR ); + cr =pl011_readw (uap,REG_CR );#define TIOCMBIT (tiocmbit, uartbit) \ if (mctrl & tiocmbit) \@@ -1469,7 +1487,7 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl) }#undef TIOCMBIT -writew (cr, uap->port.membase +REG_CR ); +pl011_writew (uap, cr,REG_CR ); }static void pl011_break_ctl (struct uart_port *port,int break_state)@@ -1480,12 +1498,12 @@ static void pl011_break_ctl(struct uart_port *port, int break_state)unsigned int lcr_h;spin_lock_irqsave (&uap->port.lock, flags); - lcr_h =readw (uap->port.membase + uap->lcrh_tx); + lcr_h =pl011_readw (uap, uap->lcrh_tx);if (break_state ==-1 ) lcr_h |= UART01x_LCRH_BRK;else lcr_h &= ~UART01x_LCRH_BRK; -writew (lcr_h, uap->port.membase + uap->lcrh_tx); +pl011_writew (uap, lcr_h, uap->lcrh_tx);spin_unlock_irqrestore (&uap->port.lock, flags); }@@ -1495,9 +1513,8 @@ static void pl011_quiesce_irqs(struct uart_port *port) {struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); -unsigned char __iomem *regs = uap->port.membase; -writew (readw (regs +REG_MIS ), regs +REG_ICR ); +pl011_writew (uap,pl011_readw (uap,REG_MIS ),REG_ICR );/* * There is no way to clear TXIM as this is "ready to transmit IRQ", so * we simply mask it. start_tx() will unmask it. @@ -1511,7 +1528,7 @@ static void pl011_quiesce_irqs(struct uart_port *port)* (including tx queue), so we're also fine with start_tx()'s caller * side. */ -writew (readw (regs +REG_IMSC ) & ~UART011_TXIM , regs +REG_IMSC ); +pl011_writew (uap,pl011_readw (uap,REG_IMSC ) & ~UART011_TXIM ,REG_IMSC ); }static int pl011_get_poll_char (struct uart_port *port)@@ -1526,11 +1543,11 @@ static int pl011_get_poll_char(struct uart_port *port)*/ pl011_quiesce_irqs (port); - status =readw (uap->port.membase +REG_FR ); + status =pl011_readw (uap,REG_FR );if (status & UART01x_FR_RXFE)return NO_POLL_CHAR ; -return readw (uap->port.membase +REG_DR ); +return pl011_readw (uap,REG_DR ); } static void pl011_put_poll_char(struct uart_port *port,@@ -1539,10 +1556,10 @@ static void pl011_put_poll_char(struct uart_port *port,struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); -while (readw (uap->port.membase +REG_FR ) & UART01x_FR_TXFF) +while (pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF)barrier (); - writew(ch, uap->port.membase + REG_DR); + pl011_writew(uap, ch, REG_DR); }#endif /* CONFIG_CONSOLE_POLL */ @@ -1566,15 +1583,15 @@ static int pl011_hwinit(struct uart_port *port) uap->port.uartclk =clk_get_rate (uap->clk);/* Clear pending error and receive interrupts */ -writew (UART011_OEIS |UART011_BEIS |UART011_PEIS |UART011_FEIS | -UART011_RTIS |UART011_RXIS , uap->port.membase +REG_ICR ); +pl011_writew (uap,UART011_OEIS |UART011_BEIS |UART011_PEIS | +UART011_FEIS |UART011_RTIS |UART011_RXIS ,REG_ICR );/* * Save interrupts enable mask, and enable RX interrupts in case if * the interrupt is used for NMI entry. */ - uap->im =readw (uap->port.membase +REG_IMSC ); -writew (UART011_RTIM |UART011_RXIM , uap->port.membase +REG_IMSC ); + uap->im =pl011_readw (uap,REG_IMSC ); +pl011_writew (uap,UART011_RTIM |UART011_RXIM ,REG_IMSC );if (dev_get_platdata (uap->port.dev)) {struct amba_pl011_data *plat;@@ -1588,7 +1605,7 @@ static int pl011_hwinit(struct uart_port *port)static void pl011_write_lcr_h (struct uart_amba_port *uap,unsigned int lcr_h) { -writew (lcr_h, uap->port.membase + uap->lcrh_rx); +pl011_writew (uap, lcr_h, uap->lcrh_rx);if (uap->lcrh_rx != uap->lcrh_tx) {int i;/* @@ -1596,14 +1613,14 @@ static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h)* to get this delay write read only register 10 times */ for (i =0 ; i <10 ; ++i) -writew (0xff , uap->port.membase +REG_MIS ); -writew (lcr_h, uap->port.membase + uap->lcrh_tx); +pl011_writew (uap,0xff ,REG_MIS ); +pl011_writew (uap, lcr_h, uap->lcrh_tx); } }static int pl011_allocate_irq (struct uart_amba_port *uap) { -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );return request_irq (uap->port.irq, pl011_int,0 ,"uart-pl011" , uap); }@@ -1618,12 +1635,11 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap)spin_lock_irq (&uap->port.lock);/* Clear out any spuriously appearing RX interrupts */ -writew (UART011_RTIS |UART011_RXIS , - uap->port.membase +REG_ICR ); +pl011_writew (uap,UART011_RTIS |UART011_RXIS ,REG_ICR ); uap->im =UART011_RTIM ;if (!pl011_dma_rx_running (uap)) uap->im |=UART011_RXIM ; -writew (uap->im, uap->port.membase +REG_IMSC ); +pl011_writew (uap, uap->im,REG_IMSC );spin_unlock_irq (&uap->port.lock); }@@ -1642,21 +1658,21 @@ static int pl011_startup(struct uart_port *port)if (retval) goto clk_dis; -writew (uap->vendor->ifls, uap->port.membase +REG_IFLS ); +pl011_writew (uap, uap->vendor->ifls,REG_IFLS );spin_lock_irq (&uap->port.lock);/* restore RTS and DTR */ cr = uap->old_cr & (UART011_CR_RTS |UART011_CR_DTR ); cr |= UART01x_CR_UARTEN |UART011_CR_RXE |UART011_CR_TXE ; -writew (cr, uap->port.membase +REG_CR ); +pl011_writew (uap, cr,REG_CR );spin_unlock_irq (&uap->port.lock);/* * initialise the old status of the modem signals */ - uap->old_status =readw (uap->port.membase +REG_FR ) & + uap->old_status =pl011_readw (uap,REG_FR ) & UART01x_FR_MODEM_ANY;/* Startup DMA */ @@ -1696,11 +1712,11 @@ static int sbsa_uart_startup(struct uart_port *port)static void pl011_shutdown_channel (struct uart_amba_port *uap,unsigned int lcrh) { -unsigned long val; +unsigned long val; - val =readw (uap->port.membase + lcrh); - val &= ~(UART01x_LCRH_BRK | UART01x_LCRH_FEN); -writew (val, uap->port.membase + lcrh); + val =pl011_readw (uap, lcrh); + val &= ~(UART01x_LCRH_BRK | UART01x_LCRH_FEN); +pl011_writew (uap, val, lcrh); }/* @@ -1714,11 +1730,11 @@ static void pl011_disable_uart(struct uart_amba_port *uap) uap->autorts = false;spin_lock_irq (&uap->port.lock); - cr =readw (uap->port.membase +REG_CR ); + cr =pl011_readw (uap,REG_CR ); uap->old_cr = cr; cr &=UART011_CR_RTS |UART011_CR_DTR ; cr |= UART01x_CR_UARTEN |UART011_CR_TXE ; -writew (cr, uap->port.membase +REG_CR ); +pl011_writew (uap, cr,REG_CR );spin_unlock_irq (&uap->port.lock);/* @@ -1735,8 +1751,8 @@ static void pl011_disable_interrupts(struct uart_amba_port *uap)/* mask all interrupts and clear all pending ones */ uap->im =0 ; -writew (uap->im, uap->port.membase +REG_IMSC ); -writew (0xffff , uap->port.membase +REG_ICR ); +pl011_writew (uap, uap->im,REG_IMSC ); +pl011_writew (0xffff ,REG_ICR );spin_unlock_irq (&uap->port.lock); }@@ -1888,8 +1904,8 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,pl011_enable_ms (port);/* first, disable everything */ - old_cr =readw (port->membase +REG_CR ); -writew (0 , port->membase +REG_CR ); + old_cr =pl011_readw (uap,REG_CR ); +pl011_writew (uap,0 ,REG_CR );if (termios->c_cflag &CRTSCTS ) {if (old_cr &UART011_CR_RTS )@@ -1922,8 +1938,8 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios, quot -=2 ; }/* Set baud rate */ -writew (quot &0x3f , port->membase +REG_FBRD ); -writew (quot >>6 , port->membase +REG_IBRD ); +pl011_writew (uap, quot &0x3f ,REG_FBRD ); +pl011_writew (uap, quot >>6 ,REG_IBRD );/* * ----------v----------v----------v----------v----- @@ -1932,7 +1948,7 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,* ----------^----------^----------^----------^----- */ pl011_write_lcr_h (uap, lcr_h); -writew (old_cr, port->membase +REG_CR ); +pl011_writew (uap, old_cr,REG_CR );spin_unlock_irqrestore (&port->lock, flags); }@@ -2073,9 +2089,9 @@ static void pl011_console_putchar(struct uart_port *port, int ch)struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port); -while (readw (uap->port.membase +REG_FR ) & UART01x_FR_TXFF) +while (pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF)barrier (); -writew (ch, uap->port.membase +REG_DR ); +pl011_writew (uap, ch,REG_DR ); } static void@@ -2100,10 +2116,10 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)* First save the CR then disable the interrupts */ if (!uap->vendor->always_enabled) { - old_cr =readw (uap->port.membase +REG_CR ); + old_cr =pl011_readw (uap,REG_CR ); new_cr = old_cr & ~UART011_CR_CTSEN ; new_cr |= UART01x_CR_UARTEN |UART011_CR_TXE ; -writew (new_cr, uap->port.membase +REG_CR ); +pl011_writew (uap, new_cr,REG_CR ); }uart_console_write (&uap->port, s, count, pl011_console_putchar);@@ -2113,10 +2129,10 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)* and restore the TCR */ do { - status =readw (uap->port.membase +REG_FR ); + status =pl011_readw (uap,REG_FR ); }while (status & UART01x_FR_BUSY);if (!uap->vendor->always_enabled) -writew (old_cr, uap->port.membase +REG_CR ); +pl011_writew (uap, old_cr,REG_CR );if (locked)spin_unlock (&uap->port.lock);@@ -2129,10 +2145,10 @@ static void __init pl011_console_get_options(struct uart_amba_port *uap, int *baud, int *parity, int *bits) { - if (readw(uap->port.membase + REG_CR) & UART01x_CR_UARTEN) { + if (pl011_readw(uap, REG_CR) & UART01x_CR_UARTEN) { unsigned int lcr_h, ibrd, fbrd; - lcr_h =readw (uap->port.membase + uap->lcrh_tx); + lcr_h =pl011_readw (uap, uap->lcrh_tx); *parity ='n' ;if (lcr_h & UART01x_LCRH_PEN) {@@ -2147,13 +2163,13 @@ pl011_console_get_options(struct uart_amba_port *uap, int *baud,else *bits =8 ; - ibrd =readw (uap->port.membase +REG_IBRD ); - fbrd =readw (uap->port.membase +REG_FBRD ); + ibrd =pl011_readw (uap,REG_IBRD ); + fbrd =pl011_readw (uap,REG_FBRD ); *baud = uap->port.uartclk *4 / (64 * ibrd + fbrd);if (uap->vendor->oversampling) { -if (readw (uap->port.membase +REG_CR ) +if (pl011_readw (uap,REG_CR ) &ST_UART011_CR_OVSFACT ) *baud *=2 ; }@@ -2225,10 +2241,13 @@ static struct console amba_console = {static void pl011_putc (struct uart_port *port,int c) { -while (readl (port->membase +REG_FR ) & UART01x_FR_TXFF) + struct uart_amba_port *uap =drivers/tty/serial/amba-pl011.c static struct console amba_console = { · line 2252
Quoted 10 lines
drivers/tty/serial/amba-pl011.c +5
-2+ container_of(port, struct uart_amba_port, port); + +while (pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF) ; -writeb (c, port->membase +REG_DR ); -while (readl (port->membase +REG_FR ) & UART01x_FR_BUSY) +pl011_writeb (uap, c,REG_DR ); +while (pl011_readw (uap,REG_FR ) & UART01x_FR_BUSY) ; }Just for the records, as this has been discussed before:
pl011_putc()is called by the earlycon code, before theuart_portis actually initialized. So we cannot rely on the accessors, but have to use the old-fashioned director accessors for this.
Quoted 7 lines
Which means you cannot use that approach to get earlycon support for the ZTE UART, if I get this correctly. It shouldn't be to hard to introduce another earlycon type specificly for that one, copying
pl011_early_writeandpl011_early_console_setupand changingpl011_putcintozte_uart_putc. But of course this belongs into the final patch (or a separate one), not in this. So I guess you just leave that function unchanged in this patch.
Yeah, I should not change earlycon function at all as the register table is not initialized yet. But I miss this point as I did not enable earlycon in my test.
Quoted 6 lines
As a side note: I wonder why those accessors here are actually 32-bit wide here, and the data register 8-bits only?
This is in contrast to the rest of the driver, which uses 16-bit accesses all of the time (in line with the spec).
I have the same wonder, but did try to change it as I not want to introduce other topic in this patch. Hope other people have clue on this point.
Quoted 18 lines
Cheers,
Andre.Quoted 15 lines
drivers/tty/serial/amba-pl011.c +2
-3@@ -2355,8 +2374,8 @@ static int pl011_register_port(struct uart_amba_port *uap)int ret;/* Ensure interrupts from this UART are masked and cleared */ -writew (0 , uap->port.membase +REG_IMSC ); -writew (0xffff , uap->port.membase +REG_ICR ); +pl011_writew (uap,0 ,REG_IMSC ); +pl011_writew (uap,0xffff ,REG_ICR );if (!amba_reg.state) { ret =uart_register_driver (&amba_reg); --1.9.1
PATCH v13 2/5 uart: pl011: Introduce register accessor
Hi Jun,
thanks for your reply. Just a quick one below...
...
Quoted 27 lines
Quoted 18 lines
Quoted 5 lines
(quoted diff) +1
-0@@ -203,6 +206,24 @@ struct uart_amba_port { #endif }; +static unsigned int pl011_readw(struct uart_amba_port *uap, int index)Review struct uart_amba_port { · line 219
Quoted 10 lines
(quoted diff) +10
-0+{ + WARN_ON(index > REG_NR); + return readw_relaxed(uap->port.membase + (index << 2)); +} + +static void pl011_writew(struct uart_amba_port *uap, int val, int index) +{ + WARN_ON(index > REG_NR); + writew_relaxed(val, uap->port.membase + (index << 2)); +}I wonder if you could rename those to pl011_{read,write}, respectively (loosing the "w" suffix).
Quoted 8 lines
The SBSA UART spec reads as the registers are actually accessible via 32-bit accesses and rumour has it that there are implementations which rely on that and don't work with ldrh/strh.
I am still waiting for reports about actual hardware to fail, but we might be forced to change the access width to 32-bit for the SBSA subset in the future. So having wrapper functions would make that change much easier, but having them without a suffix from the beginning would even be better, as I wouldn't be bothered to rename them later on.
Quoted 1 line
OK, will change to 32-bit access in future version.
Sorry, there seems to be a misunderstanding here. 16-bit accesses are totally fine for every _PL011_ part, it is just the SBSA-UART (which uses this very same driver) which _may_ require 32-bit accesses.
So I was asking just to name the functions pl011_write and pl011_read, so that their "w" suffix does not delude people into them being forever 16-bit wide. If needed (as said I am still waiting for failure reports), we can then change the access width inside these functions to the appropriate size later without requiring a name change again.
So please keep the {write,read}w_relaxed calls in there, just change the function name.
Thanks!
Andre.
PATCH v13 2/5 uart: pl011: Introduce register accessor
In reply to Andre PrzywaraQuoted 21 lines · Andre Przywara
Quoted 5 lines
static void
pl011_putc(struct uart_port *port, int c){(quoted diff) +1
-1- while (readl(port->membase + REG_FR) & UART01x_FR_TXFF) + struct uart_amba_port *uap =Review
Quoted 10 lines
(quoted diff) +5
-2+ container_of(port, struct uart_amba_port, port); + + while (pl011_readw(uap, REG_FR) & UART01x_FR_TXFF) ; - writeb(c, port->membase + REG_DR); - while (readl(port->membase + REG_FR) & UART01x_FR_BUSY) + pl011_writeb(uap, c, REG_DR); + while (pl011_readw(uap, REG_FR) & UART01x_FR_BUSY) ; }Just for the records, as this has been discussed before:
pl011_putc()is called by the earlycon code, before theuart_portis actually initialized. So we cannot rely on the accessors, but have to use the old-fashioned director accessors for this.
Quoted 7 lines
Which means you cannot use that approach to get earlycon support for the ZTE UART, if I get this correctly. It shouldn't be to hard to introduce another earlycon type specificly for that one, copying
pl011_early_writeandpl011_early_console_setupand changingpl011_putcintozte_uart_putc. But of course this belongs into the final patch (or a separate one), not in this. So I guess you just leave that function unchanged in this patch.
How about something like this? It adds the "sbsa32" option to the earlycon command-line parameter.
static void pl011_putc(struct uart_port *port, int c)
{
while (readl(port->membase + UART01x_FR) & UART01x_FR_TXFF)
cpu_relax();
writeb(c, port->membase + UART01x_DR);
while (readl(port->membase + UART01x_FR) & UART01x_FR_BUSY)
cpu_relax();
}static void pl011_early_write(struct console *con, const char *s, unsigned n)
{
struct earlycon_device *dev = con->data;
uart_console_write(&dev->port, s, n, pl011_putc);
}static void pl011_putc_sbsa32(struct uart_port *port, int c)
{
while (readl(port->membase + UART01x_FR) & UART01x_FR_TXFF)
cpu_relax();
writel(c, port->membase + UART01x_DR);
while (readl(port->membase + UART01x_FR) & UART01x_FR_BUSY)
cpu_relax();
}static void pl011_early_write_sbsa32(struct console *con, const char *s, unsigned n)
{
struct earlycon_device *dev = con->data;
uart_console_write(&dev->port, s, n, pl011_putc_sbsa32);
}static int __init pl011_early_console_setup(struct earlycon_device *device,
const char *opt)
{
if (!device->port.membase)
return -ENODEV;
if (strcmp(device->options, "sbsa32"))
device->con->write = pl011_early_write_sbsa32;
else
device->con->write = pl011_early_write;
return 0;
}Signature · 3 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v13 2/5 uart: pl011: Introduce register accessor
Quoted 68 lines · Timur Tabi
Quoted 21 lines · Andre Przywara
Quoted 5 lines
static void
pl011_putc(struct uart_port *port, int c){(quoted diff) +1
-1- while (readl(port->membase + REG_FR) & UART01x_FR_TXFF) + struct uart_amba_port *uap =Review
Quoted 10 lines
(quoted diff) +5
-2+ container_of(port, struct uart_amba_port, port); + + while (pl011_readw(uap, REG_FR) & UART01x_FR_TXFF) ; - writeb(c, port->membase + REG_DR); - while (readl(port->membase + REG_FR) & UART01x_FR_BUSY) + pl011_writeb(uap, c, REG_DR); + while (pl011_readw(uap, REG_FR) & UART01x_FR_BUSY) ; }Just for the records, as this has been discussed before:
pl011_putc()is called by the earlycon code, before theuart_portis actually initialized. So we cannot rely on the accessors, but have to use the old-fashioned director accessors for this.Quoted 7 lines
Which means you cannot use that approach to get earlycon support for the ZTE UART, if I get this correctly. It shouldn't be to hard to introduce another earlycon type specificly for that one, copying
pl011_early_writeandpl011_early_console_setupand changingpl011_putcintozte_uart_putc. But of course this belongs into the final patch (or a separate one), not in this. So I guess you just leave that function unchanged in this patch.How about something like this? It adds the "sbsa32" option to the earlycon command-line parameter.
static void
pl011_putc(struct uart_port *port, int c){ while (readl(port->membase + UART01x_FR) & UART01x_FR_TXFF) cpu_relax(); writeb(c, port->membase + UART01x_DR); while (readl(port->membase + UART01x_FR) & UART01x_FR_BUSY) cpu_relax(); }static void
pl011_early_write(struct console *con, const char *s, unsigned n){ struct earlycon_device *dev = con->data; uart_console_write(&dev->port, s, n, pl011_putc); }static void
pl011_putc_sbsa32(struct uart_port *port, int c){ while (readl(port->membase + UART01x_FR) & UART01x_FR_TXFF) cpu_relax(); writel(c, port->membase + UART01x_DR); while (readl(port->membase + UART01x_FR) & UART01x_FR_BUSY) cpu_relax(); }static void
pl011_early_write_sbsa32(struct console *con, const char *s, unsigned n){ struct earlycon_device *dev = con->data; uart_console_write(&dev->port, s, n, pl011_putc_sbsa32); }static int
__initpl011_early_console_setup(structearlycon_device*device,
const char *opt)
Quoted 11 lines
{
if (!device->port.membase)
return -ENODEV;
if (strcmp(device->options, "sbsa32"))
device->con->write = pl011_early_write_sbsa32;
else
device->con->write = pl011_early_write;
return 0;
}For earlycon support, I'd prefer to see different OF_EARLYCON_DECLARE() (and EARLYCON_DECLARE()) declarations with alternate setup() functions for zte and sbsa32. For example:
static int __init zte_early_console_setup(struct earlycon_device *device,
const char *opt)
{
if (!device->port.membase)
return -ENODEV;
device->con->write = zte_early_write;
return 0;
}static int __init sbsa32_early_console_setup(struct earlycon_device *device,
const char *opt)
{
if (!device->port.membase)
return -ENODEV;
device->con->write = sbsa32_early_write;
return 0;
}EARLYCON_DECLARE(zte, zte_early_console_setup); OF_EARLYCON_DECLARE(zte, ".......", zte_early_console_setup); EARLYCON_DECLARE(sbsa32, sbsa32_early_console_setup); OF_EARLYCON_DECLARE(sbsa32, "arm,sbsa-uart", sbsa32_early_console_setup);
The above assumes that the sbsa32 maintains the equivalence of earlycon and console; ie., that
/ {
chosen {
stdout-path = &uart0;
};
soc {
uart0: serial at xxxxxx {
compatible = "arm,sbsa-uart";
};
};
};will start a sbsa32 earlycon and later replace that with sbsa console.
Regards,
Peter Hurley
PATCH v13 2/5 uart: pl011: Introduce register accessor
Quoted 21 lines · Peter Hurley
EARLYCON_DECLARE(zte, zte_early_console_setup);OF_EARLYCON_DECLARE(zte, ".......", zte_early_console_setup);EARLYCON_DECLARE(sbsa32, sbsa32_early_console_setup);OF_EARLYCON_DECLARE(sbsa32, "arm,sbsa-uart", sbsa32_early_console_setup);The above assumes that the sbsa32 maintains the equivalence of earlycon and console; ie., that
/ { chosen { stdout-path = &uart0; }; soc { uart0: serial at xxxxxx { compatible = "arm,sbsa-uart"; }; }; };
Quoted 2 lines
will start a sbsa32 earlycon and later replace that with sbsa console.
I never really understood the device tree support for SBSA. The "S" stands for Server, and ARM Server systems are supposed to use ACPI. It's easy to add special cases via device tree, because of its flexibility. ACPI is much more difficult, and that's why I used the command-line approach.
Anyway, I found this alternate patch from Lief Lindholm that implements this idea better, although it needs work:
https://patches.linaro.org/53265/
Signature · 3 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v13 2/5 uart: pl011: Introduce register accessor
Quoted 22 lines · Timur Tabi
Quoted 21 lines · Peter Hurley
EARLYCON_DECLARE(zte, zte_early_console_setup);OF_EARLYCON_DECLARE(zte, ".......", zte_early_console_setup);EARLYCON_DECLARE(sbsa32, sbsa32_early_console_setup);OF_EARLYCON_DECLARE(sbsa32, "arm,sbsa-uart", sbsa32_early_console_setup);The above assumes that the sbsa32 maintains the equivalence of earlycon and console; ie., that
/ { chosen { stdout-path = &uart0; }; soc { uart0: serial at xxxxxx { compatible = "arm,sbsa-uart"; }; }; };
Quoted 4 lines
Quoted 2 lines
will start a sbsa32 earlycon and later replace that with sbsa console.
I never really understood the device tree support for SBSA. The "S" stands for Server, and ARM Server systems are supposed to use ACPI. It's easy to add special cases via device tree, because of its flexibility.
Agreed.
Quoted 1 line
ACPI is much more difficult, and that's why I used the command-line approach.
Understood.
Quoted 3 lines
Anyway, I found this alternate patch from Lief Lindholm that implements this idea better, although it needs work:
Yeah, I saw those, but I'm waiting for v2 to review.
Regards,
Peter Hurley
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v13 4/5 uart: pl011: Improve LCRH register access decision
Hi Jun,
Quoted 14 lines · Jun Nie
Improve LCRH register access decision as ARM PL011 lcrh register serve as both TX and RX, while other SOC may implement TX and RX function with separated register.
Signed-off-by Jun Nie <[email protected]>--- drivers/tty/serial/amba-pl011.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)drivers/tty/serial/amba-pl011.c +0
-0
drivers/tty/serial/amba-pl011.c struct uart_amba_port { · line 256
Quoted 8 lines
drivers/tty/serial/amba-pl011.c +5
-0@@ -249,6 +249,11 @@ struct uart_amba_port {#endif }; +static bool is_implemented (struct uart_amba_port *uap,unsigned int reg) +{ +return uap->reg_lut[reg] != (u16 )~0 ; +} +
Just a nit: is_implemented sounds a bit too generic for me, could this be more specific like reg_is_implemented or the like?
Other than that the patch looks good to me.
Cheers,
Andre.
Quoted 22 lines
drivers/tty/serial/amba-pl011.c +2
-2static unsigned int pl011_readw (struct uart_amba_port *uap,int index) {WARN_ON (index >REG_NR );@@ -1649,7 +1654,7 @@ static int pl011_hwinit(struct uart_port *port)static void pl011_write_lcr_h (struct uart_amba_port *uap,unsigned int lcr_h) {pl011_writew (uap, lcr_h, uap->lcrh_rx); -if (uap->lcrh_rx != uap->lcrh_tx) { +if (is_implemented (uap,REG_ST_LCRH_RX )) {int i;/* * Wait 10 PCLKs before writing LCRH_TX register, @@ -1784,7 +1789,7 @@ static void pl011_disable_uart(struct uart_amba_port *uap)* disable break condition and fifos */ pl011_shutdown_channel (uap, uap->lcrh_rx); -if (uap->lcrh_rx != uap->lcrh_tx) +if (is_implemented (uap,REG_ST_LCRH_RX ))pl011_shutdown_channel (uap, uap->lcrh_tx); }
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
Hi Jun,
Quoted 46 lines · Jun Nie
Support ZTE uart with some registers differing offset. Probe as platform device for not AMBA IP ID is available on ZTE uart.
Signed-off-by Jun Nie <[email protected]>--- drivers/tty/serial/Kconfig | 4 +- drivers/tty/serial/amba-pl011.c | 195 +++++++++++++++++++++++++++++++++++++--- include/linux/amba/serial.h | 14 +++ 3 files changed, 197 insertions(+), 16 deletions(-)drivers/tty/serial/Kconfig +2
-2@@ -47,12 +47,12 @@ config SERIAL_AMBA_PL010_CONSOLE config SERIAL_AMBA_PL011 tristate "ARM AMBA PL011 serial port support" - depends on ARM_AMBA + depends on ARM_AMBA || SOC_ZX296702 select SERIAL_CORE help This selects the ARM(R) AMBA(R) PrimeCell PL011 UART. If you have an Integrator/PP2, Integrator/CP or Versatile platform, say Y or M - here. + here. Say Y or M if you have SOC_ZX296702. If unsure, say N.drivers/tty/serial/amba-pl011.c +4
-0@@ -74,6 +74,10 @@/* There is by now at least one vendor with differing details, so handle it */ struct vendor_data {unsigned int ifls; +unsigned int fr_busy; +unsigned int fr_dsr; +unsigned int fr_cts; +unsigned int fr_ri;unsigned int lcrh_tx;unsigned int lcrh_rx;u16 *reg_lut;
drivers/tty/serial/amba-pl011.c static u16 arm_reg[] = { · line 134
Quoted 4 lines
drivers/tty/serial/amba-pl011.c +1
-0@@ -127,6 +131,7 @@ static u16 arm_reg[] = { [REG_DMACR] = UART011_DMACR, }; +#ifdef CONFIG_ARM_AMBA
Maybe I missed that discussion (reading mailing list archives on the web is just horrible!), but why do we have all these #ifdefs here? The actual design of that driver extension is meant to fold well into the driver, with the changes only coming into effect if the DT node is found. To me it looks like we could even have a PL011 instance and a ZTE UART instance in operation at the same time.
So can't we get rid of those silly #ifdef CONFIG_ARM_AMBAs at least, and CONFIG_SOC_ZX296702 as well?
Quoted 48 lines
drivers/tty/serial/amba-pl011.c +14
-0static unsigned int get_fifosize_arm (struct amba_device *dev) {return amba_rev (dev) <3 ?16 :32 ;@@ -134,6 +139,10 @@ static unsigned int get_fifosize_arm(struct amba_device *dev)static struct vendor_data vendor_arm = { .ifls =UART011_IFLS_RX4_8 |UART011_IFLS_TX4_8 , + .fr_busy = UART01x_FR_BUSY, + .fr_dsr = UART01x_FR_DSR, + .fr_cts = UART01x_FR_CTS, + .fr_ri =UART011_FR_RI , .lcrh_tx =REG_LCRH , .lcrh_rx =REG_LCRH , .reg_lut = arm_reg,@@ -144,8 +153,13 @@ static struct vendor_data vendor_arm = { .fixed_options = false, .get_fifosize = get_fifosize_arm, }; +#endif static struct vendor_data vendor_sbsa = { + .fr_busy = UART01x_FR_BUSY, + .fr_dsr = UART01x_FR_DSR, + .fr_cts = UART01x_FR_CTS, + .fr_ri =UART011_FR_RI , .reg_lut = arm_reg, .oversampling = false, .dma_threshold = false,@@ -154,6 +168,7 @@ static struct vendor_data vendor_sbsa = { .fixed_options = true, }; +#ifdef CONFIG_ARM_AMBA static u16 st_reg[] = { [REG_DR ] = UART01x_DR, [REG_RSR] = UART01x_RSR,@@ -180,6 +195,10 @@ static unsigned int get_fifosize_st(struct amba_device *dev)static struct vendor_data vendor_st = { .ifls =UART011_IFLS_RX_HALF |UART011_IFLS_TX_HALF , + .fr_busy = UART01x_FR_BUSY, + .fr_dsr = UART01x_FR_DSR, + .fr_cts = UART01x_FR_CTS, + .fr_ri =UART011_FR_RI , .lcrh_tx =REG_LCRH , .lcrh_rx =REG_ST_LCRH_RX , .reg_lut = st_reg,
drivers/tty/serial/amba-pl011.c static struct vendor_data vendor_st = { · line 214
Quoted 6 lines
drivers/tty/serial/amba-pl011.c +3
-0@@ -190,6 +209,43 @@ static struct vendor_data vendor_st = { .fixed_options = false, .get_fifosize = get_fifosize_st, }; +#endif + +#ifdef CONFIG_SOC_ZX296702
As said above, I doubt the usefulness of this bracketing. But even if there are arguments in favour for that, shouldn't it be CONFIG_ARCH_ZX instead? Or is this UART just a mishap which happened to that very particular SoC and it will not show again in any other silicon?
Cheers,
Andre.
Quoted 360 lines
drivers/tty/serial/amba-pl011.c +159
-14+static u16 zte_reg[] = { + [REG_DR ] = ZX_UART01x_DR, + [REG_RSR ] = UART01x_RSR, + [REG_ST_DMAWM ] =ST_UART011_DMAWM , + [REG_FR ] = ZX_UART01x_FR, + [REG_ST_LCRH_RX ] =ST_UART011_LCRH_RX , + [REG_ILPR ] = UART01x_ILPR, + [REG_IBRD ] =UART011_IBRD , + [REG_FBRD ] =UART011_FBRD , + [REG_LCRH ] =ZX_UART011_LCRH_TX , + [REG_CR ] =ZX_UART011_CR , + [REG_IFLS ] =ZX_UART011_IFLS , + [REG_IMSC ] =ZX_UART011_IMSC , + [REG_RIS ] =ZX_UART011_RIS , + [REG_MIS ] =ZX_UART011_MIS , + [REG_ICR ] =ZX_UART011_ICR , + [REG_DMACR ] =ZX_UART011_DMACR , +}; + +static struct vendor_data vendor_zte = { + .ifls =UART011_IFLS_RX4_8 |UART011_IFLS_TX4_8 , + .fr_busy = ZX_UART01x_FR_BUSY, + .fr_dsr = ZX_UART01x_FR_DSR, + .fr_cts = ZX_UART01x_FR_CTS, + .fr_ri =ZX_UART011_FR_RI , + .lcrh_tx =REG_LCRH , + .lcrh_rx =REG_ST_LCRH_RX , + .reg_lut = zte_reg, + .oversampling = false, + .dma_threshold = false, + .cts_event_workaround = false, + .fixed_options = false, +}; +#endif /* Deals with DMA transactions */ @@ -233,6 +289,10 @@ struct uart_amba_port {unsigned int im;/* interrupt mask */ unsigned int old_status;unsigned int fifosize;/* vendor-specific */ +unsigned int fr_busy;/* vendor-specific */ +unsigned int fr_dsr;/* vendor-specific */ +unsigned int fr_cts;/* vendor-specific */ +unsigned int fr_ri;/* vendor-specific */ unsigned int lcrh_tx;/* vendor-specific */ unsigned int lcrh_rx;/* vendor-specific */ unsigned int old_cr;/* state during shutdown */ @@ -1163,7 +1223,7 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap)return ;/* Disable RX and TX DMA */ -while (pl011_readw (uap,REG_FR ) & UART01x_FR_BUSY) +while (pl011_readw (uap,REG_FR ) & uap->fr_busy)barrier ();spin_lock_irq (&uap->port.lock);@@ -1412,11 +1472,11 @@ static void pl011_modem_status(struct uart_amba_port *uap)if (delta & UART01x_FR_DCD)uart_handle_dcd_change (&uap->port, status & UART01x_FR_DCD); -if (delta & UART01x_FR_DSR) +if (delta & uap->fr_dsr) uap->port.icount.dsr++; -if (delta & UART01x_FR_CTS) -uart_handle_cts_change (&uap->port, status & UART01x_FR_CTS); +if (delta & uap->fr_cts) +uart_handle_cts_change (&uap->port, status & uap->fr_cts);wake_up_interruptible (&uap->port.state->port.delta_msr_wait); }@@ -1487,7 +1547,7 @@ static unsigned int pl011_tx_empty(struct uart_port *port)struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port);unsigned int status =pl011_readw (uap,REG_FR ); -return status & (UART01x_FR_BUSY|UART01x_FR_TXFF) ?0 :TIOCSER_TEMT ; +return status & (uap->fr_busy|UART01x_FR_TXFF) ?0 :TIOCSER_TEMT ; }static unsigned int pl011_get_mctrl (struct uart_port *port)@@ -1502,9 +1562,9 @@ static unsigned int pl011_get_mctrl(struct uart_port *port) result |= tiocmbitTIOCMBIT (UART01x_FR_DCD,TIOCM_CAR ); -TIOCMBIT (UART01x_FR_DSR,TIOCM_DSR ); -TIOCMBIT (UART01x_FR_CTS,TIOCM_CTS ); -TIOCMBIT (UART011_FR_RI ,TIOCM_RNG ); +TIOCMBIT (uap->fr_dsr,TIOCM_DSR ); +TIOCMBIT (uap->fr_cts,TIOCM_CTS ); +TIOCMBIT (uap->fr_ri,TIOCM_RNG );#undef TIOCMBITreturn result; }@@ -1720,8 +1780,7 @@ static int pl011_startup(struct uart_port *port)/* * initialise the old status of the modem signals */ - uap->old_status =pl011_readw (uap,REG_FR ) & - UART01x_FR_MODEM_ANY; + uap->old_status =pl011_readw (uap,REG_FR ) & UART01x_FR_MODEM_ANY;/* Startup DMA */ pl011_dma_startup (uap);@@ -1800,7 +1859,7 @@ static void pl011_disable_interrupts(struct uart_amba_port *uap)/* mask all interrupts and clear all pending ones */ uap->im =0 ;pl011_writew (uap, uap->im,REG_IMSC ); -pl011_writew (0xffff ,REG_ICR ); +pl011_writew (uap,0xffff ,REG_ICR );spin_unlock_irq (&uap->port.lock); }@@ -2178,7 +2237,7 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)*/ do { status =pl011_readw (uap,REG_FR ); - }while (status & UART01x_FR_BUSY); + }while (status & uap->fr_busy);if (!uap->vendor->always_enabled)pl011_writew (uap, old_cr,REG_CR );@@ -2295,7 +2354,7 @@ static void pl011_putc(struct uart_port *port, int c)while (pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF) ;pl011_writeb (uap, c,REG_DR ); -while (pl011_readw (uap,REG_FR ) & UART01x_FR_BUSY) +while (pl011_readw (uap,REG_FR ) & uap->fr_busy) ; }@@ -2441,6 +2500,7 @@ static int pl011_register_port(struct uart_amba_port *uap)return ret; } +#ifdef CONFIG_ARM_AMBA static int pl011_probe (struct amba_device *dev,const struct amba_id *id) {struct uart_amba_port *uap;@@ -2464,6 +2524,10 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id) uap->reg_lut = vendor->reg_lut; uap->lcrh_rx = vendor->lcrh_rx; uap->lcrh_tx = vendor->lcrh_tx; + uap->fr_busy = vendor->fr_busy; + uap->fr_dsr = vendor->fr_dsr; + uap->fr_cts = vendor->fr_cts; + uap->fr_ri = vendor->fr_ri; uap->fifosize = vendor->get_fifosize (dev); uap->port.irq = dev->irq[0 ]; uap->port.ops = &amba_pl011_pops;@@ -2487,6 +2551,67 @@ static int pl011_remove(struct amba_device *dev)pl011_unregister_port (uap);return 0 ; } +#endif + +#ifdef CONFIG_SOC_ZX296702 +static int zx_uart_probe (struct platform_device *pdev) +{ +struct uart_amba_port *uap; +struct vendor_data *vendor = &vendor_zte; +struct resource *res; +int portnr, ret; + + portnr =pl011_find_free_port (); +if (portnr <0 ) +return portnr; + + uap =devm_kzalloc (&pdev->dev,sizeof (struct uart_amba_port ), +GFP_KERNEL ); +if (!uap) { + ret = -ENOMEM ; + goto out; + } + + uap->clk =devm_clk_get (&pdev->dev,NULL ); +if (IS_ERR (uap->clk)) { + ret =PTR_ERR (uap->clk); + goto out; + } + + uap->vendor = vendor; + uap->reg_lut = vendor->reg_lut; + uap->lcrh_rx = vendor->lcrh_rx; + uap->lcrh_tx = vendor->lcrh_tx; + uap->fr_busy = vendor->fr_busy; + uap->fr_dsr = vendor->fr_dsr; + uap->fr_cts = vendor->fr_cts; + uap->fr_ri = vendor->fr_ri; + uap->fifosize =16 ; + uap->port.irq =platform_get_irq (pdev,0 ); + uap->port.ops = &amba_pl011_pops; + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + + ret =pl011_setup_port (&pdev->dev, uap, res, portnr); +if (ret) +return ret; + +platform_set_drvdata (pdev, uap); + +return pl011_register_port (uap); +out: +return ret; +} + +static int zx_uart_remove (struct platform_device *pdev) +{ +struct uart_amba_port *uap =platform_get_drvdata (pdev); + +uart_remove_one_port (&amba_reg, &uap->port); +pl011_unregister_port (uap); +return 0 ; +} +#endif #ifdef CONFIG_PM_SLEEP static int pl011_suspend (struct device *dev)@@ -2544,6 +2669,10 @@ static int sbsa_uart_probe(struct platform_device *pdev) uap->vendor = &vendor_sbsa; uap->reg_lut = vendor_sbsa.reg_lut; + uap->fr_busy = vendor_sbsa.fr_busy; + uap->fr_dsr = vendor_sbsa.fr_dsr; + uap->fr_cts = vendor_sbsa.fr_cts; + uap->fr_ri = vendor_sbsa.fr_ri; uap->fifosize =32 ; uap->port.irq =platform_get_irq (pdev,0 ); uap->port.ops = &sbsa_uart_pops;@@ -2593,6 +2722,7 @@ static struct platform_driver arm_sbsa_uart_platform_driver = { }, }; +#ifdef CONFIG_ARM_AMBA static struct amba_id pl011_ids[] = { { .id = 0x00041011,@@ -2618,20 +2748,57 @@ static struct amba_driver pl011_driver = { .probe = pl011_probe, .remove = pl011_remove, }; +#endif + +#ifdef CONFIG_SOC_ZX296702 +static const struct of_device_id zx_uart_dt_ids[] = { + { .compatible ="zte,zx296702-uart" , }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, zx_uart_dt_ids); + +static struct platform_driver zx_uart_driver = { + .driver = { + .name ="zx-uart" , + .owner =THIS_MODULE , + .pm = &pl011_dev_pm_ops, + .of_match_table = zx_uart_dt_ids, + }, + .probe = zx_uart_probe, + .remove = zx_uart_remove, +}; +#endif +static int __init pl011_init (void ) { +int ret;printk (KERN_INFO "Serial: AMBA PL011 UART driver\n" );if (platform_driver_register (&arm_sbsa_uart_platform_driver))pr_warn ("could not register SBSA UART platform driver\n" ); -return amba_driver_register (&pl011_driver); + +#ifdef CONFIG_SOC_ZX296702 + ret =platform_driver_register (&zx_uart_driver); +if (ret) +pr_warn ("could not register ZX UART platform driver\n" ); +#endif + +#ifdef CONFIG_ARM_AMBA + ret =amba_driver_register (&pl011_driver); +#endif +return ret; } static void __exit pl011_exit(void) {platform_driver_unregister (&arm_sbsa_uart_platform_driver); +#ifdef CONFIG_SOC_ZX296702 +platform_driver_unregister (&zx_uart_driver); +#endif +#ifdef CONFIG_ARM_AMBA amba_driver_unregister (&pl011_driver); +#endif }/* include/linux/amba/serial.h +14
-1@@ -33,12 +33,14 @@#define UART01x_DR 0x00/* Data read or written from the interface. */ #define UART01x_RSR 0x04/* Receive status register (Read). */ #define UART01x_ECR 0x04/* Error clear register (Write). */ +#define ZX_UART01x_DR 0x04/* Data read or written from the interface. */ #define UART010_LCRH 0x08/* Line control register, high byte. */ #define ST_UART011_DMAWM 0x08/* DMA watermark configure register. */ #define UART010_LCRM 0x0C/* Line control register, middle byte. */ #define ST_UART011_TIMEOUT 0x0C/* Timeout period register. */ #define UART010_LCRL 0x10/* Line control register, low byte. */ #define UART010_CR 0x14/* Control register. */ +#define ZX_UART01x_FR 0x14/* Flag register (Read only). */ #define UART01x_FR 0x18/* Flag register (Read only). */ #define UART010_IIR 0x1C/* Interrupt identification register (Read). */ #define UART010_ICR 0x1C/* Interrupt clear register (Write). */ @@ -49,13 +51,21 @@#define UART011_LCRH 0x2c/* Line control register. */ #define ST_UART011_LCRH_TX 0x2c/* Tx Line control register. */ #define UART011_CR 0x30/* Control register. */ +#define ZX_UART011_LCRH_TX 0x30/* Tx Line control register. */ #define UART011_IFLS 0x34/* Interrupt fifo level select. */ +#define ZX_UART011_CR 0x34/* Control register. */ +#define ZX_UART011_IFLS 0x38/* Interrupt fifo level select. */ #define UART011_IMSC 0x38/* Interrupt mask. */ #define UART011_RIS 0x3c/* Raw interrupt status. */ #define UART011_MIS 0x40/* Masked interrupt status. */ +#define ZX_UART011_IMSC 0x40/* Interrupt mask. */ #define UART011_ICR 0x44/* Interrupt clear register. */ +#define ZX_UART011_RIS 0x44/* Raw interrupt status. */ #define UART011_DMACR 0x48/* DMA control register. */ +#define ZX_UART011_MIS 0x48/* Masked interrupt status. */ +#define ZX_UART011_ICR 0x4c/* Interrupt clear register. */ #define ST_UART011_XFCR 0x50/* XON/XOFF control register. */ +#define ZX_UART011_DMACR 0x50/* DMA control register. */ #define ST_UART011_XON1 0x54/* XON1 register. */ #define ST_UART011_XON2 0x58/* XON2 register. */ #define ST_UART011_XOFF1 0x5C/* XON1 register. */ @@ -75,15 +85,19 @@#define UART01x_RSR_PE 0x02#define UART01x_RSR_FE 0x01 +#define ZX_UART01x_FR_BUSY 0x300#define UART011_FR_RI 0x100#define UART011_FR_TXFE 0x080#define UART011_FR_RXFF 0x040#define UART01x_FR_TXFF 0x020#define UART01x_FR_RXFE 0x010#define UART01x_FR_BUSY 0x008 +#define ZX_UART01x_FR_DSR 0x008#define UART01x_FR_DCD 0x004#define UART01x_FR_DSR 0x002 +#define ZX_UART01x_FR_CTS 0x002#define UART01x_FR_CTS 0x001 +#define ZX_UART011_FR_RI 0x001#define UART01x_FR_TMSK (UART01x_FR_TXFF + UART01x_FR_BUSY)#define UART011_CR_CTSEN 0x8000/* CTS hardware flow control */ --1.9.1
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
I guess I need to pick up maintanence of this driver again and stop it turning into the stinking pile of dogpoo that people are trying to make it... I don't have time this week to do that, nor next week though.
Quoted 488 lines · Andre Przywara
Hi Jun,
Quoted 46 lines · Jun Nie
Support ZTE uart with some registers differing offset. Probe as platform device for not AMBA IP ID is available on ZTE uart.
Signed-off-by Jun Nie <[email protected]>--- drivers/tty/serial/Kconfig | 4 +- drivers/tty/serial/amba-pl011.c | 195 +++++++++++++++++++++++++++++++++++++--- include/linux/amba/serial.h | 14 +++ 3 files changed, 197 insertions(+), 16 deletions(-)drivers/tty/serial/Kconfig +2
-2@@ -47,12 +47,12 @@ config SERIAL_AMBA_PL010_CONSOLE config SERIAL_AMBA_PL011 tristate "ARM AMBA PL011 serial port support" - depends on ARM_AMBA + depends on ARM_AMBA || SOC_ZX296702 select SERIAL_CORE help This selects the ARM(R) AMBA(R) PrimeCell PL011 UART. If you have an Integrator/PP2, Integrator/CP or Versatile platform, say Y or M - here. + here. Say Y or M if you have SOC_ZX296702. If unsure, say N.drivers/tty/serial/amba-pl011.c +4
-0@@ -74,6 +74,10 @@/* There is by now at least one vendor with differing details, so handle it */ struct vendor_data {unsigned int ifls; +unsigned int fr_busy; +unsigned int fr_dsr; +unsigned int fr_cts; +unsigned int fr_ri;unsigned int lcrh_tx;unsigned int lcrh_rx;u16 *reg_lut;drivers/tty/serial/amba-pl011.c static u16 arm_reg[] = { · line 134
Quoted 4 lines
drivers/tty/serial/amba-pl011.c +1
-0@@ -127,6 +131,7 @@ static u16 arm_reg[] = { [REG_DMACR] = UART011_DMACR, }; +#ifdef CONFIG_ARM_AMBA Maybe I missed that discussion (reading mailing list archives on the web is just horrible!), but why do we have all these #ifdefs here? The actual design of that driver extension is meant to fold well into the driver, with the changes only coming into effect if the DT node is found. To me it looks like we could even have a PL011 instance and a ZTE UART instance in operation at the same time.
So can't we get rid of those silly #ifdef
CONFIG_ARM_AMBAsat least, andCONFIG_SOC_ZX296702as well?Quoted 48 lines
drivers/tty/serial/amba-pl011.c +14
-0static unsigned int get_fifosize_arm (struct amba_device *dev) {return amba_rev (dev) <3 ?16 :32 ;@@ -134,6 +139,10 @@ static unsigned int get_fifosize_arm(struct amba_device *dev)static struct vendor_data vendor_arm = { .ifls =UART011_IFLS_RX4_8 |UART011_IFLS_TX4_8 , + .fr_busy = UART01x_FR_BUSY, + .fr_dsr = UART01x_FR_DSR, + .fr_cts = UART01x_FR_CTS, + .fr_ri =UART011_FR_RI , .lcrh_tx =REG_LCRH , .lcrh_rx =REG_LCRH , .reg_lut = arm_reg,@@ -144,8 +153,13 @@ static struct vendor_data vendor_arm = { .fixed_options = false, .get_fifosize = get_fifosize_arm, }; +#endif static struct vendor_data vendor_sbsa = { + .fr_busy = UART01x_FR_BUSY, + .fr_dsr = UART01x_FR_DSR, + .fr_cts = UART01x_FR_CTS, + .fr_ri =UART011_FR_RI , .reg_lut = arm_reg, .oversampling = false, .dma_threshold = false,@@ -154,6 +168,7 @@ static struct vendor_data vendor_sbsa = { .fixed_options = true, }; +#ifdef CONFIG_ARM_AMBA static u16 st_reg[] = { [REG_DR ] = UART01x_DR, [REG_RSR] = UART01x_RSR,@@ -180,6 +195,10 @@ static unsigned int get_fifosize_st(struct amba_device *dev)static struct vendor_data vendor_st = { .ifls =UART011_IFLS_RX_HALF |UART011_IFLS_TX_HALF , + .fr_busy = UART01x_FR_BUSY, + .fr_dsr = UART01x_FR_DSR, + .fr_cts = UART01x_FR_CTS, + .fr_ri =UART011_FR_RI , .lcrh_tx =REG_LCRH , .lcrh_rx =REG_ST_LCRH_RX , .reg_lut = st_reg,drivers/tty/serial/amba-pl011.c static struct vendor_data vendor_st = { · line 214
Quoted 6 lines
drivers/tty/serial/amba-pl011.c +3
-0@@ -190,6 +209,43 @@ static struct vendor_data vendor_st = { .fixed_options = false, .get_fifosize = get_fifosize_st, }; +#endif + +#ifdef CONFIG_SOC_ZX296702 As said above, I doubt the usefulness of this bracketing. But even if there are arguments in favour for that, shouldn't it be
CONFIG_ARCH_ZXinstead? Or is this UART just a mishap which happened to that very particular SoC and it will not show again in any other silicon?Cheers,
Andre.Quoted 360 lines
drivers/tty/serial/amba-pl011.c +159
-14+static u16 zte_reg[] = { + [REG_DR ] = ZX_UART01x_DR, + [REG_RSR ] = UART01x_RSR, + [REG_ST_DMAWM ] =ST_UART011_DMAWM , + [REG_FR ] = ZX_UART01x_FR, + [REG_ST_LCRH_RX ] =ST_UART011_LCRH_RX , + [REG_ILPR ] = UART01x_ILPR, + [REG_IBRD ] =UART011_IBRD , + [REG_FBRD ] =UART011_FBRD , + [REG_LCRH ] =ZX_UART011_LCRH_TX , + [REG_CR ] =ZX_UART011_CR , + [REG_IFLS ] =ZX_UART011_IFLS , + [REG_IMSC ] =ZX_UART011_IMSC , + [REG_RIS ] =ZX_UART011_RIS , + [REG_MIS ] =ZX_UART011_MIS , + [REG_ICR ] =ZX_UART011_ICR , + [REG_DMACR ] =ZX_UART011_DMACR , +}; + +static struct vendor_data vendor_zte = { + .ifls =UART011_IFLS_RX4_8 |UART011_IFLS_TX4_8 , + .fr_busy = ZX_UART01x_FR_BUSY, + .fr_dsr = ZX_UART01x_FR_DSR, + .fr_cts = ZX_UART01x_FR_CTS, + .fr_ri =ZX_UART011_FR_RI , + .lcrh_tx =REG_LCRH , + .lcrh_rx =REG_ST_LCRH_RX , + .reg_lut = zte_reg, + .oversampling = false, + .dma_threshold = false, + .cts_event_workaround = false, + .fixed_options = false, +}; +#endif /* Deals with DMA transactions */ @@ -233,6 +289,10 @@ struct uart_amba_port {unsigned int im;/* interrupt mask */ unsigned int old_status;unsigned int fifosize;/* vendor-specific */ +unsigned int fr_busy;/* vendor-specific */ +unsigned int fr_dsr;/* vendor-specific */ +unsigned int fr_cts;/* vendor-specific */ +unsigned int fr_ri;/* vendor-specific */ unsigned int lcrh_tx;/* vendor-specific */ unsigned int lcrh_rx;/* vendor-specific */ unsigned int old_cr;/* state during shutdown */ @@ -1163,7 +1223,7 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap)return ;/* Disable RX and TX DMA */ -while (pl011_readw (uap,REG_FR ) & UART01x_FR_BUSY) +while (pl011_readw (uap,REG_FR ) & uap->fr_busy)barrier ();spin_lock_irq (&uap->port.lock);@@ -1412,11 +1472,11 @@ static void pl011_modem_status(struct uart_amba_port *uap)if (delta & UART01x_FR_DCD)uart_handle_dcd_change (&uap->port, status & UART01x_FR_DCD); -if (delta & UART01x_FR_DSR) +if (delta & uap->fr_dsr) uap->port.icount.dsr++; -if (delta & UART01x_FR_CTS) -uart_handle_cts_change (&uap->port, status & UART01x_FR_CTS); +if (delta & uap->fr_cts) +uart_handle_cts_change (&uap->port, status & uap->fr_cts);wake_up_interruptible (&uap->port.state->port.delta_msr_wait); }@@ -1487,7 +1547,7 @@ static unsigned int pl011_tx_empty(struct uart_port *port)struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port);unsigned int status =pl011_readw (uap,REG_FR ); -return status & (UART01x_FR_BUSY|UART01x_FR_TXFF) ?0 :TIOCSER_TEMT ; +return status & (uap->fr_busy|UART01x_FR_TXFF) ?0 :TIOCSER_TEMT ; }static unsigned int pl011_get_mctrl (struct uart_port *port)@@ -1502,9 +1562,9 @@ static unsigned int pl011_get_mctrl(struct uart_port *port) result |= tiocmbitTIOCMBIT (UART01x_FR_DCD,TIOCM_CAR ); -TIOCMBIT (UART01x_FR_DSR,TIOCM_DSR ); -TIOCMBIT (UART01x_FR_CTS,TIOCM_CTS ); -TIOCMBIT (UART011_FR_RI ,TIOCM_RNG ); +TIOCMBIT (uap->fr_dsr,TIOCM_DSR ); +TIOCMBIT (uap->fr_cts,TIOCM_CTS ); +TIOCMBIT (uap->fr_ri,TIOCM_RNG );#undef TIOCMBITreturn result; }@@ -1720,8 +1780,7 @@ static int pl011_startup(struct uart_port *port)/* * initialise the old status of the modem signals */ - uap->old_status =pl011_readw (uap,REG_FR ) & - UART01x_FR_MODEM_ANY; + uap->old_status =pl011_readw (uap,REG_FR ) & UART01x_FR_MODEM_ANY;/* Startup DMA */ pl011_dma_startup (uap);@@ -1800,7 +1859,7 @@ static void pl011_disable_interrupts(struct uart_amba_port *uap)/* mask all interrupts and clear all pending ones */ uap->im =0 ;pl011_writew (uap, uap->im,REG_IMSC ); -pl011_writew (0xffff ,REG_ICR ); +pl011_writew (uap,0xffff ,REG_ICR );spin_unlock_irq (&uap->port.lock); }@@ -2178,7 +2237,7 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)*/ do { status =pl011_readw (uap,REG_FR ); - }while (status & UART01x_FR_BUSY); + }while (status & uap->fr_busy);if (!uap->vendor->always_enabled)pl011_writew (uap, old_cr,REG_CR );@@ -2295,7 +2354,7 @@ static void pl011_putc(struct uart_port *port, int c)while (pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF) ;pl011_writeb (uap, c,REG_DR ); -while (pl011_readw (uap,REG_FR ) & UART01x_FR_BUSY) +while (pl011_readw (uap,REG_FR ) & uap->fr_busy) ; }@@ -2441,6 +2500,7 @@ static int pl011_register_port(struct uart_amba_port *uap)return ret; } +#ifdef CONFIG_ARM_AMBA static int pl011_probe (struct amba_device *dev,const struct amba_id *id) {struct uart_amba_port *uap;@@ -2464,6 +2524,10 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id) uap->reg_lut = vendor->reg_lut; uap->lcrh_rx = vendor->lcrh_rx; uap->lcrh_tx = vendor->lcrh_tx; + uap->fr_busy = vendor->fr_busy; + uap->fr_dsr = vendor->fr_dsr; + uap->fr_cts = vendor->fr_cts; + uap->fr_ri = vendor->fr_ri; uap->fifosize = vendor->get_fifosize (dev); uap->port.irq = dev->irq[0 ]; uap->port.ops = &amba_pl011_pops;@@ -2487,6 +2551,67 @@ static int pl011_remove(struct amba_device *dev)pl011_unregister_port (uap);return 0 ; } +#endif + +#ifdef CONFIG_SOC_ZX296702 +static int zx_uart_probe (struct platform_device *pdev) +{ +struct uart_amba_port *uap; +struct vendor_data *vendor = &vendor_zte; +struct resource *res; +int portnr, ret; + + portnr =pl011_find_free_port (); +if (portnr <0 ) +return portnr; + + uap =devm_kzalloc (&pdev->dev,sizeof (struct uart_amba_port ), +GFP_KERNEL ); +if (!uap) { + ret = -ENOMEM ; + goto out; + } + + uap->clk =devm_clk_get (&pdev->dev,NULL ); +if (IS_ERR (uap->clk)) { + ret =PTR_ERR (uap->clk); + goto out; + } + + uap->vendor = vendor; + uap->reg_lut = vendor->reg_lut; + uap->lcrh_rx = vendor->lcrh_rx; + uap->lcrh_tx = vendor->lcrh_tx; + uap->fr_busy = vendor->fr_busy; + uap->fr_dsr = vendor->fr_dsr; + uap->fr_cts = vendor->fr_cts; + uap->fr_ri = vendor->fr_ri; + uap->fifosize =16 ; + uap->port.irq =platform_get_irq (pdev,0 ); + uap->port.ops = &amba_pl011_pops; + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + + ret =pl011_setup_port (&pdev->dev, uap, res, portnr); +if (ret) +return ret; + +platform_set_drvdata (pdev, uap); + +return pl011_register_port (uap); +out: +return ret; +} + +static int zx_uart_remove (struct platform_device *pdev) +{ +struct uart_amba_port *uap =platform_get_drvdata (pdev); + +uart_remove_one_port (&amba_reg, &uap->port); +pl011_unregister_port (uap); +return 0 ; +} +#endif #ifdef CONFIG_PM_SLEEP static int pl011_suspend (struct device *dev)@@ -2544,6 +2669,10 @@ static int sbsa_uart_probe(struct platform_device *pdev) uap->vendor = &vendor_sbsa; uap->reg_lut = vendor_sbsa.reg_lut; + uap->fr_busy = vendor_sbsa.fr_busy; + uap->fr_dsr = vendor_sbsa.fr_dsr; + uap->fr_cts = vendor_sbsa.fr_cts; + uap->fr_ri = vendor_sbsa.fr_ri; uap->fifosize =32 ; uap->port.irq =platform_get_irq (pdev,0 ); uap->port.ops = &sbsa_uart_pops;@@ -2593,6 +2722,7 @@ static struct platform_driver arm_sbsa_uart_platform_driver = { }, }; +#ifdef CONFIG_ARM_AMBA static struct amba_id pl011_ids[] = { { .id = 0x00041011,@@ -2618,20 +2748,57 @@ static struct amba_driver pl011_driver = { .probe = pl011_probe, .remove = pl011_remove, }; +#endif + +#ifdef CONFIG_SOC_ZX296702 +static const struct of_device_id zx_uart_dt_ids[] = { + { .compatible ="zte,zx296702-uart" , }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, zx_uart_dt_ids); + +static struct platform_driver zx_uart_driver = { + .driver = { + .name ="zx-uart" , + .owner =THIS_MODULE , + .pm = &pl011_dev_pm_ops, + .of_match_table = zx_uart_dt_ids, + }, + .probe = zx_uart_probe, + .remove = zx_uart_remove, +}; +#endif +static int __init pl011_init (void ) { +int ret;printk (KERN_INFO "Serial: AMBA PL011 UART driver\n" );if (platform_driver_register (&arm_sbsa_uart_platform_driver))pr_warn ("could not register SBSA UART platform driver\n" ); -return amba_driver_register (&pl011_driver); + +#ifdef CONFIG_SOC_ZX296702 + ret =platform_driver_register (&zx_uart_driver); +if (ret) +pr_warn ("could not register ZX UART platform driver\n" ); +#endif + +#ifdef CONFIG_ARM_AMBA + ret =amba_driver_register (&pl011_driver); +#endif +return ret; } static void __exit pl011_exit(void) {platform_driver_unregister (&arm_sbsa_uart_platform_driver); +#ifdef CONFIG_SOC_ZX296702 +platform_driver_unregister (&zx_uart_driver); +#endif +#ifdef CONFIG_ARM_AMBA amba_driver_unregister (&pl011_driver); +#endif }/* include/linux/amba/serial.h +14
-1@@ -33,12 +33,14 @@#define UART01x_DR 0x00/* Data read or written from the interface. */ #define UART01x_RSR 0x04/* Receive status register (Read). */ #define UART01x_ECR 0x04/* Error clear register (Write). */ +#define ZX_UART01x_DR 0x04/* Data read or written from the interface. */ #define UART010_LCRH 0x08/* Line control register, high byte. */ #define ST_UART011_DMAWM 0x08/* DMA watermark configure register. */ #define UART010_LCRM 0x0C/* Line control register, middle byte. */ #define ST_UART011_TIMEOUT 0x0C/* Timeout period register. */ #define UART010_LCRL 0x10/* Line control register, low byte. */ #define UART010_CR 0x14/* Control register. */ +#define ZX_UART01x_FR 0x14/* Flag register (Read only). */ #define UART01x_FR 0x18/* Flag register (Read only). */ #define UART010_IIR 0x1C/* Interrupt identification register (Read). */ #define UART010_ICR 0x1C/* Interrupt clear register (Write). */ @@ -49,13 +51,21 @@#define UART011_LCRH 0x2c/* Line control register. */ #define ST_UART011_LCRH_TX 0x2c/* Tx Line control register. */ #define UART011_CR 0x30/* Control register. */ +#define ZX_UART011_LCRH_TX 0x30/* Tx Line control register. */ #define UART011_IFLS 0x34/* Interrupt fifo level select. */ +#define ZX_UART011_CR 0x34/* Control register. */ +#define ZX_UART011_IFLS 0x38/* Interrupt fifo level select. */ #define UART011_IMSC 0x38/* Interrupt mask. */ #define UART011_RIS 0x3c/* Raw interrupt status. */ #define UART011_MIS 0x40/* Masked interrupt status. */ +#define ZX_UART011_IMSC 0x40/* Interrupt mask. */ #define UART011_ICR 0x44/* Interrupt clear register. */ +#define ZX_UART011_RIS 0x44/* Raw interrupt status. */ #define UART011_DMACR 0x48/* DMA control register. */ +#define ZX_UART011_MIS 0x48/* Masked interrupt status. */ +#define ZX_UART011_ICR 0x4c/* Interrupt clear register. */ #define ST_UART011_XFCR 0x50/* XON/XOFF control register. */ +#define ZX_UART011_DMACR 0x50/* DMA control register. */ #define ST_UART011_XON1 0x54/* XON1 register. */ #define ST_UART011_XON2 0x58/* XON2 register. */ #define ST_UART011_XOFF1 0x5C/* XON1 register. */ @@ -75,15 +85,19 @@#define UART01x_RSR_PE 0x02#define UART01x_RSR_FE 0x01 +#define ZX_UART01x_FR_BUSY 0x300#define UART011_FR_RI 0x100#define UART011_FR_TXFE 0x080#define UART011_FR_RXFF 0x040#define UART01x_FR_TXFF 0x020#define UART01x_FR_RXFE 0x010#define UART01x_FR_BUSY 0x008 +#define ZX_UART01x_FR_DSR 0x008#define UART01x_FR_DCD 0x004#define UART01x_FR_DSR 0x002 +#define ZX_UART01x_FR_CTS 0x002#define UART01x_FR_CTS 0x001 +#define ZX_UART011_FR_RI 0x001#define UART01x_FR_TMSK (UART01x_FR_TXFF + UART01x_FR_BUSY)#define UART011_CR_CTSEN 0x8000/* CTS hardware flow control */ --1.9.1
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
2015-09-18 21:59 GMT+08:00 Russell King - ARM Linux <[email protected]>:
Quoted 4 lines
I guess I need to pick up maintanence of this driver again and stop it turning into the stinking pile of dogpoo that people are trying to make it... I don't have time this week to do that, nor next week though.
Welcome back to maintain this driver and your comments on CODE is also welcome.
Jun
Quoted 493 lines · Andre Przywara
Quoted 484 lines
Hi Jun,
Quoted 46 lines · Jun Nie
Support ZTE uart with some registers differing offset. Probe as platform device for not AMBA IP ID is available on ZTE uart.
Signed-off-by Jun Nie <[email protected]>--- drivers/tty/serial/Kconfig | 4 +- drivers/tty/serial/amba-pl011.c | 195 +++++++++++++++++++++++++++++++++++++--- include/linux/amba/serial.h | 14 +++ 3 files changed, 197 insertions(+), 16 deletions(-)drivers/tty/serial/Kconfig +2
-2@@ -47,12 +47,12 @@ config SERIAL_AMBA_PL010_CONSOLE config SERIAL_AMBA_PL011 tristate "ARM AMBA PL011 serial port support" - depends on ARM_AMBA + depends on ARM_AMBA || SOC_ZX296702 select SERIAL_CORE help This selects the ARM(R) AMBA(R) PrimeCell PL011 UART. If you have an Integrator/PP2, Integrator/CP or Versatile platform, say Y or M - here. + here. Say Y or M if you have SOC_ZX296702. If unsure, say N.drivers/tty/serial/amba-pl011.c +4
-0@@ -74,6 +74,10 @@/* There is by now at least one vendor with differing details, so handle it */ struct vendor_data {unsigned int ifls; +unsigned int fr_busy; +unsigned int fr_dsr; +unsigned int fr_cts; +unsigned int fr_ri;unsigned int lcrh_tx;unsigned int lcrh_rx;u16 *reg_lut;drivers/tty/serial/amba-pl011.c static u16 arm_reg[] = { · line 134
Quoted 4 lines
drivers/tty/serial/amba-pl011.c +1
-0@@ -127,6 +131,7 @@ static u16 arm_reg[] = { [REG_DMACR] = UART011_DMACR, }; +#ifdef CONFIG_ARM_AMBA Maybe I missed that discussion (reading mailing list archives on the web is just horrible!), but why do we have all these #ifdefs here? The actual design of that driver extension is meant to fold well into the driver, with the changes only coming into effect if the DT node is found. To me it looks like we could even have a PL011 instance and a ZTE UART instance in operation at the same time.
So can't we get rid of those silly #ifdef
CONFIG_ARM_AMBAsat least, andCONFIG_SOC_ZX296702as well?Quoted 48 lines
drivers/tty/serial/amba-pl011.c +14
-0static unsigned int get_fifosize_arm (struct amba_device *dev) {return amba_rev (dev) <3 ?16 :32 ;@@ -134,6 +139,10 @@ static unsigned int get_fifosize_arm(struct amba_device *dev)static struct vendor_data vendor_arm = { .ifls =UART011_IFLS_RX4_8 |UART011_IFLS_TX4_8 , + .fr_busy = UART01x_FR_BUSY, + .fr_dsr = UART01x_FR_DSR, + .fr_cts = UART01x_FR_CTS, + .fr_ri =UART011_FR_RI , .lcrh_tx =REG_LCRH , .lcrh_rx =REG_LCRH , .reg_lut = arm_reg,@@ -144,8 +153,13 @@ static struct vendor_data vendor_arm = { .fixed_options = false, .get_fifosize = get_fifosize_arm, }; +#endif static struct vendor_data vendor_sbsa = { + .fr_busy = UART01x_FR_BUSY, + .fr_dsr = UART01x_FR_DSR, + .fr_cts = UART01x_FR_CTS, + .fr_ri =UART011_FR_RI , .reg_lut = arm_reg, .oversampling = false, .dma_threshold = false,@@ -154,6 +168,7 @@ static struct vendor_data vendor_sbsa = { .fixed_options = true, }; +#ifdef CONFIG_ARM_AMBA static u16 st_reg[] = { [REG_DR ] = UART01x_DR, [REG_RSR] = UART01x_RSR,@@ -180,6 +195,10 @@ static unsigned int get_fifosize_st(struct amba_device *dev)static struct vendor_data vendor_st = { .ifls =UART011_IFLS_RX_HALF |UART011_IFLS_TX_HALF , + .fr_busy = UART01x_FR_BUSY, + .fr_dsr = UART01x_FR_DSR, + .fr_cts = UART01x_FR_CTS, + .fr_ri =UART011_FR_RI , .lcrh_tx =REG_LCRH , .lcrh_rx =REG_ST_LCRH_RX , .reg_lut = st_reg,drivers/tty/serial/amba-pl011.c static struct vendor_data vendor_st = { · line 214
Quoted 6 lines
drivers/tty/serial/amba-pl011.c +3
-0@@ -190,6 +209,43 @@ static struct vendor_data vendor_st = { .fixed_options = false, .get_fifosize = get_fifosize_st, }; +#endif + +#ifdef CONFIG_SOC_ZX296702 As said above, I doubt the usefulness of this bracketing. But even if there are arguments in favour for that, shouldn't it be
CONFIG_ARCH_ZXinstead? Or is this UART just a mishap which happened to that very particular SoC and it will not show again in any other silicon?Cheers,
Andre.Quoted 356 lines
drivers/tty/serial/amba-pl011.c +159
-14+static u16 zte_reg[] = { + [REG_DR ] = ZX_UART01x_DR, + [REG_RSR ] = UART01x_RSR, + [REG_ST_DMAWM ] =ST_UART011_DMAWM , + [REG_FR ] = ZX_UART01x_FR, + [REG_ST_LCRH_RX ] =ST_UART011_LCRH_RX , + [REG_ILPR ] = UART01x_ILPR, + [REG_IBRD ] =UART011_IBRD , + [REG_FBRD ] =UART011_FBRD , + [REG_LCRH ] =ZX_UART011_LCRH_TX , + [REG_CR ] =ZX_UART011_CR , + [REG_IFLS ] =ZX_UART011_IFLS , + [REG_IMSC ] =ZX_UART011_IMSC , + [REG_RIS ] =ZX_UART011_RIS , + [REG_MIS ] =ZX_UART011_MIS , + [REG_ICR ] =ZX_UART011_ICR , + [REG_DMACR ] =ZX_UART011_DMACR , +}; + +static struct vendor_data vendor_zte = { + .ifls =UART011_IFLS_RX4_8 |UART011_IFLS_TX4_8 , + .fr_busy = ZX_UART01x_FR_BUSY, + .fr_dsr = ZX_UART01x_FR_DSR, + .fr_cts = ZX_UART01x_FR_CTS, + .fr_ri =ZX_UART011_FR_RI , + .lcrh_tx =REG_LCRH , + .lcrh_rx =REG_ST_LCRH_RX , + .reg_lut = zte_reg, + .oversampling = false, + .dma_threshold = false, + .cts_event_workaround = false, + .fixed_options = false, +}; +#endif /* Deals with DMA transactions */ @@ -233,6 +289,10 @@ struct uart_amba_port {unsigned int im;/* interrupt mask */ unsigned int old_status;unsigned int fifosize;/* vendor-specific */ +unsigned int fr_busy;/* vendor-specific */ +unsigned int fr_dsr;/* vendor-specific */ +unsigned int fr_cts;/* vendor-specific */ +unsigned int fr_ri;/* vendor-specific */ unsigned int lcrh_tx;/* vendor-specific */ unsigned int lcrh_rx;/* vendor-specific */ unsigned int old_cr;/* state during shutdown */ @@ -1163,7 +1223,7 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap)return ;/* Disable RX and TX DMA */ -while (pl011_readw (uap,REG_FR ) & UART01x_FR_BUSY) +while (pl011_readw (uap,REG_FR ) & uap->fr_busy)barrier ();spin_lock_irq (&uap->port.lock);@@ -1412,11 +1472,11 @@ static void pl011_modem_status(struct uart_amba_port *uap)if (delta & UART01x_FR_DCD)uart_handle_dcd_change (&uap->port, status & UART01x_FR_DCD); -if (delta & UART01x_FR_DSR) +if (delta & uap->fr_dsr) uap->port.icount.dsr++; -if (delta & UART01x_FR_CTS) -uart_handle_cts_change (&uap->port, status & UART01x_FR_CTS); +if (delta & uap->fr_cts) +uart_handle_cts_change (&uap->port, status & uap->fr_cts);wake_up_interruptible (&uap->port.state->port.delta_msr_wait); }@@ -1487,7 +1547,7 @@ static unsigned int pl011_tx_empty(struct uart_port *port)struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port);unsigned int status =pl011_readw (uap,REG_FR ); -return status & (UART01x_FR_BUSY|UART01x_FR_TXFF) ?0 :TIOCSER_TEMT ; +return status & (uap->fr_busy|UART01x_FR_TXFF) ?0 :TIOCSER_TEMT ; }static unsigned int pl011_get_mctrl (struct uart_port *port)@@ -1502,9 +1562,9 @@ static unsigned int pl011_get_mctrl(struct uart_port *port) result |= tiocmbitTIOCMBIT (UART01x_FR_DCD,TIOCM_CAR ); -TIOCMBIT (UART01x_FR_DSR,TIOCM_DSR ); -TIOCMBIT (UART01x_FR_CTS,TIOCM_CTS ); -TIOCMBIT (UART011_FR_RI ,TIOCM_RNG ); +TIOCMBIT (uap->fr_dsr,TIOCM_DSR ); +TIOCMBIT (uap->fr_cts,TIOCM_CTS ); +TIOCMBIT (uap->fr_ri,TIOCM_RNG );#undef TIOCMBITreturn result; }@@ -1720,8 +1780,7 @@ static int pl011_startup(struct uart_port *port)/* * initialise the old status of the modem signals */ - uap->old_status =pl011_readw (uap,REG_FR ) & - UART01x_FR_MODEM_ANY; + uap->old_status =pl011_readw (uap,REG_FR ) & UART01x_FR_MODEM_ANY;/* Startup DMA */ pl011_dma_startup (uap);@@ -1800,7 +1859,7 @@ static void pl011_disable_interrupts(struct uart_amba_port *uap)/* mask all interrupts and clear all pending ones */ uap->im =0 ;pl011_writew (uap, uap->im,REG_IMSC ); -pl011_writew (0xffff ,REG_ICR ); +pl011_writew (uap,0xffff ,REG_ICR );spin_unlock_irq (&uap->port.lock); }@@ -2178,7 +2237,7 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)*/ do { status =pl011_readw (uap,REG_FR ); - }while (status & UART01x_FR_BUSY); + }while (status & uap->fr_busy);if (!uap->vendor->always_enabled)pl011_writew (uap, old_cr,REG_CR );@@ -2295,7 +2354,7 @@ static void pl011_putc(struct uart_port *port, int c)while (pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF) ;pl011_writeb (uap, c,REG_DR ); -while (pl011_readw (uap,REG_FR ) & UART01x_FR_BUSY) +while (pl011_readw (uap,REG_FR ) & uap->fr_busy) ; }@@ -2441,6 +2500,7 @@ static int pl011_register_port(struct uart_amba_port *uap)return ret; } +#ifdef CONFIG_ARM_AMBA static int pl011_probe (struct amba_device *dev,const struct amba_id *id) {struct uart_amba_port *uap;@@ -2464,6 +2524,10 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id) uap->reg_lut = vendor->reg_lut; uap->lcrh_rx = vendor->lcrh_rx; uap->lcrh_tx = vendor->lcrh_tx; + uap->fr_busy = vendor->fr_busy; + uap->fr_dsr = vendor->fr_dsr; + uap->fr_cts = vendor->fr_cts; + uap->fr_ri = vendor->fr_ri; uap->fifosize = vendor->get_fifosize (dev); uap->port.irq = dev->irq[0 ]; uap->port.ops = &amba_pl011_pops;@@ -2487,6 +2551,67 @@ static int pl011_remove(struct amba_device *dev)pl011_unregister_port (uap);return 0 ; } +#endif + +#ifdef CONFIG_SOC_ZX296702 +static int zx_uart_probe (struct platform_device *pdev) +{ +struct uart_amba_port *uap; +struct vendor_data *vendor = &vendor_zte; +struct resource *res; +int portnr, ret; + + portnr =pl011_find_free_port (); +if (portnr <0 ) +return portnr; + + uap =devm_kzalloc (&pdev->dev,sizeof (struct uart_amba_port ), +GFP_KERNEL ); +if (!uap) { + ret = -ENOMEM ; + goto out; + } + + uap->clk =devm_clk_get (&pdev->dev,NULL ); +if (IS_ERR (uap->clk)) { + ret =PTR_ERR (uap->clk); + goto out; + } + + uap->vendor = vendor; + uap->reg_lut = vendor->reg_lut; + uap->lcrh_rx = vendor->lcrh_rx; + uap->lcrh_tx = vendor->lcrh_tx; + uap->fr_busy = vendor->fr_busy; + uap->fr_dsr = vendor->fr_dsr; + uap->fr_cts = vendor->fr_cts; + uap->fr_ri = vendor->fr_ri; + uap->fifosize =16 ; + uap->port.irq =platform_get_irq (pdev,0 ); + uap->port.ops = &amba_pl011_pops; + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + + ret =pl011_setup_port (&pdev->dev, uap, res, portnr); +if (ret) +return ret; + +platform_set_drvdata (pdev, uap); + +return pl011_register_port (uap); +out: +return ret; +} + +static int zx_uart_remove (struct platform_device *pdev) +{ +struct uart_amba_port *uap =platform_get_drvdata (pdev); + +uart_remove_one_port (&amba_reg, &uap->port); +pl011_unregister_port (uap); +return 0 ; +} +#endif #ifdef CONFIG_PM_SLEEP static int pl011_suspend (struct device *dev)@@ -2544,6 +2669,10 @@ static int sbsa_uart_probe(struct platform_device *pdev) uap->vendor = &vendor_sbsa; uap->reg_lut = vendor_sbsa.reg_lut; + uap->fr_busy = vendor_sbsa.fr_busy; + uap->fr_dsr = vendor_sbsa.fr_dsr; + uap->fr_cts = vendor_sbsa.fr_cts; + uap->fr_ri = vendor_sbsa.fr_ri; uap->fifosize =32 ; uap->port.irq =platform_get_irq (pdev,0 ); uap->port.ops = &sbsa_uart_pops;@@ -2593,6 +2722,7 @@ static struct platform_driver arm_sbsa_uart_platform_driver = { }, }; +#ifdef CONFIG_ARM_AMBA static struct amba_id pl011_ids[] = { { .id = 0x00041011,@@ -2618,20 +2748,57 @@ static struct amba_driver pl011_driver = { .probe = pl011_probe, .remove = pl011_remove, }; +#endif + +#ifdef CONFIG_SOC_ZX296702 +static const struct of_device_id zx_uart_dt_ids[] = { + { .compatible ="zte,zx296702-uart" , }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, zx_uart_dt_ids); + +static struct platform_driver zx_uart_driver = { + .driver = { + .name ="zx-uart" , + .owner =THIS_MODULE , + .pm = &pl011_dev_pm_ops, + .of_match_table = zx_uart_dt_ids, + }, + .probe = zx_uart_probe, + .remove = zx_uart_remove, +}; +#endif +static int __init pl011_init (void ) { +int ret;printk (KERN_INFO "Serial: AMBA PL011 UART driver\n" );if (platform_driver_register (&arm_sbsa_uart_platform_driver))pr_warn ("could not register SBSA UART platform driver\n" ); -return amba_driver_register (&pl011_driver); + +#ifdef CONFIG_SOC_ZX296702 + ret =platform_driver_register (&zx_uart_driver); +if (ret) +pr_warn ("could not register ZX UART platform driver\n" ); +#endif + +#ifdef CONFIG_ARM_AMBA + ret =amba_driver_register (&pl011_driver); +#endif +return ret; } static void __exit pl011_exit(void) {platform_driver_unregister (&arm_sbsa_uart_platform_driver); +#ifdef CONFIG_SOC_ZX296702 +platform_driver_unregister (&zx_uart_driver); +#endif +#ifdef CONFIG_ARM_AMBA amba_driver_unregister (&pl011_driver); +#endif }/* include/linux/amba/serial.h +14
-0@@ -33,12 +33,14 @@#define UART01x_DR 0x00/* Data read or written from the interface. */ #define UART01x_RSR 0x04/* Receive status register (Read). */ #define UART01x_ECR 0x04/* Error clear register (Write). */ +#define ZX_UART01x_DR 0x04/* Data read or written from the interface. */ #define UART010_LCRH 0x08/* Line control register, high byte. */ #define ST_UART011_DMAWM 0x08/* DMA watermark configure register. */ #define UART010_LCRM 0x0C/* Line control register, middle byte. */ #define ST_UART011_TIMEOUT 0x0C/* Timeout period register. */ #define UART010_LCRL 0x10/* Line control register, low byte. */ #define UART010_CR 0x14/* Control register. */ +#define ZX_UART01x_FR 0x14/* Flag register (Read only). */ #define UART01x_FR 0x18/* Flag register (Read only). */ #define UART010_IIR 0x1C/* Interrupt identification register (Read). */ #define UART010_ICR 0x1C/* Interrupt clear register (Write). */ @@ -49,13 +51,21 @@#define UART011_LCRH 0x2c/* Line control register. */ #define ST_UART011_LCRH_TX 0x2c/* Tx Line control register. */ #define UART011_CR 0x30/* Control register. */ +#define ZX_UART011_LCRH_TX 0x30/* Tx Line control register. */ #define UART011_IFLS 0x34/* Interrupt fifo level select. */ +#define ZX_UART011_CR 0x34/* Control register. */ +#define ZX_UART011_IFLS 0x38/* Interrupt fifo level select. */ #define UART011_IMSC 0x38/* Interrupt mask. */ #define UART011_RIS 0x3c/* Raw interrupt status. */ #define UART011_MIS 0x40/* Masked interrupt status. */ +#define ZX_UART011_IMSC 0x40/* Interrupt mask. */ #define UART011_ICR 0x44/* Interrupt clear register. */ +#define ZX_UART011_RIS 0x44/* Raw interrupt status. */ #define UART011_DMACR 0x48/* DMA control register. */ +#define ZX_UART011_MIS 0x48/* Masked interrupt status. */ +#define ZX_UART011_ICR 0x4c/* Interrupt clear register. */ #define ST_UART011_XFCR 0x50/* XON/XOFF control register. */ +#define ZX_UART011_DMACR 0x50/* DMA control register. */ #define ST_UART011_XON1 0x54/* XON1 register. */ #define ST_UART011_XON2 0x58/* XON2 register. */ #define ST_UART011_XOFF1 0x5C/* XON1 register. */ @@ -75,15 +85,19 @@#define UART01x_RSR_PE 0x02#define UART01x_RSR_FE 0x01 +#define ZX_UART01x_FR_BUSY 0x300#define UART011_FR_RI 0x100#define UART011_FR_TXFE 0x080#define UART011_FR_RXFF 0x040#define UART01x_FR_TXFF 0x020#define UART01x_FR_RXFE 0x010#define UART01x_FR_BUSY 0x008 +#define ZX_UART01x_FR_DSR 0x008#define UART01x_FR_DCD 0x004#define UART01x_FR_DSR 0x002 +#define ZX_UART01x_FR_CTS 0x002#define UART01x_FR_CTS 0x001 +#define ZX_UART011_FR_RI 0x001#define UART01x_FR_TMSK (UART01x_FR_TXFF + UART01x_FR_BUSY)Quoted 4 lines
Quoted 4 lines
include/linux/amba/serial.h +0
-1#define UART011_CR_CTSEN 0x8000/* CTS hardware flow control */ --1.9.1
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
In reply to Andre Przywara2015-09-18 21:50 GMT+08:00 Andre Przywara <[email protected]>:
Quoted 61 lines
Hi Jun,
Quoted 46 lines · Jun Nie
Support ZTE uart with some registers differing offset. Probe as platform device for not AMBA IP ID is available on ZTE uart.
Signed-off-by Jun Nie <[email protected]>--- drivers/tty/serial/Kconfig | 4 +- drivers/tty/serial/amba-pl011.c | 195 +++++++++++++++++++++++++++++++++++++--- include/linux/amba/serial.h | 14 +++ 3 files changed, 197 insertions(+), 16 deletions(-)drivers/tty/serial/Kconfig +2
-2@@ -47,12 +47,12 @@ config SERIAL_AMBA_PL010_CONSOLE config SERIAL_AMBA_PL011 tristate "ARM AMBA PL011 serial port support" - depends on ARM_AMBA + depends on ARM_AMBA || SOC_ZX296702 select SERIAL_CORE help This selects the ARM(R) AMBA(R) PrimeCell PL011 UART. If you have an Integrator/PP2, Integrator/CP or Versatile platform, say Y or M - here. + here. Say Y or M if you have SOC_ZX296702. If unsure, say N.drivers/tty/serial/amba-pl011.c +4
-0@@ -74,6 +74,10 @@/* There is by now at least one vendor with differing details, so handle it */ struct vendor_data {unsigned int ifls; +unsigned int fr_busy; +unsigned int fr_dsr; +unsigned int fr_cts; +unsigned int fr_ri;unsigned int lcrh_tx;unsigned int lcrh_rx;u16 *reg_lut;drivers/tty/serial/amba-pl011.c static u16 arm_reg[] = { · line 134
Quoted 4 lines
drivers/tty/serial/amba-pl011.c +1
-0@@ -127,6 +131,7 @@ static u16 arm_reg[] = { [REG_DMACR] = UART011_DMACR, }; +#ifdef CONFIG_ARM_AMBA Maybe I missed that discussion (reading mailing list archives on the web is just horrible!), but why do we have all these #ifdefs here? The actual design of that driver extension is meant to fold well into the driver, with the changes only coming into effect if the DT node is found. To me it looks like we could even have a PL011 instance and a ZTE UART instance in operation at the same time.
Quoted 3 lines
So can't we get rid of those silly #ifdef
CONFIG_ARM_AMBAsat least, andCONFIG_SOC_ZX296702as well?
You are right, it is possible to have both PL011 UART and ZTE version UART in the same SOC in theory and it is better to remove the CONFIGs. Will try to change it in future.
Jun
Quoted 61 lines
Quoted 48 lines
drivers/tty/serial/amba-pl011.c +14
-0static unsigned int get_fifosize_arm (struct amba_device *dev) {return amba_rev (dev) <3 ?16 :32 ;@@ -134,6 +139,10 @@ static unsigned int get_fifosize_arm(struct amba_device *dev)static struct vendor_data vendor_arm = { .ifls =UART011_IFLS_RX4_8 |UART011_IFLS_TX4_8 , + .fr_busy = UART01x_FR_BUSY, + .fr_dsr = UART01x_FR_DSR, + .fr_cts = UART01x_FR_CTS, + .fr_ri =UART011_FR_RI , .lcrh_tx =REG_LCRH , .lcrh_rx =REG_LCRH , .reg_lut = arm_reg,@@ -144,8 +153,13 @@ static struct vendor_data vendor_arm = { .fixed_options = false, .get_fifosize = get_fifosize_arm, }; +#endif static struct vendor_data vendor_sbsa = { + .fr_busy = UART01x_FR_BUSY, + .fr_dsr = UART01x_FR_DSR, + .fr_cts = UART01x_FR_CTS, + .fr_ri =UART011_FR_RI , .reg_lut = arm_reg, .oversampling = false, .dma_threshold = false,@@ -154,6 +168,7 @@ static struct vendor_data vendor_sbsa = { .fixed_options = true, }; +#ifdef CONFIG_ARM_AMBA static u16 st_reg[] = { [REG_DR ] = UART01x_DR, [REG_RSR] = UART01x_RSR,@@ -180,6 +195,10 @@ static unsigned int get_fifosize_st(struct amba_device *dev)static struct vendor_data vendor_st = { .ifls =UART011_IFLS_RX_HALF |UART011_IFLS_TX_HALF , + .fr_busy = UART01x_FR_BUSY, + .fr_dsr = UART01x_FR_DSR, + .fr_cts = UART01x_FR_CTS, + .fr_ri =UART011_FR_RI , .lcrh_tx =REG_LCRH , .lcrh_rx =REG_ST_LCRH_RX , .reg_lut = st_reg,drivers/tty/serial/amba-pl011.c static struct vendor_data vendor_st = { · line 214
Quoted 6 lines
drivers/tty/serial/amba-pl011.c +3
-0@@ -190,6 +209,43 @@ static struct vendor_data vendor_st = { .fixed_options = false, .get_fifosize = get_fifosize_st, }; +#endif + +#ifdef CONFIG_SOC_ZX296702 As said above, I doubt the usefulness of this bracketing. But even if there are arguments in favour for that, shouldn't it be
CONFIG_ARCH_ZXinstead? Or is this UART just a mishap which happened to that very particular SoC and it will not show again in any other silicon?
Quoted 3 lines
Cheers,
Andre.
I am not sure the silicon designer's plan on the UART IP. Different silicon team have different decision on the IP choice. So I guess future ZTE SOC may reuse ZTE UART per people assignment.
Quoted 360 lines
Quoted 360 lines
drivers/tty/serial/amba-pl011.c +159
-14+static u16 zte_reg[] = { + [REG_DR ] = ZX_UART01x_DR, + [REG_RSR ] = UART01x_RSR, + [REG_ST_DMAWM ] =ST_UART011_DMAWM , + [REG_FR ] = ZX_UART01x_FR, + [REG_ST_LCRH_RX ] =ST_UART011_LCRH_RX , + [REG_ILPR ] = UART01x_ILPR, + [REG_IBRD ] =UART011_IBRD , + [REG_FBRD ] =UART011_FBRD , + [REG_LCRH ] =ZX_UART011_LCRH_TX , + [REG_CR ] =ZX_UART011_CR , + [REG_IFLS ] =ZX_UART011_IFLS , + [REG_IMSC ] =ZX_UART011_IMSC , + [REG_RIS ] =ZX_UART011_RIS , + [REG_MIS ] =ZX_UART011_MIS , + [REG_ICR ] =ZX_UART011_ICR , + [REG_DMACR ] =ZX_UART011_DMACR , +}; + +static struct vendor_data vendor_zte = { + .ifls =UART011_IFLS_RX4_8 |UART011_IFLS_TX4_8 , + .fr_busy = ZX_UART01x_FR_BUSY, + .fr_dsr = ZX_UART01x_FR_DSR, + .fr_cts = ZX_UART01x_FR_CTS, + .fr_ri =ZX_UART011_FR_RI , + .lcrh_tx =REG_LCRH , + .lcrh_rx =REG_ST_LCRH_RX , + .reg_lut = zte_reg, + .oversampling = false, + .dma_threshold = false, + .cts_event_workaround = false, + .fixed_options = false, +}; +#endif /* Deals with DMA transactions */ @@ -233,6 +289,10 @@ struct uart_amba_port {unsigned int im;/* interrupt mask */ unsigned int old_status;unsigned int fifosize;/* vendor-specific */ +unsigned int fr_busy;/* vendor-specific */ +unsigned int fr_dsr;/* vendor-specific */ +unsigned int fr_cts;/* vendor-specific */ +unsigned int fr_ri;/* vendor-specific */ unsigned int lcrh_tx;/* vendor-specific */ unsigned int lcrh_rx;/* vendor-specific */ unsigned int old_cr;/* state during shutdown */ @@ -1163,7 +1223,7 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap)return ;/* Disable RX and TX DMA */ -while (pl011_readw (uap,REG_FR ) & UART01x_FR_BUSY) +while (pl011_readw (uap,REG_FR ) & uap->fr_busy)barrier ();spin_lock_irq (&uap->port.lock);@@ -1412,11 +1472,11 @@ static void pl011_modem_status(struct uart_amba_port *uap)if (delta & UART01x_FR_DCD)uart_handle_dcd_change (&uap->port, status & UART01x_FR_DCD); -if (delta & UART01x_FR_DSR) +if (delta & uap->fr_dsr) uap->port.icount.dsr++; -if (delta & UART01x_FR_CTS) -uart_handle_cts_change (&uap->port, status & UART01x_FR_CTS); +if (delta & uap->fr_cts) +uart_handle_cts_change (&uap->port, status & uap->fr_cts);wake_up_interruptible (&uap->port.state->port.delta_msr_wait); }@@ -1487,7 +1547,7 @@ static unsigned int pl011_tx_empty(struct uart_port *port)struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port);unsigned int status =pl011_readw (uap,REG_FR ); -return status & (UART01x_FR_BUSY|UART01x_FR_TXFF) ?0 :TIOCSER_TEMT ; +return status & (uap->fr_busy|UART01x_FR_TXFF) ?0 :TIOCSER_TEMT ; }static unsigned int pl011_get_mctrl (struct uart_port *port)@@ -1502,9 +1562,9 @@ static unsigned int pl011_get_mctrl(struct uart_port *port) result |= tiocmbitTIOCMBIT (UART01x_FR_DCD,TIOCM_CAR ); -TIOCMBIT (UART01x_FR_DSR,TIOCM_DSR ); -TIOCMBIT (UART01x_FR_CTS,TIOCM_CTS ); -TIOCMBIT (UART011_FR_RI ,TIOCM_RNG ); +TIOCMBIT (uap->fr_dsr,TIOCM_DSR ); +TIOCMBIT (uap->fr_cts,TIOCM_CTS ); +TIOCMBIT (uap->fr_ri,TIOCM_RNG );#undef TIOCMBITreturn result; }@@ -1720,8 +1780,7 @@ static int pl011_startup(struct uart_port *port)/* * initialise the old status of the modem signals */ - uap->old_status =pl011_readw (uap,REG_FR ) & - UART01x_FR_MODEM_ANY; + uap->old_status =pl011_readw (uap,REG_FR ) & UART01x_FR_MODEM_ANY;/* Startup DMA */ pl011_dma_startup (uap);@@ -1800,7 +1859,7 @@ static void pl011_disable_interrupts(struct uart_amba_port *uap)/* mask all interrupts and clear all pending ones */ uap->im =0 ;pl011_writew (uap, uap->im,REG_IMSC ); -pl011_writew (0xffff ,REG_ICR ); +pl011_writew (uap,0xffff ,REG_ICR );spin_unlock_irq (&uap->port.lock); }@@ -2178,7 +2237,7 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)*/ do { status =pl011_readw (uap,REG_FR ); - }while (status & UART01x_FR_BUSY); + }while (status & uap->fr_busy);if (!uap->vendor->always_enabled)pl011_writew (uap, old_cr,REG_CR );@@ -2295,7 +2354,7 @@ static void pl011_putc(struct uart_port *port, int c)while (pl011_readw (uap,REG_FR ) & UART01x_FR_TXFF) ;pl011_writeb (uap, c,REG_DR ); -while (pl011_readw (uap,REG_FR ) & UART01x_FR_BUSY) +while (pl011_readw (uap,REG_FR ) & uap->fr_busy) ; }@@ -2441,6 +2500,7 @@ static int pl011_register_port(struct uart_amba_port *uap)return ret; } +#ifdef CONFIG_ARM_AMBA static int pl011_probe (struct amba_device *dev,const struct amba_id *id) {struct uart_amba_port *uap;@@ -2464,6 +2524,10 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id) uap->reg_lut = vendor->reg_lut; uap->lcrh_rx = vendor->lcrh_rx; uap->lcrh_tx = vendor->lcrh_tx; + uap->fr_busy = vendor->fr_busy; + uap->fr_dsr = vendor->fr_dsr; + uap->fr_cts = vendor->fr_cts; + uap->fr_ri = vendor->fr_ri; uap->fifosize = vendor->get_fifosize (dev); uap->port.irq = dev->irq[0 ]; uap->port.ops = &amba_pl011_pops;@@ -2487,6 +2551,67 @@ static int pl011_remove(struct amba_device *dev)pl011_unregister_port (uap);return 0 ; } +#endif + +#ifdef CONFIG_SOC_ZX296702 +static int zx_uart_probe (struct platform_device *pdev) +{ +struct uart_amba_port *uap; +struct vendor_data *vendor = &vendor_zte; +struct resource *res; +int portnr, ret; + + portnr =pl011_find_free_port (); +if (portnr <0 ) +return portnr; + + uap =devm_kzalloc (&pdev->dev,sizeof (struct uart_amba_port ), +GFP_KERNEL ); +if (!uap) { + ret = -ENOMEM ; + goto out; + } + + uap->clk =devm_clk_get (&pdev->dev,NULL ); +if (IS_ERR (uap->clk)) { + ret =PTR_ERR (uap->clk); + goto out; + } + + uap->vendor = vendor; + uap->reg_lut = vendor->reg_lut; + uap->lcrh_rx = vendor->lcrh_rx; + uap->lcrh_tx = vendor->lcrh_tx; + uap->fr_busy = vendor->fr_busy; + uap->fr_dsr = vendor->fr_dsr; + uap->fr_cts = vendor->fr_cts; + uap->fr_ri = vendor->fr_ri; + uap->fifosize =16 ; + uap->port.irq =platform_get_irq (pdev,0 ); + uap->port.ops = &amba_pl011_pops; + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + + ret =pl011_setup_port (&pdev->dev, uap, res, portnr); +if (ret) +return ret; + +platform_set_drvdata (pdev, uap); + +return pl011_register_port (uap); +out: +return ret; +} + +static int zx_uart_remove (struct platform_device *pdev) +{ +struct uart_amba_port *uap =platform_get_drvdata (pdev); + +uart_remove_one_port (&amba_reg, &uap->port); +pl011_unregister_port (uap); +return 0 ; +} +#endif #ifdef CONFIG_PM_SLEEP static int pl011_suspend (struct device *dev)@@ -2544,6 +2669,10 @@ static int sbsa_uart_probe(struct platform_device *pdev) uap->vendor = &vendor_sbsa; uap->reg_lut = vendor_sbsa.reg_lut; + uap->fr_busy = vendor_sbsa.fr_busy; + uap->fr_dsr = vendor_sbsa.fr_dsr; + uap->fr_cts = vendor_sbsa.fr_cts; + uap->fr_ri = vendor_sbsa.fr_ri; uap->fifosize =32 ; uap->port.irq =platform_get_irq (pdev,0 ); uap->port.ops = &sbsa_uart_pops;@@ -2593,6 +2722,7 @@ static struct platform_driver arm_sbsa_uart_platform_driver = { }, }; +#ifdef CONFIG_ARM_AMBA static struct amba_id pl011_ids[] = { { .id = 0x00041011,@@ -2618,20 +2748,57 @@ static struct amba_driver pl011_driver = { .probe = pl011_probe, .remove = pl011_remove, }; +#endif + +#ifdef CONFIG_SOC_ZX296702 +static const struct of_device_id zx_uart_dt_ids[] = { + { .compatible ="zte,zx296702-uart" , }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, zx_uart_dt_ids); + +static struct platform_driver zx_uart_driver = { + .driver = { + .name ="zx-uart" , + .owner =THIS_MODULE , + .pm = &pl011_dev_pm_ops, + .of_match_table = zx_uart_dt_ids, + }, + .probe = zx_uart_probe, + .remove = zx_uart_remove, +}; +#endif +static int __init pl011_init (void ) { +int ret;printk (KERN_INFO "Serial: AMBA PL011 UART driver\n" );if (platform_driver_register (&arm_sbsa_uart_platform_driver))pr_warn ("could not register SBSA UART platform driver\n" ); -return amba_driver_register (&pl011_driver); + +#ifdef CONFIG_SOC_ZX296702 + ret =platform_driver_register (&zx_uart_driver); +if (ret) +pr_warn ("could not register ZX UART platform driver\n" ); +#endif + +#ifdef CONFIG_ARM_AMBA + ret =amba_driver_register (&pl011_driver); +#endif +return ret; } static void __exit pl011_exit(void) {platform_driver_unregister (&arm_sbsa_uart_platform_driver); +#ifdef CONFIG_SOC_ZX296702 +platform_driver_unregister (&zx_uart_driver); +#endif +#ifdef CONFIG_ARM_AMBA amba_driver_unregister (&pl011_driver); +#endif }/* include/linux/amba/serial.h +14
-1@@ -33,12 +33,14 @@#define UART01x_DR 0x00/* Data read or written from the interface. */ #define UART01x_RSR 0x04/* Receive status register (Read). */ #define UART01x_ECR 0x04/* Error clear register (Write). */ +#define ZX_UART01x_DR 0x04/* Data read or written from the interface. */ #define UART010_LCRH 0x08/* Line control register, high byte. */ #define ST_UART011_DMAWM 0x08/* DMA watermark configure register. */ #define UART010_LCRM 0x0C/* Line control register, middle byte. */ #define ST_UART011_TIMEOUT 0x0C/* Timeout period register. */ #define UART010_LCRL 0x10/* Line control register, low byte. */ #define UART010_CR 0x14/* Control register. */ +#define ZX_UART01x_FR 0x14/* Flag register (Read only). */ #define UART01x_FR 0x18/* Flag register (Read only). */ #define UART010_IIR 0x1C/* Interrupt identification register (Read). */ #define UART010_ICR 0x1C/* Interrupt clear register (Write). */ @@ -49,13 +51,21 @@#define UART011_LCRH 0x2c/* Line control register. */ #define ST_UART011_LCRH_TX 0x2c/* Tx Line control register. */ #define UART011_CR 0x30/* Control register. */ +#define ZX_UART011_LCRH_TX 0x30/* Tx Line control register. */ #define UART011_IFLS 0x34/* Interrupt fifo level select. */ +#define ZX_UART011_CR 0x34/* Control register. */ +#define ZX_UART011_IFLS 0x38/* Interrupt fifo level select. */ #define UART011_IMSC 0x38/* Interrupt mask. */ #define UART011_RIS 0x3c/* Raw interrupt status. */ #define UART011_MIS 0x40/* Masked interrupt status. */ +#define ZX_UART011_IMSC 0x40/* Interrupt mask. */ #define UART011_ICR 0x44/* Interrupt clear register. */ +#define ZX_UART011_RIS 0x44/* Raw interrupt status. */ #define UART011_DMACR 0x48/* DMA control register. */ +#define ZX_UART011_MIS 0x48/* Masked interrupt status. */ +#define ZX_UART011_ICR 0x4c/* Interrupt clear register. */ #define ST_UART011_XFCR 0x50/* XON/XOFF control register. */ +#define ZX_UART011_DMACR 0x50/* DMA control register. */ #define ST_UART011_XON1 0x54/* XON1 register. */ #define ST_UART011_XON2 0x58/* XON2 register. */ #define ST_UART011_XOFF1 0x5C/* XON1 register. */ @@ -75,15 +85,19 @@#define UART01x_RSR_PE 0x02#define UART01x_RSR_FE 0x01 +#define ZX_UART01x_FR_BUSY 0x300#define UART011_FR_RI 0x100#define UART011_FR_TXFE 0x080#define UART011_FR_RXFF 0x040#define UART01x_FR_TXFF 0x020#define UART01x_FR_RXFE 0x010#define UART01x_FR_BUSY 0x008 +#define ZX_UART01x_FR_DSR 0x008#define UART01x_FR_DCD 0x004#define UART01x_FR_DSR 0x002 +#define ZX_UART01x_FR_CTS 0x002#define UART01x_FR_CTS 0x001 +#define ZX_UART011_FR_RI 0x001#define UART01x_FR_TMSK (UART01x_FR_TXFF + UART01x_FR_BUSY)#define UART011_CR_CTSEN 0x8000/* CTS hardware flow control */ --1.9.1
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
Quoted 4 lines · Jun Nie
I am not sure the silicon designer's plan on the UART IP. Different silicon team have different decision on the IP choice. So I guess future ZTE SOC may reuse ZTE UART per people assignment.
I would rather see this as a separate driver. I think this patch pollutes the amba-pl011.c driver too much.
Signature · 3 lines
Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
2015-10-24 5:54 GMT+08:00 Timur Tabi <[email protected]>:
Quoted 8 lines · Jun Nie
Quoted 4 lines
I am not sure the silicon designer's plan on the UART IP. Different silicon team have different decision on the IP choice. So I guess future ZTE SOC may reuse ZTE UART per people assignment.
I would rather see this as a separate driver. I think this patch pollutes the
amba-pl011.cdriver too much.
I am OK to add a new driver for ZTE UART if no other's objection though I do not agree you. Without this last patch, other patches are just for minor refactor for register access.
Comment from any other is welcome.
Jun
Quoted 5 lines
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
Quoted 3 lines · Jun Nie
I am OK to add a new driver for ZTE UART if no other's objection though I do not agree you. Without this last patch, other patches are just for minor refactor for register access.
In that case, my vote is to drop the whole patchset. Just copy/paste the amba-pl011 driver and make whatever changes you need. That sort of thing happens all the time.
Signature · 3 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
2015-10-24 11:32 GMT+08:00 Timur Tabi <[email protected]>:
Quoted 10 lines
Quoted 4 lines · Jun Nie
I am OK to add a new driver for ZTE UART if no other's objection though I do not agree you. Without this last patch, other patches are just for minor refactor for register access.
In that case, my vote is to drop the whole patchset. Just copy/paste the amba-pl011 driver and make whatever changes you need. That sort of thing happens all the time.
Greg, Peter & Russel,
What's your opinion on this separate UART driver suggestion?
Jun
Quoted 5 lines
--
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
Quoted 8 lines · Jun Nie
2015-10-24 11:32 GMT+08:00 Timur Tabi <[email protected]>:
Quoted 7 lines
Quoted 4 lines · Jun Nie
I am OK to add a new driver for ZTE UART if no other's objection though I do not agree you. Without this last patch, other patches are just for minor refactor for register access.
Quoted 7 lines
Quoted 3 lines
In that case, my vote is to drop the whole patchset. Just copy/paste the amba-pl011 driver and make whatever changes you need. That sort of thing happens all the time.
Greg, Peter & Russell,
What's your opinion on this separate UART driver suggestion?
That's crazy; fork the driver simply because the register i/o addresses are different? No.
On the contrary, this patchset directly triggered important improvements to both this driver and the ARM arch.
Regards,
Peter Hurley
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
In reply to Timur TabiHi,
Quoted 8 lines · Timur Tabi
Quoted 3 lines · Jun Nie
I am OK to add a new driver for ZTE UART if no other's objection though I do not agree you. Without this last patch, other patches are just for minor refactor for register access.
In that case, my vote is to drop the whole patchset. Just copy/paste the amba-pl011 driver and make whatever changes you need. That sort of thing happens all the time.
I don't agree here. The driver _is_ very similar (all the semantics and behaviour), it's just the register addresses that are different. In so far I'd go with that approach of supporting it within the PL011 driver - whether this has to be in this very same file is another question. I think by using register accessors (avoiding direct calls to writel/readl in the code) we can hide a lot of these differences in just two functions.
Also, as I said earlier, I guess we don't need all those #ifdefs. So in the end the driver wouldn't be too different, it's just that those diffs that look a bit scary (because they touch every readl/writel).
I tried to refactor the driver lately to split up SBSA and PL011 support and got something that compiles, though I wasn't fully satisfied and I ran out of time. The refactor idea was to split driver runtime from initialization, so the different probe and init functions can be moved into separate files. There would be one stub file with all the core driver logic (DMA, IRQ handling, buffer handling, communication parameters setup) and one file for each subtype (PL011, SBSA, ZTE, you name it).
If people are interested, I can try to clean this up and post it as an RFC.
Cheers,
Andre.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
Quoted 9 lines · Andre Przywara
I tried to refactor the driver lately to split up SBSA and PL011 support and got something that compiles, though I wasn't fully satisfied and I ran out of time. The refactor idea was to split driver runtime from initialization, so the different probe and init functions can be moved into separate files. There would be one stub file with all the core driver logic (DMA, IRQ handling, buffer handling, communication parameters setup) and one file for each subtype (PL011, SBSA, ZTE, you name it).
If people are interested, I can try to clean this up and post it as an RFC.
I am interested. We need support for subtype 13, because our hardware only supports 32-bit access to all registers. We have an internal patch that replaces all of the read/write routines with vendor function calls.
I would need to refactor our patch on top of yours.
Signature · 3 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
Hi Timur,
Quoted 12 lines · Timur Tabi
Quoted 10 lines · Andre Przywara
I tried to refactor the driver lately to split up SBSA and PL011 support and got something that compiles, though I wasn't fully satisfied and I ran out of time. The refactor idea was to split driver runtime from initialization, so the different probe and init functions can be moved into separate files. There would be one stub file with all the core driver logic (DMA, IRQ handling, buffer handling, communication parameters setup) and one file for each subtype (PL011, SBSA, ZTE, you name it).
If people are interested, I can try to clean this up and post it as an RFC.
Quoted 2 lines
I am interested. We need support for subtype 13, because our hardware only supports 32-bit access to all registers.
Yeah, I was interested in that scenario too, because the SBSA spec actually speaks of 32-bit registers and vendors may implement it strictly as that. Still waiting for actual failure reports on this before I wanted to push a fix, though.
Quoted 3 lines
We have an internal patch
that replaces all of the read/write routines with vendor function calls.
I would need to refactor our patch on top of yours.
But wouldn't Jun's patch address this more easily, because it wraps every call already? TBH I found this change the most interesting.
I will prepare something this week.
Cheers,
Andre.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
Quoted 4 lines · Andre Przywara
Yeah, I was interested in that scenario too, because the SBSA spec actually speaks of 32-bit registers and vendors may implement it strictly as that. Still waiting for actual failure reports on this before I wanted to push a fix, though.
What do you mean by failure reports? Our hardware generates an SError if you try to access the PL011 registers with 8-bit or 16-bit reads or writes.
Quoted 6 lines
Quoted 3 lines
We have an internal patch
that replaces all of the read/write routines with vendor function calls.
I would need to refactor our patch on top of yours.But wouldn't Jun's patch address this more easily, because it wraps every call already? TBH I found this change the most interesting.
Yes, but I think it changes a lot of things unnecessarily, like the register names.
Signature · 3 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
Hi,
Quoted 10 lines · Timur Tabi
Quoted 4 lines · Andre Przywara
Yeah, I was interested in that scenario too, because the SBSA spec actually speaks of 32-bit registers and vendors may implement it strictly as that. Still waiting for actual failure reports on this before I wanted to push a fix, though.
What do you mean by failure reports? Our hardware generates an SError if you try to access the PL011 registers with 8-bit or 16-bit reads or writes.
I meant that if some hardware does not work with an upstream kernel, I'd expect some kind of failure report or a patch on the public mailing list. I may have missed it, but I couldn't find anything on the list so far.
As mentioned earlier, I didn't want to change code without good reasons, that's why I was waiting for people to scream.
So I just take this as a scream, then ;-)
Can you elaborate on that? Is your UART a PL011 one (using the arm,pl011 DT binding or AMBA ID registers) or are you using the SBSA subset only? Is there some means to identify this particular UART?
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
We have an internal patch
that replaces all of the read/write routines with vendor function calls.
I would need to refactor our patch on top of yours.But wouldn't Jun's patch address this more easily, because it wraps every call already? TBH I found this change the most interesting.
Yes, but I think it changes a lot of things unnecessarily, like the register names.
Which is unfortunate, but cannot be changed anymore. And as much as I dislike this myself, I guess we cannot ignore this hardware just because it doesn't go easily with our driver code. So instead of having just another driver which is strikingly similar, I'd rather have this in the one-and-only PL011 driver which is much less subject to bit-rot.
So my idea here was to split Jun's series into introducing the readl/writel wrappers first, and adding the register address mangling on top of that. Given that those changed register addresses seem to be a mishap to me, we could just get away with a ZTE specific translate function, which takes a PL011 register number and returns the actual register offset to use. That isn't very generic, but would hide this ugliness without spoiling the whole driver.
So both you and me could benefit from the wrapper functions already, while Jun has some patches less to care about.
Cheers,
Andre.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
Quoted 3 lines · Andre Przywara
I meant that if some hardware does not work with an upstream kernel, I'd expect some kind of failure report or a patch on the public mailing list. I may have missed it, but I couldn't find anything on the list so far.
We have an internal patch that I'm trying to upstream, but I need the amba-pl011 driver to stabilize first.
Quoted 9 lines
As mentioned earlier, I didn't want to change code without good reasons, that's why I was waiting for people to scream.
So I just take this as a scream, then;-)
Can you elaborate on that? Is your UART a PL011 one (using the arm,pl011 DT binding or AMBA ID registers) or are you using the SBSA subset only? Is there some means to identify this particular UART?
We use ACPI bindings. It's our ARM64 server-class SOC. We use the ACPI subtype 13 to identify it.
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Yes, but I think it changes a lot of things unnecessarily, like the register names.
Which is unfortunate, but cannot be changed anymore. And as much as I dislike this myself, I guess we cannot ignore this hardware just because it doesn't go easily with our driver code. So instead of having just another driver which is strikingly similar, I'd rather have this in the one-and-only PL011 driver which is much less subject to bit-rot.
I guess we'll have to agree to disagree. I'd rather have the other driver subjected to bit rot. I don't understand why Jun's obscure hardware is so great that it needs to complicate things for all the other ARM vendors.
Quoted 7 lines
So my idea here was to split Jun's series into introducing the readl/writel wrappers first, and adding the register address mangling on top of that. Given that those changed register addresses seem to be a mishap to me, we could just get away with a ZTE specific translate function, which takes a PL011 register number and returns the actual register offset to use. That isn't very generic, but would hide this ugliness without spoiling the whole driver.
I'll have to see the code to form an opinion.
Quoted 2 lines
So both you and me could benefit from the wrapper functions already, while Jun has some patches less to care about.
Then I suggest that we focus on the wrapper functions now, and then let Jun add support for his stuff later.
Signature · 3 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
Hi Timur,
Quoted 8 lines · Timur Tabi
Quoted 4 lines · Andre Przywara
I meant that if some hardware does not work with an upstream kernel, I'd expect some kind of failure report or a patch on the public mailing list. I may have missed it, but I couldn't find anything on the list so far.
We have an internal patch that I'm trying to upstream, but I need the amba-pl011 driver to stabilize first.
As mentioned earlier, this looks like the perfect opportunity now to get the change in (since I see it as a subset of Jun's series). "Waiting for stabilization" sounds a bit optimistic to me in a general Linux context ;-)
Quoted 6 lines
Quoted 5 lines
As mentioned earlier, I didn't want to change code without good reasons, that's why I was waiting for people to scream.
So I just take this as a scream, then;-)
Quoted 6 lines
Quoted 3 lines
Can you elaborate on that? Is your UART a PL011 one (using the arm,pl011 DT binding or AMBA ID registers) or are you using the SBSA subset only? Is there some means to identify this particular UART?
We use ACPI bindings. It's our ARM64 server-class SOC. We use the ACPI subtype 13 to identify it.
Does "subtype 13" refer to the UUID field here? So instead of using UUID 0 you have some vendor specific field set in your ARMH0011 table? Is that "just made up" or specified somewhere so that there will be no other usage of that number later?
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Yes, but I think it changes a lot of things unnecessarily, like the register names.
Which is unfortunate, but cannot be changed anymore. And as much as I dislike this myself, I guess we cannot ignore this hardware just because it doesn't go easily with our driver code. So instead of having just another driver which is strikingly similar, I'd rather have this in the one-and-only PL011 driver which is much less subject to bit-rot.
Quoted 4 lines
I guess we'll have to agree to disagree. I'd rather have the other driver subjected to bit rot. I don't understand why Jun's obscure hardware is so great that it needs to complicate things for all the other ARM vendors.
I am not judging on greatness here, it's just that Jun went through the proper upstream process, so we considered this change. In the end the kernel is there to support hardware out there and not to match the driver architecture pipe dreams of some computer scientists ;-)
But in fact we did not merge the series (at least not in a release kernel), but send it back to the design stage. And it looks like we are on a good track to not spoil the driver unnecessarily ...
Quoted 8 lines
Quoted 7 lines
So my idea here was to split Jun's series into introducing the readl/writel wrappers first, and adding the register address mangling on top of that. Given that those changed register addresses seem to be a mishap to me, we could just get away with a ZTE specific translate function, which takes a PL011 register number and returns the actual register offset to use. That isn't very generic, but would hide this ugliness without spoiling the whole driver.
Quoted 7 lines
I'll have to see the code to form an opinion.
Quoted 2 lines
So both you and me could benefit from the wrapper functions already, while Jun has some patches less to care about.
Then I suggest that we focus on the wrapper functions now, and then let Jun add support for his stuff later.
Yes, that was my idea.
Now with a specific use case for the wrapper functions, we can push this change first.
Cheers,
Andre.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
Quoted 6 lines · Andre Przywara
Quoted 2 lines
We have an internal patch that I'm trying to upstream, but I need the amba-pl011 driver to stabilize first.
As mentioned earlier, this looks like the perfect opportunity now to get the change in (since I see it as a subset of Jun's series). "Waiting for stabilization" sounds a bit optimistic to me in a general Linux context ;-)
I will post it today.
Quoted 7 lines
Quoted 2 lines
We use ACPI bindings. It's our ARM64 server-class SOC. We use the ACPI subtype 13 to identify it.
Does "subtype 13" refer to the UUID field here? So instead of using UUID 0 you have some vendor specific field set in your ARMH0011 table? Is that "just made up" or specified somewhere so that there will be no other usage of that number later?
It's part of the ARMH0011 table. There are already a couple subtypes, but a bunch more were added. It's table 3 in this document:
http://go.microsoft.com/fwlink/p/?linkid=403551
Notice how the new subtype is already deprecated. That's because it's technically a bug for hardware to require 32-bit accesses.
PATCH v13 5/5 uart: pl011: Add support to ZTE ZX296702 uart
In reply to Andre PrzywaraQuoted 6 lines · Andre Przywara
Quoted 2 lines
Quoted 2 lines
We have an internal patch that I'm trying to upstream, but I need the amba-pl011 driver to stabilize first.
As mentioned earlier, this looks like the perfect opportunity now to get the change in (since I see it as a subset of Jun's series). "Waiting for stabilization" sounds a bit optimistic to me in a general Linux context;-)
Unfortunately, I must depend on Leif Lindholm's SPCR patches. He's working on a new version, but until then I can't do much. They should be out soon, though.