Re: [PATCH edk2-platforms 1/4] Platform/Solidrun: Add Hummingboard SmBios

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

On Thu, Aug 09, 2018 at 05:42:02AM +0000, Chris Co wrote:
> Hi Leif,
> 
> > -----Original Message-----
> > From: Leif Lindholm <leif.lindholm@linaro.org>
> > Sent: Tuesday, August 7, 2018 5:50 AM
> > To: Chris Co <Christopher.Co@microsoft.com>
> > Cc: edk2-devel@lists.01.org; Michael D Kinney
> > <michael.d.kinney@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Subject: Re: [PATCH edk2-platforms 1/4] Platform/Solidrun: Add
> > Hummingboard SmBios
> > 
> > I'll start trickling through some generic comments on the code, apart from
> > the general ones I made before.
> > 
> 
> Thank you for these comments and continuing with the review.  I do
> sincerely apologize about all the code style issues.  I should have
> been more rigorous about enforcing code style before sending it up
> (It was a lot worse before :-( )

Oh, that's what I expected, don't worry about it.
I just want the code that goes in to be as clean as practical, since
it may be used for reference for future contributions.

I'm also getting more strict as time goes on as I see previous
sloppiness I accepted in the past coming back to haunt me in new
patches.

> I am working on generating the new patch set.  Currently about
> halfway though.  I will incorporate these comments into it.  Should
> have it ready by early next week.  That said, if you do have more
> feedback on other patches, I would be glad to incorporate them in as
> soon as they are available.

FYI, I will be off Friday-Monday. So anything you don't see by end of
today won't arrive before Tuesday - and no rush getting anything out
Monday.
 
> > > +SMBIOS_TABLE_TYPE1 mSysInfoType1 = {
> > > +  { EFI_SMBIOS_TYPE_SYSTEM_INFORMATION, sizeof
> > (SMBIOS_TABLE_TYPE1), 0 },
> > > +  1,    // Manufacturer String
> > > +  2,    // ProductName String
> > > +  3,    // Version String
> > > +  4,    // SerialNumber String
> > > +  { 0x25EF0280, 0xEC82, 0x42B0, { 0x8F, 0xB6, 0x10, 0xAD, 0xCC, 0xC6,
> > > +0x7C, 0x02 } },
> > > +  SystemWakeupTypePowerSwitch,
> > > +  5,    // SKUNumber String
> > > +  6,    // Family String
> > > +};
> > > +CHAR8  *mSysInfoType1Strings[] = {
> > > +  "SolidRun",
> > > +  "HummingBoard-Edge i4Pro",
> > > +  "2.0",
> > > +  "System Serial#",
> > > +  "hb04w-e-110-00-004-0",
> > 
> > Do we have any way of obtaining the three above programmatically?
> > 
> 
> This is the default template for Sysinfo, which gets overwritten at
> runtime.  Given that we are overwriting this table at runtime, I'm
> thinking it would be better to just generate the table runtime and
> avoid confusion by having this template.

Agreed.

> > > +VOID
> > > +BIOSInfoUpdateSmbiosType0 (
> > > +  VOID
> > > +  )
> > > +{
> > > +  LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER *)&mBIOSInfoType0,
> > > +mBIOSInfoType0Strings, NULL); }
> > > +
> > > +VOID
> > > +I64ToHexString(
> > 
> > Hmm. We already have several copies of functions doing exactly this in EDK2.
> > Just no centrally reusable one. Maybe worth adding to BaseLib?
> > 
> > Regardless, it should probablt be called UINT64ToHexString or suchlike.
> > 
> 
> I'll rename it to UINT64ToHexString for the next patch set, then
> will look into making a centrally reusable version.

Thanks!

> > > +   BoardSerial = ((UINT64)(MmioRead32(RegOCOTP_CFG1Addr))) << 32;
> > > +   BoardSerial = BoardSerial |
> > > + ((UINT64)(MmioRead32(RegOCOTP_CFG0Addr)));
> > 
> > MmioRead32 (
> > A bit heavy on the parantheses.
> > 
> 
> Will fix.

For clarity: I'm all for using more parantheses than necessary where
it means avoiding needing to stop and think about operator precedence.
Ard isn't :)

But here they just don't add anything other than clutter.

Regards,

Leif