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

[Thierry Reding <treding@nvidia.com>]

On Fri, Jan 23, 2015 at 12:37:20PM +0000, Mark Rutland wrote:
> On Fri, Jan 23, 2015 at 10:10:45AM +0000, Thierry Reding wrote:
> > On Thu, Jan 22, 2015 at 07:20:15PM +0000, Mark Rutland wrote:
> > > On Fri, Jan 16, 2015 at 09:12:59AM +0000, Thierry Reding wrote:
> > > > On Thu, Jan 15, 2015 at 07:19:37PM +0000, Mark Rutland wrote:
> > > > > On Wed, Jan 14, 2015 at 07:57:25AM +0000, Thierry Reding wrote:
> > > > > > On Tue, Jan 13, 2015 at 07:44:50PM +0000, Ian Campbell wrote:
> > > > > > > Hi Thierry,
> > > > > > > 
> > > > > > > I needed to boot my Jetson in NS mode (in order to boot Xen) and was
> > > > > > > investigating the possibility of PSCI support when I discovered that you
> > > > > > > had already started on it[0]. Hurrah!
> > > > > > > 
> > > > > > > I cherry-picked the relevant commit onto u-boot-tegra#master and added a
> > > > > > > few more patches and now it boots correctly for me, both running Xen
> > > > > > > (some Xen side patches are needed too) and native Linux.
> > > > > > > 
> > > > > > > The main things which was needed was to rebase for some recent Kconfig
> > > > > > > changes relating to virt and nonsec mode and to arrange for the RAM used
> > > > > > > by the secure code to be reserved in the FDT. I also reserved the RAM
> > > > > > > using the hardware MC_SECURITY_CFG registers for good measure.
> > > > > > 
> > > > > > Great, those were all things that I had wanted to do but never got
> > > > > > around to.
> > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > We have a generic CPUidle driver on arm64 which can use PSCI as a
> > > > > backend; we should try to reuse that. The binding should certainly be
> > > > > identical.
> > > > 
> > > > Is there any reason that driver needs to be ARM64-specific? I would've
> > > > thought that there could be a generic PSCI driver that works anywhere.
> > > 
> > > Currently the arm and arm64 arch interfaces are a little different, but
> > > with some work the bulk of the code could certainly be made common
> > > (in drivers/firmware, perhaps).
> > > 
> > > > > It looks like the tegra124 dts in mainline doesn't use enable-method in
> > > > > the DT, so a better option might be to fail early in cpuidle-tegra114.c 
> > > > > if _any_ enable-method is present.
> > > > 
> > > > Yes, that sounds like a good plan. The absence of an enable-method would
> > > > signal that a kernel-native method (if any) should be used.
> > > > 
> > > > And this reminds me that we still need to find a way to synchronize
> > > > accesses to the powergate registers between secure firmware and the
> > > > kernel. Tegra has a set of hardware semaphores, but it seems like those
> > > > can only be used to synchronize between AVP and CPU, whereas for PSCI
> > > > we'd need something to synchronize between two CPUs. Do you know of any
> > > > existing mechanism to perform that type of synchronization?
> > > > 
> > > > Perhaps an option would be to add some sort of global lock in the kernel
> > > > which the cpuidle driver can grab before issuing the SMC instruction.
> > > 
> > > PSCI assumes that the FW is in full control of the registers it's
> > > poking. While a lock isn't necessarily bad, I suspect it's going to be
> > > very difficult to have that common across all users without the code
> > > becoming unmaintainable fast. I'd also hope that for arm64 we wouldn't
> > > need it.
> > > 
> > > When/how/why does the kernel to poke these registers?
> > 
> > The PMC is what controls power partitions. Some of these partitions are
> > assigned to CPUs, others are assigned to things like SATA, PCIe, display
> > and so on. The problem is that if we manage the CPU power partitions via
> > the firmware, then they will conflict with calls that we need to make
> > from other drivers that need to gate or ungate the partitions for their
> > hardware. As I understand it there's no provision in PSCI to manage non-
> > CPU devices, so this is a problem.
> 
> Ok.
> 
> > So I think either firmware needs to control everything, in which case we
> > are going to need a new interface (or extend PSCI) or it mustn't control
> > anything, in which case we need custom code in the kernel for SMP. Well,
> > the other alternative would be the lock that we can grab in the
> > powergate API and the PSCI calls.
> 
> One reason I'm not so keen on a lock is I could imagine you'd need to
> grab this for CPU_SUSPEND calls (i.e. cpuidle), at which point all CPUs
> are going to contend for the lock all the time.
> 
> One thing to bear in mind is that PSCI is only one user of the SMC
> space. Per SMC calling convention, portions of the SMC ID space are
> there to be used for other (vendor-specific) purposes.
> 
> So rather than extending PSCI, a parallel API could be implemented for
> power control of other devices, and the backend could arbitrate the two
> without the non-secure OS requiring implementation-specific mutual
> exclusion.
> 
> I think this has been brought up internally previously; I'll go and poke
> around in the area to see if we managed to figure out anything useful.

Okay, I think we'll need to coordinate to provide a common interface for
the kernel to talk to firmware.

> > Unfortunately this doesn't change on 64-bit Tegra at all.
> 
> I suspected as much. :/
> 
> How does this bode for the tegra132 dts [1] on LAKML at the moment? Is
> it just the "nvidia,tegra132-pmc" device that needs to be poked by both
> FW and kernel, or are other devices involved?
> 
> Thanks,
> Mark.
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/317013.html

Cc'ing Peter and Paul who might be more familiar with SMP.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150130/640bb8b5/attachment.sig>