[U-Boot] [PATCH V2 08/13] ARM: tegra: add SPL/AVP (arm720t) CPU files for Tegra124

Thierry Reding
2014-01-24T15:44:55Z

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140124/c5a23896/attachment.pgp>