[PATCH] tty/serial: samsung: Add earlycon support

[PATCH] tty/serial: samsung: Add earlycon support

[Alim Akhtar]

Just realized that linux-arm-kernel email-id was wrongly typed.

CCing the correct linux-arm-kernel email-id now.

Sorry for the noise.


On Tue, Sep 16, 2014 at 5:02 PM, Alim Akhtar <alim.akhtar@samsung.com> wrote:
> Add earlycon support for the samsung serial port. This allows enabling
> the samsung serial port for console when early_params are parse and processed.
>
> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
>  Documentation/kernel-parameters.txt |    6 ++++++
>  drivers/tty/serial/Kconfig          |    1 +
>  drivers/tty/serial/samsung.c        |   17 +++++++++++++++++
>  3 files changed, 24 insertions(+)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 5ae8608..e01c0e5 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -936,6 +936,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>                         must already be setup and configured. Options are not
>                         yet supported.
>
> +               samsung,<addr>
> +                       Start an early, polled-mode console on a samsung serial
> +                       port at the specified address. The samsung serial port
> +                       must already be setup and configured. Options are not
> +                       yet supported.
> +
>                 smh     Use ARM semihosting calls for early console.
>
>         earlyprintk=    [X86,SH,BLACKFIN,ARM,M68k]
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 249e340..9d42ac8 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -257,6 +257,7 @@ config SERIAL_SAMSUNG_CONSOLE
>         bool "Support for console on Samsung SoC serial port"
>         depends on SERIAL_SAMSUNG=y
>         select SERIAL_CORE_CONSOLE
> +       select SERIAL_EARLYCON
>         help
>           Allow selection of the S3C24XX on-board serial ports for use as
>           an virtual console.
> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> index c78f43a..f32e9c8 100644
> --- a/drivers/tty/serial/samsung.c
> +++ b/drivers/tty/serial/samsung.c
> @@ -917,6 +917,7 @@ s3c24xx_serial_verify_port(struct uart_port *port, struct serial_struct *ser)
>  #ifdef CONFIG_SERIAL_SAMSUNG_CONSOLE
>
>  static struct console s3c24xx_serial_console;
> +static void s3c24xx_serial_console_putchar(struct uart_port *port, int ch);
>
>  static int __init s3c24xx_serial_console_init(void)
>  {
> @@ -926,6 +927,22 @@ static int __init s3c24xx_serial_console_init(void)
>  console_initcall(s3c24xx_serial_console_init);
>
>  #define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
> +static void samsung_early_write(struct console *con, const char *s, unsigned n)
> +{
> +       struct earlycon_device *dev = con->data;
> +
> +       uart_console_write(&dev->port, s, n, s3c24xx_serial_console_putchar);
> +}
> +
> +static int __init samsung_early_console_setup(struct earlycon_device *device,
> +                                             const char *opt)
> +{
> +       if (!device->port.membase)
> +               return -ENODEV;
> +       device->con->write = samsung_early_write;
> +       return 0;
> +}
> +EARLYCON_DECLARE(samsung, samsung_early_console_setup);
>  #else
>  #define S3C24XX_SERIAL_CONSOLE NULL
>  #endif
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Regards,
Alim

[PATCH] tty/serial: samsung: Add earlycon support

[Tomasz Figa]

Hi Alim,

Please see my comments inline.

On 16.09.2014 13:32, Alim Akhtar wrote:
> Add earlycon support for the samsung serial port. This allows enabling
> the samsung serial port for console when early_params are parse and processed.
> 
> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
>  Documentation/kernel-parameters.txt |    6 ++++++
>  drivers/tty/serial/Kconfig          |    1 +
>  drivers/tty/serial/samsung.c        |   17 +++++++++++++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 5ae8608..e01c0e5 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -936,6 +936,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			must already be setup and configured. Options are not
>  			yet supported.
>  
> +		samsung,<addr>
> +			Start an early, polled-mode console on a samsung serial
> +			port at the specified address. The samsung serial port
> +			must already be setup and configured. Options are not
> +			yet supported.
> +

Couldn't you simply parse this from DT? I believe there is already code
parsing stdout property in chosen node for earlycon purposes present in
the kernel.

Anyway, we already had a patch for this in our internal tree, but it
wasn't submitted because there was no support for early ioremap on ARM
at that time. I haven't been following it since then (and I'm no longer
at Samsung; Marek might be able to take this topic), is it already
available?

