Hi Stephen,
On Mon, Jan 9, 2012 at 3:38 PM, Stephen Warren <swar...@nvidia.com> wrote:
> On 12/26/2011 12:32 PM, Simon Glass wrote:
>> From: Jimmy Zhang <jimmzh...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
