| Message ID | 20200709161208.223814-1-yamada.masahiro@socionext.com |
|---|---|
| State | Accepted |
| Commit | 70434ab9de795b205608067d9a5e067f30ad3629 |
| Headers | show |
| Series | [1/3] serial: uniphier: use register macros instead of structure | expand |
On Fri, Jul 10, 2020 at 1:13 AM Masahiro Yamada <yamada.masahiro at 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 at 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 >
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; }
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 at socionext.com> --- drivers/serial/serial_uniphier.c | 75 ++++++++++++++------------------ 1 file changed, 32 insertions(+), 43 deletions(-)