On 03/16/2015 04:51 PM, Jimmy Zhang wrote:
use option -t 210 or -s tegra210 to specify t210.
"Use" should be capitalized. A slightly more general and verbose
description might be appropriate.
diff --git a/src/bct_dump.c b/src/bct_dump.c
{ token_partition_size, "PartitionSize = ", format_u32_hex8 },
{ token_odm_data, "OdmData = ", format_u32_hex8 },
{ token_secure_jtag_control, "JtagCtrl = ", format_u32_hex8 },
+ { token_secure_debug_control, "DebugCtrl = ", format_u32 },
{ token_unique_chip_id, "ChipUid = ", format_chipuid },
I don't think the = lines up there. The open " is shifted 1 character
relative to the other lines, but the closing quote by 2 characters.
diff --git a/src/cbootimage.c b/src/cbootimage.c
@@ -71,11 +71,11 @@ usage(void)
printf(" -gbct Generate the new bct file.\n");
printf(" -o<ODM_DATA> Specify the odm_data(in hex).\n");
printf(" -t|--tegra NN Select target device. Must be one of:\n");
- printf(" 20, 30, 114, 124, 132.\n");
+ printf(" 20, 30, 114, 124, 132, 210.\n");
Since the option is deprecated, I think we should only support --soc for
Tegra210. I won't mention this in any other places that would need to be
updated to fix that.
diff --git a/src/t210/nvbctlib_t210.c b/src/t210/nvbctlib_t210.c
I did not review the bulk of this file in any detail at all; I simply
assume that it is structured the same way as the equivalent files for
previous SoCs, and likewise interacts with the rest of the code in the
exact same way. The same is actually true for pretty much any part of
the patch that's just a large list of defines, structures, or
lookup/mapping code.
+int if_bct_is_t210_get_soc_config(build_image_context *context,
+ cbootimage_soc_config **soc_config)
+{
+ nvboot_config_table *bct = (nvboot_config_table *) context->bct;
There shouldn't be a space after the ) in the cast.
diff --git a/src/t210/nvboot_config.h b/src/t210/nvboot_config.h
+/**
+ * Memory range constants.
+ * The range is defined as [Start, End)
+ */
+/*
+ * Note: The following symbolic definitions are consistent with both AP15
+ * and AP20. However, they rely upon constants from project.h, the
+ * inclusion of which in the SW tree is undesirable. Therefore, explicit
+ * addresses are used, and these are specific to individual chips or chip
+ * families. The constants here are for T35.
+ * #define NVBOOT_BL_IRAM_START (NV_ADDRESS_MAP_IRAM_A_BASE + 0xE000)
+ * #define NVBOOT_BL_IRAM_END (NV_ADDRESS_MAP_IRAM_D_LIMIT + 1)
+ * #define NVBOOT_BL_SDRAM_START (NV_ADDRESS_MAP_EMEM_BASE)
+ *
+ * As T35 bootrom needs 8K more IRAM size, NVBOOT_BL_IRAM_START has changed to,
+ * #define NVBOOT_BL_IRAM_START (NV_ADDRESS_MAP_IRAM_A_BASE + 0xE000)
+ */
I suspect we should remove the references to T35.
+/**
+ * Defines the starting physical address of IRAM
+ */
+#define NVBOOT_BL_IRAM_START (0x40000000 + 0x10000)
That isn't the physical start address of IRAM; it's 0x10000 bytes beyond
that. We should fix the comment or the value.
+/**
+ * Defines the ending physical address of IRAM
+ */
+#define NVBOOT_BL_IRAM_END (0x4003ffff + 1)
Why not just 0x40040000?
+/**
+ * Defines the IROM address where the factory secure provisioning keys start.
+ */
+#define NVBOOT_FACTORY_SECURE_PROVISIONING_KEYS_START (NV_ADDRESS_MAP_IROM_BASE + NV_ADDRESS_MAP_IROM_SIZE - 0x1000)
Are all these constants actually required? There's no equivalent in
previous header files in tegrarcm. Can you whittle this header down so
it only contains what's actually required and/or the equivalent of
pre-existing headers for other SoCs?
I wonder if this patch/header-file was IP reviewed before posting?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
