Re: [PATCH v2 08/10] serial: ambarella: add Ambarella UART driver
From: Jiri Slaby
Date: Fri Aug 07 2026 - 02:19:13 EST
Hi,
On 06. 08. 26, 11:34, Long Zhao via B4 Relay wrote:
Add an Ambarella UART driver with console support for early boot...
bring-up on CV75. Keep udelay() in wait_for_tx(); it runs under
console/poll paths that may hold the port lock with IRQs disabled.
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index bba7b21a4a1d..5c951719528b 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -26,6 +26,7 @@ obj-y += 8250/
obj-$(CONFIG_SERIAL_ALTERA_JTAGUART) += altera_jtaguart.o
obj-$(CONFIG_SERIAL_ALTERA_UART) += altera_uart.o
+obj-$(CONFIG_SERIAL_AMBARELLA) += ambarella_uart.o
I would put it after AMBA_*. That should be also the ascii order, right?
obj-$(CONFIG_SERIAL_AMBA_PL010) += amba-pl010.o
obj-$(CONFIG_SERIAL_AMBA_PL011) += amba-pl011.o
obj-$(CONFIG_SERIAL_GRLIB_GAISLER_APBUART) += apbuart.o
diff --git a/drivers/tty/serial/ambarella_uart.c b/drivers/tty/serial/ambarella_uart.c
new file mode 100644
index 000000000000..7356b242f0ef
--- /dev/null
+++ b/drivers/tty/serial/ambarella_uart.c
@@ -0,0 +1,1001 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/clk.h>
+#include <linux/console.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/serial_reg.h>
+#include <linux/serial_core.h>
+#include <linux/sysrq.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+
+#define UART_RB_OFFSET 0x00
+#define UART_TH_OFFSET 0x00
+#define UART_DLL_OFFSET 0x00
+#define UART_IE_OFFSET 0x04
+#define UART_DLH_OFFSET 0x04
+#define UART_II_OFFSET 0x08
+#define UART_FC_OFFSET 0x08
+#define UART_LC_OFFSET 0x0c
+#define UART_MC_OFFSET 0x10
+#define UART_LS_OFFSET 0x14
+#define UART_MS_OFFSET 0x18
+#define UART_US_OFFSET 0x7c
+#define UART_SRR_OFFSET 0x88
For me, this would be more understadable:
#define UART_IE 0x4
# define UART_IE_ERBFI BIT(0)
...
# define UART_IE_ETOI BIT(6)
# define UART_IE_ERETOI BIT(7)
and so on.
That is:
* define the bits near the offset
* no _OFFSET suffix
* use BIT()
...
+#define UART_FIFO_SIZE 64
+
+#define DEFAULT_AMBARELLA_UART_MCR 0
+#define DEFAULT_AMBARELLA_UART_IER (UART_IE_ELSI | UART_IE_ERBFI | \
+ UART_IE_ETOI)
+
+#define AMBA_UART_MAX_NUM 8
+
+#define AMBA_UART_RESET_FLAG 0 /* bit 0 */
Perhaps convert this one to an enum?
+/* Poll timeout in microseconds (atomic helpers use udelay). */
+#define AMBARELLA_UART_TIMEOUT_US 1000000
USEC_PER_SEC
+static inline void wait_for_tx(struct uart_port *port)
+{
+ u32 ls;
+ int ret;
+
+ ret = readl_poll_timeout_atomic(port->membase + UART_LS_OFFSET, ls,
+ ls & UART_LS_TEMT, 1,
+ AMBARELLA_UART_TIMEOUT_US);
+ if (likely(!ret))
How did you measure this "likely" matters?
+ return;...
+
+ /* Recover a stuck TX path so console/poll can continue. */
+ writel_relaxed(UART_FC_RX_2_TO_FULL | UART_FC_TX_EMPTY |
+ UART_FC_XMITR | UART_FC_RCVRR,
+ port->membase + UART_FC_OFFSET);
+ udelay(100);
+ writel_relaxed(UART_FC_FIFOE | UART_FC_RX_2_TO_FULL |
+ UART_FC_TX_EMPTY | UART_FC_XMITR |
+ UART_FC_RCVRR,
+ port->membase + UART_FC_OFFSET);
+}
+static void serial_ambarella_hw_setup(struct uart_port *port)
+{
+ struct ambarella_uart_port *amb_port = to_ambarella_uart_port(port);
+
+ if (!test_and_set_bit(AMBA_UART_RESET_FLAG, &amb_port->flags)) {
+ if (amb_port->uart_pll)
+ port->uartclk = clk_get_rate(amb_port->uart_pll);
+ /* reset the whole UART only once */
+ writel_relaxed(0x01, port->membase + UART_SRR_OFFSET);
Could you document also this bit in SRR (by a macro)?
+ mdelay(1);
1 ms of spinning? That's very bad. Why this cannot be a sleep instead?
+ writel_relaxed(0x00, port->membase + UART_SRR_OFFSET);...> +static void serial_ambarella_transmit_chars(struct uart_port *port)
+ }
+
+ writel_relaxed(UART_FC_FIFOE | UART_FC_RX_2_TO_FULL | UART_FC_TX_EMPTY |
+ UART_FC_XMITR | UART_FC_RCVRR, port->membase + UART_FC_OFFSET);
+ /* Keep interrupts disabled until the IRQ handler is registered. */
+ serial_ambarella_ier_write(port, 0);
+}
Any reason not to use uart_port_tx_limited()?
+{
+ struct tty_port *tport = &port->state->port;
+ int count;
+
+ if (port->x_char) {
+ writel_relaxed(port->x_char, port->membase + UART_TH_OFFSET);
+ port->icount.tx++;
+ port->x_char = 0;
+ return;
+ }
+
+ if (uart_tx_stopped(port) || kfifo_is_empty(&tport->xmit_fifo)) {
+ __serial_ambarella_stop_tx(port);
+ return;
+ }
+
+ count = port->fifosize;
+ while (count-- > 0) {
+ unsigned char c;
+
+ if (tx_fifo_is_full(port))
+ break;
+
+ if (!kfifo_peek(&tport->xmit_fifo, &c))
+ break;
+
+ writel_relaxed(c, port->membase + UART_TH_OFFSET);
+ kfifo_skip(&tport->xmit_fifo);
+ port->icount.tx++;
+ if (kfifo_is_empty(&tport->xmit_fifo))
+ break;
+ }
+
+ if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
+ uart_write_wakeup(port);
+ if (kfifo_is_empty(&tport->xmit_fifo))
+ __serial_ambarella_stop_tx(port);
+}
+
+static inline void serial_ambarella_check_modem_status(struct uart_port *port)
+{
+ u32 ms;
+
+ ms = __serial_ambarella_read_ms(port);
+
+ if (ms & UART_MS_RI)
+ port->icount.rng++;
+ if (ms & UART_MS_DSR)
+ port->icount.dsr++;
+ if (ms & UART_MS_DCTS)
+ uart_handle_cts_change(port, (ms & UART_MS_CTS));
+ if (ms & UART_MS_DDCD)
+ uart_handle_dcd_change(port, (ms & UART_MS_DCD));
+
+ wake_up_interruptible(&port->state->port.delta_msr_wait);
+}
+
+static irqreturn_t serial_ambarella_irq(int irq, void *dev_id)
+{
+ struct uart_port *port = dev_id;
+ u32 ii;
+
+ scoped_guard(uart_port_lock_irqsave, port) {
This needs not to be scoped. Just guard().
+ ii = readl_relaxed(port->membase + UART_II_OFFSET);
+ switch (ii & 0x0F) {
+ case UART_II_MODEM_STATUS_CHANGED:
+ serial_ambarella_check_modem_status(port);
+ break;
+ case UART_II_THR_EMPTY:
+ serial_ambarella_transmit_chars(port);
+ break;
+ case UART_II_RCV_STATUS:
+ case UART_II_RCV_DATA_AVAIL:
+ serial_ambarella_receive_chars(port, 0);
+ break;
+ case UART_II_CHAR_TIMEOUT_FIFO_EMPTY:
+ /* Clear ERETOI to dismiss timeout-with-empty-FIFO IRQ */
+ serial_ambarella_ier_toggle(port, UART_IE_ERETOI);
+ fallthrough;
+ case UART_II_CHAR_TIMEOUT:
+ serial_ambarella_receive_chars(port, 1);
Hmm, the last param looks like a bool. Why do you have tmo declared as u32?
+ break;
+ case UART_II_NO_INT_PENDING:
+ break;
+ default:
+ pr_debug("%s: 0x%x\n", __func__, ii);
Are you sure you want to dump the unknown irq for every interrupt (on DEBUG)?
+ break;...> +static unsigned int serial_ambarella_tx_empty(struct uart_port *port)
+ }
+ }
+
+ return IRQ_HANDLED;
+}
+{
+ unsigned int lsr;
+
+ guard(uart_port_lock_irqsave)(port);
+ lsr = readl_relaxed(port->membase + UART_LS_OFFSET);
u32 lsr = ...
No need for the previous declaration.
+...
+ return ((lsr & (UART_LS_TEMT | UART_LS_THRE)) ==
+ (UART_LS_TEMT | UART_LS_THRE)) ? TIOCSER_TEMT : 0;
+}
+static void serial_ambarella_set_termios(struct uart_port *port,
+ struct ktermios *termios,
+ const struct ktermios *old)
+{
+ struct ambarella_uart_port *amb_port = to_ambarella_uart_port(port);
+ unsigned int baud, quot;
+ u32 lc = 0x0;
+
+ port->uartclk = clk_get_rate(amb_port->uart_pll);
+ switch (termios->c_cflag & CSIZE) {
+ case CS5:
+ lc |= UART_LC_CLS_5_BITS;
+ break;
+ case CS6:
+ lc |= UART_LC_CLS_6_BITS;
+ break;
+ case CS7:
+ lc |= UART_LC_CLS_7_BITS;
+ break;
+ case CS8:
+ default:
+ lc |= UART_LC_CLS_8_BITS;
+ break;
+ }
+
+ if (termios->c_cflag & CSTOPB)
+ lc |= UART_LC_STOP_2BIT;
+ else
+ lc |= UART_LC_STOP_1BIT;
+
+ if (termios->c_cflag & PARENB) {
+ if (termios->c_cflag & PARODD)
+ lc |= (UART_LC_PEN | UART_LC_ODD_PARITY);
+ else
+ lc |= (UART_LC_PEN | UART_LC_EVEN_PARITY);
+ }
+
+ baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk / 16);
+ quot = uart_get_divisor(port, baud);
+
+ scoped_guard(uart_port_lock_irqsave, port) {
No need for scoped.
+ uart_update_timeout(port, termios->c_cflag, baud);
+
+ port->read_status_mask = UART_LSR_OE | UART_LSR_THRE | UART_LSR_DR;
+ if (termios->c_iflag & INPCK)
+ port->read_status_mask |= UART_LSR_FE | UART_LSR_PE;
+ if (termios->c_iflag & (BRKINT | PARMRK))
+ port->read_status_mask |= UART_LSR_BI;
+
+ port->ignore_status_mask = 0;
+ if (termios->c_iflag & IGNPAR)
+ port->ignore_status_mask |= UART_LSR_PE | UART_LSR_FE;
+ if (termios->c_iflag & IGNBRK) {
+ port->ignore_status_mask |= UART_LSR_BI;
+ if (termios->c_iflag & IGNPAR)
+ port->ignore_status_mask |= UART_LSR_OE;
+ }
+ if ((termios->c_cflag & CREAD) == 0)
+ port->ignore_status_mask |= UART_LSR_DR;
+
+ if ((termios->c_cflag & CRTSCTS) == 0) {
+ amb_port->mcr &= ~UART_MC_AFCE;
+ port->status &= ~UPSTAT_AUTOCTS;
+ } else {
+ amb_port->mcr |= UART_MC_AFCE;
+ port->status |= UPSTAT_AUTOCTS;
+ }
+
+ writel_relaxed(UART_LC_DLAB, port->membase + UART_LC_OFFSET);
+ writel_relaxed(quot & 0xff, port->membase + UART_DLL_OFFSET);
+ writel_relaxed((quot >> 8) & 0xff, port->membase + UART_DLH_OFFSET);
+ writel_relaxed(lc, port->membase + UART_LC_OFFSET);
+ if (UART_ENABLE_MS(port, termios->c_cflag))
+ __serial_ambarella_enable_ms(port);
+ else
+ __serial_ambarella_disable_ms(port);
+ serial_ambarella_set_mctrl(port, port->mctrl);
+ }
+}
+
+static void serial_ambarella_pm(struct uart_port *port,
+ unsigned int state, unsigned int oldstate)
+{
+}
+
+static void serial_ambarella_release_port(struct uart_port *port)
+{
+}
+
+static int serial_ambarella_request_port(struct uart_port *port)
+{
+ return 0;
+}
+
+static void serial_ambarella_config_port(struct uart_port *port, int flags)
+{
+}
No need for empty definitions. The hooks are optional. We should document this, likely.
+static int serial_ambarella_verify_port(struct uart_port *port,
+ struct serial_struct *ser)
+{
+ int rval = 0;
You can return immediately, without the need for this variable, right?
+...
+ if (ser->type != PORT_UNKNOWN && ser->type != PORT_UART00)
+ rval = -EINVAL;
+ if (port->irq != ser->irq)
+ rval = -EINVAL;
+ if (ser->io_type != SERIAL_IO_MEM)
+ rval = -EINVAL;
+
+ return rval;
+}
+static int serial_ambarella_probe(struct platform_device *pdev)
+{
+ struct ambarella_uart_port *amb_port;
+ struct resource *mem;
+ struct pinctrl *pinctrl;
+ int irq, id, rval;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!mem) {
+ dev_err(&pdev->dev, "no mem resource!\n");
+ return -ENODEV;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "no irq resource!\n");
+ return -ENODEV;
+ }
+
+ id = of_alias_get_id(pdev->dev.of_node, "serial");
+ if (id < 0 || id >= serial_ambarella_reg.nr) {
+ dev_err(&pdev->dev, "Invalid uart ID %d!\n", id);
+ return -ENXIO;
+ }
+
+ pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+ if (IS_ERR(pinctrl)) {
+ dev_err(&pdev->dev, "Failed to request pinctrl\n");
+ return PTR_ERR(pinctrl);
+ }
+
+ amb_port = &ambarella_port[id];
+
+ amb_port->uart_pll = devm_clk_get_enabled(&pdev->dev, NULL);
+ if (IS_ERR(amb_port->uart_pll)) {
+ dev_err(&pdev->dev, "Get uart clk failed!\n");
+ return PTR_ERR(amb_port->uart_pll);
+ }
+
+ amb_port->mcr = DEFAULT_AMBARELLA_UART_MCR;
+
+ amb_port->port.dev = &pdev->dev;
+ amb_port->port.type = PORT_UART00;
+ amb_port->port.iotype = UPIO_MEM;
+ amb_port->port.fifosize = UART_FIFO_SIZE;
+ amb_port->port.uartclk = clk_get_rate(amb_port->uart_pll);
+ amb_port->port.ops = &serial_ambarella_pops;
+ amb_port->port.irq = irq;
+ amb_port->port.line = id;
+ amb_port->port.mapbase = mem->start;
+ amb_port->port.membase = devm_ioremap_resource(&pdev->dev, mem);
+ if (IS_ERR(amb_port->port.membase))
+ return PTR_ERR(amb_port->port.membase);
+
+ rval = uart_add_one_port(&serial_ambarella_reg, &amb_port->port);
+ if (rval < 0)
+ dev_err(&pdev->dev, "failed to add port: %d, %d!\n", id, rval);
+
+ platform_set_drvdata(pdev, amb_port);
You set this even on failure. Does it matter? I don't know.
+
+ return rval;
+}
+
+static void serial_ambarella_remove(struct platform_device *pdev)
+{
+ struct ambarella_uart_port *amb_port;
+
+ amb_port = platform_get_drvdata(pdev);
This can be amended to the declaration.
+ uart_remove_one_port(&serial_ambarella_reg, &amb_port->port);
+}
+
thanks,
--
js
suse labs