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

2018-03-17 Thread 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

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

2018-03-13 Thread 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: [Xen-devel] [RFC PATCH] xen/arm: Add MVEBU UART driver for Armada 3700 SoC

2018-03-12 Thread 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: [Xen-devel] [RFC PATCH] xen/arm: Add MVEBU UART driver for Armada 3700 SoC

2018-03-12 Thread 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.

   include should be first, then .

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


They should be sorted alphabetical, but all  should be after 
 so common headers gets included first, then the arch specific ones.

+
+#define TX_FIFO_SIZE32
+#define RX_FIFO_SIZE64
+
+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: [Xen-devel] [RFC PATCH] xen/arm: Add MVEBU UART driver for Armada 3700 SoC

2018-03-12 Thread 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.

  include should be first, then .

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

>
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>
>
> xen/serial.h is mentioned twice.
Ok.

>
>> +#include 
>> +
>> +#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_ENBIT(12)
>> +#define CTRL_SND_BRK_SEQBIT(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_INTBIT(3)
>> +#define CTRL_FRM_ERR_INTBIT(2)
>> +#define CTRL_PAR_ERR_INTBIT(1)
>> +#define CTRL_OVR_ERR_INTBIT(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_SIZE32
>> +#define RX_FIFO_SIZE64
>> +
>> +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 =

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

2018-03-11 Thread 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 
---
  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 000..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


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 
+#include 


 include should be first, then .


+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 


xen/serial.h is mentioned twice.


+#include 
+
+#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_ENBIT(12)
+#define CTRL_SND_BRK_SEQBIT(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_INTBIT(3)
+#define CTRL_FRM_ERR_INTBIT(2)
+#define CTRL_PAR_ERR_INTBIT(1)
+#define CTRL_OVR_ERR_INTBIT(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