Change in coreboot[master]: soc/intel/common/block: Use pci_dev_request_bus_master for BM enabling

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45320 )

Change subject: soc/intel/common/block: Use pci_dev_request_bus_master for BM enabling
......................................................................

soc/intel/common/block: Use pci_dev_request_bus_master for BM enabling

Replace static sata_final() implementation for BM enabling with generic
pci_dev_request_bus_master() function.

TEST=Able to boot to OS from sata device on CML platform.

Signed-off-by: Subrata Banik 
Change-Id: Icd086184fd6fa9c03c806c857f13fad5a9e78a3e
---
M src/soc/intel/common/block/sata/sata.c
1 file changed, 1 insertion(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/45320/1

diff --git a/src/soc/intel/common/block/sata/sata.c b/src/soc/intel/common/block/sata/sata.c
index 1889700..7b234a9 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_request_bus_master,
    .ops_pci		= &pci_dev_ops_pci,
 };

[Angel Pons (Code Review)]

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

Change subject: soc/intel/common/block: Use pci_dev_request_bus_master for BM enabling
......................................................................

Patch Set 1: Code-Review+2

(2 comments)

Looks good to me, but I'd like to make sure we all know what the purpose of `pci_dev_request_bus_master` is.

https://review.coreboot.org/c/coreboot/+/45320/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45320/1//COMMIT_MSG@9 
PS1, Line 9: Replace static sata_final() implementation for BM enabling with generic
           : pci_dev_request_bus_master() function.
The purpose of `pci_dev_request_bus_master` is to have a single switch to disable unnecessary bus master enabling. I'd expand this to explain the purpose of this change:

Enabling Bus Master isn't required by the hardware, so we shouldn't need to enable it at all. However, some payloads do not set this bit before attempting DMA transfers, which results in boot failures.

Replace static sata_final() implementation for BM enabling with generic pci_dev_request_bus_master() function. This allows the user to control through Kconfig whether Bus Master should be enabled.

https://review.coreboot.org/c/coreboot/+/45320/1//COMMIT_MSG@12 
PS1, Line 12: sata
nit: SATA

[Subrata Banik (Code Review)]

Subrata Banik has uploaded a new patch set (#2) to the change originally created by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/45320 )

Change subject: soc/intel/common/block: Use pci_dev_request_bus_master for BM enabling
......................................................................

soc/intel/common/block: Use pci_dev_request_bus_master for BM enabling

Enabling Bus Master isn't required by the hardware, so we shouldn't need
to enable it at all. However, some payloads do not set this bit before
attempting DMA transfers, which results in boot failures.

Replace static sata_final() implementation for BM enabling with generic
pci_dev_request_bus_master() function.

This allows the user to control through Kconfig whether Bus Master
should be enabled.

TEST=Able to boot to OS from SATA device on CML platform.

Signed-off-by: Subrata Banik 
Change-Id: Icd086184fd6fa9c03c806c857f13fad5a9e78a3e
---
M src/soc/intel/common/block/sata/sata.c
1 file changed, 1 insertion(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/45320/2

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45320 )

Change subject: soc/intel/common/block: Use pci_dev_request_bus_master for BM enabling
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/45320/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45320/1//COMMIT_MSG@9 
PS1, Line 9: Replace static sata_final() implementation for BM enabling with generic
           : pci_dev_request_bus_master() function.
> The purpose of `pci_dev_request_bus_master` is to have a single switch to disable unnecessary bus ma […]

Thanks Angel for suggestion.

https://review.coreboot.org/c/coreboot/+/45320/1//COMMIT_MSG@12 
PS1, Line 12: sata
> nit: SATA

Ack

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45320 )

Change subject: soc/intel/common/block: Use pci_dev_request_bus_master for BM enabling
......................................................................

soc/intel/common/block: Use pci_dev_request_bus_master for BM enabling

Enabling Bus Master isn't required by the hardware, so we shouldn't need
to enable it at all. However, some payloads do not set this bit before
attempting DMA transfers, which results in boot failures.

Replace static sata_final() implementation for BM enabling with generic
pci_dev_request_bus_master() function.

This allows the user to control through Kconfig whether Bus Master
should be enabled.

TEST=Able to boot to OS from SATA device on CML platform.

Signed-off-by: Subrata Banik 
Change-Id: Icd086184fd6fa9c03c806c857f13fad5a9e78a3e
Reviewed-on: https://review.coreboot.org/c/coreboot/+/45320
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/soc/intel/common/block/sata/sata.c
1 file changed, 1 insertion(+), 7 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/common/block/sata/sata.c b/src/soc/intel/common/block/sata/sata.c
index 1889700..7b234a9 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_request_bus_master,
    .ops_pci		= &pci_dev_ops_pci,
 };

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45320 )

Change subject: soc/intel/common/block: Use pci_dev_request_bus_master for BM enabling
......................................................................

Patch Set 3:

Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9
"QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19253
"QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19252
"QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19251
"QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19250
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19249
"HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19257
"HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19256
"HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19255
"HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19254

Please note: This test is under development and might not be accurate at all!