Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Thu, Jan 08, 2015 at 12:41:54PM +0100, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Thu, Jan 08, 2015 at 11:32:06AM +0200, Peter De Schrijver wrote: > > > > And specify the dependencies between domains in DT? > > > > > > I think the dependencies could be in the driver. Of course the power > > > domains are per-SoC data, so really shouldn't be in the DTS either (the > > > data is all implied by the compatible value) but there's no good way to > > > get at the clocks and resets without DT, so I think that's a reasonable > > > trade-off. > > > > > > It seems to me like there are only two dependencies: > > > > > > DIS and DISB depend on SOR > > > VE depends on DIS > > > > > > That's according to 5.6.6 "Programming Guide for Power Gating and > > > Ungating" of the Tegra K1 TRM. It also seems like a bunch of modules are > > > part of seemingly unrelated domains. Especially SOR seems to cover a > > > large range of modules (MIPI-CAL, DPAUX, SOR, HDMI, DSI, DSIB and > > > HDA2HDMI). > > > > > > Given that we may want to more fine-grainedly control clocks to save > > > power, don't we need to control clocks and resets within the drivers? I > > > think the runtime PM framework makes sure to call this in the right > > > order, so for suspend, the sequence would be: > > > > > > 1) device->suspend > > > 2) domain->suspend > > > > > > and for resume: > > > > > > 1) domain->resume > > > 2) device->resume > > > > > > But then we're back to square one, namely that the MC flush doesn't work > > > properly, since it needs to be implemented in domain->suspend. Does that > > > mean we can't clock-gate modules? In order to ensure a proper powergate > > > sequence, the domain code would need to clk_enable() the module clock to > > > make sure it stays on during the reset sequence. But if the domain code > > > has a reference to the clock, then the driver can't clock-gate the > > > module anymore by calling clk_disable(). > > > > > > Am I missing something? > > > > > > > There's a difference between having a reference and actually enabling the > > clock. > > My point was that as long as anyone was keeping a reference the clock > couldn't in fact be turned off. > > > the domain powergate method will only be called when the clocks of > > all modules in the domain are off. > > No, the power domain will be disabled when all devices in the domain are > idle. > > > So the powergate method can then turn them on again, do the module > > resets and client flushes and then disable them again. Same for > > ungate. So I don't see a problem here? > > I think that could work, but we'd need to make sure that drivers that > use runtime PM and are connected to a power domain enable clocks only > after taking a runtime PM reference and disable the clocks before they > release that reference. > > So to simplify things, maybe all clock handling for drivers should be > moved into runtime PM operations, and whenever the driver needs the > clock enabled it takes a runtime PM reference. That would ensure that > clocks aren't accidentally left on. Yes. That's exactly what needs to happen (apart from some special cases where the driver also controls some clocks for the external signals like display. those clocks most likely still need to be handled by the drivers). Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Thu, Jan 08, 2015 at 11:39:57AM +0200, Peter De Schrijver wrote: > > > And specify the dependencies between domains in DT? > > > > I think the dependencies could be in the driver. Of course the power > > domains are per-SoC data, so really shouldn't be in the DTS either (the > > data is all implied by the compatible value) but there's no good way to > > The clock references could also be retrieved via clk_get_sys(). We could add > some more clkdev entries. If we use the domain name as the dev_id and the > module names as the con_id's, the domain code could then retrieve the > clocks by iterating over the module names and performing a > clk_get_sys(domain_name, module_name) for each module. Unfortunately no such > mechanism exists for resets. I don't think having both clock and reset references in the device tree is all that bad. We could possibly add a lookup mechanism for reset controls that doesn't rely on DT, but I'm not sure it's really worth it. Thierry pgpWzn6oL_Cb3.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Thu, Jan 08, 2015 at 11:32:06AM +0200, Peter De Schrijver wrote: > > > And specify the dependencies between domains in DT? > > > > I think the dependencies could be in the driver. Of course the power > > domains are per-SoC data, so really shouldn't be in the DTS either (the > > data is all implied by the compatible value) but there's no good way to > > get at the clocks and resets without DT, so I think that's a reasonable > > trade-off. > > > > It seems to me like there are only two dependencies: > > > > DIS and DISB depend on SOR > > VE depends on DIS > > > > That's according to 5.6.6 "Programming Guide for Power Gating and > > Ungating" of the Tegra K1 TRM. It also seems like a bunch of modules are > > part of seemingly unrelated domains. Especially SOR seems to cover a > > large range of modules (MIPI-CAL, DPAUX, SOR, HDMI, DSI, DSIB and > > HDA2HDMI). > > > > Given that we may want to more fine-grainedly control clocks to save > > power, don't we need to control clocks and resets within the drivers? I > > think the runtime PM framework makes sure to call this in the right > > order, so for suspend, the sequence would be: > > > > 1) device->suspend > > 2) domain->suspend > > > > and for resume: > > > > 1) domain->resume > > 2) device->resume > > > > But then we're back to square one, namely that the MC flush doesn't work > > properly, since it needs to be implemented in domain->suspend. Does that > > mean we can't clock-gate modules? In order to ensure a proper powergate > > sequence, the domain code would need to clk_enable() the module clock to > > make sure it stays on during the reset sequence. But if the domain code > > has a reference to the clock, then the driver can't clock-gate the > > module anymore by calling clk_disable(). > > > > Am I missing something? > > > > There's a difference between having a reference and actually enabling the > clock. My point was that as long as anyone was keeping a reference the clock couldn't in fact be turned off. > the domain powergate method will only be called when the clocks of > all modules in the domain are off. No, the power domain will be disabled when all devices in the domain are idle. > So the powergate method can then turn them on again, do the module > resets and client flushes and then disable them again. Same for > ungate. So I don't see a problem here? I think that could work, but we'd need to make sure that drivers that use runtime PM and are connected to a power domain enable clocks only after taking a runtime PM reference and disable the clocks before they release that reference. So to simplify things, maybe all clock handling for drivers should be moved into runtime PM operations, and whenever the driver needs the clock enabled it takes a runtime PM reference. That would ensure that clocks aren't accidentally left on. Thierry pgpN8DHQSayzi.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
> > And specify the dependencies between domains in DT? > > I think the dependencies could be in the driver. Of course the power > domains are per-SoC data, so really shouldn't be in the DTS either (the > data is all implied by the compatible value) but there's no good way to The clock references could also be retrieved via clk_get_sys(). We could add some more clkdev entries. If we use the domain name as the dev_id and the module names as the con_id's, the domain code could then retrieve the clocks by iterating over the module names and performing a clk_get_sys(domain_name, module_name) for each module. Unfortunately no such mechanism exists for resets. Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
> > And specify the dependencies between domains in DT? > > I think the dependencies could be in the driver. Of course the power > domains are per-SoC data, so really shouldn't be in the DTS either (the > data is all implied by the compatible value) but there's no good way to > get at the clocks and resets without DT, so I think that's a reasonable > trade-off. > > It seems to me like there are only two dependencies: > > DIS and DISB depend on SOR > VE depends on DIS > > That's according to 5.6.6 "Programming Guide for Power Gating and > Ungating" of the Tegra K1 TRM. It also seems like a bunch of modules are > part of seemingly unrelated domains. Especially SOR seems to cover a > large range of modules (MIPI-CAL, DPAUX, SOR, HDMI, DSI, DSIB and > HDA2HDMI). > > Given that we may want to more fine-grainedly control clocks to save > power, don't we need to control clocks and resets within the drivers? I > think the runtime PM framework makes sure to call this in the right > order, so for suspend, the sequence would be: > > 1) device->suspend > 2) domain->suspend > > and for resume: > > 1) domain->resume > 2) device->resume > > But then we're back to square one, namely that the MC flush doesn't work > properly, since it needs to be implemented in domain->suspend. Does that > mean we can't clock-gate modules? In order to ensure a proper powergate > sequence, the domain code would need to clk_enable() the module clock to > make sure it stays on during the reset sequence. But if the domain code > has a reference to the clock, then the driver can't clock-gate the > module anymore by calling clk_disable(). > > Am I missing something? > There's a difference between having a reference and actually enabling the clock. the domain powergate method will only be called when the clocks of all modules in the domain are off. So the powergate method can then turn them on again, do the module resets and client flushes and then disable them again. Same for ungate. So I don't see a problem here? Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Thu, Jan 08, 2015 at 12:25:18PM +0800, Vince Hsu wrote: > > On 01/07/2015 10:48 PM, Thierry Reding wrote: > >* PGP Signed by an unknown key > > > >On Wed, Jan 07, 2015 at 10:28:29PM +0800, Vince Hsu wrote: > >>On 04:08:52PM Jan 07, Peter De Schrijver wrote: > >>>On Wed, Jan 07, 2015 at 02:27:10PM +0100, Thierry Reding wrote: > >>> > >Yeah. I plan to have the information of all the clock client of the > >partitions and > >the memory clients be defined statically in c source, e.g. > >pmc-tegra124.c. > >All modules can declare which domain they belong to in DT. One domain can > >be really power gated only when no module is awake. Note the clock > >clients > >of > >one domain might not equal to the clocks of the module. The reset is not > >either. > >So I don't get the clock and reset from module. How do you think? > This whole situation is quite messy. The above sequence basically means > that drivers can't reset hardware modules because otherwise they might > race with the power domain code. It also means that we can't powergate > >>>The powerdomain framework won't call any powergating method as long as a > >>>module in the domain is still active. So as long as drivers don't try to > >>>reset the hw without having done a pm_runtime_get(), we shouldn't have such > >>>a race? > >>Agree. And as long as the driver has the correct reset procedure, that > >>should > >>be fine to occur between power ungating and gating sequences. > >> > modules on demand because they might be in the same power domain as one > other module that's still busy. > > >>>The powerdomain framework keeps track of which modules are active (by > >>>hooking > >>>into runtime pm) and won't try to shutdown a domain unless all modules are > >>>inactive. > >>Yeah. By the way, that means we should start supporting runtime pm for all > >>the modules to use generic power domain. > >Indeed, that'll be a prerequisite before we can merge power domain > >support. I do have a couple of local patches that add very rudimentary > >runtime PM for various drivers. For starters we could probably just do > >the > > > > pm_runtime_enable(...); > > pm_runtime_get_sync(...) > > > >in the ->probe() and > > > > pm_runtime_put_sync(...); > > pm_runtime_disable(...); > > > >in the ->remove() callbacks for those drivers. That's by no means > >optimal but should get us pretty close to what we do now and still > >support the generic power domains. > Cool. Could you send me the patches? Here are two examples: https://github.com/thierryreding/linux/commit/36b5c34f68edb8135b9afb3e62c7ce9a527d6793 https://github.com/thierryreding/linux/commit/6a6145d9e0fcbd4f9599552181fc02f4606b6a0e Thierry pgpoFA8Q06epg.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Thu, Jan 08, 2015 at 11:39:57AM +0200, Peter De Schrijver wrote: And specify the dependencies between domains in DT? I think the dependencies could be in the driver. Of course the power domains are per-SoC data, so really shouldn't be in the DTS either (the data is all implied by the compatible value) but there's no good way to The clock references could also be retrieved via clk_get_sys(). We could add some more clkdev entries. If we use the domain name as the dev_id and the module names as the con_id's, the domain code could then retrieve the clocks by iterating over the module names and performing a clk_get_sys(domain_name, module_name) for each module. Unfortunately no such mechanism exists for resets. I don't think having both clock and reset references in the device tree is all that bad. We could possibly add a lookup mechanism for reset controls that doesn't rely on DT, but I'm not sure it's really worth it. Thierry pgpWzn6oL_Cb3.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Thu, Jan 08, 2015 at 11:32:06AM +0200, Peter De Schrijver wrote: And specify the dependencies between domains in DT? I think the dependencies could be in the driver. Of course the power domains are per-SoC data, so really shouldn't be in the DTS either (the data is all implied by the compatible value) but there's no good way to get at the clocks and resets without DT, so I think that's a reasonable trade-off. It seems to me like there are only two dependencies: DIS and DISB depend on SOR VE depends on DIS That's according to 5.6.6 Programming Guide for Power Gating and Ungating of the Tegra K1 TRM. It also seems like a bunch of modules are part of seemingly unrelated domains. Especially SOR seems to cover a large range of modules (MIPI-CAL, DPAUX, SOR, HDMI, DSI, DSIB and HDA2HDMI). Given that we may want to more fine-grainedly control clocks to save power, don't we need to control clocks and resets within the drivers? I think the runtime PM framework makes sure to call this in the right order, so for suspend, the sequence would be: 1) device-suspend 2) domain-suspend and for resume: 1) domain-resume 2) device-resume But then we're back to square one, namely that the MC flush doesn't work properly, since it needs to be implemented in domain-suspend. Does that mean we can't clock-gate modules? In order to ensure a proper powergate sequence, the domain code would need to clk_enable() the module clock to make sure it stays on during the reset sequence. But if the domain code has a reference to the clock, then the driver can't clock-gate the module anymore by calling clk_disable(). Am I missing something? There's a difference between having a reference and actually enabling the clock. My point was that as long as anyone was keeping a reference the clock couldn't in fact be turned off. the domain powergate method will only be called when the clocks of all modules in the domain are off. No, the power domain will be disabled when all devices in the domain are idle. So the powergate method can then turn them on again, do the module resets and client flushes and then disable them again. Same for ungate. So I don't see a problem here? I think that could work, but we'd need to make sure that drivers that use runtime PM and are connected to a power domain enable clocks only after taking a runtime PM reference and disable the clocks before they release that reference. So to simplify things, maybe all clock handling for drivers should be moved into runtime PM operations, and whenever the driver needs the clock enabled it takes a runtime PM reference. That would ensure that clocks aren't accidentally left on. Thierry pgpN8DHQSayzi.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Thu, Jan 08, 2015 at 12:41:54PM +0100, Thierry Reding wrote: * PGP Signed by an unknown key On Thu, Jan 08, 2015 at 11:32:06AM +0200, Peter De Schrijver wrote: And specify the dependencies between domains in DT? I think the dependencies could be in the driver. Of course the power domains are per-SoC data, so really shouldn't be in the DTS either (the data is all implied by the compatible value) but there's no good way to get at the clocks and resets without DT, so I think that's a reasonable trade-off. It seems to me like there are only two dependencies: DIS and DISB depend on SOR VE depends on DIS That's according to 5.6.6 Programming Guide for Power Gating and Ungating of the Tegra K1 TRM. It also seems like a bunch of modules are part of seemingly unrelated domains. Especially SOR seems to cover a large range of modules (MIPI-CAL, DPAUX, SOR, HDMI, DSI, DSIB and HDA2HDMI). Given that we may want to more fine-grainedly control clocks to save power, don't we need to control clocks and resets within the drivers? I think the runtime PM framework makes sure to call this in the right order, so for suspend, the sequence would be: 1) device-suspend 2) domain-suspend and for resume: 1) domain-resume 2) device-resume But then we're back to square one, namely that the MC flush doesn't work properly, since it needs to be implemented in domain-suspend. Does that mean we can't clock-gate modules? In order to ensure a proper powergate sequence, the domain code would need to clk_enable() the module clock to make sure it stays on during the reset sequence. But if the domain code has a reference to the clock, then the driver can't clock-gate the module anymore by calling clk_disable(). Am I missing something? There's a difference between having a reference and actually enabling the clock. My point was that as long as anyone was keeping a reference the clock couldn't in fact be turned off. the domain powergate method will only be called when the clocks of all modules in the domain are off. No, the power domain will be disabled when all devices in the domain are idle. So the powergate method can then turn them on again, do the module resets and client flushes and then disable them again. Same for ungate. So I don't see a problem here? I think that could work, but we'd need to make sure that drivers that use runtime PM and are connected to a power domain enable clocks only after taking a runtime PM reference and disable the clocks before they release that reference. So to simplify things, maybe all clock handling for drivers should be moved into runtime PM operations, and whenever the driver needs the clock enabled it takes a runtime PM reference. That would ensure that clocks aren't accidentally left on. Yes. That's exactly what needs to happen (apart from some special cases where the driver also controls some clocks for the external signals like display. those clocks most likely still need to be handled by the drivers). Cheers, Peter. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Thu, Jan 08, 2015 at 12:25:18PM +0800, Vince Hsu wrote: On 01/07/2015 10:48 PM, Thierry Reding wrote: * PGP Signed by an unknown key On Wed, Jan 07, 2015 at 10:28:29PM +0800, Vince Hsu wrote: On 04:08:52PM Jan 07, Peter De Schrijver wrote: On Wed, Jan 07, 2015 at 02:27:10PM +0100, Thierry Reding wrote: Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? This whole situation is quite messy. The above sequence basically means that drivers can't reset hardware modules because otherwise they might race with the power domain code. It also means that we can't powergate The powerdomain framework won't call any powergating method as long as a module in the domain is still active. So as long as drivers don't try to reset the hw without having done a pm_runtime_get(), we shouldn't have such a race? Agree. And as long as the driver has the correct reset procedure, that should be fine to occur between power ungating and gating sequences. modules on demand because they might be in the same power domain as one other module that's still busy. The powerdomain framework keeps track of which modules are active (by hooking into runtime pm) and won't try to shutdown a domain unless all modules are inactive. Yeah. By the way, that means we should start supporting runtime pm for all the modules to use generic power domain. Indeed, that'll be a prerequisite before we can merge power domain support. I do have a couple of local patches that add very rudimentary runtime PM for various drivers. For starters we could probably just do the pm_runtime_enable(...); pm_runtime_get_sync(...) in the -probe() and pm_runtime_put_sync(...); pm_runtime_disable(...); in the -remove() callbacks for those drivers. That's by no means optimal but should get us pretty close to what we do now and still support the generic power domains. Cool. Could you send me the patches? Here are two examples: https://github.com/thierryreding/linux/commit/36b5c34f68edb8135b9afb3e62c7ce9a527d6793 https://github.com/thierryreding/linux/commit/6a6145d9e0fcbd4f9599552181fc02f4606b6a0e Thierry pgpoFA8Q06epg.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
And specify the dependencies between domains in DT? I think the dependencies could be in the driver. Of course the power domains are per-SoC data, so really shouldn't be in the DTS either (the data is all implied by the compatible value) but there's no good way to get at the clocks and resets without DT, so I think that's a reasonable trade-off. It seems to me like there are only two dependencies: DIS and DISB depend on SOR VE depends on DIS That's according to 5.6.6 Programming Guide for Power Gating and Ungating of the Tegra K1 TRM. It also seems like a bunch of modules are part of seemingly unrelated domains. Especially SOR seems to cover a large range of modules (MIPI-CAL, DPAUX, SOR, HDMI, DSI, DSIB and HDA2HDMI). Given that we may want to more fine-grainedly control clocks to save power, don't we need to control clocks and resets within the drivers? I think the runtime PM framework makes sure to call this in the right order, so for suspend, the sequence would be: 1) device-suspend 2) domain-suspend and for resume: 1) domain-resume 2) device-resume But then we're back to square one, namely that the MC flush doesn't work properly, since it needs to be implemented in domain-suspend. Does that mean we can't clock-gate modules? In order to ensure a proper powergate sequence, the domain code would need to clk_enable() the module clock to make sure it stays on during the reset sequence. But if the domain code has a reference to the clock, then the driver can't clock-gate the module anymore by calling clk_disable(). Am I missing something? There's a difference between having a reference and actually enabling the clock. the domain powergate method will only be called when the clocks of all modules in the domain are off. So the powergate method can then turn them on again, do the module resets and client flushes and then disable them again. Same for ungate. So I don't see a problem here? Cheers, Peter. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
And specify the dependencies between domains in DT? I think the dependencies could be in the driver. Of course the power domains are per-SoC data, so really shouldn't be in the DTS either (the data is all implied by the compatible value) but there's no good way to The clock references could also be retrieved via clk_get_sys(). We could add some more clkdev entries. If we use the domain name as the dev_id and the module names as the con_id's, the domain code could then retrieve the clocks by iterating over the module names and performing a clk_get_sys(domain_name, module_name) for each module. Unfortunately no such mechanism exists for resets. Cheers, Peter. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 01/07/2015 10:48 PM, Thierry Reding wrote: * PGP Signed by an unknown key On Wed, Jan 07, 2015 at 10:28:29PM +0800, Vince Hsu wrote: On 04:08:52PM Jan 07, Peter De Schrijver wrote: On Wed, Jan 07, 2015 at 02:27:10PM +0100, Thierry Reding wrote: Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? This whole situation is quite messy. The above sequence basically means that drivers can't reset hardware modules because otherwise they might race with the power domain code. It also means that we can't powergate The powerdomain framework won't call any powergating method as long as a module in the domain is still active. So as long as drivers don't try to reset the hw without having done a pm_runtime_get(), we shouldn't have such a race? Agree. And as long as the driver has the correct reset procedure, that should be fine to occur between power ungating and gating sequences. modules on demand because they might be in the same power domain as one other module that's still busy. The powerdomain framework keeps track of which modules are active (by hooking into runtime pm) and won't try to shutdown a domain unless all modules are inactive. Yeah. By the way, that means we should start supporting runtime pm for all the modules to use generic power domain. Indeed, that'll be a prerequisite before we can merge power domain support. I do have a couple of local patches that add very rudimentary runtime PM for various drivers. For starters we could probably just do the pm_runtime_enable(...); pm_runtime_get_sync(...) in the ->probe() and pm_runtime_put_sync(...); pm_runtime_disable(...); in the ->remove() callbacks for those drivers. That's by no means optimal but should get us pretty close to what we do now and still support the generic power domains. Cool. Could you send me the patches? Thanks, Vince Thierry * Unknown Key * 0x7F3EB3A1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 01/07/2015 11:12 PM, Thierry Reding wrote: * PGP Signed by an unknown key On Wed, Jan 07, 2015 at 10:19:52PM +0800, Vince Hsu wrote: On 04:12:54PM Jan 07, Peter De Schrijver wrote: On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu wrote: On 01/07/2015 06:19 PM, Peter De Schrijver wrote: On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: Old Signed by an unknown key 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 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: https://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. Conceptually I fully agree that we should use runtime PM and powerdomains. However I don't think the implementation you mentioned is correct. The resets of all modules in a domain need to be asserted and the memory clients need to be flushed. All this needs to be done with module clocks enabled (resets are synchronous). Then all module clocks need to be disabled and then the partition can be powergated. After ungating, the module resets need to be deasserted and the FLUSH bit cleared with clocks enabled. Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? I think it's indeed better to have a direct reference to the required clocks to powergate/ungate a domain. As you said, there is no easy way to derive the required clocks from the DT module declarations. My suggestion would be to have powerdomain definitions in DT and for each domain have references to the required clocks and resets. And specify the dependencies between domains in DT? I think the dependencies could be in the driver. Of course the power domains are per-SoC data, so really shouldn't be in the DTS either (the data is all implied by the compatible value) but there's no good way to get at the clocks and resets without DT, so I think that's a reasonable trade-off. It seems to me like there are only two dependencies: DIS and DISB depend on SOR VE depends on DIS That's according to 5.6.6 "Programming Guide for Power Gating and Ungating" of the Tegra K1 TRM. It also seems like a bunch of modules are part of seemingly unrelated domains. Especially SOR seems to cover a large range of modules (MIPI-CAL, DPAUX, SOR, HDMI, DSI, DSIB and HDA2HDMI). Given that we may want to more fine-grainedly control clocks to save power, don't we need to control clocks and resets within the drivers? I think the runtime PM framework makes sure to call this in the right order, so for suspend, the sequence would be: We need to control clocks and resets within the drivers. I believe the powergate sequence is just to provide a clean hardware state. The driver can do whatever it wants to the clocks and reset as long as that's correct procedure. 1) device->suspend 2) domain->suspend and for resume: 1) domain->resume 2) device->resume But then we're back to square one, namely that the MC flush doesn't work properly, since it
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Wed, Jan 07, 2015 at 10:19:52PM +0800, Vince Hsu wrote: > On 04:12:54PM Jan 07, Peter De Schrijver wrote: > > On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu wrote: > > > > > > On 01/07/2015 06:19 PM, Peter De Schrijver wrote: > > > >On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: > > > >>* PGP Signed by an unknown key > > > >> > > > >>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 > > > 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: > > > >> > > > >>https://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. > > > >Conceptually I fully agree that we should use runtime PM and > > > >powerdomains. > > > >However I don't think the implementation you mentioned is correct. The > > > >resets > > > >of all modules in a domain need to be asserted and the memory clients > > > >need to > > > >be flushed. All this needs to be done with module clocks enabled (resets > > > >are > > > >synchronous). Then all module clocks need to be disabled and then the > > > >partition can be powergated. After ungating, the module resets need to be > > > >deasserted and the FLUSH bit cleared with clocks enabled. > > > Yeah. I plan to have the information of all the clock client of the > > > partitions and > > > the memory clients be defined statically in c source, e.g. pmc-tegra124.c. > > > All modules can declare which domain they belong to in DT. One domain can > > > be really power gated only when no module is awake. Note the clock > > > clients of > > > one domain might not equal to the clocks of the module. The reset is > > > not either. > > > So I don't get the clock and reset from module. How do you think? > > > > > > > I think it's indeed better to have a direct reference to the required clocks > > to powergate/ungate a domain. As you said, there is no easy way to derive > > the > > required clocks from the DT module declarations. My suggestion would be to > > have powerdomain definitions in DT and for each domain have references to > > the required clocks and resets. > > > And specify the dependencies between domains in DT? I think the dependencies could be in the driver. Of course the power domains are per-SoC data, so really shouldn't be in the DTS either (the data is all implied by the compatible value) but there's no good way to get at the clocks and resets without DT, so I think that's a reasonable trade-off. It seems to me like there are only two dependencies: DIS and DISB depend on SOR VE depends on DIS That's according to 5.6.6 "Programming Guide for Power Gating and Ungating" of the Tegra K1 TRM. It also seems like a bunch of modules are part of seemingly unrelated domains. Especially SOR seems to cover a large range of modules (MIPI-CAL, DPAUX, SOR, HDMI, DSI, DSIB and HDA2HDMI). Given that we may want to more fine-grainedly control clocks to save power, don't we need to control clocks and resets within the drivers? I think the runtime PM framework makes sure to call this in the right order,
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Wed, Jan 07, 2015 at 10:28:29PM +0800, Vince Hsu wrote: > On 04:08:52PM Jan 07, Peter De Schrijver wrote: > > On Wed, Jan 07, 2015 at 02:27:10PM +0100, Thierry Reding wrote: > > > > > > Yeah. I plan to have the information of all the clock client of the > > > > partitions and > > > > the memory clients be defined statically in c source, e.g. > > > > pmc-tegra124.c. > > > > All modules can declare which domain they belong to in DT. One domain > > > > can > > > > be really power gated only when no module is awake. Note the clock > > > > clients > > > > of > > > > one domain might not equal to the clocks of the module. The reset is not > > > > either. > > > > So I don't get the clock and reset from module. How do you think? > > > > > > This whole situation is quite messy. The above sequence basically means > > > that drivers can't reset hardware modules because otherwise they might > > > race with the power domain code. It also means that we can't powergate > > > > The powerdomain framework won't call any powergating method as long as a > > module in the domain is still active. So as long as drivers don't try to > > reset the hw without having done a pm_runtime_get(), we shouldn't have such > > a race? > Agree. And as long as the driver has the correct reset procedure, that should > be fine to occur between power ungating and gating sequences. > > > > > > modules on demand because they might be in the same power domain as one > > > other module that's still busy. > > > > > > > The powerdomain framework keeps track of which modules are active (by > > hooking > > into runtime pm) and won't try to shutdown a domain unless all modules are > > inactive. > Yeah. By the way, that means we should start supporting runtime pm for all > the modules to use generic power domain. Indeed, that'll be a prerequisite before we can merge power domain support. I do have a couple of local patches that add very rudimentary runtime PM for various drivers. For starters we could probably just do the pm_runtime_enable(...); pm_runtime_get_sync(...) in the ->probe() and pm_runtime_put_sync(...); pm_runtime_disable(...); in the ->remove() callbacks for those drivers. That's by no means optimal but should get us pretty close to what we do now and still support the generic power domains. Thierry pgpnnTlxmDBc0.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 04:08:52PM Jan 07, Peter De Schrijver wrote: > On Wed, Jan 07, 2015 at 02:27:10PM +0100, Thierry Reding wrote: > > > > Yeah. I plan to have the information of all the clock client of the > > > partitions and > > > the memory clients be defined statically in c source, e.g. pmc-tegra124.c. > > > All modules can declare which domain they belong to in DT. One domain can > > > be really power gated only when no module is awake. Note the clock clients > > > of > > > one domain might not equal to the clocks of the module. The reset is not > > > either. > > > So I don't get the clock and reset from module. How do you think? > > > > This whole situation is quite messy. The above sequence basically means > > that drivers can't reset hardware modules because otherwise they might > > race with the power domain code. It also means that we can't powergate > > The powerdomain framework won't call any powergating method as long as a > module in the domain is still active. So as long as drivers don't try to > reset the hw without having done a pm_runtime_get(), we shouldn't have such > a race? Agree. And as long as the driver has the correct reset procedure, that should be fine to occur between power ungating and gating sequences. > > > modules on demand because they might be in the same power domain as one > > other module that's still busy. > > > > The powerdomain framework keeps track of which modules are active (by hooking > into runtime pm) and won't try to shutdown a domain unless all modules are > inactive. Yeah. By the way, that means we should start supporting runtime pm for all the modules to use generic power domain. Thanks, Vince > > > How would we handle a situation where a hardware module hangs and we can > > only get it back via a reset? > > > > Cheers, > > Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 04:12:54PM Jan 07, Peter De Schrijver wrote: > On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu wrote: > > > > On 01/07/2015 06:19 PM, Peter De Schrijver wrote: > > >On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: > > >>* PGP Signed by an unknown key > > >> > > >>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 > > 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: > > >> > > >> https://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. > > >Conceptually I fully agree that we should use runtime PM and powerdomains. > > >However I don't think the implementation you mentioned is correct. The > > >resets > > >of all modules in a domain need to be asserted and the memory clients need > > >to > > >be flushed. All this needs to be done with module clocks enabled (resets > > >are > > >synchronous). Then all module clocks need to be disabled and then the > > >partition can be powergated. After ungating, the module resets need to be > > >deasserted and the FLUSH bit cleared with clocks enabled. > > Yeah. I plan to have the information of all the clock client of the > > partitions and > > the memory clients be defined statically in c source, e.g. pmc-tegra124.c. > > All modules can declare which domain they belong to in DT. One domain can > > be really power gated only when no module is awake. Note the clock > > clients of > > one domain might not equal to the clocks of the module. The reset is > > not either. > > So I don't get the clock and reset from module. How do you think? > > > > I think it's indeed better to have a direct reference to the required clocks > to powergate/ungate a domain. As you said, there is no easy way to derive the > required clocks from the DT module declarations. My suggestion would be to > have powerdomain definitions in DT and for each domain have references to > the required clocks and resets. > And specify the dependencies between domains in DT? Thanks, Vince -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu wrote: > > On 01/07/2015 06:19 PM, Peter De Schrijver wrote: > >On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: > >>* PGP Signed by an unknown key > >> > >>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 > 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: > >> > >>https://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. > >Conceptually I fully agree that we should use runtime PM and powerdomains. > >However I don't think the implementation you mentioned is correct. The resets > >of all modules in a domain need to be asserted and the memory clients need to > >be flushed. All this needs to be done with module clocks enabled (resets are > >synchronous). Then all module clocks need to be disabled and then the > >partition can be powergated. After ungating, the module resets need to be > >deasserted and the FLUSH bit cleared with clocks enabled. > Yeah. I plan to have the information of all the clock client of the > partitions and > the memory clients be defined statically in c source, e.g. pmc-tegra124.c. > All modules can declare which domain they belong to in DT. One domain can > be really power gated only when no module is awake. Note the clock > clients of > one domain might not equal to the clocks of the module. The reset is > not either. > So I don't get the clock and reset from module. How do you think? > I think it's indeed better to have a direct reference to the required clocks to powergate/ungate a domain. As you said, there is no easy way to derive the required clocks from the DT module declarations. My suggestion would be to have powerdomain definitions in DT and for each domain have references to the required clocks and resets. Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Wed, Jan 07, 2015 at 02:27:10PM +0100, Thierry Reding wrote: > > Yeah. I plan to have the information of all the clock client of the > > partitions and > > the memory clients be defined statically in c source, e.g. pmc-tegra124.c. > > All modules can declare which domain they belong to in DT. One domain can > > be really power gated only when no module is awake. Note the clock clients > > of > > one domain might not equal to the clocks of the module. The reset is not > > either. > > So I don't get the clock and reset from module. How do you think? > > This whole situation is quite messy. The above sequence basically means > that drivers can't reset hardware modules because otherwise they might > race with the power domain code. It also means that we can't powergate The powerdomain framework won't call any powergating method as long as a module in the domain is still active. So as long as drivers don't try to reset the hw without having done a pm_runtime_get(), we shouldn't have such a race? > modules on demand because they might be in the same power domain as one > other module that's still busy. > The powerdomain framework keeps track of which modules are active (by hooking into runtime pm) and won't try to shutdown a domain unless all modules are inactive. > How would we handle a situation where a hardware module hangs and we can > only get it back via a reset? > Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu wrote: > > On 01/07/2015 06:19 PM, Peter De Schrijver wrote: > >On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: > >>* PGP Signed by an unknown key > >> > >>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 > 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: > >> > >>https://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. > >Conceptually I fully agree that we should use runtime PM and powerdomains. > >However I don't think the implementation you mentioned is correct. The resets > >of all modules in a domain need to be asserted and the memory clients need to > >be flushed. All this needs to be done with module clocks enabled (resets are > >synchronous). Then all module clocks need to be disabled and then the > >partition can be powergated. After ungating, the module resets need to be > >deasserted and the FLUSH bit cleared with clocks enabled. > Yeah. I plan to have the information of all the clock client of the > partitions and > the memory clients be defined statically in c source, e.g. pmc-tegra124.c. > All modules can declare which domain they belong to in DT. One domain can > be really power gated only when no module is awake. Note the clock clients > of > one domain might not equal to the clocks of the module. The reset is not > either. > So I don't get the clock and reset from module. How do you think? This whole situation is quite messy. The above sequence basically means that drivers can't reset hardware modules because otherwise they might race with the power domain code. It also means that we can't powergate modules on demand because they might be in the same power domain as one other module that's still busy. How would we handle a situation where a hardware module hangs and we can only get it back via a reset? Thierry pgpgf_1dhARft.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 01/07/2015 06:19 PM, Peter De Schrijver wrote: On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: * PGP Signed by an unknown key 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 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: https://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. Conceptually I fully agree that we should use runtime PM and powerdomains. However I don't think the implementation you mentioned is correct. The resets of all modules in a domain need to be asserted and the memory clients need to be flushed. All this needs to be done with module clocks enabled (resets are synchronous). Then all module clocks need to be disabled and then the partition can be powergated. After ungating, the module resets need to be deasserted and the FLUSH bit cleared with clocks enabled. Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? Thanks, Vince -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: > * PGP Signed by an unknown key > > 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 > > >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: > > https://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. Conceptually I fully agree that we should use runtime PM and powerdomains. However I don't think the implementation you mentioned is correct. The resets of all modules in a domain need to be asserted and the memory clients need to be flushed. All this needs to be done with module clocks enabled (resets are synchronous). Then all module clocks need to be disabled and then the partition can be powergated. After ungating, the module resets need to be deasserted and the FLUSH bit cleared with clocks enabled. Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: * PGP Signed by an unknown key 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 vin...@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: https://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. Conceptually I fully agree that we should use runtime PM and powerdomains. However I don't think the implementation you mentioned is correct. The resets of all modules in a domain need to be asserted and the memory clients need to be flushed. All this needs to be done with module clocks enabled (resets are synchronous). Then all module clocks need to be disabled and then the partition can be powergated. After ungating, the module resets need to be deasserted and the FLUSH bit cleared with clocks enabled. Cheers, Peter. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 01/07/2015 06:19 PM, Peter De Schrijver wrote: On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: * PGP Signed by an unknown key 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 vin...@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: https://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. Conceptually I fully agree that we should use runtime PM and powerdomains. However I don't think the implementation you mentioned is correct. The resets of all modules in a domain need to be asserted and the memory clients need to be flushed. All this needs to be done with module clocks enabled (resets are synchronous). Then all module clocks need to be disabled and then the partition can be powergated. After ungating, the module resets need to be deasserted and the FLUSH bit cleared with clocks enabled. Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? Thanks, Vince -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 04:12:54PM Jan 07, Peter De Schrijver wrote: On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu wrote: On 01/07/2015 06:19 PM, Peter De Schrijver wrote: On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: * PGP Signed by an unknown key 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 vin...@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: https://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. Conceptually I fully agree that we should use runtime PM and powerdomains. However I don't think the implementation you mentioned is correct. The resets of all modules in a domain need to be asserted and the memory clients need to be flushed. All this needs to be done with module clocks enabled (resets are synchronous). Then all module clocks need to be disabled and then the partition can be powergated. After ungating, the module resets need to be deasserted and the FLUSH bit cleared with clocks enabled. Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? I think it's indeed better to have a direct reference to the required clocks to powergate/ungate a domain. As you said, there is no easy way to derive the required clocks from the DT module declarations. My suggestion would be to have powerdomain definitions in DT and for each domain have references to the required clocks and resets. And specify the dependencies between domains in DT? Thanks, Vince -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu wrote: On 01/07/2015 06:19 PM, Peter De Schrijver wrote: On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: * PGP Signed by an unknown key 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 vin...@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: https://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. Conceptually I fully agree that we should use runtime PM and powerdomains. However I don't think the implementation you mentioned is correct. The resets of all modules in a domain need to be asserted and the memory clients need to be flushed. All this needs to be done with module clocks enabled (resets are synchronous). Then all module clocks need to be disabled and then the partition can be powergated. After ungating, the module resets need to be deasserted and the FLUSH bit cleared with clocks enabled. Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? This whole situation is quite messy. The above sequence basically means that drivers can't reset hardware modules because otherwise they might race with the power domain code. It also means that we can't powergate modules on demand because they might be in the same power domain as one other module that's still busy. How would we handle a situation where a hardware module hangs and we can only get it back via a reset? Thierry pgpgf_1dhARft.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Wed, Jan 07, 2015 at 10:28:29PM +0800, Vince Hsu wrote: On 04:08:52PM Jan 07, Peter De Schrijver wrote: On Wed, Jan 07, 2015 at 02:27:10PM +0100, Thierry Reding wrote: Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? This whole situation is quite messy. The above sequence basically means that drivers can't reset hardware modules because otherwise they might race with the power domain code. It also means that we can't powergate The powerdomain framework won't call any powergating method as long as a module in the domain is still active. So as long as drivers don't try to reset the hw without having done a pm_runtime_get(), we shouldn't have such a race? Agree. And as long as the driver has the correct reset procedure, that should be fine to occur between power ungating and gating sequences. modules on demand because they might be in the same power domain as one other module that's still busy. The powerdomain framework keeps track of which modules are active (by hooking into runtime pm) and won't try to shutdown a domain unless all modules are inactive. Yeah. By the way, that means we should start supporting runtime pm for all the modules to use generic power domain. Indeed, that'll be a prerequisite before we can merge power domain support. I do have a couple of local patches that add very rudimentary runtime PM for various drivers. For starters we could probably just do the pm_runtime_enable(...); pm_runtime_get_sync(...) in the -probe() and pm_runtime_put_sync(...); pm_runtime_disable(...); in the -remove() callbacks for those drivers. That's by no means optimal but should get us pretty close to what we do now and still support the generic power domains. Thierry pgpnnTlxmDBc0.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 01/07/2015 11:12 PM, Thierry Reding wrote: * PGP Signed by an unknown key On Wed, Jan 07, 2015 at 10:19:52PM +0800, Vince Hsu wrote: On 04:12:54PM Jan 07, Peter De Schrijver wrote: On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu wrote: On 01/07/2015 06:19 PM, Peter De Schrijver wrote: On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: Old Signed by an unknown key 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 vin...@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: https://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. Conceptually I fully agree that we should use runtime PM and powerdomains. However I don't think the implementation you mentioned is correct. The resets of all modules in a domain need to be asserted and the memory clients need to be flushed. All this needs to be done with module clocks enabled (resets are synchronous). Then all module clocks need to be disabled and then the partition can be powergated. After ungating, the module resets need to be deasserted and the FLUSH bit cleared with clocks enabled. Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? I think it's indeed better to have a direct reference to the required clocks to powergate/ungate a domain. As you said, there is no easy way to derive the required clocks from the DT module declarations. My suggestion would be to have powerdomain definitions in DT and for each domain have references to the required clocks and resets. And specify the dependencies between domains in DT? I think the dependencies could be in the driver. Of course the power domains are per-SoC data, so really shouldn't be in the DTS either (the data is all implied by the compatible value) but there's no good way to get at the clocks and resets without DT, so I think that's a reasonable trade-off. It seems to me like there are only two dependencies: DIS and DISB depend on SOR VE depends on DIS That's according to 5.6.6 Programming Guide for Power Gating and Ungating of the Tegra K1 TRM. It also seems like a bunch of modules are part of seemingly unrelated domains. Especially SOR seems to cover a large range of modules (MIPI-CAL, DPAUX, SOR, HDMI, DSI, DSIB and HDA2HDMI). Given that we may want to more fine-grainedly control clocks to save power, don't we need to control clocks and resets within the drivers? I think the runtime PM framework makes sure to call this in the right order, so for suspend, the sequence would be: We need to control clocks and resets within the drivers. I believe the powergate sequence is just to provide a clean hardware state. The driver can do whatever it wants to the clocks and reset as long as that's correct procedure. 1) device-suspend 2) domain-suspend and for resume: 1) domain-resume 2) device-resume But then we're back to square one, namely that the MC flush doesn't work
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 01/07/2015 10:48 PM, Thierry Reding wrote: * PGP Signed by an unknown key On Wed, Jan 07, 2015 at 10:28:29PM +0800, Vince Hsu wrote: On 04:08:52PM Jan 07, Peter De Schrijver wrote: On Wed, Jan 07, 2015 at 02:27:10PM +0100, Thierry Reding wrote: Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? This whole situation is quite messy. The above sequence basically means that drivers can't reset hardware modules because otherwise they might race with the power domain code. It also means that we can't powergate The powerdomain framework won't call any powergating method as long as a module in the domain is still active. So as long as drivers don't try to reset the hw without having done a pm_runtime_get(), we shouldn't have such a race? Agree. And as long as the driver has the correct reset procedure, that should be fine to occur between power ungating and gating sequences. modules on demand because they might be in the same power domain as one other module that's still busy. The powerdomain framework keeps track of which modules are active (by hooking into runtime pm) and won't try to shutdown a domain unless all modules are inactive. Yeah. By the way, that means we should start supporting runtime pm for all the modules to use generic power domain. Indeed, that'll be a prerequisite before we can merge power domain support. I do have a couple of local patches that add very rudimentary runtime PM for various drivers. For starters we could probably just do the pm_runtime_enable(...); pm_runtime_get_sync(...) in the -probe() and pm_runtime_put_sync(...); pm_runtime_disable(...); in the -remove() callbacks for those drivers. That's by no means optimal but should get us pretty close to what we do now and still support the generic power domains. Cool. Could you send me the patches? Thanks, Vince Thierry * Unknown Key * 0x7F3EB3A1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu wrote: On 01/07/2015 06:19 PM, Peter De Schrijver wrote: On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: * PGP Signed by an unknown key 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 vin...@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: https://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. Conceptually I fully agree that we should use runtime PM and powerdomains. However I don't think the implementation you mentioned is correct. The resets of all modules in a domain need to be asserted and the memory clients need to be flushed. All this needs to be done with module clocks enabled (resets are synchronous). Then all module clocks need to be disabled and then the partition can be powergated. After ungating, the module resets need to be deasserted and the FLUSH bit cleared with clocks enabled. Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? I think it's indeed better to have a direct reference to the required clocks to powergate/ungate a domain. As you said, there is no easy way to derive the required clocks from the DT module declarations. My suggestion would be to have powerdomain definitions in DT and for each domain have references to the required clocks and resets. Cheers, Peter. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Wed, Jan 07, 2015 at 10:19:52PM +0800, Vince Hsu wrote: On 04:12:54PM Jan 07, Peter De Schrijver wrote: On Wed, Jan 07, 2015 at 06:49:27PM +0800, Vince Hsu wrote: On 01/07/2015 06:19 PM, Peter De Schrijver wrote: On Mon, Jan 05, 2015 at 04:09:33PM +0100, Thierry Reding wrote: * PGP Signed by an unknown key 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 vin...@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: https://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. Conceptually I fully agree that we should use runtime PM and powerdomains. However I don't think the implementation you mentioned is correct. The resets of all modules in a domain need to be asserted and the memory clients need to be flushed. All this needs to be done with module clocks enabled (resets are synchronous). Then all module clocks need to be disabled and then the partition can be powergated. After ungating, the module resets need to be deasserted and the FLUSH bit cleared with clocks enabled. Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? I think it's indeed better to have a direct reference to the required clocks to powergate/ungate a domain. As you said, there is no easy way to derive the required clocks from the DT module declarations. My suggestion would be to have powerdomain definitions in DT and for each domain have references to the required clocks and resets. And specify the dependencies between domains in DT? I think the dependencies could be in the driver. Of course the power domains are per-SoC data, so really shouldn't be in the DTS either (the data is all implied by the compatible value) but there's no good way to get at the clocks and resets without DT, so I think that's a reasonable trade-off. It seems to me like there are only two dependencies: DIS and DISB depend on SOR VE depends on DIS That's according to 5.6.6 Programming Guide for Power Gating and Ungating of the Tegra K1 TRM. It also seems like a bunch of modules are part of seemingly unrelated domains. Especially SOR seems to cover a large range of modules (MIPI-CAL, DPAUX, SOR, HDMI, DSI, DSIB and HDA2HDMI). Given that we may want to more fine-grainedly control clocks to save power, don't we need to control clocks and resets within the drivers? I think the runtime PM framework makes sure to call this in the right order, so for suspend, the sequence would be: 1) device-suspend 2) domain-suspend and for resume: 1) domain-resume 2) device-resume But then we're back to square one, namely that the MC flush doesn't work properly, since it needs to be implemented in domain-suspend. Does that mean we can't clock-gate modules? In
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Wed, Jan 07, 2015 at 02:27:10PM +0100, Thierry Reding wrote: Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? This whole situation is quite messy. The above sequence basically means that drivers can't reset hardware modules because otherwise they might race with the power domain code. It also means that we can't powergate The powerdomain framework won't call any powergating method as long as a module in the domain is still active. So as long as drivers don't try to reset the hw without having done a pm_runtime_get(), we shouldn't have such a race? modules on demand because they might be in the same power domain as one other module that's still busy. The powerdomain framework keeps track of which modules are active (by hooking into runtime pm) and won't try to shutdown a domain unless all modules are inactive. How would we handle a situation where a hardware module hangs and we can only get it back via a reset? Cheers, Peter. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 04:08:52PM Jan 07, Peter De Schrijver wrote: On Wed, Jan 07, 2015 at 02:27:10PM +0100, Thierry Reding wrote: Yeah. I plan to have the information of all the clock client of the partitions and the memory clients be defined statically in c source, e.g. pmc-tegra124.c. All modules can declare which domain they belong to in DT. One domain can be really power gated only when no module is awake. Note the clock clients of one domain might not equal to the clocks of the module. The reset is not either. So I don't get the clock and reset from module. How do you think? This whole situation is quite messy. The above sequence basically means that drivers can't reset hardware modules because otherwise they might race with the power domain code. It also means that we can't powergate The powerdomain framework won't call any powergating method as long as a module in the domain is still active. So as long as drivers don't try to reset the hw without having done a pm_runtime_get(), we shouldn't have such a race? Agree. And as long as the driver has the correct reset procedure, that should be fine to occur between power ungating and gating sequences. modules on demand because they might be in the same power domain as one other module that's still busy. The powerdomain framework keeps track of which modules are active (by hooking into runtime pm) and won't try to shutdown a domain unless all modules are inactive. Yeah. By the way, that means we should start supporting runtime pm for all the modules to use generic power domain. Thanks, Vince How would we handle a situation where a hardware module hangs and we can only get it back via a reset? Cheers, Peter. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Tue, Jan 06, 2015 at 09:51:11PM +0800, Vince Hsu wrote: > On 02:29:32PM Jan 06, Thierry Reding wrote: > > * PGP Signed by an unknown key > > > > On Tue, Jan 06, 2015 at 08:03:03PM +0800, Vince Hsu wrote: > > > On 01/06/2015 07:15 PM, Thierry Reding wrote: > > > >> Old Signed by an unknown key > > > > > > > >On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote: > > > >>On 01/05/2015 11:09 PM, Thierry Reding wrote: > > > Old Signed by an unknown key > > > >>>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 > > > >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: > > > >>> > > > >>> https://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. > > > >>You generic power domains work is exactly what we want for powergate > > > >>eventually. :) I recall we used your prototyping in somewhere internal > > > >>tree. > > > >>We have to add more to complete it though, e.g. power domain > > > >>dependency, mc > > > >>flush, and clamping register difference like this patch does. > > > >> > > > >>But I have a question here. Since the GK20A is not powered on/off by > > > >>the PMC > > > >>except the clamping control, and GK20A has its own power rail, do we > > > >>really > > > >>want to hide the power sequence in the generic powergate code? We still > > > >>have > > > >>to control the voltage level in the GK20A driver through the regulator > > > >>framework. It seems weird to me if we put the regulator_{enable|disable} > > > >>somewhere other than the GK20A driver. > > > >I think we want both. The power domain to control the power partition > > > >and the regulator in the gk20a driver for the voltage control. > > > Do you mean excluding the power sequence of GK20A from the generic power > > > domain? > > > > No, what I mean is move the power gating into the PMC driver as part of > > the generic power domain implementation. At the same time, keep the > > control of the regulator within the gk20a driver. That way we can use > > runtime PM to control the powergating but we can still control the GPU > > voltage within Nouveau. > Okay. Do you insist to introduce the generic power domain at this moment? > If so, I can try to continue your previous work and rebase this series on > that. That might take some time though. Yes, I'm leaning strongly towards doing that conversion now. The problem with extending the reset API with flushing is that it will make the conversion more difficult than it already is. I'm not even sure we can go through with the conversion without breaking DT ABI stability yet again. We may be able to do so by keeping the current code as fallback if we can determine that no power domain is added (dev->pm_domain?). But adding tegra_mc_flush() calls everywhere now will mean that we have to keep even more fallback code, basically forever. I know this is going to take some time, but the longer we defer this the more work it will become. And we've already been pushing this back for over a year. Thierry pgpiDCFb8k7tP.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 02:29:32PM Jan 06, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Tue, Jan 06, 2015 at 08:03:03PM +0800, Vince Hsu wrote: > > On 01/06/2015 07:15 PM, Thierry Reding wrote: > > >> Old Signed by an unknown key > > > > > >On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote: > > >>On 01/05/2015 11:09 PM, Thierry Reding wrote: > > Old Signed by an unknown key > > >>>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 > > >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: > > >>> > > >>> https://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. > > >>You generic power domains work is exactly what we want for powergate > > >>eventually. :) I recall we used your prototyping in somewhere internal > > >>tree. > > >>We have to add more to complete it though, e.g. power domain dependency, > > >>mc > > >>flush, and clamping register difference like this patch does. > > >> > > >>But I have a question here. Since the GK20A is not powered on/off by the > > >>PMC > > >>except the clamping control, and GK20A has its own power rail, do we > > >>really > > >>want to hide the power sequence in the generic powergate code? We still > > >>have > > >>to control the voltage level in the GK20A driver through the regulator > > >>framework. It seems weird to me if we put the regulator_{enable|disable} > > >>somewhere other than the GK20A driver. > > >I think we want both. The power domain to control the power partition > > >and the regulator in the gk20a driver for the voltage control. > > Do you mean excluding the power sequence of GK20A from the generic power > > domain? > > No, what I mean is move the power gating into the PMC driver as part of > the generic power domain implementation. At the same time, keep the > control of the regulator within the gk20a driver. That way we can use > runtime PM to control the powergating but we can still control the GPU > voltage within Nouveau. Okay. Do you insist to introduce the generic power domain at this moment? If so, I can try to continue your previous work and rebase this series on that. That might take some time though. Thanks, Vince -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Tue, Jan 06, 2015 at 08:03:03PM +0800, Vince Hsu wrote: > On 01/06/2015 07:15 PM, Thierry Reding wrote: > >* PGP Signed by an unknown key > > > >On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote: > >>On 01/05/2015 11:09 PM, Thierry Reding wrote: > Old Signed by an unknown key > >>>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 > >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: > >>> > >>> https://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. > >>You generic power domains work is exactly what we want for powergate > >>eventually. :) I recall we used your prototyping in somewhere internal tree. > >>We have to add more to complete it though, e.g. power domain dependency, mc > >>flush, and clamping register difference like this patch does. > >> > >>But I have a question here. Since the GK20A is not powered on/off by the PMC > >>except the clamping control, and GK20A has its own power rail, do we really > >>want to hide the power sequence in the generic powergate code? We still have > >>to control the voltage level in the GK20A driver through the regulator > >>framework. It seems weird to me if we put the regulator_{enable|disable} > >>somewhere other than the GK20A driver. > >I think we want both. The power domain to control the power partition > >and the regulator in the gk20a driver for the voltage control. > Do you mean excluding the power sequence of GK20A from the generic power > domain? No, what I mean is move the power gating into the PMC driver as part of the generic power domain implementation. At the same time, keep the control of the regulator within the gk20a driver. That way we can use runtime PM to control the powergating but we can still control the GPU voltage within Nouveau. Thierry pgp2AmfpE5lDx.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 01/06/2015 07:15 PM, Thierry Reding wrote: * PGP Signed by an unknown key On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote: On 01/05/2015 11:09 PM, Thierry Reding wrote: Old Signed by an unknown key 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 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: https://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. You generic power domains work is exactly what we want for powergate eventually. :) I recall we used your prototyping in somewhere internal tree. We have to add more to complete it though, e.g. power domain dependency, mc flush, and clamping register difference like this patch does. But I have a question here. Since the GK20A is not powered on/off by the PMC except the clamping control, and GK20A has its own power rail, do we really want to hide the power sequence in the generic powergate code? We still have to control the voltage level in the GK20A driver through the regulator framework. It seems weird to me if we put the regulator_{enable|disable} somewhere other than the GK20A driver. I think we want both. The power domain to control the power partition and the regulator in the gk20a driver for the voltage control. Do you mean excluding the power sequence of GK20A from the generic power domain? Thanks, Vince -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote: > > On 01/05/2015 11:09 PM, Thierry Reding wrote: > >* PGP Signed by an unknown key > > > >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 > >>>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: > > > > https://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. > You generic power domains work is exactly what we want for powergate > eventually. :) I recall we used your prototyping in somewhere internal tree. > We have to add more to complete it though, e.g. power domain dependency, mc > flush, and clamping register difference like this patch does. > > But I have a question here. Since the GK20A is not powered on/off by the PMC > except the clamping control, and GK20A has its own power rail, do we really > want to hide the power sequence in the generic powergate code? We still have > to control the voltage level in the GK20A driver through the regulator > framework. It seems weird to me if we put the regulator_{enable|disable} > somewhere other than the GK20A driver. I think we want both. The power domain to control the power partition and the regulator in the gk20a driver for the voltage control. Thierry pgpJnk44eLQeZ.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote: On 01/05/2015 11:09 PM, Thierry Reding wrote: * PGP Signed by an unknown key 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 vin...@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: https://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. You generic power domains work is exactly what we want for powergate eventually. :) I recall we used your prototyping in somewhere internal tree. We have to add more to complete it though, e.g. power domain dependency, mc flush, and clamping register difference like this patch does. But I have a question here. Since the GK20A is not powered on/off by the PMC except the clamping control, and GK20A has its own power rail, do we really want to hide the power sequence in the generic powergate code? We still have to control the voltage level in the GK20A driver through the regulator framework. It seems weird to me if we put the regulator_{enable|disable} somewhere other than the GK20A driver. I think we want both. The power domain to control the power partition and the regulator in the gk20a driver for the voltage control. Thierry pgpJnk44eLQeZ.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Tue, Jan 06, 2015 at 08:03:03PM +0800, Vince Hsu wrote: On 01/06/2015 07:15 PM, Thierry Reding wrote: * PGP Signed by an unknown key On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote: On 01/05/2015 11:09 PM, Thierry Reding wrote: Old Signed by an unknown key 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 vin...@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: https://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. You generic power domains work is exactly what we want for powergate eventually. :) I recall we used your prototyping in somewhere internal tree. We have to add more to complete it though, e.g. power domain dependency, mc flush, and clamping register difference like this patch does. But I have a question here. Since the GK20A is not powered on/off by the PMC except the clamping control, and GK20A has its own power rail, do we really want to hide the power sequence in the generic powergate code? We still have to control the voltage level in the GK20A driver through the regulator framework. It seems weird to me if we put the regulator_{enable|disable} somewhere other than the GK20A driver. I think we want both. The power domain to control the power partition and the regulator in the gk20a driver for the voltage control. Do you mean excluding the power sequence of GK20A from the generic power domain? No, what I mean is move the power gating into the PMC driver as part of the generic power domain implementation. At the same time, keep the control of the regulator within the gk20a driver. That way we can use runtime PM to control the powergating but we can still control the GPU voltage within Nouveau. Thierry pgp2AmfpE5lDx.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 01/06/2015 07:15 PM, Thierry Reding wrote: * PGP Signed by an unknown key On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote: On 01/05/2015 11:09 PM, Thierry Reding wrote: Old Signed by an unknown key 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 vin...@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: https://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. You generic power domains work is exactly what we want for powergate eventually. :) I recall we used your prototyping in somewhere internal tree. We have to add more to complete it though, e.g. power domain dependency, mc flush, and clamping register difference like this patch does. But I have a question here. Since the GK20A is not powered on/off by the PMC except the clamping control, and GK20A has its own power rail, do we really want to hide the power sequence in the generic powergate code? We still have to control the voltage level in the GK20A driver through the regulator framework. It seems weird to me if we put the regulator_{enable|disable} somewhere other than the GK20A driver. I think we want both. The power domain to control the power partition and the regulator in the gk20a driver for the voltage control. Do you mean excluding the power sequence of GK20A from the generic power domain? Thanks, Vince -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On Tue, Jan 06, 2015 at 09:51:11PM +0800, Vince Hsu wrote: On 02:29:32PM Jan 06, Thierry Reding wrote: * PGP Signed by an unknown key On Tue, Jan 06, 2015 at 08:03:03PM +0800, Vince Hsu wrote: On 01/06/2015 07:15 PM, Thierry Reding wrote: Old Signed by an unknown key On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote: On 01/05/2015 11:09 PM, Thierry Reding wrote: Old Signed by an unknown key 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 vin...@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: https://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. You generic power domains work is exactly what we want for powergate eventually. :) I recall we used your prototyping in somewhere internal tree. We have to add more to complete it though, e.g. power domain dependency, mc flush, and clamping register difference like this patch does. But I have a question here. Since the GK20A is not powered on/off by the PMC except the clamping control, and GK20A has its own power rail, do we really want to hide the power sequence in the generic powergate code? We still have to control the voltage level in the GK20A driver through the regulator framework. It seems weird to me if we put the regulator_{enable|disable} somewhere other than the GK20A driver. I think we want both. The power domain to control the power partition and the regulator in the gk20a driver for the voltage control. Do you mean excluding the power sequence of GK20A from the generic power domain? No, what I mean is move the power gating into the PMC driver as part of the generic power domain implementation. At the same time, keep the control of the regulator within the gk20a driver. That way we can use runtime PM to control the powergating but we can still control the GPU voltage within Nouveau. Okay. Do you insist to introduce the generic power domain at this moment? If so, I can try to continue your previous work and rebase this series on that. That might take some time though. Yes, I'm leaning strongly towards doing that conversion now. The problem with extending the reset API with flushing is that it will make the conversion more difficult than it already is. I'm not even sure we can go through with the conversion without breaking DT ABI stability yet again. We may be able to do so by keeping the current code as fallback if we can determine that no power domain is added (dev-pm_domain?). But adding tegra_mc_flush() calls everywhere now will mean that we have to keep even more fallback code, basically forever. I know this is going to take some time, but the longer we defer this the more work it will become. And we've already been pushing this back for over a year. Thierry pgpiDCFb8k7tP.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 02:29:32PM Jan 06, Thierry Reding wrote: * PGP Signed by an unknown key On Tue, Jan 06, 2015 at 08:03:03PM +0800, Vince Hsu wrote: On 01/06/2015 07:15 PM, Thierry Reding wrote: Old Signed by an unknown key On Tue, Jan 06, 2015 at 10:11:41AM +0800, Vince Hsu wrote: On 01/05/2015 11:09 PM, Thierry Reding wrote: Old Signed by an unknown key 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 vin...@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: https://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. You generic power domains work is exactly what we want for powergate eventually. :) I recall we used your prototyping in somewhere internal tree. We have to add more to complete it though, e.g. power domain dependency, mc flush, and clamping register difference like this patch does. But I have a question here. Since the GK20A is not powered on/off by the PMC except the clamping control, and GK20A has its own power rail, do we really want to hide the power sequence in the generic powergate code? We still have to control the voltage level in the GK20A driver through the regulator framework. It seems weird to me if we put the regulator_{enable|disable} somewhere other than the GK20A driver. I think we want both. The power domain to control the power partition and the regulator in the gk20a driver for the voltage control. Do you mean excluding the power sequence of GK20A from the generic power domain? No, what I mean is move the power gating into the PMC driver as part of the generic power domain implementation. At the same time, keep the control of the regulator within the gk20a driver. That way we can use runtime PM to control the powergating but we can still control the GPU voltage within Nouveau. Okay. Do you insist to introduce the generic power domain at this moment? If so, I can try to continue your previous work and rebase this series on that. That might take some time though. Thanks, Vince -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 01/05/2015 11:09 PM, Thierry Reding wrote: * PGP Signed by an unknown key 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 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: https://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. You generic power domains work is exactly what we want for powergate eventually. :) I recall we used your prototyping in somewhere internal tree. We have to add more to complete it though, e.g. power domain dependency, mc flush, and clamping register difference like this patch does. But I have a question here. Since the GK20A is not powered on/off by the PMC except the clamping control, and GK20A has its own power rail, do we really want to hide the power sequence in the generic powergate code? We still have to control the voltage level in the GK20A driver through the regulator framework. It seems weird to me if we put the regulator_{enable|disable} somewhere other than the GK20A driver. Thanks, Vince 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 * Unknown Key * 0x7F3EB3A1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [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 > >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: https://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 pgpm8M87psgh5.pgp Description: PGP signature
Re: [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 vin...@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: https://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 pgpm8M87psgh5.pgp Description: PGP signature
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 01/05/2015 11:09 PM, Thierry Reding wrote: * PGP Signed by an unknown key 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 vin...@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: https://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. You generic power domains work is exactly what we want for powergate eventually. :) I recall we used your prototyping in somewhere internal tree. We have to add more to complete it though, e.g. power domain dependency, mc flush, and clamping register difference like this patch does. But I have a question here. Since the GK20A is not powered on/off by the PMC except the clamping control, and GK20A has its own power rail, do we really want to hide the power sequence in the generic powergate code? We still have to control the voltage level in the GK20A driver through the regulator framework. It seems weird to me if we put the regulator_{enable|disable} somewhere other than the GK20A driver. Thanks, Vince 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 * Unknown Key * 0x7F3EB3A1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 12/31/2014 12:42 AM, Lucas Stach wrote: Am Montag, den 29.12.2014, 10:49 +0800 schrieb Vince Hsu: [...] 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. Unfortunately this doesn't seem to be included in the public TRM. It would be nice if this could be documented either in the next version of the TRM or as a public Appnote. It should be in the latest public TRM. Could you check again? Thanks, Vince -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 12/31/2014 12:42 AM, Lucas Stach wrote: Am Montag, den 29.12.2014, 10:49 +0800 schrieb Vince Hsu: [...] 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. Unfortunately this doesn't seem to be included in the public TRM. It would be nice if this could be documented either in the next version of the TRM or as a public Appnote. It should be in the latest public TRM. Could you check again? Thanks, Vince -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
Am Montag, den 29.12.2014, 10:49 +0800 schrieb Vince Hsu: [...] > >> 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. > Unfortunately this doesn't seem to be included in the public TRM. It would be nice if this could be documented either in the next version of the TRM or as a public Appnote. Thanks, Lucas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
Am Montag, den 29.12.2014, 10:49 +0800 schrieb Vince Hsu: [...] 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. Unfortunately this doesn't seem to be included in the public TRM. It would be nice if this could be documented either in the next version of the TRM or as a public Appnote. Thanks, Lucas -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at
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 vin...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at
Re: [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 > > 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 > 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? And isn't it needed for the other partition ungating function too then? Regards, Lucas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [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 vin...@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 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? And isn't it needed for the other partition ungating function too then? Regards, Lucas -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 12/24/2014 09:52 PM, Dmitry Osipenko wrote: I think "ARM: tegra:" is wrong prefix for this patch and "soc: tegra:" should be used instead to show that it belongs to SoC driver, not arch code. Indeed. Will fix in v2. Thanks, Vince -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
I think "ARM: tegra:" is wrong prefix for this patch and "soc: tegra:" should be used instead to show that it belongs to SoC driver, not arch code. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [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 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, -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [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 vin...@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, -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
I think ARM: tegra: is wrong prefix for this patch and soc: tegra: should be used instead to show that it belongs to SoC driver, not arch code. -- Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/11] ARM: tegra: add function to control the GPU rail clamp
On 12/24/2014 09:52 PM, Dmitry Osipenko wrote: I think ARM: tegra: is wrong prefix for this patch and soc: tegra: should be used instead to show that it belongs to SoC driver, not arch code. Indeed. Will fix in v2. Thanks, Vince -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 vin...@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 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/