* [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
@ 2018-11-08 17:50 Jeff Brasen
2018-11-09 14:09 ` Leif Lindholm
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Brasen @ 2018-11-08 17:50 UTC (permalink / raw)
To: edk2-devel; +Cc: Jeff Brasen, Ard Biesheuvel, Leif Lindholm, Grish Pathak
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 <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(-)
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
2018-11-08 17:50 [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function Jeff Brasen
@ 2018-11-09 14:09 ` Leif Lindholm
2018-11-09 17:03 ` Jeff Brasen
0 siblings, 1 reply; 7+ messages in thread
From: Leif Lindholm @ 2018-11-09 14:09 UTC (permalink / raw)
To: Jeff Brasen; +Cc: edk2-devel, Ard Biesheuvel, Grish Pathak, Sami Mujawar
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
2018-11-09 14:09 ` Leif Lindholm
@ 2018-11-09 17:03 ` Jeff Brasen
2018-11-09 18:30 ` Leif Lindholm
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Brasen @ 2018-11-09 17:03 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel@lists.01.org, Ard Biesheuvel, Grish Pathak,
Sami Mujawar
________________________________
From: Leif Lindholm <leif.lindholm@linaro.org>
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 <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
>
Thanks,
Jeff
Thanks,
Jeff
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
2018-11-09 17:03 ` Jeff Brasen
@ 2018-11-09 18:30 ` Leif Lindholm
2018-11-09 18:35 ` Jeff Brasen
0 siblings, 1 reply; 7+ messages in thread
From: Leif Lindholm @ 2018-11-09 18:30 UTC (permalink / raw)
To: Jeff Brasen
Cc: edk2-devel@lists.01.org, Ard Biesheuvel, Grish Pathak,
Sami Mujawar
On Fri, Nov 09, 2018 at 05:03:53PM +0000, Jeff Brasen wrote:
> ________________________________
> From: Leif Lindholm <leif.lindholm@linaro.org>
> 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 <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
> >
>
>
> Thanks,
>
> Jeff
>
>
> Thanks,
>
> Jeff
>
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may contain
> confidential information. Any unauthorized review, use, disclosure or distribution
> is prohibited. If you are not the intended recipient, please contact the sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
2018-11-09 18:30 ` Leif Lindholm
@ 2018-11-09 18:35 ` Jeff Brasen
2018-11-09 18:49 ` Leif Lindholm
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Brasen @ 2018-11-09 18:35 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel@lists.01.org, Ard Biesheuvel, Grish Pathak,
Sami Mujawar
________________________________
From: Leif Lindholm <leif.lindholm@linaro.org>
Sent: Friday, November 9, 2018 11:30 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
On Fri, Nov 09, 2018 at 05:03:53PM +0000, Jeff Brasen wrote:
> ________________________________
> From: Leif Lindholm <leif.lindholm@linaro.org>
> 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?)
[JB] Yes this was what I was thinking with #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.
[JB] Sounds good, I think Girish is out of office until 11/21
Regards,
Leif
> > 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
> >
>
>
> Thanks,
>
> Jeff
>
>
> Thanks,
>
> Jeff
>
> -----------------------------------------------------------------------------------
> This email message is for the sole use of the intended recipient(s) and may contain
> confidential information. Any unauthorized review, use, disclosure or distribution
> is prohibited. If you are not the intended recipient, please contact the sender by
> reply email and destroy all copies of the original message.
> -----------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
2018-11-09 18:35 ` Jeff Brasen
@ 2018-11-09 18:49 ` Leif Lindholm
2018-11-09 20:31 ` Sami Mujawar
0 siblings, 1 reply; 7+ messages in thread
From: Leif Lindholm @ 2018-11-09 18:49 UTC (permalink / raw)
To: Jeff Brasen
Cc: edk2-devel@lists.01.org, Ard Biesheuvel, Grish Pathak,
Sami Mujawar
On Fri, Nov 09, 2018 at 06:35:02PM +0000, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
2018-11-09 18:49 ` Leif Lindholm
@ 2018-11-09 20:31 ` Sami Mujawar
0 siblings, 0 replies; 7+ messages in thread
From: Sami Mujawar @ 2018-11-09 20:31 UTC (permalink / raw)
To: Leif Lindholm, Jeff Brasen
Cc: edk2-devel@lists.01.org, Ard Biesheuvel, Girish Pathak, nd
-----Original Message-----
From: Leif Lindholm <leif.lindholm@linaro.org>
Sent: 09 November 2018 06:49 PM
To: Jeff Brasen <jbrasen@nvidia.com>
Cc: edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Girish Pathak <Girish.Pathak@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>
Subject: Re: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function
On Fri, Nov 09, 2018 at 06:35:02PM +0000, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-11-09 20:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-08 17:50 [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function Jeff Brasen
2018-11-09 14:09 ` Leif Lindholm
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox