From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail05.groups.io (mail05.groups.io [45.79.224.7]) by spool.mail.gandi.net (Postfix) with ESMTPS id 024C0740035 for ; Thu, 16 May 2024 15:23:22 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=bdbA2ondkzHksUnk7G0D8wMoeqs5zt8w1bynbs98c20=; c=relaxed/simple; d=groups.io; h=Received-SPF:Authentication-Results-Original:Message-ID:Date:User-Agent:Subject:To:Cc:References:From:In-Reply-To:MIME-Version:NoDisclaimer:Original-Authentication-Results:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Language; s=20240206; t=1715873001; v=1; b=enqdiEfBAsfXOLMQKK9SD+X4U8FiFB+4Hpu+wbJg3QN5fyuEFGD2WraBBSTVHuMY8IRRiXGp w4kYJddkSZXBWQZdT4CDaM7B5HkubBANvRMUB7oY2Im5KPEejEDqDcIJDwRv7jDOd43pmedOQch E7rQZPncvOWUE0Ax7gn96JJQNhhU65NKAkSv8zjAkAG6dpFQLBolPFZFMQFX3++0+WY0rqlZAno RRw+uy6bZ8+Zm//EkRZmMPxuWTJpqLVlQDviK9TUM2qsID+uu/khroFf67GDCGzcwvTOZRH74fd 2vBHkhsGDo5CtjrQmBkgSay88lPGCVdzJDTuzlXyV5jjA== X-Received: by 127.0.0.2 with SMTP id aL3fYY7687511xcaYqrAXJN8; Thu, 16 May 2024 08:23:21 -0700 X-Received: from EUR05-VI1-obe.outbound.protection.outlook.com (EUR05-VI1-obe.outbound.protection.outlook.com [40.107.21.83]) by mx.groups.io with SMTP id smtpd.web11.17038.1715873000057781064 for ; Thu, 16 May 2024 08:23:20 -0700 X-Received: from AM6PR0202CA0070.eurprd02.prod.outlook.com (2603:10a6:20b:3a::47) by GVXPR08MB10405.eurprd08.prod.outlook.com (2603:10a6:150:14c::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7587.27; Thu, 16 May 2024 15:23:15 +0000 X-Received: from AM2PEPF0001C717.eurprd05.prod.outlook.com (2603:10a6:20b:3a:cafe::f1) by AM6PR0202CA0070.outlook.office365.com (2603:10a6:20b:3a::47) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7587.27 via Frontend Transport; Thu, 16 May 2024 15:23:15 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 63.35.35.123) smtp.mailfrom=arm.com; dkim=pass (signature was verified) header.d=arm.com;dmarc=pass action=none header.from=arm.com; Received-SPF: Pass (protection.outlook.com: domain of arm.com designates 63.35.35.123 as permitted sender) receiver=protection.outlook.com; client-ip=63.35.35.123; helo=64aa7808-outbound-1.mta.getcheckrecipient.com; pr=C X-Received: from 64aa7808-outbound-1.mta.getcheckrecipient.com (63.35.35.123) by AM2PEPF0001C717.mail.protection.outlook.com (10.167.16.187) with Microsoft SMTP Server (version=TLS1_3, cipher=TLS_AES_256_GCM_SHA384) id 15.20.7587.21 via Frontend Transport; Thu, 16 May 2024 15:23:15 +0000 X-Received: ("Tessian outbound ba75727f6dca:v315"); Thu, 16 May 2024 15:23:15 +0000 X-CheckRecipientChecked: true X-CR-MTA-CID: 03a77a2a94820f0f X-CR-MTA-TID: 64aa7808 X-Received: from 5a8c787ecec2.1 by 64aa7808-outbound-1.mta.getcheckrecipient.com id 1744810A-8EE0-49E0-9ECF-AA3633D72960.1; Thu, 16 May 2024 15:23:08 +0000 X-Received: from EUR04-VI1-obe.outbound.protection.outlook.com by 64aa7808-outbound-1.mta.getcheckrecipient.com with ESMTPS id 5a8c787ecec2.1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384); Thu, 16 May 2024 15:23:08 +0000 Authentication-Results-Original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; X-Received: from AS8PR08MB6806.eurprd08.prod.outlook.com (2603:10a6:20b:39b::12) by AS8PR08MB9071.eurprd08.prod.outlook.com (2603:10a6:20b:5c1::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7587.27; Thu, 16 May 2024 15:23:05 +0000 X-Received: from AS8PR08MB6806.eurprd08.prod.outlook.com ([fe80::1e13:dc65:224e:219c]) by AS8PR08MB6806.eurprd08.prod.outlook.com ([fe80::1e13:dc65:224e:219c%5]) with mapi id 15.20.7587.028; Thu, 16 May 2024 15:23:05 +0000 Message-ID: <325de502-26ae-4a2d-aecd-f1252fc395f0@arm.com> Date: Thu, 16 May 2024 16:23:04 +0100 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 09/14] Platform/ARM: Add optional provision to fetch and print NOR Flash info To: Sahil Kaushal , devel@edk2.groups.io Cc: Ard Biesheuvel , Leif Lindholm , sahil , "nd@arm.com" References: <20240423055638.1271531-1-Sahil.Kaushal@arm.com> <20240423055638.1271531-10-Sahil.Kaushal@arm.com> From: "Sami Mujawar" In-Reply-To: <20240423055638.1271531-10-Sahil.Kaushal@arm.com> X-ClientProxiedBy: LO4P123CA0154.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600:188::15) To AS8PR08MB6806.eurprd08.prod.outlook.com (2603:10a6:20b:39b::12) MIME-Version: 1.0 X-MS-TrafficTypeDiagnostic: AS8PR08MB6806:EE_|AS8PR08MB9071:EE_|AM2PEPF0001C717:EE_|GVXPR08MB10405:EE_ X-MS-Office365-Filtering-Correlation-Id: 407abd51-43d5-4612-5728-08dc75bc1b78 x-checkrecipientrouted: true NoDisclaimer: true X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam-Untrusted: BCL:0;ARA:13230031|366007|1800799015|376005; X-Microsoft-Antispam-Message-Info-Original: =?us-ascii?Q?T3NdV+ACcmmRJ4wu3P5PAJALsBvKP0M+Je7bQMdIxiZeXPMMETh60DHqALwT?= =?us-ascii?Q?otT/lmRgYxknsW2VGk7lZft7qtqZ4KzizgPC3LRL1HX/G1nAqd3rgIQnxrrv?= =?us-ascii?Q?s0s3cLLpE7UAdJh2Wos5YdXAl6edbho+YlqIBn4BhMIgsLwXi9r4fXnReuh8?= =?us-ascii?Q?RpOjasdN6A/Gl7YufxnViaesvkZ+1ymB0gV1kBhTXNC7Hl6SJMqr8mmnhj2f?= =?us-ascii?Q?/tEXh27YDDJZyUQ3TgL6OWxmWI0s8OotmdsVSNMs0+bH5b7WyNTKaqw7ZEPK?= =?us-ascii?Q?gf/Ihwg95O9z1Agx9cI4Ym/XwMkiCo202O4or/Uu1FyBUKDHbzSdTANEu890?= =?us-ascii?Q?VD83l3X5Piejaj9EzyYxzoJV6OMyEYEuFJKVVj8l0S/Yy/zNhSAzczFWD4BW?= =?us-ascii?Q?ScrC9xV7BvfeY9qg8lbF7dUMa3FoPB7vBkkrdlGVWx/wmRwd59DwIepo9ylW?= =?us-ascii?Q?5K9tSaT2hDMNxT5Bq0uRP1E8DV8OgF1W5FIMBViYNp9tYjewYmqlNE0QQ+Kn?= =?us-ascii?Q?U7ReknceP66nnS+zuShsov9qRjPRCCmjWR4KSl1don2yxJuOf8iZdmVRVz2H?= =?us-ascii?Q?JqDRi4WOXmZjamJPGJTStRkN6UwGxMbqOAoVYSz705sFV7cpf0vzIYdK4cFA?= =?us-ascii?Q?H1osBfHjy560+C2Ht/8UD31NqxbLttaQNJmJxwxDnulYxF4VWsv5IQ7OD6xF?= =?us-ascii?Q?09n3Si0rEAh55MVdutOZvMVDeeYSyojYHj5zIPuIiPAUo3LBwwLPBncIbmAl?= =?us-ascii?Q?Sz4XtD9D+cdB5AMoelxamYxaBMccHxa76Is7DW44/fd5E9vWonRmnrJtM3q8?= =?us-ascii?Q?PO5drSZNEUOFKcXsqou4LPPJMusyH+n5dmDKP90jPm41rMaAy9Cg1T+1DdJz?= =?us-ascii?Q?ot7NTuxp4G7f0mMyLG1VA6tH8h0V0ZuNIhRqb3HMzOZrdzSa2nbI63Rh1/+v?= =?us-ascii?Q?ngD4j+yU46S3CX6J4HaUkfTLK2/aHxfS26Lz8wILmZP4Byu+H9gi3y5nIdMZ?= =?us-ascii?Q?HRiwlXl+DQv2iNT++haCMwZ/7qz588oMZ65LxtnRpBwT9JzFHYh+rAKkG7sK?= =?us-ascii?Q?cm44i1j13Gz3uayNjhLN8h4FU2knsjpDx5jqPO/CUB/71KjojCWWPfZkli07?= =?us-ascii?Q?4v1FOtqCGFr3W3sh9ZuM7yzGvrGZ5TFN7EGDot5J9YlssNm2kW8feKlLyuR+?= =?us-ascii?Q?eN7Ph0nqc5OI1LgfmyBdybqhWAc6Hc9P2cXclE07JcyNcKZwTIF71YF0TNZ3?= =?us-ascii?Q?3vP974n1ILytUjuFYel2qde4+CULd3vz/odRSDW0DQ=3D=3D?= X-Forefront-Antispam-Report-Untrusted: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:AS8PR08MB6806.eurprd08.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(366007)(1800799015)(376005);DIR:OUT;SFP:1101; X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR08MB9071 Original-Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; X-EOPAttributedMessage: 0 X-MS-Exchange-Transport-CrossTenantHeadersStripped: AM2PEPF0001C717.eurprd05.prod.outlook.com X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id-Prvs: efdb30eb-35db-45d7-97cd-08dc75bc1567 X-Microsoft-Antispam-Message-Info: =?utf-8?B?KzZWcExYWWMyNCs3VUtRcUpiRVBBOGwxTm1LRUlwNkljY0JqenZOV2t1Slhp?= =?utf-8?B?YVlDdDJzdytPczdVYVlydUlWTjZFRmxvWnBZN2NnVTMvRGtnRElJbSt0SUNY?= =?utf-8?B?TFpGZHRITzZodkQvZng5V1g0RUJaYUJzZEhEdDJBMXpKQStqVm1VTVMvMVha?= =?utf-8?B?ODB6bXhtYnlHVHh4aHBtOXdxQTNYVnNtRlUrMDUvdjZXTDNYdFNzNVF0bWZC?= =?utf-8?B?clppRzhpNWF3czlsV2VnV3ppUUhieGwyK1BBTENLQnp3c1Yza0p0YVZUaHRL?= =?utf-8?B?QjFDejBlaG11aUhwRW1RMTE4MWNnazdvcXdTdEd3TmRFRXVCWXJTNE0vZ0d3?= =?utf-8?B?MjM2ZU1kRVZFWkxWU3Zmb1lGTkR4eWR1bitZVHR1Z1FpZXRHYmhvSDloZ0gy?= =?utf-8?B?RVRLRVpQQU5pa0MrWFlmMWV5MjBKaExVWXVnYmFyajdVQnZVSU9iZmxsT1F1?= =?utf-8?B?Q2pHWFVOOWcxOW9LVzVISGNRRlVLOVh1cW5VRWpKQklFdjNRTXZPbUVtd3N1?= =?utf-8?B?RjUrYUwxalBrNzk1VjhjQlBqLzdyQno3QWtKSDNyczVtRUpHMzFJZFRjTU8r?= =?utf-8?B?RnYxMTlGbTRmQ2NYUU1pMDY1cVJUUlZmQTd5UXN1ZGppVXhGN0JYT28zd3c4?= =?utf-8?B?dkpWbFlTWGlCYmpra2xDZzBrWlcwKzJtUUsvczhXVWU3S2tHOThHYXhQOVBw?= =?utf-8?B?bUVYSTlab0JtdG40RFhMTTJrVHR2c0w4Z3lpM1RSTzc3ZWJNU2ZwQlJtTkcx?= =?utf-8?B?Rkw2eHh2Q1dSZmxKUnRBRVpzc3NibXhrMGkvYWxrYm8vVlhDWkJXQ3NOV2wz?= =?utf-8?B?YnBPcU1IMm5NMDk4UmlQOXBGTndWNXlVclpITThIa2lDVmpOd2VSaW5oaXN3?= =?utf-8?B?SWR6cng4MUV1R3J6SXR0ZGl2bGhZMy82WGVLb0oxaFo5UUVMMFpBSDdtQVZJ?= =?utf-8?B?T0tzOVpXMkMyTDFxYVNZQkRpMTRydlhGNExkUkpRM0Ewc3ltS2s0WERzS2ZD?= =?utf-8?B?bEpDa1k0d0U2S2VwZ2xzUDEvS0NnUnR0cTUyR29lV2FOcU4zVnBpeS9jTDhG?= =?utf-8?B?V3B1T2VIcUljQ1VSRk5oS3hJRmkzQlJZSnpqanhLdzdycFlDN2pDT0xwTTM0?= =?utf-8?B?cjVYRnlEd1dsaVQ5Z1FUL2VuRFphNjRlTjh3SThZd1dwRkx0UmVNMEdwVFFm?= =?utf-8?B?L2xhMldCREFBVzBDc3B5eXI0TEN5U0p3WG81SG5RWVhVaEZPZk1OUkE3RHBp?= =?utf-8?B?a0luRFlFclpzb2hIcDNNeWgrVWFCT0tMTHY5M3J4UFh2SEFmUjZkR2N5VHE3?= =?utf-8?B?U042V1JqWE9CUFdMQks4UHNwUzJmbTdJN2h3M0xpc1QvZTZnNFE2bzJ5Z0Mw?= =?utf-8?B?QXpPNkgzeWNJNDhpZ3F1WkJLaXdXNWN4MzVJVEpweGdGc0xRaVhGejFqWXM5?= =?utf-8?B?TDhjYmFWVldIVXB0clFTaDhYc3htWHZ1ZXRkbWoyd2o1WVhuUURNOHY2c2po?= =?utf-8?B?SzNWWnNVWXZnZHRHTFB6aWRGMElKSzQ5K0FOaEVmcVlaT1dIa2pMSHFmTGpq?= =?utf-8?B?VlJBOFVXZGMrQjNuUC9UVjhzdkFSd0FFa2daRXAwZkp6SEl4Ym1FSlJFa1V1?= =?utf-8?B?ZTdtem45TExGYVJuYVEwVFNQTERvUkpmbFQvWTVEMDBFMWVFVG95WFhFUzFr?= =?utf-8?B?ZXAwb3k5RGVMbHJWUVVNYk4rYmZESDhLdUFyaUgxUXdwdTZLeXI3clJtSnVo?= =?utf-8?B?anpvblYrRmpqd0QzY0x1aGhhSzRHcE9tVmhZUGV1OUorTWVDejMzbnhkdjRE?= =?utf-8?B?cUUvSDNZYUlMMldyZEQxWjVMbGFUZmo2bk1uL1JPSTFoWUhKa2xqTEtEUDFI?= =?utf-8?Q?iTp0b5YpROfts?= X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 May 2024 15:23:15.4159 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 407abd51-43d5-4612-5728-08dc75bc1b78 X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=f34e5979-57d9-4aaa-ad4d-b122a662184d;Ip=[63.35.35.123];Helo=[64aa7808-outbound-1.mta.getcheckrecipient.com] X-MS-Exchange-CrossTenant-AuthSource: AM2PEPF0001C717.eurprd05.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: GVXPR08MB10405 Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Resent-Date: Thu, 16 May 2024 08:23:20 -0700 Resent-From: sami.mujawar@arm.com Reply-To: devel@edk2.groups.io,sami.mujawar@arm.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: jJaf43TZaCZLBXkK4AEshYATx7686176AA= Content-Type: multipart/alternative; boundary="------------rd2Dt4WERQd8vvS0TyFmAkXQ" Content-Language: en-GB X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=enqdiEfB; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=arm.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.7 as permitted sender) smtp.mailfrom=bounce@groups.io --------------rd2Dt4WERQd8vvS0TyFmAkXQ Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Sahil, Thank you for this patch. Please find my feedback inline marked [SAMI]. Regards, Sami Mujawarnd On 23/04/2024 06:56 am, Sahil Kaushal wrote: > From: sahil > > This patch adds an optional functionality in NorFlashDxe to fetch and > print NOR Flash information from NorFlashInfoLib using its JEDEC ID. > > NOR Flash libraries will implement a function "NorFlashReadID" which > will fetch and return JEDEC ID. This JEDEC ID can be then used to > print NOR Flash info using NorFlashInfoLib. If this functionality is [SAMI] Can you explain how this information is useful, please? Is it just for debugging or it can be used for some other purpose. If it is just for printing the information, then maybe the above sentence could be silghtly modified, e.g. This JEDEC ID can be then printed along with the NOR Flash info by NorFlashInfoLib. [/SAMI] > not needed then the function can just return EFI_UNSUPPORTED. > > Signed-off-by: sahil > --- > Platform/ARM/SgiPkg/SgiPlatform.dsc.inc | 2 ++ > Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc | 2 ++ > Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc | 2 ++ > Platform/ARM/JunoPkg/ArmJuno.dsc | 2 ++ > Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc | 2 ++ > Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf | 1 + > Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf | 1 + > Platform/ARM/Include/Library/NorFlashDeviceLib.h | 6 ++++++ > Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c | 19 +++++++++++++++++++ > Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c | 19 +++++++++++++++++++ > Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c | 18 ++++++++++++++++++ > 11 files changed, 74 insertions(+) > > diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc > index 3dcf422eab4b..aef7cba5449e 100644 > --- a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc > +++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc > @@ -36,6 +36,8 @@ > LcdPlatformLib|Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgiLib.inf > > NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf > > NorFlashPlatformLib|Platform/ARM/SgiPkg/Library/NorFlashLib/NorFlashLib.inf > > + # NOR flash support > > + NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf > > TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf > > diff --git a/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc b/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc > index ab0e2a957a1b..02d684adaebd 100644 > --- a/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc > +++ b/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc > @@ -65,6 +65,8 @@ > IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf > > NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf > > NorFlashPlatformLib|Platform/ARM/SgiPkg/Library/NorFlashLib/StandaloneMmNorFlashLib.inf > > + # NOR flash support > > + NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf > > OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf > > RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf > > PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf > > diff --git a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc > index 70ff049d3248..4e208c539a88 100644 > --- a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc > +++ b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc > @@ -95,6 +95,8 @@ > ArmPlatformSysConfigLib|Platform/ARM/VExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf > > NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf > > NorFlashPlatformLib|Platform/ARM/VExpressPkg/Library/NorFlashArmVExpressLib/NorFlashArmVExpressLib.inf > > + # NOR flash support > > + NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf > > ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf > > > > # ARM PL031 RTC Driver > > diff --git a/Platform/ARM/JunoPkg/ArmJuno.dsc b/Platform/ARM/JunoPkg/ArmJuno.dsc > index 81d2cbe4359f..946b8680c8c2 100644 > --- a/Platform/ARM/JunoPkg/ArmJuno.dsc > +++ b/Platform/ARM/JunoPkg/ArmJuno.dsc > @@ -42,6 +42,8 @@ > > > NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf > > NorFlashPlatformLib|Platform/ARM/JunoPkg/Library/NorFlashJunoLib/NorFlashJunoLib.inf > > + # NOR flash support > > + NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf > > > > CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf > > CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf > > diff --git a/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc b/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc > index a5805da49c92..ee71bbb1fc09 100644 > --- a/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc > +++ b/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc > @@ -102,6 +102,8 @@ > !if $(ENABLE_UEFI_SECURE_VARIABLE) == TRUE > > NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf > > NorFlashPlatformLib|Platform/ARM/VExpressPkg/Library/NorFlashArmVExpressLib/NorFlashStMmLib.inf > > + # NOR flash support [SAMI] Nit. Should the above comment be 'NOR flash identification support'? Same comment for other places in this patch. > > + NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf > > VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf > > VariableFlashInfoLib|MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.inf > > AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf > > diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf > index 6522968d6c5a..4ab4d6a26926 100644 > --- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf > +++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf > @@ -35,6 +35,7 @@ > HobLib > > IoLib > > NorFlashDeviceLib > > + NorFlashInfoLib > > NorFlashPlatformLib > > UefiBootServicesTableLib > > UefiDriverEntryPoint > > diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf > index eb86d423f106..8b583f77d927 100644 > --- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf > +++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf > @@ -37,6 +37,7 @@ > MemoryAllocationLib > > MmServicesTableLib > > NorFlashDeviceLib > > + NorFlashInfoLib > > NorFlashPlatformLib > > StandaloneMmDriverEntryPoint > > > > diff --git a/Platform/ARM/Include/Library/NorFlashDeviceLib.h b/Platform/ARM/Include/Library/NorFlashDeviceLib.h > index 29b8b8901525..1e61f7c935ae 100644 > --- a/Platform/ARM/Include/Library/NorFlashDeviceLib.h > +++ b/Platform/ARM/Include/Library/NorFlashDeviceLib.h > @@ -154,4 +154,10 @@ NorFlashUnlock ( > IN EFI_TPL OriginalTPL > > ); > > > > +EFI_STATUS > > +NorFlashReadID ( > > + IN NOR_FLASH_INSTANCE *Instance, > > + OUT UINT8 *JedecId // Maximum length of JedecId can be upto 6 bytes. > > + ); [SAMI] You have the documentation header already in P30NorFlashDeviceLib.. Can you put that in the header file here as well, please? > > + > > #endif /* NOR_FLASH_DEVICE_LIB_H_ */ > > diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c > index f5c0dadf84e0..1084f4731ce4 100644 > --- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c > +++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c > @@ -9,6 +9,7 @@ > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -112,6 +113,8 @@ NorFlashCreateInstance ( > { > > EFI_STATUS Status; > > NOR_FLASH_INSTANCE *Instance; > > + NOR_FLASH_INFO *FlashInfo; > > + UINT8 JedecId[6]; > > > > ASSERT (NorFlashInstance != NULL); > > > > @@ -138,6 +141,22 @@ NorFlashCreateInstance ( > return EFI_OUT_OF_RESOURCES; > > } > > > > + Status = NorFlashReadID (Instance, JedecId); > > + if (EFI_ERROR (Status) && (Status != EFI_UNSUPPORTED)) { > > + FreePool (Instance); [SAMI] What about freeing the Instance->ShadowBuffer buffer? This issue appears to be present even before your changes. Can you add another patch before patch 9/14 to fix the memory leak in this funciton, please? I think it may be better to handle the error at the end of the function and cleanup the resources allocated, i.e. implement an exit_handler<1,2> label  as needed that frees up the resources and add goto exit_handler<1,2> from whereever the error occurs, setting up the Status appropriately to be returned at the end of the function. > > + return Status; > > + } > > + > > + if (Status == EFI_SUCCESS) { > > + Status = NorFlashGetInfo (JedecId, &FlashInfo, TRUE); [SAMI] It looks like the memory allocated for FlashInfo is leaked. Also do you need the memory allocation to be rom the runtime pool? Can you check, please? > > + if (EFI_ERROR (Status)) { > > + FreePool (Instance); [SAMI] Same comment as above. > > + return Status; > > + } > > + > > + NorFlashPrintInfo (FlashInfo); > > + } > > + [SAMI] I think the above code can be slightly optimised, see snippet below.   Status = NorFlashReadID (Instance, JedecId);   if (EFI_ERROR (Status)) {     if (Status != EFI_UNSUPPORTED) {          goto error_handler1;      }   } else {     Status = NorFlashGetInfo (JedecId, &FlashInfo, TRUE);     if (EFI_ERROR (Status)) { [SAMI] Question? How critical is this error? Can it not be treated as JedecID not supported, and the flash driver can proceed without printing the flash info? In other words, is it worth adding a warning message that the flash info cannot be read and proceed with loading the driver?         goto error_handler1;     }     NorFlashPrintInfo (FlashInfo);   } > if (SupportFvb) { > > NorFlashFvbInitialize (Instance); > > > > diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c > index ef9bed37d140..a621b29f34e2 100644 > --- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c > +++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c > @@ -10,6 +10,7 @@ > #include > > #include > > #include > > +#include > > > > #include "NorFlashCommon.h" > > > > @@ -106,6 +107,8 @@ NorFlashCreateInstance ( > { > > EFI_STATUS Status; > > NOR_FLASH_INSTANCE *Instance; > > + NOR_FLASH_INFO *FlashInfo; > > + UINT8 JedecId[6]; > > > > ASSERT (NorFlashInstance != NULL); > > > > @@ -132,6 +135,22 @@ NorFlashCreateInstance ( > return EFI_OUT_OF_RESOURCES; > > } > > > > + Status = NorFlashReadID (Instance, JedecId); > > + if (EFI_ERROR (Status) && (Status != EFI_UNSUPPORTED)) { > > + FreePool (Instance); > > + return Status; > > + } > > + [SAMI] Same comments as in NorFlashDxe. > > + if (Status == EFI_SUCCESS) { > > + Status = NorFlashGetInfo (JedecId, &FlashInfo, TRUE); > > + if (EFI_ERROR (Status)) { > > + FreePool (Instance); > > + return Status; > > + } > > + > > + NorFlashPrintInfo (FlashInfo); > > + } > > + > > if (SupportFvb) { > > NorFlashFvbInitialize (Instance); > > > > diff --git a/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c b/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c > index d68e237e2e26..5f8f137482ee 100644 > --- a/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c > +++ b/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c > @@ -947,3 +947,21 @@ NorFlashReset ( > SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY); > > return EFI_SUCCESS; > > } > > + > > +/** > > + Read JEDEC ID of NOR flash device. > > + > > + @param[in] Instance NOR flash Instance of variable store region. > > + @param[out] JedecId JEDEC ID of NOR flash device. > > + Maximum length of JedecId can be upto 6 bytes. > > + > > + @retval EFI_UNSUPPORTED JEDEC ID retrievel not implemented. > > +**/ > > +EFI_STATUS > > +NorFlashReadID ( [SAMI] Nit.  Rename NorFlashReadID() => NorFlashReadId(). > > + IN NOR_FLASH_INSTANCE *Instance, > > + OUT UINT8 *JedecId > > + ) > > +{ > > + return EFI_UNSUPPORTED; > > +} > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#118965): https://edk2.groups.io/g/devel/message/118965 Mute This Topic: https://groups.io/mt/105690944/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- --------------rd2Dt4WERQd8vvS0TyFmAkXQ Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi Sahil,

Thank you for this patch.

Please find my feedback inline marked [SAMI].

Regards,

Sami Mujawarnd

On 23/04/2024 06:56 am, Sahil Kaushal wrote:
From: sahil <sahil@arm.com>

This patch adds an optional functionality in NorFlashDxe to fetch and
print NOR Flash information from NorFlashInfoLib using its JEDEC ID.

NOR Flash libraries will implement a function "NorFlashReadID" which
will fetch and return JEDEC ID. This JEDEC ID can be then used to
print NOR Flash info using NorFlashInfoLib. If this functionality is

[SAMI] Can you explain how this information is useful, please? Is it just for debugging or it can be used for some other purpose.

If it is just for printing the information, then maybe the above sentence could be silghtly modified, e.g. 

This JEDEC ID can be then printed along with the NOR Flash info by NorFlashInfoLib.

[/SAMI]


not needed then the function can just return EFI_UNSUPPORTED.

Signed-off-by: sahil <sahil@arm.com>
---
 Platform/ARM/SgiPkg/SgiPlatform.dsc.inc                          |  2 ++
 Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc                        |  2 ++
 Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc                     |  2 ++
 Platform/ARM/JunoPkg/ArmJuno.dsc                                 |  2 ++
 Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc                |  2 ++
 Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf                 |  1 +
 Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf        |  1 +
 Platform/ARM/Include/Library/NorFlashDeviceLib.h                 |  6 ++++++
 Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c                   | 19 +++++++++++++++++++
 Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c          | 19 +++++++++++++++++++
 Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c | 18 ++++++++++++++++++
 11 files changed, 74 insertions(+)

diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
index 3dcf422eab4b..aef7cba5449e 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
@@ -36,6 +36,8 @@
   LcdPlatformLib|Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgiLib.inf

   NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf

   NorFlashPlatformLib|Platform/ARM/SgiPkg/Library/NorFlashLib/NorFlashLib.inf

+  # NOR flash support

+  NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf

   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

   ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf

   TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf

diff --git a/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc b/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc
index ab0e2a957a1b..02d684adaebd 100644
--- a/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc
+++ b/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc
@@ -65,6 +65,8 @@
   IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf

   NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf

   NorFlashPlatformLib|Platform/ARM/SgiPkg/Library/NorFlashLib/StandaloneMmNorFlashLib.inf

+  # NOR flash support

+  NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf

   OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf

   RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf

   PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf

diff --git a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
index 70ff049d3248..4e208c539a88 100644
--- a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
+++ b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
@@ -95,6 +95,8 @@
   ArmPlatformSysConfigLib|Platform/ARM/VExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf

   NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf

   NorFlashPlatformLib|Platform/ARM/VExpressPkg/Library/NorFlashArmVExpressLib/NorFlashArmVExpressLib.inf

+  # NOR flash support

+  NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf

   ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf

 

   # ARM PL031 RTC Driver

diff --git a/Platform/ARM/JunoPkg/ArmJuno.dsc b/Platform/ARM/JunoPkg/ArmJuno.dsc
index 81d2cbe4359f..946b8680c8c2 100644
--- a/Platform/ARM/JunoPkg/ArmJuno.dsc
+++ b/Platform/ARM/JunoPkg/ArmJuno.dsc
@@ -42,6 +42,8 @@
 

   NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf

   NorFlashPlatformLib|Platform/ARM/JunoPkg/Library/NorFlashJunoLib/NorFlashJunoLib.inf

