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