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

[chandni cherukuri <chandni.cherukuri@arm.com>]

On Mon, Nov 12, 2018 at 4:55 PM Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> +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.
>

Ok, will change the name of this function in the next version.

> *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?
>
The intention was to not modify the existing code due to the changes
in the 'system-id' device tree node. And this was done by
concatenating the platform-id and config-id value into one. So this
was the reason for not adding a 'config-id' member in the
SGI_PLATFORM_DESCRIPTOR structure.

> 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.
>

Ok. The preference was given to keep the format and the PlatformID
parsing code as before, which, otherwise would need lot more changes
to the code.
> > > >    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;
>
> .
Ok. will fix this in the next version.

Thanks
Chandni

>
> 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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel