Change in coreboot[master]: drivers/i2c/dw: Check for TX_ABORT in transfer

List overview All Threads
Download

newer

older

Change in coreboot[master]: lp4x:...

Change in coreboot[master]:...

Kangheui Won (Code Review)

9 Nov 2020 9 Nov '20

7:57 a.m.

Kangheui Won has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47360 )

Change subject: drivers/i2c/dw: Check for TX_ABORT in transfer ......................................................................

drivers/i2c/dw: Check for TX_ABORT in transfer

When the host sends data in i2c bus, device might not send ACK. It means that data is not processed on the device side, but for now we don't check for that condition thus wait for the response which will not come.

Designware i2c detect such situation and set TX_ABORT bit. Checking for the bit will enable other layers to immediately retry rather than wait-timeout-retry cycle.

BUG=b:168838505 BRANCH=zork TEST=test on zork devices, now we see "Tx abort detected" instead of I2C timeout for tpm initializtion.

Change-Id: Ib0163fbce55ccc99f677dbb096f67a58d2ef2bda --- M src/drivers/i2c/designware/dw_i2c.c 1 file changed, 10 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/47360/1

diff --git a/src/drivers/i2c/designware/dw_i2c.c b/src/drivers/i2c/designware/dw_i2c.c index 3181e7a..5601bcf 100644 --- a/src/drivers/i2c/designware/dw_i2c.c +++ b/src/drivers/i2c/designware/dw_i2c.c @@ -419,6 +419,14 @@ /* Read to clear INTR_STAT_STOP_DET */ read32(&regs->clear_stop_det_intr);

+ /* Check TX abort */ + if (read32(&regs->raw_intr_stat) & INTR_STAT_TX_ABORT) { + printk(BIOS_ERR, "I2C TX abort detected\n"); + /* clear INTR_STAT_TX_ABORT */ + read32(&regs->clear_tx_abrt_intr); + goto out; + } + /* Wait for the bus to go idle */ if (dw_i2c_wait_for_bus_idle(regs)) { printk(BIOS_ERR, "I2C timeout waiting for bus %u idle\n", bus); @@ -747,8 +755,8 @@ write32(&regs->rx_thresh, 0); write32(&regs->tx_thresh, 0);

- /* Enable stop detection interrupt */ - write32(&regs->intr_mask, INTR_STAT_STOP_DET); + /* Enable stop detection and TX abort interrupt */ + write32(&regs->intr_mask, INTR_STAT_STOP_DET | INTR_STAT_TX_ABORT);

printk(BIOS_INFO, "DW I2C bus %u at %p (%u KHz)\n", bus, regs, speed / KHz);

Attachments:

attachment.htm (text/html — 4.2 KB)

Show replies by date

Kangheui Won (Code Review)

9 Nov 9 Nov

9:35 p.m.

Hello build bot (Jenkins),

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

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

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

Change subject: drivers/i2c/dw: Check for TX_ABORT in transfer ......................................................................

drivers/i2c/dw: Check for TX_ABORT in transfer

Designware i2c detect such situation and set TX_ABORT bit. Checking for the bit will enable other layers to immediately retry rather than wait-timeout-retry cycle.

BUG=b:168838505 BRANCH=zork TEST=test on zork devices, now we see "Tx abort detected" instead of I2C timeout for tpm initializtion.

Change-Id: Ib0163fbce55ccc99f677dbb096f67a58d2ef2bda Signed-off-by: Kangheui Won khwon@chromium.org --- M src/drivers/i2c/designware/dw_i2c.c 1 file changed, 10 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/47360/2

Furquan Shaikh (Code Review)

10 Nov 10 Nov

5:54 a.m.

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

Change subject: drivers/i2c/dw: Check for TX_ABORT in transfer ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47360/2/src/drivers/i2c/designware/... File src/drivers/i2c/designware/dw_i2c.c:

https://review.coreboot.org/c/coreboot/+/47360/2/src/drivers/i2c/designware/... PS2, Line 424: \n Print the cause of abort here using &regs->tx_abort_source?

-- To view, visit https://review.coreboot.org/c/coreboot/+/47360 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib0163fbce55ccc99f677dbb096f67a58d2ef2bda Gerrit-Change-Number: 47360 Gerrit-PatchSet: 2 Gerrit-Owner: Kangheui Won khwon@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Tue, 10 Nov 2020 05:54:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Kangheui Won (Code Review)

10:51 a.m.

Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47360 )

Change subject: drivers/i2c/dw: Check for TX_ABORT in transfer ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47360/2/src/drivers/i2c/designware/... File src/drivers/i2c/designware/dw_i2c.c:

https://review.coreboot.org/c/coreboot/+/47360/2/src/drivers/i2c/designware/... PS2, Line 424: \n

...

Print the cause of abort here using &regs->tx_abort_source?

Would it be sufficient to just dump out the content of tx_abort_source?

-- To view, visit https://review.coreboot.org/c/coreboot/+/47360 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib0163fbce55ccc99f677dbb096f67a58d2ef2bda Gerrit-Change-Number: 47360 Gerrit-PatchSet: 2 Gerrit-Owner: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Tue, 10 Nov 2020 10:51:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

1:55 p.m.

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

Change subject: drivers/i2c/dw: Check for TX_ABORT in transfer ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47360/2/src/drivers/i2c/designware/... File src/drivers/i2c/designware/dw_i2c.c:

https://review.coreboot.org/c/coreboot/+/47360/2/src/drivers/i2c/designware/... PS2, Line 424: \n

