[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9bd73f85-9d9a-8c44-e4e6-3c10b76fe135@kernel.org>
Date: Fri, 14 Apr 2023 08:47:23 +0200
From: Jiri Slaby <jirislaby@...nel.org>
To: Jacky Huang <ychuang570808@...il.com>, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, lee@...nel.org,
mturquette@...libre.com, sboyd@...nel.org, p.zabel@...gutronix.de,
gregkh@...uxfoundation.org
Cc: devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
arnd@...db.de, schung@...oton.com, mjchen@...oton.com,
Jacky Huang <ychuang3@...oton.com>
Subject: Re: [PATCH v7 11/12] tty: serial: Add Nuvoton ma35d1 serial driver
support
On 12. 04. 23, 7:38, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@...oton.com>
>
> 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@...oton.com>
...
> --- /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.
> + 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?
> + 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.
> + */
> + 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?
> + 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.
> +
> + /*
> + * 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.
> + /*
> + * 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
> + 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.
> + }
> + 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?
> + up->port.dev = &pdev->dev;
> + up->port.flags = UPF_BOOT_AUTOCONF;
> + ret = uart_add_one_port(&ma35d1serial_reg, &up->port);
And this?
> + 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?
> + 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,
--
js
suse labs
Powered by blists - more mailing lists