devel@edk2.groups.io | [PATCH V1 1/4] Platform/NXP: Add generic log in CM to print SoC version

Home Messages Hashtags Subgroups Calendar

Date Date 1 - 3 of 3

[PATCH V1 1/4] Platform/NXP: Add generic log in CM to print SoC version

Vikas Singh

Summary -
1.Configuration Manager(CM) is a common implementation
  and should not evaluate the SoC version using macro's
  However CM must directly consume SoC ver string from
  platfrom who is extending CM services for ACPI table
  generation.
2.Platforms who extends CM services for themselves must
  notify their SoC details to CM.
3.This patch will update the lx2160ardb platform header
  also with PLAT_SOC_NAME, this will be consumed by CM.

Signed-off-by: Vikas Singh <vikas.singh@...>
---
 Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/Configuration=
Manager.c | 10 +++-------
 Platform/NXP/LX2160aRdbPkg/Include/Platform.h                             =
          |  5 ++---
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/C=
onfigurationManager.c b/Platform/NXP/ConfigurationManagerPkg/ConfigurationM=
anagerDxe/ConfigurationManager.c
index 80ce8412c4..dc1a7f5f85 100644
--- a/Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/Configur=
ationManager.c
+++ b/Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/Configur=
ationManager.c
@@ -2,7 +2,7 @@
   Configuration Manager Dxe=0D
 =0D
   Copyright 2020 NXP=0D
-  Copyright 2020 Puresoftware Ltd=0D
+  Copyright 2020-2021 Puresoftware Ltd=0D
 =0D
   SPDX-License-Identifier: BSD-2-Clause-Patent=0D
 =0D
@@ -170,12 +170,8 @@ InitializePlatformRepository (
   PlatformRepo =3D This->PlatRepoInfo;=0D
 =0D
   Svr =3D SocGetSvr ();=0D
-  if (SVR_SOC_VER(Svr) =3D=3D SVR_LX2160A) {=0D
-    PlatformRepo->FslBoardRevision =3D SVR_MAJOR(Svr);=0D
-    DEBUG ((DEBUG_INFO, "Fsl : SoC LX2160A Rev =3D 0x%x\n", PlatformRepo->=
FslBoardRevision));=0D
-  } else {=0D
-    DEBUG ((DEBUG_INFO, "Fsl : SoC Unknown Rev =3D 0x%x\n", PlatformRepo->=
FslBoardRevision));=0D
-  }=0D
+  PlatformRepo->FslBoardRevision =3D SVR_MAJOR(Svr);=0D
+  DEBUG ((DEBUG_INFO, "Fsl : SoC =3D %s Rev =3D 0x%x\n", PLAT_SOC_NAME, Pl=
atformRepo->FslBoardRevision));=0D
 =0D
   return EFI_SUCCESS;=0D
 }=0D
diff --git a/Platform/NXP/LX2160aRdbPkg/Include/Platform.h b/Platform/NXP/L=
X2160aRdbPkg/Include/Platform.h
index 76a41d4369..c18faf28cd 100644
--- a/Platform/NXP/LX2160aRdbPkg/Include/Platform.h
+++ b/Platform/NXP/LX2160aRdbPkg/Include/Platform.h
@@ -2,7 +2,7 @@
  *  Platform headers=0D
  *=0D
  *  Copyright 2020 NXP=0D
- *  Copyright 2020 Puresoftware Ltd=0D
+ *  Copyright 2020-2021 Puresoftware Ltd=0D
  *=0D
  *  SPDX-License-Identifier: BSD-2-Clause-Patent=0D
  *=0D
@@ -15,12 +15,11 @@
 #define EFI_ACPI_ARM_OEM_REVISION       0x00000000=0D
 =0D
 // Soc defines=0D
+#define PLAT_SOC_NAME           "LX2160ARDB"=0D
 #define SVR_SOC_VER(svr)        (((svr) >> 8) & 0xFFFFFE)=0D
 #define SVR_MAJOR(svr)          (((svr) >> 4) & 0xf)=0D
 #define SVR_MINOR(svr)          (((svr) >> 0) & 0xf)=0D
 =0D
-#define SVR_LX2160A             0x873600=0D
-=0D
 // PCLK=0D
 #define DCFG_BASE   0x1E00000=0D
 #define DCFG_LEN    0x1FFFF=0D
--=20
2.25.1

Leif Lindholm <leif@...>

#76486

On Fri, Jun 11, 2021 at 21:21:57 +0530, Vikas Singh wrote:
Summary -
1.Configuration Manager(CM) is a common implementation
  and should not evaluate the SoC version using macro's
  However CM must directly consume SoC ver string from
  platfrom who is extending CM services for ACPI table
  generation.This tells me nothing about what this patch does.

2.Platforms who extends CM services for themselves must
  notify their SoC details to CM.Neither does this.

3.This patch will update the lx2160ardb platform header
  also with PLAT_SOC_NAME, this will be consumed by CM.And this sound like it should be a separate patch.

*However* when I look at the code, this does look like a single
change. And what is descibed as point 3 is the actual change in the patch.
Moreover, this patch addresses a historic horror, in that SVR_LX2160A
was defined both in the platform header and in the SoC header (which
I'm not saying you had necessarily noticed, but I am suggesting it is
added to the message).

If I wrote this commit message, I would start with a slightly tweaked
subject line:

Platform/NXP: Make SoC version log in ConfigurationManager generic

(CM is not a recognised abbreviation, so should be printed expanded)

As for the body, I would say something like:

This patch replaces the logic in ConfigurationManager to print
platform name based on platform ID with a simple #define.
This also removes a duplication of the SVR_LX2160A definition between
SoC and platform headers.

No comments on the code itself.

/
    Leif

Signed-off-by: Vikas Singh <vikas.singh@...>
---
 Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c | 10 +++-------
 Platform/NXP/LX2160aRdbPkg/Include/Platform.h                                       |  5 ++---
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c b/Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c
index 80ce8412c4..dc1a7f5f85 100644
--- a/Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c
+++ b/Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c
@@ -2,7 +2,7 @@
   Configuration Manager Dxe
 
   Copyright 2020 NXP
-  Copyright 2020 Puresoftware Ltd
+  Copyright 2020-2021 Puresoftware Ltd
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -170,12 +170,8 @@ InitializePlatformRepository (
   PlatformRepo = This->PlatRepoInfo;
 
   Svr = SocGetSvr ();
-  if (SVR_SOC_VER(Svr) == SVR_LX2160A) {
-    PlatformRepo->FslBoardRevision = SVR_MAJOR(Svr);
-    DEBUG ((DEBUG_INFO, "Fsl : SoC LX2160A Rev = 0x%x\n", PlatformRepo->FslBoardRevision));
-  } else {
-    DEBUG ((DEBUG_INFO, "Fsl : SoC Unknown Rev = 0x%x\n", PlatformRepo->FslBoardRevision));
-  }
+  PlatformRepo->FslBoardRevision = SVR_MAJOR(Svr);
+  DEBUG ((DEBUG_INFO, "Fsl : SoC = %s Rev = 0x%x\n", PLAT_SOC_NAME, PlatformRepo->FslBoardRevision));
 
   return EFI_SUCCESS;
 }
diff --git a/Platform/NXP/LX2160aRdbPkg/Include/Platform.h b/Platform/NXP/LX2160aRdbPkg/Include/Platform.h
index 76a41d4369..c18faf28cd 100644
--- a/Platform/NXP/LX2160aRdbPkg/Include/Platform.h
+++ b/Platform/NXP/LX2160aRdbPkg/Include/Platform.h
@@ -2,7 +2,7 @@
  *  Platform headers
  *
  *  Copyright 2020 NXP
- *  Copyright 2020 Puresoftware Ltd
+ *  Copyright 2020-2021 Puresoftware Ltd
  *
  *  SPDX-License-Identifier: BSD-2-Clause-Patent
  *
@@ -15,12 +15,11 @@
 #define EFI_ACPI_ARM_OEM_REVISION       0x00000000
 
 // Soc defines
+#define PLAT_SOC_NAME           "LX2160ARDB"
 #define SVR_SOC_VER(svr)        (((svr) >> 8) & 0xFFFFFE)
 #define SVR_MAJOR(svr)          (((svr) >> 4) & 0xf)
 #define SVR_MINOR(svr)          (((svr) >> 0) & 0xf)
 
-#define SVR_LX2160A             0x873600
-
 // PCLK
 #define DCFG_BASE   0x1E00000
 #define DCFG_LEN    0x1FFFF
-- 
2.25.1

Vikas Singh

#76756

On Tue, Jun 15, 2021 at 2:28 AM Leif Lindholm <leif@...> wrote:

On Fri, Jun 11, 2021 at 21:21:57 +0530, Vikas Singh wrote:
Summary -
1.Configuration Manager(CM) is a common implementation
  and should not evaluate the SoC version using macro's
  However CM must directly consume SoC ver string from
  platfrom who is extending CM services for ACPI table
  generation.This tells me nothing about what this patch does.

2.Platforms who extends CM services for themselves must
  notify their SoC details to CM.Neither does this.

3.This patch will update the lx2160ardb platform header
  also with PLAT_SOC_NAME, this will be consumed by CM.And this sound like it should be a separate patch.

*However* when I look at the code, this does look like a single
change. And what is descibed as point 3 is the actual change in the patch.
Moreover, this patch addresses a historic horror, in that SVR_LX2160A
was defined both in the platform header and in the SoC header (which
I'm not saying you had necessarily noticed, but I am suggesting it is
added to the message).

If I wrote this commit message, I would start with a slightly tweaked
subject line:

Platform/NXP: Make SoC version log in ConfigurationManager generic

(CM is not a recognised abbreviation, so should be printed expanded)

As for the body, I would say something like:

This patch replaces the logic in ConfigurationManager to print
platform name based on platform ID with a simple #define.
This also removes a duplication of the SVR_LX2160A definition between
SoC and platform headers.

No comments on the code itself.

/
    Leif
Leif, I will do the suggested changes in subject and body of the commit.
Additionally, I will remove all the SVR_*  duplicates from the platform headers.
Will try to reuse the SVR_* definitions from the SoC headers itself.

I will share the updated V2 series shortly.

Signed-off-by: Vikas Singh <vikas.singh@...>
---
 Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c | 10 +++-------
 Platform/NXP/LX2160aRdbPkg/Include/Platform.h                                       |  5 ++---
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c b/Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c
index 80ce8412c4..dc1a7f5f85 100644
--- a/Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c
+++ b/Platform/NXP/ConfigurationManagerPkg/ConfigurationManagerDxe/ConfigurationManager.c
@@ -2,7 +2,7 @@
   Configuration Manager Dxe

   Copyright 2020 NXP
-  Copyright 2020 Puresoftware Ltd
+  Copyright 2020-2021 Puresoftware Ltd

   SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -170,12 +170,8 @@ InitializePlatformRepository (
   PlatformRepo = This->PlatRepoInfo;

   Svr = SocGetSvr ();
-  if (SVR_SOC_VER(Svr) == SVR_LX2160A) {
-    PlatformRepo->FslBoardRevision = SVR_MAJOR(Svr);
-    DEBUG ((DEBUG_INFO, "Fsl : SoC LX2160A Rev = 0x%x\n", PlatformRepo->FslBoardRevision));
-  } else {
-    DEBUG ((DEBUG_INFO, "Fsl : SoC Unknown Rev = 0x%x\n", PlatformRepo->FslBoardRevision));
-  }
+  PlatformRepo->FslBoardRevision = SVR_MAJOR(Svr);
+  DEBUG ((DEBUG_INFO, "Fsl : SoC = %s Rev = 0x%x\n", PLAT_SOC_NAME, PlatformRepo->FslBoardRevision));

   return EFI_SUCCESS;
 }
diff --git a/Platform/NXP/LX2160aRdbPkg/Include/Platform.h b/Platform/NXP/LX2160aRdbPkg/Include/Platform.h
index 76a41d4369..c18faf28cd 100644
--- a/Platform/NXP/LX2160aRdbPkg/Include/Platform.h
+++ b/Platform/NXP/LX2160aRdbPkg/Include/Platform.h
@@ -2,7 +2,7 @@
  *  Platform headers
  *
  *  Copyright 2020 NXP
- *  Copyright 2020 Puresoftware Ltd
+ *  Copyright 2020-2021 Puresoftware Ltd
  *
  *  SPDX-License-Identifier: BSD-2-Clause-Patent
  *
@@ -15,12 +15,11 @@
 #define EFI_ACPI_ARM_OEM_REVISION       0x00000000

 // Soc defines
+#define PLAT_SOC_NAME           "LX2160ARDB"
 #define SVR_SOC_VER(svr)        (((svr) >> 8) & 0xFFFFFE)
 #define SVR_MAJOR(svr)          (((svr) >> 4) & 0xf)
 #define SVR_MINOR(svr)          (((svr) >> 0) & 0xf)

-#define SVR_LX2160A             0x873600
-
 // PCLK
 #define DCFG_BASE   0x1E00000
 #define DCFG_LEN    0x1FFFF
--
2.25.1

1 - 3 of 3

Previous Topic Next Topic

Home Hashtags Subgroups Calendar About Features Pricing Updates Terms Help Twitter