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 <wangyuquan1...@phytium.com.cn>
> Cc: marcin.juszkiew...@linaro.org; gaolim...@byosoft.com.cn; Kinney,
> Michael D <michael.d.kin...@intel.com>; ardb+tianoc...@kernel.org;
> chenba...@phytium.com.cn; wangyinf...@phytium.com.cn;
> shuy...@phytium.com.cn; qemu-de...@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 <wangyuquan1...@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 <wangyuquan1...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
