[U-Boot] [PATCH 0/4] ARM: tegra: GPU WPR region support

[U-Boot] [PATCH 0/4] ARM: tegra: GPU WPR region support

[Alexandre Courbot]

This series makes U-boot program the write-protected (WPR) region of T210 chips,
allowing the kernel to perform GPU secure firmware loading.

Tegra 210's GPU requires its firmware to be loaded though a write-protected
region. An area of physical memory is carved-out, programmed into the
corresponding memory controller registers, and locked such as only the GPU can
write into it. This area needs to be set up by the bootloader since it cannot
be re-claimed for normal use after being locked.

The first 3 patches of this series are cleanup patches. Patch 2 implements a
suggestion made by Stephen, patch 3 renames GPU-related functions to sound less
generic.

The last patch adds support for the GPU WPR region. The top 256KB of memory are
removed from the available memory, and the corresponding MC registers are
programmed to point to it, which allows the kernel to initiate secure firmware
loading.

Alexandre Courbot (4):
  ARM: tegra: remove vpr_configured() function
  ARM: tegra: simplify GPU setup
  ARM: tegra: rename GPU functions
  ARM: tegra210: gpu: configure WPR region

 arch/arm/include/asm/arch-tegra/gpu.h   | 14 +++------
 arch/arm/include/asm/arch-tegra210/mc.h | 12 ++++++++
 arch/arm/mach-tegra/board.c             |  4 +++
 arch/arm/mach-tegra/board2.c            | 22 +++++++++++++-
 arch/arm/mach-tegra/gpu.c               | 52 +++++++++++++++++++++++++++++----
 board/nvidia/jetson-tk1/jetson-tk1.c    |  8 -----
 board/nvidia/p2571/p2571.c              |  7 -----
 board/nvidia/venice2/venice2.c          |  8 -----
 include/configs/jetson-tk1.h            |  2 --
 include/configs/p2571.h                 |  2 --
 include/configs/tegra-common.h          |  2 ++
 include/configs/venice2.h               |  2 --
 12 files changed, 89 insertions(+), 46 deletions(-)

-- 
2.6.1

[U-Boot] [PATCH 1/4] ARM: tegra: remove vpr_configured() function

[Alexandre Courbot]

There is no justification for this function, especially in exported
form.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/include/asm/arch-tegra/gpu.h | 6 ------
 arch/arm/mach-tegra/gpu.c             | 7 +------
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/arch/arm/include/asm/arch-tegra/gpu.h b/arch/arm/include/asm/arch-tegra/gpu.h
index 52280f40ce31..2fdb2c5049e6 100644
--- a/arch/arm/include/asm/arch-tegra/gpu.h
+++ b/arch/arm/include/asm/arch-tegra/gpu.h
@@ -11,7 +11,6 @@
 #if defined(CONFIG_TEGRA_GPU)
 
 void config_gpu(void);
-bool gpu_configured(void);
 
 #else /* CONFIG_TEGRA_GPU */
 
@@ -19,11 +18,6 @@ static inline void config_gpu(void)
 {
 }
 
-static inline bool gpu_configured(void)
-{
-	return false;
-}
-
 #endif /* CONFIG_TEGRA_GPU */
 
 #if defined(CONFIG_OF_LIBFDT)
diff --git a/arch/arm/mach-tegra/gpu.c b/arch/arm/mach-tegra/gpu.c
index 4ea046d3e5b6..dc29b79e0126 100644
--- a/arch/arm/mach-tegra/gpu.c
+++ b/arch/arm/mach-tegra/gpu.c
@@ -41,18 +41,13 @@ void config_gpu(void)
 	_configured = true;
 }
 
-bool vpr_configured(void)
-{
-	return _configured;
-}
-
 #if defined(CONFIG_OF_LIBFDT)
 
 int gpu_enable_node(void *blob, const char *gpupath)
 {
 	int offset;
 
-	if (vpr_configured()) {
+	if (_configured) {
 		offset = fdt_path_offset(blob, gpupath);
 		if (offset > 0) {
 			fdt_status_okay(blob, offset);
-- 
2.6.1

[U-Boot] [PATCH 2/4] ARM: tegra: simplify GPU setup

[Alexandre Courbot]

Enable the GPU node in the system-wide ft_system_setup() hook instead of
the board-specific ft_board_hook(). This allows us to enable GPU per SoC
generation instead of per-board as we did initially.

Reported-by: Stephen Warren <swarren@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/mach-tegra/board2.c         | 20 ++++++++++++++++++++
 board/nvidia/jetson-tk1/jetson-tk1.c |  8 --------
 board/nvidia/p2571/p2571.c           |  7 -------
 board/nvidia/venice2/venice2.c       |  8 --------
 include/configs/jetson-tk1.h         |  2 --
 include/configs/p2571.h              |  2 --
 include/configs/tegra-common.h       |  2 ++
 include/configs/venice2.h            |  2 --
 8 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/arch/arm/mach-tegra/board2.c b/arch/arm/mach-tegra/board2.c
index 8ecc67459a10..ff9e77cfa3a3 100644
--- a/arch/arm/mach-tegra/board2.c
+++ b/arch/arm/mach-tegra/board2.c
@@ -403,3 +403,23 @@ ulong board_get_usable_ram_top(ulong total_size)
 {
 	return CONFIG_SYS_SDRAM_BASE + usable_ram_size_below_4g();
 }
+
+/*
+ * This function is called right before the kernel is booted. "blob" is the
+ * device tree that will be passed to the kernel.
+ */
+int ft_system_setup(void *blob, bd_t *bd)
+{
+	const char *gpu_path =
+#if defined(CONFIG_TEGRA124) || defined(CONFIG_TEGRA210)
+		"/gpu at 0,57000000";
+#else
+		NULL;
+#endif
+
+	/* Enable GPU node if GPU setup has been performed */
+	if (gpu_path != NULL)
+		return gpu_enable_node(blob, gpu_path);
+
+	return 0;
+}
diff --git a/board/nvidia/jetson-tk1/jetson-tk1.c b/board/nvidia/jetson-tk1/jetson-tk1.c
index 3c21767ce4da..52425a8f6dea 100644
--- a/board/nvidia/jetson-tk1/jetson-tk1.c
+++ b/board/nvidia/jetson-tk1/jetson-tk1.c
@@ -11,7 +11,6 @@
 
 #include <asm/arch/gpio.h>
 #include <asm/arch/pinmux.h>
-#include <asm/arch-tegra/gpu.h>
 
 #include "pinmux-config-jetson-tk1.h"
 
@@ -80,10 +79,3 @@ int board_eth_init(bd_t *bis)
 	return pci_eth_init(bis);
 }
 #endif /* PCI */
-
-int ft_board_setup(void *blob, bd_t *bd)
-{
-	gpu_enable_node(blob, "/gpu at 0,57000000");
-
-	return 0;
-}
diff --git a/board/nvidia/p2571/p2571.c b/board/nvidia/p2571/p2571.c
index d33e4d12b2fa..d80a7d0d3e31 100644
--- a/board/nvidia/p2571/p2571.c
+++ b/board/nvidia/p2571/p2571.c
@@ -11,7 +11,6 @@
 #include <asm/arch/pinmux.h>
 #include <asm/gpio.h>
 #include "max77620_init.h"
-#include <asm/arch-tegra/gpu.h>
 #include "pinmux-config-p2571.h"
 
 void pin_mux_mmc(void)
@@ -62,9 +61,3 @@ void start_cpu_fan(void)
 	gpio_request(GPIO_PE4, "FAN_VDD");
 	gpio_direction_output(GPIO_PE4, 1);
 }
-
-int ft_board_setup(void *blob, bd_t *bd)
-{
-	gpu_enable_node(blob, "/gpu at 0,57000000");
-	return 0;
-}
diff --git a/board/nvidia/venice2/venice2.c b/board/nvidia/venice2/venice2.c
index 3e2b9a7745e9..c56ef129d6c7 100644
--- a/board/nvidia/venice2/venice2.c
+++ b/board/nvidia/venice2/venice2.c
@@ -8,7 +8,6 @@
 #include <common.h>
 #include <asm/arch/gpio.h>
 #include <asm/arch/pinmux.h>
-#include <asm/arch-tegra/gpu.h>
 #include "pinmux-config-venice2.h"
 
 /*
@@ -28,10 +27,3 @@ void pinmux_init(void)
 	pinmux_config_drvgrp_table(venice2_drvgrps,
 				   ARRAY_SIZE(venice2_drvgrps));
 }
-
-int ft_board_setup(void *blob, bd_t *bd)
-{
-	gpu_enable_node(blob, "/gpu at 0,57000000");
-
-	return 0;
-}
diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
index e87a01047d30..f63957ab92fd 100644
--- a/include/configs/jetson-tk1.h
+++ b/include/configs/jetson-tk1.h
@@ -78,6 +78,4 @@
 #define CONFIG_ARMV7_SECURE_BASE		0xfff00000
 #define CONFIG_ARMV7_SECURE_RESERVE_SIZE	0x00100000
 
-#define CONFIG_OF_BOARD_SETUP
-
 #endif /* __CONFIG_H */
diff --git a/include/configs/p2571.h b/include/configs/p2571.h
index c65d3e5fcbc1..a5de411121b0 100644
--- a/include/configs/p2571.h
+++ b/include/configs/p2571.h
@@ -60,6 +60,4 @@
 #include "tegra-common-usb-gadget.h"
 #include "tegra-common-post.h"
 
-#define CONFIG_OF_BOARD_SETUP
-
 #endif /* _P2571_H */
diff --git a/include/configs/tegra-common.h b/include/configs/tegra-common.h
index 1c469d092e8c..9f0d4644431a 100644
--- a/include/configs/tegra-common.h
+++ b/include/configs/tegra-common.h
@@ -144,4 +144,6 @@
 #define CONFIG_FAT_WRITE
 #endif
 
+#define CONFIG_OF_SYSTEM_SETUP
+
 #endif /* _TEGRA_COMMON_H_ */
diff --git a/include/configs/venice2.h b/include/configs/venice2.h
index 0fc8cf7674d1..a374cd948849 100644
--- a/include/configs/venice2.h
+++ b/include/configs/venice2.h
@@ -60,6 +60,4 @@
 #include "tegra-common-usb-gadget.h"
 #include "tegra-common-post.h"
 
-#define CONFIG_OF_BOARD_SETUP
-
 #endif /* __CONFIG_H */
-- 
2.6.1

[U-Boot] [PATCH 3/4] ARM: tegra: rename GPU functions

[Alexandre Courbot]

Rename GPU functions to less generic names to avoid potential name
collisions.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/include/asm/arch-tegra/gpu.h | 8 ++++----
 arch/arm/mach-tegra/board2.c          | 4 ++--
 arch/arm/mach-tegra/gpu.c             | 4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/arm/include/asm/arch-tegra/gpu.h b/arch/arm/include/asm/arch-tegra/gpu.h
index 2fdb2c5049e6..4423386f2805 100644
--- a/arch/arm/include/asm/arch-tegra/gpu.h
+++ b/arch/arm/include/asm/arch-tegra/gpu.h
@@ -10,11 +10,11 @@
 
 #if defined(CONFIG_TEGRA_GPU)
 
-void config_gpu(void);
+void tegra_gpu_config(void);
 
 #else /* CONFIG_TEGRA_GPU */
 
-static inline void config_gpu(void)
+static inline void tegra_gpu_config(void)
 {
 }
 
@@ -22,11 +22,11 @@ static inline void config_gpu(void)
 
 #if defined(CONFIG_OF_LIBFDT)
 
-int gpu_enable_node(void *blob, const char *gpupath);
+int tegra_gpu_enable_node(void *blob, const char *gpupath);
 
 #else /* CONFIG_OF_LIBFDT */
 
-static inline int gpu_enable_node(void *blob, const char *gpupath)
+static inline int tegra_gpu_enable_node(void *blob, const char *gpupath)
 {
 	return 0;
 }
diff --git a/arch/arm/mach-tegra/board2.c b/arch/arm/mach-tegra/board2.c
index ff9e77cfa3a3..8ba143d996ca 100644
--- a/arch/arm/mach-tegra/board2.c
+++ b/arch/arm/mach-tegra/board2.c
@@ -128,7 +128,7 @@ int board_init(void)
 	clock_init();
 	clock_verify();
 
-	config_gpu();
+	tegra_gpu_config();
 
 #ifdef CONFIG_TEGRA_SPI
 	pin_mux_spi();
@@ -419,7 +419,7 @@ int ft_system_setup(void *blob, bd_t *bd)
 
 	/* Enable GPU node if GPU setup has been performed */
 	if (gpu_path != NULL)
-		return gpu_enable_node(blob, gpu_path);
+		return tegra_gpu_enable_node(blob, gpu_path);
 
 	return 0;
 }
diff --git a/arch/arm/mach-tegra/gpu.c b/arch/arm/mach-tegra/gpu.c
index dc29b79e0126..c7d705d8efe9 100644
--- a/arch/arm/mach-tegra/gpu.c
+++ b/arch/arm/mach-tegra/gpu.c
@@ -25,7 +25,7 @@
 
 static bool _configured;
 
-void config_gpu(void)
+void tegra_gpu_config(void)
 {
 	struct mc_ctlr *mc = (struct mc_ctlr *)NV_PA_MC_BASE;
 
@@ -43,7 +43,7 @@ void config_gpu(void)
 
 #if defined(CONFIG_OF_LIBFDT)
 
-int gpu_enable_node(void *blob, const char *gpupath)
+int tegra_gpu_enable_node(void *blob, const char *gpupath)
 {
 	int offset;
 
-- 
2.6.1

[U-Boot] [PATCH 3/4] ARM: tegra: rename GPU functions

[Stephen Warren]

On 10/18/2015 10:57 PM, Alexandre Courbot wrote:
> Rename GPU functions to less generic names to avoid potential name
> collisions.

Patches 1-3,
Acked-by: Stephen Warren <swarren@nvidia.com>

[U-Boot] [PATCH 4/4] ARM: tegra210: gpu: configure WPR region

[Alexandre Courbot]

T210's GPU secure firmware loading requires a write-protected region
to be set up.

This patch reserves the upper 256KB of RAM as the WPR region and locks
it so the kernel can initiate secure firmware loading.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/include/asm/arch-tegra210/mc.h | 12 +++++++++
 arch/arm/mach-tegra/board.c             |  4 +++
 arch/arm/mach-tegra/gpu.c               | 47 ++++++++++++++++++++++++++++++++-
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/arch-tegra210/mc.h b/arch/arm/include/asm/arch-tegra210/mc.h
index 77e9aa51f60f..e6d1758d372f 100644
--- a/arch/arm/include/asm/arch-tegra210/mc.h
+++ b/arch/arm/include/asm/arch-tegra210/mc.h
@@ -62,6 +62,16 @@ struct mc_ctlr {
 	u32 mc_video_protect_bom;		/* offset 0x648 */
 	u32 mc_video_protect_size_mb;		/* offset 0x64c */
 	u32 mc_video_protect_reg_ctrl;		/* offset 0x650 */
+	u32 reserved11[385];			/* offset 0x654 - 0xc54 */
+	u32 mc_security_carveout2_cfg0;		/* offset 0xc58 */
+	u32 mc_security_carveout2_bom;		/* offset 0xc5c */
+	u32 mc_security_carveout2_bom_hi;	/* offset 0xc60 */
+	u32 mc_security_carveout2_size_128k;	/* offset 0xc64 */
+	u32 reserved12[16];			/* offset 0xc68 - 0xca4 */
+	u32 mc_security_carveout3_cfg0;		/* offset 0xca8 */
+	u32 mc_security_carveout3_bom;		/* offset 0xcac */
+	u32 mc_security_carveout3_bom_hi;	/* offset 0xcb0 */
+	u32 mc_security_carveout3_size_128k;	/* offset 0xcb4 */
 };
 
 #define TEGRA_MC_SMMU_CONFIG_ENABLE (1 << 0)
@@ -69,4 +79,6 @@ struct mc_ctlr {
 #define TEGRA_MC_VIDEO_PROTECT_REG_WRITE_ACCESS_ENABLED		(0 << 0)
 #define TEGRA_MC_VIDEO_PROTECT_REG_WRITE_ACCESS_DISABLED	(1 << 0)
 
+#define TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED			(1 << 1)
+
 #endif	/* _TEGRA210_MC_H_ */
diff --git a/arch/arm/mach-tegra/board.c b/arch/arm/mach-tegra/board.c
index b00e4b5c1e25..0bff063b00f4 100644
--- a/arch/arm/mach-tegra/board.c
+++ b/arch/arm/mach-tegra/board.c
@@ -111,6 +111,10 @@ static phys_size_t query_sdram_size(void)
 	if (size_bytes == SZ_2G)
 		size_bytes -= SZ_1M;
 #endif
+#if defined(CONFIG_TEGRA210)
+	/* Reserve GPU WPR area, 2 * 128KB */
+	size_bytes = round_down(size_bytes - (SZ_128K * 2), SZ_128K);
+#endif
 
 	return size_bytes;
 }
diff --git a/arch/arm/mach-tegra/gpu.c b/arch/arm/mach-tegra/gpu.c
index c7d705d8efe9..61d734fd5767 100644
--- a/arch/arm/mach-tegra/gpu.c
+++ b/arch/arm/mach-tegra/gpu.c
@@ -23,9 +23,11 @@
 
 #include <fdt_support.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 static bool _configured;
 
-void tegra_gpu_config(void)
+static void config_vpr(void)
 {
 	struct mc_ctlr *mc = (struct mc_ctlr *)NV_PA_MC_BASE;
 
@@ -37,6 +39,49 @@ void tegra_gpu_config(void)
 	readl(&mc->mc_video_protect_reg_ctrl);
 
 	debug("configured VPR\n");
+}
+
+#if defined(CONFIG_TEGRA210)
+static void config_wpr(void)
+{
+	struct mc_ctlr *mc = (struct mc_ctlr *)NV_PA_MC_BASE;
+	u64 wpr_start = NV_PA_SDRAM_BASE + gd->ram_size;
+	u32 reg;
+
+	/*
+	 * Carveout2 uses the upper 256KB of upper memory that we reserved as
+	 * WPR region for secure firmware loading
+	 */
+	writel(lower_32_bits(wpr_start), &mc->mc_security_carveout2_bom);
+	writel(upper_32_bits(wpr_start), &mc->mc_security_carveout2_bom_hi);
+	writel(0x2, &mc->mc_security_carveout2_size_128k);
+	reg = readl(&mc->mc_security_carveout2_cfg0);
+	reg |= TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED;
+	writel(reg, &mc->mc_security_carveout2_cfg0);
+
+	/* Carveout3 is left empty */
+	writel(0x0, &mc->mc_security_carveout3_bom);
+	writel(0x0, &mc->mc_security_carveout3_bom_hi);
+	writel(0x0, &mc->mc_security_carveout3_size_128k);
+	reg = readl(&mc->mc_security_carveout3_cfg0);
+	reg |= TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED;
+	writel(reg, &mc->mc_security_carveout3_cfg0);
+
+	/* read back to ensure the write went through */
+	readl(&mc->mc_security_carveout3_cfg0);
+
+	debug("configured WPR\n");
+}
+#else
+static inline void config_wpr(void)
+{
+}
+#endif
+
+void tegra_gpu_config(void)
+{
+	config_vpr();
+	config_wpr();
 
 	_configured = true;
 }
-- 
2.6.1

[U-Boot] [PATCH 4/4] ARM: tegra210: gpu: configure WPR region

[Stephen Warren]

On 10/18/2015 10:57 PM, Alexandre Courbot wrote:
> T210's GPU secure firmware loading requires a write-protected region
> to be set up.
>
> This patch reserves the upper 256KB of RAM as the WPR region and locks
> it so the kernel can initiate secure firmware loading.

On T210, it's the responsibility of nvtboot (which runs before U-Boot) 
to set up any and all carve-outs. This code should not be necessary, and 
indeed I expect the registers it touches can't actually be programmed 
from U-Boot, which runs in non-secure mode after WPR is already locked.

[U-Boot] [PATCH 4/4] ARM: tegra210: gpu: configure WPR region

[Alexandre Courbot]

On 10/29/2015 02:59 AM, Stephen Warren wrote:
> On 10/18/2015 10:57 PM, Alexandre Courbot wrote:
>> T210's GPU secure firmware loading requires a write-protected region
>> to be set up.
>>
>> This patch reserves the upper 256KB of RAM as the WPR region and locks
>> it so the kernel can initiate secure firmware loading.
>
> On T210, it's the responsibility of nvtboot (which runs before U-Boot)
> to set up any and all carve-outs. This code should not be necessary, and
> indeed I expect the registers it touches can't actually be programmed
> from U-Boot, which runs in non-secure mode after WPR is already locked.

Ok, I was running from Thierry's miniloader which did not program or 
lock these registers.

The question then is: do we have an official nvtboot binary available 
for upstream support? AFAICT Thierry's miniloader solution is the only 
solution available to the public, and without this setup one cannot use 
the GPU on T210.

[U-Boot] [PATCH 4/4] ARM: tegra210: gpu: configure WPR region

[Stephen Warren]

On 10/28/2015 05:55 PM, Alexandre Courbot wrote:
> On 10/29/2015 02:59 AM, Stephen Warren wrote:
>> On 10/18/2015 10:57 PM, Alexandre Courbot wrote:
>>> T210's GPU secure firmware loading requires a write-protected region
>>> to be set up.
>>>
>>> This patch reserves the upper 256KB of RAM as the WPR region and locks
>>> it so the kernel can initiate secure firmware loading.
>>
>> On T210, it's the responsibility of nvtboot (which runs before U-Boot)
>> to set up any and all carve-outs. This code should not be necessary, and
>> indeed I expect the registers it touches can't actually be programmed
>> from U-Boot, which runs in non-secure mode after WPR is already locked.
> 
> Ok, I was running from Thierry's miniloader which did not program or
> lock these registers.
> 
> The question then is: do we have an official nvtboot binary available
> for upstream support? AFAICT Thierry's miniloader solution is the only
> solution available to the public, and without this setup one cannot use
> the GPU on T210.

Once L4T for T210 has been released, it'll contain a full set of nvtboot
binaries and flashing support. You an easily acquire this internally
already; send me an email if you need a link. I'd expect a public SW
release by the time any T210 boards capable of running upstream are
available.

[U-Boot] [PATCH 4/4] ARM: tegra210: gpu: configure WPR region

[Thierry Reding]

On Wed, Oct 28, 2015 at 11:59:04AM -0600, Stephen Warren wrote:
> On 10/18/2015 10:57 PM, Alexandre Courbot wrote:
> >T210's GPU secure firmware loading requires a write-protected region
> >to be set up.
> >
> >This patch reserves the upper 256KB of RAM as the WPR region and locks
> >it so the kernel can initiate secure firmware loading.
> 
> On T210, it's the responsibility of nvtboot (which runs before U-Boot) to
> set up any and all carve-outs. This code should not be necessary, and indeed
> I expect the registers it touches can't actually be programmed from U-Boot,
> which runs in non-secure mode after WPR is already locked.

Can we document this assumption somewhere? It's entirely possible that
someone might want to run U-Boot without nvtboot, in which case nvtboot
would still need to be responsible for setting this up. Or if it isn't
we could still point at some location where the interactions between a
first stage bootloader and U-Boot are documented.

Do we have a document of this kind already?

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

[U-Boot] [PATCH 4/4] ARM: tegra210: gpu: configure WPR region

[Stephen Warren]

On 11/09/2015 07:36 AM, Thierry Reding wrote:
> On Wed, Oct 28, 2015 at 11:59:04AM -0600, Stephen Warren wrote:
>> On 10/18/2015 10:57 PM, Alexandre Courbot wrote:
>>> T210's GPU secure firmware loading requires a write-protected region
>>> to be set up.
>>>
>>> This patch reserves the upper 256KB of RAM as the WPR region and locks
>>> it so the kernel can initiate secure firmware loading.
>>
>> On T210, it's the responsibility of nvtboot (which runs before U-Boot) to
>> set up any and all carve-outs. This code should not be necessary, and indeed
>> I expect the registers it touches can't actually be programmed from U-Boot,
>> which runs in non-secure mode after WPR is already locked.
>
> Can we document this assumption somewhere? It's entirely possible that
> someone might want to run U-Boot without nvtboot

That's not currently a supported use-case.

> , in which case nvtboot
> would still need to be responsible for setting this up.

I assume s/nvtboot/whatever other bootloader is in use/?

> Or if it isn't
> we could still point at some location where the interactions between a
> first stage bootloader and U-Boot are documented.
>
> Do we have a document of this kind already?

The L4T U-Boot source code:-)

[U-Boot] [PATCH 4/4] ARM: tegra210: gpu: configure WPR region

[Thierry Reding]

On Mon, Nov 09, 2015 at 08:19:48AM -0700, Stephen Warren wrote:
> On 11/09/2015 07:36 AM, Thierry Reding wrote:
> >On Wed, Oct 28, 2015 at 11:59:04AM -0600, Stephen Warren wrote:
> >>On 10/18/2015 10:57 PM, Alexandre Courbot wrote:
> >>>T210's GPU secure firmware loading requires a write-protected region
> >>>to be set up.
> >>>
> >>>This patch reserves the upper 256KB of RAM as the WPR region and locks
> >>>it so the kernel can initiate secure firmware loading.
> >>
> >>On T210, it's the responsibility of nvtboot (which runs before U-Boot) to
> >>set up any and all carve-outs. This code should not be necessary, and indeed
> >>I expect the registers it touches can't actually be programmed from U-Boot,
> >>which runs in non-secure mode after WPR is already locked.
> >
> >Can we document this assumption somewhere? It's entirely possible that
> >someone might want to run U-Boot without nvtboot
> 
> That's not currently a supported use-case.

Isn't the whole point of open-source for people to be able to do all the
things that aren't supported?

> >, in which case nvtboot
> >would still need to be responsible for setting this up.
> 
> I assume s/nvtboot/whatever other bootloader is in use/?

Right, I was going to write U-Boot here, but that's obviously only true
if U-Boot is still running in secure mode. Otherwise, yes, it'd be what-
ever is the other bootloader being used.

> >Or if it isn't
> >we could still point at some location where the interactions between a
> >first stage bootloader and U-Boot are documented.
> >
> >Do we have a document of this kind already?
> 
> The L4T U-Boot source code:-)

There's a bunch of things that even the L4T U-Boot doesn't document. The
fact that it doesn't initialize the SMMU or WPR doesn't indicate that it
is something that a first stage bootloader needs to do.

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

[U-Boot] [PATCH 4/4] ARM: tegra210: gpu: configure WPR region

[Stephen Warren]

On 11/09/2015 08:48 AM, Thierry Reding wrote:
> On Mon, Nov 09, 2015 at 08:19:48AM -0700, Stephen Warren wrote:
>> On 11/09/2015 07:36 AM, Thierry Reding wrote:
>>> On Wed, Oct 28, 2015 at 11:59:04AM -0600, Stephen Warren wrote:
>>>> On 10/18/2015 10:57 PM, Alexandre Courbot wrote:
>>>>> T210's GPU secure firmware loading requires a write-protected region
>>>>> to be set up.
>>>>>
>>>>> This patch reserves the upper 256KB of RAM as the WPR region and locks
>>>>> it so the kernel can initiate secure firmware loading.
>>>>
>>>> On T210, it's the responsibility of nvtboot (which runs before U-Boot) to
>>>> set up any and all carve-outs. This code should not be necessary, and indeed
>>>> I expect the registers it touches can't actually be programmed from U-Boot,
>>>> which runs in non-secure mode after WPR is already locked.
>>>
>>> Can we document this assumption somewhere? It's entirely possible that
>>> someone might want to run U-Boot without nvtboot
>>
>> That's not currently a supported use-case.
>
> Isn't the whole point of open-source for people to be able to do all the
> things that aren't supported?

Sure, but the default case that we support should "just work", and be 
represented by the code we ship, and be supported by us. Any non-default 
case is going to require some research or additional coding/fixing/... 
effort, and we haven't necessarily published the information require to 
make those cases work.

>>> , in which case nvtboot
>>> would still need to be responsible for setting this up.
>>
>> I assume s/nvtboot/whatever other bootloader is in use/?
>
> Right, I was going to write U-Boot here, but that's obviously only true
> if U-Boot is still running in secure mode. Otherwise, yes, it'd be what-
> ever is the other bootloader being used.
>
>>> Or if it isn't
>>> we could still point at some location where the interactions between a
>>> first stage bootloader and U-Boot are documented.
>>>
>>> Do we have a document of this kind already?
>>
>> The L4T U-Boot source code:-)
>
> There's a bunch of things that even the L4T U-Boot doesn't document. The
> fact that it doesn't initialize the SMMU or WPR doesn't indicate that it
> is something that a first stage bootloader needs to do.

