RE: [PATCH v3 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver

From: Hammer Hsieh 謝宏孟
Date: Mon Nov 22 2021 - 23:14:35 EST

Next message: NeilBrown: "Re: [PATCH] MM: discard __GFP_ATOMIC"
Previous message: Aswath Govindraju: "Re: [PATCH RFC v2 3/4] mux: Add support for reading mux enable state from DT"
In reply to: Andy Shevchenko: "Re: [PATCH v3 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi, Andy Shevchenko :

I may not fully understand all your comments.
But most of them I will modify when next submit.
I try to answer all your comments below.
And also have some question about some of your comments.

Thanks
Regards,
Hammer

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Friday, November 19, 2021 5:35 PM
> 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 v3 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver
>
> On Fri, Nov 19, 2021 at 8:14 AM Hammer Hsieh <hammerh0314@xxxxxxxxx>
> wrote:
> >
> > Add Sunplus SoC UART Driver
>
> Thanks for update, my comments below.
>
> ...
>
> > drivers/tty/serial/sunplus-uart.c | 903
> > ++++++++++++++++++++++++++++++++++++++
>
> I believe 50 LOCs easily can be removed (see below for a few examples I caught
> just by looking into this for less than 1 minute).
>
> ...
>
> > include/soc/sunplus/sp_uart.h | 93 ++++
>
> Why do you need this header?
>
Ok, will remove header file, reg define will move to sunplus-uart.c

> ...
>
> > +config SERIAL_SUNPLUS
> > + bool "Sunplus UART support"
>
> No module? Why?

Will add tristate for it. I will confirm load/unload *.ko for it.

>
> > + depends on OF
>
> No COMPILE_TEST, why?

Will add COMPILE_TEST for it.

>
> > + 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 ttySx.
>
> If it's ttySx, it most probably 8250 compatible, no?
>

No , it is a single chip, not 8250 compatible.
I plan to use 'ttySUP' for Sunplus SoC UART.
We define PORT_SUNPLUS 123 at /include/uapi/linux/serial_core.h
Is that ok?

> ...
>
> > +/*
> > + * Sunplus SoC UART driver
> > + *
> > + * Author: Hammer Hsieh <hammer.hsieh@xxxxxxxxxxx>
> > + * Tony Huang <tony.huang@xxxxxxxxxxx>
> > + * Wells Lu <wells.lu@xxxxxxxxxxx>
>
> > + *
>
> Redundant.
>

Ok , will remove it.

> > + */
>
> ...
>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/console.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/tty.h>
> > +#include <linux/tty_flip.h>
> > +#include <linux/of_platform.h>
> > +#include <asm/irq.h>
> > +#include <linux/sysrq.h>
> > +#include <linux/serial_core.h>
> > +#include <linux/clk.h>
> > +#include <linux/reset.h>
> > +#include <linux/io.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/of.h>
> > +#include <linux/delay.h>
>
> Sort above alphabetically and get rid of unneeded headers.
>
> > +#include <soc/sunplus/sp_uart.h>
>

Ok , will do it.

> ...
>
> > +#define UART_NR 5
>
> We have this as a config option, why do you need a separate one?
>

Can I keep UART_NR in sunplus-uart.c ?
If all uart driver define CONFIG_xxx_UART_NR in Kconfig.
Kconfig will get big very fast.
In sunplus-uart.c just only one line code.
In Kconfig takes 8 ~ 10 line code.

> ...
>
> > +static struct uart_driver sunplus_uart_driver;
>
> Why global variables?
>

It is define for struct console { }
I will move it to close to struct console { }

> ...
> > +struct sunplus_uart_port {
> > + char name[16];
>
> uart_port has a name, what is this one for?
>

Ok , will remove it.

> > + struct uart_port port;
>
> It's better to make it first in the structure to optimize container_of() away.
>
> > + struct clk *clk;
> > + struct reset_control *rstc;
> > +};
>

Ok, thanks for the information.

> ...
>
> > +static inline u32 sp_uart_line_status_tx_buf_not_full(struct
> > +uart_port *port) {
> > + return ((readl(port->membase + SP_UART_LSR) &
> SP_UART_LSR_TX)
> > + ? SP_UART_LSR_TX_NOT_FULL : 0);
>
> Use temporary variables for better reading. Here and everywhere else where
> it's applicable.
>
> > +}
>

Ok, will modify it.

> ...
>
> > + writel(mcr, port->membase + SP_UART_MCR);
> > +
>
> Redundant blank line. Check everywhere you have no such waste space.
>

Ok, will remove it.

> ...
>
> > +static void sunplus_enable_ms(struct uart_port *port) {
> > + /* Do nothing */
> > +}
>
> Is this stub needed at all?
>

No, will remove it.

> ...
>
> > +
> > +
>
> One blank line is enough.
>

Ok, will remove it.

> ...
>
> > + if (port->cons == NULL)
>
> Don't we have a special API to check if the port is a console or not?
>

Can you show me the API ?

> > + dev_err(port->dev,
> "UART%d,
> > + SP_UART_LSR_FE\n", port->line);
>
> ...
>
> > +#ifdef CONFIG_PM_RUNTIME_UART
> > + if (port->line > 0) {
> > + ret = pm_runtime_get_sync(port->dev);
> > + if (ret < 0)
> > + goto out;
> > + }
> > +#endif
>
> Can we postpone implementation of it right now, please?
> Can you test this [1] series instead?
>
> [1]:
> https://lore.kernel.org/linux-serial/20211115084203.56478-1-tony@atomide.c
> om/T/#u
>

I don't understand what's going on this issue?

> ...
>
> > + /* Disable flow control of Tx, so that queued data can be sent out
> > + * There is no way for s/w to let h/w abort in the middle of
> > + * transaction.
> > + * Don't reset module except it's in idle state. Otherwise, it
> > + might
>
> the module unless
> in an idle
>
> > + * cause bus to hang.
> > + */
>
> ...
>
> > + /*
> > + * Send all data in Tx FIFO before changing clock source,
> > + * it should be UART0 only
> > + */
> > + while (!(readl(port->membase + SP_UART_LSR) &
> SP_UART_LSR_TXE))
> > + ;
>
> We do not allow busyloops in the kernel. Consider readl_poll_timeout() or its
> atomic variant.
>

Ok, will modify for it.

> ...
>
> > + clk += baud >> 1;
> > + div = clk / baud;
> > + ext = div & 0x0F;
> > + div = (div >> 4) - 1;
> > + div_l = (div & 0xFF) | (ext << 12);
> > + div_h = div >> 8;
>
> Divisor voodoo should be explained in the comment.
>

Ok, will add explanation for it.

> ...
>
> > +static void sunplus_release_port(struct uart_port *port) { }
> > +
> > +static int sunplus_request_port(struct uart_port *port) {
> > + return 0;
> > +}
>
> > +static int sunplus_verify_port(struct uart_port *port, struct
> > +serial_struct *serial) {
> > + return -EINVAL;
> > +}
>
> Why the stubs?
>

Will modify it.
Like below:
if((type!= PORT_UNKNOWN) && (type != PORT_SUNPLUS))
return -EINVAL;
return 0;

> ...
>
> > +static inline void wait_for_xmitr(struct uart_port *port) {
> > + while (1) {
> > + if (sp_uart_line_status_tx_buf_not_full(port))
> > + break;
> > + }
>
> read_poll_timeout() or its atomic variant.
>

Ok, will modify it.

> > +}
>
> ...
>
> > + if (pdev->dev.of_node) {
>
> Redundant check

Sure, will remove it.

>
> > + pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");
> > + if (pdev->id < 0)
> > + pdev->id = of_alias_get_id(pdev->dev.of_node,
> "uart");
> > + }
>
> > +
>
> Redundant blank line.

Ok, will remove it.

>
> > + if (pdev->id < 0 || pdev->id >= UART_NR)
> > + return -EINVAL;
>
> ...
>
> > + sup = devm_kzalloc(&pdev->dev, sizeof(struct sunplus_uart_port),
> > + GFP_KERNEL);
>
> sizeof(*sup) and make it one line.
>

Ok, will modify it.

> > + if (!sup)
> > + return -ENOMEM;
>
> ...
>
> > + sup->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(sup->clk)) {
> > + dev_err(&pdev->dev, "unable to get UART clock\n");
> > + return PTR_ERR(sup->clk);
>
> Respect deferred probe by
>
> return dev_err_probe(...);
>

Ok, will modify it.

> > + }
>
> ...
>
> > + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> > + if (!res_mem)
> > + return -ENODEV;
> > +
>
> Redundant check, besides that...

Ok, will remove it.

>
> > + port->membase = devm_ioremap_resource(&pdev->dev,
> res_mem);
> > + if (IS_ERR(port->membase))
> > + return PTR_ERR(port->membase);
>
> ...there is an API that does these two in one.
>

API of_ioremap(res, offset, sizeof(xx), "xxx") can do it.
But we didn't use reg-names for it in dts.
I think it is better keep orginal platform_get_resource() + devm_ioremap_resource().

> ...
>
> > + port->uartclk = clk_get_rate(sup->clk);
> > + if (!port->uartclk) {
> > + ret = -EINVAL;
>
> > + goto err_disable_clk;
>
> Instead use devm_add_action_or_reset() as many other drivers do in the
> kernel.
>
> > + }
>

Ok, will modify it.

> ...
>
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0)
> > + return -ENODEV;
>
> What's wrong with the error code in the irq variable?
>

