Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
From: Vince Hsu
Date: Sun Dec 28 2014 - 21:49:29 EST
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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/