Change in coreboot[master]: drivers/intel/dptf: Generate ACPI identifiers only for enabled devices

List overview All Threads
Download

newer

older

[M] Change in coreboot[master]:...

Karthik Ramasubramanian (Code Review)

2 Nov 2020 2 Nov '20

11:42 p.m.

Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47148 )

Change subject: drivers/intel/dptf: Generate ACPI identifiers only for enabled devices ......................................................................

drivers/intel/dptf: Generate ACPI identifiers only for enabled devices

DPTF configuration can be applied based on firmware configuration. Hence generate ACPI identifiers only for enabled devices.

BUG=b:170229672 TEST=Build and boot to OS in Drawlat and Drawcia. Ensure that the ACPI identifier is enabled only for enabled devices.

Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/drivers/intel/dptf/dptf.c 1 file changed, 3 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/47148/1

diff --git a/src/drivers/intel/dptf/dptf.c b/src/drivers/intel/dptf/dptf.c index 0f1cc9c..e0e2202 100644 --- a/src/drivers/intel/dptf/dptf.c +++ b/src/drivers/intel/dptf/dptf.c @@ -232,6 +232,9 @@ { struct drivers_intel_dptf_config *config = config_of(dev);

+ if (!dev->enabled) + return; + write_device_definitions(dev); write_policies(config); write_controls(config);

Attachments:

attachment.htm (text/html — 2.8 KB)

Show replies by date

Tim Wawrzynczak (Code Review)

2 Nov 2 Nov

11:52 p.m.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47148 )

Change subject: drivers/intel/dptf: Generate ACPI identifiers only for enabled devices ......................................................................

Patch Set 1: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Mon, 02 Nov 2020 23:52:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

3 Nov 3 Nov

12:03 a.m.

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

Change subject: drivers/intel/dptf: Generate ACPI identifiers only for enabled devices ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47148/1/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:

https://review.coreboot.org/c/coreboot/+/47148/1/src/drivers/intel/dptf/dptf... PS1, Line 235: if (!dev->enabled) : return; : I wonder - shouldn't we fix this at the source? i.e. the caller should not make the `acpi_fill_ssdt` call if device is not enabled. I think we have been adding these checks for a while now, but it seems unnecessary within the individual drivers. With the check at the caller site, we can drop all the dev->enabled checks from all these individual drivers. Thoughts?

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Tue, 03 Nov 2020 00:03:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Tim Wawrzynczak (Code Review)

1:39 a.m.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47148 )

Change subject: drivers/intel/dptf: Generate ACPI identifiers only for enabled devices ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47148/1/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:

https://review.coreboot.org/c/coreboot/+/47148/1/src/drivers/intel/dptf/dptf... PS1, Line 235: if (!dev->enabled) : return; :

...

I wonder - shouldn't we fix this at the source? i.e. […]

SGTM, I sure hope no one is expecting ACPI tables to be generated for devices that are disabled 😊

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Nov 2020 01:39:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Karthik Ramasubramanian (Code Review)

5:12 a.m.

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47148 )

Change subject: drivers/intel/dptf: Generate ACPI identifiers only for enabled devices ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47148/1/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:

https://review.coreboot.org/c/coreboot/+/47148/1/src/drivers/intel/dptf/dptf... PS1, Line 235: if (!dev->enabled) : return; :

...

SGTM, I sure hope no one is expecting ACPI tables to be generated for devices that are disabled 😊

I will do it.

For the CL here: Intel has applied passive policy 2 to address the performance issues.

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 1 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Nov 2020 05:12:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: comment

Karthik Ramasubramanian (Code Review)

4 Nov 4 Nov

12:58 a.m.

New subject: Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Sumeet R Pawnikar, Patrick Rudolph,

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

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

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

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

acpi: Call acpi_fill_ssdt() only for enabled devices

Individual drivers check whether the concerned device is enabled before filling in the SSDT. Move the check before calling acpi_fill_ssdt() and remove the check in the individual drivers.

BUG=None TEST=util/abuild/abuild

Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/acpi/acpi.c M src/drivers/generic/adau7002/adau7002.c M src/drivers/generic/gpio_keys/gpio_keys.c M src/drivers/generic/max98357a/max98357a.c M src/drivers/gfx/generic/generic.c M src/drivers/i2c/da7219/da7219.c M src/drivers/i2c/designware/dw_i2c.c M src/drivers/i2c/generic/generic.c M src/drivers/i2c/gpiomux/bus/bus.c M src/drivers/i2c/gpiomux/mux/mux.c M src/drivers/i2c/max98373/max98373.c M src/drivers/i2c/max98390/max98390.c M src/drivers/i2c/max98927/max98927.c M src/drivers/i2c/nau8825/nau8825.c M src/drivers/i2c/rt1011/rt1011.c M src/drivers/i2c/rt5663/rt5663.c M src/drivers/i2c/sx9310/sx9310.c M src/drivers/i2c/tpm/chip.c M src/drivers/intel/ish/ish.c M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/pmc_mux/conn/conn.c M src/drivers/intel/soundwire/soundwire.c M src/drivers/intel/usb4/retimer/retimer.c M src/drivers/soundwire/alc5682/alc5682.c M src/drivers/soundwire/alc711/alc711.c M src/drivers/soundwire/max98373/max98373.c M src/drivers/spi/acpi/acpi.c M src/drivers/uart/acpi/acpi.c M src/drivers/usb/acpi/usb_acpi.c M src/drivers/wifi/generic/acpi.c M src/ec/google/chromeec/audio_codec/audio_codec.c M src/ec/google/chromeec/ec_acpi.c M src/ec/google/chromeec/i2c_tunnel/i2c_tunnel.c M src/ec/google/wilco/chip.c M src/soc/intel/common/block/scs/sd.c M src/soc/intel/common/block/usb4/pcie.c M src/soc/intel/common/block/usb4/usb4.c 37 files changed, 29 insertions(+), 56 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/47148/2

Karthik Ramasubramanian (Code Review)

1:01 a.m.

New subject: Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47148 )

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47148/1/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:

https://review.coreboot.org/c/coreboot/+/47148/1/src/drivers/intel/dptf/dptf... PS1, Line 235: if (!dev->enabled) : return; :

...

I will do it. […]

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Christian Walter christian.walter@9elements.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Nov 2020 01:01:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Comment-In-Reply-To: Karthik Ramasubramanian kramasub@google.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

5:14 a.m.

New subject: Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

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

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

Patch Set 2: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Christian Walter christian.walter@9elements.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Nov 2020 05:14:44 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Sumeet R Pawnikar (Code Review)

9:35 a.m.

New subject: Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47148 )

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

Patch Set 2: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Christian Walter christian.walter@9elements.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Nov 2020 09:35:12 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Tim Wawrzynczak (Code Review)

6:43 p.m.

New subject: Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47148 )

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

Patch Set 2: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Christian Walter christian.walter@9elements.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Nov 2020 18:43:04 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Angel Pons (Code Review)

10:01 p.m.

New subject: Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

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

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

Patch Set 2: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Christian Walter christian.walter@9elements.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Nov 2020 22:01:07 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Christian Walter (Code Review)

5 Nov 5 Nov

6:56 a.m.

New subject: Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47148 )

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

Patch Set 2: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 2 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Christian Walter christian.walter@9elements.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Thu, 05 Nov 2020 06:56:47 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Georgi (Code Review)

9 Nov 9 Nov

7:24 a.m.

New subject: Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

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

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

acpi: Call acpi_fill_ssdt() only for enabled devices

Individual drivers check whether the concerned device is enabled before filling in the SSDT. Move the check before calling acpi_fill_ssdt() and remove the check in the individual drivers.

BUG=None TEST=util/abuild/abuild

Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47148 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Christian Walter christian.walter@9elements.com --- M src/acpi/acpi.c M src/drivers/generic/adau7002/adau7002.c M src/drivers/generic/gpio_keys/gpio_keys.c M src/drivers/generic/max98357a/max98357a.c M src/drivers/gfx/generic/generic.c M src/drivers/i2c/da7219/da7219.c M src/drivers/i2c/designware/dw_i2c.c M src/drivers/i2c/generic/generic.c M src/drivers/i2c/gpiomux/bus/bus.c M src/drivers/i2c/gpiomux/mux/mux.c M src/drivers/i2c/max98373/max98373.c M src/drivers/i2c/max98390/max98390.c M src/drivers/i2c/max98927/max98927.c M src/drivers/i2c/nau8825/nau8825.c M src/drivers/i2c/rt1011/rt1011.c M src/drivers/i2c/rt5663/rt5663.c M src/drivers/i2c/sx9310/sx9310.c M src/drivers/i2c/tpm/chip.c M src/drivers/intel/ish/ish.c M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/pmc_mux/conn/conn.c M src/drivers/intel/soundwire/soundwire.c M src/drivers/intel/usb4/retimer/retimer.c M src/drivers/soundwire/alc5682/alc5682.c M src/drivers/soundwire/alc711/alc711.c M src/drivers/soundwire/max98373/max98373.c M src/drivers/spi/acpi/acpi.c M src/drivers/uart/acpi/acpi.c M src/drivers/usb/acpi/usb_acpi.c M src/drivers/wifi/generic/acpi.c M src/ec/google/chromeec/audio_codec/audio_codec.c M src/ec/google/chromeec/ec_acpi.c M src/ec/google/chromeec/i2c_tunnel/i2c_tunnel.c M src/ec/google/wilco/chip.c M src/soc/intel/common/block/scs/sd.c M src/soc/intel/common/block/usb4/pcie.c M src/soc/intel/common/block/usb4/usb4.c 37 files changed, 29 insertions(+), 56 deletions(-)

Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Sumeet R Pawnikar: Looks good to me, approved Angel Pons: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved Christian Walter: Looks good to me, approved

