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

["Marcin Wojtas" <mw@semihalf.com>]

pt., 5 cze 2020 o 15:08 Ard Biesheuvel <ard.biesheuvel@arm.com> napisał(a):
>
> 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)'
>

In the diff I did not change that:
https://github.com/Semihalf/edk2-platforms/commit/99b5775e9b1692d309ca25eac2ea86b0ca71437b#diff-044fee521a7731faf141e6c6d74fd83bR181