On Tue, Jun 15, 2021 at 2:28 AM Leif Lindholm <l...@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.si...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
