lkml.org 
[lkml]   [2022]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/7] serial: bflb_uart: add Bouffalolab UART Driver
    Hi,

    On 20. 11. 22, 9:21, Jisheng Zhang wrote:
    > Add the driver for Bouffalolab UART IP which is found in Bouffalolab
    > SoCs such as bl808.
    >
    > UART driver probe will create path named "/dev/ttySx".
    >
    > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
    ...

    > #define UART_FIFO_CONFIG_0 (0x80)

    Superfluous parentheses.

    ...
    > +static void bflb_uart_set_termios(struct uart_port *port,
    > + struct ktermios *termios,
    > + const struct ktermios *old)
    > +{
    > + unsigned long flags;
    > + u32 valt, valr, val;
    > + unsigned int baud, min;
    > +
    > + valt = valr = 0;

    Unneeded (see below).

    > +
    > + spin_lock_irqsave(&port->lock, flags);
    > +
    > + /* set data length */
    > + val = tty_get_char_size(termios->c_cflag) - 1;
    > + valt |= (val << UART_CR_UTX_BIT_CNT_D_SFT);

    Use =, not |=. Other than that, can FIELD_SET() be used, provided you
    already define the constants using GENMASK()?

    > +
    > + /* calculate parity */
    > + termios->c_cflag &= ~CMSPAR; /* no support mark/space */
    > + if (termios->c_cflag & PARENB) {
    > + valt |= UART_CR_UTX_PRT_EN;
    > + if (termios->c_cflag & PARODD)
    > + valr |= UART_CR_UTX_PRT_SEL;

    This should be valt, IMO.

    > + }
    > +
    > + valr = valt;

    If not, this doesn't make sense to me.

    > + /* calculate stop bits */
    > + if (termios->c_cflag & CSTOPB)
    > + val = 2;
    > + else
    > + val = 1;
    > + valt |= (val << UART_CR_UTX_BIT_CNT_P_SFT);
    > +
    > + /* flow control */
    > + if (termios->c_cflag & CRTSCTS)
    > + valt |= UART_CR_UTX_CTS_EN;
    > +
    > + /* enable TX freerunning mode */
    > + valt |= UART_CR_UTX_FRM_EN;
    > +
    > + valt |= UART_CR_UTX_EN;
    > + valr |= UART_CR_URX_EN;

    Why this is not the very first and only for valt and copied to valr above?

    > +
    > + wrl(port, UART_UTX_CONFIG, valt);
    > + wrl(port, UART_URX_CONFIG, valr);
    > +
    > + min = port->uartclk / (UART_CR_UTX_BIT_PRD + 1);
    > + baud = uart_get_baud_rate(port, termios, old, min, 4000000);
    > +
    > + val = DIV_ROUND_CLOSEST(port->uartclk, baud) - 1;
    > + val &= UART_CR_UTX_BIT_PRD;
    > + val |= (val << 16);
    > + wrl(port, UART_BIT_PRD, val);
    > +
    > + uart_update_timeout(port, termios->c_cflag, baud);
    > +
    > + spin_unlock_irqrestore(&port->lock, flags);
    > +}
    > +
    > +static void bflb_uart_rx_chars(struct uart_port *port)
    > +{
    > + unsigned char ch, flag;

    Please use u8. (serial_core should too, but that's only on a TODO list yet)

    > + unsigned long status;

    Long? I think u32.

    > +
    > + while ((status = rdl(port, UART_FIFO_CONFIG_1)) & UART_RX_FIFO_CNT_MSK) {
    > + ch = rdl(port, UART_FIFO_RDATA) & UART_FIFO_RDATA_MSK;
    > + flag = TTY_NORMAL;

    Drop this flag completely and use the constant below directly.

    > + port->icount.rx++;
    > +
    > + if (uart_handle_sysrq_char(port, ch))
    > + continue;
    > + uart_insert_char(port, 0, 0, ch, flag);
    > + }
    > +
    > + spin_unlock(&port->lock);
    > + tty_flip_buffer_push(&port->state->port);
    > + spin_lock(&port->lock);
    > +}
    > +
    > +static void bflb_uart_tx_chars(struct uart_port *port)
    > +{

    Are you unable to use the TX helper? If so:
    * why?
    * use uart_advance_xmit() at least.

    > + struct circ_buf *xmit = &port->state->xmit;
    > + unsigned int pending, count;
    > +
    > + if (port->x_char) {
    > + /* Send special char - probably flow control */
    > + wrl(port, UART_FIFO_WDATA, port->x_char);
    > + port->x_char = 0;
    > + port->icount.tx++;
    > + return;
    > + }
    > +
    > + pending = uart_circ_chars_pending(xmit);
    > + if (pending > 0) {
    > + count = (rdl(port, UART_FIFO_CONFIG_1) &
    > + UART_TX_FIFO_CNT_MSK) >> UART_TX_FIFO_CNT_SFT;
    > + if (count > pending)
    > + count = pending;
    > + if (count > 0) {
    > + pending -= count;
    > + while (count--) {
    > + wrl(port, UART_FIFO_WDATA, xmit->buf[xmit->tail]);
    > + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
    > + port->icount.tx++;
    > + }
    > + if (pending < WAKEUP_CHARS)
    > + uart_write_wakeup(port);
    > + }
    > + }
    > +
    > + if (pending == 0)
    > + bflb_uart_stop_tx(port);
    > +}
    > +
    > +static irqreturn_t bflb_uart_interrupt(int irq, void *data)
    > +{
    > + struct uart_port *port = data;
    > + u32 isr, val;
    > +
    > + isr = rdl(port, UART_INT_STS);
    > + wrl(port, UART_INT_CLEAR, isr);
    > +
    > + isr &= ~rdl(port, UART_INT_MASK);
    > +
    > + spin_lock(&port->lock);
    > +
    > + if (isr & UART_URX_FER_INT) {
    > + /* RX FIFO error interrupt */
    > + val = rdl(port, UART_FIFO_CONFIG_0);
    > + if (val & UART_RX_FIFO_OVERFLOW)
    > + port->icount.overrun++;
    > +
    > + val |= UART_RX_FIFO_CLR;
    > + wrl(port, UART_FIFO_CONFIG_0, val);
    > + }
    > +
    > + if (isr & (UART_URX_FIFO_INT | UART_URX_RTO_INT)) {
    > + bflb_uart_rx_chars(port);
    > + }
    > + if (isr & (UART_UTX_FIFO_INT | UART_UTX_END_INT)) {
    > + bflb_uart_tx_chars(port);
    > + }

    Superfluous braces.

    > +
    > + spin_unlock(&port->lock);
    > +
    > + return IRQ_RETVAL(isr);

    Can it happen that UART_INT_STS is nonzero and UART_INT_MASK is zero?
    You'd cause "irqX: nobody cared" in that case.

    > +}
    > +
    > +static void bflb_uart_config_port(struct uart_port *port, int flags)
    > +{
    > + u32 val;
    > +
    > + port->type = PORT_BFLB;
    > +
    > + /* Clear mask, so no surprise interrupts. */

    surprising?

    > + val = rdl(port, UART_INT_MASK);
    > + val |= UART_UTX_END_INT;
    > + val |= UART_UTX_FIFO_INT;
    > + val |= UART_URX_FIFO_INT;
    > + val |= UART_URX_RTO_INT;
    > + val |= UART_URX_FER_INT;
    > + wrl(port, UART_INT_MASK, val);
    > +}
    > +
    > +static int bflb_uart_startup(struct uart_port *port)
    > +{
    > + unsigned long flags;
    > + int ret;
    > + u32 val;
    > +
    > + ret = devm_request_irq(port->dev, port->irq, bflb_uart_interrupt,
    > + IRQF_SHARED, port->name, port);
    > + if (ret) {
    > + dev_err(port->dev, "fail to request serial irq %d, ret=%d\n",
    > + port->irq, ret);
    > + return ret;
    > + }
    > +
    > + spin_lock_irqsave(&port->lock, flags);
    > +
    > + val = rdl(port, UART_INT_MASK);
    > + val |= 0xfff;

    Can this be a defined macro too, please?

    > + wrl(port, UART_INT_MASK, val);
    > +
    > + wrl(port, UART_DATA_CONFIG, 0);
    > + wrl(port, UART_SW_MODE, 0);
    > + wrl(port, UART_URX_RTO_TIMER, 0x4f);

    Another magic constant.

    > +
    > + val = rdl(port, UART_FIFO_CONFIG_1);
    > + val &= ~UART_RX_FIFO_TH_MSK;
    > + val |= BFLB_UART_RX_FIFO_TH << UART_RX_FIFO_TH_SFT;

    FIELD_SET()?

    > + wrl(port, UART_FIFO_CONFIG_1, val);
    > +
    > + /* Unmask RX interrupts now */
    > + val = rdl(port, UART_INT_MASK);
    > + val &= ~UART_URX_FIFO_INT;
    > + val &= ~UART_URX_RTO_INT;
    > + val &= ~UART_URX_FER_INT;
    > + wrl(port, UART_INT_MASK, val);
    > + val = rdl(port, UART_UTX_CONFIG);
    > + val |= UART_CR_UTX_EN;
    > + wrl(port, UART_UTX_CONFIG, val);
    > + val = rdl(port, UART_URX_CONFIG);
    > + val |= UART_CR_URX_EN;
    > + wrl(port, UART_URX_CONFIG, val);
    > +
    > + spin_unlock_irqrestore(&port->lock, flags);
    > +
    > + return 0;
    > +}
    ...
    > +/*
    > + * Interrupts are disabled on entering
    > + */
    > +static void bflb_uart_console_write(struct console *co, const char *s,
    > + u_int count)
    > +{
    > + struct uart_port *port = &bflb_uart_ports[co->index]->port;
    > + u32 status, reg, mask;
    > +
    > + /* save then disable interrupts */
    > + mask = rdl(port, UART_INT_MASK);
    > + reg = -1;

    You use 0xfff earlier, now 0xffffffff. Is that OK? Why not use the same
    constant?

    > + wrl(port, UART_INT_MASK, reg);
    > +
    > + /* Make sure that tx is enabled */
    > + reg = rdl(port, UART_UTX_CONFIG);
    > + reg |= UART_CR_UTX_EN;
    > + wrl(port, UART_UTX_CONFIG, reg);
    > +
    > + uart_console_write(port, s, count, bflb_console_putchar);
    > +
    > + /* wait for TX done */
    > + do {
    > + status = rdl(port, UART_STATUS);
    > + } while ((status & UART_STS_UTX_BUS_BUSY));
    > +
    > + /* restore IRQ mask */
    > + wrl(port, UART_INT_MASK, mask);
    > +}

    regards,
    --
    js
    suse labs

    \
     
     \ /
      Last update: 2022-11-21 09:10    [W:2.228 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site