Will change 'return -ENODEV' to 'return irq'.

> ...
>
> > + port->private_data = container_of(&sup->port,
> > + struct sunplus_uart_port, port);
>
> What does this mean?
>

Previous code use it to store 'struct sunplus_uart_port' pointer, will remove it.

> ...
>
> > +static const struct of_device_id sp_uart_of_match[] = {
> > + { .compatible = "sunplus,sp7021-uart" },
>
> > + {},
>
> No comma for terminator entries.
>

Ok, will remove it.

> > +};
>
> ...
>
> > +static struct platform_driver sunplus_uart_platform_driver = {
> > + .probe = sunplus_uart_probe,
> > + .remove = sunplus_uart_remove,
> > + .suspend = sunplus_uart_suspend,
> > + .resume = sunplus_uart_resume,
> > + .driver = {
> > + .name = "sunplus-uart",
>
> > + .owner = THIS_MODULE,
>
> This is done by registration call, no?
>

Ok, will remove it.

> > + .of_match_table = of_match_ptr(sp_uart_of_match),
>
> Effectively a warning (but you don't see it since COMPILE_TEST is missed).
> Hint: drop of_match_ptr() completely.
>

Ok, thanks for the information. I will add COMPILE_TEST in Kconfig.

> > +#ifdef CONFIG_PM_RUNTIME_UART
> > + .pm = sunplus_uart_pm_ops,
> > +#endif
> > + }
> > +};
>
> ...
>
> > + ret = platform_driver_register(&sunplus_uart_platform_driver);
>
> > + if (ret != 0) {
>
> Keep the same style over the driver.
>

Ok, will modify it.

> > + uart_unregister_driver(&sunplus_uart_driver);
> > + return ret;
> > + }
>
> ...
>
> > + for (;;) {
> > + status = readl(port->membase + SP_UART_LSR);
> > + if ((status & SP_UART_LSR_TXE) == SP_UART_LSR_TXE)
> > + break;
> > + cpu_relax();
> > + }
>
> real_poll_timeout*()
>

Ok, will modify it.

> --
> With Best Regards,
> Andy Shevchenko

Next message: NeilBrown: "Re: [PATCH] MM: discard __GFP_ATOMIC"
Previous message: Aswath Govindraju: "Re: [PATCH RFC v2 3/4] mux: Add support for reading mux enable state from DT"
In reply to: Andy Shevchenko: "Re: [PATCH v3 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]