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.web12.668.1614812050501053181 for ; Wed, 03 Mar 2021 14:54:10 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=P3ZLEoql; 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 B80DF20B83EA; Wed, 3 Mar 2021 14:54:09 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com B80DF20B83EA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1614812049; bh=OdPOAWyVLCswklbRMPLgcEr5uNdyiMb/VMB05NR2CxA=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=P3ZLEoqlZ6WBwzLNMX2hymODEk659KTdNQlwhX9wwsmiXdrlJ3HDeBPEQajahc3c0 qsZlTLaUR84DF1xUJ/OPlJ+8cmhcINH+JPxtr5G/MFJVXS6a0+O3Zsgb1+Eqi07BwI 89ODtKz4Uxh06yXhgP/3FcWUh5LCg+xJwVSONMdY= Subject: Re: [edk2-devel] [edk2-platforms][RFC] SpiFlashCommonLib Refactor To: devel@edk2.groups.io, guo.dong@intel.com, "Ni, Ray" 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> <9a764723-06b6-555a-dfb8-6dee940e7276@linux.microsoft.com> From: "Michael Kubacki" Message-ID: Date: Wed, 3 Mar 2021 14:54:10 -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 Guo, That's good to hear. Does this new "common SPI flash library" and "SMM FVB driver" have equivalent functionality to the instances being discussed here? For the platforms I have in mind, IntelSiliconPkg is an allowed dependency whereas UefiPayloadPkg is not. I have begun the work (at low priority) discussed earlier in the thread, please let me know if I should continue. Thanks, Michael On 3/3/2021 1:58 PM, Guo Dong wrote: > > Hi Michael, > > We had created a common SPI flash library and a common SMM FVB driver for all the platforms I have (including Apollo lake, Coffee lake, Kaby Lake, Comet Lake, Tiger Lake, Elkhart Lake, etc.). we plan to upstream this for UEFI Payload. > If this one could be upstream to UefiPayloadPkg, then each platform could leverage it. > > BTW, together with this, we plan to upstream SMM support, secure boot and measured boot for UEFI Payload. > So we could use a single UEFI Payload with these advanced features on different platforms. > > Thanks, > Guo > > >> -----Original Message----- >> From: Ni, Ray >> Sent: Monday, March 1, 2021 5:52 PM >> To: Michael Kubacki ; >> devel@edk2.groups.io >> Cc: Chaganty, Rangasai V ; Chiu, Chasel >> ; Desimone, Nathaniel L >> ; Luo, Heng ; >> Agyeman, Prince ; gaoliming@byosoft.com.cn; >> Dong, Eric ; Dong, Guo >> Subject: RE: [edk2-devel] [edk2-platforms][RFC] SpiFlashCommonLib >> Refactor >> >> Michael, >> I am good with that. I am also curious how you merge all the different SPI >> flash implementation into one. >> >> Thanks, >> Ray >> >>> -----Original Message----- >>> From: Michael Kubacki >>> Sent: Tuesday, March 2, 2021 3:16 AM >>> To: devel@edk2.groups.io; Ni, Ray >>> Cc: Chaganty, Rangasai V ; Chiu, Chasel >> ; Desimone, Nathaniel L >>> ; Luo, Heng ; >> Agyeman, Prince ; >>> gaoliming@byosoft.com.cn; Dong, Eric >>> Subject: Re: [edk2-devel] [edk2-platforms][RFC] SpiFlashCommonLib >> Refactor >>> >>> 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\SmmSpiFlashC >> ommonLib.inf >>>>> >>>>> 2. >>>>> >> Platform\Intel\TigerlakeOpenBoardPkg\Library\SmmSpiFlashCommonLib\S >> mmSpiFlashCommonLib.inf >>>>> >>>>> 3. >>>>> >> Silicon\Intel\KabylakeSiliconPkg\Pch\Library\SmmSpiFlashCommonLib\Smm >> SpiFlashCommonLib.inf >>>>> >>>>> 4. >>>>> >> Silicon\Intel\CoffeelakeSiliconPkg\Pch\Library\SmmSpiFlashCommonLib\Sm >> mSpiFlashCommonLib.inf >>>>> >>>>> 5. >>>>> >> Platform\Intel\MinPlatformPkg\Flash\Library\SpiFlashCommonLibNull\SpiFla >> shCommonLibNull.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\SpiFvbServiceStandalon >> eMm.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 >>>> >>>> >>>> >>>> >>>> > > > > >