Change in ...coreboot[master]: soc/intel/apollolake: Override GLK usb clock gating register

[John Zhao (Code Review)]

Hello John Zhao,

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

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

to review the following change.

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

soc/intel/apollolake: Override GLK usb clock gating register

It was observed system suspend/resume failure while running
RunInDozingStress. Apply correct GLK usb clock gating register
value to mitigate the failure.

BRANCH=octopus
BUG=b:120526309
TEST=Verified GLK clock gating register value after booting
to kernel.

Change-Id: I50fb16f5ab0e28e79f71c7f0f8e75ac8791c0747
Signed-off-by: John Zhao 
---
M src/soc/intel/apollolake/chip.c
1 file changed, 19 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/30918/1

diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index b38265f..ad98f5f 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -60,6 +60,8 @@
 #define DRD_MODE_MASK           (1 << 29)
 #define DRD_MODE_HOST           (1 << 29)

+#define CFG_XHCLKGTEN		0x8650
+
 const char *soc_acpi_name(const struct device *dev)
 {
    if (dev->path.type == DEVICE_PATH_DOMAIN)
@@ -733,6 +735,11 @@

void platform_fsp_notify_status(enum fsp_notify_phase phase)
 {
+	uint32_t *cfg;
+	const struct resource *res;
+	uint32_t reg;
+	struct device *xhci_dev = PCH_DEV_XHCI;
+
    if (phase == END_OF_FIRMWARE) {

/*
@@ -758,6 +765,18 @@
    	 */
    	if (check_xdci_enable())
    		configure_xhci_host_mode_port0();
+
+		/*
+		 * Override GLK clock gating register(XHCLKGTEN) to mitigate
+		 * usb device suspend and resume failure.
+		 */
+		if (IS_ENABLED(CONFIG_SOC_INTEL_GLK))
+		{
+			res = find_resource(xhci_dev, PCI_BASE_ADDRESS_0);
+			cfg = (void *)(uintptr_t)(res->base + CFG_XHCLKGTEN);
+			reg = 0x0FCE6E5F;
+			write32(cfg, reg);
+		}
    }
 }

[build bot (Jenkins) (Code Review)]

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

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/#/c/30918/1/src/soc/intel/apollolake/chip.c@773 
PS1, Line 773: 		if (IS_ENABLED(CONFIG_SOC_INTEL_GLK))
that open brace { should be on the previous line

[John Zhao (Code Review)]

Hello Patrick Rudolph, John Zhao,

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

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

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

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

soc/intel/apollolake: Override GLK usb clock gating register

It was observed system suspend/resume failure while running
RunInDozingStress. Apply correct GLK usb clock gating register
value to mitigate the failure.

BRANCH=octopus
BUG=b:120526309
TEST=Verified GLK clock gating register value after booting
to kernel.

Change-Id: I50fb16f5ab0e28e79f71c7f0f8e75ac8791c0747
Signed-off-by: John Zhao 
---
M src/soc/intel/apollolake/chip.c
1 file changed, 18 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/30918/2

[Aaron Durbin (Code Review)]

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

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/#/c/30918/2/src/soc/intel/apollolake/chip.c@776 
PS2, Line 776: 			reg = 0x0FCE6E5F;
We just slam in these settings?

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30918 )

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/#/c/30918/2/src/soc/intel/apollolake/chip.c@776 
PS2, Line 776: 			reg = 0x0FCE6E5F;
> We just slam in these settings?

It was determined 0x0FCE6E5F would be the right value for usb clock gating register. It could be either implemented at fsp or override by coreboot, which would reflect the same effect. After updating the value, b:12056309 shows it helps to mitigate suspend/resume failure.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30918 )

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/#/c/30918/2/src/soc/intel/apollolake/chip.c@776 
PS2, Line 776: 			reg = 0x0FCE6E5F;
> It was determined 0x0FCE6E5F would be the right value for usb clock gating register. […]

How was this value determined? I would like to know the magic behind magic values.

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30918 )

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/#/c/30918/2/src/soc/intel/apollolake/chip.c@776 
PS2, Line 776: 			reg = 0x0FCE6E5F;
> How was this value determined? I would like to know the magic behind magic values.

Along with usb modPhy tuned data, this xHCI register BIOS setting is provided by USB COE. The design specification describe all bits setting for the clock gating register in detail,

[Justin TerAvest (Code Review)]

Justin TerAvest has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30918 )

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 2: Code-Review+2

[Shamile Khan (Code Review)]

Shamile Khan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30918 )

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 2: Code-Review+1

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/#/c/30918/2/src/soc/intel/apollolake/chip.c@776 
PS2, Line 776: 			reg = 0x0FCE6E5F;
> Along with usb modPhy tuned data, this xHCI register BIOS setting is provided by USB COE. […]

Can you please add those bits as macros here and set them accordingly here? Its difficult to understand just by looking at the number what the actual intention is.

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30918 )

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/#/c/30918/2/src/soc/intel/apollolake/chip.c@776 
PS2, Line 776: 			reg = 0x0FCE6E5F;
> Can you please add those bits as macros here and set them accordingly here? Its difficult to underst […]

#define XHCI_CFG_XHCLKGTEN_MMIO   0x8650  ///< Clock Gating
                       BIT[31:29] Reserved
#define NUEFBCGPS      BIT28 ///< Naking USB2.0 EPs for Backbone Clock Gating and PLL Shutdown
#define SRAMPGTEN      BIT27 ///< SRAM Power Gate Enable
#define SSLSE          BIT26 ///< SS Link PLL Shutdown Enable
#define USB2PLLSE      BIT25 ///< USB2 PLL Shutdown Enable
#define IOSFSTCGE      BIT24 ///< IOSF Sideband Trunk Clock Gating Enable
#define HSTCGE         (BIT23 | BIT22 | BIT21 | BIT20) ///< HS Backbone PXP Trunk Clock Gate Enable
#define HSTCGE         20
#define SSTCGE         (BIT19 | BIT18 | BIT17 | BIT16) ///< SS Backbone PXP Trunk Clock Gate Enable
#define SSTCGE         16
#define XHCIGEU3S      BIT15 ///< XHC Ignore_EU3S
#define XHCFTCLKSE     BIT14 ///< XHC Frame Timer Clock Shutdown Enable
#define XHCBBTCGIPISO  BIT13 ///< XHC Backbone PXP Trunk Clock Gate In Presence of ISOCH EP
#define XHCHSTCGU2NRWE BIT12 ///< XHC HS Backbone PXP Trunk Clock Gate U2 non RWE
#define XHCUSB2PLLSDLE (BIT11 | BIT10) ///< XHC USB2 PLL Shutdown Lx Enable
#define HSUXDMIPLLSE   (BIT9 | BIT8) ///< HS Backbone PXP PLL Shutdown Ux Enable
#define HSUXDMIPLLSE   8
#define SSPLLSUE       (BIT7 | BIT6 | BIT5) ///< SS backbone PXP PLL Shutdown Ux Enable
#define SSPLLSUE       5
#define XHCBLCGE       BIT4  ///< XHC Backbone Local Clock Gating Enable
#define HSLTCGE        BIT3  ///< HS Link Trunk Clock Gating Enable
#define SSLTCGE        BIT2  ///< SS Link Trunk Clock Gating Enable
#define IOSFBTCGE      BIT1  ///< IOSF Backbone Trunk Clock Gating Enable
#define IOSFGBLCGE     BIT0  ///< IOSF Gasket Backbone Local Clock Gating Enable

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/#/c/30918/2/src/soc/intel/apollolake/chip.c@776 
PS2, Line 776: 			reg = 0x0FCE6E5F;
> #define XHCI_CFG_XHCLKGTEN_MMIO   0x8650  ///< Clock Gating […]

I meant can you add these macros in code.

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30918 )

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/#/c/30918/2/src/soc/intel/apollolake/chip.c@776 
PS2, Line 776: 			reg = 0x0FCE6E5F;
> I meant can you add these macros in code.

yes, we will update the code.

[John Zhao (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, Shamile Khan, John Zhao, build bot (Jenkins), Hannah Williams, Furquan Shaikh,

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

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

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

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

soc/intel/apollolake: Override GLK usb clock gating register

It was observed system suspend/resume failure while running
RunInDozingStress. Apply correct GLK usb clock gating register
value to mitigate the failure.

BRANCH=octopus
BUG=b:120526309
TEST=Verified GLK clock gating register value after booting
to kernel.

Change-Id: I50fb16f5ab0e28e79f71c7f0f8e75ac8791c0747
Signed-off-by: John Zhao 
---
M src/soc/intel/apollolake/chip.c
1 file changed, 56 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/30918/3

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30918 )

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/#/c/30918/2/src/soc/intel/apollolake/chip.c@776 
PS2, Line 776: 			reg = 0x0FCE6E5F;
> yes, we will update the code.

done.

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 3:

(3 comments)

https://review.coreboot.org/#/c/30918/3/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/30918/3/src/soc/intel/apollolake/chip.c@64 
PS3, Line 64: ///
Please follow commenting guidelines here: https://www.coreboot.org/Coding_Style#Commenting

https://review.coreboot.org/#/c/30918/3/src/soc/intel/apollolake/chip.c@64 
PS3, Line 64: BIT28
Can you please follow the same convention as rest of the file i.e. use (1 << 28) instead of BIT28?

https://review.coreboot.org/#/c/30918/3/src/soc/intel/apollolake/chip.c@772 
PS3, Line 772: uint32_t *cfg;
             : 	const struct resource *res;
             : 	uint32_t reg;
             : 	struct device *xhci_dev = PCH_DEV_XHCI;
These could potentially be moved to the if block being added at line 808.

[John Zhao (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, Shamile Khan, John Zhao, build bot (Jenkins), Hannah Williams, Furquan Shaikh,

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

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

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

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

soc/intel/apollolake: Override GLK usb clock gating register

It was observed system suspend/resume failure while running
RunInDozingStress. Apply correct GLK usb clock gating register
value to mitigate the failure.

BRANCH=octopus
BUG=b:120526309
TEST=Verified GLK clock gating register value after booting
to kernel.

Change-Id: I50fb16f5ab0e28e79f71c7f0f8e75ac8791c0747
Signed-off-by: John Zhao 
---
M src/soc/intel/apollolake/chip.c
1 file changed, 63 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/30918/4

[John Zhao (Code Review)]

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30918 )

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/#/c/30918/3/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/30918/3/src/soc/intel/apollolake/chip.c@64 
PS3, Line 64: BIT28
> Can you please follow the same convention as rest of the file i.e. […]

Done

https://review.coreboot.org/#/c/30918/3/src/soc/intel/apollolake/chip.c@64 
PS3, Line 64: ///
> Please follow commenting guidelines here: https://www.coreboot. […]

Done

https://review.coreboot.org/#/c/30918/3/src/soc/intel/apollolake/chip.c@772 
PS3, Line 772: uint32_t *cfg;
             : 	const struct resource *res;
             : 	uint32_t reg;
             : 	struct device *xhci_dev = PCH_DEV_XHCI;
> These could potentially be moved to the if block being added at line 808.

Done

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

Patch Set 4: Code-Review+2

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/apollolake: Override GLK usb clock gating register
......................................................................

soc/intel/apollolake: Override GLK usb clock gating register

It was observed system suspend/resume failure while running
RunInDozingStress. Apply correct GLK usb clock gating register
value to mitigate the failure.

BRANCH=octopus
BUG=b:120526309
TEST=Verified GLK clock gating register value after booting
to kernel.

Change-Id: I50fb16f5ab0e28e79f71c7f0f8e75ac8791c0747
Signed-off-by: John Zhao 
Reviewed-on: https://review.coreboot.org/c/30918
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
---
M src/soc/intel/apollolake/chip.c
1 file changed, 63 insertions(+), 3 deletions(-)

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

diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index 0d6d6d5..9912080 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -60,6 +60,46 @@
 #define DRD_MODE_MASK           (1 << 29)
 #define DRD_MODE_HOST           (1 << 29)

+#define CFG_XHCLKGTEN		0x8650
+/* Naking USB2.0 EPs for Backbone Clock Gating and PLL Shutdown */
+#define NUEFBCGPS		(1 << 28)
+/* SRAM Power Gate Enable */
+#define SRAMPGTEN		(1 << 27)
+/* SS Link PLL Shutdown Enable */
+#define SSLSE			(1 << 26)
+/* USB2 PLL Shutdown Enable */
+#define USB2PLLSE		(1 << 25)
+/* IOSF Sideband Trunk Clock Gating Enable */
+#define IOSFSTCGE		(1 << 24)
+/* BIT[23:20] HS Backbone PXP Trunk Clock Gate Enable */
+#define HSTCGE			(1 << 23 | 1 << 22)
+/* BIT[19:16] SS Backbone PXP Trunk Clock Gate Enable */
+#define SSTCGE			(1 << 19 | 1 << 18 | 1 << 17)
+/* XHC Ignore_EU3S */
+#define XHCIGEU3S		(1 << 15)
+/* XHC Frame Timer Clock Shutdown Enable */
+#define XHCFTCLKSE		(1 << 14)
+/* XHC Backbone PXP Trunk Clock Gate In Presence of ISOCH EP */
+#define XHCBBTCGIPISO		(1 << 13)
+/* XHC HS Backbone PXP Trunk Clock Gate U2 non RWE */
+#define XHCHSTCGU2NRWE		(1 << 12)
+/* BIT[11:10] XHC USB2 PLL Shutdown Lx Enable */
+#define XHCUSB2PLLSDLE		(1 << 11 | 1 << 10)
+/* BIT[9:8] HS Backbone PXP PLL Shutdown Ux Enable */
+#define HSUXDMIPLLSE		(1 << 9)
+/* BIT[7:5] SS Backbone PXP PLL shutdown Ux Enable */
+#define SSPLLSUE		(1 << 6)
+/* XHC Backbone Local Clock Gating Enable */
+#define XHCBLCGE		(1 << 4)
+/* HS Link Trunk Clock Gating Enable */
+#define HSLTCGE			(1 << 3)
+/* SS Link Trunk Clock Gating Enable */
+#define SSLTCGE			(1 << 2)
+/* IOSF Backbone Trunk Clock Gating Enable */
+#define IOSFBTCGE		(1 << 1)
+/* IOSF Gasket Backbone Local Clock Gating Enable */
+#define IOSFGBLCGE		(1 << 0)
+
 const char *soc_acpi_name(const struct device *dev)
 {
    if (dev->path.type == DEVICE_PATH_DOMAIN)
@@ -679,9 +719,9 @@

struct chip_operations soc_intel_apollolake_ops = {
    CHIP_NAME("Intel Apollolake SOC")
-	.enable_dev = enable_dev,
-	.init = soc_init,
-	.final = soc_final
+	.enable_dev = &enable_dev,
+	.init = &soc_init,
+	.final = &soc_final
 };

static void drop_privilege_all(void)
@@ -758,6 +798,26 @@
    	 */
    	if (check_xdci_enable())
    		configure_xhci_host_mode_port0();
+
+		/*
+		 * Override GLK xhci clock gating register(XHCLKGTEN) to
+		 * mitigate usb device suspend and resume failure.
+		 */
+		if (IS_ENABLED(CONFIG_SOC_INTEL_GLK)) {
+			uint32_t *cfg;
+			const struct resource *res;
+			uint32_t reg;
+			struct device *xhci_dev = PCH_DEV_XHCI;
+
+			res = find_resource(xhci_dev, PCI_BASE_ADDRESS_0);
+			cfg = (void *)(uintptr_t)(res->base + CFG_XHCLKGTEN);
+			reg = SRAMPGTEN | SSLSE | USB2PLLSE | IOSFSTCGE |
+				HSTCGE | HSUXDMIPLLSE | SSTCGE | XHCFTCLKSE |
+				XHCBBTCGIPISO | XHCUSB2PLLSDLE | SSPLLSUE |
+				XHCBLCGE | HSLTCGE | SSLTCGE | IOSFBTCGE |
+				IOSFGBLCGE;
+			write32(cfg, reg);
+		}
    }
 }