+  # NOR flash support

+  NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf

 

   CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf

   CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf

diff --git a/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc b/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc
index a5805da49c92..ee71bbb1fc09 100644
--- a/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc
+++ b/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc
@@ -102,6 +102,8 @@
 !if $(ENABLE_UEFI_SECURE_VARIABLE) == TRUE

   NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf

   NorFlashPlatformLib|Platform/ARM/VExpressPkg/Library/NorFlashArmVExpressLib/NorFlashStMmLib.inf

+  # NOR flash support

[SAMI] Nit. Should the above comment be 'NOR flash identification support'? Same comment for other places in this patch.


+  NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf

   VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf

   VariableFlashInfoLib|MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.inf

   AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf

diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf
index 6522968d6c5a..4ab4d6a26926 100644
--- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf
+++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf
@@ -35,6 +35,7 @@
   HobLib

   IoLib

   NorFlashDeviceLib

+  NorFlashInfoLib

   NorFlashPlatformLib

   UefiBootServicesTableLib

   UefiDriverEntryPoint

diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
index eb86d423f106..8b583f77d927 100644
--- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
+++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
@@ -37,6 +37,7 @@
   MemoryAllocationLib

   MmServicesTableLib

   NorFlashDeviceLib

+  NorFlashInfoLib

   NorFlashPlatformLib

   StandaloneMmDriverEntryPoint

 

