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

[Leif Lindholm <leif.lindholm@linaro.org>]

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