I meant this more along the lines of "if there's no code to do something 
in L4T U-Boot, upstream U-Boot wouldn't need that code either". That 
should be true at least w.r.t. nvtboot integration; certainly there are 
features that only exist in upstream U-Boot and haven't been back-ported 
downstream. I wasn't really implying that there's actual documentation 
(comments) in the L4T U-Boot source code.

[U-Boot] [PATCH 0/4] ARM: tegra: GPU WPR region support

[Alexandre Courbot]

Ping Tom, how does this look to you?

On Mon, Oct 19, 2015 at 1:57 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> This series makes U-boot program the write-protected (WPR) region of T210 chips,
> allowing the kernel to perform GPU secure firmware loading.
>
> Tegra 210's GPU requires its firmware to be loaded though a write-protected
> region. An area of physical memory is carved-out, programmed into the
> corresponding memory controller registers, and locked such as only the GPU can
> write into it. This area needs to be set up by the bootloader since it cannot
> be re-claimed for normal use after being locked.
>
> The first 3 patches of this series are cleanup patches. Patch 2 implements a
> suggestion made by Stephen, patch 3 renames GPU-related functions to sound less
> generic.
>
> The last patch adds support for the GPU WPR region. The top 256KB of memory are
> removed from the available memory, and the corresponding MC registers are
> programmed to point to it, which allows the kernel to initiate secure firmware
> loading.
>
> Alexandre Courbot (4):
>   ARM: tegra: remove vpr_configured() function
>   ARM: tegra: simplify GPU setup
>   ARM: tegra: rename GPU functions
>   ARM: tegra210: gpu: configure WPR region
>
>  arch/arm/include/asm/arch-tegra/gpu.h   | 14 +++------
>  arch/arm/include/asm/arch-tegra210/mc.h | 12 ++++++++
>  arch/arm/mach-tegra/board.c             |  4 +++
>  arch/arm/mach-tegra/board2.c            | 22 +++++++++++++-
>  arch/arm/mach-tegra/gpu.c               | 52 +++++++++++++++++++++++++++++----
>  board/nvidia/jetson-tk1/jetson-tk1.c    |  8 -----
>  board/nvidia/p2571/p2571.c              |  7 -----
>  board/nvidia/venice2/venice2.c          |  8 -----
>  include/configs/jetson-tk1.h            |  2 --
>  include/configs/p2571.h                 |  2 --
>  include/configs/tegra-common.h          |  2 ++
>  include/configs/venice2.h               |  2 --
>  12 files changed, 89 insertions(+), 46 deletions(-)
>
> --
> 2.6.1
>

