Re: [PATCH 2/3] Add Tegra132 support for the cbootimage utility

[Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 07/12/2014 02:23 AM, Allen Martin wrote:
> On Thu, Jul 10, 2014 at 03:21:49AM -0700, Vince Hsu wrote:
>> Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Need more of a patch description here.  At least mention the new
> command line arguments, and what Tegra132 is.
>
>
>> @@ -327,6 +328,27 @@ do {                                                        \
>>          SET_BL_FIELD(to,   field,  v);                      \
>>   } while (0);
>>
>> +#define SET_MTS_FIELD(instance, field, value)   \
>> +do {                                            \
>> +       g_soc_config->set_mts_info(context,         \
>> +               instance,                                                               \
>> +               token_mts_info_##field,                 \
>> +               value);                                                         \
>> +} while (0);
>> +
>> +#define GET_MTS_FIELD(instance, field, ptr)            \
>> +g_soc_config->get_mts_info(context,                            \
>> +               instance,                                                               \
>> +               token_mts_info_##field,                                 \
>> +               ptr);
>> +
>> +#define COPY_MTS_FIELD(from, to, field)         \
>> +do {                                            \
>> +       u_int32_t v;                                \
>> +       GET_MTS_FIELD(from, field, &v);             \
>> +       SET_MTS_FIELD(to,   field,  v);             \
>> +} while (0);
>> +
> The trailing backslashes appear to have a lot of variable whitespace
> here, can you clean these up?
>
>
>>          return err;
>>   }
>>
>> +/*
>> + * Load the mts image then update it with the information from config file.
>> + * In the interest of expediency, all mts's allocated from bottom to top start
>> + * at page 0 of a block, and all mts's allocated from top to bottom end at
>> + * the end of a block.
>> + *
>> + * @param context              The main context pointer
>> + * @return 0 for success
>> + */
>> +static int
>> +write_mts_image(build_image_context *context)
>> +{
> This whole function is almost identical to write_bootloaders(), can
> you pull out the common parts of these functions and parameterize the
> differences to avoid the code duplication?
>
>> @@ -87,6 +89,8 @@ static parse_item parse_simple_items[] =
>>          { "BootLoader=",    token_bootloader,           parse_bootloader },
>>          { "Redundancy=",    token_redundancy,           parse_value_u32 },
>>          { "Bctcopy=",       token_bct_copy,             parse_value_u32 },
>> +       { "MtsPreboot=",    token_mts_preboot,          parse_mts_image},
>> +       { "Mts=",           token_mts,                          parse_mts_image},
> Whitespace looks off here
>
>
>>          { "Version=",       token_version,              parse_value_u32 },
>>          { "PreBctPadBlocks=", token_pre_bct_pad_blocks, parse_value_u32 },
>>          { NULL, 0, NULL } /* Must be last */
>> @@ -105,6 +109,8 @@ static parse_item s_top_level_items[] = {
>>          { "BootLoader=",    token_bootloader,           parse_bootloader },
>>          { "Redundancy=",    token_redundancy,           parse_value_u32 },
>>          { "Bctcopy=",       token_bct_copy,             parse_value_u32 },
>> +       { "MtsPreboot=",    token_mts_preboot,          parse_mts_image},
>> +       { "Mts=",           token_mts,                          parse_mts_image},
> and here
Will modify for all the comments above accordingly. Thanks.
>
>
>> +cbootimage_soc_config tegra132_config = {
>> +       .init_bad_block_table           = t132_init_bad_block_table,
>> +       .set_dev_param                          = t132_set_dev_param,
>> +       .get_dev_param                          = t132_get_dev_param,
>> +       .set_sdram_param                        = t132_set_sdram_param,
>> +       .get_sdram_param                        = t132_get_sdram_param,
>> +       .setbl_param                            = t132_setbl_param,
>> +       .getbl_param                            = t132_getbl_param,
>> +       .set_value                                      = t132_bct_set_value,
>> +       .get_value                                      = t132_bct_get_value,
>> +       .set_data                                       = t132_bct_set_data,
>> +       .get_bct_size                           = t132_get_bct_size,
>> +       .set_mts_info                           = t132_set_mts_info,
>> +       .get_mts_info                           = t132_get_mts_info,
>> +       .token_supported                        = t132_bct_token_supported,
>> +
>> +       .devtype_table                          = s_devtype_table_t132,
>> +       .sdmmc_data_width_table         = s_sdmmc_data_width_table_t132,
>> +       .spi_clock_source_table         = s_spi_clock_source_table_t132,
>> +       .nvboot_memory_type_table       = s_nvboot_memory_type_table_t132,
>> +       .sdram_field_table                      = s_sdram_field_table_t132,
>> +       .nand_table                                     = 0,
>> +       .sdmmc_table                            = s_sdmmc_table_t132,
>> +       .spiflash_table                         = s_spiflash_table_t132,
>> +       .device_type_table                      = s_device_type_table_t132,
>> +};
>> +
> Since Tegra132 and Tegra124 are so similar, can we reuse the Tegra124
> version of any of these to avoid the duplication?
We can export most of the functions in nvbctlib_t124.c for this. Do you 
like that?

Thanks,
Vince
>
>
> -Allen
>
> nvpublic