[U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
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. 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). 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. Albert, I've CCd you on a couple of patches which touch common ARM code. Nothing too major I think. Cheers, Ian. [0] https://github.com/thierryreding/u-boot/commit/5996d2b0dea2953ae8881f40b7f85b6fb8c50791 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Fri, Jan 23, 2015 at 12:37:20PM +, Mark Rutland wrote: > On Fri, Jan 23, 2015 at 10:10:45AM +, Thierry Reding wrote: > > On Thu, Jan 22, 2015 at 07:20:15PM +, Mark Rutland wrote: > > > On Fri, Jan 16, 2015 at 09:12:59AM +, Thierry Reding wrote: > > > > On Thu, Jan 15, 2015 at 07:19:37PM +, Mark Rutland wrote: > > > > > On Wed, Jan 14, 2015 at 07:57:25AM +, Thierry Reding wrote: > > > > > > On Tue, Jan 13, 2015 at 07:44:50PM +, 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
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Thu, 2015-02-05 at 12:44 +0100, Thierry Reding wrote: > 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. > > 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. Thanks, any *info would be good even though I could likely decode it from the driver... I'm afraid I've been a bit slack about updating the series with the other comments, mostly because I've been procrastinating over this powergate issue. Ian. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Thu, Feb 05, 2015 at 11:44:25AM +, Thierry Reding wrote: > On Fri, Jan 23, 2015 at 12:37:20PM +, Mark Rutland wrote: > > On Fri, Jan 23, 2015 at 10:10:45AM +, Thierry Reding wrote: > > > On Thu, Jan 22, 2015 at 07:20:15PM +, Mark Rutland wrote: > > > > On Fri, Jan 16, 2015 at 09:12:59AM +, Thierry Reding wrote: > > > > > On Thu, Jan 15, 2015 at 07:19:37PM +, Mark Rutland wrote: > > > > > > On Wed, Jan 14, 2015 at 07:57:25AM +, Thierry Reding wrote: > > > > > > > On Tue, Jan 13, 2015 at 07:44:50PM +, 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 sy
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Thu, Feb 05, 2015 at 12:37:39PM +, Mark Rutland wrote: > On Thu, Feb 05, 2015 at 11:44:25AM +, Thierry Reding wrote: > > On Fri, Jan 23, 2015 at 12:37:20PM +, Mark Rutland wrote: > > > On Fri, Jan 23, 2015 at 10:10:45AM +, Thierry Reding wrote: > > > > On Thu, Jan 22, 2015 at 07:20:15PM +, Mark Rutland wrote: > > > > > On Fri, Jan 16, 2015 at 09:12:59AM +, Thierry Reding wrote: > > > > > > On Thu, Jan 15, 2015 at 07:19:37PM +, Mark Rutland wrote: > > > > > > > On Wed, Jan 14, 2015 at 07:57:25AM +, Thierry Reding wrote: > > > > > > > > On Tue, Jan 13, 2015 at 07:44:50PM +, 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
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Thu, 2015-02-05 at 14:55 +0100, Thierry Reding wrote: > Ian, are you planning on revising the series based on the outcome above? Yes, although I don't have any 114 or 30 hardware, just a single Jetson. (there should probably be an "eventually" near the start of that sentence, I'm travelling for the next 10 days and don't have remote power control...) Ian. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
[...] > > > 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. This was my concern. It would be good to avoid a case where we accidentally rely on some subtle interactiion where both the FW and kernel poke some registers in a particular way. I guess we can check for the presence of an enable-method, and if there is one don't register the Tegra-specific cpuidle driver; in that case we expect the FW to own that side of things. > > > 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. Nice! I should look into getting hold of a relevant platform; I only have a (T20) AC100, and I guess that's a bit different at the system-level. Thanks, Mark. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On 2015-02-09 12:26, Mark Rutland wrote: > [...] > 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. > > This was my concern. It would be good to avoid a case where we > accidentally rely on some subtle interactiion where both the FW and > kernel poke some registers in a particular way. > > I guess we can check for the presence of an enable-method, and if there > is one don't register the Tegra-specific cpuidle driver; in that case we > expect the FW to own that side of things. > 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. > > Nice! > > I should look into getting hold of a relevant platform; I only have a > (T20) AC100, and I guess that's a bit different at the system-level. To avoid duplicate work: I started to implement the suggested algorithm on the TK1. Just like Ian, I don't have access to any other platform (nor docs at hand), so I will stick with Tegra124 support for the first step. I suppose we can easily extend this later on. Flow controller setup and the PSCI service CPU_ON already works. I'll post patches once CPU_OFF is working as well. Jan signature.asc Description: OpenPGP digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Sat, 2015-02-14 at 16:08 +0100, Jan Kiszka wrote: > To avoid duplicate work: I started to implement the suggested algorithm > on the TK1. Just like Ian, I don't have access to any other platform > (nor docs at hand), so I will stick with Tegra124 support for the first > step. I suppose we can easily extend this later on. After a conference, illness and vacation back to back I'm waaay behind on my mail. I see there have been several iterations of this series now, thanks for picking it up! Ian. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Tue, Jan 13, 2015 at 07:44:50PM +, 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. Adding Stephen and Tom for visibility. Thanks, Thierry pgpbgswpAarz8.pgp Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
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. > 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. > Adding Stephen and Tom for visibility. Oops, sorry, I got them for the patches (via docs/git-mailrc) but forgot about the cover letter. Ian. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Wed, Jan 14, 2015 at 08:58:41AM +, 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 > > 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? Thierry pgpyuHDJ7WLNE.pgp Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Wed, Jan 14, 2015 at 07:57:25AM +, Thierry Reding wrote: > On Tue, Jan 13, 2015 at 07:44:50PM +, 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. 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. Thanks, Mark. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Thu, Jan 15, 2015 at 07:19:37PM +, Mark Rutland wrote: > On Wed, Jan 14, 2015 at 07:57:25AM +, Thierry Reding wrote: > > On Tue, Jan 13, 2015 at 07:44:50PM +, 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. > 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. Thierry pgptEci1sW7k8.pgp Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote: > On Wed, Jan 14, 2015 at 08:58:41AM +, 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 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 Ian. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Fri, Jan 16, 2015 at 09:43:22AM +, Ian Campbell wrote: > On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote: > > On Wed, Jan 14, 2015 at 08:58:41AM +, 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 > > 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. 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. Thierry pgpzDrWmUq0Hr.pgp Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Fri, 2015-01-16 at 11:05 +0100, Thierry Reding wrote: > On Fri, Jan 16, 2015 at 09:43:22AM +, Ian Campbell wrote: > > On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote: > > > On Wed, Jan 14, 2015 at 08:58:41AM +, 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 > > > > 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Fri, Jan 16, 2015 at 10:24:03AM +, Ian Campbell wrote: > On Fri, 2015-01-16 at 11:05 +0100, Thierry Reding wrote: > > On Fri, Jan 16, 2015 at 09:43:22AM +, Ian Campbell wrote: > > > On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote: > > > > On Wed, Jan 14, 2015 at 08:58:41AM +, 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 > > > > > > 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 see how that's relevant. Even if it was possible to secure the registers against access from NS mode, there's no way we could do that because many drivers rely on controlling their power domain using that functionality. Or perhaps I misunderstand what you're suggesting. Thierry pgp65L97dH0Ur.pgp Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Fri, 2015-01-16 at 17:03 +0100, Thierry Reding wrote: > On Fri, Jan 16, 2015 at 10:24:03AM +, Ian Campbell wrote: > > On Fri, 2015-01-16 at 11:05 +0100, Thierry Reding wrote: > > > On Fri, Jan 16, 2015 at 09:43:22AM +, Ian Campbell wrote: > > > > On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote: > > > > > On Wed, Jan 14, 2015 at 08:58:41AM +, 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 > > > > > > > > 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 see how that's relevant. Even if it was possible to secure the > registers against access from NS mode, there's no way we could do that > because many drivers rely on controlling their power domain using that > functionality. > > Or perhaps I misunderstand what you're suggesting. If the PMC registers aren't available to NS then the damage the powergate driver can do by conflicting with PSCI is more limited i.e not going to fry the h/w somehow. Ian. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Fri, Jan 16, 2015 at 04:11:19PM +, Ian Campbell wrote: > On Fri, 2015-01-16 at 17:03 +0100, Thierry Reding wrote: > > On Fri, Jan 16, 2015 at 10:24:03AM +, Ian Campbell wrote: > > > On Fri, 2015-01-16 at 11:05 +0100, Thierry Reding wrote: > > > > On Fri, Jan 16, 2015 at 09:43:22AM +, Ian Campbell wrote: > > > > > On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote: > > > > > > On Wed, Jan 14, 2015 at 08:58:41AM +, 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 > > > > > > > > > > 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 see how that's relevant. Even if it was possible to secure the > > registers against access from NS mode, there's no way we could do that > > because many drivers rely on controlling their power domain using that > > functionality. > > > > Or perhaps I misunderstand what you're suggesting. > > If the PMC registers aren't available to NS then the damage the > powergate driver can do by conflicting with PSCI is more limited i.e not > going to fry the h/w somehow. As far as I can tell these register
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Mon, 2015-01-19 at 10:25 +0100, Thierry Reding wrote: > On Fri, Jan 16, 2015 at 04:11:19PM +, Ian Campbell wrote: > > On Fri, 2015-01-16 at 17:03 +0100, Thierry Reding wrote: > > > On Fri, Jan 16, 2015 at 10:24:03AM +, Ian Campbell wrote: > > > > On Fri, 2015-01-16 at 11:05 +0100, Thierry Reding wrote: > > > > > On Fri, Jan 16, 2015 at 09:43:22AM +, Ian Campbell wrote: > > > > > > On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote: > > > > > > > On Wed, Jan 14, 2015 at 08:58:41AM +, 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 > > > > > > > > > > > > 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 see how that's relevant. Even if it was possible to secure the > > > registers against access from NS mode, there's no way we could do that > > > because many drivers rely on controlling their power domain using that > >
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Fri, Jan 16, 2015 at 09:12:59AM +, Thierry Reding wrote: > On Thu, Jan 15, 2015 at 07:19:37PM +, Mark Rutland wrote: > > On Wed, Jan 14, 2015 at 07:57:25AM +, Thierry Reding wrote: > > > On Tue, Jan 13, 2015 at 07:44:50PM +, 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? Mark. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Thu, Jan 22, 2015 at 07:20:15PM +, Mark Rutland wrote: > On Fri, Jan 16, 2015 at 09:12:59AM +, Thierry Reding wrote: > > On Thu, Jan 15, 2015 at 07:19:37PM +, Mark Rutland wrote: > > > On Wed, Jan 14, 2015 at 07:57:25AM +, Thierry Reding wrote: > > > > On Tue, Jan 13, 2015 at 07:44:50PM +, 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,
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Fri, Jan 23, 2015 at 10:10:45AM +, Thierry Reding wrote: > On Thu, Jan 22, 2015 at 07:20:15PM +, Mark Rutland wrote: > > On Fri, Jan 16, 2015 at 09:12:59AM +, Thierry Reding wrote: > > > On Thu, Jan 15, 2015 at 07:19:37PM +, Mark Rutland wrote: > > > > On Wed, Jan 14, 2015 at 07:57:25AM +, Thierry Reding wrote: > > > > > On Tue, Jan 13, 2015 at 07:44:50PM +, 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 t
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
[...] > > > 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. It sounds like what we figured out internally is roughly what I stated above: Allocate some SMC calls in the SIP and/or OEM Service Calls range for vendor-specific device power management, and have the implementation on the secure side (which would do the actual register poking) arbitrate with any other secure-side access to those registers (i.e. CPU power management, which it will already have to arbitrate). Thanks, Mark. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 0/4] Jetson-TK1 support for PSCI
On Fri, Jan 23, 2015 at 12:37:20PM +, Mark Rutland wrote: > On Fri, Jan 23, 2015 at 10:10:45AM +, Thierry Reding wrote: > > On Thu, Jan 22, 2015 at 07:20:15PM +, Mark Rutland wrote: > > > On Fri, Jan 16, 2015 at 09:12:59AM +, Thierry Reding wrote: > > > > On Thu, Jan 15, 2015 at 07:19:37PM +, Mark Rutland wrote: > > > > > On Wed, Jan 14, 2015 at 07:57:25AM +, Thierry Reding wrote: > > > > > > On Tue, Jan 13, 2015 at 07:44:50PM +, 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
