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

["Leif Lindholm" <quic_llindhol@quicinc.com>]

On 2023-11-10 09:11, Pierre Gondois wrote:
> 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.

Oh, right.

I was referring to "latest version" being a moving target.
If I go to https://developer.arm.com/documentation/den0056/latest/, that 
currently means "version 3.2". At some point in the future, that number 
will change, but this code won't automatically get updated.

So I'd prefer something like
"System Control and Management Interface v3.2,
Latest version of the specification can be downloaded from
https://developer.arm.com/documentation/den0056/latest/"

If that makes more sense?

Regards,

Leif

> 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 (#111038): https://edk2.groups.io/g/devel/message/111038
Mute This Topic: https://groups.io/mt/102175810/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-