Change in coreboot[master]: soc/intel/apollolake: Disable XHCI LFPS power management

[Marx Wang (Code Review)]

Marx Wang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40255 )

Change subject: soc/intel/apollolake: Disable XHCI LFPS power management
......................................................................

soc/intel/apollolake: Disable XHCI LFPS power management

Provide the option to disable XHCI LFPS power managemnt.
If the option is set in device tress, the bit[7:4] in XHCI
MMIO BAR + offset 0x80A4(PMCTRL_REG) will be updated from
default 9 to 0

BUG=b:146768983
BRANCH=None
TEST=build coreboot with DisableXhciLfpsPM being set to 1 and flash
     the image to the device. Run following command to check if
     bit[7:4] is set 0:
     >iotools mmio_read32 "XHCI MMIO BAR + 0x80A4"

Signed-off-by: Marx Wang 
Change-Id: Ic603e3b919d8b443c6ede8bb5e46e2de07fcb856
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/chip.h
2 files changed, 48 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/40255/1

diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index 2c8737f..5633493 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -99,6 +99,10 @@
 /* IOSF Gasket Backbone Local Clock Gating Enable */
 #define IOSFGBLCGE		(1 << 0)

+#define CFG_XHCPMCTRL           0x80a4
+/* BIT[7:4] LFPS periodic sampling for USB3 Ports */
+#define LFPS_PM_DISABLE_MASK    0xFFFFFF0F
+
 const char *soc_acpi_name(const struct device *dev)
 {
    if (dev->path.type == DEVICE_PATH_DOMAIN)
@@ -830,6 +834,37 @@
    return !!dev->enabled;
 }

+static void config_xhci_lfps_pm(void)
+{
+	static struct soc_intel_apollolake_config *cfg;
+	struct device *dev = SA_DEV_ROOT;
+
+	if (!dev || !dev->chip_info) {
+		printk(BIOS_ERR, "BUG! Could not find SOC devicetree config\n");
+		return;
+	}
+
+	cfg = dev->chip_info;
+
+	if (cfg->DisableXhciLfpsPM)
+	{
+		uint32_t *addr;
+		const struct resource *res;
+		uint32_t reg;
+		struct device *xhci_dev = PCH_DEV_XHCI;
+
+		res = find_resource(xhci_dev, PCI_BASE_ADDRESS_0);
+		addr = (void *)(uintptr_t)(res->base + CFG_XHCPMCTRL);
+		reg = read32(addr);
+		printk(BIOS_INFO, "XHCI PM control reg=0x%x.\n", reg);
+		if (reg){
+			reg &= LFPS_PM_DISABLE_MASK;
+			write32(cfg, reg);
+			printk(BIOS_INFO, "XHCI PM control reg updated to=0x%x.\n", reg);
+		}
+	}
+}
+
 void platform_fsp_notify_status(enum fsp_notify_phase phase)
 {
    if (phase == END_OF_FIRMWARE) {
@@ -877,6 +912,11 @@
    			IOSFGBLCGE;
    		write32(cfg, reg);
    	}
+
+		/*
+		 * Disable XHCI LFPS power management if the option in dev tree is set
+		 */
+		config_xhci_lfps_pm();
    }
 }

diff --git a/src/soc/intel/apollolake/chip.h b/src/soc/intel/apollolake/chip.h
index c7974a6..00d5764 100644
--- a/src/soc/intel/apollolake/chip.h
+++ b/src/soc/intel/apollolake/chip.h
@@ -184,6 +184,14 @@
     * the Upd parameter VtdEnable.
     */
    uint8_t enable_vtd;
+
+	/* Options to disable control the OFF time for the LFPS periodic sampling
+	 * for USB3 Ports. Default is 9: OFF time is 9ms
+	 *
+	 * Set 1 to update XHCI host MMIO BAR + PMCTRL_REG(0x80A4 bits[7:4)] to 0
+	 * 0:FALSE(Default), 1:True.
+	*/
+	uint8_t DisableXhciLfpsPM;
 };

typedef struct soc_intel_apollolake_config config_t;

[build bot (Jenkins) (Code Review)]

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

