* [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