diff --git a/Platform/ARM/Include/Library/NorFlashDeviceLib.h b/Platform/ARM/Include/Library/NorFlashDeviceLib.h
index 29b8b8901525..1e61f7c935ae 100644
--- a/Platform/ARM/Include/Library/NorFlashDeviceLib.h
+++ b/Platform/ARM/Include/Library/NorFlashDeviceLib.h
@@ -154,4 +154,10 @@ NorFlashUnlock (
   IN EFI_TPL  OriginalTPL

   );

 

+EFI_STATUS

+NorFlashReadID (

+  IN  NOR_FLASH_INSTANCE  *Instance,

+  OUT UINT8               *JedecId  // Maximum length of JedecId can be upto 6 bytes.

+  );
[SAMI] You have the documentation header already in P30NorFlashDeviceLib.. Can you put that in the header file here as well, please? 

+

 #endif /* NOR_FLASH_DEVICE_LIB_H_ */

diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
index f5c0dadf84e0..1084f4731ce4 100644
--- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
+++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
@@ -9,6 +9,7 @@
 #include <Library/UefiLib.h>

 #include <Library/BaseMemoryLib.h>

 #include <Library/MemoryAllocationLib.h>

+#include <Library/NorFlashInfoLib.h>

 #include <Library/UefiBootServicesTableLib.h>

 #include <Library/PcdLib.h>

 #include <Library/HobLib.h>

@@ -112,6 +113,8 @@ NorFlashCreateInstance (
 {

   EFI_STATUS          Status;

   NOR_FLASH_INSTANCE  *Instance;

+  NOR_FLASH_INFO      *FlashInfo;

+  UINT8               JedecId[6];

 

   ASSERT (NorFlashInstance != NULL);

 

@@ -138,6 +141,22 @@ NorFlashCreateInstance (
     return EFI_OUT_OF_RESOURCES;

   }

 

+  Status = NorFlashReadID (Instance, JedecId);

+  if (EFI_ERROR (Status) && (Status != EFI_UNSUPPORTED)) {

+    FreePool (Instance);

[SAMI] What about freeing the Instance->ShadowBuffer buffer? This issue appears to be present even before your changes. Can you add another patch before patch 9/14 to fix the memory leak in this funciton, please?

I think it may be better to handle the error at the end of the function and cleanup the resources allocated, i.e. implement an exit_handler<1,2> label  as needed that frees up the resources and add goto exit_handler<1,2> from whereever the error occurs, setting up the Status appropriately to be returned at the end of the function.


+    return Status;

+  }

+

+  if (Status == EFI_SUCCESS) {

+    Status = NorFlashGetInfo (JedecId, &FlashInfo, TRUE);
[SAMI] It looks like the memory allocated for FlashInfo is leaked. Also do you need the memory allocation to be rom the runtime pool? Can you check, please?

+    if (EFI_ERROR (Status)) {

+      FreePool (Instance);
[SAMI] Same comment as above.

+      return Status;

+    }

+

+    NorFlashPrintInfo (FlashInfo);

+  }

+

[SAMI] I think the above code can be slightly optimised, see snippet below.

<SNIP>

  Status = NorFlashReadID (Instance, JedecId);
  if (EFI_ERROR (Status)) {

    if (Status != EFI_UNSUPPORTED) {

         goto error_handler1;

     }

  } else {
    Status = NorFlashGetInfo (JedecId, &FlashInfo, TRUE);
    if (EFI_ERROR (Status)) {

[SAMI] Question? How critical is this error? Can it not be treated as JedecID not supported, and the flash driver can proceed without printing the flash info?

In other words, is it worth adding a warning message that the flash info cannot be read and proceed with loading the driver?

        goto error_handler1;
    }
    NorFlashPrintInfo (FlashInfo);
  }

</SNIP>



   if (SupportFvb) {

     NorFlashFvbInitialize (Instance);

 

diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
index ef9bed37d140..a621b29f34e2 100644
--- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
+++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
@@ -10,6 +10,7 @@
 #include <Library/BaseMemoryLib.h>

 #include <Library/MemoryAllocationLib.h>

 #include <Library/MmServicesTableLib.h>

+#include <Library/NorFlashInfoLib.h>

 

 #include "NorFlashCommon.h"

 

@@ -106,6 +107,8 @@ NorFlashCreateInstance (
 {

   EFI_STATUS          Status;

   NOR_FLASH_INSTANCE  *Instance;

+  NOR_FLASH_INFO      *FlashInfo;

+  UINT8               JedecId[6];

 

   ASSERT (NorFlashInstance != NULL);

 

@@ -132,6 +135,22 @@ NorFlashCreateInstance (
     return EFI_OUT_OF_RESOURCES;

   }

 

+  Status = NorFlashReadID (Instance, JedecId);

+  if (EFI_ERROR (Status) && (Status != EFI_UNSUPPORTED)) {

+    FreePool (Instance);

+    return Status;

+  }

+
[SAMI] Same comments as in NorFlashDxe. 

+  if (Status == EFI_SUCCESS) {

+    Status = NorFlashGetInfo (JedecId, &FlashInfo, TRUE);

+    if (EFI_ERROR (Status)) {

+      FreePool (Instance);

+      return Status;

+    }

+

+    NorFlashPrintInfo (FlashInfo);

+  }

+

   if (SupportFvb) {

     NorFlashFvbInitialize (Instance);

 

diff --git a/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c b/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c
index d68e237e2e26..5f8f137482ee 100644
--- a/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c
+++ b/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c
@@ -947,3 +947,21 @@ NorFlashReset (
   SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);

   return EFI_SUCCESS;

 }

+

+/**

+  Read JEDEC ID of NOR flash device.

+

+  @param[in]     Instance               NOR flash Instance of variable store region.

+  @param[out]    JedecId                JEDEC ID of NOR flash device.

+                                        Maximum length of JedecId can be upto 6 bytes.

+

+  @retval        EFI_UNSUPPORTED        JEDEC ID retrievel not implemented.

+**/

+EFI_STATUS

+NorFlashReadID (
[SAMI] Nit.  Rename NorFlashReadID() => NorFlashReadId(). 

+  IN  NOR_FLASH_INSTANCE  *Instance,

+  OUT UINT8               *JedecId

+  )

+{

+  return EFI_UNSUPPORTED;

+}
_._,_._,_
Groups.io Links:

You receive all messages sent to this group.

View/Reply Online (#118965) | | Mute This Topic | New Topic
Your Subscription | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--------------rd2Dt4WERQd8vvS0TyFmAkXQ--