Re: [PATCH V4 08/16] soc: tegra: pmc: Fix checking of valid partitions

[Jon Hunter <jonathanh@nvidia.com>]

On 14/01/16 14:11, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Fri, Dec 04, 2015 at 02:57:09PM +0000, Jon Hunter wrote:
>> The tegra power partitions are referenced by a numerical ID which are
>> the same values programmed into the PMC registers for controlling the
>> partition. For a given device, the valid partition IDs may not be
>> contiguous and so simply checking that an ID is not greater than the
>> maximum ID supported may not mean it is valid. Fix this by computing
>> a bit-mask of the valid partition IDs for a device and add a macro that
>> will test if the partition is valid based upon this mask.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/soc/tegra/pmc.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index 28d3106d3add..0967bba13947 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -103,6 +103,7 @@
>>  #define GPU_RG_CNTRL			0x2d4
>>  
>>  #define PMC_PWRGATE_STATE(status, id)	((status & BIT(id)) != 0)
>> +#define PMC_PWRGATE_IS_VALID(id)	(pmc->powergates_mask & BIT(id))
> 
> I'd prefer a static inline function here, too. If you do that you will
> also need to move the static inline below the struct tegra_pmc to avoid
> build errors. Also don't forget to check for id < 0, because it
> technically could be. It may also be worth checking for an upper bound
> just in case anybody was going to pass in a value other that the ones
> defined in include/soc/tegra/pmc.h.

Ok.

>>  struct tegra_pmc_soc {
>>  	unsigned int num_powergates;
>> @@ -134,6 +135,7 @@ struct tegra_pmc_soc {
>>   * @cpu_pwr_good_en: CPU power good signal is enabled
>>   * @lp0_vec_phys: physical base address of the LP0 warm boot code
>>   * @lp0_vec_size: size of the LP0 warm boot code
>> + * @powergates_mask: Bit mask of valid power gates
>>   * @powergates_lock: mutex for power gate register access
>>   */
>>  struct tegra_pmc {
>> @@ -158,6 +160,7 @@ struct tegra_pmc {
>>  	bool cpu_pwr_good_en;
>>  	u32 lp0_vec_phys;
>>  	u32 lp0_vec_size;
>> +	u32 powergates_mask;
> 
> This seems rather risky. The highest partition ID that we currently have
> is 29, so there is potential that 32 bits will be exceeded in the near
> future. It'd be more future-proof to turn this into a bitmap from the
> start so that we never have to worry about it.

Yes, I had thought about that, but figured we could handle later. Ok, I
will look at this now.

>>  
>>  	struct mutex powergates_lock;
>>  };
>> @@ -213,7 +216,7 @@ static int tegra_powergate_set(int id, bool new_state)
>>   */
>>  int tegra_powergate_power_on(int id)
>>  {
>> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> +	if (!PMC_PWRGATE_IS_VALID(id))
>>  		return -EINVAL;
> 
> In a similar vein I've been meaning, for a long time, to make the ID an
> unsigned integer. It might be worth doing that, even with this patch
> applied, but it can be a separate patch.
> 
>>  	return tegra_powergate_set(id, true);
>> @@ -225,7 +228,7 @@ int tegra_powergate_power_on(int id)
>>   */
>>  int tegra_powergate_power_off(int id)
>>  {
>> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> +	if (!PMC_PWRGATE_IS_VALID(id))
>>  		return -EINVAL;
>>  
>>  	return tegra_powergate_set(id, false);
>> @@ -240,7 +243,7 @@ int tegra_powergate_is_powered(int id)
>>  {
>>  	u32 status;
>>  
>> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> +	if (!PMC_PWRGATE_IS_VALID(id))
>>  		return -EINVAL;
>>  
>>  	status = tegra_pmc_readl(PWRGATE_STATUS);
>> @@ -256,7 +259,7 @@ int tegra_powergate_remove_clamping(int id)
>>  {
>>  	u32 mask;
>>  
>> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
>> +	if (!PMC_PWRGATE_IS_VALID(id))
>>  		return -EINVAL;
>>  
>>  	/*
>> @@ -1084,7 +1087,7 @@ static int __init tegra_pmc_early_init(void)
>>  	struct device_node *np;
>>  	struct resource regs;
>>  	bool invert;
>> -	u32 value;
>> +	u32 value, i;
> 
> I'd prefer an unsized type (unsigned int) for i because there is no
> reason why it should be sized.

Ok.

Jon