|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH] xen/arm: Add MVEBU UART driver for Armada 3700 SoC
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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |