Re: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function

[Jeff Brasen <jbrasen@nvidia.com>]

________________________________
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.
-----------------------------------------------------------------------------------