Change in coreboot[master]: soc/intel/common/gpio_defs: Remove incorrent GPO macro

[Maxim Polyakov (Code Review)]

Maxim Polyakov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44471 )

Change subject: soc/intel/common/gpio_defs: Remove incorrent GPO macro
......................................................................

soc/intel/common/gpio_defs: Remove incorrent GPO macro

GPIO Driver mode is used for configuration interrupt routing for
external devices through GPI. But there is no point in configuring
this for GPO. For this reason, this patch removes the
PAD_CFG_GPO_GPIO_DRIVER macro as it is not correct. This has also
been removed from the pad configuration in the follow boards and
this patch should be added after:

[1] Change-Id: I5aed5769600bb9c442ad13a2efbcf9d5fd8da537
[2] Change-Id: Iac7d674e79e0caee467fc087e6d36192e84a12d8
[3] Change-Id: Icbbc644adda0dde768171e9581ae345cb9750eea
[4] Change-Id: I406a08e526a6c655f38e4c0a355957c98e93881c

Change-Id: I74c318897647836f4604a937543254f44b470433
Signed-off-by: Maxim Polyakov 
---
M Documentation/getting_started/gpio.md
M src/soc/intel/common/block/include/intelblocks/gpio_defs.h
2 files changed, 0 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/44471/1

diff --git a/Documentation/getting_started/gpio.md b/Documentation/getting_started/gpio.md
index 13aeed5..339289b 100644
--- a/Documentation/getting_started/gpio.md
+++ b/Documentation/getting_started/gpio.md
@@ -92,8 +92,6 @@
     #define PAD_CFG_GPO(pad, val, rst)
     /* General purpose output, with termination specified */
     #define PAD_CFG_TERM_GPO(pad, val, pull, rst)
-    /* General purpose output, no pullup/down. */
-    #define PAD_CFG_GPO_GPIO_DRIVER(pad, val, rst, pull)
     /* General purpose input */
     #define PAD_CFG_GPI(pad, pull, rst)
 ```
diff --git a/src/soc/intel/common/block/include/intelblocks/gpio_defs.h b/src/soc/intel/common/block/include/intelblocks/gpio_defs.h
index 3d3c831..4dcf04c 100644
--- a/src/soc/intel/common/block/include/intelblocks/gpio_defs.h
+++ b/src/soc/intel/common/block/include/intelblocks/gpio_defs.h
@@ -229,14 +229,6 @@
    	PAD_TRIG(OFF) | PAD_BUF(RX_DISABLE) | !!val,	\
    	PAD_PULL(pull) | PAD_IOSSTATE(TxLASTRxE))

-/* General purpose output, no pullup/down. */
-#define PAD_CFG_GPO_GPIO_DRIVER(pad, val, rst, pull)		\
-	_PAD_CFG_STRUCT(pad,					\
-		PAD_FUNC(GPIO) | PAD_RESET(rst) |		\
-		PAD_TRIG(OFF) | PAD_BUF(RX_DISABLE) | !!val,	\
-		PAD_PULL(pull) | PAD_IOSSTATE(TxLASTRxE) |	\
-		PAD_CFG_OWN_GPIO(DRIVER))
-
 /* General purpose output. */
 #define PAD_CFG_GPO_IOSSTATE_IOSTERM(pad, val, rst, pull, iosstate, ioterm)	\
    _PAD_CFG_STRUCT(pad,							\

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44471 )

Change subject: soc/intel/common/gpio_defs: Remove incorrent GPO macro
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44471/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44471/2//COMMIT_MSG@9 
PS2, Line 9: GPIO Driver mode is used for configuration interrupt routing for
           : external devices through GPI. But there is no point in configuring
           : this for GPO.
do we have any datasheet reference here? I'm still unsure about this driver mode...

GpioIo in ACPI seems to be valid for outputs, too, where driver mode can (shall?) be set according to FSP (3rdparty/fsp/CometLakeFspBinPkg/CometLake1/Include/GpioConfig.h for example)

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44471 )

Change subject: soc/intel/common/gpio_defs: Remove incorrent GPO macro
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/44471/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44471/2//COMMIT_MSG@7 
PS2, Line 7: incorrent
Incoherent? Incorrect?

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44471 )

Change subject: soc/intel/common/gpio_defs: Remove incorrent GPO macro
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44471/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44471/2//COMMIT_MSG@7 
PS2, Line 7: incorrent
> Incoherent? Incorrect?

I guess the latter ;)

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44471 )

Change subject: soc/intel/common/gpio_defs: Remove incorrent GPO macro
......................................................................

Patch Set 2:

Just what I found out so it's not forgotten: d28bd0c introduced
the macro. Most likely by accident, because the commit message
said it would be porting things from APL but that was an under-
statement that most likely deceived reviewers. The macro didn't
exist before. It seems likely that it was never reviewed.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44471 )

Change subject: soc/intel/common/gpio_defs: Remove incorrent GPO macro
......................................................................

Patch Set 2: Code-Review-1

Sorry to delaying this, once again :S I found something in the linux pinctrl driver (linux/drivers/pinctrl/intel/pinctrl-intel.c). Look for intel_config_get/set(), intel_pad_usable() and intel_pad_owned_by_host(). pinctrl only allows to control pads that have ownership set to driver. That confirms my initial idea that driver mode is to control which pads are allowed to be altered by the OS (driver mode) or not (ACPI mode). Further, it applies to GPI, as well as GPO.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44471 )

Change subject: soc/intel/common/gpio_defs: Remove incorrent GPO macro
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review-1
>
> Sorry to delaying this, once again :S I found something in the linux pinctrl driver (linux/drivers/pinctrl/intel/pinctrl-intel.c). Look for intel_config_get/set(), intel_pad_usable() and intel_pad_owned_by_host(). pinctrl only allows to control pads that have ownership set to driver. That confirms my initial idea that driver mode is to control which pads are allowed to be altered by the OS (driver mode) or not (ACPI mode). Further, it applies to GPI, as well as GPO.

During some research I noticed that some vendors (at least vendors using Insyde UEFI like Supermicro) set driver mode for GPIOs that read board id, board revision, bios reset jumper, nmi jumper and all sorts of other stuff. Most of them are *not* using any interrupt at all. Also GPOs used for signalling to BMC (POST done) are set to driver mode.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44471 )

Change subject: soc/intel/common/gpio_defs: Remove incorrent GPO macro
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2: Code-Review-1
> >
> > Sorry to delaying this, once again :S I found something in the linux pinctrl driver (linux/drivers/pinctrl/intel/pinctrl-intel.c). Look for intel_config_get/set(), intel_pad_usable() and intel_pad_owned_by_host(). pinctrl only allows to control pads that have ownership set to driver. That confirms my initial idea that driver mode is to control which pads are allowed to be altered by the OS (driver mode) or not (ACPI mode). Further, it applies to GPI, as well as GPO.
>
> During some research I noticed that some vendors (at least vendors using Insyde UEFI like Supermicro) set driver mode for GPIOs that read board id, board revision, bios reset jumper, nmi jumper and all sorts of other stuff. Most of them are *not* using any interrupt at all. Also GPOs used for signalling to BMC (POST done) are set to driver mode.

This one is veeeery similiar to what Supermicro does on the X11SSM/X11SSH btw. I'm pretty sure they use the SkylakeOpenBoardPkg ;-)
https://github.com/maxpoliak/purley-platforms/blob/master/Platform/Intel/Pur...

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44471 )

Change subject: soc/intel/common/gpio_defs: Remove incorrent GPO macro
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2: Code-Review-1
> > >
> > > Sorry to delaying this, once again :S I found something in the linux pinctrl driver (linux/drivers/pinctrl/intel/pinctrl-intel.c). Look for intel_config_get/set(), intel_pad_usable() and intel_pad_owned_by_host(). pinctrl only allows to control pads that have ownership set to driver. That confirms my initial idea that driver mode is to control which pads are allowed to be altered by the OS (driver mode) or not (ACPI mode). Further, it applies to GPI, as well as GPO.
> >
> > During some research I noticed that some vendors (at least vendors using Insyde UEFI like Supermicro) set driver mode for GPIOs that read board id, board revision, bios reset jumper, nmi jumper and all sorts of other stuff. Most of them are *not* using any interrupt at all. Also GPOs used for signalling to BMC (POST done) are set to driver mode.
>
> This one is veeeery similiar to what Supermicro does on the X11SSM/X11SSH btw. I'm pretty sure they use the SkylakeOpenBoardPkg ;-)
> https://github.com/maxpoliak/purley-platforms/blob/master/Platform/Intel/Pur...

I think you can find schematics for this somewhere. I have them, at least.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44471 )

Change subject: soc/intel/common/gpio_defs: Remove incorrent GPO macro
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2:
> > > > Patch Set 2: Code-Review-1
> > > >
> > > > Sorry to delaying this, once again :S I found something in the linux pinctrl driver (linux/drivers/pinctrl/intel/pinctrl-intel.c). Look for intel_config_get/set(), intel_pad_usable() and intel_pad_owned_by_host(). pinctrl only allows to control pads that have ownership set to driver. That confirms my initial idea that driver mode is to control which pads are allowed to be altered by the OS (driver mode) or not (ACPI mode). Further, it applies to GPI, as well as GPO.
> > >
> > > During some research I noticed that some vendors (at least vendors using Insyde UEFI like Supermicro) set driver mode for GPIOs that read board id, board revision, bios reset jumper, nmi jumper and all sorts of other stuff. Most of them are *not* using any interrupt at all. Also GPOs used for signalling to BMC (POST done) are set to driver mode.
> >
> > This one is veeeery similiar to what Supermicro does on the X11SSM/X11SSH btw. I'm pretty sure they use the SkylakeOpenBoardPkg ;-)
> > https://github.com/maxpoliak/purley-platforms/blob/master/Platform/Intel/Pur...
>
> I think you can find schematics for this somewhere. I have them, at least.

Me too, but that doesn't help on X11*. I'm currently tracing the connections on the board, though, so I can adapt the X11SSM's gpio config.

However, have nothing to do with driver vs acpi mode ;)

[S] Change in coreboot[master]: soc/intel/common/gpio_defs: Remove incorrent GPO macro

[Martin L Roth (Code Review)]

Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/44471?usp=email )

Change subject: soc/intel/common/gpio_defs: Remove incorrent GPO macro
......................................................................

Abandoned

This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.