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