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

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

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