Re: [PATCH 05/19] LoongArch: Add boot and setup routines

[Huacai Chen <chenhuacai@gmail.com>]

Hi, Arnd,

On Tue, Jul 6, 2021 at 6:16 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Jul 6, 2021 at 6:18 AM Huacai Chen <chenhuacai@loongson.cn> wrote:
>
> > +
> > +#ifdef CONFIG_64BIT
> > +       /* Guess if the sign extension was forgotten by bootloader */
> > +       if (start < CAC_BASE)
> > +               start = (int)start;
> > +#endif
> > +       initrd_start = start;
> > +       initrd_end += start;
> > +       return 0;
> > +}
> > +early_param("rd_start", rd_start_early);
> > +
> > +static int __init rd_size_early(char *p)
> > +{
> > +       initrd_end += memparse(p, &p);
> > +       return 0;
> > +}
> > +early_param("rd_size", rd_size_early);
>
> The early parameters should not be used for this, I'm fairly sure the UEFI
> boot protocol already has ways to communicate all necessary information.
We use grub to boot the Linux kernel. We found X86 uses private data
structures (seems not UEFI-specific) to pass initrd information from
grub to kernel. Some archs use fdt, and other archs use cmdline with
"initrd=start,size" (init/do_mounts_initrd.c). So, I think use cmdline
is not unacceptable, but we do can remove the the rd_start/rd_size
parsing code here.

>
> > +
> > +#ifdef CONFIG_ACPI
> > +       init_initrd();
> > +#endif
>
> Why is the initrd support tied to ACPI? Can you actually boot without ACPI?
>
> > +#if defined(CONFIG_VT)
> > +#if defined(CONFIG_VGA_CONSOLE)
> > +       conswitchp = &vga_con;
> > +#elif defined(CONFIG_DUMMY_CONSOLE)
> > +       conswitchp = &dummy_con;
> > +#endif
> > +#endif
>
> The VGA console seems rather outdated. If you have UEFI, why not use
> the provided framebuffer for the console?
OK, we will remove this.

>
> > +u64 cpu_clock_freq;
> > +EXPORT_SYMBOL(cpu_clock_freq);
> > +u64 const_clock_freq;
> > +EXPORT_SYMBOL(const_clock_freq);
>
> You should generally not rely on the CPU clock frequency being fixed
> like this, as this breaks down as soon as you add a drivers/cpufreq/ driver.
>
> What code uses these?
cpu_clock_freq is a const which records the "standard frequency", it
is just used to display the frequency now. In future when cpufreq
added, cpu_clock_freq will be used to calculate the target frequency.

Huacai
>
> > +void __init time_init(void)
> > +{
> > +       if (!cpu_has_cpucfg)
> > +               const_clock_freq = cpu_clock_freq;
> > +       else
> > +               const_clock_freq = calc_const_freq();
> > +
> > +       init_timeval = drdtime() - csr_readq(LOONGARCH_CSR_CNTC);
> > +
> > +       constant_clockevent_init();
> > +       constant_clocksource_init();
> > +}
>
> Clocksource and clockevents drivers should be located in drivers/clocksource
> and reviewed by its maintainers.
>
>          Arnd