Re: [edk2-platforms][PATCH V2 02/11] Platform/Sgi: Add GetProductId API for SGI/RD Platforms

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Pranav,

Please find my feedback inline marked [SAMI].

With those addressed.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 16/05/2021 10:29 AM, Pranav Madhu wrote:
> Add GetProductId API for SGI/RD Platform. The API returns a product id
> in integer format based on the platform description data. The product id
> is required for other drivers such as SMBIOS.
>
> Signed-off-by: Pranav Madhu <pranav.madhu@arm.com>
> ---
>   Platform/ARM/SgiPkg/Include/SgiPlatform.h             | 21 +++++
>   Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c | 86 +++++++++++++++++++-
>   2 files changed, 106 insertions(+), 1 deletion(-)
>
> diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> index 1c5366878712..142fa29bb9fa 100644
> --- a/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> +++ b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> @@ -68,4 +68,25 @@ typedef struct {
>     UINTN  MultiChipMode;
>   } SGI_PLATFORM_DESCRIPTOR;
>   
> +// Arm SGI/RD Product IDs
> +enum ARM_RD_PRODUCT_ID {
> +  UnknownId = 0,
> +  Sgi575,
> +  RdN1Edge,
> +  RdN1EdgeX2,
> +  RdE1Edge,
> +  RdV1,
> +  RdV1Mc,
> +  RdN2
> +};
[SAMI]
> +
> +// Arm ProductId look-up table
> +typedef struct {
> +  UINTN  ProductId;
> +  UINTN  PlatformId;
> +  UINTN  ConfigId;
> +  UINTN  MultiChipMode;
> +} SGI_PRODUCT_ID_LOOKUP;
> +
> +UINT8 SgiGetProductId ( VOID );
[SAMI] Please add doxygen header for this function. Also there should 
not be spaces before and after VOID.
[/SAMI]
>   #endif // __SGI_PLATFORM_H__
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> index 9731d7cccede..57b1658ab0af 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> @@ -1,6 +1,6 @@
>   /** @file
>   *
> -*  Copyright (c) 2018, ARM Limited. All rights reserved.
> +*  Copyright (c) 2018-2021, ARM Limited. All rights reserved.
>   *
>   *  SPDX-License-Identifier: BSD-2-Clause-Patent
>   *
> @@ -8,9 +8,12 @@
>   
>   #include <Library/ArmPlatformLib.h>
>   #include <Library/BaseLib.h>
> +#include <Library/HobLib.h>
>   #include <Ppi/ArmMpCoreInfo.h>
>   #include <Ppi/SgiPlatformId.h>
>   
> +#include "SgiPlatform.h"
> +
>   UINT64 NtFwConfigDtBlob;
>   STATIC SGI_NT_FW_CONFIG_INFO_PPI mNtFwConfigDtInfoPpi;
>   
> @@ -21,6 +24,51 @@ STATIC ARM_CORE_INFO mCoreInfoTable[] = {
>     },
>   };
>   
> +STATIC SGI_PRODUCT_ID_LOOKUP SgiProductIdLookup[] = {
[SAMI] Can this be made STATIC CONST?
[/SAMI]
> +  {
> +    Sgi575,
> +    SGI575_PART_NUM,
> +    SGI575_CONF_NUM,
> +    0
> +  },
> +  {
> +    RdN1Edge,
> +    RD_N1E1_EDGE_PART_NUM,
> +    RD_N1_EDGE_CONF_ID,
> +    0
> +  },
> +  {
> +    RdN1EdgeX2,
> +    RD_N1E1_EDGE_PART_NUM,
> +    RD_N1_EDGE_CONF_ID,
> +    1
> +  },
> +  {
> +    RdE1Edge,
> +    RD_N1E1_EDGE_PART_NUM,
> +    RD_E1_EDGE_CONF_ID,
> +    0
> +  },
> +  {
> +    RdV1,
> +    RD_V1_PART_NUM,
> +    RD_V1_CONF_ID,
> +    0
> +  },
> +  {
> +    RdV1Mc,
> +    RD_V1_PART_NUM,
> +    RD_V1_MC_CONF_ID,
> +    1
> +  },
> +  {
> +    RdN2,
> +    RD_N2_PART_NUM,
> +    RD_N2_CONF_ID,
> +    0
> +  }
> +};
> +
>   EFI_BOOT_MODE
>   ArmPlatformGetBootMode (
>     VOID
> @@ -75,3 +123,39 @@ ArmPlatformGetPlatformPpiList (
>     *PpiListSize = sizeof (gPlatformPpiTable);
>     *PpiList = gPlatformPpiTable;
>   }
> +
> +/**
> +  Derermine the product ID.
> +
> +  Determine the product ID by using the data in the Platform ID Descriptor HOB
> +  to lookup for a matching product ID.
> +
> +  @retval Zero           Failed identify platform.
> +  @retval Others         ARM_RD_PRODUCT_ID of the identified platform.
> +**/
> +UINT8
> +SgiGetProductId (
> +         VOID
[SAMI] Allign VOID at 2 spaces from the start of the line.
[/SAMI]
> +  )
> +{
> +  VOID *SystemIdHob;
> +  UINT8 Idx;
> +  SGI_PLATFORM_DESCRIPTOR *HobData;
> +
> +  SystemIdHob = GetFirstGuidHob (&gArmSgiPlatformIdDescriptorGuid);
> +  if (SystemIdHob == NULL) {
> +    return UnknownId;
> +  }
> +
> +  HobData = (SGI_PLATFORM_DESCRIPTOR *)GET_GUID_HOB_DATA (SystemIdHob);
> +
> +  for (Idx = 0; Idx < ARRAY_SIZE (SgiProductIdLookup); Idx++) {
> +    if ((HobData->PlatformId == SgiProductIdLookup[Idx].PlatformId) &&
> +        (HobData->ConfigId == SgiProductIdLookup[Idx].ConfigId) &&
> +        (HobData->MultiChipMode == SgiProductIdLookup[Idx].MultiChipMode)) {
> +      return SgiProductIdLookup[Idx].ProductId;
> +    }
> +  }
> +
> +  return UnknownId;
> +}