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

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

[Ian Campbell]

Hi Thierry,

I needed to boot my Jetson in NS mode (in order to boot Xen) and was
investigating the possibility of PSCI support when I discovered that you
had already started on it[0]. Hurrah!

I cherry-picked the relevant commit onto u-boot-tegra#master and added a
few more patches and now it boots correctly for me, both running Xen
(some Xen side patches are needed too) and native Linux.

The main things which was needed was to rebase for some recent Kconfig
changes relating to virt and nonsec mode and to arrange for the RAM used
by the secure code to be reserved in the FDT. I also reserved the RAM
using the hardware MC_SECURITY_CFG registers for good measure.

I also pushed my tree to gitorious:
        https://gitorious.org/ijc/u-boot jetson-psci-v1

I would Ack your patch, but I don't think you've posted it and it has no
S-o-b so that would seem a bit premature/rude of me. For the same reason
I've not actually included it in the series posted (but it is in the
gitorious branch).

FWIW I think you could drop your stub versions of psci_cpu_off and
psci_cpu_suspend (assuming you don't want to implement them) since the
common code has stubs.

Albert, I've CCd you on a couple of patches which touch common ARM code.
Nothing too major I think.

Cheers,
Ian.

[0]
https://github.com/thierryreding/u-boot/commit/5996d2b0dea2953ae8881f40b7f85b6fb8c50791

[U-Boot] [PATCH v1 1/4] tegra124: Add more registers to struct mc_ctlr

[Ian Campbell]

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 */
-- 
2.1.3

[U-Boot] [PATCH v1 2/4] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Ian Campbell]

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);
-- 
2.1.3

[U-Boot] [PATCH v1 3/4] jetson-tk1: Add PSCI configuration options and reserve secure code

[Ian Campbell]

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 */
-- 
2.1.3

[U-Boot] [PATCH v1 4/4] tegra124: Reserve secure RAM using MC_SECURITY_CFG{0, 1}_0

[Ian Campbell]

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);
-- 
2.1.3

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

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150114/be89bdb9/attachment.pgp>

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

[Ian Campbell]

On Wed, 2015-01-14 at 08:57 +0100, Thierry Reding wrote:
> > I also pushed my tree to gitorious:
> >         https://gitorious.org/ijc/u-boot jetson-psci-v1
> > 
> > I would Ack your patch, but I don't think you've posted it and it has no
> > S-o-b so that would seem a bit premature/rude of me. For the same reason
> > I've not actually included it in the series posted (but it is in the
> > gitorious branch).
> 
> Feel free to take ownership of that patch. I currently don't have the
> time to work on this and it seems you've made good progress on it.

Will do. Could you offer a S-o-b for it please so I can pick it up.

> It could probably use some cleanup because there's a bit of debug output
> still in there. Also...
> 
> > FWIW I think you could drop your stub versions of psci_cpu_off and
> > psci_cpu_suspend (assuming you don't want to implement them) since the
> > common code has stubs.
> 
> ... I'd think you'd need to implement these so that you can get proper
> suspend/resume support in the kernel. I've had to disable cpuidle (via
> #undef CONFIG_PM_SLEEP in arch/arm/mach-tegra/cpuidle-tegra114.c) in the
> kernel to make that code not powergate CPUs. Ideally I think the kernel
> would check that it's running with PSCI support and disable the cpuidle
> driver. Maybe that could be done by introducing a new cpuidle driver
> that checks for PSCI availability and uses it when present.

Hrm, I'm not sure how this all fits together, it's not a problem I've
noted before.

FWIW I think cpu_off and cpu_suspend are optional in PSCI v0.1 so an
initial version doesn't necessarily need to implement them (sunxi
doesn't for example), but as you say they do enable useful features.

> Adding Stephen and Tom for visibility.

Oops, sorry, I got them for the patches (via docs/git-mailrc) but forgot
about the cover letter.

Ian.

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

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150115/44f76207/attachment.pgp>

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

[Mark Rutland]

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.

[U-Boot] [PATCH v1 1/4] tegra124: Add more registers to struct mc_ctlr

[Stephen Warren]

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?

[U-Boot] [PATCH v1 2/4] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Stephen Warren]

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.

