[edk2-platforms] [PATCH 0/3] Fix SimicsOpenBoardPkg GCC Build

[edk2-platforms] [PATCH 0/3] Fix SimicsOpenBoardPkg GCC Build

[Agyeman, Prince]

This patch series fix gcc build issues seen
in SimicsOpenBoardPkg.
Patches were tested on GCC 5

Prince Agyeman (3):
  AdvancedFeaturePkg: Fix GCC build
  SimicsOpenBoardPkg: Fix GCC build issues
  SimicsIch10Pkg: Fix GCC build issues

 .../Smbios/SmbiosBasicDxe/SmbiosBasic.h       |  2 +-
 .../PlatformBootManagerLib/BdsPlatform.c      | 38 -------------------
 .../SimicsOpenBoardPkg/SecCore/SecMain.c      | 23 -----------
 .../BasePchSpiCommonLib/SpiCommon.c           |  3 --
 .../SmmControl/RuntimeDxe/SmmControl2Dxe.c    |  7 +---
 5 files changed, 3 insertions(+), 70 deletions(-)

-- 
2.19.1.windows.1

[edk2-platforms] [PATCH 1/3] AdvancedFeaturePkg: Fix GCC build

[Agyeman, Prince]

Cc: David Wei  <david.y.wei@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Ankit Sinha <ankit.sinha@intel.com>
Cc: Agyeman Prince <prince.agyeman@intel.com>
Cc: Kubacki Michael A <michael.a.kubacki@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>

Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
---
 .../AdvancedFeaturePkg/Smbios/SmbiosBasicDxe/SmbiosBasic.h      | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/Intel/AdvancedFeaturePkg/Smbios/SmbiosBasicDxe/SmbiosBasic.h b/Platform/Intel/AdvancedFeaturePkg/Smbios/SmbiosBasicDxe/SmbiosBasic.h
index e278a4ed18..944946b5b8 100644
--- a/Platform/Intel/AdvancedFeaturePkg/Smbios/SmbiosBasicDxe/SmbiosBasic.h
+++ b/Platform/Intel/AdvancedFeaturePkg/Smbios/SmbiosBasicDxe/SmbiosBasic.h
@@ -11,7 +11,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include <PiDxe.h>
 #include <Protocol/Smbios.h>
-#include <IndustryStandard/Smbios.h>
+#include <IndustryStandard/SmBios.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/DebugLib.h>
 #include <Library/BaseLib.h>
-- 
2.19.1.windows.1

[edk2-platforms] [PATCH 2/3] SimicsOpenBoardPkg: Fix GCC build issues

[Agyeman, Prince]

Cc: David Wei  <david.y.wei@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Ankit Sinha <ankit.sinha@intel.com>
Cc: Agyeman Prince <prince.agyeman@intel.com>
Cc: Kubacki Michael A <michael.a.kubacki@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>

Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
---
 .../PlatformBootManagerLib/BdsPlatform.c      | 38 -------------------
 .../SimicsOpenBoardPkg/SecCore/SecMain.c      | 23 -----------
 2 files changed, 61 deletions(-)

diff --git a/Platform/Intel/SimicsOpenBoardPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/Platform/Intel/SimicsOpenBoardPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 117c72b35f..953a4a6c15 100644
--- a/Platform/Intel/SimicsOpenBoardPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/Platform/Intel/SimicsOpenBoardPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -316,12 +316,6 @@ ConnectRootBridge (
   IN VOID        *Context
   );
 
-STATIC
-VOID
-SaveS3BootScript (
-  VOID
-  );
-
 //
 // BDS Platform Functions
 //
@@ -1296,38 +1290,6 @@ PlatformBdsConnectSequence (
   PciAcpiInitialization ();
 }
 
-/**
-  Save the S3 boot script.
-
-  Note that DxeSmmReadyToLock must be signaled after this function returns;
-  otherwise the script wouldn't be saved actually.
-**/
-STATIC
-VOID
-SaveS3BootScript (
-  VOID
-  )
-{
-  EFI_STATUS                 Status;
-  EFI_S3_SAVE_STATE_PROTOCOL *BootScript;
-  STATIC CONST UINT8         Info[] = { 0xDE, 0xAD, 0xBE, 0xEF };
-
-  Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid, NULL,
-                  (VOID **) &BootScript);
-  ASSERT_EFI_ERROR (Status);
-
-  //
-  // Despite the opcode documentation in the PI spec, the protocol
-  // implementation embeds a deep copy of the info in the boot script, rather
-  // than storing just a pointer to runtime or NVS storage.
-  //
-  Status = BootScript->Write(BootScript, EFI_BOOT_SCRIPT_INFORMATION_OPCODE,
-                         (UINT32) sizeof Info,
-                         (EFI_PHYSICAL_ADDRESS)(UINTN) &Info);
-  ASSERT_EFI_ERROR (Status);
-}
-
-
 /**
   Do the platform specific action after the console is ready
 
diff --git a/Platform/Intel/SimicsOpenBoardPkg/SecCore/SecMain.c b/Platform/Intel/SimicsOpenBoardPkg/SecCore/SecMain.c
index 5cbb47687b..4514641b46 100644
--- a/Platform/Intel/SimicsOpenBoardPkg/SecCore/SecMain.c
+++ b/Platform/Intel/SimicsOpenBoardPkg/SecCore/SecMain.c
@@ -495,29 +495,6 @@ FindPeiCoreImageBaseInFv (
   return EFI_SUCCESS;
 }
 
-
-/**
-  Reads 8-bits of CMOS data.
-
-  Reads the 8-bits of CMOS data at the location specified by Index.
-  The 8-bit read value is returned.
-
-  @param  Index  The CMOS location to read.
-
-  @return The value read.
-
-**/
-STATIC
-UINT8
-CmosRead8 (
-  IN UINTN        Index
-  )
-{
-  IoWrite8 (0x70, (UINT8) Index);
-  return IoRead8 (0x71);
-}
-
-
 STATIC
 BOOLEAN
 IsS3Resume (
-- 
2.19.1.windows.1

[edk2-platforms] [PATCH 3/3] SimicsIch10Pkg: Fix GCC build issues

[Agyeman, Prince]

Cc: David Wei  <david.y.wei@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Ankit Sinha <ankit.sinha@intel.com>
Cc: Agyeman Prince <prince.agyeman@intel.com>
Cc: Kubacki Michael A <michael.a.kubacki@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>

Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
---
 .../LibraryPrivate/BasePchSpiCommonLib/SpiCommon.c         | 3 ---
 .../SimicsIch10Pkg/SmmControl/RuntimeDxe/SmmControl2Dxe.c  | 7 ++-----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/Silicon/Intel/SimicsIch10Pkg/LibraryPrivate/BasePchSpiCommonLib/SpiCommon.c b/Silicon/Intel/SimicsIch10Pkg/LibraryPrivate/BasePchSpiCommonLib/SpiCommon.c
index bd08b2453b..3e7dffedfb 100644
--- a/Silicon/Intel/SimicsIch10Pkg/LibraryPrivate/BasePchSpiCommonLib/SpiCommon.c
+++ b/Silicon/Intel/SimicsIch10Pkg/LibraryPrivate/BasePchSpiCommonLib/SpiCommon.c
@@ -637,7 +637,6 @@ SendSpiCmd (
   EFI_STATUS      Status;
   UINT32          Index;
   SPI_INSTANCE    *SpiInstance;
-  UINTN           SpiBaseAddress;
   UINTN           PchSpiBar0;
   UINT32          HardwareSpiAddr;
   UINT32          FlashRegionSize;
@@ -648,9 +647,7 @@ SendSpiCmd (
 
   Status            = EFI_SUCCESS;
   SpiInstance       = SPI_INSTANCE_FROM_SPIPROTOCOL (This);
-  SpiBaseAddress    = SpiInstance->PchSpiBase;
   PchSpiBar0        = AcquireSpiBar0 (SpiInstance);
-  SpiBaseAddress    = SpiInstance->PchSpiBase;
   ABase             = SpiInstance->PchAcpiBase;
 
   //
diff --git a/Silicon/Intel/SimicsIch10Pkg/SmmControl/RuntimeDxe/SmmControl2Dxe.c b/Silicon/Intel/SimicsIch10Pkg/SmmControl/RuntimeDxe/SmmControl2Dxe.c
index 268b04d25a..cc2d00b785 100644
--- a/Silicon/Intel/SimicsIch10Pkg/SmmControl/RuntimeDxe/SmmControl2Dxe.c
+++ b/Silicon/Intel/SimicsIch10Pkg/SmmControl/RuntimeDxe/SmmControl2Dxe.c
@@ -131,7 +131,6 @@ SmmControl2DxeTrigger (
   IN UINTN                            ActivationInterval OPTIONAL
   )
 {
-  EFI_STATUS  Status;
   //
   // No support for queued or periodic activation.
   //
@@ -141,7 +140,7 @@ SmmControl2DxeTrigger (
   ///
   /// Clear any pending the APM SMI
   ///
-  Status = SmmClear();
+  SmmClear();
   //
   // The so-called "Advanced Power Management Status Port Register" is in fact
   // a generic data passing register, between the caller and the SMI
@@ -181,8 +180,6 @@ SmmControl2DxeClear (
   IN BOOLEAN                          Periodic OPTIONAL
   )
 {
-  EFI_STATUS  Status;
-
   if (Periodic) {
     return EFI_INVALID_PARAMETER;
   }
@@ -201,7 +198,7 @@ SmmControl2DxeClear (
   //
   // So, nothing to do here.
   //
-  Status = SmmClear();
+  SmmClear();
 
   return EFI_SUCCESS;
 }
-- 
2.19.1.windows.1

Re: [edk2-devel] [edk2-platforms] [PATCH 0/3] Fix SimicsOpenBoardPkg GCC Build

[Philippe Mathieu-Daudé]

Hi Prince,

On 9/10/19 1:15 AM, Agyeman, Prince wrote:
> This patch series fix gcc build issues seen
> in SimicsOpenBoardPkg.
> Patches were tested on GCC 5
> 
> Prince Agyeman (3):
>   AdvancedFeaturePkg: Fix GCC build
>   SimicsOpenBoardPkg: Fix GCC build issues
>   SimicsIch10Pkg: Fix GCC build issues

It seems you forgot to describe what each patch does and why.

The EDK2 project has few recommendations:
https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format

Personally I find this link very explicit:
https://wiki.openstack.org/wiki/GitCommitMessages#Information_in_commit_messages

You could paste the relevant part of the GCC error you get,
and explain why you decided to do your change:
"This variable is never read, we can safely remove it because ..."

Sometime the proper fix for a warning is not to remove the unused code,
but check why this code is not used, and fix by using it correctly.

If you can justify it is safe to remove unused code, then it is fine.

Regards,

Phil.

Re: [edk2-platforms] [PATCH 1/3] AdvancedFeaturePkg: Fix GCC build

[Nate DeSimone]

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

-----Original Message-----
From: Agyeman, Prince 
Sent: Monday, September 9, 2019 4:15 PM
To: devel@edk2.groups.io
Cc: Wei, David Y <david.y.wei@intel.com>; Gao, Liming <liming.gao@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>; Agyeman, Prince <prince.agyeman@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Subject: [edk2-platforms] [PATCH 1/3] AdvancedFeaturePkg: Fix GCC build

Cc: David Wei  <david.y.wei@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Ankit Sinha <ankit.sinha@intel.com>
Cc: Agyeman Prince <prince.agyeman@intel.com>
Cc: Kubacki Michael A <michael.a.kubacki@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>

Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
---
 .../AdvancedFeaturePkg/Smbios/SmbiosBasicDxe/SmbiosBasic.h      | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/Intel/AdvancedFeaturePkg/Smbios/SmbiosBasicDxe/SmbiosBasic.h b/Platform/Intel/AdvancedFeaturePkg/Smbios/SmbiosBasicDxe/SmbiosBasic.h
index e278a4ed18..944946b5b8 100644
--- a/Platform/Intel/AdvancedFeaturePkg/Smbios/SmbiosBasicDxe/SmbiosBasic.h
+++ b/Platform/Intel/AdvancedFeaturePkg/Smbios/SmbiosBasicDxe/SmbiosBasic.h
@@ -11,7 +11,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include <PiDxe.h>
 #include <Protocol/Smbios.h>
-#include <IndustryStandard/Smbios.h>
+#include <IndustryStandard/SmBios.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/DebugLib.h>
 #include <Library/BaseLib.h>
-- 
2.19.1.windows.1

Re: [edk2-platforms] [PATCH 2/3] SimicsOpenBoardPkg: Fix GCC build issues

[Nate DeSimone]

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

-----Original Message-----
From: Agyeman, Prince 
Sent: Monday, September 9, 2019 4:15 PM
To: devel@edk2.groups.io
Cc: Wei, David Y <david.y.wei@intel.com>; Gao, Liming <liming.gao@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>; Agyeman, Prince <prince.agyeman@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Subject: [edk2-platforms] [PATCH 2/3] SimicsOpenBoardPkg: Fix GCC build issues

Cc: David Wei  <david.y.wei@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Ankit Sinha <ankit.sinha@intel.com>
Cc: Agyeman Prince <prince.agyeman@intel.com>
Cc: Kubacki Michael A <michael.a.kubacki@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>

Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
---
 .../PlatformBootManagerLib/BdsPlatform.c      | 38 -------------------
 .../SimicsOpenBoardPkg/SecCore/SecMain.c      | 23 -----------
 2 files changed, 61 deletions(-)

diff --git a/Platform/Intel/SimicsOpenBoardPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/Platform/Intel/SimicsOpenBoardPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 117c72b35f..953a4a6c15 100644
--- a/Platform/Intel/SimicsOpenBoardPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/Platform/Intel/SimicsOpenBoardPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -316,12 +316,6 @@ ConnectRootBridge (
   IN VOID        *Context
   );
 
-STATIC
-VOID
-SaveS3BootScript (
-  VOID
-  );
-
 //
 // BDS Platform Functions
 //
@@ -1296,38 +1290,6 @@ PlatformBdsConnectSequence (
   PciAcpiInitialization ();
 }
 
-/**
-  Save the S3 boot script.
-
-  Note that DxeSmmReadyToLock must be signaled after this function returns;
-  otherwise the script wouldn't be saved actually.
-**/
-STATIC
-VOID
-SaveS3BootScript (
-  VOID
-  )
-{
-  EFI_STATUS                 Status;
-  EFI_S3_SAVE_STATE_PROTOCOL *BootScript;
-  STATIC CONST UINT8         Info[] = { 0xDE, 0xAD, 0xBE, 0xEF };
-
-  Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid, NULL,
-                  (VOID **) &BootScript);
-  ASSERT_EFI_ERROR (Status);
-
-  //
-  // Despite the opcode documentation in the PI spec, the protocol
-  // implementation embeds a deep copy of the info in the boot script, rather
-  // than storing just a pointer to runtime or NVS storage.
-  //
-  Status = BootScript->Write(BootScript, EFI_BOOT_SCRIPT_INFORMATION_OPCODE,
-                         (UINT32) sizeof Info,
-                         (EFI_PHYSICAL_ADDRESS)(UINTN) &Info);
-  ASSERT_EFI_ERROR (Status);
-}
-
-
 /**
   Do the platform specific action after the console is ready
 
diff --git a/Platform/Intel/SimicsOpenBoardPkg/SecCore/SecMain.c b/Platform/Intel/SimicsOpenBoardPkg/SecCore/SecMain.c
index 5cbb47687b..4514641b46 100644
--- a/Platform/Intel/SimicsOpenBoardPkg/SecCore/SecMain.c
+++ b/Platform/Intel/SimicsOpenBoardPkg/SecCore/SecMain.c
@@ -495,29 +495,6 @@ FindPeiCoreImageBaseInFv (
   return EFI_SUCCESS;
 }
 
-
-/**
-  Reads 8-bits of CMOS data.
-
-  Reads the 8-bits of CMOS data at the location specified by Index.
-  The 8-bit read value is returned.
-
-  @param  Index  The CMOS location to read.
-
-  @return The value read.
-
-**/
-STATIC
-UINT8
-CmosRead8 (
-  IN UINTN        Index
-  )
-{
-  IoWrite8 (0x70, (UINT8) Index);
-  return IoRead8 (0x71);
-}
-
-
 STATIC
 BOOLEAN
 IsS3Resume (
-- 
2.19.1.windows.1

Re: [edk2-platforms] [PATCH 3/3] SimicsIch10Pkg: Fix GCC build issues

[Nate DeSimone]

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

-----Original Message-----
From: Agyeman, Prince 
Sent: Monday, September 9, 2019 4:15 PM
To: devel@edk2.groups.io
Cc: Wei, David Y <david.y.wei@intel.com>; Gao, Liming <liming.gao@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>; Agyeman, Prince <prince.agyeman@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Subject: [edk2-platforms] [PATCH 3/3] SimicsIch10Pkg: Fix GCC build issues

Cc: David Wei  <david.y.wei@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Ankit Sinha <ankit.sinha@intel.com>
Cc: Agyeman Prince <prince.agyeman@intel.com>
Cc: Kubacki Michael A <michael.a.kubacki@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>

Signed-off-by: Prince Agyeman <prince.agyeman@intel.com>
---
 .../LibraryPrivate/BasePchSpiCommonLib/SpiCommon.c         | 3 ---
 .../SimicsIch10Pkg/SmmControl/RuntimeDxe/SmmControl2Dxe.c  | 7 ++-----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/Silicon/Intel/SimicsIch10Pkg/LibraryPrivate/BasePchSpiCommonLib/SpiCommon.c b/Silicon/Intel/SimicsIch10Pkg/LibraryPrivate/BasePchSpiCommonLib/SpiCommon.c
index bd08b2453b..3e7dffedfb 100644
--- a/Silicon/Intel/SimicsIch10Pkg/LibraryPrivate/BasePchSpiCommonLib/SpiCommon.c
+++ b/Silicon/Intel/SimicsIch10Pkg/LibraryPrivate/BasePchSpiCommonLib/Sp
+++ iCommon.c
@@ -637,7 +637,6 @@ SendSpiCmd (
   EFI_STATUS      Status;
   UINT32          Index;
   SPI_INSTANCE    *SpiInstance;
-  UINTN           SpiBaseAddress;
   UINTN           PchSpiBar0;
   UINT32          HardwareSpiAddr;
   UINT32          FlashRegionSize;
@@ -648,9 +647,7 @@ SendSpiCmd (
 
   Status            = EFI_SUCCESS;
   SpiInstance       = SPI_INSTANCE_FROM_SPIPROTOCOL (This);
-  SpiBaseAddress    = SpiInstance->PchSpiBase;
   PchSpiBar0        = AcquireSpiBar0 (SpiInstance);
-  SpiBaseAddress    = SpiInstance->PchSpiBase;
   ABase             = SpiInstance->PchAcpiBase;
 
   //
diff --git a/Silicon/Intel/SimicsIch10Pkg/SmmControl/RuntimeDxe/SmmControl2Dxe.c b/Silicon/Intel/SimicsIch10Pkg/SmmControl/RuntimeDxe/SmmControl2Dxe.c
index 268b04d25a..cc2d00b785 100644
--- a/Silicon/Intel/SimicsIch10Pkg/SmmControl/RuntimeDxe/SmmControl2Dxe.c
+++ b/Silicon/Intel/SimicsIch10Pkg/SmmControl/RuntimeDxe/SmmControl2Dxe.
+++ c
@@ -131,7 +131,6 @@ SmmControl2DxeTrigger (
   IN UINTN                            ActivationInterval OPTIONAL
   )
 {
-  EFI_STATUS  Status;
   //
   // No support for queued or periodic activation.
   //
@@ -141,7 +140,7 @@ SmmControl2DxeTrigger (
   ///
   /// Clear any pending the APM SMI
   ///
-  Status = SmmClear();
+  SmmClear();
   //
   // The so-called "Advanced Power Management Status Port Register" is in fact
   // a generic data passing register, between the caller and the SMI @@ -181,8 +180,6 @@ SmmControl2DxeClear (
   IN BOOLEAN                          Periodic OPTIONAL
   )
 {
-  EFI_STATUS  Status;
-
   if (Periodic) {
     return EFI_INVALID_PARAMETER;
   }
@@ -201,7 +198,7 @@ SmmControl2DxeClear (
   //
   // So, nothing to do here.
   //
-  Status = SmmClear();
+  SmmClear();
 
   return EFI_SUCCESS;
 }
--
2.19.1.windows.1