Re: [PATCH 2/7] serial: bflb_uart: add Bouffalolab UART Driver

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

 



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@xxxxxxxxxx>
...

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




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux