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
pgpxojgfNAvRM.pgp
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
