[U-Boot] [PATCH v3 0/3] Raspberry Pi EFI enablement

[U-Boot] [PATCH v3 0/3] Raspberry Pi EFI enablement

[Alexander Graf]

With this patch set I can successfully boot a Raspberry Pi 3 into Linux, with
efifb, efi based reset/power off and spin table secondary bringup working.

Please apply.


v2 -> v3:

  - Rebase
  - Adjust to new runtime macro names

Alexander Graf (3):
  ARM: bcm283x: Implement EFI RTS reset_system
  bcm2835 video: Map frame buffer as 32bpp
  bcm2835: Reserve the spin table in efi memory map

 arch/arm/mach-bcm283x/include/mach/wdog.h |  2 +-
 arch/arm/mach-bcm283x/reset.c             | 59 +++++++++++++++++++++++++++----
 board/raspberrypi/rpi/rpi.c               |  6 ++++
 drivers/video/bcm2835.c                   |  6 ++--
 include/configs/rpi.h                     |  2 +-
 5 files changed, 64 insertions(+), 11 deletions(-)

-- 
1.8.5.6

[U-Boot] [PATCH v3 1/3] ARM: bcm283x: Implement EFI RTS reset_system

[Alexander Graf]

The rpi has a pretty simple way of resetting the whole system. All it takes
is to poke a few registers at a well defined location in MMIO space.

This patch adds support for the EFI loader implementation to allow an OS to
reset and power off the system when we're outside of boot time.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v2 -> v3:

  - Use new runtime markers
---
 arch/arm/mach-bcm283x/include/mach/wdog.h |  2 +-
 arch/arm/mach-bcm283x/reset.c             | 59 +++++++++++++++++++++++++++----
 2 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-bcm283x/include/mach/wdog.h b/arch/arm/mach-bcm283x/include/mach/wdog.h
index 7741d7b..b4caca1 100644
--- a/arch/arm/mach-bcm283x/include/mach/wdog.h
+++ b/arch/arm/mach-bcm283x/include/mach/wdog.h
@@ -16,7 +16,7 @@
 struct bcm2835_wdog_regs {
 	u32 unknown0[7];
 	u32 rstc;
-	u32 unknown1;
+	u32 rsts;
 	u32 wdog;
 };
 
diff --git a/arch/arm/mach-bcm283x/reset.c b/arch/arm/mach-bcm283x/reset.c
index 72cdc31..685815c 100644
--- a/arch/arm/mach-bcm283x/reset.c
+++ b/arch/arm/mach-bcm283x/reset.c
@@ -10,19 +10,66 @@
 #include <common.h>
 #include <asm/io.h>
 #include <asm/arch/wdog.h>
+#include <efi_loader.h>
 
 #define RESET_TIMEOUT 10
 
-void reset_cpu(ulong addr)
+/*
+ * The Raspberry Pi firmware uses the RSTS register to know which partiton
+ * to boot from. The partiton value is spread into bits 0, 2, 4, 6, 8, 10.
+ * Partiton 63 is a special partition used by the firmware to indicate halt.
+ */
+#define BCM2835_WDOG_RSTS_RASPBERRYPI_HALT	0x555
+
+__efi_runtime_data struct bcm2835_wdog_regs *wdog_regs =
+	(struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
+
+void __efi_runtime reset_cpu(ulong addr)
 {
-	struct bcm2835_wdog_regs *regs =
-		(struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
 	uint32_t rstc;
 
-	rstc = readl(&regs->rstc);
+	rstc = readl(&wdog_regs->rstc);
 	rstc &= ~BCM2835_WDOG_RSTC_WRCFG_MASK;
 	rstc |= BCM2835_WDOG_RSTC_WRCFG_FULL_RESET;
 
-	writel(BCM2835_WDOG_PASSWORD | RESET_TIMEOUT, &regs->wdog);
-	writel(BCM2835_WDOG_PASSWORD | rstc, &regs->rstc);
+	writel(BCM2835_WDOG_PASSWORD | RESET_TIMEOUT, &wdog_regs->wdog);
+	writel(BCM2835_WDOG_PASSWORD | rstc, &wdog_regs->rstc);
+}
+
+#ifdef CONFIG_EFI_LOADER
+
+void __efi_runtime EFIAPI efi_reset_system(
+			enum efi_reset_type reset_type,
+			efi_status_t reset_status,
+			unsigned long data_size, void *reset_data)
+{
+	u32 val;
+
+	switch (reset_type) {
+	case EFI_RESET_COLD:
+	case EFI_RESET_WARM:
+		reset_cpu(0);
+		break;
+	case EFI_RESET_SHUTDOWN:
+		/*
+		 * We set the watchdog hard reset bit here to distinguish this reset
+		 * from the normal (full) reset. bootcode.bin will not reboot after a
+		 * hard reset.
+		 */
+		val = readl(&wdog_regs->rsts);
+		val |= BCM2835_WDOG_PASSWORD;
+		val |= BCM2835_WDOG_RSTS_RASPBERRYPI_HALT;
+		writel(val, &wdog_regs->rsts);
+		reset_cpu(0);
+		break;
+	}
+
+	while (1) { }
 }
+
+void efi_reset_system_init(void)
+{
+	efi_add_runtime_mmio(&wdog_regs, sizeof(*wdog_regs));
+}
+
+#endif
-- 
1.8.5.6

[U-Boot] [PATCH v3 2/3] bcm2835 video: Map frame buffer as 32bpp

[Alexander Graf]

To enable working efifb support, let's map the frame buffer as 32bpp
instead of 16bpp.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 drivers/video/bcm2835.c | 6 +++---
 include/configs/rpi.h   | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/video/bcm2835.c b/drivers/video/bcm2835.c
index cd605e6..cc6454f 100644
--- a/drivers/video/bcm2835.c
+++ b/drivers/video/bcm2835.c
@@ -71,9 +71,9 @@ void lcd_ctrl_init(void *lcdbase)
 	msg_setup->virtual_w_h.body.req.width = w;
 	msg_setup->virtual_w_h.body.req.height = h;
 	BCM2835_MBOX_INIT_TAG(&msg_setup->depth, SET_DEPTH);
-	msg_setup->depth.body.req.bpp = 16;
+	msg_setup->depth.body.req.bpp = 32;
 	BCM2835_MBOX_INIT_TAG(&msg_setup->pixel_order, SET_PIXEL_ORDER);
-	msg_setup->pixel_order.body.req.order = BCM2835_MBOX_PIXEL_ORDER_BGR;
+	msg_setup->pixel_order.body.req.order = BCM2835_MBOX_PIXEL_ORDER_RGB;
 	BCM2835_MBOX_INIT_TAG(&msg_setup->alpha_mode, SET_ALPHA_MODE);
 	msg_setup->alpha_mode.body.req.alpha = BCM2835_MBOX_ALPHA_MODE_IGNORED;
 	BCM2835_MBOX_INIT_TAG(&msg_setup->virtual_offset, SET_VIRTUAL_OFFSET);
@@ -103,7 +103,7 @@ void lcd_ctrl_init(void *lcdbase)
 
 	panel_info.vl_col = w;
 	panel_info.vl_row = h;
-	panel_info.vl_bpix = LCD_COLOR16;
+	panel_info.vl_bpix = LCD_COLOR32;
 
 	gd->fb_base = bus_to_phys(
 		msg_setup->allocate_buffer.body.resp.fb_address);
diff --git a/include/configs/rpi.h b/include/configs/rpi.h
index 45c8234..19e0212 100644
--- a/include/configs/rpi.h
+++ b/include/configs/rpi.h
@@ -72,7 +72,7 @@
 #define CONFIG_BCM2835_GPIO
 /* LCD */
 #define CONFIG_LCD_DT_SIMPLEFB
-#define LCD_BPP				LCD_COLOR16
+#define LCD_BPP				LCD_COLOR32
 /*
  * Prevent allocation of RAM for FB; the real FB address is queried
  * dynamically from the VideoCore co-processor, and comes from RAM
-- 
1.8.5.6

[U-Boot] [PATCH v3 3/3] bcm2835: Reserve the spin table in efi memory map

[Alexander Graf]

Firmware provides a spin table on the raspberry pi. This table shouldn't
get overwritten by payloads, so we need to mark it as reserved.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 board/raspberrypi/rpi/rpi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
index 6245b36..7f057e1 100644
--- a/board/raspberrypi/rpi/rpi.c
+++ b/board/raspberrypi/rpi/rpi.c
@@ -8,6 +8,7 @@
 #include <inttypes.h>
 #include <config.h>
 #include <dm.h>
+#include <efi_loader.h>
 #include <fdt_support.h>
 #include <fdt_simplefb.h>
 #include <lcd.h>
@@ -514,5 +515,10 @@ int ft_board_setup(void *blob, bd_t *bd)
 	 */
 	lcd_dt_simplefb_add_node(blob);
 
+#ifdef CONFIG_EFI_LOADER
+	/* Reserve the spin table */
+	efi_add_memory_map(0, 1, EFI_RESERVED_MEMORY_TYPE, 0);
+#endif
+
 	return 0;
 }
-- 
1.8.5.6

[U-Boot] [PATCH v3 1/3] ARM: bcm283x: Implement EFI RTS reset_system

[Stephen Warren]

On 11/02/2016 03:36 AM, Alexander Graf wrote:
> The rpi has a pretty simple way of resetting the whole system. All it takes
> is to poke a few registers at a well defined location in MMIO space.
>
> This patch adds support for the EFI loader implementation to allow an OS to
> reset and power off the system when we're outside of boot time.

(As an aside, I'm not sure why someone wanting EFI wouldn't just use a 
complete EFI implementation such as TianoCore.)

> diff --git a/arch/arm/mach-bcm283x/reset.c b/arch/arm/mach-bcm283x/reset.c

> +__efi_runtime_data struct bcm2835_wdog_regs *wdog_regs =
> +	(struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
> +
> +void __efi_runtime reset_cpu(ulong addr)
>  {
> -	struct bcm2835_wdog_regs *regs =
> -		(struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;

I'm not sure why that change is required. The value of the variable is 
the same in both cases?

Perhaps it's trying to ensure that if this gets compiled into an ldr 
instruction, the referenced data value is in a linker section that's 
still around when EFI runs? If so fine, but how is that ensured for all 
the other constants that this code uses, and if that happens 
automatically due to the __efi_runtime marker above, why doesn't it work 
for this one constant?

Does U-Boot have a halt/poweroff/shutdown shell command? If so, it might 
be nice to enable it as part of this series, since the code to perform 
that operation is now present.

[U-Boot] [PATCH v3 2/3] bcm2835 video: Map frame buffer as 32bpp

[Stephen Warren]

On 11/02/2016 03:36 AM, Alexander Graf wrote:
> To enable working efifb support, let's map the frame buffer as 32bpp
> instead of 16bpp.

Hmmm. I feel like there was some good reason why I picked 16bpp when I 
first wrote this, especially since I had to add 16bpp support into the 
kernel's simplefb driver specifically to support the RPI. However, I 
honestly can't remember why now. So, if you've tested this on all 3 
generations of RPi and HDMI still works on all 3,

Acked-by: Stephen Warren <swarren@wwwdotorg.org>

[U-Boot] [PATCH v3 3/3] bcm2835: Reserve the spin table in efi memory map

[Stephen Warren]

On 11/02/2016 03:36 AM, Alexander Graf wrote:
> Firmware provides a spin table on the raspberry pi. This table shouldn't
> get overwritten by payloads, so we need to mark it as reserved.

This is probably fine for now so,
Acked-by: Stephen Warren <swarren@wwwdotorg.org>

However in the long term I wonder if U-Boot shouldn't find out the spin 
table address from the FW-provided DTB, then boot all CPUs into a 
spin-table implementation that's provided by U-Boot. That would avoid 
having to hard-code the address/size of the spin table code/data in 
U-Boot, which in theory at least could change to an arbitrary location 
in a future FW release.

[U-Boot] [PATCH v3 1/3] ARM: bcm283x: Implement EFI RTS reset_system

[Alexander Graf]

On 05/11/2016 23:01, Stephen Warren wrote:
> On 11/02/2016 03:36 AM, Alexander Graf wrote:
>> The rpi has a pretty simple way of resetting the whole system. All it
>> takes
>> is to poke a few registers at a well defined location in MMIO space.
>>
>> This patch adds support for the EFI loader implementation to allow an
>> OS to
>> reset and power off the system when we're outside of boot time.
>
> (As an aside, I'm not sure why someone wanting EFI wouldn't just use a
> complete EFI implementation such as TianoCore.)
>
>> diff --git a/arch/arm/mach-bcm283x/reset.c
>> b/arch/arm/mach-bcm283x/reset.c
>
>> +__efi_runtime_data struct bcm2835_wdog_regs *wdog_regs =
>> +    (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>> +
>> +void __efi_runtime reset_cpu(ulong addr)
>>  {
>> -    struct bcm2835_wdog_regs *regs =
>> -        (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>
> I'm not sure why that change is required. The value of the variable is
> the same in both cases?

Take a look a few lines down in the patch:

> +void efi_reset_system_init(void)
> +{
> +	efi_add_runtime_mmio(&wdog_regs, sizeof(*wdog_regs));
> +}

What this does is register a *pointer* as run time service pointer. What 
does that mean?

When we enter RTS, Linux can map any region in the EFI memory map into a 
different place in its own virtual memory map. So any pointers we use 
inside RTS have to be relocated to the new locations.

For normal relocations, we move the relocations from linker time to run 
time, so that we can relocate ourselves when Linux does the switch-over 
to a new address space.

However, for MMIO that's trickier. That's where the 
efi_add_runtime_mmio() function comes into play. It takes care of adding 
the page around the references address to the EFI memory map as RTS MMIO 
and relocates the pointer when Linux switches us into the new address space.

Does that explain why we need to move from an inline address to an 
address stored in a memory location?

> Perhaps it's trying to ensure that if this gets compiled into an ldr
> instruction, the referenced data value is in a linker section that's
> still around when EFI runs? If so fine, but how is that ensured for all
> the other constants that this code uses, and if that happens
> automatically due to the __efi_runtime marker above, why doesn't it work
> for this one constant?
>
> Does U-Boot have a halt/poweroff/shutdown shell command? If so, it might
> be nice to enable it as part of this series, since the code to perform
> that operation is now present.

That's what I originally wanted, yes :). Unfortunately due to the 
relocation explained above, it's basically impossible for any reset 
function that calls into MMIO space.

However, we do have it now for PSCI. If you have a PSCI enabled system, 
we don't need to call into MMIO space and thus make the common reset 
function available as RTS.


Alex

[U-Boot] [PATCH v3 3/3] bcm2835: Reserve the spin table in efi memory map

[Alexander Graf]

On 05/11/2016 23:07, Stephen Warren wrote:
> On 11/02/2016 03:36 AM, Alexander Graf wrote:
>> Firmware provides a spin table on the raspberry pi. This table shouldn't
>> get overwritten by payloads, so we need to mark it as reserved.
>
> This is probably fine for now so,
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
>
> However in the long term I wonder if U-Boot shouldn't find out the spin
> table address from the FW-provided DTB, then boot all CPUs into a
> spin-table implementation that's provided by U-Boot. That would avoid
> having to hard-code the address/size of the spin table code/data in
> U-Boot, which in theory at least could change to an arbitrary location
> in a future FW release.

I think in the long term, it would make much more sense to use PSCI on 
the RPi3. Then we wouldn't have to worry about any of the above, except 
for an EL3 reserved memory region that we could also enquire using PSCI.

For 32bit systems, yes, probably :).


Alex

[U-Boot] [PATCH v3 1/3] ARM: bcm283x: Implement EFI RTS reset_system

[Alexander Graf]

On 05/11/2016 23:01, Stephen Warren wrote:
> On 11/02/2016 03:36 AM, Alexander Graf wrote:
>> The rpi has a pretty simple way of resetting the whole system. All it
>> takes
>> is to poke a few registers at a well defined location in MMIO space.
>>
>> This patch adds support for the EFI loader implementation to allow an
>> OS to
>> reset and power off the system when we're outside of boot time.
>
> (As an aside, I'm not sure why someone wanting EFI wouldn't just use a
> complete EFI implementation such as TianoCore.)

Sorry, I didn't reply to this part earlier.

If you have a TianoCore port, using that is almost always a better idea. 
I'd compare it to wine vs native Windows. With native Windows, you get 
guaranteed compatibility, wine only tries really hard :).

However, if you compare the size of this patch set to a TianoCore port, 
the rationale becomes pretty clear I guess. Porting a system that is 
already U-Boot enabled to be RTS enabled is a matter of a day of work. 
Non-RTS EFI enablement comes for free in U-Boot.

Porting TianoCore to a new platform takes slightly longer. And you have 
to embrace CamelCase.


Alex

[U-Boot] [PATCH v3 1/3] ARM: bcm283x: Implement EFI RTS reset_system

[Stephen Warren]

On 11/06/2016 03:24 AM, Alexander Graf wrote:
>
>
> On 05/11/2016 23:01, Stephen Warren wrote:
>> On 11/02/2016 03:36 AM, Alexander Graf wrote:
>>> The rpi has a pretty simple way of resetting the whole system. All it
>>> takes
>>> is to poke a few registers at a well defined location in MMIO space.
>>>
>>> This patch adds support for the EFI loader implementation to allow an
>>> OS to
>>> reset and power off the system when we're outside of boot time.
>>
>> (As an aside, I'm not sure why someone wanting EFI wouldn't just use a
>> complete EFI implementation such as TianoCore.)
>>
>>> diff --git a/arch/arm/mach-bcm283x/reset.c
>>> b/arch/arm/mach-bcm283x/reset.c
>>
>>> +__efi_runtime_data struct bcm2835_wdog_regs *wdog_regs =
>>> +    (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>>> +
>>> +void __efi_runtime reset_cpu(ulong addr)
>>>  {
>>> -    struct bcm2835_wdog_regs *regs =
>>> -        (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>>
>> I'm not sure why that change is required. The value of the variable is
>> the same in both cases?
>
> Take a look a few lines down in the patch:
>
>> +void efi_reset_system_init(void)
>> +{
>> +    efi_add_runtime_mmio(&wdog_regs, sizeof(*wdog_regs));
>> +}
>
> What this does is register a *pointer* as run time service pointer. What
> does that mean?
>
> When we enter RTS, Linux can map any region in the EFI memory map into a
> different place in its own virtual memory map. So any pointers we use
> inside RTS have to be relocated to the new locations.
>
> For normal relocations, we move the relocations from linker time to run
> time, so that we can relocate ourselves when Linux does the switch-over
> to a new address space.
>
> However, for MMIO that's trickier. That's where the
> efi_add_runtime_mmio() function comes into play. It takes care of adding
> the page around the references address to the EFI memory map as RTS MMIO
> and relocates the pointer when Linux switches us into the new address
> space.
>
> Does that explain why we need to move from an inline address to an
> address stored in a memory location?

So EFI RTS runs in the same exception level as the rich OS, and not in 
EL3? I would have expected EFI to run in EL3 with a completely separate 
MMU configuration. If that's not the case, then this part of the patch 
does make sense.

>> Perhaps it's trying to ensure that if this gets compiled into an ldr
>> instruction, the referenced data value is in a linker section that's
>> still around when EFI runs? If so fine, but how is that ensured for all
>> the other constants that this code uses, and if that happens
>> automatically due to the __efi_runtime marker above, why doesn't it work
>> for this one constant?
>>
>> Does U-Boot have a halt/poweroff/shutdown shell command? If so, it might
>> be nice to enable it as part of this series, since the code to perform
>> that operation is now present.
>
> That's what I originally wanted, yes :). Unfortunately due to the
> relocation explained above, it's basically impossible for any reset
> function that calls into MMIO space.
>
> However, we do have it now for PSCI. If you have a PSCI enabled system,
> we don't need to call into MMIO space and thus make the common reset
> function available as RTS.

Can't the same U-Boot function be called both (a) during U-Boot runtime, 
where wdog_regs are pre-initialized to match U-Boot's MMU configuration, 
and (b) once the OS has booted, where wdog_regs has been modified 
according to the new memory map?

If not, one could implement a reset/powerdown/... function that takes 
the MMIO virtual address as a pointer, and then separate trivial 
wrappers that pass in either the static/U-Boot MMIO address, or the 
value of the EFI runtime variable that points at the MMIO mapping.

[U-Boot] [PATCH v3 1/3] ARM: bcm283x: Implement EFI RTS reset_system

[Alexander Graf]

On 07/11/2016 22:26, Stephen Warren wrote:
> On 11/06/2016 03:24 AM, Alexander Graf wrote:
>>
>>
>> On 05/11/2016 23:01, Stephen Warren wrote:
>>> On 11/02/2016 03:36 AM, Alexander Graf wrote:
>>>> The rpi has a pretty simple way of resetting the whole system. All it
>>>> takes
>>>> is to poke a few registers at a well defined location in MMIO space.
>>>>
>>>> This patch adds support for the EFI loader implementation to allow an
>>>> OS to
>>>> reset and power off the system when we're outside of boot time.
>>>
>>> (As an aside, I'm not sure why someone wanting EFI wouldn't just use a
>>> complete EFI implementation such as TianoCore.)
>>>
>>>> diff --git a/arch/arm/mach-bcm283x/reset.c
>>>> b/arch/arm/mach-bcm283x/reset.c
>>>
>>>> +__efi_runtime_data struct bcm2835_wdog_regs *wdog_regs =
>>>> +    (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>>>> +
>>>> +void __efi_runtime reset_cpu(ulong addr)
>>>>  {
>>>> -    struct bcm2835_wdog_regs *regs =
>>>> -        (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>>>
>>> I'm not sure why that change is required. The value of the variable is
>>> the same in both cases?
>>
>> Take a look a few lines down in the patch:
>>
>>> +void efi_reset_system_init(void)
>>> +{
>>> +    efi_add_runtime_mmio(&wdog_regs, sizeof(*wdog_regs));
>>> +}
>>
>> What this does is register a *pointer* as run time service pointer. What
>> does that mean?
>>
>> When we enter RTS, Linux can map any region in the EFI memory map into a
>> different place in its own virtual memory map. So any pointers we use
>> inside RTS have to be relocated to the new locations.
>>
>> For normal relocations, we move the relocations from linker time to run
>> time, so that we can relocate ourselves when Linux does the switch-over
>> to a new address space.
>>
>> However, for MMIO that's trickier. That's where the
>> efi_add_runtime_mmio() function comes into play. It takes care of adding
>> the page around the references address to the EFI memory map as RTS MMIO
>> and relocates the pointer when Linux switches us into the new address
>> space.
>>
>> Does that explain why we need to move from an inline address to an
>> address stored in a memory location?
>
> So EFI RTS runs in the same exception level as the rich OS, and not in
> EL3? I would have expected EFI to run in EL3 with a completely separate
> MMU configuration. If that's not the case, then this part of the patch
> does make sense.

Right, it runs in EL2/EL1 with a virtual memory layout that is provided 
by the OS.

>
>>> Perhaps it's trying to ensure that if this gets compiled into an ldr
>>> instruction, the referenced data value is in a linker section that's
>>> still around when EFI runs? If so fine, but how is that ensured for all
>>> the other constants that this code uses, and if that happens
>>> automatically due to the __efi_runtime marker above, why doesn't it work
>>> for this one constant?
>>>
>>> Does U-Boot have a halt/poweroff/shutdown shell command? If so, it might
>>> be nice to enable it as part of this series, since the code to perform
>>> that operation is now present.
>>
>> That's what I originally wanted, yes :). Unfortunately due to the
>> relocation explained above, it's basically impossible for any reset
>> function that calls into MMIO space.
>>
>> However, we do have it now for PSCI. If you have a PSCI enabled system,
>> we don't need to call into MMIO space and thus make the common reset
>> function available as RTS.
>
> Can't the same U-Boot function be called both (a) during U-Boot runtime,
> where wdog_regs are pre-initialized to match U-Boot's MMU configuration,
> and (b) once the OS has booted, where wdog_regs has been modified
> according to the new memory map?

That's exactly what this patch does, no?

> If not, one could implement a reset/powerdown/... function that takes
> the MMIO virtual address as a pointer, and then separate trivial
> wrappers that pass in either the static/U-Boot MMIO address, or the
> value of the EFI runtime variable that points at the MMIO mapping.

You could, but because the runtime version would still have to rely on 
an external variable because it doesn't know where it'll end up at 
runtime, you may as well use an external variable throughout and arrive 
back at this patch ;).


Alex

[U-Boot] [PATCH v3 1/3] ARM: bcm283x: Implement EFI RTS reset_system

[Stephen Warren]

On 11/09/2016 04:43 AM, Alexander Graf wrote:
>
>
> On 07/11/2016 22:26, Stephen Warren wrote:
>> On 11/06/2016 03:24 AM, Alexander Graf wrote:
>>>
>>>
>>> On 05/11/2016 23:01, Stephen Warren wrote:
>>>> On 11/02/2016 03:36 AM, Alexander Graf wrote:
>>>>> The rpi has a pretty simple way of resetting the whole system. All it
>>>>> takes
>>>>> is to poke a few registers at a well defined location in MMIO space.
>>>>>
>>>>> This patch adds support for the EFI loader implementation to allow an
>>>>> OS to
>>>>> reset and power off the system when we're outside of boot time.
>>>>
>>>> (As an aside, I'm not sure why someone wanting EFI wouldn't just use a
>>>> complete EFI implementation such as TianoCore.)
>>>>
>>>>> diff --git a/arch/arm/mach-bcm283x/reset.c
>>>>> b/arch/arm/mach-bcm283x/reset.c
>>>>
>>>>> +__efi_runtime_data struct bcm2835_wdog_regs *wdog_regs =
>>>>> +    (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>>>>> +
>>>>> +void __efi_runtime reset_cpu(ulong addr)
>>>>>  {
>>>>> -    struct bcm2835_wdog_regs *regs =
>>>>> -        (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>>>>
>>>> I'm not sure why that change is required. The value of the variable is
>>>> the same in both cases?
>>>
>>> Take a look a few lines down in the patch:
>>>
>>>> +void efi_reset_system_init(void)
>>>> +{
>>>> +    efi_add_runtime_mmio(&wdog_regs, sizeof(*wdog_regs));
>>>> +}
>>>
>>> What this does is register a *pointer* as run time service pointer. What
>>> does that mean?
>>>
>>> When we enter RTS, Linux can map any region in the EFI memory map into a
>>> different place in its own virtual memory map. So any pointers we use
>>> inside RTS have to be relocated to the new locations.
>>>
>>> For normal relocations, we move the relocations from linker time to run
>>> time, so that we can relocate ourselves when Linux does the switch-over
>>> to a new address space.
>>>
>>> However, for MMIO that's trickier. That's where the
>>> efi_add_runtime_mmio() function comes into play. It takes care of adding
>>> the page around the references address to the EFI memory map as RTS MMIO
>>> and relocates the pointer when Linux switches us into the new address
>>> space.
>>>
>>> Does that explain why we need to move from an inline address to an
>>> address stored in a memory location?
>>
>> So EFI RTS runs in the same exception level as the rich OS, and not in
>> EL3? I would have expected EFI to run in EL3 with a completely separate
>> MMU configuration. If that's not the case, then this part of the patch
>> does make sense.
>
> Right, it runs in EL2/EL1 with a virtual memory layout that is provided
> by the OS.
>
>>
>>>> Perhaps it's trying to ensure that if this gets compiled into an ldr
>>>> instruction, the referenced data value is in a linker section that's
>>>> still around when EFI runs? If so fine, but how is that ensured for all
>>>> the other constants that this code uses, and if that happens
>>>> automatically due to the __efi_runtime marker above, why doesn't it
>>>> work
>>>> for this one constant?
>>>>
>>>> Does U-Boot have a halt/poweroff/shutdown shell command? If so, it
>>>> might
>>>> be nice to enable it as part of this series, since the code to perform
>>>> that operation is now present.
>>>
>>> That's what I originally wanted, yes :). Unfortunately due to the
>>> relocation explained above, it's basically impossible for any reset
>>> function that calls into MMIO space.
>>>
>>> However, we do have it now for PSCI. If you have a PSCI enabled system,
>>> we don't need to call into MMIO space and thus make the common reset
>>> function available as RTS.
>>
>> Can't the same U-Boot function be called both (a) during U-Boot runtime,
>> where wdog_regs are pre-initialized to match U-Boot's MMU configuration,
>> and (b) once the OS has booted, where wdog_regs has been modified
>> according to the new memory map?
>
> That's exactly what this patch does, no?

I assume not, since you said just a few lines above that doing so was 
impossible, hence why it doesn't implement any halt/poweroff/shutdown 
shell commands.

[U-Boot] [PATCH v3 1/3] ARM: bcm283x: Implement EFI RTS reset_system

[Alexander Graf]

On 09/11/2016 10:50, Stephen Warren wrote:
> On 11/09/2016 04:43 AM, Alexander Graf wrote:
>>
>>
>> On 07/11/2016 22:26, Stephen Warren wrote:
>>> On 11/06/2016 03:24 AM, Alexander Graf wrote:
>>>>
>>>>
>>>> On 05/11/2016 23:01, Stephen Warren wrote:
>>>>> On 11/02/2016 03:36 AM, Alexander Graf wrote:
>>>>>> The rpi has a pretty simple way of resetting the whole system. All it
>>>>>> takes
>>>>>> is to poke a few registers at a well defined location in MMIO space.
>>>>>>
>>>>>> This patch adds support for the EFI loader implementation to allow an
>>>>>> OS to
>>>>>> reset and power off the system when we're outside of boot time.
>>>>>
>>>>> (As an aside, I'm not sure why someone wanting EFI wouldn't just use a
>>>>> complete EFI implementation such as TianoCore.)
>>>>>
>>>>>> diff --git a/arch/arm/mach-bcm283x/reset.c
>>>>>> b/arch/arm/mach-bcm283x/reset.c
>>>>>
>>>>>> +__efi_runtime_data struct bcm2835_wdog_regs *wdog_regs =
>>>>>> +    (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>>>>>> +
>>>>>> +void __efi_runtime reset_cpu(ulong addr)
>>>>>>  {
>>>>>> -    struct bcm2835_wdog_regs *regs =
>>>>>> -        (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
>>>>>
>>>>> I'm not sure why that change is required. The value of the variable is
>>>>> the same in both cases?
>>>>
>>>> Take a look a few lines down in the patch:
>>>>
>>>>> +void efi_reset_system_init(void)
>>>>> +{
>>>>> +    efi_add_runtime_mmio(&wdog_regs, sizeof(*wdog_regs));
>>>>> +}
>>>>
>>>> What this does is register a *pointer* as run time service pointer.
>>>> What
>>>> does that mean?
>>>>
>>>> When we enter RTS, Linux can map any region in the EFI memory map
>>>> into a
>>>> different place in its own virtual memory map. So any pointers we use
>>>> inside RTS have to be relocated to the new locations.
>>>>
>>>> For normal relocations, we move the relocations from linker time to run
>>>> time, so that we can relocate ourselves when Linux does the switch-over
>>>> to a new address space.
>>>>
>>>> However, for MMIO that's trickier. That's where the
>>>> efi_add_runtime_mmio() function comes into play. It takes care of
>>>> adding
>>>> the page around the references address to the EFI memory map as RTS
>>>> MMIO
>>>> and relocates the pointer when Linux switches us into the new address
>>>> space.
>>>>
>>>> Does that explain why we need to move from an inline address to an
>>>> address stored in a memory location?
>>>
>>> So EFI RTS runs in the same exception level as the rich OS, and not in
>>> EL3? I would have expected EFI to run in EL3 with a completely separate
>>> MMU configuration. If that's not the case, then this part of the patch
>>> does make sense.
>>
>> Right, it runs in EL2/EL1 with a virtual memory layout that is provided
>> by the OS.
>>
>>>
>>>>> Perhaps it's trying to ensure that if this gets compiled into an ldr
>>>>> instruction, the referenced data value is in a linker section that's
>>>>> still around when EFI runs? If so fine, but how is that ensured for
>>>>> all
>>>>> the other constants that this code uses, and if that happens
>>>>> automatically due to the __efi_runtime marker above, why doesn't it
>>>>> work
>>>>> for this one constant?
>>>>>
>>>>> Does U-Boot have a halt/poweroff/shutdown shell command? If so, it
>>>>> might
>>>>> be nice to enable it as part of this series, since the code to perform
>>>>> that operation is now present.
>>>>
>>>> That's what I originally wanted, yes :). Unfortunately due to the
>>>> relocation explained above, it's basically impossible for any reset
>>>> function that calls into MMIO space.
>>>>
>>>> However, we do have it now for PSCI. If you have a PSCI enabled system,
>>>> we don't need to call into MMIO space and thus make the common reset
>>>> function available as RTS.
>>>
>>> Can't the same U-Boot function be called both (a) during U-Boot runtime,
>>> where wdog_regs are pre-initialized to match U-Boot's MMU configuration,
>>> and (b) once the OS has booted, where wdog_regs has been modified
>>> according to the new memory map?
>>
>> That's exactly what this patch does, no?
>
> I assume not, since you said just a few lines above that doing so was
> impossible, hence why it doesn't implement any halt/poweroff/shutdown
> shell commands.

So this patch transforms the code in a way that allows us to call the 
same function from boot time (1:1 map) as well as run time 
(non-voluntary map). I thought that's what you were suggesting.

Either way, does the approach make sense to you now?


Alex

[U-Boot] [U-Boot, v3, 1/3] ARM: bcm283x: Implement EFI RTS reset_system

[Tom Rini]

On Wed, Nov 02, 2016 at 10:36:18AM +0100, Alexander Graf wrote:

> The rpi has a pretty simple way of resetting the whole system. All it takes
> is to poke a few registers at a well defined location in MMIO space.
> 
> This patch adds support for the EFI loader implementation to allow an OS to
> reset and power off the system when we're outside of boot time.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161129/4108a76d/attachment.sig>

[U-Boot] [U-Boot, v3, 2/3] bcm2835 video: Map frame buffer as 32bpp

[Tom Rini]

On Wed, Nov 02, 2016 at 10:36:19AM +0100, Alexander Graf wrote:

> To enable working efifb support, let's map the frame buffer as 32bpp
> instead of 16bpp.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161129/cfa4333f/attachment.sig>

[U-Boot] [U-Boot, v3, 3/3] bcm2835: Reserve the spin table in efi memory map

[Tom Rini]

On Wed, Nov 02, 2016 at 10:36:20AM +0100, Alexander Graf wrote:

> Firmware provides a spin table on the raspberry pi. This table shouldn't
> get overwritten by payloads, so we need to mark it as reserved.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161129/9d50c08a/attachment.sig>