Re: [edk2] [platforms: PATCH v2 08/12] Marvell/Drivers: MvGpioDxe: Introduce platform GPIO driver
wt., 15 sty 2019 o 11:04 Leif Lindholm napisał(a):
>
> On Tue, Jan 15, 2019 at 07:19:04AM +0100, Marcin Wojtas wrote:
> > > > + if (MmioRead32 (BaseAddress + MV_GPIO_OUT_EN_REG) & (1 << GpioPin)) {
> > > > +*Mode = GPIO_MODE_INPUT;
> > > > + } else {
> > > > +if (MmioRead32 (BaseAddress + MV_GPIO_DATA_IN_REG) & (1 <<
> > > > GpioPin)) {
> > > > + *Mode = GPIO_MODE_OUTPUT_1;
> > > > +} else {
> > > > + *Mode = GPIO_MODE_OUTPUT_0;
> > >
> > > Ah, right, it's the change to EMBEDDED_GPIO that means we have two
> > > output modes to return instead of just input or output.
> > > Could I just ask that you're a bit more explicit about such things in
> > > the cover letter? Would have saved me a couple of minutes of head
> > > scratching.
> >
> > Well, in the cover letter I wrote:
> > "The biggest change is dropping custom GPIO protocol and start using
> > the generic EMBEDDED_GPIO with all its types."
> >
> > And in the commit log of both drivers:
> > "The new driver implements a generic EMBEDDED_GPIO protocol."
> >
> > Wasn't it explicit enough? :)
>
> I mean, it's fine - you've technically given all the information
> needed to deduce that. But it does sort of imply that the reader knows
> all EDK2 interfaces by heart. And I'm not quite there.
>
Ok, sorry for not being specific enough.
Best regards,
Marcin
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH v2 08/12] Marvell/Drivers: MvGpioDxe: Introduce platform GPIO driver
On Tue, Jan 15, 2019 at 07:19:04AM +0100, Marcin Wojtas wrote:
> > > + if (MmioRead32 (BaseAddress + MV_GPIO_OUT_EN_REG) & (1 << GpioPin)) {
> > > +*Mode = GPIO_MODE_INPUT;
> > > + } else {
> > > +if (MmioRead32 (BaseAddress + MV_GPIO_DATA_IN_REG) & (1 << GpioPin))
> > > {
> > > + *Mode = GPIO_MODE_OUTPUT_1;
> > > +} else {
> > > + *Mode = GPIO_MODE_OUTPUT_0;
> >
> > Ah, right, it's the change to EMBEDDED_GPIO that means we have two
> > output modes to return instead of just input or output.
> > Could I just ask that you're a bit more explicit about such things in
> > the cover letter? Would have saved me a couple of minutes of head
> > scratching.
>
> Well, in the cover letter I wrote:
> "The biggest change is dropping custom GPIO protocol and start using
> the generic EMBEDDED_GPIO with all its types."
>
> And in the commit log of both drivers:
> "The new driver implements a generic EMBEDDED_GPIO protocol."
>
> Wasn't it explicit enough? :)
I mean, it's fine - you've technically given all the information
needed to deduce that. But it does sort of imply that the reader knows
all EDK2 interfaces by heart. And I'm not quite there.
/
Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH v2 08/12] Marvell/Drivers: MvGpioDxe: Introduce platform GPIO driver
Hi Leif,
wt., 15 sty 2019 o 00:32 Leif Lindholm napisał(a):
>
> On Thu, Jan 10, 2019 at 02:44:35AM +0100, Marcin Wojtas wrote:
> > Marvell Armada 7k/8k SoCs comprise integrated GPIO controllers,
> > one in AP806 and two in each south bridge hardware blocks.
> >
> > This patch introduces support for them. The new driver implements
> > a generic EMBEDDED_GPIO protocol.
> >
> > In order to ease description of used GPIO pins and controllers
> > of the Armada 7k8k platforms, add a common enum type.
> >
> > Based on original work of Jing Hua .
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas
> > ---
> > Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf | 44 +++
> > Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h | 49 +++
> > Silicon/Marvell/Include/Protocol/MvGpio.h| 10 +
> > Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c | 372
> >
> > 4 files changed, 475 insertions(+)
> > create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> > create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> > create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c
> >
> > diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> > b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> > new file mode 100644
> > index 000..5ff9130
> > --- /dev/null
> > +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> > @@ -0,0 +1,44 @@
> > +## @file
> > +#
> > +# Copyright (c) 2017, Marvell International Ltd. All rights reserved.
> > +#
> > +# 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.
> > +#
> > +
> > +[Defines]
> > + INF_VERSION= 0x0001001A
> > + BASE_NAME = MvGpioDxe
> > + FILE_GUID = 706eb761-b3b5-4f41-8558-5fd9217c0079
> > + MODULE_TYPE= DXE_DRIVER
> > + VERSION_STRING = 1.0
> > + ENTRY_POINT= MvGpioEntryPoint
> > +
> > +[Sources]
> > + MvGpioDxe.c
> > + MvGpioDxe.h
> > +
> > +[Packages]
> > + EmbeddedPkg/EmbeddedPkg.dec
> > + MdeModulePkg/MdeModulePkg.dec
> > + MdePkg/MdePkg.dec
> > + Silicon/Marvell/Marvell.dec
> > +
> > +[LibraryClasses]
> > + ArmadaSoCDescLib
> > + DebugLib
> > + MemoryAllocationLib
> > + UefiDriverEntryPoint
> > + UefiLib
> > +
> > +[Protocols]
> > + gEmbeddedGpioProtocolGuid
> > + gMarvellBoardDescProtocolGuid
> > +
> > +[Depex]
> > + TRUE
> > diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> > b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> > new file mode 100644
> > index 000..4e5422b
> > --- /dev/null
> > +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> > @@ -0,0 +1,49 @@
> > +/**
> > +*
> > +* Copyright (c) 2018, Marvell International Ltd. All rights reserved.
> > +*
> > +* 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 __MV_GPIO_H__
> > +#define __MV_GPIO_H__
> > +
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +
> > +#include
> > +
> > +#define MV_GPIO_SIGNATURESIGNATURE_32 ('G', 'P', 'I', 'O')
>
> Unaddressed feedback from last time. But rereading it makes it clear
> my feedback was too imprecise, apologies. The point was that "GPIO" is
> not a very good choice of key to distinguish _this_ GPIO driver from
> other GPIO drivers. (As well as the MV_ prefix.)
>
Sure, I will modify it to be more specific signature.
> One more comment below, but it does not require code changes.
> If you can just change the signature, I'm happy with this patch.
>
> > +
> > +// Marvell MV_GPIO Controller Registers
> > +#define MV_GPIO_DATA_OUT_REG (0x0)
> > +#define MV_GPIO_OUT_EN_REG (0x4)
> > +#define MV_GPIO_BLINK_EN_REG (0x8)
> > +#define MV_GPIO_DATA_IN_POL_REG (0xc)
> > +#define MV_GPIO_DATA_IN_REG (0x10)
> > +
> > +typedef struct {
> > + EMBEDDED_GPIO GpioProtocol;
> > + GPIO_CONTROLLER *SoCGpio;
> > + UINTN GpioDeviceCount;
> > + UINTN Signature;
> > +
Re: [edk2] [platforms: PATCH v2 08/12] Marvell/Drivers: MvGpioDxe: Introduce platform GPIO driver
On Thu, Jan 10, 2019 at 02:44:35AM +0100, Marcin Wojtas wrote:
> Marvell Armada 7k/8k SoCs comprise integrated GPIO controllers,
> one in AP806 and two in each south bridge hardware blocks.
>
> This patch introduces support for them. The new driver implements
> a generic EMBEDDED_GPIO protocol.
>
> In order to ease description of used GPIO pins and controllers
> of the Armada 7k8k platforms, add a common enum type.
>
> Based on original work of Jing Hua .
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas
> ---
> Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf | 44 +++
> Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h | 49 +++
> Silicon/Marvell/Include/Protocol/MvGpio.h| 10 +
> Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c | 372
>
> 4 files changed, 475 insertions(+)
> create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c
>
> diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> new file mode 100644
> index 000..5ff9130
> --- /dev/null
> +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> @@ -0,0 +1,44 @@
> +## @file
> +#
> +# Copyright (c) 2017, Marvell International Ltd. All rights reserved.
> +#
> +# 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.
> +#
> +
> +[Defines]
> + INF_VERSION= 0x0001001A
> + BASE_NAME = MvGpioDxe
> + FILE_GUID = 706eb761-b3b5-4f41-8558-5fd9217c0079
> + MODULE_TYPE= DXE_DRIVER
> + VERSION_STRING = 1.0
> + ENTRY_POINT= MvGpioEntryPoint
> +
> +[Sources]
> + MvGpioDxe.c
> + MvGpioDxe.h
> +
> +[Packages]
> + EmbeddedPkg/EmbeddedPkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> + MdePkg/MdePkg.dec
> + Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> + ArmadaSoCDescLib
> + DebugLib
> + MemoryAllocationLib
> + UefiDriverEntryPoint
> + UefiLib
> +
> +[Protocols]
> + gEmbeddedGpioProtocolGuid
> + gMarvellBoardDescProtocolGuid
> +
> +[Depex]
> + TRUE
> diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> new file mode 100644
> index 000..4e5422b
> --- /dev/null
> +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> @@ -0,0 +1,49 @@
> +/**
> +*
> +* Copyright (c) 2018, Marvell International Ltd. All rights reserved.
> +*
> +* 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 __MV_GPIO_H__
> +#define __MV_GPIO_H__
> +
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include
> +
> +#define MV_GPIO_SIGNATURESIGNATURE_32 ('G', 'P', 'I', 'O')
Unaddressed feedback from last time. But rereading it makes it clear
my feedback was too imprecise, apologies. The point was that "GPIO" is
not a very good choice of key to distinguish _this_ GPIO driver from
other GPIO drivers. (As well as the MV_ prefix.)
One more comment below, but it does not require code changes.
If you can just change the signature, I'm happy with this patch.
> +
> +// Marvell MV_GPIO Controller Registers
> +#define MV_GPIO_DATA_OUT_REG (0x0)
> +#define MV_GPIO_OUT_EN_REG (0x4)
> +#define MV_GPIO_BLINK_EN_REG (0x8)
> +#define MV_GPIO_DATA_IN_POL_REG (0xc)
> +#define MV_GPIO_DATA_IN_REG (0x10)
> +
> +typedef struct {
> + EMBEDDED_GPIO GpioProtocol;
> + GPIO_CONTROLLER *SoCGpio;
> + UINTN GpioDeviceCount;
> + UINTN Signature;
> + EFI_HANDLEHandle;
> +} MV_GPIO;
> +
> +#endif // __MV_GPIO_H__
> diff --git a/Silicon/Marvell/Include/Protocol/MvGpio.h
> b/Silicon/Marvell/Include/Protocol/MvGpio.h
> index c9f1007..3319b79 100644
> --- a/Silicon/Marvell/Include/Protocol/MvGpio.h
> +++ b/Silicon/Marvell/Include/Protocol/MvGpio.h
> @@ -27,6 +27,16 @@ typedef enum {
>MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER,
> }