...

Would it be sufficient to just dump out the content of tx_abort_source?

I think dumping the raw content should be fine. Just helpful to have that extra bit of information.

-- To view, visit https://review.coreboot.org/c/coreboot/+/47360 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib0163fbce55ccc99f677dbb096f67a58d2ef2bda Gerrit-Change-Number: 47360 Gerrit-PatchSet: 2 Gerrit-Owner: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 10 Nov 2020 13:55:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Kangheui Won khwon@chromium.org Gerrit-MessageType: comment

Kangheui Won (Code Review)

11 Nov 11 Nov

5:52 a.m.

Hello build bot (Jenkins), Martin Roth,

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

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

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

Change subject: drivers/i2c/dw: Check for TX_ABORT in transfer ......................................................................

drivers/i2c/dw: Check for TX_ABORT in transfer

Designware i2c detect such situation and set TX_ABORT bit. Checking for the bit will enable other layers to immediately retry rather than wait-timeout-retry cycle.

BUG=b:168838505 BRANCH=zork TEST=test on zork devices, now we see "Tx abort detected" instead of I2C timeout for tpm initializtion.

Change-Id: Ib0163fbce55ccc99f677dbb096f67a58d2ef2bda Signed-off-by: Kangheui Won khwon@chromium.org --- M src/drivers/i2c/designware/dw_i2c.c 1 file changed, 11 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/47360/3

-- To view, visit https://review.coreboot.org/c/coreboot/+/47360 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib0163fbce55ccc99f677dbb096f67a58d2ef2bda Gerrit-Change-Number: 47360 Gerrit-PatchSet: 3 Gerrit-Owner: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Furquan Shaikh (Code Review)

8:46 p.m.

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

Change subject: drivers/i2c/dw: Check for TX_ABORT in transfer ......................................................................

Patch Set 3: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/47360 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib0163fbce55ccc99f677dbb096f67a58d2ef2bda Gerrit-Change-Number: 47360 Gerrit-PatchSet: 3 Gerrit-Owner: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 11 Nov 2020 20:46:30 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Kangheui Won (Code Review)

10:30 p.m.

Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47360 )

Change subject: drivers/i2c/dw: Check for TX_ABORT in transfer ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47360/2/src/drivers/i2c/designware/... File src/drivers/i2c/designware/dw_i2c.c:

https://review.coreboot.org/c/coreboot/+/47360/2/src/drivers/i2c/designware/... PS2, Line 424: \n

...

I think dumping the raw content should be fine. Just helpful to have that extra bit of information.

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/47360 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib0163fbce55ccc99f677dbb096f67a58d2ef2bda Gerrit-Change-Number: 47360 Gerrit-PatchSet: 3 Gerrit-Owner: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 11 Nov 2020 22:30:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Kangheui Won khwon@chromium.org Gerrit-MessageType: comment

Patrick Georgi (Code Review)

16 Nov 16 Nov

11 a.m.

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

Change subject: drivers/i2c/dw: Check for TX_ABORT in transfer ......................................................................

drivers/i2c/dw: Check for TX_ABORT in transfer

Designware i2c detect such situation and set TX_ABORT bit. Checking for the bit will enable other layers to immediately retry rather than wait-timeout-retry cycle.

BUG=b:168838505 BRANCH=zork TEST=test on zork devices, now we see "Tx abort detected" instead of I2C timeout for tpm initializtion.

Change-Id: Ib0163fbce55ccc99f677dbb096f67a58d2ef2bda Signed-off-by: Kangheui Won khwon@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/47360 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/drivers/i2c/designware/dw_i2c.c 1 file changed, 11 insertions(+), 2 deletions(-)

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

diff --git a/src/drivers/i2c/designware/dw_i2c.c b/src/drivers/i2c/designware/dw_i2c.c index 3181e7a..c0212c3 100644 --- a/src/drivers/i2c/designware/dw_i2c.c +++ b/src/drivers/i2c/designware/dw_i2c.c @@ -419,6 +419,15 @@ /* Read to clear INTR_STAT_STOP_DET */ read32(&regs->clear_stop_det_intr);

+ /* Check TX abort */ + if (read32(&regs->raw_intr_stat) & INTR_STAT_TX_ABORT) { + printk(BIOS_ERR, "I2C TX abort detected (%08x)\n", + read32(&regs->tx_abort_source)); + /* clear INTR_STAT_TX_ABORT */ + read32(&regs->clear_tx_abrt_intr); + goto out; + } + /* Wait for the bus to go idle */ if (dw_i2c_wait_for_bus_idle(regs)) { printk(BIOS_ERR, "I2C timeout waiting for bus %u idle\n", bus); @@ -747,8 +756,8 @@ write32(&regs->rx_thresh, 0); write32(&regs->tx_thresh, 0);

printk(BIOS_INFO, "DW I2C bus %u at %p (%u KHz)\n", bus, regs, speed / KHz);

-- To view, visit https://review.coreboot.org/c/coreboot/+/47360 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib0163fbce55ccc99f677dbb096f67a58d2ef2bda Gerrit-Change-Number: 47360 Gerrit-PatchSet: 4 Gerrit-Owner: Kangheui Won khwon@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1514

days inactive

1521

days old

coreboot-gerrit@coreboot.org

8 comments

3 participants

tags (0)

participants (3)

Furquan Shaikh (Code Review)
Kangheui Won (Code Review)
Patrick Georgi (Code Review)