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\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 > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#72303): https://edk2.groups.io/g/devel/message/72303 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] -=-=-=-=-=-=-=-=-=-=-=-
