Re: [edk2-platforms: PATCH 2/3] Marvell/Drivers: MvSpiOrionDxe: Keep clock enabled at runtime

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

On Wed, 17 Apr 2019 at 11:27, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Wed, 17 Apr 2019 at 11:06, Marcin Wojtas <mw@semihalf.com> wrote:
> >
> > Linux MTD subsystem enables the SPI controller clock
> > only when explicitly accessing the device. Because of that,
> > using variables stored in the SPI flash could cause a hang
> > when this clock remained disabled.
> >
> > Such issue was prevented when booting with the default
> > DTB - see commit 124073e4d440 ("Marvell/Armada7k8k: DeviceTree:
> > Use fixed clocks"). However in case someone wishes to
> > try a different DTB, the issue can become problematic again.
> >
> > Fix above problem by enabling the clock when accessing
> > SPI controller registers or mapped memory region
> > AtRuntime. For this purpose add required callbacks
> > to the SpiMaster and SpiFlash protocols that are
> > called from MvFvbDxe driver.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>
> This is really not the right solution. Linux runs UEFI runtime
> services on a single core with interrupts enabled, and it thinks it
> owns the SPI subsystem if you describe it in the DT, and may decide to
> access it at the same time, either in interrupt ot softirq context, or
> from another core.
>

Note that this applies to just the clock as well.

>
> > ---
> >  Silicon/Marvell/Marvell.dec                                 |  3 +
> >  Silicon/Marvell/Drivers/Spi/MvSpiOrionDxe/MvSpiOrionDxe.inf |  3 +
> >  Silicon/Marvell/Include/Protocol/Spi.h                      | 10 ++++
> >  Silicon/Marvell/Include/Protocol/SpiFlash.h                 |  7 +++
> >  Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c             | 34 +++++++++++
> >  Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c   | 11 ++++
> >  Silicon/Marvell/Drivers/Spi/MvSpiOrionDxe/MvSpiOrionDxe.c   | 63 +++++++++++++++++++-
> >  7 files changed, 129 insertions(+), 2 deletions(-)
> >
> > diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec
> > index 7210ba2..201a62f 100644
> > --- a/Silicon/Marvell/Marvell.dec
> > +++ b/Silicon/Marvell/Marvell.dec
> > @@ -138,6 +138,9 @@
> >    gMarvellTokenSpaceGuid.PcdI2cBusCount|0|UINT32|0x3000183
> >
> >  #SPI
> > +  gMarvellTokenSpaceGuid.PcdSpiClockFixed|TRUE|BOOLEAN|0x30000054
> > +  gMarvellTokenSpaceGuid.PcdSpiClockMask|0|UINT32|0x30000055
> > +  gMarvellTokenSpaceGuid.PcdSpiClockRegBase|0|UINT64|0x30000056
> >    gMarvellTokenSpaceGuid.PcdSpiRegBase|0|UINT32|0x3000051
> >    gMarvellTokenSpaceGuid.PcdSpiMemoryBase|0|UINT64|0x3000059
> >    gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|0|UINT32|0x30000052
> > diff --git a/Silicon/Marvell/Drivers/Spi/MvSpiOrionDxe/MvSpiOrionDxe.inf b/Silicon/Marvell/Drivers/Spi/MvSpiOrionDxe/MvSpiOrionDxe.inf
> > index 4779371..1857484 100644
> > --- a/Silicon/Marvell/Drivers/Spi/MvSpiOrionDxe/MvSpiOrionDxe.inf
> > +++ b/Silicon/Marvell/Drivers/Spi/MvSpiOrionDxe/MvSpiOrionDxe.inf
> > @@ -59,7 +59,10 @@
> >    UefiRuntimeLib
> >
> >  [FixedPcd]
> > +  gMarvellTokenSpaceGuid.PcdSpiClockFixed
> >    gMarvellTokenSpaceGuid.PcdSpiClockFrequency
> > +  gMarvellTokenSpaceGuid.PcdSpiClockMask
> > +  gMarvellTokenSpaceGuid.PcdSpiClockRegBase
> >    gMarvellTokenSpaceGuid.PcdSpiMaxFrequency
> >    gMarvellTokenSpaceGuid.PcdSpiRegBase
> >
> > diff --git a/Silicon/Marvell/Include/Protocol/Spi.h b/Silicon/Marvell/Include/Protocol/Spi.h
> > index abbad19..4d66f7f 100644
> > --- a/Silicon/Marvell/Include/Protocol/Spi.h
> > +++ b/Silicon/Marvell/Include/Protocol/Spi.h
> > @@ -54,6 +54,9 @@ typedef struct {
> >    UINT32 AddrSize;
> >    NOR_FLASH_INFO *Info;
> >    UINTN HostRegisterBaseAddress;
> > +  BOOLEAN HostClockFixed;
> > +  UINTN HostClockEnableRegister;
> > +  UINT32 HostClockEnableMask;
> >    UINTN CoreClock;
> >  } SPI_DEVICE;
> >
> > @@ -107,6 +110,12 @@ EFI_STATUS
> >    IN SPI_DEVICE *SpiDev
> >    );
> >
> > +typedef
> > +EFI_STATUS
> > +(EFIAPI *MV_SPI_POWER_ON) (
> > +  IN SPI_DEVICE *SpiDev
> > +  );
> > +
> >  struct _MARVELL_SPI_MASTER_PROTOCOL {
> >    MV_SPI_INIT         Init;
> >    MV_SPI_READ_WRITE   ReadWrite;
> > @@ -114,6 +123,7 @@ struct _MARVELL_SPI_MASTER_PROTOCOL {
> >    MV_SPI_SETUP_DEVICE SetupDevice;
> >    MV_SPI_FREE_DEVICE  FreeDevice;
> >    MV_SPI_CONFIG_RT    ConfigRuntime;
> > +  MV_SPI_POWER_ON     ControllerPowerOn;
> >  };
> >
> >  #endif // __MARVELL_SPI_MASTER_PROTOCOL_H__
> > diff --git a/Silicon/Marvell/Include/Protocol/SpiFlash.h b/Silicon/Marvell/Include/Protocol/SpiFlash.h
> > index e703330..0336dda 100644
> > --- a/Silicon/Marvell/Include/Protocol/SpiFlash.h
> > +++ b/Silicon/Marvell/Include/Protocol/SpiFlash.h
> > @@ -102,6 +102,12 @@ EFI_STATUS
> >    IN UINTN                                          EndPercentage
> >    );
> >
> > +typedef
> > +EFI_STATUS
> > +(EFIAPI *MV_SPI_FLASH_POWER_ON) (
> > +  IN SPI_DEVICE *SpiDev
> > +  );
> > +
> >  struct _MARVELL_SPI_FLASH_PROTOCOL {
> >    MV_SPI_FLASH_INIT    Init;
> >    MV_SPI_FLASH_READ_ID ReadId;
> > @@ -110,6 +116,7 @@ struct _MARVELL_SPI_FLASH_PROTOCOL {
> >    MV_SPI_FLASH_ERASE   Erase;
> >    MV_SPI_FLASH_UPDATE  Update;
> >    MV_SPI_FLASH_UPDATE_WITH_PROGRESS  UpdateWithProgress;
> > +  MV_SPI_FLASH_POWER_ON FlashPowerOn;
> >  };
> >
> >  #endif // __MV_SPI_FLASH__
> > diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
> > index cb006cd..b57f415 100644
> > --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
> > +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
> > @@ -311,6 +311,11 @@ MvFvbGetAttributes (
> >
> >    FlashInstance = INSTANCE_FROM_FVB_THIS (This);
> >
> > +  // Ensure proper access to the flash device from OS level
> > +  if (EfiAtRuntime ()) {
> > +    FlashInstance->SpiFlashProtocol->FlashPowerOn (&FlashInstance->SpiDevice);
> > +  }
> > +
> >    FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)FlashInstance->RegionBaseAddress;
> >    FlashFvbAttributes = (EFI_FVB_ATTRIBUTES_2 *)&(FwVolHeader->Attributes);
> >
> > @@ -349,10 +354,18 @@ MvFvbSetAttributes (
> >    EFI_FVB_ATTRIBUTES_2  OldAttributes;
> >    EFI_FVB_ATTRIBUTES_2  FlashFvbAttributes;
> >    EFI_FVB_ATTRIBUTES_2  UnchangedAttributes;
> > +  FVB_DEVICE            *FlashInstance;
> >    UINT32                Capabilities;
> >    UINT32                OldStatus;
> >    UINT32                NewStatus;
> >
> > +  FlashInstance = INSTANCE_FROM_FVB_THIS (This);
> > +
> > +  // Ensure proper access to the flash device from OS level
> > +  if (EfiAtRuntime ()) {
> > +    FlashInstance->SpiFlashProtocol->FlashPowerOn (&FlashInstance->SpiDevice);
> > +  }
> > +
> >    //
> >    // Obtain attributes from FVB header
> >    //
> > @@ -468,6 +481,11 @@ MvFvbGetPhysicalAddress (
> >
> >    FlashInstance = INSTANCE_FROM_FVB_THIS (This);
> >
> > +  // Ensure proper access to the flash device from OS level
> > +  if (EfiAtRuntime ()) {
> > +    FlashInstance->SpiFlashProtocol->FlashPowerOn (&FlashInstance->SpiDevice);
> > +  }
> > +
> >    *Address = FlashInstance->RegionBaseAddress;
> >
> >    return EFI_SUCCESS;
> > @@ -588,6 +606,10 @@ MvFvbRead (
> >
> >    FlashInstance = INSTANCE_FROM_FVB_THIS (This);
> >
> > +  // Ensure proper access to the flash device from OS level
> > +  if (EfiAtRuntime ()) {
> > +    FlashInstance->SpiFlashProtocol->FlashPowerOn (&FlashInstance->SpiDevice);
> > +  }
> >
> >    // Cache the block size to avoid de-referencing pointers all the time
> >    BlockSize = FlashInstance->Media.BlockSize;
> > @@ -697,6 +719,11 @@ MvFvbWrite (
> >
> >    FlashInstance = INSTANCE_FROM_FVB_THIS (This);
> >
> > +  // Ensure proper access to the flash device from OS level
> > +  if (EfiAtRuntime ()) {
> > +    FlashInstance->SpiFlashProtocol->FlashPowerOn (&FlashInstance->SpiDevice);
> > +  }
> > +
> >    DataOffset = GET_DATA_OFFSET (FlashInstance->FvbOffset + Offset,
> >                   FlashInstance->StartLba + Lba,
> >                   FlashInstance->Media.BlockSize);
> > @@ -770,6 +797,11 @@ MvFvbEraseBlocks (
> >
> >    FlashInstance = INSTANCE_FROM_FVB_THIS (This);
> >
> > +  // Ensure proper access to the flash device from OS level
> > +  if (EfiAtRuntime ()) {
> > +    FlashInstance->SpiFlashProtocol->FlashPowerOn (&FlashInstance->SpiDevice);
> > +  }
> > +
> >    Status = EFI_SUCCESS;
> >
> >    // Detect WriteDisabled state
> > @@ -882,11 +914,13 @@ MvFvbVirtualNotifyEvent (
> >    // Convert SPI device description
> >    EfiConvertPointer (0x0, (VOID**)&mFvbDevice->SpiDevice.Info);
> >    EfiConvertPointer (0x0, (VOID**)&mFvbDevice->SpiDevice.HostRegisterBaseAddress);
> > +  EfiConvertPointer (0x0, (VOID**)&mFvbDevice->SpiDevice.HostClockEnableRegister);
> >    EfiConvertPointer (0x0, (VOID**)&mFvbDevice->SpiDevice);
> >
> >    // Convert SpiFlashProtocol
> >    EfiConvertPointer (0x0, (VOID**)&mFvbDevice->SpiFlashProtocol->Erase);
> >    EfiConvertPointer (0x0, (VOID**)&mFvbDevice->SpiFlashProtocol->Write);
> > +  EfiConvertPointer (0x0, (VOID**)&mFvbDevice->SpiFlashProtocol->FlashPowerOn);
> >    EfiConvertPointer (0x0, (VOID**)&mFvbDevice->SpiFlashProtocol);
> >
> >    return;
> > diff --git a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
> > index d81f6e3..4cc897f 100755
> > --- a/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
> > +++ b/Silicon/Marvell/Drivers/Spi/MvSpiFlashDxe/MvSpiFlashDxe.c
> > @@ -548,6 +548,15 @@ MvSpiFlashInit (
> >  }
> >
> >  EFI_STATUS
> > +EFIAPI
> > +MvSpiFlashPowerOn (
> > +  IN SPI_DEVICE *Slave
> > +  )
> > +{
> > +  return SpiMasterProtocol->ControllerPowerOn (Slave);
> > +}
> > +
> > +EFI_STATUS
> >  MvSpiFlashInitProtocol (
> >    IN MARVELL_SPI_FLASH_PROTOCOL *SpiFlashProtocol
> >    )
> > @@ -560,6 +569,7 @@ MvSpiFlashInitProtocol (
> >    SpiFlashProtocol->Erase = MvSpiFlashErase;
> >    SpiFlashProtocol->Update = MvSpiFlashUpdate;
> >    SpiFlashProtocol->UpdateWithProgress = MvSpiFlashUpdateWithProgress;
> > +  SpiFlashProtocol->FlashPowerOn = MvSpiFlashPowerOn;
> >
> >    return EFI_SUCCESS;
> >  }
> > @@ -586,6 +596,7 @@ MvSpiFlashVirtualNotifyEvent (
> >    //
> >    EfiConvertPointer (0x0, (VOID**)&SpiMasterProtocol->ReadWrite);
> >    EfiConvertPointer (0x0, (VOID**)&SpiMasterProtocol->Transfer);
> > +  EfiConvertPointer (0x0, (VOID**)&SpiMasterProtocol->ControllerPowerOn);
> >    EfiConvertPointer (0x0, (VOID**)&SpiMasterProtocol);
> >
> >    return;
> > diff --git a/Silicon/Marvell/Drivers/Spi/MvSpiOrionDxe/MvSpiOrionDxe.c b/Silicon/Marvell/Drivers/Spi/MvSpiOrionDxe/MvSpiOrionDxe.c
> > index cbf403f..3f7f67e 100755
> > --- a/Silicon/Marvell/Drivers/Spi/MvSpiOrionDxe/MvSpiOrionDxe.c
> > +++ b/Silicon/Marvell/Drivers/Spi/MvSpiOrionDxe/MvSpiOrionDxe.c
> > @@ -323,6 +323,11 @@ MvSpiSetupSlave (
> >    }
> >
> >    Slave->HostRegisterBaseAddress = PcdGet32 (PcdSpiRegBase);
> > +  Slave->HostClockFixed = PcdGetBool (PcdSpiClockFixed);
> > +  if (!Slave->HostClockFixed) {
> > +    Slave->HostClockEnableRegister = PcdGet64 (PcdSpiClockRegBase);
> > +    Slave->HostClockEnableMask = PcdGet32 (PcdSpiClockMask);
> > +  }
> >    Slave->CoreClock = PcdGet32 (PcdSpiClockFrequency);
> >    Slave->MaxFreq = PcdGet32 (PcdSpiMaxFrequency);
> >
> > @@ -344,12 +349,26 @@ MvSpiFreeSlave (
> >
> >  EFI_STATUS
> >  EFIAPI
> > +MvSpiControllerPowerOn (
> > +  IN SPI_DEVICE *Slave
> > +  )
> > +{
> > +  if (!Slave->HostClockFixed) {
> > +    MmioOr32 (Slave->HostClockEnableRegister, Slave->HostClockEnableMask);
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +EFI_STATUS
> > +EFIAPI
> >  MvSpiConfigRuntime (
> >    IN SPI_DEVICE *Slave
> >    )
> >  {
> >    EFI_STATUS Status;
> >    UINTN AlignedAddress;
> > +  UINTN ClockEnableAlignedAddress;
> >
> >    //
> >    // Host register base may be not aligned to the page size,
> > @@ -373,11 +392,50 @@ MvSpiConfigRuntime (
> >                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((DEBUG_ERROR, "%a: Failed to set memory attributes\n", __FUNCTION__));
> > -    gDS->RemoveMemorySpace (AlignedAddress, SIZE_4KB);
> > -    return Status;
> > +    goto ErrorSetHostMemorySpace;
> > +  }
> > +
> > +  //
> > +  // In case a clock feeding the SPI interface is always on, skip its
> > +  // configuration for Runtime.
> > +  //
> > +  if (Slave->HostClockFixed) {
> > +    return EFI_SUCCESS;
> > +  }
> > +
> > +  //
> > +  // Make sure about an alignment of the memory space needed for clock enabling.
> > +  // Add one aligned page of memory space which covers the clock enabling
> > +  // register.
> > +  //
> > +  ClockEnableAlignedAddress = Slave->HostClockEnableRegister & ~(SIZE_4KB - 1);
> > +
> > +  Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo,
> > +                  ClockEnableAlignedAddress,
> > +                  SIZE_4KB,
> > +                  EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "%a: Failed to add memory space\n", __FUNCTION__));
> > +    goto ErrorSetHostMemorySpace;
> > +  }
> > +
> > +  Status = gDS->SetMemorySpaceAttributes (ClockEnableAlignedAddress,
> > +                  SIZE_4KB,
> > +                  EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "%a: Failed to set memory attributes\n", __FUNCTION__));
> > +    goto ErrorSetClockMemorySpace;
> >    }
> >
> >    return EFI_SUCCESS;
> > +
> > +ErrorSetClockMemorySpace:
> > +  gDS->RemoveMemorySpace (ClockEnableAlignedAddress, SIZE_4KB);
> > +
> > +ErrorSetHostMemorySpace:
> > +  gDS->RemoveMemorySpace (AlignedAddress, SIZE_4KB);
> > +
> > +  return Status;
> >  }
> >
> >  STATIC
> > @@ -393,6 +451,7 @@ SpiMasterInitProtocol (
> >    SpiMasterProtocol->Transfer    = MvSpiTransfer;
> >    SpiMasterProtocol->ReadWrite   = MvSpiReadWrite;
> >    SpiMasterProtocol->ConfigRuntime = MvSpiConfigRuntime;
> > +  SpiMasterProtocol->ControllerPowerOn = MvSpiControllerPowerOn;
> >
> >    return EFI_SUCCESS;
> >  }
> > --
> > 2.7.4
> >