Re: [PATCH] ArmPlatformPkg/ArmVExpressPkg: Fix memory attributes for NOR Flash

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 19 January 2017 at 12:31, Achin Gupta <achin.gupta@arm.com> wrote:
> Hi Leif/Ard,
>
> On Wed, Jan 18, 2017 at 10:05:00PM +0000, Leif Lindholm wrote:
>> Hi Achin,
>>
>> On Wed, Jan 18, 2017 at 08:24:06PM +0000, achin.gupta@arm.com wrote:
>> > From: Achin Gupta <achin.gupta@arm.com>
>> >
>> > The NOR flash banks were being mapped in the translation tables with the same
>> > memory attributes as RAM in the system. These attributes mark the region as
>> > Normal Memory and could additionally be cacheable or non-cacheable.
>> >
>> > Either type of attributes are unsuitable for NOR Flash since write operations
>> > could be performed on it. Normal Memory does not guarantee ordering of
>> > transactions that Device memory does. So the commands sent to the Flash device
>> > may not arrive in the right order unless barriers are used. Commands might not
>> > get past the CPU caches in case the region has been mapped with cacheable
>> > attributes.
>> >
>> > This patch fixes the problem by mapping the NOR Flash memory region with Device
>> > memory attributes.
>>
>> To add some background to Ard's comment - this was not unintentionally
>> done:
>> https://github.com/tianocore/edk2/commit/19bb46c411279dcd30d540c56e5993c5f771c319
>
> Thanks! I missed this commit. There is some background to the problem I am
> facing below.
>
>>
>> Was the reasoning behind this commit incorrect - do you have a
>> (pre-DXE?) use-case that creates a problem?
>
> AFAIU, The current memory attributes for NOR Flash work only for reads. They
> should additionally be RO to flag any unexpected writes.
>
> Mine is a DXE use case! In NorFlashDxe.c, commands are send to the Flash (erase
> block etc.). They might never reach the device if there is a write-back cache in
> between. So either device or Normal memory with non-cacheable/write-through
> attributes and barriers should work.
>
> If I turn on cache state modelling in the Base FVP, the code gets stuck in
> NorFlashReadStatusRegister() in the below loop in NorFlashEraseSingleBlock():
>
>   // Wait until the status register gives us the all clear
>   do {
>     StatusRegister = NorFlashReadStatusRegister (Instance, BlockAddress);
>   } while ((StatusRegister & P30_SR_BIT_WRITE) != P30_SR_BIT_WRITE);
>
> I think the SEND_NOR_COMMANDs at the beginning of the function get stuck in the
> cache. Changing the flash memory attributes as per this patch solves the
> problem.
>
> The original patch from Ard mentions that the NOR Flash DXE driver should change
> the attributes of the region it wants to write to. Is this what is missing?
>

NorFlashFvbInitialize() in
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c contains the
following calls:

  Status = gDS->AddMemorySpace (
      EfiGcdMemoryTypeMemoryMappedIo,
      Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
      EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
      );
  ASSERT_EFI_ERROR (Status);

  Status = gDS->SetMemorySpaceAttributes (
      Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
      EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
  ASSERT_EFI_ERROR (Status);

which is supposed to set device attributes on the NOR region that is
actually exposed to the upper layers as read-write capable.

Perhaps you can enable GCD debugging to trace these calls, to ensure
that the address you are stalled on is actually covered?