Messages in this thread | | | Date | Thu, 16 Mar 2023 21:28:02 +0800 | Subject | Re: [PATCH 14/15] tty: serial: Add Nuvoton ma35d1 serial driver support | From | Jacky Huang <> |
| |
Dear Jiri,
Thanks for your review.
I will create #define constant to replace every occurrence of magics or hard coding number.
For every occurrence of "struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; ", it will be replaced with to_ma35d1_uart_port() .
On 2023/3/15 下午 06:13, Jiri Slaby wrote: > On 15. 03. 23, 8:29, Jacky Huang wrote: >> --- /dev/null >> +++ b/drivers/tty/serial/ma35d1_serial.c >> @@ -0,0 +1,842 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * MA35D1 serial driver >> + * Copyright (C) 2023 Nuvoton Technology Corp. >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/moduleparam.h> > > What parameters does this module have? +#include <linux/ioport.h> >> +#include <linux/init.h> >> +#include <linux/console.h> >> +#include <linux/sysrq.h> >> +#include <linux/delay.h> > > What do you use from delay.h? > >> +#include <linux/platform_device.h> >> +#include <linux/tty.h> >> +#include <linux/tty_flip.h> >> +#include <linux/clk.h> >> +#include <linux/serial_reg.h> >> +#include <linux/serial_core.h> >> +#include <linux/serial.h> >> +#include <linux/nmi.h> > > nmi.h? > > Please clean up all of the includes. >
moduleparam.h, delay.h, and nmi.h are not used.
I will test and remove all unused #include.
>> +#include <linux/mutex.h> >> +#include <linux/slab.h> >> +#include <linux/uaccess.h> >> +#include <linux/of.h> >> +#include <linux/of_irq.h> >> +#include <linux/of_address.h> >> +#include <linux/of_platform.h> >> +#include <linux/io.h> >> +#include <asm/irq.h> >> +#include <asm/serial.h> >> +#include "ma35d1_serial.h" >> + >> +#define UART_NR 17 >> + >> +static struct uart_driver ma35d1serial_reg; >> +struct clk *clk; >> + >> +struct uart_ma35d1_port { >> + struct uart_port port; >> + u16 capabilities; /* port capabilities */ >> + u8 ier; >> + u8 lcr; >> + u8 mcr; >> + u8 mcr_mask; /* mask of user bits */ >> + u8 mcr_force; /* mask of forced bits */ > > Where are all those used? >
I will remove the unused structure members in the next version.
>> + struct serial_rs485 rs485; /* rs485 settings */ >> + u32 baud_rate; > > And this one. >
It's not used. I will remove it.
>> + int rx_count; >> + u32 console_baud_rate; >> + u32 console_line; >> + u32 console_int; >> +}; >> + >> +static struct device_node *ma35d1serial_uart_nodes[UART_NR]; >> +static struct uart_ma35d1_port ma35d1serial_ports[UART_NR] = { 0 }; > >> +static void __stop_tx(struct uart_ma35d1_port *p); > > What for?
This function is used to disable TX FIFO empty interrupt.
naming issue?
> >> +static void transmit_chars(struct uart_ma35d1_port *up); >> + >> +static struct uart_ma35d1_port *to_ma35d1_uart_port(struct uart_port >> *uart) >> +{ >> + return container_of(uart, struct uart_ma35d1_port, port); >> +} >> + >> +static u32 serial_in(struct uart_ma35d1_port *p, int offset) > > Q: int? A: No.
will modify "int offset" to "u32 offset"
> >> +{ >> + return __raw_readl(p->port.membase + offset); >> +} >> + >> +static void serial_out(struct uart_ma35d1_port *p, int offset, int >> value) > > No ints here, please.
will modify offset and value data type to u32
> >> +{ >> + __raw_writel(value, p->port.membase + offset); >> +} >> + >> +static void __stop_tx(struct uart_ma35d1_port *p) >> +{ >> + u32 ier; >> + >> + ier = serial_in(p, UART_REG_IER); >> + if (ier & THRE_IEN) >> + serial_out(p, UART_REG_IER, ier & ~THRE_IEN); >> +} >> + >> +static void ma35d1serial_stop_tx(struct uart_port *port) >> +{ >> + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; > > Despite you have to_ma35d1_uart_port(), you do this? >
You're right. I will use to_ma35d1_uart_port() to replace it.
>> + >> + __stop_tx(up); >> +} >> + >> +static void ma35d1serial_start_tx(struct uart_port *port) >> +{ >> + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; >> + u32 ier; >> + struct circ_buf *xmit = &up->port.state->xmit; >> + >> + ier = serial_in(up, UART_REG_IER); >> + serial_out(up, UART_REG_IER, ier & ~THRE_IEN); >> + if (uart_circ_chars_pending(xmit) < >> + (16 - ((serial_in(up, UART_REG_FSR) >> 16) & 0x3F))) > > You look like you need a helper for this computation (hint: > GENMASK()). What do those magic constants mean? >
I will add #define macro for the bit mask..
>> + transmit_chars(up); >> + serial_out(up, UART_REG_IER, ier | THRE_IEN); >> +} >> + >> +static void ma35d1serial_stop_rx(struct uart_port *port) >> +{ >> + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; > > Bah. Nah.
>> + >> + serial_out(up, UART_REG_IER, serial_in(up, UART_REG_IER) & >> ~RDA_IEN); >> +} >> + >> +static void >> +receive_chars(struct uart_ma35d1_port *up) >> +{ >> + u8 ch; >> + u32 fsr; >> + u32 isr; >> + u32 dcnt; >> + char flag; > > flag is u8 too. And a reverse xmas tree, please. Actually, you can put > all those u32 to a single line. >
OK, I will fix it.
>> + >> + isr = serial_in(up, UART_REG_ISR); >> + fsr = serial_in(up, UART_REG_FSR); >> + >> + while (!(fsr & RX_EMPTY)) { >> + flag = TTY_NORMAL; >> + up->port.icount.rx++; >> + >> + if (unlikely(fsr & (BIF | FEF | PEF | RX_OVER_IF))) { >> + if (fsr & BIF) { >> + serial_out(up, UART_REG_FSR, BIF); >> + up->port.icount.brk++; >> + if (uart_handle_break(&up->port)) >> + continue; >> + } >> + if (fsr & FEF) { >> + serial_out(up, UART_REG_FSR, FEF); >> + up->port.icount.frame++; >> + } >> + if (fsr & PEF) { >> + serial_out(up, UART_REG_FSR, PEF); >> + up->port.icount.parity++; >> + } >> + if (fsr & RX_OVER_IF) { >> + serial_out(up, UART_REG_FSR, RX_OVER_IF); >> + up->port.icount.overrun++; >> + } >> + if (fsr & BIF) >> + flag = TTY_BREAK; >> + if (fsr & PEF) >> + flag = TTY_PARITY; >> + if (fsr & FEF) >> + flag = TTY_FRAME; >> + } >> + ch = (u8)serial_in(up, UART_REG_RBR); >> + if (uart_handle_sysrq_char(&up->port, ch)) >> + continue; >> + >> + uart_insert_char(&up->port, fsr, RX_OVER_IF, ch, flag); > > No lock needed? >
I will wrap it with spin_lock and spin_unlock.
>> + up->rx_count++; >> + dcnt = (serial_in(up, UART_REG_FSR) >> 8) & 0x3f; > > More magic constants. No. > >> + if (up->rx_count > 1023) { >> + spin_lock(&up->port.lock); >> + tty_flip_buffer_push(&up->port.state->port); >> + spin_unlock(&up->port.lock); >> + up->rx_count = 0; >> + if ((isr & RXTO_IF) && (dcnt == 0)) >> + goto tout_end; >> + } >> + if (isr & RDA_IF) { >> + if (dcnt == 1) >> + return; >> + } >> + fsr = serial_in(up, UART_REG_FSR); >> + } >> + spin_lock(&up->port.lock); >> + tty_flip_buffer_push(&up->port.state->port); >> + spin_unlock(&up->port.lock); >> +tout_end: >> + up->rx_count = 0; >> +} >> + >> +static void transmit_chars(struct uart_ma35d1_port *up) > > Why this cannot use uart_port_tx()?
OK, I will rename it as uart_port_tx().
> >> +{ >> + struct circ_buf *xmit = &up->port.state->xmit; >> + int count = 16 - ((serial_in(up, UART_REG_FSR) >> 16) & 0xF); >> + >> + if (serial_in(up, UART_REG_FSR) & TX_FULL) >> + count = 0; >> + if (up->port.x_char) { >> + serial_out(up, UART_REG_THR, up->port.x_char); >> + up->port.icount.tx++; >> + up->port.x_char = 0; >> + return; >> + } >> + if (uart_tx_stopped(&up->port)) { >> + ma35d1serial_stop_tx(&up->port); >> + return; >> + } >> + if (uart_circ_empty(xmit)) { >> + __stop_tx(up); >> + return; >> + } >> + while (count > 0) { >> + serial_out(up, UART_REG_THR, xmit->buf[xmit->tail]); >> + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); >> + up->port.icount.tx++; >> + count--; >> + if (uart_circ_empty(xmit)) >> + break; >> + } >> + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) >> + uart_write_wakeup(&up->port); >> + if (uart_circ_empty(xmit)) >> + __stop_tx(up); >> +} >> + >> +static irqreturn_t ma35d1serial_interrupt(int irq, void *dev_id) >> +{ >> + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)dev_id; >> + u32 isr, fsr; >> + >> + isr = serial_in(up, UART_REG_ISR); >> + fsr = serial_in(up, UART_REG_FSR); >> + if (isr & (RDA_IF | RXTO_IF)) >> + receive_chars(up); >> + if (isr & THRE_INT) >> + transmit_chars(up); >> + if (fsr & (BIF | FEF | PEF | RX_OVER_IF | TX_OVER_IF)) >> + serial_out(up, UART_REG_FSR, >> + (BIF | FEF | PEF | RX_OVER_IF | TX_OVER_IF)); >> + >> + return IRQ_HANDLED; > > You give no way for OS to disable the irq when the HW goes crazy. I.e. > you should return IRQ_HANDLED only when you really handled the irq. > >> +} > ... >> +static u32 ma35d1serial_get_mctrl(struct uart_port *port) >> +{ >> + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; >> + u32 status; >> + u32 ret = 0; >> + >> + status = serial_in(up, UART_REG_MSR); >> + if (!(status & 0x10)) > > 0x10 is magic. > >> + ret |= TIOCM_CTS; >> + return ret; >> +} >> + >> +static void ma35d1serial_set_mctrl(struct uart_port *port, u32 mctrl) >> +{ >> + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; >> + u32 mcr = 0; >> + u32 ier = 0; >> + >> + if (mctrl & TIOCM_RTS) { >> + /* set RTS high level trigger */ >> + mcr = serial_in(up, UART_REG_MCR); >> + mcr |= 0x200; >> + mcr &= ~(0x2); >> + } >> + if (up->mcr & UART_MCR_AFE) { >> + /* set RTS high level trigger */ >> + mcr = serial_in(up, UART_REG_MCR); >> + mcr |= 0x200; >> + mcr &= ~(0x2); > > This is repeated. Parentheses are superfluous. And again, 0x200, 0x2 > are magic. > >> + >> + /* enable CTS/RTS auto-flow control */ >> + serial_out(up, UART_REG_IER, >> + (serial_in(up, UART_REG_IER) | (0x3000))); >> + >> + /* Set hardware flow control */ >> + up->port.flags |= UPF_HARD_FLOW; >> + } else { >> + /* disable CTS/RTS auto-flow control */ >> + ier = serial_in(up, UART_REG_IER); >> + ier &= ~(0x3000); > > Detto. > >> + serial_out(up, UART_REG_IER, ier); >> + >> + /* un-set hardware flow control */ >> + up->port.flags &= ~UPF_HARD_FLOW; >> + } >> + >> + /* set CTS high level trigger */ >> + serial_out(up, UART_REG_MSR, (serial_in(up, UART_REG_MSR) | >> (0x100))); >> + serial_out(up, UART_REG_MCR, mcr); >> +} > ... >> +static int ma35d1serial_startup(struct uart_port *port) >> +{ >> + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; >> + struct tty_struct *tty = port->state->port.tty; >> + int retval; >> + >> + /* Reset FIFO */ >> + serial_out(up, UART_REG_FCR, TFR | RFR /* | RX_DIS */); > > So why not RX_DIS? > >> + >> + /* Clear pending interrupts */ >> + serial_out(up, UART_REG_ISR, 0xFFFFFFFF); >> + >> + retval = request_irq(port->irq, ma35d1serial_interrupt, 0, >> + tty ? tty->name : "ma35d1_serial", port); >> + if (retval) { >> + dev_err(up->port.dev, "request irq failed.\n"); >> + return retval; >> + } >> + >> + /* Now, initialize the UART */ >> + /* FIFO trigger level 4 byte */ >> + /* RTS trigger level 8 bytes */ >> + serial_out(up, UART_REG_FCR, >> + serial_in(up, UART_REG_FCR) | 0x10 | 0x20000); >> + serial_out(up, UART_REG_LCR, 0x7); /* 8 bit */ >> + serial_out(up, UART_REG_TOR, 0x40); > > You know what. > >> + serial_out(up, UART_REG_IER, >> + RTO_IEN | RDA_IEN | TIME_OUT_EN | BUFERR_IEN); >> + return 0; >> +} >> + >> +static void ma35d1serial_shutdown(struct uart_port *port) >> +{ >> + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; >> + >> + free_irq(port->irq, port); >> + >> + /* Disable interrupts from this port */ >> + serial_out(up, UART_REG_IER, 0); > > The two lines are switched, IMO. First disable HW, then let the ISR > finish and free it.
You're right. I will fix it.
> >> +} >> + >> +static u32 ma35d1serial_get_divisor(struct uart_port *port, u32 baud) >> +{ >> + u32 quot; >> + >> + quot = (port->uartclk / baud) - 2; >> + return quot; > > quot variable is completely superfluous.
remove quot and return (port->uartclk / baud) - 2;
> >> +} >> + >> +static void ma35d1serial_set_termios(struct uart_port *port, >> + struct ktermios *termios, >> + const struct ktermios *old) >> +{ >> + struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port; >> + u32 lcr = 0; >> + unsigned long flags; >> + u32 baud, quot; >> + >> + switch (termios->c_cflag & CSIZE) { >> + case CS5: >> + lcr = 0; >> + break; >> + case CS6: >> + lcr |= 1; >> + break; >> + case CS7: >> + lcr |= 2; >> + break; >> + case CS8: >> + default: >> + lcr |= 3; >> + break; >> + } > > IOW: > lcr = UART_LCR_WLEN(tty_get_char_size(termios->c_cflag)); > All registers in this uart controller are 32-bits. lcr will finally be written to the register UART_REG_LCR.of which bit[1:0] represents for UART word length. 00 - 5 bits 01 - 6 bits 10 - 7 bits 11 - 8bits
Instead, I will add #define WLS_5 0x0 #define WLS_6 0x1 #define WLS_7 0x2 #define WLS_8 0x3
and replace lcr |= 1 with lcr |= WLS_6;
>> + >> + if (termios->c_cflag & CSTOPB) >> + lcr |= NSB; >> + if (termios->c_cflag & PARENB) >> + lcr |= PBE; >> + if (!(termios->c_cflag & PARODD)) >> + lcr |= EPE; >> + if (termios->c_cflag & CMSPAR) >> + 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 = RX_OVER_IF; >> + if (termios->c_iflag & INPCK) >> + up->port.read_status_mask |= FEF | PEF; >> + if (termios->c_iflag & (BRKINT | PARMRK)) >> + up->port.read_status_mask |= BIF; >> + >> + /* >> + * Characteres to ignore >> + */ >> + up->port.ignore_status_mask = 0; >> + if (termios->c_iflag & IGNPAR) >> + up->port.ignore_status_mask |= FEF | PEF; >> + if (termios->c_iflag & IGNBRK) { >> + up->port.ignore_status_mask |= 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 |= RX_OVER_IF; >> + } >> + if (termios->c_cflag & CRTSCTS) >> + up->mcr |= UART_MCR_AFE; >> + else >> + up->mcr &= ~UART_MCR_AFE; >> + >> + ma35d1serial_set_mctrl(&up->port, up->port.mctrl); >> + serial_out(up, UART_REG_BAUD, quot | 0x30000000); >> + serial_out(up, UART_REG_LCR, lcr); >> + spin_unlock_irqrestore(&up->port.lock, flags); >> +} > ... >> +static void ma35d1serial_config_port(struct uart_port *port, int flags) >> +{ >> + int ret; >> + >> + /* >> + * Find the region that we can probe for. This in turn >> + * tells us whether we can probe for the type of port. >> + */ >> + ret = ma35d1serial_request_port(port); >> + if (ret < 0) >> + return; > > ma35d1serial_request_port() does nothing. You can remove it altogether. >
OK, I will remove it.
>> + port->type = PORT_MA35D1; >> +} > > >> +static int ma35d1serial_ioctl(struct uart_port *port, u32 cmd, >> unsigned long arg) >> +{ >> + switch (cmd) { >> + default: >> + return -ENOIOCTLCMD; >> + } >> + return 0; >> +} > > Drop that completely. >
OK.
>> +static void >> +ma35d1serial_console_init_port(void) >> +{ >> + int i = 0; >> + struct device_node *np; >> + >> + for_each_matching_node(np, ma35d1_serial_of_match) { >> + if (ma35d1serial_uart_nodes[i] == NULL) { >> + ma35d1serial_uart_nodes[i] = np; >> + i++; > > Unless the dt is broken, this is OK. But I would add a sanity check to i. >
I will add 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; >> + 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, 0x10000); >> + 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; >> + } >> + err = clk_prepare_enable(clk); >> + if (err) >> + return -ENOENT; >> + >> + if (up->port.line != 0) >> + up->port.uartclk = clk_get_rate(clk); >> + up->port.irq = platform_get_irq(pdev, 0); >> + up->port.dev = &pdev->dev; >> + up->port.flags = UPF_BOOT_AUTOCONF; >> + up->port.rs485_config = ma35d1serial_config_rs485; >> + ret = uart_add_one_port(&ma35d1serial_reg, &up->port); > > What if this fails? >
I will add return value check.
>> + platform_set_drvdata(pdev, up); >> + return 0; >> +} >> + >> +/* >> + * Remove serial ports registered against a platform device. >> + */ >> +static int ma35d1serial_remove(struct platform_device *dev) >> +{ >> + int i; >> + struct uart_port *port = platform_get_drvdata(dev); >> + >> + free_irq(port->irq, port); > > Hmm, this doesn't look right. You did that already, or? > >> + for (i = 0; i < UART_NR; i++) { >> + struct uart_ma35d1_port *up = &ma35d1serial_ports[i]; >> + >> + if (up->port.dev == &dev->dev) > > You did platform_set_drvdata(), so why all this? >
static int ma35d1serial_remove(struct platform_device *dev) { struct uart_port *port = platform_get_drvdata(dev);
uart_remove_one_port(&ma35d1serial_reg, &up->port); free_irq(port->irq, port); return 0; }
>> + uart_remove_one_port(&ma35d1serial_reg, &up->port); >> + } >> + return 0; >> +} >
Best Regards,
Jacky Huang
| |