From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web08.2441.1614626165499394629 for ; Mon, 01 Mar 2021 11:16:05 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=WFmAm6Js; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [10.124.238.202] (unknown [131.107.174.202]) by linux.microsoft.com (Postfix) with ESMTPSA id C66FB20B6C40; Mon, 1 Mar 2021 11:16:04 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com C66FB20B6C40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1614626164; bh=BRnDd4BOOcH5TcExP31R+AprGea1IsAiD1rABSnOCBE=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=WFmAm6Js61bs3V7Yv7yfojvNkvZK+8+/vPgs/H7ObivUoSeHcrdqn9M5SO9FlLtPr D0wkw03hRjBYAhHmfQNKaspZ8FRrmHlxzUVAzV+ZASOvvHGp3eg0KrkHtHlza3WAr1 B0AUtF6nmlNEf0VRvPRohOo1CGSyjvLMxGUMdcE8= Subject: Re: [edk2-devel] [edk2-platforms][RFC] SpiFlashCommonLib Refactor To: devel@edk2.groups.io, ray.ni@intel.com Cc: "Chaganty, Rangasai V" , "Chiu, Chasel" , "Desimone, Nathaniel L" , "Luo, Heng" , "Agyeman, Prince" , "gaoliming@byosoft.com.cn" , "Dong, Eric" References: <2a2a1cc0-3de9-fbd5-1bfb-eb5dbc9d1bc6@linux.microsoft.com> From: "Michael Kubacki" Message-ID: <9a764723-06b6-555a-dfb8-6dee940e7276@linux.microsoft.com> Date: Mon, 1 Mar 2021 11:16:04 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Ray, That sounds reasonable to me. I was attempting to preserve the design that isolates the silicon-specific logic to a library via an interface to a silicon package. However, the real abstraction here is the firmware volume block protocol which could simply be produced by a silicon driver with the separation of such logic to a library being an implementation detail of the driver. In summary, here is the updated proposal: 1. Consolidate the library interface into a single header in IntelSiliconPkg. 2. Consolidate the library implementation into a single instance in IntelSiliconPkg. 3. Move SpiFvbServiceSmm out of MinPlatformPkg into IntelSiliconPkg. 4. Add SpiFvbServiceStandaloneMm to IntelSiliconPkg sharing implementation with SpiFvbServiceSmm where appropriate. Intel board packages would then use the SpiFlashCommonLib from IntelSiliconPkg (a generation specific instance could be created if needed) and use the SpiFvbServiceXyz driver from IntelSiliconPkg. Please let me know if this is acceptable and I'd be happy to send the patches. Thanks, Michael On 3/1/2021 1:07 AM, Ni, Ray wrote: > Michael, > I agree with your thoughts to consolidate the lib header and implementation to IntelSiliconPkg. > I didn't read the different implementations. But the implementation consolidation means you see all the existing implementations are the same. Right? > > But why don't you put the driver in IntelSiliconPkg as well? Creating an advanced feature for this fundamental service seems over-kill. > > Thanks, > Ray > >> -----Original Message----- >> From: Chaganty, Rangasai V >> Sent: Monday, March 1, 2021 4:46 PM >> To: Ni, Ray >> Subject: RE: [edk2-platforms][RFC] SpiFlashCommonLib Refactor >> >> Did you get a chance to look into this ? >> >> -----Original Message----- >> From: Michael Kubacki >> Sent: Tuesday, February 16, 2021 4:58 PM >> To: devel@edk2.groups.io; Ni, Ray ; Chaganty, Rangasai V ; Chiu, Chasel >> ; Desimone, Nathaniel L ; Luo, Heng ; >> Agyeman, Prince ; gaoliming@byosoft.com.cn; Dong, Eric >> Subject: [edk2-platforms][RFC] SpiFlashCommonLib Refactor >> >> Hello, >> >> I'm planning to submit support for Standalone MM in SpiFlashCommonLib soon. Currently, there's quite a bit of duplication with >> SpiFlashCommonLib. >> >> I would like to have this Standalone MM support be available in as consistent of a location as possible so I'd like to see if there is >> anything I can do to help clean this up in the early part of the patch series. >> >> >> The library interface is currently defined in the following header files: >> >> 1. Platform\Intel\MinPlatformPkg\Include\Library\SpiFlashCommonLib.h >> >> 2. Silicon\Intel\SimicsIch10Pkg\Include\Library\SpiFlashCommonLib.h >> >> 3. Silicon\Intel\KabylakeSiliconPkg\Pch\Include\Library\SpiFlashCommonLib.h >> >> 4. >> Silicon\Intel\CoffeelakeSiliconPkg\Pch\Include\Library\SpiFlashCommonLib.h >> >> >> Instances of SmmSpiFlashCommonLib implementation exist in a mix of platform and silicon packages: >> >> 1. >> Silicon\Intel\SimicsIch10Pkg\Library\SmmSpiFlashCommonLib\SmmSpiFlashCommonLib.inf >> >> 2. >> Platform\Intel\TigerlakeOpenBoardPkg\Library\SmmSpiFlashCommonLib\SmmSpiFlashCommonLib.inf >> >> 3. >> Silicon\Intel\KabylakeSiliconPkg\Pch\Library\SmmSpiFlashCommonLib\SmmSpiFlashCommonLib.inf >> >> 4. >> Silicon\Intel\CoffeelakeSiliconPkg\Pch\Library\SmmSpiFlashCommonLib\SmmSpiFlashCommonLib.inf >> >> 5. >> Platform\Intel\MinPlatformPkg\Flash\Library\SpiFlashCommonLibNull\SpiFlashCommonLibNull.inf >> >> >> The library class is currently consumed in the following INFs: >> >> 1. Platform\Intel\MinPlatformPkg\Flash\SpiFvbService\SpiFvbServiceSmm.inf >> >> 2. >> Platform\Intel\MinPlatformPkg\Flash\SpiFvbService\SpiFvbServiceStandaloneMm.inf >> >> >> My understanding is: >> >> 1. The header file is defined in each silicon package because silicon cannot depend upon platform (i.e. the MinPlatformPkg >> header). >> >> 2. The header is present in each silicon package because the implementation is silicon-specific and these packages cannot >> depend on one another. >> >> 3. The header is defined in MinPlatformPkg because MinPlatformPkg should be silicon vendor agnostic (cannot depend on the >> silicon packages). >> >> 4. The header is needed in MinPlatformPkg because the SpiFvbService there depends on SPI flash operations implemented in >> SpiFlashCommonLib. >> >> >> Here's an initial proposal: >> >> 1. Consolidate the library interface into a single header. In >> IntelSiliconPkg? >> >> 2. Consolidate library implementation into a single instance. In >> IntelSiliconPkg? >> >> 3. Move SpiFvbServiceXyz out of MinPlatformPkg. >> 3.a. Make a "SPI flash" feature? >> 3.b. Allow the Intel implementation of this feature to depend on >> SpiFlashCommonLib defined in IntelSiliconPkg. >> >> Intel board packages could then use the SpiFlashCommonLib from >> IntelSiliconPkg (a generation specific instance could be created if >> needed) and use the SpiFvbServiceXyz driver from the "SpiFlash" feature. >> >> Look forward to your thoughts and feedback. >> >> Thanks, >> Michael > > > > >