[edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 26/29] RiscVPkg/SmbiosDxe: Generic SMBIOS DXE driver for RISC-V platforms.

[Leif Lindholm]

On Mon, Oct 14, 2019 at 11:27:51AM +0000, Abner Chang wrote:
> 
> 
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif.lindholm at linaro.org]
> > Sent: Tuesday, October 1, 2019 6:40 AM
> > To: devel at edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> > <abner.chang at hpe.com>
> > Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 26/29]
> > RiscVPkg/SmbiosDxe: Generic SMBIOS DXE driver for RISC-V platforms.
> > 
> > On Mon, Sep 23, 2019 at 08:31:52AM +0800, Abner Chang wrote:
> > > RISC-V generic SMBIOS DXE driver for building up SMBIOS type 4, type 7
> > > and type 44 records.
> > >
> > > Signed-off-by: Abner Chang <abner.chang at hpe.com>
> > > ---
> > >  RiscVPkg/Include/ProcessorSpecificDataHob.h        |  95 ++++++
> > >  RiscVPkg/Include/SmbiosProcessorSpecificData.h     |  58 ++++
> > >  RiscVPkg/RiscVPkg.dec                              |   6 +
> > >  RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.c      | 339
> > +++++++++++++++++++++
> > >  RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.h      |  32 ++
> > >  RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.inf    |  58 ++++
> > >  RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.uni    |  12 +
> > >  .../Universal/SmbiosDxe/RiscVSmbiosDxeExtra.uni    |  13 +
> > >  8 files changed, 613 insertions(+)
> > >  create mode 100644 RiscVPkg/Include/ProcessorSpecificDataHob.h
> > >  create mode 100644 RiscVPkg/Include/SmbiosProcessorSpecificData.h
> > >  create mode 100644 RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.c
> > >  create mode 100644 RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.h
> > >  create mode 100644 RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.inf
> > >  create mode 100644 RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxe.uni
> > >  create mode 100644
> > > RiscVPkg/Universal/SmbiosDxe/RiscVSmbiosDxeExtra.uni
> > >
> > > diff --git a/RiscVPkg/Include/ProcessorSpecificDataHob.h
> > > b/RiscVPkg/Include/ProcessorSpecificDataHob.h
> > > new file mode 100644
> > > index 0000000..6798a9d
> > > --- /dev/null
> > > +++ b/RiscVPkg/Include/ProcessorSpecificDataHob.h
> > 
> > None of the things defined in here are HOBs, they are structures to hold
> > information that will be put into HOBs.
> > Can we merge all of these definitions into SmbiosProcessorSpecificData.h
> > and delete this file?
>
> No. SmbiosProcessorSpecificData.h defines the structure declared in
> RISC-V SMBIOS processor specific data spec
> (https://github.com/riscv/riscv-smbios).
> ProcessorSpecificDataHob is the implementation to deliver processor
> information in HOB for building SMBIOS type 44 record.

Fair enough.
Nevertheless, I find it confusing to refer to the structures being
bundled together into a HOB as if theye were themselves HOB.

An alternative naming scheme for me woud be to rename the file
ProcessorSpecificHobData.h and the structs _HOB_DATA instead of
_DATA_HOB. But I am fully open to replacing these with better names.

> I would like to keep these two files separately.

This is fine, as long as the above concern is addressed.

> > > @@ -0,0 +1,95 @@
> > > +/** @file
> > > +  Definition of Processor Specific Data HOB.
> > > +
> > > +  Copyright (c) 2019, Hewlett Packard Enterprise Development LP. All
> > > + rights reserved.<BR>
> > > +
> > > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +
> > > +**/
> > > +#ifndef _RISC_V_PROCESSOR_SPECIFIC_DATA_HOB_H_
> > > +#define _RISC_V_PROCESSOR_SPECIFIC_DATA_HOB_H_
> > 
> > Please drop leading _.
> > 
> > > +
> > > +#include <IndustryStandard/SmBios.h>
> > 
> > This file also uses Uefi.h, please include it, so we don't depend on other files
> > pulling it in for us.
> > 
> > > +
> > > +#define TO_BE_FILLED 0
> > > +#define TO_BE_FILLED_BY_VENDOR 0
> > > +#define TO_BE_FILLED_BY_RISC_V_SMBIOS_DXE_DRIVER 0 #define
> > > +TO_BE_FILLED_BY_CODE 0
> > 
> > These defines are never used, please drop,
>
> These definitions are used in platform code to indicates the value
> is not set correctly and should be set by certain code.

Yes they are, sorry. I had inadvertently checked out the wrong branch
when searching for its use. This and all such related comments can be
ignored.

Best Regards,

Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48894): https://edk2.groups.io/g/devel/message/48894
Mute This Topic: https://groups.io/mt/34258224/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-