All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1] xen/arm: Add MVEBU UART driver for Armada 3700 SoC
@ 2018-03-16 17:34 Amit Singh Tomar
  2018-03-21 17:09 ` Wei Liu
  2018-03-30  0:46 ` André Przywara
  0 siblings, 2 replies; 7+ messages in thread
From: Amit Singh Tomar @ 2018-03-16 17:34 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, andre.przywara,
	ian.jackson, tim, julien.grall, jbeulich, andrew.cooper3,
	Amit Singh Tomar

This patch adds driver for UART controller found on Armada 3700 SoC.

There is no reference manuals available for 3700 SoC in public and this
driver is derived by looking at Linux driver.
https://github.com/torvalds/linux/blob/master/drivers/tty/serial/mvebu-uart.c

It allows XEN to boot on ESPRESSObin board based on Marvell's ARMADA 3700 SoC.

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
Changes since RFC:
	* Addressed Julien's Comments. 
TODO:
	* Wiki page to capture XEN boot info.
	* earlyprintk support.
---
 xen/drivers/char/Kconfig         |   8 ++
 xen/drivers/char/Makefile        |   1 +
 xen/drivers/char/mvebu-uart.c    | 260 +++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/mvebu-uart.h |  60 +++++++++
 4 files changed, 329 insertions(+)
 create mode 100644 xen/drivers/char/mvebu-uart.c
 create mode 100644 xen/include/asm-arm/mvebu-uart.h

diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index fb53dd8..690eda6 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -12,6 +12,14 @@ config HAS_CADENCE_UART
 	  This selects the Xilinx Zynq Cadence UART. If you have a Xilinx Zynq
 	  based board, say Y.
 
+config HAS_MVEBU
+        bool
+        default y
+        depends on ARM_64
+        help
+          This selects the Marvell MVEBU UART. if you have an ARMADA 3700
+          based board, say Y.
+
 config HAS_PL011
 	bool
 	default y
diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
index 0d48b16..b68c330 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_MVEBU) += mvebu-uart.o
 obj-$(CONFIG_HAS_OMAP) += omap-uart.o
 obj-$(CONFIG_HAS_SCIF) += scif-uart.o
 obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
diff --git a/xen/drivers/char/mvebu-uart.c b/xen/drivers/char/mvebu-uart.c
new file mode 100644
index 0000000..c88d5e7
--- /dev/null
+++ b/xen/drivers/char/mvebu-uart.c
@@ -0,0 +1,260 @@
+/*
+ * xen/drivers/char/mvebu3700-uart.c
+ *
+ * Driver for Marvell MVEBU UART.
+ *
+ * Amit Singh Tomar <amittomer25@gmail.com>
+ * Copyright (c) 2018.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#include <xen/console.h>
+#include <xen/errno.h>
+#include <xen/init.h>
+#include <xen/irq.h>
+#include <xen/mm.h>
+#include <xen/serial.h>
+#include <xen/vmap.h>
+#include <asm/device.h>
+#include <asm/io.h>
+#include <asm/mvebu-uart.h>
+
+static struct mvebu3700_uart {
+    unsigned int irq;
+    void __iomem *regs;
+    struct irqaction irqaction;
+    struct vuart_info vuart;
+} mvebu3700_com = {0};
+
+#define mvebu3700_read(uart, off)           readl((uart)->regs + off)
+#define mvebu3700_write(uart, off, val)     writel(val, (uart->regs) + off)
+
+static void mvebu3700_uart_interrupt(int irq, void *data,
+                                     struct cpu_user_regs *regs)
+{
+    struct serial_port *port = data;
+    struct mvebu3700_uart *uart = port->uart;
+    uint32_t st = mvebu3700_read(uart, UART_STATUS_REG);
+
+    if ( st & STATUS_TX_RDY )
+        serial_tx_interrupt(port, regs);
+
+    if ( st & (STATUS_RX_RDY | STATUS_OVR_ERR | STATUS_FRM_ERR |
+               STATUS_BRK_DET) )
+        serial_rx_interrupt(port, regs);
+}
+
+static void __init mvebu3700_uart_init_preirq(struct serial_port *port)
+{
+    struct mvebu3700_uart *uart = port->uart;
+    uint32_t reg;
+
+    reg = mvebu3700_read(uart, UART_CTRL_REG);
+    reg |= (CTRL_TXFIFO_RST | CTRL_RXFIFO_RST);
+    mvebu3700_write(uart, UART_CTRL_REG, reg);
+
+    /* Before we make IRQ request, clear the error bits of state register. */
+    reg = mvebu3700_read(uart, UART_STATUS_REG);
+    reg |= STATUS_BRK_ERR;
+    mvebu3700_write(uart, UART_STATUS_REG, reg);
+
+    /* Clear error interrupts. */
+    mvebu3700_write(uart, UART_CTRL_REG, CTRL_RX_INT);
+
+    /* Disable Rx/Tx interrupts. */
+    reg = mvebu3700_read(uart, UART_CTRL_REG);
+    reg &= ~(CTRL_RX_RDY_INT | CTRL_TX_RDY_INT);
+    mvebu3700_write(uart, UART_CTRL_REG, reg);
+}
+
+static void __init mvebu3700_uart_init_postirq(struct serial_port *port)
+{
+    struct mvebu3700_uart *uart = port->uart;
+    uint32_t reg;
+
+    if ( uart->irq > 0 )
+    {
+        uart->irqaction.handler = mvebu3700_uart_interrupt;
+        uart->irqaction.name    = "mvebu3700_uart";
+        uart->irqaction.dev_id  = port;
+    }
+
+    if ( setup_irq(uart->irq, 0, &uart->irqaction) != 0 )
+    {
+        printk("Failed to allocated mvebu3700_uart IRQ %d\n",
+                uart->irq);
+        return;
+    }
+
+    /* Make sure Rx/Tx interrupts are enabled now */
+    reg = mvebu3700_read(uart, UART_CTRL_REG);
+    reg |= (CTRL_RX_RDY_INT | CTRL_TX_RDY_INT);
+    mvebu3700_write(uart, UART_CTRL_REG, reg);
+}
+
+static void mvebu3700_uart_suspend(struct serial_port *port)
+{
+    BUG();
+}
+
+static void mvebu3700_uart_resume(struct serial_port *port)
+{
+    BUG();
+}
+
+static void mvebu3700_uart_putc(struct serial_port *port, char c)
+{
+    struct mvebu3700_uart *uart = port->uart;
+
+    mvebu3700_write(uart, UART_TX_REG, c);
+}
+
+static int mvebu3700_uart_getc(struct serial_port *port, char *c)
+{
+    struct mvebu3700_uart *uart = port->uart;
+
+    if ( !(mvebu3700_read(uart, UART_STATUS_REG) & STATUS_RX_RDY) )
+        return 0;
+
+    *c = mvebu3700_read(uart, UART_RX_REG) & 0xff;
+
+    return 1;
+}
+
+static int __init mvebu3700_irq(struct serial_port *port)
+{
+    struct mvebu3700_uart *uart = port->uart;
+
+    return ( (uart->irq > 0) ? uart->irq : -1 );
+}
+
+static const struct vuart_info *mvebu3700_vuart_info(struct serial_port *port)
+{
+    struct mvebu3700_uart *uart = port->uart;
+
+    return &uart->vuart;
+}
+
+static void mvebu3700_uart_stop_tx(struct serial_port *port)
+{
+    struct mvebu3700_uart *uart = port->uart;
+    uint32_t reg;
+
+    reg = mvebu3700_read(uart, UART_CTRL_REG);
+    reg &= ~CTRL_TX_RDY_INT;
+    mvebu3700_write(uart, UART_CTRL_REG, reg);
+}
+
+static void mvebu3700_uart_start_tx(struct serial_port *port)
+{
+    struct mvebu3700_uart *uart = port->uart;
+    uint32_t reg;
+
+    reg = mvebu3700_read(uart, UART_CTRL_REG);
+    reg |= CTRL_TX_RDY_INT;
+    mvebu3700_write(uart, UART_CTRL_REG, reg);
+}
+
+static int mvebu3700_uart_tx_ready(struct serial_port *port)
+{
+    struct mvebu3700_uart *uart = port->uart;
+    uint32_t reg;
+
+    reg = mvebu3700_read(uart, UART_STATUS_REG);
+
+    return ( reg & STATUS_TXFIFO_EMP ? TX_FIFO_SIZE : 0 );
+}
+
+static struct uart_driver __read_mostly mvebu3700_uart_driver = {
+    .init_preirq  = mvebu3700_uart_init_preirq,
+    .init_postirq = mvebu3700_uart_init_postirq,
+    .endboot      = NULL,
+    .suspend      = mvebu3700_uart_suspend,
+    .resume       = mvebu3700_uart_resume,
+    .putc         = mvebu3700_uart_putc,
+    .getc         = mvebu3700_uart_getc,
+    .tx_ready     = mvebu3700_uart_tx_ready,
+    .stop_tx      = mvebu3700_uart_stop_tx,
+    .start_tx     = mvebu3700_uart_start_tx,
+    .irq          = mvebu3700_irq,
+    .vuart_info   = mvebu3700_vuart_info,
+};
+
+static int __init mvebu_uart_init(struct dt_device_node *dev,
+                                       const void *data)
+{
+    const char *config = data;
+    struct mvebu3700_uart *uart;
+    int res;
+    u64 addr, size;
+
+    if ( strcmp(config, "") )
+        printk("WARNING: UART configuration is not supported\n");
+
+    uart = &mvebu3700_com;
+
+    res = dt_device_get_address(dev, 0, &addr, &size);
+    if ( res )
+    {
+        printk("mvebu3700: Unable to retrieve the base address of the UART\n");
+        return res;
+    }
+
+    res = platform_get_irq(dev, 0);
+    if ( res < 0 )
+    {
+        printk("mvebu3700: Unable to retrieve the IRQ\n");
+        return -EINVAL;
+    }
+
+    uart->irq  = res;
+
+    uart->regs = ioremap_nocache(addr, size);
+    if ( !uart->regs )
+    {
+        printk("mvebu3700: Unable to map the UART memory\n");
+        return -ENOMEM;
+    }
+
+    uart->vuart.base_addr = addr;
+    uart->vuart.size = size;
+    uart->vuart.data_off = UART_CTRL_REG;
+    uart->vuart.status_off = UART_STATUS_REG;
+    uart->vuart.status = STATUS_TX_RDY | STATUS_RX_RDY;
+
+    /* Register with generic serial driver. */
+    serial_register_uart(SERHND_DTUART, &mvebu3700_uart_driver, uart);
+
+    dt_device_set_used_by(dev, DOMID_XEN);
+
+    return 0;
+}
+
+static const struct dt_device_match mvebu_dt_match[] __initconst =
+{
+    DT_MATCH_COMPATIBLE("marvell,armada-3700-uart"),
+    { /* sentinel */ },
+};
+
+DT_DEVICE_START(mvebu, "Marvell Armada-3700 UART", DEVICE_SERIAL)
+    .dt_match = mvebu_dt_match,
+    .init = mvebu_uart_init,
+DT_DEVICE_END
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/mvebu-uart.h b/xen/include/asm-arm/mvebu-uart.h
new file mode 100644
index 0000000..0405b1d
--- /dev/null
+++ b/xen/include/asm-arm/mvebu-uart.h
@@ -0,0 +1,60 @@
+/*
+ * xen/include/asm-arm/mvebu-uart.h
+ *
+ * Amit Singh Tomar <amittomer25@gmail.com>
+ * Copyright (c) 2018.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+#ifndef __ASM_ARM_MVEBU_UART_H__
+#define __ASM_ARM_MVEBU_UART_H__
+
+/* Register offsets */
+#define UART_RX_REG             0x00
+
+#define UART_TX_REG             0x04
+
+#define UART_CTRL_REG           0x08
+#define CTRL_TXFIFO_RST         BIT(15)
+#define CTRL_RXFIFO_RST         BIT(14)
+#define CTRL_TX_RDY_INT         BIT(5)
+#define CTRL_RX_RDY_INT         BIT(4)
+#define CTRL_BRK_DET_INT        BIT(3)
+#define CTRL_FRM_ERR_INT        BIT(2)
+#define CTRL_PAR_ERR_INT        BIT(1)
+#define CTRL_OVR_ERR_INT        BIT(0)
+#define CTRL_RX_INT             (CTRL_BRK_DET_INT | CTRL_FRM_ERR_INT | \
+                                 CTRL_PAR_ERR_INT | CTRL_OVR_ERR_INT)
+
+#define UART_STATUS_REG         0x0c
+#define STATUS_TXFIFO_EMP       BIT(13)
+#define STATUS_TX_RDY           BIT(5)
+#define STATUS_RX_RDY           BIT(4)
+#define STATUS_BRK_DET          BIT(3)
+#define STATUS_FRM_ERR          BIT(2)
+#define STATUS_PAR_ERR          BIT(1)
+#define STATUS_OVR_ERR          BIT(0)
+#define STATUS_BRK_ERR          (STATUS_BRK_DET | STATUS_FRM_ERR | \
+                                STATUS_PAR_ERR | STATUS_OVR_ERR)
+
+#define TX_FIFO_SIZE            32
+
+#endif /* __ASM_ARM_MVEBU_UART_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.9.1


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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] xen/arm: Add MVEBU UART driver for Armada 3700 SoC
  2018-03-16 17:34 [PATCH v1] xen/arm: Add MVEBU UART driver for Armada 3700 SoC Amit Singh Tomar
@ 2018-03-21 17:09 ` Wei Liu
  2018-03-30  0:46 ` André Przywara
  1 sibling, 0 replies; 7+ messages in thread
From: Wei Liu @ 2018-03-21 17:09 UTC (permalink / raw)
  To: Amit Singh Tomar
  Cc: sstabellini, wei.liu2, George.Dunlap, tim, ian.jackson,
	julien.grall, andrew.cooper3, jbeulich, andre.przywara,
	xen-devel

On Fri, Mar 16, 2018 at 11:04:22PM +0530, Amit Singh Tomar wrote:
> diff --git a/xen/drivers/char/mvebu-uart.c b/xen/drivers/char/mvebu-uart.c
> new file mode 100644
> index 0000000..c88d5e7
> --- /dev/null
> +++ b/xen/drivers/char/mvebu-uart.c
> @@ -0,0 +1,260 @@
> +/*
> + * xen/drivers/char/mvebu3700-uart.c
> + *
> + * Driver for Marvell MVEBU UART.
> + *
> + * Amit Singh Tomar <amittomer25@gmail.com>
> + * Copyright (c) 2018.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *

Picking which licence is entirely up to you. But may I suggest you use
the section in CONTRIBUTING file? The new hypervisor code is normally
GPL v2 only.

Wei.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] xen/arm: Add MVEBU UART driver for Armada 3700 SoC
  2018-03-16 17:34 [PATCH v1] xen/arm: Add MVEBU UART driver for Armada 3700 SoC Amit Singh Tomar
  2018-03-21 17:09 ` Wei Liu
@ 2018-03-30  0:46 ` André Przywara
  2018-03-30 10:45   ` Amit Tomer
  1 sibling, 1 reply; 7+ messages in thread
From: André Przywara @ 2018-03-30  0:46 UTC (permalink / raw)
  To: Amit Singh Tomar, xen-devel
  Cc: sstabellini, wei.liu2, George.Dunlap, ian.jackson, tim,
	julien.grall, jbeulich, andrew.cooper3

Hi,

I tested this on my board and it works like expected. I would very much
like to see this driver still in 4.11.

Some (minor) comments on the code below.

On 16/03/18 17:34, Amit Singh Tomar wrote:
> This patch adds driver for UART controller found on Armada 3700 SoC.

Can you please mention "Marvell" in the subject?

> There is no reference manuals available for 3700 SoC in public and this
> driver is derived by looking at Linux driver.
> https://github.com/torvalds/linux/blob/master/drivers/tty/serial/mvebu-uart.c
> 
> It allows XEN to boot on ESPRESSObin board based on Marvell's ARMADA 3700 SoC.
> 
> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
> ---
> Changes since RFC:
> 	* Addressed Julien's Comments. 
> TODO:
> 	* Wiki page to capture XEN boot info.
> 	* earlyprintk support.

This has been done already. Thanks! Please provide a link to the Wiki page.

> ---
>  xen/drivers/char/Kconfig         |   8 ++
>  xen/drivers/char/Makefile        |   1 +
>  xen/drivers/char/mvebu-uart.c    | 260 +++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/mvebu-uart.h |  60 +++++++++
>  4 files changed, 329 insertions(+)
>  create mode 100644 xen/drivers/char/mvebu-uart.c
>  create mode 100644 xen/include/asm-arm/mvebu-uart.h
> 
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index fb53dd8..690eda6 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -12,6 +12,14 @@ config HAS_CADENCE_UART
>  	  This selects the Xilinx Zynq Cadence UART. If you have a Xilinx Zynq
>  	  based board, say Y.
>  
> +config HAS_MVEBU
> +        bool
> +        default y
> +        depends on ARM_64
> +        help
> +          This selects the Marvell MVEBU UART. if you have an ARMADA 3700
> +          based board, say Y.
> +

These should be indented by one tab (plus two spaces for the help text).
It's not obvious - I got this wrong myself the other day ;-), but it's
how the rest of the file works.

>  config HAS_PL011
>  	bool
>  	default y
> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> index 0d48b16..b68c330 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_MVEBU) += mvebu-uart.o
>  obj-$(CONFIG_HAS_OMAP) += omap-uart.o
>  obj-$(CONFIG_HAS_SCIF) += scif-uart.o
>  obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
> diff --git a/xen/drivers/char/mvebu-uart.c b/xen/drivers/char/mvebu-uart.c
> new file mode 100644
> index 0000000..c88d5e7
> --- /dev/null
> +++ b/xen/drivers/char/mvebu-uart.c
> @@ -0,0 +1,260 @@
> +/*
> + * xen/drivers/char/mvebu3700-uart.c
> + *
> + * Driver for Marvell MVEBU UART.
> + *
> + * Amit Singh Tomar <amittomer25@gmail.com>
> + * Copyright (c) 2018.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + */
> +
> +#include <xen/console.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/irq.h>
> +#include <xen/mm.h>
> +#include <xen/serial.h>
> +#include <xen/vmap.h>
> +#include <asm/device.h>
> +#include <asm/io.h>
> +#include <asm/mvebu-uart.h>
> +
> +static struct mvebu3700_uart {
> +    unsigned int irq;
> +    void __iomem *regs;
> +    struct irqaction irqaction;
> +    struct vuart_info vuart;
> +} mvebu3700_com = {0};
> +
> +#define mvebu3700_read(uart, off)           readl((uart)->regs + off)
> +#define mvebu3700_write(uart, off, val)     writel(val, (uart->regs) + off)
> +
> +static void mvebu3700_uart_interrupt(int irq, void *data,
> +                                     struct cpu_user_regs *regs)
> +{
> +    struct serial_port *port = data;
> +    struct mvebu3700_uart *uart = port->uart;
> +    uint32_t st = mvebu3700_read(uart, UART_STATUS_REG);
> +
> +    if ( st & STATUS_TX_RDY )
> +        serial_tx_interrupt(port, regs);
> +
> +    if ( st & (STATUS_RX_RDY | STATUS_OVR_ERR | STATUS_FRM_ERR |
> +               STATUS_BRK_DET) )
> +        serial_rx_interrupt(port, regs);
> +}
> +
> +static void __init mvebu3700_uart_init_preirq(struct serial_port *port)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    reg = mvebu3700_read(uart, UART_CTRL_REG);
> +    reg |= (CTRL_TXFIFO_RST | CTRL_RXFIFO_RST);
> +    mvebu3700_write(uart, UART_CTRL_REG, reg);
> +
> +    /* Before we make IRQ request, clear the error bits of state register. */
> +    reg = mvebu3700_read(uart, UART_STATUS_REG);
> +    reg |= STATUS_BRK_ERR;
> +    mvebu3700_write(uart, UART_STATUS_REG, reg);
> +
> +    /* Clear error interrupts. */
> +    mvebu3700_write(uart, UART_CTRL_REG, CTRL_RX_INT);
> +
> +    /* Disable Rx/Tx interrupts. */
> +    reg = mvebu3700_read(uart, UART_CTRL_REG);
> +    reg &= ~(CTRL_RX_RDY_INT | CTRL_TX_RDY_INT);
> +    mvebu3700_write(uart, UART_CTRL_REG, reg);
> +}
> +
> +static void __init mvebu3700_uart_init_postirq(struct serial_port *port)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    if ( uart->irq > 0 )
> +    {
> +        uart->irqaction.handler = mvebu3700_uart_interrupt;
> +        uart->irqaction.name    = "mvebu3700_uart";
> +        uart->irqaction.dev_id  = port;
> +    }
> +
> +    if ( setup_irq(uart->irq, 0, &uart->irqaction) != 0 )
> +    {
> +        printk("Failed to allocated mvebu3700_uart IRQ %d\n",
> +                uart->irq);
> +        return;
> +    }
> +
> +    /* Make sure Rx/Tx interrupts are enabled now */
> +    reg = mvebu3700_read(uart, UART_CTRL_REG);
> +    reg |= (CTRL_RX_RDY_INT | CTRL_TX_RDY_INT);
> +    mvebu3700_write(uart, UART_CTRL_REG, reg);
> +}
> +
> +static void mvebu3700_uart_suspend(struct serial_port *port)
> +{
> +    BUG();
> +}
> +
> +static void mvebu3700_uart_resume(struct serial_port *port)
> +{
> +    BUG();
> +}
> +
> +static void mvebu3700_uart_putc(struct serial_port *port, char c)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +
> +    mvebu3700_write(uart, UART_TX_REG, c);
> +}
> +
> +static int mvebu3700_uart_getc(struct serial_port *port, char *c)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +
> +    if ( !(mvebu3700_read(uart, UART_STATUS_REG) & STATUS_RX_RDY) )
> +        return 0;
> +
> +    *c = mvebu3700_read(uart, UART_RX_REG) & 0xff;
> +
> +    return 1;
> +}
> +
> +static int __init mvebu3700_irq(struct serial_port *port)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +
> +    return ( (uart->irq > 0) ? uart->irq : -1 );

No need for the brackets.

> +}
> +
> +static const struct vuart_info *mvebu3700_vuart_info(struct serial_port *port)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +
> +    return &uart->vuart;
> +}
> +
> +static void mvebu3700_uart_stop_tx(struct serial_port *port)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    reg = mvebu3700_read(uart, UART_CTRL_REG);
> +    reg &= ~CTRL_TX_RDY_INT;
> +    mvebu3700_write(uart, UART_CTRL_REG, reg);
> +}
> +
> +static void mvebu3700_uart_start_tx(struct serial_port *port)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    reg = mvebu3700_read(uart, UART_CTRL_REG);
> +    reg |= CTRL_TX_RDY_INT;
> +    mvebu3700_write(uart, UART_CTRL_REG, reg);
> +}
> +
> +static int mvebu3700_uart_tx_ready(struct serial_port *port)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +    uint32_t reg;
> +
> +    reg = mvebu3700_read(uart, UART_STATUS_REG);
> +
> +    return ( reg & STATUS_TXFIFO_EMP ? TX_FIFO_SIZE : 0 );

Same here.

> +}
> +
> +static struct uart_driver __read_mostly mvebu3700_uart_driver = {
> +    .init_preirq  = mvebu3700_uart_init_preirq,
> +    .init_postirq = mvebu3700_uart_init_postirq,
> +    .endboot      = NULL,
> +    .suspend      = mvebu3700_uart_suspend,
> +    .resume       = mvebu3700_uart_resume,
> +    .putc         = mvebu3700_uart_putc,
> +    .getc         = mvebu3700_uart_getc,
> +    .tx_ready     = mvebu3700_uart_tx_ready,
> +    .stop_tx      = mvebu3700_uart_stop_tx,
> +    .start_tx     = mvebu3700_uart_start_tx,
> +    .irq          = mvebu3700_irq,
> +    .vuart_info   = mvebu3700_vuart_info,
> +};
> +
> +static int __init mvebu_uart_init(struct dt_device_node *dev,
> +                                       const void *data)

Indentation.

> +{
> +    const char *config = data;
> +    struct mvebu3700_uart *uart;
> +    int res;
> +    u64 addr, size;
> +
> +    if ( strcmp(config, "") )
> +        printk("WARNING: UART configuration is not supported\n");
> +
> +    uart = &mvebu3700_com;
> +
> +    res = dt_device_get_address(dev, 0, &addr, &size);
> +    if ( res )
> +    {
> +        printk("mvebu3700: Unable to retrieve the base address of the UART\n");
> +        return res;
> +    }
> +
> +    res = platform_get_irq(dev, 0);
> +    if ( res < 0 )
> +    {
> +        printk("mvebu3700: Unable to retrieve the IRQ\n");
> +        return -EINVAL;
> +    }
> +
> +    uart->irq  = res;
> +
> +    uart->regs = ioremap_nocache(addr, size);
> +    if ( !uart->regs )
> +    {
> +        printk("mvebu3700: Unable to map the UART memory\n");
> +        return -ENOMEM;
> +    }
> +
> +    uart->vuart.base_addr = addr;
> +    uart->vuart.size = size;
> +    uart->vuart.data_off = UART_CTRL_REG;
> +    uart->vuart.status_off = UART_STATUS_REG;
> +    uart->vuart.status = STATUS_TX_RDY | STATUS_RX_RDY;
> +
> +    /* Register with generic serial driver. */
> +    serial_register_uart(SERHND_DTUART, &mvebu3700_uart_driver, uart);
> +
> +    dt_device_set_used_by(dev, DOMID_XEN);
> +
> +    return 0;
> +}
> +
> +static const struct dt_device_match mvebu_dt_match[] __initconst =
> +{
> +    DT_MATCH_COMPATIBLE("marvell,armada-3700-uart"),
> +    { /* sentinel */ },
> +};
> +
> +DT_DEVICE_START(mvebu, "Marvell Armada-3700 UART", DEVICE_SERIAL)
> +    .dt_match = mvebu_dt_match,
> +    .init = mvebu_uart_init,
> +DT_DEVICE_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/mvebu-uart.h b/xen/include/asm-arm/mvebu-uart.h
> new file mode 100644
> index 0000000..0405b1d
> --- /dev/null
> +++ b/xen/include/asm-arm/mvebu-uart.h

So why do we need this include file, in a shared directory?
All those bits are private to the UART driver and don't need to be
exposed to Xen at all.
If it's about the earlyprintk support: that's just two values needed
there, nothing worth a new include file, I think.
So I would recommend to declare the required constants directly in the
driver file.

Cheers,
Andre.

> @@ -0,0 +1,60 @@
> +/*
> + * xen/include/asm-arm/mvebu-uart.h
> + *
> + * Amit Singh Tomar <amittomer25@gmail.com>
> + * Copyright (c) 2018.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + */
> +
> +#ifndef __ASM_ARM_MVEBU_UART_H__
> +#define __ASM_ARM_MVEBU_UART_H__
> +
> +/* Register offsets */
> +#define UART_RX_REG             0x00
> +
> +#define UART_TX_REG             0x04
> +
> +#define UART_CTRL_REG           0x08
> +#define CTRL_TXFIFO_RST         BIT(15)
> +#define CTRL_RXFIFO_RST         BIT(14)
> +#define CTRL_TX_RDY_INT         BIT(5)
> +#define CTRL_RX_RDY_INT         BIT(4)
> +#define CTRL_BRK_DET_INT        BIT(3)
> +#define CTRL_FRM_ERR_INT        BIT(2)
> +#define CTRL_PAR_ERR_INT        BIT(1)
> +#define CTRL_OVR_ERR_INT        BIT(0)
> +#define CTRL_RX_INT             (CTRL_BRK_DET_INT | CTRL_FRM_ERR_INT | \
> +                                 CTRL_PAR_ERR_INT | CTRL_OVR_ERR_INT)
> +
> +#define UART_STATUS_REG         0x0c
> +#define STATUS_TXFIFO_EMP       BIT(13)
> +#define STATUS_TX_RDY           BIT(5)
> +#define STATUS_RX_RDY           BIT(4)
> +#define STATUS_BRK_DET          BIT(3)
> +#define STATUS_FRM_ERR          BIT(2)
> +#define STATUS_PAR_ERR          BIT(1)
> +#define STATUS_OVR_ERR          BIT(0)
> +#define STATUS_BRK_ERR          (STATUS_BRK_DET | STATUS_FRM_ERR | \
> +                                STATUS_PAR_ERR | STATUS_OVR_ERR)
> +
> +#define TX_FIFO_SIZE            32
> +
> +#endif /* __ASM_ARM_MVEBU_UART_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> 

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] xen/arm: Add MVEBU UART driver for Armada 3700 SoC
  2018-03-30  0:46 ` André Przywara
@ 2018-03-30 10:45   ` Amit Tomer
  2018-03-30 16:25     ` Stefano Stabellini
  0 siblings, 1 reply; 7+ messages in thread
From: Amit Tomer @ 2018-03-30 10:45 UTC (permalink / raw)
  To: André Przywara
  Cc: Stefano Stabellini, wei.liu2, George Dunlap, tim, ian.jackson,
	Julien Grall, Jan Beulich, andrew.cooper3, xen-devel

Hello,

> I tested this on my board and it works like expected. I would very much
> like to see this driver still in 4.11.

Thanks for looking into it and Many Thanks for testing it out.

>
> Some (minor) comments on the code below.
>
> On 16/03/18 17:34, Amit Singh Tomar wrote:
>> This patch adds driver for UART controller found on Armada 3700 SoC.
>
> Can you please mention "Marvell" in the subject?

Ok.

> These should be indented by one tab (plus two spaces for the help text).
> It's not obvious - I got this wrong myself the other day ;-), but it's
> how the rest of the file works.

Ok.

> No need for the brackets.

Ok.

> Indentation.

Ok.

> So why do we need this include file, in a shared directory?
> All those bits are private to the UART driver and don't need to be
> exposed to Xen at all.
> If it's about the earlyprintk support: that's just two values needed
> there, nothing worth a new include file, I think.
> So I would recommend to declare the required constants directly in the
> driver file.

Yes, I thought earlyprintk could also use a couple of common defines and other
drivers do the same way.


Thanks
-Amit

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] xen/arm: Add MVEBU UART driver for Armada 3700 SoC
  2018-03-30 10:45   ` Amit Tomer
@ 2018-03-30 16:25     ` Stefano Stabellini
  2018-03-31  9:15       ` Julien Grall
  0 siblings, 1 reply; 7+ messages in thread
From: Stefano Stabellini @ 2018-03-30 16:25 UTC (permalink / raw)
  To: Amit Tomer
  Cc: Stefano Stabellini, wei.liu2, George Dunlap, André Przywara,
	ian.jackson, tim, Julien Grall, Jan Beulich, andrew.cooper3,
	xen-devel

On Fri, 30 Mar 2018, Amit Tomer wrote:
> Hello,
> 
> > I tested this on my board and it works like expected. I would very much
> > like to see this driver still in 4.11.
> 
> Thanks for looking into it and Many Thanks for testing it out.

FYI the deadline is the end of next week. If you submit a patch by then
addressing Andre's comment I would be happy to check it in.


> > Some (minor) comments on the code below.
> >
> > On 16/03/18 17:34, Amit Singh Tomar wrote:
> >> This patch adds driver for UART controller found on Armada 3700 SoC.
> >
> > Can you please mention "Marvell" in the subject?
> 
> Ok.
> 
> > These should be indented by one tab (plus two spaces for the help text).
> > It's not obvious - I got this wrong myself the other day ;-), but it's
> > how the rest of the file works.
> 
> Ok.
> 
> > No need for the brackets.
> 
> Ok.
> 
> > Indentation.
> 
> Ok.
> 
> > So why do we need this include file, in a shared directory?
> > All those bits are private to the UART driver and don't need to be
> > exposed to Xen at all.
> > If it's about the earlyprintk support: that's just two values needed
> > there, nothing worth a new include file, I think.
> > So I would recommend to declare the required constants directly in the
> > driver file.
> 
> Yes, I thought earlyprintk could also use a couple of common defines and other
> drivers do the same way.
> 
> 
> Thanks
> -Amit
> 

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] xen/arm: Add MVEBU UART driver for Armada 3700 SoC
  2018-03-30 16:25     ` Stefano Stabellini
@ 2018-03-31  9:15       ` Julien Grall
  2018-04-04  0:39         ` Stefano Stabellini
  0 siblings, 1 reply; 7+ messages in thread
From: Julien Grall @ 2018-03-31  9:15 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: tim, wei.liu2, George Dunlap, André Przywara, ian.jackson,
	Amit Tomer, Julien Grall, Jan Beulich, andrew.cooper3, xen-devel


[-- Attachment #1.1: Type: text/plain, Size: 1854 bytes --]

On Sat, 31 Mar 2018, 00:27 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Fri, 30 Mar 2018, Amit Tomer wrote:
> > Hello,
> >
> > > I tested this on my board and it works like expected. I would very much
> > > like to see this driver still in 4.11.
> >
> > Thanks for looking into it and Many Thanks for testing it out.
>
> FYI the deadline is the end of next week. If you submit a patch by then
> addressing Andre's comment I would be happy to check it in.
>

I also had comments on the previous version. So it would be nice to give me
sometimes to review it again.

Cheers,


>
> > > Some (minor) comments on the code below.
> > >
> > > On 16/03/18 17:34, Amit Singh Tomar wrote:
> > >> This patch adds driver for UART controller found on Armada 3700 SoC.
> > >
> > > Can you please mention "Marvell" in the subject?
> >
> > Ok.
> >
> > > These should be indented by one tab (plus two spaces for the help
> text).
> > > It's not obvious - I got this wrong myself the other day ;-), but it's
> > > how the rest of the file works.
> >
> > Ok.
> >
> > > No need for the brackets.
> >
> > Ok.
> >
> > > Indentation.
> >
> > Ok.
> >
> > > So why do we need this include file, in a shared directory?
> > > All those bits are private to the UART driver and don't need to be
> > > exposed to Xen at all.
> > > If it's about the earlyprintk support: that's just two values needed
> > > there, nothing worth a new include file, I think.
> > > So I would recommend to declare the required constants directly in the
> > > driver file.
> >
> > Yes, I thought earlyprintk could also use a couple of common defines and
> other
> > drivers do the same way.
> >
> >
> > Thanks
> > -Amit
> >
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

[-- Attachment #1.2: Type: text/html, Size: 2849 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] xen/arm: Add MVEBU UART driver for Armada 3700 SoC
  2018-03-31  9:15       ` Julien Grall