[U-Boot] [PATCH 0/4] ARM: tegra: GPU WPR region support

[Tom Warren]

Sorry, Alex. Missed these.

> -----Original Message-----
> From: Alexandre Courbot [mailto:gnurou at gmail.com]
> Sent: Sunday, October 25, 2015 10:50 PM
> To: Alex Courbot <acourbot@nvidia.com>
> Cc: Tom Warren <TWarren@nvidia.com>; Stephen Warren
> <swarren@nvidia.com>; Thierry Reding <treding@nvidia.com>; u-
> boot at lists.denx.de; linux-tegra at vger.kernel.org
> Subject: Re: [PATCH 0/4] ARM: tegra: GPU WPR region support
> 
> Ping Tom, how does this look to you?
Looks pretty good, but what about saving the security_carveout reg settings back to the BCT or scratch regs so those settings will be restored on LP0 resume?

Tom
--
nvpublic
> 
> On Mon, Oct 19, 2015 at 1:57 PM, Alexandre Courbot <acourbot@nvidia.com>
> wrote:
> > This series makes U-boot program the write-protected (WPR) region of
> > T210 chips, allowing the kernel to perform GPU secure firmware loading.
> >
> > Tegra 210's GPU requires its firmware to be loaded though a
> > write-protected region. An area of physical memory is carved-out,
> > programmed into the corresponding memory controller registers, and
> > locked such as only the GPU can write into it. This area needs to be
> > set up by the bootloader since it cannot be re-claimed for normal use after
> being locked.
> >
> > The first 3 patches of this series are cleanup patches. Patch 2
> > implements a suggestion made by Stephen, patch 3 renames GPU-related
> > functions to sound less generic.
> >
> > The last patch adds support for the GPU WPR region. The top 256KB of
> > memory are removed from the available memory, and the corresponding MC
> > registers are programmed to point to it, which allows the kernel to
> > initiate secure firmware loading.
> >
> > Alexandre Courbot (4):
> >   ARM: tegra: remove vpr_configured() function
> >   ARM: tegra: simplify GPU setup
> >   ARM: tegra: rename GPU functions
> >   ARM: tegra210: gpu: configure WPR region
> >
> >  arch/arm/include/asm/arch-tegra/gpu.h   | 14 +++------
> >  arch/arm/include/asm/arch-tegra210/mc.h | 12 ++++++++
> >  arch/arm/mach-tegra/board.c             |  4 +++
> >  arch/arm/mach-tegra/board2.c            | 22 +++++++++++++-
> >  arch/arm/mach-tegra/gpu.c               | 52
> +++++++++++++++++++++++++++++----
> >  board/nvidia/jetson-tk1/jetson-tk1.c    |  8 -----
> >  board/nvidia/p2571/p2571.c              |  7 -----
> >  board/nvidia/venice2/venice2.c          |  8 -----
> >  include/configs/jetson-tk1.h            |  2 --
> >  include/configs/p2571.h                 |  2 --
> >  include/configs/tegra-common.h          |  2 ++
> >  include/configs/venice2.h               |  2 --
> >  12 files changed, 89 insertions(+), 46 deletions(-)
> >
> > --
> > 2.6.1
> >