diff --git a/src/acpi/acpi.c b/src/acpi/acpi.c index 89f2a46..259813b 100644 --- a/src/acpi/acpi.c +++ b/src/acpi/acpi.c @@ -468,7 +468,7 @@ { struct device *dev; for (dev = all_devices; dev; dev = dev->next) - if (dev->ops && dev->ops->acpi_fill_ssdt) + if (dev->enabled && dev->ops && dev->ops->acpi_fill_ssdt) dev->ops->acpi_fill_ssdt(dev); current = (unsigned long) acpigen_get_current(); } diff --git a/src/drivers/generic/adau7002/adau7002.c b/src/drivers/generic/adau7002/adau7002.c index 88ee34d..26da09b 100644 --- a/src/drivers/generic/adau7002/adau7002.c +++ b/src/drivers/generic/adau7002/adau7002.c @@ -17,7 +17,7 @@ struct drivers_generic_adau7002_config *config; struct acpi_dp *dp;

- if (!dev || !dev->enabled) + if (!dev) return;

const char *scope = acpi_device_scope(dev); diff --git a/src/drivers/generic/gpio_keys/gpio_keys.c b/src/drivers/generic/gpio_keys/gpio_keys.c index 80ac407..3d273a0 100644 --- a/src/drivers/generic/gpio_keys/gpio_keys.c +++ b/src/drivers/generic/gpio_keys/gpio_keys.c @@ -57,7 +57,7 @@ const char *drv_string = config->is_polled ? "gpio-keys-polled" : "gpio-keys";

- if (!dev->enabled || !scope || !path || !config->gpio.pin_count) + if (!scope || !path || !config->gpio.pin_count) return;

/* Device */ diff --git a/src/drivers/generic/max98357a/max98357a.c b/src/drivers/generic/max98357a/max98357a.c index 44f8802..9ae4bf4 100644 --- a/src/drivers/generic/max98357a/max98357a.c +++ b/src/drivers/generic/max98357a/max98357a.c @@ -18,7 +18,7 @@ const char *path; struct acpi_dp *dp;

- if (!dev->enabled || !config) + if (!config) return;

const char *scope = acpi_device_scope(dev); diff --git a/src/drivers/gfx/generic/generic.c b/src/drivers/gfx/generic/generic.c index 546f30b..d3a4518 100644 --- a/src/drivers/gfx/generic/generic.c +++ b/src/drivers/gfx/generic/generic.c @@ -106,7 +106,7 @@

const char *scope = acpi_device_scope(dev);

- if (!scope || !dev->enabled) + if (!scope) return;

acpigen_write_scope(scope); diff --git a/src/drivers/i2c/da7219/da7219.c b/src/drivers/i2c/da7219/da7219.c index 1d98023..0423460 100644 --- a/src/drivers/i2c/da7219/da7219.c +++ b/src/drivers/i2c/da7219/da7219.c @@ -27,7 +27,7 @@ }; struct acpi_dp *dsd, *aad;

- if (!dev->enabled || !scope) + if (!scope) return;

/* Device */ diff --git a/src/drivers/i2c/designware/dw_i2c.c b/src/drivers/i2c/designware/dw_i2c.c index e01b5a8..3181e7a 100644 --- a/src/drivers/i2c/designware/dw_i2c.c +++ b/src/drivers/i2c/designware/dw_i2c.c @@ -824,9 +824,6 @@ const char *path; unsigned int speed;

- if (!dev->enabled) - return; - bus = dw_i2c_soc_dev_to_bus(dev);

if (bus < 0) diff --git a/src/drivers/i2c/generic/generic.c b/src/drivers/i2c/generic/generic.c index 18fd55c..cd74068 100644 --- a/src/drivers/i2c/generic/generic.c +++ b/src/drivers/i2c/generic/generic.c @@ -57,7 +57,7 @@ int reset_gpio_index = -1, enable_gpio_index = -1, irq_gpio_index = -1; const char *path = acpi_device_path(dev);

- if (!dev->enabled || !scope) + if (!scope) return;

