Re: [PATCH 08/17] tty: New RISC-V SBI console driver

[Michael Ellerman <mpe@ellerman.id.au>]

Palmer Dabbelt <palmer@dabbelt.com> writes:
> On Thu, 13 Jul 2017 05:32:26 PDT (-0700), james.hogan@imgtec.com wrote:
>> On Thu, Jul 13, 2017 at 09:59:53PM +1000, Michael Ellerman wrote:
>>>
>>> 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/):
...
>>
>> 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)
                                        ^
                                        This is always true because you
                                        said so in your Kconfig.

> +/*
> + * 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

I would have put it in one of your arch headers, so that the hvc driver
can include it too.

Personally I tend not to bother #ifdef'ing every extern declaration, but
there are arguments both ways.

>  #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)
                      ^
                      HVC

> +       if (likely(early_console == NULL)) {

I don't think you need to check. You're the boss of early_console at
this point in boot.

> +               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);

cheers