[edk2-platforms][RFC] SpiFlashCommonLib Refactor

[edk2-platforms][RFC] SpiFlashCommonLib Refactor

[Michael Kubacki]

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

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

[Ni, Ray]

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

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

[Michael Kubacki]

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
> 
> 
> 
> 
> 

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

[Ni, Ray]

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
> >
> >
> > 
> >
> >

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

[Guo Dong]

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
> > >
> > >
> > > 
> > >
> > >

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

[Michael Kubacki]

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
>>>>
>>>>
>>>>
>>>>
>>>>
> 
> 
> 
> 
> 

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

[Guo Dong]

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
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >
> >
> > 
> >
> >

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

[Ni, Ray]

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>
> Sent: Thursday, March 4, 2021 7:22 AM
> To: Michael Kubacki <mikuback@linux.microsoft.com>; 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 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
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >
> > >
> > > 
> > >
> > >

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

[Bret Barkelew]

[-- Attachment #1: Type: text/plain, Size: 12762 bytes --]

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>
> Sent: Thursday, March 4, 2021 7:22 AM
> To: Michael Kubacki <mikuback@linux.microsoft.com>; 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 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
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >
> > >
> > >
> > >
> > >






[-- Attachment #2: Type: text/html, Size: 20616 bytes --]

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

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 15266 bytes --]

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
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >
> > >
> > >
> > >
> > >





[-- Attachment #2: Type: text/html, Size: 24743 bytes --]

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

[Michael Kubacki]

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
>  > > >>>>
>  > > >>>>
>  > > >>>>
>  > > >>>>
>  > > >>>>
>  > > >
>  > > >
>  > > >
>  > > >
>  > > >
> 
> 
>