Change in coreboot[master]: mb/google/hatch: Fine-tune Kohaku I2C CLK frequency

[Philip Chen (Code Review)]

Philip Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34559 )

Change subject: mb/google/hatch: Fine-tune Kohaku I2C CLK frequency
......................................................................

mb/google/hatch: Fine-tune Kohaku I2C CLK frequency

Add rise time / fall time to I2C config in device tree
to ensure I2C CLK runs accurately at I2C_SPEED_FAST (400 kHz).

BUG=b:138258384
BRANCH=none
TEST=probe I2C0/I2C2/I2C3 SCL on Kohaku board,
verify all of them run at 395-399 kHz.

Change-Id: Id98079e717f0db3fdcb88f85e45693925d11d7fd
Signed-off-by: Philip Chen 
---
M src/mainboard/google/hatch/variants/kohaku/overridetree.cb
1 file changed, 6 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/34559/1

diff --git a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
index e463b8b..6fdc0a0 100644
--- a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
+++ b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
@@ -29,15 +29,21 @@
    register "common_soc_config" = "{
    	.i2c[0] = {
    		.speed = I2C_SPEED_FAST,
+			.rise_time_ns = 135,
+			.fall_time_ns = 45,
    	},
    	.i2c[1] = {
    		.speed = I2C_SPEED_FAST,
    	},
    	.i2c[2] = {
    		.speed = I2C_SPEED_FAST,
+			.rise_time_ns = 95,
+			.fall_time_ns = 55,
    	},
    	.i2c[4] = {
    		.speed = I2C_SPEED_FAST,
+			.rise_time_ns = 100,
+			.fall_time_ns = 20,
    	},
    	.gspi[0] = {
    	       .speed_mhz = 1,

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34559 )

Change subject: mb/google/hatch: Fine-tune Kohaku I2C CLK frequency
......................................................................

Patch Set 1:

LGTM, but some reason you didn't look at I2C1?

[Paul Fagerburg (Code Review)]

Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34559 )

Change subject: mb/google/hatch: Fine-tune Kohaku I2C CLK frequency
......................................................................

Patch Set 1: Code-Review+1

[Philip Chen (Code Review)]

Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34559 )

Change subject: mb/google/hatch: Fine-tune Kohaku I2C CLK frequency
......................................................................

Patch Set 1:
> Patch Set 1:
>
> LGTM, but some reason you didn't look at I2C1?

There is some HW error on touchscreen cable, which goes to I2C1.
So even the touchscreen device is not added into the device tree yet.

I didn't look at I2C CLK on I2C1, but I assume the rise time / fall time might change after we use the new cable in the next build?
So let's leave the tuning work for I2C1 to later time.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34559 )

Change subject: mb/google/hatch: Fine-tune Kohaku I2C CLK frequency
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/c/coreboot/+/34559/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34559/1//COMMIT_MSG@9 
PS1, Line 9: Add rise time / fall time to I2C config in device tree
Where did you get the timings from?

https://review.coreboot.org/c/coreboot/+/34559/1//COMMIT_MSG@10 
PS1, Line 10: to ensure I2C CLK runs accurately at I2C_SPEED_FAST (400 kHz).
Please use the full text width.

https://review.coreboot.org/c/coreboot/+/34559/1//COMMIT_MSG@15 
PS1, Line 15: verify all of them run at 395-399 kHz.
Ditto.

[Philip Chen (Code Review)]

Hello Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: mb/google/hatch: Fine-tune Kohaku I2C CLK frequency
......................................................................

mb/google/hatch: Fine-tune Kohaku I2C CLK frequency

Add rise time / fall time to I2C config in device tree to ensure I2C
CLK runs accurately at I2C_SPEED_FAST (400 kHz).

BUG=b:138258384
BRANCH=none
TEST=probe I2C0/I2C2/I2C3 SCL on Kohaku board, verify all of them run
at 395-399 kHz.

Change-Id: Id98079e717f0db3fdcb88f85e45693925d11d7fd
Signed-off-by: Philip Chen 
---
M src/mainboard/google/hatch/variants/kohaku/overridetree.cb
1 file changed, 6 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/34559/2

[Philip Chen (Code Review)]

Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34559 )

Change subject: mb/google/hatch: Fine-tune Kohaku I2C CLK frequency
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/34559/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34559/1//COMMIT_MSG@9 
PS1, Line 9: Add rise time / fall time to I2C config in device tree
> Where did you get the timings from?

probing + minor twiddle

https://review.coreboot.org/c/coreboot/+/34559/1//COMMIT_MSG@10 
PS1, Line 10: to ensure I2C CLK runs accurately at I2C_SPEED_FAST (400 kHz).
> Please use the full text width.

Done

https://review.coreboot.org/c/coreboot/+/34559/1//COMMIT_MSG@15 
PS1, Line 15: verify all of them run at 395-399 kHz.
> Ditto.

Done

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34559 )

Change subject: mb/google/hatch: Fine-tune Kohaku I2C CLK frequency
......................................................................

Patch Set 2: Code-Review+2

[Furquan Shaikh (Code Review)]

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

Change subject: mb/google/hatch: Fine-tune Kohaku I2C CLK frequency
......................................................................

Patch Set 2: Code-Review+2

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34559 )

Change subject: mb/google/hatch: Fine-tune Kohaku I2C CLK frequency
......................................................................

Patch Set 2:

(1 comment)

Please make sure you mark all comments in the code as resolved so the patch can be submitted.

Thanks.

https://review.coreboot.org/c/coreboot/+/34559/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34559/1//COMMIT_MSG@9 
PS1, Line 9: Add rise time / fall time to I2C config in device tree
> probing + minor twiddle

Done

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34559 )

Change subject: mb/google/hatch: Fine-tune Kohaku I2C CLK frequency
......................................................................

mb/google/hatch: Fine-tune Kohaku I2C CLK frequency

Add rise time / fall time to I2C config in device tree to ensure I2C
CLK runs accurately at I2C_SPEED_FAST (400 kHz).

BUG=b:138258384
BRANCH=none
TEST=probe I2C0/I2C2/I2C3 SCL on Kohaku board, verify all of them run
at 395-399 kHz.

Change-Id: Id98079e717f0db3fdcb88f85e45693925d11d7fd
Signed-off-by: Philip Chen 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34559
Reviewed-by: Tim Wawrzynczak 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Paul Fagerburg 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/google/hatch/variants/kohaku/overridetree.cb
1 file changed, 6 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Tim Wawrzynczak: Looks good to me, approved
  Paul Fagerburg: Looks good to me, but someone else must approve

diff --git a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
index fa64d60..b3ae1bc 100644
--- a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
+++ b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
@@ -32,15 +32,21 @@
    register "common_soc_config" = "{
    	.i2c[0] = {
    		.speed = I2C_SPEED_FAST,
+			.rise_time_ns = 135,
+			.fall_time_ns = 45,
    	},
    	.i2c[1] = {
    		.speed = I2C_SPEED_FAST,
    	},
    	.i2c[2] = {
    		.speed = I2C_SPEED_FAST,
+			.rise_time_ns = 95,
+			.fall_time_ns = 55,
    	},
    	.i2c[4] = {
    		.speed = I2C_SPEED_FAST,
+			.rise_time_ns = 100,
+			.fall_time_ns = 20,
    	},
    	.gspi[0] = {
    	       .speed_mhz = 1,