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

["Rebecca Cran via groups.io" <rebecca=os.amperecomputing.com@groups.io>]

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 <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 (#120482): https://edk2.groups.io/g/devel/message/120482
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]
-=-=-=-=-=-=-=-=-=-=-=-