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.web12.6463.1593696539911282185
 for <devel@edk2.groups.io>;
 Thu, 02 Jul 2020 06:29:00 -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 34DD91FB;
	Thu,  2 Jul 2020 06:28:57 -0700 (PDT)
Received: from [192.168.1.81] (unknown [172.31.20.19])
	by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 83CAA3F68F;
	Thu,  2 Jul 2020 06:28:56 -0700 (PDT)
Subject: Re: [edk2-platforms][PATCH 1/1] Silicon/Broadcom/Bcm283x: GpioLib enhancements
To: Pete Batard <pete@akeo.ie>, devel@edk2.groups.io
Cc: leif@nuviainc.com, awarkentin@vmware.com
References: <20200629183926.12332-1-pete@akeo.ie>
 <c9f252e1-52e7-2735-6053-f1a797563bc3@arm.com>
 <cbb06384-db43-b608-e01b-d587a276ac9d@akeo.ie>
From: "Ard Biesheuvel" <ard.biesheuvel@arm.com>
Message-ID: <dae5c418-edb3-45fc-5d3f-ebdc8e48eb65@arm.com>
Date: Thu, 2 Jul 2020 15:28:40 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <cbb06384-db43-b608-e01b-d587a276ac9d@akeo.ie>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable

On 7/2/20 2:19 PM, Pete Batard wrote:
> On 2020.07.02 13:03, Ard Biesheuvel wrote:
>> On 6/29/20 8:39 PM, Pete Batard wrote:
>>> * Add GpioPinSet () function to set a GPIO pin value
>>> * Add GpioPinGet () function to read a GPIO pin value
>>> * Add GpioSetPull () function to set pullup/down state of a GPIO pin
>>>
>>> GpioSetPull () supports both the legacy method used on Bcm283[5-7]
>>> as well as the new method used on Bcm2711.
>>>
>>> Each of these calls was tested on Raspberry Pi 3 B+ as well as on a
>>> Raspberry Pi 4 B.
>>>
>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>
>> Remind me again what the purpose of this patch is?
>=20
> Feature completion, potential future improvements and providing=20
> additional options for testing (since one will now be able to use this=20
> driver to control LEDs or read switches).
>=20
> It doesn't serve any *immediate* purpose.
>=20
> However, I've been playing with it to simulate an SD card detect, which=
=20
> came handy for developing the other patch I sent, and I would assert=20
> having an established means to control GPIO signals is likely to help=20
> others as well.
>=20

Fair enough.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

Pushed as edde5826b0d2..b716f363e752


