[platforms PATCH 0/2] Armada7k8k adjustments to EDK2

[platforms PATCH 0/2] Armada7k8k adjustments to EDK2

[Marcin Wojtas]

Hi,

Recent changes in EDK2 mainline resulted in problems with
compiling/booting of Armada platforms. This short patchset
adjust RTC library and variable driver to those modifications.

More details can be found in the commit log. The code is also
available in the github:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/adjustments-r20180416

I'm looking forward to your feedback.

Best regards,
Marcin


Marcin Wojtas (2):
  Marvell/Drivers: MvFvbDxe: Adjust to new dependencies
  Marvell/Armada: RealTimeClockLib: Depend on gEfiCpuArchProtocolGuid

 Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf |  5 ++++-
 Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c                          | 21 ++++++++++++++++++++
 Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf                        |  7 ++-----
 3 files changed, 27 insertions(+), 6 deletions(-)

-- 
2.7.4

[platforms PATCH 1/2] Marvell/Drivers: MvFvbDxe: Adjust to new dependencies

[Marcin Wojtas]

Recent changes in the EDK2 mainline resulted in breaking
of compilation and booting of Armada platforms.
This patch adjust the MvFvbDxe driver by:

 * installation of gEdkiiNvVarStoreFormattedGuid in order to signal
   NvVarStoreFormattedLib to the generic variable runtime driver

 * making explicit dependency to ArmPkg/Drivers/CpuDxe drivers in order
   to enable successful calling of gDS->SetMemorySpaceAttributes

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c   | 21 ++++++++++++++++++++
 Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf |  7 ++-----
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
index 252ef67..6e583a3 100644
--- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
+++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
@@ -26,6 +26,7 @@
 #include <Library/UefiLib.h>
 #include <Library/UefiRuntimeLib.h>
 
+#include <Guid/NvVarStoreFormatted.h>
 #include <Guid/SystemNvDataGuid.h>
 #include <Guid/VariableFormat.h>
 
@@ -1076,6 +1077,21 @@ MvFvbEntryPoint (
   }
 
   //
+  // The driver implementing the variable read service can now be dispatched;
+  // the varstore headers are in place.
+  //
+  Status = gBS->InstallProtocolInterface (&gImageHandle,
+                  &gEdkiiNvVarStoreFormattedGuid,
+                  EFI_NATIVE_INTERFACE,
+                  NULL);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR,
+      "%a: Failed to install gEdkiiNvVarStoreFormattedGuid\n",
+      __FUNCTION__));
+    goto ErrorInstallNvVarStoreFormatted;
+  }
+
+  //
   // Declare the Non-Volatile storage as EFI_MEMORY_RUNTIME
   //
   RuntimeMmioRegionSize = mFvbDevice->FvbSize;
@@ -1126,6 +1142,11 @@ ErrorSetMemAttr:
   gDS->RemoveMemorySpace (RegionBaseAddress, RuntimeMmioRegionSize);
 
 ErrorAddSpace:
+  gBS->UninstallProtocolInterface (&gImageHandle,
+                  &gEdkiiNvVarStoreFormattedGuid,
+                  NULL);
+
+ErrorInstallNvVarStoreFormatted:
   gBS->UninstallMultipleProtocolInterfaces (&mFvbDevice->Handle,
          &gEfiDevicePathProtocolGuid,
          &gEfiFirmwareVolumeBlockProtocolGuid,
diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
index 117fe8b..fd3f2f7 100644
--- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
+++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
@@ -63,6 +63,7 @@
   UefiRuntimeServicesTableLib
 
 [Guids]
+  gEdkiiNvVarStoreFormattedGuid
   gEfiAuthenticatedVariableGuid
   gEfiEventVirtualAddressChangeGuid
   gEfiSystemNvDataFvGuid
@@ -84,8 +85,4 @@
   gMarvellTokenSpaceGuid.PcdSpiMemoryBase
 
 [Depex]
-  #
-  # MvFvbDxe must be loaded before VariableRuntimeDxe in case empty
-  # flash needs populating with default values.
-  #
-  BEFORE gVariableRuntimeDxeFileGuid
+  gEfiCpuArchProtocolGuid
-- 
2.7.4

[platforms PATCH 2/2] Marvell/Armada: RealTimeClockLib: Depend on gEfiCpuArchProtocolGuid

[Marcin Wojtas]

Recent changes in the EDK2 mainline resulted in breaking
RTC functionality of Armada platforms.

The RealTimeClockLib instance calls gDS->SetMemorySpaceAttributes()
in the LibRtcInitialize() public function. This DXE service depends
on the CPU Arch Protocol. Add it to the depex.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf
index 2f842e8..59c71c4 100644
--- a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf
+++ b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf
@@ -25,7 +25,7 @@
   FILE_GUID                      = fa81e889-045b-4c96-9093-742554fd0588
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
-  LIBRARY_CLASS                  = RealTimeClockLib
+  LIBRARY_CLASS                  = RealTimeClockLib|DXE_RUNTIME_DRIVER
 
 [Sources.common]
   RealTimeClockLib.c
@@ -50,3 +50,6 @@
 
 [Pcd]
   gMarvellTokenSpaceGuid.PcdRtcEnabled
+
+[Depex.common.DXE_RUNTIME_DRIVER]
+  gEfiCpuArchProtocolGuid
-- 
2.7.4

Re: [platforms PATCH 1/2] Marvell/Drivers: MvFvbDxe: Adjust to new dependencies

[Ard Biesheuvel]

(+ Laszlo)

On 16 April 2018 at 07:09, Marcin Wojtas <mw@semihalf.com> wrote:
> Recent changes in the EDK2 mainline resulted in breaking
> of compilation and booting of Armada platforms.
> This patch adjust the MvFvbDxe driver by:
>
>  * installation of gEdkiiNvVarStoreFormattedGuid in order to signal
>    NvVarStoreFormattedLib to the generic variable runtime driver
>

Hello Marcin,

Installing this GUID is only necessary if you update your platform
.DSC to make the generic variable runtime driver depend on it by
adding a NULL library class resolution using NvVarStoreFormattedLib.
So I think this patch is correct, but you'll need an additional change
to make it work as expected. (Otherwise, the variable runtime driver
could still be dispatched early and invoked for read access before the
variable store is reformatted)

>  * making explicit dependency to ArmPkg/Drivers/CpuDxe drivers in order
>    to enable successful calling of gDS->SetMemorySpaceAttributes
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c   | 21 ++++++++++++++++++++
>  Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf |  7 ++-----
>  2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
> index 252ef67..6e583a3 100644
> --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
> +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
> @@ -26,6 +26,7 @@
>  #include <Library/UefiLib.h>
>  #include <Library/UefiRuntimeLib.h>
>
> +#include <Guid/NvVarStoreFormatted.h>
>  #include <Guid/SystemNvDataGuid.h>
>  #include <Guid/VariableFormat.h>
>
> @@ -1076,6 +1077,21 @@ MvFvbEntryPoint (
>    }
>
>    //
> +  // The driver implementing the variable read service can now be dispatched;
> +  // the varstore headers are in place.
> +  //
> +  Status = gBS->InstallProtocolInterface (&gImageHandle,
> +                  &gEdkiiNvVarStoreFormattedGuid,
> +                  EFI_NATIVE_INTERFACE,
> +                  NULL);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Failed to install gEdkiiNvVarStoreFormattedGuid\n",
> +      __FUNCTION__));
> +    goto ErrorInstallNvVarStoreFormatted;
> +  }
> +
> +  //
>    // Declare the Non-Volatile storage as EFI_MEMORY_RUNTIME
>    //
>    RuntimeMmioRegionSize = mFvbDevice->FvbSize;
> @@ -1126,6 +1142,11 @@ ErrorSetMemAttr:
>    gDS->RemoveMemorySpace (RegionBaseAddress, RuntimeMmioRegionSize);
>
>  ErrorAddSpace:
> +  gBS->UninstallProtocolInterface (&gImageHandle,
> +                  &gEdkiiNvVarStoreFormattedGuid,
> +                  NULL);
> +
> +ErrorInstallNvVarStoreFormatted:
>    gBS->UninstallMultipleProtocolInterfaces (&mFvbDevice->Handle,
>           &gEfiDevicePathProtocolGuid,
>           &gEfiFirmwareVolumeBlockProtocolGuid,
> diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
> index 117fe8b..fd3f2f7 100644
> --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
> +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
> @@ -63,6 +63,7 @@
>    UefiRuntimeServicesTableLib
>
>  [Guids]
> +  gEdkiiNvVarStoreFormattedGuid
>    gEfiAuthenticatedVariableGuid
>    gEfiEventVirtualAddressChangeGuid
>    gEfiSystemNvDataFvGuid
> @@ -84,8 +85,4 @@
>    gMarvellTokenSpaceGuid.PcdSpiMemoryBase
>
>  [Depex]
> -  #
> -  # MvFvbDxe must be loaded before VariableRuntimeDxe in case empty
> -  # flash needs populating with default values.
> -  #
> -  BEFORE gVariableRuntimeDxeFileGuid
> +  gEfiCpuArchProtocolGuid
> --
> 2.7.4
>

Re: [platforms PATCH 2/2] Marvell/Armada: RealTimeClockLib: Depend on gEfiCpuArchProtocolGuid

[Ard Biesheuvel]

On 16 April 2018 at 07:09, Marcin Wojtas <mw@semihalf.com> wrote:
> Recent changes in the EDK2 mainline resulted in breaking
> RTC functionality of Armada platforms.
>
> The RealTimeClockLib instance calls gDS->SetMemorySpaceAttributes()
> in the LibRtcInitialize() public function. This DXE service depends
> on the CPU Arch Protocol. Add it to the depex.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf
> index 2f842e8..59c71c4 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf
> +++ b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf
> @@ -25,7 +25,7 @@
>    FILE_GUID                      = fa81e889-045b-4c96-9093-742554fd0588
>    MODULE_TYPE                    = BASE
>    VERSION_STRING                 = 1.0
> -  LIBRARY_CLASS                  = RealTimeClockLib
> +  LIBRARY_CLASS                  = RealTimeClockLib|DXE_RUNTIME_DRIVER
>
>  [Sources.common]
>    RealTimeClockLib.c
> @@ -50,3 +50,6 @@
>
>  [Pcd]
>    gMarvellTokenSpaceGuid.PcdRtcEnabled
> +
> +[Depex.common.DXE_RUNTIME_DRIVER]
> +  gEfiCpuArchProtocolGuid
> --
> 2.7.4
>

Re: [platforms PATCH 1/2] Marvell/Drivers: MvFvbDxe: Adjust to new dependencies

[Marcin Wojtas]

Hi Ard,

2018-04-16 7:40 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> (+ Laszlo)
>
> On 16 April 2018 at 07:09, Marcin Wojtas <mw@semihalf.com> wrote:
>> Recent changes in the EDK2 mainline resulted in breaking
>> of compilation and booting of Armada platforms.
>> This patch adjust the MvFvbDxe driver by:
>>
>>  * installation of gEdkiiNvVarStoreFormattedGuid in order to signal
>>    NvVarStoreFormattedLib to the generic variable runtime driver
>>
>
> Hello Marcin,
>
> Installing this GUID is only necessary if you update your platform
> .DSC to make the generic variable runtime driver depend on it by
> adding a NULL library class resolution using NvVarStoreFormattedLib.
> So I think this patch is correct, but you'll need an additional change
> to make it work as expected. (Otherwise, the variable runtime driver
> could still be dispatched early and invoked for read access before the
> variable store is reformatted)

I added NULL class lib hook for VariableRuntimeDxe and it still works.
I'll send third patch on top of already submitted (the branch is
updated as well).

