Change in ...coreboot[master]: soc/intel/apollolake: Add GLK usb2eye configuration override

[Seunghwan Kim (Code Review)]

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

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

soc/intel/apollolake: Add GLK usb2eye configuration override

Now we have usb2eye configuration register in FSPUPD, so we would
add an interface to override usb2eye setting.

BRANCH=octopus
BUG=NONE
TEST=Verified usb2eye custom setting works

Change-Id: I5c500964658072eaaf59364242aa928df25d99d1
Signed-off-by: Seunghwan Kim 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/include/soc/usb.h
2 files changed, 22 insertions(+), 0 deletions(-)

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

diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index 0d6d6d5..f120f47 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -548,6 +548,25 @@
    struct soc_intel_apollolake_config *cfg, FSP_S_CONFIG *silconfig)
 {
 #if IS_ENABLED(CONFIG_SOC_INTEL_GLK)
+	uint8_t port;
+
+	for (port = 0; port < APOLLOLAKE_USB2_PORT_MAX; port++) {
+		if (!cfg->usb2eye[port].Usb20OverrideEn) continue;
+
+		silconfig->Usb2AfePehalfbit[port] =
+			cfg->usb2eye[port].Usb20PerPortTxPeHalf;
+
+		silconfig->Usb2AfePetxiset[port] =
+			cfg->usb2eye[port].Usb20PerPortPeTxiSet;
+
+		silconfig->Usb2AfeTxiset[port] =
+			cfg->usb2eye[port].Usb20PerPortTxiSet;
+
+		silconfig->Usb2AfePredeemp[port] =
+			cfg->usb2eye[port].Usb20IUsbTxEmphasisEn;
+	}
+
+
    silconfig->Gmm = 0;

/* On Geminilake, we need to override the default FSP PCIe de-emphasis
diff --git a/src/soc/intel/apollolake/include/soc/usb.h b/src/soc/intel/apollolake/include/soc/usb.h
index 7dd9ec0..a2a2a63 100644
--- a/src/soc/intel/apollolake/include/soc/usb.h
+++ b/src/soc/intel/apollolake/include/soc/usb.h
@@ -30,6 +30,9 @@
    uint8_t Usb20IUsbTxEmphasisEn;
    uint8_t Usb20PerPortRXISet;
    uint8_t Usb20HsNpreDrvSel;
+#if IS_ENABLED(CONFIG_SOC_INTEL_GLK)
+	uint8_t Usb20OverrideEn;
+#endif
 };

#endif /* _SOC_APOLLOLAKE_USB_H_ */

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31060 )

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31060/1/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/31060/1/src/soc/intel/apollolake/chip.c@554 
PS1, Line 554: 		if (!cfg->usb2eye[port].Usb20OverrideEn) continue;
trailing statements should be on next line

[Seunghwan Kim (Code Review)]

Hello Patrick Rudolph, Seunghwan Kim, Marx Wang, Furquan Shaikh,

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

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

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

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

soc/intel/apollolake: Add GLK usb2eye configuration override

Now we have usb2eye configuration register in FSPUPD, so we would
add an interface to override usb2eye setting.

BRANCH=octopus
BUG=NONE
TEST=Verified usb2eye custom setting works

Change-Id: I5c500964658072eaaf59364242aa928df25d99d1
Signed-off-by: Seunghwan Kim 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/include/soc/usb.h
2 files changed, 20 insertions(+), 0 deletions(-)

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

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/chip.c@554 
PS2, Line 554: if (!cfg->usb2eye[port].Usb20OverrideEn)
             : 			continue;
Why is this check required? In case the values are not set, FSP UPDs would be set to 0 and that should be fine.

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/chip.c@567 
PS2, Line 567: 
One blank line should be sufficient.

[Seunghwan Kim (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Karthik Ramasubramanian, Seunghwan Kim, Marx Wang, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

soc/intel/apollolake: Add GLK usb2eye configuration override

Now we have usb2eye configuration register in FSPUPD, so we would
add an interface to override usb2eye setting.

BRANCH=octopus
BUG=NONE
TEST=Verified usb2eye custom setting works

Change-Id: I5c500964658072eaaf59364242aa928df25d99d1
Signed-off-by: Seunghwan Kim 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/include/soc/usb.h
2 files changed, 19 insertions(+), 0 deletions(-)

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

[Seunghwan Kim (Code Review)]

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

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/chip.c@554 
PS2, Line 554: if (!cfg->usb2eye[port].Usb20OverrideEn)
             : 			continue;
> Why is this check required? In case the values are not set, FSP UPDs would be set to 0 and that shou […]

I thought someone may want to override to 0 value from not zero.
Do you think we don't have to care about overriding to 0?

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/chip.c@567 
PS2, Line 567:
> One blank line should be sufficient.

Done

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/chip.c@554 
PS2, Line 554: if (!cfg->usb2eye[port].Usb20OverrideEn)
             : 			continue;
> I thought someone may want to override to 0 value from not zero. […]

Is the default value of this UPD non-zero? If not, then it doesn't matter. That would make the behavior similar to what APL does above.

[Seunghwan Kim (Code Review)]

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

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/chip.c@554 
PS2, Line 554: if (!cfg->usb2eye[port].Usb20OverrideEn)
             : 			continue;
> Is the default value of this UPD non-zero? If not, then it doesn't matter. […]

https://chrome-internal.googlesource.com/chromeos/third_party/intel-fsp/glk/...

Usb2AfePetxiset and Usb2AfePredeemp have non zero default values.

# !BSF NAME:{USB Per Port HS Preemphasis Bias} TYPE:{EditNum, HEX, (0x00,0xFFFFFFFFFFFFFFFF)}
  # !BSF HELP:{USB Per Port HS Preemphasis Bias. 000b-0mV, 001b-40.5mV, 010b-60.5mV, 011b-102mV, 100b-102mV, 101b-142mV, 110b-162.5mV, 111b-202.5mV. One byte for each port.}
  gGlkFspPkgTokenSpaceGuid.Usb2AfePetxiset                      | 0x0388 | 0x8 | {0x01, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05}
  # !BSF NAME:{USB Per Port HS Transmitter Bias} TYPE:{EditNum, HEX, (0x00,0x0xFFFFFFFFFFFFFFFF)}
  # !BSF HELP:{USB Per Port HS Transmitter Bias. 000b-0mV, 001b-40.5mV, 010b-60.5mV, 011b-102mV, 100b-102mV, 101b-142mV, 110b-162.5mV, 111b-202.5mV. One byte for each port.}
  gGlkFspPkgTokenSpaceGuid.Usb2AfeTxiset                        | 0x0390 | 0x8 | {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}
  # !BSF NAME:{USB Per Port HS Transmitter Emphasis} TYPE:{EditNum, HEX, (0x00,0x0xFFFFFFFFFFFFFFFF)}
  # !BSF HELP:{USB Per Port HS Transmitter Emphasis. 00b - Emphasis OFF, 01b - De-emphasis ON, 10b - Pre-emphasis ON, 11b - Pre-emphasis & De-emphasis ON. One byte for each port.}
  gGlkFspPkgTokenSpaceGuid.Usb2AfePredeemp                      | 0x0398 | 0x8 | {0x03, 0x03, 0x03, 0x03, 0x03, 0x03, 0x03, 0x03}
  # !BSF NAME:{USB Per Port Half Bit Pre-emphasis} TYPE:{EditNum, HEX, (0x00,0x0xFFFFFFFFFFFFFFFF)}
  # !BSF HELP:{USB Per Port Half Bit Pre-emphasis. 1b - half-bit pre-emphasis, 0b - full-bit pre-emphasis. One byte for each port.}
  gGlkFspPkgTokenSpaceGuid.Usb2AfePehalfbit                     | 0x03A0 | 0x8 | {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/chip.c@554 
PS2, Line 554: if (!cfg->usb2eye[port].Usb20OverrideEn)
             : 			continue;
> https://chrome-internal.googlesource. […]

Aah I see.

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/include/soc... 
File src/soc/intel/apollolake/include/soc/usb.h:

https://review.coreboot.org/#/c/31060/2/src/soc/intel/apollolake/include/soc... 
PS2, Line 33: #if IS_ENABLED(CONFIG_SOC_INTEL_GLK)
I think we can remove this. It will remain unused for APL.

[shkim (Code Review)]

shkim has uploaded a new patch set (#4) to the change originally created by Seunghwan Kim. ( https://review.coreboot.org/c/coreboot/+/31060 )

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

soc/intel/apollolake: Add GLK usb2eye configuration override

Now we have usb2eye configuration register in FSPUPD, so we would
add an interface to override usb2eye setting.

BRANCH=octopus
BUG=NONE
TEST=Verified usb2eye custom setting works

Change-Id: I5c500964658072eaaf59364242aa928df25d99d1
Signed-off-by: Seunghwan Kim 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/include/soc/usb.h
2 files changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/31060/4

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

Patch Set 4: Code-Review+2

[Paul Menzel (Code Review)]

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

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

Patch Set 4: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/31060/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31060/4//COMMIT_MSG@9 
PS4, Line 9: would
need

[shkim (Code Review)]

shkim has uploaded a new patch set (#5) to the change originally created by Seunghwan Kim. ( https://review.coreboot.org/c/coreboot/+/31060 )

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

soc/intel/apollolake: Add GLK usb2eye configuration override

Now we have usb2eye configuration register in FSPUPD, so we need
to add an interface to override usb2eye setting.

BRANCH=octopus
BUG=NONE
TEST=Verified usb2eye custom setting works

Change-Id: I5c500964658072eaaf59364242aa928df25d99d1
Signed-off-by: Seunghwan Kim 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/include/soc/usb.h
2 files changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/31060/5

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/apollolake: Add GLK usb2eye configuration override
......................................................................

soc/intel/apollolake: Add GLK usb2eye configuration override

Now we have usb2eye configuration register in FSPUPD, so we need
to add an interface to override usb2eye setting.

BRANCH=octopus
BUG=NONE
TEST=Verified usb2eye custom setting works

Change-Id: I5c500964658072eaaf59364242aa928df25d99d1
Signed-off-by: Seunghwan Kim 
Reviewed-on: https://review.coreboot.org/c/31060
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Paul Menzel 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/include/soc/usb.h
2 files changed, 17 insertions(+), 0 deletions(-)

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

diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index afbb45c..a3ce4838 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -593,6 +593,22 @@
    struct soc_intel_apollolake_config *cfg, FSP_S_CONFIG *silconfig)
 {
 #if IS_ENABLED(CONFIG_SOC_INTEL_GLK)
+	uint8_t port;
+
+	for (port = 0; port < APOLLOLAKE_USB2_PORT_MAX; port++) {
+		if (!cfg->usb2eye[port].Usb20OverrideEn)
+			continue;
+
+		silconfig->Usb2AfePehalfbit[port] =
+			cfg->usb2eye[port].Usb20PerPortTxPeHalf;
+		silconfig->Usb2AfePetxiset[port] =
+			cfg->usb2eye[port].Usb20PerPortPeTxiSet;
+		silconfig->Usb2AfeTxiset[port] =
+			cfg->usb2eye[port].Usb20PerPortTxiSet;
+		silconfig->Usb2AfePredeemp[port] =
+			cfg->usb2eye[port].Usb20IUsbTxEmphasisEn;
+	}
+
    silconfig->Gmm = 0;

/* On Geminilake, we need to override the default FSP PCIe de-emphasis
diff --git a/src/soc/intel/apollolake/include/soc/usb.h b/src/soc/intel/apollolake/include/soc/usb.h
index 7dd9ec0..28cad37 100644
--- a/src/soc/intel/apollolake/include/soc/usb.h
+++ b/src/soc/intel/apollolake/include/soc/usb.h
@@ -30,6 +30,7 @@
    uint8_t Usb20IUsbTxEmphasisEn;
    uint8_t Usb20PerPortRXISet;
    uint8_t Usb20HsNpreDrvSel;
+	uint8_t Usb20OverrideEn;
 };

#endif /* _SOC_APOLLOLAKE_USB_H_ */