Re: [PATCH v2 edk2-platforms 1/5] Platform/ARM/Sgi: Adapt to changes in system-id DT node.

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

On Mon, Nov 05, 2018 at 02:56:55PM +0530, Chandni Cherukuri wrote:
> The 'system-id' node of HW_CONFIG device tree has been updated to have
> a new property 'config-id' to hold the platform configuration value.
> Prior to this, configuration ID value was represented by the the upper
> four bits of the 'platform ID' property value but it now has a seperate
> property of its own in the 'system-id' node. So adapt to these changes
> in the 'system-id' node.

This gets a bit hairy semantically:
- PlatformId used to be the contents of node "platform-id"
- PlatformId is now the union of the lower 28 bits of node
  "platform-id" and the 4 bottom bits of new node "config-id",
- The result is still called PlatformId.

Is there some better way of describing this? Should the function name
change?

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chandni Cherukuri <chandni.cherukuri@arm.com>
> ---
>  .../SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c   | 19 ++++++++++++++++++-

Can you ensure you generate patches in accordance with the
instructions at
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
?

>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
> index 081d329..8b6c71b 100644
> --- a/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
> @@ -42,6 +42,8 @@ GetSgiPlatformId (

Since this is changing what the return value of the function means,
can the function description comment be updated as well?

>    CONST VOID                    *HwCfgDtBlob;
>    SGI_HW_CONFIG_INFO_PPI        *HwConfigInfoPpi;
>    EFI_STATUS                    Status;
> +  UINT32                        PlatformId;
> +  UINT32                        ConfigId;
>  
>    Status = PeiServicesLocatePpi (&gHwConfigDtInfoPpiGuid, 0, NULL,
>               (VOID**)&HwConfigInfoPpi);
> @@ -69,7 +71,22 @@ GetSgiPlatformId (
>      return 0;
>    }
>  
> -  return fdt32_to_cpu (*Property);
> +  PlatformId = fdt32_to_cpu (*Property);
> +
> +  Property = fdt_getprop (HwCfgDtBlob, Offset, "config-id", NULL);
> +  if (Property == NULL) {
> +    DEBUG ((DEBUG_ERROR, "Config Id is NULL\n"));
> +    return 0;
> +  }
> +
> +  ConfigId = fdt32_to_cpu (*Property);
> +
> +  // Concatenate the value of config ID into the platform ID value with
> +  // config ID occupying the upper 4 bits of platform ID.
> +  ConfigId = ConfigId & SGI_CONFIG_MASK;

We are here silently discarding 28 bits of data that need to be set to
0 for the below operation to make sense.
I think an ASSERT might be in order.

I also note that we've neither looked at nor cleared the top four bits
of PlatformId.

> +  PlatformId = PlatformId | (ConfigId << SGI_CONFIG_SHIFT);
> +
> +  return PlatformId;

I would be happier with just the return statement - reusing the
PlatformId variable does not simplify anything.

/
    Leif.

>  }
>  
>  /**
> -- 
> 2.7.4
>