From: Chunyan Zhang <[email protected]>
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
From: Chunyan Zhang <[email protected]>
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 <[email protected]>
---
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
From: Chunyan Zhang <[email protected]>
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/[email protected]/
Signed-off-by: Chunyan Zhang <[email protected]>
---
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
From: Chunyan Zhang <[email protected]>
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 <[email protected]>
---
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
On Mon, Mar 16, 2020 at 6:19 PM Chunyan Zhang <[email protected]> wrote:
>
> From: Chunyan Zhang <[email protected]>
>
> 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/[email protected]/
>
> Signed-off-by: Chunyan Zhang <[email protected]>
Reviewed-by: Baolin Wang <[email protected]>
> ---
> 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
On Mon, Mar 16, 2020 at 6:19 PM Chunyan Zhang <[email protected]> wrote:
>
> From: Chunyan Zhang <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Baolin Wang <[email protected]>
> ---
> 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
On Mon, Mar 16, 2020 at 6:19 PM Chunyan Zhang <[email protected]> wrote:
>
> From: Chunyan Zhang <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On Tue, 17 Mar 2020 at 14:22, Baolin Wang <[email protected]> wrote:
>
> On Mon, Mar 16, 2020 at 6:19 PM Chunyan Zhang <[email protected]> wrote:
> >
> > From: Chunyan Zhang <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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