Re: [RFT PATCH edk2-platforms] Silicon/Marvell/MvI2cDxe: connect all I2C masters at EndOfDxe

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 6/5/20 1:54 PM, Marcin Wojtas wrote:
> Hi Ard,
> 
> 
> czw., 4 cze 2020 o 23:35 Ard Biesheuvel <ard.biesheuvel@arm.com> napisał(a):
>>
>> To ensure that platforms incorporating MvI2cDxe will keep working
>> as intended once the platform BDS code stops calling ConnectAll(),
>> connect the I2C masters explicitly at EndOfDxe.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>> Build tested only.
>>
>>   Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf |  3 ++
>>   Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h   |  1 +
>>   Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c   | 54 ++++++++++++++++++--
>>   3 files changed, 55 insertions(+), 3 deletions(-)
>>
>> diff --git a/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf b/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
>> index e59fee0ac1b5..f631fbe797fc 100755
>> --- a/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
>> +++ b/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.inf
>> @@ -45,5 +45,8 @@ [Pcd]
>>     gMarvellTokenSpaceGuid.PcdI2cBaudRate
>>     gMarvellTokenSpaceGuid.PcdI2cBusCount
>>
>> +[Guids]
>> +  gEfiEndOfDxeEventGroupGuid
>> +
>>   [Depex]
>>     TRUE
>> diff --git a/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h b/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h
>> index f5c2cdd8ab3a..6caaa45cece0 100644
>> --- a/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h
>> +++ b/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h
>> @@ -80,6 +80,7 @@ typedef struct {
>>
>>   typedef struct {
>>     VENDOR_DEVICE_PATH            Guid;
>> +  UINTN                         Instance;
>>     EFI_DEVICE_PATH_PROTOCOL      End;
>>   } MV_I2C_DEVICE_PATH;
>>
>> diff --git a/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
>> index b13ab8f02c99..dfe8da9891a5 100755
>> --- a/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
>> +++ b/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
>> @@ -30,12 +30,13 @@ STATIC MV_I2C_DEVICE_PATH MvI2cDevicePathProtocol = {
>>         HARDWARE_DEVICE_PATH,
>>         HW_VENDOR_DP,
>>         {
>> -  (UINT8) (sizeof(VENDOR_DEVICE_PATH)),
>> -  (UINT8) (sizeof(VENDOR_DEVICE_PATH) >> 8),
>> +        (UINT8) (OFFSET_OF (MV_I2C_DEVICE_PATH, End)),
>> +        (UINT8) (OFFSET_OF (MV_I2C_DEVICE_PATH, End) >> 8),
>>         },
>>       },
>>       EFI_CALLER_ID_GUID
>>     },
>> +  0,  // Instance
>>     {
>>       END_DEVICE_PATH_TYPE,
>>       END_ENTIRE_DEVICE_PATH_SUBTYPE,
>> @@ -86,7 +87,7 @@ MvI2cInitialiseController (
>>       DEBUG((DEBUG_ERROR, "MvI2cDxe: I2C device path allocation failed\n"));
>>       return EFI_OUT_OF_RESOURCES;
>>     }
>> -  DevicePath->Guid.Guid.Data4[0] = Bus;
>> +  DevicePath->Instance = Bus;
>>
>>     /* if attachment succeeds, this gets freed at ExitBootServices */
>>     I2cMasterContext = AllocateZeroPool (sizeof (I2C_MASTER_CONTEXT));
>> @@ -139,6 +140,47 @@ MvI2cInitialiseController (
>>     return Status;
>>   }
>>
>> +STATIC
>> +VOID
>> +EFIAPI
>> +OnEndOfDxe (
>> +  IN EFI_EVENT  Event,
>> +  IN VOID       *Context
>> +  )
>> +{
>> +  MV_I2C_DEVICE_PATH        *DevicePath;
>> +  EFI_DEVICE_PATH_PROTOCOL  *DevicePathPointer;
>> +  EFI_HANDLE                DeviceHandle;
>> +  EFI_STATUS                Status;
>> +
>> +  gBS->CloseEvent (Event);
>> +
>> +  DevicePath = AllocateCopyPool (sizeof (MvI2cDevicePathProtocol),
>> +                 &MvI2cDevicePathProtocol);
>> +  if (DevicePath == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "MvI2cDxe: I2C device path allocation failed\n"));
>> +    return;
>> +  }
>> +
>> +  do {
>> +    DevicePathPointer = (EFI_DEVICE_PATH_PROTOCOL *)DevicePath;
>> +    Status = gBS->LocateDevicePath (&gEfiI2cMasterProtocolGuid,
>> +                    &DevicePathPointer, &DeviceHandle);
>> +    if (EFI_ERROR (Status)) {
>> +      break;
>> +    }
>> +
>> +    Status = gBS->ConnectController (DeviceHandle, NULL, NULL, TRUE);
>> +    DEBUG ((DEBUG_INFO, "%a: ConnectController () returned %r\n",
>> +      __FUNCTION__, Status));
>> +
>> +    DevicePath->Instance++;
>> +  } while (TRUE);
>> +
>> +  gBS->FreePool (DevicePath);
>> +}
>> +
>> +
>>   EFI_STATUS
>>   EFIAPI
>>   MvI2cInitialise (
>> @@ -150,6 +192,8 @@ MvI2cInitialise (
>>     MV_BOARD_I2C_DESC *Desc;
>>     EFI_STATUS Status;
>>     UINTN Index;
>> +  EFI_EVENT EndOfDxeEvent;
>> +
>>
>>     /* Obtain list of available controllers */
>>     Status = gBS->LocateProtocol (&gMarvellBoardDescProtocolGuid,
>> @@ -177,6 +221,10 @@ MvI2cInitialise (
>>
>>     BoardDescProtocol->BoardDescFree (Desc);
>>
>> +  Status = gBS->CreateEventEx (EVT_NOTIFY_SIGNAL, TPL_CALLBACK, OnEndOfDxe,
>> +                  NULL, &gEfiEndOfDxeEventGroupGuid, &EndOfDxeEvent);
>> +  ASSERT_EFI_ERROR (Status);
>> +
>>     return EFI_SUCCESS;
>>   }
>>
>> --
>> 2.26.2
>>
> 
> This change works as planned, so you can add my:
> Tested-by: Marcin Wojtas <mw@semihalf.com>
> 
> I pushed this patch with slightly improved style to:
> https://github.com/Semihalf/edk2-platforms/commits/i2c-end-of-dxe
> 

Thanks Marcin.

I'll cherry pick some of those improvement, although I don't see the 
point of replace 'while (TRUE)' with 'while (1)'