From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ot1-f54.google.com (mail-ot1-f54.google.com [209.85.210.54]) by mx.groups.io with SMTP id smtpd.web11.8456.1624025797389690731 for ; Fri, 18 Jun 2021 07:16:37 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@puresoftware.com header.s=google header.b=axMcXOig; spf=pass (domain: puresoftware.com, ip: 209.85.210.54, mailfrom: vikas.singh@puresoftware.com) Received: by mail-ot1-f54.google.com with SMTP id 6-20020a9d07860000b02903e83bf8f8fcso9788185oto.12 for ; Fri, 18 Jun 2021 07:16:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=puresoftware.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=zfQM0nXHHrMKYkw0T899ZFkhP2OHf9Y2xNrq34gG5Y8=; b=axMcXOigwnRIeK4NmaNUavMxspZjo8OyclB8IHCc1/hEnanKnhUQ8V4jMK2ufF7tmI Np42mucJdtQ8moN45RZlN32yhEdKh4MdKsJlVbHENXaelyVoIxgAaGpdvpkdQo8jYZbN laZbGtEUJCDonb3jjAjdJejZme9VGQikZ0T44= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=zfQM0nXHHrMKYkw0T899ZFkhP2OHf9Y2xNrq34gG5Y8=; b=lUtoRgBv4H7y+htFWTqGt/CVBcJs+dji7bUfxsr1cOvJ488jBS3sXf501zBf1WrCUg MqjY6jKhGeOz2qucmxqYoDs1y6eXyl+WCJavhm/LK4kMfVnkSu+j1N51UK/FqIKd7DiR QT36cNA9edKqeDsEtpty2QJeot25dc0Ft4XtkQulDd9tVB7Zri1CZdflPDqWZFIKHIg7 mLsMYLSNsz7pBmuI4+OWerTmbDOQfGaNIVqwgdC4MrNZV5u59vOb/fCw/gNuZY4tJFu8 I27Ts5lp/MwEN8d9sotvyMNWJUZIjwitF60mjR9UGzBUpiNIXZlpqYrxumg28pu6QNP8 rhSA== X-Gm-Message-State: AOAM532vtIOIsLzl2iCoREtvRAp8ylA41ulY+B4suNgwiDeCrdSPR8FQ acWb6tjukGs6hZoPeNQ2crvhK+STQVCwxiOhQ7O7Aw== X-Google-Smtp-Source: ABdhPJwnlJgX2KO+CAvGCMb36oij9rgSxJOcijdiDKpZ3Xi0hDX7cZaR5I85jkmnKKHJEtLNMudVh77LEvu1kiw8aIA= X-Received: by 2002:a05:6830:33ef:: with SMTP id i15mr9664837otu.311.1624025796717; Fri, 18 Jun 2021 07:16:36 -0700 (PDT) MIME-Version: 1.0 References: <20210611155200.15535-1-vikas.singh@puresoftware.com> <20210611155200.15535-2-vikas.singh@puresoftware.com> <20210614205802.wbhlvqz65ttc7b4n@leviathan> In-Reply-To: <20210614205802.wbhlvqz65ttc7b4n@leviathan> From: "Vikas Singh" Date: Fri, 18 Jun 2021 19:46:10 +0530 Message-ID: Subject: Re: [PATCH V1 1/4] Platform/NXP: Add generic log in CM to print SoC version To: Leif Lindholm Cc: devel@edk2.groups.io, Sami Mujawar , meenakshi.aggarwal@nxp.com, Samer El-Haj-Mahmoud , Varun Sethi , Arokia Samy , Kuldip Dwivedi , ard.biesheuvel@arm.com, Vikas Singh , Sunny Wang Content-Type: text/plain; charset="UTF-8" On Tue, Jun 15, 2021 at 2:28 AM Leif Lindholm 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 > > --- > > 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 > >