diff mbox series

[edk2,edk2-platforms,3/8] Silicon/NXP/Pcf8563RealTimeClockLib: avoid driver binding protocol

Message ID 20180125122736.5427-4-ard.biesheuvel@linaro.org
State New
Headers show
Series Socionext SynQuacer updates | expand

Commit Message

Ard Biesheuvel Jan. 25, 2018, 12:27 p.m. UTC 
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.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c   | 31 ++++++++------------
 Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf |  1 -
 2 files changed, 13 insertions(+), 19 deletions(-)

-- 
2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Jan. 25, 2018, 12:54 p.m. UTC | #1 
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?

If so:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  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

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Jan. 25, 2018, 1:07 p.m. UTC | #2 
On 25 January 2018 at 12:54, Leif Lindholm <leif.lindholm@linaro.org> 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 <leif.lindholm@linaro.org>

>


Thanks.

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

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

>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

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