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

[Vikas Singh via groups.io]

On Tue, Jun 15, 2021 at 2:28 AM Leif Lindholm <leif at nuviainc.com> 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 at 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
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76756): https://edk2.groups.io/g/devel/message/76756
Mute This Topic: https://groups.io/mt/83471705/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-