[PATCH v13 2/5] uart: pl011: Introduce register accessor

[PATCH v13 2/5] uart: pl011: Introduce register accessor

[Andre Przywara]

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.

On 31/07/15 08:49, Jun Nie wrote:
> 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 ...

> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/serial/amba-pl011.c | 263 +++++++++++++++++++++-------------------
>  1 file changed, 141 insertions(+), 122 deletions(-)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index ee57e2b..29a291d 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -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)
> +{
> +       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.

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

>  /*
>   * 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 =
> +           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.

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

[Jun Nie]

2015-09-18 18:51 GMT+08:00 Andre Przywara <andre.przywara@arm.com>:
> 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.

>
> On 31/07/15 08:49, Jun Nie wrote:
>> 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?

>
> More comments inline ...
>
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>  drivers/tty/serial/amba-pl011.c | 263 +++++++++++++++++++++-------------------
>>  1 file changed, 141 insertions(+), 122 deletions(-)
>>
>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>> index ee57e2b..29a291d 100644
>> --- a/drivers/tty/serial/amba-pl011.c
>> +++ b/drivers/tty/serial/amba-pl011.c
>> @@ -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)
>> +{
>> +       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.

OK, will change to 32-bit access in future version.
>
>> +
>> +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.

>
>>  /*
>>   * 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 =
>> +           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.

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.

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

> Cheers,
> Andre.
>
>>
>> @@ -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

[Andre Przywara]

Hi Jun,

thanks for your reply. Just a quick one below...

...
>>> @@ -203,6 +206,24 @@ struct uart_amba_port {
>>>  #endif
>>>  };
>>>
>>> +static unsigned int pl011_readw(struct uart_amba_port *uap, int index)
>>> +{
>>> +       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.
> 
> 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

[Timur Tabi]

On Fri, Sep 18, 2015 at 5:51 AM, Andre Przywara <andre.przywara@arm.com> wrote:
>
>>  static void pl011_putc(struct uart_port *port, int c)
>>  {
>> -       while (readl(port->membase + REG_FR) & UART01x_FR_TXFF)
>> +       struct uart_amba_port *uap =
>> +           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.

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


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

[Peter Hurley]

On 10/22/2015 07:36 PM, Timur Tabi wrote:
> On Fri, Sep 18, 2015 at 5:51 AM, Andre Przywara <andre.przywara@arm.com> wrote:
>>
>>>  static void pl011_putc(struct uart_port *port, int c)
>>>  {
>>> -       while (readl(port->membase + REG_FR) & UART01x_FR_TXFF)
>>> +       struct uart_amba_port *uap =
>>> +           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.
> 
> 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;
> }


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

[Timur Tabi]

On 10/28/2015 09:22 AM, Peter Hurley wrote:
> 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.

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/




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

[Peter Hurley]

On 10/28/2015 10:51 AM, Timur Tabi wrote:
> On 10/28/2015 09:22 AM, Peter Hurley wrote:
>> 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.
> 
> 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.

> ACPI is much more difficult, and that's why I used the command-line approach.

Understood.

> Anyway, I found this alternate patch from Lief Lindholm that implements this idea better, although it needs work:
> 
> https://patches.linaro.org/53265/

Yeah, I saw those, but I'm waiting for v2 to review.

Regards,
Peter Hurley

[PATCH v13 4/5] uart: pl011: Improve LCRH register access decision

[Andre Przywara]

Hi Jun,

On 31/07/15 08:49, Jun Nie wrote:
> 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 <jun.nie@linaro.org>
> ---
>  drivers/tty/serial/amba-pl011.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index e1f3bd5..017443d 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -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.

>  static 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);
>  }
>  
> 

[PATCH v13 5/5] uart: pl011: Add support to ZTE ZX296702 uart

[Andre Przywara]

Hi Jun,

On 31/07/15 08:49, Jun Nie wrote:
> 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 <jun.nie@linaro.org>
> ---
>  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(-)
> 
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 15b4079..2103765 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -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.
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 017443d..2af09ab 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -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;
> @@ -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?

>  static 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,
> @@ -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.

> +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 |= tiocmbit
> 
>         TIOCMBIT(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 TIOCMBIT
>         return 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
>  }
> 
>  /*
> diff --git a/include/linux/amba/serial.h b/include/linux/amba/serial.h
> index 0ddb5c0..6a0a89e 100644
> --- a/include/linux/amba/serial.h
> +++ b/include/linux/amba/serial.h
> @@ -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

[Russell King - ARM Linux]

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.

On Fri, Sep 18, 2015 at 02:50:26PM +0100, Andre Przywara wrote:
> Hi Jun,
> 
> On 31/07/15 08:49, Jun Nie wrote:
> > 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 <jun.nie@linaro.org>
> > ---
> >  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(-)
> > 
> > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> > index 15b4079..2103765 100644
> > --- a/drivers/tty/serial/Kconfig
> > +++ b/drivers/tty/serial/Kconfig
> > @@ -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.
> > 
> > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > index 017443d..2af09ab 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -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;
> > @@ -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?
> 
> >  static 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,
> > @@ -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.
> 
> > +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 |= tiocmbit
> > 
> >         TIOCMBIT(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 TIOCMBIT
> >         return 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
> >  }
> > 
> >  /*
> > diff --git a/include/linux/amba/serial.h b/include/linux/amba/serial.h
> > index 0ddb5c0..6a0a89e 100644
> > --- a/include/linux/amba/serial.h
> > +++ b/include/linux/amba/serial.h
> > @@ -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
> > 

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

[Jun Nie]

2015-09-18 21:59 GMT+08:00 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> 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

> On Fri, Sep 18, 2015 at 02:50:26PM +0100, Andre Przywara wrote:
>> Hi Jun,
>>
>> On 31/07/15 08:49, Jun Nie wrote:
>> > 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 <jun.nie@linaro.org>
>> > ---
>> >  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(-)
>> >
>> > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>> > index 15b4079..2103765 100644
>> > --- a/drivers/tty/serial/Kconfig
>> > +++ b/drivers/tty/serial/Kconfig
>> > @@ -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.
>> >
>> > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>> > index 017443d..2af09ab 100644
>> > --- a/drivers/tty/serial/amba-pl011.c
>> > +++ b/drivers/tty/serial/amba-pl011.c
>> > @@ -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;
>> > @@ -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?
>>
>> >  static 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,
>> > @@ -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.
>>
>> > +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 |= tiocmbit
>> >
>> >         TIOCMBIT(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 TIOCMBIT
>> >         return 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
>> >  }
>> >
>> >  /*
>> > diff --git a/include/linux/amba/serial.h b/include/linux/amba/serial.h
>> > index 0ddb5c0..6a0a89e 100644
>> > --- a/include/linux/amba/serial.h
>> > +++ b/include/linux/amba/serial.h
>> > @@ -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
>> >
>
> --
> 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

[Jun Nie]

2015-09-18 21:50 GMT+08:00 Andre Przywara <andre.przywara@arm.com>:
> Hi Jun,
>
> On 31/07/15 08:49, Jun Nie wrote:
>> 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 <jun.nie@linaro.org>
>> ---
>>  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(-)
>>
>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>> index 15b4079..2103765 100644
>> --- a/drivers/tty/serial/Kconfig
>> +++ b/drivers/tty/serial/Kconfig
>> @@ -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.
>>
>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>> index 017443d..2af09ab 100644
>> --- a/drivers/tty/serial/amba-pl011.c
>> +++ b/drivers/tty/serial/amba-pl011.c
>> @@ -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;
>> @@ -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?
>
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

>>  static 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,
>> @@ -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.
>
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.


>> +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 |= tiocmbit
>>
>>         TIOCMBIT(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 TIOCMBIT
>>         return 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
>>  }
>>
>>  /*
>> diff --git a/include/linux/amba/serial.h b/include/linux/amba/serial.h
>> index 0ddb5c0..6a0a89e 100644
>> --- a/include/linux/amba/serial.h
>> +++ b/include/linux/amba/serial.h
>> @@ -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

[Timur Tabi]

On Sat, Sep 19, 2015 at 1:54 AM, Jun Nie <jun.nie@linaro.org> wrote:
>
> 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.

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

[Jun Nie]

2015-10-24 5:54 GMT+08:00 Timur Tabi <timur@codeaurora.org>:
> On Sat, Sep 19, 2015 at 1:54 AM, Jun Nie <jun.nie@linaro.org> wrote:
>>
>> 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.

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

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

[Timur Tabi]

Jun Nie wrote:
> 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.

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

[Jun Nie]

2015-10-24 11:32 GMT+08:00 Timur Tabi <timur@codeaurora.org>:
> Jun Nie wrote:
>>
>> 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

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

[Peter Hurley]

On 10/25/2015 09:27 PM, Jun Nie wrote:
> 2015-10-24 11:32 GMT+08:00 Timur Tabi <timur@codeaurora.org>:
>> Jun Nie wrote:
>>>
>>> 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 & 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

[Andre Przywara]

Hi,

On 24/10/15 04:32, Timur Tabi wrote:
> Jun Nie wrote:
>> 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

[Timur Tabi]

Andre Przywara wrote:
> 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.

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

[Andre Przywara]

Hi Timur,

On 26/10/15 12:46, Timur Tabi wrote:
> Andre Przywara wrote:
>> 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. 

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.

> 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

[Timur Tabi]

Andre Przywara wrote:

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

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

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

[Andre Przywara]

Hi,

On 26/10/15 14:07, Timur Tabi wrote:
> Andre Przywara wrote:
> 
>> 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?

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

[Timur Tabi]

Andre Przywara wrote:
> 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, 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.

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

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

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

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

[Andre Przywara]

Hi Timur,

On 26/10/15 14:47, Timur Tabi wrote:
> Andre Przywara wrote:
>> 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 ;-)

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

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?

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

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

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

[Timur Tabi]

On 10/26/2015 10:19 AM, Andre Przywara wrote:

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

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

[Timur Tabi]

On 10/26/2015 10:19 AM, Andre Przywara wrote:
>> >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.