Change in coreboot[master]: soc/intel/common/gpio_defs: Add a macro for EDGE_BOTH trigger pad

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Kaiyen Chang (Code Review)

4 Dec 2020 4 Dec '20

4:42 a.m.

Hello Kaiyen Chang,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: soc/intel/common/gpio_defs: Add a macro for EDGE_BOTH trigger pad ......................................................................

soc/intel/common/gpio_defs: Add a macro for EDGE_BOTH trigger pad

Adds a new macro to configure the pad to EDGE_BOTH trigger mode.

BUG=b:174336541 TEST=emerge-dedede coreboot

Signed-off-by: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Change-Id: Icc805f5cfe45e5cc991fb0561f669907ac454a03 --- M src/soc/intel/common/block/include/intelblocks/gpio_defs.h 1 file changed, 6 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/48302/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 422720b..f082470 100644 --- a/src/soc/intel/common/block/include/intelblocks/gpio_defs.h +++ b/src/soc/intel/common/block/include/intelblocks/gpio_defs.h @@ -276,6 +276,12 @@ PAD_FUNC(GPIO) | PAD_RESET(rst) | PAD_BUF(TX_DISABLE), \ PAD_PULL(pull) | PAD_CFG_OWN_GPIO(DRIVER) | PAD_IOSSTATE(TxDRxE))

+#define PAD_CFG_GPI_GPIO_DRIVER_EDGE_BOTH(pad, pull, rst) \ + _PAD_CFG_STRUCT(pad, \ + PAD_FUNC(GPIO) | PAD_RESET(rst) | PAD_BUF(TX_DISABLE) | \ + PAD_CFG0_TRIG_EDGE_BOTH, \ + PAD_PULL(pull) | PAD_CFG_OWN_GPIO(DRIVER) | PAD_IOSSTATE(TxDRxE)) + #define PAD_CFG_GPIO_DRIVER_HI_Z(pad, pull, rst, iosstate, iosterm) \ _PAD_CFG_STRUCT(pad, \ PAD_FUNC(GPIO) | PAD_RESET(rst) | PAD_BUF(TX_RX_DISABLE), \

Attachments:

attachment.htm (text/html — 3.3 KB)

Show replies by date

Kaiyen Chang (Code Review)

4 Dec 4 Dec

4:53 a.m.

Kaiyen Chang has uploaded a new patch set (#2) to the change originally created by Kaiyen Chang. ( https://review.coreboot.org/c/coreboot/+/48302 )

Change subject: soc/intel/common/gpio_defs: Add a macro for EDGE_BOTH trigger pad ......................................................................

soc/intel/common/gpio_defs: Add a macro for EDGE_BOTH trigger pad

Adds a new macro to configure the pad to EDGE_BOTH trigger mode.

This macro is used in the pad configuration for Dedede boards. See CB:48303.

BUG=b:174336541 TEST=emerge-dedede coreboot

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/48302/2

Kaiyen Chang (Code Review)

5:21 a.m.

Hello build bot (Jenkins), Kaiyen Chang, Patrick Rudolph,

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

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

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

Change subject: soc/intel/common/gpio_defs: Add a macro for EDGE_BOTH trigger pad ......................................................................

soc/intel/common/gpio_defs: Add a macro for EDGE_BOTH trigger pad

Adds a new macro to configure the pad to EDGE_BOTH trigger mode.

This macro is used in the pad configuration for Dedede boards. See CB:48303.

BUG=b:174336541 TEST=emerge-dedede coreboot

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/48302/3

Furquan Shaikh (Code Review)

9:38 a.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48302 )

Change subject: soc/intel/common/gpio_defs: Add a macro for EDGE_BOTH trigger pad ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48302/3/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/gpio_defs.h:

https://review.coreboot.org/c/coreboot/+/48302/3/src/soc/intel/common/block/... PS3, Line 276: , I think the actual problem here is that PAD_CFG_GPI_GPIO_DRIVER isn't configuring trigger as PAD_TRIG(OFF).

The way GPIO interrupts are handled is: 1. Pad is configured as input in coreboot. 2. Pad IRQ information is passed in ACPI tables to kernel. 3. Kernel configures the required pad trigger.

This is the reason why `PAD_CFG_GPI_GPIO_DRIVER_EDGE_BOTH` doesn't exist in coreboot. Can you please try adding PAD_TRIG(OFF) here and check if that fixes the issue for you?

-- To view, visit https://review.coreboot.org/c/coreboot/+/48302 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc805f5cfe45e5cc991fb0561f669907ac454a03 Gerrit-Change-Number: 48302 Gerrit-PatchSet: 3 Gerrit-Owner: Kaiyen Chang kaiyen.chang@intel.com Gerrit-Reviewer: Henry Sun henrysun@google.com Gerrit-Reviewer: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Gerrit-Reviewer: Kane Chen kane.chen@intel.corp-partner.google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Fri, 04 Dec 2020 17:38:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Kaiyen Chang (Code Review)

3:12 p.m.

Kaiyen Chang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48302 )

Change subject: soc/intel/common/gpio_defs: Add a macro for EDGE_BOTH trigger pad ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48302/3/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/gpio_defs.h:

https://review.coreboot.org/c/coreboot/+/48302/3/src/soc/intel/common/block/... PS3, Line 276: ,

...

I think the actual problem here is that PAD_CFG_GPI_GPIO_DRIVER isn't configuring trigger as PAD_TRI […]

Thanks for your comment. I will try it. Your description for GPIO interrupts handling is correct, but I'm not sure if there are other GPIO settings rely on current setting (I mean didn't turn the TRIG off here) because maybe they (Depthcharge or Kernel) don't configure the interrupts correctly. I'm not sure if it will be better that we create another macro for GPI_GPIO setting with PAD_TRIG(OFF), instead of directly adding PAD_TRIG(OFF) here? What do you think?

-- To view, visit https://review.coreboot.org/c/coreboot/+/48302 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc805f5cfe45e5cc991fb0561f669907ac454a03 Gerrit-Change-Number: 48302 Gerrit-PatchSet: 3 Gerrit-Owner: Kaiyen Chang kaiyen.chang@intel.com Gerrit-Reviewer: Henry Sun henrysun@google.com Gerrit-Reviewer: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Gerrit-Reviewer: Kane Chen kane.chen@intel.corp-partner.google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Fri, 04 Dec 2020 23:12:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

3:16 p.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48302 )

Change subject: soc/intel/common/gpio_defs: Add a macro for EDGE_BOTH trigger pad ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48302/3/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/gpio_defs.h:

https://review.coreboot.org/c/coreboot/+/48302/3/src/soc/intel/common/block/... PS3, Line 276: ,

...

Thanks for your comment. I will try it. […]

I think it is just a miss. We should not rely on the trigger being configured in coreboot and just set it to off. Kernel is responsible for configuring the interrupts and it should set up the trigger correctly. I think it is okay to update the same macro to set PAD_TRIG(OFF) rather than creating a new macro (Reason is that more macros = more confusion and in this case the right thing to do is keep trigger off).

-- To view, visit https://review.coreboot.org/c/coreboot/+/48302 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc805f5cfe45e5cc991fb0561f669907ac454a03 Gerrit-Change-Number: 48302 Gerrit-PatchSet: 3 Gerrit-Owner: Kaiyen Chang kaiyen.chang@intel.com Gerrit-Reviewer: Henry Sun henrysun@google.com Gerrit-Reviewer: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Gerrit-Reviewer: Kane Chen kane.chen@intel.corp-partner.google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Fri, 04 Dec 2020 23:16:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Gerrit-MessageType: comment

Kaiyen Chang (Code Review)

3:48 p.m.

Kaiyen Chang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48302 )

Change subject: soc/intel/common/gpio_defs: Add a macro for EDGE_BOTH trigger pad ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48302/3/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/gpio_defs.h:

https://review.coreboot.org/c/coreboot/+/48302/3/src/soc/intel/common/block/... PS3, Line 276: ,

...

I think it is just a miss. […]

Got it!

-- To view, visit https://review.coreboot.org/c/coreboot/+/48302 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc805f5cfe45e5cc991fb0561f669907ac454a03 Gerrit-Change-Number: 48302 Gerrit-PatchSet: 3 Gerrit-Owner: Kaiyen Chang kaiyen.chang@intel.com Gerrit-Reviewer: Henry Sun henrysun@google.com Gerrit-Reviewer: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Gerrit-Reviewer: Kane Chen kane.chen@intel.corp-partner.google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Fri, 04 Dec 2020 23:48:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Gerrit-MessageType: comment

Kaiyen Chang (Code Review)

9:12 p.m.

New subject: Change in coreboot[master]: soc/intel/common/gpio_defs: Add PAD_TRIG(OFF) in PAD_CFG_GPI_GPIO_DRIVER

Kaiyen Chang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48302 )

Change subject: soc/intel/common/gpio_defs: Add PAD_TRIG(OFF) in PAD_CFG_GPI_GPIO_DRIVER ......................................................................

Set Ready For Review

-- To view, visit https://review.coreboot.org/c/coreboot/+/48302 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc805f5cfe45e5cc991fb0561f669907ac454a03 Gerrit-Change-Number: 48302 Gerrit-PatchSet: 7 Gerrit-Owner: Kaiyen Chang kaiyen.chang@intel.com Gerrit-Reviewer: Henry Sun henrysun@google.com Gerrit-Reviewer: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Gerrit-Reviewer: Kane Chen kane.chen@intel.corp-partner.google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Sat, 05 Dec 2020 05:12:54 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Kaiyen Chang (Code Review)

9:21 p.m.

New subject: Change in coreboot[master]: soc/intel/common/gpio_defs: Add PAD_TRIG(OFF) in PAD_CFG_GPI_GPIO_DRIVER

Kaiyen Chang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48302 )

Change subject: soc/intel/common/gpio_defs: Add PAD_TRIG(OFF) in PAD_CFG_GPI_GPIO_DRIVER ......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/48302/3/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/gpio_defs.h:

https://review.coreboot.org/c/coreboot/+/48302/3/src/soc/intel/common/block/... PS3, Line 276: ,

...

Got it!

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/48302 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc805f5cfe45e5cc991fb0561f669907ac454a03 Gerrit-Change-Number: 48302 Gerrit-PatchSet: 7 Gerrit-Owner: Kaiyen Chang kaiyen.chang@intel.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Henry Sun henrysun@google.com Gerrit-Reviewer: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Gerrit-Reviewer: Kane Chen kane.chen@intel.corp-partner.google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Sat, 05 Dec 2020 05:21:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

9:40 p.m.

New subject: Change in coreboot[master]: soc/intel/common/gpio_defs: Add PAD_TRIG(OFF) in PAD_CFG_GPI_GPIO_DRIVER

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48302 )

Change subject: soc/intel/common/gpio_defs: Add PAD_TRIG(OFF) in PAD_CFG_GPI_GPIO_DRIVER ......................................................................

Patch Set 7: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/48302 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc805f5cfe45e5cc991fb0561f669907ac454a03 Gerrit-Change-Number: 48302 Gerrit-PatchSet: 7 Gerrit-Owner: Kaiyen Chang kaiyen.chang@intel.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Henry Sun henrysun@google.com Gerrit-Reviewer: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Gerrit-Reviewer: Kane Chen kane.chen@intel.corp-partner.google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Sat, 05 Dec 2020 05:40:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

EricR Lai (Code Review)

6 Dec 6 Dec

9:43 p.m.

New subject: Change in coreboot[master]: soc/intel/common/gpio_defs: Add PAD_TRIG(OFF) in PAD_CFG_GPI_GPIO_DRIVER

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48302 )

Change subject: soc/intel/common/gpio_defs: Add PAD_TRIG(OFF) in PAD_CFG_GPI_GPIO_DRIVER ......................................................................

Patch Set 7: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/48302 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc805f5cfe45e5cc991fb0561f669907ac454a03 Gerrit-Change-Number: 48302 Gerrit-PatchSet: 7 Gerrit-Owner: Kaiyen Chang kaiyen.chang@intel.com Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Henry Sun henrysun@google.com Gerrit-Reviewer: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Gerrit-Reviewer: Kane Chen kane.chen@intel.corp-partner.google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Mon, 07 Dec 2020 05:43:15 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Georgi (Code Review)

8 Dec 8 Dec

8:56 a.m.

New subject: Change in coreboot[master]: soc/intel/common/gpio_defs: Add PAD_TRIG(OFF) in PAD_CFG_GPI_GPIO_DRIVER

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48302 )

Change subject: soc/intel/common/gpio_defs: Add PAD_TRIG(OFF) in PAD_CFG_GPI_GPIO_DRIVER ......................................................................

soc/intel/common/gpio_defs: Add PAD_TRIG(OFF) in PAD_CFG_GPI_GPIO_DRIVER

Probabilistic interrupt storm is observed while kernel is configuring the GPIO for SD card CD pin. The root cause is that the macro PAD_CFG_GPI_GPIO_DRIVER isn't configuring trigger as PAD_TRIG(OFF).

The way GPIO interrupts are handled is: 1. Pad is configured as input in coreboot. 2. Pad IRQ information is passed in ACPI tables to kernel. 3. Kernel configures the required pad trigger.

Therefore, PAD_TRIG(OFF) should be added in PAD_CFG_GPI_GPIO_DRIVER to turn off the trigger while pad is configured as input in coreboot and then let kernel to configure the required pad trigger.

BUG=b:174336541 TEST=Run 1500 reboot iterations successfully without any interrupts storm.

Signed-off-by: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Change-Id: Icc805f5cfe45e5cc991fb0561f669907ac454a03 Reviewed-on: https://review.coreboot.org/c/coreboot/+/48302 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: EricR Lai ericr_lai@compal.corp-partner.google.com --- M src/soc/intel/common/block/include/intelblocks/gpio_defs.h 1 file changed, 2 insertions(+), 1 deletion(-)

Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved EricR Lai: Looks good to me, approved

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 422720b..2d6f62a 100644 --- a/src/soc/intel/common/block/include/intelblocks/gpio_defs.h +++ b/src/soc/intel/common/block/include/intelblocks/gpio_defs.h @@ -273,7 +273,8 @@

#define PAD_CFG_GPIO_DRIVER_HI_Z(pad, pull, rst, iosstate, iosterm) \

-- To view, visit https://review.coreboot.org/c/coreboot/+/48302 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc805f5cfe45e5cc991fb0561f669907ac454a03 Gerrit-Change-Number: 48302 Gerrit-PatchSet: 8 Gerrit-Owner: Kaiyen Chang kaiyen.chang@intel.com Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Henry Sun henrysun@google.com Gerrit-Reviewer: Kaiyen Chang kaiyen.chang@intel.corp-partner.google.com Gerrit-Reviewer: Kane Chen kane.chen@intel.corp-partner.google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: merged

1440

days inactive

1444

days old

coreboot-gerrit@coreboot.org

11 comments

4 participants

tags (0)

participants (4)

EricR Lai (Code Review)
Furquan Shaikh (Code Review)
Kaiyen Chang (Code Review)
Patrick Georgi (Code Review)