if (!config->hid) { diff --git a/src/drivers/i2c/gpiomux/bus/bus.c b/src/drivers/i2c/gpiomux/bus/bus.c index 66aef8e..0bcf36a 100644 --- a/src/drivers/i2c/gpiomux/bus/bus.c +++ b/src/drivers/i2c/gpiomux/bus/bus.c @@ -22,7 +22,7 @@ const char *scope = acpi_device_scope(dev); const char *path = acpi_device_path(dev);

- if (!dev || !dev->enabled || !scope || !path) + if (!dev || !scope || !path) return;

/* Device */ diff --git a/src/drivers/i2c/gpiomux/mux/mux.c b/src/drivers/i2c/gpiomux/mux/mux.c index 66c8cc5..c1ae758 100644 --- a/src/drivers/i2c/gpiomux/mux/mux.c +++ b/src/drivers/i2c/gpiomux/mux/mux.c @@ -27,7 +27,7 @@ struct acpi_gpio_res_params param[MAX_NUM_MUX_GPIOS]; int i;

- if (!dev->enabled || !scope || !path) + if (!scope || !path) return;

/* Device */ diff --git a/src/drivers/i2c/max98373/max98373.c b/src/drivers/i2c/max98373/max98373.c index 1f8a4f3..b078674 100644 --- a/src/drivers/i2c/max98373/max98373.c +++ b/src/drivers/i2c/max98373/max98373.c @@ -24,7 +24,7 @@ }; struct acpi_dp *dp;

- if (!dev->enabled || !scope) { + if (!scope) { printk(BIOS_ERR, "%s: dev not enabled\n", __func__); return; } diff --git a/src/drivers/i2c/max98390/max98390.c b/src/drivers/i2c/max98390/max98390.c index 24c500b..c216391 100644 --- a/src/drivers/i2c/max98390/max98390.c +++ b/src/drivers/i2c/max98390/max98390.c @@ -28,7 +28,7 @@ struct acpi_dp *dp; uint64_t r0_value, temp_value;

- if (!dev->enabled || !scope) + if (!scope) return;

/* Device */ diff --git a/src/drivers/i2c/max98927/max98927.c b/src/drivers/i2c/max98927/max98927.c index 9429e4a..642eccf 100644 --- a/src/drivers/i2c/max98927/max98927.c +++ b/src/drivers/i2c/max98927/max98927.c @@ -24,7 +24,7 @@ }; struct acpi_dp *dp;

- if (!dev->enabled || !scope) + if (!scope) return;

/* Device */ diff --git a/src/drivers/i2c/nau8825/nau8825.c b/src/drivers/i2c/nau8825/nau8825.c index e995ebd..a0769d0 100644 --- a/src/drivers/i2c/nau8825/nau8825.c +++ b/src/drivers/i2c/nau8825/nau8825.c @@ -30,7 +30,7 @@ }; struct acpi_dp *dp = NULL;

- if (!dev->enabled || !scope) + if (!scope) return; if (config->sar_threshold_num > NAU8825_MAX_BUTTONS) return; diff --git a/src/drivers/i2c/rt1011/rt1011.c b/src/drivers/i2c/rt1011/rt1011.c index d1732f7..70f1881 100644 --- a/src/drivers/i2c/rt1011/rt1011.c +++ b/src/drivers/i2c/rt1011/rt1011.c @@ -28,7 +28,7 @@ struct acpi_dp *dp; uint64_t r0_value, temp_value;

- if (!dev->enabled || !scope) + if (!scope) return;

/* Device */ diff --git a/src/drivers/i2c/rt5663/rt5663.c b/src/drivers/i2c/rt5663/rt5663.c index 272cf78..565e3bb 100644 --- a/src/drivers/i2c/rt5663/rt5663.c +++ b/src/drivers/i2c/rt5663/rt5663.c @@ -27,7 +27,7 @@ }; struct acpi_dp *dp;

- if (!dev->enabled || !scope) + if (!scope) return;

/* Device */ diff --git a/src/drivers/i2c/sx9310/sx9310.c b/src/drivers/i2c/sx9310/sx9310.c index c12e4ea..8dc57a2 100644 --- a/src/drivers/i2c/sx9310/sx9310.c +++ b/src/drivers/i2c/sx9310/sx9310.c @@ -28,7 +28,7 @@ }; struct acpi_dp *dsd;

- if (!dev->enabled || !scope || !config) + if (!scope || !config) return;

if (config->speed) diff --git a/src/drivers/i2c/tpm/chip.c b/src/drivers/i2c/tpm/chip.c index 2baec42..07791c3 100644 --- a/src/drivers/i2c/tpm/chip.c +++ b/src/drivers/i2c/tpm/chip.c @@ -20,7 +20,7 @@ .resource = scope, };

- if (!dev->enabled || !scope) + if (!scope) return;

