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

[Havard Skinnemoen]

On Thu, Aug 20, 2020 at 10:46 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 8/20/20 6:24 PM, Havard Skinnemoen wrote:
> > On Wed, Aug 19, 2020 at 10:29 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> > wrote:
> >>
> >> +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
> >> +}
> >
> > I think this is actually a problem regardless of optimization level.
> > Turning optimization off amplifies the problem, but the problem is
> > still there with optimization on.
>
> OK, this reminds me few more details about the problem I had with the
> raspi3 when adding the ClockPowerResetManager block.
> Found the branch. A bit bitter/sad it was more than 1 year ago.
>
> So if ARM_FEATURE_GENERIC_TIMER is available, Linux polls the CNTFRQ_EL0
> register. At that time this register were using a fixed frequency:
>
> #define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */
>
> Xilinx' fork does it this way:
> https://github.com/Xilinx/qemu/commit/9e939b54e2d
>
> Now I see Andrew Jeffery fixed that in 96eec6b2b38
> ("target/arm: Prepare generic timer for per-platform CNTFRQ")
> adding a 'cntfrq' property, which he then sets in the Aspeed
> machine in commit 058d095532d ("ast2600: Configure CNTFRQ at 1125MHz").
>
> Maybe your SoC is simply missing this property?

Hmm, it doesn't look like Cortex-A9 has this property...

Unexpected error in object_property_find() at
/usr/local/google/home/hskinnemoen/qemu/upstream/qom/object.c:1254:
qemu-system-arm: Property '.cntfrq' not found

However, I did notice there are a lot of constraints on input
frequencies to the CPU and various peripherals, and many of these are
not met by the power-on reset values.

For example, the UART needs a 24 MHz input clock, but there's no way
to generate this from the default 25 MHz reference clock. However,
PLL2 is set up to run at 960 MHz by default (as soon as it's locked),
with a fixed /2 divider. The UART uses a /20 divider by default, so it
gets a 25 MHz / 20 = 1.25 MHz clock with power-on defaults. However,
switching the source to PLL2 results in 960 MHz / 2 / 20 = 24 MHz.

Similarly, the CPU is supposed to run at 800 MHz, but it runs at 25
MHz with power-on defaults. PLL1 runs at 800 MHz by default, with a
fixed /2 divider. The boot loader doubles the feedback divider so it
turns into 1600 MHz / 2 = 800 MHz.

Turns out I was wrong above, the patch snippet routes PLL1, not PLL2,
to the CPU. But it will only result in half the recommended speed, so
a patch to PLLCON1 is needed as well.

It seems like the cleanest solution would be to add a
write_board_setup hook to fix up these registers so clocks are within
normal ranges when bypassing the boot loader. In particular:

  - Switch UART to PLL2 for a 24 MHz input clock.
  - Set up PLL1 to run at 1600 MHz.
  - Switch the CPU core to PLL1 / 2 for a 800 MHz input clock.

Does that make sense? It should be just three simple register writes,
which is doable with hand-edited machine code.

I'll add this as a separate patch (right before the acceptance test)
so it's clear what's happening.

Unfortunately, I haven't been able to make the flash boot test pass
without optimization. U-boot seems to have a tiny buffer for the
command line, so I can only disable two or three systemd services
before it gets truncated. I might try to create a reduced openbmc
image instead.

Havard