Change in coreboot[master]: mb/google/dedede/var/magolor: Configure I2C high and low time

[Ren Kuo (Code Review)]

Ren Kuo has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46613 )

Change subject: mb/google/dedede/var/magolor: Configure I2C high and low time
......................................................................

mb/google/dedede/var/magolor: Configure I2C high and low time

Configure the I2C bus high and low time for all enabled I2C buses.

BUG=b:168783630
TEST=Measured the I2C bus frequency reduce to 387 KHz.

Signed-off-by: Ren Kuo 
Change-Id: I9f5b81815f86db7bdcea95a95b9c9b235b4a34b1
---
M src/mainboard/google/dedede/variants/magolor/overridetree.cb
1 file changed, 24 insertions(+), 0 deletions(-)

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

diff --git a/src/mainboard/google/dedede/variants/magolor/overridetree.cb b/src/mainboard/google/dedede/variants/magolor/overridetree.cb
index f41e9fa..625546e 100644
--- a/src/mainboard/google/dedede/variants/magolor/overridetree.cb
+++ b/src/mainboard/google/dedede/variants/magolor/overridetree.cb
@@ -25,18 +25,42 @@
    	},
    	.i2c[0] = {
    		.speed = I2C_SPEED_FAST,
+			.speed_config[0] = {
+				.speed = I2C_SPEED_FAST,
+				.scl_lcnt = 190,
+				.scl_hcnt = 100,
+				.sda_hold = 40,
+			}
    	},
    	.i2c[1] = {
    		.speed = I2C_SPEED_FAST,
    	},
    	.i2c[2] = {
    		.speed = I2C_SPEED_FAST,
+			.speed_config[0] = {
+				.speed = I2C_SPEED_FAST,
+				.scl_lcnt = 190,
+				.scl_hcnt = 100,
+				.sda_hold = 40,
+			}
    	},
    	.i2c[3] = {
    		.speed = I2C_SPEED_FAST,
+			.speed_config[0] = {
+				.speed = I2C_SPEED_FAST,
+				.scl_lcnt = 190,
+				.scl_hcnt = 100,
+				.sda_hold = 40,
+			}
    	},
    	.i2c[4] = {
    		.speed = I2C_SPEED_FAST,
+			.speed_config[0] = {
+				.speed = I2C_SPEED_FAST,
+				.scl_lcnt = 190,
+				.scl_hcnt = 100,
+				.sda_hold = 40,
+			}
    	},
    }"

[Ren Kuo (Code Review)]

Ren Kuo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46613 )

Change subject: mb/google/dedede/var/magolor: Configure I2C high and low time
......................................................................

Patch Set 1: Code-Review+1

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46613 )

Change subject: mb/google/dedede/var/magolor: Configure I2C high and low time
......................................................................

Patch Set 1: Code-Review+2

[Paul Menzel (Code Review)]

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

Change subject: mb/google/dedede/var/magolor: Configure I2C high and low time
......................................................................

Patch Set 1:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/46613/1//COMMIT_MSG@12 
PS1, Line 12: TEST=Measured the I2C bus frequency reduce to 387 KHz.
What was the value before?

https://review.coreboot.org/c/coreboot/+/46613/1/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/variants/magolor/overridetree.cb:

https://review.coreboot.org/c/coreboot/+/46613/1/src/mainboard/google/dedede... 
PS1, Line 27: .speed = I2C_SPEED_FAST,
            : 			.speed_config[0] = {
            : 				.speed = I2C_SPEED_FAST,
Is `.speed` really specified twice?

[Ren Kuo (Code Review)]

Ren Kuo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46613 )

Change subject: mb/google/dedede/var/magolor: Configure I2C high and low time
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/46613/1//COMMIT_MSG@12 
PS1, Line 12: TEST=Measured the I2C bus frequency reduce to 387 KHz.
> What was the value before?

the i2c bus frequency is 438.809KHz in previous settings.
refer to
https://partnerissuetracker.corp.google.com/issues/168783630#comment1

[Ren Kuo (Code Review)]

Ren Kuo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46613 )

