Hi, Andy Shevchenko :

Thanks for your review.
Please see my response in below mail.

Regards,
Hammer Hsieh

-----Original Message-----
From: Andy Shevchenko <redacted>
Sent: Friday, December 3, 2021 4:03 AM
To: Hammer Hsieh <hammerh0314@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring
[off-list ref]; open list:SERIAL DRIVERS
[off-list ref]; devicetree [off-list ref];
Linux Kernel Mailing List [off-list ref]; Jiri Slaby
[off-list ref]; Philipp Zabel [off-list ref]; Tony Huang
黃懷厚 [off-list ref]; Wells Lu 呂芳騰
[off-list ref]; Hammer Hsieh 謝宏孟
[off-list ref]
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 [off-list ref]
wrote:

quoted

Add Sunplus SoC UART Driver

...

quoted

+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."

...

quoted

+/*
+ * Sunplus SoC UART driver
+ *
+ * Author: Hammer Hsieh [off-list ref]

Authors:

quoted

+ * Tony Huang [off-list ref]
+ * Wells Lu [off-list ref]

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.

quoted

+ */

...

quoted

+#define UART_AUTOSUSPEND_TIMEOUT       3000

Add units to the name.

Will add it. /* units: ms */

...

quoted

+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.

quoted

+}

ok, I have found similar case. I will modify it.

...

quoted

+       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.

quoted

+               port->icount.tx++;
+
+               if (uart_circ_empty(xmit))
+                       break;
+       } while (sunplus_tx_buf_not_full(port));

...

quoted

+       spin_lock_irqsave(&port->lock, flags);

Why irqsave here?

...

quoted

+       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;
}

...

quoted

+static int sunplus_startup(struct uart_port *port) {
+       unsigned int isc;
+       int ret;

quoted

+#ifdef CONFIG_PM

Why is this ifdeffery around the driver?

quoted

+       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);

quoted

+       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);

quoted

+#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.

quoted

+       return 0;
+out:
+       if (!uart_console(port)) {
+               pm_runtime_mark_last_busy(port->dev);
+               pm_runtime_put_autosuspend(port->dev);
+       }
+#endif
+       return 0;
+}

...

quoted

+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;

quoted

+       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;

quoted

+       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.

quoted

+       /*
+        * 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
 */

quoted

+        */
+       clk += baud >> 1;
+       div = clk / baud;
+       ext = div & 0x0F;
+       div = (div >> 4) - 1;
+       div_l = (div & 0xFF) | (ext << 12);
+       div_h = div >> 8;

...

quoted

+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?

...

quoted

+static void sunplus_release_port(struct uart_port *port) { }
+
+static int sunplus_request_port(struct uart_port *port) {
+       return 0;
+}

Are these stubs mandatory?

...

quoted

+static void sunplus_config_port(struct uart_port *port, int type) {

quoted

+       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.

quoted

+}

...

quoted

+static int sunplus_poll_init(struct uart_port *port) {
+       return 0;
+}

Why is this stub needed?

Will remove it.

...

quoted

+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.

quoted

+}

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;
   }
}

...

quoted

+       sup->clk = devm_clk_get(&pdev->dev, NULL);
+       if (!IS_ERR(sup->clk)) {

Instead use devm_clk_get_optional().

quoted

+               ret = clk_prepare_enable(sup->clk);
+               if (ret)
+                       return ret;

quoted

+               ret = devm_add_action_or_reset(&pdev->dev,
+                                              (void(*)(void

*))clk_disable_unprepare,

quoted

+                                              sup->clk);

Look at the examples of how other drivers do this (that were submitted more
or less recently).

quoted

+               if (ret)
+                       return ret;
+       } else {

quoted

+               if (PTR_ERR(sup->clk) == -EPROBE_DEFER)
+                       return -EPROBE_DEFER;

Why?!

quoted

+               return dev_err_probe(&pdev->dev, PTR_ERR(sup->clk),

"clk not found\n");

quoted

+       }

...

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.

quoted

+       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);

quoted

+       if (IS_ERR(port->membase))
+               return dev_err_probe(&pdev->dev,
+ PTR_ERR(port->membase), "membase not found\n");

...

quoted

+       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;

...

quoted

+#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.

...

quoted

+static struct platform_driver sunplus_uart_platform_driver = {
+       .probe          = sunplus_uart_probe,
+       .remove         = sunplus_uart_remove,
+       .driver = {
+               .name   = "sunplus_uart",

quoted

+               .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).

quoted

+               .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( ).

...

quoted

+static void __exit sunplus_uart_exit(void) {
+       platform_driver_unregister(&sunplus_uart_platform_driver);
+       uart_unregister_driver(&sunplus_uart_driver);
+}

quoted

+

Dtop this blank line...

quoted

+module_init(sunplus_uart_init);
+module_exit(sunplus_uart_exit);

...and attach each of the calls to the implemented function.

Ok , will modify it.

...

quoted

+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;
+}

quoted

+

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.

quoted

+OF_EARLYCON_DECLARE(sunplus_uart, "sunplus,sp7021-uart",
+sunplus_uart_early_setup);

--
With Best Regards,
Andy Shevchenko