| 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: Thu, 25 Dec 2014 10:28:08 +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/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?
>
> 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,
>
--
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