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

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

[Jeff Brasen]

Add function to allow enabling and disabling of the clock using the SCMI
interface. Add gArmScmiClock2ProtocolGuid to distinguish platforms that
support new API from those that just have the older protocol.

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                                  |  1 +
 .../ArmScmiDxe/ArmScmiClockProtocolPrivate.h       |  7 +++
 ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf           |  1 +
 ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c      | 52 +++++++++++++++++++++-
 ArmPkg/Include/Protocol/ArmScmiClockProtocol.h     | 27 +++++++++++
 5 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index d99eb67..2f5e5b3 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -59,6 +59,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 } }
+  gArmScmiClock2ProtocolGuid = { 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/ArmScmiDxe.inf b/ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf
index 05ce9c0..9b29b9f 100644
--- a/ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf
+++ b/ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf
@@ -46,6 +46,7 @@
 [Protocols]
   gArmScmiBaseProtocolGuid
   gArmScmiClockProtocolGuid
+  gArmScmiClock2ProtocolGuid
   gArmScmiPerformanceProtocolGuid
 
 [Depex]
diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c b/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c
index 64d2afa..27b53d3 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_CLOCK2_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.
@@ -413,6 +461,8 @@ ScmiClockProtocolInit (
                 Handle,
                 &gArmScmiClockProtocolGuid,
                 &ScmiClockProtocol,
+                &gArmScmiClock2ProtocolGuid,
+                &ScmiClockProtocol,
                 NULL
                 );
 }
diff --git a/ArmPkg/Include/Protocol/ArmScmiClockProtocol.h b/ArmPkg/Include/Protocol/ArmScmiClockProtocol.h
index 3db26cb..28d3d59 100644
--- a/ArmPkg/Include/Protocol/ArmScmiClockProtocol.h
+++ b/ArmPkg/Include/Protocol/ArmScmiClockProtocol.h
@@ -24,7 +24,12 @@
   0x91ce67a8, 0xe0aa, 0x4012, {0xb9, 0x9f, 0xb6, 0xfc, 0xf3, 0x4, 0x8e, 0xaa} \
   }
 
+#define ARM_SCMI_CLOCK2_PROTOCOL_GUID { \
+  0xb8d8caf2, 0x9e94, 0x462c, { 0xa8, 0x34, 0x6c, 0x99, 0xfc, 0x05, 0xef, 0xcf } \
+  }
+
 extern EFI_GUID gArmScmiClockProtocolGuid;
+extern EFI_GUID gArmScmiClock2ProtocolGuid;
 
 // Message Type for clock management protocol.
 typedef enum {
@@ -74,6 +79,7 @@ typedef struct {
 #pragma pack()
 
 typedef struct _SCMI_CLOCK_PROTOCOL SCMI_CLOCK_PROTOCOL;
+typedef SCMI_CLOCK_PROTOCOL SCMI_CLOCK2_PROTOCOL;
 
 // Protocol Interface functions.
 
@@ -205,6 +211,25 @@ EFI_STATUS
   IN UINT64               Rate
   );
 
+/** Enable/Disable specified clock.
+    Function is only available under gArmScmiClock2ProtocolGuid
+
+  @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_CLOCK2_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 +237,8 @@ typedef struct _SCMI_CLOCK_PROTOCOL {
   SCMI_CLOCK_DESCRIBE_RATES DescribeRates;
   SCMI_CLOCK_RATE_GET RateGet;
   SCMI_CLOCK_RATE_SET RateSet;
+  //Only available under gArmScmiClock2ProtocolGuid
+  SCMI_CLOCK_ENABLE Enable;
 } SCMI_CLOCK_PROTOCOL;
 
 #endif /* ARM_SCMI_CLOCK_PROTOCOL_H_ */
-- 
2.7.4

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

[Jeff Brasen]

Leif/Ard,


  Any comments on this v2 patch for this?


Thanks,

Jeff

________________________________
From: Jeff Brasen
Sent: Wednesday, November 28, 2018 1:36:16 PM
To: edk2-devel@lists.01.org
Cc: Jeff Brasen; Ard Biesheuvel; Leif Lindholm; Grish Pathak
Subject: [PATCH v2] ArmPkg/ArmScmiDxe: Add clock enable function

Add function to allow enabling and disabling of the clock using the SCMI
interface. Add gArmScmiClock2ProtocolGuid to distinguish platforms that
support new API from those that just have the older protocol.

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                                  |  1 +
 .../ArmScmiDxe/ArmScmiClockProtocolPrivate.h       |  7 +++
 ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf           |  1 +
 ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c      | 52 +++++++++++++++++++++-
 ArmPkg/Include/Protocol/ArmScmiClockProtocol.h     | 27 +++++++++++
 5 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index d99eb67..2f5e5b3 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -59,6 +59,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 } }
+  gArmScmiClock2ProtocolGuid = { 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/ArmScmiDxe.inf b/ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf
index 05ce9c0..9b29b9f 100644
--- a/ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf
+++ b/ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf
@@ -46,6 +46,7 @@
 [Protocols]
   gArmScmiBaseProtocolGuid
   gArmScmiClockProtocolGuid
+  gArmScmiClock2ProtocolGuid
   gArmScmiPerformanceProtocolGuid

 [Depex]
diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c b/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c
index 64d2afa..27b53d3 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_CLOCK2_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.
@@ -413,6 +461,8 @@ ScmiClockProtocolInit (
                 Handle,
                 &gArmScmiClockProtocolGuid,
                 &ScmiClockProtocol,
+                &gArmScmiClock2ProtocolGuid,
+                &ScmiClockProtocol,
                 NULL
                 );
 }
diff --git a/ArmPkg/Include/Protocol/ArmScmiClockProtocol.h b/ArmPkg/Include/Protocol/ArmScmiClockProtocol.h
index 3db26cb..28d3d59 100644
--- a/ArmPkg/Include/Protocol/ArmScmiClockProtocol.h
+++ b/ArmPkg/Include/Protocol/ArmScmiClockProtocol.h
@@ -24,7 +24,12 @@
   0x91ce67a8, 0xe0aa, 0x4012, {0xb9, 0x9f, 0xb6, 0xfc, 0xf3, 0x4, 0x8e, 0xaa} \
   }

+#define ARM_SCMI_CLOCK2_PROTOCOL_GUID { \
+  0xb8d8caf2, 0x9e94, 0x462c, { 0xa8, 0x34, 0x6c, 0x99, 0xfc, 0x05, 0xef, 0xcf } \
+  }
+
 extern EFI_GUID gArmScmiClockProtocolGuid;