Change subject: mb/google/dedede/var/magolor: Configure I2C high and low time
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46613/1/src/mainboard/google/dedede... 
File src/mainboard/google/dedede/variants/magolor/overridetree.cb:

https://review.coreboot.org/c/coreboot/+/46613/1/src/mainboard/google/dedede... 
PS1, Line 27: .speed = I2C_SPEED_FAST,
            : 			.speed_config[0] = {
            : 				.speed = I2C_SPEED_FAST,
> Is `. […]

from current fsp code, the "speed" will be specified twice.
initial i2c parameters by different power on stages.

FSP code, struct definition:

struct soc_intel_common_config{
    ...
    struct dw_i2c_bus_config
    ...
}

struct dw_i2c_bus_config {
   /* Bus should be enabled prior to ramstage with temporary base */
   ...
   enum i2c_speed speed;
   ...
  /* Specific bus speed configuration */
   struct dw_i2c_speed_config
}

struct dw_i2c_speed_config {
    enum i2c_speed speed;
    uint16_t scl_lcnt;
    uint16_t scl_hcnt;
    uint32_t sda_hold;
};

[Patrick Georgi (Code Review)]

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

Change subject: mb/google/dedede/var/magolor: Configure I2C high and low time
......................................................................

mb/google/dedede/var/magolor: Configure I2C high and low time

Configure the I2C bus high and low time for all enabled I2C buses.

BUG=b:168783630
TEST=Measured the I2C bus frequency reduce to 387 KHz.

Signed-off-by: Ren Kuo 
Change-Id: I9f5b81815f86db7bdcea95a95b9c9b235b4a34b1
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46613
Reviewed-by: Karthik Ramasubramanian 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/google/dedede/variants/magolor/overridetree.cb
1 file changed, 24 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Ren Kuo: Looks good to me, but someone else must approve
  Karthik Ramasubramanian: Looks good to me, approved

diff --git a/src/mainboard/google/dedede/variants/magolor/overridetree.cb b/src/mainboard/google/dedede/variants/magolor/overridetree.cb
index f41e9fa..625546e 100644
--- a/src/mainboard/google/dedede/variants/magolor/overridetree.cb
+++ b/src/mainboard/google/dedede/variants/magolor/overridetree.cb
@@ -25,18 +25,42 @@
    	},
    	.i2c[0] = {
    		.speed = I2C_SPEED_FAST,
+			.speed_config[0] = {
+				.speed = I2C_SPEED_FAST,
+				.scl_lcnt = 190,
+				.scl_hcnt = 100,
+				.sda_hold = 40,
+			}
    	},
    	.i2c[1] = {
    		.speed = I2C_SPEED_FAST,
    	},
    	.i2c[2] = {
    		.speed = I2C_SPEED_FAST,
+			.speed_config[0] = {
+				.speed = I2C_SPEED_FAST,
+				.scl_lcnt = 190,
+				.scl_hcnt = 100,
+				.sda_hold = 40,
+			}
    	},
    	.i2c[3] = {
    		.speed = I2C_SPEED_FAST,
+			.speed_config[0] = {
+				.speed = I2C_SPEED_FAST,
+				.scl_lcnt = 190,
+				.scl_hcnt = 100,
+				.sda_hold = 40,
+			}
    	},
    	.i2c[4] = {
    		.speed = I2C_SPEED_FAST,
+			.speed_config[0] = {
+				.speed = I2C_SPEED_FAST,
+				.scl_lcnt = 190,
+				.scl_hcnt = 100,
+				.sda_hold = 40,
+			}
    	},
    }"

[Paul Menzel (Code Review)]

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

Change subject: mb/google/dedede/var/magolor: Configure I2C high and low time
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/46613/1//COMMIT_MSG@12 
PS1, Line 12: TEST=Measured the I2C bus frequency reduce to 387 KHz.
> the i2c bus frequency is 438.809KHz in previous settings. […]

Thank you for your anwser. I am not allowed to view that bug report, so please update the commit message next time with that information. If possible, commit messages should be selfcontained.