[U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI

[Ian Campbell]

On Fri, 2015-01-16 at 11:05 +0100, Thierry Reding wrote:
> On Fri, Jan 16, 2015 at 09:43:22AM +0000, Ian Campbell wrote:
> > On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote:
> > > On Wed, Jan 14, 2015 at 08:58:41AM +0000, Ian Campbell wrote:
> > > > On Wed, 2015-01-14 at 08:57 +0100, Thierry Reding wrote:
> > > > > > I also pushed my tree to gitorious:
> > > > > >         https://gitorious.org/ijc/u-boot jetson-psci-v1
> > > > > > 
> > > > > > I would Ack your patch, but I don't think you've posted it and it has no
> > > > > > S-o-b so that would seem a bit premature/rude of me. For the same reason
> > > > > > I've not actually included it in the series posted (but it is in the
> > > > > > gitorious branch).
> > > > > 
> > > > > Feel free to take ownership of that patch. I currently don't have the
> > > > > time to work on this and it seems you've made good progress on it.
> > > > 
> > > > Will do. Could you offer a S-o-b for it please so I can pick it up.
> > > 
> > > Sure:
> > > 
> > > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > 
> > Thanks!
> > 
> > > > > It could probably use some cleanup because there's a bit of debug output
> > > > > still in there. Also...
> > > > > 
> > > > > > FWIW I think you could drop your stub versions of psci_cpu_off and
> > > > > > psci_cpu_suspend (assuming you don't want to implement them) since the
> > > > > > common code has stubs.
> > > > > 
> > > > > ... I'd think you'd need to implement these so that you can get proper
> > > > > suspend/resume support in the kernel. I've had to disable cpuidle (via
> > > > > #undef CONFIG_PM_SLEEP in arch/arm/mach-tegra/cpuidle-tegra114.c) in the
> > > > > kernel to make that code not powergate CPUs. Ideally I think the kernel
> > > > > would check that it's running with PSCI support and disable the cpuidle
> > > > > driver. Maybe that could be done by introducing a new cpuidle driver
> > > > > that checks for PSCI availability and uses it when present.
> > > > 
> > > > Hrm, I'm not sure how this all fits together, it's not a problem I've
> > > > noted before.
> > > > 
> > > > FWIW I think cpu_off and cpu_suspend are optional in PSCI v0.1 so an
> > > > initial version doesn't necessarily need to implement them (sunxi
> > > > doesn't for example), but as you say they do enable useful features.
> > > 
> > > I think when I tried last time, without disable the cpuidle driver
> > > things would hang at boot. I would expect that problem to exist for
> > > any board. Perhaps you've disabled PM_SLEEP in your config?
> > 
> > I don't think so:
> > # grep PM_SLEEP /boot/config-3.18.0-trunk-armmp-lpae 
> > CONFIG_PM_SLEEP=y
> > CONFIG_PM_SLEEP_SMP=y
> > CONFIG_PM_SLEEP_DEBUG=y
> > 
> > I don't see anything about cpuidle in dmesg either.
> > 
> > Did you perhaps mean CPU_IDLE rather than PM_SLEEP because:
> > # CONFIG_CPU_IDLE is not set
> 
> Yes, I think that would have the same effect as disable PM_SLEEP (at
> least regarding the powergate stuff that's conflicting with the PSCI
> implementation).
> 
> Note also, as mentioned in another reply, that with the PSCI support
> there's now two sources that can simultaneously access the powergate
> functionality in the PMC. We have some locking in place to make sure
> that concurrent accesses from within the kernel are serialized, but
> there's no mechanism in place to protect from concurrent accesses in
> secure firmware and the kernel.

The docs are on another machine, but I take it the PMC registers are
available to NS mode? Is that configurable (from S mode) perhaps?

> I don't have any good ideas on how to solve this nicely. The best I
> could come up with is to make sure that we grab a lock before doing
> any PSCI calls from the kernel and release the lock upon return.

Ian.