Lucas Stach
2014-Dec-24 13:16 UTC
[Nouveau] [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
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 at 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. 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.> +{ > + 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.> + 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,
Vince Hsu
2014-Dec-25 02:28 UTC
[Nouveau] [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 at 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 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, >
Lucas Stach
2014-Dec-25 20:34 UTC
[Nouveau] [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
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 at 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.> > > > 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 > 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? And isn't it needed for the other partition ungating function too then? Regards, Lucas
Thierry Reding
2015-Jan-05 15:09 UTC
[Nouveau] [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Thu, Dec 25, 2014 at 10:28:08AM +0800, Vince Hsu wrote:> 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 at 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?I don't think extending the powergate API is useful at this point. We've long had an open TODO item to replace this with a generic API. I did some prototyping a while ago to use generic power domains for this, that way all the details and dependencies between the partitions could be properly modeled. Can you take a look at my staging/powergate branch here: github.com/thierryreding/linux/commits/staging/powergate and see if you can use that instead? The idea is to completely hide the details of power partitions from drivers and use runtime PM instead. Also adding Peter whom I had discussed this with earlier. Can we finally get this converted? I'd rather not keep complicating this custom API to avoid making the conversion even more difficult. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <lists.freedesktop.org/archives/nouveau/attachments/20150105/fae76020/attachment.sig>
Possibly Parallel Threads
- [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
- [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
- [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
- [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
- [PATCH 0/11] Add suspend/resume support for GK20A