[U-Boot] [PATCH v1 3/4] jetson-tk1: Add PSCI configuration options and reserve secure code

[Stephen Warren]

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);
}

[U-Boot] [PATCH v1 3/4] jetson-tk1: Add PSCI configuration options and reserve secure code

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150116/428f8694/attachment.pgp>

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

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150116/33cab463/attachment.pgp>

[U-Boot] [PATCH v1 1/4] tegra124: Add more registers to struct mc_ctlr

[Ian Campbell]

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.

[U-Boot] [PATCH v1 2/4] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Ian Campbell]

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.

[U-Boot] [PATCH v1 3/4] jetson-tk1: Add PSCI configuration options and reserve secure code

[Ian Campbell]

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.

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

[Ian Campbell]

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.

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

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150116/c26b94e9/attachment.pgp>

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

[Ian Campbell]

On Fri, 2015-01-16 at 11:05 +0100, Thierry Reding wrote:
> On Fri, Jan 16, 2015 at 09:43:22AM +0000, Ian Campbell wrote:
> > On Thu, 2015-01-15 at 15:55 +0100, Thierry Reding wrote:
> > > On Wed, Jan 14, 2015 at 08:58:41AM +0000, Ian Campbell wrote:
> > > > On Wed, 2015-01-14 at 08:57 +0100, Thierry Reding wrote:
> > > > > > I also pushed my tree to gitorious:
> > > > > >         https://gitorious.org/ijc/u-boot jetson-psci-v1
> > > > > > 
> > > > > > I would Ack your patch, but I don't think you've posted it and it has no
> > > > > > S-o-b so that would seem a bit premature/rude of me. For the same reason
> > > > > > I've not actually included it in the series posted (but it is in the
> > > > > > gitorious branch).
> > > > > 
> > > > > Feel free to take ownership of that patch. I currently don't have the
> > > > > time to work on this and it seems you've made good progress on it.
> > > > 
> > > > Will do. Could you offer a S-o-b for it please so I can pick it up.
> > > 
> > > Sure:
> > > 
> > > Signed-off-by: Thierry Reding <treding@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.

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

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150116/6b9c0a6c/attachment.pgp>

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

[Ian Campbell]

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.

[U-Boot] [PATCH v1 2/4] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Ian Campbell]

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.

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

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150119/f37a87b3/attachment.pgp>

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

[Ian Campbell]

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.

[U-Boot] [PATCH v1 3/4] jetson-tk1: Add PSCI configuration options and reserve secure code

[Stephen Warren]

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.

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

[Mark Rutland]

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.

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

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150123/17f0802e/attachment.pgp>

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

[Mark Rutland]

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.html

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

[Mark Rutland]

[...]

> > > PSCI assumes that the FW is in full control of the registers it's
> > > poking. While a lock isn't necessarily bad, I suspect it's going to be
> > > very difficult to have that common across all users without the code
> > > becoming unmaintainable fast. I'd also hope that for arm64 we wouldn't
> > > need it.
> > > 
> > > When/how/why does the kernel to poke these registers?
> > 
> > The PMC is what controls power partitions. Some of these partitions are
> > assigned to CPUs, others are assigned to things like SATA, PCIe, display
> > and so on. The problem is that if we manage the CPU power partitions via
> > the firmware, then they will conflict with calls that we need to make
> > from other drivers that need to gate or ungate the partitions for their
> > hardware. As I understand it there's no provision in PSCI to manage non-
> > CPU devices, so this is a problem.
> 
> Ok.
> 
> > So I think either firmware needs to control everything, in which case we
> > are going to need a new interface (or extend PSCI) or it mustn't control
> > anything, in which case we need custom code in the kernel for SMP. Well,
> > the other alternative would be the lock that we can grab in the
> > powergate API and the PSCI calls.
> 
> One reason I'm not so keen on a lock is I could imagine you'd need to
> grab this for CPU_SUSPEND calls (i.e. cpuidle), at which point all CPUs
> are going to contend for the lock all the time.
> 
> One thing to bear in mind is that PSCI is only one user of the SMC
> space. Per SMC calling convention, portions of the SMC ID space are
> there to be used for other (vendor-specific) purposes.
> 
> So rather than extending PSCI, a parallel API could be implemented for
> power control of other devices, and the backend could arbitrate the two
> without the non-secure OS requiring implementation-specific mutual
> exclusion.
> 
> I think this has been brought up internally previously; I'll go and poke
> around in the area to see if we managed to figure out anything useful.

It sounds like what we figured out internally is roughly what I stated
above:

Allocate some SMC calls in the SIP and/or OEM Service Calls range for
vendor-specific device power management, and have the implementation on
the secure side (which would do the actual register poking) arbitrate
with any other secure-side access to those registers (i.e. CPU power
management, which it will already have to arbitrate).

Thanks,
Mark.

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

[Thierry Reding]

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

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

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

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

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

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

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150205/4970f08c/attachment.sig>

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

[Ian Campbell]

On Thu, 2015-02-05 at 12:44 +0100, Thierry Reding wrote:

> We've had some discussions about this internally and I think this should
> not be a problem after all. The Tegra flow controller can be programmed
> to automatically coordinate with the PMC to powergate CPUs when they
> encounter a WFI instruction and unpowergate CPUs again when they are
> woken up. With that in place the PMC registers don't need to be touched
> anymore once the CPUs have been booted once.
> 
> The solution that was discussed internally would involve having the
> secure monitor (U-Boot's PSCI implementation in this case) program the
> flow controller appropriately, point the CPU reset vectors to a location
> containing a WFI instruction and power up the CPUs. That way they should
> immediately be powergated when they reach the WFI instruction and the
> PSCI implementation would then be able to wake them up without accessing
> the PMC registers once the kernel has booted.
> 
> Adding Peter. Please correct me if I misunderstood what we discussed.
> Can you also provide Ian with pointers to the registers that need to be
> programmed to make this work? I suspect that a lot of it can be gleaned
> from the cpuidle drivers in arch/arm/mach-tegra in the upstream Linux
> kernel.

Thanks, any *info would be good even though I could likely decode it
from the driver...

I'm afraid I've been a bit slack about updating the series with the
other comments, mostly because I've been procrastinating over this
powergate issue.

Ian.

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

[Mark Rutland]

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.

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

[Thierry Reding]

On Thu, Feb 05, 2015 at 12:37:39PM +0000, Mark Rutland wrote:
> On Thu, Feb 05, 2015 at 11:44:25AM +0000, Thierry Reding wrote:
> > On Fri, Jan 23, 2015 at 12:37:20PM +0000, Mark Rutland wrote:
> > > On Fri, Jan 23, 2015 at 10:10:45AM +0000, Thierry Reding wrote:
> > > > On Thu, Jan 22, 2015 at 07:20:15PM +0000, Mark Rutland wrote:
> > > > > On Fri, Jan 16, 2015 at 09:12:59AM +0000, Thierry Reding wrote:
> > > > > > On Thu, Jan 15, 2015 at 07:19:37PM +0000, Mark Rutland wrote:
> > > > > > > On Wed, Jan 14, 2015 at 07:57:25AM +0000, Thierry Reding wrote:
> > > > > > > > On Tue, Jan 13, 2015 at 07:44:50PM +0000, Ian Campbell wrote:
> > > > > > > > > Hi Thierry,
> > > > > > > > > 
> > > > > > > > > I needed to boot my Jetson in NS mode (in order to boot Xen) and was
> > > > > > > > > investigating the possibility of PSCI support when I discovered that you
> > > > > > > > > had already started on it[0]. Hurrah!
> > > > > > > > > 
> > > > > > > > > I cherry-picked the relevant commit onto u-boot-tegra#master and added a
> > > > > > > > > few more patches and now it boots correctly for me, both running Xen
> > > > > > > > > (some Xen side patches are needed too) and native Linux.
> > > > > > > > > 
> > > > > > > > > The main things which was needed was to rebase for some recent Kconfig
> > > > > > > > > changes relating to virt and nonsec mode and to arrange for the RAM used
> > > > > > > > > by the secure code to be reserved in the FDT. I also reserved the RAM
> > > > > > > > > using the hardware MC_SECURITY_CFG registers for good measure.
> > > > > > > > 
> > > > > > > > Great, those were all things that I had wanted to do but never got
> > > > > > > > around to.
> > > > > > > > 
> > > > > > > > > I also pushed my tree to gitorious:
> > > > > > > > >         https://gitorious.org/ijc/u-boot jetson-psci-v1
> > > > > > > > > 
> > > > > > > > > I would Ack your patch, but I don't think you've posted it and it has no
> > > > > > > > > S-o-b so that would seem a bit premature/rude of me. For the same reason
> > > > > > > > > I've not actually included it in the series posted (but it is in the
> > > > > > > > > gitorious branch).
> > > > > > > > 
> > > > > > > > Feel free to take ownership of that patch. I currently don't have the
> > > > > > > > time to work on this and it seems you've made good progress on it.
> > > > > > > > 
> > > > > > > > It could probably use some cleanup because there's a bit of debug output
> > > > > > > > still in there. Also...
> > > > > > > > 
> > > > > > > > > FWIW I think you could drop your stub versions of psci_cpu_off and
> > > > > > > > > psci_cpu_suspend (assuming you don't want to implement them) since the
> > > > > > > > > common code has stubs.
> > > > > > > > 
> > > > > > > > ... I'd think you'd need to implement these so that you can get proper
> > > > > > > > suspend/resume support in the kernel. I've had to disable cpuidle (via
> > > > > > > > #undef CONFIG_PM_SLEEP in arch/arm/mach-tegra/cpuidle-tegra114.c) in the
> > > > > > > > kernel to make that code not powergate CPUs. Ideally I think the kernel
> > > > > > > > would check that it's running with PSCI support and disable the cpuidle
> > > > > > > > driver. Maybe that could be done by introducing a new cpuidle driver
> > > > > > > > that checks for PSCI availability and uses it when present.
> > > > > > > 
> > > > > > > We have a generic CPUidle driver on arm64 which can use PSCI as a
> > > > > > > backend; we should try to reuse that. The binding should certainly be
> > > > > > > identical.
> > > > > > 
> > > > > > Is there any reason that driver needs to be ARM64-specific? I would've
> > > > > > thought that there could be a generic PSCI driver that works anywhere.
> > > > > 
> > > > > Currently the arm and arm64 arch interfaces are a little different, but
> > > > > with some work the bulk of the code could certainly be made common
> > > > > (in drivers/firmware, perhaps).
> > > > > 
> > > > > > > It looks like the tegra124 dts in mainline doesn't use enable-method in
> > > > > > > the DT, so a better option might be to fail early in cpuidle-tegra114.c 
> > > > > > > if _any_ enable-method is present.
> > > > > > 
> > > > > > Yes, that sounds like a good plan. The absence of an enable-method would
> > > > > > signal that a kernel-native method (if any) should be used.
> > > > > > 
> > > > > > And this reminds me that we still need to find a way to synchronize
> > > > > > accesses to the powergate registers between secure firmware and the
> > > > > > kernel. Tegra has a set of hardware semaphores, but it seems like those
> > > > > > can only be used to synchronize between AVP and CPU, whereas for PSCI
> > > > > > we'd need something to synchronize between two CPUs. Do you know of any
> > > > > > existing mechanism to perform that type of synchronization?
> > > > > > 
> > > > > > Perhaps an option would be to add some sort of global lock in the kernel
> > > > > > which the cpuidle driver can grab before issuing the SMC instruction.
> > > > > 
> > > > > PSCI assumes that the FW is in full control of the registers it's
> > > > > poking. While a lock isn't necessarily bad, I suspect it's going to be
> > > > > very difficult to have that common across all users without the code
> > > > > becoming unmaintainable fast. I'd also hope that for arm64 we wouldn't
> > > > > need it.
> > > > > 
> > > > > When/how/why does the kernel to poke these registers?
> > > > 
> > > > The PMC is what controls power partitions. Some of these partitions are
> > > > assigned to CPUs, others are assigned to things like SATA, PCIe, display
> > > > and so on. The problem is that if we manage the CPU power partitions via
> > > > the firmware, then they will conflict with calls that we need to make
> > > > from other drivers that need to gate or ungate the partitions for their
> > > > hardware. As I understand it there's no provision in PSCI to manage non-
> > > > CPU devices, so this is a problem.
> > > 
> > > Ok.
> > > 
> > > > So I think either firmware needs to control everything, in which case we
> > > > are going to need a new interface (or extend PSCI) or it mustn't control
> > > > anything, in which case we need custom code in the kernel for SMP. Well,
> > > > the other alternative would be the lock that we can grab in the
> > > > powergate API and the PSCI calls.
> > > 
> > > One reason I'm not so keen on a lock is I could imagine you'd need to
> > > grab this for CPU_SUSPEND calls (i.e. cpuidle), at which point all CPUs
> > > are going to contend for the lock all the time.
> > 
> > We've had some discussions about this internally and I think this should
> > not be a problem after all. The Tegra flow controller can be programmed
> > to automatically coordinate with the PMC to powergate CPUs when they
> > encounter a WFI instruction and unpowergate CPUs again when they are
> > woken up. With that in place the PMC registers don't need to be touched
> > anymore once the CPUs have been booted once.
> > 
> > The solution that was discussed internally would involve having the
> > secure monitor (U-Boot's PSCI implementation in this case) program the
> > flow controller appropriately, point the CPU reset vectors to a location
> > containing a WFI instruction and power up the CPUs. That way they should
> > immediately be powergated when they reach the WFI instruction and the
> > PSCI implementation would then be able to wake them up without accessing
> > the PMC registers once the kernel has booted.
> 
> That sounds far, far better than I had hoped!
> 
> I guess we need to tell the kernel that portions of the PMC are reserved
> by FW (in the sense that they must not be modified by the kernel rather
> than that FW is going to poke them), to avoid mishaps.

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

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

> > Adding Peter. Please correct me if I misunderstood what we discussed.
> > Can you also provide Ian with pointers to the registers that need to be
> > programmed to make this work? I suspect that a lot of it can be gleaned
> > from the cpuidle drivers in arch/arm/mach-tegra in the upstream Linux
> > kernel.
> > 
> > Also adding Paul for visibility.
> > 
> > > One thing to bear in mind is that PSCI is only one user of the SMC
> > > space. Per SMC calling convention, portions of the SMC ID space are
> > > there to be used for other (vendor-specific) purposes.
> > > 
> > > So rather than extending PSCI, a parallel API could be implemented for
> > > power control of other devices, and the backend could arbitrate the two
> > > without the non-secure OS requiring implementation-specific mutual
> > > exclusion.
> > > 
> > > I think this has been brought up internally previously; I'll go and poke
> > > around in the area to see if we managed to figure out anything useful.
> > > 
> > > > Unfortunately this doesn't change on 64-bit Tegra at all.
> > > 
> > > I suspected as much. :/
> > > 
> > > How does this bode for the tegra132 dts [1] on LAKML at the moment? Is
> > > it just the "nvidia,tegra132-pmc" device that needs to be poked by both
> > > FW and kernel, or are other devices involved?
> > 
> > As I understand it, only the flow controller is involved with CPU power
> > management once the above steps have been performed by the secure
> > monitor. And I don't think anyone in the kernel would need access to the
> > flow controller at that point either, so I think that problem resolved
> > itself nicely.
> > 
> > Also note that the above should work as far back as Tegra30.
> 
> It would be amazing if we could gain PSCI for all the platforms that
> covers!

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

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

Thierry

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150205/1e190257/attachment.sig>

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

[Ian Campbell]

On Thu, 2015-02-05 at 14:55 +0100, Thierry Reding wrote:
> Ian, are you planning on revising the series based on the outcome above?

Yes, although I don't have any 114 or 30 hardware, just a single Jetson.

(there should probably be an "eventually" near the start of that
sentence, I'm travelling for the next 10 days and don't have remote
power control...)

Ian.

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

[Mark Rutland]

[...]

> > > The solution that was discussed internally would involve having the
> > > secure monitor (U-Boot's PSCI implementation in this case) program the
> > > flow controller appropriately, point the CPU reset vectors to a location
> > > containing a WFI instruction and power up the CPUs. That way they should
> > > immediately be powergated when they reach the WFI instruction and the
> > > PSCI implementation would then be able to wake them up without accessing
> > > the PMC registers once the kernel has booted.
> > 
> > That sounds far, far better than I had hoped!
> > 
> > I guess we need to tell the kernel that portions of the PMC are reserved
> > by FW (in the sense that they must not be modified by the kernel rather
> > than that FW is going to poke them), to avoid mishaps.
> 
> I'm not sure we need even that. As I understand it the kernel can still
> touch all the registers and none of it should influence the CPU power-
> gating done by the secure monitor.
> 
> Well, I guess you'd need to make sure that the PMC driver doesn't try to
> powergate or unpowergate the CPU partitions, but since the cpuidle
> driver is the only one doing that it should resolve itself if a generic,
> PSCI-based cpuidle driver takes over instead of a Tegra-specific one.

This was my concern. It would be good to avoid a case where we
accidentally rely on some subtle interactiion where both the FW and
kernel poke some registers in a particular way.

I guess we can check for the presence of an enable-method, and if there
is one don't register the Tegra-specific cpuidle driver; in that case we
expect the FW to own that side of things.

> > > Adding Peter. Please correct me if I misunderstood what we discussed.
> > > Can you also provide Ian with pointers to the registers that need to be
> > > programmed to make this work? I suspect that a lot of it can be gleaned
> > > from the cpuidle drivers in arch/arm/mach-tegra in the upstream Linux
> > > kernel.
> > > 
> > > Also adding Paul for visibility.
> > > 
> > > > One thing to bear in mind is that PSCI is only one user of the SMC
> > > > space. Per SMC calling convention, portions of the SMC ID space are
> > > > there to be used for other (vendor-specific) purposes.
> > > > 
> > > > So rather than extending PSCI, a parallel API could be implemented for
> > > > power control of other devices, and the backend could arbitrate the two
> > > > without the non-secure OS requiring implementation-specific mutual
> > > > exclusion.
> > > > 
> > > > I think this has been brought up internally previously; I'll go and poke
> > > > around in the area to see if we managed to figure out anything useful.
> > > > 
> > > > > Unfortunately this doesn't change on 64-bit Tegra at all.
> > > > 
> > > > I suspected as much. :/
> > > > 
> > > > How does this bode for the tegra132 dts [1] on LAKML at the moment? Is
> > > > it just the "nvidia,tegra132-pmc" device that needs to be poked by both
> > > > FW and kernel, or are other devices involved?
> > > 
> > > As I understand it, only the flow controller is involved with CPU power
> > > management once the above steps have been performed by the secure
> > > monitor. And I don't think anyone in the kernel would need access to the
> > > flow controller at that point either, so I think that problem resolved
> > > itself nicely.
> > > 
> > > Also note that the above should work as far back as Tegra30.
> > 
> > It would be amazing if we could gain PSCI for all the platforms that
> > covers!
> 
> It should be relatively easy to support at least Tegra114 with much the
> same code as Tegra124, and some slight changes on Tegra30. But yeah, it
> would be great to see this work.

Nice!

I should look into getting hold of a relevant platform; I only have a
(T20) AC100, and I guess that's a bit different at the system-level.

Thanks,
Mark.

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

[Jan Kiszka]

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150214/03ec12f2/attachment.sig>

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

[Ian Campbell]

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.