Hi Stephen,
> -----Original Message-----
> From: Stephen Warren [mailto:[email protected]]
> Sent: Wednesday, March 18, 2015 11:03 AM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; [email protected]
> Subject: Re: [cbootimage PATCH v1 1/2] Add support for Tegra210
>
> 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.
>
Will do.
> > 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.
>
Ok, it can be removed here. But, the place that actually interprets -t option
may not worth being changed because
both -t and -s run the same code:
case 's':
if (strncmp("tegra", optarg, 5)) {
printf("Unsupported chipname!\n");
usage();
return -EINVAL;
}
optarg += 5;
/* Deliberate fall-through */
case 't':
/* Assign the soc_config based on the chip. */
if (!strcasecmp("20", optarg)) {
t20_get_soc_config(context, &g_soc_config);
} else if (!strcasecmp("30", optarg)) {
t30_get_soc_config(context, &g_soc_config);
} else if (!strcasecmp("114", optarg)) {
t114_get_soc_config(context, &g_soc_config);
} else if (!strcasecmp("124", optarg)) {
t124_get_soc_config(context, &g_soc_config);
} else if (!strcasecmp("132", optarg)) {
t132_get_soc_config(context, &g_soc_config);
} else if (!strcasecmp("210", optarg)) {
t210_get_soc_config(context, &g_soc_config);
} else {
printf("Unsupported chipname!\n");
usage();
return -EINVAL;
}
break;
> > 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.
>
Will do.
> > 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?
Both file nvboot_config.h and sdram_param.h are not needed.
Will submit V2 shortly.
Jimmy
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
