On Wed, 17 Apr 2019 at 11:27, Ard Biesheuvel <[email protected]> wrote:
>
> On Wed, 17 Apr 2019 at 11:06, Marcin Wojtas <[email protected]> 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 <[email protected]>
>
> 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
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#39266): https://edk2.groups.io/g/devel/message/39266
Mute This Topic: https://groups.io/mt/31215005/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
