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)
> >> > @@ -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
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.
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
