Re: [PATCH v7 11/12] tty: serial: Add Nuvoton ma35d1 serial driver support

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

 



Dear Jiri,


Thanks for you advice.


On 2023/4/14 下午 02:47, Jiri Slaby wrote:
On 12. 04. 23, 7:38, Jacky Huang wrote:
From: Jacky Huang <ychuang3@xxxxxxxxxxx>

This adds UART and console driver for Nuvoton ma35d1 Soc.
It supports full-duplex communication, FIFO control, and
hardware flow control.

Signed-off-by: Jacky Huang <ychuang3@xxxxxxxxxxx>
...
--- /dev/null
+++ b/drivers/tty/serial/ma35d1_serial.c
@@ -0,0 +1,773 @@
...
+static void transmit_chars(struct uart_ma35d1_port *up)
+{
+    int count;

count is unsigned.


I will use 'u32' instead.



+    u8 ch;
+
+    if (uart_tx_stopped(&up->port)) {
+        ma35d1serial_stop_tx(&up->port);
+        return;
+    }
+    count = UART_FIFO_DEPTH - ((serial_in(up, UART_REG_FSR) & FSR_TXPTR_MSK) >> 16);

So this could be FIELD_GET() while you are defining FSR_TXPTR_MSK using GENMASK(), right?


Yes, I will modify it as

    count = UART_FIFO_DEPTH - FIELD_GET(FSR_TXPTR_MSK, serial_in(up, UART_REG_FSR));



+ uart_port_tx_limited(&up->port, ch, count,
+                 !(serial_in(up, UART_REG_FSR) & FSR_TX_FULL),
+                 serial_out(up, UART_REG_THR, ch),
+                 ({}));
+}
...
+static void ma35d1serial_set_termios(struct uart_port *port,
+                     struct ktermios *termios,
+                     const struct ktermios *old)
+{
+    struct uart_ma35d1_port *up = to_ma35d1_uart_port(port);
+    u32 lcr = 0;
+    unsigned long flags;
+    u32 baud, quot;
+
+    lcr = UART_LCR_WLEN(tty_get_char_size(termios->c_cflag));
+
+    if (termios->c_cflag & CSTOPB)
+        lcr |= LCR_NSB;
+    if (termios->c_cflag & PARENB)
+        lcr |= LCR_PBE;
+    if (!(termios->c_cflag & PARODD))
+        lcr |= LCR_EPE;
+    if (termios->c_cflag & CMSPAR)
+        lcr |= LCR_SPE;
+
+    baud = uart_get_baud_rate(port, termios, old, port->uartclk / 0xffff,
+                  port->uartclk / 11);
+
+    quot = ma35d1serial_get_divisor(port, baud);
+
+    /*
+     * Ok, we're now changing the port state.  Do it with
+     * interrupts disabled.
+     */
+    spin_lock_irqsave(&up->port.lock, flags);
+
+    up->port.read_status_mask = FSR_RX_OVER_IF;
+    if (termios->c_iflag & INPCK)
+        up->port.read_status_mask |= FSR_FEF | FSR_PEF;
+    if (termios->c_iflag & (BRKINT | PARMRK))
+        up->port.read_status_mask |= FSR_BIF;
+
+    /*
+     * Characteres to ignore

"Characters"

The comment could be oneline.

Okay, I will modify it as as oneline.



+     */
+    up->port.ignore_status_mask = 0;
+    if (termios->c_iflag & IGNPAR)
+        up->port.ignore_status_mask |= FSR_FEF | FSR_PEF;
+    if (termios->c_iflag & IGNBRK) {
+        up->port.ignore_status_mask |= FSR_BIF;
+        /*
+         * If we're ignoring parity and break indicators,
+         * ignore overruns too (for real raw support).
+         */
+        if (termios->c_iflag & IGNPAR)
+            up->port.ignore_status_mask |= FSR_RX_OVER_IF;
+    }

Actually I don't understand the "Characteres" comment above at all. What characters?


We just follow what other serial drivers have done.

You can find the same code segment in tty/serial dz.c, fsl_lpuart.c, atmel_serial.c, 8250_port.c ....



+    if (termios->c_cflag & CRTSCTS)
+        up->mcr |= UART_MCR_AFE;
+    else
+        up->mcr &= ~UART_MCR_AFE;
+
+    uart_update_timeout(port, termios->c_cflag, baud);
+    ma35d1serial_set_mctrl(&up->port, up->port.mctrl);
+    serial_out(up, UART_REG_BAUD, quot | BAUD_MODE2);
+    serial_out(up, UART_REG_LCR, lcr);
+    spin_unlock_irqrestore(&up->port.lock, flags);
+}
...

+/*
+ *  Print a string to the serial port trying not to disturb
+ *  any possible real use of the port...
+ *
+ *  The console_lock must be held when we get here.
+ */
+static void ma35d1serial_console_write(struct console *co,
+                       const char *s, u32 count)
+{
+    struct uart_ma35d1_port *up = &ma35d1serial_ports[co->index];
+    unsigned long flags;
+    u32 ier;
+
+    local_irq_save(flags);

This doesn't protect access to the registers on other CPUs.


We will add spin_lock protect to it.


+
+    /*
+     *  First save the IER then disable the interrupts
+     */
+    ier = serial_in(up, UART_REG_IER);
+    serial_out(up, UART_REG_IER, 0);
+
+    uart_console_write(&up->port, s, count, ma35d1serial_console_putchar);
+
+    wait_for_xmitr(up);
+
+    serial_out(up, UART_REG_IER, ier);
+    local_irq_restore(flags);
+}
+
+static int __init ma35d1serial_console_setup(struct console *co,
+                         char *options)
+{
+    struct device_node *np = ma35d1serial_uart_nodes[co->index];
+    struct uart_ma35d1_port *p = &ma35d1serial_ports[co->index];
+    u32 val32[4];
+    struct uart_port *port;
+    int baud = 115200;
+    int bits = 8;
+    int parity = 'n';
+    int flow = 'n';

If you don't do uart_parse_options() (why you don't?), you don't need the variables.


On this platform, the serial console is fixed to be 11520, n, 8, since booting from

the non-volatile firmware in MaskROM, arm-trust-firmware, uboot. So we do not

use usrt_parse_options().

I will remove these variables and modify 'uart_set_options(port, co, baud, parity, bits, flow)'

as 'uart_set_options(port, co, 115200, n, 8, n)'



+    /*
+     * Check whether an invalid uart number has been specified, and
+     * if so, search for the first available port that does have
+     * console support.
+     */
+    if ((co->index < 0) || (co->index >= UART_NR)) {
+        pr_debug("Console Port%x out of range\n", co->index);
+        return -EINVAL;
+    }
+
+    if (of_property_read_u32_array(np, "reg", val32, 4) != 0)
+        return -EINVAL;
+    p->port.iobase = val32[1];
+    p->port.membase = ioremap(p->port.iobase, UART_REG_SIZE);
+    p->port.ops = &ma35d1serial_ops;
+    p->port.line = 0;
+    p->port.uartclk = UART_CONSOLE_CLK;
+
+    port = &ma35d1serial_ports[co->index].port;
+    return uart_set_options(port, co, baud, parity, bits, flow);
+}
...> +static void ma35d1serial_console_init_port(void)
+{
+    int i = 0;

unsigned

I will fix it.



+    struct device_node *np;
+
+    for_each_matching_node(np, ma35d1_serial_of_match) {
+        if (ma35d1serial_uart_nodes[i] == NULL) {
+            of_node_get(np);
+            ma35d1serial_uart_nodes[i] = np;
+            i++;
+            if (i == UART_NR)
+                break;
+        }
+    }
+}

+/*
+ * Register a set of serial devices attached to a platform device.
+ * The list is terminated with a zero flags entry, which means we expect
+ * all entries to have at least UPF_BOOT_AUTOCONF set.
+ */
+static int ma35d1serial_probe(struct platform_device *pdev)
+{
+    struct resource *res_mem;
+    struct uart_ma35d1_port *up;
+    int ret = 0;
+    struct clk *clk;
+    int err;
+
+    if (pdev->dev.of_node) {
+        ret = of_alias_get_id(pdev->dev.of_node, "serial");
+        if (ret < 0) {
+            dev_err(&pdev->dev, "failed to get alias/pdev id, errno %d\n", ret);
+            return ret;
+        }
+    }
+    up = &ma35d1serial_ports[ret];
+    up->port.line = ret;
+    res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+    if (!res_mem)
+        return -ENODEV;
+
+    up->port.iobase = res_mem->start;
+    up->port.membase = ioremap(up->port.iobase, UART_REG_SIZE);
+    up->port.ops = &ma35d1serial_ops;
+
+    spin_lock_init(&up->port.lock);
+
+    clk = of_clk_get(pdev->dev.of_node, 0);
+    if (IS_ERR(clk)) {
+        err = PTR_ERR(clk);
+        dev_err(&pdev->dev, "failed to get core clk: %d\n", err);
+        return -ENOENT;

iounmap(membase) missing.


I will use devm_ioremap instead, and add return value check to it.



+    }
+    err = clk_prepare_enable(clk);
+    if (err)
+        return -ENOENT;

Dtto.

+
+    if (up->port.line != 0)
+        up->port.uartclk = clk_get_rate(clk);
+    up->port.irq = platform_get_irq(pdev, 0);

What if this fails?


I will add return value check to it.



+    up->port.dev = &pdev->dev;
+    up->port.flags = UPF_BOOT_AUTOCONF;
+    ret = uart_add_one_port(&ma35d1serial_reg, &up->port);

And this?


I will add return value check to it.


+    platform_set_drvdata(pdev, up);
+    return 0;
+}
+
+/*
+ * Remove serial ports registered against a platform device.
+ */
+static int ma35d1serial_remove(struct platform_device *dev)
+{
+    struct uart_port *port = platform_get_drvdata(dev);
+
+    uart_remove_one_port(&ma35d1serial_reg, port);
+    free_irq(port->irq, port);

You do this in ma35d1serial_shutdown() already, correct? So this will error out, right?


Yes, the free_irq() should be removed. I  will fix it.



+    return 0;
+}

Just a couple of questions about testing. Have you tried to:
* remove the module?
* suspend/resume the machine while having a line active (device node opened)? * running at least a lockdep-enabled kernel while using the device extensively?

thanks,

Thank you for the suggestions. Actually, we never remove the serial driver module.

We will have a test on serial module remove, and test suspend/resume with at least one uart port enabled.

For the last question, as this SoC have dual-core Cortex-A35 CPU. We will a test

to run independent uart transfer application on different core.


Best regards,

Jacky Huang








[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux