Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Add North XHCI block driver ......................................................................
soc/intel/common: Add North XHCI block driver
Add a driver for the North XHCI block, which is part of the Type-C Subsystem. This driver only adds support for logging wake events from USB2 and USB3 ports associated with the North XHCI.
Change-Id: I9f28354e031e3eda587f4faf8ef7595dce8b33ea Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A src/soc/intel/common/block/include/intelblocks/north_xhci.h A src/soc/intel/common/block/north_xhci/Kconfig A src/soc/intel/common/block/north_xhci/Makefile.inc A src/soc/intel/common/block/north_xhci/elog.c 4 files changed, 138 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/47411/1
diff --git a/src/soc/intel/common/block/include/intelblocks/north_xhci.h b/src/soc/intel/common/block/include/intelblocks/north_xhci.h new file mode 100644 index 0000000..a561847 --- /dev/null +++ b/src/soc/intel/common/block/include/intelblocks/north_xhci.h @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef SOC_INTEL_COMMON_BLOCK_NORTH_XHCI_H +#define SOC_INTEL_COMMON_BLOCK_NORTH_XHCI_H + +#include <stdint.h> +#include <types.h> + +struct north_xhci_usb_info { + uint32_t usb2_port_status_reg; + uint32_t num_usb2_ports; + uint32_t usb3_port_status_reg; + uint32_t num_usb3_ports; +}; + +/* + * soc_get_north_xhci_usb_info() - Get the information about North USB2 & USB3 ports. + * + * This function is used to get USB ports and status register offset information within a North + * XHCI controller. + * + * Return: North USB ports and status register offset info for the SoC. + */ +const struct north_xhci_usb_info *soc_get_north_xhci_usb_info(void); + +/* + * pch_xhci_update_wake_event() - Identify and log North XHCI wake events. + * @info: Information about number of North USB ports and their status reg offset. + * + * This function goes through individual USB port status registers within the North XHCI block + * and identifies if any of those USB ports triggered a wake-up and logs information about those + * ports to the event log. + * + * Returns true if a North XHCI USB port was the source of a wake. + */ +bool north_xhci_update_wake_event(const struct north_xhci_usb_info *info); + +#endif /* SOC_INTEL_COMMON_BLOCK_NORTH_XHCI_H */ diff --git a/src/soc/intel/common/block/north_xhci/Kconfig b/src/soc/intel/common/block/north_xhci/Kconfig new file mode 100644 index 0000000..0517d25 --- /dev/null +++ b/src/soc/intel/common/block/north_xhci/Kconfig @@ -0,0 +1,6 @@ +config SOC_INTEL_COMMON_BLOCK_NORTH_XHCI_ELOG + bool + default n + help + Set this option to identify if the North XHCI caused a wake and log + that information to the event log. diff --git a/src/soc/intel/common/block/north_xhci/Makefile.inc b/src/soc/intel/common/block/north_xhci/Makefile.inc new file mode 100644 index 0000000..9ac3048 --- /dev/null +++ b/src/soc/intel/common/block/north_xhci/Makefile.inc @@ -0,0 +1,2 @@ +#ramstage-$(SOC_INTEL_COMMON_BLOCK_NORTH_XHCI_ELOG) += elog.c +ramstage-y += elog.c diff --git a/src/soc/intel/common/block/north_xhci/elog.c b/src/soc/intel/common/block/north_xhci/elog.c new file mode 100644 index 0000000..5006318 --- /dev/null +++ b/src/soc/intel/common/block/north_xhci/elog.c @@ -0,0 +1,92 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <arch/io.h> +#include <device/pci_ops.h> +#include <elog.h> +#include <intelblocks/north_xhci.h> +#include <soc/pci_devs.h> +#include <stdint.h> +#include <types.h> + +/* Port Link State */ +#define PORTSC_PLS_MASK 0xf0 +#define PORTSC_PLS_RESUME 0xf0 +/* Connect Status Change */ +#define PORTSC_CSC BIT(17) +/* Port Link State Change */ +#define PORTSC_PLC BIT(22) +/* Wake On Connect Enable */ +#define PORTSC_WCE BIT(25) +/* Wake On Disconnect Enable */ +#define PORTSC_WDE BIT(26) + +/* MMIO space between PORTSC registers */ +#define NEXT_PORT_OFFSET 0x10 + +static bool is_connect_wake_capable(uint32_t portsc) +{ + return !!(portsc & (PORTSC_WDE | PORTSC_WCE)); +} + +static bool connect_status_changed(uint32_t portsc) +{ + return !!(portsc & PORTSC_CSC); +} + +static bool is_link_state_changed(uint32_t portsc) +{ + return !!(portsc & PORTSC_PLC); +} + +static bool link_status_is_resume(uint32_t portsc) +{ + return (portsc & PORTSC_PLS_MASK) == PORTSC_PLS_RESUME; +} + +static bool log_port_wakes(uintptr_t base, uint8_t num, uint8_t event) +{ + uint32_t portsc; + unsigned int i; + bool found = false; + + /* For each port, if it is: + * 1) Capable of wakes on connect/disconnect and connect status changed, or: + * 2) Link state changed and its new state is a resume (indicating USB activity), + * then add a wake event to the elog. + */ + for (i = 0; i < num; i++, base += NEXT_PORT_OFFSET) { + portsc = read32((void *)base); + if (portsc == 0xffffffff) + continue; + + if ((is_connect_wake_capable(portsc) && connect_status_changed(portsc)) || + (is_link_state_changed(portsc) && link_status_is_resume(portsc))) { + elog_add_event_wake(event, i + 1); + found = true; + } + } + + return found; +} + +bool north_xhci_update_wake_event(const struct north_xhci_usb_info *info) +{ + const struct device *tcss_xhci = pcidev_path_on_root(SA_DEV_TCSS_XHCI); + bool found = false; + + if (!tcss_xhci) + return false; + + const uintptr_t mmio_base = pci_read_config32(tcss_xhci, PCI_BASE_ADDRESS_0) & + ~PCI_BASE_ADDRESS_MEM_ATTR_MASK; + + found |= log_port_wakes(mmio_base + info->usb2_port_status_reg, + info->num_usb2_ports, + ELOG_WAKE_SOURCE_PME_TCSS_XHCI_USB_2); + + found |= log_port_wakes(mmio_base + info->usb3_port_status_reg, + info->num_usb3_ports, + ELOG_WAKE_SOURCE_PME_TCSS_XHCI_USB_3); + + return found; +}
Hello Patrick Georgi, Martin Roth, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47411
to look at the new patch set (#2).
Change subject: soc/intel/common: Add North XHCI block driver ......................................................................
soc/intel/common: Add North XHCI block driver
Add a driver for the North XHCI block, which is part of the Type-C Subsystem. This driver only adds support for logging wake events from USB2 and USB3 ports associated with the North XHCI.
Change-Id: I9f28354e031e3eda587f4faf8ef7595dce8b33ea Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A src/soc/intel/common/block/include/intelblocks/north_xhci.h A src/soc/intel/common/block/north_xhci/Kconfig A src/soc/intel/common/block/north_xhci/Makefile.inc A src/soc/intel/common/block/north_xhci/elog.c 4 files changed, 137 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/47411/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Add North XHCI block driver ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47411/2/src/soc/intel/common/block/... File src/soc/intel/common/block/north_xhci/elog.c:
PS2: Isn't this exactly the same as common/block/xhci/elog.c? Can we not reuse the same for TCSS XHCI as well?
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Caveh Jalali, Nick Vaccaro, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47411
to look at the new patch set (#3).
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
soc/intel/common: Adapt XHCI elog driver for reuse
Currently this XHCI driver assumes the PCH XHCI controller, but the TCSS or North XHCI block has a similar enough PCI MMIO structure to make this code mostly reusable.
1) Rename everything to drop the `pch_` prefix 2) xhci_update_wake_event() now takes in a pci_devfn_t for the XHCI controller
Change-Id: I9f28354e031e3eda587f4faf8ef7595dce8b33ea Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/apollolake/elog.c M src/soc/intel/cannonlake/elog.c M src/soc/intel/common/block/include/intelblocks/xhci.h M src/soc/intel/common/block/xhci/elog.c M src/soc/intel/jasperlake/elog.c M src/soc/intel/skylake/elog.c M src/soc/intel/tigerlake/elog.c 7 files changed, 52 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/47411/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/xhci.h:
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... PS3, Line 45: void If you pass in pci_devfn_t here, then a separate `soc_get_north_xhci_usb_info()` shouldn't be required. Basically, the driver doesn't have to care about where the controller lives as long as it can use xhci_devfn as the handle.
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... File src/soc/intel/common/block/xhci/elog.c:
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... PS3, Line 125: ELOG_WAKE_SOURCE_PME_XHCI_USB_3 I think this will have to be accepted as an input from the caller.
BTW, this makes me think - should we just reuse ELOG_WAKE_SOURCE_PME_XHCI_USB_3 and ELOG_WAKE_SOURCE_PME_XHCI_USB_2 and differentiate using XHCI controller?
i.e. eventlog would look something like this 8 | ... | S0ix Enter 9 | ... | S0ix Exit 10 | ... | Wake Source | PME - XHCI | 0 11 | ... | Wake Source | PME - XHCI (USB 2.0 port) | 8 12 | ... | S0ix Enter 13 | ... | S0ix Exit 14 | ... | Wake Source | PME - TCSS XHCI | 0 15 | ... | Wake Source | PME - XHCI (USB 2.0 port) | 2 16 | ... | Wake Source | PME - XHCI (USB 3.0 port) | 3
It will probably require passing in a table of XHCI controllers from SoC to this driver { { PCH_DEVFN_XHCI, ELOG_WAKE_SOURCE_PME_XHCI}, { PCH_DEVFN_TCSS_XHCI, ELOG_WAKE_SOURCE_PME_TCSS_XHCI}, }
and this driver can loop over each entry: 1. Get SoC USB2/3 port info (BTW are there separate bits for USB4 ports?) 2. Check if any of the ports are wake sources. Log Information about the controller and the port.
What do you think?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/xhci.h:
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... PS3, Line 45: void
If you pass in pci_devfn_t here, then a separate `soc_get_north_xhci_usb_info()` shouldn't be requir […]
That works.
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... File src/soc/intel/common/block/xhci/elog.c:
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... PS3, Line 125: ELOG_WAKE_SOURCE_PME_XHCI_USB_3
I think this will have to be accepted as an input from the caller.
yes you're right
BTW, this makes me think - should we just reuse ELOG_WAKE_SOURCE_PME_XHCI_USB_3 and ELOG_WAKE_SOURCE_PME_XHCI_USB_2 and differentiate using XHCI controller?
i.e. eventlog would look something like this 8 | ... | S0ix Enter 9 | ... | S0ix Exit 10 | ... | Wake Source | PME - XHCI | 0 11 | ... | Wake Source | PME - XHCI (USB 2.0 port) | 8 12 | ... | S0ix Enter 13 | ... | S0ix Exit 14 | ... | Wake Source | PME - TCSS XHCI | 0 15 | ... | Wake Source | PME - XHCI (USB 2.0 port) | 2 16 | ... | Wake Source | PME - XHCI (USB 3.0 port) | 3
I think it's a little more clear if each line doesn't make assumptions about entries that have preceded it, so I would rather keep separate elog codes for `TCSS XHCI (USB 2.0 port) | 2` for example.
It will probably require passing in a table of XHCI controllers from SoC to this driver { { PCH_DEVFN_XHCI, ELOG_WAKE_SOURCE_PME_XHCI}, { PCH_DEVFN_TCSS_XHCI, ELOG_WAKE_SOURCE_PME_TCSS_XHCI},
As long as they are represented under the same PM bit, then yeah it'll work. I would also add in the usb2/usb3 port elog entries too as I mentioned above.
and this driver can loop over each entry:
- Get SoC USB2/3 port info (BTW are there separate bits for USB4 ports?)
I did look and I couldn't find any mention of power status for usb4 ports
- Check if any of the ports are wake sources. Log Information about the controller and the port.
What do you think?
SG
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... File src/soc/intel/common/block/xhci/elog.c:
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... PS3, Line 125: ELOG_WAKE_SOURCE_PME_XHCI_USB_3
I think it's a little more clear if each line doesn't make assumptions about entries that have preceded it, so I would rather keep separate elog codes for `TCSS XHCI (USB 2.0 port) | 2` for example.
I think in general it is okay as long as we can correctly associate the port with the controller i.e. print controller information in this driver where the port is being printed. Ultimately, it is useful for building the timeline, so as long as the semantics are consistent and clear, it should work fine.
The reason I mentioned it is if in a future Intel platform there is another XHCI controller, it will save us from having to allocate 2 more wake source types for its usb2/3 ports. I haven't seen multiple XHCI controllers in PCH on Intel platforms, but zork had 2 XHCI controllers (though it never enabled elog logging for USB ports).
I did look and I couldn't find any mention of power status for usb4 ports
Interesting. Probably something to check with Intel if it is just missing from the EDS.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Caveh Jalali, Nick Vaccaro, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47411
to look at the new patch set (#4).
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
soc/intel/common: Adapt XHCI elog driver for reuse
Currently this XHCI driver assumes the PCH XHCI controller, but the TCSS or North XHCI block has a similar enough PCI MMIO structure to make this code mostly reusable.
1) Rename everything to drop the `pch_` prefix 2) xhci_update_wake_event() now takes in a pci_devfn_t for the XHCI controller 3) soc_get_xhci_usb_info() also now takes a pci_devfn_t for the XHCI controller
Change-Id: I9f28354e031e3eda587f4faf8ef7595dce8b33ea Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/apollolake/elog.c M src/soc/intel/apollolake/xhci.c M src/soc/intel/cannonlake/elog.c M src/soc/intel/cannonlake/xhci.c M src/soc/intel/common/block/include/intelblocks/xhci.h M src/soc/intel/common/block/xhci/elog.c M src/soc/intel/common/block/xhci/xhci.c M src/soc/intel/jasperlake/elog.c M src/soc/intel/jasperlake/xhci.c M src/soc/intel/skylake/elog.c M src/soc/intel/skylake/xhci.c M src/soc/intel/tigerlake/elog.c M src/soc/intel/tigerlake/xhci.c 13 files changed, 138 insertions(+), 162 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/47411/4
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... File src/soc/intel/common/block/xhci/elog.c:
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... PS3, Line 125: ELOG_WAKE_SOURCE_PME_XHCI_USB_3
I think it's a little more clear if each line doesn't make assumptions about entries that have pre […]
b/173063895
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Caveh Jalali, Nick Vaccaro, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47411
to look at the new patch set (#5).
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
soc/intel/common: Adapt XHCI elog driver for reuse
Currently this XHCI driver assumes the PCH XHCI controller, but the TCSS or North XHCI block has a similar enough PCI MMIO structure to make this code mostly reusable.
1) Rename everything to drop the `pch_` prefix 2) xhci_update_wake_event() now takes in a pci_devfn_t for the XHCI controller 3) soc_get_xhci_usb_info() also now takes a pci_devfn_t for the XHCI controller
Change-Id: I9f28354e031e3eda587f4faf8ef7595dce8b33ea Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/apollolake/elog.c M src/soc/intel/apollolake/xhci.c M src/soc/intel/cannonlake/elog.c M src/soc/intel/cannonlake/xhci.c M src/soc/intel/common/block/include/intelblocks/xhci.h M src/soc/intel/common/block/xhci/elog.c M src/soc/intel/common/block/xhci/xhci.c M src/soc/intel/jasperlake/elog.c M src/soc/intel/jasperlake/xhci.c M src/soc/intel/skylake/elog.c M src/soc/intel/skylake/xhci.c M src/soc/intel/tigerlake/elog.c M src/soc/intel/tigerlake/xhci.c 13 files changed, 138 insertions(+), 162 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/47411/5
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... File src/soc/intel/common/block/xhci/elog.c:
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... PS3, Line 125: ELOG_WAKE_SOURCE_PME_XHCI_USB_3
b/173063895
USB4 status is very different from the others.... I think it will have to be a separate change.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... File src/soc/intel/common/block/xhci/elog.c:
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... PS3, Line 125: ELOG_WAKE_SOURCE_PME_XHCI_USB_3
USB4 status is very different from the others.... I think it will have to be a separate change.
Thanks for checking on the USB4 status, Tim. I agree it will have to be a separate change.
Going back to the original implementation for TCSS XHCI: Are you planning on reusing ELOG_WAKE_SOURCE_PME_XHCI_USB_3/ELOG_WAKE_SOURCE_PME_XHCI_USB_2 for TCSS?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... File src/soc/intel/common/block/xhci/elog.c:
https://review.coreboot.org/c/coreboot/+/47411/3/src/soc/intel/common/block/... PS3, Line 125: ELOG_WAKE_SOURCE_PME_XHCI_USB_3
Thanks for checking on the USB4 status, Tim. I agree it will have to be a separate change. […]
Yeah I'm OK with that, see latest patch set.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 5:
ping 😊
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 5: Code-Review+2
Nick Vaccaro has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Removed Code-Review+2 by Nick Vaccaro nvaccaro@google.com
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47411/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47411/5//COMMIT_MSG@18 PS5, Line 18: Is there a bug for this? TEST?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 5: Code-Review+1
(2 comments)
Overall LGTM. I think we only need to avoid the unconditional logging of wake source for controller.
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/xhci.h:
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... PS5, Line 36: and a usb_info : * struct. Leftover comment? Or were you intending to add usb_info pointer to the above structure as well?
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... File src/soc/intel/common/block/xhci/elog.c:
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... PS5, Line 111: elog_add_event_wake(wake_info[i].elog_wake_type_host, 0); Isn't this being logged unconditionally now?
The problem we run into here is: we cannot just rely on the XHCI controller PMCS since that gets cleared in case of S0ix. So, the host controller wake event needs to be logged only if any of the USB2 or USB3 ports are wake sources. That makes it a little icky.
xhci_stash_host_wake(wake_info[i].elog_wake_type_host);
and within xhci_port_wake_check(...), elog_add_event_wake() will have to be changed to: xhci_port_log_wake() which calls xhci_pop_host_wake() and if it is not -1 (or some undefined value), then log host wake first and then the port.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Caveh Jalali, Nick Vaccaro, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47411
to look at the new patch set (#6).
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
soc/intel/common: Adapt XHCI elog driver for reuse
Currently this XHCI driver assumes the PCH XHCI controller, but the TCSS or North XHCI block has a similar enough PCI MMIO structure to make this code mostly reusable.
1) Rename everything to drop the `pch_` prefix 2) xhci_update_wake_event() now takes in a pci_devfn_t for the XHCI controller 3) soc_get_xhci_usb_info() also now takes a pci_devfn_t for the XHCI controller
BUG=b:172279037
Change-Id: I9f28354e031e3eda587f4faf8ef7595dce8b33ea Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/apollolake/elog.c M src/soc/intel/apollolake/xhci.c M src/soc/intel/cannonlake/elog.c M src/soc/intel/cannonlake/xhci.c M src/soc/intel/common/block/include/intelblocks/xhci.h M src/soc/intel/common/block/xhci/elog.c M src/soc/intel/common/block/xhci/xhci.c M src/soc/intel/jasperlake/elog.c M src/soc/intel/jasperlake/xhci.c M src/soc/intel/skylake/elog.c M src/soc/intel/skylake/xhci.c M src/soc/intel/tigerlake/elog.c M src/soc/intel/tigerlake/xhci.c 13 files changed, 157 insertions(+), 163 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/47411/6
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/xhci.h:
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... PS5, Line 36: and a usb_info : * struct.
Leftover comment? Or were you intending to add usb_info pointer to the above structure as well?
leftover for now.
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... File src/soc/intel/common/block/xhci/elog.c:
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... PS5, Line 111: elog_add_event_wake(wake_info[i].elog_wake_type_host, 0);
Isn't this being logged unconditionally now? […]
Oops yes, missed the check, but yeah forgot the s0ix resume clearing the wake status bit. Agreed, it would be nice to get the host controller before the port.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 6: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47411/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47411/5//COMMIT_MSG@18 PS5, Line 18:
Is there a bug for this? TEST?
is the gsmi handler in the kernel fixed yet? the version I have on my volteer is still broken.
https://review.coreboot.org/c/coreboot/+/47411/2/src/soc/intel/common/block/... File src/soc/intel/common/block/north_xhci/elog.c:
PS2:
Isn't this exactly the same as common/block/xhci/elog. […]
Done
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... File src/soc/intel/common/block/xhci/elog.c:
https://review.coreboot.org/c/coreboot/+/47411/5/src/soc/intel/common/block/... PS5, Line 111: elog_add_event_wake(wake_info[i].elog_wake_type_host, 0);
Oops yes, missed the check, but yeah forgot the s0ix resume clearing the wake status bit. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47411/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47411/5//COMMIT_MSG@18 PS5, Line 18:
is the gsmi handler in the kernel fixed yet? the version I have on my volteer is still broken.
Yes, that was fixed a while back. https://crrev.com/c/2486920
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47411/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47411/5//COMMIT_MSG@18 PS5, Line 18:
Yes, that was fixed a while back. https://crrev. […]
Ok, I can update my system to a newer OS image tomorrow and test
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Caveh Jalali, Nick Vaccaro, Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47411
to look at the new patch set (#8).
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
soc/intel/common: Adapt XHCI elog driver for reuse
Currently this XHCI driver assumes the PCH XHCI controller, but the TCSS or North XHCI block has a similar enough PCI MMIO structure to make this code mostly reusable.
1) Rename everything to drop the `pch_` prefix 2) xhci_update_wake_event() now takes in a pci_devfn_t for the XHCI controller 3) soc_get_xhci_usb_info() also now takes a pci_devfn_t for the XHCI controller
BUG=b:172279037 TEST=plug in USB keyboard while in S0, enter S0ix and verify entry via EC; type on keyboard, verify it wakes up, eventlog contains: 39 | 2020-12-10 09:40:21 | S0ix Enter 40 | 2020-12-10 09:40:42 | S0ix Exit 41 | 2020-12-10 09:40:42 | Wake Source | PME - XHCI (USB 2.0 port) | 1 42 | 2020-12-10 09:40:42 | Wake Source | GPE # | 109 which verifies it still functions for the PCH XHCI controller
Change-Id: I9f28354e031e3eda587f4faf8ef7595dce8b33ea Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/apollolake/elog.c M src/soc/intel/apollolake/xhci.c M src/soc/intel/cannonlake/elog.c M src/soc/intel/cannonlake/xhci.c M src/soc/intel/common/block/include/intelblocks/xhci.h M src/soc/intel/common/block/xhci/elog.c M src/soc/intel/common/block/xhci/xhci.c M src/soc/intel/jasperlake/elog.c M src/soc/intel/jasperlake/xhci.c M src/soc/intel/skylake/elog.c M src/soc/intel/skylake/xhci.c M src/soc/intel/tigerlake/elog.c M src/soc/intel/tigerlake/xhci.c 13 files changed, 157 insertions(+), 163 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/47411/8
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47411/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47411/5//COMMIT_MSG@18 PS5, Line 18:
Ok, I can update my system to a newer OS image tomorrow and test
Done
Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47411 )
Change subject: soc/intel/common: Adapt XHCI elog driver for reuse ......................................................................
soc/intel/common: Adapt XHCI elog driver for reuse
Currently this XHCI driver assumes the PCH XHCI controller, but the TCSS or North XHCI block has a similar enough PCI MMIO structure to make this code mostly reusable.
1) Rename everything to drop the `pch_` prefix 2) xhci_update_wake_event() now takes in a pci_devfn_t for the XHCI controller 3) soc_get_xhci_usb_info() also now takes a pci_devfn_t for the XHCI controller
BUG=b:172279037 TEST=plug in USB keyboard while in S0, enter S0ix and verify entry via EC; type on keyboard, verify it wakes up, eventlog contains: 39 | 2020-12-10 09:40:21 | S0ix Enter 40 | 2020-12-10 09:40:42 | S0ix Exit 41 | 2020-12-10 09:40:42 | Wake Source | PME - XHCI (USB 2.0 port) | 1 42 | 2020-12-10 09:40:42 | Wake Source | GPE # | 109 which verifies it still functions for the PCH XHCI controller
Change-Id: I9f28354e031e3eda587f4faf8ef7595dce8b33ea Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/47411 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/apollolake/elog.c M src/soc/intel/apollolake/xhci.c M src/soc/intel/cannonlake/elog.c M src/soc/intel/cannonlake/xhci.c M src/soc/intel/common/block/include/intelblocks/xhci.h M src/soc/intel/common/block/xhci/elog.c M src/soc/intel/common/block/xhci/xhci.c M src/soc/intel/jasperlake/elog.c M src/soc/intel/jasperlake/xhci.c M src/soc/intel/skylake/elog.c M src/soc/intel/skylake/xhci.c M src/soc/intel/tigerlake/elog.c M src/soc/intel/tigerlake/xhci.c 13 files changed, 157 insertions(+), 163 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/intel/apollolake/elog.c b/src/soc/intel/apollolake/elog.c index 3e82c32..b65ab10 100644 --- a/src/soc/intel/apollolake/elog.c +++ b/src/soc/intel/apollolake/elog.c @@ -2,6 +2,7 @@
#include <cbmem.h> #include <console/console.h> +#include <device/pci_type.h> #include <elog.h> #include <intelblocks/pmclib.h> #include <intelblocks/xhci.h> @@ -24,6 +25,10 @@
static void pch_log_wake_source(struct chipset_power_state *ps) { + const struct xhci_wake_info xhci_wake_info[] = { + { PCH_DEVFN_XHCI, ELOG_WAKE_SOURCE_PME_XHCI }, + }; + /* Power Button */ if (ps->pm1_sts & PWRBTN_STS) elog_add_event_wake(ELOG_WAKE_SOURCE_PWRBTN, 0); @@ -42,7 +47,8 @@
/* XHCI */ if (ps->gpe0_sts[GPE0_A] & XHCI_PME_STS) - pch_xhci_update_wake_event(soc_get_xhci_usb_info()); + xhci_update_wake_event(xhci_wake_info, + ARRAY_SIZE(xhci_wake_info));
/* SMBUS Wake */ if (ps->gpe0_sts[GPE0_A] & SMB_WAK_STS) diff --git a/src/soc/intel/apollolake/xhci.c b/src/soc/intel/apollolake/xhci.c index 4584dc7..47156f4 100644 --- a/src/soc/intel/apollolake/xhci.c +++ b/src/soc/intel/apollolake/xhci.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <device/pci_type.h> #include <intelblocks/xhci.h>
#define XHCI_USB2_PORT_STATUS_REG 0x480 @@ -19,7 +20,8 @@ .num_usb3_ports = XHCI_USB3_PORT_NUM, };
-const struct xhci_usb_info *soc_get_xhci_usb_info(void) +const struct xhci_usb_info *soc_get_xhci_usb_info(pci_devfn_t xhci_dev) { + /* Apollo Lake only has one XHCI controller */ return &usb_info; } diff --git a/src/soc/intel/cannonlake/elog.c b/src/soc/intel/cannonlake/elog.c index 3600d76..104a78c 100644 --- a/src/soc/intel/cannonlake/elog.c +++ b/src/soc/intel/cannonlake/elog.c @@ -22,19 +22,6 @@
#define PME_STS_BIT (1 << 15)
-static void pch_log_add_elog_event(const struct pme_status_info *info) -{ - /* - * If wake source is XHCI, check for detailed wake source events on - * USB2/3 ports. - */ - if ((info->dev == PCH_DEV_XHCI) && - pch_xhci_update_wake_event(soc_get_xhci_usb_info())) - return; - - elog_add_event_wake(info->elog_event, 0); -} - static void pch_log_pme_internal_wake_source(void) { size_t i; @@ -46,12 +33,11 @@ uint16_t val; bool dev_found = false;
- struct pme_status_info pme_status_info[] = { + const struct pme_status_info pme_status_info[] = { { PCH_DEV_HDA, 0x54, ELOG_WAKE_SOURCE_PME_HDA }, { PCH_DEV_GBE, 0xcc, ELOG_WAKE_SOURCE_PME_GBE }, { PCH_DEV_SATA, 0x74, ELOG_WAKE_SOURCE_PME_SATA }, { PCH_DEV_CSE, 0x54, ELOG_WAKE_SOURCE_PME_CSE }, - { PCH_DEV_XHCI, 0x74, ELOG_WAKE_SOURCE_PME_XHCI }, { PCH_DEV_USBOTG, 0x84, ELOG_WAKE_SOURCE_PME_XDCI }, /* * The power management control/status register is not @@ -60,6 +46,9 @@ */ { PCH_DEV_CNViWIFI, 0xcc, ELOG_WAKE_SOURCE_PME_WIFI }, }; + const struct xhci_wake_info xhci_wake_info[] = { + { PCH_DEVFN_XHCI, ELOG_WAKE_SOURCE_PME_XHCI }, + };
for (i = 0; i < ARRAY_SIZE(pme_status_info); i++) { dev = pme_status_info[i].dev; @@ -71,19 +60,20 @@ if ((val == 0xFFFF) || !(val & PME_STS_BIT)) continue;
- pch_log_add_elog_event(&pme_status_info[i]); + elog_add_event_wake(pme_status_info[i].elog_event, 0); dev_found = true; }
/* - * If device is still not found, but the wake source is internal PME, - * try probing XHCI ports to see if any of the USB2/3 ports indicate - * that it was the wake source. This path would be taken in case of GSMI - * logging with S0ix where the pci_pm_resume_noirq runs and clears the - * PME_STS_BIT in controller register. + * Check the XHCI controllers' USB2 & USB3 ports for wake events. There + * are cases (GSMI logging for S0ix clears PME_STS_BIT) where the XHCI + * controller's PME_STS_BIT may have already been cleared, so the host + * controller wake wouldn't get logged here; therefore, the host + * controller wake event is logged before its corresponding port wake + * event is logged. */ - if (!dev_found) - dev_found = pch_xhci_update_wake_event(soc_get_xhci_usb_info()); + dev_found |= xhci_update_wake_event(xhci_wake_info, + ARRAY_SIZE(xhci_wake_info));
if (!dev_found) elog_add_event_wake(ELOG_WAKE_SOURCE_PME_INTERNAL, 0); diff --git a/src/soc/intel/cannonlake/xhci.c b/src/soc/intel/cannonlake/xhci.c index 46cc120..0fb0936 100644 --- a/src/soc/intel/cannonlake/xhci.c +++ b/src/soc/intel/cannonlake/xhci.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <device/pci_type.h> #include <intelblocks/xhci.h>
#define XHCI_USB2_PORT_STATUS_REG 0x480 @@ -14,7 +15,7 @@ .num_usb3_ports = XHCI_USB3_PORT_NUM, };
-const struct xhci_usb_info *soc_get_xhci_usb_info(void) +const struct xhci_usb_info *soc_get_xhci_usb_info(pci_devfn_t xhci_dev) { return &usb_info; } diff --git a/src/soc/intel/common/block/include/intelblocks/xhci.h b/src/soc/intel/common/block/include/intelblocks/xhci.h index 568d292..1adcbc0 100644 --- a/src/soc/intel/common/block/include/intelblocks/xhci.h +++ b/src/soc/intel/common/block/include/intelblocks/xhci.h @@ -4,6 +4,8 @@ #define SOC_INTEL_COMMON_BLOCK_XHCI_H
#include <device/device.h> +#include <elog.h> +#include <stdint.h>
/* * struct xhci_usb_info - Data containing number of USB ports & offset. @@ -20,7 +22,19 @@ };
/* - * pch_xhci_update_wake_event() - Identify and log XHCI wake events. + * struct xhci_wake_info - Relates an XHCI device to registers and wake types + * @xhci_dev: devfn of the XHCI device + * @elog_wake_type_host: the wake type for the controller device + */ +struct xhci_wake_info { + pci_devfn_t xhci_dev; + uint8_t elog_wake_type_host; +}; + +/* + * xhci_update_wake_event() - Identify and log XHCI wake events. + * @wake_info: A mapping of XHCI devfn to elog wake types + * @wake_info_count: Count of items in wake_info * @info: Information about number of USB ports and their status reg offset. * * This function goes through individual USB port status registers within the @@ -29,7 +43,8 @@ * * Return: True if any port is identified as a wake source, false if none. */ -bool pch_xhci_update_wake_event(const struct xhci_usb_info *info); +bool xhci_update_wake_event(const struct xhci_wake_info *wake_info, + size_t wake_info_count);
void soc_xhci_init(struct device *dev);
@@ -41,7 +56,7 @@ * * Return: USB ports and status register offset info for the SoC. */ -const struct xhci_usb_info *soc_get_xhci_usb_info(void); +const struct xhci_usb_info *soc_get_xhci_usb_info(pci_devfn_t xhci_dev);
/* * usb_xhci_disable_unused() - Disable unused USB devices diff --git a/src/soc/intel/common/block/xhci/elog.c b/src/soc/intel/common/block/xhci/elog.c index c3043cb..fb59897 100644 --- a/src/soc/intel/common/block/xhci/elog.c +++ b/src/soc/intel/common/block/xhci/elog.c @@ -20,23 +20,23 @@ #define XHCI_STATUS_PLS_MASK (0xF << XHCI_STATUS_PLS_SHIFT) #define XHCI_STATUS_PLS_RESUME (15 << XHCI_STATUS_PLS_SHIFT)
-static bool pch_xhci_csc_set(uint32_t port_status) +static bool xhci_csc_set(uint32_t port_status) { return !!(port_status & XHCI_STATUS_CSC); }
-static bool pch_xhci_wake_capable(uint32_t port_status) +static bool xhci_wake_capable(uint32_t port_status) { return !!((port_status & XHCI_STATUS_WCE) | (port_status & XHCI_STATUS_WDE)); }
-static bool pch_xhci_plc_set(uint32_t port_status) +static bool xhci_plc_set(uint32_t port_status) { return !!(port_status & XHCI_STATUS_PLC); }
-static bool pch_xhci_resume(uint32_t port_status) +static bool xhci_resume(uint32_t port_status) { return (port_status & XHCI_STATUS_PLS_MASK) == XHCI_STATUS_PLS_RESUME; } @@ -55,7 +55,7 @@ * true : Wake source was found. * false : Wake source was not found. */ -static bool pch_xhci_port_wake_check(uintptr_t base, uint8_t num, uint8_t event) +static bool xhci_port_wake_check(uintptr_t base, uint8_t num, uint8_t host_event, uint8_t event) { uint32_t i, port_status; bool found = false; @@ -73,8 +73,9 @@ * connect/disconnect to identify if the port caused wake * event for USB attach/detach. */ - if (pch_xhci_csc_set(port_status) && - pch_xhci_wake_capable(port_status)) { + if (xhci_csc_set(port_status) && + xhci_wake_capable(port_status)) { + elog_add_event_wake(host_event, 0); elog_add_event_wake(event, i + 1); found = true; continue; @@ -84,8 +85,9 @@ * Check if PLC is set and PLS indicates resume to identify if * the port caused wake event for USB activity. */ - if (pch_xhci_plc_set(port_status) && - pch_xhci_resume(port_status)) { + if (xhci_plc_set(port_status) && + xhci_resume(port_status)) { + elog_add_event_wake(host_event, 0); elog_add_event_wake(event, i + 1); found = true; } @@ -93,50 +95,35 @@ return found; }
-/* - * Update elog event and instance depending upon the USB2 port that caused - * the wake event. - * - * Return value: - * true = Indicates that USB2 wake event was found. - * false = Indicates that USB2 wake event was not found. - */ -static inline bool pch_xhci_usb2_update_wake_event(uintptr_t mmio_base, - const struct xhci_usb_info *info) +bool xhci_update_wake_event(const struct xhci_wake_info *wake_info, + size_t wake_info_count) { - return pch_xhci_port_wake_check(mmio_base + info->usb2_port_status_reg, - info->num_usb2_ports, - ELOG_WAKE_SOURCE_PME_XHCI_USB_2); -} - -/* - * Update elog event and instance depending upon the USB3 port that caused - * the wake event. - * - * Return value: - * true = Indicates that USB3 wake event was found. - * false = Indicates that USB3 wake event was not found. - */ -static inline bool pch_xhci_usb3_update_wake_event(uintptr_t mmio_base, - const struct xhci_usb_info *info) -{ - return pch_xhci_port_wake_check(mmio_base + info->usb3_port_status_reg, - info->num_usb3_ports, - ELOG_WAKE_SOURCE_PME_XHCI_USB_3); -} - -bool pch_xhci_update_wake_event(const struct xhci_usb_info *info) -{ + const struct xhci_usb_info *usb_info; uintptr_t mmio_base; bool event_found = false; - mmio_base = ALIGN_DOWN(pci_read_config32(PCH_DEV_XHCI, - PCI_BASE_ADDRESS_0), 16); + size_t i;
- if (pch_xhci_usb2_update_wake_event(mmio_base, info)) - event_found = true; + for (i = 0; i < wake_info_count; ++i) { + /* Assumes BAR0 is MBAR */ + mmio_base = pci_s_read_config32(wake_info[i].xhci_dev, + PCI_BASE_ADDRESS_0); + mmio_base &= ~PCI_BASE_ADDRESS_MEM_ATTR_MASK; + usb_info = soc_get_xhci_usb_info(wake_info[i].xhci_dev);
- if (pch_xhci_usb3_update_wake_event(mmio_base, info)) - event_found = true; + /* Check USB2 port status & control registers */ + if (xhci_port_wake_check(mmio_base + usb_info->usb2_port_status_reg, + usb_info->num_usb2_ports, + wake_info[i].elog_wake_type_host, + ELOG_WAKE_SOURCE_PME_XHCI_USB_2)) + event_found = true; + + /* Check USB3 port status & control registers */ + if (xhci_port_wake_check(mmio_base + usb_info->usb3_port_status_reg, + usb_info->num_usb3_ports, + wake_info[i].elog_wake_type_host, + ELOG_WAKE_SOURCE_PME_XHCI_USB_3)) + event_found = true; + }
return event_found; } diff --git a/src/soc/intel/common/block/xhci/xhci.c b/src/soc/intel/common/block/xhci/xhci.c index 47f2567..705fbb0 100644 --- a/src/soc/intel/common/block/xhci/xhci.c +++ b/src/soc/intel/common/block/xhci/xhci.c @@ -57,7 +57,7 @@ unsigned int port_id)) { struct device *xhci, *hub = NULL, *port = NULL; - const struct xhci_usb_info *info = soc_get_xhci_usb_info(); + const struct xhci_usb_info *info = soc_get_xhci_usb_info(PCH_DEVFN_XHCI); struct drivers_usb_acpi_config *config; bool enable;
diff --git a/src/soc/intel/jasperlake/elog.c b/src/soc/intel/jasperlake/elog.c index 5d3c69e..cb9625a 100644 --- a/src/soc/intel/jasperlake/elog.c +++ b/src/soc/intel/jasperlake/elog.c @@ -57,19 +57,6 @@ } }
-static void pch_log_add_elog_event(const struct pme_map *ipme_map) -{ - /* - * If wake source is XHCI, check for detailed wake source events on - * USB2/3 ports. - */ - if ((ipme_map->devfn == PCH_DEVFN_XHCI) && - pch_xhci_update_wake_event(soc_get_xhci_usb_info())) - return; - - elog_add_event_wake(ipme_map->wake_source, 0); -} - static void pch_log_pme_internal_wake_source(void) { size_t i; @@ -80,10 +67,12 @@ { PCH_DEVFN_GBE, ELOG_WAKE_SOURCE_PME_GBE }, { PCH_DEVFN_SATA, ELOG_WAKE_SOURCE_PME_SATA }, { PCH_DEVFN_CSE, ELOG_WAKE_SOURCE_PME_CSE }, - { PCH_DEVFN_XHCI, ELOG_WAKE_SOURCE_PME_XHCI }, { PCH_DEVFN_USBOTG, ELOG_WAKE_SOURCE_PME_XDCI }, { PCH_DEVFN_CNVI_WIFI, ELOG_WAKE_SOURCE_PME_WIFI }, }; + const struct xhci_wake_info xhci_wake_info[] = { + { PCH_DEVFN_XHCI, ELOG_WAKE_SOURCE_PME_XHCI }, + };
for (i = 0; i < ARRAY_SIZE(ipme_map); i++) { const struct device *dev = pcidev_path_on_root(ipme_map[i].devfn); @@ -91,20 +80,21 @@ continue;
if (pci_dev_is_wake_source(dev)) { - pch_log_add_elog_event(&ipme_map[i]); + elog_add_event_wake(ipme_map[i].wake_source, 0); dev_found = true; } }
/* - * If device is still not found, but the wake source is internal PME, - * try probing XHCI ports to see if any of the USB2/3 ports indicate - * that it was the wake source. This path would be taken in case of GSMI - * logging with S0ix where the pci_pm_resume_noirq runs and clears the - * PME_STS_BIT in controller register. + * Check the XHCI controllers' USB2 & USB3 ports for wake events. There + * are cases (GSMI logging for S0ix clears PME_STS_BIT) where the XHCI + * controller's PME_STS_BIT may have already been cleared, so the host + * controller wake wouldn't get logged here; therefore, the host + * controller wake event is logged before its corresponding port wake + * event is logged. */ - if (!dev_found) - dev_found = pch_xhci_update_wake_event(soc_get_xhci_usb_info()); + dev_found |= xhci_update_wake_event(xhci_wake_info, + ARRAY_SIZE(xhci_wake_info));
if (!dev_found) elog_add_event_wake(ELOG_WAKE_SOURCE_PME_INTERNAL, 0); diff --git a/src/soc/intel/jasperlake/xhci.c b/src/soc/intel/jasperlake/xhci.c index 424751e..eebacca 100644 --- a/src/soc/intel/jasperlake/xhci.c +++ b/src/soc/intel/jasperlake/xhci.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <device/pci_type.h> #include <intelblocks/xhci.h>
#define XHCI_USB2_PORT_STATUS_REG 0x480 @@ -14,7 +15,8 @@ .num_usb3_ports = XHCI_USB3_PORT_NUM, };
-const struct xhci_usb_info *soc_get_xhci_usb_info(void) +const struct xhci_usb_info *soc_get_xhci_usb_info(pci_devfn_t xhci_dev) { + /* Jasper Lake only has one XHCI controller */ return &usb_info; } diff --git a/src/soc/intel/skylake/elog.c b/src/soc/intel/skylake/elog.c index dee93d8..1332e2d 100644 --- a/src/soc/intel/skylake/elog.c +++ b/src/soc/intel/skylake/elog.c @@ -34,33 +34,22 @@
#define PME_STS_BIT (1 << 15)
-static void pch_log_add_elog_event(const struct pme_status_info *info) -{ - /* - * If wake source is XHCI, check for detailed wake source events on - * USB2/3 ports. - */ - if ((info->devfn == PCH_DEVFN_XHCI) && - pch_xhci_update_wake_event(soc_get_xhci_usb_info())) - return; - - elog_add_event_wake(info->elog_event, 0); -} - static void pch_log_pme_internal_wake_source(void) { size_t i; uint16_t val; bool dev_found = false;
- struct pme_status_info pme_status_info[] = { + const struct pme_status_info pme_status_info[] = { { PCH_DEVFN_HDA, 0x54, ELOG_WAKE_SOURCE_PME_HDA }, { PCH_DEVFN_GBE, 0xcc, ELOG_WAKE_SOURCE_PME_GBE }, { PCH_DEVFN_SATA, 0x74, ELOG_WAKE_SOURCE_PME_SATA }, { PCH_DEVFN_CSE, 0x54, ELOG_WAKE_SOURCE_PME_CSE }, - { PCH_DEVFN_XHCI, 0x74, ELOG_WAKE_SOURCE_PME_XHCI }, { PCH_DEVFN_USBOTG, 0x84, ELOG_WAKE_SOURCE_PME_XDCI }, }; + const struct xhci_wake_info xhci_wake_info[] = { + { PCH_DEVFN_XHCI, ELOG_WAKE_SOURCE_PME_XHCI }, + };
for (i = 0; i < ARRAY_SIZE(pme_status_info); i++) { pci_devfn_t dev = PCI_DEV(0, PCI_SLOT(pme_status_info[i].devfn), @@ -71,19 +60,20 @@ if ((val == 0xFFFF) || !(val & PME_STS_BIT)) continue;
- pch_log_add_elog_event(&pme_status_info[i]); + elog_add_event_wake(pme_status_info[i].elog_event, 0); dev_found = true; }
/* - * If device is still not found, but the wake source is internal PME, - * try probing XHCI ports to see if any of the USB2/3 ports indicate - * that it was the wake source. This path would be taken in case of GSMI - * logging with S0ix where the pci_pm_resume_noirq runs and clears the - * PME_STS_BIT in controller register. + * Check the XHCI controllers' USB2 & USB3 ports for wake events. There + * are cases (GSMI logging for S0ix clears PME_STS_BIT) where the XHCI + * controller's PME_STS_BIT may have already been cleared, so the host + * controller wake wouldn't get logged here; therefore, the host + * controller wake event is logged before its corresponding port wake + * event is logged. */ - if (!dev_found) - dev_found = pch_xhci_update_wake_event(soc_get_xhci_usb_info()); + dev_found |= xhci_update_wake_event(xhci_wake_info, + ARRAY_SIZE(xhci_wake_info));
if (!dev_found) elog_add_event_wake(ELOG_WAKE_SOURCE_PME_INTERNAL, 0); @@ -137,7 +127,7 @@ * Linux kernel uses PME STS bit information. So do not clear * this bit. */ - pch_log_add_elog_event(&pme_status_info[i]); + elog_add_event_wake(pme_status_info[i].elog_event, 0); } }
diff --git a/src/soc/intel/skylake/xhci.c b/src/soc/intel/skylake/xhci.c index 048f1f0..66edb3d 100644 --- a/src/soc/intel/skylake/xhci.c +++ b/src/soc/intel/skylake/xhci.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <device/pci_type.h> #include <intelblocks/xhci.h>
#define XHCI_USB2_PORT_STATUS_REG 0x480 @@ -14,7 +15,7 @@ .num_usb3_ports = XHCI_USB3_PORT_NUM, };
-const struct xhci_usb_info *soc_get_xhci_usb_info(void) +const struct xhci_usb_info *soc_get_xhci_usb_info(pci_devfn_t xhci_dev) { return &usb_info; } diff --git a/src/soc/intel/tigerlake/elog.c b/src/soc/intel/tigerlake/elog.c index 88324d6..531f5c1 100644 --- a/src/soc/intel/tigerlake/elog.c +++ b/src/soc/intel/tigerlake/elog.c @@ -57,54 +57,45 @@ } }
-static void pch_log_add_elog_event(const struct pme_map *ipme_map) -{ - /* - * If wake source is XHCI, check for detailed wake source events on - * USB2/3 ports. - */ - if ((ipme_map->devfn == PCH_DEVFN_XHCI) && - pch_xhci_update_wake_event(soc_get_xhci_usb_info())) - return; - - elog_add_event_wake(ipme_map->wake_source, 0); -} - static void pch_log_pme_internal_wake_source(void) { - size_t i; - bool dev_found = false; - const struct pme_map ipme_map[] = { { PCH_DEVFN_HDA, ELOG_WAKE_SOURCE_PME_HDA }, { PCH_DEVFN_GBE, ELOG_WAKE_SOURCE_PME_GBE }, { PCH_DEVFN_SATA, ELOG_WAKE_SOURCE_PME_SATA }, { PCH_DEVFN_CSE, ELOG_WAKE_SOURCE_PME_CSE }, - { PCH_DEVFN_XHCI, ELOG_WAKE_SOURCE_PME_XHCI }, { PCH_DEVFN_USBOTG, ELOG_WAKE_SOURCE_PME_XDCI }, { PCH_DEVFN_CNVI_WIFI, ELOG_WAKE_SOURCE_PME_WIFI }, }; + const struct xhci_wake_info xhci_wake_info[] = { + { PCH_DEVFN_XHCI, ELOG_WAKE_SOURCE_PME_XHCI }, + { SA_DEVFN_TCSS_XHCI, ELOG_WAKE_SOURCE_PME_TCSS_XHCI }, + }; + bool dev_found = false; + size_t i;
for (i = 0; i < ARRAY_SIZE(ipme_map); i++) { - const struct device *dev = pcidev_path_on_root(ipme_map[i].devfn); + const struct device *dev = + pcidev_path_on_root(ipme_map[i].devfn); if (!dev) continue;
if (pci_dev_is_wake_source(dev)) { - pch_log_add_elog_event(&ipme_map[i]); + elog_add_event_wake(ipme_map[i].wake_source, 0); dev_found = true; } }
/* - * If device is still not found, but the wake source is internal PME, - * try probing XHCI ports to see if any of the USB2/3 ports indicate - * that it was the wake source. This path would be taken in case of GSMI - * logging with S0ix where the pci_pm_resume_noirq runs and clears the - * PME_STS_BIT in controller register. + * Check the XHCI controllers' USB2 & USB3 ports for wake events. There + * are cases (GSMI logging for S0ix clears PME_STS_BIT) where the XHCI + * controller's PME_STS_BIT may have already been cleared, so the host + * controller wake wouldn't get logged here; therefore, the host + * controller wake event is logged before its corresponding port wake + * event is logged. */ - if (!dev_found) - dev_found = pch_xhci_update_wake_event(soc_get_xhci_usb_info()); + dev_found |= xhci_update_wake_event(xhci_wake_info, + ARRAY_SIZE(xhci_wake_info));
if (!dev_found) elog_add_event_wake(ELOG_WAKE_SOURCE_PME_INTERNAL, 0); diff --git a/src/soc/intel/tigerlake/xhci.c b/src/soc/intel/tigerlake/xhci.c index 18bb129..6f095fa 100644 --- a/src/soc/intel/tigerlake/xhci.c +++ b/src/soc/intel/tigerlake/xhci.c @@ -1,20 +1,39 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <device/pci_type.h> #include <intelblocks/xhci.h> +#include <soc/pci_devs.h>
-#define XHCI_USB2_PORT_STATUS_REG 0x480 -#define XHCI_USB3_PORT_STATUS_REG 0x520 -#define XHCI_USB2_PORT_NUM 10 -#define XHCI_USB3_PORT_NUM 4 +#define PCH_XHCI_USB2_PORT_STATUS_REG 0x480 +#define PCH_XHCI_USB3_PORT_STATUS_REG 0x520 +#define PCH_XHCI_USB2_PORT_NUM 10 +#define PCH_XHCI_USB3_PORT_NUM 4
-static const struct xhci_usb_info usb_info = { - .usb2_port_status_reg = XHCI_USB2_PORT_STATUS_REG, - .num_usb2_ports = XHCI_USB2_PORT_NUM, - .usb3_port_status_reg = XHCI_USB3_PORT_STATUS_REG, - .num_usb3_ports = XHCI_USB3_PORT_NUM, +#define TCSS_XHCI_USB2_PORT_STATUS_REG 0x480 +#define TCSS_XHCI_USB3_PORT_STATUS_REG 0x490 +#define TCSS_XHCI_USB2_PORT_NUM 1 +#define TCSS_XHCI_USB3_PORT_NUM 4 + +static const struct xhci_usb_info pch_usb_info = { + .usb2_port_status_reg = PCH_XHCI_USB2_PORT_STATUS_REG, + .num_usb2_ports = PCH_XHCI_USB2_PORT_NUM, + .usb3_port_status_reg = PCH_XHCI_USB3_PORT_STATUS_REG, + .num_usb3_ports = PCH_XHCI_USB3_PORT_NUM, };
-const struct xhci_usb_info *soc_get_xhci_usb_info(void) +static const struct xhci_usb_info tcss_usb_info = { + .usb2_port_status_reg = TCSS_XHCI_USB2_PORT_STATUS_REG, + .num_usb2_ports = TCSS_XHCI_USB2_PORT_NUM, + .usb3_port_status_reg = TCSS_XHCI_USB3_PORT_STATUS_REG, + .num_usb3_ports = TCSS_XHCI_USB3_PORT_NUM, +}; + +const struct xhci_usb_info *soc_get_xhci_usb_info(pci_devfn_t xhci_dev) { - return &usb_info; + if (xhci_dev == PCH_DEVFN_XHCI) + return &pch_usb_info; + else if (xhci_dev == SA_DEVFN_TCSS_XHCI) + return &tcss_usb_info; + + return NULL; }
-
Furquan Shaikh (Code Review) -
Nick Vaccaro (Code Review)
-
Tim Wawrzynczak (Code Review)