Re: [PATCH v7 13/13] tests/acceptance: console boot tests for quanta-gsj

[Philippe Mathieu-Daudé]

+Eric / Richard for compiler optimizations.

On 8/20/20 3:53 AM, Havard Skinnemoen wrote:
> On Tue, Aug 11, 2020 at 8:26 PM Havard Skinnemoen
> <hskinnemoen@google.com> wrote:
>>
>> On Tue, Aug 11, 2020 at 1:48 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 
>> wrote:
>>> INTERRUPTED: Test interrupted by SIGTERM
>>> Runner error occurred: Timeout reached
>>> (240.45 s)
>>>
>>> Is that expected?
>>
>> I'm not sure why it only happens when running direct kernel boot with
>> unoptimized qemu, but it seems a little happier if I enable a few more
>> peripherals that I have queued up (sd, ehci, ohci and rng), though not
>> enough.
>>
>> It still stalls for an awfully long time on "console: Run /init as
>> init process" though. I'm not sure what it's doing there. With -O2 it
>> only takes a couple of seconds to move on.
> 
> So it turns out that the kernel gets _really_ sluggish when skipping
> the clock initialization normally done by the boot loader.
> 
> I changed the reset value of CLKSEL like this:
> 
> diff --git a/hw/misc/npcm7xx_clk.c b/hw/misc/npcm7xx_clk.c
> index 21ab4200d1..5e9849410f 100644
> --- a/hw/misc/npcm7xx_clk.c
> +++ b/hw/misc/npcm7xx_clk.c
> @@ -67,7 +67,7 @@ enum NPCM7xxCLKRegisters {
>   */
>  static const uint32_t cold_reset_values[NPCM7XX_CLK_NR_REGS] = {
>      [NPCM7XX_CLK_CLKEN1]        = 0xffffffff,
> -    [NPCM7XX_CLK_CLKSEL]        = 0x004aaaaa,
> +    [NPCM7XX_CLK_CLKSEL]        = 0x004aaba9,
>      [NPCM7XX_CLK_CLKDIV1]       = 0x5413f855,
>      [NPCM7XX_CLK_PLLCON0]       = 0x00222101 | PLLCON_LOKI,
>      [NPCM7XX_CLK_PLLCON1]       = 0x00202101 | PLLCON_LOKI,
> 
> which switches the CPU core and UART to run from PLL2 instead of
> CLKREF (25 MHz).
> 
> With this change, the test passes without optimization:
> 
>  (02/19) 
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_quanta_gsj_initrd:
> PASS (39.62 s)
> 
> It doesn't look like this change hurts booting from the bootrom (IIUC
> the nuvoton bootblock overwrites CLKSEL anyway), but it's not super
> clean.
> 
> Perhaps I should make it conditional on kernel_filename being set? Or
> would it be better to provide a write_board_setup hook for this?

QEMU prefers to avoid ifdef'ry at all cost. However I find this
approach acceptable (anyway up to the maintainer):

+static void npcm7xx_clk_cold_reset_fixup(NPCM7xxCLKState *s)
+{
+#ifndef __OPTIMIZE__
+    /*
+     * When built without optimization, ...
+     * so run CPU core and UART from PLL2 instead of CLKREF.
+     */
+    s->regs[NPCM7XX_CLK_CLKSEL] |= 0x103,
+#endif
+}

 static void npcm7xx_clk_enter_reset(Object *obj, ResetType type)
 {
     NPCM7xxCLKState *s = NPCM7XX_CLK(obj);

     QEMU_BUILD_BUG_ON(sizeof(s->regs) != sizeof(cold_reset_values));

     switch (type) {
     case RESET_TYPE_COLD:
         memcpy(s->regs, cold_reset_values, sizeof(cold_reset_values));
+        npcm7xx_clk_cold_reset_fixup(s);
         s->ref_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
         return;
     }
     ...

Regards,

Phil.