Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp

[Vince Hsu <vinceh@nvidia.com>]

On 12/24/2014 09:16 PM, Lucas Stach wrote:
> Am Dienstag, den 23.12.2014, 18:39 +0800 schrieb Vince Hsu:
>> The Tegra124 and later Tegra SoCs have a sepatate rail gating register
>> to enable/disable the clamp. The original function
>> tegra_powergate_remove_clamping() is not sufficient for the enable
>> function. So add a new function which is dedicated to the GPU rail
>> gating. Also don't refer to the powergate ID since the GPU ID makes no
>> sense here.
>>
>> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
> To be honest I don't see the point of this patch.
> You are bloating the PMC interface by introducing another exported
> function that does nothing different than what the current function
> already does.
>
> If you need a way to assert the clamp I would have expected you to
> introduce a common function to do this for all power partitions.
I thought about adding an tegra_powergate_assert_clamping(), but that 
doesn't make sense to all the power partitions except GPU. Note the 
difference in TRM. Any suggestion for the common function?
>
> Other comments inline.
>
> Regards,
> Lucas
>
>> ---
>>   drivers/soc/tegra/pmc.c | 34 +++++++++++++++++++++++-----------
>>   include/soc/tegra/pmc.h |  2 ++
>>   2 files changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index a2c0ceb95f8f..7798c530ead1 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -225,17 +225,6 @@ int tegra_powergate_remove_clamping(int id)
>>   		return -EINVAL;
>>   
>>   	/*
>> -	 * The Tegra124 GPU has a separate register (with different semantics)
>> -	 * to remove clamps.
>> -	 */
>> -	if (tegra_get_chip_id() == TEGRA124) {
>> -		if (id == TEGRA_POWERGATE_3D) {
>> -			tegra_pmc_writel(0, GPU_RG_CNTRL);
>> -			return 0;
>> -		}
>> -	}
>> -
>> -	/*
>>   	 * Tegra 2 has a bug where PCIE and VDE clamping masks are
>>   	 * swapped relatively to the partition ids
>>   	 */
>> @@ -253,6 +242,29 @@ int tegra_powergate_remove_clamping(int id)
>>   EXPORT_SYMBOL(tegra_powergate_remove_clamping);
>>   
>>   /**
>> + * tegra_powergate_gpu_set_clamping - control GPU-SOC clamps
>> + *
>> + * The post-Tegra114 chips have a separate rail gating register to configure
>> + * clamps.
>> + *
>> + * @assert: true to assert clamp, and false to remove
>> + */
>> +int tegra_powergate_gpu_set_clamping(bool assert)
> Those functions with a bool parameter to set/unset something are really
> annoying. Please avoid this pattern. The naming of the original function
> is much more intuitive.
But the original function is not sufficient. Maybe add a 
tegra_powergate_assert_gpu_clamping()? That way I will prefer to adding 
one more removal function for GPU. And then again that's a bloat, too.
>
>> +{
>> +	if (!pmc->soc)
>> +		return -EINVAL;
>> +
>> +	if (tegra_get_chip_id() == TEGRA124) {
>> +		tegra_pmc_writel(assert ? 1 : 0, GPU_RG_CNTRL);
>> +		tegra_pmc_readl(GPU_RG_CNTRL);
> You are reading the register back here, which to me seems like you are
> trying to make sure that the write is flushed. Why is this needed?
> I also observed the need to do this while working on Tegra124 PCIe in
> Barebox, otherwise the partition wouldn't power up. I didn't have time
> to follow up on this yet, so it would be nice if you could explain why
> this is needed, or if you don't know ask HW about it.
That's a read fence to assure the post of the previous writes through 
Tegra interconnect. (copy-paster from 
https://android.googlesource.com/kernel/tegra.git/+/28b107dcb3aa122de8e94e48af548140d519298f)
>
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(tegra_powergate_gpu_set_clamping);
>> +
>> +/**
>>    * tegra_powergate_sequence_power_up() - power up partition
>>    * @id: partition ID
>>    * @clk: clock for partition
>> diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
>> index 65a93273e72f..53d620525a9e 100644
>> --- a/include/soc/tegra/pmc.h
>> +++ b/include/soc/tegra/pmc.h
>> @@ -109,6 +109,8 @@ int tegra_powergate_is_powered(int id);
>>   int tegra_powergate_power_on(int id);
>>   int tegra_powergate_power_off(int id);
>>   int tegra_powergate_remove_clamping(int id);
>> +/* Only for Tegra124 and later */
>> +int tegra_powergate_gpu_set_clamping(bool assert);
>>   
>>   /* Must be called with clk disabled, and returns with clk enabled */
>>   int tegra_powergate_sequence_power_up(int id, struct clk *clk,
>