On Thu, 13 Jul 2017 05:32:26 PDT (-0700), james.ho...@imgtec.com wrote:
> On Thu, Jul 13, 2017 at 09:59:53PM +1000, Michael Ellerman wrote:
>> Palmer Dabbelt <pal...@dabbelt.com> writes:
>>
>> > On Wed, 12 Jul 2017 04:04:00 PDT (-0700), m...@ellerman.id.au wrote:
>> >> Palmer Dabbelt <pal...@dabbelt.com> writes:
>> >>
>> >>> On Mon, 10 Jul 2017 23:21:07 PDT (-0700), m...@ellerman.id.au wrote:
>> >>>> Palmer Dabbelt <pal...@dabbelt.com> writes:
>> >>>>>
>> >>>> ...
>> >>>>> +#ifdef CONFIG_EARLY_PRINTK
>> >>>>> +static void sbi_console_write(struct console *co, const char *buf,
>> >>>>> + unsigned int n)
>> >>>>> +{
>> >>>>> + int i;
>> >>>>> +
>> >>>>> + for (i = 0; i < n; ++i) {
>> >>>>> + if (buf[i] == '\n')
>> >>>>> + sbi_console_putchar('\r');
>> >>>>> + sbi_console_putchar(buf[i]);
>> >>>>> + }
>> >>>>> +}
>> >>>>> +
>> >>>>> +static struct console early_console_dev __initdata = {
>> >>>>> + .name = "early",
>> >>>>> + .write = sbi_console_write,
>> >>>>> + .flags = CON_PRINTBUFFER | CON_BOOT,
>> >>>>
>> >>>> AFAICS you could add CON_ANYTIME here, which would mean this console
>> >>>> would print output before the CPU is online.
>> >>>>
>> >>>> I think it doesn't currently matter because you call parse_early_param()
>> >>>> from setup_arch(), at which point the boot CPU has been marked online.
>> >>>>
>> >>>> But if this console can actually work earlier then you might be better
>> >>>> off just registering it unconditionally very early.
>> >>>
>> >>> That seems like a good idea. I'm not familiar with how all this works,
>> >>> but
>> >>> from my understanding of this early_initcall() should be sufficient to
>> >>> make
>> >>> this work? The only other driver that sets CON_ANYTIME and supports
>> >>> EARLY_PRINTK is hvc_xen, but that installs a header to let init code
>> >>> register
>> >>> the console directly. The early_initcall mechanism seems cleaner if it
>> >>> does
>> >>> the right thing.
>> >>
>> >> Unfortunately early_initcall is not very "early" :) It's earlier than
>> >> all the other initcalls, but it's late compared to most of the arch boot
>> >> code.
>> >>
>> >> The early_param() will work better, ie. register the console earlier
>> >> and increase the chance of you getting output from an early crash, than
>> >> early_initcall. But it requires you to put earlyprintk on the command
>> >> line.
>> >>
>> >> The best option is to just register the console as early as you can, ie.
>> >> as soon as it can give you output. So somewhere in your setup_arch(), or
>> >> even earlier (I haven't read your boot code).
>> >
>> > Doing it that way would require either moving the TTY driver into arch
>> > code (it
>> > was specifically suggested we move it out) or adding a header file to allow
>> > setup_arch() to call into the driver (XEN does this, and we're doing it
>> > for our
>> > timer, but it seems hacky).
>>
>> I think it's fairly uncontroversial to have the early console in arch
>> code, especially in a case like this where there's no code shared
>> between the console and the TTY driver. But maybe someone will prove me
>> wrong.
>>
>> Doing it the other way is not really hacky IMO, you can just have an
>> extern for the early console in one of your asm headers.
>
> For reference both metag and mips do something like this for JTAG based
> consoles (with drivers both residing in drivers/tty/):
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/metag/kernel/setup.c#n107
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/metag/kernel/setup.c#n234
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/include/asm/cdmm.h#n98
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/setup.c#n958
>
> Its not all that pretty but it gets you console output that much
> earlier and is a fairly special case, so I think its worth it.
If someone else is doing it, then it's good enough for me :). How does this
look?
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 319fad96f537..148fd0dc414b 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -59,6 +59,14 @@ unsigned long pfn_base;
/* The lucky hart to first increment this variable will boot the other cores */
atomic_t hart_lottery;
+#if defined(CONFIG_HVC_RISCV_SBI) && defined(CONFIG_EARLY_PRINTK)
+/*
+ * The SBI's early console lives in hvc_riscv_sbi.c, but we want very early
+ * access
+ */
+extern struct console riscv_sbi_early_console_dev;
+#endif
+
#ifdef CONFIG_BLK_DEV_INITRD
static void __init setup_initrd(void)
{
@@ -203,6 +211,13 @@ static void __init setup_bootmem(void)
void __init setup_arch(char **cmdline_p)
{
+#if defined(CONFIG_TTY_RISCV_SBI) && defined(CONFIG_EARLY_PRINTK)
+ if (likely(early_console == NULL)) {
+ early_console = &riscv_sbi_early_console;
+ register_console(early_console);
+ }
+#endif
+
#ifdef CONFIG_CMDLINE_BOOL
#ifdef CONFIG_CMDLINE_OVERRIDE
strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
diff --git a/drivers/tty/hvc/hvc_riscv_sbi.c b/drivers/tty/hvc/hvc_riscv_sbi.c
index 534d6b75a2c6..20a6bfda4e32 100644
--- a/drivers/tty/hvc/hvc_riscv_sbi.c
+++ b/drivers/tty/hvc/hvc_riscv_sbi.c
@@ -84,20 +84,11 @@ static void sbi_console_write(struct console *co, const
char *buf,
}
}
-static struct console early_console_dev __initdata = {
+/* This is used by arch/riscv/kernel/setup.c */
+struct console riscv_sbi_early_console_dev __initdata = {
.name = "early",
.write = sbi_console_write,
.flags = CON_PRINTBUFFER | CON_BOOT | CON_ANYTIME,
.index = -1
};
-
-static int __init setup_early_printk(void)
-{
- if (early_console == NULL) {
- early_console = &early_console_dev;
- register_console(early_console);
- }
- return 0;
-}
-early_initcall(setup_early_printk);
#endif
