tegra: warmboot (suspend / resume) support

+static u8 rcon[11] = {

lib/aes.c line 299

Quoted 11 lines

lib/aes.c new file +8 -0

 
+static u8 sbox[256] = {
+static u8 inv_sbox[256] = {
+static u8 x2_sbox[256] = {
+static u8 x3_sbox[256] = {
+static u8 x_time_9[256] = {
+static u8 x_time_b[256] = {
+static u8 x_time_d[256] = {
+static u8 x_time_e[256] = {

const

lib/aes.c line 439

Quoted 4 lines

lib/aes.c new file +3 -0

+static void mix_sub_columns(u8 *state)
+{
...
+		 x3_sbox[state[2]] ^       sbox[state[7]];

looks bad whitespace after that ^

Quoted 1 line

+static u8 rcon[11] = {

const
-mike

Quoted 22 lines · On Monday 26 December 2011 14:32:54 Simon Glass wrote:

lib/aes.c line 299

Quoted 11 lines

lib/aes.c new file +8 -0

 
+static u8 sbox[256] = {
+static u8 inv_sbox[256] = {
+static u8 x2_sbox[256] = {
+static u8 x3_sbox[256] = {
+static u8 x_time_9[256] = {
+static u8 x_time_b[256] = {
+static u8 x_time_d[256] = {
+static u8 x_time_e[256] = {

const

lib/aes.c line 439

Quoted 4 lines

lib/aes.c new file +3 -0

+static void mix_sub_columns(u8 *state)
+{
...
+		 x3_sbox[state[2]] ^       sbox[state[7]];

looks bad whitespace after that ^

Quoted 3 lines

Quoted 1 line

+static u8 rcon[11] = {

const

We already have checkpatch in tools ... run it on the series.

Quoted 22 lines

Quoted 22 lines · On Monday 26 December 2011 14:32:54 Simon Glass wrote:

lib/aes.c line 299

Quoted 11 lines

lib/aes.c new file +8 -0

 
+static u8 sbox[256] = {
+static u8 inv_sbox[256] = {
+static u8 x2_sbox[256] = {
+static u8 x3_sbox[256] = {
+static u8 x_time_9[256] = {
+static u8 x_time_b[256] = {
+static u8 x_time_d[256] = {
+static u8 x_time_e[256] = {

const

lib/aes.c line 439

Quoted 4 lines

lib/aes.c new file +3 -0

+static void mix_sub_columns(u8 *state)
+{
...
+		 x3_sbox[state[2]] ^       sbox[state[7]];

looks bad whitespace after that ^

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+static u8 rcon[11] = {

const

We already have checkpatch in tools ... run it on the series.

i don't think checkpatch catches const
-mike

Quoted 23 lines · On Sunday 08 January 2012 03:57:47 Marek Vasut wrote:

Quoted 22 lines

Quoted 22 lines · On Monday 26 December 2011 14:32:54 Simon Glass wrote:

lib/aes.c line 299

Quoted 11 lines

lib/aes.c new file +8 -0

 
+static u8 sbox[256] = {
+static u8 inv_sbox[256] = {
+static u8 x2_sbox[256] = {
+static u8 x3_sbox[256] = {
+static u8 x_time_9[256] = {
+static u8 x_time_b[256] = {
+static u8 x_time_d[256] = {
+static u8 x_time_e[256] = {

const

lib/aes.c

Quoted 4 lines

lib/aes.c new file +3 -0

+static void mix_sub_columns(u8 *state)
+{
...
+ ? ? ? ? ? ? ? ?x3_sbox[state[2]] ^ ? ? ? sbox[state[7]];

looks bad whitespace after that ^

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+static u8 rcon[11] = {

const

We already have checkpatch in tools ... run it on the series.

Quoted 2 lines

i don't think checkpatch catches const

Quoted 1 line

-mike

No, but it catches whitespaces ... M

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

arch/arm/cpu/armv7/tegra2/crypto.c new file +8 -0

 
+#ifdef DEBUG
+static void debug_print_vector(char *name, u32 num_bytes, u8 *data)
+{
+	u32 i;
+
+	printf("%s [%d] @0x%08x", name, num_bytes, (u32)data);
+	for (i = 0; i < num_bytes; i++) {
+		if (i % 16 == 0)

Quoted 16 lines · On Monday 26 December 2011 14:32:56 Simon Glass wrote:

arch/arm/cpu/armv7/tegra2/crypto.c new file +13 -0

 
+#ifdef DEBUG
+static void debug_print_vector(char *name, u32 num_bytes, u8 *data)
+{
+ ? ? u32 i;
+
+ ? ? printf("%s [%d] @0x%08x", name, num_bytes, (u32)data);
+ ? ? for (i = 0; i < num_bytes; i++) {
+ ? ? ? ? ? ? if (i % 16 == 0)
+ ? ? ? ? ? ? ? ? ? ? printf(" = ");
+ ? ? ? ? ? ? printf("%02x", data[i]);
+ ? ? ? ? ? ? if ((i+1) % 16 != 0)
+ ? ? ? ? ? ? ? ? ? ? printf(" ");
+ ? ? }

arch/arm/cpu/armv7/tegra2/crypto.c line 55

Quoted 5 lines

+ ? ? printf("\n");

arch/arm/cpu/armv7/tegra2/crypto.c new file +4 -0

+}
+#else
+#define debug_print_vector(name, num_bytes, data)
+#endif

if you used debug() instead of printf(), wouldn't this get optimized without the #ifdef protection ?

-mike

We want to know which type of chip we are running on - the Tegra family has several SKUs. This can be determined by reading a fuse register, so add this function to ap20.

arch/arm/cpu/armv7/tegra2/ap20.c +0 -0

Quoted 3 lines · Simon Glass

We want to know which type of chip we are running on - the Tegra family has several SKUs. This can be determined by reading a fuse register, so add this function to ap20.

...

Quoted 1 line

arch/arm/cpu/armv7/tegra2/ap20.c +0 -0

...

arch/arm/cpu/armv7/tegra2/ap20.c

Quoted 8 lines

arch/arm/cpu/armv7/tegra2/ap20.c +8 -0

+int tegra_get_chip_type(void)
+{
+ ? ? struct fuse_regs *fuse = (struct fuse_regs *)TEGRA2_FUSE_BASE;
+ ? ? uint tegra_sku_id;
+
+ ? ? tegra_sku_id = readl(&fuse->sku_info) & 0xff;
+
+ ? ? switch (tegra_sku_id) {

I think this should read/validate/switch-on the SoC ID field too; I assume the SKU IDs can only be interpreted relative to some specific SoC ID.

Chip ID is bits 15:8 of the register APB_MISC + 0x804, and has value 0x20 for Tegra20, 0x30 for Tegra30.

--
nvpublic

Quoted 1 line · Simon Glass

Quoted 5 lines

I think this should read/validate/switch-on the SoC ID field too; I assume the SKU IDs can only be interpreted relative to some specific SoC ID.

Chip ID is bits 15:8 of the register APB_MISC + 0x804, and has value 0x20 for Tegra20, 0x30 for Tegra30.

My TRM doesn't mention those values. What version are you referring to? I have v06.

Simon Glass wrote at Thursday, January 12, 2012 12:36 PM:

Quoted 2 lines · Stephen Warren

Quoted 1 line · Simon Glass

...

Quoted 8 lines

Quoted 5 lines

I think this should read/validate/switch-on the SoC ID field too; I assume the SKU IDs can only be interpreted relative to some specific SoC ID.

Chip ID is bits 15:8 of the register APB_MISC + 0x804, and has value 0x20 for Tegra20, 0x30 for Tegra30.

My TRM doesn't mention those values. What version are you referring to? I have v06.

In both v06 and v01p (the public version), the register is in section 11.1.6.8. I guess that doesn't actually define the values of the fields, sigh. But, I can say that Tegra20 is 0x20 and Tegra30 is 0x30.

For example, see:

http://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=blob;f=arch/arm/mach-tegra/fuse.c;h=fd703f9c1621a6d93db787e60b0d1b6314f8fb3e;hb=android-tegra-nv-2.6.39

function void tegra_get_tegraid_from_hw, and:

http://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=blob;f=arch/arm/mach-tegra/include/mach/hardware.h;h=d8f9bfbce42d4255d3e91db166f283a48bb4a8f2;hb=android-tegra-nv-2.6.39

for definitions of TEGRA_CHIPID_*.

--
nvpublic

From: Jimmy Zhang <[email protected]>

Add support for setting up the memory controller parameters. Boards can call tegra_set_emc() with a table containing the required parameters.

arch/arm/cpu/armv7/tegra2/emc.c +0 -0

arch/arm/cpu/armv7/tegra2/emc.c +8 -0

+/* The EMC registers have shadow registers.  When the EMC clock is updated
+ * in the clock controller, the shadow registers are copied to the active
+ * registers, allowing glitchless memory bus frequency changes.
+ * This function updates the shadow registers for a new clock frequency,
+ * and relies on the clock lock on the emc clock to avoid races between
+ * multiple frequency changes */
+#define EMC_SDRAM_RATE_T20     (333000 * 2 * 1000)
+#define EMC_SDRAM_RATE_T25     (380000 * 2 * 1000)

+int tegra_set_emc(const struct tegra_emc_table *table, int table_size)

Quoted 5 lines · Simon Glass

From: Jimmy Zhang <[email protected]>

Add support for setting up the memory controller parameters. Boards can call tegra_set_emc() with a table containing the required parameters.

...

Quoted 1 line

arch/arm/cpu/armv7/tegra2/emc.c +0 -0

...

arch/arm/cpu/armv7/tegra2/emc.c line 31

Quoted 2 lines

arch/arm/cpu/armv7/tegra2/emc.c +2 -0

+static const struct tegra_emc_table *tegra_emc_table;
+static int tegra_emc_table_size;

This isn't "table_size", but "number_of_tables" or "num_tables" or "table_count". This sounds nit-picky, but it made me think about this change more than I needed to given its simplicity.

Quoted 3 lines

+static const unsigned long emc_reg_addr[TEGRA_EMC_NUM_REGS] = {
+ ? ? ? 0x2c, ? /* RC */

For reference, I validated that the order or registers here matches that in the DT binding that Olof published for the kernel, which is good.

http://patchwork.ozlabs.org/patch/132928/

Quoted 9 lines

arch/arm/cpu/armv7/tegra2/emc.c +8 -0

+/* The EMC registers have shadow registers. ?When the EMC clock is updated
+ * in the clock controller, the shadow registers are copied to the active
+ * registers, allowing glitchless memory bus frequency changes.
+ * This function updates the shadow registers for a new clock frequency,
+ * and relies on the clock lock on the emc clock to avoid races between
+ * multiple frequency changes */
+#define EMC_SDRAM_RATE_T20 ? ? (333000 * 2 * 1000)
+#define EMC_SDRAM_RATE_T25 ? ? (380000 * 2 * 1000)

...

Quoted 1 line

+int tegra_set_emc(const struct tegra_emc_table *table, int table_size)

...

Quoted 11 lines

+ ? ? ? switch (tegra_get_chip_type()) {
+ ? ? ? case TEGRA_SOC_T20:
+ ? ? ? ? ? ? ? rate ?= EMC_SDRAM_RATE_T20;
+ ? ? ? ? ? ? ? break;
+ ? ? ? case TEGRA_SOC_T25:
+ ? ? ? ? ? ? ? rate ?= EMC_SDRAM_RATE_T25;
+ ? ? ? ? ? ? ? break;
+ ? ? ? default:
+ ? ? ? ? ? ? ? /* unknown chip type, no clk change*/
+ ? ? ? ? ? ? ? return -1;
+ ? ? ? }

I'm not convinced that limiting this to those two specific clocks is correct. I've certainly seen BCTs that appear to run the EMC clock at other frequencies. Specifically, for Seaboard, we appear to have BCTs for 190, 333, 380, and 400MHz internally. I think a board should be able to at least override the default rate selected by that switch statement.

In tegra_emc_set_rate(), it's unclear to me why clock_ll_set_source_divisor() is used to trigger use of the new EMC register content, rather than say clock_set_rate(). I guess that's just how the HW work?

--
nvpublic

Hi Stephen,

Quoted 7 lines · Stephen Warren

Quoted 5 lines · Simon Glass

From: Jimmy Zhang <[email protected]>

Add support for setting up the memory controller parameters. Boards can call tegra_set_emc() with a table containing the required parameters.

...

Quoted 8 lines

Quoted 1 line

arch/arm/cpu/armv7/tegra2/emc.c +0 -0

...

arch/arm/cpu/armv7/tegra2/emc.c line 31

Quoted 2 lines

arch/arm/cpu/armv7/tegra2/emc.c +2 -0

+static const struct tegra_emc_table *tegra_emc_table;
+static int tegra_emc_table_size;

This isn't "table_size", but "number_of_tables" or "num_tables" or "table_count". This sounds nit-picky, but it made me think about this change more than I needed to given its simplicity.

OK changed to num_emc_tables.

Quoted 8 lines

Quoted 3 lines

+static const unsigned long emc_reg_addr[TEGRA_EMC_NUM_REGS] = {
+ ? ? ? 0x2c, ? /* RC */

For reference, I validated that the order or registers here matches that in the DT binding that Olof published for the kernel, which is good.

http://patchwork.ozlabs.org/patch/132928/

Yes I would hope so :-) Thanks for checking. I can't see it committed, so will reply on that thread.

Quoted 24 lines

Quoted 9 lines

arch/arm/cpu/armv7/tegra2/emc.c +8 -0

+/* The EMC registers have shadow registers. ?When the EMC clock is updated
+ * in the clock controller, the shadow registers are copied to the active
+ * registers, allowing glitchless memory bus frequency changes.
+ * This function updates the shadow registers for a new clock frequency,
+ * and relies on the clock lock on the emc clock to avoid races between
+ * multiple frequency changes */
+#define EMC_SDRAM_RATE_T20 ? ? (333000 * 2 * 1000)
+#define EMC_SDRAM_RATE_T25 ? ? (380000 * 2 * 1000)

...

Quoted 1 line

+int tegra_set_emc(const struct tegra_emc_table *table, int table_size)

...

Quoted 11 lines

+ ? ? ? switch (tegra_get_chip_type()) {
+ ? ? ? case TEGRA_SOC_T20:
+ ? ? ? ? ? ? ? rate ?= EMC_SDRAM_RATE_T20;
+ ? ? ? ? ? ? ? break;
+ ? ? ? case TEGRA_SOC_T25:
+ ? ? ? ? ? ? ? rate ?= EMC_SDRAM_RATE_T25;
+ ? ? ? ? ? ? ? break;
+ ? ? ? default:
+ ? ? ? ? ? ? ? /* unknown chip type, no clk change*/
+ ? ? ? ? ? ? ? return -1;
+ ? ? ? }

Quoted 5 lines

I'm not convinced that limiting this to those two specific clocks is correct. I've certainly seen BCTs that appear to run the EMC clock at other frequencies. Specifically, for Seaboard, we appear to have BCTs for 190, 333, 380, and 400MHz internally. I think a board should be able to at least override the default rate selected by that switch statement.

OK. It might be time to move this to the fdt, I will take a look.

Quoted 5 lines

In tegra_emc_set_rate(), it's unclear to me why clock_ll_set_source_divisor() is used to trigger use of the new EMC register content, rather than say clock_set_rate(). I guess that's just how the HW work?

It should be possible to do something like:

? ? ? ?clock_adjust_periph_pll(PERIPH_ID_EMC, CLOCK_ID_MEMORY, 0, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?clock_get_rate(CLOCK_ID_MEMORY));

although it is a lot less efficient. I will try it.

Regards,
Simon

Quoted 3 lines

--
nvpublic

From: Jimmy Zhang <[email protected]>

Power supplies must be adjusted in line with clock frequency. This code provides a simple routine to set the voltage to allow operation at maximum frequency.

Signed-off-by Simon Glass <[email protected]>

---
 arch/arm/cpu/armv7/tegra2/Makefile     |    1 +
 arch/arm/cpu/armv7/tegra2/pmu.c        |  355 ++++++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-tegra2/pmu.h |   63 ++++++
 3 files changed, 419 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/tegra2/pmu.c
 create mode 100644 arch/arm/include/asm/arch-tegra2/pmu.h

Quoted 6 lines · Simon Glass

From: Jimmy Zhang <[email protected]>

Power supplies must be adjusted in line with clock frequency. This code provides a simple routine to set the voltage to allow operation at maximum frequency.

Quoted 8 lines

Signed-off-by Simon Glass <[email protected]>

---

?arch/arm/cpu/armv7/tegra2/Makefile ? ? | ? ?1 +
?arch/arm/cpu/armv7/tegra2/pmu.c ? ? ? ?| ?355 ++++++++++++++++++++++++++++++++
?arch/arm/include/asm/arch-tegra2/pmu.h | ? 63 ++++++

?3 files changed, 419 insertions(+), 0 deletions(-) ?create mode 100644 arch/arm/cpu/armv7/tegra2/pmu.c ?create mode 100644 arch/arm/include/asm/arch-tegra2/pmu.h

This driver appears to be for an I2C-based device, so I assume it'd for a particular PMIC not Tegra's PMC HW module. I imagine this is a driver for the TI TPS6586X, right?

As such, naming this "pmu" and putting it into the Tegra directory doesn't make sense. There should be a generic TPS6586X driver, and possibly a separate file and patch to implement the use of that chip in conjunction with Tegra.

--
nvpublic

Quoted 20 lines · Simon Glass

Quoted 6 lines

From: Jimmy Zhang <[email protected]>

Power supplies must be adjusted in line with clock frequency. This code provides a simple routine to set the voltage to allow operation at maximum frequency.

Quoted 8 lines

Signed-off-by Simon Glass <[email protected]>

---

?arch/arm/cpu/armv7/tegra2/Makefile ? ? | ? ?1 +
?arch/arm/cpu/armv7/tegra2/pmu.c ? ? ? ?| ?355 ++++++++++++++++++++++++++++++++
?arch/arm/include/asm/arch-tegra2/pmu.h | ? 63 ++++++

?3 files changed, 419 insertions(+), 0 deletions(-) ?create mode 100644 arch/arm/cpu/armv7/tegra2/pmu.c ?create mode 100644 arch/arm/include/asm/arch-tegra2/pmu.h

This driver appears to be for an I2C-based device, so I assume it'd for a particular PMIC not Tegra's PMC HW module. I imagine this is a driver for the TI TPS6586X, right?

Quoted 4 lines

As such, naming this "pmu" and putting it into the Tegra directory doesn't make sense. There should be a generic TPS6586X driver, and possibly a separate file and patch to implement the use of that chip in conjunction with Tegra.

Oh dear, but yes you are right. It is a TPS658621C according to my schematic. I will leap into this also.

Simon Glass wrote at Thursday, January 12, 2012 4:17 PM:

Quoted 26 lines · Stephen Warren

Quoted 24 lines · Simon Glass

Quoted 6 lines

From: Jimmy Zhang <[email protected]>

Power supplies must be adjusted in line with clock frequency. This code provides a simple routine to set the voltage to allow operation at maximum frequency.

Quoted 8 lines

Signed-off-by Simon Glass <[email protected]>

---

?arch/arm/cpu/armv7/tegra2/Makefile ? ? | ? ?1 +
?arch/arm/cpu/armv7/tegra2/pmu.c ? ? ? ?| ?355 ++++++++++++++++++++++++++++++++
?arch/arm/include/asm/arch-tegra2/pmu.h | ? 63 ++++++

?3 files changed, 419 insertions(+), 0 deletions(-) ?create mode 100644 arch/arm/cpu/armv7/tegra2/pmu.c ?create mode 100644 arch/arm/include/asm/arch-tegra2/pmu.h

This driver appears to be for an I2C-based device, so I assume it'd for a particular PMIC not Tegra's PMC HW module. I imagine this is a driver for the TI TPS6586X, right?

As such, naming this "pmu" and putting it into the Tegra directory doesn't make sense. There should be a generic TPS6586X driver, and possibly a separate file and patch to implement the use of that chip in conjunction with Tegra.

Quoted 2 lines

Oh dear, but yes you are right. It is a TPS658621C according to my schematic. I will leap into this also.

I believe some number of the devices are compatible, hence the kernel has a tps6586x driver rather than more individual drivers for each value of "x".

--
nvpublic

Adjust PMU to permit maximum frequency operation.

board/nvidia/common/board.c +0 -0

Quoted 1 line · Simon Glass

Adjust PMU to permit maximum frequency operation.

...

Quoted 1 line

board/nvidia/common/board.c +0 -0

...

Quoted 10 lines

board/nvidia/common/board.c +8 -0

+#ifdef CONFIG_TEGRA2_I2C
+ ? ? i2c_init_board();
+
+#ifdef CONFIG_TEGRA_PMU
+ ? ? pmu_set_nominal();
+#endif
+#endif
+

? ? ? return 0;
?}

I think there's some patch confusion here; weren't the outer ifdef and call to i2c_init_board() added in the I2C patch series too?

--
nvpublic

From: Yen Lin <[email protected]>

Add code to set up the warm boot area in the Tegra CPU ready for a resume after suspend.

Signed-off-by Simon Glass <[email protected]>

arch/arm/cpu/armv7/tegra2/Makefile +0 -0

arch/arm/cpu/armv7/tegra2/Makefile +1 -1

-SOBJS  := lowlevel_init.o
+SOBJS-y        := lowlevel_init.o

+SOBJS := $(SOBJS-y)

arch/arm/cpu/armv7/tegra2/warmboot.c +0 -0

+int warmboot_prepare_code(u32 seg_address, u32 seg_length)

arch/arm/cpu/armv7/tegra2/warmboot.c +8 -0

+       /* 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)

arch/arm/cpu/armv7/tegra2/warmboot.c +2 -0

+               } while (aes_block < end);
+       }

arch/arm/cpu/armv7/tegra2/warmboot_avp.c +0 -0

arch/arm/cpu/armv7/tegra2/warmboot_avp.c +15 -0

+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);

arch/arm/include/asm/arch-tegra2/warmboot.h +0 -0

arch/arm/include/asm/arch-tegra2/warmboot.h +8 -0

+       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 */
+};

Quoted 5 lines · Simon Glass

From: Yen Lin <[email protected]>

Add code to set up the warm boot area in the Tegra CPU ready for a resume after suspend.

Quoted 1 line

Signed-off-by Simon Glass <[email protected]>

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.

Quoted 1 line

arch/arm/cpu/armv7/tegra2/Makefile +0 -0

...

Quoted 2 lines

arch/arm/cpu/armv7/tegra2/Makefile +1 -1

-SOBJS ?:= lowlevel_init.o
+SOBJS-y ? ? ? ?:= lowlevel_init.o

...

Quoted 1 line

+SOBJS ?:= $(SOBJS-y)

That's really a separate patch, but I guess it's fine.

Quoted 2 lines

arch/arm/cpu/armv7/tegra2/warmboot.c +0 -0

...

arch/arm/cpu/armv7/tegra2/warmboot.c

Quoted 3 lines

arch/arm/cpu/armv7/tegra2/warmboot.c +3 -0

+#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.

arch/arm/cpu/armv7/tegra2/warmboot.c

Quoted 11 lines

arch/arm/cpu/armv7/tegra2/warmboot.c +10 -0

+/*
+ * 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?

arch/arm/cpu/armv7/tegra2/warmboot.c line 323

Quoted 6 lines

arch/arm/cpu/armv7/tegra2/warmboot.c +5 -0

+/* 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.

Quoted 2 lines

+int warmboot_prepare_code(u32 seg_address, u32 seg_length)

...

Quoted 9 lines

+ ? ? ? /*
+ ? ? ? ?* 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.

...

Quoted 13 lines

+ ? ? ? /* 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));

arch/arm/cpu/armv7/tegra2/warmboot.c +8 -0

+
+ ? ? ? ? ? ? ? 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;

arch/arm/cpu/armv7/tegra2/warmboot.c

Quoted 5 lines

arch/arm/cpu/armv7/tegra2/warmboot.c +5 -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?

Quoted 5 lines

+ ? ? ? ? ? ? ? } while (aes_block < end);
+ ? ? ? }

arch/arm/cpu/armv7/tegra2/warmboot_avp.c +0 -0

...

Quoted 22 lines

arch/arm/cpu/armv7/tegra2/warmboot_avp.c +22 -0

+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 */
+ ? ? ? );

Quoted 3 lines

+
+ ? ? ? 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).

Quoted 6 lines

+ ? ? ? /* 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?

Quoted 2 lines

arch/arm/include/asm/arch-tegra2/warmboot.h +0 -0

...

arch/arm/include/asm/arch-tegra2/warmboot.h

Quoted 11 lines

arch/arm/include/asm/arch-tegra2/warmboot.h +11 -0

+/*
+ * 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/ ?

Quoted 9 lines

+ ? ? ? 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

Hi Stephen,

[Yen please can you read this also?]

Quoted 6 lines · Stephen Warren

Quoted 5 lines · Simon Glass

From: Yen Lin <[email protected]>

Add code to set up the warm boot area in the Tegra CPU ready for a resume after suspend.

Quoted 8 lines

Quoted 1 line

Signed-off-by Simon Glass <[email protected]>

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.

Hi Simon,

Quoted 35 lines · Simon Glass

Hi Stephen,

[Yen please can you read this also?]

Quoted 6 lines · Stephen Warren

Quoted 5 lines · Simon Glass

From: Yen Lin <[email protected]>

Add code to set up the warm boot area in the Tegra CPU ready for a resume after suspend.

Quoted 8 lines

Quoted 1 line

Signed-off-by Simon Glass <[email protected]>

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!

Quoted 7 lines

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.

U-boot code saves sdram parameters to scratch2, scratch4, and scratch 24 registers, so when system resumes, bootrom can restore sdram parameters from these registers.

The scratch3 register content was saved by kernel before suspend, and is used by the warmboot code to restore PLLX settings.

Regards,

Yen

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain confidential information. ?Any unauthorized review, use, disclosure or distribution is prohibited. ?If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Quoted 3 lines · Yen Lin

Quoted 2 lines · Simon Glass

Quoted 1 line · Stephen Warren

On 12/26/2011 12:33 PM, Simon Glass wrote:

Quoted 12 lines

Quoted 7 lines

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.

U-boot code saves sdram parameters to scratch2, scratch4, and scratch 24 registers, so when system resumes, bootrom can restore sdram parameters from these registers.

Quoted 2 lines

The scratch3 register content was saved by kernel before suspend, and is used by the warmboot code to restore PLLX settings.

Thanks for the info.

On resume I do not see it using scratch2, 4 or 24 within U-Boot. Are those used in the kernel?

I should add something to document this so the kernel people can match up when that side of it goes up stream.

Quoted 4 lines · Simon Glass

Quoted 3 lines · Yen Lin

Quoted 2 lines · Simon Glass

Quoted 1 line · Stephen Warren

On 12/26/2011 12:33 PM, Simon Glass wrote:

...

Quoted 12 lines

Quoted 12 lines

Quoted 7 lines

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.

U-boot code saves sdram parameters to scratch2, scratch4, and scratch 24 registers, so when system resumes, bootrom can restore sdram parameters from these registers.

Quoted 7 lines

Quoted 2 lines

The scratch3 register content was saved by kernel before suspend, and is used by the warmboot code to restore PLLX settings.

Thanks for the info.

On resume I do not see it using scratch2, 4 or 24 within U-Boot. Are those used in the kernel?

The Tegra boot ROM (which contains the power-on code for the AVP) is what interprets these, according to Yen above.

One question for Yen: Why doesn't the boot ROM read the SDRAM parameters from the BCT like it does during a cold boot? Perhaps there's some interaction with clock settings, so the boot ROM needs to know the clock/voltage/... state the kernel was in when suspending, rather than just using the "usual" cold boot settings?

Quoted 2 lines

I should add something to document this so the kernel people can match up when that side of it goes up stream.

From: Jimmy Zhang <[email protected]>

At cold boot, the default voltage supplied by pmu is not high enough to support emc to run at its highest clock frequency. The code added here is to update the default vdd_core and vdd_cpu to higher values.

(Sorry if I have mangled this comment)

board/nvidia/common/board.c +0 -0

/*
+ * Routine: power_det_init
+ * Description: turn off power detects
+ */

Quoted 7 lines · Simon Glass

From: Jimmy Zhang <[email protected]>

At cold boot, the default voltage supplied by pmu is not high enough to support emc to run at its highest clock frequency. The code added here is to update the default vdd_core and vdd_cpu to higher values.

(Sorry if I have mangled this comment)

?? Remove that last line?

I thought this issue only affected the T25 SKU when coupled with a particular voltage regulator that wasn't reprogrammed to explicitly support T25. Hence, shouldn't this code only be enabled for specific boards?

Quoted 1 line

board/nvidia/common/board.c +0 -0

...

Quoted 4 lines

?/*
+ * Routine: power_det_init
+ * Description: turn off power detects
+ */

board/nvidia/common/board.c int timer_init(void) · line 72

Quoted 10 lines

board/nvidia/common/board.c +10 -0

+static void power_det_init(void)
+{
+#if defined(CONFIG_TEGRA2)
+ ? ? struct pmc_ctlr *const pmc = (struct pmc_ctlr *)TEGRA2_PMC_BASE;
+
+ ? ? /* turn off power detects */
+ ? ? writel(0, &pmc->pmc_pwr_det_latch);
+ ? ? writel(0, &pmc->pmc_pwr_det);
+#endif
+}

I can't tell how that code relates to the commit description.

--
nvpublic

From: Jimmy Zhang <[email protected]>

Set Seaboard and Harmony to optimal memory settings based on the SOC in use (T20 or T25).

Signed-off-by Simon Glass <[email protected]>

board/nvidia/common/emc.c +0 -0

+static const struct tegra_emc_table seaboard_emc_tables_hynix_333Mhz[] = {

+static const struct tegra_emc_table seaboard_emc_tables_hynix_380Mhz[] = {

+void seaboard_emc_init(void)

Quoted 8 lines · Simon Glass

From: Jimmy Zhang <[email protected]>

Set Seaboard and Harmony to optimal memory settings based on the SOC in use (T20 or T25).

Signed-off-by Simon Glass <[email protected]>

board/nvidia/common/emc.c +0 -0

...

Quoted 1 line

+static const struct tegra_emc_table seaboard_emc_tables_hynix_333Mhz[] = {

...

Quoted 1 line

+static const struct tegra_emc_table seaboard_emc_tables_hynix_380Mhz[] = {

...

Quoted 1 line

+void seaboard_emc_init(void)

...

board/nvidia/common/emc.c

Quoted 10 lines

board/nvidia/common/emc.c +10 -0

+static struct emc_init board_table[] = {
+ ? ? ? {
+ ? ? ? ? ? ? ? .id ? ? = MACH_TYPE_HARMONY,
+ ? ? ? ? ? ? ? .init ? = NULL,
+ ? ? ? },
+ ? ? ? {
+ ? ? ? ? ? ? ? .id ? ? = MACH_TYPE_SEABOARD,
+ ? ? ? ? ? ? ? .init ? = seaboard_emc_init,
+ ? ? ? },
+};

Why not just put the Seaboard-specific code into Seaboard's board directory instead of a common location? Then you wouldn't need this table at all, and boards other than Seaboard wouldn't be required to carry the Seaboard EMC tables.

There don't appear to be any tables for Harmony, which is inconsistent with the commit description.

board/nvidia/common/emc.c line 284

Quoted 11 lines

board/nvidia/common/emc.c +10 -0

+int board_emc_init(void)
+{
+ ? ? ? int ? ? i;
+ ? ? ? DECLARE_GLOBAL_DATA_PTR;
+
+#ifdef CONFIG_TEGRA_PMU
+ ? ? ? /* if voltage has not been set properly, return */
+ ? ? ? if (!pmu_is_voltage_nominal())
+ ? ? ? ? ? ? ? return -1;
+#endif

Why/when would the PMU voltage not be nominal?

Can't we error out the compile if the options that cause the PMU voltage to be initialized to nominal are not set, instead of detecting this at runtime?

Finally, I don't think there are EMC tables here for all the EMC frequencies that we have BCTs for. Perhaps that's not an issue, and perhaps I should ignore the other BCTs (e.g. 400MHZ EMC) since nobody actually uses them?

--
nvpublic

Quoted 5 lines · Simon Glass

Quoted 4 lines

From: Jimmy Zhang <[email protected]>

Set Seaboard and Harmony to optimal memory settings based on the SOC in use (T20 or T25).

Quoted 4 lines

Quoted 4 lines

board/nvidia/common/emc.c new file +4 -0

+int board_emc_init(void)
+{
+ ? ? ? int ? ? i;
+ ? ? ? DECLARE_GLOBAL_DATA_PTR;

Quoted 8 lines

board/nvidia/common/emc.c line 284

Quoted 6 lines

board/nvidia/common/emc.c new file +6 -0

+
+#ifdef CONFIG_TEGRA_PMU
+ ? ? ? /* if voltage has not been set properly, return */
+ ? ? ? if (!pmu_is_voltage_nominal())
+ ? ? ? ? ? ? ? return -1;
+#endif

Why/when would the PMU voltage not be nominal?

On boot, it starts up lower and we raise it to nominal so we can run at full speed.

Quoted 3 lines

Can't we error out the compile if the options that cause the PMU voltage to be initialized to nominal are not set, instead of detecting this at runtime?

I don't think so, since we can't know in U-Boot what the start-up voltages are.

Quoted 4 lines

Finally, I don't think there are EMC tables here for all the EMC frequencies that we have BCTs for. Perhaps that's not an issue, and perhaps I should ignore the other BCTs (e.g. 400MHZ EMC) since nobody actually uses them?

Yes I think maximum speed is all anyone is probably interested in now.

Simon Glass wrote at Thursday, January 12, 2012 4:05 PM:

Quoted 6 lines · Stephen Warren

Quoted 5 lines · Simon Glass

Quoted 4 lines

From: Jimmy Zhang <[email protected]>

Set Seaboard and Harmony to optimal memory settings based on the SOC in use (T20 or T25).

...

Quoted 20 lines

Quoted 4 lines

Quoted 4 lines

board/nvidia/common/emc.c new file +4 -0

+int board_emc_init(void)
+{
+ ? ? ? int ? ? i;
+ ? ? ? DECLARE_GLOBAL_DATA_PTR;

Quoted 8 lines

board/nvidia/common/emc.c line 284

Quoted 6 lines

board/nvidia/common/emc.c new file +6 -0

+
+#ifdef CONFIG_TEGRA_PMU
+ ? ? ? /* if voltage has not been set properly, return */
+ ? ? ? if (!pmu_is_voltage_nominal())
+ ? ? ? ? ? ? ? return -1;
+#endif

Why/when would the PMU voltage not be nominal?

On boot, it starts up lower and we raise it to nominal so we can run at full speed.

Quoted 3 lines

Can't we error out the compile if the options that cause the PMU voltage to be initialized to nominal are not set, instead of detecting this at runtime?

Quoted 1 line

I don't think so, since we can't know in U-Boot what the start-up voltages are.

So how does the system get to the nominal state? And if board_emc_init() is called when the system isn't in the nominal state, does it somehow get called again later once it is, so that the EMC initialization doesn't fail the error-check quoted above?

In other words, presumably U-Boot explicitly programs the PMU into the nominal stage at some point. Shouldn't we defer calling board_emc_init() until after that time, thus making that error-check redundant?

Quoted 7 lines

Quoted 6 lines

Finally, I don't think there are EMC tables here for all the EMC frequencies that we have BCTs for. Perhaps that's not an issue, and perhaps I should ignore the other BCTs (e.g. 400MHZ EMC) since nobody actually uses them?

Quoted 1 line

Yes I think maximum speed is all anyone is probably interested in now.

400MHz is faster than the entries in the table in this change, IIRC. But if the tables in this change are for the standard frequencies average people use (i.e. not those doing board/memory quals), it's probably fine.

--
nvpublic

Quoted 28 lines

Simon Glass wrote at Thursday, January 12, 2012 4:05 PM:

Quoted 6 lines · Stephen Warren

Quoted 5 lines · Simon Glass

Quoted 4 lines

From: Jimmy Zhang <[email protected]>

Set Seaboard and Harmony to optimal memory settings based on the SOC in use (T20 or T25).

...

Quoted 20 lines

Quoted 4 lines

Quoted 4 lines

board/nvidia/common/emc.c new file +4 -0

+int board_emc_init(void)
+{
+ ? ? ? int ? ? i;
+ ? ? ? DECLARE_GLOBAL_DATA_PTR;

Quoted 8 lines

board/nvidia/common/emc.c line 284

Quoted 6 lines

board/nvidia/common/emc.c new file +6 -0

+
+#ifdef CONFIG_TEGRA_PMU
+ ? ? ? /* if voltage has not been set properly, return */
+ ? ? ? if (!pmu_is_voltage_nominal())
+ ? ? ? ? ? ? ? return -1;
+#endif

Why/when would the PMU voltage not be nominal?

On boot, it starts up lower and we raise it to nominal so we can run at full speed.

Quoted 3 lines

Can't we error out the compile if the options that cause the PMU voltage to be initialized to nominal are not set, instead of detecting this at runtime?

Quoted 6 lines

Quoted 1 line

I don't think so, since we can't know in U-Boot what the start-up voltages are.

So how does the system get to the nominal state? And if board_emc_init() is called when the system isn't in the nominal state, does it somehow get called again later once it is, so that the EMC initialization doesn't fail the error-check quoted above?

We call board_emc_init() after pmu_set_nominal().

Quoted 4 lines

In other words, presumably U-Boot explicitly programs the PMU into the nominal stage at some point. Shouldn't we defer calling board_emc_init() until after that time, thus making that error-check redundant?

Yes, but if you look at the patch above, that's what we do:

 #ifdef CONFIG_TEGRA_PMU
       pmu_set_nominal();

Simon Glass wrote at Thursday, January 12, 2012 4:55 PM:

Quoted 43 lines · Stephen Warren

Quoted 28 lines

Simon Glass wrote at Thursday, January 12, 2012 4:05 PM:

Quoted 6 lines · Stephen Warren

Quoted 5 lines · Simon Glass

Quoted 4 lines

From: Jimmy Zhang <[email protected]>

Set Seaboard and Harmony to optimal memory settings based on the SOC in use (T20 or T25).

...

Quoted 20 lines

Quoted 4 lines

Quoted 4 lines

board/nvidia/common/emc.c new file +4 -0

+int board_emc_init(void)
+{
+ ? ? ? int ? ? i;
+ ? ? ? DECLARE_GLOBAL_DATA_PTR;

Quoted 8 lines

board/nvidia/common/emc.c line 284

Quoted 6 lines

board/nvidia/common/emc.c new file +6 -0

+
+#ifdef CONFIG_TEGRA_PMU
+ ? ? ? /* if voltage has not been set properly, return */
+ ? ? ? if (!pmu_is_voltage_nominal())
+ ? ? ? ? ? ? ? return -1;
+#endif

Why/when would the PMU voltage not be nominal?

On boot, it starts up lower and we raise it to nominal so we can run at full speed.

Quoted 3 lines

Can't we error out the compile if the options that cause the PMU voltage to be initialized to nominal are not set, instead of detecting this at runtime?

Quoted 6 lines

Quoted 1 line

I don't think so, since we can't know in U-Boot what the start-up voltages are.

So how does the system get to the nominal state? And if board_emc_init() is called when the system isn't in the nominal state, does it somehow get called again later once it is, so that the EMC initialization doesn't fail the error-check quoted above?

We call board_emc_init() after pmu_set_nominal().

Quoted 4 lines

In other words, presumably U-Boot explicitly programs the PMU into the nominal stage at some point. Shouldn't we defer calling board_emc_init() until after that time, thus making that error-check redundant?

Quoted 8 lines

Yes, but if you look at the patch above, that's what we do:

?#ifdef CONFIG_TEGRA_PMU
? ? ? ?pmu_set_nominal();
+
+ ? ? ? board_emc_init();
?#endif
?#endif

OK, so in practice,

/* if voltage has not been set properly, return */ if (!pmu_is_voltage_nominal())

... will never fire. My original point was that if so, why is that check needed? I suppose it's a reasonable safety net though - that's the reason?

--
nvpublic

Quoted 53 lines

Simon Glass wrote at Thursday, January 12, 2012 4:55 PM:

Quoted 43 lines · Stephen Warren

Quoted 28 lines

Simon Glass wrote at Thursday, January 12, 2012 4:05 PM:

Quoted 6 lines · Stephen Warren

Quoted 5 lines · Simon Glass

Quoted 4 lines

From: Jimmy Zhang <[email protected]>

Set Seaboard and Harmony to optimal memory settings based on the SOC in use (T20 or T25).

...

Quoted 20 lines

Quoted 4 lines

Quoted 4 lines

(quoted diff) +4 -0

+int board_emc_init(void)
+{
+ ? ? ? int ? ? i;
+ ? ? ? DECLARE_GLOBAL_DATA_PTR;

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+
+#ifdef CONFIG_TEGRA_PMU
+ ? ? ? /* if voltage has not been set properly, return */
+ ? ? ? if (!pmu_is_voltage_nominal())
+ ? ? ? ? ? ? ? return -1;
+#endif

Why/when would the PMU voltage not be nominal?

On boot, it starts up lower and we raise it to nominal so we can run at full speed.

Quoted 3 lines

Can't we error out the compile if the options that cause the PMU voltage to be initialized to nominal are not set, instead of detecting this at runtime?

Quoted 6 lines

Quoted 1 line

I don't think so, since we can't know in U-Boot what the start-up voltages are.

So how does the system get to the nominal state? And if board_emc_init() is called when the system isn't in the nominal state, does it somehow get called again later once it is, so that the EMC initialization doesn't fail the error-check quoted above?

We call board_emc_init() after pmu_set_nominal().

Quoted 4 lines

In other words, presumably U-Boot explicitly programs the PMU into the nominal stage at some point. Shouldn't we defer calling board_emc_init() until after that time, thus making that error-check redundant?

Quoted 8 lines

Yes, but if you look at the patch above, that's what we do:

?#ifdef CONFIG_TEGRA_PMU
? ? ? ?pmu_set_nominal();
+
+ ? ? ? board_emc_init();
?#endif
?#endif

Quoted 8 lines

OK, so in practice,

/* if voltage has not been set properly, return */ if (!pmu_is_voltage_nominal())

... will never fire. My original point was that if so, why is that check needed? I suppose it's a reasonable safety net though - that's the reason?

OK I see. It certainly shouldn't - it is a check that everything is well since this code is in a different file and it is possible that someone may get this wrong. If they do then the system may continue but die later in interesting ways. Still, the user has other equally complex things to worry about.

I'm happy to remove this particularly as this might become example code for other boards - what do you think?

Simon Glass wrote at Thursday, January 12, 2012 5:06 PM:

Quoted 63 lines · Stephen Warren

Quoted 53 lines

Simon Glass wrote at Thursday, January 12, 2012 4:55 PM:

Quoted 43 lines · Stephen Warren

Quoted 28 lines

Simon Glass wrote at Thursday, January 12, 2012 4:05 PM:

Quoted 6 lines · Stephen Warren

Quoted 5 lines · Simon Glass

Quoted 4 lines

From: Jimmy Zhang <[email protected]>

Set Seaboard and Harmony to optimal memory settings based on the SOC in use (T20 or T25).

...

Quoted 20 lines

Quoted 4 lines

Quoted 4 lines

(quoted diff) +4 -0

+int board_emc_init(void)
+{
+ ? ? ? int ? ? i;
+ ? ? ? DECLARE_GLOBAL_DATA_PTR;

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+
+#ifdef CONFIG_TEGRA_PMU
+ ? ? ? /* if voltage has not been set properly, return */
+ ? ? ? if (!pmu_is_voltage_nominal())
+ ? ? ? ? ? ? ? return -1;
+#endif

Why/when would the PMU voltage not be nominal?

On boot, it starts up lower and we raise it to nominal so we can run at full speed.

Quoted 3 lines

Can't we error out the compile if the options that cause the PMU voltage to be initialized to nominal are not set, instead of detecting this at runtime?

Quoted 6 lines

Quoted 1 line

I don't think so, since we can't know in U-Boot what the start-up voltages are.

So how does the system get to the nominal state? And if board_emc_init() is called when the system isn't in the nominal state, does it somehow get called again later once it is, so that the EMC initialization doesn't fail the error-check quoted above?

We call board_emc_init() after pmu_set_nominal().

Quoted 4 lines

In other words, presumably U-Boot explicitly programs the PMU into the nominal stage at some point. Shouldn't we defer calling board_emc_init() until after that time, thus making that error-check redundant?

Quoted 8 lines

Yes, but if you look at the patch above, that's what we do:

?#ifdef CONFIG_TEGRA_PMU
? ? ? ?pmu_set_nominal();
+
+ ? ? ? board_emc_init();
?#endif
?#endif

Quoted 8 lines

OK, so in practice,

/* if voltage has not been set properly, return */ if (!pmu_is_voltage_nominal())

... will never fire. My original point was that if so, why is that check needed? I suppose it's a reasonable safety net though - that's the reason?

Quoted 8 lines

OK I see. It certainly shouldn't - it is a check that everything is well since this code is in a different file and it is possible that someone may get this wrong. If they do then the system may continue but die later in interesting ways. Still, the user has other equally complex things to worry about.

I'm happy to remove this particularly as this might become example code for other boards - what do you think?

The check itself is probably fine for the reasons you state. However, I'd suggest adjusting the comment to something more like:

This code should only be called once the PMU is operating at nominal voltage. Hence, this test should never fail. However, this prevents unpredictable failures from occurring later if this pre-condition is not met.

--
nvpublic