Change subject: soc/intel/apollolake: Disable XHCI LFPS power management
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40255/1/src/soc/intel/apollolake/ch... 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/c/coreboot/+/40255/1/src/soc/intel/apollolake/ch... 
PS1, Line 849: 	if (cfg->DisableXhciLfpsPM)
that open brace { should be on the previous line

https://review.coreboot.org/c/coreboot/+/40255/1/src/soc/intel/apollolake/ch... 
PS1, Line 860: 		if (reg){
space required before the open brace '{'

[Marx Wang (Code Review)]

Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40255 )

Change subject: soc/intel/apollolake: Disable XHCI LFPS power management
......................................................................

Patch Set 20:

(13 comments)

This change is ready for review.

https://review.coreboot.org/c/coreboot/+/40255/15//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40255/15//COMMIT_MSG@9 
PS15, Line 9: managemnt
> management

Done

https://review.coreboot.org/c/coreboot/+/40255/15//COMMIT_MSG@10 
PS15, Line 10: bit
> bits?

Done

https://review.coreboot.org/c/coreboot/+/40255/15//COMMIT_MSG@10 
PS15, Line 10: device tress
> the devicetree

Done

https://review.coreboot.org/c/coreboot/+/40255/15//COMMIT_MSG@11 
PS15, Line 11: 0x80A4(PMCTRL_REG)
> Please add a space before (.

Done

https://review.coreboot.org/c/coreboot/+/40255/19/src/mainboard/google/octop... 
File src/mainboard/google/octopus/variants/bobba/variant.c:

https://review.coreboot.org/c/coreboot/+/40255/19/src/mainboard/google/octop... 
PS19, Line 81: NULL
> Dead assignment (gets overwritten on the line just below)

it's variable initialization. i think each variable has initial value is a good practice no matter it's overwritten or not.

https://review.coreboot.org/c/coreboot/+/40255/15/src/soc/intel/apollolake/c... 
File src/soc/intel/apollolake/chip.h:

https://review.coreboot.org/c/coreboot/+/40255/15/src/soc/intel/apollolake/c... 
PS15, Line 189: 	 *  Default is 9 meaning periodic sampling interval is 9ms
> Remove space in the beginning? […]

Done

https://review.coreboot.org/c/coreboot/+/40255/15/src/soc/intel/apollolake/c... 
PS15, Line 191: PMCTRL_REG(0x80A4 bits[7:4)]
> Space and )] are switched.

Done

https://review.coreboot.org/c/coreboot/+/40255/15/src/soc/intel/apollolake/c... 
PS15, Line 192: 	 * 0:FALSE(Default), 1:True.
> Space and lowercase *default*.

Done

https://review.coreboot.org/c/coreboot/+/40255/15/src/soc/intel/apollolake/c... 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/c/coreboot/+/40255/15/src/soc/intel/apollolake/c... 
PS15, Line 102: #define CFG_XHCPMCTRL           0x80a4
> Align with tabs as above.

Done

https://review.coreboot.org/c/coreboot/+/40255/15/src/soc/intel/apollolake/c... 
PS15, Line 837: config_xhci_lfps_pm
> Rename to `disable_xhce_lfps_pm()`?

Done

https://review.coreboot.org/c/coreboot/+/40255/15/src/soc/intel/apollolake/c... 
PS15, Line 852: BIOS_INFO
> BIOS_DEBUG?

Done

https://review.coreboot.org/c/coreboot/+/40255/15/src/soc/intel/apollolake/c... 
PS15, Line 858: 	}
> A BIOS_INFO message would be nice to have. Something to start with: […]

Done

https://review.coreboot.org/c/coreboot/+/40255/15/src/soc/intel/apollolake/c... 
PS15, Line 911: 		 */
> One line.

Done

[Marx Wang (Code Review)]

Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40255 )

Change subject: soc/intel/apollolake: Disable XHCI LFPS power management
......................................................................

Patch Set 20:

(3 comments)

https://review.coreboot.org/c/coreboot/+/40255/19/src/soc/intel/apollolake/c... 
File src/soc/intel/apollolake/chip.h:

https://review.coreboot.org/c/coreboot/+/40255/19/src/soc/intel/apollolake/c... 
PS19, Line 189: 	 * Default is 9 meaning periodic sampling interval is 9ms.
> Default value... […]

yes, that's much better to describe it.

https://review.coreboot.org/c/coreboot/+/40255/19/src/soc/intel/apollolake/c... 
PS19, Line 193: */
> Add a space to align this

Done

https://review.coreboot.org/c/coreboot/+/40255/19/src/soc/intel/apollolake/c... 
PS19, Line 194: 	uint8_t DisableXhciLfpsPM;
> Maybe use `bool` instead? And since this is not a FSP UPD, I think we should use snake_case for it: […]

1. referring to other variables in "chip.h" such as "enable_vtd" which is also "uint8_t". i think to align it will be better?
2. yes, disable_xhci_lfps_pm is better as it's not FSP USD.

[Marx Wang (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Angel Pons, Kane Chen, Andrey Petrov, Patrick Rudolph,

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

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

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

Change subject: soc/intel/apollolake: Disable XHCI LFPS power management
......................................................................

soc/intel/apollolake: Disable XHCI LFPS power management

Provide the option to disable XHCI LFPS power management.
If the option is set in the devicetree, the bits[7:4] in
XHCI MMIO BAR + offset 0x80A4 (PMCTRL_REG) will be updated
from default 9 to 0.

BUG=b:146768983
BRANCH=None
TEST=build coreboot with DisableXhciLfpsPM being set to 1 and flash
     the image to the device. Run following command to check if
     bits[7:4] is set 0:
     >iotools mmio_read32 "XHCI MMIO BAR + 0x80A4"

Signed-off-by: Marx Wang 
Change-Id: Ic603e3b919d8b443c6ede8bb5e46e2de07fcb856
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/chip.h
2 files changed, 39 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/40255/21

[Marx Wang (Code Review)]

Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40255 )

Change subject: soc/intel/apollolake: Disable XHCI LFPS power management
......................................................................

Patch Set 21:

(3 comments)

https://review.coreboot.org/c/coreboot/+/40255/19/src/soc/intel/apollolake/c... 
File src/soc/intel/apollolake/chip.h:

https://review.coreboot.org/c/coreboot/+/40255/19/src/soc/intel/apollolake/c... 
PS19, Line 192:  * 0:FALSE (default), 1:True.
> I think this should be clear enough if using a `bool` type (see comment below)

proposed to change it to:
0:Enable (default), 1:Disable

https://review.coreboot.org/c/coreboot/+/40255/19/src/soc/intel/apollolake/c... 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/c/coreboot/+/40255/19/src/soc/intel/apollolake/c... 
PS19, Line 844: uint32_t
> void

Done

https://review.coreboot.org/c/coreboot/+/40255/19/src/soc/intel/apollolake/c... 
PS19, Line 856: configured
> to make it shorter, maybe use "set" instead?

Done

[Angel Pons (Code Review)]

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

Change subject: soc/intel/apollolake: Disable XHCI LFPS power management
......................................................................

Patch Set 21: Code-Review+2

(2 comments)

https://review.coreboot.org/c/coreboot/+/40255/19/src/mainboard/google/octop... 
File src/mainboard/google/octopus/variants/bobba/variant.c:

https://review.coreboot.org/c/coreboot/+/40255/19/src/mainboard/google/octop... 
PS19, Line 81: NULL
> it's variable initialization. […]

Well, it will make scan-build complain about it. Since the code was moved to another change, marking as resolved

https://review.coreboot.org/c/coreboot/+/40255/19/src/mainboard/google/octop... 
PS19, Line 85: cfg != NULL
> `cfg != NULL` is the same as just `cfg`

Ack

[Angel Pons (Code Review)]

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

Change subject: soc/intel/apollolake: Disable XHCI LFPS power management
......................................................................

Patch Set 21:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40255/19/src/soc/intel/apollolake/c... 
File src/soc/intel/apollolake/chip.h:

https://review.coreboot.org/c/coreboot/+/40255/19/src/soc/intel/apollolake/c... 
PS19, Line 192:  * 0:FALSE (default), 1:True.
> proposed to change it to: […]

Looks good, thanks.

https://review.coreboot.org/c/coreboot/+/40255/19/src/soc/intel/apollolake/c... 
PS19, Line 194: 	uint8_t DisableXhciLfpsPM;
> referring to other variables in "chip.h" such as "enable_vtd" which is also "uint8_t". […]

Well, a bool is a typedef for an uint8_t, so they should be equivalent. In any case, since even ints are used for boolean values (`int dptf_enable`), this can be changed in a follow-up.

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/apollolake: Disable XHCI LFPS power management
......................................................................

soc/intel/apollolake: Disable XHCI LFPS power management

Provide the option to disable XHCI LFPS power management.
If the option is set in the devicetree, the bits[7:4] in
XHCI MMIO BAR + offset 0x80A4 (PMCTRL_REG) will be updated
from default 9 to 0.

BUG=b:146768983
BRANCH=None
TEST=build coreboot with DisableXhciLfpsPM being set to 1 and flash
     the image to the device. Run following command to check if
     bits[7:4] is set 0:
     >iotools mmio_read32 "XHCI MMIO BAR + 0x80A4"

Signed-off-by: Marx Wang 
Change-Id: Ic603e3b919d8b443c6ede8bb5e46e2de07fcb856
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40255
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/chip.h
2 files changed, 39 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index 1075642..c4e068d 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -99,6 +99,10 @@
 /* IOSF Gasket Backbone Local Clock Gating Enable */
 #define IOSFGBLCGE		(1 << 0)

+#define CFG_XHCPMCTRL		0x80a4
+/* BIT[7:4] LFPS periodic sampling for USB3 Ports */
+#define LFPS_PM_DISABLE_MASK    0xFFFFFF0F
+
 const char *soc_acpi_name(const struct device *dev)
 {
    if (dev->path.type == DEVICE_PATH_DOMAIN)
@@ -829,6 +833,30 @@
    return !!dev->enabled;
 }

+static void disable_xhci_lfps_pm(void)
+{
+	struct soc_intel_apollolake_config *cfg;
+
+	cfg = config_of_soc();
+
+	if (cfg->disable_xhci_lfps_pm) {
+		void *addr;
+		const struct resource *res;
+		uint32_t reg;
+		struct device *xhci_dev = PCH_DEV_XHCI;
+
+		res = find_resource(xhci_dev, PCI_BASE_ADDRESS_0);
+		addr = (void *)(uintptr_t)(res->base + CFG_XHCPMCTRL);
+		reg = read32(addr);
+		printk(BIOS_DEBUG, "XHCI PM: control reg=0x%x.\n", reg);
+		if (reg) {
+			reg &= LFPS_PM_DISABLE_MASK;
+			write32(addr, reg);
+			printk(BIOS_INFO, "XHCI PM: Disable xHCI LFPS as configured in devicetree.\n");
+		}
+	}
+}
+
 void platform_fsp_notify_status(enum fsp_notify_phase phase)
 {
    if (phase == END_OF_FIRMWARE) {
@@ -876,6 +904,9 @@
    			IOSFGBLCGE;
    		write32(cfg, reg);
    	}
+
+		/* Disable XHCI LFPS power management if the option in dev tree is set. */
+		disable_xhci_lfps_pm();
    }
 }

diff --git a/src/soc/intel/apollolake/chip.h b/src/soc/intel/apollolake/chip.h
index c7974a6..ac36b70 100644
--- a/src/soc/intel/apollolake/chip.h
+++ b/src/soc/intel/apollolake/chip.h
@@ -184,6 +184,14 @@
     * the Upd parameter VtdEnable.
     */
    uint8_t enable_vtd;
+
+	/* Options to disable the LFPS periodic sampling for USB3 Ports.
+	 * Default value of PMCTRL_REG bits[7:4] is 9 which means periodic sampling
+	 * interval is 9ms.
+	 * Set 1 to update XHCI host MMIO BAR + PMCTRL_REG (0x80A4 bits[7:4]) to 0
+	 * 0:Enable (default), 1:Disable.
+	 */
+	uint8_t disable_xhci_lfps_pm;
 };

typedef struct soc_intel_apollolake_config config_t;

[Marx Wang (Code Review)]

Marx Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40255 )

Change subject: soc/intel/apollolake: Disable XHCI LFPS power management
......................................................................

Patch Set 22: Code-Review+1