From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::22f; helo=mail-io0-x22f.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x22f.google.com (mail-io0-x22f.google.com [IPv6:2607:f8b0:4001:c06::22f]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9B27E21D2BEE1 for ; Fri, 12 Jan 2018 05:29:53 -0800 (PST) Received: by mail-io0-x22f.google.com with SMTP id n14so5871441iob.4 for ; Fri, 12 Jan 2018 05:35:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=BVvK5k6KIcvODyxYSvmSZwjPFTtNw+1kWo1za0cjeqo=; b=NU1sNCMSHYGl6EN4L2KxdPNRBapHPnXagMrp2+/b/Vj5NNKiuTlJRl6r80421MhVeT eMDLfZxyvyGXkvvg6fmbszT+chG9424qBV6x9QKV4IxXtJEdX25PYmGAWjRxiFBmOUuX TDy18F7tOh5XQUqWNe+W0Ix6eSZ9P6vkbU9xY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=BVvK5k6KIcvODyxYSvmSZwjPFTtNw+1kWo1za0cjeqo=; b=uc9OUX/T6ABVohzkeMyecGu5Ko8AYjmwxxtFVplvVc7VLi67votGGgiLIHTiRv9ar/ QXHQxEEyrFruGhjL7hsJje1RmO26h/P/jXasNiCKxzlqv2TO/jm0zvBwJDmGuV7Qz3Xx HKGRnRnag075WwLOo/cqZBymfIW9CG7GbO/kMUBcgvkN2EcDftpicoRuq47uGDjhdIDD wKqYOXEiwG6jEhS4AYx5DPokN3yH+Wo74vXbEM8H1AaZ7NLG5QOx24qJLeYoos87Y7py BHPezHTGPsJLwovfeqEtvBn+2WJGPp5SuaPVguJEC2vIdRjC1lxF0V0NfYJMtuCLsopq kHoQ== X-Gm-Message-State: AKwxytfXdq3UED/rFPyqjABCu+T6nXHYYswz/w8HYuUG9XXfU1aUlstA 81kYLLXDeIPKmDSBgotBH6MXbKtrC6TOSL+DBS/BCA== X-Google-Smtp-Source: ACJfBovQq7+4rMtDEtrQvJvQtPe6M4QdHl0BVEJt6kWpP19ALFuQ8/9PAvl2xNi7mqW1qH4H0cbJEd5OmB/KaNpgK5w= X-Received: by 10.107.135.208 with SMTP id r77mr4411445ioi.248.1515764106965; Fri, 12 Jan 2018 05:35:06 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.37.197 with HTTP; Fri, 12 Jan 2018 05:35:06 -0800 (PST) In-Reply-To: References: From: Ard Biesheuvel Date: Fri, 12 Jan 2018 13:35:06 +0000 Message-ID: To: Evan Lloyd Cc: "edk2-devel@lists.01.org" , "ard.biesheuvel@linaro.org@arm.com" <"ard.biesheuvel@linaro.org"@arm.com>, "leif.lindholm@linaro.org@arm.com" <"leif.lindholm@linaro.org"@arm.com>, "Matteo.Carlini@arm.com@arm.com" <"Matteo.Carlini@arm.com"@arm.com>, "nd@arm.com@arm.com" <"nd@arm.com"@arm.com> Subject: Re: [PATCH v2 13/13] ArmPlatformPkg: Introduce SCMI protocol X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Jan 2018 13:29:54 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 11 January 2018 at 16:33, Evan Lloyd wrote: > > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: 23 December 2017 14:06 >> To: Evan Lloyd >> 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, wrote: >> > From: Girish Pathak >> > >> > 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 >> > --- > ... >> > + >> > +#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 =3D 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 a= ny 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 for= matting 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 thou= ghts. > 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 s= hould 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 y= ou 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-platfo= rms/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 proto= col. > 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 gi= ven >> 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 manage= ment >> > + protocol. >> > + >> > + @retval EFI_SUCCESS Performance protocol installed succe= ssfully. >> > +**/ >> > +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 conf= idential and may also be privileged. If you are not the intended recipient,= please notify the sender immediately and do not disclose the contents to a= ny other person, use it for any purpose, or store or copy the information i= n any medium. Thank you.