+extern EFI_GUID gArmScmiClock2ProtocolGuid;

 // Message Type for clock management protocol.
 typedef enum {
@@ -74,6 +79,7 @@ typedef struct {
 #pragma pack()

 typedef struct _SCMI_CLOCK_PROTOCOL SCMI_CLOCK_PROTOCOL;
+typedef SCMI_CLOCK_PROTOCOL SCMI_CLOCK2_PROTOCOL;

 // Protocol Interface functions.

@@ -205,6 +211,25 @@ EFI_STATUS
   IN UINT64               Rate
   );

+/** Enable/Disable specified clock.
+    Function is only available under gArmScmiClock2ProtocolGuid
+
+  @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_CLOCK2_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 +237,8 @@ typedef struct _SCMI_CLOCK_PROTOCOL {
   SCMI_CLOCK_DESCRIBE_RATES DescribeRates;
   SCMI_CLOCK_RATE_GET RateGet;
   SCMI_CLOCK_RATE_SET RateSet;
+  //Only available under gArmScmiClock2ProtocolGuid
+  SCMI_CLOCK_ENABLE Enable;
 } SCMI_CLOCK_PROTOCOL;

 #endif /* ARM_SCMI_CLOCK_PROTOCOL_H_ */
--
2.7.4


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

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

[Ard Biesheuvel]

On Thu, 6 Dec 2018 at 01:37, Jeff Brasen <jbrasen@nvidia.com> wrote:
>
> Leif/Ard,
>
>
>   Any comments on this v2 patch for this?
>
>

Hi Jeff,

I'm not sure what level of bikeshedding is justified when it comes to
a driver such as this one, which is very recent, and mostly for
platform internal use. However, I will note that the current
versioning approach permits a *client* of the old SCMI_CLOCK_PROTOCOL
to be built that invokes ->Enable(), which is not defined for it. This
somewhat defeats the purpose of the versioning, since the whole point
is to avoid invoking ->Enable() on older implementations of the
protocol.

I'd be fine with just modifying the protocol, but if we decide we need
versioning, we should not modify the public interface of the old one.
How the driver reuses one implementation to back the other is another
matter, of course.

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

[Jeff Brasen]

-----Original Message-----
From: Ard Biesheuvel <ard.biesheuvel@linaro.org> 
Sent: Thursday, December 6, 2018 9:54 AM
To: Jeff Brasen <jbrasen@nvidia.com>
Cc: edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>; Girish Pathak <girish.pathak@arm.com>
Subject: Re: [PATCH v2] ArmPkg/ArmScmiDxe: Add clock enable function

On Thu, 6 Dec 2018 at 01:37, Jeff Brasen <jbrasen@nvidia.com> wrote:
>
> Leif/Ard,
>
>
>   Any comments on this v2 patch for this?
>
>

Hi Jeff,

I'm not sure what level of bikeshedding is justified when it comes to a driver such as this one, which is very recent, and mostly for platform internal use. However, I will note that the current versioning approach permits a *client* of the old SCMI_CLOCK_PROTOCOL to be built that invokes ->Enable(), which is not defined for it. This somewhat defeats the purpose of the versioning, since the whole point is to avoid invoking ->Enable() on older implementations of the protocol.

I'd be fine with just modifying the protocol, but if we decide we need versioning, we should not modify the public interface of the old one.
How the driver reuses one implementation to back the other is another matter, of course.
[JMB] I can either just change without versioning (that was my original approach but I also changed the guid which would primarily catch new clients running on old platforms from calling an undefined function), I am fine with either that (with maybe a switch back to original guid if we are not concerned about that issue) or a future update that creates a full v2 version of the protocol in the header. 

 

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

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

[Ard Biesheuvel]

On Thu, 6 Dec 2018 at 18:02, Jeff Brasen <jbrasen@nvidia.com> wrote:
>
>
>
> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: Thursday, December 6, 2018 9:54 AM
> To: Jeff Brasen <jbrasen@nvidia.com>
> Cc: edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>; Girish Pathak <girish.pathak@arm.com>
> Subject: Re: [PATCH v2] ArmPkg/ArmScmiDxe: Add clock enable function
>
> On Thu, 6 Dec 2018 at 01:37, Jeff Brasen <jbrasen@nvidia.com> wrote:
> >
> > Leif/Ard,
> >
> >
> >   Any comments on this v2 patch for this?
> >
> >
>
> Hi Jeff,
>
> I'm not sure what level of bikeshedding is justified when it comes to a driver such as this one, which is very recent, and mostly for platform internal use. However, I will note that the current versioning approach permits a *client* of the old SCMI_CLOCK_PROTOCOL to be built that invokes ->Enable(), which is not defined for it. This somewhat defeats the purpose of the versioning, since the whole point is to avoid invoking ->Enable() on older implementations of the protocol.
>
> I'd be fine with just modifying the protocol, but if we decide we need versioning, we should not modify the public interface of the old one.
> How the driver reuses one implementation to back the other is another matter, of course.
> [JMB] I can either just change without versioning (that was my original approach but I also changed the guid which would primarily catch new clients running on old platforms from calling an undefined function), I am fine with either that (with maybe a switch back to original guid if we are not concerned about that issue) or a future update that creates a full v2 version of the protocol in the header.
>

Maybe Leif disagrees, but I am not too concerned about just changing
it. This is not a protocol that 3rd party drivers would invoke, right?

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

[Leif Lindholm]

On Thu, Dec 06, 2018 at 06:09:26PM +0100, Ard Biesheuvel wrote:
> > -----Original Message-----
> > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Sent: Thursday, December 6, 2018 9:54 AM
> > To: Jeff Brasen <jbrasen@nvidia.com>
> > Cc: edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>; Girish Pathak <girish.pathak@arm.com>
> > Subject: Re: [PATCH v2] ArmPkg/ArmScmiDxe: Add clock enable function
> >
> > On Thu, 6 Dec 2018 at 01:37, Jeff Brasen <jbrasen@nvidia.com> wrote:
> > >
> > > Leif/Ard,
> > >
> > >
> > >   Any comments on this v2 patch for this?
> > >
> > >
> >
> > Hi Jeff,
> >
> > I'm not sure what level of bikeshedding is justified when it comes
> > to a driver such as this one, which is very recent, and mostly for
> > platform internal use. However, I will note that the current
> > versioning approach permits a *client* of the old
> > SCMI_CLOCK_PROTOCOL to be built that invokes ->Enable(), which is
> > not defined for it. This somewhat defeats the purpose of the
> > versioning, since the whole point is to avoid invoking ->Enable()
> > on older implementations of the protocol.
> >
> > I'd be fine with just modifying the protocol, but if we decide we
> > need versioning, we should not modify the public interface of the
> > old one.
> > How the driver reuses one implementation to back the other is another matter, of course.
> > [JMB] I can either just change without versioning (that was my
> > original approach but I also changed the guid which would
> > primarily catch new clients running on old platforms from calling
> > an undefined function), I am fine with either that (with maybe a
> > switch back to original guid if we are not concerned about that
> > issue) or a future update that creates a full v2 version of the
> > protocol in the header.
> 
> Maybe Leif disagrees, but I am not too concerned about just changing
> it. This is not a protocol that 3rd party drivers would invoke, right?

It's a protocol that a 3rd party driver _could_ invoke.
Whether that is a likely thing to happen, I just don't know.
Or whether BIOS vendors cherry-picking things badly would cause
interesting things to happen.

On the one hand, I would prefer to see a complete version duplication
of the protocol, just so we _won't_ let existing apps/drivers call the
Enable function.

On the other hand, I don't think this would be the last protocol
update we would ever see, and moving to an internally versioned
interface may make more sense (i.e. adding Version and possibly Size
fields) would be more resilient for that. But that would still require
a full Protocol2 implementation.

At which point, if we don't want to add the Version field, we may just
change the original and see if we break anything?

/
    Leif

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

[Jeff Brasen]

-----Original Message-----
From: Leif Lindholm <leif.lindholm@linaro.org> 
Sent: Wednesday, December 12, 2018 11:49 AM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jeff Brasen <jbrasen@nvidia.com>; edk2-devel@lists.01.org; Girish Pathak <girish.pathak@arm.com>
Subject: Re: [PATCH v2] ArmPkg/ArmScmiDxe: Add clock enable function

On Thu, Dec 06, 2018 at 06:09:26PM +0100, Ard Biesheuvel wrote:
> > -----Original Message-----
> > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Sent: Thursday, December 6, 2018 9:54 AM
> > To: Jeff Brasen <jbrasen@nvidia.com>
> > Cc: edk2-devel@lists.01.org; Leif Lindholm 
> > <leif.lindholm@linaro.org>; Girish Pathak <girish.pathak@arm.com>
> > Subject: Re: [PATCH v2] ArmPkg/ArmScmiDxe: Add clock enable function
> >
> > On Thu, 6 Dec 2018 at 01:37, Jeff Brasen <jbrasen@nvidia.com> wrote:
> > >
> > > Leif/Ard,
> > >
> > >
> > >   Any comments on this v2 patch for this?
> > >
> > >
> >
> > Hi Jeff,
> >
> > I'm not sure what level of bikeshedding is justified when it comes 
> > to a driver such as this one, which is very recent, and mostly for 
> > platform internal use. However, I will note that the current 
> > versioning approach permits a *client* of the old 
> > SCMI_CLOCK_PROTOCOL to be built that invokes ->Enable(), which is 
> > not defined for it. This somewhat defeats the purpose of the 
> > versioning, since the whole point is to avoid invoking ->Enable() on 
> > older implementations of the protocol.
> >
> > I'd be fine with just modifying the protocol, but if we decide we 
> > need versioning, we should not modify the public interface of the 
> > old one.
> > How the driver reuses one implementation to back the other is another matter, of course.
> > [JMB] I can either just change without versioning (that was my 
> > original approach but I also changed the guid which would primarily 
> > catch new clients running on old platforms from calling an undefined 
> > function), I am fine with either that (with maybe a switch back to 
> > original guid if we are not concerned about that
> > issue) or a future update that creates a full v2 version of the 
> > protocol in the header.
> 
> Maybe Leif disagrees, but I am not too concerned about just changing 
> it. This is not a protocol that 3rd party drivers would invoke, right?

It's a protocol that a 3rd party driver _could_ invoke.
Whether that is a likely thing to happen, I just don't know.
Or whether BIOS vendors cherry-picking things badly would cause interesting things to happen.

On the one hand, I would prefer to see a complete version duplication of the protocol, just so we _won't_ let existing apps/drivers call the Enable function.

On the other hand, I don't think this would be the last protocol update we would ever see, and moving to an internally versioned interface may make more sense (i.e. adding Version and possibly Size
fields) would be more resilient for that. But that would still require a full Protocol2 implementation.

At which point, if we don't want to add the Version field, we may just change the original and see if we break anything?
[JMB] If we add a version to this we should probably add one to the other SCMI protocols as well for consistency right? I'll make a cleaner version of v2 that separates the protocols better and upload that

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