Change in ...coreboot[master]: mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK fr...

[Seunghwan Kim (Code Review)]

Seunghwan Kim has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31085

Change subject: mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency
......................................................................

mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency

ELAN touchpad supports up to 400KHz, so we need to limit its CLK
frequency to 400HKz.

BUG=NONE
BRANCH=octopus
TEST=built and verified touchpad I2C clk frequency gets be lower than 400KHz

Change-Id: If7a43fe20c7e5abdf23c8c36e34c072c371563bf
Signed-off-by: Seunghwan Kim 
---
M src/mainboard/google/octopus/variants/casta/overridetree.cb
1 file changed, 13 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/31085/1

diff --git a/src/mainboard/google/octopus/variants/casta/overridetree.cb b/src/mainboard/google/octopus/variants/casta/overridetree.cb
index 8915e1a..fbb1826 100644
--- a/src/mainboard/google/octopus/variants/casta/overridetree.cb
+++ b/src/mainboard/google/octopus/variants/casta/overridetree.cb
@@ -1,5 +1,4 @@
 chip soc/intel/apollolake
-
    # Intel Common SoC Config
    #+-------------------+---------------------------+
    #| Field             |  Value                    |
@@ -8,12 +7,25 @@
    #|                   | required to set up a BAR  |
    #|                   | for TPM communication     |
    #|                   | before memory is up       |
+	#| I2C5              | Audio                     |
+	#| I2C6              | Trackpad                  |
    #+-------------------+---------------------------+
    register "common_soc_config" = "{
    	.gspi[0] = {
    		.speed_mhz = 1,
    		.early_init = 1,
    	},
+		.i2c[5] = {
+			.speed = I2C_SPEED_FAST,
+			.rise_time_ns = 104,
+			.fall_time_ns = 52,
+		},
+		.i2c[6] = {
+			.speed = I2C_SPEED_FAST,
+			.rise_time_ns = 66,
+			.fall_time_ns = 90,
+			.data_hold_time_ns = 350,
+		},
    }"

device domain 0 on

[Furquan Shaikh (Code Review)]

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

Change subject: mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/31085/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31085/1//COMMIT_MSG@7 
PS1, Line 7: touchpad
This CL is configuring more than just touchpad.

https://review.coreboot.org/#/c/31085/1//COMMIT_MSG@12 
PS1, Line 12: NONE
Can you please raise a bug and attach more details in it:
1. I2C CLK measurements
2. Scope shots captured for verification

[Seunghwan Kim (Code Review)]

Hello Aaron Durbin, Karthik Ramasubramanian, Justin TerAvest, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,

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

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

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

Change subject: mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency
......................................................................

mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency

ELAN touchpad supports up to 400KHz, so we need to limit its CLK
frequency to 400HKz.

BUG=b:123376618
BRANCH=octopus
TEST=built and verified touchpad I2C clk frequency gets be lower than 400KHz

Change-Id: If7a43fe20c7e5abdf23c8c36e34c072c371563bf
Signed-off-by: Seunghwan Kim 
---
M src/mainboard/google/octopus/variants/casta/overridetree.cb
1 file changed, 8 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/31085/2

[Seunghwan Kim (Code Review)]

Seunghwan Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31085 )

Change subject: mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/31085/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31085/1//COMMIT_MSG@7 
PS1, Line 7: touchpad
> This CL is configuring more than just touchpad.

Done

https://review.coreboot.org/#/c/31085/1//COMMIT_MSG@12 
PS1, Line 12: NONE
> Can you please raise a bug and attach more details in it: […]

I just opened b:123376618, our H/W guy will share the measured data.

[Karthik Ramasubramanian (Code Review)]

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

Change subject: mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31085/2/src/mainboard/google/octopus/variant... 
File src/mainboard/google/octopus/variants/casta/overridetree.cb:

https://review.coreboot.org/#/c/31085/2/src/mainboard/google/octopus/variant... 
PS2, Line 10: 	#| I2C5              | Audio                     |
This can be removed since there is no I2C configuration for audio

[Seunghwan Kim (Code Review)]

Seunghwan Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31085 )

Change subject: mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31085/2/src/mainboard/google/octopus/variant... 
File src/mainboard/google/octopus/variants/casta/overridetree.cb:

https://review.coreboot.org/#/c/31085/2/src/mainboard/google/octopus/variant... 
PS2, Line 10: 	#| I2C5              | Audio                     |
> This can be removed since there is no I2C configuration for audio

Don't we need to specify this even we have real audio I2C connection on I2C5 port?

[Karthik Ramasubramanian (Code Review)]

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

Change subject: mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31085/2/src/mainboard/google/octopus/variant... 
File src/mainboard/google/octopus/variants/casta/overridetree.cb:

https://review.coreboot.org/#/c/31085/2/src/mainboard/google/octopus/variant... 
PS2, Line 10: 	#| I2C5              | Audio                     |
> Don't we need to specify this even we have real audio I2C connection on I2C5 port?

In this CL, I don't see any I2C tuning for I2C 5 port. That is why I added that comment. Do you plan to add the tuning for that port in a different CL. If so, please add it in that CL.

[Seunghwan Kim (Code Review)]

Seunghwan Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31085 )

Change subject: mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31085/2/src/mainboard/google/octopus/variant... 
File src/mainboard/google/octopus/variants/casta/overridetree.cb:

https://review.coreboot.org/#/c/31085/2/src/mainboard/google/octopus/variant... 
PS2, Line 10: 	#| I2C5              | Audio                     |
> In this CL, I don't see any I2C tuning for I2C 5 port. That is why I added that comment. […]

If we don't have to tune audio port I2C timing, then don't we specify about audio device in this structure?

[Furquan Shaikh (Code Review)]

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

Change subject: mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31085/2/src/mainboard/google/octopus/variant... 
File src/mainboard/google/octopus/variants/casta/overridetree.cb:

https://review.coreboot.org/#/c/31085/2/src/mainboard/google/octopus/variant... 
PS2, Line 10: 	#| I2C5              | Audio                     |
> If we don't have to tune audio port I2C timing, then don't we specify about audio device in this str […]

Is the audio I2C clock frequency okay without any special tuning parameters?

[Seunghwan Kim (Code Review)]

Seunghwan Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31085 )

Change subject: mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31085/2/src/mainboard/google/octopus/variant... 
File src/mainboard/google/octopus/variants/casta/overridetree.cb:

https://review.coreboot.org/#/c/31085/2/src/mainboard/google/octopus/variant... 
PS2, Line 10: 	#| I2C5              | Audio                     |
> Is the audio I2C clock frequency okay without any special tuning parameters?

Yes, our audio guy confirmed.

[Furquan Shaikh (Code Review)]

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

Change subject: mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31085 )

Change subject: mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency
......................................................................

mainboard/google/octopus/variants/casta: Decrease touchpad I2C CLK frequency

ELAN touchpad supports up to 400KHz, so we need to limit its CLK
frequency to 400HKz.

BUG=b:123376618
BRANCH=octopus
TEST=built and verified touchpad I2C clk frequency gets be lower than 400KHz

Change-Id: If7a43fe20c7e5abdf23c8c36e34c072c371563bf
Signed-off-by: Seunghwan Kim 
Reviewed-on: https://review.coreboot.org/c/31085
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
---
M src/mainboard/google/octopus/variants/casta/overridetree.cb
1 file changed, 8 insertions(+), 1 deletion(-)

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

diff --git a/src/mainboard/google/octopus/variants/casta/overridetree.cb b/src/mainboard/google/octopus/variants/casta/overridetree.cb
index 8915e1a..d70a6ea 100644
--- a/src/mainboard/google/octopus/variants/casta/overridetree.cb
+++ b/src/mainboard/google/octopus/variants/casta/overridetree.cb
@@ -1,5 +1,4 @@
 chip soc/intel/apollolake
-
    # Intel Common SoC Config
    #+-------------------+---------------------------+
    #| Field             |  Value                    |
@@ -8,12 +7,20 @@
    #|                   | required to set up a BAR  |
    #|                   | for TPM communication     |
    #|                   | before memory is up       |
+	#| I2C5              | Audio                     |
+	#| I2C6              | Trackpad                  |
    #+-------------------+---------------------------+
    register "common_soc_config" = "{
    	.gspi[0] = {
    		.speed_mhz = 1,
    		.early_init = 1,
    	},
+		.i2c[6] = {
+			.speed = I2C_SPEED_FAST,
+			.rise_time_ns = 66,
+			.fall_time_ns = 90,
+			.data_hold_time_ns = 350,
+		},
    }"

device domain 0 on