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

["Guo Dong" <guo.dong@intel.com>]

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 <mikuback@linux.microsoft.com>
> Sent: Wednesday, March 3, 2021 3:54 PM
> To: devel@edk2.groups.io; Dong, Guo <guo.dong@intel.com>; 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 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.ni@intel.com>
> >> Sent: Monday, March 1, 2021 5:52 PM
> >> To: Michael Kubacki <mikuback@linux.microsoft.com>;
> >> devel@edk2.groups.io
> >> 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>; Dong, Guo <guo.dong@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 <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\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
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >
> >
> > 
> >
> >