Re: [edk2] [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
-Original Message- From: Leif Lindholm Sent: 09 November 2018 06:49 PM To: Jeff Brasen Cc: edk2-devel@lists.01.org; Ard Biesheuvel ; Girish Pathak ; Sami Mujawar Subject: Re: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function On Fri, Nov 09, 2018 at 06:35:02PM +, Jeff Brasen wrote: > > 1. Add new ArmScmiClock2Protocol.h + guid > > 2. Add new guid and document that you have to use that guid for the > > enable function > > 3. Add new guid under old name and have the old guid installed on > > the handle as well, this would keep things fairly clean but > > would have a guid that wouldn't map to a protocol in header > > form. > > 4. Just change the protocol without changing the guid, this has > > the reverse issue of the change I made (except errors can > > result in a crash) (don't really like this option) > > I think my (quite puritan) preference would be 5. Add another guid, > describe it in the same .h file. > > For example, see (among several others) > EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL/EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL > in MdePkg/Include/Protocol/FirmwareVolumeBlock.h. > (This may be what you mean by 2?) > > [JB] Yes this was what I was thinking with #2 OK, cool, then we're on the same page. [SAMI] I think this would be the right approach. > It's a bit of a sledgehammer, but it is a well known and common > pattern in edk2. > > However, if we do this, I would prefer to take the opportunity to add > any new functions not already implemented at the same time. Do you > know if we have other missing calls? > > Now, this _isn't_ a protocol described by any external specification, > so we don't need to be quite as rigid as for public interfaces. > Basically, there shouldn't be (non-debug/non-devtool) dynamic > applications making use of this protocol in the first place. (If we > think there should be, we need to document this GUID and protocol in a > spec somewhere.) So in reality, I think 4 would also be a valid thing > to do. > > But I do want feedback from the original author. > > [JB] Sounds good, I think Girish is out of office until 11/21 Yeah. I'd take feedback from Sami as well :) But both me and Ard will be at plumbers next week, and we are expecting the stable tag to happen then as well - so that is basically lost anyway. Regards, Leif ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
On Fri, Nov 09, 2018 at 06:35:02PM +, Jeff Brasen wrote: > > 1. Add new ArmScmiClock2Protocol.h + guid > > 2. Add new guid and document that you have to use that guid for the > > enable function > > 3. Add new guid under old name and have the old guid installed on > > the handle as well, this would keep things fairly clean but > > would have a guid that wouldn't map to a protocol in header > > form. > > 4. Just change the protocol without changing the guid, this has > > the reverse issue of the change I made (except errors can > > result in a crash) (don't really like this option) > > I think my (quite puritan) preference would be > 5. Add another guid, describe it in the same .h file. > > For example, see (among several others) > EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL/EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL > in MdePkg/Include/Protocol/FirmwareVolumeBlock.h. > (This may be what you mean by 2?) > > [JB] Yes this was what I was thinking with #2 OK, cool, then we're on the same page. > It's a bit of a sledgehammer, but it is a well known and common > pattern in edk2. > > However, if we do this, I would prefer to take the opportunity to add > any new functions not already implemented at the same time. Do you > know if we have other missing calls? > > Now, this _isn't_ a protocol described by any external specification, > so we don't need to be quite as rigid as for public interfaces. > Basically, there shouldn't be (non-debug/non-devtool) dynamic > applications making use of this protocol in the first place. (If we > think there should be, we need to document this GUID and protocol in a > spec somewhere.) > So in reality, I think 4 would also be a valid thing to do. > > But I do want feedback from the original author. > > [JB] Sounds good, I think Girish is out of office until 11/21 Yeah. I'd take feedback from Sami as well :) But both me and Ard will be at plumbers next week, and we are expecting the stable tag to happen then as well - so that is basically lost anyway. Regards, Leif ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
On Fri, Nov 09, 2018 at 05:03:53PM +, Jeff Brasen wrote:
>
> From: Leif Lindholm
> Sent: Friday, November 9, 2018 7:09 AM
> To: Jeff Brasen
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel; Grish Pathak; Sami Mujawar
> Subject: Re: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
>
> 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?
>
> [JB] I was trying to prevent new dynamic apps from crashing on old platforms.
> I figured this was ok as this is a fairly new non-specification based
> protocol. I do see the concern with this though. Of the following what is
> your preference?
>
>
> 1. Add new ArmScmiClock2Protocol.h + guid
> 2. Add new guid and document that you have to use that guid for the enable
> function
> 3. Add new guid under old name and have the old guid installed on
> the handle as well, this would keep things fairly clean but
> would have a guid that wouldn't map to a protocol in header
> form.
> 4. Just change the protocol without changing the guid, this has
> the reverse issue of the change I made (except errors can
> result in a crash) (don't really like this option)
I think my (quite puritan) preference would be
5. Add another guid, describe it in the same .h file.
For example, see (among several others)
EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL/EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL
in MdePkg/Include/Protocol/FirmwareVolumeBlock.h.
(This may be what you mean by 2?)
It's a bit of a sledgehammer, but it is a well known and common
pattern in edk2.
However, if we do this, I would prefer to take the opportunity to add
any new functions not already implemented at the same time. Do you
know if we have other missing calls?
Now, this _isn't_ a protocol described by any external specification,
so we don't need to be quite as rigid as for public interfaces.
Basically, there shouldn't be (non-debug/non-devtool) dynamic
applications making use of this protocol in the first place. (If we
think there should be, we need to document this GUID and protocol in a
spec somewhere.)
So in reality, I think 4 would also be a valid thing to do.
But I do want feedback from the original author.
Regards,
Leif
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jeff Brasen
> > Cc: Ard Biesheuvel
> > Cc: Leif Lindholm
> > Cc: Grish Pathak
> > ---
> > 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)
Re: [edk2] [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
From: Leif Lindholm
Sent: Friday, November 9, 2018 7:09 AM
To: Jeff Brasen
Cc: edk2-devel@lists.01.org; Ard Biesheuvel; Grish Pathak; Sami Mujawar
Subject: Re: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
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?
[JB] I was trying to prevent new dynamic apps from crashing on old platforms. I
figured this was ok as this is a fairly new non-specification based protocol. I
do see the concern with this though. Of the following what is your preference?
1. Add new ArmScmiClock2Protocol.h + guid
2. Add new guid and document that you have to use that guid for the enable
function
3. Add new guid under old name and have the old guid installed on the handle
as well, this would keep things fairly clean but would have a guid that
wouldn't map to a protocol in header form.
4. Just change the protocol without changing the guid, this has the reverse
issue of the change I made (except errors can result in a crash) (don't really
like this option)
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jeff Brasen
> Cc: Ard Biesheuvel
> Cc: Leif Lindholm
> Cc: Grish Pathak
> ---
> 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] ThisA 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_COMMANDCmd;
> + UINT32 PayloadLength;
> +
> + Status = ScmiCommandGetPayload ((UINT32**));
> + if (EFI_ERROR (Status)) {
> +return Status;
> + }
> +
> + // Fill arguments for clock protocol command.
> +
Re: [edk2] [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
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
> Cc: Ard Biesheuvel
> Cc: Leif Lindholm
> Cc: Grish Pathak
> ---
> 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] ThisA 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_COMMANDCmd;
> + UINT32 PayloadLength;
> +
> + Status = ScmiCommandGetPayload ((UINT32**));
> + 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 (
> + ,
> + ,
> + 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
>
[edk2] [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
Add function to allow enabling and disabling of the clock using the SCMI
interface. Update the protocol GUID as the protocol interface has
changed.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jeff Brasen
Cc: Ard Biesheuvel
Cc: Leif Lindholm
Cc: Grish Pathak
---
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(-)
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] ThisA 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_COMMANDCmd;
+ UINT32 PayloadLength;
+
+ Status = ScmiCommandGetPayload ((UINT32**));
+ 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 (
+ ,
+ ,
+ 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
#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] ThisA 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
