[Xen-devel] [RFC PATCH 2/2] xen/arm: Add MESON UART driver for Amlogic S905 SoC

[André Przywara <andre.przywara@xxxxxxx>]

On 8/7/18 6:07 PM, Amit Singh Tomar wrote:

Hi,

> This patch adds driver for UART controller present on Amlogic S905
> SoC.
> https://dn.odroid.com/S905/DataSheet/S905_Public_Datasheet_V1.1.4.pdf
> 
> Signed-off-by: Amit Singh Tomar <amittomer25@xxxxxxxxx>
> ---
>  xen/drivers/char/Kconfig      |   8 ++
>  xen/drivers/char/Makefile     |   1 +
>  xen/drivers/char/meson-uart.c | 290
> ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 299
> insertions(+) create mode 100644 xen/drivers/char/meson-uart.c
> 
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index cc78ec3..b9fee4e 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -20,6 +20,14 @@ config HAS_MVEBU
>         This selects the Marvell MVEBU UART. If you have a ARMADA
> 3700 based board, say Y.
>  
> +config HAS_MESON
> +    bool
> +    default y
> +    depends on ARM_64
> +    help
> +      This selects the Marvell MESON UART. If you have a Amlogic S905
> +      based board, say Y.

It seems this UART driver supports all Amlogic SoCs (905X, 805X, 912,
...), not just the S905. So please either remove the number or make it
clear that it's just an example.
And it's not a Marvell UART ;-)

> +
>  config HAS_PL011
>       bool
>       default y
> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> index b68c330..7c646d7 100644
> --- a/xen/drivers/char/Makefile
> +++ b/xen/drivers/char/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_HAS_NS16550) += ns16550.o
>  obj-$(CONFIG_HAS_CADENCE_UART) += cadence-uart.o
>  obj-$(CONFIG_HAS_PL011) += pl011.o
>  obj-$(CONFIG_HAS_EXYNOS4210) += exynos4210-uart.o
> +obj-$(CONFIG_HAS_MESON) += meson-uart.o
>  obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
>  obj-$(CONFIG_HAS_OMAP) += omap-uart.o
>  obj-$(CONFIG_HAS_SCIF) += scif-uart.o
> diff --git a/xen/drivers/char/meson-uart.c
> b/xen/drivers/char/meson-uart.c new file mode 100644
> index 0000000..8fe7e62
> --- /dev/null
> +++ b/xen/drivers/char/meson-uart.c
> @@ -0,0 +1,290 @@
> +/*
> + * xen/drivers/char/meson-uart.c
> + *
> + * Driver for Amlogic MESON UART
> + *
> + * Copyright (c) 2018, Amit Singh Tomar <amittomer25@xxxxxxxxx>.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/irq.h>
> +#include <xen/serial.h>
> +#include <xen/vmap.h>
> +#include <asm/io.h>
> +
> +/* Register offsets */
> +#define UART_WFIFO                  0x00
> +#define UART_RFIFO                  0x04
> +#define UART_CONTROL                0x08
> +#define UART_STATUS                 0x0c
> +#define UART_MISC                   0x10
> +#define UART_REG5                   0x14
> +
> +/* UART_CONTROL bits */
> +#define UART_TX_EN                  BIT(12)
> +#define UART_RX_EN                  BIT(13)

You don't seem to use them in the code? This seems somewhat wrong, you
shouldn't rely on those bits being set by previous boot stages.

> +#define UART_TX_RST                 BIT(22)
> +#define UART_RX_RST                 BIT(23)
> +#define UART_CLEAR_ERR              BIT(24)
> +#define UART_RX_INT_EN              BIT(27)
> +#define UART_TX_INT_EN              BIT(28)
> +
> +/* UART_STATUS bits */
> +#define UART_PARITY_ERR             BIT(16)
> +#define UART_FRAME_ERR              BIT(17)
> +#define UART_TX_FIFO_WERR           BIT(18)

You don't use those, so I don't see a need to define them.

> +#define UART_RX_EMPTY               BIT(20)
> +#define UART_TX_FULL                BIT(21)
> +#define UART_TX_EMPTY               BIT(22)

Might be worth to add FIFO_ in those names.

> +#define UART_TX_CNT_MASK            GENMASK(14, 8)
> +
> +
> +#define UART_XMIT_IRQ_CNT_MASK      GENMASK(15, 8)
> +#define UART_RECV_IRQ_CNT_MASK      GENMASK(7, 0)
> +
> +#define TX_FIFO_SIZE                64
> +
> +static struct meson_s905_uart {
> +    unsigned int irq;
> +    void __iomem *regs;
> +    struct irqaction irqaction;
> +    struct vuart_info vuart;
> +} meson_s905_com = {0};
> +
> +#define meson_s905_read(uart, off)  readl((uart)->regs + off)
> +#define meson_s905_write(uart, off, val) writel(val, (uart->regs) + off)

