Re: [edk2-devel] [RFC PATCH 1/1] MdePkg/IndustryStandard: add definitions for ACPI 6.4 CEDT

["Michael D Kinney" <michael.d.kinney@intel.com>]

For this MdePkg change to add an ACPI table type, do you mind opening a PR?

There are some minor code style issues that need to be addressed.

Structure type names and define names should be all upper case.

	__CXL_Early_Discovery_TABLE_H__ -> __CXL_EARLY_DISCOVERY_TABLE_H__

File names should be camel case.

	CXLEarlyDiscoveryTable.h -> CxlEarlyDiscoveryTable.h

Also, please provide links to the supporting public specifications in
the include file headers.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jonathan
> Cameron via groups.io
> Sent: Friday, August 30, 2024 4:17 AM
> To: Yuquan Wang <wangyuquan1236@phytium.com.cn>
> Cc: marcin.juszkiewicz@linaro.org; gaoliming@byosoft.com.cn; Kinney,
> Michael D <michael.d.kinney@intel.com>; ardb+tianocore@kernel.org;
> chenbaozi@phytium.com.cn; wangyinfeng@phytium.com.cn;
> shuyiqi@phytium.com.cn; qemu-devel@nongnu.org; devel@edk2.groups.io
> Subject: Re: [edk2-devel] [RFC PATCH 1/1] MdePkg/IndustryStandard: add
> definitions for ACPI 6.4 CEDT
> 
> On Fri, 30 Aug 2024 10:11:17 +0800
> Yuquan Wang <wangyuquan1236@phytium.com.cn> wrote:
> One request - when cross posting to multiple lists, it is useful to
> add something to the patch title to make it clear what is EDK2, what
> is QEMU etc.
> 
> [RFC EDK2 PATCH 1/1]
> 
> It might irritate the EDK2 folk but it is useful for everyone else
> to figure out what they are looking at.
> 
> > This adds #defines and struct typedefs for the various structure
> > types in the ACPI 6.4 CXL Early Discovery Table (CEDT).
> 
> It's in the CXL spec, not ACPI spec so call out in this
> patch description that all that was added in ACPI 6.4 was
> the reservation of the ID.  The rest needs to refer to appropriate
> CXL specifications.
> 
> For naming I have no idea on the EDK2 Convention for
> structures in specifications other than ACPI that are for
> ACPI structures.  The current one is definitely missleading
> however.
> 
> 
> >
> > Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
> > ---
> >  MdePkg/Include/IndustryStandard/Acpi64.h      |  5 ++
> >  .../IndustryStandard/CXLEarlyDiscoveryTable.h | 69
> +++++++++++++++++++
> >  2 files changed, 74 insertions(+)
> >  create mode 100644
> MdePkg/Include/IndustryStandard/CXLEarlyDiscoveryTable.h
> >
> 
> > diff --git a/MdePkg/Include/IndustryStandard/CXLEarlyDiscoveryTable.h
> b/MdePkg/Include/IndustryStandard/CXLEarlyDiscoveryTable.h
> > new file mode 100644
> > index 0000000000..84f88dc737
> > --- /dev/null
> > +++ b/MdePkg/Include/IndustryStandard/CXLEarlyDiscoveryTable.h
> > @@ -0,0 +1,69 @@
> > +/** @file
> > +  ACPI CXL Early Discovery Table (CEDT) definitions.
> > +
> > +  Copyright (c) 2024, Phytium Technology Co Ltd. All rights reserved.
> > +
> > +**/
> > +
> > +#ifndef __CXL_Early_Discovery_TABLE_H__
> > +#define __CXL_Early_Discovery_TABLE_H__
> > +
> > +#include <IndustryStandard/Acpi.h>
> > +#include <IndustryStandard/Acpi64.h>
> > +
> > +#define EFI_ACPI_CXL_Early_Discovery_TABLE_REVISION_01  0x1
> //CXL2.0
> > +#define EFI_ACPI_CXL_Early_Discovery_TABLE_REVISION_02  0x2
> //CXL3.1
> > +
> > +#define EFI_ACPI_CEDT_TYPE_CHBS     0x0
> > +#define EFI_ACPI_CEDT_TYPE_CFMWS    0x1
> 
> Sensible to add all defines from the start?
> So CXIMS, RDPAS and CSDS
> (only that last one was added in 3.1 / revision 2.0)
> 
> 
> > +} EFI_ACPI_6_4_CEDT_Structure;
> > +
> 
> > +///
> > +/// Definition for CXL Host Bridge Structure
> > +///
> > +typedef struct {
> > +  EFI_ACPI_6_4_CEDT_Structure    header;
> > +  UINT32                         UID;
> > +  UINT32                         CXLVersion;
> > +  UINT32                         Reserved;
> > +  UINT64                         Base;
> > +  UINT64                         Length;
> > +} EFI_ACPI_6_4_CXL_Host_Bridge_Structure;
> Should this naming reflect where it's actually defined?
> EFI_ACPI_CXL_3_1_CXL_Host_Bridge_Structure etc
> 
> > +
> > +///
> > +/// Definition for CXL Fixed Memory Window Structure
> > +///
> > +typedef struct {
> > +  EFI_ACPI_6_4_CEDT_Structure    header;
> > +  UINT32                         Reserved;
> > +  UINT64                         BaseHPA;
> > +  UINT64                         WindowSize;
> > +  UINT8                          InterleaveMembers;
> > +  UINT8                          InterleaveArithmetic;
> > +  UINT16                         Reserved1;
> > +  UINT32                         Granularity;
> > +  UINT16                         Restrictions;
> > +  UINT16                         QtgId;
> > +  UINT32                         FirstTarget;
> 
> Is this common for an EDK2 definition?  If it were kernel we'd
> be using a [] to indicate this has variable number of elements.
> I'm too lazy to check for EDK2 equivalents ;)
> 
> > +} EFI_ACPI_6_4_CXL_Fixed_Memory_Window_Structure;
> > +
> > +#pragma pack()
> > +
> > +#endif
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120453): https://edk2.groups.io/g/devel/message/120453
Mute This Topic: https://groups.io/mt/108173030/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-