Hi Stephen,
On Mon, Jan 9, 2012 at 3:38 PM, Stephen Warren swarren@nvidia.com wrote:
On 12/26/2011 12:32 PM, Simon Glass wrote:
From: Jimmy Zhang jimmzhang@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.
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