[U-Boot] [PATCH 06/14] tegra: Add EMC support for optimal memory timings

[Simon Glass]

Hi Stephen,

On Mon, Jan 9, 2012 at 3:38 PM, Stephen Warren <swarren at nvidia.com> wrote:
> On 12/26/2011 12:32 PM, Simon Glass wrote:
>> From: Jimmy Zhang <jimmzhang at nvidia.com>
>>
>> Add support for setting up the memory controller parameters. Boards
>> can call tegra_set_emc() with a table containing the required
>> parameters.
> ...
>> diff --git a/arch/arm/cpu/armv7/tegra2/emc.c b/arch/arm/cpu/armv7/tegra2/emc.c
> ...
>> +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.

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

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

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

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

>
> --
> nvpublic