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

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

Hi Chandni,

On Mon, Nov 12, 2018 at 07:46:38PM +0530, chandni cherukuri wrote:
> > > > 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.

Thanks.

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

Understood. But I think this cleaner end result is well worth the
larger patch.

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

This bit would no longer be needed if the change is made, and the
function returns the struct directly.

Best Regards,

Leif