* [PATCH 0/2] PEI phase boot mode setting @ 2017-11-01 13:11 Ard Biesheuvel 2017-11-01 13:11 ` [PATCH 1/2] EmbeddedPkg: introduce GPIO PPI Ard Biesheuvel 2017-11-01 13:11 ` [PATCH 2/2] ArmPlatformPkg/PlatformPeim: allow PlatformPeiLib to set the boot mode Ard Biesheuvel 0 siblings, 2 replies; 7+ messages in thread From: Ard Biesheuvel @ 2017-11-01 13:11 UTC (permalink / raw) To: edk2-devel, leif.lindholm; +Cc: daniel.thompson, Ard Biesheuvel Two patches to allow a platform to set the boot mode from its PlatformPeiLib implementation using services exposed by a GPIO PPI. These are needed to implement a 'clear NVRAM' feature based on a DIP switch on the Developer Box platform. Ard Biesheuvel (2): EmbeddedPkg: introduce GPIO PPI ArmPlatformPkg/PlatformPeim: allow PlatformPeiLib to set the boot mode ArmPlatformPkg/PlatformPei/PlatformPeim.c | 12 +- ArmPlatformPkg/PlatformPei/PlatformPeim.inf | 1 + EmbeddedPkg/EmbeddedPkg.dec | 3 + EmbeddedPkg/Include/Ppi/EmbeddedGpio.h | 151 ++++++++++++++++++++ 4 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 EmbeddedPkg/Include/Ppi/EmbeddedGpio.h -- 2.11.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] EmbeddedPkg: introduce GPIO PPI 2017-11-01 13:11 [PATCH 0/2] PEI phase boot mode setting Ard Biesheuvel @ 2017-11-01 13:11 ` Ard Biesheuvel 2017-11-05 5:38 ` Leif Lindholm 2017-11-01 13:11 ` [PATCH 2/2] ArmPlatformPkg/PlatformPeim: allow PlatformPeiLib to set the boot mode Ard Biesheuvel 1 sibling, 1 reply; 7+ messages in thread From: Ard Biesheuvel @ 2017-11-01 13:11 UTC (permalink / raw) To: edk2-devel, leif.lindholm; +Cc: daniel.thompson, Ard Biesheuvel Introduce a PPI counterpart of the existing 'embedded GPIO' protocol, so we can manipulate GPIOs from PEI modules. This allows things like setting the boot mode based on a DIP switch setting. Note that the naming is slightly awkward, as there is nothing 'embedded' about a GPIO, but given that the DXE protocol already resides here and has the 'embedded' prefix, it makes sense to retain uniformity. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- EmbeddedPkg/EmbeddedPkg.dec | 3 + EmbeddedPkg/Include/Ppi/EmbeddedGpio.h | 151 ++++++++++++++++++++ 2 files changed, 154 insertions(+) diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec index 52482af13aeb..cb07d3ece685 100644 --- a/EmbeddedPkg/EmbeddedPkg.dec +++ b/EmbeddedPkg/EmbeddedPkg.dec @@ -86,6 +86,9 @@ [Protocols.common] gPlatformGpioProtocolGuid = { 0x52ce9845, 0x5af4, 0x43e2, {0xba, 0xfd, 0x23, 0x08, 0x12, 0x54, 0x7a, 0xc2 }} gAndroidBootImgProtocolGuid = { 0x9859bb19, 0x407c, 0x4f8b, {0xbc, 0xe1, 0xf8, 0xda, 0x65, 0x65, 0xf4, 0xa5 }} +[Ppis] + gEdkiiEmbeddedGpioPpiGuid = { 0x21c3b115, 0x4e0b, 0x470c, { 0x85, 0xc7, 0xe1, 0x05, 0xa5, 0x75, 0xc9, 0x7b }} + [PcdsFeatureFlag.common] gEmbeddedTokenSpaceGuid.PcdEmbeddedMacBoot|FALSE|BOOLEAN|0x00000001 gEmbeddedTokenSpaceGuid.PcdEmbeddedDirCmd|TRUE|BOOLEAN|0x00000002 diff --git a/EmbeddedPkg/Include/Ppi/EmbeddedGpio.h b/EmbeddedPkg/Include/Ppi/EmbeddedGpio.h new file mode 100644 index 000000000000..d87c860a17fb --- /dev/null +++ b/EmbeddedPkg/Include/Ppi/EmbeddedGpio.h @@ -0,0 +1,151 @@ +/** @file + + Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> + Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR> + + This program and the accompanying materials + are licensed and made available under the terms and conditions of the BSD License + which accompanies this distribution. The full text of the license may be found at + http://opensource.org/licenses/bsd-license.php + + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. + +**/ + +#ifndef __EMBEDDED_GPIO_PPI_H__ +#define __EMBEDDED_GPIO_PPI_H__ + +// +// Protocol interface structure +// +typedef struct _EMBEDDED_GPIO_PPI EMBEDDED_GPIO_PPI; + +// +// Data Types +// +typedef UINTN EMBEDDED_GPIO_PIN; + +#define GPIO(Port, Pin) ((EMBEDDED_GPIO_PIN)(((Port) << (16)) | (Pin))) +#define GPIO_PIN(x) ((EMBEDDED_GPIO_PIN)(x) & (0xFFFF)) +#define GPIO_PORT(x) ((EMBEDDED_GPIO_PIN)(x) >> (16)) + +typedef enum { + GPIO_MODE_INPUT = 0x00, + GPIO_MODE_OUTPUT_0 = 0x0E, + GPIO_MODE_OUTPUT_1 = 0x0F, + GPIO_MODE_SPECIAL_FUNCTION_2 = 0x02, + GPIO_MODE_SPECIAL_FUNCTION_3 = 0x03, + GPIO_MODE_SPECIAL_FUNCTION_4 = 0x04, + GPIO_MODE_SPECIAL_FUNCTION_5 = 0x05, + GPIO_MODE_SPECIAL_FUNCTION_6 = 0x06, + GPIO_MODE_SPECIAL_FUNCTION_7 = 0x07 +} EMBEDDED_GPIO_MODE; + +typedef enum { + GPIO_PULL_NONE, + GPIO_PULL_UP, + GPIO_PULL_DOWN +} EMBEDDED_GPIO_PULL; + +// +// Function Prototypes +// + +/** + + Gets the state of a GPIO pin + + @param This Pointer to protocol + @param Gpio Which pin to read + @param Value State of the pin + + @retval EFI_SUCCESS GPIO state returned in Value + @retval EFI_INVALID_PARAMETER Value is NULL + @retval EFI_NOT_FOUND Pin does not exit + +**/ +typedef +EFI_STATUS +(EFIAPI *EMBEDDED_GPIO_GET) ( + IN EMBEDDED_GPIO_PPI *This, + IN EMBEDDED_GPIO_PIN Gpio, + OUT UINTN *Value + ); + +/** + + Sets the state of a GPIO pin + + @param This Pointer to protocol + @param Gpio Which pin to modify + @param Mode Mode to set + + @retval EFI_SUCCESS GPIO set as requested + @retval EFI_INVALID_PARAMETER Invalid mode + @retval EFI_NOT_FOUND Pin does not exit + +**/ +typedef +EFI_STATUS +(EFIAPI *EMBEDDED_GPIO_SET) ( + IN EMBEDDED_GPIO_PPI *This, + IN EMBEDDED_GPIO_PIN Gpio, + IN EMBEDDED_GPIO_MODE Mode + ); + + +/** + + Gets the mode (function) of a GPIO pin + + @param This Pointer to protocol + @param Gpio Which pin + @param Mode Pointer to output mode value + + @retval EFI_SUCCESS Mode value retrieved + @retval EFI_INVALID_PARAMETER Mode is NULL + @retval EFI_NOT_FOUND Pin does not exit + +**/ +typedef +EFI_STATUS +(EFIAPI *EMBEDDED_GPIO_GET_MODE) ( + IN EMBEDDED_GPIO_PPI *This, + IN EMBEDDED_GPIO_PIN Gpio, + OUT EMBEDDED_GPIO_MODE *Mode + ); + + +/** + + Sets the pull-up / pull-down resistor of a GPIO pin + + @param This Pointer to PPI + @param Gpio Port/pin index + @param Pull The pullup/pulldown mode to set + + @retval EFI_SUCCESS Mode was set + @retval EFI_NOT_FOUND Pin does not exist + @retval EFI_UNSUPPORTED Action not supported + +**/ +typedef +EFI_STATUS +(EFIAPI *EMBEDDED_GPIO_SET_PULL) ( + IN EMBEDDED_GPIO_PPI *This, + IN EMBEDDED_GPIO_PIN Gpio, + IN EMBEDDED_GPIO_PULL Direction + ); + + +struct _EMBEDDED_GPIO_PPI { + EMBEDDED_GPIO_GET Get; + EMBEDDED_GPIO_SET Set; + EMBEDDED_GPIO_GET_MODE GetMode; + EMBEDDED_GPIO_SET_PULL SetPull; +}; + +extern EFI_GUID gEmbeddedGpioPpiGuid; + +#endif -- 2.11.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] EmbeddedPkg: introduce GPIO PPI 2017-11-01 13:11 ` [PATCH 1/2] EmbeddedPkg: introduce GPIO PPI Ard Biesheuvel @ 2017-11-05 5:38 ` Leif Lindholm 2017-11-05 10:05 ` Ard Biesheuvel 0 siblings, 1 reply; 7+ messages in thread From: Leif Lindholm @ 2017-11-05 5:38 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: edk2-devel, daniel.thompson On Wed, Nov 01, 2017 at 01:11:44PM +0000, Ard Biesheuvel wrote: > Introduce a PPI counterpart of the existing 'embedded GPIO' protocol, > so we can manipulate GPIOs from PEI modules. This allows things like > setting the boot mode based on a DIP switch setting. > > Note that the naming is slightly awkward, as there is nothing 'embedded' > about a GPIO, but given that the DXE protocol already resides here and > has the 'embedded' prefix, it makes sense to retain uniformity. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> I have no objection to this, so: Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> ...but I do have a question: Do we strictly need to duplicate this include file? It feels like a bit of a maintenance hazard/burden. / Leif ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] EmbeddedPkg: introduce GPIO PPI 2017-11-05 5:38 ` Leif Lindholm @ 2017-11-05 10:05 ` Ard Biesheuvel 2017-11-05 16:21 ` Ard Biesheuvel 0 siblings, 1 reply; 7+ messages in thread From: Ard Biesheuvel @ 2017-11-05 10:05 UTC (permalink / raw) To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Daniel Thompson On 5 November 2017 at 05:38, Leif Lindholm <leif.lindholm@linaro.org> wrote: > On Wed, Nov 01, 2017 at 01:11:44PM +0000, Ard Biesheuvel wrote: >> Introduce a PPI counterpart of the existing 'embedded GPIO' protocol, >> so we can manipulate GPIOs from PEI modules. This allows things like >> setting the boot mode based on a DIP switch setting. >> >> Note that the naming is slightly awkward, as there is nothing 'embedded' >> about a GPIO, but given that the DXE protocol already resides here and >> has the 'embedded' prefix, it makes sense to retain uniformity. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > I have no objection to this, so: > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > > ...but I do have a question: > Do we strictly need to duplicate this include file? It feels like a > bit of a maintenance hazard/burden. > Yeah, I wondered about that as well, but given that it's only two enum typedefs and a couple of macros that we can really share, I didn't bother. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] EmbeddedPkg: introduce GPIO PPI 2017-11-05 10:05 ` Ard Biesheuvel @ 2017-11-05 16:21 ` Ard Biesheuvel 0 siblings, 0 replies; 7+ messages in thread From: Ard Biesheuvel @ 2017-11-05 16:21 UTC (permalink / raw) To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Daniel Thompson On 5 November 2017 at 10:05, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 5 November 2017 at 05:38, Leif Lindholm <leif.lindholm@linaro.org> wrote: >> On Wed, Nov 01, 2017 at 01:11:44PM +0000, Ard Biesheuvel wrote: >>> Introduce a PPI counterpart of the existing 'embedded GPIO' protocol, >>> so we can manipulate GPIOs from PEI modules. This allows things like >>> setting the boot mode based on a DIP switch setting. >>> >>> Note that the naming is slightly awkward, as there is nothing 'embedded' >>> about a GPIO, but given that the DXE protocol already resides here and >>> has the 'embedded' prefix, it makes sense to retain uniformity. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> I have no objection to this, so: >> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> >> >> ...but I do have a question: >> Do we strictly need to duplicate this include file? It feels like a >> bit of a maintenance hazard/burden. >> > > Yeah, I wondered about that as well, but given that it's only two enum > typedefs and a couple of macros that we can really share, I didn't > bother. Pushed as b1832e16ddce EmbeddedPkg: introduce GPIO PPI 3909c4a19344 ArmPlatformPkg/PlatformPeim: allow PlatformPeiLib to set the boot mode Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] ArmPlatformPkg/PlatformPeim: allow PlatformPeiLib to set the boot mode 2017-11-01 13:11 [PATCH 0/2] PEI phase boot mode setting Ard Biesheuvel 2017-11-01 13:11 ` [PATCH 1/2] EmbeddedPkg: introduce GPIO PPI Ard Biesheuvel @ 2017-11-01 13:11 ` Ard Biesheuvel 2017-11-05 5:43 ` Leif Lindholm 1 sibling, 1 reply; 7+ messages in thread From: Ard Biesheuvel @ 2017-11-01 13:11 UTC (permalink / raw) To: edk2-devel, leif.lindholm; +Cc: daniel.thompson, Ard Biesheuvel The current interdepencies between the PrePeiCore SEC module, the platform PEIM and ArmPlatformLib is a bit awkward: due to the fact that ArmPlatformLib is also used by SEC modules, we cannot use PEI specific facilities in the implementation of ArmPlatformGetBootMode. However, given that we call that library function /after/ invoking PlatformPeiLib, there is no way for that library to set the boot mode other than resorting to tricks like notification callbacks on arbitrary unrelated events. ArmPlatformLib should probably be phased out anyway, given its quirky nature, but for now, let's fix this particular issue by deferring the call to PlatformPeim() to after the point where we set the boot mode by calling ArmPlatformGetBootMode (). While we're at it, clean up the code slightly by using PeiServicesLib instead of doing double pointer dereferencing. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmPlatformPkg/PlatformPei/PlatformPeim.c | 12 +++++++----- ArmPlatformPkg/PlatformPei/PlatformPeim.inf | 1 + 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/ArmPlatformPkg/PlatformPei/PlatformPeim.c b/ArmPlatformPkg/PlatformPei/PlatformPeim.c index e4535250c245..14f301e947a8 100644 --- a/ArmPlatformPkg/PlatformPei/PlatformPeim.c +++ b/ArmPlatformPkg/PlatformPei/PlatformPeim.c @@ -83,21 +83,23 @@ InitializePlatformPeim ( ) { EFI_STATUS Status; - UINTN BootMode; + EFI_BOOT_MODE BootMode; DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Platform PEIM Loaded\n")); + Status = PeiServicesSetBootMode (ArmPlatformGetBootMode ()); + ASSERT_EFI_ERROR (Status); + PlatformPeim (); - BootMode = ArmPlatformGetBootMode (); - Status = (**PeiServices).SetBootMode (PeiServices, (UINT8) BootMode); + Status = PeiServicesGetBootMode (&BootMode); ASSERT_EFI_ERROR (Status); - Status = (**PeiServices).InstallPpi (PeiServices, &mPpiListBootMode); + Status = PeiServicesInstallPpi (&mPpiListBootMode); ASSERT_EFI_ERROR (Status); if (BootMode == BOOT_IN_RECOVERY_MODE) { - Status = (**PeiServices).InstallPpi (PeiServices, &mPpiListRecoveryBootMode); + Status = PeiServicesInstallPpi (&mPpiListRecoveryBootMode); ASSERT_EFI_ERROR (Status); } diff --git a/ArmPlatformPkg/PlatformPei/PlatformPeim.inf b/ArmPlatformPkg/PlatformPei/PlatformPeim.inf index f466c1412ad3..21701cdc0731 100644 --- a/ArmPlatformPkg/PlatformPei/PlatformPeim.inf +++ b/ArmPlatformPkg/PlatformPei/PlatformPeim.inf @@ -43,6 +43,7 @@ [LibraryClasses] HobLib ArmPlatformLib PlatformPeiLib + PeiServicesLib [Ppis] gEfiPeiMasterBootModePpiGuid # PPI ALWAYS_PRODUCED -- 2.11.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] ArmPlatformPkg/PlatformPeim: allow PlatformPeiLib to set the boot mode 2017-11-01 13:11 ` [PATCH 2/2] ArmPlatformPkg/PlatformPeim: allow PlatformPeiLib to set the boot mode Ard Biesheuvel @ 2017-11-05 5:43 ` Leif Lindholm 0 siblings, 0 replies; 7+ messages in thread From: Leif Lindholm @ 2017-11-05 5:43 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: edk2-devel, daniel.thompson On Wed, Nov 01, 2017 at 01:11:45PM +0000, Ard Biesheuvel wrote: > The current interdepencies between the PrePeiCore SEC module, the > platform PEIM and ArmPlatformLib is a bit awkward: due to the fact > that ArmPlatformLib is also used by SEC modules, we cannot use PEI > specific facilities in the implementation of ArmPlatformGetBootMode. > However, given that we call that library function /after/ invoking > PlatformPeiLib, there is no way for that library to set the boot mode > other than resorting to tricks like notification callbacks on arbitrary > unrelated events. > > ArmPlatformLib should probably be phased out anyway, given its quirky > nature, Yes, it should. > but for now, let's fix this particular issue by deferring the > call to PlatformPeim() to after the point where we set the boot mode > by calling ArmPlatformGetBootMode (). > > While we're at it, clean up the code slightly by using PeiServicesLib > instead of doing double pointer dereferencing. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmPlatformPkg/PlatformPei/PlatformPeim.c | 12 +++++++----- > ArmPlatformPkg/PlatformPei/PlatformPeim.inf | 1 + > 2 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/ArmPlatformPkg/PlatformPei/PlatformPeim.c b/ArmPlatformPkg/PlatformPei/PlatformPeim.c > index e4535250c245..14f301e947a8 100644 > --- a/ArmPlatformPkg/PlatformPei/PlatformPeim.c > +++ b/ArmPlatformPkg/PlatformPei/PlatformPeim.c > @@ -83,21 +83,23 @@ InitializePlatformPeim ( > ) > { > EFI_STATUS Status; > - UINTN BootMode; > + EFI_BOOT_MODE BootMode; > > DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Platform PEIM Loaded\n")); > > + Status = PeiServicesSetBootMode (ArmPlatformGetBootMode ()); > + ASSERT_EFI_ERROR (Status); > + > PlatformPeim (); > > - BootMode = ArmPlatformGetBootMode (); > - Status = (**PeiServices).SetBootMode (PeiServices, (UINT8) BootMode); > + Status = PeiServicesGetBootMode (&BootMode); > ASSERT_EFI_ERROR (Status); > > - Status = (**PeiServices).InstallPpi (PeiServices, &mPpiListBootMode); > + Status = PeiServicesInstallPpi (&mPpiListBootMode); > ASSERT_EFI_ERROR (Status); > > if (BootMode == BOOT_IN_RECOVERY_MODE) { > - Status = (**PeiServices).InstallPpi (PeiServices, &mPpiListRecoveryBootMode); > + Status = PeiServicesInstallPpi (&mPpiListRecoveryBootMode); > ASSERT_EFI_ERROR (Status); > } > > diff --git a/ArmPlatformPkg/PlatformPei/PlatformPeim.inf b/ArmPlatformPkg/PlatformPei/PlatformPeim.inf > index f466c1412ad3..21701cdc0731 100644 > --- a/ArmPlatformPkg/PlatformPei/PlatformPeim.inf > +++ b/ArmPlatformPkg/PlatformPei/PlatformPeim.inf > @@ -43,6 +43,7 @@ [LibraryClasses] > HobLib > ArmPlatformLib > PlatformPeiLib > + PeiServicesLib If you move that one up one line: Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > > [Ppis] > gEfiPeiMasterBootModePpiGuid # PPI ALWAYS_PRODUCED > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-11-05 16:17 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-11-01 13:11 [PATCH 0/2] PEI phase boot mode setting Ard Biesheuvel 2017-11-01 13:11 ` [PATCH 1/2] EmbeddedPkg: introduce GPIO PPI Ard Biesheuvel 2017-11-05 5:38 ` Leif Lindholm 2017-11-05 10:05 ` Ard Biesheuvel 2017-11-05 16:21 ` Ard Biesheuvel 2017-11-01 13:11 ` [PATCH 2/2] ArmPlatformPkg/PlatformPeim: allow PlatformPeiLib to set the boot mode Ard Biesheuvel 2017-11-05 5:43 ` Leif Lindholm
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox