[RFC PATCH 0/2] xen/arm64: Add Support for Amlogic S905 SoC

[RFC PATCH 0/2] xen/arm64: Add Support for Amlogic S905 SoC

[Amit Singh Tomar]

This small series enabled XEN booting on NanoPI K2 board[1] based
on Amlogic SoC. 

It has been tested by booting Dom0 Kernel.

TODO:
       * Wiki page to capture XEN boot info.

[1]: https://www.friendlyarm.com/index.php?route=product/product&product_id=186

Amit Singh Tomar (2):
  xen/arm: Add Amlogic S905 SoC early printk support
  xen/arm: Add MESON UART driver for Amlogic S905 SoC

 docs/misc/arm/early-printk.txt     |   1 +
 xen/arch/arm/Rules.mk              |   1 +
 xen/arch/arm/arm64/debug-meson.inc |  50 +++++++
 xen/drivers/char/Kconfig           |   8 +
 xen/drivers/char/Makefile          |   1 +
 xen/drivers/char/meson-uart.c      | 290 +++++++++++++++++++++++++++++++++++++
 6 files changed, 351 insertions(+)
 create mode 100644 xen/arch/arm/arm64/debug-meson.inc
 create mode 100644 xen/drivers/char/meson-uart.c

-- 
1.9.1


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

[RFC PATCH 1/2] xen/arm: Add Amlogic S905 SoC early printk support

[Amit Singh Tomar]

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
 docs/misc/arm/early-printk.txt     |  1 +
 xen/arch/arm/Rules.mk              |  1 +
 xen/arch/arm/arm64/debug-meson.inc | 50 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)
 create mode 100644 xen/arch/arm/arm64/debug-meson.inc

diff --git a/docs/misc/arm/early-printk.txt b/docs/misc/arm/early-printk.txt
index f765f59..2aa9528 100644
--- a/docs/misc/arm/early-printk.txt
+++ b/docs/misc/arm/early-printk.txt
@@ -41,6 +41,7 @@ the name of the machine:
   - juno: printk with pl011 on Juno platform
   - lager: printk with SCIF0 on Renesas R-Car H2 processors
   - midway: printk with the pl011 on Calxeda Midway processors
+  - meson: printk with the MESON for Amlogic S905 SoCs
   - mvebu: printk with the MVEBU for Marvell Armada 3700 SoCs
   - omap5432: printk with UART3 on TI OMAP5432 processors
   - rcar3: printk with SCIF2 on Renesas R-Car Gen3 processors
diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index f264592..d4fabdc 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -36,6 +36,7 @@ EARLY_PRINTK_hikey960       := pl011,0xfff32000
 EARLY_PRINTK_juno           := pl011,0x7ff80000
 EARLY_PRINTK_lager          := scif,0xe6e60000
 EARLY_PRINTK_midway         := pl011,0xfff36000
+EARLY_PRINTK_meson          := meson,0xc81004c0
 EARLY_PRINTK_mvebu          := mvebu,0xd0012000
 EARLY_PRINTK_omap5432       := 8250,0x48020000,2
 EARLY_PRINTK_rcar3          := scif,0xe6e88000
