Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38494 )
Change subject: drivers/net/r8168: Add SSDT Power Resource Methods ......................................................................
drivers/net/r8168: Add SSDT Power Resource Methods
Turns out when going into S0ix we want the kernel to toggle de-assert to 0 for the ISOLATE# pin on the NIC for S0ix not to be woken by PCIe traffic on PCH. Upon resume the ISOLATE# pin on the NIC is then re-asserted for it to become lively again.
BUG=b:XXX BRANCH=none TEST=hopeful.
Change-Id: I3ae8dc30f45f55eec23f45e7b5fbc67a4542f87d Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/drivers/net/chip.h M src/drivers/net/r8168.c M src/mainboard/google/hatch/variants/puff/overridetree.cb 3 files changed, 42 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/38494/1
diff --git a/src/drivers/net/chip.h b/src/drivers/net/chip.h index 430bc33..3c47c01 100644 --- a/src/drivers/net/chip.h +++ b/src/drivers/net/chip.h @@ -19,6 +19,29 @@ struct drivers_net_config { uint16_t customized_leds; unsigned int wake; /* Wake pin for ACPI _PRW */ + + /* Does the device have a power resource? */ + bool has_power_resource; + + /* GPIO used to take device out of reset or to put it into reset. */ + struct acpi_gpio reset_gpio; + /* Delay to be inserted after device is taken out of reset. */ + unsigned int reset_delay_ms; + /* Delay to be inserted after device is put into reset. */ + unsigned int reset_off_delay_ms; + /* GPIO used to enable device. */ + struct acpi_gpio enable_gpio; + /* Delay to be inserted after device is enabled. */ + unsigned int enable_delay_ms; + /* Delay to be inserted after device is disabled. */ + unsigned int enable_off_delay_ms; + /* GPIO used to stop operation of device. */ + struct acpi_gpio stop_gpio; + /* Delay to be inserted after disabling stop. */ + unsigned int stop_delay_ms; + /* Delay to be inserted after enabling stop. */ + unsigned int stop_off_delay_ms; + /* * There maybe many NIC cards in a system. * This parameter is for driver to identify what diff --git a/src/drivers/net/r8168.c b/src/drivers/net/r8168.c index 1bca879..00c4eb3 100644 --- a/src/drivers/net/r8168.c +++ b/src/drivers/net/r8168.c @@ -317,6 +317,22 @@ if (dev->chip_ops) acpigen_write_name_string("_DDN", dev->chip_ops->name);
+ /* Power Resource */ + if (config->has_power_resource) { + const struct acpi_power_res_params power_res_params = { + &config->reset_gpio, + config->reset_delay_ms, + config->reset_off_delay_ms, + &config->enable_gpio, + config->enable_delay_ms, + config->enable_off_delay_ms, + &config->stop_gpio, + config->stop_delay_ms, + config->stop_off_delay_ms + }; + acpi_device_add_power_res(&power_res_params); + } + /* Address */ address = PCI_SLOT(dev->path.pci.devfn) & 0xffff; address <<= 16; diff --git a/src/mainboard/google/hatch/variants/puff/overridetree.cb b/src/mainboard/google/hatch/variants/puff/overridetree.cb index e7fe907..671e69f 100644 --- a/src/mainboard/google/hatch/variants/puff/overridetree.cb +++ b/src/mainboard/google/hatch/variants/puff/overridetree.cb @@ -270,6 +270,9 @@ device pci 1c.0 on chip drivers/net register "customized_leds" = "0x05af" + register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_A18)" + register "enable_delay_ms" = "1" # 90 ns + register "has_power_resource" = "1" device pci 00.0 on end end end # FSP requires func0 be enabled.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38494 )
Change subject: drivers/net/r8168: Add SSDT Power Resource Methods ......................................................................
Patch Set 4:
(1 comment)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/38494/1/src/drivers/net/chip.h File src/drivers/net/chip.h:
https://review.coreboot.org/c/coreboot/+/38494/1/src/drivers/net/chip.h@23 PS1, Line 23: /* Does the device have a power resource? */
Is clear from the variable name?
Ack
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38494 )
Change subject: drivers/net/r8168: Add SSDT Power Resource Methods ......................................................................
Patch Set 4:
Patch Set 4:
(1 comment)
This change is ready for review.
Edward O'Callaghan has removed Paul Menzel from this change. ( https://review.coreboot.org/c/coreboot/+/38494 )
Change subject: drivers/net/r8168: Add SSDT Power Resource Methods ......................................................................
Removed reviewer Paul Menzel.
Hello Kangheui Won, Daniel Kurtz, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38494
to look at the new patch set (#5).
Change subject: drivers/net/r8168: Add SSDT Power Resource Methods ......................................................................
drivers/net/r8168: Add SSDT Power Resource Methods
Turns out when going into S0ix we want the kernel to toggle de-assert to 0 for the ISOLATE# pin on the NIC for S0ix not to be woken by PCIe traffic on PCH. Upon resume the ISOLATE# pin on the NIC is then re-asserted for it to become lively again.
V.2: Ensure reset_gpio && enable_gpio are optional.
BUG=b:147026979 BRANCH=none TEST=Boot puff and do 100 cycles of S0ix.
Change-Id: I3ae8dc30f45f55eec23f45e7b5fbc67a4542f87d Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/arch/x86/acpi_device.c M src/drivers/net/chip.h M src/drivers/net/r8168.c M src/mainboard/google/hatch/variants/puff/overridetree.cb 4 files changed, 29 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/38494/5
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38494 )
Change subject: drivers/net/r8168: Add SSDT Power Resource Methods ......................................................................
Patch Set 5: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38494 )
Change subject: drivers/net/r8168: Add SSDT Power Resource Methods ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38494/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38494/5//COMMIT_MSG@9 PS5, Line 9: Turns out when going into S0ix we want the kernel to toggle de-assert to 0 for : the ISOLATE# pin on the NIC for S0ix not to be woken by PCIe traffic on PCH. Does this still allow wake from ethernet to work?
https://review.coreboot.org/c/coreboot/+/38494/5/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
PS5: Can you please push this change as a separate CL?
https://review.coreboot.org/c/coreboot/+/38494/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/overridetree.cb:
PS5: Can you please push the mainboard change as a separate CL?
Hello Kane Chen, Kangheui Won, Paul Menzel, Daniel Kurtz, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38494
to look at the new patch set (#6).
Change subject: drivers/net/r8168: Add SSDT Power Resource Methods ......................................................................
drivers/net/r8168: Add SSDT Power Resource Methods
Turns out when going into S0ix we want the kernel to toggle de-assert to 0 for the ISOLATE# pin on the NIC for S0ix not to be woken by PCIe traffic on PCH. Upon resume the ISOLATE# pin on the NIC is then re-asserted for it to become lively again.
V.2: Ensure reset_gpio && enable_gpio are optional.
BUG=b:147026979 BRANCH=none TEST=Boot puff and do 100 cycles of S0ix.
Change-Id: I3ae8dc30f45f55eec23f45e7b5fbc67a4542f87d Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/drivers/net/chip.h M src/drivers/net/r8168.c 2 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/38494/6
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38494 )
Change subject: drivers/net/r8168: Add SSDT Power Resource Methods ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38494/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38494/5//COMMIT_MSG@9 PS5, Line 9: Turns out when going into S0ix we want the kernel to toggle de-assert to 0 for : the ISOLATE# pin on the NIC for S0ix not to be woken by PCIe traffic on PCH.
Does this still allow wake from ethernet to work?
Although WoL still doesn't work the Realtek datasheet says that the wake pin is explicitly not affected by this. The intent is for the wake and isolate to be used in tandem.
https://review.coreboot.org/c/coreboot/+/38494/5/src/arch/x86/acpi_device.c File src/arch/x86/acpi_device.c:
PS5:
Can you please push this change as a separate CL?
Done
https://review.coreboot.org/c/coreboot/+/38494/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/overridetree.cb:
PS5:
Can you please push the mainboard change as a separate CL?
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38494 )
Change subject: drivers/net/r8168: Add SSDT Power Resource Methods ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38494 )
Change subject: drivers/net/r8168: Add SSDT Power Resource Methods ......................................................................
drivers/net/r8168: Add SSDT Power Resource Methods
Turns out when going into S0ix we want the kernel to toggle de-assert to 0 for the ISOLATE# pin on the NIC for S0ix not to be woken by PCIe traffic on PCH. Upon resume the ISOLATE# pin on the NIC is then re-asserted for it to become lively again.
V.2: Ensure reset_gpio && enable_gpio are optional.
BUG=b:147026979 BRANCH=none TEST=Boot puff and do 100 cycles of S0ix.
Change-Id: I3ae8dc30f45f55eec23f45e7b5fbc67a4542f87d Signed-off-by: Edward O'Callaghan quasisec@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38494 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/drivers/net/chip.h M src/drivers/net/r8168.c 2 files changed, 22 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/drivers/net/chip.h b/src/drivers/net/chip.h index 430bc33..249b80f 100644 --- a/src/drivers/net/chip.h +++ b/src/drivers/net/chip.h @@ -15,10 +15,22 @@ #define __DRIVERS_R8168_CHIP_H__
#include <stdint.h> +#include <arch/acpi_device.h>
struct drivers_net_config { uint16_t customized_leds; unsigned int wake; /* Wake pin for ACPI _PRW */ + + /* Does the device have a power resource? */ + bool has_power_resource; + + /* GPIO used to stop operation of device. */ + struct acpi_gpio stop_gpio; + /* Delay to be inserted after disabling stop. */ + unsigned int stop_delay_ms; + /* Delay to be inserted after enabling stop. */ + unsigned int stop_off_delay_ms; + /* * There maybe many NIC cards in a system. * This parameter is for driver to identify what diff --git a/src/drivers/net/r8168.c b/src/drivers/net/r8168.c index 1bca879..07069aa 100644 --- a/src/drivers/net/r8168.c +++ b/src/drivers/net/r8168.c @@ -317,6 +317,16 @@ if (dev->chip_ops) acpigen_write_name_string("_DDN", dev->chip_ops->name);
+ /* Power Resource */ + if (config->has_power_resource) { + const struct acpi_power_res_params power_res_params = { + .stop_gpio = &config->stop_gpio, + .stop_delay_ms = config->stop_delay_ms, + .stop_off_delay_ms = config->stop_off_delay_ms + }; + acpi_device_add_power_res(&power_res_params); + } + /* Address */ address = PCI_SLOT(dev->path.pci.devfn) & 0xffff; address <<= 16;
-
Edward O'Callaghan (Code Review) -
Furquan Shaikh (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)