lkml.org 
[lkml]   [2014]   [Dec]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp

On 12/26/2014 04:34 AM, Lucas Stach wrote:
> Am Donnerstag, den 25.12.2014, 10:28 +0800 schrieb Vince Hsu:
>> 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?
> Is there anything speaking against adding this function and only accept
> the GPU partition as valid parameter for now. So at least the interface
> stays symmetric and can be easily extended if any future partitions have
> similar characteristics as the GPU one.
The register APBDEV_PMC_GPU_RG_CNTRL_0 is only for GPU and can be used
for assertion and deassertion. The APBDEV_PMC_REMOVE_CLAMPING_CMD_0 is
only used for deassertion. If we have any future partitions that can be
asserted by SW like GPU, we can improve the interface then.

>
>>> 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)
> I see what it does, the question is more about why this is needed.
> What is the Tegra interconnect? According to the TRM the Tegra contains
> some standard AXI <-> AHB <-> APB bridges. That a read is needed to
> assure the write is posted to the APB bus seems to imply that there is
> some write buffering in one of those bridges. Can we get this documented
> somewhere?
The TRM does mention a read after the write. Check the section 32.2.2.3.

Thanks,
Vince

>
> And isn't it needed for the other partition ungating function too then?
I believe yes.

>
> Regards,
> Lucas
>
>



\
 
 \ /
  Last update: 2014-12-29 04:21    [W:0.160 / U:1.316 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site