diff --git a/xen/arch/arm/arm64/debug-meson.inc b/xen/arch/arm/arm64/debug-meson.inc
new file mode 100644
index 0000000..d5507d3
--- /dev/null
+++ b/xen/arch/arm/arm64/debug-meson.inc
@@ -0,0 +1,50 @@
+/*
+ * xen/arch/arm/arm64/debug-meson.inc
+ *
+ * MESON specific debug code.
+ *
+ * Copyright (c) 2018, Amit Singh Tomar <amittomer25@gmail.com>.
+ *
+ * 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/>.
+ */
+
+#define UART_STATUS_REG     0x0c
+#define UART_TX_REG         0x00
+
+/*
+ * MESON UART wait UART to be ready to transmit
+ * xb: register which contains the UART base address
+ * c: scratch register
+ */
+.macro early_uart_ready xb c
+1:
+        ldrh   w\c, [\xb, #UART_STATUS_REG] /* status register */
+        tst    w\c, #(1 << 21)              /* Check TXFIFO FULL bit */
+        b.ne   1b                           /* Wait for the UART to be ready */
+.endm
+
+/*
+ * MESON UART transmit character
+ * xb: register which contains the UART base address
+ * wt: register which contains the character to transmit
+ */
+.macro early_uart_transmit xb wt
+	strb  \wt, [\xb, #UART_TX_REG]
+.endm
+
+/*
+ * Local variables:
+ * mode: ASM
+ * 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

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

[Amit Singh Tomar]

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@gmail.com>
---
 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.
+
 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@gmail.com>.
+ *
+ * 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)
+#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)
+#define UART_RX_EMPTY               BIT(20)
+#define UART_TX_FULL                BIT(21)
+#define UART_TX_EMPTY               BIT(22)
+#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)
+
+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 )
+            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);
+}
+
+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));
+    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,
+    .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");
+        return -ENOMEM;
+    }
+
+    uart->vuart.base_addr = addr;
+    uart->vuart.size = size;
+    uart->vuart.data_off = UART_CONTROL;
+    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"),
+    { /* 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:
+*/
-- 
1.9.1


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

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

[Jan Beulich]

>>> On 07.08.18 at 19:07, <amittomer25@gmail.com> wrote:
> 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@gmail.com>
> ---
>  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

The driver being ARM-specific, you will want to update
./MAINTAINERS to also list this new file as ARM-specific.

> --- 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.
> +
>  config HAS_PL011
>  	bool
>  	default y

Please fix indentation to match that of surrounding code. Also
please use "def_bool y" rather than its longer two line equivalent.

Jan



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

Re: [RFC PATCH 1/2] xen/arm: Add Amlogic S905 SoC early printk support

[Julien Grall]

Hi Amit,

On 07/08/18 18:07, Amit Singh Tomar wrote:
> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
> ---
>   docs/misc/arm/early-printk.txt     |  1 +
>   xen/arch/arm/Rules.mk              |  1 +
>   xen/arch/arm/arm64/debug-meson.inc | 50 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 52 insertions(+)
>   create mode 100644 xen/arch/arm/arm64/debug-meson.inc
> 
> diff --git a/docs/misc/arm/early-printk.txt b/docs/misc/arm/early-printk.txt
> index f765f59..2aa9528 100644
> --- a/docs/misc/arm/early-printk.txt
> +++ b/docs/misc/arm/early-printk.txt
> @@ -41,6 +41,7 @@ the name of the machine:
>     - juno: printk with pl011 on Juno platform
>     - lager: printk with SCIF0 on Renesas R-Car H2 processors
>     - midway: printk with the pl011 on Calxeda Midway processors
> +  - meson: printk with the MESON for Amlogic S905 SoCs
>     - mvebu: printk with the MVEBU for Marvell Armada 3700 SoCs
>     - omap5432: printk with UART3 on TI OMAP5432 processors
>     - rcar3: printk with SCIF2 on Renesas R-Car Gen3 processors
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index f264592..d4fabdc 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -36,6 +36,7 @@ EARLY_PRINTK_hikey960       := pl011,0xfff32000
>   EARLY_PRINTK_juno           := pl011,0x7ff80000
>   EARLY_PRINTK_lager          := scif,0xe6e60000
>   EARLY_PRINTK_midway         := pl011,0xfff36000
> +EARLY_PRINTK_meson          := meson,0xc81004c0

I would prefer if no new alias are added. The same could be achieved 
with CONFIG_EARLY_PRINTK=meson,0xc81004c0.

This could be documented on the wiki.

Cheers,

>   EARLY_PRINTK_mvebu          := mvebu,0xd0012000
>   EARLY_PRINTK_omap5432       := 8250,0x48020000,2
>   EARLY_PRINTK_rcar3          := scif,0xe6e88000
> diff --git a/xen/arch/arm/arm64/debug-meson.inc b/xen/arch/arm/arm64/debug-meson.inc
> new file mode 100644
> index 0000000..d5507d3
> --- /dev/null
> +++ b/xen/arch/arm/arm64/debug-meson.inc
> @@ -0,0 +1,50 @@
> +/*
> + * xen/arch/arm/arm64/debug-meson.inc
> + *
> + * MESON specific debug code.
> + *
> + * Copyright (c) 2018, Amit Singh Tomar <amittomer25@gmail.com>.
> + *
> + * 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/>.
> + */
> +
> +#define UART_STATUS_REG     0x0c
> +#define UART_TX_REG         0x00

I would prefer if we stick with the spec name. So UART_TX_REG should be 
renamed UART_WFIFO_REG.

Also, it might be worth considering to prefix them with AML_ so it is 
easy to find them on lookup.

> +

Looking at the earlyconsole implementation in Linux, it seems that TX 
needs to be enabled first (see meson_uart_enable_tx_engine).

Is it now done in the firmware?

> +/*
> + * MESON UART wait UART to be ready to transmit
> + * xb: register which contains the UART base address
> + * c: scratch register
> + */
> +.macro early_uart_ready xb c
> +1:
> +        ldrh   w\c, [\xb, #UART_STATUS_REG] /* status register */
> +        tst    w\c, #(1 << 21)              /* Check TXFIFO FULL bit */

Please define 1 << 21 rather than hardcoding it.

> +        b.ne   1b                           /* Wait for the UART to be ready */
> +.endm
> +
> +/*
> + * MESON UART transmit character
> + * xb: register which contains the UART base address
> + * wt: register which contains the character to transmit
> + */
> +.macro early_uart_transmit xb wt
> +	strb  \wt, [\xb, #UART_TX_REG]
> +.endm
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> 

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Amit,

On 07/08/18 18:07, Amit Singh Tomar wrote:
> 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

The spec does not seems to provide the offset register. Where did you 
find them?

> 
> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
> ---
>   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.
> +
>   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@gmail.com>.
> + *
> + * 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)
> +#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)
> +#define UART_RX_EMPTY               BIT(20)
> +#define UART_TX_FULL                BIT(21)
> +#define UART_TX_EMPTY               BIT(22)
> +#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};

AFAIK, {0} is not necessary.

> +
> +#define meson_s905_read(uart, off)           readl((uart)->regs + off)
> +#define meson_s905_write(uart, off, val)     writel(val, (uart->regs) + off)

s/(uart->regs)/(uart)->regs/

> +
> +static void meson_s905_uart_interrupt(int irq, void *data,
> +        struct cpu_user_regs *regs)

The indentation looks wrong here.

> +{
> +    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 )
> +            serial_tx_interrupt(port, regs);
> +    }
> +

NIT: No need for this newline.

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

I am not sure why you are clearing those bits. AFAIU, init_preirq will 
reset the serials, so you want to set thoses bits. This seems to be 
confirmed by Linux in meson_uart_reset.

> +    meson_s905_write(uart, UART_CONTROL, reg);
> +
> +    /* Disbale Rx/Tx interrupts */

s/Disbale/Disable/

> +    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 __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);

You read UART_MISC here but ...

> +    reg = (UART_RECV_IRQ_CNT_MASK & 1) |

... override the value here. You either want to drop reading UART_MISC 
or add | here.

> +           (UART_XMIT_IRQ_CNT_MASK & ((TX_FIFO_SIZE / 2) << 8));

This is a bit difficult to read. It feels like you want to use a macro 
with a parameter that will do the correct masking.

> +    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,
> +    .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");
> +        return -ENOMEM;
> +    }
> +
> +    uart->vuart.base_addr = addr;
> +    uart->vuart.size = size;
> +    uart->vuart.data_off = UART_CONTROL;
> +    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"),

Looking at Linux, this is considered as a legacy bindings. Would not it 
be better to use stable bindings in Xen?

Cheers,

-- 
Julien Grall

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

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

[Amit Tomer]

Hello,

Thanks for having a look at it.

> The spec does not seems to provide the offset register. Where did you find
> them?

Actually, looked at couple of references from u-boot and Linux. These
headers are picked from there.

> AFAIK, {0} is not necessary.

Ok.

>> +
>> +#define meson_s905_read(uart, off)           readl((uart)->regs + off)
>> +#define meson_s905_write(uart, off, val)     writel(val, (uart->regs) +
>> off)
>
>
> s/(uart->regs)/(uart)->regs/
>
>> +
>> +static void meson_s905_uart_interrupt(int irq, void *data,
>> +        struct cpu_user_regs *regs)
>
>
> The indentation looks wrong here.
>
>> +{
>> +    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 )
>> +            serial_tx_interrupt(port, regs);
>> +    }
>> +
>
>
> NIT: No need for this newline.
>
>> +}
>> +
>> +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);
>
>
> I am not sure why you are clearing those bits. AFAIU, init_preirq will reset
> the serials, so you want to set thoses bits. This seems to be confirmed by
> Linux in meson_uart_reset.

Idea here is to set these bits to their default values(which is 0 ) and if you
look at other drivers in XEN, it seems to be done same thing(clear
those bits) with them.

>
>> +    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 __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);
>
>
> You read UART_MISC here but ...
>
>> +    reg = (UART_RECV_IRQ_CNT_MASK & 1) |
>
>
> ... override the value here. You either want to drop reading UART_MISC or
> add | here.

Sorry, missed "|" somehow.
>
>> +           (UART_XMIT_IRQ_CNT_MASK & ((TX_FIFO_SIZE / 2) << 8));
>
>
> This is a bit difficult to read. It feels like you want to use a macro with
> a parameter that will do the correct masking.

Ok, shall I take it from Linux ?

>> +
>> +static const struct dt_device_match meson_dt_match[] __initconst =
>> +{
>> +    DT_MATCH_COMPATIBLE("amlogic,meson-uart"),
>
>
> Looking at Linux, this is considered as a legacy bindings. Would not it be
> better to use stable bindings in Xen?
>

Yeah, I took it from u-boot source and didn't realize that there are
stable binding exists.

Thanks
-Amit

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

Re: [RFC PATCH 1/2] xen/arm: Add Amlogic S905 SoC early printk support

[Amit Tomer]

Hello,

> I would prefer if no new alias are added. The same could be achieved with
> CONFIG_EARLY_PRINTK=meson,0xc81004c0.
>
> This could be documented on the wiki.

Ok.

> I would prefer if we stick with the spec name. So UART_TX_REG should be
> renamed UART_WFIFO_REG.

Yeah right, got your point.
>
> Also, it might be worth considering to prefix them with AML_ so it is easy
> to find them on lookup.

Initially used AML_ as prefix but then I just wanted to be consistent it with
other uart drivers in XEN.

> Looking at the earlyconsole implementation in Linux, it seems that TX needs
> to be enabled first (see meson_uart_enable_tx_engine).
>
> Is it now done in the firmware?

Yes, this has been done in u-boot.
shouldn't we trust it?


>> + */
>> +.macro early_uart_ready xb c
>> +1:
>> +        ldrh   w\c, [\xb, #UART_STATUS_REG] /* status register */
>> +        tst    w\c, #(1 << 21)              /* Check TXFIFO FULL bit */
>
>
> Please define 1 << 21 rather than hardcoding it.

Ok.


Thanks
-Amit

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

Re: [RFC PATCH 1/2] xen/arm: Add Amlogic S905 SoC early printk support

[Julien Grall]

On 20/08/18 10:16, Amit Tomer wrote:
> Hello,

Hi Amit,

>> I would prefer if no new alias are added. The same could be achieved with
>> CONFIG_EARLY_PRINTK=meson,0xc81004c0.
>>
>> This could be documented on the wiki.
> 
> Ok.
> 
>> I would prefer if we stick with the spec name. So UART_TX_REG should be
>> renamed UART_WFIFO_REG.
> 
> Yeah right, got your point.
>>
>> Also, it might be worth considering to prefix them with AML_ so it is easy
>> to find them on lookup.
> 
> Initially used AML_ as prefix but then I just wanted to be consistent it with
> other uart drivers in XEN.

I think I would prefer to be prefixed with AML_. The non-prefixed 
version is not that convenient for lookup and adding an extra one is not 
going to make better.

> 
>> Looking at the earlyconsole implementation in Linux, it seems that TX needs
>> to be enabled first (see meson_uart_enable_tx_engine).
>>
>> Is it now done in the firmware?
> 
> Yes, this has been done in u-boot.
> shouldn't we trust it?

I am trying to understand why Linux is doing it. Do you expect all 
U-Boot version to do it?

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

On 20/08/18 09:12, Amit Tomer wrote:
> Hello,

Hi,

> Thanks for having a look at it.
> 
>> The spec does not seems to provide the offset register. Where did you find
>> them?
> 
> Actually, looked at couple of references from u-boot and Linux. These
> headers are picked from there.

Please mention it in the commit message then.

[...]

>>> +}
>>> +
>>> +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);
>>
>>
>> I am not sure why you are clearing those bits. AFAIU, init_preirq will reset
>> the serials, so you want to set thoses bits. This seems to be confirmed by
>> Linux in meson_uart_reset.
> 
> Idea here is to set these bits to their default values(which is 0 ) and if you
> look at other drivers in XEN, it seems to be done same thing(clear
> those bits) with them.

Are you sure about this? RX_RST and TX_RST are bit to reset the 
transmission and receive path. Looking at a couple of different drivers 
(cache-uart.c and mvebu-uart.c), those 2 bits are set and I suspect be 
cleared by the hardware once reset.

Regarding UART_CLEAR_ERR, you indeed needs to clear the potential errors 
by zeroing it.

> 
>>
>>> +    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 __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);
>>
>>
>> You read UART_MISC here but ...
>>
>>> +    reg = (UART_RECV_IRQ_CNT_MASK & 1) |
>>
>>
>> ... override the value here. You either want to drop reading UART_MISC or
>> add | here.
> 
> Sorry, missed "|" somehow.
>>
>>> +           (UART_XMIT_IRQ_CNT_MASK & ((TX_FIFO_SIZE / 2) << 8));
>>
>>
>> This is a bit difficult to read. It feels like you want to use a macro with
>> a parameter that will do the correct masking.
> 
> Ok, shall I take it from Linux ?

Yes please.

> 
>>> +
>>> +static const struct dt_device_match meson_dt_match[] __initconst =
>>> +{
>>> +    DT_MATCH_COMPATIBLE("amlogic,meson-uart"),
>>
>>
>> Looking at Linux, this is considered as a legacy bindings. Would not it be
>> better to use stable bindings in Xen?
>>
> 
> Yeah, I took it from u-boot source and didn't realize that there are
> stable binding exists.

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH 1/2] xen/arm: Add Amlogic S905 SoC early printk support

[Amit Tomer]

Hello,

> I am trying to understand why Linux is doing it. Do you expect all
> U-Boot version to do it?

It's because Linux doesn't really trust u-boot and initializes every
thing again ?
Mainline U-boot does it but that may also not required since ATF does it for us.

Thanks
-Amit

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

Re: [RFC PATCH 1/2] xen/arm: Add Amlogic S905 SoC early printk support

[Julien Grall]

On 09/12/2018 07:43 AM, Amit Tomer wrote:
> Hello,
> 
>> I am trying to understand why Linux is doing it. Do you expect all
>> U-Boot version to do it?
> 
> It's because Linux doesn't really trust u-boot and initializes every
> thing again ?

You don't know the state of the UART once you left U-boot. For 
earlyprintk this may not be a big deal (they are for debugging only).

> Mainline U-boot does it but that may also not required since ATF does it for us.

What I wanted to check is whether there are U-boot out configuring 
"incorrectly" the UART for earlyprintk. But I guess we can leave that 
aside for now.

Cheers,

-- 
Julien Grall


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

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

[Amit Tomer]

Hello,

> >>> +    reg = meson_s905_read(uart, UART_CONTROL);
> >>> +    reg &= ~(UART_RX_RST | UART_TX_RST | UART_CLEAR_ERR);
> >>
> >>
> >> I am not sure why you are clearing those bits. AFAIU, init_preirq will reset
> >> the serials, so you want to set thoses bits. This seems to be confirmed by
> >> Linux in meson_uart_reset.
> >
> > Idea here is to set these bits to their default values(which is 0 ) and if you
> > look at other drivers in XEN, it seems to be done same thing(clear
> > those bits) with them.
>
> Are you sure about this? RX_RST and TX_RST are bit to reset the
> transmission and receive path. Looking at a couple of different drivers
> (cache-uart.c and mvebu-uart.c), those 2 bits are set and I suspect be
> cleared by the hardware once reset.

It's bit confusing to me, eventually Linux driver seems to clear those bits

https://github.com/torvalds/linux/blob/master/drivers/tty/serial/meson_uart.c#L266

Thanks
-Amit

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

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

[Julien Grall]

Hi,

On 04/10/2018 08:11, Amit Tomer wrote:
>>>>> +    reg = meson_s905_read(uart, UART_CONTROL);
>>>>> +    reg &= ~(UART_RX_RST | UART_TX_RST | UART_CLEAR_ERR);
>>>>
>>>>
>>>> I am not sure why you are clearing those bits. AFAIU, init_preirq will reset
>>>> the serials, so you want to set thoses bits. This seems to be confirmed by
>>>> Linux in meson_uart_reset.
>>>
>>> Idea here is to set these bits to their default values(which is 0 ) and if you
>>> look at other drivers in XEN, it seems to be done same thing(clear
>>> those bits) with them.
>>
>> Are you sure about this? RX_RST and TX_RST are bit to reset the
>> transmission and receive path. Looking at a couple of different drivers
>> (cache-uart.c and mvebu-uart.c), those 2 bits are set and I suspect be
>> cleared by the hardware once reset.
> 
> It's bit confusing to me, eventually Linux driver seems to clear those bits

But it sets them right before hand. What does the spec says about those 
bits?

Overall, I feels to me it is better to mimic the Linux driver as I am 
quite confident that the driver is doing the right thing.

Cheers,

-- 
Julien Grall

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

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

[André Przywara]

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@gmail.com>
> ---
>  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@gmail.com>.
> + *
> + * 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH 1/2] xen/arm: Add Amlogic S905 SoC early printk support

[André Przywara]

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

Hi,

commit message?

> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
> ---
>  docs/misc/arm/early-printk.txt     |  1 +
>  xen/arch/arm/Rules.mk              |  1 +
>  xen/arch/arm/arm64/debug-meson.inc | 50
> ++++++++++++++++++++++++++++++++++++++ 3 files changed, 52
> insertions(+) create mode 100644 xen/arch/arm/arm64/debug-meson.inc
> 
> diff --git a/docs/misc/arm/early-printk.txt
> b/docs/misc/arm/early-printk.txt index f765f59..2aa9528 100644
> --- a/docs/misc/arm/early-printk.txt
> +++ b/docs/misc/arm/early-printk.txt
> @@ -41,6 +41,7 @@ the name of the machine:
>    - juno: printk with pl011 on Juno platform
>    - lager: printk with SCIF0 on Renesas R-Car H2 processors
>    - midway: printk with the pl011 on Calxeda Midway processors
> +  - meson: printk with the MESON for Amlogic S905 SoCs
>    - mvebu: printk with the MVEBU for Marvell Armada 3700 SoCs
>    - omap5432: printk with UART3 on TI OMAP5432 processors
>    - rcar3: printk with SCIF2 on Renesas R-Car Gen3 processors
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index f264592..d4fabdc 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -36,6 +36,7 @@ EARLY_PRINTK_hikey960       := pl011,0xfff32000
>  EARLY_PRINTK_juno           := pl011,0x7ff80000
>  EARLY_PRINTK_lager          := scif,0xe6e60000
>  EARLY_PRINTK_midway         := pl011,0xfff36000
> +EARLY_PRINTK_meson          := meson,0xc81004c0
>  EARLY_PRINTK_mvebu          := mvebu,0xd0012000
>  EARLY_PRINTK_omap5432       := 8250,0x48020000,2
>  EARLY_PRINTK_rcar3          := scif,0xe6e88000
> diff --git a/xen/arch/arm/arm64/debug-meson.inc
> b/xen/arch/arm/arm64/debug-meson.inc new file mode 100644
> index 0000000..d5507d3
> --- /dev/null
> +++ b/xen/arch/arm/arm64/debug-meson.inc
> @@ -0,0 +1,50 @@
> +/*
> + * xen/arch/arm/arm64/debug-meson.inc
> + *
> + * MESON specific debug code.
> + *
> + * Copyright (c) 2018, Amit Singh Tomar <amittomer25@gmail.com>.
> + *
> + * 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/>.
> + */
> +
> +#define UART_STATUS_REG     0x0c
> +#define UART_TX_REG         0x00

As Julien mentioned, please stick to the manual names and be consistent
with the proper Xen driver. Also, please sort by offset.

> +
> +/*
> + * MESON UART wait UART to be ready to transmit
> + * xb: register which contains the UART base address
> + * c: scratch register
> + */
> +.macro early_uart_ready xb c
> +1:
> +        ldrh   w\c, [\xb, #UART_STATUS_REG] /* status register */

Why ldrh? This is a 32-bit register, actually you can't be sure that the
device supports a 16-bit access. Besides: the bit you are after is in
the upper half, so you actually will never see the bit set. I wonder if
you are loosing characters here.

> +        tst    w\c, #(1 << 21)              /* Check TXFIFO FULL bit
> */
> +        b.ne   1b                           /* Wait for the UART to
> be ready */

You can use "tbnz" to replace those two instructions.

> +.endm
> +
> +/*
> + * MESON UART transmit character
> + * xb: register which contains the UART base address
> + * wt: register which contains the character to transmit
> + */
> +.macro early_uart_transmit xb wt
> +	strb  \wt, [\xb, #UART_TX_REG]

TX_WFIFO is a 32-bit register, so you should rather use a 32-bit
accessor.

Cheers,
Andre.


> +.endm
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> 


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

Re: [RFC PATCH 1/2] xen/arm: Add Amlogic S905 SoC early printk support

[Amit Tomer]

Hi,

Thanks for having a detailed look at it.

> > +#define UART_STATUS_REG     0x0c
> > +#define UART_TX_REG         0x00
>> Why ldrh? This is a 32-bit register, actually you can't be sure that the
> device supports a 16-bit access. Besides: the bit you are after is in
> the upper half, so you actually will never see the bit set. I wonder if
> you are loosing characters here.

Let me confess with ldrh, I do see output scattered all over the screen and
really wondering why the hell I didn't notice it when I sent this RFC.

Very sorry about that.

> > +.macro early_uart_transmit xb wt
> > +     strb  \wt, [\xb, #UART_TX_REG]
>
> TX_WFIFO is a 32-bit register, so you should rather use a 32-bit
> accessor.

Ok.I guess "str" would be OK to use ?

Thanks
-Amit.

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