[U-Boot] [PATCH 09/14] tegra: Add warmboot implementation

[Simon Glass]

Hi Stephen,

[Yen please can you read this also?]

On Tue, Jan 10, 2012 at 10:30 AM, Stephen Warren <swarren at nvidia.com> wrote:
> On 12/26/2011 12:33 PM, Simon Glass wrote:
>> From: Yen Lin <yelin at nvidia.com>
>>
>> Add code to set up the warm boot area in the Tegra CPU ready for a
>> resume after suspend.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>
> How come Yen's S-o-b line is missing here?
>
> As a general comment on this patch, it contains a lot of structure
> definitions and defines that should really be part of the clock module's
> headers and similar; why not just included the clock headers and place
> all the definitions in those headers if they aren't already there?

Yes this bit of it got a pretty light review at the time and I was
reluctant to press on the bitfields when that side of things was still
up in the air. No functions exist to access these things and using the
clk_rst.h header adds a mountain of ugly reg & OSC_FREQ_MASK) >>
OSC_FREQ_SHIFT to the code. I am still uncomfortable with how this
sort of thing is done in U-Boot.

Also the AVP code cannot call functions in clock.c (they are in SDRAM
which isn't on yet), so we need the structures or some sort of access
there anyway.

I will tidy up some low-hanging fruit in warmboot.c but don't know
that I can do much with warmboot_avp.c. At least it isn't assembler
anymore!

But there a few questions here. Firstly it seems that it saves some
memory speed info to scratch2 but never restores it. Then it restores
CPU speed from scratch3 but I can't see where it saves it. Does Yen
have any comments on that? Apparently it works because we have been
using it, but... If it is saving things that are never restored I
could dump that and reduce the code size.

>
> There's also some amount of code duplication; e.g. wasn't
> get_major_version() part of another patch you recently posted that
> implements APIs to return SKU IDs etc? If not, it seems like it belongs
> there. Only warmboot_avp.c appears to run on the AVP, so I don't think
> needing ARMv4 compile flags is an argument for not putting this code in
> a more logical place.
>
>> diff --git a/arch/arm/cpu/armv7/tegra2/Makefile b/arch/arm/cpu/armv7/tegra2/Makefile
> ...
>> -SOBJS  := lowlevel_init.o
>> +SOBJS-y        := lowlevel_init.o
> ...
>> +SOBJS  := $(SOBJS-y)
>
> That's really a separate patch, but I guess it's fine.

I can remove it now that wamboot is in C.

>
>> diff --git a/arch/arm/cpu/armv7/tegra2/warmboot.c b/arch/arm/cpu/armv7/tegra2/warmboot.c
> ...
>> +#define BCT_OFFSET             0x100           /* BCT starts at 0x100 */
>> +#define BCT_SDRAM_PARAMS_OFFSET        (BCT_OFFSET + 0x88)
>> +#define SDRAM_PARAMS_BASE      (AP20_BASE_PA_SRAM + BCT_SDRAM_PARAMS_OFFSET)
>
> BCT_OFFSET is where the BCT is stored within some warmboot-specific
> memory location. I think the comment should be explicit about this,
> because IIRC the BCT starts at offset 0 within the (cold) boot flash
> memory, so the comment is confusing until you trace through the code and
> find what SDRAM_PARAMS_BASE is used for.

OK, I think I can just remove those first two defines.

>
>> +/*
>> + * NOTE: If more than one of the following is enabled, only one of them will
>> + *      actually be used. RANDOM takes precedence over PATTERN and ZERO, and
>> + *      PATTERN takes precedence overy ZERO.
>> + *
>> + *      RANDOM_AES_BLOCK_IS_PATTERN is to define a 32-bit PATTERN.
>> + */
>> +#define RANDOM_AES_BLOCK_IS_RANDOM     /* to randomize the header */
>> +#undef RANDOM_AES_BLOCK_IS_PATTERN     /* to patternize the header */
>> +#undef RANDOM_AES_BLOCK_IS_ZERO                /* to clear the header */
>
> Why not just used ifdefs to make sure that only (and exactly) one of
> these is defined?

OK

>
>> +/* Currently, this routine returns a 32-bit all 0 seed. */
>> +static u32 query_random_seed(void)
>> +{
>> +       return 0;
>> +}
>
> So the random option isn't really implemented at all. Perhaps just
> remove it completely; if someone wants it, they have to implement this
> function for it to be useful anyway, so adding the call to the function
> at the same time seems reasonable enough?
>
> If not, at least make this print something at run-time so it's obvious
> it isn't really working.

Well I think I'll just punt this stuff.

>
>> +int warmboot_prepare_code(u32 seg_address, u32 seg_length)
> ...
>> +       /*
>> +        * The region specified by seg_address must not be in IRAM and must be
>> +        * nonzero in length.
>> +        */
>> +       if ((seg_length == 0) || (seg_address == 0) ||
>> +           (seg_address >= AP20_BASE_PA_SRAM)) {
>> +               err = -EFAULT;
>> +               goto fail;
>> +       }
>
> The IRAM check only validates that the address isn't at, within, or
> after the IRAM location, which doesn't exactly match the comment. If the
> restriction is "within" IRAM, the end address needs to be checked too.
> Isn't the real restriction that the address needs to be within SDRAM
> though? If so, it seems better to check that directly.
>

Done

> ...
>> +       /* Populate the random_aes_block as requested. */
>> +       {
>> +               u32 *aes_block = (u32 *)&(dst_header->random_aes_block);
>> +               u32 *end = (u32 *)(((u32)aes_block) +
>> +                                  sizeof(dst_header->random_aes_block));
>> +
>> +               do {
>> +#if defined(RANDOM_AES_BLOCK_IS_RANDOM)
>> +                       *aes_block++ = query_random_seed();
>> +#elif defined(RANDOM_AES_BLOCK_IS_PATTERN)
>> +                       *aes_block++ = RANDOM_AES_BLOCK_IS_PATTERN;
>> +#elif defined(RANDOM_AES_BLOCK_IS_ZERO)
>> +                       *aes_block++ = 0;
>> +#else
>> +                       printf("None of RANDOM_AES_BLOCK_IS_XXX is defined; ");
>> +                       printf("Default to pattern 0.\n");
>> +                       *aes_block++ = 0;
>> +#endif
>
> Why not issue a compile error so the person who configured U-Boot is
> immediately aware of the problem, instead of delaying the issue to run-time?

Will punt this.

>
>> +               } while (aes_block < end);
>> +       }
>
>> diff --git a/arch/arm/cpu/armv7/tegra2/warmboot_avp.c b/arch/arm/cpu/armv7/tegra2/warmboot_avp.c
> ...
>> +void wb_start(void)
>> +{
>> +       struct pmux_tri_ctlr *pmt = (struct pmux_tri_ctlr *)NV_PA_APB_MISC_BASE;
>> +       struct pmc_ctlr *pmc = (struct pmc_ctlr *)TEGRA2_PMC_BASE;
>> +       struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE;
>> +       struct clk_rst_ctlr *clkrst =
>> +                       (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE;
>> +       union osc_ctrl_reg osc_ctrl;
>> +       union pllx_base_reg pllx_base;
>> +       union pllx_misc_reg pllx_misc;
>> +       union scratch3_reg scratch3;
>> +       u32 reg;
>> +
>> +       /* enable JTAG & TBE */
>> +       writel(CONFIG_CTL_TBE | CONFIG_CTL_JTAG, &pmt->pmt_cfg_ctl);
>> +
>> +       /* Are we running where we're supposed to be? */
>> +       asm volatile (
>> +               "adr    %0, wb_start;"  /* reg: wb_start address */
>> +               : "=r"(reg)             /* output */
>> +                                       /* no input, no clobber list */
>> +       );
>> +
>> +       if (reg != AP20_WB_RUN_ADDRESS)
>> +               goto do_reset;
>
> Does that code actually work; for it to work, the code would need to be
> either explicitly compile PIC (position independent) or accidentally be
> synthesized to code that happens to be PIC (not sure that's
> possible/likely).

ARM is normally PIC unless you try hard.

>
>> +       /* Set the drive strength */
>> +       osc_ctrl.word = readl(&clkrst->crc_osc_ctrl);
>> +       osc_ctrl.xofs = 4;
>> +       osc_ctrl.xoe = 1;
>> +       writel(osc_ctrl.word, &clkrst->crc_osc_ctrl);
>
> Isn't that board-specific?

If so it currently works on all boards.

>
> Shouldn't the code save the old value during suspend, and restore it
> later or something like that, or be a board-specific #define?

Maybe, but I'm not aware of any differences. Perhaps something to look
at later if it is needed? I will add a TODO.

>
> A general question: How much does this all depend on specific
> interactions with the kernel? Does the kernel have to follow some
> specific protocol during suspend to make this all work? In mainline,
> there's no suspend support in Tegra, so I'm slightly worried that when
> it gets upstreamed, this code might need changes to address any review
> comments there?

My limited understanding is that U-Boot's bit has a clear and small
responsibility to get the memory and CPU clocks running so that the A9
cores start up again, and it is up to Linux to take it from there.

Both the code to save the info and restore it are in U-Boot. There may
be some gotchas when the kernel stuff goes upstream, not sure.

>
>> diff --git a/arch/arm/include/asm/arch-tegra2/warmboot.h b/arch/arm/include/asm/arch-tegra2/warmboot.h
> ...
>> +/*
>> + * Defines the code header information for the boot rom.
>> + *
>> + * The code immediately follows the code header.
>> + *
>> + * Note that the code header needs to be 16 bytes aligned to preserve
>> + * the alignment of relevant data for hash and decryption computations without
>> + * requiring extra copies to temporary memory areas.
>> + */
>> +struct wb_header {
>> +       u32 length_in_secure;   /* length of the code header */
>
> I assume s/in_secure/insecure/ ?

Seems likely to me, will change it.

>
>> +       u32 reserved[3];
>> +       struct hash hash;       /* hash of header+code, starts next field*/
>> +       struct hash random_aes_block;   /* a data block to aid security. */
>> +       u32 length_secure;      /* length of the code header */
>> +       u32 destination;        /* destination address to put the wb code */
>> +       u32 entry_point;        /* execution address of the wb code */
>> +       u32 code_length;        /* length of the code */
>> +};
>
> --
> nvpublic

Regards,
Simon