Thanks,
Marcin


>
>>  * making explicit dependency to ArmPkg/Drivers/CpuDxe drivers in order
>>    to enable successful calling of gDS->SetMemorySpaceAttributes
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c   | 21 ++++++++++++++++++++
>>  Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf |  7 ++-----
>>  2 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
>> index 252ef67..6e583a3 100644
>> --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
>> +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
>> @@ -26,6 +26,7 @@
>>  #include <Library/UefiLib.h>
>>  #include <Library/UefiRuntimeLib.h>
>>
>> +#include <Guid/NvVarStoreFormatted.h>
>>  #include <Guid/SystemNvDataGuid.h>
>>  #include <Guid/VariableFormat.h>
>>
>> @@ -1076,6 +1077,21 @@ MvFvbEntryPoint (
>>    }
>>
>>    //
>> +  // The driver implementing the variable read service can now be dispatched;
>> +  // the varstore headers are in place.
>> +  //
>> +  Status = gBS->InstallProtocolInterface (&gImageHandle,
>> +                  &gEdkiiNvVarStoreFormattedGuid,
>> +                  EFI_NATIVE_INTERFACE,
>> +                  NULL);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_ERROR,
>> +      "%a: Failed to install gEdkiiNvVarStoreFormattedGuid\n",
>> +      __FUNCTION__));
>> +    goto ErrorInstallNvVarStoreFormatted;
>> +  }
>> +
>> +  //
>>    // Declare the Non-Volatile storage as EFI_MEMORY_RUNTIME
>>    //
>>    RuntimeMmioRegionSize = mFvbDevice->FvbSize;
>> @@ -1126,6 +1142,11 @@ ErrorSetMemAttr:
>>    gDS->RemoveMemorySpace (RegionBaseAddress, RuntimeMmioRegionSize);
>>
>>  ErrorAddSpace:
>> +  gBS->UninstallProtocolInterface (&gImageHandle,
>> +                  &gEdkiiNvVarStoreFormattedGuid,
>> +                  NULL);
>> +
>> +ErrorInstallNvVarStoreFormatted:
>>    gBS->UninstallMultipleProtocolInterfaces (&mFvbDevice->Handle,
>>           &gEfiDevicePathProtocolGuid,
>>           &gEfiFirmwareVolumeBlockProtocolGuid,
>> diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>> index 117fe8b..fd3f2f7 100644
>> --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>> +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>> @@ -63,6 +63,7 @@
>>    UefiRuntimeServicesTableLib
>>
>>  [Guids]
>> +  gEdkiiNvVarStoreFormattedGuid
>>    gEfiAuthenticatedVariableGuid
>>    gEfiEventVirtualAddressChangeGuid
>>    gEfiSystemNvDataFvGuid
>> @@ -84,8 +85,4 @@
>>    gMarvellTokenSpaceGuid.PcdSpiMemoryBase
>>
>>  [Depex]
>> -  #
>> -  # MvFvbDxe must be loaded before VariableRuntimeDxe in case empty
>> -  # flash needs populating with default values.
>> -  #
>> -  BEFORE gVariableRuntimeDxeFileGuid
>> +  gEfiCpuArchProtocolGuid
>> --
>> 2.7.4
>>

Re: [platforms PATCH 1/2] Marvell/Drivers: MvFvbDxe: Adjust to new dependencies

[Laszlo Ersek]

On 04/16/18 07:40, Ard Biesheuvel wrote:
> (+ Laszlo)
> 
> On 16 April 2018 at 07:09, Marcin Wojtas <mw@semihalf.com> wrote:
>> Recent changes in the EDK2 mainline resulted in breaking
>> of compilation and booting of Armada platforms.
>> This patch adjust the MvFvbDxe driver by:
>>
>>  * installation of gEdkiiNvVarStoreFormattedGuid in order to signal
>>    NvVarStoreFormattedLib to the generic variable runtime driver
>>
> 
> Hello Marcin,
> 
> Installing this GUID is only necessary if you update your platform
> .DSC to make the generic variable runtime driver depend on it by
> adding a NULL library class resolution using NvVarStoreFormattedLib.
> So I think this patch is correct, but you'll need an additional change
> to make it work as expected. (Otherwise, the variable runtime driver
> could still be dispatched early and invoked for read access before the
> variable store is reformatted)

I agree.

I'd also like to point out another frequent pitfall in this patch:

>>  * making explicit dependency to ArmPkg/Drivers/CpuDxe drivers in order
>>    to enable successful calling of gDS->SetMemorySpaceAttributes
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c   | 21 ++++++++++++++++++++
>>  Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf |  7 ++-----
>>  2 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
>> index 252ef67..6e583a3 100644
>> --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
>> +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
>> @@ -26,6 +26,7 @@
>>  #include <Library/UefiLib.h>
>>  #include <Library/UefiRuntimeLib.h>
>>
>> +#include <Guid/NvVarStoreFormatted.h>
>>  #include <Guid/SystemNvDataGuid.h>
>>  #include <Guid/VariableFormat.h>
>>
>> @@ -1076,6 +1077,21 @@ MvFvbEntryPoint (
>>    }
>>
>>    //
>> +  // The driver implementing the variable read service can now be dispatched;
>> +  // the varstore headers are in place.
>> +  //
>> +  Status = gBS->InstallProtocolInterface (&gImageHandle,
>> +                  &gEdkiiNvVarStoreFormattedGuid,
>> +                  EFI_NATIVE_INTERFACE,
>> +                  NULL);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_ERROR,
>> +      "%a: Failed to install gEdkiiNvVarStoreFormattedGuid\n",
>> +      __FUNCTION__));
>> +    goto ErrorInstallNvVarStoreFormatted;
>> +  }
>> +
>> +  //
>>    // Declare the Non-Volatile storage as EFI_MEMORY_RUNTIME
>>    //
>>    RuntimeMmioRegionSize = mFvbDevice->FvbSize;
>> @@ -1126,6 +1142,11 @@ ErrorSetMemAttr:
>>    gDS->RemoveMemorySpace (RegionBaseAddress, RuntimeMmioRegionSize);
>>
>>  ErrorAddSpace:
>> +  gBS->UninstallProtocolInterface (&gImageHandle,
>> +                  &gEdkiiNvVarStoreFormattedGuid,
>> +                  NULL);
>> +

