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 22F6C7803CE for ; Thu, 16 May 2024 15:25:28 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=bEucVwUs3lH+htNuV0GyXo7tEdWVGd0PnOY4tkeuUhA=; 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=1715873127; v=1; b=xfcvfW9g3SrL53w9/yMDlTU/styCrdrHX0AhoYFThMcKRPm8H/jWeIO07ny+ODwto7lKt/6N DJgCStyx7VlFsxbR9i1ohKFwJeiSRoqdNSLgNeT3uZOknispQPRDkPzYw33cBG8PzkgV3pKhLky 0TsttOB2iZkEKmSMVL3tybk5Myl/T5ZbW0+aZJls5Sy5WMjVVFtWqbq63z1p20B77Jt1Pc5yQzu gC44CZXIkHr28FuoadxPwZK2RQzrGAnIV0f9tPGIuYF2hmVRHx3BV2qC86ZD7DX2HtFXJV3HYxc CWJFKPjAcg9nLmsn3ewQP5kQrb6YfNjEygc4eS308dkxw== X-Received: by 127.0.0.2 with SMTP id laZgYY7687511xWSywo8fMI0; Thu, 16 May 2024 08:25:27 -0700 X-Received: from EUR05-AM6-obe.outbound.protection.outlook.com (EUR05-AM6-obe.outbound.protection.outlook.com [40.107.22.50]) by mx.groups.io with SMTP id smtpd.web10.16923.1715873126573568354 for ; Thu, 16 May 2024 08:25:27 -0700 X-Received: from AM6PR10CA0080.EURPRD10.PROD.OUTLOOK.COM (2603:10a6:209:8c::21) by VI1PR08MB10197.eurprd08.prod.outlook.com (2603:10a6:800:1bc::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7587.28; Thu, 16 May 2024 15:25:18 +0000 X-Received: from AMS0EPF000001A5.eurprd05.prod.outlook.com (2603:10a6:209:8c:cafe::a) by AM6PR10CA0080.outlook.office365.com (2603:10a6:209:8c::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7587.28 via Frontend Transport; Thu, 16 May 2024 15:25:18 +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 AMS0EPF000001A5.mail.protection.outlook.com (10.167.16.232) 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:25:18 +0000 X-Received: ("Tessian outbound ba75727f6dca:v315"); Thu, 16 May 2024 15:25:18 +0000 X-CheckRecipientChecked: true X-CR-MTA-CID: e87471c01147baca X-CR-MTA-TID: 64aa7808 X-Received: from 8f40b000abab.1 by 64aa7808-outbound-1.mta.getcheckrecipient.com id 839B5A94-4356-45AC-AA2D-A68BE32F3207.1; Thu, 16 May 2024 15:25:11 +0000 X-Received: from EUR04-VI1-obe.outbound.protection.outlook.com by 64aa7808-outbound-1.mta.getcheckrecipient.com with ESMTPS id 8f40b000abab.1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384); Thu, 16 May 2024 15:25:11 +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:25:09 +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:25:09 +0000 Message-ID: Date: Thu, 16 May 2024 16:25:07 +0100 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 12/14] Platform/ARM: Add CadenceQspiNorFlashDeviceLib for NorFlashDxe To: Sahil , devel@edk2.groups.io, pierre.gondois@arm.com Cc: sahil.kaushal@arm.com, Ard Biesheuvel , Leif Lindholm , "levi.yun" , "nd@arm.com" References: <20240423055638.1271531-1-Sahil.Kaushal@arm.com> <20240423055638.1271531-13-Sahil.Kaushal@arm.com> <8f292b6a-4c18-48c5-8e20-5eb9c86538e0@arm.com> From: "Sami Mujawar" In-Reply-To: X-ClientProxiedBy: LO2P265CA0038.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:61::26) To AS8PR08MB6806.eurprd08.prod.outlook.com (2603:10a6:20b:39b::12) MIME-Version: 1.0 X-MS-TrafficTypeDiagnostic: AS8PR08MB6806:EE_|AS8PR08MB9071:EE_|AMS0EPF000001A5:EE_|VI1PR08MB10197:EE_ X-MS-Office365-Filtering-Correlation-Id: 5e8bad3f-85c7-4ad3-83a2-08dc75bc64cd 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?IsSs/vKu/Yr/QYj1+F+y/x0PW4PJHN+DX+jHBgLtT31gGT7s7oZVCQMr8LII?= =?us-ascii?Q?Z9Q6cq64vu/J6EuCiOUhqCfuDzT83CePm8IFBj2YLDCjLY9xUqiyq5Kx1eao?= =?us-ascii?Q?D2nE641ka/VywToPXbrbOlz2pUJF5oUyXDGznHp5AsmbgBzSHyekyjtFIXFT?= =?us-ascii?Q?bg20MZt0PY5hTUTiCrVZlQxuEyK3lmZBk1MOqU1nxYpphlvi1NCcMxBAAaAM?= =?us-ascii?Q?+4qtnI/r9C97qWRO8hZw+hDiG75/1zzrQAl+vSFPX9SQwz4q4ZP20pDZuNzs?= =?us-ascii?Q?3x16Y76RwQlGEr0vEPH3g2nXQklok1+vIMl1r4yP0oEfWpYGVe/8DH8tLmyE?= =?us-ascii?Q?AbZPhGzqxvBiPMDO4EQeBVSpXPTO2nAKEbwqzQYdopvwlWaAKuKDP0Hym6bu?= =?us-ascii?Q?Go3sPRSI1mcHLO2/4sqBbTTmP9GEh7sBzX0x5MpugoOF703vDp9fP2DRcXET?= =?us-ascii?Q?lRuh927VirTLA/T8l5sgZevi7Q23GICLNMpLs0Jzvy+BwomHPioVqPv0xtu7?= =?us-ascii?Q?2IWtrq+Phym5tQOY8zQj95LtIzGow/1yz7xGlgCjBwE+VrwJVNXfiJZmj4ma?= =?us-ascii?Q?IqruNhZpDKgLkAEjQXatDWhzlgzhI9ZazYfhf/b8MxsUjCuISO5FjTHTGXk5?= =?us-ascii?Q?ko0RHraLM8mIXtD4kLygG+ckazxkUG4uI1QxdfaPdWipVwKKmOjkxmk9YKeV?= =?us-ascii?Q?/+ZrIJlMABccWhvHsqs6Pwf3mr/hoUG05lgCuXg695JNppy1/pQowAas6rTO?= =?us-ascii?Q?FWVe0IuVOBlMit64zY4soOr7GToE8olV8qbeDz3KRfRpWd/9orJaonaPfSEK?= =?us-ascii?Q?4cZQ01NdGt15qpR0d+j9w5tTCORcfSyvVpepFR6nAIbY9sbP+afKbz6OfzWD?= =?us-ascii?Q?VS6SkGNbcpiOPo06uGCVpO4R2GWoa5vBjK8BXdyU07CArvnF9hP4xIT9xli7?= =?us-ascii?Q?lHxkAkXc8ZepKWhGJmdwoJZehszSsHDZd/LKe2LwioVGd8F+8szEJBDHsFC3?= =?us-ascii?Q?jfJWXzm7Whnsp9iu8v7tIEnkBfjZa3U+8ApA/oBJn7HK4tdWG4fBWCybPE42?= =?us-ascii?Q?W2BemO5g4I8Us/RMrax83Wdlh36+aprWP2zt1vpcl5Rmnh4JRs9uaFcrZfdw?= =?us-ascii?Q?NlKaFemTQz6/qHL1/Q16pbAKzcZJNdvOfAHmMl40wf8dR2GN/fpyOGl2zACU?= =?us-ascii?Q?5jhwKXsBZHTWRQxbz3tcsgn8++OD1p4cJuxoRdy57RmTPiFHOwOBV2wwgnI?= =?us-ascii?Q?=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: AMS0EPF000001A5.eurprd05.prod.outlook.com X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id-Prvs: 84b420f5-ebf6-49ed-b21d-08dc75bc5f08 X-Microsoft-Antispam-Message-Info: =?utf-8?B?cGxHLytCcFJSY3NZekhLWGdqa0JBOEdRNGFTUlNVZWlKMmR0MXIrVmY0ZjJX?= =?utf-8?B?UFR4K2pqWGdqWEJqV0d1ZTFDR1NFVytXN2tEUVh3Y3lvWTVuQk5PSGNSbWVH?= =?utf-8?B?dUU0bHYyK2Vvc0pRZXNtVEgxU3IxNlByekxjUnhEM1p5NS9GSzlQMUhaYlQ5?= =?utf-8?B?cFdWdkN5bFY1OEl1dGp4WlBma2w5YTNsL3JZbk0rU1phWHZaM1lNY2h3US9k?= =?utf-8?B?Q0d0bEhxZ2dYbThETnAvM0gySTI0bGdhcmczaE5ibEtvaDZRMkFSOEtqdGh5?= =?utf-8?B?bXcyVG8wdFBpTGd6amllV3RsTWdSTDlWT05nVE90Y3ZaV04xZUJuV3VOYnpW?= =?utf-8?B?K1VSS3BmSE9mUFo1SzY5TUw5b0VUUW1PY3o5c21jV0NQYjNMVDhmNW5HR2Y1?= =?utf-8?B?aUZIc3NVRHM3Y2hyWU9ZMnZIOS9mcFZMaVV6MDdNaFh1eXA1bjZFdVRBeEZD?= =?utf-8?B?ZWJEZkl2ODRXTWJHYk50Ny9OQXVzaUdqcGtocGpXRGFSclluVEdIWE5zQitX?= =?utf-8?B?U0haODYyUW9MSmIxN2g1aHg0ZU16WEVpMWVDS1QyMU9XWVI5emZvV1ZTaDR1?= =?utf-8?B?VEtYaWQzejVRZFV3SERaRUtPWXpOL0tsd1ZESk5ZVmZGZVcvU2UyUXR0dUFL?= =?utf-8?B?TDkyK0NoVGZERE5ocElLNkZJcU1qNC9oOVNhTUJjTWcyTmtUdkxuRFlZWm53?= =?utf-8?B?dXFNeEg3RHhUZzNZYlcwdFlYR0VvaSthdWNiNHlQQW4wUUxIUGJSSEFjM0F5?= =?utf-8?B?NUZQNU5yMG8rUXFEcjdaSURkR0lIS2pxYWh2ckRsY1FzV3VEVEk1UnNJRmRa?= =?utf-8?B?eGhUOUdPVWtrUHlSdkM1ZWErS1lKQlpnbHZjNXpNTzMwbHpDbVNPZzg4aitI?= =?utf-8?B?N3ltTjVIeGJmRUc2NFhuczdNRFd2RXpndXducldxYktXWWVtKzliLzBxbXR2?= =?utf-8?B?eDVPRHAzWmtlN1VMZ3FvZVcrOWloSDRzZ2M2ZlNrR2I2bWM2SVE1WVVCeEZK?= =?utf-8?B?YTNqRDdJREpUeEtMV1Ryc1Q2bXZ5cTNvUXM4ZXA0OTFvUzhzQVNLVUZxT1JP?= =?utf-8?B?NE10MEt3RitnUDNscEF4UnV2VlhjM2dLM0w1eXR3cUpaRDhGVkRkZ3hra2VM?= =?utf-8?B?K0pqai95VmFwdjNtTVB6dlVtTXRMMTMvMXVodDVheWJyREt0Q0h5Z1NLcUFF?= =?utf-8?B?YUlqTy9BRFc0WUt0d1hURjNpdm1DbVhFd2s1M2hkQlBpRXB1TUtFRWRXTnNP?= =?utf-8?B?RUFpUkVpVDF4dnNFMVlucEE5cW1wcG03dEsvelhZWURYbWIxemxxcnhWQkJ0?= =?utf-8?B?YW93Z0dYN3JQNUh6aFNPelk4SXJFSExiYzlYUDJWMGI5SlBxVjNoekFvOTdm?= =?utf-8?B?bUJXd3RMbjJoc3dLMVlFZHhhT2xKSmtaYTBWQ0syMWdiWmI0NnZXMXBjNVU5?= =?utf-8?B?a1pmNnR2bXl0QU5QZ3V6NFlNSDJuV1k0Mnl4d2s2RVJXNlJ4Nm9LRFhhRUox?= =?utf-8?B?OWtra0RDSlVwRzUxbEZsWEF3NzZuSDhicnpWbkd0bW01STZqSi9Edi8vYmpw?= =?utf-8?B?YkpmK1gvNDVTYzJKck1ydVpubVo0TnlVYUFZZmVDYkt2ck9wY3RSd3JKUEYx?= =?utf-8?B?S29MdVUzZGJER0xyOEJyZGFheDFCSDVHRVVjTWN0bTVFbmUxYkd5NDBXeVdz?= =?utf-8?B?RDEyRkFxSGsyMnBwQlQ4OXFDNnBGUDFqVm9vNzZZeEhDTnFhR3Jhbm9IR1FX?= =?utf-8?Q?0gSq6XEDAYpUYysMf0Eluo6hbFu37UkuyFEWnWa?= X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 May 2024 15:25:18.4634 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 5e8bad3f-85c7-4ad3-83a2-08dc75bc64cd 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: AMS0EPF000001A5.eurprd05.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR08MB10197 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:25:27 -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: rBKohvEBVU80AKpIcJhYoQEcx7686176AA= Content-Type: multipart/alternative; boundary="------------N0FU6hnOSmIVEsVd2BEsv41B" 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=xfcvfW9g; 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 --------------N0FU6hnOSmIVEsVd2BEsv41B Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Sahil, Please find my response inline marked [SAMI]. Regards, Sami Mujawar On 09/05/2024 07:25 am, Sahil wrote: > Hi Pierre, Thanks for reviewing the patchset. Please find my comment > inline below. > > On Thu, 2 May 2024 at 18:47, PierreGondois via groups.io > wrote: > > > > Hello Sahil, > > > > On 4/23/24 07:56, Sahil Kaushal via groups.io wrote: > > > From: sahil > > > > > > In N1Sdp platform, the SoC is connected to IOFPGA which has a > > > Cadence Quad SPI (QSPI) controller. This QSPI controller manages > > > the flash chip device via QSPI bus. > > > > > > This patch adds CadenceQspiNorFlashDeviceLib which is used to > > > manage and access the above configuration. > > > > > > Signed-off-by: sahil > > > --- > > > > Platform/ARM/Library/CadenceQspiNorFlashDeviceLib/CadenceQspiNorFlashDeviceLib.inf > |   32 + > > > > Platform/ARM/Library/CadenceQspiNorFlashDeviceLib/CadenceQspiNorFlashDeviceLib.h >   |   44 + > > > > Platform/ARM/Library/CadenceQspiNorFlashDeviceLib/CadenceQspiNorFlashDeviceLib.c >   | 1011 ++++++++++++++++++++ > > >   3 files changed, 1087 insertions(+) > > > > > > > [snip] > > > > > + > > > +/** > > > +  Converts milliseconds into number of ticks of the performance > counter. > > > + > > > +  @param[in] Milliseconds  Milliseconds to convert into ticks. > > > + > > > +  @retval Milliseconds expressed as number of ticks. > > > + > > > +**/ > > > +STATIC > > > +UINT64 > > > +MilliSecondsToTicks ( > > > +  IN UINTN  Milliseconds > > > +  ) > > > +{ > > > +  CONST UINT64  NanoSecondsPerTick = GetTimeInNanoSecond (1); > > > + > > > +  return (Milliseconds * 1000000) / NanoSecondsPerTick; > > > > Should use DivU64x64Remainder() here: > > { > >    UINT64  NanoSecondsPerTick; > >    UINT64  NanoSeconds; > > > >    NanoSecondsPerTick = GetTimeInNanoSecond (1); > >    NanoSeconds = MultU64x32 (Milliseconds, 1000000); > > > >    return DivU64x64Remainder (NanoSeconds, NanoSecondsPerTick, NULL); > > } > > > > > +} > > > + > > > +/** > > > +  Poll Status register for NOR flash erase/write completion. > > > + > > > +  @param[in]      Instance           NOR flash Instance. > > > + > > > +  @retval         EFI_SUCCESS        Request is executed > successfully. > > > +  @retval         EFI_TIMEOUT        Operation timed out. > > > +  @retval         EFI_DEVICE_ERROR   Controller operartion failed. > > > > operartion -> typo > > (same at another place I think) > > > > [snip] > > > > > + > > > +/** > > > +  Read from nor flash. > > > + > > > +  @param[in]     Instance               NOR flash Instance of > variable store region. > > > +  @param[in]     Lba                    The starting logical > block index to read from. > > > +  @param[in]     Offset                 Offset into the block at > which to begin reading. > > > +  @param[in]     BufferSizeInBytes      The number of bytes to read. > > > +  @param[out]    Buffer                 The pointer to a > caller-allocated buffer that > > > +                                        should copied with read data. > > > + > > > +  @retval        EFI_SUCCESS            The read is completed. > > > +  @retval        EFI_INVALID_PARAMETER  Invalid parameters passed. > > > +**/ > > > +EFI_STATUS > > > +NorFlashRead ( > > > +  IN NOR_FLASH_INSTANCE  *Instance, > > > +  IN EFI_LBA             Lba, > > > +  IN UINTN               Offset, > > > +  IN UINTN               BufferSizeInBytes, > > > +  OUT VOID               *Buffer > > > +  ) > > > +{ > > > +  UINTN  StartAddress; > > > + > > > +  // The buffer must be valid > > > +  if (Buffer == NULL) { > > > +    return EFI_INVALID_PARAMETER; > > > +  } > > > + > > > +  // Return if we do not have any byte to read > > > +  if (BufferSizeInBytes == 0) { > > > +    return EFI_SUCCESS; > > > +  } > > > + > > > +  if (((Lba * Instance->Media.BlockSize) + Offset + > BufferSizeInBytes) > > > > +      Instance->Size) > > > +  { > > > +    DEBUG (( > > > +      DEBUG_ERROR, > > > +      "NorFlashRead: ERROR - Read will exceed device size.\n" > > > +      )); > > > +    return EFI_INVALID_PARAMETER; > > > +  } > > > + > > > +  // Get the address to start reading from > > > +  StartAddress = GET_NOR_BLOCK_ADDRESS ( > > > +                   Instance->RegionBaseAddress, > > > +                   Lba, > > > +                   Instance->Media.BlockSize > > > +                   ); > > > + > > > +  // Readout the data > > > +  CopyMem (Buffer, (UINTN *)(StartAddress + Offset), > BufferSizeInBytes); > > > > The original code at: > >  Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c > > > > implements and uses AlignedCopyMem()/NorFlashWriteBuffer() which seems > > to be more efficient. > > Just to be sure I understand correctly, is the maximal read/write size > > of 4 bytes ? Meaning that these functions are not needed ? > > > > --- > > > > NorFlashWriteBuffer() is not implemented here IIUC won't be > implemtned as not > > needed. Maybe in an additional patch, the function could be removed > from the > > library interface at: > >    Platform/ARM/Include/Library/NorFlashDeviceLib.h > > and made static in: > >  Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c > > > CopyMem() and AlignedCopyMem() have nearly identical implementations, > therefore I think we can > continue using CopyMem() here. > > For NorFlashWriteBuffer(), in the P30 spec, it looks like buffered > programming is one of the features > of the IP whereas there is no such feature in cadence IP. So, I think > there is no need for > NorFlashWriteBuffer() for Cadence controller Library. > > I will push another patch in the next patchset to remove >  NorFlashWriteBuffer() from the > NorFlashDeviceLib.h header file. [SAMI] Also make NorFlashWriteBuffer() a STATIC function in P30NorFlashDeviceLib.c. > > > > + > > > +  return EFI_SUCCESS; > > > +} > > > + > > > +/** > > > +  Write a full or portion of a block. > > > + > > > +  @param[in]       Instance              NOR flash Instance of > variable store region. > > > +  @param[in]       Lba                   The starting logical > block index to write to. > > > +  @param[in]       Offset                Offset into the block at > which to begin writing. > > > +  @param[in, out]  NumBytes              The total size of the > buffer. > > > +  @param[in]       Buffer                The pointer to a > caller-allocated buffer that > > > +                                         contains the source for > the write. > > > + > > > +  @retval          EFI_SUCCESS           The write is completed. > > > +  @retval          EFI_OUT_OF_RESOURCES  Invalid Buffer passed. > > > +  @retval          EFI_BAD_BUFFER_SIZE   Buffer size not enough. > > > +  @retval          EFI_DEVICE_ERROR      The device reported an > error. > > > +**/ > > > +EFI_STATUS > > > +NorFlashWriteSingleBlock ( > > > +  IN        NOR_FLASH_INSTANCE  *Instance, > > > +  IN        EFI_LBA             Lba, > > > +  IN        UINTN               Offset, > > > +  IN OUT    UINTN               *NumBytes, > > > +  IN        UINT8               *Buffer > > > +  ) > > > +{ > > > +  EFI_STATUS  Status; > > > +  UINT32      Tmp; > > > +  UINT32      TmpBuf; > > > +  UINT32      WordToWrite; > > > +  UINT32      Mask; > > > +  BOOLEAN     DoErase; > > > +  UINTN       BytesToWrite; > > > +  UINTN       CurOffset; > > > +  UINTN       WordAddr; > > > +  UINTN       BlockSize; > > > +  UINTN       BlockAddress; > > > +  UINTN       PrevBlockAddress; > > > + > > > +  if (Buffer == NULL) { > > > +    DEBUG (( > > > +      DEBUG_ERROR, > > > +      "NorFlashWriteSingleBlock: ERROR - Buffer is invalid\n" > > > +      )); > > > +    return EFI_OUT_OF_RESOURCES; > > > > EFI_INVALID_PARAMETER instead I think > > > > > +  } > > > + > > > +  PrevBlockAddress = 0; > > > + > > > +  DEBUG (( > > > +    DEBUG_INFO, > > > +    "NorFlashWriteSingleBlock(Parameters: Lba=%ld, Offset=0x%x, " > > > +    "*NumBytes=0x%x, Buffer @ 0x%08x)\n", > > > +    Lba, > > > +    Offset, > > > +    *NumBytes, > > > +    Buffer > > > +    )); > > > + > > > +  // Localise the block size to avoid de-referencing pointers all > the time > > > > Localise -> Locate > > > > > +  BlockSize = Instance->Media.BlockSize; > > > + > > > > [snip] > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#118971): https://edk2.groups.io/g/devel/message/118971 Mute This Topic: https://groups.io/mt/105690947/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- --------------N0FU6hnOSmIVEsVd2BEsv41B Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi Sahil,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 09/05/2024 07:25 am, Sahil wrote:
Hi Pierre, Thanks for reviewing the patchset. Please find my comment inline below.

On Thu, 2 May 2024 at 18:47, PierreGondois via groups.io <pierre.gondois=arm.com@groups.io> wrote:
>
> Hello Sahil,
>
> On 4/23/24 07:56, Sahil Kaushal via groups.io wrote:
> > From: sahil <sahil@arm.com>
> >
> > In N1Sdp platform, the SoC is connected to IOFPGA which has a
> > Cadence Quad SPI (QSPI) controller. This QSPI controller manages
> > the flash chip device via QSPI bus.
> >
> > This patch adds CadenceQspiNorFlashDeviceLib which is used to
> > manage and access the above configuration.
> >
> > Signed-off-by: sahil <sahil@arm.com>
> > ---
> >   Platform/ARM/Library/CadenceQspiNorFlashDeviceLib/CadenceQspiNorFlashDeviceLib.inf |   32 +
> >   Platform/ARM/Library/CadenceQspiNorFlashDeviceLib/CadenceQspiNorFlashDeviceLib.h   |   44 +
> >   Platform/ARM/Library/CadenceQspiNorFlashDeviceLib/CadenceQspiNorFlashDeviceLib.c   | 1011 ++++++++++++++++++++
> >   3 files changed, 1087 insertions(+)
> >
>
> [snip]
>
> > +
> > +/**
> > +  Converts milliseconds into number of ticks of the performance counter.
> > +
> > +  @param[in] Milliseconds  Milliseconds to convert into ticks.
> > +
> > +  @retval Milliseconds expressed as number of ticks.
> > +
> > +**/
> > +STATIC
> > +UINT64
> > +MilliSecondsToTicks (
> > +  IN UINTN  Milliseconds
> > +  )
> > +{
> > +  CONST UINT64  NanoSecondsPerTick = GetTimeInNanoSecond (1);
> > +
> > +  return (Milliseconds * 1000000) / NanoSecondsPerTick;
>
> Should use DivU64x64Remainder() here:
> {
>    UINT64  NanoSecondsPerTick;
>    UINT64  NanoSeconds;
>
>    NanoSecondsPerTick = GetTimeInNanoSecond (1);
>    NanoSeconds = MultU64x32 (Milliseconds, 1000000);
>
>    return DivU64x64Remainder (NanoSeconds, NanoSecondsPerTick, NULL);
> }
>
> > +}
> > +
> > +/**
> > +  Poll Status register for NOR flash erase/write completion.
> > +
> > +  @param[in]      Instance           NOR flash Instance.
> > +
> > +  @retval         EFI_SUCCESS        Request is executed successfully.
> > +  @retval         EFI_TIMEOUT        Operation timed out.
> > +  @retval         EFI_DEVICE_ERROR   Controller operartion failed.
>
> operartion -> typo
> (same at another place I think)
>
> [snip]
>
> > +
> > +/**
> > +  Read from nor flash.
> > +
> > +  @param[in]     Instance               NOR flash Instance of variable store region.
> > +  @param[in]     Lba                    The starting logical block index to read from.
> > +  @param[in]     Offset                 Offset into the block at which to begin reading.
> > +  @param[in]     BufferSizeInBytes      The number of bytes to read.
> > +  @param[out]    Buffer                 The pointer to a caller-allocated buffer that
> > +                                        should copied with read data.
> > +
> > +  @retval        EFI_SUCCESS            The read is completed.
> > +  @retval        EFI_INVALID_PARAMETER  Invalid parameters passed.
> > +**/
> > +EFI_STATUS
> > +NorFlashRead (
> > +  IN NOR_FLASH_INSTANCE  *Instance,
> > +  IN EFI_LBA             Lba,
> > +  IN UINTN               Offset,
> > +  IN UINTN               BufferSizeInBytes,
> > +  OUT VOID               *Buffer
> > +  )
> > +{
> > +  UINTN  StartAddress;
> > +
> > +  // The buffer must be valid
> > +  if (Buffer == NULL) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  // Return if we do not have any byte to read
> > +  if (BufferSizeInBytes == 0) {
> > +    return EFI_SUCCESS;
> > +  }
> > +
> > +  if (((Lba * Instance->Media.BlockSize) + Offset + BufferSizeInBytes) >
> > +      Instance->Size)
> > +  {
> > +    DEBUG ((
> > +      DEBUG_ERROR,
> > +      "NorFlashRead: ERROR - Read will exceed device size.\n"
> > +      ));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  // Get the address to start reading from
> > +  StartAddress = GET_NOR_BLOCK_ADDRESS (
> > +                   Instance->RegionBaseAddress,
> > +                   Lba,
> > +                   Instance->Media.BlockSize
> > +                   );
> > +
> > +  // Readout the data
> > +  CopyMem (Buffer, (UINTN *)(StartAddress + Offset), BufferSizeInBytes);
>
> The original code at:
>    Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c
>
> implements and uses AlignedCopyMem()/NorFlashWriteBuffer() which seems
> to be more efficient.
> Just to be sure I understand correctly, is the maximal read/write size
> of 4 bytes ? Meaning that these functions are not needed ?
>
> ---
>
> NorFlashWriteBuffer() is not implemented here IIUC won't be implemtned as not
> needed. Maybe in an additional patch, the function could be removed from the
> library interface at:
>    Platform/ARM/Include/Library/NorFlashDeviceLib.h
> and made static in:
>    Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c
>
CopyMem() and AlignedCopyMem() have nearly identical implementations, therefore I think we can
continue using CopyMem() here.

For NorFlashWriteBuffer(), in the P30 spec, it looks like buffered programming is one of the features
of the IP whereas there is no such feature in cadence IP. So, I think there is no need for
NorFlashWriteBuffer() for Cadence controller Library.

I will push another patch in the next patchset to remove  NorFlashWriteBuffer() from the
NorFlashDeviceLib.h header file.
[SAMI] Also make NorFlashWriteBuffer() a STATIC function in P30NorFlashDeviceLib.c.

> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  Write a full or portion of a block.
> > +
> > +  @param[in]       Instance              NOR flash Instance of variable store region.
> > +  @param[in]       Lba                   The starting logical block index to write to.
> > +  @param[in]       Offset                Offset into the block at which to begin writing.
> > +  @param[in, out]  NumBytes              The total size of the buffer.
> > +  @param[in]       Buffer                The pointer to a caller-allocated buffer that
> > +                                         contains the source for the write.
> > +
> > +  @retval          EFI_SUCCESS           The write is completed.
> > +  @retval          EFI_OUT_OF_RESOURCES  Invalid Buffer passed.
> > +  @retval          EFI_BAD_BUFFER_SIZE   Buffer size not enough.
> > +  @retval          EFI_DEVICE_ERROR      The device reported an error.
> > +**/
> > +EFI_STATUS
> > +NorFlashWriteSingleBlock (
> > +  IN        NOR_FLASH_INSTANCE  *Instance,
> > +  IN        EFI_LBA             Lba,
> > +  IN        UINTN               Offset,
> > +  IN OUT    UINTN               *NumBytes,
> > +  IN        UINT8               *Buffer
> > +  )
> > +{
> > +  EFI_STATUS  Status;
> > +  UINT32      Tmp;
> > +  UINT32      TmpBuf;
> > +  UINT32      WordToWrite;
> > +  UINT32      Mask;
> > +  BOOLEAN     DoErase;
> > +  UINTN       BytesToWrite;
> > +  UINTN       CurOffset;
> > +  UINTN       WordAddr;
> > +  UINTN       BlockSize;
> > +  UINTN       BlockAddress;
> > +  UINTN       PrevBlockAddress;
> > +
> > +  if (Buffer == NULL) {
> > +    DEBUG ((
> > +      DEBUG_ERROR,
> > +      "NorFlashWriteSingleBlock: ERROR - Buffer is invalid\n"
> > +      ));
> > +    return EFI_OUT_OF_RESOURCES;
>
> EFI_INVALID_PARAMETER instead I think
>
> > +  }
> > +
> > +  PrevBlockAddress = 0;
> > +
> > +  DEBUG ((
> > +    DEBUG_INFO,
> > +    "NorFlashWriteSingleBlock(Parameters: Lba=%ld, Offset=0x%x, "
> > +    "*NumBytes=0x%x, Buffer @ 0x%08x)\n",
> > +    Lba,
> > +    Offset,
> > +    *NumBytes,
> > +    Buffer
> > +    ));
> > +
> > +  // Localise the block size to avoid de-referencing pointers all the time
>
> Localise -> Locate
>
> > +  BlockSize = Instance->Media.BlockSize;
> > +
>
> [snip]
>
>
>
>
>
_._,_._,_
Groups.io Links:

You receive all messages sent to this group.

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

_._,_._,_
--------------N0FU6hnOSmIVEsVd2BEsv41B--