Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 01/14] Silicon/SiFive: Initial version of SiFive silicon package

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Thu, Sep 19, 2019 at 11:51:18AM +0800, Gilbert Chen wrote:
> Add SiFive silicon EDK2 metafile and header files of
>  SiFive RISC-V cores.
> 
> Signed-off-by: Gilbert Chen <gilbert.chen@hpe.com>
> ---
>  Silicon/SiFive/Include/Library/SiFiveE51.h         | 60 ++++++++++++++++++++++
>  Silicon/SiFive/Include/Library/SiFiveU54.h         | 60 ++++++++++++++++++++++
>  .../SiFive/Include/Library/SiFiveU54MCCoreplex.h   | 55 ++++++++++++++++++++
>  Silicon/SiFive/SiFive.dec                          | 39 ++++++++++++++
>  4 files changed, 214 insertions(+)
>  create mode 100644 Silicon/SiFive/Include/Library/SiFiveE51.h
>  create mode 100644 Silicon/SiFive/Include/Library/SiFiveU54.h
>  create mode 100644 Silicon/SiFive/Include/Library/SiFiveU54MCCoreplex.h
>  create mode 100644 Silicon/SiFive/SiFive.dec

Hmm, this seems a bit sideways to me.
This patch adds the headers for 3 different libraries. The subsequent
patch adds the code for those 3 different libraries.

Heck, this is an initial bootstrap of several new platforms, I would
even take all 3 libraries (with headers) as a single patch.

But there is a greater issue here which I will cover in more detail in
reply to the next patch; these are 3 near-identical libraries
fulfilling the same function given their own global header files and
each implementing their own library class.