I was wondering whether a clrsetbit helper would be more useful than
these very thin wrappers.

> +static void meson_s905_uart_interrupt(int irq, void *data,
> +        struct cpu_user_regs *regs)
> +{
> +    struct serial_port *port = data;
> +    struct meson_s905_uart *uart = port->uart;
> +    uint32_t st = meson_s905_read(uart, UART_STATUS);
> +
> +    if ( !(st & UART_RX_EMPTY) )
> +    {
> +        serial_rx_interrupt(port, regs);
> +    }
> +
> +    if ( !(st & UART_TX_FULL) )
> +    {
> +        if ( st & UART_TX_INT_EN )

No. This bit is in the control register, not the status register.
And do you actually need to read this from the hardware?

> +            serial_tx_interrupt(port, regs);
> +    }
> +
> +}
> +
> +static void __init meson_s905_uart_init_preirq(struct serial_port
> *port) +{
> +    struct meson_s905_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    reg = meson_s905_read(uart, UART_CONTROL);
> +    reg &= ~(UART_RX_RST | UART_TX_RST | UART_CLEAR_ERR);
> +    meson_s905_write(uart, UART_CONTROL, reg);
> +
> +    /* Disbale Rx/Tx interrupts */
> +    reg = meson_s905_read(uart, UART_CONTROL);
> +    reg &= ~(UART_RX_INT_EN | UART_TX_INT_EN);
> +    meson_s905_write(uart, UART_CONTROL, reg);

Why are those two separate sequences, the second just for disabling the
interrupts? They should be off from the beginning.
Besides: You should properly reset the UART, by first setting the RST
bits, then clearing them again (Julien: they are not self-resetting).
So I guess it's one MMIO read, and two writes.

> +}
> +
> +static void __init meson_s905_uart_init_postirq(struct serial_port
> *port) +{
> +    struct meson_s905_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    uart->irqaction.handler = meson_s905_uart_interrupt;
> +    uart->irqaction.name    = "meson_s905_uart";
> +    uart->irqaction.dev_id  = port;
> +
> +    if ( setup_irq(uart->irq, 0, &uart->irqaction) != 0 )
> +    {
> +        printk("Failed to allocated meson_s905_uart IRQ %d\n",
> uart->irq);
> +        return;
> +    }
> +
> +    /* Configure Rx/Tx interrupts based on bytes in FIFO */
> +    reg = meson_s905_read(uart, UART_MISC);
> +    reg = (UART_RECV_IRQ_CNT_MASK & 1) |
> +           (UART_XMIT_IRQ_CNT_MASK & ((TX_FIFO_SIZE / 2) << 8));

I think Julien mentioned that before: this looks wrong.
Even with using |= this is still failing, as you want to clear the bits
before ORing in some new values.

> +    meson_s905_write(uart, UART_MISC, reg);
> +
> +    /* Make sure Rx/Tx interrupts are enabled now */
> +    reg = meson_s905_read(uart, UART_CONTROL);
> +    reg |= (UART_RX_INT_EN | UART_TX_INT_EN);
> +    meson_s905_write(uart, UART_CONTROL, reg);
> +}
> +
> +static void meson_s905_uart_suspend(struct serial_port *port)
> +{
> +    BUG();
> +}
> +
> +static void meson_s905_uart_resume(struct serial_port *port)
> +{
> +    BUG();
> +}
> +
> +static void meson_s905_uart_putc(struct serial_port *port, char c)
> +{
> +    struct meson_s905_uart *uart = port->uart;
> +
> +    meson_s905_write(uart, UART_WFIFO, c);
> +}
> +
> +static int meson_s905_uart_getc(struct serial_port *port, char *c)
> +{
> +    struct meson_s905_uart *uart = port->uart;
> +
> +    if ( (meson_s905_read(uart, UART_STATUS) & UART_RX_EMPTY) )
> +        return 0;
> +
> +    *c =  meson_s905_read(uart, UART_RFIFO) & 0xff;
> +
> +    return 1;
> +}
> +
> +static int __init meson_s905_irq(struct serial_port *port)
> +{
> +    struct meson_s905_uart *uart = port->uart;
> +
> +    return uart->irq;
> +}
> +
> +static const struct vuart_info *meson_s905_vuart_info(struct
> serial_port *port) +{
> +    struct meson_s905_uart *uart = port->uart;
> +
> +    return &uart->vuart;
> +}
> +
> +static void meson_s905_uart_stop_tx(struct serial_port *port)
> +{
> +    struct meson_s905_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    reg = meson_s905_read(uart, UART_CONTROL);
> +    reg &= ~UART_TX_INT_EN;
> +    meson_s905_write(uart, UART_CONTROL, reg);
> +}
> +
> +static void meson_s905_uart_start_tx(struct serial_port *port)
> +{
> +    struct meson_s905_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    reg = meson_s905_read(uart, UART_CONTROL);
> +    reg |= UART_TX_INT_EN;
> +    meson_s905_write(uart, UART_CONTROL, reg);
> +}
> +
> +static int meson_s905_uart_tx_ready(struct serial_port *port)
> +{
> +    struct meson_s905_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    reg = meson_s905_read(uart, UART_STATUS);
> +
> +    if ( reg & UART_TX_EMPTY )
> +        return TX_FIFO_SIZE;
> +    if ( reg & UART_TX_FULL )
> +        return 0;
> +
> +    return (reg & UART_TX_CNT_MASK) >> 8;
> +}
> +
> +static struct uart_driver __read_mostly meson_uart_driver = {
> +    .init_preirq  = meson_s905_uart_init_preirq,
> +    .init_postirq = meson_s905_uart_init_postirq,
> +    .endboot      = NULL,
> +    .suspend      = meson_s905_uart_suspend,
> +    .resume       = meson_s905_uart_resume,
> +    .putc         = meson_s905_uart_putc,
> +    .getc         = meson_s905_uart_getc,
> +    .tx_ready    = meson_s905_uart_tx_ready,

w/s

> +    .stop_tx      = meson_s905_uart_stop_tx,
> +    .start_tx     = meson_s905_uart_start_tx,
> +    .irq          = meson_s905_irq,
> +    .vuart_info   = meson_s905_vuart_info,
> +};
> +
> +static int __init meson_uart_init(struct dt_device_node *dev, const
> void *data) +{
> +    const char *config = data;
> +    struct meson_s905_uart *uart;
> +    int res;
> +    u64 addr, size;
> +
> +    if ( strcmp(config, "") )
> +        printk("WARNING: UART configuration is not supported\n");
> +
> +    uart = &meson_s905_com;
> +
> +    res = dt_device_get_address(dev, 0, &addr, &size);
> +    if ( res )
> +    {
> +        printk("meson_s905: Unable to retrieve the base address of
> the UART\n");
> +        return res;
> +    }
> +
> +    res = platform_get_irq(dev, 0);
> +    if ( res < 0 )
> +    {
> +        printk("meson_s905: Unable to retrieve the IRQ\n");
> +        return -EINVAL;
> +    }
> +
> +    uart->irq  = res;
> +
> +    uart->regs = ioremap_nocache(addr, size);
> +    if ( !uart->regs )
> +    {
> +        printk("meson_s905: Unable to map the UART memory\n");

UART memory sounds weird. Either just UART or UART MMIO frame.

> +        return -ENOMEM;
> +    }
> +
> +    uart->vuart.base_addr = addr;
> +    uart->vuart.size = size;
> +    uart->vuart.data_off = UART_CONTROL;

This should be WFIFO.

> +    uart->vuart.status_off = UART_STATUS;
> +    uart->vuart.status = UART_RX_EMPTY | UART_TX_EMPTY;
> +
> +    /* Register with generic serial driver. */
> +    serial_register_uart(SERHND_DTUART, &meson_uart_driver, uart);
> +
> +    dt_device_set_used_by(dev, DOMID_XEN);
> +
> +    return 0;
> +}
> +
> +static const struct dt_device_match meson_dt_match[] __initconst =
> +{
> +    DT_MATCH_COMPATIBLE("amlogic,meson-uart"),

You should list all the UART compatible names the Linux driver lists.
The difference between the legacy binding and the "stable" ones seems to
be only the clock references, which we don't care about.
Their usage of compatible is a bit weird, but at least you need
"amlogic,meson-gx-uart" to cover the 64-bit parts.

Cheers,
Andre.

> +    { /* sentinel */ },
> +};
> +
> +DT_DEVICE_START(meson, "Amlogic-S905 UART", DEVICE_SERIAL)
> +    .dt_match = meson_dt_match,
> +    .init = meson_uart_init,
> +DT_DEVICE_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> +*/
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel