From: Leif Lindholm <leif.lindholm@linaro.org>
To: Jeff Brasen <jbrasen@nvidia.com>
Cc: edk2-devel@lists.01.org,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Grish Pathak <girish.pathak@arm.com>,
Sami Mujawar <Sami.Mujawar@arm.com>
Subject: Re: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
Date: Fri, 9 Nov 2018 14:09:51 +0000 [thread overview]
Message-ID: <20181109140951.jcworswoffwot2q4@bivouac.eciton.net> (raw)
In-Reply-To: <6dbe50db1293266db7588630fc97328276e82843.1541699412.git.jbrasen@nvidia.com>
Hi Jeff,
On Thu, Nov 08, 2018 at 10:50:44AM -0700, Jeff Brasen wrote:
> Add function to allow enabling and disabling of the clock using the SCMI
> interface. Update the protocol GUID as the protocol interface has
> changed.
Changing a protocol GUID for an interface update feels a bit
un-idiomatic for tianocore. (Generally, a new version of the protocol
is added.)
My main concern is that I can't see how removing the ability to
discover the protocol by the old GUID could ever have a desirable
outcome.
Girish, Sami, what's your take?
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Grish Pathak <girish.pathak@arm.com>
> ---
> ArmPkg/ArmPkg.dec | 2 +-
> .../ArmScmiDxe/ArmScmiClockProtocolPrivate.h | 7 +++
> ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c | 50 +++++++++++++++++++++-
> ArmPkg/Include/Protocol/ArmScmiClockProtocol.h | 21 ++++++++-
> 4 files changed, 77 insertions(+), 3 deletions(-)
Could you make sure you follow
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
when generating patches (or let os know if you are, and we have more
git breakage)?
/
Leif
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index 84e57a0..a7b55a1 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -57,7 +57,7 @@
>
> ## Arm System Control and Management Interface(SCMI) Clock management protocol
> ## ArmPkg/Include/Protocol/ArmScmiClockProtocol.h
> - gArmScmiClockProtocolGuid = { 0x91ce67a8, 0xe0aa, 0x4012, { 0xb9, 0x9f, 0xb6, 0xfc, 0xf3, 0x4, 0x8e, 0xaa } }
> + gArmScmiClockProtocolGuid = { 0xb8d8caf2, 0x9e94, 0x462c, { 0xa8, 0x34, 0x6c, 0x99, 0xfc, 0x05, 0xef, 0xcf } }
>
> ## Arm System Control and Management Interface(SCMI) Clock management protocol
> ## ArmPkg/Include/Protocol/ArmScmiPerformanceProtocol.h
> diff --git a/ArmPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h b/ArmPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h
> index 0d1ec6f..c135bac 100644
> --- a/ArmPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h
> +++ b/ArmPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h
> @@ -59,6 +59,13 @@ typedef struct {
> CLOCK_RATE_DWORD Rate;
> } CLOCK_RATE_SET_ATTRIBUTES;
>
> +
> +// Message parameters for CLOCK_CONFIG_SET command.
> +typedef struct {
> + UINT32 ClockId;
> + UINT32 Attributes;
> +} CLOCK_CONFIG_SET_ATTRIBUTES;
> +
> // if ClockAttr Bit[0] is set then clock device is enabled.
> #define CLOCK_ENABLE_MASK 0x1
> #define CLOCK_ENABLED(ClockAttr) ((ClockAttr & CLOCK_ENABLE_MASK) == 1)
> diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c b/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c
> index 64d2afa..493eb45 100644
> --- a/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c
> +++ b/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c
> @@ -388,6 +388,53 @@ ClockRateSet (
> return Status;
> }
>
> +/** Enable/Disable specified clock.
> +
> + @param[in] This A Pointer to SCMI_CLOCK_PROTOCOL Instance.
> + @param[in] ClockId Identifier for the clock device.
> + @param[in] Enable TRUE to enable, FALSE to disable.
> +
> + @retval EFI_SUCCESS Clock enable/disable successful.
> + @retval EFI_DEVICE_ERROR SCP returns an SCMI error.
> + @retval !(EFI_SUCCESS) Other errors.
> +**/
> +STATIC
> +EFI_STATUS
> +ClockEnable (
> + IN SCMI_CLOCK_PROTOCOL *This,
> + IN UINT32 ClockId,
> + IN BOOLEAN Enable
> + )
> +{
> + EFI_STATUS Status;
> + CLOCK_CONFIG_SET_ATTRIBUTES *ClockConfigSetAttributes;
> + SCMI_COMMAND Cmd;
> + UINT32 PayloadLength;
> +
> + Status = ScmiCommandGetPayload ((UINT32**)&ClockConfigSetAttributes);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + // Fill arguments for clock protocol command.
> + ClockConfigSetAttributes->ClockId = ClockId;
> + ClockConfigSetAttributes->Attributes = Enable ? BIT0 : 0;
> +
> + Cmd.ProtocolId = SCMI_PROTOCOL_ID_CLOCK;
> + Cmd.MessageId = SCMI_MESSAGE_ID_CLOCK_CONFIG_SET;
> +
> + PayloadLength = sizeof (CLOCK_CONFIG_SET_ATTRIBUTES);
> +
> + // Execute and wait for response on a SCMI channel.
> + Status = ScmiCommandExecute (
> + &Cmd,
> + &PayloadLength,
> + NULL
> + );
> +
> + return Status;
> +}
> +
> // Instance of the SCMI clock management protocol.
> STATIC CONST SCMI_CLOCK_PROTOCOL ScmiClockProtocol = {
> ClockGetVersion,
> @@ -395,7 +442,8 @@ STATIC CONST SCMI_CLOCK_PROTOCOL ScmiClockProtocol = {
> ClockGetClockAttributes,
> ClockDescribeRates,
> ClockRateGet,
> - ClockRateSet
> + ClockRateSet,
> + ClockEnable
> };
>
> /** Initialize clock management protocol and install protocol on a given handle.
> diff --git a/ArmPkg/Include/Protocol/ArmScmiClockProtocol.h b/ArmPkg/Include/Protocol/ArmScmiClockProtocol.h
> index 3db26cb..d367f37 100644
> --- a/ArmPkg/Include/Protocol/ArmScmiClockProtocol.h
> +++ b/ArmPkg/Include/Protocol/ArmScmiClockProtocol.h
> @@ -21,7 +21,7 @@
> #include <Protocol/ArmScmi.h>
>
> #define ARM_SCMI_CLOCK_PROTOCOL_GUID { \
> - 0x91ce67a8, 0xe0aa, 0x4012, {0xb9, 0x9f, 0xb6, 0xfc, 0xf3, 0x4, 0x8e, 0xaa} \
> + 0xb8d8caf2, 0x9e94, 0x462c, { 0xa8, 0x34, 0x6c, 0x99, 0xfc, 0x05, 0xef, 0xcf } \
> }
>
> extern EFI_GUID gArmScmiClockProtocolGuid;
> @@ -205,6 +205,24 @@ EFI_STATUS
> IN UINT64 Rate
> );
>
> +/** Enable/Disable specified clock.
> +
> + @param[in] This A Pointer to SCMI_CLOCK_PROTOCOL Instance.
> + @param[in] ClockId Identifier for the clock device.
> + @param[in] Enable TRUE to enable, FALSE to disable.
> +
> + @retval EFI_SUCCESS Clock enable/disable successful.
> + @retval EFI_DEVICE_ERROR SCP returns an SCMI error.
> + @retval !(EFI_SUCCESS) Other errors.
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *SCMI_CLOCK_ENABLE) (
> + IN SCMI_CLOCK_PROTOCOL *This,
> + IN UINT32 ClockId,
> + IN BOOLEAN Enable
> + );
> +
> typedef struct _SCMI_CLOCK_PROTOCOL {
> SCMI_CLOCK_GET_VERSION GetVersion;
> SCMI_CLOCK_GET_TOTAL_CLOCKS GetTotalClocks;
> @@ -212,6 +230,7 @@ typedef struct _SCMI_CLOCK_PROTOCOL {
> SCMI_CLOCK_DESCRIBE_RATES DescribeRates;
> SCMI_CLOCK_RATE_GET RateGet;
> SCMI_CLOCK_RATE_SET RateSet;
> + SCMI_CLOCK_ENABLE Enable;
> } SCMI_CLOCK_PROTOCOL;
>
> #endif /* ARM_SCMI_CLOCK_PROTOCOL_H_ */
> --
> 2.7.4
>
next prev parent reply other threads:[~2018-11-09 14:10 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-08 17:50 [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function Jeff Brasen
2018-11-09 14:09 ` Leif Lindholm [this message]
2018-11-09 17:03 ` Jeff Brasen
2018-11-09 18:30 ` Leif Lindholm
2018-11-09 18:35 ` Jeff Brasen
2018-11-09 18:49 ` Leif Lindholm
2018-11-09 20:31 ` Sami Mujawar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181109140951.jcworswoffwot2q4@bivouac.eciton.net \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox