[v2,3/7] tty/serial: convert 8250 to generic earlycon

Commit Message

Rob Herring April 28, 2014, 11:24 p.m. UTC

On Sat, Apr 26, 2014 at 1:29 AM, Yinghai Lu <[e-mail]> wrote:
> On Fri, Apr 18, 2014 at 3:19 PM, Rob Herring <[e-mail]> wrote:
>> From: Rob Herring <[e-mail]>
>>
>> With the generic earlycon infrastructure in place, convert the 8250
>> early console to use it.
>>
>> Signed-off-by: Rob Herring <[e-mail]>
>> Cc: Greg Kroah-Hartman <[e-mail]>
>> Cc: Jiri Slaby <[e-mail]>
>> ---
>>  drivers/tty/serial/8250/8250_early.c | 138 ++++-------------------------------
>>  drivers/tty/serial/8250/Kconfig      |   1 +
>>  2 files changed, 16 insertions(+), 123 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
>> index c100d63..e83c9db 100644
>> --- a/drivers/tty/serial/8250/8250_early.c
>> +++ b/drivers/tty/serial/8250/8250_early.c
>> @@ -35,18 +35,8 @@
> ...
>> -
>> -int __init setup_early_serial8250_console(char *cmdline)
>
> You removed global function, but still left declaration and calling...
>
> arch/mips/mti-malta/malta-init.c:
> setup_early_serial8250_console(console_string);
> drivers/firmware/pcdp.c:        return setup_early_serial8250_console(options);
> include/linux/serial_8250.h:extern int
> setup_early_serial8250_console(char *cmdline);

Thanks for finding these. I missed them in my build tests. This should fix them:

        struct earlycon_device *device = early_device;

Comments

Yinghai Lu April 29, 2014, 2:56 a.m. UTC | #1

