| Message ID | 54E42D93.2090809@siemens.com |
|---|---|
| State | RFC |
| Delegated to: | Tom Warren |
| Headers | show |
On 02/17/2015 11:13 PM, Jan Kiszka wrote: > On 2015-02-17 22:03, Stephen Warren wrote: >> On 02/16/2015 05:54 AM, Jan Kiszka wrote: >>> This is based on Thierry Reding's work and uses Ian Campell's >>> preparatory patches. It comes with full support for CPU_ON/OFF PSCI >>> services. The algorithm used in this version for turning CPUs on and >>> off was proposed by Thierry Reding in >>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It >>> consists of first enabling CPU1..3 via the PMC, just to powergate them >>> again with the help of the Flow Controller. Once the Flow Controller is >>> in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF >>> PSCI requests. >> >>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c >>> b/arch/arm/cpu/armv7/tegra124/ap.c >> >>> +void ap_pm_init(void) >>> +{ >>> + struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE; >>> + struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE; >>> + >>> + writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR); >>> + >>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); >>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); >>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); >>> + >>> + writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr); >>> + writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr); >>> + writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr); >>> + >>> + writel(EVENT_MODE_STOP, &flow->halt_cpu1_events); >>> + writel(EVENT_MODE_STOP, &flow->halt_cpu2_events); >>> + writel(EVENT_MODE_STOP, &flow->halt_cpu3_events); >> >> I would expect to set up halt_cpu*_events before powering on the CPUs, >> to make sure that they do the expected action on the very first WFI. So, >> shouldn't the order above be: >> >> Write to halt_cpu*_events >> Write to cpu*_csr >> power_on > > Yeah, that was my original expectation as well. But > > diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c > index eebc0ea..240c71d 100644 > --- a/arch/arm/cpu/armv7/tegra124/ap.c > +++ b/arch/arm/cpu/armv7/tegra124/ap.c > @@ -25,10 +25,6 @@ void ap_pm_init(void) > > writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR); > > - tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); > - tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); > - tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); > - > writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr); > writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr); > writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr); > @@ -37,6 +33,10 @@ void ap_pm_init(void) > writel(EVENT_MODE_STOP, &flow->halt_cpu2_events); > writel(EVENT_MODE_STOP, &flow->halt_cpu3_events); > > + tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); > + tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); > + tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); > + > while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) | > (1 << TEGRA_POWERGATE_CPU2) | > (1 << TEGRA_POWERGATE_CPU3))) > > doesn't work in practice. I suspect the power-on overwrites what the > flow controller configures in the PMC beforehand. But maybe someone can > explain this better than me. Thierry, Peter, can you comment on why that is, and whether the original code sequence is safe; does it matter that the target CPU executes WFI before the flow controller is configured what to do on WFI?
On Wed, Feb 18, 2015 at 09:34:53AM -0700, Stephen Warren wrote: > On 02/17/2015 11:13 PM, Jan Kiszka wrote: > >On 2015-02-17 22:03, Stephen Warren wrote: > >>On 02/16/2015 05:54 AM, Jan Kiszka wrote: > >>>This is based on Thierry Reding's work and uses Ian Campell's > >>>preparatory patches. It comes with full support for CPU_ON/OFF PSCI > >>>services. The algorithm used in this version for turning CPUs on and > >>>off was proposed by Thierry Reding in > >>>http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It > >>>consists of first enabling CPU1..3 via the PMC, just to powergate them > >>>again with the help of the Flow Controller. Once the Flow Controller is > >>>in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF > >>>PSCI requests. > >> > >>>diff --git a/arch/arm/cpu/armv7/tegra124/ap.c > >>>b/arch/arm/cpu/armv7/tegra124/ap.c > >> > >>>+void ap_pm_init(void) > >>>+{ > >>>+ struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE; > >>>+ struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE; > >>>+ > >>>+ writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR); > >>>+ > >>>+ tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); > >>>+ tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); > >>>+ tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); > >>>+ > >>>+ writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr); > >>>+ writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr); > >>>+ writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr); > >>>+ > >>>+ writel(EVENT_MODE_STOP, &flow->halt_cpu1_events); > >>>+ writel(EVENT_MODE_STOP, &flow->halt_cpu2_events); > >>>+ writel(EVENT_MODE_STOP, &flow->halt_cpu3_events); > >> > >>I would expect to set up halt_cpu*_events before powering on the CPUs, > >>to make sure that they do the expected action on the very first WFI. So, > >>shouldn't the order above be: > >> > >>Write to halt_cpu*_events > >>Write to cpu*_csr > >>power_on > > > >Yeah, that was my original expectation as well. But > > > >diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c > >index eebc0ea..240c71d 100644 > >--- a/arch/arm/cpu/armv7/tegra124/ap.c > >+++ b/arch/arm/cpu/armv7/tegra124/ap.c > >@@ -25,10 +25,6 @@ void ap_pm_init(void) > > > > writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR); > > > >- tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); > >- tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); > >- tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); > >- > > writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr); > > writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr); > > writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr); > >@@ -37,6 +33,10 @@ void ap_pm_init(void) > > writel(EVENT_MODE_STOP, &flow->halt_cpu2_events); > > writel(EVENT_MODE_STOP, &flow->halt_cpu3_events); > > > >+ tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); > >+ tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); > >+ tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); > >+ > > while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) | > > (1 << TEGRA_POWERGATE_CPU2) | > > (1 << TEGRA_POWERGATE_CPU3))) > > > >doesn't work in practice. I suspect the power-on overwrites what the > >flow controller configures in the PMC beforehand. But maybe someone can > >explain this better than me. > > Thierry, Peter, can you comment on why that is, and whether the original > code sequence is safe; does it matter that the target CPU executes WFI > before the flow controller is configured what to do on WFI? As I mentioned before, I don't think it's safe to change the powergate status of more than one partition at once. I'm not sure that this will change anything regarding the relative positioning of powergate on vs. writing CPU halt events, but I agree with Stephen that running the CPU without the halt events being programmed could cause them to simply go into a WFI without them actually being turned off. Perhaps if unpowergating after writing the halt events registers doesn't work a safer way would be to go and forcibly wake up all CPUs again after they are powered up (using the IMMEDIATE_WAKE bit in the CSR)? I haven't seen anything in the documentation regarding why unpowergating after writing halt event registers wouldn't work. I'm sure I haven't looked at all the documentation, but this is about as knowledgeable as I am regarding the CPUs and the flow controller. Perhaps Peter will indeed know more than that. Thierry
On 2015-02-19 10:14, Thierry Reding wrote: > On Wed, Feb 18, 2015 at 09:34:53AM -0700, Stephen Warren wrote: >> On 02/17/2015 11:13 PM, Jan Kiszka wrote: >>> On 2015-02-17 22:03, Stephen Warren wrote: >>>> On 02/16/2015 05:54 AM, Jan Kiszka wrote: >>>>> This is based on Thierry Reding's work and uses Ian Campell's >>>>> preparatory patches. It comes with full support for CPU_ON/OFF PSCI >>>>> services. The algorithm used in this version for turning CPUs on and >>>>> off was proposed by Thierry Reding in >>>>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It >>>>> consists of first enabling CPU1..3 via the PMC, just to powergate them >>>>> again with the help of the Flow Controller. Once the Flow Controller is >>>>> in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF >>>>> PSCI requests. >>>> >>>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c >>>>> b/arch/arm/cpu/armv7/tegra124/ap.c >>>> >>>>> +void ap_pm_init(void) >>>>> +{ >>>>> + struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE; >>>>> + struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE; >>>>> + >>>>> + writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR); >>>>> + >>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); >>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); >>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); >>>>> + >>>>> + writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr); >>>>> + writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr); >>>>> + writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr); >>>>> + >>>>> + writel(EVENT_MODE_STOP, &flow->halt_cpu1_events); >>>>> + writel(EVENT_MODE_STOP, &flow->halt_cpu2_events); >>>>> + writel(EVENT_MODE_STOP, &flow->halt_cpu3_events); >>>> >>>> I would expect to set up halt_cpu*_events before powering on the CPUs, >>>> to make sure that they do the expected action on the very first WFI. So, >>>> shouldn't the order above be: >>>> >>>> Write to halt_cpu*_events >>>> Write to cpu*_csr >>>> power_on >>> >>> Yeah, that was my original expectation as well. But >>> >>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c >>> index eebc0ea..240c71d 100644 >>> --- a/arch/arm/cpu/armv7/tegra124/ap.c >>> +++ b/arch/arm/cpu/armv7/tegra124/ap.c >>> @@ -25,10 +25,6 @@ void ap_pm_init(void) >>> >>> writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR); >>> >>> - tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); >>> - tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); >>> - tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); >>> - >>> writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr); >>> writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr); >>> writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr); >>> @@ -37,6 +33,10 @@ void ap_pm_init(void) >>> writel(EVENT_MODE_STOP, &flow->halt_cpu2_events); >>> writel(EVENT_MODE_STOP, &flow->halt_cpu3_events); >>> >>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); >>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); >>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); >>> + >>> while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) | >>> (1 << TEGRA_POWERGATE_CPU2) | >>> (1 << TEGRA_POWERGATE_CPU3))) >>> >>> doesn't work in practice. I suspect the power-on overwrites what the >>> flow controller configures in the PMC beforehand. But maybe someone can >>> explain this better than me. >> >> Thierry, Peter, can you comment on why that is, and whether the original >> code sequence is safe; does it matter that the target CPU executes WFI >> before the flow controller is configured what to do on WFI? > > As I mentioned before, I don't think it's safe to change the powergate > status of more than one partition at once. I'm not sure that this will tegra_powergate_set() already synchronizes the caller on the completion of the switch. So the existing code is safe in this regard. However, the K1 manual also states that the START bit of the toggle register should be checked prior to starting a request. This is not done by tegra_powergate_set() - probably because it is a K1-only requirement, not applying to older CPUs. Not sure, though, if waiting for START=0 is practically required when already waiting for the switch to be processed by the PMC before continuing. > change anything regarding the relative positioning of powergate on vs. > writing CPU halt events, but I agree with Stephen that running the CPU > without the halt events being programmed could cause them to simply go > into a WFI without them actually being turned off. The CPUs most probably go into WFI first, because we wait for the partition to be reported as powered up, but it seems they can be turned off while in WFI as well. I'm not basing this on anything stated in the manual, just on experiments. > > Perhaps if unpowergating after writing the halt events registers doesn't > work a safer way would be to go and forcibly wake up all CPUs again > after they are powered up (using the IMMEDIATE_WAKE bit in the CSR)? > > I haven't seen anything in the documentation regarding why unpowergating > after writing halt event registers wouldn't work. I'm sure I haven't > looked at all the documentation, but this is about as knowledgeable as I > am regarding the CPUs and the flow controller. Perhaps Peter will indeed > know more than that. Yes, more insights would indeed be welcome! Thanks, Jan
On 2015-02-20 10:36, Jan Kiszka wrote: > On 2015-02-19 10:14, Thierry Reding wrote: >> On Wed, Feb 18, 2015 at 09:34:53AM -0700, Stephen Warren wrote: >>> On 02/17/2015 11:13 PM, Jan Kiszka wrote: >>>> On 2015-02-17 22:03, Stephen Warren wrote: >>>>> On 02/16/2015 05:54 AM, Jan Kiszka wrote: >>>>>> This is based on Thierry Reding's work and uses Ian Campell's >>>>>> preparatory patches. It comes with full support for CPU_ON/OFF PSCI >>>>>> services. The algorithm used in this version for turning CPUs on and >>>>>> off was proposed by Thierry Reding in >>>>>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It >>>>>> consists of first enabling CPU1..3 via the PMC, just to powergate them >>>>>> again with the help of the Flow Controller. Once the Flow Controller is >>>>>> in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF >>>>>> PSCI requests. >>>>> >>>>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c >>>>>> b/arch/arm/cpu/armv7/tegra124/ap.c >>>>> >>>>>> +void ap_pm_init(void) >>>>>> +{ >>>>>> + struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE; >>>>>> + struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE; >>>>>> + >>>>>> + writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR); >>>>>> + >>>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); >>>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); >>>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); >>>>>> + >>>>>> + writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr); >>>>>> + writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr); >>>>>> + writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr); >>>>>> + >>>>>> + writel(EVENT_MODE_STOP, &flow->halt_cpu1_events); >>>>>> + writel(EVENT_MODE_STOP, &flow->halt_cpu2_events); >>>>>> + writel(EVENT_MODE_STOP, &flow->halt_cpu3_events); >>>>> >>>>> I would expect to set up halt_cpu*_events before powering on the CPUs, >>>>> to make sure that they do the expected action on the very first WFI. So, >>>>> shouldn't the order above be: >>>>> >>>>> Write to halt_cpu*_events >>>>> Write to cpu*_csr >>>>> power_on >>>> >>>> Yeah, that was my original expectation as well. But >>>> >>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c >>>> index eebc0ea..240c71d 100644 >>>> --- a/arch/arm/cpu/armv7/tegra124/ap.c >>>> +++ b/arch/arm/cpu/armv7/tegra124/ap.c >>>> @@ -25,10 +25,6 @@ void ap_pm_init(void) >>>> >>>> writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR); >>>> >>>> - tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); >>>> - tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); >>>> - tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); >>>> - >>>> writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr); >>>> writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr); >>>> writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr); >>>> @@ -37,6 +33,10 @@ void ap_pm_init(void) >>>> writel(EVENT_MODE_STOP, &flow->halt_cpu2_events); >>>> writel(EVENT_MODE_STOP, &flow->halt_cpu3_events); >>>> >>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); >>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); >>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); >>>> + >>>> while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) | >>>> (1 << TEGRA_POWERGATE_CPU2) | >>>> (1 << TEGRA_POWERGATE_CPU3))) >>>> >>>> doesn't work in practice. I suspect the power-on overwrites what the >>>> flow controller configures in the PMC beforehand. But maybe someone can >>>> explain this better than me. >>> >>> Thierry, Peter, can you comment on why that is, and whether the original >>> code sequence is safe; does it matter that the target CPU executes WFI >>> before the flow controller is configured what to do on WFI? >> >> As I mentioned before, I don't think it's safe to change the powergate >> status of more than one partition at once. I'm not sure that this will > > tegra_powergate_set() already synchronizes the caller on the completion > of the switch. So the existing code is safe in this regard. > > However, the K1 manual also states that the START bit of the toggle > register should be checked prior to starting a request. This is not done > by tegra_powergate_set() - probably because it is a K1-only requirement, > not applying to older CPUs. Not sure, though, if waiting for START=0 is > practically required when already waiting for the switch to be processed > by the PMC before continuing. > >> change anything regarding the relative positioning of powergate on vs. >> writing CPU halt events, but I agree with Stephen that running the CPU >> without the halt events being programmed could cause them to simply go >> into a WFI without them actually being turned off. > > The CPUs most probably go into WFI first, because we wait for the > partition to be reported as powered up, but it seems they can be turned > off while in WFI as well. I'm not basing this on anything stated in the > manual, just on experiments. > >> >> Perhaps if unpowergating after writing the halt events registers doesn't >> work a safer way would be to go and forcibly wake up all CPUs again >> after they are powered up (using the IMMEDIATE_WAKE bit in the CSR)? >> >> I haven't seen anything in the documentation regarding why unpowergating >> after writing halt event registers wouldn't work. I'm sure I haven't >> looked at all the documentation, but this is about as knowledgeable as I >> am regarding the CPUs and the flow controller. Perhaps Peter will indeed >> know more than that. > > Yes, more insights would indeed be welcome! Ping regarding this last open point. I'm sitting on v4 of this series - v3 had a nasty bug in the CNTFRQ fixup, I addressed the reviews and added further cleanups - and I would like to close this topic eventually. Jan PS: After KVM, I was also able to get the Jailhouse hypervisor running on the TK1 - thanks to this series (v4, to be precise): http://thread.gmane.org/gmane.linux.jailhouse/2580
On Tue, Feb 24, 2015 at 08:23:55AM +0100, Jan Kiszka wrote: > On 2015-02-20 10:36, Jan Kiszka wrote: > > On 2015-02-19 10:14, Thierry Reding wrote: > >> On Wed, Feb 18, 2015 at 09:34:53AM -0700, Stephen Warren wrote: > >>> On 02/17/2015 11:13 PM, Jan Kiszka wrote: > >>>> On 2015-02-17 22:03, Stephen Warren wrote: > >>>>> On 02/16/2015 05:54 AM, Jan Kiszka wrote: > >>>>>> This is based on Thierry Reding's work and uses Ian Campell's > >>>>>> preparatory patches. It comes with full support for CPU_ON/OFF PSCI > >>>>>> services. The algorithm used in this version for turning CPUs on and > >>>>>> off was proposed by Thierry Reding in > >>>>>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It > >>>>>> consists of first enabling CPU1..3 via the PMC, just to powergate them > >>>>>> again with the help of the Flow Controller. Once the Flow Controller is > >>>>>> in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF > >>>>>> PSCI requests. > >>>>> > >>>>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c > >>>>>> b/arch/arm/cpu/armv7/tegra124/ap.c > >>>>> > >>>>>> +void ap_pm_init(void) > >>>>>> +{ > >>>>>> + struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE; > >>>>>> + struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE; > >>>>>> + > >>>>>> + writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR); > >>>>>> + > >>>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); > >>>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); > >>>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); > >>>>>> + > >>>>>> + writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr); > >>>>>> + writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr); > >>>>>> + writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr); > >>>>>> + > >>>>>> + writel(EVENT_MODE_STOP, &flow->halt_cpu1_events); > >>>>>> + writel(EVENT_MODE_STOP, &flow->halt_cpu2_events); > >>>>>> + writel(EVENT_MODE_STOP, &flow->halt_cpu3_events); > >>>>> > >>>>> I would expect to set up halt_cpu*_events before powering on the CPUs, > >>>>> to make sure that they do the expected action on the very first WFI. So, > >>>>> shouldn't the order above be: > >>>>> > >>>>> Write to halt_cpu*_events > >>>>> Write to cpu*_csr > >>>>> power_on > >>>> > >>>> Yeah, that was my original expectation as well. But > >>>> > >>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c > >>>> index eebc0ea..240c71d 100644 > >>>> --- a/arch/arm/cpu/armv7/tegra124/ap.c > >>>> +++ b/arch/arm/cpu/armv7/tegra124/ap.c > >>>> @@ -25,10 +25,6 @@ void ap_pm_init(void) > >>>> > >>>> writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR); > >>>> > >>>> - tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); > >>>> - tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); > >>>> - tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); > >>>> - > >>>> writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr); > >>>> writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr); > >>>> writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr); > >>>> @@ -37,6 +33,10 @@ void ap_pm_init(void) > >>>> writel(EVENT_MODE_STOP, &flow->halt_cpu2_events); > >>>> writel(EVENT_MODE_STOP, &flow->halt_cpu3_events); > >>>> > >>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); > >>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); > >>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); > >>>> + > >>>> while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) | > >>>> (1 << TEGRA_POWERGATE_CPU2) | > >>>> (1 << TEGRA_POWERGATE_CPU3))) > >>>> > >>>> doesn't work in practice. I suspect the power-on overwrites what the > >>>> flow controller configures in the PMC beforehand. But maybe someone can > >>>> explain this better than me. > >>> > >>> Thierry, Peter, can you comment on why that is, and whether the original > >>> code sequence is safe; does it matter that the target CPU executes WFI > >>> before the flow controller is configured what to do on WFI? > >> > >> As I mentioned before, I don't think it's safe to change the powergate > >> status of more than one partition at once. I'm not sure that this will > > > > tegra_powergate_set() already synchronizes the caller on the completion > > of the switch. So the existing code is safe in this regard. > > > > However, the K1 manual also states that the START bit of the toggle > > register should be checked prior to starting a request. This is not done > > by tegra_powergate_set() - probably because it is a K1-only requirement, > > not applying to older CPUs. Not sure, though, if waiting for START=0 is > > practically required when already waiting for the switch to be processed > > by the PMC before continuing. > > > >> change anything regarding the relative positioning of powergate on vs. > >> writing CPU halt events, but I agree with Stephen that running the CPU > >> without the halt events being programmed could cause them to simply go > >> into a WFI without them actually being turned off. > > > > The CPUs most probably go into WFI first, because we wait for the > > partition to be reported as powered up, but it seems they can be turned > > off while in WFI as well. I'm not basing this on anything stated in the > > manual, just on experiments. > > > >> > >> Perhaps if unpowergating after writing the halt events registers doesn't > >> work a safer way would be to go and forcibly wake up all CPUs again > >> after they are powered up (using the IMMEDIATE_WAKE bit in the CSR)? > >> > >> I haven't seen anything in the documentation regarding why unpowergating > >> after writing halt event registers wouldn't work. I'm sure I haven't > >> looked at all the documentation, but this is about as knowledgeable as I > >> am regarding the CPUs and the flow controller. Perhaps Peter will indeed > >> know more than that. > > > > Yes, more insights would indeed be welcome! > > Ping regarding this last open point. I'm sitting on v4 of this series - > v3 had a nasty bug in the CNTFRQ fixup, I addressed the reviews and > added further cleanups - and I would like to close this topic eventually. I applied your series (though I had to apply some patches manually because I was applying on top of the latest master branch where a bunch of Tegra-related files have moved around) and tested on Jetson TK1 as well as Dalmore (Tegra114). I have a couple of follow-up patches that shuffle around some code to share the PSCI implementation with Tegra114 since it should be compatible in that regard. One of the things I also did was try to use the more natural sequence that Stephen had pointed out. It turns out that, at least for me, that works just fine both on Tegra114 and Tegra124. Do you think you could look at the branch I uploaded and see if it works for you as well? One other thing I've had to do was enable CONFIG_ARMV7_NONSEC (I also threw in CONFIG_ARMV7_VIRT for good measure) so that the FDT would get updated, which is no longer the case with only CONFIG_ARMV7_PSCI. The last change I did was enable the SMMU from within U-Boot, which is necessary if the kernel runs in non-secure mode because some of the registers are secure-only. My branch is here: https://github.com/thierryreding/u-boot/commits/staging/psci It contains your v3 plus the above changes on top. I might have fumbled the rebase, so it might not actually build until somewhere near the top but I assumed you were going to do that anyway and therefore focused on other parts. Thierry
On 2015-02-24 09:18, Thierry Reding wrote: > On Tue, Feb 24, 2015 at 08:23:55AM +0100, Jan Kiszka wrote: >> On 2015-02-20 10:36, Jan Kiszka wrote: >>> On 2015-02-19 10:14, Thierry Reding wrote: >>>> On Wed, Feb 18, 2015 at 09:34:53AM -0700, Stephen Warren wrote: >>>>> On 02/17/2015 11:13 PM, Jan Kiszka wrote: >>>>>> On 2015-02-17 22:03, Stephen Warren wrote: >>>>>>> On 02/16/2015 05:54 AM, Jan Kiszka wrote: >>>>>>>> This is based on Thierry Reding's work and uses Ian Campell's >>>>>>>> preparatory patches. It comes with full support for CPU_ON/OFF PSCI >>>>>>>> services. The algorithm used in this version for turning CPUs on and >>>>>>>> off was proposed by Thierry Reding in >>>>>>>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It >>>>>>>> consists of first enabling CPU1..3 via the PMC, just to powergate them >>>>>>>> again with the help of the Flow Controller. Once the Flow Controller is >>>>>>>> in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF >>>>>>>> PSCI requests. >>>>>>> >>>>>>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c >>>>>>>> b/arch/arm/cpu/armv7/tegra124/ap.c >>>>>>> >>>>>>>> +void ap_pm_init(void) >>>>>>>> +{ >>>>>>>> + struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE; >>>>>>>> + struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE; >>>>>>>> + >>>>>>>> + writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR); >>>>>>>> + >>>>>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); >>>>>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); >>>>>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); >>>>>>>> + >>>>>>>> + writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr); >>>>>>>> + writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr); >>>>>>>> + writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr); >>>>>>>> + >>>>>>>> + writel(EVENT_MODE_STOP, &flow->halt_cpu1_events); >>>>>>>> + writel(EVENT_MODE_STOP, &flow->halt_cpu2_events); >>>>>>>> + writel(EVENT_MODE_STOP, &flow->halt_cpu3_events); >>>>>>> >>>>>>> I would expect to set up halt_cpu*_events before powering on the CPUs, >>>>>>> to make sure that they do the expected action on the very first WFI. So, >>>>>>> shouldn't the order above be: >>>>>>> >>>>>>> Write to halt_cpu*_events >>>>>>> Write to cpu*_csr >>>>>>> power_on >>>>>> >>>>>> Yeah, that was my original expectation as well. But >>>>>> >>>>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c >>>>>> index eebc0ea..240c71d 100644 >>>>>> --- a/arch/arm/cpu/armv7/tegra124/ap.c >>>>>> +++ b/arch/arm/cpu/armv7/tegra124/ap.c >>>>>> @@ -25,10 +25,6 @@ void ap_pm_init(void) >>>>>> >>>>>> writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR); >>>>>> >>>>>> - tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); >>>>>> - tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); >>>>>> - tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); >>>>>> - >>>>>> writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr); >>>>>> writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr); >>>>>> writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr); >>>>>> @@ -37,6 +33,10 @@ void ap_pm_init(void) >>>>>> writel(EVENT_MODE_STOP, &flow->halt_cpu2_events); >>>>>> writel(EVENT_MODE_STOP, &flow->halt_cpu3_events); >>>>>> >>>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); >>>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); >>>>>> + tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); >>>>>> + >>>>>> while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) | >>>>>> (1 << TEGRA_POWERGATE_CPU2) | >>>>>> (1 << TEGRA_POWERGATE_CPU3))) >>>>>> >>>>>> doesn't work in practice. I suspect the power-on overwrites what the >>>>>> flow controller configures in the PMC beforehand. But maybe someone can >>>>>> explain this better than me. >>>>> >>>>> Thierry, Peter, can you comment on why that is, and whether the original >>>>> code sequence is safe; does it matter that the target CPU executes WFI >>>>> before the flow controller is configured what to do on WFI? >>>> >>>> As I mentioned before, I don't think it's safe to change the powergate >>>> status of more than one partition at once. I'm not sure that this will >>> >>> tegra_powergate_set() already synchronizes the caller on the completion >>> of the switch. So the existing code is safe in this regard. >>> >>> However, the K1 manual also states that the START bit of the toggle >>> register should be checked prior to starting a request. This is not done >>> by tegra_powergate_set() - probably because it is a K1-only requirement, >>> not applying to older CPUs. Not sure, though, if waiting for START=0 is >>> practically required when already waiting for the switch to be processed >>> by the PMC before continuing. >>> >>>> change anything regarding the relative positioning of powergate on vs. >>>> writing CPU halt events, but I agree with Stephen that running the CPU >>>> without the halt events being programmed could cause them to simply go >>>> into a WFI without them actually being turned off. >>> >>> The CPUs most probably go into WFI first, because we wait for the >>> partition to be reported as powered up, but it seems they can be turned >>> off while in WFI as well. I'm not basing this on anything stated in the >>> manual, just on experiments. >>> >>>> >>>> Perhaps if unpowergating after writing the halt events registers doesn't >>>> work a safer way would be to go and forcibly wake up all CPUs again >>>> after they are powered up (using the IMMEDIATE_WAKE bit in the CSR)? >>>> >>>> I haven't seen anything in the documentation regarding why unpowergating >>>> after writing halt event registers wouldn't work. I'm sure I haven't >>>> looked at all the documentation, but this is about as knowledgeable as I >>>> am regarding the CPUs and the flow controller. Perhaps Peter will indeed >>>> know more than that. >>> >>> Yes, more insights would indeed be welcome! >> >> Ping regarding this last open point. I'm sitting on v4 of this series - >> v3 had a nasty bug in the CNTFRQ fixup, I addressed the reviews and >> added further cleanups - and I would like to close this topic eventually. > > I applied your series (though I had to apply some patches manually > because I was applying on top of the latest master branch where a bunch > of Tegra-related files have moved around) and tested on Jetson TK1 as > well as Dalmore (Tegra114). I have a couple of follow-up patches that > shuffle around some code to share the PSCI implementation with Tegra114 > since it should be compatible in that regard. Can you use v4 on github instead (https://github.com/siemens/u-boot/commits/jetson-tk1-v4)? It is based on today's master and known to work better than v3. > > One of the things I also did was try to use the more natural sequence > that Stephen had pointed out. It turns out that, at least for me, that > works just fine both on Tegra114 and Tegra124. Do you think you could > look at the branch I uploaded and see if it works for you as well? One > other thing I've had to do was enable CONFIG_ARMV7_NONSEC (I also threw > in CONFIG_ARMV7_VIRT for good measure) so that the FDT would get > updated, which is no longer the case with only CONFIG_ARMV7_PSCI. The > last change I did was enable the SMMU from within U-Boot, which is > necessary if the kernel runs in non-secure mode because some of the > registers are secure-only. > > My branch is here: > > https://github.com/thierryreding/u-boot/commits/staging/psci > > It contains your v3 plus the above changes on top. I might have fumbled > the rebase, so it might not actually build until somewhere near the top > but I assumed you were going to do that anyway and therefore focused on > other parts. I'll look into your changes in parallel. Thanks, Jan
diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c index eebc0ea..240c71d 100644 --- a/arch/arm/cpu/armv7/tegra124/ap.c +++ b/arch/arm/cpu/armv7/tegra124/ap.c @@ -25,10 +25,6 @@ void ap_pm_init(void) writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR); - tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); - tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); - tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); - writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr); writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr); writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr); @@ -37,6 +33,10 @@ void ap_pm_init(void) writel(EVENT_MODE_STOP, &flow->halt_cpu2_events); writel(EVENT_MODE_STOP, &flow->halt_cpu3_events); + tegra_powergate_power_on(TEGRA_POWERGATE_CPU1); + tegra_powergate_power_on(TEGRA_POWERGATE_CPU2); + tegra_powergate_power_on(TEGRA_POWERGATE_CPU3); + while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) | (1 << TEGRA_POWERGATE_CPU2) | (1 << TEGRA_POWERGATE_CPU3)))