>  		smh	Use ARM semihosting calls for early console.
>  
>  	earlyprintk=	[X86,SH,BLACKFIN,ARM,M68k]
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 249e340..9d42ac8 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -257,6 +257,7 @@ config SERIAL_SAMSUNG_CONSOLE
>  	bool "Support for console on Samsung SoC serial port"
>  	depends on SERIAL_SAMSUNG=y
>  	select SERIAL_CORE_CONSOLE
> +	select SERIAL_EARLYCON
>  	help
>  	  Allow selection of the S3C24XX on-board serial ports for use as
>  	  an virtual console.
> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> index c78f43a..f32e9c8 100644
> --- a/drivers/tty/serial/samsung.c
> +++ b/drivers/tty/serial/samsung.c
> @@ -917,6 +917,7 @@ s3c24xx_serial_verify_port(struct uart_port *port, struct serial_struct *ser)
>  #ifdef CONFIG_SERIAL_SAMSUNG_CONSOLE
>  
>  static struct console s3c24xx_serial_console;
> +static void s3c24xx_serial_console_putchar(struct uart_port *port, int ch);
>  
>  static int __init s3c24xx_serial_console_init(void)
>  {
> @@ -926,6 +927,22 @@ static int __init s3c24xx_serial_console_init(void)
>  console_initcall(s3c24xx_serial_console_init);
>  
>  #define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
> +static void samsung_early_write(struct console *con, const char *s, unsigned n)
> +{
> +	struct earlycon_device *dev = con->data;
> +
> +	uart_console_write(&dev->port, s, n, s3c24xx_serial_console_putchar);

Hmm, I'm not sure how this is supposed to work before the driver is
fully initialized.

s3c24xx_serial_console_putchar() will call
s3c24xx_serial_console_txrdy(), which in turn requires the port argument
to be a pointer to the member of a s3c24xx_uart_port struct, with filled
info pointer, which I believe is ready only after s3c24xx_serial_probe().

Has this patch been tested with earlycon enabled and it was indeed
verified that earlycon is actually used?

Best regards,
Tomasz

[PATCH] tty/serial: samsung: Add earlycon support

[Rob Herring]

On 09/20/2014 08:39 AM, Tomasz Figa wrote:
> Hi Alim,
> 
> Please see my comments inline.
> 
> On 16.09.2014 13:32, Alim Akhtar wrote:
>> Add earlycon support for the samsung serial port. This allows enabling
>> the samsung serial port for console when early_params are parse and processed.
>>
>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
>> ---
>>  Documentation/kernel-parameters.txt |    6 ++++++
>>  drivers/tty/serial/Kconfig          |    1 +
>>  drivers/tty/serial/samsung.c        |   17 +++++++++++++++++
>>  3 files changed, 24 insertions(+)
>>
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index 5ae8608..e01c0e5 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -936,6 +936,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>  			must already be setup and configured. Options are not
>>  			yet supported.
>>  
>> +		samsung,<addr>

There will only ever be 1 samsung uart? This is an ABI to some extent,
so you will be stuck with it.

>> +			Start an early, polled-mode console on a samsung serial
>> +			port at the specified address. The samsung serial port
>> +			must already be setup and configured. Options are not
>> +			yet supported.
>> +
> 
> Couldn't you simply parse this from DT? I believe there is already code
> parsing stdout property in chosen node for earlycon purposes present in
> the kernel.

You should support both and it is just a OF_EARLYCON_DECLARE line to add.

> Anyway, we already had a patch for this in our internal tree, but it
> wasn't submitted because there was no support for early ioremap on ARM
> at that time. I haven't been following it since then (and I'm no longer
> at Samsung; Marek might be able to take this topic), is it already
> available?

No, but should be soonish. fixmap parts are being worked on by Kees.

Rob

[PATCH] tty/serial: samsung: Add earlycon support

[Alim Akhtar]

Hi Tomasz,
Thanks for your valuable feedback on this patch.
Please see my comments inline.

On Sat, Sep 20, 2014 at 7:09 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Alim,
>
> Please see my comments inline.
>
> On 16.09.2014 13:32, Alim Akhtar wrote:
>> Add earlycon support for the samsung serial port. This allows enabling
>> the samsung serial port for console when early_params are parse and processed.
>>
>> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
>> ---
>>  Documentation/kernel-parameters.txt |    6 ++++++
>>  drivers/tty/serial/Kconfig          |    1 +
>>  drivers/tty/serial/samsung.c        |   17 +++++++++++++++++
>>  3 files changed, 24 insertions(+)
>>
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index 5ae8608..e01c0e5 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -936,6 +936,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>                       must already be setup and configured. Options are not
>>                       yet supported.
>>
>> +             samsung,<addr>
>> +                     Start an early, polled-mode console on a samsung serial
>> +                     port at the specified address. The samsung serial port
>> +                     must already be setup and configured. Options are not
>> +                     yet supported.
>> +
>
> Couldn't you simply parse this from DT? I believe there is already code
> parsing stdout property in chosen node for earlycon purposes present in
> the kernel.
>
> Anyway, we already had a patch for this in our internal tree, but it
> wasn't submitted because there was no support for early ioremap on ARM
> at that time. I haven't been following it since then (and I'm no longer
> at Samsung; Marek might be able to take this topic), is it already
> available?
I am not sure what you have internally, any further suggestions on
this is most welcome.
As you said there is no support for ioremap on ARM, so this is not
tested on ARM.
>
>>               smh     Use ARM semihosting calls for early console.
>>
>>       earlyprintk=    [X86,SH,BLACKFIN,ARM,M68k]
>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>> index 249e340..9d42ac8 100644
>> --- a/drivers/tty/serial/Kconfig
>> +++ b/drivers/tty/serial/Kconfig
>> @@ -257,6 +257,7 @@ config SERIAL_SAMSUNG_CONSOLE
>>       bool "Support for console on Samsung SoC serial port"
>>       depends on SERIAL_SAMSUNG=y
>>       select SERIAL_CORE_CONSOLE
>> +     select SERIAL_EARLYCON
>>       help
>>         Allow selection of the S3C24XX on-board serial ports for use as
>>         an virtual console.
>> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
>> index c78f43a..f32e9c8 100644
>> --- a/drivers/tty/serial/samsung.c
>> +++ b/drivers/tty/serial/samsung.c
>> @@ -917,6 +917,7 @@ s3c24xx_serial_verify_port(struct uart_port *port, struct serial_struct *ser)
>>  #ifdef CONFIG_SERIAL_SAMSUNG_CONSOLE
>>
>>  static struct console s3c24xx_serial_console;
>> +static void s3c24xx_serial_console_putchar(struct uart_port *port, int ch);
>>
>>  static int __init s3c24xx_serial_console_init(void)
>>  {
>> @@ -926,6 +927,22 @@ static int __init s3c24xx_serial_console_init(void)
>>  console_initcall(s3c24xx_serial_console_init);
>>
>>  #define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
>> +static void samsung_early_write(struct console *con, const char *s, unsigned n)
>> +{
>> +     struct earlycon_device *dev = con->data;
>> +
>> +     uart_console_write(&dev->port, s, n, s3c24xx_serial_console_putchar);
>
> Hmm, I'm not sure how this is supposed to work before the driver is
> fully initialized.
>
> s3c24xx_serial_console_putchar() will call
> s3c24xx_serial_console_txrdy(), which in turn requires the port argument
> to be a pointer to the member of a s3c24xx_uart_port struct, with filled
> info pointer, which I believe is ready only after s3c24xx_serial_probe().
>
I see your point here, but I did not hit any error or any  runtime
crash with this patch when test on ARM64. In order to keep my changes
minimal I tried to reused the code already there in this file and that
worked for me. The reason why this is working is, if you see
s3c24xx_serial_console_txdy(), _info_ pointer is used in a conditional
operator and not really involved in any manipulation.
I am not sure if compiler should have give some warnings or error here.

But certainly this is not the way this is suppose to work probably.
Let me avoid a call to s3c24xx_serial_console_putchar() in my next respin.
As this is a very early call and earlycon expect that port are already
initialized (by bootloader), I will write a new function to be used
instead of s3c24xx_serial_console_putchar() that will directly write
to the serial port.

> Has this patch been tested with earlycon enabled and it was indeed
> verified that earlycon is actually used?
>
Yes, this is tested on ARM64 with earlycon enabled, the way I tested is:
"Disabled" serial dt node in the device tree file,
passed earlycon = samsung,<addr> in CMDLINE
put some debug prints in setup() method, and it is getting printed on
console very early.
Below is my partial bootlog:

-----
Initializing cgroup subsys cpu
Linux version 3.17.0-rc5+ (alim at alim) (gcc version 4.8.3 20140106 (prerelease)
CPU: AArch64 Processor
Detected VIPT I-cache on CPU0
Early serial console at I/O port 0x0 (options '')
samsung_early_console_setup:954
bootconsole [uart0] enabled
.
.
Kernel command line: earlycon=samsung,0x14c30000 console=ttySAC0,115200
.
.
io scheduler noop registered
io scheduler cfq registered (default)
Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
.
.
TCP: cubic registered
NET: Registered protocol family 17
9pnet: Installing 9P2000 support
bootconsole [uart0] disabled
-------
As I disabled serial node from DT, it did not printed any further
bootlog after "bootconsole [uar0] disabled"
And removing "earlycon" from CMDLINE does not give any bootlog on console.
So I believe earlycon is working on this platform atleast.

Let me know if this test is convincing to you.

Thanks,

> Best regards,
> Tomasz
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Regards,
Alim

[PATCH] tty/serial: samsung: Add earlycon support

[Tomasz Figa]

On 21.09.2014 16:36, Alim Akhtar wrote:
> Hi Tomasz,
> Thanks for your valuable feedback on this patch.

You're welcome.

>>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>>> index 5ae8608..e01c0e5 100644
>>> --- a/Documentation/kernel-parameters.txt
>>> +++ b/Documentation/kernel-parameters.txt
>>> @@ -936,6 +936,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>>                       must already be setup and configured. Options are not
>>>                       yet supported.
>>>
>>> +             samsung,<addr>
>>> +                     Start an early, polled-mode console on a samsung serial
>>> +                     port at the specified address. The samsung serial port
>>> +                     must already be setup and configured. Options are not
>>> +                     yet supported.
>>> +
>>
>> Couldn't you simply parse this from DT? I believe there is already code
>> parsing stdout property in chosen node for earlycon purposes present in
>> the kernel.
>>
>> Anyway, we already had a patch for this in our internal tree, but it
>> wasn't submitted because there was no support for early ioremap on ARM
>> at that time. I haven't been following it since then (and I'm no longer
>> at Samsung; Marek might be able to take this topic), is it already
>> available?
> I am not sure what you have internally, any further suggestions on
> this is most welcome.

I believe Marek should be able to post our internal patch, so maybe you
could reuse it and adapt for your needs.

> As you said there is no support for ioremap on ARM, so this is not
> tested on ARM.

Don't forget that this driver is primarily targeted for ARM platforms
(versus just one ARM64-based Exynos7), so either this feature should be
clearly added as ARM64-specific (and compiled in conditionally) or made
work for all supported platforms.

>>
>>>               smh     Use ARM semihosting calls for early console.
>>>
>>>       earlyprintk=    [X86,SH,BLACKFIN,ARM,M68k]
>>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>>> index 249e340..9d42ac8 100644
>>> --- a/drivers/tty/serial/Kconfig
>>> +++ b/drivers/tty/serial/Kconfig
>>> @@ -257,6 +257,7 @@ config SERIAL_SAMSUNG_CONSOLE
>>>       bool "Support for console on Samsung SoC serial port"
>>>       depends on SERIAL_SAMSUNG=y
>>>       select SERIAL_CORE_CONSOLE
>>> +     select SERIAL_EARLYCON
>>>       help
>>>         Allow selection of the S3C24XX on-board serial ports for use as
>>>         an virtual console.
>>> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
>>> index c78f43a..f32e9c8 100644
>>> --- a/drivers/tty/serial/samsung.c
>>> +++ b/drivers/tty/serial/samsung.c
>>> @@ -917,6 +917,7 @@ s3c24xx_serial_verify_port(struct uart_port *port, struct serial_struct *ser)
>>>  #ifdef CONFIG_SERIAL_SAMSUNG_CONSOLE
>>>
>>>  static struct console s3c24xx_serial_console;
>>> +static void s3c24xx_serial_console_putchar(struct uart_port *port, int ch);
>>>
>>>  static int __init s3c24xx_serial_console_init(void)
>>>  {
>>> @@ -926,6 +927,22 @@ static int __init s3c24xx_serial_console_init(void)
>>>  console_initcall(s3c24xx_serial_console_init);
>>>
>>>  #define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
>>> +static void samsung_early_write(struct console *con, const char *s, unsigned n)
>>> +{
>>> +     struct earlycon_device *dev = con->data;
>>> +
>>> +     uart_console_write(&dev->port, s, n, s3c24xx_serial_console_putchar);
>>
>> Hmm, I'm not sure how this is supposed to work before the driver is
>> fully initialized.
>>
>> s3c24xx_serial_console_putchar() will call
>> s3c24xx_serial_console_txrdy(), which in turn requires the port argument
>> to be a pointer to the member of a s3c24xx_uart_port struct, with filled
>> info pointer, which I believe is ready only after s3c24xx_serial_probe().
>>
> I see your point here, but I did not hit any error or any  runtime
> crash with this patch when test on ARM64. In order to keep my changes
> minimal I tried to reused the code already there in this file and that
> worked for me. The reason why this is working is, if you see
> s3c24xx_serial_console_txdy(), _info_ pointer is used in a conditional
> operator and not really involved in any manipulation.
> I am not sure if compiler should have give some warnings or error here.
> 

The info pointer is always used whenever FIFO mode is enabled at
boot-up, which is often the case on many boards (and depends on
firmware). So the fact that it worked for you was purely a coincidence
due to your bootloader not using this mode. (Btw. it should be mentioned
in patch description on what hardware it was tested.)

> But certainly this is not the way this is suppose to work probably.
> Let me avoid a call to s3c24xx_serial_console_putchar() in my next respin.
> As this is a very early call and earlycon expect that port are already
> initialized (by bootloader), I will write a new function to be used
> instead of s3c24xx_serial_console_putchar() that will directly write
> to the serial port.
> 

The mentioned patch already has this implemented, including support for
all hardware variants. I'd strongly suggest basing your work on top of that.

Marek, could you post that patch as an RFC, so that Alim could continue
his work?

Best regards,
Tomasz

[PATCH] tty/serial: samsung: Add earlycon support

[Alim Akhtar]

Hi Tomasz,

On Sun, Sep 21, 2014 at 10:54 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 21.09.2014 16:36, Alim Akhtar wrote:
>> Hi Tomasz,
>> Thanks for your valuable feedback on this patch.
>
> You're welcome.
>
>>>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>>>> index 5ae8608..e01c0e5 100644
>>>> --- a/Documentation/kernel-parameters.txt
>>>> +++ b/Documentation/kernel-parameters.txt
>>>> @@ -936,6 +936,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>>>                       must already be setup and configured. Options are not
>>>>                       yet supported.
>>>>
>>>> +             samsung,<addr>
>>>> +                     Start an early, polled-mode console on a samsung serial
>>>> +                     port at the specified address. The samsung serial port
>>>> +                     must already be setup and configured. Options are not
>>>> +                     yet supported.
>>>> +
>>>
>>> Couldn't you simply parse this from DT? I believe there is already code
>>> parsing stdout property in chosen node for earlycon purposes present in
>>> the kernel.
>>>
>>> Anyway, we already had a patch for this in our internal tree, but it
>>> wasn't submitted because there was no support for early ioremap on ARM
>>> at that time. I haven't been following it since then (and I'm no longer
>>> at Samsung; Marek might be able to take this topic), is it already
>>> available?
>> I am not sure what you have internally, any further suggestions on
>> this is most welcome.
>
> I believe Marek should be able to post our internal patch, so maybe you
> could reuse it and adapt for your needs.
>
>> As you said there is no support for ioremap on ARM, so this is not
>> tested on ARM.
>
> Don't forget that this driver is primarily targeted for ARM platforms
> (versus just one ARM64-based Exynos7), so either this feature should be
> clearly added as ARM64-specific (and compiled in conditionally) or made
> work for all supported platforms.
>
Well, this will work on every platform which uses
"samsung,exynos4210-uart" as a UART controller.
Exynos7 also use same, there is nothing special about ARM64 bit here.
please see[1].
For "s3c24xx-uart", this has to be implemented separably as that is a
bit different.
>>>
>>>>               smh     Use ARM semihosting calls for early console.
>>>>
>>>>       earlyprintk=    [X86,SH,BLACKFIN,ARM,M68k]
>>>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>>>> index 249e340..9d42ac8 100644
>>>> --- a/drivers/tty/serial/Kconfig
>>>> +++ b/drivers/tty/serial/Kconfig
>>>> @@ -257,6 +257,7 @@ config SERIAL_SAMSUNG_CONSOLE
>>>>       bool "Support for console on Samsung SoC serial port"
>>>>       depends on SERIAL_SAMSUNG=y
>>>>       select SERIAL_CORE_CONSOLE
>>>> +     select SERIAL_EARLYCON
>>>>       help
>>>>         Allow selection of the S3C24XX on-board serial ports for use as
>>>>         an virtual console.
>>>> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
>>>> index c78f43a..f32e9c8 100644
>>>> --- a/drivers/tty/serial/samsung.c
>>>> +++ b/drivers/tty/serial/samsung.c
>>>> @@ -917,6 +917,7 @@ s3c24xx_serial_verify_port(struct uart_port *port, struct serial_struct *ser)
>>>>  #ifdef CONFIG_SERIAL_SAMSUNG_CONSOLE
>>>>
>>>>  static struct console s3c24xx_serial_console;
>>>> +static void s3c24xx_serial_console_putchar(struct uart_port *port, int ch);
>>>>
>>>>  static int __init s3c24xx_serial_console_init(void)
>>>>  {
>>>> @@ -926,6 +927,22 @@ static int __init s3c24xx_serial_console_init(void)
>>>>  console_initcall(s3c24xx_serial_console_init);
>>>>
>>>>  #define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
>>>> +static void samsung_early_write(struct console *con, const char *s, unsigned n)
>>>> +{
>>>> +     struct earlycon_device *dev = con->data;
>>>> +
>>>> +     uart_console_write(&dev->port, s, n, s3c24xx_serial_console_putchar);
>>>
>>> Hmm, I'm not sure how this is supposed to work before the driver is
>>> fully initialized.
>>>
>>> s3c24xx_serial_console_putchar() will call
>>> s3c24xx_serial_console_txrdy(), which in turn requires the port argument
>>> to be a pointer to the member of a s3c24xx_uart_port struct, with filled
>>> info pointer, which I believe is ready only after s3c24xx_serial_probe().
>>>
>> I see your point here, but I did not hit any error or any  runtime
>> crash with this patch when test on ARM64. In order to keep my changes
>> minimal I tried to reused the code already there in this file and that
>> worked for me. The reason why this is working is, if you see
>> s3c24xx_serial_console_txdy(), _info_ pointer is used in a conditional
>> operator and not really involved in any manipulation.
>> I am not sure if compiler should have give some warnings or error here.
>>
>
> The info pointer is always used whenever FIFO mode is enabled at
> boot-up, which is often the case on many boards (and depends on
> firmware). So the fact that it worked for you was purely a coincidence
> due to your bootloader not using this mode. (Btw. it should be mentioned
> in patch description on what hardware it was tested.)
>
>> But certainly this is not the way this is suppose to work probably.
>> Let me avoid a call to s3c24xx_serial_console_putchar() in my next respin.
>> As this is a very early call and earlycon expect that port are already
>> initialized (by bootloader), I will write a new function to be used
>> instead of s3c24xx_serial_console_putchar() that will directly write
>> to the serial port.
>>
>
> The mentioned patch already has this implemented, including support for
> all hardware variants. I'd strongly suggest basing your work on top of that.
>
> Marek, could you post that patch as an RFC, so that Alim could continue
> his work?
>
Please CC me when you post that.

Tomasz/Marek,
Do you think something like below make sense here?

+static void exynos4210_serial_console_putc(struct uart_port *port, int ch)
+{
+       while (!(readl(port->membase + S3C2410_UFCON) & S3C2410_UFCON_FIFOMODE))
+               ;
+
+       wr_regb(port, S3C2410_UTXH, ch);
+
+       while ((readl(port->membase + S3C2410_UFSTAT) & S5PV210_UFSTAT_TXFULL))
+               ;
+}

and call exynos4210_serial_console_putc() in samsung_early_write()?
Anyway functions names need to be changes accordingly.

[1] http://www.spinics.net/lists/devicetree/msg49918.html

> Best regards,
> Tomasz



-- 
Regards,
Alim

[PATCH] tty/serial: samsung: Add earlycon support

[Tomasz Figa]

On 22.09.2014 01:10, Alim Akhtar wrote:

[snip]

>>> As you said there is no support for ioremap on ARM, so this is not
>>> tested on ARM.
>>
>> Don't forget that this driver is primarily targeted for ARM platforms
>> (versus just one ARM64-based Exynos7), so either this feature should be
>> clearly added as ARM64-specific (and compiled in conditionally) or made
>> work for all supported platforms.
>>
> Well, this will work on every platform which uses
> "samsung,exynos4210-uart" as a UART controller.
> Exynos7 also use same, there is nothing special about ARM64 bit here.
> please see[1].
> For "s3c24xx-uart", this has to be implemented separably as that is a
> bit different.

For this feature, they differ only in locations and lengths of
FIFO-related bit fields. We had this already implemented for all
hardware variants and so my recommendation to reuse that code.

>>>>
>>>>>               smh     Use ARM semihosting calls for early console.
>>>>>
>>>>>       earlyprintk=    [X86,SH,BLACKFIN,ARM,M68k]

[snip]

> Tomasz/Marek,
> Do you think something like below make sense here?
> 
> +static void exynos4210_serial_console_putc(struct uart_port *port, int ch)
> +{
> +       while (!(readl(port->membase + S3C2410_UFCON) & S3C2410_UFCON_FIFOMODE))
> +               ;

Waiting in a loop for a software-writable FIFO mode enable bit doesn't
look reasonable to me. Probably a typo?

> +
> +       wr_regb(port, S3C2410_UTXH, ch);
> +
> +       while ((readl(port->membase + S3C2410_UFSTAT) & S5PV210_UFSTAT_TXFULL))
> +               ;

I wonder if you need to wait for the character to be sent. If you ensure
before writing a character that there is no previous one in the buffer
or there is space in FIFO then I believe you should be fine.

> +}
> 
> and call exynos4210_serial_console_putc() in samsung_early_write()?
> Anyway functions names need to be changes accordingly.

Yes, this is exactly what we had implemented in the patch I mentioned,
except that the putc function was generic for all hardware variants.

Best regards,
Tomasz

[PATCH] tty/serial: samsung: Add earlycon support

[Alim Akhtar]

Hi Tomasz,

On Mon, Sep 22, 2014 at 4:49 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 22.09.2014 01:10, Alim Akhtar wrote:
>
> [snip]
>
>>>> As you said there is no support for ioremap on ARM, so this is not
>>>> tested on ARM.
>>>
>>> Don't forget that this driver is primarily targeted for ARM platforms
>>> (versus just one ARM64-based Exynos7), so either this feature should be
>>> clearly added as ARM64-specific (and compiled in conditionally) or made
>>> work for all supported platforms.
>>>
>> Well, this will work on every platform which uses
>> "samsung,exynos4210-uart" as a UART controller.
>> Exynos7 also use same, there is nothing special about ARM64 bit here.
>> please see[1].
>> For "s3c24xx-uart", this has to be implemented separably as that is a
>> bit different.
>
> For this feature, they differ only in locations and lengths of
> FIFO-related bit fields. We had this already implemented for all
> hardware variants and so my recommendation to reuse that code.
>
>>>>>
>>>>>>               smh     Use ARM semihosting calls for early console.
>>>>>>
>>>>>>       earlyprintk=    [X86,SH,BLACKFIN,ARM,M68k]
>
> [snip]
>
>> Tomasz/Marek,
>> Do you think something like below make sense here?
>>
>> +static void exynos4210_serial_console_putc(struct uart_port *port, int ch)
>> +{
>> +       while (!(readl(port->membase + S3C2410_UFCON) & S3C2410_UFCON_FIFOMODE))
>> +               ;
>
> Waiting in a loop for a software-writable FIFO mode enable bit doesn't
> look reasonable to me. Probably a typo?
>
>> +
>> +       wr_regb(port, S3C2410_UTXH, ch);
>> +
>> +       while ((readl(port->membase + S3C2410_UFSTAT) & S5PV210_UFSTAT_TXFULL))
>> +               ;
>
> I wonder if you need to wait for the character to be sent. If you ensure
> before writing a character that there is no previous one in the buffer
> or there is space in FIFO then I believe you should be fine.
>
Well, below is what I was about to post:

 #define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
+static void s3c24xx_serial_console_putc(struct uart_port *port, int ch)
+{
+ unsigned int ufcon = rd_regl(port, S3C2410_UFCON);
+
+ if (ufcon & S3C2410_UFCON_FIFOMODE) {
+  while((readl(port->membase + S3C2410_UFSTAT) &
+      S5PV210_UFSTAT_TXFULL))
+   ;
+  wr_regb(port, S3C2410_UTXH, ch);
+ }
+}
+
+static void samsung_early_write(struct console *con, const char *s, unsigned n)
+{
+ struct earlycon_device *dev = con->data;
+
+ uart_console_write(&dev->port, s, n, s3c24xx_serial_console_putc);
+}
+
+static int __init samsung_early_console_setup(struct earlycon_device *device,
+           const char *opt)
+{
+ if (!device->port.membase)
+  return -ENODEV;
+ device->con->write = samsung_early_write;
+ return 0;
+}
+EARLYCON_DECLARE(samsung, samsung_early_console_setup);
+OF_EARLYCON_DECLARE(samsung, "samsung,exynos4210-uart",
samsung_early_console_setup);

but just saw patches from Marek few min back which is similar except
for the handle non-fifo mode, which is good anyway. :-)
So lets drops this patch and discussion here.

Thanks!!!

>> +}
>>
>> and call exynos4210_serial_console_putc() in samsung_early_write()?
>> Anyway functions names need to be changes accordingly.
>
> Yes, this is exactly what we had implemented in the patch I mentioned,
> except that the putc function was generic for all hardware variants.
>
> Best regards,
> Tomasz



-- 
Regards,
Alim