This patch applies necessary modifications, which allow to use
MvSpiDxe driver in variable support as a runtime service.
Its type is modified to DXE_RUNTIME_DRIVER, as well as
a new callback is introduced as a part of the SpiMasterProtocol.
It is supposed to configure the memory space for mmio access to
the host controller registers. Moreover gBS locking usage in
MvSpiTransfer is limited to the firmware, as the runtime access
to the flash is protected within the OS.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Drivers/Spi/MvSpiDxe.c | 50 ++++++++++++++++++--
Platform/Marvell/Drivers/Spi/MvSpiDxe.h | 2 +
Platform/Marvell/Drivers/Spi/MvSpiDxe.inf | 4 +-
Platform/Marvell/Include/Protocol/Spi.h | 7 +++
4 files changed, 58 insertions(+), 5 deletions(-)
diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
index c60a520..bab6cf4 100755
--- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
+++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
@@ -211,7 +211,9 @@ MvSpiTransfer (
Length = 8 * DataByteCount;
- EfiAcquireLock (&SpiMaster->Lock);
+ if (!EfiAtRuntime ()) {
+ EfiAcquireLock (&SpiMaster->Lock);
+ }
if (Flag & SPI_TRANSFER_BEGIN) {
SpiActivateCs (Slave);
@@ -254,7 +256,9 @@ MvSpiTransfer (
SpiDeactivateCs (Slave);
}
- EfiReleaseLock (&SpiMaster->Lock);
+ if (!EfiAtRuntime ()) {
+ EfiReleaseLock (&SpiMaster->Lock);
+ }
return EFI_SUCCESS;
}
@@ -338,6 +342,44 @@ MvSpiFreeSlave (
return EFI_SUCCESS;
}
+EFI_STATUS
+EFIAPI
+MvSpiConfigRuntime (
+ IN SPI_DEVICE *Slave
+ )
+{
+ EFI_STATUS Status;
+ UINTN AlignedAddress;
+
+ //
+ // Host register base may be not aligned to the page size,
+ // which is not accepted when setting memory space attributes.
+ // Add one aligned page of memory space which covers the host
+ // controller registers.
+ //
+ AlignedAddress = Slave->HostRegisterBaseAddress & ~(SIZE_4KB - 1);
+
+ Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo,
+ AlignedAddress,
+ SIZE_4KB,
+ EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: Failed to add memory space\n", __FUNCTION__));
+ return Status;
+ }
+
+ Status = gDS->SetMemorySpaceAttributes (AlignedAddress,
+ SIZE_4KB,
+ 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;
+ }
+
+ return EFI_SUCCESS;
+}
+
STATIC
EFI_STATUS
SpiMasterInitProtocol (
@@ -350,6 +392,7 @@ SpiMasterInitProtocol (
SpiMasterProtocol->FreeDevice = MvSpiFreeSlave;
SpiMasterProtocol->Transfer = MvSpiTransfer;
SpiMasterProtocol->ReadWrite = MvSpiReadWrite;
+ SpiMasterProtocol->ConfigRuntime = MvSpiConfigRuntime;
return EFI_SUCCESS;
}
@@ -363,8 +406,7 @@ SpiMasterEntryPoint (
{
EFI_STATUS Status;
- mSpiMasterInstance = AllocateZeroPool (sizeof (SPI_MASTER));
-
+ mSpiMasterInstance = AllocateRuntimeZeroPool (sizeof (SPI_MASTER));
if (mSpiMasterInstance == NULL) {
return EFI_OUT_OF_RESOURCES;
}
diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
index e7e280a..50cdc02 100644
--- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
+++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
@@ -38,10 +38,12 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <Library/PcdLib.h>
#include <Library/UefiLib.h>
#include <Library/DebugLib.h>
+#include <Library/DxeServicesTableLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Uefi/UefiBaseType.h>
#include <Library/BaseMemoryLib.h>
#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiRuntimeLib.h>
#include <Protocol/Spi.h>
diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf b/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
index 08c6c04..9fe246f 100644
--- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
+++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
@@ -33,7 +33,7 @@
INF_VERSION = 0x00010005
BASE_NAME = SpiMasterDxe
FILE_GUID = c19dbc8a-f4f9-43b0-aee5-802e3ed03d15
- MODULE_TYPE = DXE_DRIVER
+ MODULE_TYPE = DXE_RUNTIME_DRIVER
VERSION_STRING = 1.0
ENTRY_POINT = SpiMasterEntryPoint
@@ -53,8 +53,10 @@
TimerLib
UefiLib
DebugLib
+ DxeServicesTableLib
MemoryAllocationLib
IoLib
+ UefiRuntimeLib
[FixedPcd]
gMarvellTokenSpaceGuid.PcdSpiRegBase
diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
index d993021..abbad19 100644
--- a/Platform/Marvell/Include/Protocol/Spi.h
+++ b/Platform/Marvell/Include/Protocol/Spi.h
@@ -101,12 +101,19 @@ EFI_STATUS
IN SPI_DEVICE *SpiDev
);
+typedef
+EFI_STATUS
+(EFIAPI *MV_SPI_CONFIG_RT) (
+ IN SPI_DEVICE *SpiDev
+ );
+
struct _MARVELL_SPI_MASTER_PROTOCOL {
MV_SPI_INIT Init;
MV_SPI_READ_WRITE ReadWrite;
MV_SPI_TRANSFER Transfer;
MV_SPI_SETUP_DEVICE SetupDevice;
MV_SPI_FREE_DEVICE FreeDevice;
+ MV_SPI_CONFIG_RT ConfigRuntime;
};
#endif // __MARVELL_SPI_MASTER_PROTOCOL_H__
--
2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On Sun, Nov 05, 2017 at 11:55:37AM +0100, Marcin Wojtas wrote:
> This patch applies necessary modifications, which allow to use
> MvSpiDxe driver in variable support as a runtime service.
> Its type is modified to DXE_RUNTIME_DRIVER, as well as
> a new callback is introduced as a part of the SpiMasterProtocol.
> It is supposed to configure the memory space for mmio access to
> the host controller registers. Moreover gBS locking usage in
> MvSpiTransfer is limited to the firmware, as the runtime access
> to the flash is protected within the OS.
Break the commit message up a bit:
---
This patch applies necessary modifications, which allow to use
MvSpiDxe driver in variable support as a runtime service.
Its type is modified to DXE_RUNTIME_DRIVER, as well as
a new callback is introduced as a part of the SpiMasterProtocol.
---
And then this needs rewording
---
It is supposed to configure the memory space for mmio access to
the host controller registers.
---
(Say what it does, not what it should be doing.)
---
Moreover gBS locking usage in MvSpiTransfer is limited to the
firmware, as the runtime access to the flash is protected within the
OS.
---
And "is limited to the firmware". Just because it is used at runtime
does not make it not firmware. I would say something like:
"Apply locking in the driver only during boot services. Once at
runtime, resource protection is handled by the operating system.".
Also, "Its" -> "The driver's" and "It" -> "The driver".
Other than that, can you move the Depex addition here from 4/4 please?
/
Leif
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
> Platform/Marvell/Drivers/Spi/MvSpiDxe.c | 50 ++++++++++++++++++--
> Platform/Marvell/Drivers/Spi/MvSpiDxe.h | 2 +
> Platform/Marvell/Drivers/Spi/MvSpiDxe.inf | 4 +-
> Platform/Marvell/Include/Protocol/Spi.h | 7 +++
> 4 files changed, 58 insertions(+), 5 deletions(-)
>
> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> index c60a520..bab6cf4 100755
> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> @@ -211,7 +211,9 @@ MvSpiTransfer (
>
> Length = 8 * DataByteCount;
>
> - EfiAcquireLock (&SpiMaster->Lock);
> + if (!EfiAtRuntime ()) {
> + EfiAcquireLock (&SpiMaster->Lock);
> + }
>
> if (Flag & SPI_TRANSFER_BEGIN) {
> SpiActivateCs (Slave);
> @@ -254,7 +256,9 @@ MvSpiTransfer (
> SpiDeactivateCs (Slave);
> }
>
> - EfiReleaseLock (&SpiMaster->Lock);
> + if (!EfiAtRuntime ()) {
> + EfiReleaseLock (&SpiMaster->Lock);
> + }
>
> return EFI_SUCCESS;
> }
> @@ -338,6 +342,44 @@ MvSpiFreeSlave (
> return EFI_SUCCESS;
> }
>
> +EFI_STATUS
> +EFIAPI
> +MvSpiConfigRuntime (
> + IN SPI_DEVICE *Slave
> + )
> +{
> + EFI_STATUS Status;
> + UINTN AlignedAddress;
> +
> + //
> + // Host register base may be not aligned to the page size,
> + // which is not accepted when setting memory space attributes.
> + // Add one aligned page of memory space which covers the host
> + // controller registers.
> + //
> + AlignedAddress = Slave->HostRegisterBaseAddress & ~(SIZE_4KB - 1);
> +
> + Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo,
> + AlignedAddress,
> + SIZE_4KB,
> + EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "%a: Failed to add memory space\n", __FUNCTION__));
> + return Status;
> + }
> +
> + Status = gDS->SetMemorySpaceAttributes (AlignedAddress,
> + SIZE_4KB,
> + 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;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> STATIC
> EFI_STATUS
> SpiMasterInitProtocol (
> @@ -350,6 +392,7 @@ SpiMasterInitProtocol (
> SpiMasterProtocol->FreeDevice = MvSpiFreeSlave;
> SpiMasterProtocol->Transfer = MvSpiTransfer;
> SpiMasterProtocol->ReadWrite = MvSpiReadWrite;
> + SpiMasterProtocol->ConfigRuntime = MvSpiConfigRuntime;
>
> return EFI_SUCCESS;
> }
> @@ -363,8 +406,7 @@ SpiMasterEntryPoint (
> {
> EFI_STATUS Status;
>
> - mSpiMasterInstance = AllocateZeroPool (sizeof (SPI_MASTER));
> -
> + mSpiMasterInstance = AllocateRuntimeZeroPool (sizeof (SPI_MASTER));
> if (mSpiMasterInstance == NULL) {
> return EFI_OUT_OF_RESOURCES;
> }
> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> index e7e280a..50cdc02 100644
> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> @@ -38,10 +38,12 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> #include <Library/PcdLib.h>
> #include <Library/UefiLib.h>
> #include <Library/DebugLib.h>
> +#include <Library/DxeServicesTableLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Uefi/UefiBaseType.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiRuntimeLib.h>
>
> #include <Protocol/Spi.h>
>
> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf b/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
> index 08c6c04..9fe246f 100644
> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
> @@ -33,7 +33,7 @@
> INF_VERSION = 0x00010005
> BASE_NAME = SpiMasterDxe
> FILE_GUID = c19dbc8a-f4f9-43b0-aee5-802e3ed03d15
> - MODULE_TYPE = DXE_DRIVER
> + MODULE_TYPE = DXE_RUNTIME_DRIVER
> VERSION_STRING = 1.0
> ENTRY_POINT = SpiMasterEntryPoint
>
> @@ -53,8 +53,10 @@
> TimerLib
> UefiLib
> DebugLib
> + DxeServicesTableLib
> MemoryAllocationLib
> IoLib
> + UefiRuntimeLib
>
> [FixedPcd]
> gMarvellTokenSpaceGuid.PcdSpiRegBase
> diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
> index d993021..abbad19 100644
> --- a/Platform/Marvell/Include/Protocol/Spi.h
> +++ b/Platform/Marvell/Include/Protocol/Spi.h
> @@ -101,12 +101,19 @@ EFI_STATUS
> IN SPI_DEVICE *SpiDev
> );
>
> +typedef
> +EFI_STATUS
> +(EFIAPI *MV_SPI_CONFIG_RT) (
> + IN SPI_DEVICE *SpiDev
> + );
> +
> struct _MARVELL_SPI_MASTER_PROTOCOL {
> MV_SPI_INIT Init;
> MV_SPI_READ_WRITE ReadWrite;
> MV_SPI_TRANSFER Transfer;
> MV_SPI_SETUP_DEVICE SetupDevice;
> MV_SPI_FREE_DEVICE FreeDevice;
> + MV_SPI_CONFIG_RT ConfigRuntime;
> };
>
> #endif // __MARVELL_SPI_MASTER_PROTOCOL_H__
> --
> 2.7.4
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif 2017-11-09 14:44 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>: > On Sun, Nov 05, 2017 at 11:55:37AM +0100, Marcin Wojtas wrote: >> This patch applies necessary modifications, which allow to use >> MvSpiDxe driver in variable support as a runtime service. >> Its type is modified to DXE_RUNTIME_DRIVER, as well as >> a new callback is introduced as a part of the SpiMasterProtocol. >> It is supposed to configure the memory space for mmio access to >> the host controller registers. Moreover gBS locking usage in >> MvSpiTransfer is limited to the firmware, as the runtime access >> to the flash is protected within the OS. > > Break the commit message up a bit: > --- > This patch applies necessary modifications, which allow to use > MvSpiDxe driver in variable support as a runtime service. > > Its type is modified to DXE_RUNTIME_DRIVER, as well as > a new callback is introduced as a part of the SpiMasterProtocol. > --- > > And then this needs rewording > --- > It is supposed to configure the memory space for mmio access to > the host controller registers. > --- > (Say what it does, not what it should be doing.) > > --- > Moreover gBS locking usage in MvSpiTransfer is limited to the > firmware, as the runtime access to the flash is protected within the > OS. > --- > And "is limited to the firmware". Just because it is used at runtime > does not make it not firmware. I would say something like: > "Apply locking in the driver only during boot services. Once at > runtime, resource protection is handled by the operating system.". > > Also, "Its" -> "The driver's" and "It" -> "The driver". > > Other than that, can you move the Depex addition here from 4/4 please? > > / Sure. Thanks, Marcin _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.