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

[Thierry Reding <thierry.reding@gmail.com>]

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>