Also, leading underscores are supposed to be reserved for compiler implementations (and there only needs to be a single trailing underscore) so it should really be:

__CXL_Early_Discovery_TABLE_H__ -> CXL_EARLY_DISCOVERY_TABLE_H_

--
Rebecca


On 8/30/2024 12:06 PM, Michael D Kinney via groups.io wrote:
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 (#120482): https://edk2.groups.io/g/devel/message/120482
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]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to