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

[Allen Martin <amartin-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On Mon, Jul 14, 2014 at 10:40:51PM -0700, Vince Hsu wrote:
> On 07/15/2014 06:36 AM, Allen Martin wrote:
> > On Mon, Jul 14, 2014 at 01:56:34PM -0700, Vince Hsu wrote:
> >> Hi,
> >>
> >> On 07/12/2014 02:23 AM, Allen Martin wrote:
> >>>>> +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?
> >> Some of these functions like setbl_param refer to the macros/definitions
> >> in nvboot_bct_txx.h. So maybe they look similar, actually they don't. If
> >> we want to generalize these functions, we might have to refactor
> >> nvboot_bct_txx.h and some more stuff. Can we leave it as is?
> > I can see how this could turn into a can of worms.  I just seems like
> > a shame to have to duplicate so much code for a new SoC that's ~90% the
> > same as the previous SoC.  Can the token tables be reused, or do they
> > have the same issue?
> The token table is something like supported feature list. Every SoC from 
> t20 to t132 has only slight differences from others. But keeping a 
> simple table for each SoC might be reasonable here?
> 

Some of them, like s_sdram_field_table_t124 are not so simple.  Maybe
what's really needed is a better way to describe the BCT that's not
implemented in C code combined with a very generic parser.  

It just doesn't seem maintainable to keep having to copy over a
thousand lines of code for each SoC to just tweak a few fields.  It
makes it hard to review too, since it's really hard to see what the
new Tegra132 features are.

I agree it's probably beyond the scope of this patch to fix this, but
could you add detail to the commit message about the differences
between the Tegra124 and Tegra132 BCT and what changes you made
relative to Tegra124 to help me with the review?

-Allen