From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.12568.1591368405297855133 for ; Fri, 05 Jun 2020 07:46:45 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E3B8F31B; Fri, 5 Jun 2020 07:46:43 -0700 (PDT) Received: from [192.168.1.69] (unknown [10.37.8.209]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2977F3F305; Fri, 5 Jun 2020 07:46:42 -0700 (PDT) Subject: Re: [RFT PATCH edk2-platforms] Silicon/Marvell/MvI2cDxe: connect all I2C masters at EndOfDxe To: Marcin Wojtas Cc: edk2-devel-groups-io , Leif Lindholm References: <20200604213544.279976-1-ard.biesheuvel@arm.com> From: "Ard Biesheuvel" Message-ID: <5903c3ba-8869-5b7e-73e0-cd3a90217e98@arm.com> Date: Fri, 5 Jun 2020 16:46:41 +0200 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 6/5/20 3:27 PM, Marcin Wojtas wrote: > pt., 5 cze 2020 o 15:08 Ard Biesheuvel napisa=C5= =82(a): >> >> On 6/5/20 1:54 PM, Marcin Wojtas wrote: >>> Hi Ard, >>> >>> >>> czw., 4 cze 2020 o 23:35 Ard Biesheuvel napi= sa=C5=82(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 >>>> --- >>>> 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/Sil= icon/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/Silic= on/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/Silic= on/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 MvI2cDevicePathProtoco= l =3D { >>>> 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 fai= led\n")); >>>> return EFI_OUT_OF_RESOURCES; >>>> } >>>> - DevicePath->Guid.Guid.Data4[0] =3D Bus; >>>> + DevicePath->Instance =3D Bus; >>>> >>>> /* if attachment succeeds, this gets freed at ExitBootServices = */ >>>> I2cMasterContext =3D AllocateZeroPool (sizeof (I2C_MASTER_CONTE= XT)); >>>> @@ -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 =3D AllocateCopyPool (sizeof (MvI2cDevicePathProtocol)= , >>>> + &MvI2cDevicePathProtocol); >>>> + if (DevicePath =3D=3D NULL) { >>>> + DEBUG ((DEBUG_ERROR, "MvI2cDxe: I2C device path allocation fail= ed\n")); >>>> + return; >>>> + } >>>> + >>>> + do { >>>> + DevicePathPointer =3D (EFI_DEVICE_PATH_PROTOCOL *)DevicePath; >>>> + Status =3D gBS->LocateDevicePath (&gEfiI2cMasterProtocolGuid, >>>> + &DevicePathPointer, &DeviceHandle); >>>> + if (EFI_ERROR (Status)) { >>>> + break; >>>> + } >>>> + >>>> + Status =3D gBS->ConnectController (DeviceHandle, NULL, NULL, TR= UE); >>>> + 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 =3D gBS->LocateProtocol (&gMarvellBoardDescProtocolGuid, >>>> @@ -177,6 +221,10 @@ MvI2cInitialise ( >>>> >>>> BoardDescProtocol->BoardDescFree (Desc); >>>> >>>> + Status =3D gBS->CreateEventEx (EVT_NOTIFY_SIGNAL, TPL_CALLBACK, O= nEndOfDxe, >>>> + 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 >>> >>> 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)' >> >=20 > In the diff I did not change that: > https://github.com/Semihalf/edk2-platforms/commit/99b5775e9b1692d309ca2= 5eac2ea86b0ca71437b#diff-044fee521a7731faf141e6c6d74fd83bR181 >=20 Fair enough. I have just grabbed your version, and applied it as=20 c8000ecccc83..ed4cc8059ec5 Thanks,