From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::341; helo=mail-wm1-x341.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x341.google.com (mail-wm1-x341.google.com [IPv6:2a00:1450:4864:20::341]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E56162194D3AE for ; Fri, 9 Nov 2018 10:30:22 -0800 (PST) Received: by mail-wm1-x341.google.com with SMTP id f10-v6so2887384wme.3 for ; Fri, 09 Nov 2018 10:30:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=yKamo5JPKzCRQJv74PNqetscUOAc1NCDOz2nf2MXa+g=; b=dgMrCKAdNiAsv/+qDJcTh6R7Ms/3FjF7yHL8AKz65nRWcsUXofiPFp0PsFpuQ77xeX 3/F2GFOP/4IUh24z6JtBN0UflD96HbhiXhlPJOPlWVA2ZWaOv/yVA+6aY5RQyPch4Zlz UZxD24t51VGXxc/g464XZysa1T9hBUW8apL3g= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=yKamo5JPKzCRQJv74PNqetscUOAc1NCDOz2nf2MXa+g=; b=ikmD1WCOeEs8n97HwzRzyB1UTeIKOwTEM1/cyiunp8uXkMqBICkayIkUnNPQ2Z7HzG Rbe3iAeasRyI3H/r08Cv591CDe/fxqsCknVk/fCY9yaLHaO+3gXaFXKGHUbDqxD0kUpD ogfACLcbUhcLCe+FP4hI5VRWs7tTm33FbZstZvnTAByrdQkBdDTJ5xCL2Cw/ZhsBuvUM puZ6vqi2W0tUuk98MjEai+CtLG3zIXMimM9ubKgL+js4iYRwU/PoXokQDjQIkmdLHH24 c2h5dz2gjf1Ddvm9BLvC/m9zdrLcP/xBE/vJ25rK4jI9lJL1/W19IKWEywhPtdidjrza sFKQ== X-Gm-Message-State: AGRZ1gL2Jpwuqs/+c3N8n6vnCwElluLOjEUdW3reFYigu+h4db8L8wPt 9RkdatZaa0Izp01rCQ7FYeRN8w== X-Google-Smtp-Source: AJdET5ftRxLZN0r1CMqsQs9K0dTYUc0szdbkgOFs8UDDs7cC+djdgS8QYhFteEijbYrSaQPE5fhWJw== X-Received: by 2002:a1c:f313:: with SMTP id q19-v6mr352733wmq.87.1541788221124; Fri, 09 Nov 2018 10:30:21 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 82-v6sm2103596wms.17.2018.11.09.10.30.20 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 09 Nov 2018 10:30:20 -0800 (PST) Date: Fri, 9 Nov 2018 18:30:18 +0000 From: Leif Lindholm To: Jeff Brasen Cc: "edk2-devel@lists.01.org" , Ard Biesheuvel , Grish Pathak , Sami Mujawar Message-ID: <20181109183018.asma3ynwkr7dww57@bivouac.eciton.net> References: <6dbe50db1293266db7588630fc97328276e82843.1541699412.git.jbrasen@nvidia.com> <20181109140951.jcworswoffwot2q4@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Nov 2018 18:30:23 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Nov 09, 2018 at 05:03:53PM +0000, Jeff Brasen wrote: > ________________________________ > From: Leif Lindholm > 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 > > Cc: Ard Biesheuvel > > Cc: Leif Lindholm > > Cc: Grish Pathak > > --- > > 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 > > > > #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. > -----------------------------------------------------------------------------------