[PATCH] ARM64: juno: add NOR flash to device tree

[PATCH] ARM64: juno: add NOR flash to device tree

[Linus Walleij]

The Juno motherboard has a NOR flash on the motherboard, enable
this to be accessed with the CFI flash driver. Results after
enabling MTD, MTD_CFI, MTD_PHYSMAP, MTD_PHYSMAP_OF,
MTD_CFI_INTELEXT:

8000000.flash: Found 2 x16 devices at 0x0 in 32-bit bank.
Manufacturer ID 0x000089 Chip ID 0x008919
Intel/Sharp Extended Query Table at 0x010A
Intel/Sharp Extended Query Table at 0x010A
Intel/Sharp Extended Query Table at 0x010A
Intel/Sharp Extended Query Table at 0x010A
Intel/Sharp Extended Query Table at 0x010A
Using buffer write method
Using auto-unlock on power-up/resume
cfi_cmdset_0001: Erase suspend on write enabled
erase region 0: offset=0x0,size=0x40000,blocks=255
erase region 1: offset=0x3fc0000,size=0x10000,blocks=4

Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ARM SoC folks: please apply this directly for -next if
people are OK with it, as I have no other Juno patches
pending for now.
---
 arch/arm64/boot/dts/arm/juno-motherboard.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/arm/juno-motherboard.dtsi b/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
index 637e046f0e36..c7c99a42e2e9 100644
--- a/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
@@ -103,6 +103,14 @@
 				};
 			};
 
+			flash at 0,00000000 {
+				/* 2 * 32MiB NOR Flash memory mounted on CS0 */
+				compatible = "arm,vexpress-flash", "cfi-flash";
+				linux,part-probe = "afs";
+				reg = <0 0x00000000 0x04000000>;
+				bank-width = <4>;
+			};
+
 			ethernet at 2,00000000 {
 				compatible = "smsc,lan9118", "smsc,lan9115";
 				reg = <2 0x00000000 0x10000>;
-- 
2.4.3

[PATCH] ARM64: juno: add NOR flash to device tree

[Liviu Dudau]

On Thu, Oct 15, 2015 at 12:20:15PM +0200, Linus Walleij wrote:
> The Juno motherboard has a NOR flash on the motherboard, enable
> this to be accessed with the CFI flash driver. Results after
> enabling MTD, MTD_CFI, MTD_PHYSMAP, MTD_PHYSMAP_OF,
> MTD_CFI_INTELEXT:
> 
> 8000000.flash: Found 2 x16 devices at 0x0 in 32-bit bank.
> Manufacturer ID 0x000089 Chip ID 0x008919
> Intel/Sharp Extended Query Table at 0x010A
> Intel/Sharp Extended Query Table at 0x010A
> Intel/Sharp Extended Query Table at 0x010A
> Intel/Sharp Extended Query Table at 0x010A
> Intel/Sharp Extended Query Table at 0x010A
> Using buffer write method
> Using auto-unlock on power-up/resume
> cfi_cmdset_0001: Erase suspend on write enabled
> erase region 0: offset=0x0,size=0x40000,blocks=255
> erase region 1: offset=0x3fc0000,size=0x10000,blocks=4
> 
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Acked-by: Liviu Dudau <Liviu.Dudau@arm.com>

> ---
> ARM SoC folks: please apply this directly for -next if
> people are OK with it, as I have no other Juno patches
> pending for now.
> ---
>  arch/arm64/boot/dts/arm/juno-motherboard.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/arm/juno-motherboard.dtsi b/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
> index 637e046f0e36..c7c99a42e2e9 100644
> --- a/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
> +++ b/arch/arm64/boot/dts/arm/juno-motherboard.dtsi
> @@ -103,6 +103,14 @@
>  				};
>  			};
>  
> +			flash at 0,00000000 {
> +				/* 2 * 32MiB NOR Flash memory mounted on CS0 */
> +				compatible = "arm,vexpress-flash", "cfi-flash";
> +				linux,part-probe = "afs";

It would be nice if the linux,part-probe binding was documented somewhere.

> +				reg = <0 0x00000000 0x04000000>;
> +				bank-width = <4>;
> +			};
> +
>  			ethernet at 2,00000000 {
>  				compatible = "smsc,lan9118", "smsc,lan9115";
>  				reg = <2 0x00000000 0x10000>;
> -- 
> 2.4.3
> 

Thanks for this,
Liviu


-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?

[PATCH] ARM64: juno: add NOR flash to device tree

[Arnd Bergmann]

On Thursday 15 October 2015 12:58:25 Liviu Dudau wrote:
> On Thu, Oct 15, 2015 at 12:20:15PM +0200, Linus Walleij wrote:
> > The Juno motherboard has a NOR flash on the motherboard, enable
> > this to be accessed with the CFI flash driver. Results after
> > enabling MTD, MTD_CFI, MTD_PHYSMAP, MTD_PHYSMAP_OF,
> > MTD_CFI_INTELEXT:
> > 
> > 8000000.flash: Found 2 x16 devices at 0x0 in 32-bit bank.
> > Manufacturer ID 0x000089 Chip ID 0x008919
> > Intel/Sharp Extended Query Table at 0x010A
> > Intel/Sharp Extended Query Table at 0x010A
> > Intel/Sharp Extended Query Table at 0x010A
> > Intel/Sharp Extended Query Table at 0x010A
> > Intel/Sharp Extended Query Table at 0x010A
> > Using buffer write method
> > Using auto-unlock on power-up/resume
> > cfi_cmdset_0001: Erase suspend on write enabled
> > erase region 0: offset=0x0,size=0x40000,blocks=255
> > erase region 1: offset=0x3fc0000,size=0x10000,blocks=4
> > 
> > Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Acked-by: Liviu Dudau <Liviu.Dudau@arm.com>

Applied to next/dt, thanks!

	Arnd

[PATCH] ARM64: juno: add NOR flash to device tree

[Sudeep Holla]

Hi Linus,

On 15/10/15 11:20, Linus Walleij wrote:
> The Juno motherboard has a NOR flash on the motherboard, enable
> this to be accessed with the CFI flash driver. Results after
> enabling MTD, MTD_CFI, MTD_PHYSMAP, MTD_PHYSMAP_OF,
> MTD_CFI_INTELEXT:
>

Just a note on this: we can't enable these when CPUIDLE is enabled as
the firmware assumes the flash is always in read mode while Linux leaves
NOR flash in "read id" mode after initialization.

So until the firmware is ready to handle that, we can't enabled both
on Juno. We had same issue even on TC2. I will follow up internally to
see if we can fix on Juno at-least.

So until then, keep an eye on this if someone complains about boot issue
with both MTD and CPUIDLE enabled.

-- 
Regards,
Sudeep

[PATCH] ARM64: juno: add NOR flash to device tree

[Linus Walleij]

On Thu, Oct 15, 2015 at 1:58 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:

>> +                     flash at 0,00000000 {
>> +                             /* 2 * 32MiB NOR Flash memory mounted on CS0 */
>> +                             compatible = "arm,vexpress-flash", "cfi-flash";
>> +                             linux,part-probe = "afs";
>
> It would be nice if the linux,part-probe binding was documented somewhere.

Right, I only found it lying around in the kernel sourcetree
(drivers/mtd/maps/physmap_of.c) with no documentation
whatsoever.

I'll send a separate patch for this.

Yours,
Linus Walleij

[PATCH] ARM64: juno: add NOR flash to device tree

[Linus Walleij]

On Thu, Oct 15, 2015 at 5:30 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> Me
>>  Results after
>> enabling MTD, MTD_CFI, MTD_PHYSMAP, MTD_PHYSMAP_OF,
>> MTD_CFI_INTELEXT:
>
> Just a note on this: we can't enable these when CPUIDLE is enabled as
> the firmware assumes the flash is always in read mode while Linux leaves
> NOR flash in "read id" mode after initialization.

Sorry I'm too unfamiliar with this lingo. Can you give me some detail
on what "read mode" and "read id" mode is all about?

> So until the firmware is ready to handle that, we can't enabled both
> on Juno. We had same issue even on TC2. I will follow up internally to
> see if we can fix on Juno at-least.

Sounds like something that should be fixed for all Versatile
Express that have cpuidle?

Yours,
Linus Walleij

[PATCH] ARM64: juno: add NOR flash to device tree

[Sudeep Holla]

On 18/10/15 10:25, Linus Walleij wrote:
> On Thu, Oct 15, 2015 at 5:30 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> Me
>>>   Results after
>>> enabling MTD, MTD_CFI, MTD_PHYSMAP, MTD_PHYSMAP_OF,
>>> MTD_CFI_INTELEXT:
>>
>> Just a note on this: we can't enable these when CPUIDLE is enabled as
>> the firmware assumes the flash is always in read mode while Linux leaves
>> NOR flash in "read id" mode after initialization.
>
> Sorry I'm too unfamiliar with this lingo. Can you give me some detail
> on what "read mode" and "read id" mode is all about?
>

OK, sorry for that I just quoted from my understanding. Here is the
actual one from the data sheet:

"The device can be in any of four read states: Read Array, Read
  Identifier, Read Status or Read Query. Upon power-up, or after a reset,
  the device defaults to Read Array. To change the read state, the
  appropriate read command must be written to the device..."

After the driver gets initialized, it usually leaves the device in Read
Identifier state as the last command sent is to read ID from the chip.

>> So until the firmware is ready to handle that, we can't enabled both
>> on Juno. We had same issue even on TC2. I will follow up internally to
>> see if we can fix on Juno at-least.
>
> Sounds like something that should be fixed for all Versatile
> Express that have cpuidle?
>

Correct, I just mentioned so that if you see/get any boot issue report
with NOR flash and CPUIdle enabled, you will have the background instead
of investigating the known issue again.

-- 
Regards,
Sudeep

[PATCH] ARM64: juno: add NOR flash to device tree

[Mark Rutland]

On Mon, Oct 19, 2015 at 11:17:01AM +0100, Sudeep Holla wrote:
> 
> 
> On 18/10/15 10:25, Linus Walleij wrote:
> >On Thu, Oct 15, 2015 at 5:30 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >>Me
> >>>  Results after
> >>>enabling MTD, MTD_CFI, MTD_PHYSMAP, MTD_PHYSMAP_OF,
> >>>MTD_CFI_INTELEXT:
> >>
> >>Just a note on this: we can't enable these when CPUIDLE is enabled as
> >>the firmware assumes the flash is always in read mode while Linux leaves
> >>NOR flash in "read id" mode after initialization.
> >
> >Sorry I'm too unfamiliar with this lingo. Can you give me some detail
> >on what "read mode" and "read id" mode is all about?
> >
> 
> OK, sorry for that I just quoted from my understanding. Here is the
> actual one from the data sheet:
> 
> "The device can be in any of four read states: Read Array, Read
>  Identifier, Read Status or Read Query. Upon power-up, or after a reset,
>  the device defaults to Read Array. To change the read state, the
>  appropriate read command must be written to the device..."
> 
> After the driver gets initialized, it usually leaves the device in Read
> Identifier state as the last command sent is to read ID from the chip.
> 
> >>So until the firmware is ready to handle that, we can't enabled both
> >>on Juno. We had same issue even on TC2. I will follow up internally to
> >>see if we can fix on Juno at-least.
> >
> >Sounds like something that should be fixed for all Versatile
> >Express that have cpuidle?
> >
> 
> Correct, I just mentioned so that if you see/get any boot issue report
> with NOR flash and CPUIdle enabled, you will have the background instead
> of investigating the known issue again.

Also, for non U-Boot systems I was under the impression that the flash
was effectively owned by EFI (for variable storage), so it doesn't seem
like a sensible idea to me to poke the flash behind its back in that
case.

Given that, I'm not sure that the flash node should be enabled by
default in the DT.

Leif?

Thanks,
Mark.

[PATCH] ARM64: juno: add NOR flash to device tree

[Linus Walleij]

On Mon, Oct 19, 2015 at 12:29 PM, Mark Rutland <mark.rutland@arm.com> wrote:

> Also, for non U-Boot systems I was under the impression that the flash
> was effectively owned by EFI (for variable storage),

If it is "owned" by something it is owned by the boot monitor, neither
by U-Boot or (U)EFI. The boot monitor can flash "images" into
the flash using USB or ethernet/TFTP and it is not part of any
boot loader, but an autonomous flash image on the board.
(Actually it can replace itself.)

EFI or U-Boot is one such "image", as is the kernel and the
other patch series I've sent (AFSv2 support) makes these
"images" visible to the kernel as individual MTD partitions.

This flash image format has nothing to do with UEFI or any other
boot loader, it has been around since the RealView platforms,
and has basically not changed since. (Well the forst pointer in
the image information structure was changed from u32 to u64).

My impression is that the boot monitor code is just a continous
evolution from reference design to reference design inside ARM.
It cares very little about what boot loader happens to be the
fashion of the day, it is also used for flashing images of Windows
mobile or QNX or whatever people want to run as well.

The "images" are very simplistic things designed to support
execute-in-place. New images can be added/removed by any
compliant software, be it EFI, U-Boot or the kernel. But the
format of these images is part of the hardware or rather the boot
monitor, nothing else.

Yours,
Linus Walleij

[PATCH] ARM64: juno: add NOR flash to device tree

[Leif Lindholm]

On Mon, Oct 19, 2015 at 11:29:32AM +0100, Mark Rutland wrote:
> > Correct, I just mentioned so that if you see/get any boot issue report
> > with NOR flash and CPUIdle enabled, you will have the background instead
> > of investigating the known issue again.
> 
> Also, for non U-Boot systems I was under the impression that the flash
> was effectively owned by EFI (for variable storage), so it doesn't seem
> like a sensible idea to me to poke the flash behind its back in that
> case.
> 
> Given that, I'm not sure that the flash node should be enabled by
> default in the DT.
> 
> Leif?

Well, given that it's a NOR device (or even two), it doesn't
necessarily have to be an all-or-nothing decision.

0x0BFC0000-0x0BFFFFFF ("erase region 1", the last 256KB) is used for
the persistent storage. This is hard wired into the UEFI image.

But the rest of the flash consists of whatever the contents of
images.txt in the magic configuration-via-USB-filesystem says it is.
I'm not convinced that is written to the device in AFS format.

/
    Leif

[PATCH] ARM64: juno: add NOR flash to device tree

[Leif Lindholm]

On Mon, Oct 19, 2015 at 01:19:14PM +0200, Linus Walleij wrote:
> On Mon, Oct 19, 2015 at 12:29 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > Also, for non U-Boot systems I was under the impression that the flash
> > was effectively owned by EFI (for variable storage),
> 
> If it is "owned" by something it is owned by the boot monitor, neither
> by U-Boot or (U)EFI. The boot monitor can flash "images" into
> the flash using USB or ethernet/TFTP and it is not part of any
> boot loader, but an autonomous flash image on the board.
> (Actually it can replace itself.)

No.

This would be true for any preceding ARM devboard, but Juno's boot
architecture is different. The thing that gives the 'Cmd>' prompt is
running on a system microcontroller. It writes images to the system
NOR based on the contents of images.txt in the magic USB filesystem.

While I think you _can_ run the boot monitor on Juno, it it not part
of the UEFI boot path, and I didn't think it was for U-Boot either?
It certainly shouldn't be needed.

/
    Leif

[PATCH] ARM64: juno: add NOR flash to device tree

[Mark Rutland]

On Mon, Oct 19, 2015 at 01:19:14PM +0200, Linus Walleij wrote:
> On Mon, Oct 19, 2015 at 12:29 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > Also, for non U-Boot systems I was under the impression that the flash
> > was effectively owned by EFI (for variable storage),
> 
> If it is "owned" by something it is owned by the boot monitor, neither
> by U-Boot or (U)EFI. The boot monitor can flash "images" into
> the flash using USB or ethernet/TFTP and it is not part of any
> boot loader, but an autonomous flash image on the board.
> (Actually it can replace itself.)
> 
> EFI or U-Boot is one such "image", as is the kernel and the
> other patch series I've sent (AFSv2 support) makes these
> "images" visible to the kernel as individual MTD partitions.

I understand this.

> This flash image format has nothing to do with UEFI or any other
> boot loader, it has been around since the RealView platforms,
> and has basically not changed since. (Well the forst pointer in
> the image information structure was changed from u32 to u64).
> 
> My impression is that the boot monitor code is just a continous
> evolution from reference design to reference design inside ARM.
> It cares very little about what boot loader happens to be the
> fashion of the day, it is also used for flashing images of Windows
> mobile or QNX or whatever people want to run as well.

I'm on about _runtime_ firmware (UEFI and/or Arm Trusted Firmware), not
the first stage firmware that's out of the way by the time the OS is
active.

As I mentioned, I was under the impression that at least a portion of
the flash was used by UEFI for variable storage, and that UEFI could
therefore access the flash at runtime (due to runtime services calls).

Accessing the flash at the same time as another agent (UEFI or the
Trusted Firmare) does not seem like a good idea, and is potentially
unsafe (see Sudeep's example w.r.t. cpuidle).

> The "images" are very simplistic things designed to support
> execute-in-place. New images can be added/removed by any
> compliant software, be it EFI, U-Boot or the kernel. But the
> format of these images is part of the hardware or rather the boot
> monitor, nothing else.

This is technically true. However, my argument is not that this doesn't
describe the hardware, but rather that this describes a piece of
hardware which it is not safe to use unless it is known that any runtime
firmware is not concurrently accessing it.

Given that there is no mechanism to mediate access to the flash,
allowing Linux to access it does not seem safe.

Thanks,
Mark.

[PATCH] ARM64: juno: add NOR flash to device tree

[Ard Biesheuvel]

On 19 October 2015 at 13:39, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Oct 19, 2015 at 01:19:14PM +0200, Linus Walleij wrote:
>> On Mon, Oct 19, 2015 at 12:29 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>
>> > Also, for non U-Boot systems I was under the impression that the flash
>> > was effectively owned by EFI (for variable storage),
>>
>> If it is "owned" by something it is owned by the boot monitor, neither
>> by U-Boot or (U)EFI. The boot monitor can flash "images" into
>> the flash using USB or ethernet/TFTP and it is not part of any
>> boot loader, but an autonomous flash image on the board.
>> (Actually it can replace itself.)
>>
>> EFI or U-Boot is one such "image", as is the kernel and the
>> other patch series I've sent (AFSv2 support) makes these
>> "images" visible to the kernel as individual MTD partitions.
>
> I understand this.
>
>> This flash image format has nothing to do with UEFI or any other
>> boot loader, it has been around since the RealView platforms,
>> and has basically not changed since. (Well the forst pointer in
>> the image information structure was changed from u32 to u64).
>>
>> My impression is that the boot monitor code is just a continous
>> evolution from reference design to reference design inside ARM.
>> It cares very little about what boot loader happens to be the
>> fashion of the day, it is also used for flashing images of Windows
>> mobile or QNX or whatever people want to run as well.
>
> I'm on about _runtime_ firmware (UEFI and/or Arm Trusted Firmware), not
> the first stage firmware that's out of the way by the time the OS is
> active.
>
> As I mentioned, I was under the impression that at least a portion of
> the flash was used by UEFI for variable storage, and that UEFI could
> therefore access the flash at runtime (due to runtime services calls).
>
> Accessing the flash at the same time as another agent (UEFI or the
> Trusted Firmare) does not seem like a good idea, and is potentially
> unsafe (see Sudeep's example w.r.t. cpuidle).
>
>> The "images" are very simplistic things designed to support
>> execute-in-place. New images can be added/removed by any
>> compliant software, be it EFI, U-Boot or the kernel. But the
>> format of these images is part of the hardware or rather the boot
>> monitor, nothing else.
>
> This is technically true. However, my argument is not that this doesn't
> describe the hardware, but rather that this describes a piece of
> hardware which it is not safe to use unless it is known that any runtime
> firmware is not concurrently accessing it.
>
> Given that there is no mechanism to mediate access to the flash,
> allowing Linux to access it does not seem safe.
>

I sent some patches around about a year ago that registers UEFI
runtime MMIO regions with /proc/iomem, which should prevent drivers
from attaching to the same physical memory regions. With that in
place, describing it in the DT would not be a problem, and on devices
with multiple NOR flashes of which only one is used at runtime by
UEFI, it would allow Linux to still access the other one.

-- 
Ard.

[PATCH] ARM64: juno: add NOR flash to device tree

[Linus Walleij]

On Mon, Oct 19, 2015 at 1:27 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> This would be true for any preceding ARM devboard, but Juno's boot
> architecture is different. The thing that gives the 'Cmd>' prompt is
> running on a system microcontroller. It writes images to the system
> NOR based on the contents of images.txt in the magic USB filesystem.

Yeah OK I stand corrected ... I just called that thing boot monitor
because I never saw a real name for it. It's something system-resident
that is not the boot loader anyways, and it doesn't seem designed
with any one particular boot architecture in mind.

Yours,
Linus Walleij

[PATCH] ARM64: juno: add NOR flash to device tree

[Linus Walleij]

On Mon, Oct 19, 2015 at 1:39 PM, Mark Rutland <mark.rutland@arm.com> wrote:

> As I mentioned, I was under the impression that at least a portion of
> the flash was used by UEFI for variable storage, and that UEFI could
> therefore access the flash at runtime (due to runtime services calls).

OK I see. But doesn't that portion of the flash live inside one of the
AFS partitions simply?

If it is not then Juno has a mess of structured/unstructured flash
layout and it should be fixed by wrapping it into an AFS partition.

> Accessing the flash at the same time as another agent (UEFI or the
> Trusted Firmare) does not seem like a good idea, and is potentially
> unsafe (see Sudeep's example w.r.t. cpuidle).

No why would anyone do that. As with all file systems and
partitions it is possible to destroy them if you can access them,
with great power comes great responsibility as always, I'm just
not fond of the idea of hiding things behind the back for hackers
"for their own good", that is not the right spirit I think.

> Given that there is no mechanism to mediate access to the flash,
> allowing Linux to access it does not seem safe.

Linux does not access it, it just gives it a name and says
"there it is". Policy regarding accessing it is another issue
altogether. I also made the series with AFSv2 support.

Linus

[PATCH] ARM64: juno: add NOR flash to device tree

[Linus Walleij]

On Mon, Oct 19, 2015 at 4:40 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:

> I sent some patches around about a year ago that registers UEFI
> runtime MMIO regions with /proc/iomem, which should prevent drivers
> from attaching to the same physical memory regions. With that in
> place, describing it in the DT would not be a problem, and on devices
> with multiple NOR flashes of which only one is used at runtime by
> UEFI, it would allow Linux to still access the other one.

That is very un-kernellike I think, isn't it better that it gets defined as
an MTD partition?

None of it should affect registering the flash in the device tree
however, it is indeed the hardware that is there, it is a device tree.

We could discuss switching MTD and/or AFS off for EFI boots,
but it seems a bit overzealous to me.

Yours,
Linus Walleij

[PATCH] ARM64: juno: add NOR flash to device tree

[Leif Lindholm]

On Mon, Oct 19, 2015 at 11:55:32PM +0200, Linus Walleij wrote:
> > As I mentioned, I was under the impression that at least a portion of
> > the flash was used by UEFI for variable storage, and that UEFI could
> > therefore access the flash at runtime (due to runtime services calls).
> 
> OK I see. But doesn't that portion of the flash live inside one of the
> AFS partitions simply?

Are you saying that you see AFS partitions matching the entries in
your platform's SITE1/HBI0262?/images.txt?

If that is the case, then certainly that is a reasonable mechanism to
make use of. However, the log in your commit message didn't show
any partitions being detected. And you should have at least a bl0.bin,
bl1.bin, fip.bin and hdlcdclk.dat in there.

> No why would anyone do that. As with all file systems and
> partitions it is possible to destroy them if you can access them,
> with great power comes great responsibility as always, I'm just
> not fond of the idea of hiding things behind the back for hackers
> "for their own good", that is not the right spirit I think.

If it is actually possible to describe this properly to the kernel, I
agree. But, due to the somewhat hyper-flexible boot architecture of
Juno, we may end up with different boards of the same revision having
different "safe" regions.

And if the regions are detected, we still need to have a protective
entry for the UEFI persistent data region added to future Juno firmare
releases.

/
    Leif

[PATCH] ARM64: juno: add NOR flash to device tree

[Ard Biesheuvel]

On 19 October 2015 at 23:58, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Oct 19, 2015 at 4:40 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>
>> I sent some patches around about a year ago that registers UEFI
>> runtime MMIO regions with /proc/iomem, which should prevent drivers
>> from attaching to the same physical memory regions. With that in
>> place, describing it in the DT would not be a problem, and on devices
>> with multiple NOR flashes of which only one is used at runtime by
>> UEFI, it would allow Linux to still access the other one.
>
> That is very un-kernellike I think, isn't it better that it gets defined as
> an MTD partition?
>

I think it makes perfect sense to reserve an MMIO region if it is in
active use by a driver, which is arguably the case for UEFI Runtime
Services that implement the abstract UEFI variable store on top of a
NOR flash device. Note that it is not the content that is reserved, it
is the fact that resident code may be invoked that assumes that it is
the sole owner of the NOR flash.

So this has nothing to do with partitioning. NOR write commands are
issues to the MMIO base address, and so a driver must own the entire
MMIO range, and not simply a slice that represents the data it is
interested in.

> None of it should affect registering the flash in the device tree
> however, it is indeed the hardware that is there, it is a device tree.
>

Agreed.

> We could discuss switching MTD and/or AFS off for EFI boots,
> but it seems a bit overzealous to me.
>

Nope. Each flash /device/ is either owned by UEFI or by the kernel, not both.

-- 
Ard.

[PATCH] ARM64: juno: add NOR flash to device tree

[Ryan Harkin]

On 19 October 2015 at 12:20, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Mon, Oct 19, 2015 at 11:29:32AM +0100, Mark Rutland wrote:
>> > Correct, I just mentioned so that if you see/get any boot issue report
>> > with NOR flash and CPUIdle enabled, you will have the background instead
>> > of investigating the known issue again.
>>
>> Also, for non U-Boot systems I was under the impression that the flash
>> was effectively owned by EFI (for variable storage), so it doesn't seem
>> like a sensible idea to me to poke the flash behind its back in that
>> case.
>>
>> Given that, I'm not sure that the flash node should be enabled by
>> default in the DT.
>>
>> Leif?
>
> Well, given that it's a NOR device (or even two), it doesn't
> necessarily have to be an all-or-nothing decision.
>
> 0x0BFC0000-0x0BFFFFFF ("erase region 1", the last 256KB) is used for
> the persistent storage. This is hard wired into the UEFI image.
>
> But the rest of the flash consists of whatever the contents of
> images.txt in the magic configuration-via-USB-filesystem says it is.
> I'm not convinced that is written to the device in AFS format.

After a discussion on IRC, Leif asked me to email the list to describe
how AFS works.  I read Linus' code in u-boot [1] to work it out.

There is no global TOC placed in NOR or elsewhere.

Each image stored in NOR flash contains a "footer" at the end of the
last sector of the image.  This footer describes the image, including
it's image "name".

To find all of the images in NOR flash, the AFS code has to scan for
the footer's signature at the end of every sector in flash.  The v1
footer is the last 4 bytes of the sector, the v2 footer is the last 8
bytes.  Juno uses v2 footers.

For example, the first file in Juno's NOR flash is "fip".  It lives at
address 0x08000000 and occupies two 256k sectors (08000000 and
08040000).  So the footer lives at the end of the last sector, from
address 0807ff70 to 0807ffff.

[1] common/cmd_armflash.c
http://git.denx.de/?p=u-boot.git;a=blob;f=common/cmd_armflash.c;h=af453f7b3b84aba97704fd9ff2178d00876b6aad;hb=5ec0003b19cbdf06ccd6941237cbc0d1c3468e2d

[PATCH] arm64/efi: register UEFI runtime mmio regions as iomem resources

[Ard Biesheuvel]

In order to prevent kernel drivers from attaching to MMIO regions
that are owned by the firmware, register them as iomem resources.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/efi.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index a48d1f477b2e..f2c6763d6c37 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -229,6 +229,22 @@ void __init efi_init(void)
 	early_memunmap(memmap.map, params.mmap_size);
 }
 
+static void __init efi_reserve_iomem_resource(efi_memory_desc_t *md)
+{
+	struct resource *res;
+
+	res = kzalloc(sizeof(*res), GFP_KERNEL);
+	if (WARN_ON_ONCE(!res))
+		return;
+
+	res->start = md->phys_addr & PAGE_MASK;
+	res->end = PAGE_ALIGN(md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT)) - 1;
+	res->name = "UEFI Runtime MMIO";
+	res->flags = IORESOURCE_MEM | IORESOURCE_BUSY | IORESOURCE_EXCLUSIVE;
+
+	request_resource(&iomem_resource, res);
+}
+
 static bool __init efi_virtmap_init(void)
 {
 	efi_memory_desc_t *md;
@@ -239,6 +255,8 @@ static bool __init efi_virtmap_init(void)
 
 		if (!(md->attribute & EFI_MEMORY_RUNTIME))
 			continue;
+		if (md->type == EFI_MEMORY_MAPPED_IO)
+			efi_reserve_iomem_resource(md);
 		if (md->virt_addr == 0)
 			return false;
 
-- 
2.1.4

[PATCH] ARM64: juno: add NOR flash to device tree

[Linus Walleij]

On Wed, Oct 21, 2015 at 12:07 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote:

> FYI, in the latest Juno motherboard firmware [1], I have a file called
> "blank.img" in the NOR flash at address 0x0BFC0000 that represents the UEFI
> (and now u-boot) config area at the end of the NOR flash.
>
> I mostly put this there so that you can "touch blank.img" to erase the
> config, but also to show that the region is reserved to anyone who might
> think about putting something there.

Then it is safe to enable NOR flash and AFS in the kernel.
The AFS "partition" should probably have a better name than
"blank.img" (if this is the name it gets in the flash), rather
"BOOTENV" or something.

I can augment the code in the recently cleaned-up AFS driver
to make that partition read-only by setting the MTD_WRITABLE
flag in the mask_flags field if we agree on some good name
like that.

Yours,
Linus Walleij

[PATCH] ARM64: juno: add NOR flash to device tree

[Linus Walleij]

On Wed, Oct 21, 2015 at 1:18 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote:

> There is no global TOC placed in NOR or elsewhere.
>
> Each image stored in NOR flash contains a "footer" at the end of the
> last sector of the image.  This footer describes the image, including
> it's image "name".
>
> To find all of the images in NOR flash, the AFS code has to scan for
> the footer's signature at the end of every sector in flash.  The v1
> footer is the last 4 bytes of the sector, the v2 footer is the last 8
> bytes.  Juno uses v2 footers.

Yeah it's made like this to facilitate execute-in-place. You can't
mess with the data in the beginning of the image if you're going
to be able to execute it right off.

Not that I know if execute-in-place for ARM64 was ever an option,
it can be done I guess... anyways this is a legacy format for
all ARM reference designs.

As mentioned it the other mail, if we agree on a name for the
read-only bootloader/EFI data partition, it can easily be made into
as a readonly thing.

Yours,
Linus Walleij

[PATCH] ARM64: juno: add NOR flash to device tree

[Sudeep Holla]

On 27/10/15 11:55, Linus Walleij wrote:
> On Wed, Oct 21, 2015 at 12:07 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote:
>
>> FYI, in the latest Juno motherboard firmware [1], I have a file called
>> "blank.img" in the NOR flash at address 0x0BFC0000 that represents the UEFI
>> (and now u-boot) config area at the end of the NOR flash.
>>
>> I mostly put this there so that you can "touch blank.img" to erase the
>> config, but also to show that the region is reserved to anyone who might
>> think about putting something there.
>
> Then it is safe to enable NOR flash and AFS in the kernel.

*No*, not on Juno. If we need to enable them for other platforms, then
we should either disable NOR flash in DT(or even remove it completely
whichever is appropriate).

Since idle is enable in defconfig and if DT has idle states, then it
can't boot for the reason I previously mentioned.

-- 
Regards,
Sudeep

[PATCH] arm64/efi: register UEFI runtime mmio regions as iomem resources

[Linus Walleij]

On Mon, Oct 26, 2015 at 9:41 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:

> In order to prevent kernel drivers from attaching to MMIO regions
> that are owned by the firmware, register them as iomem resources.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Is this related to the flash memory discussion so that NOR flash
will be part of what is considered "I/O memory" and flash
protection achieved this way?

Yours,
Linus Walleij

[PATCH] ARM64: juno: add NOR flash to device tree

[Linus Walleij]

On Tue, Oct 27, 2015 at 1:01 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On 27/10/15 11:55, Linus Walleij wrote:
>> On Wed, Oct 21, 2015 at 12:07 PM, Ryan Harkin <ryan.harkin@linaro.org>
>> wrote:
>>
>>> FYI, in the latest Juno motherboard firmware [1], I have a file called
>>> "blank.img" in the NOR flash at address 0x0BFC0000 that represents the
>>> UEFI
>>> (and now u-boot) config area at the end of the NOR flash.
>>>
>>> I mostly put this there so that you can "touch blank.img" to erase the
>>> config, but also to show that the region is reserved to anyone who might
>>> think about putting something there.
>>
>>
>> Then it is safe to enable NOR flash and AFS in the kernel.
>
> *No*, not on Juno. If we need to enable them for other platforms, then
> we should either disable NOR flash in DT(or even remove it completely
> whichever is appropriate).
>
> Since idle is enable in defconfig and if DT has idle states, then it
> can't boot for the reason I previously mentioned.

Yeah right I remember that now. Let's say it is safe to enable
for the tamper-security reason. There may be other reasons
not to...

If this is about different idle functionalities blocking flash
access, what we should do is to in Kconfig make it impossible
to compile in both deep idle states and flash support at the
same time, as that is how the system really behaves.

I.e do you mean something like this, or am I getting it wrong?

index 21340e0be73e..07d91776bcfe 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -4,6 +4,7 @@
 config ARM_CPUIDLE
         bool "Generic ARM/ARM64 CPU idle Driver"
         select DT_IDLE_STATES
+       depends on !(ARCH_VEXPRESS && MTD)
         help
           Select this to enable generic cpuidle driver for ARM.
           It provides a generic idle driver whose idle states are configured

Hiding hardware from the devicetree seems over the top to me,
it is better to keep the device trees describing the actual hardware
and the operating system to figure out how things need to be
set up to interoperate, at config-time or run-time.

Yours,
Linus Walleij

[PATCH] arm64/efi: register UEFI runtime mmio regions as iomem resources

[Ard Biesheuvel]

On 27 October 2015 at 21:08, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Oct 26, 2015 at 9:41 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>
>> In order to prevent kernel drivers from attaching to MMIO regions
>> that are owned by the firmware, register them as iomem resources.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Is this related to the flash memory discussion so that NOR flash
> will be part of what is considered "I/O memory"

Yes

> and flash
> protection achieved this way?
>

... and no, not entirely. Note that this is not about 'protecting' the
flash in a security sense. It is simply to reserve the resources that
should be considered occupied by the UEFI firmware drivers. This may
be NOR flash, but it may just as well be the RTC or potentially a
syscon register block to do reset and/or poweroff.

My QEMU mach-virt /proc/iomem looks like this with the patch applied:

04000000-07ffffff : UEFI Runtime MMIO
09000000-09000fff : /pl011 at 9000000
  09000000-09000fff : /pl011 at 9000000
09010000-0901ffff : UEFI Runtime MMIO
40000000-7fffffff : System RAM
  40080000-408a27d3 : Kernel code
  40970000-40adffff : Kernel data

where the first UEFI mmio region is the second bank of NOR flash (the
first one holds the firmware image itself, so that is not claimed
since the variable store driver never accesses it) and the second UEFI
mmio region is the PL031 RTC.

-- 
Ard.

[PATCH] ARM64: juno: add NOR flash to device tree

[Sudeep Holla]

On 27/10/15 12:22, Linus Walleij wrote:
> On Tue, Oct 27, 2015 at 1:01 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On 27/10/15 11:55, Linus Walleij wrote:

[...]

>>> Then it is safe to enable NOR flash and AFS in the kernel.
>>
>> *No*, not on Juno. If we need to enable them for other platforms, then
>> we should either disable NOR flash in DT(or even remove it completely
>> whichever is appropriate).
>>
>> Since idle is enable in defconfig and if DT has idle states, then it
>> can't boot for the reason I previously mentioned.
>
> Yeah right I remember that now. Let's say it is safe to enable
> for the tamper-security reason. There may be other reasons
> not to...
>
> If this is about different idle functionalities blocking flash
> access, what we should do is to in Kconfig make it impossible
> to compile in both deep idle states and flash support at the
> same time, as that is how the system really behaves.
>
> I.e do you mean something like this, or am I getting it wrong?
>

But with this you will never enable idle on all VEXPRESS platforms, what
if the issue on Juno is resolved in future silicon. So I prefer DT way
if we have to enable NOR flash in defconfig.

> index 21340e0be73e..07d91776bcfe 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -4,6 +4,7 @@
>   config ARM_CPUIDLE
>           bool "Generic ARM/ARM64 CPU idle Driver"
>           select DT_IDLE_STATES
> +       depends on !(ARCH_VEXPRESS && MTD)
>           help
>             Select this to enable generic cpuidle driver for ARM.
>             It provides a generic idle driver whose idle states are configured
>
> Hiding hardware from the devicetree seems over the top to me,

OK, I agree with you on not hiding. But disabling it seems OK for me as
it's a hardware errata(SROM can't be used and hence NOR is used in warm
reset path)

> it is better to keep the device trees describing the actual hardware

Agreed, but we need a way to specify that it used as alternative for
SROM to workaround the hardware issue and hence not available for Linux.
One way I believe is setting the status as disabled.

-- 
Regards,
Sudeep

[PATCH] ARM64: juno: add NOR flash to device tree

[Mark Rutland]

On Tue, Oct 27, 2015 at 01:22:39PM +0100, Linus Walleij wrote:
> On Tue, Oct 27, 2015 at 1:01 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > On 27/10/15 11:55, Linus Walleij wrote:
> >> On Wed, Oct 21, 2015 at 12:07 PM, Ryan Harkin <ryan.harkin@linaro.org>
> >> wrote:
> >>
> >>> FYI, in the latest Juno motherboard firmware [1], I have a file called
> >>> "blank.img" in the NOR flash at address 0x0BFC0000 that represents the
> >>> UEFI
> >>> (and now u-boot) config area at the end of the NOR flash.
> >>>
> >>> I mostly put this there so that you can "touch blank.img" to erase the
> >>> config, but also to show that the region is reserved to anyone who might
> >>> think about putting something there.
> >>
> >>
> >> Then it is safe to enable NOR flash and AFS in the kernel.
> >
> > *No*, not on Juno. If we need to enable them for other platforms, then
> > we should either disable NOR flash in DT(or even remove it completely
> > whichever is appropriate).
> >
> > Since idle is enable in defconfig and if DT has idle states, then it
> > can't boot for the reason I previously mentioned.
> 
> Yeah right I remember that now. Let's say it is safe to enable
> for the tamper-security reason. There may be other reasons
> not to...
> 
> If this is about different idle functionalities blocking flash
> access, what we should do is to in Kconfig make it impossible
> to compile in both deep idle states and flash support at the
> same time, as that is how the system really behaves.
> 
> I.e do you mean something like this, or am I getting it wrong?
> 
> index 21340e0be73e..07d91776bcfe 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -4,6 +4,7 @@
>  config ARM_CPUIDLE
>          bool "Generic ARM/ARM64 CPU idle Driver"
>          select DT_IDLE_STATES
> +       depends on !(ARCH_VEXPRESS && MTD)
>          help
>            Select this to enable generic cpuidle driver for ARM.
>            It provides a generic idle driver whose idle states are configured
> 
> Hiding hardware from the devicetree seems over the top to me,
> it is better to keep the device trees describing the actual hardware
> and the operating system to figure out how things need to be
> set up to interoperate, at config-time or run-time.

... except that the current bindings do not imply any of said
negotiation, and imply that the hardware is available for use. We'd need
new bindings to describe that.

For situations like these we typically have the node, but ensure that it
has status = "disabled". That way the DT is safe by default, and FW can
grant the OS the ability to use the hardware by replacing/removing the
status property.

It sounds like even for U-Boot, PSCI* and Non-secure OS access to the NOR
are mutually exclusive. Given the inability to describe that constraint,
the only agent that can possibly filter the DT appropriately is the FW.

* Sudeep, I assume the constraints on CPU_SUSPEND would also apply to
  CPU_ON, is that correct?

Thanks,
Mark.

[PATCH] ARM64: juno: add NOR flash to device tree

[Sudeep Holla]

On 27/10/15 12:33, Mark Rutland wrote:

[...]

>
> * Sudeep, I assume the constraints on CPU_SUSPEND would also apply to
>    CPU_ON, is that correct?
>

Yes, if you meant that this is an issue for even CPU hotplug. We need to
disable CPU_HOTPLUG in that case which is horrible.

So this kind of conditional compile is simply not a solution to
support/work around an hardware issue on one platform.

-- 
Regards,
Sudeep

[PATCH] ARM64: juno: add NOR flash to device tree

[Ryan Harkin]

On 27 October 2015 at 11:55, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Oct 21, 2015 at 12:07 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote:
>
>> FYI, in the latest Juno motherboard firmware [1], I have a file called
>> "blank.img" in the NOR flash at address 0x0BFC0000 that represents the UEFI
>> (and now u-boot) config area at the end of the NOR flash.
>>
>> I mostly put this there so that you can "touch blank.img" to erase the
>> config, but also to show that the region is reserved to anyone who might
>> think about putting something there.
>
> Then it is safe to enable NOR flash and AFS in the kernel.
> The AFS "partition" should probably have a better name than
> "blank.img" (if this is the name it gets in the flash), rather
> "BOOTENV" or something.
>
> I can augment the code in the recently cleaned-up AFS driver
> to make that partition read-only by setting the MTD_WRITABLE
> flag in the mask_flags field if we agree on some good name
> like that.

Agreed.  I'll rename the AFS partition to "BOOTENV" using the NORxNAME
entry in images.txt.  I'll rename the file to "bootenv.img".

[PATCH] ARM64: juno: add NOR flash to device tree

[Linus Walleij]

On Tue, Oct 27, 2015 at 1:33 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

> OK, I agree with you on not hiding. But disabling it seems OK for me as
> it's a hardware errata(SROM can't be used and hence NOR is used in warm
> reset path)

OK then, what about:

status = "disabled";

With a comment explaining why?

Yours,
Linus Walleij

[PATCH] ARM64: juno: add NOR flash to device tree

[Linus Walleij]

On Tue, Oct 27, 2015 at 1:33 PM, Mark Rutland <mark.rutland@arm.com> wrote:

> For situations like these we typically have the node, but ensure that it
> has status = "disabled". That way the DT is safe by default, and FW can
> grant the OS the ability to use the hardware by replacing/removing the
> status property.

Fair enough, I suggested the same as it happens. Will repost
with this changed and a fat comment describing why.

Yours,
Linus Walleij

[PATCH] arm64/efi: register UEFI runtime mmio regions as iomem resources

[Ard Biesheuvel]

On 27 October 2015 at 13:31, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 27 October 2015 at 21:08, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, Oct 26, 2015 at 9:41 AM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>
>>> In order to prevent kernel drivers from attaching to MMIO regions
>>> that are owned by the firmware, register them as iomem resources.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> Is this related to the flash memory discussion so that NOR flash
>> will be part of what is considered "I/O memory"
>
> Yes
>
>> and flash
>> protection achieved this way?
>>
>
> ... and no, not entirely. Note that this is not about 'protecting' the
> flash in a security sense. It is simply to reserve the resources that
> should be considered occupied by the UEFI firmware drivers. This may
> be NOR flash, but it may just as well be the RTC or potentially a
> syscon register block to do reset and/or poweroff.
>
> My QEMU mach-virt /proc/iomem looks like this with the patch applied:
>
> 04000000-07ffffff : UEFI Runtime MMIO
> 09000000-09000fff : /pl011 at 9000000
>   09000000-09000fff : /pl011 at 9000000
> 09010000-0901ffff : UEFI Runtime MMIO
> 40000000-7fffffff : System RAM
>   40080000-408a27d3 : Kernel code
>   40970000-40adffff : Kernel data
>
> where the first UEFI mmio region is the second bank of NOR flash (the
> first one holds the firmware image itself, so that is not claimed
> since the variable store driver never accesses it) and the second UEFI
> mmio region is the PL031 RTC.
>

Ping?