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.
> > +EFI_STATUS
> > +EFIAPI
> > +PL061Locate (
> > + IN EMBEDDED_GPIO_PIN Gpio,
> > + OUT UINT32 *ControllerIndex,
> > + OUT UINT32 *ControllerOffset,
> > + OUT UINT32 *RegisterBase
> > + )
> > +{
> > + UINT32 Index;
> > +
> > + for (Index = 0; Index < mPL061PlatformGpio->GpioControllerCount;
> > Index++) {
> > + if ( (Gpio >= mPL061PlatformGpio->GpioController[Index].GpioIndex)
> > + && (Gpio < mPL061PlatformGpio->GpioController[Index].GpioIndex
> > + +
> > mPL061PlatformGpio->GpioController[Index].InternalGpioCount)) {
> > + *ControllerIndex = Index;
> > + *ControllerOffset = Gpio %
> > mPL061PlatformGpio->GpioController[Index].InternalGpioCount;
>
> Since you are relying on the internal GPIO count to be the same for
> all instances, you should probably check that the value is correct in
> PL061Identify()
OK. I'll check whether it's 8 at here.
> > @@ -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.
2. Since every driver is built as module, we must create the driver
dependency.
> > // 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.
I can't find that any driver is using PL061 in edk2 code repository. So
I can't update the code to reference PL061 driver.
Regards
Haojian
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