[U-Boot] [PATCH 0/4] ARM: tegra: GPU WPR region support

[Alexandre Courbot]

On Wed, Oct 28, 2015 at 12:57 AM, Tom Warren <TWarren@nvidia.com> wrote:
> Sorry, Alex. Missed these.
>
>> -----Original Message-----
>> From: Alexandre Courbot [mailto:gnurou at gmail.com]
>> Sent: Sunday, October 25, 2015 10:50 PM
>> To: Alex Courbot <acourbot@nvidia.com>
>> Cc: Tom Warren <TWarren@nvidia.com>; Stephen Warren
>> <swarren@nvidia.com>; Thierry Reding <treding@nvidia.com>; u-
>> boot at lists.denx.de; linux-tegra at vger.kernel.org
>> Subject: Re: [PATCH 0/4] ARM: tegra: GPU WPR region support
>>
>> Ping Tom, how does this look to you?
> Looks pretty good, but what about saving the security_carveout reg settings back to the BCT or scratch regs so those settings will be restored on LP0 resume?

Absolutely - I am not familiar at all with BCT or scratch registers
(and U-boot in general :) ) though, could you point me to some part of
the code which I could use as a reference for this?

Also, how can I decide which mechanism to use over the other?

Thanks,
Alex.

[U-Boot] [PATCH 0/4] ARM: tegra: GPU WPR region support

[Tom Warren]

Alex,

> -----Original Message-----
> From: Alexandre Courbot [mailto:gnurou at gmail.com]
> Sent: Wednesday, October 28, 2015 2:13 AM
> To: Tom Warren <TWarren@nvidia.com>
> Cc: Alex Courbot <acourbot@nvidia.com>; Stephen Warren
> <swarren@nvidia.com>; Thierry Reding <treding@nvidia.com>; u-
> boot at lists.denx.de; linux-tegra at vger.kernel.org
> Subject: Re: [PATCH 0/4] ARM: tegra: GPU WPR region support
> 
> On Wed, Oct 28, 2015 at 12:57 AM, Tom Warren <TWarren@nvidia.com>
> wrote:
> > Sorry, Alex. Missed these.
> >
> >> -----Original Message-----
> >> From: Alexandre Courbot [mailto:gnurou at gmail.com]
> >> Sent: Sunday, October 25, 2015 10:50 PM
> >> To: Alex Courbot <acourbot@nvidia.com>
> >> Cc: Tom Warren <TWarren@nvidia.com>; Stephen Warren
> >> <swarren@nvidia.com>; Thierry Reding <treding@nvidia.com>; u-
> >> boot at lists.denx.de; linux-tegra at vger.kernel.org
> >> Subject: Re: [PATCH 0/4] ARM: tegra: GPU WPR region support
> >>
> >> Ping Tom, how does this look to you?
> > Looks pretty good, but what about saving the security_carveout reg settings
> back to the BCT or scratch regs so those settings will be restored on LP0
> resume?
> 
> Absolutely - I am not familiar at all with BCT or scratch registers (and U-boot in
> general :) ) though, could you point me to some part of the code which I could
> use as a reference for this?
> 
> Also, how can I decide which mechanism to use over the other?
I'll have to refresh my meat RAM on how U-Boot handles LP0 WRT BCT/scratch regs, but in coreboot I had to 'flush' the updated carveout regs back to the BCT copy in SDRAM so that they'd be properly restored on resume. I only pushed the modified regs (BOM and CFG0, IIRC).  If you have access to Google's coreboot repo, look in src/soc/nvidia/tegra210/sdram_lp0.c, commit ID 920968258. Or just Google for sdram_lp0.c, should be the top hit.

> 
> Thanks,
> Alex.