@ 2018-04-04  0:39         ` Stefano Stabellini
  0 siblings, 0 replies; 7+ messages in thread
From: Stefano Stabellini @ 2018-04-04  0:39 UTC (permalink / raw)
  To: Julien Grall
  Cc: tim, Stefano Stabellini, wei.liu2, George Dunlap,
	André Przywara, ian.jackson, Amit Tomer, Julien Grall,
	Jan Beulich, andrew.cooper3, xen-devel

On Sat, 31 Mar 2018, Julien Grall wrote:
> On Sat, 31 Mar 2018, 00:27 Stefano Stabellini, <sstabellini@kernel.org> wrote:
>       On Fri, 30 Mar 2018, Amit Tomer wrote:
>       > Hello,
>       >
>       > > I tested this on my board and it works like expected. I would very much
>       > > like to see this driver still in 4.11.
>       >
>       > Thanks for looking into it and Many Thanks for testing it out.
> 
>       FYI the deadline is the end of next week. If you submit a patch by then
>       addressing Andre's comment I would be happy to check it in.
> 
> 
> I also had comments on the previous version. So it would be nice to give me sometimes to review it again.

Welcome back. I'll do.

Cheers,

Stefano


>       > > Some (minor) comments on the code below.
>       > >
>       > > On 16/03/18 17:34, Amit Singh Tomar wrote:
>       > >> This patch adds driver for UART controller found on Armada 3700 SoC.
>       > >
>       > > Can you please mention "Marvell" in the subject?
>       >
>       > Ok.
>       >
>       > > These should be indented by one tab (plus two spaces for the help text).
>       > > It's not obvious - I got this wrong myself the other day ;-), but it's
>       > > how the rest of the file works.
>       >
>       > Ok.
>       >
>       > > No need for the brackets.
>       >
>       > Ok.
>       >
>       > > Indentation.
>       >
>       > Ok.
>       >
>       > > So why do we need this include file, in a shared directory?
>       > > All those bits are private to the UART driver and don't need to be
>       > > exposed to Xen at all.
>       > > If it's about the earlyprintk support: that's just two values needed
>       > > there, nothing worth a new include file, I think.
>       > > So I would recommend to declare the required constants directly in the
>       > > driver file.
>       >
>       > Yes, I thought earlyprintk could also use a couple of common defines and other
>       > drivers do the same way.
>       >
>       >
>       > Thanks
>       > -Amit
>       >
> 
>       _______________________________________________
>       Xen-devel mailing list
>       Xen-devel@lists.xenproject.org
>       https://lists.xenproject.org/mailman/listinfo/xen-devel
> 
> 
> 

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-04-04  0:39 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-16 17:34 [PATCH v1] xen/arm: Add MVEBU UART driver for Armada 3700 SoC Amit Singh Tomar
2018-03-21 17:09 ` Wei Liu
2018-03-30  0:46 ` André Przywara
2018-03-30 10:45   ` Amit Tomer
2018-03-30 16:25     ` Stefano Stabellini
2018-03-31  9:15       ` Julien Grall
2018-04-04  0:39         ` Stefano Stabellini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.