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

[Simon Glass]

Hi Stephen,

On Thu, Jan 12, 2012 at 12:43 PM, Simon Glass <sjg at chromium.org> wrote:
> 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.

Yes that works. I want to reduce use of the xxx_ll() functions as much
as possible so have changed this.

Regards,
Simon

>
> Regards,
> Simon
>
>>
>> --
>> nvpublic