Re: [edk2-devel] [PATCH 1/1] edk2-platforms: Deduplicate RISC-V SMBIOS

["Leif Lindholm" <leif@nuviainc.com>]

Hi Daniel,

Thanks for this rework. It looks a massive improvement.

On Fri, Aug 07, 2020 at 18:44:44 +0200, Daniel Schaefer wrote:
> There was too much code, which wasn't called but it could have generated those SMBIOS table entries:
> 
> - Type 4 for each core (4xU51, 1xE51)
> - Type 7 L1 instruction/data for each core
> - Type 7 L2 for U54
> - Type 44 for each core
> - Type 4 for the coreplex
> - Type 7 L2 for the coreplex
> 
> Now it only has code for those entries:
> 
> - Type 4 for SOC [1x]
> - Type 7 L1 for SOC [1x] (even though every hart has own L1, but my Laptop's Intel i5 does that also)
> - Type 7 L2 for SOC [1x]
> - Type 44 for each hart, associated with CPU [5x]
> 
> In addition to simplifying the SMBIOS tables, the code for U54 and E51 is
> combined, like Leif suggested in his review.
> 
> Here's what happened to the files:
> 
> Expanded:
> 
> - Platform/RISC-V/PlatformPkg/Library/FirmwareContextProcessorSpecificLib/FirmwareContextProcessorSpecificLib.c
> 
> Deleted file:
> 
> - Silicon/SiFive/E51/Library/PeiCoreInfoHobLib/CoreInfoHob.c
> - Silicon/SiFive/U54MCCoreplex/Library/PeiCoreInfoHobLib/CoreInfoHob.c
> 
> Merged with E51 code into single file:
> 
> - Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c
> 
> Added SMBIOS Type 7 for L1 Cache, removed duplicated SMBIOS (Type 4 and 7 code):
> 
> - Platform/SiFive/U5SeriesPkg/Library/PeiCoreInfoHobLib/CoreInfoHob.c
> 
> Cc: Abner Chang <abner.chang@hpe.com>
> Cc: Gilbert Chen <gilbert.chen@hpe.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Silicon/SiFive/SiFive.dec                     |   2 -
>  .../FreedomU500VC707Board/U500.dsc            |   1 -
>  .../FreedomU540HiFiveUnleashedBoard/U540.dsc  |   1 -
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf   |   1 -
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf   |  47 ----
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf   |   4 +
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf   |  46 ----
>  .../FirmwareContextProcessorSpecificLib.h     |  11 +
>  .../Include/ProcessorSpecificHobData.h        |   3 +-
>  Silicon/SiFive/Include/Library/SiFiveE51.h    |  60 -----
>  Silicon/SiFive/Include/Library/SiFiveU54.h    |  50 ++--
>  .../Include/Library/SiFiveU54MCCoreplex.h     |  55 ----
>  .../FirmwareContextProcessorSpecificLib.c     |  26 ++
>  .../Universal/Pei/PlatformPei/Platform.c      |   2 +-
>  .../Universal/Pei/PlatformPei/Platform.c      |   2 +-
>  .../Library/PeiCoreInfoHobLib/CoreInfoHob.c   |  58 +----
>  .../Library/PeiCoreInfoHobLib/CoreInfoHob.c   | 235 -----------------
>  .../Library/PeiCoreInfoHobLib/CoreInfoHob.c   | 244 +++++++-----------
>  .../Library/PeiCoreInfoHobLib/CoreInfoHob.c   | 184 -------------
>  19 files changed, 178 insertions(+), 854 deletions(-)

I know you dropped some tables, but that's a *nice* diffstat.

I guess this is effectively meant to be folded into existing commits?
If so:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
If not, I might start grumbling about some unrelated cleanup in this
patch...

/
    Leif