From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=l4ZB9OTq; spf=pass (domain: linaro.org, ip: 209.85.166.66, mailfrom: ard.biesheuvel@linaro.org) Received: from mail-io1-f66.google.com (mail-io1-f66.google.com [209.85.166.66]) by groups.io with SMTP; Wed, 17 Apr 2019 11:30:16 -0700 Received: by mail-io1-f66.google.com with SMTP id v4so21454062ioj.5 for ; Wed, 17 Apr 2019 11:30:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=JFeYmvJpocgmpKS6NQsrbbz25z8+hnxkjOU5+47vsTo=; b=l4ZB9OTqkROwgxc34lby5XVjsAcnXsaN6lD9lWLOwtUexUF+BaeI4UEYi98JSSt14N UEidiqMopjb3SVCv3y++j7/t+UPxFSg08qqLvfFOMTAb0KAnGGa5Dn/rE+jz7FqNn2Wr Xb60uRIMf+BAQHocuqYQFNFFdE0HvUUo4bNQX9g6JJ6SX/MWq2Zrhlrv8AlLDsMQClqb VV/mEYOVdZfxicBbZh2Xjwju6QWJ6ZugepTxJtSIh/SpdlI3oiIiEZ64hgLhsS61EZZw Way85IdadnDRKjl/uWPpt9oLeLNUYQ55wES/huDYIPuhWIceZD09SmD/NpaPfMFgtwDy QFMg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=JFeYmvJpocgmpKS6NQsrbbz25z8+hnxkjOU5+47vsTo=; b=slB8dJ0gdBuX/n3EYdnxzZmYE9qsiAr0KYqvjJA1KSmMUuxEGtuwPkSaooB7WpvvuX QdCzBf9WJ5d5+kd1O0dHeao/Nu3rY4q4kJ8vAP3FbcB1a/YMZvUGwYo1fS86WQIm8J8V C755n8vgzaYa1YIJBmMejnCgGM+ozBRGIYaqPDhdAjFciPXThl3mVDtgyA68CwEZeW/c pA3UOKIxYxfZ85K6c52oVlSDoF0evU82C5cxzeUGrNZ4gypd7rF7HiFNU7xRLpkbAczq CTx66vOxr6cQqynZAilXWN0ftgO4Fj5lYX3beMskl+eor8Yr8xPCghVZdWm7u8K43tvS AWdw== X-Gm-Message-State: APjAAAWfO5Gu0dlK4En2n9Lr20L0IjtUxGwRPqwvk89htr4XpNHEIHW0 HYq53q0oX4L7guYJvfzfZj0zKzVfcA+NOcVm1p92rA== X-Google-Smtp-Source: APXvYqzwmEmiDFKbLYU2QxzkQzWoilozaX219mCyzdVJC+vRBbg+wbeGLUCt5EVmF3o+Hp9GCSOSyNJDChcuPPqEj/M= X-Received: by 2002:a5d:8b41:: with SMTP id c1mr23671127iot.173.1555525815198; Wed, 17 Apr 2019 11:30:15 -0700 (PDT) MIME-Version: 1.0 References: <1555524356-1740-1-git-send-email-mw@semihalf.com> <1555524356-1740-3-git-send-email-mw@semihalf.com> In-Reply-To: From: "Ard Biesheuvel" Date: Wed, 17 Apr 2019 11:30:03 -0700 Message-ID: Subject: Re: [edk2-platforms: PATCH 2/3] Marvell/Drivers: MvSpiOrionDxe: Keep clock enabled at runtime To: Marcin Wojtas Cc: edk2-devel-groups-io , Leif Lindholm , Nadav Haklai , =?UTF-8?B?SmFuIETEhWJyb8Wb?= , Grzegorz Jaszczyk , Kostya Porotchkin , Jici Gao Content-Type: text/plain; charset="UTF-8" On Wed, 17 Apr 2019 at 11:27, Ard Biesheuvel wrote: > > On Wed, 17 Apr 2019 at 11:06, Marcin Wojtas 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 > > 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 > >