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

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

+edk2-devel

On Mon, Nov 12, 2018 at 04:28:35PM +0530, chandni cherukuri wrote:
> On Fri, Nov 9, 2018 at 9:59 PM Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > 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?
> >
> Before the 'system-id'  node is represented as -
> system-id {
>    platform-id = <>; /* Consists of both PART NUMBER and Configuration ID */
> }
> 
> Earlier for the 'platform-id' property, the upper 4 bits consisted of
> the configuration
> number and the lower 12 bits consisted of the part number of the platform but
> now that value has been split across two properties ('platform-id' and
> 'config-id')
> , 'platform-id' consists of the part number and 'config-id' consists
> of the configuration
> number of the platform.
> 
> Currently the system-id node is represented as below -
> system-id {
>    platform-id = <>;     /* PART NUMBER */
>     config-id = <>;       /* Configuration ID */
> }
> 
> So even now the contents of the 'PlatformId' is still the same. To
> account for the
> changes done in the 'system-id' device tree node the code as been
> modified so that
> the 'PlatformId' variable still consists of both the PART NUMBER and
> Configuration ID
> of the platform.

OK, so given that (excellent) explanation, it sounds to me like the
function should be renamed (more below).

> > > 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
> > ?
> >
> ok. will generate the patches accordingly.
> 
> > >  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?
> >
> The return value of the function is still the same as before. The function
> returns the 'PlatformId' variable in which the upper 4 bits consists of the
> Configuration ID and the lower 12 bits consists of the part number of
> the platform.

Yes, but the fact that the return value is numerically the same is
irrelevant; it now means something different than what it used to be.

Based on your explanation above, I would suggest a more appropriate
name for this function would now be GetSgiSystemId.

*however* ... it also makes me question why this change was made in
this way? If we're changing the underlying data structure, why are we
not also changing SGI_PLATFORM_DESCRIPTOR to reflect this?

If we do that, we can then get rid of the opposite shifting/masking in
ArmSgiPkgEntryPoint() and nuke SGI_PART_NUM_MASK, SGI_CONFIG_SHIFT and
SGI_CONFIG_MASK completely.

> > >    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.
> >
> okay. will introduce an assert if the upper 28 bits of data for
> ConfigId is not 0.
> 
> > I also note that we've neither looked at nor cleared the top four bits
> > of PlatformId.
> >
> Okay. will introduce an ASSERT if the upper 4 bits of PlatformId id not 0.
> 
> > > +  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.
> >
>  I did not understand this point.

Merely that

  return PlatformId | (ConfigId << SGI_CONFIG_SHIFT);
  
is easier to read than (and hence preferable to)

  PlatformId = PlatformId | (ConfigId << SGI_CONFIG_SHIFT);

  return PlatformId;

.

But depending on your take on my comments above, this may now be
irrelevant.

Regards,

Leif

> > /
> >     Leif.
> >
> > >  }
> > >
> > >  /**
> > > --
> > > 2.7.4
> > >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel