| Message ID | 1410867163-27962-1-git-send-email-alim.akhtar@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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(+)