This reverts commit a659652f6169240a5818cb244b280c5a362ef5a4.
This broke the earlycon on LS1021A processors because the order of the
earlycon_setup() functions were changed. Before the commit the normal
lpuart32_early_console_setup() was called. After the commit the
lpuart32_imx_early_console_setup() is called instead.
Fixes: a659652f6169 ("tty: serial: fsl_lpuart: drop EARLYCON_DECLARE")
Signed-off-by: Michael Walle <[email protected]>
---
drivers/tty/serial/fsl_lpuart.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 91e2805e6441..27fdc131c352 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -2390,6 +2390,8 @@ static int __init lpuart32_imx_early_console_setup(struct earlycon_device *devic
OF_EARLYCON_DECLARE(lpuart, "fsl,vf610-lpuart", lpuart_early_console_setup);
OF_EARLYCON_DECLARE(lpuart32, "fsl,ls1021a-lpuart", lpuart32_early_console_setup);
OF_EARLYCON_DECLARE(lpuart32, "fsl,imx7ulp-lpuart", lpuart32_imx_early_console_setup);
+EARLYCON_DECLARE(lpuart, lpuart_early_console_setup);
+EARLYCON_DECLARE(lpuart32, lpuart32_early_console_setup);
#define LPUART_CONSOLE (&lpuart_console)
#define LPUART32_CONSOLE (&lpuart32_console)
--
2.20.1
> Subject: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop EARLYCON_DECLARE"
>
> This reverts commit a659652f6169240a5818cb244b280c5a362ef5a4.
>
> This broke the earlycon on LS1021A processors because the order of the
> earlycon_setup() functions were changed. Before the commit the normal
> lpuart32_early_console_setup() was called. After the commit the
> lpuart32_imx_early_console_setup() is called instead.
How do you pass earlycon args to kernel?
Regards,
Peng.
>
> Fixes: a659652f6169 ("tty: serial: fsl_lpuart: drop EARLYCON_DECLARE")
> Signed-off-by: Michael Walle <[email protected]>
> ---
> drivers/tty/serial/fsl_lpuart.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c index
> 91e2805e6441..27fdc131c352 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -2390,6 +2390,8 @@ static int __init
> lpuart32_imx_early_console_setup(struct earlycon_device *devic
> OF_EARLYCON_DECLARE(lpuart, "fsl,vf610-lpuart",
> lpuart_early_console_setup); OF_EARLYCON_DECLARE(lpuart32,
> "fsl,ls1021a-lpuart", lpuart32_early_console_setup);
> OF_EARLYCON_DECLARE(lpuart32, "fsl,imx7ulp-lpuart",
> lpuart32_imx_early_console_setup);
> +EARLYCON_DECLARE(lpuart, lpuart_early_console_setup);
> +EARLYCON_DECLARE(lpuart32, lpuart32_early_console_setup);
>
> #define LPUART_CONSOLE (&lpuart_console)
> #define LPUART32_CONSOLE (&lpuart32_console)
> --
> 2.20.1
Hi,
Am 2020-02-21 02:30, schrieb Peng Fan:
>> Subject: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop
>> EARLYCON_DECLARE"
>>
>> This reverts commit a659652f6169240a5818cb244b280c5a362ef5a4.
>>
>> This broke the earlycon on LS1021A processors because the order of the
>> earlycon_setup() functions were changed. Before the commit the normal
>> lpuart32_early_console_setup() was called. After the commit the
>> lpuart32_imx_early_console_setup() is called instead.
>
> How do you pass earlycon args to kernel?
earlycon=lpuart32,mmio32be,0x2950000,115200
please note that there are two possible declarations: (1) an OF/ACPI
based
earlycon, eg just "earlycon" on the bootargs and (2) an elaborate one
where
you can give the offset and access method yourself, eg. the one from
above.
(1) will still work even with the EARLYCON_DECLARE() removed. But (2)
will
search through all possible
OF_DELARE_EARLYCON(lpuart32,..)
EARLYCON_DECLARE(lpuart32,..)
and doesn't take the compatible into account. So which setup function is
actually called depends on (a) the order of the OF_DECLARE_EARLYCON()
and
EARLYCON_DECLARE() statements and (b) on the compiler (thats just a
guess!). For me, the order in which it will actually end up in the
__earlycon_table is reversed, eg. the last one is called. So now that
you've removed the EARLYCON_DECLARE() the last one is the imx setup
function which will add the reg offset and doesn't work on LS1021A.
I've proposed a fix of the underlying problem [1]. But that fix also
requires
the EARLYCON_DECLARE() in this driver.
-michael
[1]
https://lore.kernel.org/linux-serial/[email protected]/
> Subject: Re: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop
> EARLYCON_DECLARE"
>
> Hi,
>
> Am 2020-02-21 02:30, schrieb Peng Fan:
> >> Subject: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop
> >> EARLYCON_DECLARE"
> >>
> >> This reverts commit a659652f6169240a5818cb244b280c5a362ef5a4.
> >>
> >> This broke the earlycon on LS1021A processors because the order of
> >> the
> >> earlycon_setup() functions were changed. Before the commit the normal
> >> lpuart32_early_console_setup() was called. After the commit the
> >> lpuart32_imx_early_console_setup() is called instead.
> >
> > How do you pass earlycon args to kernel?
>
> earlycon=lpuart32,mmio32be,0x2950000,115200
>
> please note that there are two possible declarations: (1) an OF/ACPI based
> earlycon, eg just "earlycon" on the bootargs and (2) an elaborate one where
> you can give the offset and access method yourself, eg. the one from above.
>
> (1) will still work even with the EARLYCON_DECLARE() removed. But (2) will
> search through all possible
> OF_DELARE_EARLYCON(lpuart32,..)
> EARLYCON_DECLARE(lpuart32,..)
>
> and doesn't take the compatible into account. So which setup function is
> actually called depends on (a) the order of the OF_DECLARE_EARLYCON() and
> EARLYCON_DECLARE() statements and (b) on the compiler (thats just a guess!).
> For me, the order in which it will actually end up in the __earlycon_table is
> reversed, eg. the last one is called. So now that you've removed the
> EARLYCON_DECLARE() the last one is the imx setup function which will add
> the reg offset and doesn't work on LS1021A.
You mean the OF_DECLARD_EARLYCON for i.MX7ULP will be put before LS1021A
in Image? I am not sure about this. If this is true, you could try below diff, to see
whether it works. i.MX always use little endian.
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 91e2805e6441..1b0aa3b836c5 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -2381,8 +2381,10 @@ static int __init lpuart32_imx_early_console_setup(struct earlycon_device *devic
if (!device->port.membase)
return -ENODEV;
- device->port.iotype = UPIO_MEM32;
- device->port.membase += IMX_REG_OFF;
+ if (device->port.iotype != UPIO_MEM32BE) {
+ device->port.iotype = UPIO_MEM32;
+ device->port.membase += IMX_REG_OFF;
+ }
device->con->write = lpuart32_early_write;
return 0;
Thanks,
Peng.
>
> I've proposed a fix of the underlying problem [1]. But that fix also requires the
> EARLYCON_DECLARE() in this driver.
>
>
> -michael
>
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ke
> rnel.org%2Flinux-serial%2F20200220174607.24285-1-michael%40walle.cc%
> 2F&data=02%7C01%7Cpeng.fan%40nxp.com%7Cc411bf1d5d45435be2a
> 308d7b6b14a2c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
> 7178744883261681&sdata=gjl8rie%2FXhUQ0pxcAbbDI4NGqGDYj1jpqfR
> mRi%2FAgFk%3D&reserved=0
Hi,
Am 2020-02-24 02:12, schrieb Peng Fan:
>> Subject: Re: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop
>> EARLYCON_DECLARE"
>>
>> Hi,
>>
>> Am 2020-02-21 02:30, schrieb Peng Fan:
>> >> Subject: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop
>> >> EARLYCON_DECLARE"
>> >>
>> >> This reverts commit a659652f6169240a5818cb244b280c5a362ef5a4.
>> >>
>> >> This broke the earlycon on LS1021A processors because the order of
>> >> the
>> >> earlycon_setup() functions were changed. Before the commit the normal
>> >> lpuart32_early_console_setup() was called. After the commit the
>> >> lpuart32_imx_early_console_setup() is called instead.
>> >
>> > How do you pass earlycon args to kernel?
>>
>> earlycon=lpuart32,mmio32be,0x2950000,115200
>>
>> please note that there are two possible declarations: (1) an OF/ACPI
>> based
>> earlycon, eg just "earlycon" on the bootargs and (2) an elaborate one
>> where
>> you can give the offset and access method yourself, eg. the one from
>> above.
>>
>> (1) will still work even with the EARLYCON_DECLARE() removed. But (2)
>> will
>> search through all possible
>> OF_DELARE_EARLYCON(lpuart32,..)
>> EARLYCON_DECLARE(lpuart32,..)
>>
>> and doesn't take the compatible into account. So which setup function
>> is
>> actually called depends on (a) the order of the OF_DECLARE_EARLYCON()
>> and
>> EARLYCON_DECLARE() statements and (b) on the compiler (thats just a
>> guess!).
>> For me, the order in which it will actually end up in the
>> __earlycon_table is
>> reversed, eg. the last one is called. So now that you've removed the
>> EARLYCON_DECLARE() the last one is the imx setup function which will
>> add
>> the reg offset and doesn't work on LS1021A.
>
> You mean the OF_DECLARD_EARLYCON for i.MX7ULP will be put before
> LS1021A
> in Image? I am not sure about this. If this is true, you could try
> below diff, to see
well, you could have a look at the disassembly; so no need for real
hardware,
if thats what you're missing.
> whether it works. i.MX always use little endian.
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c
> b/drivers/tty/serial/fsl_lpuart.c
> index 91e2805e6441..1b0aa3b836c5 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -2381,8 +2381,10 @@ static int __init
> lpuart32_imx_early_console_setup(struct earlycon_device *devic
> if (!device->port.membase)
> return -ENODEV;
>
> - device->port.iotype = UPIO_MEM32;
> - device->port.membase += IMX_REG_OFF;
> + if (device->port.iotype != UPIO_MEM32BE) {
> + device->port.iotype = UPIO_MEM32;
> + device->port.membase += IMX_REG_OFF;
> + }
> device->con->write = lpuart32_early_write;
>
> return 0;
while that would also fix the problem (I guess; I'm too lazy to try), it
is
really bad because:
(A) it doesn't fix the underlying problem that there are multiple setup
functions for lpuart32 (and this is not only a problem for
lpuart32)
(B) it would've broken the lpuart32 for IMX in the form (2), if it had
ever
been working.
(C) it will break if some developer adds a new setup function to the
end of
the list
(D) you basically make every setup() function behave like the ls1021a
setup()
if its called in form (2)
Please, lets try to find a more generic solution for this, like my
proposed
patch [1].
-michael
[1]
https://lore.kernel.org/linux-serial/[email protected]