From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::242; helo=mail-it0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x242.google.com (mail-it0-x242.google.com [IPv6:2607:f8b0:4001:c0b::242]) (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 980CA21E2568E for ; Thu, 25 Jan 2018 05:02:14 -0800 (PST) Received: by mail-it0-x242.google.com with SMTP id b5so9176213itc.3 for ; Thu, 25 Jan 2018 05:07:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=RkURyS2OoS1jyvv2uIfK+m9v20tWtb5UdBWnk4gAGCA=; b=fz8lMSuTbV8UqcOxtzeKR2ofgA9fDgE9huc42C8qVVBaOlzqk6FyFP7hvCXV5f/Vxj 0dRZl6PbSknagBPjOEbEJC9jhs/IHo81Pmr7u3uX3vq7viv5ES6y2k5zOLseqFYrhke7 Qq7K5/aPsil8VAkCzPQCnjy+oy6KKBvFVyj90= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=RkURyS2OoS1jyvv2uIfK+m9v20tWtb5UdBWnk4gAGCA=; b=G48tMiNzZuTMqpwQNEPUBD9pRjOH6jwtBBwduZwIT9ngGZuAlqx+j8kE96boxeJF0P +ltHSE5zj/9h2BziR8b4gd1N45UipqDexGyH0RWPwZ63O85EUUAF+Hj80PRdLOBmJhgP C+hQup5GH280SkX2Cu9CIuAGkLR5RIhdj080dCstiM0KQP2bxbIevJEFmABus1eVStRI I+S1xV7dO1mzCLOZq2V8ZR/LF7kY9Oy5HYqlAPLIq36MrQIFWoBmw5V1ZOQwH516IGRE TVR5Sa3uRrDwJhrAl0CI8VdwFi7RVBmHPcB2ZO6ePGF+hRGEGM8hiUEKbuJXJcop7oVX 0ZHA== X-Gm-Message-State: AKwxytdw6aYAgIJElRxRcHMlEICtEeILPvJui5+LnzCI65X8VfxiZgsd EyEGtT6+DOFCE/gsibP6DKA9ReGpMOoINR6/UhRQ6Q== X-Google-Smtp-Source: AH8x2251BUn82ngv3A4oKfPMsf9lkN15SMJr4NRjtqRyBKFC/1b1g8pYL+ZT5/CHvX33O3VrfDkq5PjXq8KgE42lS6o= X-Received: by 10.36.44.197 with SMTP id i188mr13396251iti.102.1516885663148; Thu, 25 Jan 2018 05:07:43 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.112.13 with HTTP; Thu, 25 Jan 2018 05:07:42 -0800 (PST) In-Reply-To: <20180125125434.553ff6xamfpnr6bd@bivouac.eciton.net> References: <20180125122736.5427-1-ard.biesheuvel@linaro.org> <20180125122736.5427-4-ard.biesheuvel@linaro.org> <20180125125434.553ff6xamfpnr6bd@bivouac.eciton.net> From: Ard Biesheuvel Date: Thu, 25 Jan 2018 13:07:42 +0000 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" Subject: Re: [PATCH edk2-platforms 3/8] Silicon/NXP/Pcf8563RealTimeClockLib: avoid driver binding protocol X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Jan 2018 13:02:15 -0000 Content-Type: text/plain; charset="UTF-8" On 25 January 2018 at 12:54, Leif Lindholm wrote: > On Thu, Jan 25, 2018 at 12:27:31PM +0000, Ard Biesheuvel wrote: >> Instead of registering a notification callback on the driver binding >> protocol, and attempting to connect our I2C master handle each time >> a new driver is registered, switch to the more obvious approach of >> registering a notification callback on the I2C master protocol directly. >> >> The original code was written under the assumption that it would make >> the RTC available at an earlier time, but given that all handles that >> are created during the execution of a driver entry point are connected >> by DXE core right away (i.e., before StartImage() returns), this is not >> really necessary, and in fact, may result in the driver already having >> been connected by the time we attempt to connect it. >> >> Note that it is now up to the platform to ensure that ConnectController() >> is called for the handle if DXE core does not call it by itself, or does >> call it but at a time when no I2C master protocol driver is available >> yet. > > Presumably the platforms in edk2-platforms using this library already > follow these constraints? > There aren't any. But I did CC the NXP guys on this patch when I sent it the first time around, to give them the head's up that they probably shouldn't copy the original pattern. > If so: > Reviewed-by: Leif Lindholm > Thanks. >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Ard Biesheuvel >> --- >> Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c | 31 ++++++++------------ >> Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf | 1 - >> 2 files changed, 13 insertions(+), 19 deletions(-) >> >> diff --git a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c >> index 6bc4aef28849..fb58e1feb424 100644 >> --- a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c >> +++ b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c >> @@ -41,7 +41,7 @@ >> #define EPOCH_BASE 2000 >> >> STATIC EFI_HANDLE mI2cMasterHandle; >> -STATIC VOID *mDriverEventRegistration; >> +STATIC VOID *mI2cMasterEventRegistration; >> STATIC EFI_I2C_MASTER_PROTOCOL *mI2cMaster; >> STATIC EFI_EVENT mRtcVirtualAddrChangeEvent; >> >> @@ -263,12 +263,12 @@ LibSetWakeupTime ( >> >> STATIC >> VOID >> -DriverRegistrationEvent ( >> +I2cMasterRegistrationEvent ( >> IN EFI_EVENT Event, >> IN VOID *Context >> ) >> { >> - EFI_HANDLE Handle[2]; >> + EFI_HANDLE Handle; >> UINTN BufferSize; >> EFI_STATUS Status; >> EFI_I2C_MASTER_PROTOCOL *I2cMaster; >> @@ -280,10 +280,10 @@ DriverRegistrationEvent ( >> do { >> BufferSize = sizeof (EFI_HANDLE); >> Status = gBS->LocateHandle (ByRegisterNotify, >> - &gEfiDriverBindingProtocolGuid, >> - mDriverEventRegistration, >> + &gEfiI2cMasterProtocolGuid, >> + mI2cMasterEventRegistration, >> &BufferSize, >> - Handle); >> + &Handle); >> if (EFI_ERROR (Status)) { >> if (Status != EFI_NOT_FOUND) { >> DEBUG ((DEBUG_WARN, "%a: gBS->LocateHandle () returned %r\n", >> @@ -292,12 +292,7 @@ DriverRegistrationEvent ( >> break; >> } >> >> - // >> - // Check if we can connect our handle to this driver. >> - // >> - Handle[1] = NULL; >> - Status = gBS->ConnectController (mI2cMasterHandle, Handle, NULL, FALSE); >> - if (EFI_ERROR (Status)) { >> + if (Handle != mI2cMasterHandle) { >> continue; >> } >> >> @@ -378,16 +373,16 @@ LibRtcInitialize ( >> ASSERT_EFI_ERROR (Status); >> >> // >> - // Register a protocol registration notification callback on the driver >> - // binding protocol so we can attempt to connect our I2C master to it >> - // as soon as it appears. >> + // Register a protocol registration notification callback on the I2C master >> + // protocol. This will notify us even if the protocol instance we are looking >> + // for has already been installed. >> // >> EfiCreateProtocolNotifyEvent ( >> - &gEfiDriverBindingProtocolGuid, >> + &gEfiI2cMasterProtocolGuid, >> TPL_CALLBACK, >> - DriverRegistrationEvent, >> + I2cMasterRegistrationEvent, >> NULL, >> - &mDriverEventRegistration); >> + &mI2cMasterEventRegistration); >> >> // >> // Register for the virtual address change event >> diff --git a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf >> index 1a9a6f6c9cf3..e232902c6b5d 100644 >> --- a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf >> +++ b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf >> @@ -40,7 +40,6 @@ [Guids] >> gEfiEventVirtualAddressChangeGuid >> >> [Protocols] >> - gEfiDriverBindingProtocolGuid ## CONSUMES >> gEfiI2cMasterProtocolGuid ## CONSUMES >> gPcf8563RealTimeClockLibI2cMasterProtocolGuid ## CONSUMES >> >> -- >> 2.11.0 >>