From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=216.228.121.65; helo=hqemgate16.nvidia.com; envelope-from=jbrasen@nvidia.com; receiver=edk2-devel@lists.01.org Received: from hqemgate16.nvidia.com (hqemgate16.nvidia.com [216.228.121.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6CB3A2194D3AE for ; Fri, 9 Nov 2018 10:35:23 -0800 (PST) Received: from hqpgpgate102.nvidia.com (Not Verified[216.228.121.13]) by hqemgate16.nvidia.com (using TLS: TLSv1.2, DES-CBC3-SHA) id ; Fri, 09 Nov 2018 10:35:31 -0800 Received: from hqmail.nvidia.com ([172.20.161.6]) by hqpgpgate102.nvidia.com (PGP Universal service); Fri, 09 Nov 2018 10:35:22 -0800 X-PGP-Universal: processed; by hqpgpgate102.nvidia.com on Fri, 09 Nov 2018 10:35:22 -0800 Received: from HQMAIL102.nvidia.com (172.18.146.10) by HQMAIL103.nvidia.com (172.20.187.11) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Fri, 9 Nov 2018 18:35:22 +0000 Received: from HQMAIL109.nvidia.com (172.20.187.15) by HQMAIL102.nvidia.com (172.18.146.10) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Fri, 9 Nov 2018 18:35:21 +0000 Received: from NAM04-BN3-obe.outbound.protection.outlook.com (216.32.180.112) by HQMAIL109.nvidia.com (172.20.187.15) with Microsoft SMTP Server (TLS) id 15.0.1395.4 via Frontend Transport; Fri, 9 Nov 2018 18:35:21 +0000 Received: from DM5PR12MB2439.namprd12.prod.outlook.com (52.132.141.32) by DM5PR12MB1369.namprd12.prod.outlook.com (10.168.238.20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1294.28; Fri, 9 Nov 2018 18:35:02 +0000 Received: from DM5PR12MB2439.namprd12.prod.outlook.com ([fe80::d0a8:220b:4ebc:35e2]) by DM5PR12MB2439.namprd12.prod.outlook.com ([fe80::d0a8:220b:4ebc:35e2%4]) with mapi id 15.20.1294.034; Fri, 9 Nov 2018 18:35:02 +0000 From: Jeff Brasen To: Leif Lindholm CC: "edk2-devel@lists.01.org" , Ard Biesheuvel , Grish Pathak , "Sami Mujawar" Thread-Topic: [PATCH] ArmPkg/ArmScmiDxe: Add clock enable function Thread-Index: AQHUd4uhcqcHw3MLLUeQSNcll5pwwKVHfKuAgAAvC5mAABm6AIAAAQCw Date: Fri, 9 Nov 2018 18:35:02 +0000 Message-ID: References: <6dbe50db1293266db7588630fc97328276e82843.1541699412.git.jbrasen@nvidia.com> <20181109140951.jcworswoffwot2q4@bivouac.eciton.net> , <20181109183018.asma3ynwkr7dww57@bivouac.eciton.net> In-Reply-To: <20181109183018.asma3ynwkr7dww57@bivouac.eciton.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=jbrasen@nvidia.com; x-originating-ip: [216.228.112.22] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DM5PR12MB1369; 6:yyt81T+EoRphaBFCB7/aWkMqh8JZc5vHgN9tP8tkPy1BPsFS3FyOqrJzuq3ugjApXqkvh4l/hGfdjVs3+QO2HhjNnSZq43FekdYjA7gQUz2/vuWcaoO9nqsWH3ErkCCNnm5He4c5tBt5nFHD57mpKOB99QRnf9qj0eLuK0DPG9rvSUaKq+E8wUGcsiD4ESA/YMxkMfwvoMi5AKLl+Z2QgZ1DCV6ObuBlyG0Ag28v7sDz9Q9o6CFjoxBy0VM4dMPpfjcofqL6rhSewOtagvq0IdrriylG8x4GDSVV2AXNW1mkosQi38pzmmZVZ+W+65DwANWjJ/6v32G6JdRpgviv36uJakC4UCqTZFks3CrYZixYv+y3MNlVHIRS3OHACjja3Jxbd+wr78c/7BiMN1WM9ELAUx6OcUFxYYoBRLrqjjBz5CdKfxweX73/G6hRHQh3nXJrBR3nooPk9/H/Czw6Uw==; 5:RMKteIkknoImSIfAWEmSQiAA8W6j2MID7D9GOqvhL8iLrEw6mCpNxSaxO3xOGUuHJ2Tn8Jh99OurVjm00Oi/Urof+cqfQrjMcy8+Oqfokg4XhYnaofiKbkDVEszbhTotuy6XecaaSckycz95CtMY798dWKpgsau2NTkfHPEQRfA=; 7:VEBpQ+0MaxAp3TXhUo3jK52uYcVkHSWYOkpBmeitiACcCvNFvBilg7B4S5cmNlxXyy5V+mJLE7sLAhuSX26D0Nx69cwD5ZewRaOjJBpRdK5GueWahSnK6uTMHGL9fRCbIU4dR9L8Q8B2ZuhfCovalA== x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-correlation-id: 05d1b82b-79ea-450a-ee2b-08d646720fe2 x-microsoft-antispam: BCL:0; PCL:0; RULEID:(7020095)(4652040)(8989299)(5600074)(711020)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7153060)(7193020); SRVR:DM5PR12MB1369; x-ms-traffictypediagnostic: DM5PR12MB1369: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(162533806227266)(278428928389397)(18589796830644)(180628864354917)(166708455590820); x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040522)(2401047)(8121501046)(5005006)(3002001)(10201501046)(93006095)(93001095)(3231382)(944501410)(52105095)(148016)(149066)(150057)(6041310)(20161123562045)(20161123564045)(20161123560045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123558120)(201708071742011)(7699051)(76991095); SRVR:DM5PR12MB1369; BCL:0; PCL:0; RULEID:; SRVR:DM5PR12MB1369; x-forefront-prvs: 08512C5403 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(136003)(346002)(376002)(366004)(396003)(39860400002)(189003)(61684003)(199004)(74316002)(11346002)(1015004)(81156014)(53936002)(81166006)(54896002)(229853002)(55016002)(71190400001)(606006)(8936002)(2906002)(8676002)(71200400001)(4326008)(6436002)(3846002)(476003)(186003)(25786009)(19627235002)(486006)(7736002)(68736007)(236005)(6306002)(9686003)(6606003)(446003)(6116002)(6246003)(86362001)(33656002)(316002)(93886005)(575784001)(106356001)(105586002)(19627405001)(6506007)(54906003)(66066001)(76176011)(256004)(53546011)(102836004)(97736004)(14444005)(5660300001)(14454004)(99286004)(478600001)(2900100001)(26005)(7696005)(6916009)(966005); DIR:OUT; SFP:1101; SCL:1; SRVR:DM5PR12MB1369; H:DM5PR12MB2439.namprd12.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; received-spf: None (protection.outlook.com: nvidia.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: j6sbYibJ+jrB3b/d/nluTzCqDHg3BagtEalS22VqXyUDkUegMnb9T5VqyrwKVCSXhgr+UMwT0HmAXRymYFhpwS7ykxrhxnWoFS26hoOaEsgVI/+r0lGvVN5nAp6TUZz34aCUIoWhaPqhO/Of1NyJ+ZTcyZc04apSWus8xOlEZj2G6N/QeypbAfuTjZEJG3n37vFIrvOx6mDGrRJQG4haXOgkx4X0P38qh4oPH7of031Au9b2pMruzUbP0exGBWkE0IVCbKZk1QGVeEvO9qxGnC4YV4UZVdaY3oMTSzGClc+C2Xz7F6CiFesUtpNjGO8rJj2agJR0YSemj1+2gLSnJMsEzlBfjB0e7xFrhPzGlBQ= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 05d1b82b-79ea-450a-ee2b-08d646720fe2 X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Nov 2018 18:35:02.5156 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB1369 X-OriginatorOrg: Nvidia.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1; t=1541788531; bh=9nwTcLqgOhrnKVfa2vPJVamOeJTCiWEqFe/NWlMY11Q=; h=X-PGP-Universal:From:To:CC:Subject:Thread-Topic:Thread-Index:Date: Message-ID:References:In-Reply-To:Accept-Language:X-MS-Has-Attach: X-MS-TNEF-Correlator:authentication-results:x-originating-ip: x-ms-publictraffictype:x-microsoft-exchange-diagnostics: x-ms-exchange-antispam-srfa-diagnostics: x-ms-office365-filtering-correlation-id:x-microsoft-antispam: x-ms-traffictypediagnostic:x-microsoft-antispam-prvs: x-exchange-antispam-report-test:x-ms-exchange-senderadcheck: x-exchange-antispam-report-cfa-test:x-forefront-prvs: x-forefront-antispam-report:received-spf: x-microsoft-antispam-message-info:spamdiagnosticoutput: spamdiagnosticmetadata:MIME-Version: X-MS-Exchange-CrossTenant-Network-Message-Id: X-MS-Exchange-CrossTenant-originalarrivaltime: X-MS-Exchange-CrossTenant-fromentityheader: X-MS-Exchange-CrossTenant-id: X-MS-Exchange-Transport-CrossTenantHeadersStamped:X-OriginatorOrg: Content-Language:Content-Type; b=o99pWHmB2mPiAWAVUaRba3rt6XW1ih4icU0gqaHIspVHdPk4T5/+6HXdtnebn8wUO qTBqM2OvTRkpTq/cxt0vgWBxCCa/SDwBwUzMHUlfwxVSn1yMUyBiVtMuE9iwUlB/zQ /H+euQegySR70Yg+LTYjeeF+Flf+s6HmyMZp/Lie8wu7rxl0wl/b6AxbEHPIsJbibJ vAxx0mG+t0KC1xS3wMJoyUJHcLWlsozg65IBafxvORz9eDzMaTZ8NpsCoKaRl5+WcI YRvQGTbBXrisTHTpIEa2Fnj8lnLNs+QNaHSzj36x8Culp97wv5VsVzmwgCmwqDEKj8 ZIGlVQsvDF1wg== X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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:35:23 -0000 Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable ________________________________ From: Leif Lindholm Sent: Friday, November 9, 2018 11:30 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 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 SCM= I > > 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 platfo= rms. 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 y= our preference? > > > 1. Add new ArmScmiClock2Protocol.h + guid > 2. Add new guid and document that you have to use that guid for the en= able 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?) [JB] Yes this was what I was thinking with #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. [JB] Sounds good, I think Girish is out of office until 11/21 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-gi= t-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 managemen= t protocol > > ## ArmPkg/Include/Protocol/ArmScmiClockProtocol.h > > - gArmScmiClockProtocolGuid =3D { 0x91ce67a8, 0xe0aa, 0x4012, { 0xb9, = 0x9f, 0xb6, 0xfc, 0xf3, 0x4, 0x8e, 0xaa } } > > + gArmScmiClockProtocolGuid =3D { 0xb8d8caf2, 0x9e94, 0x462c, { 0xa8, = 0x34, 0x6c, 0x99, 0xfc, 0x05, 0xef, 0xcf } } > > > > ## Arm System Control and Management Interface(SCMI) Clock managemen= t 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) =3D= =3D 1) > > diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c b/ArmPkg/Dri= vers/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 =3D ScmiCommandGetPayload ((UINT32**)&ClockConfigSetAttribute= s); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + // Fill arguments for clock protocol command. > > + ClockConfigSetAttributes->ClockId =3D ClockId; > > + ClockConfigSetAttributes->Attributes =3D Enable ? BIT0 : 0; > > + > > + Cmd.ProtocolId =3D SCMI_PROTOCOL_ID_CLOCK; > > + Cmd.MessageId =3D SCMI_MESSAGE_ID_CLOCK_CONFIG_SET; > > + > > + PayloadLength =3D sizeof (CLOCK_CONFIG_SET_ATTRIBUTES); > > + > > + // Execute and wait for response on a SCMI channel. > > + Status =3D ScmiCommandExecute ( > > + &Cmd, > > + &PayloadLength, > > + NULL > > + ); > > + > > + return Status; > > +} > > + > > // Instance of the SCMI clock management protocol. > > STATIC CONST SCMI_CLOCK_PROTOCOL ScmiClockProtocol =3D { > > ClockGetVersion, > > @@ -395,7 +442,8 @@ STATIC CONST SCMI_CLOCK_PROTOCOL ScmiClockProtocol = =3D { > > ClockGetClockAttributes, > > ClockDescribeRates, > > ClockRateGet, > > - ClockRateSet > > + ClockRateSet, > > + ClockEnable > > }; > > > > /** Initialize clock management protocol and install protocol on a giv= en handle. > > diff --git a/ArmPkg/Include/Protocol/ArmScmiClockProtocol.h b/ArmPkg/In= clude/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, 0x= ef, 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 m= ay contain > confidential information. Any unauthorized review, use, disclosure or di= stribution > is prohibited. If you are not the intended recipient, please contact the= sender by > reply email and destroy all copies of the original message. > -------------------------------------------------------------------------= ----------