Re: [PATCH 1/3] serial: uniphier: use register macros instead of structure

On Fri, Jul 10, 2020 at 1:13 AM Masahiro Yamada yamada.masahiro@socionext.com wrote:

After all, I am not a big fan of using a structure to represent the hardware register map.

You do not need to know the entire register map.

Add only necessary register macros.

Use FIELD_PREP() instead of maintaining a pair of shift and mask.

Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com

Series, applied to u-boot-uniphier.

drivers/serial/serial_uniphier.c | 75 ++++++++++++++------------------ 1 file changed, 32 insertions(+), 43 deletions(-)

diff --git a/drivers/serial/serial_uniphier.c b/drivers/serial/serial_uniphier.c index c7f46e5598..2ffab004bd 100644 --- a/drivers/serial/serial_uniphier.c +++ b/drivers/serial/serial_uniphier.c @@ -7,6 +7,8 @@

#include <common.h> #include <dm.h> +#include <linux/bitfield.h> +#include <linux/bitops.h> #include <linux/bug.h> #include <linux/io.h> #include <linux/serial_reg.h> @@ -15,77 +17,67 @@ #include <serial.h> #include <fdtdec.h>

-/*

• • Note: Register map is slightly different from that of 16550.
• */

-struct uniphier_serial {

•   u32 rx;                 /* In:  Receive buffer */
  

-#define tx rx /* Out: Transmit buffer */

•   u32 ier;                /* Interrupt Enable Register */
  

•   u32 iir;                /* In: Interrupt ID Register */
  

•   u32 char_fcr;           /* Charactor / FIFO Control Register */
  

•   u32 lcr_mcr;            /* Line/Modem Control Register */
  

-#define LCR_SHIFT 8 -#define LCR_MASK (0xff << (LCR_SHIFT))

•   u32 lsr;                /* In: Line Status Register */
  

•   u32 msr;                /* In: Modem Status Register */
  

•   u32 __rsv0;
  

•   u32 __rsv1;
  

•   u32 dlr;                /* Divisor Latch Register */
  

-}; +#define UNIPHIER_UART_REGSHIFT 2

+#define UNIPHIER_UART_RX (0 << (UNIPHIER_UART_REGSHIFT)) +#define UNIPHIER_UART_TX UNIPHIER_UART_RX +/* bit[15:8] = CHAR, bit[7:0] = FCR */ +#define UNIPHIER_UART_CHAR_FCR (3 << (UNIPHIER_UART_REGSHIFT)) +/* bit[15:8] = LCR, bit[7:0] = MCR */ +#define UNIPHIER_UART_LCR_MCR (4 << (UNIPHIER_UART_REGSHIFT)) +#define UNIPHIER_UART_LCR_MASK GENMASK(15, 8) +#define UNIPHIER_UART_LSR (5 << (UNIPHIER_UART_REGSHIFT)) +/* Divisor Latch Register */ +#define UNIPHIER_UART_DLR (9 << (UNIPHIER_UART_REGSHIFT))

struct uniphier_serial_priv {

•   struct uniphier_serial __iomem *membase;
  

•   void __iomem *membase;
    unsigned int uartclk;
  

};

-#define uniphier_serial_port(dev) \

•   ((struct uniphier_serial_priv *)dev_get_priv(dev))->membase
  

static int uniphier_serial_setbrg(struct udevice *dev, int baudrate) { struct uniphier_serial_priv *priv = dev_get_priv(dev);

•   struct uniphier_serial __iomem *port = uniphier_serial_port(dev);
  

•   const unsigned int mode_x_div = 16;
  

•   static const unsigned int mode_x_div = 16;
    unsigned int divisor;
  
    divisor = DIV_ROUND_CLOSEST(priv->uartclk, mode_x_div * baudrate);
  
  

•   writel(divisor, &port->dlr);
  

•   writel(divisor, priv->membase + UNIPHIER_UART_DLR);
  
    return 0;
  

}

static int uniphier_serial_getc(struct udevice *dev) {

•   struct uniphier_serial __iomem *port = uniphier_serial_port(dev);
  

•   struct uniphier_serial_priv *priv = dev_get_priv(dev);
  
  

•   if (!(readl(&port->lsr) & UART_LSR_DR))
  

•   if (!(readl(priv->membase + UNIPHIER_UART_LSR) & UART_LSR_DR))
            return -EAGAIN;
  
  

•   return readl(&port->rx);
  

•   return readl(priv->membase + UNIPHIER_UART_RX);
  

}

static int uniphier_serial_putc(struct udevice *dev, const char c) {

•   struct uniphier_serial __iomem *port = uniphier_serial_port(dev);
  

•   struct uniphier_serial_priv *priv = dev_get_priv(dev);
  
  

•   if (!(readl(&port->lsr) & UART_LSR_THRE))
  

•   if (!(readl(priv->membase + UNIPHIER_UART_LSR) & UART_LSR_THRE))
            return -EAGAIN;
  
  

•   writel(c, &port->tx);
  

•   writel(c, priv->membase + UNIPHIER_UART_TX);
  
    return 0;
  

}

static int uniphier_serial_pending(struct udevice *dev, bool input) {

•   struct uniphier_serial __iomem *port = uniphier_serial_port(dev);
  

•   struct uniphier_serial_priv *priv = dev_get_priv(dev);
  
    if (input)
  

•           return readl(&port->lsr) & UART_LSR_DR;
  

•           return readl(priv->membase + UNIPHIER_UART_LSR) & UART_LSR_DR;
    else
  

•           return !(readl(&port->lsr) & UART_LSR_THRE);
  

•           return !(readl(priv->membase + UNIPHIER_UART_LSR) & UART_LSR_THRE);
  

}

/* @@ -113,7 +105,6 @@ static const struct uniphier_serial_clk_data uniphier_serial_clk_data[] = { static int uniphier_serial_probe(struct udevice *dev) { struct uniphier_serial_priv *priv = dev_get_priv(dev);

•   struct uniphier_serial __iomem *port;
    const struct uniphier_serial_clk_data *clk_data;
    ofnode root_node;
    fdt_addr_t base;
  

@@ -123,12 +114,10 @@ static int uniphier_serial_probe(struct udevice *dev) if (base == FDT_ADDR_T_NONE) return -EINVAL;

•   port = devm_ioremap(dev, base, SZ_64);
  

•   if (!port)
  

•   priv->membase = devm_ioremap(dev, base, SZ_64);
  

•   if (!priv->membase)
            return -ENOMEM;
  
  

•   priv->membase = port;
  

•   root_node = ofnode_path("/");
    clk_data = uniphier_serial_clk_data;
    while (clk_data->compatible) {
  

@@ -143,10 +132,10 @@ static int uniphier_serial_probe(struct udevice *dev)

    priv->uartclk = clk_data->clk_rate;

•   tmp = readl(&port->lcr_mcr);
  

•   tmp &= ~LCR_MASK;
  

•   tmp |= UART_LCR_WLEN8 << LCR_SHIFT;
  

•   writel(tmp, &port->lcr_mcr);
  

•   tmp = readl(priv->membase + UNIPHIER_UART_LCR_MCR);
  

•   tmp &= ~UNIPHIER_UART_LCR_MASK;
  

•   tmp |= FIELD_PREP(UNIPHIER_UART_LCR_MASK, UART_LCR_WLEN8);
  

•   writel(tmp, priv->membase + UNIPHIER_UART_LCR_MCR);
  
    return 0;
  

}

2.25.1