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

["Leif Lindholm" <leif@nuviainc.com>]

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@puresoftware.com>
> ---
>  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
>