Re: [edk2-devel] [edk2-platforms][RFC] SpiFlashCommonLib Refactor

["Ni, Ray" <ray.ni@intel.com>]

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 <mikuback@linux.microsoft.com>
> Sent: Tuesday, March 2, 2021 3:16 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Luo, Heng <heng.luo@intel.com>; Agyeman, Prince <prince.agyeman@intel.com>;
> gaoliming@byosoft.com.cn; Dong, Eric <eric.dong@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.chaganty@intel.com>
> >> Sent: Monday, March 1, 2021 4:46 PM
> >> To: Ni, Ray <ray.ni@intel.com>
> >> Subject: RE: [edk2-platforms][RFC] SpiFlashCommonLib Refactor
> >>
> >> Did you get a chance to look into this ?
> >>
> >> -----Original Message-----
> >> From: Michael Kubacki <mikuback@linux.microsoft.com>
> >> Sent: Tuesday, February 16, 2021 4:58 PM
> >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Chiu,
> Chasel
> >> <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Luo, Heng <heng.luo@intel.com>;
> >> Agyeman, Prince <prince.agyeman@intel.com>; gaoliming@byosoft.com.cn; Dong, Eric <eric.dong@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
> >
> >
> > 
> >
> >