if (!config->hid) { diff --git a/src/drivers/intel/ish/ish.c b/src/drivers/intel/ish/ish.c index f82f7fc..19cbd82 100644 --- a/src/drivers/intel/ish/ish.c +++ b/src/drivers/intel/ish/ish.c @@ -13,7 +13,7 @@ struct device *root = dev->bus->dev; struct acpi_dp *dsd;

- if (!dev->enabled || !config || !config->firmware_name) + if (!config || !config->firmware_name) return;

acpigen_write_scope(acpi_device_path(root)); diff --git a/src/drivers/intel/mipi_camera/camera.c b/src/drivers/intel/mipi_camera/camera.c index d4cf33d..7dfd650 100644 --- a/src/drivers/intel/mipi_camera/camera.c +++ b/src/drivers/intel/mipi_camera/camera.c @@ -909,9 +909,6 @@ const char *scope = NULL; const struct device *pdev;

- if (!dev->enabled) - return; - if (config->has_power_resource) { pdev = dev->bus->dev; if (!pdev || !pdev->enabled) diff --git a/src/drivers/intel/pmc_mux/conn/conn.c b/src/drivers/intel/pmc_mux/conn/conn.c index 16d113b..9fd8543 100644 --- a/src/drivers/intel/pmc_mux/conn/conn.c +++ b/src/drivers/intel/pmc_mux/conn/conn.c @@ -32,9 +32,6 @@ const char *scope; const char *name;

- if (!dev->enabled) - return; - /* Reference the existing scope and write CONx device */ scope = acpi_device_scope(dev); name = acpi_device_name(dev); diff --git a/src/drivers/intel/soundwire/soundwire.c b/src/drivers/intel/soundwire/soundwire.c index 34ecd86..c7e84a5 100644 --- a/src/drivers/intel/soundwire/soundwire.c +++ b/src/drivers/intel/soundwire/soundwire.c @@ -50,7 +50,7 @@ struct intel_soundwire_controller *controller; const char *scope = acpi_device_scope(dev);

- if (!dev->enabled || !scope) + if (!scope) return;

if (soc_fill_soundwire_controller(&controller) < 0 || !controller) diff --git a/src/drivers/intel/usb4/retimer/retimer.c b/src/drivers/intel/usb4/retimer/retimer.c index be9ec35..7a693ff 100644 --- a/src/drivers/intel/usb4/retimer/retimer.c +++ b/src/drivers/intel/usb4/retimer/retimer.c @@ -101,7 +101,7 @@ const struct drivers_intel_usb4_retimer_config *config = dev->chip_info; const char *scope = acpi_device_scope(dev);

- if (!dev->enabled || !scope || !config) + if (!scope || !config) return;

if (!config->power_gpio.pin_count) { diff --git a/src/drivers/soundwire/alc5682/alc5682.c b/src/drivers/soundwire/alc5682/alc5682.c index 79ed610..e15ecd4 100644 --- a/src/drivers/soundwire/alc5682/alc5682.c +++ b/src/drivers/soundwire/alc5682/alc5682.c @@ -128,7 +128,7 @@ const char *scope = acpi_device_scope(dev); struct acpi_dp *dsd;

- if (!dev->enabled || !scope) + if (!scope) return;

acpigen_write_scope(scope); diff --git a/src/drivers/soundwire/alc711/alc711.c b/src/drivers/soundwire/alc711/alc711.c index 8382fc9..44a9e98 100644 --- a/src/drivers/soundwire/alc711/alc711.c +++ b/src/drivers/soundwire/alc711/alc711.c @@ -105,7 +105,7 @@ const char *scope = acpi_device_scope(dev); struct acpi_dp *dsd;

- if (!dev->enabled || !scope) + if (!scope) return;

acpigen_write_scope(scope); diff --git a/src/drivers/soundwire/max98373/max98373.c b/src/drivers/soundwire/max98373/max98373.c index 231385c..28796c0 100644 --- a/src/drivers/soundwire/max98373/max98373.c +++ b/src/drivers/soundwire/max98373/max98373.c @@ -114,7 +114,7 @@ const char *scope = acpi_device_scope(dev); struct acpi_dp *dsd;

- if (!dev->enabled || !scope) + if (!scope) return;

acpigen_write_scope(scope); diff --git a/src/drivers/spi/acpi/acpi.c b/src/drivers/spi/acpi/acpi.c index c0e776e..b23bc9d 100644 --- a/src/drivers/spi/acpi/acpi.c +++ b/src/drivers/spi/acpi/acpi.c @@ -77,7 +77,7 @@ int reset_gpio_index = -1; int enable_gpio_index = -1;

- if (!dev->enabled || !scope) + if (!scope) return;

if (spi_acpi_get_bus(dev) == -1) { diff --git a/src/drivers/uart/acpi/acpi.c b/src/drivers/uart/acpi/acpi.c index f9d9d8f..d4b14aa 100644 --- a/src/drivers/uart/acpi/acpi.c +++ b/src/drivers/uart/acpi/acpi.c @@ -46,7 +46,7 @@ int reset_gpio_index = -1; int enable_gpio_index = -1;

- if (!dev->enabled || !scope) + if (!scope) return;

if (!config->hid) { diff --git a/src/drivers/usb/acpi/usb_acpi.c b/src/drivers/usb/acpi/usb_acpi.c index d33b7de..55ef1d3 100644 --- a/src/drivers/usb/acpi/usb_acpi.c +++ b/src/drivers/usb/acpi/usb_acpi.c @@ -24,7 +24,7 @@ struct drivers_usb_acpi_config *config = dev->chip_info; const char *path = acpi_device_path(dev);

- if (!dev->enabled || !path || !config) + if (!path || !config) return;

/* Don't generate output for hubs, only ports */ diff --git a/src/drivers/wifi/generic/acpi.c b/src/drivers/wifi/generic/acpi.c index ac2c5eb..1cc4bd0 100644 --- a/src/drivers/wifi/generic/acpi.c +++ b/src/drivers/wifi/generic/acpi.c @@ -222,9 +222,6 @@ { const char *path;

- if (!is_dev_enabled(dev)) - return; - path = acpi_device_path(dev); if (!path) return; @@ -247,9 +244,6 @@ { const char *path;

- if (!is_dev_enabled(dev)) - return; - path = acpi_device_path(dev->bus->dev); if (!path) return; diff --git a/src/ec/google/chromeec/audio_codec/audio_codec.c b/src/ec/google/chromeec/audio_codec/audio_codec.c index 612b1f6..53037eb 100644 --- a/src/ec/google/chromeec/audio_codec/audio_codec.c +++ b/src/ec/google/chromeec/audio_codec/audio_codec.c @@ -15,7 +15,7 @@ const char *scope = acpi_device_scope(dev); struct ec_google_chromeec_audio_codec_config *cfg = dev->chip_info;

- if (!dev->enabled || !scope || !cfg) + if (!scope || !cfg) return;

acpigen_write_scope(scope); diff --git a/src/ec/google/chromeec/ec_acpi.c b/src/ec/google/chromeec/ec_acpi.c index 448ea4d..fff3954 100644 --- a/src/ec/google/chromeec/ec_acpi.c +++ b/src/ec/google/chromeec/ec_acpi.c @@ -223,9 +223,6 @@ struct device_path path; struct device *ec;

- if (!dev->enabled) - return; - /* Set up a minimal EC0 device to pass to the DPTF helpers */ path.type = DEVICE_PATH_GENERIC; path.generic.id = 0; diff --git a/src/ec/google/chromeec/i2c_tunnel/i2c_tunnel.c b/src/ec/google/chromeec/i2c_tunnel/i2c_tunnel.c index ec8bdfc..e61ecfd 100644 --- a/src/ec/google/chromeec/i2c_tunnel/i2c_tunnel.c +++ b/src/ec/google/chromeec/i2c_tunnel/i2c_tunnel.c @@ -17,7 +17,7 @@ struct ec_google_chromeec_i2c_tunnel_config *cfg = dev->chip_info; struct acpi_dp *dsd;

- if (!dev->enabled || !scope || !cfg) + if (!scope || !cfg) return;

acpigen_write_scope(scope); diff --git a/src/ec/google/wilco/chip.c b/src/ec/google/wilco/chip.c index dccaa23..911eb25 100644 --- a/src/ec/google/wilco/chip.c +++ b/src/ec/google/wilco/chip.c @@ -184,9 +184,6 @@ void *region_ptr; size_t ucsi_alloc_region_len;

- if (!dev->enabled) - return; - ucsi_alloc_region_len = ucsi_region_len < UCSI_MIN_ALLOC_REGION_LEN ? UCSI_MIN_ALLOC_REGION_LEN : ucsi_region_len; region_ptr = cbmem_add(CBMEM_ID_ACPI_UCSI, ucsi_alloc_region_len); diff --git a/src/soc/intel/common/block/scs/sd.c b/src/soc/intel/common/block/scs/sd.c index 04ebb13..be59a3d 100644 --- a/src/soc/intel/common/block/scs/sd.c +++ b/src/soc/intel/common/block/scs/sd.c @@ -12,9 +12,6 @@ struct acpi_gpio default_gpio = { 0 }; struct acpi_dp *dp;

- if (!dev->enabled) - return; - if (sd_fill_soc_gpio_info(&default_gpio, dev) != 0) return;

diff --git a/src/soc/intel/common/block/usb4/pcie.c b/src/soc/intel/common/block/usb4/pcie.c index eae9027..81496ce 100644 --- a/src/soc/intel/common/block/usb4/pcie.c +++ b/src/soc/intel/common/block/usb4/pcie.c @@ -26,7 +26,7 @@ return; }

- if (!dev->enabled || !parent->enabled) + if (!parent->enabled) return;

config = config_of(dev); diff --git a/src/soc/intel/common/block/usb4/usb4.c b/src/soc/intel/common/block/usb4/usb4.c index df2dfdd..a8ab736 100644 --- a/src/soc/intel/common/block/usb4/usb4.c +++ b/src/soc/intel/common/block/usb4/usb4.c @@ -28,9 +28,6 @@ { struct acpi_dp *dsd, *pkg;

- if (!dev->enabled) - return; - acpigen_write_scope(acpi_device_path(dev));

dsd = acpi_dp_new_table("_DSD");

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 3 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Christian Walter christian.walter@9elements.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: merged

Felix Held (Code Review)

7 Aug 7 Aug

4:09 p.m.

New subject: [M] Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47148?usp=email )

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

Patch Set 3:

(1 comment)

Patchset:

PS3: this sort-of breaks uart_inject_ssdt for disabled uarts on amd soc which was added in https://review.coreboot.org/c/coreboot/+/42327

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 3 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Christian Walter christian.walter@9elements.com Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Patrick Georgi patrick@coreboot.org Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak inforichland@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Felix Held felix-coreboot@felixheld.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Comment-Date: Mon, 07 Aug 2023 16:09:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Felix Held (Code Review)

4:13 p.m.

New subject: [M] Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

Attention is currently required from: Karthik Ramasubramanian.

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47148?usp=email )

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

Patch Set 3:

(1 comment)

File src/drivers/intel/dptf/dptf.c:

https://review.coreboot.org/c/coreboot/+/47148/comment/bd506aab_45ac9373 : PS1, Line 235: if (!dev->enabled) : return; :

...

Done

the amd soc uart code expects the ssdt generation function to be called in all cases to be able to write a _STA 0 for all disabled devices. when the _STA method is missing, the OS will assume that the device is there and working

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 3 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Christian Walter christian.walter@9elements.com Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Patrick Georgi patrick@coreboot.org Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak inforichland@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Felix Held felix-coreboot@felixheld.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Karthik Ramasubramanian kramasub@google.com Gerrit-Comment-Date: Mon, 07 Aug 2023 16:13:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan.m.shaikh@gmail.com Comment-In-Reply-To: Tim Wawrzynczak inforichland@gmail.com Comment-In-Reply-To: Karthik Ramasubramanian kramasub@google.com Gerrit-MessageType: comment

Tim Wawrzynczak (Code Review)

8 Aug 8 Aug

3:21 p.m.

New subject: [M] Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

Attention is currently required from: Karthik Ramasubramanian.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47148?usp=email )

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

Patch Set 3:

(1 comment)

File src/drivers/intel/dptf/dptf.c:

https://review.coreboot.org/c/coreboot/+/47148/comment/c65c67d9_ec1c6e1c : PS1, Line 235: if (!dev->enabled) : return; :

...

the amd soc uart code expects the ssdt generation function to be called in all cases to be able to w […]

It feels wrong to do things on disabled devices though, doesn't it? Switching to using acpigen for the UART definition would solve that, or even just setting the raw ASL to have _STA set to the value of a Name which has a default value of 0, and then if it's enabled write one line of acpigen to set that Name to 1

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 3 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Christian Walter christian.walter@9elements.com Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Patrick Georgi patrick@coreboot.org Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak inforichland@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Felix Held felix-coreboot@felixheld.de Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Karthik Ramasubramanian kramasub@google.com Gerrit-Comment-Date: Tue, 08 Aug 2023 15:21:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan.m.shaikh@gmail.com Comment-In-Reply-To: Tim Wawrzynczak inforichland@gmail.com Comment-In-Reply-To: Karthik Ramasubramanian kramasub@google.com Comment-In-Reply-To: Felix Held felix-coreboot@felixheld.de Gerrit-MessageType: comment

Matt DeVillier (Code Review)

3:51 p.m.

New subject: [M] Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

Attention is currently required from: Karthik Ramasubramanian.

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47148?usp=email )

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

Patch Set 3:

(1 comment)

File src/drivers/intel/dptf/dptf.c:

https://review.coreboot.org/c/coreboot/+/47148/comment/271597a8_2a01d40f : PS1, Line 235: if (!dev->enabled) : return; :

...

It feels wrong to do things on disabled devices though, doesn't it? Switching to using acpigen for t […]

lol Tim, that's actually the workaround I came up with (though setting the name to 0xF), as IMO it's simply not possible to implement the entire UART device in acpigen, given the complexity of the IRQ assignment (PIC vs APIC). I pushed a revert of this as well, so whichever solution is preferred we can implement

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 3 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Christian Walter christian.walter@9elements.com Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Patrick Georgi patrick@coreboot.org Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak inforichland@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Felix Held felix-coreboot@felixheld.de Gerrit-CC: Matt DeVillier matt.devillier@amd.corp-partner.google.com Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Karthik Ramasubramanian kramasub@google.com Gerrit-Comment-Date: Tue, 08 Aug 2023 15:51:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan.m.shaikh@gmail.com Comment-In-Reply-To: Tim Wawrzynczak inforichland@gmail.com Comment-In-Reply-To: Karthik Ramasubramanian kramasub@google.com Comment-In-Reply-To: Felix Held felix-coreboot@felixheld.de Gerrit-MessageType: comment

Karthik Ramasubramanian (Code Review)

4:16 p.m.

New subject: [M] Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47148?usp=email )

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

Patch Set 3:

(1 comment)

File src/drivers/intel/dptf/dptf.c:

https://review.coreboot.org/c/coreboot/+/47148/comment/7cce8503_cb732a89 : PS1, Line 235: if (!dev->enabled) : return; :

...

lol Tim, that's actually the workaround I came up with (though setting the name to 0xF), as IMO it's […]

IIUC, this is because the AMD UART device is using a hybrid approach of using raw ASL as well as acpigen. Since this is hopefully an one-off situation, I prefer Tim's recommendation of "setting the raw ASL to have _STA set to the value of a Name which has a default value of 0, and then if it's enabled write one line of acpigen to set that Name to 1".

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 3 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Christian Walter christian.walter@9elements.com Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Patrick Georgi patrick@coreboot.org Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak inforichland@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Felix Held felix-coreboot@felixheld.de Gerrit-CC: Matt DeVillier matt.devillier@amd.corp-partner.google.com Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Comment-Date: Tue, 08 Aug 2023 16:16:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan.m.shaikh@gmail.com Comment-In-Reply-To: Matt DeVillier matt.devillier@amd.corp-partner.google.com Comment-In-Reply-To: Tim Wawrzynczak inforichland@gmail.com Comment-In-Reply-To: Karthik Ramasubramanian kramasub@google.com Comment-In-Reply-To: Felix Held felix-coreboot@felixheld.de Gerrit-MessageType: comment

Felix Held (Code Review)

4:52 p.m.

New subject: [M] Change in coreboot[master]: acpi: Call acpi_fill_ssdt() only for enabled devices

Attention is currently required from: Karthik Ramasubramanian.

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47148?usp=email )

Change subject: acpi: Call acpi_fill_ssdt() only for enabled devices ......................................................................

Patch Set 3:

(1 comment)

File src/drivers/intel/dptf/dptf.c:

https://review.coreboot.org/c/coreboot/+/47148/comment/c2f2a111_6d78757d : PS1, Line 235: if (!dev->enabled) : return; :

...

IIUC, this is because the AMD UART device is using a hybrid approach of using raw ASL as well as acp […]

i had some out of band discussion with Matt and i'd say that CB:77092 is the best feasible solution that doesn't have significant side effects

-- To view, visit https://review.coreboot.org/c/coreboot/+/47148?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib042bec7e8c68b38fafa60a8e965d781bddcd1f0 Gerrit-Change-Number: 47148 Gerrit-PatchSet: 3 Gerrit-Owner: Karthik Ramasubramanian kramasub@google.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Christian Walter christian.walter@9elements.com Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Patrick Georgi patrick@coreboot.org Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Gerrit-Reviewer: Tim Wawrzynczak inforichland@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Felix Held felix-coreboot@felixheld.de Gerrit-CC: Matt DeVillier matt.devillier@amd.corp-partner.google.com Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Karthik Ramasubramanian kramasub@google.com Gerrit-Comment-Date: Tue, 08 Aug 2023 16:52:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan.m.shaikh@gmail.com Comment-In-Reply-To: Matt DeVillier matt.devillier@amd.corp-partner.google.com Comment-In-Reply-To: Tim Wawrzynczak inforichland@gmail.com Comment-In-Reply-To: Karthik Ramasubramanian kramasub@google.com Comment-In-Reply-To: Felix Held felix-coreboot@felixheld.de Gerrit-MessageType: comment

501

days inactive

1510

days old

coreboot-gerrit@coreboot.org

18 comments

9 participants

tags (0)

participants (9)

Angel Pons (Code Review)
Christian Walter (Code Review)
Felix Held (Code Review)
Furquan Shaikh (Code Review)
Karthik Ramasubramanian (Code Review)
Matt DeVillier (Code Review)
Patrick Georgi (Code Review)
Sumeet R Pawnikar (Code Review)
Tim Wawrzynczak (Code Review)