Re: [PATCH V3 07/19] soc: tegra: pmc: Wait for powergate state to change

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5713 bytes --]

On Mon, Jul 13, 2015 at 01:39:45PM +0100, Jon Hunter wrote:
> Currently, the function tegra_powergate_set() simply sets the desired
> powergate state but does not wait for the state to change. In some
> circumstances this can be desirable. However, in most cases we should
> wait for the state to change before proceeding. Therefore, add a
> parameter to tegra_powergate_set() to indicate whether we should wait
> for the state to change.
> 
> By adding this feature, we can also eliminate the polling loop from
> tegra30_boot_secondary().
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/mach-tegra/platsmp.c | 18 ++++--------------
>  drivers/soc/tegra/pmc.c       | 29 +++++++++++++++++++++++------
>  include/soc/tegra/pmc.h       |  2 +-
>  3 files changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
> index b45086666648..13982b5936c0 100644
> --- a/arch/arm/mach-tegra/platsmp.c
> +++ b/arch/arm/mach-tegra/platsmp.c
> @@ -108,19 +108,9 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  	 * be un-gated by un-toggling the power gate register
>  	 * manually.
>  	 */
> -	if (!tegra_pmc_cpu_is_powered(cpu)) {
> -		ret = tegra_pmc_cpu_power_on(cpu);
> -		if (ret)
> -			return ret;
> -
> -		/* Wait for the power to come up. */
> -		timeout = jiffies + msecs_to_jiffies(100);
> -		while (!tegra_pmc_cpu_is_powered(cpu)) {
> -			if (time_after(jiffies, timeout))
> -				return -ETIMEDOUT;
> -			udelay(10);
> -		}
> -	}
> +	ret = tegra_pmc_cpu_power_on(cpu, true);
> +	if (ret)
> +		return ret;
>  
>  remove_clamps:
>  	/* CPU partition is powered. Enable the CPU clock. */
> @@ -162,7 +152,7 @@ static int tegra114_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  		 * also initial power state in flow controller. After that,
>  		 * the CPU's power state is maintained by flow controller.
>  		 */
> -		ret = tegra_pmc_cpu_power_on(cpu);
> +		ret = tegra_pmc_cpu_power_on(cpu, false);
>  	}
>  
>  	return ret;
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 300f11e0c3bb..c0635bdd4ee3 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -175,9 +175,11 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
>   * @id: partition ID
>   * @new_state: new state of the partition

The comment here isn't updated.

>   */
> -static int tegra_powergate_set(int id, bool new_state)
> +static int tegra_powergate_set(int id, bool new_state, bool wait)

Can we please not chain boolean parameters, it makes the call sites
impossible to parse for humans. Instead, can we simply leave
tegra_powergate_set() as it is and add another function, such as
tegra_powergate_set_sync() or tegra_powergate_set_and_wait(), to achieve
the same? You may have to split out a tegra_powergate_set_unlocked() or
similar to make sure you get to keep the lock across both operations:

	static int tegra_powergate_set_unlocked(int id, bool new_state)
	{
		...
	}

	static int tegra_powergate_set(int id, bool new_state)
	{
		int err;

		mutex_lock(&pmc->powergates_lock);
		err = tegra_powergate_set_unlocked(id, new_state);
		mutex_unlock(&pmc->powergates_lock);

		return err;
	}

	/*
	 * Must be called with pmc->powergates_lock mutex held.
	 */
	static int tegra_powergate_wait(int id, bool new_state)
	{
		...
	}

	static int tegra_powergate_set_and_wait(int id, bool new_state)
	{
		int err;

		mutex_lock(&pmc->powergates_lock);

		err = tegra_powergate_set_unlocked(id, new_state);
		if (err < 0)
			goto unlock;

		err = tegra_powergate_wait(id, new_state);
		if (err < 0)
			goto unlock;

	unlock:
		mutex_unlock(&pmc->powergates_lock);
		return err;
	}

>  {
> +	unsigned long timeout;
>  	bool status;
> +	int ret = 0;
>  
>  	mutex_lock(&pmc->powergates_lock);
>  
> @@ -190,9 +192,23 @@ static int tegra_powergate_set(int id, bool new_state)
>  
>  	tegra_pmc_writel(PWRGATE_TOGGLE_START | id, PWRGATE_TOGGLE);
>  
> +	if (wait) {
> +		timeout = jiffies + msecs_to_jiffies(100);
> +		ret = -ETIMEDOUT;
> +
> +		while (time_before(jiffies, timeout)) {
> +			status = !!(tegra_pmc_readl(PWRGATE_STATUS) & BIT(id));
> +			if (status == new_state) {
> +				ret = 0;
> +				break;
> +			}
> +			udelay(10);
> +		}
> +	}
> +
>  	mutex_unlock(&pmc->powergates_lock);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> @@ -204,7 +220,7 @@ int tegra_powergate_power_on(int id)
>  	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>  		return -EINVAL;
>  
> -	return tegra_powergate_set(id, true);
> +	return tegra_powergate_set(id, true, true);
>  }
>  
>  /**
> @@ -216,7 +232,7 @@ int tegra_powergate_power_off(int id)
>  	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>  		return -EINVAL;
>  
> -	return tegra_powergate_set(id, false);
> +	return tegra_powergate_set(id, false, true);
>  }
>  EXPORT_SYMBOL(tegra_powergate_power_off);
>  
> @@ -351,8 +367,9 @@ bool tegra_pmc_cpu_is_powered(int cpuid)
>  /**
>   * tegra_pmc_cpu_power_on() - power on CPU partition
>   * @cpuid: CPU partition ID
> + * @wait:  Wait for CPU state to transition
>   */
> -int tegra_pmc_cpu_power_on(int cpuid)
> +int tegra_pmc_cpu_power_on(int cpuid, bool wait)

This one is probably fine since it's the only boolean parameter so far.
That said, I see that we call this exactly twice, so I wonder if there'd
be any harm in having the second occurrence wait as well and hence get
rid of the parameter.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]