[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/5996d2b0dea2953ae8881f40b7f85...
I will need mc_security_cfg0/1 in a future patch and I added the rest while debugging, so thought I might as well commit them.
Signed-off-by: Ian Campbell ijc@hellion.org.uk --- arch/arm/include/asm/arch-tegra124/mc.h | 35 +++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-)
diff --git a/arch/arm/include/asm/arch-tegra124/mc.h b/arch/arm/include/asm/arch-tegra124/mc.h index d526dfe..5557732 100644 --- a/arch/arm/include/asm/arch-tegra124/mc.h +++ b/arch/arm/include/asm/arch-tegra124/mc.h @@ -35,9 +35,40 @@ struct mc_ctlr { u32 mc_emem_adr_cfg; /* offset 0x54 */ u32 mc_emem_adr_cfg_dev0; /* offset 0x58 */ u32 mc_emem_adr_cfg_dev1; /* offset 0x5C */ - u32 reserved3[12]; /* offset 0x60 - 0x8C */ + u32 reserved3[4]; /* offset 0x60 - 0x6C */ + u32 mc_security_cfg0; /* offset 0x70 */ + u32 mc_security_cfg1; /* offset 0x74 */ + u32 reserved4[6]; /* offset 0x7C - 0x8C */ u32 mc_emem_arb_reserved[28]; /* offset 0x90 - 0xFC */ - u32 reserved4[338]; /* offset 0x100 - 0x644 */ + u32 reserved5[74]; /* offset 0x100 - 0x224 */ + u32 mc_smmu_translation_enable_0; /* offset 0x228 */ + u32 mc_smmu_translation_enable_1; /* offset 0x22C */ + u32 mc_smmu_translation_enable_2; /* offset 0x230 */ + u32 mc_smmu_translation_enable_3; /* offset 0x234 */ + u32 mc_smmu_afi_asid; /* offset 0x238 */ + u32 mc_smmu_avpc_asid; /* offset 0x23C */ + u32 mc_smmu_dc_asid; /* offset 0x240 */ + u32 mc_smmu_dcb_asid; /* offset 0x244 */ + u32 reserved6[2]; /* offset 0x248 - 0x24C */ + u32 mc_smmu_hc_asid; /* offset 0x250 */ + u32 mc_smmu_hda_asid; /* offset 0x254 */ + u32 mc_smmu_isp2_asid; /* offset 0x258 */ + u32 reserved7[2]; /* offset 0x25C - 0x260 */ + u32 mc_smmu_msenc_asid; /* offset 0x264 */ + u32 mc_smmu_nv_asid; /* offset 0x268 */ + u32 mc_smmu_nv2_asid; /* offset 0x26C */ + u32 mc_smmu_ppcs_asid; /* offset 0x270 */ + u32 mc_smmu_sata_asid; /* offset 0x274 */ + u32 reserved8[1]; /* offset 0x278 */ + u32 mc_smmu_vde_asid; /* offset 0x27C */ + u32 mc_smmu_vi_asid; /* offset 0x280 */ + u32 mc_smmu_vic_asid; /* offset 0x284 */ + u32 mc_smmu_xusb_host_asid; /* offset 0x288 */ + u32 mc_smmu_xusb_dev_asid; /* offset 0x28C */ + u32 reserved9[1]; /* offset 0x290 */ + u32 mc_smmu_tsec_asid; /* offset 0x294 */ + u32 mc_smmu_ppcs1_asid; /* offset 0x298 */ + u32 reserved10[235]; /* offset 0x29C - 0x644 */ u32 mc_video_protect_bom; /* offset 0x648 */ u32 mc_video_protect_size_mb; /* offset 0x64c */ u32 mc_video_protect_reg_ctrl; /* offset 0x650 */
On 01/13/2015 12:45 PM, Ian Campbell wrote:
I will need mc_security_cfg0/1 in a future patch and I added the rest while debugging, so thought I might as well commit them.
diff --git a/arch/arm/include/asm/arch-tegra124/mc.h b/arch/arm/include/asm/arch-tegra124/mc.h
@@ -35,9 +35,40 @@ struct mc_ctlr { u32 mc_emem_adr_cfg; /* offset 0x54 */ u32 mc_emem_adr_cfg_dev0; /* offset 0x58 */ u32 mc_emem_adr_cfg_dev1; /* offset 0x5C */
- u32 reserved3[12]; /* offset 0x60 - 0x8C */
- u32 reserved3[4]; /* offset 0x60 - 0x6C */
- u32 mc_security_cfg0; /* offset 0x70 */
I didn't check the fields you've added, but this patch sounds fine.
One thing I did wonder though: rather than naming the reserved registers 1, 2, 3, 4, ... (which must be renumbered any time we add new registers in the middle of a previously reserved section), perhaps we should name the reserved registers after the base offset they cover, e.g.:
- u32 reserved1[3]; /* offset 0x24 - 0x2C */
- u32 reserved24[3]; /* offset 0x24 - 0x2C */ u32 mc_smmu_tlb_flush; /* offset 0x30 */ u32 mc_smmu_ptc_flush; /* offset 0x34 */
- u32 reserved2[6]; /* offset 0x38 - 0x4C */
- u32 reserved38[6]; /* offset 0x38 - 0x4C */
I presume this isn't necessary for the T124 header since I presume you've filled out everything from the docs, but perhaps it'd make it easier to extend this header to future chips assuming the MC register layout is similar but modified there?
On Thu, 2015-01-15 at 16:37 -0700, Stephen Warren wrote:
On 01/13/2015 12:45 PM, Ian Campbell wrote:
I will need mc_security_cfg0/1 in a future patch and I added the rest while debugging, so thought I might as well commit them.
diff --git a/arch/arm/include/asm/arch-tegra124/mc.h b/arch/arm/include/asm/arch-tegra124/mc.h
@@ -35,9 +35,40 @@ struct mc_ctlr { u32 mc_emem_adr_cfg; /* offset 0x54 */ u32 mc_emem_adr_cfg_dev0; /* offset 0x58 */ u32 mc_emem_adr_cfg_dev1; /* offset 0x5C */
- u32 reserved3[12]; /* offset 0x60 - 0x8C */
- u32 reserved3[4]; /* offset 0x60 - 0x6C */
- u32 mc_security_cfg0; /* offset 0x70 */
I didn't check the fields you've added, but this patch sounds fine.
One thing I did wonder though: rather than naming the reserved registers 1, 2, 3, 4, ... (which must be renumbered any time we add new registers in the middle of a previously reserved section), perhaps we should name the reserved registers after the base offset they cover,
That's a good idea, I'll do that for the header I'm touching at least.
e.g.:
- u32 reserved1[3]; /* offset 0x24 - 0x2C */
- u32 reserved24[3]; /* offset 0x24 - 0x2C */ u32 mc_smmu_tlb_flush; /* offset 0x30 */ u32 mc_smmu_ptc_flush; /* offset 0x34 */
- u32 reserved2[6]; /* offset 0x38 - 0x4C */
- u32 reserved38[6]; /* offset 0x38 - 0x4C */
I presume this isn't necessary for the T124 header since I presume you've filled out everything from the docs,
Up to the register I was interested in yes (at least I think so), but there are more past where I stopped, I think your suggestion makes good sense nonetheless.
The is another MC_SECURITY register further up which covers the base address bits 32 onwards, it resets to zero so since the secure region is <4G I don't strictly need to configure it, but I've been considering setting it any way for belt and braces reasons.
but perhaps it'd make it easier to extend this header to future chips assuming the MC register layout is similar but modified there?
Sounds plausible to me.
Ian.
In this case the secure code lives in RAM, and hence needs to be reserved, but it has been relocated, so the reservation of __secure_start does not apply.
Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a region.
This will be used in a subsequent patch for Jetson-TK1
Signed-off-by: Ian Campbell ijc@hellion.org.uk Cc: Albert Aribaud albert.u.boot@aribaud.net --- arch/arm/cpu/armv7/virt-dt.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c index ad19e4c..eb95031 100644 --- a/arch/arm/cpu/armv7/virt-dt.c +++ b/arch/arm/cpu/armv7/virt-dt.c @@ -96,6 +96,11 @@ int armv7_update_dt(void *fdt) /* secure code lives in RAM, keep it alive */ fdt_add_mem_rsv(fdt, (unsigned long)__secure_start, __secure_end - __secure_start); +#elif defined(CONFIG_ARMV7_SECURE_RESERVE_SIZE) + /* secure code has been relocated into RAM carveout, keep it alive */ + fdt_add_mem_rsv(fdt, + CONFIG_ARMV7_SECURE_BASE, + CONFIG_ARMV7_SECURE_RESERVE_SIZE); #endif
return fdt_psci(fdt);
On 01/13/2015 12:45 PM, Ian Campbell wrote:
In this case the secure code lives in RAM, and hence needs to be reserved, but it has been relocated, so the reservation of __secure_start does not apply.
Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a region.
This will be used in a subsequent patch for Jetson-TK1
It's rather hard to review this without any documentation in the README, of the new symbol, or any of the existing:
CONFIG_ARMV7_NONSEC CONFIG_ARMV7_VIRT CONFIG_ARMV7_PSCI
It'd be nice to have a description of what those do exactly, and how they interact or conflict.
On Thu, 2015-01-15 at 16:49 -0700, Stephen Warren wrote:
On 01/13/2015 12:45 PM, Ian Campbell wrote:
In this case the secure code lives in RAM, and hence needs to be reserved, but it has been relocated, so the reservation of __secure_start does not apply.
Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a region.
This will be used in a subsequent patch for Jetson-TK1
It's rather hard to review this without any documentation in the README, of the new symbol, or any of the existing:
CONFIG_ARMV7_NONSEC CONFIG_ARMV7_VIRT CONFIG_ARMV7_PSCI
It'd be nice to have a description of what those do exactly, and how they interact or conflict.
Ack, I'll see what I can do about the existing ones too.
Ian.
On Thu, 2015-01-15 at 16:49 -0700, Stephen Warren wrote:
On 01/13/2015 12:45 PM, Ian Campbell wrote:
In this case the secure code lives in RAM, and hence needs to be reserved, but it has been relocated, so the reservation of __secure_start does not apply.
Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a region.
This will be used in a subsequent patch for Jetson-TK1
It's rather hard to review this without any documentation in the README, of the new symbol, or any of the existing:
CONFIG_ARMV7_NONSEC CONFIG_ARMV7_VIRT CONFIG_ARMV7_PSCI
It'd be nice to have a description of what those do exactly, and how they interact or conflict.
Anyone got any opinions in the new Kconfig-world regarding whether it is more or less appropriate to use the help section of the Kconfig file vs. the README file?
...and only now do I notice that the first two of the three options mentioned above are already documented in the Kconfig and the third (PSCI) isn't a Kconfig option (yet?) so there's no ambiguity about where it should be put.
I also just noticed that the README says "Later we will add a configuration tool - probably similar to or even identical to what's used for the Linux kernel".
Ian.
The secure world code is relocated to the MB just below the top of 4G, we reserve it in the FDT (by setting CONFIG_ARMV7_SECURE_RESERVE_SIZE) but it is not protected in h/w. See next patch.
Signed-off-by: Ian Campbell ijc@hellion.org.uk --- (This was originally commit ed48864fb69d352291ed75d50a215d60864b021b in https://github.com/thierryreding/u-boot.git#staging/work but required rebasing/reworking for Kconfig changes and addition of secure RAM fdt reservation, not a lot of the original remains) --- arch/arm/cpu/armv7/tegra124/Kconfig | 2 ++ include/configs/jetson-tk1.h | 5 +++++ 2 files changed, 7 insertions(+)
diff --git a/arch/arm/cpu/armv7/tegra124/Kconfig b/arch/arm/cpu/armv7/tegra124/Kconfig index 88f627c..5114299 100644 --- a/arch/arm/cpu/armv7/tegra124/Kconfig +++ b/arch/arm/cpu/armv7/tegra124/Kconfig @@ -5,6 +5,8 @@ choice
config TARGET_JETSON_TK1 bool "NVIDIA Tegra124 Jetson TK1 board" + select CPU_V7_HAS_NONSEC if !SPL_BUILD + select CPU_V7_HAS_VIRT if !SPL_BUILD
config TARGET_NYAN_BIG bool "Google/NVIDIA Nyan-big Chrombook" diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h index 0a79c7c..80c2952 100644 --- a/include/configs/jetson-tk1.h +++ b/include/configs/jetson-tk1.h @@ -81,4 +81,9 @@ #include "tegra-common-usb-gadget.h" #include "tegra-common-post.h"
+#define CONFIG_ARMV7_PSCI 1 +/* Reserve top 1M for secure RAM */ +#define CONFIG_ARMV7_SECURE_BASE 0xfff00000 +#define CONFIG_ARMV7_SECURE_RESERVE_SIZE 0x00100000 + #endif /* __CONFIG_H */
On 01/13/2015 12:45 PM, Ian Campbell wrote:
The secure world code is relocated to the MB just below the top of 4G, we reserve it in the FDT (by setting CONFIG_ARMV7_SECURE_RESERVE_SIZE) but it is not protected in h/w. See next patch.
diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
+#define CONFIG_ARMV7_PSCI 1 +/* Reserve top 1M for secure RAM */ +#define CONFIG_ARMV7_SECURE_BASE 0xfff00000 +#define CONFIG_ARMV7_SECURE_RESERVE_SIZE 0x00100000
I /think/ the assumption in the existing code is that CONFIG_ARMV7_SECURE_BASE is the base of some out-of-DRAM secure memory, and hence that's why arch/arm/cpu/armv7/virt-dt.c() only reserves memory if that symbol is *not* set? That seems like rather a confusing semantic given the variable name. Introducing a new define that looks like it's simply the size of that region but actually changes the reservation semantics makes the situation worse for me.
Wouldn't it be better to have:
CONFIG_ARMV7_SECURE_BASE defines where the secure code is copied to.
CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM defines the obvious; whether the secure base is in DRAM or not.
That define would default to unset and you'd get the current behaviour.
If that define was set, then CONFIG_ARMV7_SECURE_BASE through CONFIG_ARMV7_SECURE_BASE + (__secure_end - __secure_start) would be reserved in RAM?
That way, armv7_update_dt would be more like:
int armv7_update_dt(void *fdt) { #if defined(CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM) || \ !defined(CONFIG_ARMV7_SECURE_BASE) /* secure code lives in RAM, keep it alive */ #if defined(CONFIG_ARMV7_SECURE_BASE) base = CONFIG_ARMV7_SECURE_BASE; #else base = __secure_start; #endif fdt_add_mem_rsv(fdt, base, __secure_end - __secure_start); #endif
return fdt_psci(fdt); }
On Thu, Jan 15, 2015 at 04:59:12PM -0700, Stephen Warren wrote:
On 01/13/2015 12:45 PM, Ian Campbell wrote:
The secure world code is relocated to the MB just below the top of 4G, we reserve it in the FDT (by setting CONFIG_ARMV7_SECURE_RESERVE_SIZE) but it is not protected in h/w. See next patch.
diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
+#define CONFIG_ARMV7_PSCI 1 +/* Reserve top 1M for secure RAM */ +#define CONFIG_ARMV7_SECURE_BASE 0xfff00000 +#define CONFIG_ARMV7_SECURE_RESERVE_SIZE 0x00100000
I /think/ the assumption in the existing code is that CONFIG_ARMV7_SECURE_BASE is the base of some out-of-DRAM secure memory, and hence that's why arch/arm/cpu/armv7/virt-dt.c() only reserves memory if that symbol is *not* set? That seems like rather a confusing semantic given the variable name. Introducing a new define that looks like it's simply the size of that region but actually changes the reservation semantics makes the situation worse for me.
Wouldn't it be better to have:
CONFIG_ARMV7_SECURE_BASE defines where the secure code is copied to.
CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM defines the obvious; whether the secure base is in DRAM or not.
That define would default to unset and you'd get the current behaviour.
If that define was set, then CONFIG_ARMV7_SECURE_BASE through CONFIG_ARMV7_SECURE_BASE + (__secure_end - __secure_start) would be reserved in RAM?
That way, armv7_update_dt would be more like:
int armv7_update_dt(void *fdt) { #if defined(CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM) || \ !defined(CONFIG_ARMV7_SECURE_BASE) /* secure code lives in RAM, keep it alive */ #if defined(CONFIG_ARMV7_SECURE_BASE) base = CONFIG_ARMV7_SECURE_BASE; #else base = __secure_start; #endif fdt_add_mem_rsv(fdt, base, __secure_end - __secure_start); #endif
return fdt_psci(fdt);}
As I understand it, one of the purposes of the RESERVE_SIZE is that hardware may not allow regions of arbitrary size to be reserved. On Tegra for example I think the restriction is that memory can only be secured on 1 MiB boundaries.
So unless explicitly specified we'd need a way for platforms to be able to adjust the reserved region accordingly.
Thierry
On Fri, 2015-01-16 at 09:52 +0100, Thierry Reding wrote:
On Thu, Jan 15, 2015 at 04:59:12PM -0700, Stephen Warren wrote:
On 01/13/2015 12:45 PM, Ian Campbell wrote:
The secure world code is relocated to the MB just below the top of 4G, we reserve it in the FDT (by setting CONFIG_ARMV7_SECURE_RESERVE_SIZE) but it is not protected in h/w. See next patch.
diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
+#define CONFIG_ARMV7_PSCI 1 +/* Reserve top 1M for secure RAM */ +#define CONFIG_ARMV7_SECURE_BASE 0xfff00000 +#define CONFIG_ARMV7_SECURE_RESERVE_SIZE 0x00100000
I /think/ the assumption in the existing code is that CONFIG_ARMV7_SECURE_BASE is the base of some out-of-DRAM secure memory, and hence that's why arch/arm/cpu/armv7/virt-dt.c() only reserves memory if that symbol is *not* set? That seems like rather a confusing semantic given the variable name. Introducing a new define that looks like it's simply the size of that region but actually changes the reservation semantics makes the situation worse for me.
Wouldn't it be better to have:
CONFIG_ARMV7_SECURE_BASE defines where the secure code is copied to.
CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM defines the obvious; whether the secure base is in DRAM or not.
I started off with this but then removed it as redundant, but you are right that it makes it more obvious what is happening, and hence isn't really redundant at all. I'll add it back.
That define would default to unset and you'd get the current behaviour.
If that define was set, then CONFIG_ARMV7_SECURE_BASE through CONFIG_ARMV7_SECURE_BASE + (__secure_end - __secure_start) would be reserved in RAM?
That way, armv7_update_dt would be more like:
int armv7_update_dt(void *fdt) { #if defined(CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM) || \ !defined(CONFIG_ARMV7_SECURE_BASE) /* secure code lives in RAM, keep it alive */ #if defined(CONFIG_ARMV7_SECURE_BASE) base = CONFIG_ARMV7_SECURE_BASE; #else base = __secure_start; #endif fdt_add_mem_rsv(fdt, base, __secure_end - __secure_start); #endif
return fdt_psci(fdt);}
As I understand it, one of the purposes of the RESERVE_SIZE is that hardware may not allow regions of arbitrary size to be reserved. On Tegra for example I think the restriction is that memory can only be secured on 1 MiB boundaries.
Exactly, the FDT reservation needs to precisely match what the hardware is protecting, which has MB granularity on this platform.
So unless explicitly specified we'd need a way for platforms to be able to adjust the reserved region accordingly.
How about if CONFIG_ARMV7_SECURE_SIZE is set we reserve that amount, otherwise we reserve __secure_end - __secure_start, with the proposed SECURE_BASE_IS_IN_DRAM || !SECURE_BASE handling surrounding that?
IOW modifying Stephen's suggestion to something like:
#if defined(CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM) || \ !defined(CONFIG_ARMV7_SECURE_BASE) /* secure code lives in RAM, keep it alive */ #if defined(CONFIG_ARMV7_SECURE_BASE) base = CONFIG_ARMV7_SECURE_BASE; #else base = __secure_start; #endif #if defined(CONFIG_ARMV7_SECURE_SIZE) size = CONFIG_ARMV7_SECURE_SIZE; #else size = __secure_end - __secure_start; #endif fdt_add_mem_rsv(fdt, base, size); #endif
return fdt_psci(fdt); }
Ian.
On 01/16/2015 02:39 AM, Ian Campbell wrote:
On Fri, 2015-01-16 at 09:52 +0100, Thierry Reding wrote:
On Thu, Jan 15, 2015 at 04:59:12PM -0700, Stephen Warren wrote:
On 01/13/2015 12:45 PM, Ian Campbell wrote:
The secure world code is relocated to the MB just below the top of 4G, we reserve it in the FDT (by setting CONFIG_ARMV7_SECURE_RESERVE_SIZE) but it is not protected in h/w. See next patch.
diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
+#define CONFIG_ARMV7_PSCI 1 +/* Reserve top 1M for secure RAM */ +#define CONFIG_ARMV7_SECURE_BASE 0xfff00000 +#define CONFIG_ARMV7_SECURE_RESERVE_SIZE 0x00100000
I /think/ the assumption in the existing code is that CONFIG_ARMV7_SECURE_BASE is the base of some out-of-DRAM secure memory, and hence that's why arch/arm/cpu/armv7/virt-dt.c() only reserves memory if that symbol is *not* set? That seems like rather a confusing semantic given the variable name. Introducing a new define that looks like it's simply the size of that region but actually changes the reservation semantics makes the situation worse for me.
Wouldn't it be better to have:
CONFIG_ARMV7_SECURE_BASE defines where the secure code is copied to.
CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM defines the obvious; whether the secure base is in DRAM or not.
I started off with this but then removed it as redundant, but you are right that it makes it more obvious what is happening, and hence isn't really redundant at all. I'll add it back.
That define would default to unset and you'd get the current behaviour.
If that define was set, then CONFIG_ARMV7_SECURE_BASE through CONFIG_ARMV7_SECURE_BASE + (__secure_end - __secure_start) would be reserved in RAM?
That way, armv7_update_dt would be more like:
int armv7_update_dt(void *fdt) { #if defined(CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM) || \ !defined(CONFIG_ARMV7_SECURE_BASE) /* secure code lives in RAM, keep it alive */ #if defined(CONFIG_ARMV7_SECURE_BASE) base = CONFIG_ARMV7_SECURE_BASE; #else base = __secure_start; #endif fdt_add_mem_rsv(fdt, base, __secure_end - __secure_start); #endif
return fdt_psci(fdt);}
As I understand it, one of the purposes of the RESERVE_SIZE is that hardware may not allow regions of arbitrary size to be reserved. On Tegra for example I think the restriction is that memory can only be secured on 1 MiB boundaries.
Exactly, the FDT reservation needs to precisely match what the hardware is protecting, which has MB granularity on this platform.
So unless explicitly specified we'd need a way for platforms to be able to adjust the reserved region accordingly.
How about if CONFIG_ARMV7_SECURE_SIZE is set we reserve that amount, otherwise we reserve __secure_end - __secure_start, with the proposed SECURE_BASE_IS_IN_DRAM || !SECURE_BASE handling surrounding that?
IOW modifying Stephen's suggestion to something like:
#if defined(CONFIG_ARMV7_SECURE_BASE_IS_IN_DRAM) || \ !defined(CONFIG_ARMV7_SECURE_BASE) /* secure code lives in RAM, keep it alive */ #if defined(CONFIG_ARMV7_SECURE_BASE) base = CONFIG_ARMV7_SECURE_BASE; #else base = __secure_start; #endif #if defined(CONFIG_ARMV7_SECURE_SIZE) size = CONFIG_ARMV7_SECURE_SIZE; #else size = __secure_end - __secure_start; #endif fdt_add_mem_rsv(fdt, base, size); #endif return fdt_psci(fdt); }
That sounds nice and orthogonal/flexible:-)
If we want to, that scheme is pretty easy to extend with a run-time hook to "round" the value of size at run-time, rather than hard-coding it in a config file, if we ever need that.
These registers can be used to prevent non-secure world from accessing a megabyte aligned region of RAM, use them to protect the u-boot secure monitor code.
At first I tried to do this from s_init(), however this inexplicably causes u-boot's networking (e.g. DHCP) to fail, while networking under Linux was fine.
So instead I have added a new weak arch function protect_secure_section() called from relocate_secure_section() and reserved the region there. This is better overall since it defers the reservation until after the sec vs. non-sec decision (which can be influenced by an envvar) has been made when booting the os.
Signed-off-by: Ian Campbell ijc@hellion.org.uk Cc: Albert Aribaud albert.u.boot@aribaud.net --- arch/arm/cpu/armv7/virt-v7.c | 5 +++++ arch/arm/cpu/tegra-common/ap.c | 15 +++++++++++++++ arch/arm/include/asm/system.h | 1 + 3 files changed, 21 insertions(+)
diff --git a/arch/arm/cpu/armv7/virt-v7.c b/arch/arm/cpu/armv7/virt-v7.c index 651ca40..a83214b 100644 --- a/arch/arm/cpu/armv7/virt-v7.c +++ b/arch/arm/cpu/armv7/virt-v7.c @@ -48,6 +48,10 @@ static unsigned long get_gicd_base_address(void) #endif }
+/* Define a specific version of this function to enable any available + * hardware protections for the reserved region */ +void __weak protect_secure_section(void) {} + static void relocate_secure_section(void) { #ifdef CONFIG_ARMV7_SECURE_BASE @@ -56,6 +60,7 @@ static void relocate_secure_section(void) memcpy((void *)CONFIG_ARMV7_SECURE_BASE, __secure_start, sz); flush_dcache_range(CONFIG_ARMV7_SECURE_BASE, CONFIG_ARMV7_SECURE_BASE + sz + 1); + protect_secure_section(); invalidate_icache_all(); #endif } diff --git a/arch/arm/cpu/tegra-common/ap.c b/arch/arm/cpu/tegra-common/ap.c index a17dfd1..f1d3070 100644 --- a/arch/arm/cpu/tegra-common/ap.c +++ b/arch/arm/cpu/tegra-common/ap.c @@ -10,6 +10,7 @@ #include <common.h> #include <asm/io.h> #include <asm/arch/gp_padctrl.h> +#include <asm/arch/mc.h> #include <asm/arch-tegra/ap.h> #include <asm/arch-tegra/clock.h> #include <asm/arch-tegra/fuse.h> @@ -154,6 +155,20 @@ static void init_pmc_scratch(void) writel(odmdata, &pmc->pmc_scratch20); }
+#ifdef CONFIG_ARMV7_SECURE_RESERVE_SIZE +void protect_secure_section(void) +{ + struct mc_ctlr *mc = (struct mc_ctlr *)NV_PA_MC_BASE; + + /* Must be MB aligned */ + BUILD_BUG_ON(CONFIG_ARMV7_SECURE_BASE & 0xFFFFF); + BUILD_BUG_ON(CONFIG_ARMV7_SECURE_RESERVE_SIZE & 0xFFFFF); + + writel(CONFIG_ARMV7_SECURE_BASE, &mc->mc_security_cfg0); + writel(CONFIG_ARMV7_SECURE_RESERVE_SIZE>>20, &mc->mc_security_cfg1); +} +#endif + void s_init(void) { /* Init PMC scratch memory */ diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h index 89f2294..21be69d 100644 --- a/arch/arm/include/asm/system.h +++ b/arch/arm/include/asm/system.h @@ -76,6 +76,7 @@ void armv8_switch_to_el1(void); void gic_init(void); void gic_send_sgi(unsigned long sgino); void wait_for_wakeup(void); +void protect_secure_region(void); void smp_kick_all_cpus(void);
void flush_l3_cache(void);
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.
Adding Stephen and Tom for visibility.
Thanks, Thierry
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.
On Wed, Jan 14, 2015 at 08:58:41AM +0000, Ian Campbell wrote:
On Wed, 2015-01-14 at 08:57 +0100, Thierry Reding wrote:
I also pushed my tree to gitorious: https://gitorious.org/ijc/u-boot jetson-psci-v1
I would Ack your patch, but I don't think you've posted it and it has no S-o-b so that would seem a bit premature/rude of me. For the same reason I've not actually included it in the series posted (but it is in the gitorious branch).
Feel free to take ownership of that patch. I currently don't have the time to work on this and it seems you've made good progress on it.
Will do. Could you offer a S-o-b for it please so I can pick it up.
Sure:
Signed-off-by: Thierry Reding treding@nvidia.com
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
On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote:
On Wed, Jan 14, 2015 at 08:58:41AM +0000, Ian Campbell wrote:
On Wed, 2015-01-14 at 08:57 +0100, Thierry Reding wrote:
I also pushed my tree to gitorious: https://gitorious.org/ijc/u-boot jetson-psci-v1
I would Ack your patch, but I don't think you've posted it and it has no S-o-b so that would seem a bit premature/rude of me. For the same reason I've not actually included it in the series posted (but it is in the gitorious branch).
Feel free to take ownership of that patch. I currently don't have the time to work on this and it seems you've made good progress on it.
Will do. Could you offer a S-o-b for it please so I can pick it up.
Sure:
Signed-off-by: Thierry Reding treding@nvidia.com
Thanks!
It could probably use some cleanup because there's a bit of debug output still in there. Also...
FWIW I think you could drop your stub versions of psci_cpu_off and psci_cpu_suspend (assuming you don't want to implement them) since the common code has stubs.
... I'd think you'd need to implement these so that you can get proper suspend/resume support in the kernel. I've had to disable cpuidle (via #undef CONFIG_PM_SLEEP in arch/arm/mach-tegra/cpuidle-tegra114.c) in the kernel to make that code not powergate CPUs. Ideally I think the kernel would check that it's running with PSCI support and disable the cpuidle driver. Maybe that could be done by introducing a new cpuidle driver that checks for PSCI availability and uses it when present.
Hrm, I'm not sure how this all fits together, it's not a problem I've noted before.
FWIW I think cpu_off and cpu_suspend are optional in PSCI v0.1 so an initial version doesn't necessarily need to implement them (sunxi doesn't for example), but as you say they do enable useful features.
I think when I tried last time, without disable the cpuidle driver things would hang at boot. I would expect that problem to exist for any board. Perhaps you've disabled PM_SLEEP in your config?
I don't think so: # grep PM_SLEEP /boot/config-3.18.0-trunk-armmp-lpae CONFIG_PM_SLEEP=y CONFIG_PM_SLEEP_SMP=y CONFIG_PM_SLEEP_DEBUG=y
I don't see anything about cpuidle in dmesg either.
Did you perhaps mean CPU_IDLE rather than PM_SLEEP because: # CONFIG_CPU_IDLE is not set
Ian.
On Fri, Jan 16, 2015 at 09:43:22AM +0000, Ian Campbell wrote:
On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote:
On Wed, Jan 14, 2015 at 08:58:41AM +0000, Ian Campbell wrote:
On Wed, 2015-01-14 at 08:57 +0100, Thierry Reding wrote:
I also pushed my tree to gitorious: https://gitorious.org/ijc/u-boot jetson-psci-v1
I would Ack your patch, but I don't think you've posted it and it has no S-o-b so that would seem a bit premature/rude of me. For the same reason I've not actually included it in the series posted (but it is in the gitorious branch).
Feel free to take ownership of that patch. I currently don't have the time to work on this and it seems you've made good progress on it.
Will do. Could you offer a S-o-b for it please so I can pick it up.
Sure:
Signed-off-by: Thierry Reding treding@nvidia.com
Thanks!
It could probably use some cleanup because there's a bit of debug output still in there. Also...
FWIW I think you could drop your stub versions of psci_cpu_off and psci_cpu_suspend (assuming you don't want to implement them) since the common code has stubs.
... I'd think you'd need to implement these so that you can get proper suspend/resume support in the kernel. I've had to disable cpuidle (via #undef CONFIG_PM_SLEEP in arch/arm/mach-tegra/cpuidle-tegra114.c) in the kernel to make that code not powergate CPUs. Ideally I think the kernel would check that it's running with PSCI support and disable the cpuidle driver. Maybe that could be done by introducing a new cpuidle driver that checks for PSCI availability and uses it when present.
Hrm, I'm not sure how this all fits together, it's not a problem I've noted before.
FWIW I think cpu_off and cpu_suspend are optional in PSCI v0.1 so an initial version doesn't necessarily need to implement them (sunxi doesn't for example), but as you say they do enable useful features.
I think when I tried last time, without disable the cpuidle driver things would hang at boot. I would expect that problem to exist for any board. Perhaps you've disabled PM_SLEEP in your config?
I don't think so: # grep PM_SLEEP /boot/config-3.18.0-trunk-armmp-lpae CONFIG_PM_SLEEP=y CONFIG_PM_SLEEP_SMP=y CONFIG_PM_SLEEP_DEBUG=y
I don't see anything about cpuidle in dmesg either.
Did you perhaps mean CPU_IDLE rather than PM_SLEEP because: # CONFIG_CPU_IDLE is not set
Yes, I think that would have the same effect as disable PM_SLEEP (at least regarding the powergate stuff that's conflicting with the PSCI implementation).
Note also, as mentioned in another reply, that with the PSCI support there's now two sources that can simultaneously access the powergate functionality in the PMC. We have some locking in place to make sure that concurrent accesses from within the kernel are serialized, but there's no mechanism in place to protect from concurrent accesses in secure firmware and the kernel.
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
On Fri, 2015-01-16 at 11:05 +0100, Thierry Reding wrote:
On Fri, Jan 16, 2015 at 09:43:22AM +0000, Ian Campbell wrote:
On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote:
On Wed, Jan 14, 2015 at 08:58:41AM +0000, Ian Campbell wrote:
On Wed, 2015-01-14 at 08:57 +0100, Thierry Reding wrote:
I also pushed my tree to gitorious: https://gitorious.org/ijc/u-boot jetson-psci-v1
I would Ack your patch, but I don't think you've posted it and it has no S-o-b so that would seem a bit premature/rude of me. For the same reason I've not actually included it in the series posted (but it is in the gitorious branch).
Feel free to take ownership of that patch. I currently don't have the time to work on this and it seems you've made good progress on it.
Will do. Could you offer a S-o-b for it please so I can pick it up.
Sure:
Signed-off-by: Thierry Reding treding@nvidia.com
Thanks!
It could probably use some cleanup because there's a bit of debug output still in there. Also...
FWIW I think you could drop your stub versions of psci_cpu_off and psci_cpu_suspend (assuming you don't want to implement them) since the common code has stubs.
... I'd think you'd need to implement these so that you can get proper suspend/resume support in the kernel. I've had to disable cpuidle (via #undef CONFIG_PM_SLEEP in arch/arm/mach-tegra/cpuidle-tegra114.c) in the kernel to make that code not powergate CPUs. Ideally I think the kernel would check that it's running with PSCI support and disable the cpuidle driver. Maybe that could be done by introducing a new cpuidle driver that checks for PSCI availability and uses it when present.
Hrm, I'm not sure how this all fits together, it's not a problem I've noted before.
FWIW I think cpu_off and cpu_suspend are optional in PSCI v0.1 so an initial version doesn't necessarily need to implement them (sunxi doesn't for example), but as you say they do enable useful features.
I think when I tried last time, without disable the cpuidle driver things would hang at boot. I would expect that problem to exist for any board. Perhaps you've disabled PM_SLEEP in your config?
I don't think so: # grep PM_SLEEP /boot/config-3.18.0-trunk-armmp-lpae CONFIG_PM_SLEEP=y CONFIG_PM_SLEEP_SMP=y CONFIG_PM_SLEEP_DEBUG=y
I don't see anything about cpuidle in dmesg either.
Did you perhaps mean CPU_IDLE rather than PM_SLEEP because: # CONFIG_CPU_IDLE is not set
Yes, I think that would have the same effect as disable PM_SLEEP (at least regarding the powergate stuff that's conflicting with the PSCI implementation).
Note also, as mentioned in another reply, that with the PSCI support there's now two sources that can simultaneously access the powergate functionality in the PMC. We have some locking in place to make sure that concurrent accesses from within the kernel are serialized, but there's no mechanism in place to protect from concurrent accesses in secure firmware and the kernel.
The docs are on another machine, but I take it the PMC registers are available to NS mode? Is that configurable (from S mode) perhaps?
I don't have any good ideas on how to solve this nicely. The best I could come up with is to make sure that we grab a lock before doing any PSCI calls from the kernel and release the lock upon return.
Ian.
On Fri, Jan 16, 2015 at 10:24:03AM +0000, Ian Campbell wrote:
On Fri, 2015-01-16 at 11:05 +0100, Thierry Reding wrote:
On Fri, Jan 16, 2015 at 09:43:22AM +0000, Ian Campbell wrote:
On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote:
On Wed, Jan 14, 2015 at 08:58:41AM +0000, Ian Campbell wrote:
On Wed, 2015-01-14 at 08:57 +0100, Thierry Reding wrote:
> I also pushed my tree to gitorious: > https://gitorious.org/ijc/u-boot jetson-psci-v1 > > I would Ack your patch, but I don't think you've posted it and it has no > S-o-b so that would seem a bit premature/rude of me. For the same reason > I've not actually included it in the series posted (but it is in the > gitorious branch).
Feel free to take ownership of that patch. I currently don't have the time to work on this and it seems you've made good progress on it.
Will do. Could you offer a S-o-b for it please so I can pick it up.
Sure:
Signed-off-by: Thierry Reding treding@nvidia.com
Thanks!
It could probably use some cleanup because there's a bit of debug output still in there. Also...
> FWIW I think you could drop your stub versions of psci_cpu_off and > psci_cpu_suspend (assuming you don't want to implement them) since the > common code has stubs.
... I'd think you'd need to implement these so that you can get proper suspend/resume support in the kernel. I've had to disable cpuidle (via #undef CONFIG_PM_SLEEP in arch/arm/mach-tegra/cpuidle-tegra114.c) in the kernel to make that code not powergate CPUs. Ideally I think the kernel would check that it's running with PSCI support and disable the cpuidle driver. Maybe that could be done by introducing a new cpuidle driver that checks for PSCI availability and uses it when present.
Hrm, I'm not sure how this all fits together, it's not a problem I've noted before.
FWIW I think cpu_off and cpu_suspend are optional in PSCI v0.1 so an initial version doesn't necessarily need to implement them (sunxi doesn't for example), but as you say they do enable useful features.
I think when I tried last time, without disable the cpuidle driver things would hang at boot. I would expect that problem to exist for any board. Perhaps you've disabled PM_SLEEP in your config?
I don't think so: # grep PM_SLEEP /boot/config-3.18.0-trunk-armmp-lpae CONFIG_PM_SLEEP=y CONFIG_PM_SLEEP_SMP=y CONFIG_PM_SLEEP_DEBUG=y
I don't see anything about cpuidle in dmesg either.
Did you perhaps mean CPU_IDLE rather than PM_SLEEP because: # CONFIG_CPU_IDLE is not set
Yes, I think that would have the same effect as disable PM_SLEEP (at least regarding the powergate stuff that's conflicting with the PSCI implementation).
Note also, as mentioned in another reply, that with the PSCI support there's now two sources that can simultaneously access the powergate functionality in the PMC. We have some locking in place to make sure that concurrent accesses from within the kernel are serialized, but there's no mechanism in place to protect from concurrent accesses in secure firmware and the kernel.
The docs are on another machine, but I take it the PMC registers are available to NS mode? Is that configurable (from S mode) perhaps?
I don't 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
On Fri, 2015-01-16 at 17:03 +0100, Thierry Reding wrote:
On Fri, Jan 16, 2015 at 10:24:03AM +0000, Ian Campbell wrote:
On Fri, 2015-01-16 at 11:05 +0100, Thierry Reding wrote:
On Fri, Jan 16, 2015 at 09:43:22AM +0000, Ian Campbell wrote:
On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote:
On Wed, Jan 14, 2015 at 08:58:41AM +0000, Ian Campbell wrote:
On Wed, 2015-01-14 at 08:57 +0100, Thierry Reding wrote: > > I also pushed my tree to gitorious: > > https://gitorious.org/ijc/u-boot jetson-psci-v1 > > > > I would Ack your patch, but I don't think you've posted it and it has no > > S-o-b so that would seem a bit premature/rude of me. For the same reason > > I've not actually included it in the series posted (but it is in the > > gitorious branch). > > Feel free to take ownership of that patch. I currently don't have the > time to work on this and it seems you've made good progress on it.
Will do. Could you offer a S-o-b for it please so I can pick it up.
Sure:
Signed-off-by: Thierry Reding treding@nvidia.com
Thanks!
> It could probably use some cleanup because there's a bit of debug output > still in there. Also... > > > FWIW I think you could drop your stub versions of psci_cpu_off and > > psci_cpu_suspend (assuming you don't want to implement them) since the > > common code has stubs. > > ... I'd think you'd need to implement these so that you can get proper > suspend/resume support in the kernel. I've had to disable cpuidle (via > #undef CONFIG_PM_SLEEP in arch/arm/mach-tegra/cpuidle-tegra114.c) in the > kernel to make that code not powergate CPUs. Ideally I think the kernel > would check that it's running with PSCI support and disable the cpuidle > driver. Maybe that could be done by introducing a new cpuidle driver > that checks for PSCI availability and uses it when present.
Hrm, I'm not sure how this all fits together, it's not a problem I've noted before.
FWIW I think cpu_off and cpu_suspend are optional in PSCI v0.1 so an initial version doesn't necessarily need to implement them (sunxi doesn't for example), but as you say they do enable useful features.
I think when I tried last time, without disable the cpuidle driver things would hang at boot. I would expect that problem to exist for any board. Perhaps you've disabled PM_SLEEP in your config?
I don't think so: # grep PM_SLEEP /boot/config-3.18.0-trunk-armmp-lpae CONFIG_PM_SLEEP=y CONFIG_PM_SLEEP_SMP=y CONFIG_PM_SLEEP_DEBUG=y
I don't see anything about cpuidle in dmesg either.
Did you perhaps mean CPU_IDLE rather than PM_SLEEP because: # CONFIG_CPU_IDLE is not set
Yes, I think that would have the same effect as disable PM_SLEEP (at least regarding the powergate stuff that's conflicting with the PSCI implementation).
Note also, as mentioned in another reply, that with the PSCI support there's now two sources that can simultaneously access the powergate functionality in the PMC. We have some locking in place to make sure that concurrent accesses from within the kernel are serialized, but there's no mechanism in place to protect from concurrent accesses in secure firmware and the kernel.
The docs are on another machine, but I take it the PMC registers are available to NS mode? Is that configurable (from S mode) perhaps?
I don't 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.
On Fri, Jan 16, 2015 at 04:11:19PM +0000, Ian Campbell wrote:
On Fri, 2015-01-16 at 17:03 +0100, Thierry Reding wrote:
On Fri, Jan 16, 2015 at 10:24:03AM +0000, Ian Campbell wrote:
On Fri, 2015-01-16 at 11:05 +0100, Thierry Reding wrote:
On Fri, Jan 16, 2015 at 09:43:22AM +0000, Ian Campbell wrote:
On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote:
On Wed, Jan 14, 2015 at 08:58:41AM +0000, Ian Campbell wrote: > On Wed, 2015-01-14 at 08:57 +0100, Thierry Reding wrote: > > > I also pushed my tree to gitorious: > > > https://gitorious.org/ijc/u-boot jetson-psci-v1 > > > > > > I would Ack your patch, but I don't think you've posted it and it has no > > > S-o-b so that would seem a bit premature/rude of me. For the same reason > > > I've not actually included it in the series posted (but it is in the > > > gitorious branch). > > > > Feel free to take ownership of that patch. I currently don't have the > > time to work on this and it seems you've made good progress on it. > > Will do. Could you offer a S-o-b for it please so I can pick it up.
Sure:
Signed-off-by: Thierry Reding treding@nvidia.com
Thanks!
> > It could probably use some cleanup because there's a bit of debug output > > still in there. Also... > > > > > FWIW I think you could drop your stub versions of psci_cpu_off and > > > psci_cpu_suspend (assuming you don't want to implement them) since the > > > common code has stubs. > > > > ... I'd think you'd need to implement these so that you can get proper > > suspend/resume support in the kernel. I've had to disable cpuidle (via > > #undef CONFIG_PM_SLEEP in arch/arm/mach-tegra/cpuidle-tegra114.c) in the > > kernel to make that code not powergate CPUs. Ideally I think the kernel > > would check that it's running with PSCI support and disable the cpuidle > > driver. Maybe that could be done by introducing a new cpuidle driver > > that checks for PSCI availability and uses it when present. > > Hrm, I'm not sure how this all fits together, it's not a problem I've > noted before. > > FWIW I think cpu_off and cpu_suspend are optional in PSCI v0.1 so an > initial version doesn't necessarily need to implement them (sunxi > doesn't for example), but as you say they do enable useful features.
I think when I tried last time, without disable the cpuidle driver things would hang at boot. I would expect that problem to exist for any board. Perhaps you've disabled PM_SLEEP in your config?
I don't think so: # grep PM_SLEEP /boot/config-3.18.0-trunk-armmp-lpae CONFIG_PM_SLEEP=y CONFIG_PM_SLEEP_SMP=y CONFIG_PM_SLEEP_DEBUG=y
I don't see anything about cpuidle in dmesg either.
Did you perhaps mean CPU_IDLE rather than PM_SLEEP because: # CONFIG_CPU_IDLE is not set
Yes, I think that would have the same effect as disable PM_SLEEP (at least regarding the powergate stuff that's conflicting with the PSCI implementation).
Note also, as mentioned in another reply, that with the PSCI support there's now two sources that can simultaneously access the powergate functionality in the PMC. We have some locking in place to make sure that concurrent accesses from within the kernel are serialized, but there's no mechanism in place to protect from concurrent accesses in secure firmware and the kernel.
The docs are on another machine, but I take it the PMC registers are available to NS mode? Is that configurable (from S mode) perhaps?
I don't 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 registers are available in NS mode. They have to because a bunch of drivers rely on it. The powergate driver controls not only CPU power domains, but also display, SATA, PCIe and so on.
Thierry
On Mon, 2015-01-19 at 10:25 +0100, Thierry Reding wrote:
On Fri, Jan 16, 2015 at 04:11:19PM +0000, Ian Campbell wrote:
On Fri, 2015-01-16 at 17:03 +0100, Thierry Reding wrote:
On Fri, Jan 16, 2015 at 10:24:03AM +0000, Ian Campbell wrote:
On Fri, 2015-01-16 at 11:05 +0100, Thierry Reding wrote:
On Fri, Jan 16, 2015 at 09:43:22AM +0000, Ian Campbell wrote:
On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote: > On Wed, Jan 14, 2015 at 08:58:41AM +0000, Ian Campbell wrote: > > On Wed, 2015-01-14 at 08:57 +0100, Thierry Reding wrote: > > > > I also pushed my tree to gitorious: > > > > https://gitorious.org/ijc/u-boot jetson-psci-v1 > > > > > > > > I would Ack your patch, but I don't think you've posted it and it has no > > > > S-o-b so that would seem a bit premature/rude of me. For the same reason > > > > I've not actually included it in the series posted (but it is in the > > > > gitorious branch). > > > > > > Feel free to take ownership of that patch. I currently don't have the > > > time to work on this and it seems you've made good progress on it. > > > > Will do. Could you offer a S-o-b for it please so I can pick it up. > > Sure: > > Signed-off-by: Thierry Reding treding@nvidia.com
Thanks!
> > > It could probably use some cleanup because there's a bit of debug output > > > still in there. Also... > > > > > > > FWIW I think you could drop your stub versions of psci_cpu_off and > > > > psci_cpu_suspend (assuming you don't want to implement them) since the > > > > common code has stubs. > > > > > > ... I'd think you'd need to implement these so that you can get proper > > > suspend/resume support in the kernel. I've had to disable cpuidle (via > > > #undef CONFIG_PM_SLEEP in arch/arm/mach-tegra/cpuidle-tegra114.c) in the > > > kernel to make that code not powergate CPUs. Ideally I think the kernel > > > would check that it's running with PSCI support and disable the cpuidle > > > driver. Maybe that could be done by introducing a new cpuidle driver > > > that checks for PSCI availability and uses it when present. > > > > Hrm, I'm not sure how this all fits together, it's not a problem I've > > noted before. > > > > FWIW I think cpu_off and cpu_suspend are optional in PSCI v0.1 so an > > initial version doesn't necessarily need to implement them (sunxi > > doesn't for example), but as you say they do enable useful features. > > I think when I tried last time, without disable the cpuidle driver > things would hang at boot. I would expect that problem to exist for > any board. Perhaps you've disabled PM_SLEEP in your config?
I don't think so: # grep PM_SLEEP /boot/config-3.18.0-trunk-armmp-lpae CONFIG_PM_SLEEP=y CONFIG_PM_SLEEP_SMP=y CONFIG_PM_SLEEP_DEBUG=y
I don't see anything about cpuidle in dmesg either.
Did you perhaps mean CPU_IDLE rather than PM_SLEEP because: # CONFIG_CPU_IDLE is not set
Yes, I think that would have the same effect as disable PM_SLEEP (at least regarding the powergate stuff that's conflicting with the PSCI implementation).
Note also, as mentioned in another reply, that with the PSCI support there's now two sources that can simultaneously access the powergate functionality in the PMC. We have some locking in place to make sure that concurrent accesses from within the kernel are serialized, but there's no mechanism in place to protect from concurrent accesses in secure firmware and the kernel.
The docs are on another machine, but I take it the PMC registers are available to NS mode? Is that configurable (from S mode) perhaps?
I don't 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 registers are available in NS mode. They have to because a bunch of drivers rely on it. The powergate driver controls not only CPU power domains, but also display, SATA, PCIe and so on.
That makes sense.
BTW, I noticed at the weekend that u-boot unconditionally exposes all the PSCI functions (cpu_on/off/suspend and migrate) regardless of whether they are implemented for the platform. At least for PSCI v0.1 these are all optional and the right thing to do is omit those you don't support from the DT (so the OS kernel knows). This might suffice to fix (or at least defer) the PMC issue here too, so I'll try it (and it's the right thing to do regardless IMHO).
Also PSCI migrate makes no sense whatsoever for u-boot.
Ian.
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.
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.
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.
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
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?
Mark.
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.
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.
Unfortunately this doesn't change on 64-bit Tegra at all.
Thierry
On Fri, Jan 23, 2015 at 10:10:45AM +0000, Thierry Reding wrote:
On Thu, Jan 22, 2015 at 07:20:15PM +0000, Mark Rutland wrote:
On Fri, Jan 16, 2015 at 09:12:59AM +0000, Thierry Reding wrote:
On Thu, Jan 15, 2015 at 07:19:37PM +0000, Mark Rutland wrote:
On Wed, Jan 14, 2015 at 07:57:25AM +0000, Thierry Reding wrote:
On Tue, Jan 13, 2015 at 07:44:50PM +0000, Ian Campbell wrote:
Hi Thierry,
I needed to boot my Jetson in NS mode (in order to boot Xen) and was investigating the possibility of PSCI support when I discovered that you had already started on it[0]. Hurrah!
I cherry-picked the relevant commit onto u-boot-tegra#master and added a few more patches and now it boots correctly for me, both running Xen (some Xen side patches are needed too) and native Linux.
The main things which was needed was to rebase for some recent Kconfig changes relating to virt and nonsec mode and to arrange for the RAM used by the secure code to be reserved in the FDT. I also reserved the RAM using the hardware MC_SECURITY_CFG registers for good measure.
Great, those were all things that I had wanted to do but never got around to.
I also pushed my tree to gitorious: https://gitorious.org/ijc/u-boot jetson-psci-v1
I would Ack your patch, but I don't think you've posted it and it has no S-o-b so that would seem a bit premature/rude of me. For the same reason I've not actually included it in the series posted (but it is in the gitorious branch).
Feel free to take ownership of that patch. I currently don't have the time to work on this and it seems you've made good progress on it.
It could probably use some cleanup because there's a bit of debug output still in there. Also...
FWIW I think you could drop your stub versions of psci_cpu_off and psci_cpu_suspend (assuming you don't want to implement them) since the common code has stubs.
... I'd think you'd need to implement these so that you can get proper suspend/resume support in the kernel. I've had to disable cpuidle (via #undef CONFIG_PM_SLEEP in arch/arm/mach-tegra/cpuidle-tegra114.c) in the kernel to make that code not powergate CPUs. Ideally I think the kernel would check that it's running with PSCI support and disable the cpuidle driver. Maybe that could be done by introducing a new cpuidle driver that checks for PSCI availability and uses it when present.
We have a generic CPUidle driver on arm64 which can use PSCI as a backend; we should try to reuse that. The binding should certainly be identical.
Is there any reason that driver needs to be ARM64-specific? I would've thought that there could be a generic PSCI driver that works anywhere.
Currently the arm and arm64 arch interfaces are a little different, but with some work the bulk of the code could certainly be made common (in drivers/firmware, perhaps).
It looks like the tegra124 dts in mainline doesn't use enable-method in the DT, so a better option might be to fail early in cpuidle-tegra114.c if _any_ enable-method is present.
Yes, that sounds like a good plan. The absence of an enable-method would signal that a kernel-native method (if any) should be used.
And this reminds me that we still need to find a way to synchronize accesses to the powergate registers between secure firmware and the kernel. Tegra has a set of hardware semaphores, but it seems like those can only be used to synchronize between AVP and CPU, whereas for PSCI we'd need something to synchronize between two CPUs. Do you know of any existing mechanism to perform that type of synchronization?
Perhaps an option would be to add some sort of global lock in the kernel which the cpuidle driver can grab before issuing the SMC instruction.
PSCI assumes that the FW is in full control of the registers it's poking. While a lock isn't necessarily bad, I suspect it's going to be very difficult to have that common across all users without the code becoming unmaintainable fast. I'd also hope that for arm64 we wouldn't need it.
When/how/why does the kernel to poke these registers?
The PMC is what controls power partitions. Some of these partitions are assigned to CPUs, others are assigned to things like SATA, PCIe, display and so on. The problem is that if we manage the CPU power partitions via the firmware, then they will conflict with calls that we need to make from other drivers that need to gate or ungate the partitions for their hardware. As I understand it there's no provision in PSCI to manage non- CPU devices, so this is a problem.
Ok.
So I think either firmware needs to control everything, in which case we are going to need a new interface (or extend PSCI) or it mustn't control anything, in which case we need custom code in the kernel for SMP. Well, the other alternative would be the lock that we can grab in the powergate API and the PSCI calls.
One reason I'm not so keen on a lock is I could imagine you'd need to grab this for CPU_SUSPEND calls (i.e. cpuidle), at which point all CPUs are going to contend for the lock all the time.
One thing to bear in mind is that PSCI is only one user of the SMC space. Per SMC calling convention, portions of the SMC ID space are there to be used for other (vendor-specific) purposes.
So rather than extending PSCI, a parallel API could be implemented for power control of other devices, and the backend could arbitrate the two without the non-secure OS requiring implementation-specific mutual exclusion.
I think this has been brought up internally previously; I'll go and poke around in the area to see if we managed to figure out anything useful.
Unfortunately this doesn't change on 64-bit Tegra at all.
I suspected as much. :/
How does this bode for the tegra132 dts [1] on LAKML at the moment? Is it just the "nvidia,tegra132-pmc" device that needs to be poked by both FW and kernel, or are other devices involved?
Thanks, Mark.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/317013.ht...
[...]
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.
On Fri, Jan 23, 2015 at 12:37:20PM +0000, Mark Rutland wrote:
On Fri, Jan 23, 2015 at 10:10:45AM +0000, Thierry Reding wrote:
On Thu, Jan 22, 2015 at 07:20:15PM +0000, Mark Rutland wrote:
On Fri, Jan 16, 2015 at 09:12:59AM +0000, Thierry Reding wrote:
On Thu, Jan 15, 2015 at 07:19:37PM +0000, Mark Rutland wrote:
On Wed, Jan 14, 2015 at 07:57:25AM +0000, Thierry Reding wrote:
On Tue, Jan 13, 2015 at 07:44:50PM +0000, Ian Campbell wrote: > Hi Thierry, > > I needed to boot my Jetson in NS mode (in order to boot Xen) and was > investigating the possibility of PSCI support when I discovered that you > had already started on it[0]. Hurrah! > > I cherry-picked the relevant commit onto u-boot-tegra#master and added a > few more patches and now it boots correctly for me, both running Xen > (some Xen side patches are needed too) and native Linux. > > The main things which was needed was to rebase for some recent Kconfig > changes relating to virt and nonsec mode and to arrange for the RAM used > by the secure code to be reserved in the FDT. I also reserved the RAM > using the hardware MC_SECURITY_CFG registers for good measure.
Great, those were all things that I had wanted to do but never got around to.
> I also pushed my tree to gitorious: > https://gitorious.org/ijc/u-boot jetson-psci-v1 > > I would Ack your patch, but I don't think you've posted it and it has no > S-o-b so that would seem a bit premature/rude of me. For the same reason > I've not actually included it in the series posted (but it is in the > gitorious branch).
Feel free to take ownership of that patch. I currently don't have the time to work on this and it seems you've made good progress on it.
It could probably use some cleanup because there's a bit of debug output still in there. Also...
> FWIW I think you could drop your stub versions of psci_cpu_off and > psci_cpu_suspend (assuming you don't want to implement them) since the > common code has stubs.
... I'd think you'd need to implement these so that you can get proper suspend/resume support in the kernel. I've had to disable cpuidle (via #undef CONFIG_PM_SLEEP in arch/arm/mach-tegra/cpuidle-tegra114.c) in the kernel to make that code not powergate CPUs. Ideally I think the kernel would check that it's running with PSCI support and disable the cpuidle driver. Maybe that could be done by introducing a new cpuidle driver that checks for PSCI availability and uses it when present.
We have a generic CPUidle driver on arm64 which can use PSCI as a backend; we should try to reuse that. The binding should certainly be identical.
Is there any reason that driver needs to be ARM64-specific? I would've thought that there could be a generic PSCI driver that works anywhere.
Currently the arm and arm64 arch interfaces are a little different, but with some work the bulk of the code could certainly be made common (in drivers/firmware, perhaps).
It looks like the tegra124 dts in mainline doesn't use enable-method in the DT, so a better option might be to fail early in cpuidle-tegra114.c if _any_ enable-method is present.
Yes, that sounds like a good plan. The absence of an enable-method would signal that a kernel-native method (if any) should be used.
And this reminds me that we still need to find a way to synchronize accesses to the powergate registers between secure firmware and the kernel. Tegra has a set of hardware semaphores, but it seems like those can only be used to synchronize between AVP and CPU, whereas for PSCI we'd need something to synchronize between two CPUs. Do you know of any existing mechanism to perform that type of synchronization?
Perhaps an option would be to add some sort of global lock in the kernel which the cpuidle driver can grab before issuing the SMC instruction.
PSCI assumes that the FW is in full control of the registers it's poking. While a lock isn't necessarily bad, I suspect it's going to be very difficult to have that common across all users without the code becoming unmaintainable fast. I'd also hope that for arm64 we wouldn't need it.
When/how/why does the kernel to poke these registers?
The PMC is what controls power partitions. Some of these partitions are assigned to CPUs, others are assigned to things like SATA, PCIe, display and so on. The problem is that if we manage the CPU power partitions via the firmware, then they will conflict with calls that we need to make from other drivers that need to gate or ungate the partitions for their hardware. As I understand it there's no provision in PSCI to manage non- CPU devices, so this is a problem.
Ok.
So I think either firmware needs to control everything, in which case we are going to need a new interface (or extend PSCI) or it mustn't control anything, in which case we need custom code in the kernel for SMP. Well, the other alternative would be the lock that we can grab in the powergate API and the PSCI calls.
One reason I'm not so keen on a lock is I could imagine you'd need to grab this for CPU_SUSPEND calls (i.e. cpuidle), at which point all CPUs are going to contend for the lock all the time.
One thing to bear in mind is that PSCI is only one user of the SMC space. Per SMC calling convention, portions of the SMC ID space are there to be used for other (vendor-specific) purposes.
So rather than extending PSCI, a parallel API could be implemented for power control of other devices, and the backend could arbitrate the two without the non-secure OS requiring implementation-specific mutual exclusion.
I think this has been brought up internally previously; I'll go and poke around in the area to see if we managed to figure out anything useful.
Okay, I think we'll need to coordinate to provide a common interface for the kernel to talk to firmware.
Unfortunately this doesn't change on 64-bit Tegra at all.
I suspected as much. :/
How does this bode for the tegra132 dts [1] on LAKML at the moment? Is it just the "nvidia,tegra132-pmc" device that needs to be poked by both FW and kernel, or are other devices involved?
Thanks, Mark.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/317013.ht...
Cc'ing Peter and Paul who might be more familiar with SMP.
Thierry
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.
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.
Thierry
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.
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.
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!
Thanks, Mark.
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. -----------------------------------------------------------------------------------
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.
[...]
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.
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
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.
participants (5)
-
Ian Campbell -
Jan Kiszka -
Mark Rutland -
Stephen Warren -
Thierry Reding