Hi, Andy Shevchenko :
Thanks for your review.
Please see my response in below mail.
Regards,
Hammer Hsieh
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Friday, December 3, 2021 4:03 AM
> To: Hammer Hsieh <hammerh0314@xxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; open list:SERIAL DRIVERS
> <linux-serial@xxxxxxxxxxxxxxx>; devicetree <devicetree@xxxxxxxxxxxxxxx>;
> Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; Jiri Slaby
> <jirislaby@xxxxxxxxxx>; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>; Tony Huang
> 黃懷厚 <tony.huang@xxxxxxxxxxx>; Wells Lu 呂芳騰
> <wells.lu@xxxxxxxxxxx>; Hammer Hsieh 謝宏孟
> <hammer.hsieh@xxxxxxxxxxx>
> Subject: Re: [PATCH v4 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver
>
> On Thu, Dec 2, 2021 at 9:35 PM Hammer Hsieh <hammerh0314@xxxxxxxxx>
> wrote:
> >
> > Add Sunplus SoC UART Driver
>
> ...
>
> > +config SERIAL_SUNPLUS
> > + tristate "Sunplus UART support"
> > + depends on OF || COMPILE_TEST
> > + select SERIAL_CORE
> > + help
> > + Select this option if you would like to use Sunplus serial port on
> > + Sunplus SoC SP7021.
> > + If you enable this option, Sunplus serial ports in the system will
> > + be registered as ttySUPx.
>
> What will be the module name in case of module build?
>
will add info as below shows in Kconfig:
"This driver can also be built as a module. If so, the module will be called sunplus-uart."
> ...
>
> > +/*
> > + * Sunplus SoC UART driver
> > + *
> > + * Author: Hammer Hsieh <hammer.hsieh@xxxxxxxxxxx>
>
> Authors:
>
> > + * Tony Huang <tony.huang@xxxxxxxxxxx>
> > + * Wells Lu <wells.lu@xxxxxxxxxxx>
>
> And please indent names to be on the same column.
>
I rewrite almost all uart driver.
The other authors ask me to remove their names on this driver.
Will modify it.
> > + */
>
> ...
>
> > +#define UART_AUTOSUSPEND_TIMEOUT 3000
>
> Add units to the name.
>
Will add it. /* units: ms */
> ...
>
> > +static inline u32 sunplus_tx_buf_not_full(struct uart_port *port) {
> > + unsigned int lsr = readl(port->membase + SUP_UART_LSR);
> > +
> > + return ((lsr & SUP_UART_LSR_TX) ? SUP_UART_LSR_TX_NOT_FULL :
> > + 0);
>
> Too many parentheses. Ditto for all similar cases.
>
> > +}
>
ok, I have found similar case. I will modify it.
> ...
>
> > + do {
> > + sp_uart_put_char(port, xmit->buf[xmit->tail]);
> > + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>
> "% UART_XMIT_SIZE" is more accurate since it doesn't require a value to be a
> power of 2. In case of power of 2 it will be properly optimized by a compiler.
>
Currently, I have found drivers/tty/serial/mps2-uart.c use it only.
ok, will modify it.
> > + port->icount.tx++;
> > +
> > + if (uart_circ_empty(xmit))
> > + break;
> > + } while (sunplus_tx_buf_not_full(port));
>
> ...
>
> > + spin_lock_irqsave(&port->lock, flags);
>
> Why irqsave here?
>
> ...
>
> > + if (readl(port->membase + SUP_UART_ISC) & SUP_UART_ISC_RX)
> > + receive_chars(port);
> > +
> > + if (readl(port->membase + SUP_UART_ISC) & SUP_UART_ISC_TX)
> > + transmit_chars(port);
>
> Do you really need to perform two I/O against the very same register?
>
I will rewrite as below, how do you think?
Static irqreturn_t sunplus_uart_irq(int irq, void *args)
{
Struct uart_port *port = (struct uart_port *)args;
unsigned int isc = readl(port->membase + SUP_UART_ISC);
if (isc & SUP_UART_ISC_RX)
receive_chars(port);
if (isc & SUP_UART_ISC_TX)
transmit_chars(port);
return IRQ_HANDLED;
}
> ...
>
> > +static int sunplus_startup(struct uart_port *port) {
> > + unsigned int isc;
> > + int ret;
>
> > +#ifdef CONFIG_PM
>
> Why is this ifdeffery around the driver?
>
> > + if (!uart_console(port)) {
> > + ret = pm_runtime_get_sync(port->dev);
> > + if (ret < 0)
> > + goto out;
> > + }
> > +#endif
> > + ret = request_irq(port->irq, sunplus_uart_irq, 0, "sunplus_uart",
> port);
> > + if (ret)
> > + return ret;
> > +
> > + spin_lock_irq(&port->lock);
> > +
> > + isc |= SUP_UART_ISC_RXM;
> > + writel(isc, port->membase + SUP_UART_ISC);
> > +
> > + spin_unlock_irq(&port->lock);
>
> > +#ifdef CONFIG_PM
> > + if (!uart_console(port))
> > + pm_runtime_put(port->dev);
>
> Why doesn't it set autosuspend, i.o.w. Why is it different from an error case?
>
Autosuspend already init at probe.
I remove #ifdef CONFIG_PM code in sunplus_startup() and test runtime function.
linux-serial-test -y 0x55 -z 0x30 -p /dev/ttySUP1 -b 115200
runtime_resume and runtime_suspend still work.
I will remove it.
> > + return 0;
> > +out:
> > + if (!uart_console(port)) {
> > + pm_runtime_mark_last_busy(port->dev);
> > + pm_runtime_put_autosuspend(port->dev);
> > + }
> > +#endif
> > + return 0;
> > +}
>
> ...
>
> > +static void sunplus_set_termios(struct uart_port *port,
> > + struct ktermios *termios,
> > + struct ktermios *oldtermios) {
> > + u32 clk, ext, div, div_l, div_h, baud;
> > + u32 lcr, val;
> > + unsigned long flags;
>
> > + clk = port->uartclk;
>
> This can be done in the definition block above.
>
I think you want the code like below, right ?
u32 ext, div, div_l, div_h, baud, lcr;
u32 clk = port->uartclk;
unsigned long flags;
> > + baud = uart_get_baud_rate(port, termios, oldtermios, 0,
> > + port->uartclk / 16);
> > +
> > + readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
> > + (val & SUP_UART_LSR_TXE), 1,
> 10000);
>
> No error check?
>
remove this code in set_termios( ).
I think it is not necessary to check tx empty here.
> > + /*
> > + * baud rate = uartclk / ((16 * div[15:0] + 1) + div_ext[3:0])
> > + * get target baud rate and uartclk
> > + * auto calculate div and div_ext
> > + * div_h = (div[15:8] >> 8); div_l = (div_ext[3:0] << 12) +
> > + div[7:0]
>
> There is no need to explain the code, please add excerpts from the data sheet
> on how the divisors and baud rate are calculated, use mathematical language,
> and not programming in the comment.
>
Ok, will modify it. Which one is better?
/* baud rate = uartclk / ((16 * divisor + 1) + divisor_ext) */
Or
/* uartclk
* baud rate = -------------------------------------------
* (16 * divisor + 1) + divisor_ext
*/
> > + */
> > + clk += baud >> 1;
> > + div = clk / baud;
> > + ext = div & 0x0F;
> > + div = (div >> 4) - 1;
> > + div_l = (div & 0xFF) | (ext << 12);
> > + div_h = div >> 8;
>
> ...
>
> > +static const char *sunplus_type(struct uart_port *port) {
> > + struct sunplus_uart_port *sup = to_sunplus_uart(port);
> > +
> > + return sup->port.type == PORT_SUNPLUS ? "sunplus_uart" : NULL;
> > +}
>
>
> What do we achieve with this? Who and how will see this information?
> Under which circumstances the port type is not SUNPLUS?
>
> ...
>
> > +static void sunplus_release_port(struct uart_port *port) { }
> > +
> > +static int sunplus_request_port(struct uart_port *port) {
> > + return 0;
> > +}
>
> Are these stubs mandatory?
>
> ...
>
> > +static void sunplus_config_port(struct uart_port *port, int type) {
>
> > + if (type & UART_CONFIG_TYPE) {
> > + port->type = PORT_SUNPLUS;
> > + sunplus_request_port(port);
> > + }
>
> if (!(type & ...))
> return;
>
> ?
>
About these functions
sunplus_type / sunplus_release_port / sunplus_request_port / sunplus_config_port
Almost all uart driver have these function, but actually I don't know when/how to use it.
I will study it.
If you have more information, please share it to me.
> > +}
>
> ...
>
> > +static int sunplus_poll_init(struct uart_port *port) {
> > + return 0;
> > +}
>
> Why is this stub needed?
>
Will remove it.
> ...
>
> > +static inline void wait_for_xmitr(struct uart_port *port) {
> > + unsigned int val;
> > +
> > + readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
> > + (val & SUP_UART_LSR_TX), 1,
> 10000);
>
> Error handling?
> Or explain why it's not needed.
>
> > +}
>
I refer to /drivers/tty/serial/owl-uart.c
Will add error handling for it as below , is that OK?
static inline void wait_for_xmitr(struct uart_port *port) {
unsigned int val;
int ret;
/*Wait for FIFO is full or timeout */
ret = readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
(val & SUP_UART_LSR_TX), 1, 10000);
If (ret == -ETIMEOUT) {
dev_err(port->dev, "Timeout waiting while UART TX FULL);
return;
}
}
> ...
>
> > + sup->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (!IS_ERR(sup->clk)) {
>
> Instead use devm_clk_get_optional().
>
> > + ret = clk_prepare_enable(sup->clk);
> > + if (ret)
> > + return ret;
>
> > + ret = devm_add_action_or_reset(&pdev->dev,
> > + (void(*)(void
> *))clk_disable_unprepare,
> > + sup->clk);
>
> Look at the examples of how other drivers do this (that were submitted more
> or less recently).
>
> > + if (ret)
> > + return ret;
> > + } else {
>
> > + if (PTR_ERR(sup->clk) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
>
> Why?!
>
> > + return dev_err_probe(&pdev->dev, PTR_ERR(sup->clk),
> "clk not found\n");
> > + }
>
> ...
In case of without deferred probe.
I refer to /drivers/tty/serial/meson_uart.c
And I will modify as below, is that ok?
sup->clk = devm_clk_get_optional(&pdev->dev, NULL);
if (IS_ERR(sup->clk))
return dev_err_probe(&pdev->dev, PTR_ERR(sup->clk), "clk not found\n");
ret = clk_prepare_enable(sup->clk);
if (ret)
return ret;
ret = devm_add_action_or_reset(&pdev->dev, (void(*)(void*))clk_disable_unprepare,
sup->clk);
if (ret)
return ret;
As your comment last patch v3 , you said "think of deferred".
So I refer to /drivers/tty/serial/sccnxp.c
Maybe I just need to do it without deferred probe.
>
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + port->membase = devm_ioremap_resource(&pdev->dev, res);
>
> We have an API that does both at once. Use it.
>
ok, will modify for it.
devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > + if (IS_ERR(port->membase))
> > + return dev_err_probe(&pdev->dev,
> > + PTR_ERR(port->membase), "membase not found\n");
>
> ...
>
> > + ret = reset_control_deassert(sup->rstc);
> > + if (ret)
> > + return ret;
>
> From here no reset assertion on error? Why?
>
I found I didn't add devm_add_action_or_reset( ) to run reset_control_assert( ) for it.
I will add it as bleow.
ret = devm_add_action_or_reset(&pdev->dev, (void(*)(void*))reset_control_assert,
sup->rstc);
if (ret)
return ret;
> ...
>
> > +#ifdef CONFIG_SERIAL_SUNPLUS_CONSOLE
> > + if (pdev->id == 0)
> > + port->cons = &sunplus_uart_console;
> > + sunplus_console_ports[sup->port.line] = sup; #endif
>
> Actually why don't you use register_console() ?
>
I will think how to do it.
> ...
>
> > +static struct platform_driver sunplus_uart_platform_driver = {
> > + .probe = sunplus_uart_probe,
> > + .remove = sunplus_uart_remove,
> > + .driver = {
> > + .name = "sunplus_uart",
>
> > + .of_match_table = of_match_ptr(sp_uart_of_match),
>
> How did you test this when OF=n and COMPILE_TEST=y?
> Hint: Compiler will warn about unused variables (you need W=1).
>
> > + .pm = &sunplus_uart_pm_ops,
> > + }
> > +};
>
I have found many patch drop of_match_ptr( ) cause warning unused variable.
Now I know what you means at last PATCH v3 comment.
I set OF=n and COMPILE_TEST=y, but other error come out first.
I didn't confirm it further.
Will remove of_match_ptr( ).
> ...
>
> > +static void __exit sunplus_uart_exit(void) {
> > + platform_driver_unregister(&sunplus_uart_platform_driver);
> > + uart_unregister_driver(&sunplus_uart_driver);
> > +}
>
> > +
>
> Dtop this blank line...
>
> > +module_init(sunplus_uart_init);
> > +module_exit(sunplus_uart_exit);
>
> ...and attach each of the calls to the implemented function.
>
Ok , will modify it.
> ...
>
> > +static int __init
> > +sunplus_uart_early_setup(struct earlycon_device *dev, const char
> > +*opt) {
> > + if (!(dev->port.membase || dev->port.iobase))
> > + return -ENODEV;
> > +
> > + dev->con->write = sunplus_uart_early_write;
> > +
> > + return 0;
> > +}
>
> > +
>
> No blank line.
>
Use scripts/checkpatch.pl --strict -f drivers/tty/serial/sunplus-uart.c
It will show "CHECK: Please use a blank line after function/struct/union/enum declarations".
That's why I confuse it and add a blank for it.
ok, will remove the blank.
> > +OF_EARLYCON_DECLARE(sunplus_uart, "sunplus,sp7021-uart",
> > +sunplus_uart_early_setup);
>
> --
> With Best Regards,
> Andy Shevchenko