> 
> diff --git a/Silicon/SiFive/Include/Library/SiFiveE51.h b/Silicon/SiFive/Include/Library/SiFiveE51.h
> new file mode 100644
> index 00000000..5faea5c7
> --- /dev/null
> +++ b/Silicon/SiFive/Include/Library/SiFiveE51.h
> @@ -0,0 +1,60 @@
> +/** @file
> +  SiFive E51 Core library definitions.
> +
> +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#ifndef _SIFIVE_E51_CORE_H_
> +#define _SIFIVE_E51_CORE_H_

Please drop leading _.
This applies to all header files throughout the set.
I will not point each one out.

> +
> +#include <PiPei.h>
> +
> +#include <SmbiosProcessorSpecificData.h>
> +#include <ProcessorSpecificDataHob.h>
> +
> +/**
> +  Function to build core specific information HOB.
> +
> +  @param  ParentProcessorGuid    Parent processor od this core. ParentProcessorGuid
> +                                 could be the same as CoreGuid if one processor has
> +                                 only one core.
> +  @param  ParentProcessorUid     Unique ID of pysical processor which owns this core.
> +  @param  HartId                 Hart ID of this core.
> +  @param  IsBootHart             TRUE means this is the boot HART.
> +  @param  GuidHobData            Pointer to receive RISC_V_PROCESSOR_SPECIFIC_DATA_HOB.
> +
> +  @return EFI_SUCCESS     The PEIM initialized successfully.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +CreateE51CoreProcessorSpecificDataHob (
> +  IN EFI_GUID  *ParentProcessorGuid,
> +  IN UINTN     ParentProcessorUid,
> +  IN UINTN     HartId,
> +  IN BOOLEAN   IsBootHart,
> +  OUT RISC_V_PROCESSOR_SPECIFIC_DATA_HOB **GuidHobData
> +  );
> +
> +/**
> +  Function to build processor related SMBIOS information. RISC-V SMBIOS DXE driver collect
> +  this information and build SMBIOS Type4 and Type7 record.
> +
> +  @param  ProcessorUid    Unique ID of pysical processor which owns this core.
> +  @param  SmbiosHobPtr    Pointer to receive RISC_V_PROCESSOR_SMBIOS_DATA_HOB. The pointers
> +                          maintained in this structure is only valid before memory is discovered.
> +                          Access to those pointers after memory is installed will cause unexpected issues.
> +
> +  @return EFI_SUCCESS     The PEIM initialized successfully.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +CreateE51ProcessorSmbiosDataHob (
> +  IN UINTN     ProcessorUid,
> +  OUT RISC_V_PROCESSOR_SMBIOS_DATA_HOB **SmbiosHobPtr
> +  );
> +
> +#endif
> diff --git a/Silicon/SiFive/Include/Library/SiFiveU54.h b/Silicon/SiFive/Include/Library/SiFiveU54.h
> new file mode 100644
> index 00000000..2e3a1c75
> --- /dev/null
> +++ b/Silicon/SiFive/Include/Library/SiFiveU54.h
> @@ -0,0 +1,60 @@
> +/** @file
> +  SiFive U54 Core library definitions.
> +
> +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#ifndef _SIFIVE_U54_CORE_H_
> +#define _SIFIVE_U54_CORE_H_
> +
> +#include <PiPei.h>
> +
> +#include <SmbiosProcessorSpecificData.h>
> +#include <ProcessorSpecificDataHob.h>
> +
> +/**
> +  Function to build core specific information HOB.
> +
> +  @param  ParentProcessorGuid    Parent processor od this core. ParentProcessorGuid
> +                                 could be the same as CoreGuid if one processor has
> +                                 only one core.
> +  @param  ParentProcessorUid     Unique ID of pysical processor which owns this core.
> +  @param  HartId                 Hart ID of this core.
> +  @param  IsBootHart             TRUE means this is the boot HART.
> +  @param  GuidHobdata            Pointer to RISC_V_PROCESSOR_SPECIFIC_DATA_HOB.
> +
> +  @return EFI_SUCCESS     The PEIM initialized successfully.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +CreateU54CoreProcessorSpecificDataHob (
> +  IN EFI_GUID  *ParentProcessorGuid,
> +  IN UINTN     ParentProcessorUid,
> +  IN UINTN     HartId,
> +  IN BOOLEAN   IsBootHart,
> +  OUT RISC_V_PROCESSOR_SPECIFIC_DATA_HOB **GuidHobdata
> +  );
> +
> +/**
> +  Function to build processor related SMBIOS information. RISC-V SMBIOS DXE driver collect
> +  this information and build SMBIOS Type4 and Type7 record.
> +
> +  @param  ProcessorUid    Unique ID of pysical processor which owns this core.
> +  @param  SmbiosHobPtr    Pointer to receive RISC_V_PROCESSOR_SMBIOS_DATA_HOB. The pointers
> +                          maintained in this structure is only valid before memory is discovered.
> +                          Access to those pointers after memory is installed will cause unexpected issues.
> +
> +  @return EFI_SUCCESS     The PEIM initialized successfully.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +CreateU54ProcessorSmbiosDataHob (
> +  IN UINTN     ProcessorUid,
> +  IN RISC_V_PROCESSOR_SMBIOS_DATA_HOB **SmbiosHobPtr
> +  );
> +
> +#endif
> diff --git a/Silicon/SiFive/Include/Library/SiFiveU54MCCoreplex.h b/Silicon/SiFive/Include/Library/SiFiveU54MCCoreplex.h
> new file mode 100644
> index 00000000..3d23b34c
> --- /dev/null
> +++ b/Silicon/SiFive/Include/Library/SiFiveU54MCCoreplex.h
> @@ -0,0 +1,55 @@
> +/** @file
> +  SiFive U54 Coreplex library definitions.
> +
> +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#ifndef _SIFIVE_U54MC_COREPLEX_CORE_H_
> +#define _SIFIVE_U54MC_COREPLEX_CORE_H_
> +
> +#include <PiPei.h>
> +
> +#include <SmbiosProcessorSpecificData.h>
> +#include <ProcessorSpecificDataHob.h>
> +
> +#define SIFIVE_U54MC_COREPLEX_E51_HART_ID     0
> +#define SIFIVE_U54MC_COREPLEX_U54_0_HART_ID   1
> +#define SIFIVE_U54MC_COREPLEX_U54_1_HART_ID   2
> +#define SIFIVE_U54MC_COREPLEX_U54_2_HART_ID   3
> +#define SIFIVE_U54MC_COREPLEX_U54_3_HART_ID   4
> +
> +/**
> +  Build up U54MC coreplex processor core-specific information.
> +
> +  @param  UniqueId      U54MC unique ID.
> +
> +  @return EFI_STATUS
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +CreateU54MCCoreplexProcessorSpecificDataHob (
> +  IN UINTN UniqueId
> +  );
> +
> +/**
> +  Function to build processor related SMBIOS information. RISC-V SMBIOS DXE driver collect
> +  this information and build SMBIOS Type4 and Type7 record.
> +
> +  @param  ProcessorUid    Unique ID of pysical processor which owns this core.
> +  @param  SmbiosHobPtr    Pointer to receive RISC_V_PROCESSOR_SMBIOS_DATA_HOB. The pointers
> +                          maintained in this structure is only valid before memory is discovered.
> +                          Access to those pointers after memory is installed will cause unexpected issues.
> +
> +  @return EFI_SUCCESS     The PEIM initialized successfully.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +CreateU54MCProcessorSmbiosDataHob (
> +  IN UINTN     ProcessorUid,
> +  IN RISC_V_PROCESSOR_SMBIOS_DATA_HOB **SmbiosHobPtr
> +  );
> +#endif
> diff --git a/Silicon/SiFive/SiFive.dec b/Silicon/SiFive/SiFive.dec
> new file mode 100644
> index 00000000..7aca3e75
> --- /dev/null
> +++ b/Silicon/SiFive/SiFive.dec
> @@ -0,0 +1,39 @@
> +## @file
> +#  SiFive silicon package definitions
> +#
> +#  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  DEC_SPECIFICATION              = 0x00010005

Please use the current specification version, unless you have need of
building with an older version of BaseTools. (In which case, use that
specification version.)

> +  PACKAGE_NAME                   = SiFiveSiliconPkg
> +  PACKAGE_GUID                   = 576912B2-7077-4B78-A934-4C133FEB20BB
> +  PACKAGE_VERSION                = 1.0
> +
> +[Includes]
> +  Include                        # Root include for the package
> +
> +[LibraryClasses]
> +
> +[Guids]
> +  gEfiSiFiveSiliconSpaceGuid  = {0x5F3E9E15, 0x8FFC, 0x4F53, { 0x8E, 0x64, 0x92, 0x0B, 0xA5, 0x39, 0x81, 0xB0 }}

TokenSpaceGuid, not just SpaceGuid.

/
    Leif

> +
> +[Protocols]
> +
> +[PcdsFixedAtBuild]
> +  # E51 Core GUID
> +  gEfiSiFiveSiliconSpaceGuid.PcdSiFiveE51CoreGuid |{0xD4, 0x69, 0x54, 0x87, 0x96, 0x96, 0x48, 0x7F, 0x9F, 0x57, 0xB6, 0xF1, 0xDE, 0x7D, 0x97, 0x42}|VOID*|0x00001000
> +  # U54 Core GUID
> +  gEfiSiFiveSiliconSpaceGuid.PcdSiFiveU54CoreGuid |{0x64, 0x70, 0xF6, 0x90, 0x11, 0x59, 0x47, 0xF1, 0xB8, 0xD5, 0xCF, 0x89, 0x10, 0xC5, 0x30, 0x20}|VOID*|0x00001001
> +  # U54 MC Coreplex GUID
> +  gEfiSiFiveSiliconSpaceGuid.PcdSiFiveU54MCCoreplexGuid |{0x67, 0xBF, 0x15, 0xD9, 0x7E, 0x4F, 0x48, 0x27, 0x87, 0x19, 0x79, 0x0B, 0xA6, 0x22, 0x7C, 0xBE}|VOID*|0x00001002
> +  # U5 MC Coreplex GUID
> +  gEfiSiFiveSiliconSpaceGuid.PcdSiFiveU5MCCoreplexGuid |{0x06, 0x38, 0x9F, 0x33, 0xF9, 0xDB, 0x43, 0x13, 0x9A, 0x9B, 0x1C, 0x68, 0xD6, 0x04, 0xEA, 0xFF}|VOID*|0x00001003
> +
> +[PcdsDynamic, PcdsDynamicEx]
> +
> +[PcdsFeatureFlag]
> +
> -- 
> 2.12.0.windows.1
> 
> 
> 
>