[U-Boot] [PATCH v2 09/12] tegra124: Add PSCI support for Tegra124

Tue Feb 24 09:23:43 CET 2015

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