While gBS->InstallProtocolInterface() takes a *pointer* to a handle
(because it can *create* a handle, if the handle is NULL on input, and
the first protocol interface is installed on it),
gBS->UninstallProtocolInterface() takes the handle *itself*. If the last
protocol interface is uninstalled from the handle, then the handle is
destroyed, but gBS->UninstallProtocolInterface() does not attempt to
NULL the handle itself. So, here you should pass "gImageHandle", not
"&gImageHandle".

There's also a bit of whitespace mangling here that's not compatible
with either multiline function call style that we like in edk2, but
perhaps edk2-platforms treats that more laxly.

Thanks
Laszlo

>> +ErrorInstallNvVarStoreFormatted:
>>    gBS->UninstallMultipleProtocolInterfaces (&mFvbDevice->Handle,
>>           &gEfiDevicePathProtocolGuid,
>>           &gEfiFirmwareVolumeBlockProtocolGuid,
>> diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>> index 117fe8b..fd3f2f7 100644
>> --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>> +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>> @@ -63,6 +63,7 @@
>>    UefiRuntimeServicesTableLib
>>
>>  [Guids]
>> +  gEdkiiNvVarStoreFormattedGuid
>>    gEfiAuthenticatedVariableGuid
>>    gEfiEventVirtualAddressChangeGuid
>>    gEfiSystemNvDataFvGuid
>> @@ -84,8 +85,4 @@
>>    gMarvellTokenSpaceGuid.PcdSpiMemoryBase
>>
>>  [Depex]
>> -  #
>> -  # MvFvbDxe must be loaded before VariableRuntimeDxe in case empty
>> -  # flash needs populating with default values.
>> -  #
>> -  BEFORE gVariableRuntimeDxeFileGuid
>> +  gEfiCpuArchProtocolGuid
>> --
>> 2.7.4
>>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [platforms PATCH 2/2] Marvell/Armada: RealTimeClockLib: Depend on gEfiCpuArchProtocolGuid

[Laszlo Ersek]

On 04/16/18 07:09, Marcin Wojtas wrote:
> Recent changes in the EDK2 mainline resulted in breaking
> RTC functionality of Armada platforms.
> 
> The RealTimeClockLib instance calls gDS->SetMemorySpaceAttributes()
> in the LibRtcInitialize() public function. This DXE service depends
> on the CPU Arch Protocol. Add it to the depex.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf
> index 2f842e8..59c71c4 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf
> +++ b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.inf
> @@ -25,7 +25,7 @@
>    FILE_GUID                      = fa81e889-045b-4c96-9093-742554fd0588
>    MODULE_TYPE                    = BASE
>    VERSION_STRING                 = 1.0
> -  LIBRARY_CLASS                  = RealTimeClockLib
> +  LIBRARY_CLASS                  = RealTimeClockLib|DXE_RUNTIME_DRIVER
>  
>  [Sources.common]
>    RealTimeClockLib.c
> @@ -50,3 +50,6 @@
>  
>  [Pcd]
>    gMarvellTokenSpaceGuid.PcdRtcEnabled
> +
> +[Depex.common.DXE_RUNTIME_DRIVER]
> +  gEfiCpuArchProtocolGuid
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>

Re: [platforms PATCH 1/2] Marvell/Drivers: MvFvbDxe: Adjust to new dependencies

[Marcin Wojtas]

Hi Laszlo,

2018-04-16 21:41 GMT+02:00 Laszlo Ersek <lersek@redhat.com>:
> On 04/16/18 07:40, Ard Biesheuvel wrote:
>> (+ Laszlo)
>>
>> On 16 April 2018 at 07:09, Marcin Wojtas <mw@semihalf.com> wrote:
>>> Recent changes in the EDK2 mainline resulted in breaking
>>> of compilation and booting of Armada platforms.
>>> This patch adjust the MvFvbDxe driver by:
>>>
>>>  * installation of gEdkiiNvVarStoreFormattedGuid in order to signal
>>>    NvVarStoreFormattedLib to the generic variable runtime driver
>>>
>>
>> Hello Marcin,
>>
>> Installing this GUID is only necessary if you update your platform
>> .DSC to make the generic variable runtime driver depend on it by
>> adding a NULL library class resolution using NvVarStoreFormattedLib.
>> So I think this patch is correct, but you'll need an additional change
>> to make it work as expected. (Otherwise, the variable runtime driver
>> could still be dispatched early and invoked for read access before the
>> variable store is reformatted)
>
> I agree.
>
> I'd also like to point out another frequent pitfall in this patch:
>
>
> While gBS->InstallProtocolInterface() takes a *pointer* to a handle
> (because it can *create* a handle, if the handle is NULL on input, and
> the first protocol interface is installed on it),
> gBS->UninstallProtocolInterface() takes the handle *itself*. If the last
> protocol interface is uninstalled from the handle, then the handle is
> destroyed, but gBS->UninstallProtocolInterface() does not attempt to
> NULL the handle itself. So, here you should pass "gImageHandle", not
> "&gImageHandle".
>

Right, I'll correct it.

> There's also a bit of whitespace mangling here that's not compatible
> with either multiline function call style that we like in edk2, but
> perhaps edk2-platforms treats that more laxly.
>

We had some discussions last year - I followed the coding standards:

Function (
  Argument1,
  Argument2,
  Argument3
  );

But was requested to place Argument1 to the function line and the last
bracket to Argument3 line:

Function (Argument1,
  Argument2,
  Argument3);

Afair, there were some attempts to modify coding standards at the
time, but I see the original version persisted. In fact I can do
whatever line-breaking necessary:

Ard - what style do you prefer in future patches?

Best regards,
Marcin

> Thanks
> Laszlo
>
>>> +ErrorInstallNvVarStoreFormatted:
>>>    gBS->UninstallMultipleProtocolInterfaces (&mFvbDevice->Handle,
>>>           &gEfiDevicePathProtocolGuid,
>>>           &gEfiFirmwareVolumeBlockProtocolGuid,
>>> diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>>> index 117fe8b..fd3f2f7 100644
>>> --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>>> +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
>>> @@ -63,6 +63,7 @@
>>>    UefiRuntimeServicesTableLib
>>>
>>>  [Guids]
>>> +  gEdkiiNvVarStoreFormattedGuid
>>>    gEfiAuthenticatedVariableGuid
>>>    gEfiEventVirtualAddressChangeGuid
>>>    gEfiSystemNvDataFvGuid
>>> @@ -84,8 +85,4 @@
>>>    gMarvellTokenSpaceGuid.PcdSpiMemoryBase
>>>
>>>  [Depex]
>>> -  #
>>> -  # MvFvbDxe must be loaded before VariableRuntimeDxe in case empty
>>> -  # flash needs populating with default values.
>>> -  #
>>> -  BEFORE gVariableRuntimeDxeFileGuid
>>> +  gEfiCpuArchProtocolGuid
>>> --
>>> 2.7.4
>>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>

Re: [platforms PATCH 1/2] Marvell/Drivers: MvFvbDxe: Adjust to new dependencies

[Ard Biesheuvel]

On 17 April 2018 at 07:15, Marcin Wojtas <mw@semihalf.com> wrote:
> Hi Laszlo,
>
> 2018-04-16 21:41 GMT+02:00 Laszlo Ersek <lersek@redhat.com>:
>> On 04/16/18 07:40, Ard Biesheuvel wrote:
>>> (+ Laszlo)
>>>
>>> On 16 April 2018 at 07:09, Marcin Wojtas <mw@semihalf.com> wrote:
>>>> Recent changes in the EDK2 mainline resulted in breaking
>>>> of compilation and booting of Armada platforms.
>>>> This patch adjust the MvFvbDxe driver by:
>>>>
>>>>  * installation of gEdkiiNvVarStoreFormattedGuid in order to signal
>>>>    NvVarStoreFormattedLib to the generic variable runtime driver
>>>>
>>>
>>> Hello Marcin,
>>>
>>> Installing this GUID is only necessary if you update your platform
>>> .DSC to make the generic variable runtime driver depend on it by
>>> adding a NULL library class resolution using NvVarStoreFormattedLib.
>>> So I think this patch is correct, but you'll need an additional change
>>> to make it work as expected. (Otherwise, the variable runtime driver
>>> could still be dispatched early and invoked for read access before the
>>> variable store is reformatted)
>>
>> I agree.
>>
>> I'd also like to point out another frequent pitfall in this patch:
>>
>>
>> While gBS->InstallProtocolInterface() takes a *pointer* to a handle
>> (because it can *create* a handle, if the handle is NULL on input, and
>> the first protocol interface is installed on it),
>> gBS->UninstallProtocolInterface() takes the handle *itself*. If the last
>> protocol interface is uninstalled from the handle, then the handle is
>> destroyed, but gBS->UninstallProtocolInterface() does not attempt to
>> NULL the handle itself. So, here you should pass "gImageHandle", not
>> "&gImageHandle".
>>
>
> Right, I'll correct it.
>

Ah, I missed that. Thanks for spotting that Laszlo

>> There's also a bit of whitespace mangling here that's not compatible
>> with either multiline function call style that we like in edk2, but
>> perhaps edk2-platforms treats that more laxly.
>>
>
> We had some discussions last year - I followed the coding standards:
>
> Function (
>   Argument1,
>   Argument2,
>   Argument3
>   );
>
> But was requested to place Argument1 to the function line and the last
> bracket to Argument3 line:
>
> Function (Argument1,
>   Argument2,
>   Argument3);
>
> Afair, there were some attempts to modify coding standards at the
> time, but I see the original version persisted. In fact I can do
> whatever line-breaking necessary:
>
> Ard - what style do you prefer in future patches?
>

I tend to treat the coding style document as a guideline rather than
rule of law, given that it is not entirely consistent with current
practice to begin with. In my opinion, self consistency and legibility
are more important than adhering to some rule, although I realize
legibility is a subjective quality

Personally, I think the former takes up too much space in general, but
with complex expressions as arguments, it is more readable than the
latter.

Re: [platforms PATCH 1/2] Marvell/Drivers: MvFvbDxe: Adjust to new dependencies

[Marcin Wojtas]

Hi Ard,

2018-04-17 7:32 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> On 17 April 2018 at 07:15, Marcin Wojtas <mw@semihalf.com> wrote:
>> Hi Laszlo,
>>
>> 2018-04-16 21:41 GMT+02:00 Laszlo Ersek <lersek@redhat.com>:
>>> On 04/16/18 07:40, Ard Biesheuvel wrote:
>>>> (+ Laszlo)
>>>>
>>>> On 16 April 2018 at 07:09, Marcin Wojtas <mw@semihalf.com> wrote:
>>>>> Recent changes in the EDK2 mainline resulted in breaking
>>>>> of compilation and booting of Armada platforms.
>>>>> This patch adjust the MvFvbDxe driver by:
>>>>>
>>>>>  * installation of gEdkiiNvVarStoreFormattedGuid in order to signal
>>>>>    NvVarStoreFormattedLib to the generic variable runtime driver
>>>>>
>>>>
>>>> Hello Marcin,
>>>>
>>>> Installing this GUID is only necessary if you update your platform
>>>> .DSC to make the generic variable runtime driver depend on it by
>>>> adding a NULL library class resolution using NvVarStoreFormattedLib.
>>>> So I think this patch is correct, but you'll need an additional change
>>>> to make it work as expected. (Otherwise, the variable runtime driver
>>>> could still be dispatched early and invoked for read access before the
>>>> variable store is reformatted)
>>>
>>> I agree.
>>>
>>> I'd also like to point out another frequent pitfall in this patch:
>>>
>>>
>>> While gBS->InstallProtocolInterface() takes a *pointer* to a handle
>>> (because it can *create* a handle, if the handle is NULL on input, and
>>> the first protocol interface is installed on it),
>>> gBS->UninstallProtocolInterface() takes the handle *itself*. If the last
>>> protocol interface is uninstalled from the handle, then the handle is
>>> destroyed, but gBS->UninstallProtocolInterface() does not attempt to
>>> NULL the handle itself. So, here you should pass "gImageHandle", not
>>> "&gImageHandle".
>>>
>>
>> Right, I'll correct it.
>>
>
> Ah, I missed that. Thanks for spotting that Laszlo
>
>>> There's also a bit of whitespace mangling here that's not compatible
>>> with either multiline function call style that we like in edk2, but
>>> perhaps edk2-platforms treats that more laxly.
>>>
>>
>> We had some discussions last year - I followed the coding standards:
>>
>> Function (
>>   Argument1,
>>   Argument2,
>>   Argument3
>>   );
>>
>> But was requested to place Argument1 to the function line and the last
>> bracket to Argument3 line:
>>
>> Function (Argument1,
>>   Argument2,
>>   Argument3);
>>
>> Afair, there were some attempts to modify coding standards at the
>> time, but I see the original version persisted. In fact I can do
>> whatever line-breaking necessary:
>>
>> Ard - what style do you prefer in future patches?
>>
>
> I tend to treat the coding style document as a guideline rather than
> rule of law, given that it is not entirely consistent with current
> practice to begin with. In my opinion, self consistency and legibility
> are more important than adhering to some rule, although I realize
> legibility is a subjective quality
>
> Personally, I think the former takes up too much space in general, but
> with complex expressions as arguments, it is more readable than the
> latter.

Well, I see it can be treated sort of as a matter of taste. In order
to be consistent with my previous code, I'd keep variant2 as the line
breaking scheme in edk2-platforms. In edk2 I don't suspect much code
submitted, but to be on a safe side I will use variant1. Is it ok for
you?

Thanks,
Marcin

Re: [platforms PATCH 1/2] Marvell/Drivers: MvFvbDxe: Adjust to new dependencies

[Ard Biesheuvel]

On 17 April 2018 at 08:04, Marcin Wojtas <mw@semihalf.com> wrote:
> Hi Ard,
>
> 2018-04-17 7:32 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
>> On 17 April 2018 at 07:15, Marcin Wojtas <mw@semihalf.com> wrote:
>>> Hi Laszlo,
>>>
>>> 2018-04-16 21:41 GMT+02:00 Laszlo Ersek <lersek@redhat.com>:
>>>> On 04/16/18 07:40, Ard Biesheuvel wrote:
>>>>> (+ Laszlo)
>>>>>
>>>>> On 16 April 2018 at 07:09, Marcin Wojtas <mw@semihalf.com> wrote:
>>>>>> Recent changes in the EDK2 mainline resulted in breaking
>>>>>> of compilation and booting of Armada platforms.
>>>>>> This patch adjust the MvFvbDxe driver by:
>>>>>>
>>>>>>  * installation of gEdkiiNvVarStoreFormattedGuid in order to signal
>>>>>>    NvVarStoreFormattedLib to the generic variable runtime driver
>>>>>>
>>>>>
>>>>> Hello Marcin,
>>>>>
>>>>> Installing this GUID is only necessary if you update your platform
>>>>> .DSC to make the generic variable runtime driver depend on it by
>>>>> adding a NULL library class resolution using NvVarStoreFormattedLib.
>>>>> So I think this patch is correct, but you'll need an additional change
>>>>> to make it work as expected. (Otherwise, the variable runtime driver
>>>>> could still be dispatched early and invoked for read access before the
>>>>> variable store is reformatted)
>>>>
>>>> I agree.
>>>>
>>>> I'd also like to point out another frequent pitfall in this patch:
>>>>
>>>>
>>>> While gBS->InstallProtocolInterface() takes a *pointer* to a handle
>>>> (because it can *create* a handle, if the handle is NULL on input, and
>>>> the first protocol interface is installed on it),
>>>> gBS->UninstallProtocolInterface() takes the handle *itself*. If the last
>>>> protocol interface is uninstalled from the handle, then the handle is
>>>> destroyed, but gBS->UninstallProtocolInterface() does not attempt to
>>>> NULL the handle itself. So, here you should pass "gImageHandle", not
>>>> "&gImageHandle".
>>>>
>>>
>>> Right, I'll correct it.
>>>
>>
>> Ah, I missed that. Thanks for spotting that Laszlo
>>
>>>> There's also a bit of whitespace mangling here that's not compatible
>>>> with either multiline function call style that we like in edk2, but
>>>> perhaps edk2-platforms treats that more laxly.
>>>>
>>>
>>> We had some discussions last year - I followed the coding standards:
>>>
>>> Function (
>>>   Argument1,
>>>   Argument2,
>>>   Argument3
>>>   );
>>>
>>> But was requested to place Argument1 to the function line and the last
>>> bracket to Argument3 line:
>>>
>>> Function (Argument1,
>>>   Argument2,
>>>   Argument3);
>>>
>>> Afair, there were some attempts to modify coding standards at the
>>> time, but I see the original version persisted. In fact I can do
>>> whatever line-breaking necessary:
>>>
>>> Ard - what style do you prefer in future patches?
>>>
>>
>> I tend to treat the coding style document as a guideline rather than
>> rule of law, given that it is not entirely consistent with current
>> practice to begin with. In my opinion, self consistency and legibility
>> are more important than adhering to some rule, although I realize
>> legibility is a subjective quality
>>
>> Personally, I think the former takes up too much space in general, but
>> with complex expressions as arguments, it is more readable than the
>> latter.
>
> Well, I see it can be treated sort of as a matter of taste. In order
> to be consistent with my previous code, I'd keep variant2 as the line
> breaking scheme in edk2-platforms. In edk2 I don't suspect much code
> submitted, but to be on a safe side I will use variant1. Is it ok for
> you?
>

Works for me

Re: [platforms PATCH 1/2] Marvell/Drivers: MvFvbDxe: Adjust to new dependencies

[Laszlo Ersek]

On 04/17/18 07:15, Marcin Wojtas wrote:
> 2018-04-16 21:41 GMT+02:00 Laszlo Ersek <lersek@redhat.com>:

>> There's also a bit of whitespace mangling here that's not compatible
>> with either multiline function call style that we like in edk2, but
>> perhaps edk2-platforms treats that more laxly.
>>
> 
> We had some discussions last year - I followed the coding standards:
> 
> Function (
>   Argument1,
>   Argument2,
>   Argument3
>   );

Right, this is option #1.

> But was requested to place Argument1 to the function line and the last
> bracket to Argument3 line:
> 
> Function (Argument1,
>   Argument2,
>   Argument3);

In this case I prefer compressing the arguments as much as possible, for
example

  Status = ProtocolInstance->LongMemberFunctionName (Argument1,
                               Argument2, Argument3, Argument4,
                               Argument5, Argument6);

So either break the first argument and the closing paren as well to new
lines, or else keep all arguments compressed -- break them to new lines
only when the max line width dictates so.

No need to resubmit the patch because of this, I just wanted to clarify
my remark.

Thanks
Laszlo