| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 29 Dec 2014 10:49:44 +0800
From: Vince Hsu <vinceh@...dia.com>
To: Lucas Stach <dev@...xeye.de>
CC: <thierry.reding@...il.com>, <swarren@...dotorg.org>,
<gnurou@...il.com>, <bskeggs@...hat.com>, <martin.peres@...e.fr>,
<seven@...rod-online.com>, <samuel.pitoiset@...il.com>,
<nouveau@...ts.freedesktop.org>, <linux-tegra@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [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@...dia.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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists