Re: [PATCH v2 13/13] ArmPlatformPkg: Introduce SCMI protocol

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 11 January 2018 at 16:33, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
>
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: 23 December 2017 14:06
>> To: Evan Lloyd <Evan.Lloyd@arm.com>
>> Cc: edk2-devel@lists.01.org; "ard.biesheuvel@linaro.org"@arm.com;
>> "leif.lindholm@linaro.org"@arm.com;
>> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com
>> Subject: Re: [PATCH v2 13/13] ArmPlatformPkg: Introduce SCMI protocol
>>
>> , couOn 22 December 2017 at 18:34,  <evan.lloyd@arm.com> wrote:
>> > From: Girish Pathak <girish.pathak@arm.com>
>> >
>> > This change introduces a new SCMI protocol driver for
>> > Arm Platforms. The driver currently supports only clock
>> > and performance management protocols. Other protocols
>> > will be added as and when needed.
>> >
>> > Clock management protocol is used to configure the HDLCD clock
>> > on Juno platforms.
>> >
>> > Whereas performance management protocol allows adjustment
>> > of various performance domains to evaluate performance of the
>> > Juno platform.
>> >
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Girish Pathak <girish.pathak@arm.com>
>> > ---
> ...
>> > +
>> > +#ifndef ARM_SCMI_BASE_PROTOCOL_PRIVATE_H_
>> > +#define ARM_SCMI_BASE_PROTOCOL_PRIVATE_H_
>> > +
>> > +// Return values of BASE_DISCOVER_LIST_PROTOCOLS command.
>> > +typedef struct {
>> > +  UINT32 NumProtocols;
>> > +  // Array of four protocols in each element
>> > +  // Total elements = 1 + (NumProtocols-1)/4
>> > +  UINT8 Protocols[];
>>
>> For paleontological reasons, EDK2 code does not allow arrays of
>> unspecified length at the end of a struct. I don't know which version
>> of which toolchain used to be the issue here, and I would be surprised
>> if anyone went through the trouble of writing that down, but it is the
>> reason that EDK2 only allows a [1] array, and hence care needs to be
>> taken to add/substract 1 as appropriate when sizing the variable.
>
>  [[Evan Lloyd]] Whilst not disputing your claim, we have failed to find any such restriction in the coding standard.  Do you have a reference?
>

I haven't been able to dig anything up. Perhaps it is worth a message
to the list?
>>
>> So now, it's my turn to cut /you/ a deal here. If you stop whingeing
>> about frivolous patches that only move whitespace around or change //
>> for /*, or move ASSERT()s into if () conditions on cold-as-ice code
>> paths, I am not going to complain about the arrays of unspecified
>> length in this patch, simply because I don't take the coding standard
>> as gospel, and feel that the Tianocore could do with a bit more
>> pragmatism when it comes to matters like these.
>
>  [[Evan Lloyd]] I'm happy to take the deal.  You know why some of the formatting changes are there, and why the ASSERT moves are relevant, so I can't promise we'll stop doing it.
>

Sure :-)

>>
>>
>> > +} BASE_DISCOVER_LIST;
>> > +
>> > +#endif /* ARM_SCMI_BASE_PROTOCOL_PRIVATE_H_ */
>> > diff --git
>> a/ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h
>> b/ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h
>> > new file mode 100644
>> > index
>> 0000000000000000000000000000000000000000..2807b6b476ac1b8cf8
>> 21a29ca7a59a78e9188c52
>> > --- /dev/null
>> > +++
>> b/ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h
> ....
>> > +
>> > +#endif /* SCMI_PRIVATE_H_ */
>> > diff --git a/ArmPlatformPkg/Include/Drivers/ArmScmi.h
>> b/ArmPlatformPkg/Include/Drivers/ArmScmi.h
>>
>> Please don't put stuff in Include/Drivers.
>>
>> Things derived from industry specs belong in Include/IndustryStandard,
>> but given that this header only defines SCMI_MAX_STR_LEN, could you
>> move it into the protocol header instead?
>
>  [[Evan Lloyd]] Can do.  However, this point raises some interesting thoughts.
> e.g. Does SBBR count as an "Industry" standard?  How about SCMI?

I don't care if it is an official or a de facto industry standard, or
just something we decided between ourselves.

> Also, I believe we aim to eventually put ArmPlatformPkg out of our misery.
> So, would the SCMI driver belong in ArmPkg (as an arm ATG standard), or should it be in edk2-platforms?
>

Good point. This should be in ArmPkg not in ArmPlatformPkg, like PSCI
and other architectural specs.

>>
>>
>> > new file mode 100644
>> > index
>> 0000000000000000000000000000000000000000..04ea3de5b34157ed45
>> 9ee47440abbcaa7114e93a
>> > --- /dev/null
>> > +++ b/ArmPlatformPkg/Include/Drivers/ArmScmi.h
>> > @@ -0,0 +1,27 @@
> ...
>> > diff --git a/ArmPlatformPkg/Include/Drivers/ArmScmiBaseProtocol.h
>> b/ArmPlatformPkg/Include/Drivers/ArmScmiBaseProtocol.h
>>
>> This belongs in Include/Protocol, and needs to be declared in the
>> package .dec file as well, along with its GUID.
>>
>
>  [[Evan Lloyd]]   We can do that, but can I check that is actually what you want.
> This file relates to an SCMI (communications) protocol definition, not a UEFI protocol (although Girish has used the familiar style).
> Also, is that Include/Protocol in ArmPkg, ArmPlatformsPkg, or edk2-platforms/Platform/ARM ?
>

If SCMI stuff goes into ArmPkg, so should these protocol definitions.
And anything you install as a protocol interface is a protocol, as
well as 'base' protocols that are essentially abstract (although I am
not sure if there are precedents for that)

>>
> ...
>> > +#define ARM_SCMI_BASE_PROTOCOL_GUID  { 0xd7e5abe9, 0x33ab,
>> 0x418e, { 0x9f, 0x91, 0x72, 0xda, 0xe2, 0xba, 0x8e, 0x2f } }
>> > +
>>
>> Please wrap this line
> [[Evan Lloyd]] Can do, but I don't think it needs to be in the .h file at all.

It needs to be in the protocol header. Otherwise, you cannot
statically initialized EFI_GUID type variables with this protocol's
GUID value

>>
>> > +extern EFI_GUID gArmScmiBaseProtocolGuid;
>> > +
> ...
>> > +/** Initialize Base protocol and install protocol on a given handle.
>> > +
>> > +   @param[in] Handle              Handle to install Base protocol.
>> > +
>> > +   @retval EFI_SUCCESS            Base protocol interface installed
>> > +                                  successfully.
>> > +**/
>> > +EFI_STATUS
>> > +ScmiBaseProtocolInit (
>> > +  IN OUT EFI_HANDLE* Handle
>> > +  );
>> > +
>>
>> This is not part of the protocol so it needs to be moved elsewhere.
>
>  [[Evan Lloyd]]  It initialises the SCMI Base Protocol - not a UEFI protocol.
>

Again, this applies to anything that gets installed as a protocol interface.

>
>>
>> > +#endif /* ARM_SCMI_BASE_PROTOCOL_H_ */
>> > +
>> > diff --git a/ArmPlatformPkg/Include/Drivers/ArmScmiClockProtocol.h
>> b/ArmPlatformPkg/Include/Drivers/ArmScmiClockProtocol.h
>>
>> Please move to Include/Protocol as well, and add declaration to the
>> package file.
>>
>> > new file mode 100644
>> > index
>> 0000000000000000000000000000000000000000..a97728e4dfe8efc3cd8
>> dc29dc94987c1cc6c6a80
>> > --- /dev/null
>> > +++ b/ArmPlatformPkg/Include/Drivers/ArmScmiClockProtocol.h
> ...
>> > +/** Initialize clock management protocol and install protocol on a given
>> handle.
>> > +
>> > +  @param[in] Handle              Handle to install clock management
>> protocol.
>> > +
>> > +  @retval EFI_SUCCESS            Clock protocol interface installed
>> successfully.
>> > +**/
>> > +EFI_STATUS
>> > +ScmiClockProtocolInit (
>> > +  IN EFI_HANDLE *Handle
>> > +  );
>> > +
>>
>> Please move this into a separate header, it is not part of the protocol.
> [[Evan Lloyd]] ditto
>>
>> > +#endif /* ARM_SCMI_CLOCK_PROTOCOL_H_ */
>> > +
>> > diff --git
>> a/ArmPlatformPkg/Include/Drivers/ArmScmiPerformanceProtocol.h
>> b/ArmPlatformPkg/Include/Drivers/ArmScmiPerformanceProtocol.h
>>
>> Same as above
>>
>> > new file mode 100644
>> > index
>> 0000000000000000000000000000000000000000..cb4aa6bf71df86cfd7a0
>> dabb354112c5a38c978f
>> > --- /dev/null
>> > +++ b/ArmPlatformPkg/Include/Drivers/ArmScmiPerformanceProtocol.h
>> > @@ -0,0 +1,274 @@
> ...
>> > +/** Initialize performance management protocol and install on a given
>> Handle.
>> > +
>> > +  @param[in] Handle              Handle to install performance management
>> > +                                 protocol.
>> > +
>> > +  @retval EFI_SUCCESS            Performance protocol installed successfully.
>> > +**/
>> > +EFI_STATUS
>> > +ScmiPerformanceProtocolInit (
>> > +  IN EFI_HANDLE* Handle
>> > +  );
>> > +
>>
>> Same as above
>>
>> > +#endif /* ARM_SCMI_PERFORMANCE_PROTOCOL_H_ */
>> > +
>> > diff --git a/ArmPlatformPkg/Include/Library/ArmMtl.h
>> b/ArmPlatformPkg/Include/Library/ArmMtl.h
>>
>> Please rename to ArmMtlLib, and declare it as a library class in the
>> package file.
> [[Evan Lloyd]] Quite right
>>
>> > new file mode 100644
>> > index
>> 0000000000000000000000000000000000000000..9be65cfa0a1dcf0d984f
>> 29e5d95aedf5e0afac2b
>> > --- /dev/null
>> > +++ b/ArmPlatformPkg/Include/Library/ArmMtl.h
>> > @@ -0,0 +1,132 @@
>> > +/** @file
> ...
>> > --
>> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>> >
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.