Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
drivers/intel/usb4: Add driver for USB4 retimer device
The USB4 retimer device needs to declare a _DSM with specific functions that allow for GPIO control to turn on the power when an external device is not connected. This driver allows the mainboard to provide the GPIO that is connected to the power control.
Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: Icfb85dc3c0885d828aba3855a66109043250ab86 --- A src/drivers/intel/usb4/retimer/Kconfig A src/drivers/intel/usb4/retimer/Makefile.inc A src/drivers/intel/usb4/retimer/chip.h A src/drivers/intel/usb4/retimer/retimer.c 4 files changed, 164 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/44918/1
diff --git a/src/drivers/intel/usb4/retimer/Kconfig b/src/drivers/intel/usb4/retimer/Kconfig new file mode 100644 index 0000000..b2bd99c --- /dev/null +++ b/src/drivers/intel/usb4/retimer/Kconfig @@ -0,0 +1,2 @@ +config DRIVERS_INTEL_USB4_RETIMER + bool diff --git a/src/drivers/intel/usb4/retimer/Makefile.inc b/src/drivers/intel/usb4/retimer/Makefile.inc new file mode 100644 index 0000000..bca23aa --- /dev/null +++ b/src/drivers/intel/usb4/retimer/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_DRIVERS_INTEL_USB4_RETIMER) += retimer.c diff --git a/src/drivers/intel/usb4/retimer/chip.h b/src/drivers/intel/usb4/retimer/chip.h new file mode 100644 index 0000000..789d824 --- /dev/null +++ b/src/drivers/intel/usb4/retimer/chip.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef __DRIVERS_INTEL_USB4_RETIMER_H__ +#define __DRIVERS_INTEL_USB4_RETIMER_H__ + +#include <acpi/acpi_device.h> + +struct drivers_intel_usb4_retimer_config { + /* GPIO used to control power of retimer device. */ + struct acpi_gpio power_gpio; +}; + +#endif /* __DRIVERS_INTEL_USB4_RETIMER_H__ */ diff --git a/src/drivers/intel/usb4/retimer/retimer.c b/src/drivers/intel/usb4/retimer/retimer.c new file mode 100644 index 0000000..3613012 --- /dev/null +++ b/src/drivers/intel/usb4/retimer/retimer.c @@ -0,0 +1,148 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <acpi/acpigen.h> +#include <acpi/acpi_device.h> +#include <console/console.h> +#include <device/device.h> +#include <device/i2c_simple.h> +#include <device/path.h> +#include <gpio.h> +#include <string.h> +#include "chip.h" + +/* Unique ID for the retimer _DSM. */ +#define INTEL_USB4_RETIMER_DSM_UUID "61788900-C470-42BB-80F0-23A313864593" + +/* + * Arg0: UUID + * Arg1: Revision ID (set to 1) + * Arg2: Function Index + * 0: Query command implemented + * 1: Query force power enable state + * 2: Set force power state + * Arg3: A package containing parameters for the function specified + * by the UUID, revision ID and function index. + */ + +static void usb4_retimer_cb_standard_query(void *arg) +{ + /* + * ToInteger (Arg1, Local2) + * If (Local2 == 1) { + * Return(Buffer() {0x07}) + * } + * Return (Buffer() {0x01}) + */ + acpigen_write_to_integer(ARG1_OP, LOCAL2_OP); + acpigen_write_if_lequal_op_int(LOCAL2_OP, 1); + acpigen_write_return_singleton_buffer(0x07); + acpigen_pop_len(); /* If */ + acpigen_write_return_singleton_buffer(0x01); +} + +static void usb4_retimer_cb_get_power_state(void *arg) +{ + struct acpi_gpio *power_gpio = arg; + + /* + * // Read power gpio into Local0 + * Store (_SB.PCI0.GTXS (power_gpio), Local0) + * Return (Local0) + */ + acpigen_get_tx_gpio(power_gpio); + acpigen_write_return_op(LOCAL0_OP); +} + +static void usb4_retimer_cb_set_power_state(void *arg) +{ + struct acpi_gpio *power_gpio = arg; + + /* + * // Read power gpio into Local0 + * Store (_SB.PCI0.GTXS (power_gpio), Local0) + */ + acpigen_get_tx_gpio(power_gpio); + + /* + * // Get argument for on/off from Arg3[0] + * Local2 = DeRefOf (Arg3[0]) + */ + acpigen_get_package_op_element(ARG3_OP, 0, LOCAL2_OP); + + /* + * If (Local0 == Local2) { + * // Skip if already set + * Return (Zero) + * } + */ + acpigen_write_if_lequal_op_op(LOCAL0_OP, LOCAL2_OP); + acpigen_write_return_integer(0); + acpigen_pop_len(); + + /* + * If (Local0 == 0) { + * // Turn power off + * _SB.PCI0.CTXS (power_gpio) + * } + */ + acpigen_write_if_lequal_op_int(LOCAL0_OP, 0); + acpigen_disable_tx_gpio(power_gpio); + acpigen_pop_len(); + + /* + * If (Local0 == 1) { + * // Turn power on + * _SB.PCI0.STXS (power_gpio) + * } + */ + acpigen_write_if_lequal_op_int(LOCAL0_OP, 1); + acpigen_enable_tx_gpio(power_gpio); + acpigen_pop_len(); + + /* Return (Zero) */ + acpigen_write_return_integer(0); +} + +static void (*usb4_retimer_callbacks[3])(void *) = { + usb4_retimer_cb_standard_query, /* Function 0 */ + usb4_retimer_cb_get_power_state, /* Function 1 */ + usb4_retimer_cb_set_power_state, /* Function 2 */ +}; + +static void usb4_retimer_fill_ssdt(const struct device *dev) +{ + struct drivers_intel_usb4_retimer_config *config = dev->chip_info; + const char *scope = acpi_device_scope(dev); + + if (!dev->enabled || !scope) + return; + if (!config->power_gpio.pin_count) { + printk(BIOS_ERR, "%s: Power GPIO required for %s\n", __func__, dev_path(dev)); + return; + } + + /* Write the _DSM that toggles power with provided GPIO. */ + acpigen_write_scope(scope); + acpigen_write_dsm(INTEL_USB4_RETIMER_DSM_UUID, usb4_retimer_callbacks, + ARRAY_SIZE(usb4_retimer_callbacks), &config->power_gpio); + acpigen_pop_len(); /* Scope */ + + printk(BIOS_INFO, "%s: %s at %s\n", acpi_device_path(dev), dev->chip_ops->name, + dev_path(dev)); +} + +static struct device_operations usb4_retimer_dev_ops = { + .read_resources = noop_read_resources, + .set_resources = noop_set_resources, + .acpi_fill_ssdt = usb4_retimer_fill_ssdt, +}; + +static void usb4_retimer_enable(struct device *dev) +{ + dev->ops = &usb4_retimer_dev_ops; +} + +struct chip_operations drivers_intel_usb4_retimer_ops = { + CHIP_NAME("Intel USB4 Retimer") + .enable_dev = usb4_retimer_enable +};
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 1:
This needs to handle both active high and active low GPIOs. One of the changes I lost...
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/Kconfig:
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... PS1, Line 1: RETIMER Silly question: What's a retimer in this context? It would be nice to have a help text that explains a bit what a USB4 retimer is.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/Kconfig:
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... PS1, Line 1: RETIMER
Silly question: What's a retimer in this context? It would be nice to have a help text that explains […]
A retimer retransmits a fresh copy of the signal, by doing CDR and retransmitting the data (i.e., it is protocol-aware), https://www.asteralabs.com/2019/06/26/pci-express-retimers-vs-redrivers-an-e...
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/Kconfig:
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... PS1, Line 1: RETIMER
A retimer retransmits a fresh copy of the signal, by doing CDR and retransmitting the data (i.e. […]
Good read, thank you! The help text could be something like this, if the second sentence is accurate:
A retimer is a device that retransmits a fresh copy of the signal it receives, by doing CDR and retransmitting the data (i.e., it is protocol-aware). If your mainboard has a USB4 retimer (usually located close to the USB4 ports), then select this driver.
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... PS1, Line 82: /* : * If (Local0 == 0) { : * // Turn power off : * _SB.PCI0.CTXS (power_gpio) : * } : */ : acpigen_write_if_lequal_op_int(LOCAL0_OP, 0); : acpigen_disable_tx_gpio(power_gpio); : acpigen_pop_len(); : : /* : * If (Local0 == 1) { : * // Turn power on : * _SB.PCI0.STXS (power_gpio) : * } : */ : acpigen_write_if_lequal_op_int(LOCAL0_OP, 1); : acpigen_enable_tx_gpio(power_gpio); : acpigen_pop_len(); Don't we have a way to write an `Else` in acpigen? Or is it necessary to only handle 0 and 1?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/Kconfig:
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... PS1, Line 1: RETIMER
Good read, thank you! The help text could be something like this, if the second sentence is accurate […]
+1
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... PS1, Line 82: /* : * If (Local0 == 0) { : * // Turn power off : * _SB.PCI0.CTXS (power_gpio) : * } : */ : acpigen_write_if_lequal_op_int(LOCAL0_OP, 0); : acpigen_disable_tx_gpio(power_gpio); : acpigen_pop_len(); : : /* : * If (Local0 == 1) { : * // Turn power on : * _SB.PCI0.STXS (power_gpio) : * } : */ : acpigen_write_if_lequal_op_int(LOCAL0_OP, 1); : acpigen_enable_tx_gpio(power_gpio); : acpigen_pop_len();
Don't we have a way to write an `Else` in acpigen? Or is it necessary to only handle 0 and 1?
yes, there's acpigen_write_else(), but the result (Local0) is coming from the GTXS Method, which reads a GPIO and returns 0 or 1 (https://doc.coreboot.org/acpi/gpio.html)
Tim Wawrzynczak has uploaded a new patch set (#2) to the change originally created by Duncan Laurie. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
drivers/intel/usb4: Add driver for USB4 retimer device
The USB4 retimer device needs to declare a _DSM with specific functions that allow for GPIO control to turn on the power when an external device is not connected. This driver allows the mainboard to provide the GPIO that is connected to the power control.
Change-Id: Icfb85dc3c0885d828aba3855a66109043250ab86 Signed-off-by: Duncan Laurie dlaurie@google.com --- A src/drivers/intel/usb4/retimer/Kconfig A src/drivers/intel/usb4/retimer/Makefile.inc A src/drivers/intel/usb4/retimer/chip.h A src/drivers/intel/usb4/retimer/retimer.c 4 files changed, 164 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/44918/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 3:
Patch Set 1:
This needs to handle both active high and active low GPIOs. One of the changes I lost...
What am I missing? The acpigen_{tx,rx}_* functions already take care of the polarity
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 3:
could you add a doc node in Documentation/drivers?
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 1:
This needs to handle both active high and active low GPIOs. One of the changes I lost...
What am I missing? The acpigen_{tx,rx}_* functions already take care of the polarity
You are right. that means what I had before was not helping anyway.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44918/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44918/3//COMMIT_MSG@10 PS3, Line 10: on off ?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 1:
This needs to handle both active high and active low GPIOs. One of the changes I lost...
What am I missing? The acpigen_{tx,rx}_* functions already take care of the polarity
You are right. that means what I had before was not helping anyway.
Do you mean this is broken?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 3:
Patch Set 3:
could you add a doc node in Documentation/drivers?
will do
Tim Wawrzynczak has uploaded a new patch set (#4) to the change originally created by Duncan Laurie. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
drivers/intel/usb4: Add driver for USB4 retimer device
The USB4 retimer device needs to declare a _DSM with specific functions that allow for GPIO control to turn on the power when an external device is not connected. This driver allows the mainboard to provide the GPIO that is connected to the power control.
Change-Id: Icfb85dc3c0885d828aba3855a66109043250ab86 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A Documentation/drivers/retimer.md A src/drivers/intel/usb4/retimer/Kconfig A src/drivers/intel/usb4/retimer/Makefile.inc A src/drivers/intel/usb4/retimer/chip.h A src/drivers/intel/usb4/retimer/retimer.c 5 files changed, 210 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/44918/4
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 4:
ping!
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 4:
ping 😎
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 4: Code-Review+2
Tim Wawrzynczak has uploaded a new patch set (#5) to the change originally created by Duncan Laurie. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
drivers/intel/usb4: Add driver for USB4 retimer device
The USB4 retimer device needs to declare a _DSM with specific functions that allow for GPIO control to turn off the power when an external device is not connected. This driver allows the mainboard to provide the GPIO that is connected to the power control.
Change-Id: Icfb85dc3c0885d828aba3855a66109043250ab86 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A Documentation/drivers/retimer.md A src/drivers/intel/usb4/retimer/Kconfig A src/drivers/intel/usb4/retimer/Makefile.inc A src/drivers/intel/usb4/retimer/chip.h A src/drivers/intel/usb4/retimer/retimer.c 5 files changed, 210 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/44918/5
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44918/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44918/3//COMMIT_MSG@10 PS3, Line 10: on
off ?
Done
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/Kconfig:
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... PS1, Line 1: RETIMER
+1
Ack
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/Kconfig:
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 3: help depends on HAVE_ACPI_TABLES
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 7: device/i2c_simple.h Is this required?
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 65: / Read power gpio into Local0 Why is this required? Can we not simply make a call to enable/disable without looking at the current state? Setting to the same state should be harmless.
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 118: struct const
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 118: dev->chip_info config_of(dev)
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 7: device/i2c_simple.h
Is this required?
I think left over from when I was under the impression the retimer was attached to smbus based on the early documentation.
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 65: / Read power gpio into Local0
Why is this required? Can we not simply make a call to enable/disable without looking at the current […]
I don't think it hurts but could probably be skipped.
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 87: Local0 Hm this and the other one should be local2 I think
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 87: Local0
Hm this and the other one should be local2 I think
Yeah, that's right.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/44918/5/Documentation/drivers/retim... File Documentation/drivers/retimer.md:
https://review.coreboot.org/c/coreboot/+/44918/5/Documentation/drivers/retim... PS5, Line 4: grow increase
https://review.coreboot.org/c/coreboot/+/44918/5/Documentation/drivers/retim... PS5, Line 13: A redriver is a device that boosts the high-frequency content of a signal in nit: I'd place the `redriver` section before the `retimer` one. Then you can append these sentences:
Redrivers are not protocol-aware, which makes them relatively simple. However, their effectiveness is limited, and may not work at all in some scenarios.
And then, go on explaining the retimers. This helps one see the differences between a redriver and a retimer, and why using the latter can be necessary.
https://review.coreboot.org/c/coreboot/+/44918/5/Documentation/drivers/retim... PS5, Line 35: replacing the GPIO with the appropriate pin and polarity. In this case, "active high" means the retimer will be reset when the GPIO outputs a logic high signal. Or am I mistaken?
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 66: * Store (_SB.PCI0.GTXS (power_gpio), Local0) nit: use ASL 2.0 instead of `Store`
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 118: dev->chip_info
config_of(dev)
Is it a fatal error (i.e. booting can never succeed) to have a NULL config here? I don't think so.
Remember that `config_of` will die if either `dev` or `dev->chip_info` are NULL, which should never be the case but isn't a reason to cause a boot failure here. But do check that neither is NULL here.
Refer to CB:44476 and CB:46046 for a related discussion.
Tim Wawrzynczak has uploaded a new patch set (#6) to the change originally created by Duncan Laurie. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
drivers/intel/usb4: Add driver for USB4 retimer device
The USB4 retimer device needs to declare a _DSM with specific functions that allow for GPIO control to turn off the power when an external device is not connected. This driver allows the mainboard to provide the GPIO that is connected to the power control.
Change-Id: Icfb85dc3c0885d828aba3855a66109043250ab86 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A Documentation/drivers/retimer.md M Makefile.inc M src/Kconfig A src/drivers/intel/usb4/retimer/Kconfig A src/drivers/intel/usb4/retimer/Makefile.inc A src/drivers/intel/usb4/retimer/chip.h A src/drivers/intel/usb4/retimer/retimer.c 7 files changed, 212 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/44918/6
Tim Wawrzynczak has uploaded a new patch set (#7) to the change originally created by Duncan Laurie. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
drivers/intel/usb4: Add driver for USB4 retimer device
The USB4 retimer device needs to declare a _DSM with specific functions that allow for GPIO control to turn off the power when an external device is not connected. This driver allows the mainboard to provide the GPIO that is connected to the power control.
Change-Id: Icfb85dc3c0885d828aba3855a66109043250ab86 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A Documentation/drivers/retimer.md M Makefile.inc M src/Kconfig A src/drivers/intel/usb4/retimer/Kconfig A src/drivers/intel/usb4/retimer/Makefile.inc A src/drivers/intel/usb4/retimer/chip.h A src/drivers/intel/usb4/retimer/retimer.c 7 files changed, 200 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/44918/7
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 6:
(13 comments)
https://review.coreboot.org/c/coreboot/+/44918/5/Documentation/drivers/retim... File Documentation/drivers/retimer.md:
https://review.coreboot.org/c/coreboot/+/44918/5/Documentation/drivers/retim... PS5, Line 4: grow
increase
Done
https://review.coreboot.org/c/coreboot/+/44918/5/Documentation/drivers/retim... PS5, Line 13: A redriver is a device that boosts the high-frequency content of a signal in
nit: I'd place the `redriver` section before the `retimer` one. […]
Done
https://review.coreboot.org/c/coreboot/+/44918/5/Documentation/drivers/retim... PS5, Line 35: replacing the GPIO with the appropriate pin and polarity.
In this case, "active high" means the retimer will be reset when the GPIO outputs a logic high signa […]
Sorry, will reword, it's a "power" GPIO, not a reset GPIO, so when it's active, it's switching on power to the retimer.
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/Kconfig:
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 3: help
depends on HAVE_ACPI_TABLES
Done
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... PS1, Line 38: 0x07 Also this needs to be 0xF, bit 0 only indicates there are further bits set in the field (why it is set up that way though, IDK).
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... PS1, Line 40: 0x01 Likewise this needs to be 0x3
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... PS1, Line 82: /* : * If (Local0 == 0) { : * // Turn power off : * _SB.PCI0.CTXS (power_gpio) : * } : */ : acpigen_write_if_lequal_op_int(LOCAL0_OP, 0); : acpigen_disable_tx_gpio(power_gpio); : acpigen_pop_len(); : : /* : * If (Local0 == 1) { : * // Turn power on : * _SB.PCI0.STXS (power_gpio) : * } : */ : acpigen_write_if_lequal_op_int(LOCAL0_OP, 1); : acpigen_enable_tx_gpio(power_gpio); : acpigen_pop_len();
yes, there's acpigen_write_else(), but the result (Local0) is coming from the GTXS Method, which rea […]
I'll just switch to Else.
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 7: device/i2c_simple.h
I think left over from when I was under the impression the retimer was attached to smbus based on th […]
Done
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 65: / Read power gpio into Local0
I don't think it hurts but could probably be skipped.
Done
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 66: * Store (_SB.PCI0.GTXS (power_gpio), Local0)
nit: use ASL 2. […]
Done
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 87: Local0
Yeah, that's right.
Done
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 118: dev->chip_info
Is it a fatal error (i.e. booting can never succeed) to have a NULL config here? I don't think so. […]
I see, thanks for the pointers, Angel. However, you'll never run into this code if there is a valid chip driver (and hence even an empty chip_info, not NULL). IOW, the only way to get NULL is to add a chip driver that doesn't exist, and that should cause sconfig to throw an error during the build. Unless I'm misunderstanding something?
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 118: struct
const
Done
Tim Wawrzynczak has uploaded a new patch set (#12) to the change originally created by Duncan Laurie. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
drivers/intel/usb4: Add driver for USB4 retimer device
The USB4 retimer device needs to declare a _DSM with specific functions that allow for GPIO control to turn off the power when an external device is not connected. This driver allows the mainboard to provide the GPIO that is connected to the power control.
Change-Id: Icfb85dc3c0885d828aba3855a66109043250ab86 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A Documentation/drivers/retimer.md M Makefile.inc M src/Kconfig A src/drivers/intel/usb4/retimer/Kconfig A src/drivers/intel/usb4/retimer/Makefile.inc A src/drivers/intel/usb4/retimer/chip.h A src/drivers/intel/usb4/retimer/retimer.c 7 files changed, 200 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/44918/12
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... PS12, Line 39: 0x0F This doesn't match the comment above. Should be 0x7?
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... PS12, Line 43: 0x03 Doesn't match the comment. Should be 0x1?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... PS12, Line 39: 0x0F
This doesn't match the comment above. […]
When bit 0 is set, it means that "more bits follow":
``` Bit 0 indicates whether there is support for any functions other than function 0 for the specified UUID and Revision ID. If set to zero, no functions are supported (other than function zero) for the specified UUID and Revision ID. If set to one, at least one additional function is supported ```
We should make a macro for this b/c I've seen this come up in reviews before.
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... PS12, Line 43: 0x03
Doesn't match the comment. […]
same.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... PS12, Line 39: 0x0F
When bit 0 is set, it means that "more bits follow": […]
Why not adjust the function so that accounting for this bit is unnecessary?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... PS12, Line 39: 0x0F
When bit 0 is set, it means that "more bits follow":
Even in that case this will have to be 0x3 and not 0xF. There are 2 additional functions supported with revision 1 i.e. "Query force power enable state" and "Set force power state". So, this should be returning 0x3.
Why not adjust the function so that accounting for this bit is unnecessary?
That is a good idea. I think that will help eliminate some of the confusion. Basically, the query function will have to be implemented by the generic helper and it will have to take in sets of functions for each revision. Anyways, I think that should be done as a separate change.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... PS12, Line 39: 0x0F
When bit 0 is set, it means that "more bits follow": […]
Oops, I quoted it myself, and forgot about the standard query. Good idea Angel, that would keep me from making silly mistakes in the future 😋
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... PS12, Line 43: 0x03
same.
They support only get power state, but not set?
Tim Wawrzynczak has uploaded a new patch set (#13) to the change originally created by Duncan Laurie. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
drivers/intel/usb4: Add driver for USB4 retimer device
The USB4 retimer device needs to declare a _DSM with specific functions that allow for GPIO control to turn off the power when an external device is not connected. This driver allows the mainboard to provide the GPIO that is connected to the power control.
Change-Id: Icfb85dc3c0885d828aba3855a66109043250ab86 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A Documentation/drivers/retimer.md M Makefile.inc M src/Kconfig A src/drivers/intel/usb4/retimer/Kconfig A src/drivers/intel/usb4/retimer/Makefile.inc A src/drivers/intel/usb4/retimer/chip.h A src/drivers/intel/usb4/retimer/retimer.c 7 files changed, 200 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/44918/13
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... PS12, Line 43: 0x03
They support only get power state, but not set?
1 function beyond the standard query would be 0x3, if it's only the standard query, you can return 0 ("if [bit zero is] set to one, at least one additional function is supported"), meaning 0x1 would seem to be an invalid value here. Examples in the spec show returning 0 for just the query supported, and 3 returned for 1 function beyond the query. According to Intel's type-c acpi specs, other revs don't have other functions, so this should be 0.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 13:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... PS12, Line 43: 0x03
1 function beyond the standard query would be 0x3, if it's only the standard query, you can return 0 […]
Makes sense to me.
https://review.coreboot.org/c/coreboot/+/44918/13/src/drivers/intel/usb4/ret... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/13/src/drivers/intel/usb4/ret... PS13, Line 31: 0x07 0x03
https://review.coreboot.org/c/coreboot/+/44918/13/src/drivers/intel/usb4/ret... PS13, Line 33: 0x01 0x00
https://review.coreboot.org/c/coreboot/+/44918/13/src/drivers/intel/usb4/ret... PS13, Line 39: 0x07 0x03?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 13: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/44918/13/src/drivers/intel/usb4/ret... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/13/src/drivers/intel/usb4/ret... PS13, Line 31: 0x07
0x03
My bad. 0x07 is correct. Bit 0 indicates additional functions are supported and then each bit after that indicates support for the additional function.
https://review.coreboot.org/c/coreboot/+/44918/13/src/drivers/intel/usb4/ret... PS13, Line 39: 0x07
0x03?
Same as above.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/12/src/drivers/intel/usb4/ret... PS12, Line 39: 0x0F
Oops, I quoted it myself, and forgot about the standard query. […]
Ack
https://review.coreboot.org/c/coreboot/+/44918/13/src/drivers/intel/usb4/ret... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/13/src/drivers/intel/usb4/ret... PS13, Line 33: 0x01
0x00
Ack
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44918/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44918/5//COMMIT_MSG@13 PS5, Line 13: Is there a bug for this?
Tim Wawrzynczak has uploaded a new patch set (#14) to the change originally created by Duncan Laurie. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
drivers/intel/usb4: Add driver for USB4 retimer device
The USB4 retimer device needs to declare a _DSM with specific functions that allow for GPIO control to turn off the power when an external device is not connected. This driver allows the mainboard to provide the GPIO that is connected to the power control.
BUG=b:156957424
Change-Id: Icfb85dc3c0885d828aba3855a66109043250ab86 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A Documentation/drivers/retimer.md M Makefile.inc M src/Kconfig A src/drivers/intel/usb4/retimer/Kconfig A src/drivers/intel/usb4/retimer/Makefile.inc A src/drivers/intel/usb4/retimer/chip.h A src/drivers/intel/usb4/retimer/retimer.c 7 files changed, 200 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/44918/14
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44918/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44918/5//COMMIT_MSG@13 PS5, Line 13:
Is there a bug for this?
Done
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 14: Code-Review+2
Tim Wawrzynczak has uploaded a new patch set (#15) to the change originally created by Duncan Laurie. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
drivers/intel/usb4: Add driver for USB4 retimer device
The USB4 retimer device needs to declare a _DSM with specific functions that allow for GPIO control to turn off the power when an external device is not connected. This driver allows the mainboard to provide the GPIO that is connected to the power control.
BUG=b:156957424
Change-Id: Icfb85dc3c0885d828aba3855a66109043250ab86 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- A Documentation/drivers/retimer.md M Makefile.inc M src/Kconfig A src/drivers/intel/usb4/retimer/Kconfig A src/drivers/intel/usb4/retimer/Makefile.inc A src/drivers/intel/usb4/retimer/chip.h A src/drivers/intel/usb4/retimer/retimer.c 7 files changed, 200 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/44918/15
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 118: dev->chip_info
I see, thanks for the pointers, Angel. […]
Alright I read your arguments, I'm fine with that.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 15: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
drivers/intel/usb4: Add driver for USB4 retimer device
The USB4 retimer device needs to declare a _DSM with specific functions that allow for GPIO control to turn off the power when an external device is not connected. This driver allows the mainboard to provide the GPIO that is connected to the power control.
BUG=b:156957424
Change-Id: Icfb85dc3c0885d828aba3855a66109043250ab86 Signed-off-by: Duncan Laurie dlaurie@google.com Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/44918 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- A Documentation/drivers/retimer.md M Makefile.inc M src/Kconfig A src/drivers/intel/usb4/retimer/Kconfig A src/drivers/intel/usb4/retimer/Makefile.inc A src/drivers/intel/usb4/retimer/chip.h A src/drivers/intel/usb4/retimer/retimer.c 7 files changed, 200 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/Documentation/drivers/retimer.md b/Documentation/drivers/retimer.md new file mode 100644 index 0000000..d83b50b --- /dev/null +++ b/Documentation/drivers/retimer.md @@ -0,0 +1,40 @@ +# USB4 Retimers + +# Introduction +As USB speeds continue to increase (up to 5G, 10G, and even 20G or higher in +newer revisions of the spec), it becomes more difficult to maintain signal +integrity for longer traces. Devices such as retimers and redrivers can be used +to help signals maintain their integrity over long distances. + +A redriver is a device that boosts the high-frequency content of a signal in +order to compensate for the attenuation typically caused by travelling through +various circuit components (PCB, connectors, CPU, etc.). Redrivers are not +protocol-aware, which makes them relatively simple. However, their effectiveness +is limited, and may not work at all in some scenarios. + +A retimer is a device that retransmits a fresh copy of the signal it receives, +by doing CDR and retransmitting the data (i.e., it is protocol-aware). Since +this is a digital component, it may have firmware. + + +# Driver Usage + +Some operating systems may have the ability to update firmware on USB4 retimers, +and ultimately will need some way to power the device on and off so that its new +firmware can be loaded. This is achieved by providing a GPIO signal that can be +used for this purpose; its active state must be the one in which power is +applied to the retimer. This driver will generate the required ACPI AML code +which will toggle the GPIO in response to the kernel's request (through the +`_DSM` ACPI method). Simply put something like the following in your devicetree: + +``` +device pci 0.0 on + chip drivers/intel/usb4/retimer + register "power_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_A0)" + device generic 0 on end + end +end +``` + +replacing the GPIO with the appropriate pin and polarity. + diff --git a/Makefile.inc b/Makefile.inc index 882673b..297f7b1 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -80,7 +80,7 @@ subdirs-y += src/ec/acpi $(wildcard src/ec/*/*) $(wildcard src/southbridge/*/*) subdirs-y += $(wildcard src/soc/*/*) $(wildcard src/northbridge/*/*) subdirs-y += src/superio -subdirs-y += $(wildcard src/drivers/*) $(wildcard src/drivers/*/*) +subdirs-y += $(wildcard src/drivers/*) $(wildcard src/drivers/*/*) $(wildcard src/drivers/*/*/*) subdirs-y += src/cpu src/vendorcode subdirs-y += util/cbfstool util/sconfig util/nvramtool util/pgtblgen util/amdfwtool subdirs-y += util/futility util/marvell util/bincfg util/supermicro diff --git a/src/Kconfig b/src/Kconfig index 9cc9d31..d265da7 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -554,6 +554,7 @@ menu "Generic Drivers" source "src/drivers/*/Kconfig" source "src/drivers/*/*/Kconfig" +source "src/drivers/*/*/*/Kconfig" source "src/commonlib/storage/Kconfig" endmenu
diff --git a/src/drivers/intel/usb4/retimer/Kconfig b/src/drivers/intel/usb4/retimer/Kconfig new file mode 100644 index 0000000..eee8fe1 --- /dev/null +++ b/src/drivers/intel/usb4/retimer/Kconfig @@ -0,0 +1,8 @@ +config DRIVERS_INTEL_USB4_RETIMER + bool + depends on HAVE_ACPI_TABLES + help + A retimer is a device that retransmits a fresh copy of the signal it + receives, by doing CDR and retransmitting the data (i.e., it is + protocol-aware). If your mainboard has a USB4 retimer (usually + located close to the USB4 ports), then select this driver. diff --git a/src/drivers/intel/usb4/retimer/Makefile.inc b/src/drivers/intel/usb4/retimer/Makefile.inc new file mode 100644 index 0000000..bca23aa --- /dev/null +++ b/src/drivers/intel/usb4/retimer/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_DRIVERS_INTEL_USB4_RETIMER) += retimer.c diff --git a/src/drivers/intel/usb4/retimer/chip.h b/src/drivers/intel/usb4/retimer/chip.h new file mode 100644 index 0000000..789d824 --- /dev/null +++ b/src/drivers/intel/usb4/retimer/chip.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef __DRIVERS_INTEL_USB4_RETIMER_H__ +#define __DRIVERS_INTEL_USB4_RETIMER_H__ + +#include <acpi/acpi_device.h> + +struct drivers_intel_usb4_retimer_config { + /* GPIO used to control power of retimer device. */ + struct acpi_gpio power_gpio; +}; + +#endif /* __DRIVERS_INTEL_USB4_RETIMER_H__ */ diff --git a/src/drivers/intel/usb4/retimer/retimer.c b/src/drivers/intel/usb4/retimer/retimer.c new file mode 100644 index 0000000..be9ec35 --- /dev/null +++ b/src/drivers/intel/usb4/retimer/retimer.c @@ -0,0 +1,136 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <acpi/acpigen.h> +#include <acpi/acpi_device.h> +#include <console/console.h> +#include <device/device.h> +#include <device/path.h> +#include <gpio.h> +#include <string.h> +#include "chip.h" + +/* Unique ID for the retimer _DSM. */ +#define INTEL_USB4_RETIMER_DSM_UUID "61788900-C470-42BB-80F0-23A313864593" + +/* + * Arg0: UUID + * Arg1: Revision ID (set to 1) + * Arg2: Function Index + * 0: Query command implemented + * 1: Query force power enable state + * 2: Set force power state + * Arg3: A package containing parameters for the function specified + * by the UUID, revision ID and function index. + */ + +static void usb4_retimer_cb_standard_query(void *arg) +{ + /* + * ToInteger (Arg1, Local2) + * If (Local2 == 1) { + * Return(Buffer() {0x07}) + * } + * Return (Buffer() {0x01}) + */ + acpigen_write_to_integer(ARG1_OP, LOCAL2_OP); + + /* Revision 1 supports 2 Functions beyond the standard query */ + acpigen_write_if_lequal_op_int(LOCAL2_OP, 1); + acpigen_write_return_singleton_buffer(0x07); + acpigen_pop_len(); /* If */ + + /* Other revisions support no additional functions */ + acpigen_write_return_singleton_buffer(0); +} + +static void usb4_retimer_cb_get_power_state(void *arg) +{ + struct acpi_gpio *power_gpio = arg; + + /* + * // Read power gpio into Local0 + * Store (_SB.PCI0.GTXS (power_gpio), Local0) + * Return (Local0) + */ + acpigen_get_tx_gpio(power_gpio); + acpigen_write_return_op(LOCAL0_OP); +} + +static void usb4_retimer_cb_set_power_state(void *arg) +{ + struct acpi_gpio *power_gpio = arg; + + /* + * // Get argument for on/off from Arg3[0] + * Local0 = DeRefOf (Arg3[0]) + */ + acpigen_get_package_op_element(ARG3_OP, 0, LOCAL0_OP); + + /* + * If (Local0 == 0) { + * // Turn power off + * _SB.PCI0.CTXS (power_gpio) + * } + */ + acpigen_write_if_lequal_op_int(LOCAL0_OP, 0); + acpigen_disable_tx_gpio(power_gpio); + acpigen_pop_len(); /* If */ + + /* + * Else { + * // Turn power on + * _SB.PCI0.STXS (power_gpio) + * } + */ + acpigen_write_else(); + acpigen_enable_tx_gpio(power_gpio); + acpigen_pop_len(); + + /* Return (Zero) */ + acpigen_write_return_integer(0); +} + +static void (*usb4_retimer_callbacks[3])(void *) = { + usb4_retimer_cb_standard_query, /* Function 0 */ + usb4_retimer_cb_get_power_state, /* Function 1 */ + usb4_retimer_cb_set_power_state, /* Function 2 */ +}; + +static void usb4_retimer_fill_ssdt(const struct device *dev) +{ + const struct drivers_intel_usb4_retimer_config *config = dev->chip_info; + const char *scope = acpi_device_scope(dev); + + if (!dev->enabled || !scope || !config) + return; + + if (!config->power_gpio.pin_count) { + printk(BIOS_ERR, "%s: Power GPIO required for %s\n", __func__, dev_path(dev)); + return; + } + + /* Write the _DSM that toggles power with provided GPIO. */ + acpigen_write_scope(scope); + acpigen_write_dsm(INTEL_USB4_RETIMER_DSM_UUID, usb4_retimer_callbacks, + ARRAY_SIZE(usb4_retimer_callbacks), (void *)&config->power_gpio); + acpigen_pop_len(); /* Scope */ + + printk(BIOS_INFO, "%s: %s at %s\n", acpi_device_path(dev), dev->chip_ops->name, + dev_path(dev)); +} + +static struct device_operations usb4_retimer_dev_ops = { + .read_resources = noop_read_resources, + .set_resources = noop_set_resources, + .acpi_fill_ssdt = usb4_retimer_fill_ssdt, +}; + +static void usb4_retimer_enable(struct device *dev) +{ + dev->ops = &usb4_retimer_dev_ops; +} + +struct chip_operations drivers_intel_usb4_retimer_ops = { + CHIP_NAME("Intel USB4 Retimer") + .enable_dev = usb4_retimer_enable +};
-
Angel Pons (Code Review) -
Caveh Jalali (Code Review)
-
Duncan Laurie (Code Review)
-
Furquan Shaikh (Code Review)
-
Nick Vaccaro (Code Review)
-
Patrick Georgi (Code Review)
-
Stefan Reinauer (Code Review)
-
Tim Wawrzynczak (Code Review)