Re: [edk2-devel] [PATCH v2 01/11] ArmPkg/ArmScmiDxe: Rename PERFORMANCE_PROTOCOL_VERSION

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

Hello Leif,

On 11/2/23 11:20, Pierre Gondois wrote:
> Hello Leif,
> Thanks for the review,
> 
> On 10/26/23 12:05, Leif Lindholm wrote:
>> On Wed, Oct 25, 2023 at 13:25:30 +0200, pierre.gondois@arm.com wrote:
>>> From: Pierre Gondois <pierre.gondois@arm.com>
>>>
>>> Rename PERFORMANCE_PROTOCOL_VERSION to reflect the different
>>> versions of the protocol. The macro is neither used in edk2 nor
>>> in edk2-platforms.
>>
>> OK, so slight nitpick, but mainly because it parses a bit weirdly...
>> *Will* it be used after this series is merged, or is this an update
>> for completeness?
> 
> The 'fast channels' were added in the v2.0 SCMI specification. This patch-set
> relies on this feature, so it is checked in:
>      [PATCH v2 10/11] DynamicTablesPkg: Add ArmScmiInfoLib
> that the underlying SCP is at least at this version.
> 
> ```
>      // FastChannels were added in SCMI v2.0 spec.
>      if (Version < PERFORMANCE_PROTOCOL_VERSION_V2) {
>        DEBUG ((DEBUG_ERROR, "ArmScmiInfoLib requires SCMI version > 2.0\n"));
>        return EFI_UNSUPPORTED;
>      }
> ```
> 
>>
>>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
>>> ---
>>>    ArmPkg/Include/Library/ArmLib.h                     |  1 +
>>>    .../Include/Protocol/ArmScmiPerformanceProtocol.h   | 13 ++++++++-----
>>>    2 files changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
>>> index 0169dbc1092c..7b2b2238fed9 100644
>>> --- a/ArmPkg/Include/Library/ArmLib.h
>>> +++ b/ArmPkg/Include/Library/ArmLib.h
>>> @@ -780,6 +780,7 @@ EFIAPI
>>>    ArmHasVhe (
>>>      VOID
>>>      );
>>> +
>>>    #endif // MDE_CPU_AARCH64
>>>    
>>>    #ifdef MDE_CPU_ARM
>>> diff --git a/ArmPkg/Include/Protocol/ArmScmiPerformanceProtocol.h b/ArmPkg/Include/Protocol/ArmScmiPerformanceProtocol.h
>>> index 7e548e4765c2..8e8e05d5a5f6 100644
>>> --- a/ArmPkg/Include/Protocol/ArmScmiPerformanceProtocol.h
>>> +++ b/ArmPkg/Include/Protocol/ArmScmiPerformanceProtocol.h
>>> @@ -1,12 +1,12 @@
>>>    /** @file
>>>    
>>> -  Copyright (c) 2017-2021, Arm Limited. All rights reserved.
>>> +  Copyright (c) 2017-2023, Arm Limited. All rights reserved.
>>>    
>>>      SPDX-License-Identifier: BSD-2-Clause-Patent
>>>    
>>> -  System Control and Management Interface V1.0
>>> -    http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/
>>> -    DEN0056A_System_Control_and_Management_Interface.pdf
>>> +  System Control and Management Interface, latest version:
>>
>> I see this as a pattern throughout the series.
>> But this statement will at some point become untrue; this
>> implementation is written against a specific version. I think this
>> version shold be reflected in the comment. (And that applies
>> throughout the series.)
>>
>>> +  - https://developer.arm.com/documentation/den0056/latest/
>>
>> But I think the above is the most useful link.

I was referring to this point I'm not sure I understood.

Regards,
Pierre

> 
> I am not sure I understand completely. Do you mean that the SCMI
> structures/interfaces defined in:
>      ArmPkg/Include/Protocol/ArmScmiPerformanceProtocol.h
> and that were written against the SCMI v1.0 specification should
> not be used as such for other SCMI specification version ?
> I.e. the same process as for the AcpiXX.h files
> (MdePkg/Include/IndustryStandard/Acpi65.h) should be used ?
> 
> Or do you mean that the _CPC object generation implies that the
> SCP should comply to the v2.0 version at least and this should be
> reflected in the commit messages ?
> 
> Regards,
> Pierre
> 
>>
>> /
>>       Leif
>>
>>> +
>>>    **/
>>>    
>>>    #ifndef ARM_SCMI_PERFORMANCE_PROTOCOL_H_
>>> @@ -14,7 +14,10 @@
>>>    
>>>    #include <Protocol/ArmScmi.h>
>>>    
>>> -#define PERFORMANCE_PROTOCOL_VERSION  0x10000
>>> +/// Arm Scmi performance protocol versions.
>>> +#define PERFORMANCE_PROTOCOL_VERSION_V1  0x10000
>>> +#define PERFORMANCE_PROTOCOL_VERSION_V2  0x20000
>>> +#define PERFORMANCE_PROTOCOL_VERSION_V3  0x30000
>>>    
>>>    #define ARM_SCMI_PERFORMANCE_PROTOCOL_GUID  { \
>>>      0x9b8ba84, 0x3dd3, 0x49a6, {0xa0, 0x5a, 0x31, 0x34, 0xa5, 0xf0, 0x7b, 0xad} \
>>> -- 
>>> 2.25.1
>>>


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