> Regards,
>=20
> /Pete
>=20
>>
>>> ---
>>> =C2=A0 Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836Gpio.=
h |=20
>>> 44 ++++++++-
>>> =C2=A0 Silicon/Broadcom/Bcm283x/Include/Library/GpioLib.h=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=20
>>> 17 ++++
>>> =C2=A0 Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.c=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=20
>>> 93 ++++++++++++++++++++
>>> =C2=A0 Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.inf=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 1 +
>>> =C2=A0 4 files changed, 152 insertions(+), 3 deletions(-)
>>>
>>> diff --git=20
>>> a/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836Gpio.h=20
>>> b/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836Gpio.h
>>> index e65cc5c3bbb4..8ad81776b43a 100644
>>> --- a/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836Gpio.h
>>> +++ b/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836Gpio.h
>>> @@ -1,5 +1,6 @@
>>> =C2=A0 /** @file
>>> =C2=A0=C2=A0 *
>>> + *=C2=A0 Copyright (c) 2020, Pete Batard <pete@akeo.ie>
>>> =C2=A0=C2=A0 *=C2=A0 Copyright (c) 2018, Andrei Warkentin <andrey.war=
kentin@gmail.com>
>>> =C2=A0=C2=A0 *=C2=A0 Copyright (c) Microsoft Corporation. All rights =
reserved.
>>> =C2=A0=C2=A0 *
>>> @@ -23,12 +24,45 @@
>>> =C2=A0 #define GPIO_GPFSEL4=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO=
_BASE_ADDRESS + 0x10)
>>> =C2=A0 #define GPIO_GPFSEL5=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO=
_BASE_ADDRESS + 0x14)
>>> -#define GPIO_GPCLR0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x28)
>>> -#define GPIO_GPCLR1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x2C)
>>> -
>>> =C2=A0 #define GPIO_GPSET0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
(GPIO_BASE_ADDRESS + 0x1C)
>>> =C2=A0 #define GPIO_GPSET1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
(GPIO_BASE_ADDRESS + 0x20)
>>> +#define GPIO_GPCLR0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x28)
>>> +#define GPIO_GPCLR1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x2C)
>>> +
>>> +#define GPIO_GPLEV0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x34)
>>> +#define GPIO_GPLEV1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x38)
>>> +
>>> +#define GPIO_GPEDS0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x40)
>>> +#define GPIO_GPEDS1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x44)
>>> +
>>> +#define GPIO_GPREN0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x4C)
>>> +#define GPIO_GPREN1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x50)
>>> +
>>> +#define GPIO_GPFEN0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x58)
>>> +#define GPIO_GPFEN1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x5C)
>>> +
>>> +#define GPIO_GPHEN0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x64)
>>> +#define GPIO_GPHEN1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x68)
>>> +
>>> +#define GPIO_GPLEN0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x70)
>>> +#define GPIO_GPLEN1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_=
BASE_ADDRESS + 0x74)
>>> +
>>> +#define GPIO_GPAREN0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_BASE_=
ADDRESS + 0x7C)
>>> +#define GPIO_GPAREN1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_BASE_=
ADDRESS + 0x80)
>>> +
>>> +#define GPIO_GPAFEN0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_BASE_=
ADDRESS + 0x88)
>>> +#define GPIO_GPAFEN1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_BASE_=
ADDRESS + 0x8C)
>>> +
>>> +#define GPIO_GPPUD=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (=
GPIO_BASE_ADDRESS + 0x94)
>>> +#define GPIO_GPPUDCLK0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_BASE_ADDRESS + =
0x98)
>>> +#define GPIO_GPPUDCLK1=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_BASE_ADDRESS + =
0x9C)
>>> +
>>> +#define GPIO_GPPUPPDN0=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_BASE_ADDRESS + =
0xE4)
>>> +#define GPIO_GPPUPPDN1=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_BASE_ADDRESS + =
0xE8)
>>> +#define GPIO_GPPUPPDN2=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_BASE_ADDRESS + =
0xEC)
>>> +#define GPIO_GPPUPPDN3=C2=A0=C2=A0=C2=A0=C2=A0 (GPIO_BASE_ADDRESS + =
0xF0)
>>> +
>>> =C2=A0 #define GPIO_FSEL_INPUT=C2=A0=C2=A0=C2=A0 0x0
>>> =C2=A0 #define GPIO_FSEL_OUTPUT=C2=A0=C2=A0 0x1
>>> =C2=A0 #define GPIO_FSEL_ALT0=C2=A0=C2=A0=C2=A0=C2=A0 0x4
>>> @@ -44,4 +78,8 @@
>>> =C2=A0 #define GPIO_PINS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 54
>>> +#define GPIO_PULL_NONE=C2=A0=C2=A0=C2=A0=C2=A0 0x00
>>> +#define GPIO_PULL_DOWN=C2=A0=C2=A0=C2=A0=C2=A0 0x01
>>> +#define GPIO_PULL_UP=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 0x02
>>> +
>>> =C2=A0 #endif /* __BCM2836_GPIO_H__ */
>>> diff --git a/Silicon/Broadcom/Bcm283x/Include/Library/GpioLib.h=20
>>> b/Silicon/Broadcom/Bcm283x/Include/Library/GpioLib.h
>>> index 014c6b07a29f..75c2c8be51aa 100644
>>> --- a/Silicon/Broadcom/Bcm283x/Include/Library/GpioLib.h
>>> +++ b/Silicon/Broadcom/Bcm283x/Include/Library/GpioLib.h
>>> @@ -24,4 +24,21 @@ GpioPinFuncGet (
>>> =C2=A0=C2=A0=C2=A0 IN=C2=A0 UINTN Pin
>>> =C2=A0=C2=A0=C2=A0 );
>>> +VOID
>>> +GpioPinSet (
>>> +=C2=A0 IN=C2=A0 UINTN Pin,
>>> +=C2=A0 IN=C2=A0 UINTN Val
>>> +=C2=A0 );
>>> +
>>> +UINTN
>>> +GpioPinGet (
>>> +=C2=A0 IN=C2=A0 UINTN Pin
>>> +=C2=A0 );
>>> +
>>> +VOID
>>> +GpioSetPull (
>>> +=C2=A0 IN=C2=A0 UINTN=C2=A0=C2=A0 Pin,
>>> +=C2=A0 IN=C2=A0 UINTN=C2=A0=C2=A0 Pud
>>> +);
>>> +
>>> =C2=A0 #endif /* __GPIO_LIB__ */
>>> diff --git a/Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.c=20
>>> b/Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.c
>>> index 542b6e8f6b2f..a4b4af59ebb1 100644
>>> --- a/Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.c
>>> +++ b/Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.c
>>> @@ -2,6 +2,7 @@
>>> =C2=A0=C2=A0 *
>>> =C2=A0=C2=A0 *=C2=A0 GPIO manipulation.
>>> =C2=A0=C2=A0 *
>>> + *=C2=A0 Copyright (c) 2020, Pete Batard <pete@akeo.ie>
>>> =C2=A0=C2=A0 *=C2=A0 Copyright (c) 2018, Andrei Warkentin <andrey.war=
kentin@gmail.com>
>>> =C2=A0=C2=A0 *
>>> =C2=A0=C2=A0 *=C2=A0 SPDX-License-Identifier: BSD-2-Clause-Patent
>>> @@ -13,6 +14,7 @@
>>> =C2=A0 #include <Library/DebugLib.h>
>>> =C2=A0 #include <Library/IoLib.h>
>>> =C2=A0 #include <Library/GpioLib.h>
>>> +#include <Library/TimerLib.h>
>>> =C2=A0 #include <IndustryStandard/Bcm2836.h>
>>> =C2=A0 #include <IndustryStandard/Bcm2836Gpio.h>
>>> @@ -81,3 +83,94 @@ GpioPinFuncGet (
>>> =C2=A0=C2=A0=C2=A0 Val &=3D GPIO_FSEL_MASK;
>>> =C2=A0=C2=A0=C2=A0 return Val;
>>> =C2=A0 }
>>> +
>>> +VOID
>>> +GpioPinSet (
>>> +=C2=A0 IN=C2=A0 UINTN Pin,
>>> +=C2=A0 IN=C2=A0 UINTN Val
>>> +=C2=A0 )
>>> +{
>>> +=C2=A0 EFI_PHYSICAL_ADDRESS Reg;
>>> +=C2=A0 UINTN RegIndex;
>>> +=C2=A0 UINTN SelIndex;
>>> +
>>> +=C2=A0 ASSERT (Pin < GPIO_PINS);
>>> +
>>> +=C2=A0 RegIndex =3D Pin / 32;
>>> +=C2=A0 SelIndex =3D Pin % 32;
>>> +
>>> +=C2=A0 //
>>> +=C2=A0 // Different base addresses are used for clear and set
>>> +=C2=A0 //
>>> +=C2=A0 Reg =3D (Val =3D=3D 0) ? GPIO_GPCLR0 : GPIO_GPSET0;
>>> +=C2=A0 Reg +=3D RegIndex * sizeof (UINT32);
>>> +=C2=A0 MmioWrite32 (Reg, 1 << SelIndex);
>>> +}
>>> +
>>> +UINTN
>>> +GpioPinGet (
>>> +=C2=A0 IN=C2=A0 UINTN Pin
>>> +=C2=A0 )
>>> +{
>>> +=C2=A0 EFI_PHYSICAL_ADDRESS Reg;
>>> +=C2=A0 UINTN RegIndex;
>>> +=C2=A0 UINTN SelIndex;
>>> +=C2=A0 UINT32 Val;
>>> +
>>> +=C2=A0 ASSERT (Pin < GPIO_PINS);
>>> +
>>> +=C2=A0 RegIndex =3D Pin / 32;
>>> +=C2=A0 SelIndex =3D Pin % 32;
>>> +
>>> +=C2=A0 Reg =3D RegIndex * sizeof (UINT32) + GPIO_GPLEV0;
>>> +=C2=A0 Val =3D MmioRead32 (Reg) >> SelIndex;
>>> +=C2=A0 return Val & 1;
>>> +}
>>> +
>>> +VOID
>>> +GpioSetPull (
>>> +=C2=A0 IN=C2=A0 UINTN Pin,
>>> +=C2=A0 IN=C2=A0 UINTN Pud
>>> +)
>>> +{
>>> +=C2=A0 EFI_PHYSICAL_ADDRESS Reg;
>>> +=C2=A0 UINTN RegIndex;
>>> +=C2=A0 UINTN SelIndex;
>>> +=C2=A0 UINT32 Val;
>>> +
>>> +=C2=A0 ASSERT (Pin < GPIO_PINS);
>>> +=C2=A0 ASSERT (Pud <=3D GPIO_PULL_UP);
>>> +
>>> +=C2=A0 //
>>> +=C2=A0 // Check if we should use the legacy or newer method of
>>> +=C2=A0 // enabling pullup/pulldown by querying GPPUPPDN3, which
>>> +=C2=A0 // is unused on Bcm2835/2836/2837 and returns 'gpio'.
>>> +=C2=A0 //
>>> +=C2=A0 if (MmioRead32 (GPIO_GPPUPPDN3) =3D=3D 0x6770696f) {
>>> +=C2=A0=C2=A0=C2=A0 RegIndex =3D Pin / 32;
>>> +=C2=A0=C2=A0=C2=A0 SelIndex =3D Pin % 32;
>>> +
>>> +=C2=A0=C2=A0=C2=A0 MmioWrite32 (GPIO_GPPUD, Pud);
>>> +=C2=A0=C2=A0=C2=A0 MicroSecondDelay (5);
>>> +
>>> +=C2=A0=C2=A0=C2=A0 Reg =3D RegIndex * sizeof (UINT32) + GPIO_GPPUDCL=
K0;
>>> +=C2=A0=C2=A0=C2=A0 MmioWrite32 (Reg, 1 << SelIndex);
>>> +=C2=A0=C2=A0=C2=A0 MicroSecondDelay (5);
>>> +
>>> +=C2=A0=C2=A0=C2=A0 MmioWrite32 (GPIO_GPPUD, 0);
>>> +=C2=A0=C2=A0=C2=A0 MmioWrite32 (Reg, 0);
>>> +=C2=A0 } else {
>>> +=C2=A0=C2=A0=C2=A0 //
>>> +=C2=A0=C2=A0=C2=A0 // The newer method uses inverted values for up/d=
own.
>>> +=C2=A0=C2=A0=C2=A0 //
>>> +=C2=A0=C2=A0=C2=A0 if (Pud !=3D 0)
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Pud ^=3D 0x3;
>>> +=C2=A0=C2=A0=C2=A0 RegIndex =3D Pin / 16;
>>> +=C2=A0=C2=A0=C2=A0 SelIndex =3D (Pin % 16) * 2;
>>> +=C2=A0=C2=A0=C2=A0 Reg =3D RegIndex * sizeof (UINT32) + GPIO_GPPUPPD=
N0;
>>> +=C2=A0=C2=A0=C2=A0 Val =3D MmioRead32 (Reg);
>>> +=C2=A0=C2=A0=C2=A0 Val &=3D ~(0x3 << SelIndex);
>>> +=C2=A0=C2=A0=C2=A0 Val |=3D Pud << SelIndex;
>>> +=C2=A0=C2=A0=C2=A0 MmioWrite32 (Reg, Val);
>>> +=C2=A0 }
>>> +}
>>> diff --git a/Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.inf=20
>>> b/Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.inf
>>> index ff1b5af6db6e..a0356306d83c 100644
>>> --- a/Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.inf
>>> +++ b/Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.inf
>>> @@ -30,6 +30,7 @@ [LibraryClasses]
>>> =C2=A0=C2=A0=C2=A0 BaseLib
>>> =C2=A0=C2=A0=C2=A0 DebugLib
>>> =C2=A0=C2=A0=C2=A0 IoLib
>>> +=C2=A0 TimerLib
>>> =C2=A0 [FixedPcd]
>>> =C2=A0=C2=A0=C2=A0 gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress
>>>
>>
>=20