(+Juergen)
Hi,
On 04/05/2018 11:16 AM, Amit Singh Tomar wrote:
This patch adds driver for UART controller found on Armada 3700 SoC.
There is no reference manuals available for 3700 SoC in public and it
is derived by looking at Linux driver[1].
[1]https://github.com/torvalds/linux/blob/master/drivers/tty/serial/mvebu-uart.c
Signed-off-by: Amit Singh Tomar <amittome...@gmail.com>
---
Changes since v2:
* Addressed Andre's comments.
Changes since v1:
* Addressed Wei Liu's comments
* Addressed Andre's comments.
Changes since RFC:
* Addressed Julien's comments.
---
xen/drivers/char/Kconfig | 8 ++
xen/drivers/char/Makefile | 1 +
xen/drivers/char/mvebu-uart.c | 296 ++++++++++++++++++++++++++++++++++++++++++
As I said on the first version, I think this should fold under "ARM" as
all the arch specific UART driver (PL011 & co).
I was expecting this patch to modify MAINTAINERS. I would be ok to
commit like that but please send a patch to address this issue as soon
as possible.
[...]
+static void __init mvebu3700_uart_init_postirq(struct serial_port *port)
+{
+ struct mvebu3700_uart *uart = port->uart;
+ uint32_t reg;
+
+ if ( uart->irq > 0 )
+ {
+ uart->irqaction.handler = mvebu3700_uart_interrupt;
+ uart->irqaction.name = "mvebu3700_uart";
+ uart->irqaction.dev_id = port;
+ }
+
+ if ( setup_irq(uart->irq, 0, &uart->irqaction) != 0 )
+ {
+ printk("Failed to allocated mvebu3700_uart IRQ %d\n", uart->irq);
+ return;
+ }
This code looks wrong. You only setup the action when uart->irq is > 0,
but you unconditionally call setup_irq.
Other drivers are calling setup_irq(...) when uart->irq > 0.
However, this driver seems to assume the interrupt will always be there
as you enable the interrupt below and the initialized would bail out if
platform_irq() fails.
So I think you want to drop uart->irq.
[...]
+static int __init mvebu3700_irq(struct serial_port *port)
+{
+ struct mvebu3700_uart *uart = port->uart;
+
+ return (uart->irq > 0) ? uart->irq : -1;
Same remark here.
[...]
+static int mvebu3700_uart_tx_ready(struct serial_port *port)
+{
+ struct mvebu3700_uart *uart = port->uart;
+ uint32_t reg;
+
+ reg = mvebu3700_read(uart, UART_STATUS_REG);
+
+ if ( reg & STATUS_TXFIFO_EMP )
+ return TX_FIFO_SIZE;
+ if ( reg & STAT_TX_FIFO_FUL )
+ return 0;
+ if ( reg & STAT_TX_FIFO_HFL )
+ return TX_FIFO_SIZE / 2;
+
+ /* if we reach here, we don't know the number of free char in FIFO
Coding style:
/*
*
*/
The rest of the code looks good. Please resend the patch with that
addressed.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
