Re: [edk2-devel] [edk2-platforms][PATCH V1 1/8] Platform/Sgi: Helper macros for PPTT Table

["PierreGondois" <pierre.gondois@arm.com>]

Hi Pranav,

@@ -20,6 +20,132 @@
>>> �#define EFI_ACPI_ARM_CREATOR_ID SIGNATURE_32('A','R','M',' ')
>>> �#define EFI_ACPI_ARM_CREATOR_REVISION 0x00000099
>>>
>>> +#define CORE_COUNT����� FixedPcdGet32 (PcdCoreCount)
>>> +#define CLUSTER_COUNT�� FixedPcdGet32 (PcdClusterCount)
>>> +
>>> +#pragma pack(1)
>>> +// PPTT processor core structure
>>> +typedef struct {
>>> +� EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR� Core; �
>>>
>> +UINT32��������������������ï¿
>> ½
>>> +����������� Offset[2];
>> I think there should be 3 entries (DCache, ICache, L2Cache). Updating this will
>> require updating the other PPTT tables written.
> As per ACPI spec 6.4, chapter '5.2.29.2 Cache Type Structure - Type 1', " Only
> the head of the list needs to be listed as a resource by a processor node (and
> counted toward Number of Private Resources), as the cache node itself
> contains a link to the next level of cache."
> Here L2 cache is represented as next level of L1, so no need to count it.
Yes indeed you are right.
>> Would it be also possible to rename the field 'PrivateResources' as in the
>> spec ?
> Yes, but in actual, it is not the private resource count.

This was nit picking, 'Offset' also works for me, and other PPTT tables 
are calling this field as 'Offset'.

>
>> Another question: what does 'RD_' stands for ?
> RD Stands for Reference Design, it is the convention we follow.
>
>>> +� EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE����� DCache; �
>>> +EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE����� ICache; �
>>> +EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE����� L2Cache; }
>>> +RD_PPTT_CORE;
>>> +
>>> +// PPTT processor cluster structure
>>> +typedef struct {
>>> +� EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR� Cluster; �
>>>
>> +UINT32��������������������ï¿
>> ½
>>> +����������� Offset; �
>>> +EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE����� L3Cache; �
>>> +RD_PPTT_CORE Core[CORE_COUNT]; } RD_PPTT_CLUSTER;
>>> +
>>> +// PPTT processor cluster structure without cache typedef struct {
>>> +� EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR� Cluster; �
>>>
>> +UINT32��������������������ï¿
>> ½
>>> +����������� Offset;
>> I think there is no need for an offset here. Updating this will require updating
>> the other PPTT tables written.
> Right. Will update.
>
>>> +� RD_PPTT_CORE Core[CORE_COUNT];
>>> +} RD_PPTT_MINIMAL_CLUSTER;
>>> +
>>> +// PPTT processor package structure
>>> +typedef struct {
>>> +� EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR� Package; �
>>>
>> +UINT32��������������������ï¿
>> ½
>>> +����������� Offset; �
>>> +EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE����� Slc; �
>>> +RD_PPTT_MINIMAL_CLUSTER Cluster[CLUSTER_COUNT]; }
>>> +RD_PPTT_SLC_PACKAGE; #pragma pack ()
>>> +
>>> +//
>>> +// PPTT processor structure flags for different SoC components as
>>> defined in
>>> +// ACPI 6.3 specification
>>> +//
>>> +
>> [...]
>>> +// EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR
>>> +#define EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR_INIT(Length, Flag,
>>> Parent,������ \
>>> +� ACPIProcessorID, NumberOfPrivateResource) \
>> I think it should be possible to remove the 'Length' parameter and compute it
>> as:
>> sizeof (EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR) +
>> NumberOfPrivateResource * sizeof
>> (EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE) + NumberOfPrivateResource *
>> sizeof (UINT32)
>>
> As per 6.4 specification, table 5.138, the Length is "Length of the local processor
> structure in bytes" It is just the length of local processor, not the entire structure.
Yes indeed you are right.

Thanks for the answer,
Pierre