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

[Jon Hunter <jonathanh@nvidia.com>]

On 17/07/15 11:17, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> 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@nvidia.com>
>> ---
>>  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.

I would agree and think we should drop the 2nd parameter and just always
wait. This is only done at boot time and so would only add a little
delay at that point.

Cheers
Jon