On Fri, Nov 09, 2018 at 05:03:53PM +0000, Jeff Brasen wrote: > ________________________________ > From: Leif Lindholm <[email protected]> > Sent: Friday, November 9, 2018 7:09 AM > To: Jeff Brasen > Cc: [email protected]; 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 <[email protected]> > > Cc: Ard Biesheuvel <[email protected]> > > Cc: Leif Lindholm <[email protected]> > > Cc: Grish Pathak <[email protected]> > > --- > > 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. > ----------------------------------------------------------------------------------- _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
