Change in coreboot[master]: soc/intel/common: Call pci_dev_enable_bus_master() from .final ops
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_enable_bus_master() from .final ops ......................................................................
soc/intel/common: Call pci_dev_enable_bus_master() from .final ops
Enable PCI_COMMAND_MASTER for all PCI controller to ensure device can behave as a bus master. Otherwise, the device can not generate PCI accesses.
BUG=b:154900210 TEST=Able to build and boot CML and TGL platform.
Change-Id: Ife65f6029d2f966e321f616e85f59f4c37c42145 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/dsp/dsp.c M src/soc/intel/common/block/dtt/dtt.c M src/soc/intel/common/block/graphics/graphics.c M src/soc/intel/common/block/hda/hda.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/ipu/ipu.c M src/soc/intel/common/block/lpc/lpc.c M src/soc/intel/common/block/p2sb/p2sb.c M src/soc/intel/common/block/pcie/pcie.c M src/soc/intel/common/block/pmc/pmc.c M src/soc/intel/common/block/sata/sata.c M src/soc/intel/common/block/scs/mmc.c M src/soc/intel/common/block/scs/sd.c M src/soc/intel/common/block/smbus/smbus.c M src/soc/intel/common/block/spi/spi.c M src/soc/intel/common/block/sram/sram.c M src/soc/intel/common/block/systemagent/systemagent.c M src/soc/intel/common/block/uart/uart.c M src/soc/intel/common/block/xdci/xdci.c M src/soc/intel/common/block/xhci/xhci.c 21 files changed, 21 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/44512/1
diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c index 4b598e2..5d73812 100644 --- a/src/soc/intel/common/block/cse/cse.c +++ b/src/soc/intel/common/block/cse/cse.c @@ -870,6 +870,7 @@ .read_resources = pci_dev_read_resources, .enable_resources = pci_dev_enable_resources, .init = pci_dev_init, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, };
diff --git a/src/soc/intel/common/block/dsp/dsp.c b/src/soc/intel/common/block/dsp/dsp.c index 776a22b..80f758a 100644 --- a/src/soc/intel/common/block/dsp/dsp.c +++ b/src/soc/intel/common/block/dsp/dsp.c @@ -8,6 +8,7 @@ .read_resources = pci_dev_read_resources, .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, .scan_bus = scan_static_bus, }; diff --git a/src/soc/intel/common/block/dtt/dtt.c b/src/soc/intel/common/block/dtt/dtt.c index f396993..a04bc7b 100644 --- a/src/soc/intel/common/block/dtt/dtt.c +++ b/src/soc/intel/common/block/dtt/dtt.c @@ -14,6 +14,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, .scan_bus = scan_generic_bus, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, };
diff --git a/src/soc/intel/common/block/graphics/graphics.c b/src/soc/intel/common/block/graphics/graphics.c index 38d41df..0aa832b 100644 --- a/src/soc/intel/common/block/graphics/graphics.c +++ b/src/soc/intel/common/block/graphics/graphics.c @@ -170,6 +170,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, .init = gma_init, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, #if CONFIG(HAVE_ACPI_TABLES) .acpi_fill_ssdt = gma_generate_ssdt, diff --git a/src/soc/intel/common/block/hda/hda.c b/src/soc/intel/common/block/hda/hda.c index e4bcf99..2f8bc24 100644 --- a/src/soc/intel/common/block/hda/hda.c +++ b/src/soc/intel/common/block/hda/hda.c @@ -53,6 +53,7 @@ #if CONFIG(SOC_INTEL_COMMON_BLOCK_HDA_VERB) .init = hda_init, #endif + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, .scan_bus = scan_static_bus }; diff --git a/src/soc/intel/common/block/i2c/i2c.c b/src/soc/intel/common/block/i2c/i2c.c index 0de3bd3..5b2a755 100644 --- a/src/soc/intel/common/block/i2c/i2c.c +++ b/src/soc/intel/common/block/i2c/i2c.c @@ -168,6 +168,7 @@ .enable_resources = pci_dev_enable_resources, .scan_bus = scan_smbus, .ops_i2c_bus = &dw_i2c_bus_ops, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, .init = dw_i2c_device_init, #if CONFIG(HAVE_ACPI_TABLES) diff --git a/src/soc/intel/common/block/ipu/ipu.c b/src/soc/intel/common/block/ipu/ipu.c index f7b01aa..58030a1 100644 --- a/src/soc/intel/common/block/ipu/ipu.c +++ b/src/soc/intel/common/block/ipu/ipu.c @@ -8,6 +8,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_bus_enable_resources, .scan_bus = scan_generic_bus, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, };
diff --git a/src/soc/intel/common/block/lpc/lpc.c b/src/soc/intel/common/block/lpc/lpc.c index 212fd70..a16f10f 100644 --- a/src/soc/intel/common/block/lpc/lpc.c +++ b/src/soc/intel/common/block/lpc/lpc.c @@ -105,6 +105,7 @@ #endif .init = lpc_soc_init, .scan_bus = scan_static_bus, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, };
diff --git a/src/soc/intel/common/block/p2sb/p2sb.c b/src/soc/intel/common/block/p2sb/p2sb.c index d97cd8d..f7c7545 100644 --- a/src/soc/intel/common/block/p2sb/p2sb.c +++ b/src/soc/intel/common/block/p2sb/p2sb.c @@ -124,6 +124,7 @@ static const struct device_operations device_ops = { .read_resources = read_resources, .set_resources = noop_set_resources, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, };
diff --git a/src/soc/intel/common/block/pcie/pcie.c b/src/soc/intel/common/block/pcie/pcie.c index e506905..55ada78 100644 --- a/src/soc/intel/common/block/pcie/pcie.c +++ b/src/soc/intel/common/block/pcie/pcie.c @@ -64,6 +64,7 @@ .enable_resources = pci_bus_enable_resources, .init = pch_pcie_init, .scan_bus = pciexp_scan_bridge, + .final = pci_dev_enable_bus_master, .ops_pci = &pcie_ops, };
diff --git a/src/soc/intel/common/block/pmc/pmc.c b/src/soc/intel/common/block/pmc/pmc.c index 24f28e3..44a77b2 100644 --- a/src/soc/intel/common/block/pmc/pmc.c +++ b/src/soc/intel/common/block/pmc/pmc.c @@ -103,6 +103,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, .init = pmc_soc_init, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, .scan_bus = scan_static_bus, }; diff --git a/src/soc/intel/common/block/sata/sata.c b/src/soc/intel/common/block/sata/sata.c index 1889700..eb1d8c6 100644 --- a/src/soc/intel/common/block/sata/sata.c +++ b/src/soc/intel/common/block/sata/sata.c @@ -4,17 +4,11 @@ #include <device/pci.h> #include <device/pci_ids.h>
-static void sata_final(struct device *dev) -{ - /* Set Bus Master */ - pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER); -} - static struct device_operations sata_ops = { .read_resources = pci_dev_read_resources, .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, - .final = sata_final, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, };
diff --git a/src/soc/intel/common/block/scs/mmc.c b/src/soc/intel/common/block/scs/mmc.c index 6772f74..debec19 100644 --- a/src/soc/intel/common/block/scs/mmc.c +++ b/src/soc/intel/common/block/scs/mmc.c @@ -67,6 +67,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, .init = mmc_soc_init, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, };
diff --git a/src/soc/intel/common/block/scs/sd.c b/src/soc/intel/common/block/scs/sd.c index 04ebb13..17d508f 100644 --- a/src/soc/intel/common/block/scs/sd.c +++ b/src/soc/intel/common/block/scs/sd.c @@ -48,6 +48,7 @@ #if CONFIG(HAVE_ACPI_TABLES) .acpi_fill_ssdt = sd_fill_ssdt, #endif + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, };
diff --git a/src/soc/intel/common/block/smbus/smbus.c b/src/soc/intel/common/block/smbus/smbus.c index ae9f650..70581dc 100644 --- a/src/soc/intel/common/block/smbus/smbus.c +++ b/src/soc/intel/common/block/smbus/smbus.c @@ -70,6 +70,7 @@ .enable_resources = pci_dev_enable_resources, .scan_bus = scan_smbus, .init = pch_smbus_init, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, .ops_smbus_bus = &lops_smbus_bus, }; diff --git a/src/soc/intel/common/block/spi/spi.c b/src/soc/intel/common/block/spi/spi.c index 295df09..d73c008 100644 --- a/src/soc/intel/common/block/spi/spi.c +++ b/src/soc/intel/common/block/spi/spi.c @@ -35,6 +35,7 @@ .enable_resources = pci_dev_enable_resources, .scan_bus = scan_generic_bus, .ops_spi_bus = &spi_bus_ops, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, };
diff --git a/src/soc/intel/common/block/sram/sram.c b/src/soc/intel/common/block/sram/sram.c index 426a5f7..002b164 100644 --- a/src/soc/intel/common/block/sram/sram.c +++ b/src/soc/intel/common/block/sram/sram.c @@ -29,6 +29,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, .init = soc_sram_init, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, };
diff --git a/src/soc/intel/common/block/systemagent/systemagent.c b/src/soc/intel/common/block/systemagent/systemagent.c index 39ac53f..4eb7c9d 100644 --- a/src/soc/intel/common/block/systemagent/systemagent.c +++ b/src/soc/intel/common/block/systemagent/systemagent.c @@ -306,6 +306,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, .init = sa_soc_systemagent_init, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, #if CONFIG(HAVE_ACPI_TABLES) .write_acpi_tables = sa_write_acpi_tables, diff --git a/src/soc/intel/common/block/uart/uart.c b/src/soc/intel/common/block/uart/uart.c index ed4f9c6..039dcd2 100644 --- a/src/soc/intel/common/block/uart/uart.c +++ b/src/soc/intel/common/block/uart/uart.c @@ -229,6 +229,7 @@ .read_resources = uart_read_resources, .set_resources = pci_dev_set_resources, .enable_resources = uart_common_enable_resources, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, };
diff --git a/src/soc/intel/common/block/xdci/xdci.c b/src/soc/intel/common/block/xdci/xdci.c index 54cb076..4008177 100644 --- a/src/soc/intel/common/block/xdci/xdci.c +++ b/src/soc/intel/common/block/xdci/xdci.c @@ -18,6 +18,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, .init = soc_xdci_init, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, };
diff --git a/src/soc/intel/common/block/xhci/xhci.c b/src/soc/intel/common/block/xhci/xhci.c index 47f2567..f8b9571 100644 --- a/src/soc/intel/common/block/xhci/xhci.c +++ b/src/soc/intel/common/block/xhci/xhci.c @@ -98,6 +98,7 @@ .set_resources = pci_dev_set_resources, .enable_resources = pci_dev_enable_resources, .init = soc_xhci_init, + .final = pci_dev_enable_bus_master, .ops_pci = &pci_dev_ops_pci, .scan_bus = scan_static_bus, #if CONFIG(HAVE_ACPI_TABLES)
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_enable_bus_master() from .final ops ......................................................................
Patch Set 1:
Please give a reason why devices should behave as a bus master. The PCI_COMMAND_MASTER has been removed on nearly all devices as it doesn't seem necessary. To make sure it's not removed again, please give some details what doesn't work.
Hello build bot (Jenkins), Furquan Shaikh, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44512
to look at the new patch set (#2).
Change subject: soc/intel/common: Call pci_dev_enable_bus_master() from .final ops ......................................................................
soc/intel/common: Call pci_dev_enable_bus_master() from .final ops
Enable PCI_COMMAND_MASTER for all PCI controller to ensure device can behave as a bus master. Otherwise, the device can not generate PCI accesses.
This patch ensures that coreboot would set PCI_COMMAND_MASTER at end of POST to be independent of FSP supported IA-platform where FSP might clear PCI_COMMAND_MASTER accidentally and payload won't be able to access the PCI device resources hence leads to unnecessary delay or even timeout.
BUG=b:154900210 TEST=Able to build and boot CML and TGL platform.
Change-Id: Ife65f6029d2f966e321f616e85f59f4c37c42145 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/dsp/dsp.c M src/soc/intel/common/block/dtt/dtt.c M src/soc/intel/common/block/graphics/graphics.c M src/soc/intel/common/block/hda/hda.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/ipu/ipu.c M src/soc/intel/common/block/lpc/lpc.c M src/soc/intel/common/block/p2sb/p2sb.c M src/soc/intel/common/block/pcie/pcie.c M src/soc/intel/common/block/pmc/pmc.c M src/soc/intel/common/block/sata/sata.c M src/soc/intel/common/block/scs/mmc.c M src/soc/intel/common/block/scs/sd.c M src/soc/intel/common/block/smbus/smbus.c M src/soc/intel/common/block/spi/spi.c M src/soc/intel/common/block/sram/sram.c M src/soc/intel/common/block/systemagent/systemagent.c M src/soc/intel/common/block/uart/uart.c M src/soc/intel/common/block/xdci/xdci.c M src/soc/intel/common/block/xhci/xhci.c 21 files changed, 21 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/44512/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_enable_bus_master() from .final ops ......................................................................
Patch Set 2:
Patch Set 1:
Please give a reason why devices should behave as a bus master. The PCI_COMMAND_MASTER has been removed on nearly all devices as it doesn't seem necessary. To make sure it's not removed again, please give some details what doesn't work.
Thanks Patrick, added section in commit msg itself
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_enable_bus_master() from .final ops ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access Not sure what you mean here. To access PCI device resources PCI_COMMAND_IO and PCI_COMMAND_MEMORY should be sufficient. Which device needs PCI_COMMAND_MASTER and why can't the payload or OS set PCI_COMMAND_MASTER?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_enable_bus_master() from .final ops ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
Not sure what you mean here. […]
valid point.
Ideally below register bit should be written by the firmware and should never be changed by the device driver. FW OS Command/Bus Master Write to 0 (reset value) unless boot device. Must write to a 1 before the first DMA operation. Must write to a 0 before unconfiguring device driver.
will wait for Furquan to share his idea, bt i think we should only enable bus master for boot associated device like SATA, USB, PCIE, EMMC, SD etc and all other controller should be ignored by BIOS/FW but OS driver should enable 1 to allow DMA access. I guess Furquan meant to enable this for all PCI device by default
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_enable_bus_master() from .final ops ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
valid point. […]
Shouldn't the payload set bus master, then? If it doesn't, then it's a problem in the payload.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_enable_bus_master() from .final ops ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
Shouldn't the payload set bus master, then? If it doesn't, then it's a problem in the payload.
i agree, will wait for Furquan to chime in ahd share his view on this.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_enable_bus_master() from .final ops ......................................................................
Patch Set 2:
@Furquan, if you could share your feedback on this CL would be good. i'm thinking to rebase this CL based on Felix BM CL
Hello build bot (Jenkins), Furquan Shaikh, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44512
to look at the new patch set (#3).
Change subject: soc/intel/common: Call pci_dev_enable_bus_master() from .final ops ......................................................................
soc/intel/common: Call pci_dev_enable_bus_master() from .final ops
Enable PCI_COMMAND_MASTER for graphics, storage and host bridge PCI controller to ensure device can behave as a bus master. Otherwise, the device can not generate PCI accesses.
This patch ensures that coreboot would set PCI_COMMAND_MASTER at end of POST to be independent of FSP supported IA-platform where FSP might clear PCI_COMMAND_MASTER accidentally and payload won't be able to access the PCI device resources hence leads to unnecessary delay or even timeout.
BUG=b:154900210 TEST=Able to build and boot CML and TGL platform.
Change-Id: Ife65f6029d2f966e321f616e85f59f4c37c42145 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/graphics/graphics.c M src/soc/intel/common/block/pcie/pcie.c M src/soc/intel/common/block/sata/sata.c M src/soc/intel/common/block/scs/mmc.c M src/soc/intel/common/block/scs/sd.c M src/soc/intel/common/block/systemagent/systemagent.c M src/soc/intel/common/block/xhci/xhci.c 7 files changed, 7 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/44512/3
Subrata Banik has uploaded a new patch set (#4) to the change originally created by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_enable_bus_master() from .final ops ......................................................................
soc/intel/common: Call pci_dev_enable_bus_master() from .final ops
Enable PCI_COMMAND_MASTER for graphics, storage and host bridge PCI controller to ensure device can behave as a bus master. Otherwise, the device can not generate PCI accesses.
This patch ensures that coreboot would set PCI_COMMAND_MASTER at end of POST to be independent of FSP supported IA-platform where FSP might clear PCI_COMMAND_MASTER accidentally and payload won't be able to access the PCI device resources hence leads to unnecessary delay or even timeout.
BUG=b:154900210 TEST=Able to build and boot CML and TGL platform.
Change-Id: Ife65f6029d2f966e321f616e85f59f4c37c42145 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/graphics/graphics.c M src/soc/intel/common/block/pcie/pcie.c M src/soc/intel/common/block/sata/sata.c M src/soc/intel/common/block/scs/mmc.c M src/soc/intel/common/block/scs/sd.c M src/soc/intel/common/block/systemagent/systemagent.c M src/soc/intel/common/block/xhci/xhci.c 7 files changed, 7 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/44512/4
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_enable_bus_master() from .final ops ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
i agree, will wait for Furquan to chime in ahd share his view on this.
enable BM for BIOS critical device like host bridge, output, input and block device
Hello build bot (Jenkins), Furquan Shaikh, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44512
to look at the new patch set (#5).
Change subject: soc/intel/common: Call pci_dev_request_bus_master() from .final ops ......................................................................
soc/intel/common: Call pci_dev_request_bus_master() from .final ops
Enable PCI_COMMAND_MASTER for graphics, storage and host bridge PCI controller to ensure device can behave as a bus master. Otherwise, the device can not generate PCI accesses.
This patch ensures that coreboot would set PCI_COMMAND_MASTER at end of POST to be independent of FSP supported IA-platform where FSP might clear PCI_COMMAND_MASTER accidentally and payload won't be able to access the PCI device resources hence leads to unnecessary delay or even timeout.
BUG=b:154900210 TEST=Able to build and boot CML and TGL platform.
Change-Id: Ife65f6029d2f966e321f616e85f59f4c37c42145 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/graphics/graphics.c M src/soc/intel/common/block/pcie/pcie.c M src/soc/intel/common/block/sata/sata.c M src/soc/intel/common/block/scs/mmc.c M src/soc/intel/common/block/scs/sd.c M src/soc/intel/common/block/systemagent/systemagent.c M src/soc/intel/common/block/xhci/xhci.c 7 files changed, 7 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/44512/5
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_request_bus_master() from .final ops ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
enable BM for BIOS critical device like host bridge, output, input and block device
My understanding is that we currently enable PCI_COMMAND_MASTER for most internal controllers within coreboot - I2C, GSPI, Fast SPI, UART, integrated graphics, etc. These are primarily the controllers that get utilized within coreboot/payload.
However, in a lot of cases, we rely on FSP to do this configuration for us. Example: Early I2C configuration does the PCI_COMMAND_MASTER setting, but late I2C configuration doesn't do this. Also, I don't think FSP is consistent about performing this configuration for all controllers.
The most recent issue that we ran into was SATA configuration being performed by FSP but not really setting PCI_COMMAND_MASTER which resulted in issues in payload.
I have thought of coreboot as performing the initialization of controllers so that the payload can use them without having to redo that initialization. Thus, my comment to Subrata that we should ensure that PCI_COMMAND_MASTER is set for all controllers consistently within coreboot. I understand concerns about doing this for hotpluggable buses(e.g. usb4 controllers). But, other than that I believe it should be okay to do the setting of PCI_COMMAND_MASTER within coreboot.
I might be wrong with my understanding here. So if this understanding is not correct, I think we should at least ensure that the assumptions are properly documented so that the payloads can take appropriate actions to handle this.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_request_bus_master() from .final ops ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
My understanding is that we currently enable PCI_COMMAND_MASTER for most internal controllers within […]
Thanks Furquan, we will wait for Angel and Patrick to get back.Present CL is doing BM enable for boot critical device but agree with you that we might need the same for I2C and GSPI as well.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_request_bus_master() from .final ops ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
Thanks Furquan, we will wait for Angel and Patrick to get back. […]
I'm fine with this change as long as it's clear which specification is followed here. Which specification states that PCI_COMMAND_MASTER must be activated? If this is not part of an official BWG/bootloader bringup guide then we should write it down somewhere in the Documentation folder and thus follow our own specification.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_request_bus_master() from .final ops ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
I'm fine with this change as long as it's clear which specification is followed here. […]
Great plan. Furquan, would you like to come up with spec?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_request_bus_master() from .final ops ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
Great plan. […]
Something that would be good to know is whether Bus Master needs to be enabled *while* initializing the device. AFAIK, the only device for which BWGs explicitly state that Bus Master needs to be set during initialization is the iGPU.
If we don't need to enable Bus Master at all until we're ready to jump to the payload, we can postpone setting it as much as possible to mitigate DMA attacks
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_request_bus_master() from .final ops ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
My understanding is that we currently enable PCI_COMMAND_MASTER for most internal controllers within coreboot - I2C, GSPI, Fast SPI, UART, integrated graphics, etc. These are primarily the controllers that get utilized within coreboot/payload.
Yes, but mostly by accident. To my knowledge, there are historically three cases when we enable bus mastering: * When documentation tells us to for the initialization sequence. This is for instance the case for Intels IGD device (I guess the sequence happens in FSP nowadays anyway), albeit for unknown reasons (why would the configuration need it if we don't trigger DMA transfers?). * When we are going to hide a device and don't have a chance to enable it later (e.g. "ACPI mode" of some devices, if they really need bus mastering, idk). * When people confuse enabling of MMIO resources with bus mastering (this seems to be the majority of quirks in coreboot today). * When we want compatibility with a broken payload driver. This is rather new. While some of us try to get rid of quirks, others currently add new ones *shrug*
However, in a lot of cases, we rely on FSP to do this configuration for us. Example: Early I2C configuration does the PCI_COMMAND_MASTER setting, but late I2C configuration doesn't do this. Also, I don't think FSP is consistent about performing this configuration for all controllers.
Bus mastering and configuration are traditionally two separate things. With rare exceptions, it's up to the driver that wants a device to perform DMA to enable bus mastering. It doesn't belong into coreboot if it can be done later, IMHO.
The most recent issue that we ran into was SATA configuration being performed by FSP but not really setting PCI_COMMAND_MASTER which resulted in issues in payload.
I read about it. What payload was that actually?
I have thought of coreboot as performing the initialization of controllers so that the payload can use them without having to redo that initialization. Thus, my comment to Subrata that we should ensure that PCI_COMMAND_MASTER is set for all controllers consistently within coreboot.
If we want an option to enable it globally, ok. But why are we adding quirks for individual devices?
I understand concerns about doing this for hotpluggable buses(e.g. usb4 controllers). But, other than that I believe it should be okay to do the setting of PCI_COMMAND_MASTER within coreboot.
There may be more than a single chip on the mainboard. And not everything soldered to a mainboard has the same level of trust.
I might be wrong with my understanding here. So if this understanding is not correct, I think we should at least ensure that the assumptions are properly documented so that the payloads can take appropriate actions to handle this.
Yes, documentation please :) If I look at recently added quirks, they look like accidents, e.g. 41934bfe no word about payload compatibility. For somebody who knews that bus mastering doesn't belong into coreboot, it looks like something to revert.
Our current goal at secunet was to make the current spurious bus master settings optional (if somebody really needs them for compatibility with broken payloads, that's ok for me). And then probably take care that no new quirks are added. Adding more cases where it gets enabled seems completely wrong, IMHO. Because it just hides errors somewhere else. For the same reason, I think all optional cases should be disabled by default. It's just hiding errors and then we need it for compatibility.
And then there are security concerns. I can write something about that too. Maybe it suffices to say, the current state in coreboot makes us look like amateurs ;)
If we don't need to enable Bus Master at all until we're ready to jump to the payload, we can postpone setting it as much as possible to mitigate DMA attacks
That is roughly my thought as well. For a spec this should suffice:
By default, all PCI devices are left with bus mastering disabled unless * a device already performs DMA (e.g. graphics framebuffer) * a device may need DMA later but it can't be enabled later (e.g. ACPI mode).
What is missing is the handling of bridge devices which currently get bus mastering enabled in generic code. That aside, we have already tested minimal bus master settings. It works well with FILO and, IIRC, SeaBIOS too.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_request_bus_master() from .final ops ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access Thanks for the detailed inputs, Nico! They are helpful to understand the history here.
Yes, but mostly by accident. To my knowledge, there are historically three cases when we enable bus mastering:
If that is the case, I think we should fix coreboot to not enable the bus mastering for any of these devices which don't need it at least in the early initialization sequence.
I read about it. What payload was that actually?
Ran into this with depthcharge. However, if the spec we want to enforce in coreboot is to not enable bus mastering for payloads, then I can look into following up on getting this fixed in depthcharge.
There may be more than a single chip on the mainboard. And not everything soldered to a mainboard has the same level of trust.
Agreed.
That aside, we have already tested minimal bus master settings.
Are there more details that you can share about the minimal bus master settings? What devices and platforms were tested and if there were settings identified in coreboot that are clearly wrong. I agree with what you said above - if these are really not required within coreboot, I think we should clean those up rather than adding more quirks.
Subrata - is this something you can follow up on?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_request_bus_master() from .final ops ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
Thanks for the detailed inputs, Nico! They are helpful to understand the history here. […]
There's CB:42460 which replaces all instances where enabling Bus Master was deemed to be unnecessary with `pci_dev_request_bus_master`. This is an intermediate step to allow "busmasterless" (I just made this name up) coreboot while preserving the original behavior by default, in case it breaks something.
If depthcharge tries to use DMA without enabling Bus Master first, then I'd say depthcharge has a bug.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_request_bus_master() from .final ops ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
There's CB:42460 which replaces all instances where enabling Bus Master was deemed to be unnecessary […]
I think the policy we should look at should be only enabling the bare minimum, like Nico suggested above. If coreboot isn't going to use the bus mastering (which it doesn't), then why should it enable it when most of the time, it is the payload's responsibility to determine which devices might require it (say, the boot device). I don't think coreboot should be enabling bus mastering on every possible boot media's controller. I agree with Angel, I think this is a problem in depthcharge.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_request_bus_master() from .final ops ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
I think the policy we should look at should be only enabling the bare minimum, like Nico suggested a […]
I'm a little confused. Isn't depthcharge using libpayload's AHCI driver?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44512 )
Change subject: soc/intel/common: Call pci_dev_request_bus_master() from .final ops ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44512/2//COMMIT_MSG@15 PS2, Line 15: access
I'm a little confused. […]
Nvm. It has its own.
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/44512?usp=email )
Change subject: soc/intel/common: Call pci_dev_request_bus_master() from .final ops ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Stefan Reinauer (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)