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 -- Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
