[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 1/2] xen/arm: Add MVEBU UART driver for Marvell Armada 3700 SoC



(+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 <amittomer25@xxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.