Re: [edk2-platforms: PATCH v2 3/3] Marvell/Drivers: Add non-mmio mode to MvFvbDxe

["Marcin Wojtas" <mw@semihalf.com>]

śr., 24 kwi 2019 o 15:51 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
>
> On Wed, 24 Apr 2019 at 15:48, Marcin Wojtas <mw@semihalf.com> wrote:
> >
> > Hi Ard,
> >
> > śr., 24 kwi 2019 o 15:02 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
> > >
> > > On Wed, 24 Apr 2019 at 09:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > > >
> > > > On Wed, 24 Apr 2019 at 08:52, Marcin Wojtas <mw@semihalf.com> wrote:
> > > > >
> > > > > From: Kornel Duleba <mindal@semihalf.com>
> > > > >
> > > > > This path enables support for reading variables directly from flash without
> > > > > relying on it to be memory mapped. It adds PcdSpiMemoryMapped PCD that
> > > > > allows to switch between the modes. When in non-memory-mapped mode the
> > > > > driver will copy the variables from flash to previously allocated buffer
> > > > > and set PcdFlashNvStorageVariableBase64, PcdFlashNvStorageFtwWorkingBase64
> > > > > and PcdFlashNvStorageFtwSpareBase64 accordingly.
> > > > >
> > > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > > > > ---
> > > > >  Silicon/Marvell/Marvell.dec                       |   8 ++
> > > > >  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc     |  10 +-
> > > > >  Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf |  17 ++-
> > > > >  Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.h   |   1 +
> > > > >  Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c   | 135 +++++++++++++++-----
> > > > >  5 files changed, 135 insertions(+), 36 deletions(-)
> > > > >
> > > > > diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec
> > > > > index 7210ba2..a23c329 100644
> > > > > --- a/Silicon/Marvell/Marvell.dec
> > > > > +++ b/Silicon/Marvell/Marvell.dec
> > > > > @@ -58,6 +58,12 @@
> > > > >
> > > > >    gMarvellFvbDxeGuid = { 0x42903750, 0x7e61, 0x4aaf, { 0x83, 0x29, 0xbf, 0x42, 0x36, 0x4e, 0x24, 0x85 } }
> > > > >    gMarvellSpiFlashDxeGuid = { 0x49d7fb74, 0x306d, 0x42bd, { 0x94, 0xc8, 0xc0, 0xc5, 0x4b, 0x18, 0x1d, 0xd7 } }
> > > > > +  #
> > > > > +  # Generic FaultTolerantWriteDxe driver use variables,
> > > > > +  # whose setting is done in MvFvbDxe driver in case
> > > > > +  # the SPI contents are not mapped in memory.
> > > > > +  #
> > > > > +  gFaultTolerantWriteDxeFileGuid = { 0xfe5cea76, 0x4f72, 0x49e8, { 0x98, 0x6f, 0x2c, 0xd8, 0x99, 0xdf, 0xfe, 0x5d} }
> > > > >
> > > > >  [LibraryClasses]
> > > > >    ArmadaBoardDescLib|Include/Library/ArmadaBoardDescLib.h
> > > > > @@ -140,6 +146,8 @@
> > > > >  #SPI
> > > > >    gMarvellTokenSpaceGuid.PcdSpiRegBase|0|UINT32|0x3000051
> > > > >    gMarvellTokenSpaceGuid.PcdSpiMemoryBase|0|UINT64|0x3000059
> > > > > +  gMarvellTokenSpaceGuid.PcdSpiMemoryMapped|TRUE|BOOLEAN|0x3000060
> > > > > +  gMarvellTokenSpaceGuid.PcdSpiVariableOffset|0|UINT32|0x3000061
> > > > >    gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|0|UINT32|0x30000052
> > > > >    gMarvellTokenSpaceGuid.PcdSpiClockFrequency|0|UINT32|0x30000053
> > > > >
> > > > > diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > > index ca3de2e..d53d128 100644
> > > > > --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > > +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> > > > > @@ -256,6 +256,11 @@
> > > > >    # USB support
> > > > >    gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
> > > > >
> > > > > +[PcdsDynamicDefault.common]
> > > > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0xF93C0000
> > > > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64|0xF93E0000
> > > > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64|0xF93D0000
> > > > > +
> > > > >  [PcdsFixedAtBuild.common]
> > > > >    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"MARVELL_EFI"
> > > > >    gArmPlatformTokenSpaceGuid.PcdCoreCount|4
> > > > > @@ -396,11 +401,10 @@
> > > > >    # Variable store - default values
> > > > >    #
> > > > >    gMarvellTokenSpaceGuid.PcdSpiMemoryBase|0xF9000000
> > > > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0xF93C0000
> > > > > +  gMarvellTokenSpaceGuid.PcdSpiMemoryMapped|TRUE
> > > > > +  gMarvellTokenSpaceGuid.PcdSpiVariableOffset|0x3C0000
> > > > >    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000
> > > > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64|0xF93D0000
> > > > >    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00010000
> > > > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64|0xF93E0000
> > > > >    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000
> > > > >
> > > > >  !if $(CAPSULE_ENABLE)
> > > > > diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
> > > > > index ef10bfd..c85e8a6 100644
> > > > > --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
> > > > > +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
> > > > > @@ -76,13 +76,22 @@
> > > > >    gMarvellSpiMasterProtocolGuid
> > > > >
> > > > >  [FixedPcd]
> > > > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
> > > > >    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> > > > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64
> > > > >    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
> > > > > -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64
> > > > >    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
> > > > >    gMarvellTokenSpaceGuid.PcdSpiMemoryBase
> > > > > +  gMarvellTokenSpaceGuid.PcdSpiMemoryMapped
> > > > > +  gMarvellTokenSpaceGuid.PcdSpiVariableOffset
> > > > > +
> > > > > +[Pcd]
> > > > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
> > > > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64
> > > > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64
> > > > >
> > > > >  [Depex]
> > > > > -  gEfiCpuArchProtocolGuid
> > > > > +  #
> > > > > +  # Generic FaultTolerantWriteDxe driver use variables,
> > > > > +  # whose setting is done in MvFvbDxe driver in case
> > > > > +  # the SPI contents are not mapped in memory.
> > > > > +  #
> > > > > +  BEFORE gFaultTolerantWriteDxeFileGuid
> > > >
> > > > Apologies for not spotting this before, but there is a problem here:
> > > > FaultTolerantWriteDxe.inf does not depend on gEfiCpuArchProtocolGuid,
> > > > but we do, and so we could now be dispatched before
> > > > gEfiCpuArchProtocolGuid becomes available. This means you need to
> > > > update the code to deal with that (or explain to me how if it already
> > > > does)
> > > >
> > >
> > > You should be able to fix this by adding a NULL resolution for
> > >
> > > EmbeddedPkg/Library/NvVarStoreFormattedLib/NvVarStoreFormattedLib.inf
> > >
> > > to the FTW driver, similar to what you are already doing for the
> > > variable driver.
> > >
> >
> > Thanks for the hint, I'll try that.
> >
> > I tried to modify dependencies within edk2-platforms to explicitly
> > cover FaultTolerantWriteDxe.inf and gEfiCpuArchProtocolGuid, but did
> > not succeed.
> >
>
> You cannot combine BEFORE/AFTER depexes with boolean depex
> expressions. In general, BEFORE/AFTER should be avoided since it
> defeats the purpose of protocol dependencies.
>

I know that. But since I have chain of dependencies between MvFvbDxe /
MvSpiFlashDxe / MvSpiOrionDxe, I wanted to blend
gEfiCpuArchProtocolGuid somewhere.

> > With this patch the things work properly, but I am wondering if only
> > by luck. I found following line in
> > BaseTools/Source/Python/UPT/Xml/XmlParser.py:
> > DxeObj.SetDepex("gEfiBdsArchProtocolGuid AND \ngEfiCpuArchProtocolGuid
> > AND\n" + \
> > it seems not to apply for MvFvbDxe though. So simply it may be an
> > order of entries in Armada7k8k.fdf file.
> >
>
> Most likely. The implicit depex is probably for UEFI_DRIVER modules
> not DXE_DRIVER