[PATCH 0/3] a few fixes for sprd serial driver

[PATCH 0/3] a few fixes for sprd serial driver

[Chunyan Zhang]

From: Chunyan Zhang <chunyan.zhang@unisoc.com>

Recently I found that sprd serial driver has a few problems if the clock
drivers were built as modules which were needed by serial driver, and the
serial driver would be postponed until those clock modules were installed.

Chunyan Zhang (3):
  serial: sprd: check console via stdout-path in addition
  serial: sprd: remove __init from sprd_console_setup
  serial: sprd: cleanup the sprd_port when return -EPROBE_DEFER

 drivers/tty/serial/sprd_serial.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

-- 
2.20.1

[PATCH 1/3] serial: sprd: check console via stdout-path in addition

[Chunyan Zhang]

From: Chunyan Zhang <chunyan.zhang@unisoc.com>

The SPRD serial driver need to know which serial port would be used as
console in an early period during initialization, the purpose is to
keep the console port alive as possible even if there's some error
caused by no clock configured under serial devicetree nodes. But with
the patch [1], the console port couldn't be identified if missing
console command line.

So this patch adds using another interface to do check by reading
stdout-path.

[1] https://lore.kernel.org/lkml/20190826072929.7696-4-zhang.lyra@gmail.com/

Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/tty/serial/sprd_serial.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 3d3c70634589..18706333f146 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -1147,7 +1147,8 @@ static bool sprd_uart_is_console(struct uart_port *uport)
 {
 	struct console *cons = sprd_uart_driver.cons;
 
-	if (cons && cons->index >= 0 && cons->index == uport->line)
+	if ((cons && cons->index >= 0 && cons->index == uport->line) ||
+	    of_console_check(uport->dev->of_node, SPRD_TTY_NAME, uport->line))
 		return true;
 
 	return false;
-- 
2.20.1

Re: [PATCH 1/3] serial: sprd: check console via stdout-path in addition

[Baolin Wang]

On Mon, Mar 16, 2020 at 6:19 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
>
> From: Chunyan Zhang <chunyan.zhang@unisoc.com>
>
> The SPRD serial driver need to know which serial port would be used as
> console in an early period during initialization, the purpose is to
> keep the console port alive as possible even if there's some error
> caused by no clock configured under serial devicetree nodes. But with
> the patch [1], the console port couldn't be identified if missing
> console command line.
>
> So this patch adds using another interface to do check by reading
> stdout-path.
>
> [1] https://lore.kernel.org/lkml/20190826072929.7696-4-zhang.lyra@gmail.com/
>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>

Reviewed-by: Baolin Wang <baolin.wang7@gmail.com>

> ---
>  drivers/tty/serial/sprd_serial.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> index 3d3c70634589..18706333f146 100644
> --- a/drivers/tty/serial/sprd_serial.c
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -1147,7 +1147,8 @@ static bool sprd_uart_is_console(struct uart_port *uport)
>  {
>         struct console *cons = sprd_uart_driver.cons;
>
> -       if (cons && cons->index >= 0 && cons->index == uport->line)
> +       if ((cons && cons->index >= 0 && cons->index == uport->line) ||
> +           of_console_check(uport->dev->of_node, SPRD_TTY_NAME, uport->line))
>                 return true;
>
>         return false;
> --
> 2.20.1
>


-- 
Baolin Wang

[PATCH 2/3] serial: sprd: remove __init from sprd_console_setup

[Chunyan Zhang]

From: Chunyan Zhang <chunyan.zhang@unisoc.com>

The function sprd_console_setup() would be called from .probe() which can
be called after freeing __init functions, for example the .probe() would
return -EPROBE_DEFER since it depends on clock modules.

Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/tty/serial/sprd_serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 18706333f146..914862844790 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -1013,7 +1013,7 @@ static void sprd_console_write(struct console *co, const char *s,
 		spin_unlock_irqrestore(&port->lock, flags);
 }
 
-static int __init sprd_console_setup(struct console *co, char *options)
+static int sprd_console_setup(struct console *co, char *options)
 {
 	struct sprd_uart_port *sprd_uart_port;
 	int baud = 115200;
-- 
2.20.1

Re: [PATCH 2/3] serial: sprd: remove __init from sprd_console_setup

[Baolin Wang]

On Mon, Mar 16, 2020 at 6:19 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
>
> From: Chunyan Zhang <chunyan.zhang@unisoc.com>
>
> The function sprd_console_setup() would be called from .probe() which can
> be called after freeing __init functions, for example the .probe() would
> return -EPROBE_DEFER since it depends on clock modules.
>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>

Reviewed-by: Baolin Wang <baolin.wang7@gmail.com>

> ---
>  drivers/tty/serial/sprd_serial.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> index 18706333f146..914862844790 100644
> --- a/drivers/tty/serial/sprd_serial.c
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -1013,7 +1013,7 @@ static void sprd_console_write(struct console *co, const char *s,
>                 spin_unlock_irqrestore(&port->lock, flags);
>  }
>
> -static int __init sprd_console_setup(struct console *co, char *options)
> +static int sprd_console_setup(struct console *co, char *options)
>  {
>         struct sprd_uart_port *sprd_uart_port;
>         int baud = 115200;
> --
> 2.20.1
>


-- 
Baolin Wang

[PATCH 3/3] serial: sprd: cleanup the sprd_port when return -EPROBE_DEFER

[Chunyan Zhang]

From: Chunyan Zhang <chunyan.zhang@unisoc.com>

cleanup the sprd_port for the device when its .probe() would be called
later, and then alloc memory for its sprd_port again.

Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/tty/serial/sprd_serial.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 914862844790..9917d7240172 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -1230,8 +1230,13 @@ static int sprd_probe(struct platform_device *pdev)
 	up->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SPRD_CONSOLE);
 
 	ret = sprd_clk_init(up);
-	if (ret)
+	if (ret) {
+		if (ret == -EPROBE_DEFER) {
+			devm_kfree(&pdev->dev, sprd_port[index]);
+			sprd_port[index] = NULL;
+		}
 		return ret;
+	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	up->membase = devm_ioremap_resource(&pdev->dev, res);
-- 
2.20.1

Re: [PATCH 3/3] serial: sprd: cleanup the sprd_port when return -EPROBE_DEFER

[Baolin Wang]

On Mon, Mar 16, 2020 at 6:19 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
>
> From: Chunyan Zhang <chunyan.zhang@unisoc.com>
>
> cleanup the sprd_port for the device when its .probe() would be called
> later, and then alloc memory for its sprd_port again.
>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> ---
>  drivers/tty/serial/sprd_serial.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> index 914862844790..9917d7240172 100644
> --- a/drivers/tty/serial/sprd_serial.c
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -1230,8 +1230,13 @@ static int sprd_probe(struct platform_device *pdev)
>         up->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SPRD_CONSOLE);
>
>         ret = sprd_clk_init(up);
> -       if (ret)
> +       if (ret) {
> +               if (ret == -EPROBE_DEFER) {

I think we can remove the condition and devm_kfree() here, just
simplify the code as below?
if (ret) {
       sprd_port[index] = NULL;
       return ret;
}

> +                       devm_kfree(&pdev->dev, sprd_port[index]);
> +                       sprd_port[index] = NULL;
> +               }
>                 return ret;
> +       }
>
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>         up->membase = devm_ioremap_resource(&pdev->dev, res);
> --
> 2.20.1
>


-- 
Baolin Wang

Re: [PATCH 3/3] serial: sprd: cleanup the sprd_port when return -EPROBE_DEFER

[Chunyan Zhang]

On Tue, 17 Mar 2020 at 14:22, Baolin Wang <baolin.wang7@gmail.com> wrote:
>
> On Mon, Mar 16, 2020 at 6:19 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
> >
> > From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> >
> > cleanup the sprd_port for the device when its .probe() would be called
> > later, and then alloc memory for its sprd_port again.
> >
> > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > ---
> >  drivers/tty/serial/sprd_serial.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> > index 914862844790..9917d7240172 100644
> > --- a/drivers/tty/serial/sprd_serial.c
> > +++ b/drivers/tty/serial/sprd_serial.c
> > @@ -1230,8 +1230,13 @@ static int sprd_probe(struct platform_device *pdev)
> >         up->has_sysrq = IS_ENABLED(CONFIG_SERIAL_SPRD_CONSOLE);
> >
> >         ret = sprd_clk_init(up);
> > -       if (ret)
> > +       if (ret) {
> > +               if (ret == -EPROBE_DEFER) {
>
> I think we can remove the condition and devm_kfree() here, just
> simplify the code as below?
> if (ret) {
>        sprd_port[index] = NULL;
>        return ret;
> }

Humm.. I admit that here's a little confused.
Let assume we don't have aliases for serial nodes,  and we set both
earlycon and console via bootargs, what will happen?
We can simplify the code like above and we have to ensure all
platforms have serial aliases in their devicetree, then we also can
simplify the process of getting port index with of_alias_get_id()
only.
We can discuss further offline, and also save community resource :)

Thanks,
Chunyan

>
> > +                       devm_kfree(&pdev->dev, sprd_port[index]);
> > +                       sprd_port[index] = NULL;
> > +               }
> >                 return ret;
> > +       }
> >
> >         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >         up->membase = devm_ioremap_resource(&pdev->dev, res);
> > --
> > 2.20.1
> >
>
>
> --
> Baolin Wang