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?
>> > +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.
>
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.
>> > // 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 can't find that any driver is using PL061 in edk2 code repository. So
> I can't update the code to reference PL061 driver.
>
Well, that is not entirely the point. First of all, it may be used
outside of the edk2 tree. But we should also not complicate the common
case by forcing everyone to use the multiple GPIO protocol and install
it programmatically rather than simply setting a PCD
--
Ard.
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
