Hi Michael, I didn't initiate any discussion on this yet. And I am not sure if this idea could be accepted. From my view point, IntelSiliconPkg is a proper place for SPI flash library. But UefiPayloadPkg could not use that package since it is in another repo. Since these dependencies, you could go a head to put it into IntelSiliconPkg.
Once I clean up my branch (expected complete next week), I could send my patch for your reference so that we could at least share code if possible to reduce the code maintenance. Thanks, Guo > -----Original Message----- > From: Michael Kubacki <mikub...@linux.microsoft.com> > Sent: Wednesday, March 3, 2021 3:54 PM > To: devel@edk2.groups.io; Dong, Guo <guo.d...@intel.com>; Ni, Ray > <ray...@intel.com> > Cc: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>; Chiu, Chasel > <chasel.c...@intel.com>; Desimone, Nathaniel L > <nathaniel.l.desim...@intel.com>; Luo, Heng <heng....@intel.com>; > Agyeman, Prince <prince.agye...@intel.com>; gaolim...@byosoft.com.cn; > Dong, Eric <eric.d...@intel.com> > Subject: Re: [edk2-devel] [edk2-platforms][RFC] SpiFlashCommonLib > Refactor > > 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 <ray...@intel.com> > >> Sent: Monday, March 1, 2021 5:52 PM > >> To: Michael Kubacki <mikub...@linux.microsoft.com>; > >> devel@edk2.groups.io > >> Cc: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>; Chiu, Chasel > >> <chasel.c...@intel.com>; Desimone, Nathaniel L > >> <nathaniel.l.desim...@intel.com>; Luo, Heng <heng....@intel.com>; > >> Agyeman, Prince <prince.agye...@intel.com>; > gaolim...@byosoft.com.cn; > >> Dong, Eric <eric.d...@intel.com>; Dong, Guo <guo.d...@intel.com> > >> 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 <mikub...@linux.microsoft.com> > >>> Sent: Tuesday, March 2, 2021 3:16 AM > >>> To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com> > >>> Cc: Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>; Chiu, Chasel > >> <chasel.c...@intel.com>; Desimone, Nathaniel L > >>> <nathaniel.l.desim...@intel.com>; Luo, Heng <heng....@intel.com>; > >> Agyeman, Prince <prince.agye...@intel.com>; > >>> gaolim...@byosoft.com.cn; Dong, Eric <eric.d...@intel.com> > >>> 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 <rangasai.v.chaga...@intel.com> > >>>>> Sent: Monday, March 1, 2021 4:46 PM > >>>>> To: Ni, Ray <ray...@intel.com> > >>>>> Subject: RE: [edk2-platforms][RFC] SpiFlashCommonLib Refactor > >>>>> > >>>>> Did you get a chance to look into this ? > >>>>> > >>>>> -----Original Message----- > >>>>> From: Michael Kubacki <mikub...@linux.microsoft.com> > >>>>> Sent: Tuesday, February 16, 2021 4:58 PM > >>>>> To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; Chaganty, > >> Rangasai V <rangasai.v.chaga...@intel.com>; Chiu, > >>> Chasel > >>>>> <chasel.c...@intel.com>; Desimone, Nathaniel L > >> <nathaniel.l.desim...@intel.com>; Luo, Heng <heng....@intel.com>; > >>>>> Agyeman, Prince <prince.agye...@intel.com>; > >> gaolim...@byosoft.com.cn; Dong, Eric <eric.d...@intel.com> > >>>>> 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 > >>>> > >>>> > >>>> > >>>> > >>>> > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#72407): https://edk2.groups.io/g/devel/message/72407 Mute This Topic: https://groups.io/mt/80694124/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
