From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f44.google.com (mail-wm1-f44.google.com [209.85.128.44]) by mx.groups.io with SMTP id smtpd.web10.529.1623704286437133694 for ; Mon, 14 Jun 2021 13:58:06 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=tZldKTif; spf=pass (domain: nuviainc.com, ip: 209.85.128.44, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f44.google.com with SMTP id b205so8162081wmb.3 for ; Mon, 14 Jun 2021 13:58:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=bsyCa5sRyjVso7yJU8ScmEQjUNWpbsmZBtqHM7wbKh0=; b=tZldKTifMq5NGsTMdeWEAkTcSGx5kS+KRxunwWAFXYTFJzqEOceEdSyXgKcqWReers VeNIOVlVP7H5pi5iV/i9HlcAJJibcgrlTeCEUhUALLYJEU4yqymWZJTGdkj2e+Daw+Su 5uEeu0IoQbUrKFf9AAwL9pAnoZKV2U2IK4/BnshfwcpaFObV5hnNNKNjhnlZgFpmYT4D OuknphizGAdll3VtkxckYIJ5dKRWe5HczO/N6GrqGzvOcJS2pBJxmv72U2KZ7Ue/jfsE 21rdOKf0ic39VsvOD8g8ozULudLrSn31zxF/nx2lKDqH+cB9Z1IyMdE8l/r/qq+P0tOi PdfQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=bsyCa5sRyjVso7yJU8ScmEQjUNWpbsmZBtqHM7wbKh0=; b=ZFE6MeRlWM0tB3HwgpQQFjzCRTKQTkSm5Z9IJuRr/8ep60YE/ClX98ZS9mUtmXp3bh AMW+ytSUbJzjWD8Mk9aT3lxiOMTyWoClaMVxonj3Ha4wSMYQMqG9d5sA9MEa7XWmA1fw H4ue2njboMibSurlvAK/ag4ZQw35EEgTUcQXgK7TBWfBlQPP1iYO9La1h1BqoPPixB8a aL27lqk/nIqRX1Ul1cjxl9nLDktrJk0iTxHsvHPgk16ImMlKNu6htiDCfTDx1GaWCEly 2P7ajVxZArTnRHwWNU761/QBAzhrSuG828srNwaGgalXofhyp7S2XlyezESp2Lu36oLE QBRQ== X-Gm-Message-State: AOAM5303KMNXJDxycFamdm9sCunqQha6g5Br0Kw9s/7rsYNy4oGqPTXN 441I+ejh03ENhu5633XWgppQZA== X-Google-Smtp-Source: ABdhPJyLkjhXsoFhXOydOPZm7K1e7yVPqIIQOhnzRA1AVkzs011HLpEor0wVxGhwTzz8l3BczhbJpw== X-Received: by 2002:a7b:ce8a:: with SMTP id q10mr18669500wmj.184.1623704284877; Mon, 14 Jun 2021 13:58:04 -0700 (PDT) Return-Path: Received: from leviathan (cpc1-cmbg19-2-0-cust915.5-4.cable.virginm.net. [82.27.183.148]) by smtp.gmail.com with ESMTPSA id g21sm17640307wrb.46.2021.06.14.13.58.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 14 Jun 2021 13:58:04 -0700 (PDT) Date: Mon, 14 Jun 2021 21:58:02 +0100 From: "Leif Lindholm" To: Vikas Singh Cc: devel@edk2.groups.io, sami.mujawar@arm.com, meenakshi.aggarwal@nxp.com, samer.el-haj-mahmoud@arm.com, v.sethi@nxp.com, arokia.samy@puresoftware.com, kuldip.dwivedi@puresoftware.com, ard.biesheuvel@arm.com, vikas.singh@nxp.com, Sunny.Wang@arm.com Subject: Re: [PATCH V1 1/4] Platform/NXP: Add generic log in CM to print SoC version Message-ID: <20210614205802.wbhlvqz65ttc7b4n@leviathan> References: <20210611155200.15535-1-vikas.singh@puresoftware.com> <20210611155200.15535-2-vikas.singh@puresoftware.com> MIME-Version: 1.0 In-Reply-To: <20210611155200.15535-2-vikas.singh@puresoftware.com> Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > --- > 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 >