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.
> +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/
> +/* 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.
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?
--
nvpublic
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
