[RFC PATCH] xen/arm64: Add Support for Marvell ARMADA 3700 SoC

[RFC PATCH] xen/arm64: Add Support for Marvell ARMADA 3700 SoC

[Amit Singh Tomar]

This patch-set is an attempt to enable XEN on ESPRESSObin[1] based on 
Marvell's ARMADA 3700 SoC

It includes simple serial port driver for ARMADA 3700 SoC.Platform itself
is discovered via device-tree.

With this patch, we can boot both Dom0 and DomU kernel on ESPRESSObin
board.

root@localhost:/home/amit/xen/tools/xl# xl list
Name                                        ID   Mem VCPUs	State	Time(s)
Domain-0                                     0   512     2     r-----      18.1
guest-1                                      1   256     1     -b----       6.0

Following command line is passed to XEN from bootloader.

tftp 0x2500000 Image;tftp 0x1500000 armada-3720-espressobin.dtb;tftp 0x1000000 xen
setenv Image_name Image;setenv fdt_name armada-3720-espressobin.dtb
setenv kernel_addr_r 0x2500000;setenv fdt_addr_r 0x1500000;setenv xen_addr_r 0x1000000
setenv fdt_high 0xffffffff;fdt addr $fdt_addr_r;fdt resize	
setenv xen_bootargs console=dtuart dtuart=/soc/internal-regs@d0000000/serial@12000 dom0_mem=512M
setenv dom0_bootargs  console=ttyMV0 console=hvc0,115200n8 earlycon=xenboot debug clk_ignore_unused root=/dev/mmcblk0p1 rw rootwait
fdt set /chosen xen,xen-bootargs \"$xen_bootargs\";fdt resize
fdt set /chosen xen,dom0-bootargs \"$dom0_bootargs\";fdt mknode /chosen modules
fdt set /chosen/modules '#address-cells' <1>;fdt set /chosen/modules '#size-cells' <1>;fdt mknode /chosen/modules module@0
fdt set /chosen/modules/module@0 compatible "multiboot,kernel", "multiboot,module";fdt resize
fdt set /chosen/modules/module@0 reg < $kernel_addr_r 0x1800000 >
booti ${xen_addr_r} - ${fdt_addr_r}	

I would really like to Thanks Andre for helping me out on this.

[1]http://wiki.espressobin.net/tiki-index.php

Amit Singh Tomar (1):
  xen/arm: Add MVEBU UART driver for Armada 3700 SoC

 xen/drivers/char/Kconfig      |   8 ++
 xen/drivers/char/Makefile     |   1 +
 xen/drivers/char/mvebu-uart.c | 315 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 324 insertions(+)
 create mode 100644 xen/drivers/char/mvebu-uart.c

-- 
1.9.1


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

[RFC PATCH] xen/arm: Add MVEBU UART driver for Armada 3700 SoC

[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.

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
 xen/drivers/char/Kconfig      |   8 ++
 xen/drivers/char/Makefile     |   1 +
 xen/drivers/char/mvebu-uart.c | 315 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 324 insertions(+)
 create mode 100644 xen/drivers/char/mvebu-uart.c

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..fdcc153
--- /dev/null
+++ b/xen/drivers/char/mvebu-uart.c
@@ -0,0 +1,315 @@
+/*
+ * 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 <asm/device.h>
+#include <asm/io.h>
+#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/serial.h>
+#include <xen/vmap.h>
+
+#define UART_RX_REG             0x00
+#define RBR_BRK_DET             BIT(15)
+#define RBR_FRM_ERR_DET         BIT(14)
+#define RBR_PAR_ERR_DET         BIT(13)
+#define RBR_OVR_ERR_DET         BIT(12)
+
+#define UART_TX_REG             0x04
+
+#define UART_CTRL_REG           0x08
+#define CTRL_SOFT_RST           BIT(31)
+#define CTRL_TXFIFO_RST         BIT(15)
+#define CTRL_RXFIFO_RST         BIT(14)
+#define CTRL_ST_MIRR_EN         BIT(13)
+#define CTRL_LPBK_EN            BIT(12)
+#define CTRL_SND_BRK_SEQ        BIT(11)
+#define CTRL_PAR_EN             BIT(10)
+#define CTRL_TWO_STOP           BIT(9)
+#define CTRL_TX_HFL_INT         BIT(8)
+#define CTRL_RX_HFL_INT         BIT(7)
+#define CTRL_TX_EMP_INT         BIT(6)
+#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_RXFIFO_EMP       BIT(12)
+#define STATUS_TXFIFO_FUL       BIT(11)
+#define STATUS_TXFIFO_HFL       BIT(10)
+#define STATUS_RX_TOGL          BIT(9)
+#define STATUS_RXFIFO_FUL       BIT(8)
+#define STATUS_RXFIFO_HFL       BIT(7)
+#define STATUS_TX_EMP           BIT(6)
+#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 UART_BAUD_REG           0x10
+#define UART_POSSR_REG          0x14
+
+#define TX_FIFO_SIZE            32
+#define RX_FIFO_SIZE            64
+
+static struct mvebu3700_uart {
+    unsigned int baud, data_bits, parity, stop_bits;
+    unsigned int irq;
+    void __iomem *regs;
+    struct irqaction irqaction;
+    struct vuart_info vuart;
+} mvebu3700_com = {0};
+
+#define PARITY_NONE  (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;
+    unsigned int 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;
+    unsigned ret;
+
+    ret = mvebu3700_read(uart, UART_CTRL_REG);
+    ret |= (CTRL_TXFIFO_RST | CTRL_RXFIFO_RST);
+    mvebu3700_write(uart, UART_CTRL_REG, ret);
+
+    /* Before we make IRQ request, Clear the error bits of state register */
+    ret = mvebu3700_read(uart, UART_STATUS_REG);
+    ret |= STATUS_BRK_ERR;
+    mvebu3700_write(uart, UART_STATUS_REG, ret);
+
+    /* Clear error interrupts */
+    mvebu3700_write(uart, UART_CTRL_REG, CTRL_RX_INT);
+
+    /* Disable Rx/Tx interrupts */
+    ret = mvebu3700_read(uart, UART_CTRL_REG);
+    ret &= ~(CTRL_RX_RDY_INT | CTRL_TX_RDY_INT);
+    mvebu3700_write(uart, UART_CTRL_REG, ret);
+}
+
+static void __init mvebu3700_uart_init_postirq(struct serial_port *port)
+{
+    struct mvebu3700_uart *uart = port->uart;
+    int ret;
+
+    if ( uart->irq > 0 )
+    {
+        uart->irqaction.handler = mvebu3700_uart_interrupt;
+        uart->irqaction.name    = "mvebu3700_uart";
+        uart->irqaction.dev_id  = port;
+    }
+
+    if ( (ret = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
+        dprintk(XENLOG_ERR, "Failed to allocated mvebu3700_uart IRQ %d\n",
+                uart->irq);
+
+    /* Make sure Rx/Tx interrupts are enabled now */
+    ret = mvebu3700_read(uart, UART_CTRL_REG);
+    ret |= (CTRL_RX_RDY_INT | CTRL_TX_RDY_INT);
+    mvebu3700_write(uart, UART_CTRL_REG, ret);
+}
+
+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;
+    unsigned int ctl;
+
+    ctl = mvebu3700_read(uart, UART_CTRL_REG);
+    ctl &= ~CTRL_TX_RDY_INT;
+    mvebu3700_write(uart, UART_CTRL_REG, ctl);
+}
+
+static void mvebu3700_uart_start_tx(struct serial_port *port)
+{
+    struct mvebu3700_uart *uart = port->uart;
+    unsigned int ctl;
+
+    ctl = mvebu3700_read(uart, UART_CTRL_REG);
+    ctl |= CTRL_TX_RDY_INT;
+    mvebu3700_write(uart, UART_CTRL_REG, ctl);
+}
+
+static int mvebu3700_uart_tx_ready(struct serial_port *port)
+{
+    struct mvebu3700_uart *uart = port->uart;
+
+    return ( mvebu3700_read(uart, UART_STATUS_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->baud = BAUD_AUTO;
+    uart->data_bits = 8;
+    uart->parity = PARITY_NONE;
+    uart->stop_bits = 1;
+
+    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:
+ */
-- 
1.9.1


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

Re: [RFC PATCH] xen/arm64: Add Support for Marvell ARMADA 3700 SoC

[Julien Grall]

Hi Amit,

On 03/10/2018 04:44 PM, Amit Singh Tomar wrote:
> This patch-set is an attempt to enable XEN on ESPRESSObin[1] based on
> Marvell's ARMADA 3700 SoC

Thank you for adding support to boot Xen on that board!

Would you mind creating a page on Xen wiki to explain how to boot Xen on 
that board? See [1].

Furthermore, we are always looking for user to test Xen RC on supported 
hardware. Would you be willing to step up for that board? If so, can you 
add your name on [2]?

Cheers,

[1] 
https://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions#Hardware

[2] https://wiki.xenproject.org/wiki/Xen_ARM_Manual_Smoke_Test/Results

-- 
Julien Grall

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

Re: [RFC PATCH] xen/arm: Add MVEBU UART driver for Armada 3700 SoC

[Julien Grall]

Hi,

On 03/10/2018 04:44 PM, Amit Singh Tomar wrote:
> This patch adds driver for UART controller found on Armada 3700 SoC.

OOI, do you have any plan for adding earlyprintk support for that platform?

> 
> There is no reference manuals available for 3700 SoC in public and this
> driver is derived by looking at Linux driver.

Please give a link to the Linux driver. This would help me for reviewing 
and also for future reference.

For now, see below for some generic comments regarding the code.

> 
> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
> ---
>   xen/drivers/char/Kconfig      |   8 ++
>   xen/drivers/char/Makefile     |   1 +
>   xen/drivers/char/mvebu-uart.c | 315 ++++++++++++++++++++++++++++++++++++++++++

This is part of xen/drivers/char/* so even if the driver if only for ARM 
hardware, you likely want to CC "THE REST" maintainers as this is under 
drivers/char. scripts/get_maintainers.pl can help you to find relevant 
maintainers to CC on each patch.

Regarding the maintenance after it is merged, I think it should fold 
under "ARM" as for all the other arch specific UART driver (PL011 & co).

>   3 files changed, 324 insertions(+)
>   create mode 100644 xen/drivers/char/mvebu-uart.c
> 
> 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..fdcc153
> --- /dev/null
> +++ b/xen/drivers/char/mvebu-uart.c
> @@ -0,0 +1,315 @@
> +/*
> + * xen/drivers/char/mvebu3700-uart.c
> + *
> + * Driver for Marvell MVEBU UART.
> + *
> + * Amit Singh Tomar<amittomer25@gmail.com>

NIT: space before <.

> + * 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 <asm/device.h>
> +#include <asm/io.h>

<xen/*> include should be first, then <asm/*>.

> +#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/serial.h>

xen/serial.h is mentioned twice.

> +#include <xen/vmap.h>
> +
> +#define UART_RX_REG             0x00
> +#define RBR_BRK_DET             BIT(15)
> +#define RBR_FRM_ERR_DET         BIT(14)
> +#define RBR_PAR_ERR_DET         BIT(13)
> +#define RBR_OVR_ERR_DET         BIT(12)
> +
> +#define UART_TX_REG             0x04
> +
> +#define UART_CTRL_REG           0x08
> +#define CTRL_SOFT_RST           BIT(31)
> +#define CTRL_TXFIFO_RST         BIT(15)
> +#define CTRL_RXFIFO_RST         BIT(14)
> +#define CTRL_ST_MIRR_EN         BIT(13)
> +#define CTRL_LPBK_EN            BIT(12)
> +#define CTRL_SND_BRK_SEQ        BIT(11)
> +#define CTRL_PAR_EN             BIT(10)
> +#define CTRL_TWO_STOP           BIT(9)
> +#define CTRL_TX_HFL_INT         BIT(8)
> +#define CTRL_RX_HFL_INT         BIT(7)
> +#define CTRL_TX_EMP_INT         BIT(6)
> +#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_RXFIFO_EMP       BIT(12)
> +#define STATUS_TXFIFO_FUL       BIT(11)
> +#define STATUS_TXFIFO_HFL       BIT(10)
> +#define STATUS_RX_TOGL          BIT(9)
> +#define STATUS_RXFIFO_FUL       BIT(8)
> +#define STATUS_RXFIFO_HFL       BIT(7)
> +#define STATUS_TX_EMP           BIT(6)
> +#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 UART_BAUD_REG           0x10
> +#define UART_POSSR_REG          0x14

Can you please only define only registers/bits used in the code?

> +
> +#define TX_FIFO_SIZE            32
> +#define RX_FIFO_SIZE            64
> +
> +static struct mvebu3700_uart {
> +    unsigned int baud, data_bits, parity, stop_bits;

Are all those fields necessary? For instance, you always set baud but 
never read it.

> +    unsigned int irq;
> +    void __iomem *regs;
> +    struct irqaction irqaction;
> +    struct vuart_info vuart;
> +} mvebu3700_com = {0};
> +
> +#define PARITY_NONE  (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)

The indentation looks wrong here.

> +{
> +    struct serial_port *port = data;
> +    struct mvebu3700_uart *uart = port->uart;
> +    unsigned int 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;
> +    unsigned ret;

'ret' is a bit confusion. I would expect to be the return value of the 
function but it is used a temporary variable for reading/write reg. You 
might want to rename to 'reg' for more clarity.

But as this is a register value (i.e specific size), please use uint32_t.

> +
> +    ret = mvebu3700_read(uart, UART_CTRL_REG);
> +    ret |= (CTRL_TXFIFO_RST | CTRL_RXFIFO_RST);
> +    mvebu3700_write(uart, UART_CTRL_REG, ret);
> +
> +    /* Before we make IRQ request, Clear the error bits of state register */

s/Clear/clear/ and missing full stop.

> +    ret = mvebu3700_read(uart, UART_STATUS_REG);
> +    ret |= STATUS_BRK_ERR;
> +    mvebu3700_write(uart, UART_STATUS_REG, ret);
> +
> +    /* Clear error interrupts */
> +    mvebu3700_write(uart, UART_CTRL_REG, CTRL_RX_INT);
> +
> +    /* Disable Rx/Tx interrupts */
> +    ret = mvebu3700_read(uart, UART_CTRL_REG);
> +    ret &= ~(CTRL_RX_RDY_INT | CTRL_TX_RDY_INT);
> +    mvebu3700_write(uart, UART_CTRL_REG, ret);
> +}
> +
> +static void __init mvebu3700_uart_init_postirq(struct serial_port *port)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +    int ret;
> +
> +    if ( uart->irq > 0 )
> +    {
> +        uart->irqaction.handler = mvebu3700_uart_interrupt;
> +        uart->irqaction.name    = "mvebu3700_uart";
> +        uart->irqaction.dev_id  = port;
> +    }
> +
> +    if ( (ret = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )

Why do you set 'ret' here?

> +        dprintk(XENLOG_ERR, "Failed to allocated mvebu3700_uart IRQ %d\n",
> +                uart->irq);

dprintk will only be used in debug build. I think this should be printk 
here.

> +
> +    /* Make sure Rx/Tx interrupts are enabled now */
> +    ret = mvebu3700_read(uart, UART_CTRL_REG);

ret is an int. This is usually a pretty bad idea to use signed value for 
register. Furthermore, I would highly recommend to specific the size in 
the variable type (e.g uint32_t).

> +    ret |= (CTRL_RX_RDY_INT | CTRL_TX_RDY_INT);
> +    mvebu3700_write(uart, UART_CTRL_REG, ret);
> +}
> +
> +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;
> +    unsigned int ctl;

s/unsigned int/uint32_t/.

> +
> +    ctl = mvebu3700_read(uart, UART_CTRL_REG);
> +    ctl &= ~CTRL_TX_RDY_INT;
> +    mvebu3700_write(uart, UART_CTRL_REG, ctl);
> +}
> +
> +static void mvebu3700_uart_start_tx(struct serial_port *port)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +    unsigned int ctl;
> +
> +    ctl = mvebu3700_read(uart, UART_CTRL_REG);
> +    ctl |= CTRL_TX_RDY_INT;
> +    mvebu3700_write(uart, UART_CTRL_REG, ctl);
> +}
> +
> +static int mvebu3700_uart_tx_ready(struct serial_port *port)
> +{
> +    struct mvebu3700_uart *uart = port->uart;
> +
> +    return ( mvebu3700_read(uart, UART_STATUS_REG) & STATUS_TXFIFO_EMP ?
> +             TX_FIFO_SIZE : 0 );

This is not so nice to read. Can you introduce a temporary variable to 
read the register?

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

Please don't split message (unless there are a newline in it). This is 
more difficult to grep in the code. This is one place where we accept 
line greater than 80 characters.

> +        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->baud = BAUD_AUTO;
> +    uart->data_bits = 8;
> +    uart->parity = PARITY_NONE;
> +    uart->stop_bits = 1;
> +
> +    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:
> + */
> 

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH] xen/arm64: Add Support for Marvell ARMADA 3700 SoC

[Amit Tomer]

Hi,

Thanks for looking into.

> Would you mind creating a page on Xen wiki to explain how to boot Xen on
> that board? See [1].

Sure, I would do it and it was on my TODO list as well.

Thanks
Amit.

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

Re: [RFC PATCH] xen/arm: Add MVEBU UART driver for Armada 3700 SoC

[Amit Tomer]

Hi

Thanks for the comments.

> OOI, do you have any plan for adding earlyprintk support for that platform?

I didn't think about it but I would look into it.

> Please give a link to the Linux driver. This would help me for reviewing and
> also for future reference.

Ok.

> This is part of xen/drivers/char/* so even if the driver if only for ARM
> hardware, you likely want to CC "THE REST" maintainers as this is under
> drivers/char. scripts/get_maintainers.pl can help you to find relevant
> maintainers to CC on each patch.

Ok.

 <xen/*> include should be first, then <asm/*>.

Ok, I was under the impression that it should be sorted in alphabetical order.

>
>> +#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/serial.h>
>
>
> xen/serial.h is mentioned twice.
Ok.

>
>> +#include <xen/vmap.h>
>> +
>> +#define UART_RX_REG             0x00
>> +#define RBR_BRK_DET             BIT(15)
>> +#define RBR_FRM_ERR_DET         BIT(14)
>> +#define RBR_PAR_ERR_DET         BIT(13)
>> +#define RBR_OVR_ERR_DET         BIT(12)
>> +
>> +#define UART_TX_REG             0x04
>> +
>> +#define UART_CTRL_REG           0x08
>> +#define CTRL_SOFT_RST           BIT(31)
>> +#define CTRL_TXFIFO_RST         BIT(15)
>> +#define CTRL_RXFIFO_RST         BIT(14)
>> +#define CTRL_ST_MIRR_EN         BIT(13)
>> +#define CTRL_LPBK_EN            BIT(12)
>> +#define CTRL_SND_BRK_SEQ        BIT(11)
>> +#define CTRL_PAR_EN             BIT(10)
>> +#define CTRL_TWO_STOP           BIT(9)
>> +#define CTRL_TX_HFL_INT         BIT(8)
>> +#define CTRL_RX_HFL_INT         BIT(7)
>> +#define CTRL_TX_EMP_INT         BIT(6)
>> +#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_RXFIFO_EMP       BIT(12)
>> +#define STATUS_TXFIFO_FUL       BIT(11)
>> +#define STATUS_TXFIFO_HFL       BIT(10)
>> +#define STATUS_RX_TOGL          BIT(9)
>> +#define STATUS_RXFIFO_FUL       BIT(8)
>> +#define STATUS_RXFIFO_HFL       BIT(7)
>> +#define STATUS_TX_EMP           BIT(6)
>> +#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 UART_BAUD_REG           0x10
>> +#define UART_POSSR_REG          0x14
>
>
> Can you please only define only registers/bits used in the code?

Ok.

>> +
>> +#define TX_FIFO_SIZE            32
>> +#define RX_FIFO_SIZE            64
>> +
>> +static struct mvebu3700_uart {
>> +    unsigned int baud, data_bits, parity, stop_bits;
>
>
> Are all those fields necessary? For instance, you always set baud but never
> read it.

Not sure about this as I don't know if these fields are used by XEN
serial infrastructure later on.

>> +    unsigned int irq;
>> +    void __iomem *regs;
>> +    struct irqaction irqaction;
>> +    struct vuart_info vuart;
>> +} mvebu3700_com = {0};
>> +
>> +#define PARITY_NONE  (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)
>
>
> The indentation looks wrong here.
>
>> +{
>> +    struct serial_port *port = data;
>> +    struct mvebu3700_uart *uart = port->uart;
>> +    unsigned int 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;
>> +    unsigned ret;
>
>
> 'ret' is a bit confusion. I would expect to be the return value of the
> function but it is used a temporary variable for reading/write reg. You
> might want to rename to 'reg' for more clarity.
Ok.

> But as this is a register value (i.e specific size), please use uint32_t.
>
>> +
>> +    ret = mvebu3700_read(uart, UART_CTRL_REG);
>> +    ret |= (CTRL_TXFIFO_RST | CTRL_RXFIFO_RST);
>> +    mvebu3700_write(uart, UART_CTRL_REG, ret);
>> +
>> +    /* Before we make IRQ request, Clear the error bits of state register
>> */
>
>
> s/Clear/clear/ and missing full stop.

Ok.
>
>
>> +    ret = mvebu3700_read(uart, UART_STATUS_REG);
>> +    ret |= STATUS_BRK_ERR;
>> +    mvebu3700_write(uart, UART_STATUS_REG, ret);
>> +
>> +    /* Clear error interrupts */
>> +    mvebu3700_write(uart, UART_CTRL_REG, CTRL_RX_INT);
>> +
>> +    /* Disable Rx/Tx interrupts */
>> +    ret = mvebu3700_read(uart, UART_CTRL_REG);
>> +    ret &= ~(CTRL_RX_RDY_INT | CTRL_TX_RDY_INT);
>> +    mvebu3700_write(uart, UART_CTRL_REG, ret);
>> +}
>> +
>> +        dprintk(XENLOG_ERR, "Failed to allocated mvebu3700_uart IRQ
>> %d\n",
>> +                uart->irq);
>
>
> dprintk will only be used in debug build. I think this should be printk
> here.

Ok.

>> +
>> +    /* Make sure Rx/Tx interrupts are enabled now */
>> +    ret = mvebu3700_read(uart, UART_CTRL_REG);
>
>
> ret is an int. This is usually a pretty bad idea to use signed value for
> register. Furthermore, I would highly recommend to specific the size in the
> variable type (e.g uint32_t).

Ok.
>
>> +    ret |= (CTRL_RX_RDY_INT | CTRL_TX_RDY_INT);
>> +    mvebu3700_write(uart, UART_CTRL_REG, ret);
>> +}
>> +
>> +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;
>> +    unsigned int ctl;
>
>
> s/unsigned int/uint32_t/.

Ok.
>
>> +
>> +    ctl = mvebu3700_read(uart, UART_CTRL_REG);
>> +    ctl &= ~CTRL_TX_RDY_INT;
>> +    mvebu3700_write(uart, UART_CTRL_REG, ctl);
>> +}
>> +
>> +static void mvebu3700_uart_start_tx(struct serial_port *port)
>> +{
>> +    struct mvebu3700_uart *uart = port->uart;
>> +    unsigned int ctl;
>> +
>> +    ctl = mvebu3700_read(uart, UART_CTRL_REG);
>> +    ctl |= CTRL_TX_RDY_INT;
>> +    mvebu3700_write(uart, UART_CTRL_REG, ctl);
>> +}
>> +
>> +static int mvebu3700_uart_tx_ready(struct serial_port *port)
>> +{
>> +    struct mvebu3700_uart *uart = port->uart;
>> +
>> +    return ( mvebu3700_read(uart, UART_STATUS_REG) & STATUS_TXFIFO_EMP ?
>> +             TX_FIFO_SIZE : 0 );
>
>
> This is not so nice to read. Can you introduce a temporary variable to read
> the register?

Ok.

>> +    {
>> +        printk("mvebu3700: Unable to retrieve the base"
>> +               " address of the UART\n");
>
>
> Please don't split message (unless there are a newline in it). This is more
> difficult to grep in the code. This is one place where we accept line
> greater than 80 characters.

Ok.

Thanks
Amit

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

Re: [RFC PATCH] xen/arm: Add MVEBU UART driver for Armada 3700 SoC

[Julien Grall]

On 12/03/18 14:36, Amit Tomer wrote:
> Hi

Hi Amit,

> 
> Thanks for the comments.
> 
>> OOI, do you have any plan for adding earlyprintk support for that platform?
> 
> I didn't think about it but I would look into it.

This is quite useful to get output without any serial driver. I am quite 
impressed you managed to debug your serial driver without it :).

> 
>> Please give a link to the Linux driver. This would help me for reviewing and
>> also for future reference.
> 
> Ok.
> 
>> This is part of xen/drivers/char/* so even if the driver if only for ARM
>> hardware, you likely want to CC "THE REST" maintainers as this is under
>> drivers/char. scripts/get_maintainers.pl can help you to find relevant
>> maintainers to CC on each patch.
> 
> Ok.
> 
>   <xen/*> include should be first, then <asm/*>.
> 
> Ok, I was under the impression that it should be sorted in alphabetical order.

They should be sorted alphabetical, but all <asm/*> should be after 
<xen/*> so common headers gets included first, then the arch specific ones.
>>> +
>>> +#define TX_FIFO_SIZE            32
>>> +#define RX_FIFO_SIZE            64
>>> +
>>> +static struct mvebu3700_uart {
>>> +    unsigned int baud, data_bits, parity, stop_bits;
>>
>>
>> Are all those fields necessary? For instance, you always set baud but never
>> read it.
> 
> Not sure about this as I don't know if these fields are used by XEN
> serial infrastructure later on.

This is an internal structure. I can't see how the serial code would 
know the layout and access the fields.

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH] xen/arm: Add MVEBU UART driver for Armada 3700 SoC

[Amit Tomer]

Hi,

> This is quite useful to get output without any serial driver. I am quite
> impressed you managed to debug your serial driver without it :).

Actually,  we have earlycon=xenboot(suggested by Andre) enabled in
Dom0 bootargs and it allowed us to
debug XEN boot further.

I am wondering if ealycon interface can be used in absence of
earlyprintk doing same work?

Thanks
Amit.

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

Re: [RFC PATCH] xen/arm: Add MVEBU UART driver for Armada 3700 SoC

[Julien Grall]

On 12/03/18 17:33, Amit Tomer wrote:
> Hi,

Hi,

> 
>> This is quite useful to get output without any serial driver. I am quite
>> impressed you managed to debug your serial driver without it :).
> 
> Actually,  we have earlycon=xenboot(suggested by Andre) enabled in
> Dom0 bootargs and it allowed us to
> debug XEN boot further.
> 
> I am wondering if ealycon interface can be used in absence of > earlyprintk doing same work?

earlycon=xenboot enables the early console for the hardware domain only. 
What I meant is having earlyprintk for Xen (see CONFIG_EARLY_PRINTK). 
This is used for low-level debug when booting the hypervisor.

Cheers,

-- 
Julien Grall

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

Re: [RFC PATCH] xen/arm: Add MVEBU UART driver for Armada 3700 SoC

[Amit Tomer]

> earlycon=xenboot enables the early console for the hardware domain only.
> What I meant is having earlyprintk for Xen (see CONFIG_EARLY_PRINTK). This
> is used for low-level debug when booting the hypervisor.

Ok, Thanks. It's now clear to me.

Thanks
Amit

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