On 12/26/2011 12:33 PM, Simon Glass wrote:
From: Yen Lin yelin@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@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?
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.
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.
+/*
- 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?
+/* 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.
+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.
...
/* 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?
} 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).
/* 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?
Shouldn't the code save the old value during suspend, and restore it later or something like that, or be a board-specific #define?
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?
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/ ?
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 */+};