On 18 August 2015 at 14:30, Haojian Zhuang <haojian.zhu...@linaro.org> wrote:
> On Tue, 2015-08-18 at 11:43 +0200, Ard Biesheuvel wrote:
>> On 18 August 2015 at 11:31, Haojian Zhuang <haojian.zhu...@linaro.org> wrote:
>> > On Tue, 2015-08-18 at 11:22 +0200, Ard Biesheuvel wrote:
>> >> On 18 August 2015 at 11:04, Haojian Zhuang <haojian.zhu...@linaro.org>
>> >> wrote:
>> >> > Support multiple PL061 controllers.
>> >> >
>> >> > Contributed-under: TianoCore Contribution Agreement 1.0
>> >> > Signed-off-by: Haojian Zhuang <haojian.zhu...@linaro.org>
>> >> > ---
>> >> > ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c | 107
>> >> > ++++++++++++++-------
>> >> > .../Drivers/PL061GpioDxe/PL061GpioDxe.inf | 3 +-
>> >> > ArmPlatformPkg/Include/Drivers/PL061Gpio.h | 40 ++++----
>> >> > 3 files changed, 96 insertions(+), 54 deletions(-)
>> >> >
>> >> > diff --git a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
>> >> > b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
>> >> > index e8a2094..3027656 100644
>> >> > --- a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
>> >> > +++ b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
>> >> > @@ -28,6 +28,7 @@
>> >> > #include <Drivers/PL061Gpio.h>
>> >> >
>> >> > BOOLEAN mPL061Initialized = FALSE;
>> >> > +PLATFORM_GPIO_CONTROLLER *mPL061PlatformGpio;
>> >> >
>> >> > /**
>> >> > Function implementations
>> >> > @@ -38,20 +39,31 @@ PL061Identify (
>> >> > VOID
>> >> > )
>> >> > {
>> >> > - // Check if this is a PrimeCell Peripheral
>> >> > - if ( (MmioRead8 (PL061_GPIO_PCELL_ID0) != 0x0D)
>> >> > - || (MmioRead8 (PL061_GPIO_PCELL_ID1) != 0xF0)
>> >> > - || (MmioRead8 (PL061_GPIO_PCELL_ID2) != 0x05)
>> >> > - || (MmioRead8 (PL061_GPIO_PCELL_ID3) != 0xB1)) {
>> >> > - return EFI_NOT_FOUND;
>> >> > + UINTN Index;
>> >> > + UINT32 RegisterBase;
>> >>
>> >> Why is this a 32-bit value?
>> >
>> > All registers in PL061 are 32-bit. So we don't need to define UINTN at
>> > here.
>> >
>>
>> So what happens if the GPIO controller is mapped above 4 GB in physical
>> memory?
>>
>
> OK. I'll use 64-bit register base at here. But the original PL061 driver
> is also using 32-bit register base. We could get the definition from
> PL061Gpio.h
> #define PL061_GPIO_DATA_REG ((UINT32)PcdGet32
> (PcdPL061GpioBase) + 0x000)
>
OK. We should also fix that at some point, but for new code, please
don't make any 32-bit assumptions
>> >> > @@ -329,6 +367,9 @@ PL061InstallProtocol (
>> >> > //
>> >> > ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gEmbeddedGpioProtocolGuid);
>> >> >
>> >> > + Status = gBS->LocateProtocol (&gPlatformGpioProtocolGuid, NULL,
>> >> > (VOID **)&mPL061PlatformGpio);
>> >> > + ASSERT_EFI_ERROR (Status);
>> >> > +
>> >>
>> >> We should fall back to using the PCD if the protocol cannot be found.
>> >> This way, we won't break existing users.
>> >>
>> >
>> > I was intended to use fallback. But I think it's not good enough.
>> >
>> > 1. If anyone defines the PCD register base in dec file. The fallback
>> > mechanism will be broken.
>> >
>>
>> Well, you would only look at the PCD if the protocol is not installed,
>> so I don't see how that would change anything compared to the current
>> situation.
>>
>> > 2. Since every driver is built as module, we must create the driver
>> > dependency.
>> >
>>
>> Please try and use a BEFORE ... depex instead, you can put it in your
>> platform GPIO DXE with the file GUID of the PL061 DXE driver.
>>
>
> I tried to use BEFORE in inf. Here's the content in platform gpio inf.
> [Defines]
> INF_VERSION = 0x00010005
> BASE_NAME = HiKeyGpio
> FILE_GUID = b51a851c-7bf7-463f-b261-cfb158b7f699
> MODULE_TYPE = DXE_DRIVER
> VERSION_STRING = 1.0
> ENTRY_POINT = HiKeyGpioEntryPoint
>
> [Protocols]
> gPlatformGpioProtocolGuid
>
> [Depex]
> BEFORE gEmbeddedGpioProtocolGuid
>
You need to use the file GUID of PL061 DXE here. Perhaps you should
add it as a new GUID definition in the HiSiPkg .dec, with the value
5c1997d7-8d45-4f21-af3c-2206b8ed8bec
> And here's updated PL061 driver.
>
> PL061InstallProtocol (
> IN EFI_HANDLE ImageHandle,
> IN EFI_SYSTEM_TABLE *SystemTable
> )
> {
> EFI_STATUS Status;
> EFI_HANDLE Handle;
> GPIO_CONTROLLER *GpioController;
>
> //
> // Make sure the Gpio protocol has not been installed in the system
> yet.
> //
> ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gEmbeddedGpioProtocolGuid);
>
> Status = gBS->LocateProtocol (&gPlatformGpioProtocolGuid, NULL, (VOID
> **)&mPL061PlatformGpio);
> DEBUG ((EFI_D_ERROR, "#%a, %d, Status:%r\n", __func__, __LINE__,
> Status));
> if (EFI_ERROR (Status) && (Status == EFI_NOT_FOUND)) {
> // Create the mPL061PlatformGpio
> mPL061PlatformGpio = (PLATFORM_GPIO_CONTROLLER *)AllocateZeroPool
> (sizeof (PLATFORM_GPIO_CONTROLLER) + sizeof (GPIO_CONTROLLER));
> if (mPL061PlatformGpio == NULL) {
> DEBUG ((EFI_D_ERROR, "%a: failed to allocate
> PLATFORM_GPIO_CONTROLLER\n", __func__));
> return EFI_BAD_BUFFER_SIZE;
> }
>
> mPL061PlatformGpio->GpioCount = PL061_GPIO_PINS;
> mPL061PlatformGpio->GpioControllerCount = 1;
> mPL061PlatformGpio->GpioControllerSize = sizeof (GPIO_CONTROLLER);
> mPL061PlatformGpio->GpioController = (GPIO_CONTROLLER *)((UINTN)
> mPL061PlatformGpio + sizeof (PLATFORM_GPIO_CONTROLLER));
>
> GpioController = mPL061PlatformGpio->GpioController;
> GpioController->RegisterBase = (UINTN) PcdGet32 (PcdPL061GpioBase);
> GpioController->GpioIndex = 0;
> GpioController->InternalGpioCount = PL061_GPIO_PINS;
> }
> ...
> }
>
> The log of uefi is in below.
>
> add-symbol-file
> /opt/workspace/boot/uefi/linaro-edk2/Build/HiKey/DEBUG_GCC49/AAR
> CH64/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061GpioDxe/DEBUG/PL061GpioDxe.dll
> 0x3D9B3260
> Loading driver at 0x0003D9B3000 EntryPoint=0x0003D9B32A8
> PL061GpioDxe.efi
> #PL061InstallProtocol, 377, Status:Not
> Found
>
> add-symbol-file
> /opt/workspace/boot/uefi/linaro-edk2/Build/HiKey/DEBUG_GCC49/AAR
> CH64/MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe/DEBUG/En
> glishDxe.dll
> 0x3D89E260
> Loading driver at 0x0003D89E000 EntryPoint=0x0003D89E2A8
> EnglishDxe.efi
> Driver B51A851C-7BF7-463F-B261-CFB158B7F699 was discovered but not
> loaded!!
> HiKeyDetectJumper: failed to set jumper as gpio input
>
> It seems that "BEFORE" can't make platform gpio driver loaded just
> before PL061. It's not loaded at all.
>
>> >> > // Install the Embedded GPIO Protocol onto a new handle
>> >> > Handle = NULL;
>> >> > Status = gBS->InstallMultipleProtocolInterfaces(
>> >> > diff --git a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061GpioDxe.inf
>> >> > b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061GpioDxe.inf
>> >> > index 9d9e4cd..971452c 100644
>> >> > --- a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061GpioDxe.inf
>> >> > +++ b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061GpioDxe.inf
>> >> > @@ -45,6 +45,7 @@
>> >> >
>> >> > [Protocols]
>> >> > gEmbeddedGpioProtocolGuid
>> >> > + gPlatformGpioProtocolGuid
>> >> >
>> >> > [Depex]
>> >> > - TRUE
>> >> > + gPlatformGpioProtocolGuid
>> >>
>> >> Likewise, this breaks existing users
>> >
>> > Withouth this dependency, LocateProtocol() will be invoked before
>> > InstallMultipleProtocols(). So I have to add the dependency at here.
>> >
>>
>> Where is the InstallMultipleProtocols() call? Are you going to post it as
>> well?
>>
> I didn't post it since it's in the platform gpio driver. And the
> platform isn't created in edk2 code repository yet.
>
OK that is fine. I was just curious
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
