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

[Thierry Reding]

On Thu, Feb 05, 2015 at 12:37:39PM +0000, Mark Rutland wrote:
> On Thu, Feb 05, 2015 at 11:44:25AM +0000, Thierry Reding wrote:
> > 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.
> > 
> > We've had some discussions about this internally and I think this should
> > not be a problem after all. The Tegra flow controller can be programmed
> > to automatically coordinate with the PMC to powergate CPUs when they
> > encounter a WFI instruction and unpowergate CPUs again when they are
> > woken up. With that in place the PMC registers don't need to be touched
> > anymore once the CPUs have been booted once.
> > 
> > The solution that was discussed internally would involve having the
> > secure monitor (U-Boot's PSCI implementation in this case) program the
> > flow controller appropriately, point the CPU reset vectors to a location
> > containing a WFI instruction and power up the CPUs. That way they should
> > immediately be powergated when they reach the WFI instruction and the
> > PSCI implementation would then be able to wake them up without accessing
> > the PMC registers once the kernel has booted.
> 
> That sounds far, far better than I had hoped!
> 
> I guess we need to tell the kernel that portions of the PMC are reserved
> by FW (in the sense that they must not be modified by the kernel rather
> than that FW is going to poke them), to avoid mishaps.

I'm not sure we need even that. As I understand it the kernel can still
touch all the registers and none of it should influence the CPU power-
gating done by the secure monitor.

Well, I guess you'd need to make sure that the PMC driver doesn't try to
powergate or unpowergate the CPU partitions, but since the cpuidle
driver is the only one doing that it should resolve itself if a generic,
PSCI-based cpuidle driver takes over instead of a Tegra-specific one.

> > Adding Peter. Please correct me if I misunderstood what we discussed.
> > Can you also provide Ian with pointers to the registers that need to be
> > programmed to make this work? I suspect that a lot of it can be gleaned
> > from the cpuidle drivers in arch/arm/mach-tegra in the upstream Linux
> > kernel.
> > 
> > Also adding Paul for visibility.
> > 
> > > 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.
> > > 
> > > > 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?
> > 
> > As I understand it, only the flow controller is involved with CPU power
> > management once the above steps have been performed by the secure
> > monitor. And I don't think anyone in the kernel would need access to the
> > flow controller at that point either, so I think that problem resolved
> > itself nicely.
> > 
> > Also note that the above should work as far back as Tegra30.
> 
> It would be amazing if we could gain PSCI for all the platforms that
> covers!

It should be relatively easy to support at least Tegra114 with much the
same code as Tegra124, and some slight changes on Tegra30. But yeah, it
would be great to see this work.

Ian, are you planning on revising the series based on the outcome above?

Thierry

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
-------------- 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/20150205/1e190257/attachment.sig>