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

[Supreeth Venkatesh <supreeth.venkatesh@arm.com>]

On Wed, 2017-01-18 at 22:05 +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/19bb46c411279dcd30d540c56e59
> 93c5f771c319
> 
> Was the reasoning behind this commit incorrect - do you have a
> (pre-DXE?) use-case that creates a problem?
> 
Use Case that creates a problem:
With cache state modeling enabled (i.e, setting it to "1") in FVP, it
will not boot up. 
Perhaps, need to check FVP implementation is correct? 

> Regards,
> 
> Leif
> 
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> > ---
> >  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> > | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git
> > a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.
> > c
> > b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.
> > c
> > index 14c7e8e..2685114 100644
> > ---
> > a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.
> > c
> > +++
> > b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.
> > c
> > @@ -116,7 +116,7 @@ ArmPlatformGetVirtualMemoryMap (
> >    VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_NOR0_BASE;
> >    VirtualMemoryTable[Index].VirtualBase = ARM_VE_SMB_NOR0_BASE;
> >    VirtualMemoryTable[Index].Length = ARM_VE_SMB_NOR0_SZ +
> > ARM_VE_SMB_NOR1_SZ;
> > -  VirtualMemoryTable[Index].Attributes = CacheAttributes;
> > +  VirtualMemoryTable[Index].Attributes =
> > ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >  
> >    // SMB CS2 - SRAM
> >    VirtualMemoryTable[++Index].PhysicalBase = ARM_VE_SMB_SRAM_BASE;