On Mon, Apr 28, 2014 at 4:24 PM, Rob Herring <[e-mail]> wrote:
> On Sat, Apr 26, 2014 at 1:29 AM, Yinghai Lu <[e-mail]> wrote:
>
> Thanks for finding these. I missed them in my build tests. This should fix them:
>
> diff --git a/drivers/tty/serial/8250/8250_early.c
> b/drivers/tty/serial/8250/8250_early.c
> index e83c9db..2094c3b 100644
> --- a/drivers/tty/serial/8250/8250_early.c
> +++ b/drivers/tty/serial/8250/8250_early.c
> @@ -156,6 +156,11 @@ static int __init early_serial8250_setup(struct
> earlycon_device *device,
>  EARLYCON_DECLARE(uart8250, early_serial8250_setup);
>  EARLYCON_DECLARE(uart, early_serial8250_setup);
>
> +int __init setup_early_serial8250_console(char *cmdline)
> +{
> +       return setup_earlycon(cmdline, "uart8250", early_serial8250_setup);
> +}
> +
>  int serial8250_find_port_for_earlycon(void)
>  {
>         struct earlycon_device *device = early_device;

that only handle "uart8250,", may need to add more lines to handle "uart,"

+int __init setup_early_serial8250_console(char *cmdline)
+{
+       char *options;
+       options = strstr(cmdline, "uart8250,");
+       if (options)
+               return setup_earlycon(cmdline, "uart8250",
early_serial8250_setup);
+
+       options = strstr(cmdline, "uart,");
+       if (options)
+              return setup_earlycon(cmdline, "uart", early_serial8250_setup);
+
+      return 0;
+}
+

Thanks

Yinghai

Rob Herring April 29, 2014, 3:16 p.m. UTC | #2

On Mon, Apr 28, 2014 at 9:56 PM, Yinghai Lu <[e-mail]> wrote:
> On Mon, Apr 28, 2014 at 4:24 PM, Rob Herring <[e-mail]> wrote:
>> On Sat, Apr 26, 2014 at 1:29 AM, Yinghai Lu <[e-mail]> wrote:
>>
>> Thanks for finding these. I missed them in my build tests. This should fix them:
>>
>> diff --git a/drivers/tty/serial/8250/8250_early.c
>> b/drivers/tty/serial/8250/8250_early.c
>> index e83c9db..2094c3b 100644
>> --- a/drivers/tty/serial/8250/8250_early.c
>> +++ b/drivers/tty/serial/8250/8250_early.c
>> @@ -156,6 +156,11 @@ static int __init early_serial8250_setup(struct
>> earlycon_device *device,
>>  EARLYCON_DECLARE(uart8250, early_serial8250_setup);
>>  EARLYCON_DECLARE(uart, early_serial8250_setup);
>>
>> +int __init setup_early_serial8250_console(char *cmdline)
>> +{
>> +       return setup_earlycon(cmdline, "uart8250", early_serial8250_setup);
>> +}
>> +
>>  int serial8250_find_port_for_earlycon(void)
>>  {
>>         struct earlycon_device *device = early_device;
>
> that only handle "uart8250,", may need to add more lines to handle "uart,"


That is on purpose because the only 2 users use uart8250. I consider
this a legacy interface and use of "uart" is horrible because there
are lots of uarts which are not 8250.

Rob

>
> +int __init setup_early_serial8250_console(char *cmdline)
> +{
> +       char *options;
> +       options = strstr(cmdline, "uart8250,");
> +       if (options)
> +               return setup_earlycon(cmdline, "uart8250",
> early_serial8250_setup);
> +
> +       options = strstr(cmdline, "uart,");
> +       if (options)
> +              return setup_earlycon(cmdline, "uart", early_serial8250_setup);
> +
> +      return 0;
> +}
> +
>
> Thanks
>
> Yinghai

Yinghai Lu April 29, 2014, 6:22 p.m. UTC | #3

On Tue, Apr 29, 2014 at 8:16 AM, Rob Herring <[e-mail]> wrote:
> On Mon, Apr 28, 2014 at 9:56 PM, Yinghai Lu <[e-mail]> wrote:
>> On Mon, Apr 28, 2014 at 4:24 PM, Rob Herring <[e-mail]> wrote:
>>> On Sat, Apr 26, 2014 at 1:29 AM, Yinghai Lu <[e-mail]> wrote:
>>>
>>> Thanks for finding these. I missed them in my build tests. This should fix them:
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_early.c
>>> b/drivers/tty/serial/8250/8250_early.c
>>> index e83c9db..2094c3b 100644
>>> --- a/drivers/tty/serial/8250/8250_early.c
>>> +++ b/drivers/tty/serial/8250/8250_early.c
>>> @@ -156,6 +156,11 @@ static int __init early_serial8250_setup(struct
>>> earlycon_device *device,
>>>  EARLYCON_DECLARE(uart8250, early_serial8250_setup);
>>>  EARLYCON_DECLARE(uart, early_serial8250_setup);
>>>
>>> +int __init setup_early_serial8250_console(char *cmdline)
>>> +{
>>> +       return setup_earlycon(cmdline, "uart8250", early_serial8250_setup);
>>> +}
>>> +
>>>  int serial8250_find_port_for_earlycon(void)
>>>  {
>>>         struct earlycon_device *device = early_device;
>>
>> that only handle "uart8250,", may need to add more lines to handle "uart,"
>
>
> That is on purpose because the only 2 users use uart8250. I consider
> this a legacy interface and use of "uart" is horrible because there
> are lots of uarts which are not 8250.
>
> Rob
>
>>
>> +int __init setup_early_serial8250_console(char *cmdline)
>> +{
>> +       char *options;
>> +       options = strstr(cmdline, "uart8250,");
>> +       if (options)
>> +               return setup_earlycon(cmdline, "uart8250",
>> early_serial8250_setup);
>> +
>> +       options = strstr(cmdline, "uart,");
>> +       if (options)
>> +              return setup_earlycon(cmdline, "uart", early_serial8250_setup);
>> +
>> +      return 0;
>> +}

You want to obsolete "console=uart,io,0x3f8,115200n8" ?

 Let's check with Andrew. He suggested to use uart and uart8250 at that time.

Thanks

Yinghai

Rob Herring April 29, 2014, 8:41 p.m. UTC | #4

On Tue, Apr 29, 2014 at 1:22 PM, Yinghai Lu <[e-mail]> wrote:
> On Tue, Apr 29, 2014 at 8:16 AM, Rob Herring <[e-mail]> wrote:
>> On Mon, Apr 28, 2014 at 9:56 PM, Yinghai Lu <[e-mail]> wrote:
>>> On Mon, Apr 28, 2014 at 4:24 PM, Rob Herring <[e-mail]> wrote:
>>>> On Sat, Apr 26, 2014 at 1:29 AM, Yinghai Lu <[e-mail]> wrote:
>>>>
>>>> Thanks for finding these. I missed them in my build tests. This should fix them:
>>>>
>>>> diff --git a/drivers/tty/serial/8250/8250_early.c
>>>> b/drivers/tty/serial/8250/8250_early.c
>>>> index e83c9db..2094c3b 100644
>>>> --- a/drivers/tty/serial/8250/8250_early.c
>>>> +++ b/drivers/tty/serial/8250/8250_early.c
>>>> @@ -156,6 +156,11 @@ static int __init early_serial8250_setup(struct
>>>> earlycon_device *device,
>>>>  EARLYCON_DECLARE(uart8250, early_serial8250_setup);
>>>>  EARLYCON_DECLARE(uart, early_serial8250_setup);
>>>>
>>>> +int __init setup_early_serial8250_console(char *cmdline)
>>>> +{
>>>> +       return setup_earlycon(cmdline, "uart8250", early_serial8250_setup);
>>>> +}
>>>> +
>>>>  int serial8250_find_port_for_earlycon(void)
>>>>  {
>>>>         struct earlycon_device *device = early_device;
>>>
>>> that only handle "uart8250,", may need to add more lines to handle "uart,"
>>
>>
>> That is on purpose because the only 2 users use uart8250. I consider
>> this a legacy interface and use of "uart" is horrible because there
>> are lots of uarts which are not 8250.
>>
>> Rob
>>
>>>
>>> +int __init setup_early_serial8250_console(char *cmdline)
>>> +{
>>> +       char *options;
>>> +       options = strstr(cmdline, "uart8250,");
>>> +       if (options)
>>> +               return setup_earlycon(cmdline, "uart8250",
>>> early_serial8250_setup);
>>> +
>>> +       options = strstr(cmdline, "uart,");
>>> +       if (options)
>>> +              return setup_earlycon(cmdline, "uart", early_serial8250_setup);
>>> +
>>> +      return 0;
>>> +}
>
> You want to obsolete "console=uart,io,0x3f8,115200n8" ?
>
>  Let's check with Andrew. He suggested to use uart and uart8250 at that time.

No, that is not what I'm saying. For the 2 callers of
setup_early_serial8250_console which are crafting a console string
from firmware data, they can and do use uart8250. I don't expect this
mechanism for setting up early console to expand to other users. The
whole point of this series is to allow any uart to be supported for
earlycon. For anyone using the kernel command line, both uart and
uart8250 are still supported.

Rob

Patch

diff --git a/drivers/tty/serial/8250/8250_early.c
b/drivers/tty/serial/8250/8250_early.c
index e83c9db..2094c3b 100644
--- a/drivers/tty/serial/8250/8250_early.c
+++ b/drivers/tty/serial/8250/8250_early.c
@@ -156,6 +156,11 @@  static int __init early_serial8250_setup(struct
earlycon_device *device,
 EARLYCON_DECLARE(uart8250, early_serial8250_setup);
 EARLYCON_DECLARE(uart, early_serial8250_setup);

+int __init setup_early_serial8250_console(char *cmdline)
+{
+       return setup_earlycon(cmdline, "uart8250", early_serial8250_setup);
+}
+
 int serial8250_find_port_for_earlycon(void)
 {