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

[Jan Kiszka <jan.kiszka@siemens.com>]

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

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux