Change in coreboot[master]: mb/google/kohaku: Adjust I2C clock frequency

[shkim (Code Review)]

Hello Seunghwan Kim,

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

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

to review the following change.

Change subject: mb/google/kohaku: Adjust I2C clock frequency
......................................................................

mb/google/kohaku: Adjust I2C clock frequency

This change adjusts I2C speed settings to limit frequency to 400KHz.

The new setting values have been from other projects using same I2C
components.

BUG=b:144885961
BRANCH=firmware-hatch-12672.B
TEST=Verified I2C1 and I2C4 frequency not over 400KHz

Change-Id: I9614fb39b6e55cb2ce1b0879a9f5204e55002f8d
Signed-off-by: Seunghwan Kim 
---
M src/mainboard/google/hatch/variants/kohaku/overridetree.cb
1 file changed, 4 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/37313/1

diff --git a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
index 55ac071..d515ecc 100644
--- a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
+++ b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
@@ -60,6 +60,8 @@
    	},
    	.i2c[1] = {
    		.speed = I2C_SPEED_FAST,
+			.rise_time_ns = 60,
+			.fall_time_ns = 25,
    	},
    	.i2c[2] = {
    		.speed = I2C_SPEED_FAST,
@@ -68,8 +70,8 @@
    	},
    	.i2c[4] = {
    		.speed = I2C_SPEED_FAST,
-			.rise_time_ns = 100,
-			.fall_time_ns = 20,
+			.rise_time_ns = 104,
+			.fall_time_ns = 52,
    	},
    	.gspi[0] = {
    	       .speed_mhz = 1,

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/kohaku: Adjust I2C clock frequency
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/37313/1//COMMIT_MSG@11 
PS1, Line 11: The new setting values have been from other projects using same I2C
            : components.
Aren't these settings board-specific?

[shkim (Code Review)]

shkim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37313 )

Change subject: mb/google/kohaku: Adjust I2C clock frequency
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/37313/1//COMMIT_MSG@11 
PS1, Line 11: The new setting values have been from other projects using same I2C
            : components.
> Aren't these settings board-specific?

We just verified that I2C frequency < 400MHz with the new parameters on kohaku. (b:144885961)
As we measured w/o this change, I2C1/I2C4 frequency > 400MHz. So I needed to find new parameters, I got them from other project's than hatch which were using "Elan touchscreen" and "da7219" codec.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37313 )

Change subject: mb/google/kohaku: Adjust I2C clock frequency
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/37313/1//COMMIT_MSG@11 
PS1, Line 11: The new setting values have been from other projects using same I2C
            : components.
> We just verified that I2C frequency < 400MHz with the new parameters on kohaku. (b:144885961) […]

These are board-specific, but it sounds like shkim@ empirically verified the changes to meet their requirements. @shkim, I think the commit description should be updated with the information in your comment above to make it clearer.

[Tim Wawrzynczak (Code Review)]

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

Change subject: mb/google/kohaku: Adjust I2C clock frequency
......................................................................

Patch Set 1: Code-Review+2

[shkim (Code Review)]

Hello Seunghwan Kim, Tim Wawrzynczak, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: mb/google/kohaku: Adjust I2C clock frequency
......................................................................

mb/google/kohaku: Adjust I2C clock frequency

All serial I2C bus frequencies should not be over 400KHz in kohaku,
but the measurement showed frequencies of I2C1 and I2C4 were over
400KHz. (b:144885961)

This change adjusts I2C speed settings to limit that frequencies to
400KHz.

The new setting values have been from other projects using same I2C
components, and verified I2C1 and I2C4 frequencies < 400MHz internally.

BUG=b:144885961
BRANCH=firmware-hatch-12672.B
TEST=Verified I2C1 and I2C4 frequency not over 400KHz

Change-Id: I9614fb39b6e55cb2ce1b0879a9f5204e55002f8d
Signed-off-by: Seunghwan Kim 
---
M src/mainboard/google/hatch/variants/kohaku/overridetree.cb
1 file changed, 4 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/37313/2

[shkim (Code Review)]

shkim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37313 )

Change subject: mb/google/kohaku: Adjust I2C clock frequency
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/37313/1//COMMIT_MSG@11 
PS1, Line 11: The new setting values have been from other projects using same I2C
            : components.
> These are board-specific, but it sounds like shkim@ empirically verified the changes to meet their r […]

Done

[Patrick Georgi (Code Review)]

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

Change subject: mb/google/kohaku: Adjust I2C clock frequency
......................................................................

mb/google/kohaku: Adjust I2C clock frequency

All serial I2C bus frequencies should not be over 400KHz in kohaku,
but the measurement showed frequencies of I2C1 and I2C4 were over
400KHz. (b:144885961)

This change adjusts I2C speed settings to limit that frequencies to
400KHz.

The new setting values have been from other projects using same I2C
components, and verified I2C1 and I2C4 frequencies < 400MHz internally.

BUG=b:144885961
BRANCH=firmware-hatch-12672.B
TEST=Verified I2C1 and I2C4 frequency not over 400KHz

Change-Id: I9614fb39b6e55cb2ce1b0879a9f5204e55002f8d
Signed-off-by: Seunghwan Kim 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/37313
Tested-by: build bot (Jenkins) 
Reviewed-by: Tim Wawrzynczak 
---
M src/mainboard/google/hatch/variants/kohaku/overridetree.cb
1 file changed, 4 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Tim Wawrzynczak: Looks good to me, approved

diff --git a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
index 55ac071..d515ecc 100644
--- a/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
+++ b/src/mainboard/google/hatch/variants/kohaku/overridetree.cb
@@ -60,6 +60,8 @@
    	},
    	.i2c[1] = {
    		.speed = I2C_SPEED_FAST,
+			.rise_time_ns = 60,
+			.fall_time_ns = 25,
    	},
    	.i2c[2] = {
    		.speed = I2C_SPEED_FAST,
@@ -68,8 +70,8 @@
    	},
    	.i2c[4] = {
    		.speed = I2C_SPEED_FAST,
-			.rise_time_ns = 100,
-			.fall_time_ns = 20,
+			.rise_time_ns = 104,
+			.fall_time_ns = 52,
    	},
    	.gspi[0] = {
    	       .speed_mhz = 1,