Change in coreboot[master]: soc/intel/common/gpio_defs: Add macros for bidirectional pad

[Maxim Polyakov (Code Review)]

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

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

soc/intel/common/gpio_defs: Add macros for bidirectional pad

Adds new macros to configure the pad in bidirectional mode when both
(Tx/Rx) buffers are enabled in the configuration register DW0.

Change-Id: I7b65f4da7616f2eefcd33a728d4d3ae5a79b014e
Signed-off-by: Maxim Polyakov 
---
M src/soc/intel/common/block/include/intelblocks/gpio_defs.h
1 file changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/42914/1

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 0668131..9bfc5dd 100644
--- a/src/soc/intel/common/block/include/intelblocks/gpio_defs.h
+++ b/src/soc/intel/common/block/include/intelblocks/gpio_defs.h
@@ -301,6 +301,20 @@
 #define PAD_CFG_GPI_INT(pad, pull, rst, trig)	\
    	PAD_CFG_GPI_TRIG_OWN(pad, pull, rst, trig, DRIVER)

+/* Bidirectional GPIO port when both RX and TX buffer is enabled */
+#define PAD_CFG_GPIO_BIDIRECT_IOS(pad, val, pull, rst, trig, iosstate, iosterm, own) \
+	_PAD_CFG_STRUCT(pad,						\
+		PAD_FUNC(GPIO) | PAD_RESET(rst) | PAD_TRIG(trig) |	\
+		PAD_BUF(NO_DISABLE) | val,				\
+		PAD_PULL(pull) | PAD_CFG_OWN_GPIO(own) |		\
+		PAD_IOSSTATE(iosstate) | PAD_IOSTERM(iosterm))
+
+#define PAD_CFG_GPIO_BIDIRECT(pad, val, pull, rst, trig, own)		\
+	_PAD_CFG_STRUCT(pad,						\
+		PAD_FUNC(GPIO) | PAD_RESET(rst) | PAD_TRIG(trig) |	\
+		PAD_BUF(NO_DISABLE) | val,				\
+		PAD_PULL(pull) | PAD_CFG_OWN_GPIO(own))
+
 /*
  * No Connect configuration for unused pad.
  * Both TX and RX are disabled. RX disabling is done to avoid unnecessary

[Maxim Polyakov (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42914

to look at the new patch set (#3).

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

soc/intel/common/gpio_defs: Add macros for bidirectional pad

Adds new macros to configure the pad in bidirectional mode when both
(Tx/Rx) buffers are enabled in the configuration register DW0.

Change-Id: I7b65f4da7616f2eefcd33a728d4d3ae5a79b014e
Signed-off-by: Maxim Polyakov 
---
M src/soc/intel/common/block/include/intelblocks/gpio_defs.h
1 file changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/42914/3

[Maxim Polyakov (Code Review)]

Hello Lance Zhao, build bot (Jenkins), Nico Huber, Angel Pons, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42914

to look at the new patch set (#10).

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

soc/intel/common/gpio_defs: Add macros for bidirectional pad

Adds new macros to configure the pad in bidirectional mode when both
(Tx/Rx) buffers are enabled in the configuration register DW0.

This macro is used in the pad configuration for some boards:
[1] CB:43456 - mb/intel/cedarisland_crb;
[2] CB:39133 - mb/kontron/mal10;
[3] CB:43410 - mb/51nb/x210;
[4] CB:43411 - mb/razer/blade_stealth_kbl.

Change-Id: I7b65f4da7616f2eefcd33a728d4d3ae5a79b014e
Signed-off-by: Maxim Polyakov 
---
M src/soc/intel/common/block/include/intelblocks/gpio_defs.h
1 file changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/42914/10

[Maxim Polyakov (Code Review)]

Hello Lance Zhao, build bot (Jenkins), Nico Huber, Angel Pons, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42914

to look at the new patch set (#11).

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

soc/intel/common/gpio_defs: Add macros for bidirectional pad

Adds new macros to configure the pad in bidirectional mode when both
(Tx/Rx) buffers are enabled in the configuration register DW0. This
corresponds to FSP's < GpioDirInOut > parameter for port direction
(for example see config for SOUTH_GROUP0_DFX_SPARE2 pad in
src/mainboard/intel/harcuvar/gpio.h).

This macro is used in the pad configuration for some boards:
 CB:43456 - mb/intel/cedarisland_crb;
 CB:39133 - mb/kontron/mal10;
 CB:43410 - mb/51nb/x210;
 CB:43411 - mb/razer/blade_stealth_kbl.

Change-Id: I7b65f4da7616f2eefcd33a728d4d3ae5a79b014e
Signed-off-by: Maxim Polyakov 
---
M src/soc/intel/common/block/include/intelblocks/gpio_defs.h
1 file changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/42914/11

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

Patch Set 11: Code-Review+1

[Maxim Polyakov (Code Review)]

Hello Lance Zhao, build bot (Jenkins), Nico Huber, Angel Pons, Michael Niewöhner, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42914

to look at the new patch set (#13).

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

soc/intel/common/gpio_defs: Add macros for bidirectional pad

Adds new macros to configure the pad in bidirectional mode when both
(Tx/Rx) buffers are enabled in the configuration register DW0. This
corresponds to FSP's < GpioDirInOut > parameter for port direction
(for example see config for SOUTH_GROUP0_DFX_SPARE2 pad in
src/mainboard/intel/harcuvar/gpio.h).

This macro is used in the pad configuration for some boards:
 CB:43456 - mb/intel/cedarisland_crb;
 CB:39133 - mb/kontron/mal10;
 CB:43410 - mb/51nb/x210;
 CB:43411 - mb/razer/blade_stealth_kbl.

Change-Id: I7b65f4da7616f2eefcd33a728d4d3ae5a79b014e
Signed-off-by: Maxim Polyakov 
---
M src/soc/intel/common/block/include/intelblocks/gpio_defs.h
1 file changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/42914/13

[Maxim Polyakov (Code Review)]

Hello Lance Zhao, build bot (Jenkins), Nico Huber, Angel Pons, Michael Niewöhner, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42914

to look at the new patch set (#14).

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

soc/intel/common/gpio_defs: Add macros for bidirectional pad

Adds new macros to configure the pad in bidirectional mode when both
(Tx/Rx) buffers are enabled in the configuration register DW0. This
corresponds to FSP's < GpioDirInOut > parameter for port direction
(for example see config for SOUTH_GROUP0_DFX_SPARE2 pad in
src/mainboard/intel/harcuvar/gpio.h).

This macro is used in the pad configuration for some boards:
 CB:43456 - mb/intel/cedarisland_crb;
 CB:39133 - mb/kontron/mal10;
 CB:43410 - mb/51nb/x210;
 CB:43411 - mb/razer/blade_stealth_kbl.

Change-Id: I7b65f4da7616f2eefcd33a728d4d3ae5a79b014e
Signed-off-by: Maxim Polyakov 
---
M src/soc/intel/common/block/include/intelblocks/gpio_defs.h
1 file changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/42914/14

[Nico Huber (Code Review)]

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

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

Patch Set 14:

What is this useful for? All the mentioned boards have probably
copy-pasta entries that nobody reasoned about. Adding a macro
might make it look like the entries are intentional and yet
something more to mantain... I don't think we should add any
entry that doesn't make sense in the context of the coreboot
ports.

[Angel Pons (Code Review)]

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

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

Patch Set 14:
> Patch Set 14:
>
> What is this useful for? All the mentioned boards have probably
> copy-pasta entries that nobody reasoned about. Adding a macro
> might make it look like the entries are intentional and yet
> something more to mantain... I don't think we should add any
> entry that doesn't make sense in the context of the coreboot
> ports.

Agreed.

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

Patch Set 14: -Code-Review

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

Patch Set 14:
> Patch Set 14:
> > Patch Set 14:
> >
> > What is this useful for? All the mentioned boards have probably
> > copy-pasta entries that nobody reasoned about. Adding a macro
> > might make it look like the entries are intentional and yet
> > something more to mantain... I don't think we should add any
> > entry that doesn't make sense in the context of the coreboot
> > ports.
>
> Agreed.

ack

[Maxim Polyakov (Code Review)]

Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42914 )

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

Patch Set 15:

This patch was published for the following reasons:

1. Reply to the review:
https://review.coreboot.org/c/coreboot/+/39133/20/src/mainboard/kontron/mal1...

2. I have been working on converting raw values to macros in GPIO config and
this config for some boards contains pads in which both the Rx and Tx buffers
are enabled.

Please can you help me with this macro?
I'm not sure about this port configuration and I didn’t find any information
about it, except for a few pads from the gpio.h files (see the commit message).
Do you know anything about this? Is this configuration correct for a pad in the
Intel GPIO controller? If not, what should I use instead? GPI or GPO?

Thanks.

Alternative way:

1) add this macro to the local gpio.h for each of the boards;
2) always use bitfield macros and don't convert this to PAD_CFG_*() macro:

_PAD_CFG_STRUCT(GPP_I4,
    	PAD_FUNC(GPIO) | PAD_RESET(RSMRST) |
    	PAD_CFG0_TRIG_OFF | PAD_CFG0_RX_POL_NONE | PAD_BUF(NO_DISABLE),
    	PAD_CFG_OWN_GPIO(ACPI) | PAD_PULL(NONE)),

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

Patch Set 15:
> Patch Set 15:
>
> This patch was published for the following reasons:
> Reply to the review:
>
> https://review.coreboot.org/c/coreboot/+/39133/20/src/mainboard/kontron/mal1...
> I have been working on converting raw values to macros in GPIO config and
>
> this config for some boards contains pads in which both the Rx and Tx buffers
> are enabled.
>
> Please can you help me with this macro?
> I'm not sure about this port configuration and I didn’t find any information
> about it, except for a few pads from the gpio.h files (see the commit message).
> Do you know anything about this? Is this configuration correct for a pad in the
> Intel GPIO controller? If not, what should I use instead? GPI or GPO?
>
> Thanks.
>
> Alternative way:
> add this macro to the local gpio.h for each of the boards;
> always use bitfield macros and don't convert this to PAD_CFG_*() macro:
>
> _PAD_CFG_STRUCT(GPP_I4,
>    	PAD_FUNC(GPIO) | PAD_RESET(RSMRST) |
>    	PAD_CFG0_TRIG_OFF | PAD_CFG0_RX_POL_NONE | PAD_BUF(NO_DISABLE),
>    	PAD_CFG_OWN_GPIO(ACPI) | PAD_PULL(NONE)),

Do you know what the bidir pad is used for on Kontron mal10? If not, Nico or Felix maybe?

[S] Change in coreboot[main]: soc/intel/common/gpio_defs: Add macros for bidirectional pad

[Matt DeVillier (Code Review)]

Attention is currently required from: Maxim.

Matt DeVillier has posted comments on this change by Maxim. ( https://review.coreboot.org/c/coreboot/+/42914?usp=email )

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

Patch Set 15:

(1 comment)

Patchset:

PS15: 
looks like this needs a manual rebase @[e-mail]

[S] Change in coreboot[main]: soc/intel/common/gpio_defs: Add macros for bidirectional pad

[Maxim (Code Review)]

Attention is currently required from: Maxim.

Hello Aaron Durbin, Andrey Petrov, Angel Pons, Felix Held, Felix Singer, Furquan Shaikh, Lance Zhao, Michael Niewöhner, Nico Huber, Patrick Rudolph, Subrata Banik, Werner Zeh, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42914?usp=email

to look at the new patch set (#16).

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

soc/intel/common/gpio_defs: Add macros for bidirectional pad

Adds new macros to configure the pad in bidirectional mode when both
(Tx/Rx) buffers are enabled in the configuration register DW0. This
corresponds to FSP's < GpioDirInOut > parameter for port direction
(for example see config for SOUTH_GROUP0_DFX_SPARE2 pad in
src/mainboard/intel/harcuvar/gpio.h).

This macro is used in the pad configuration for some boards:
 CB:43456 - mb/intel/cedarisland_crb;
 CB:39133 - mb/kontron/mal10;
 CB:43410 - mb/51nb/x210;
 CB:43411 - mb/razer/blade_stealth_kbl.

Change-Id: I7b65f4da7616f2eefcd33a728d4d3ae5a79b014e
Signed-off-by: Maxim Polyakov 
---
M src/soc/intel/common/block/include/intelblocks/gpio_defs.h
1 file changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/42914/16

[S] Change in coreboot[main]: soc/intel/common/gpio_defs: Add macros for bidirectional pad

[Nico Huber (Code Review)]

Attention is currently required from: Maxim.

Nico Huber has posted comments on this change by Maxim. ( https://review.coreboot.org/c/coreboot/+/42914?usp=email )

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

Patch Set 16:

(1 comment)

Patchset:

PS16: 
I still believe we shouldn't use inexplicable vendor values as a
role model for coreboot. If I don't miss anything, with these
settings one could read back the exact value written. That's all
isn't it? If we don't have any software that does that (why would
it?), why make our GPIO configs less transparent?

[S] Change in coreboot[main]: soc/intel/common/gpio_defs: Add macros for bidirectional pad

[Maxim (Code Review)]

Attention is currently required from: Matt DeVillier, Nico Huber.

Maxim has posted comments on this change by Maxim. ( https://review.coreboot.org/c/coreboot/+/42914?usp=email )

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

Patch Set 16:

(2 comments)

Patchset:

PS15:
> looks like this needs a manual rebase @max.senia.poliak@gmail. […]

Done

Patchset:

PS16:
> I still believe we shouldn't use inexplicable vendor values as a […]

In my understanding, this is an alternative to < GpioDirInOut > config from fsp or reference bios. But I don't insist on it and the decision is yours =)

[S] Change in coreboot[main]: soc/intel/common/gpio_defs: Add macros for bidirectional pad

[Angel Pons (Code Review)]

Attention is currently required from: Matt DeVillier, Maxim, Nico Huber.

Angel Pons has posted comments on this change by Maxim. ( https://review.coreboot.org/c/coreboot/+/42914?usp=email )

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

Patch Set 16: Code-Review+1

(1 comment)

Patchset:

PS16:
> In my understanding, this is an alternative to < GpioDirInOut > config from fsp or reference bios. […]

Now I'm curious. Is there any known use-case anywhere for this `GpioDirInOut` setting? Ideally something with schematics.

[S] Change in coreboot[main]: soc/intel/common/gpio_defs: Add macros for bidirectional pad

[Maxim (Code Review)]

Attention is currently required from: Angel Pons, Matt DeVillier, Nico Huber.

Maxim has posted comments on this change by Maxim. ( https://review.coreboot.org/c/coreboot/+/42914?usp=email )

Change subject: soc/intel/common/gpio_defs: Add macros for bidirectional pad
......................................................................

Patch Set 16:

(1 comment)

Patchset:

PS16:
> Now I'm curious. […]

It seems to me that this is used for testing: the circuit is configured to allow us to read the value from the GPIO output buffer and check its value (for example, it does not match the set value if there is a short circuit). You'd better ask Intel about this. It depends on the internal circuit.