On Thu, Jan 23, 2014 at 05:42:55PM -0700, Stephen Warren wrote: [...]
diff --git a/arch/arm/cpu/arm720t/tegra-common/cpu.c b/arch/arm/cpu/arm720t/tegra-common/cpu.c
[...]
@@ -128,6 +144,18 @@ int pllx_set_rate(struct clk_pll_simple *pll , u32 divn, u32 divm,
debug(" pllx_set_rate entry\n");
+#if defined(CONFIG_TEGRA124)
- struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE;
Eek! You're mixing declarations and code! I'd prefer this to be moved up to where the other variables are declared.
We could turn the #if defined(CONFIG_TEGRA124) into something like this:
if (CONFIG_TEGRA124) { ... }
so that you get a new scope in which the variable can be declared. Of course that won't work since in U-Boot variables aren't usually defined empty...
diff --git a/arch/arm/cpu/arm720t/tegra124/cpu.c b/arch/arm/cpu/arm720t/tegra124/cpu.c
[...]
+static void enable_cpu_power_rail(void) +{
[...]
- /* un-tristate PWR_I2C SCL/SDA, rest of the defaults are correct */
- pinmux_tristate_disable(PINGRP_PWR_I2C_SCL);
- pinmux_tristate_disable(PINGRP_PWR_I2C_SDA);
- pmic_enable_cpu_vdd();
Should the pinmux changes perhaps be moved to the pmic_enable_cpu_vdd() function?
I guess it doesn't really matter since it should be pretty safe to assume that all boards will always use the power I2C to talk to the PMU?
+static bool is_partition_powered(u32 mask) +{
- struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
- u32 reg;
- /* Get power gate status */
- reg = readl(&pmc->pmc_pwrgate_status);
- return (reg & mask) == mask;
+}
Why can't we pass in the partition ID? That way we don't even have to define the masks in the header file. It's pretty redundant.
+static void power_partition(u32 status, u32 partid) +{
- struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
- debug("%s: status = %08X, part ID = %08X\n", __func__, status, partid);
- /* Is the partition already on? */
- if (!is_partition_powered(status)) {
/* No, toggle the partition power state (OFF -> ON) */debug("power_partition, toggling state\n");writel(START_CP | partid, &pmc->pmc_pwrgate_toggle);/* Wait for the power to come up */while (!is_partition_powered(status));/* Give I/O signals time to stabilize */udelay(IO_STABILIZATION_DELAY);- }
+}
This is being called as follows:
power_partition(CRAIL, CRAILID); power_partition(C0NC, C0NCID); power_partition(CE0, CE0ID);
So instead of passing in (1 << CRAILID, CRAILID), why not just pass around the partition ID only and compute the status mask as needed?
Now that I mention it, I do have a vague recollection that I said the exact same thing during my initial review of Tom's patches.
Thierry