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

["Michael Kubacki" <mikuback@linux.microsoft.com>]

Going back a bit earlier in the thread, I believe the interface is 
already abstracted at the right level - the Firmware Volume Block 
protocol defined in MdePkg.

Here we have a driver that produces the protocol with a helper library. 
I believe both of these are implementation details behind the protocol 
and I view IntelSiliconPkg as a reasonable place to hold the current 
implementation within the scope it currently serves.

To be clear, I'm also just consolidating what already exists in the 
instances I mentioned at the beginning of the thread so you can use that 
as a reference for what to expect. The main goal of this particular 
activity is to simply eliminate redundant code that already exists in 
edk2-platforms.

Thanks,
Michael

On 3/3/2021 4:54 PM, Ni, Ray wrote:
> Bret,
> 
> UefiPayloadPkg contains UEFI modules that’re needed for booting UEFI OS.
> 
> So, it’s about that UefiPayloadPkg cannot contain code outside of edk2 repo.
> 
> Thanks,
> 
> Ray
> 
> *From:* Bret Barkelew <Bret.Barkelew@microsoft.com>
> *Sent:* Thursday, March 4, 2021 7:59 AM
> *To:* devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Dong, Guo 
> <guo.dong@intel.com>; Michael Kubacki <mikuback@linux.microsoft.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
> 
> Furthermore, is it the code that cannot cross the repo boundary, or the 
> interface?
> 
> Is there any use in defining the interface at a lower level such as 
> MdeModulePkg? Then a platform can consume the implementation from 
> IntelSiliconPkg if it so chooses?
> 
> - Bret
> 
> *From: *Ni, Ray via groups.io <mailto:ray.ni=intel.com@groups.io>
> *Sent: *Wednesday, March 3, 2021 3:38 PM
> *To: *Dong, Guo <mailto:guo.dong@intel.com>; Michael Kubacki 
> <mailto:mikuback@linux.microsoft.com>; devel@edk2.groups.io 
> <mailto:devel@edk2.groups.io>
> *Cc: *Chaganty, Rangasai V <mailto:rangasai.v.chaganty@intel.com>; Chiu, 
> Chasel <mailto:chasel.chiu@intel.com>; Desimone, Nathaniel L 
> <mailto:nathaniel.l.desimone@intel.com>; Luo, Heng 
> <mailto:heng.luo@intel.com>; Agyeman, Prince 
> <mailto:prince.agyeman@intel.com>; gaoliming@byosoft.com.cn 
> <mailto:gaoliming@byosoft.com.cn>; Dong, Eric <mailto:eric.dong@intel.com>
> *Subject: *[EXTERNAL] Re: [edk2-devel] [edk2-platforms][RFC] 
> SpiFlashCommonLib Refactor
> 
> Guo,
> I understand your concern that UefiPayloadPkg should NOT use any 
> components outside of edk2 repo (edk2-platform repo in this case).
> If the two SPI drivers you are talking about are the same one, we could 
> go back to put the driver to PcAtchipsetPkg.
> Let's see what the driver contains and then decide where to put.
> 
> Thanks,
> Ray
> 
>  > -----Original Message-----
>  > From: Dong, Guo <guo.dong@intel.com <mailto:guo.dong@intel.com>>
>  > Sent: Thursday, March 4, 2021 7:22 AM
>  > To: Michael Kubacki <mikuback@linux.microsoft.com 
> <mailto:mikuback@linux.microsoft.com>>; devel@edk2.groups.io 
> <mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com 
> <mailto:ray.ni@intel.com>>
>  > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com 
> <mailto:rangasai.v.chaganty@intel.com>>; Chiu, Chasel 
> <chasel.chiu@intel.com <mailto:chasel.chiu@intel.com>>; Desimone, 
> Nathaniel L
>  > <nathaniel.l.desimone@intel.com 
> <mailto:nathaniel.l.desimone@intel.com>>; Luo, Heng <heng.luo@intel.com 
> <mailto:heng.luo@intel.com>>; Agyeman, Prince <prince.agyeman@intel.com 
> <mailto:prince.agyeman@intel.com>>;
>  > gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>; Dong, 
> Eric <eric.dong@intel.com <mailto:eric.dong@intel.com>>
>  > Subject: RE: [edk2-devel] [edk2-platforms][RFC] SpiFlashCommonLib 
> Refactor
>  >
>  >
>  > 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 
> <mailto:mikuback@linux.microsoft.com>>
>  > > Sent: Wednesday, March 3, 2021 3:54 PM
>  > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Dong, Guo 
> <guo.dong@intel.com <mailto:guo.dong@intel.com>>; Ni, Ray
>  > > <ray.ni@intel.com <mailto:ray.ni@intel.com>>
>  > > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com 
> <mailto:rangasai.v.chaganty@intel.com>>; Chiu, Chasel
>  > > <chasel.chiu@intel.com <mailto:chasel.chiu@intel.com>>; Desimone, 
> Nathaniel L
>  > > <nathaniel.l.desimone@intel.com 
> <mailto:nathaniel.l.desimone@intel.com>>; Luo, Heng <heng.luo@intel.com 
> <mailto:heng.luo@intel.com>>;
>  > > Agyeman, Prince <prince.agyeman@intel.com 
> <mailto:prince.agyeman@intel.com>>; gaoliming@byosoft.com.cn 
> <mailto:gaoliming@byosoft.com.cn>;
>  > > Dong, Eric <eric.dong@intel.com <mailto: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 <mailto:ray.ni@intel.com>>
>  > > >> Sent: Monday, March 1, 2021 5:52 PM
>  > > >> To: Michael Kubacki <mikuback@linux.microsoft.com 
> <mailto:mikuback@linux.microsoft.com>>;
>  > > >> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
>  > > >> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com 
> <mailto:rangasai.v.chaganty@intel.com>>; Chiu, Chasel
>  > > >> <chasel.chiu@intel.com <mailto:chasel.chiu@intel.com>>; 
> Desimone, Nathaniel L
>  > > >> <nathaniel.l.desimone@intel.com 
> <mailto:nathaniel.l.desimone@intel.com>>; Luo, Heng <heng.luo@intel.com 
> <mailto:heng.luo@intel.com>>;
>  > > >> Agyeman, Prince <prince.agyeman@intel.com 
> <mailto:prince.agyeman@intel.com>>;
>  > > gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>;
>  > > >> Dong, Eric <eric.dong@intel.com <mailto:eric.dong@intel.com>>; 
> Dong, Guo <guo.dong@intel.com <mailto: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 
> <mailto:mikuback@linux.microsoft.com>>
>  > > >>> Sent: Tuesday, March 2, 2021 3:16 AM
>  > > >>> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Ni, Ray 
> <ray.ni@intel.com <mailto:ray.ni@intel.com>>
>  > > >>> Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com 
> <mailto:rangasai.v.chaganty@intel.com>>; Chiu, Chasel
>  > > >> <chasel.chiu@intel.com <mailto:chasel.chiu@intel.com>>; 
> Desimone, Nathaniel L
>  > > >>> <nathaniel.l.desimone@intel.com 
> <mailto:nathaniel.l.desimone@intel.com>>; Luo, Heng <heng.luo@intel.com 
> <mailto:heng.luo@intel.com>>;
>  > > >> Agyeman, Prince <prince.agyeman@intel.com 
> <mailto:prince.agyeman@intel.com>>;
>  > > >>> gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>; 
> Dong, Eric <eric.dong@intel.com <mailto: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 
> <mailto:rangasai.v.chaganty@intel.com>>
>  > > >>>>> Sent: Monday, March 1, 2021 4:46 PM
>  > > >>>>> To: Ni, Ray <ray.ni@intel.com <mailto: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 
> <mailto:mikuback@linux.microsoft.com>>
>  > > >>>>> Sent: Tuesday, February 16, 2021 4:58 PM
>  > > >>>>> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Ni, 
> Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Chaganty,
>  > > >> Rangasai V <rangasai.v.chaganty@intel.com 
> <mailto:rangasai.v.chaganty@intel.com>>; Chiu,
>  > > >>> Chasel
>  > > >>>>> <chasel.chiu@intel.com <mailto:chasel.chiu@intel.com>>; 
> Desimone, Nathaniel L
>  > > >> <nathaniel.l.desimone@intel.com 
> <mailto:nathaniel.l.desimone@intel.com>>; Luo, Heng <heng.luo@intel.com 
> <mailto:heng.luo@intel.com>>;
>  > > >>>>> Agyeman, Prince <prince.agyeman@intel.com 
> <mailto:prince.agyeman@intel.com>>;
>  > > >> gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>; 
> Dong, Eric <eric.dong@intel.com <mailto: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
>  > > >>>>
>  > > >>>>
>  > > >>>>
>  > > >>>>
>  > > >>>>
>  > > >
>  > > >
>  > > >
>  > > >
>  > > >
> 
> 
>