Change in ...coreboot[master]: [WIP] AGESA/binaryPI: Add NULL pointers check

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31026

Change subject: [WIP] AGESA/binaryPI: Add NULL pointers check
......................................................................

[WIP] AGESA/binaryPI: Add NULL pointers check

Previously used call dev_find_slot() returned
pointers to PCI device nodes that were actually
not present in the hardware at all. Register
reads would come back with invalid (0xff) values
and writes ignored.

After change to pcidev_on_root(), attempting to
do register operations with non-present PCI
hardware immediately halts with error
  get_pbus: dev is NULL!

Change-Id: I785350c171a642207c5fab884a953d45a3bfe592
Signed-off-by: Kyösti Mälkki 
---
M src/northbridge/amd/pi/00730F01/northbridge.c
1 file changed, 9 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/31026/1

diff --git a/src/northbridge/amd/pi/00730F01/northbridge.c b/src/northbridge/amd/pi/00730F01/northbridge.c
index 2579d37..4066d12 100644
--- a/src/northbridge/amd/pi/00730F01/northbridge.c
+++ b/src/northbridge/amd/pi/00730F01/northbridge.c
@@ -787,14 +787,18 @@
    struct device *dev;
    u32 value;
    dev = pcidev_on_root(0, 0); /* clear IoapicSbFeatureEn */
-	pci_write_config32(dev, 0xF8, 0);
-	pci_write_config32(dev, 0xFC, 5); /* TODO: move it to dsdt.asl */
+	if (dev != NULL) {
+		pci_write_config32(dev, 0xF8, 0);
+		pci_write_config32(dev, 0xFC, 5); /* TODO: move it to dsdt.asl */
+	}

/* disable No Snoop */
    dev = pcidev_on_root(1, 1);
-	value = pci_read_config32(dev, 0x60);
-	value &= ~(1 << 11);
-	pci_write_config32(dev, 0x60, value);
+	if (dev != NULL) {
+		value = pci_read_config32(dev, 0x60);
+		value &= ~(1 << 11);
+		pci_write_config32(dev, 0x60, value);
+	}
 }

struct chip_operations northbridge_amd_pi_00730F01_ops = {

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31026 )

Change subject: [WIP] AGESA/binaryPI: Add NULL pointers check
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31026/1/src/northbridge/amd/pi/00730F01/nort... 
File src/northbridge/amd/pi/00730F01/northbridge.c:

https://review.coreboot.org/#/c/31026/1/src/northbridge/amd/pi/00730F01/nort... 
PS1, Line 792: 		pci_write_config32(dev, 0xFC, 5); /* TODO: move it to dsdt.asl */
line over 80 characters

[Angel Pons (Code Review)]

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

Change subject: [WIP] AGESA/binaryPI: Add NULL pointers check
......................................................................

Patch Set 1: Code-Review+1

Change in ...coreboot[master]: AGESA/binaryPI: Add NULL pointers check

[Kyösti Mälkki (Code Review)]

Hello Piotr Król, Angel Pons, build bot (Jenkins), Michał Żygowski,

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

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

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

Change subject: AGESA/binaryPI: Add NULL pointers check
......................................................................

AGESA/binaryPI: Add NULL pointers check

Fix regression after commits
  4ad7f5b AGESA: Use pcidev_on_root()
  33ff44c binaryPI: Use pcidev_on_root()

Previously used call dev_find_slot() returned
pointers to PCI device nodes that were actually
not present in the hardware at all. Register
reads would come back with invalid (0xff) values
and writes would be ignored.

After change to pcidev_on_root(), attempting to
do register operations with non-present PCI
hardware immediately halts with error
  get_pbus: dev is NULL!

Change-Id: I785350c171a642207c5fab884a953d45a3bfe592
Signed-off-by: Kyösti Mälkki 
---
M src/northbridge/amd/agesa/family16kb/northbridge.c
M src/northbridge/amd/pi/00660F01/northbridge.c
M src/northbridge/amd/pi/00730F01/northbridge.c
3 files changed, 15 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/31026/2

Change in ...coreboot[master]: AGESA/binaryPI: Add NULL pointers check

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31026 )

Change subject: AGESA/binaryPI: Add NULL pointers check
......................................................................

Patch Set 2: Code-Review+1

apu2 is now booting fine. Thank You.

Change in ...coreboot[master]: AGESA/binaryPI: Add NULL pointers check

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31026 )

Change subject: AGESA/binaryPI: Add NULL pointers check
......................................................................

Patch Set 2: Code-Review+1

Change in ...coreboot[master]: AGESA/binaryPI: Add NULL pointers check

[Angel Pons (Code Review)]

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

Change subject: AGESA/binaryPI: Add NULL pointers check
......................................................................

Patch Set 2: Code-Review+2

Change in ...coreboot[master]: AGESA/binaryPI: Add NULL pointers check

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31026 )

Change subject: AGESA/binaryPI: Add NULL pointers check
......................................................................

Patch Set 2:

(1 comment)

I spent a little time looking at build//static.c for apu2 vs. a stoneyridge board.  It's not immediately clear to me why apu2 isn't working but grunt is OK.

https://review.coreboot.org/#/c/31026/2/src/northbridge/amd/agesa/family16kb... 
File src/northbridge/amd/agesa/family16kb/northbridge.c:

https://review.coreboot.org/#/c/31026/2/src/northbridge/amd/agesa/family16kb... 
PS2, Line 576: 	dev = pcidev_on_root(1, 1);
For each file, maybe reverse the check to print an error and get out?

if (!dev) {
    printk(BIOS_ERR, "Can't find device 1.1 to disable NoSnoop\n");
    return;
  }

Change in ...coreboot[master]: AGESA/binaryPI: Add NULL pointers check

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31026 )

Change subject: AGESA/binaryPI: Add NULL pointers check
......................................................................

Patch Set 2:
> Patch Set 2:
>
> (1 comment)
>
> I spent a little time looking at build//static.c for apu2 vs. a stoneyridge board.  It's not immediately clear to me why apu2 isn't working but grunt is OK.

These apu2 variant boards are headless, integrated graphices 0:1.0 and 0:1.1 are disabled devices (which may not even exist in the silicon at all).

Change in ...coreboot[master]: AGESA/binaryPI: Add NULL pointers check

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31026 )

Change subject: AGESA/binaryPI: Add NULL pointers check
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31026/2/src/northbridge/amd/agesa/family16kb... 
File src/northbridge/amd/agesa/family16kb/northbridge.c:

https://review.coreboot.org/#/c/31026/2/src/northbridge/amd/agesa/family16kb... 
PS2, Line 576: 	dev = pcidev_on_root(1, 1);
> For each file, maybe reverse the check to print an error and get out? […]

It is not an error to have IGD 0:1.0 and associated HDA(?) 0:1.1 not present.

Change in ...coreboot[master]: AGESA/binaryPI: Add NULL pointers check

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31026 )

Change subject: AGESA/binaryPI: Add NULL pointers check
......................................................................

AGESA/binaryPI: Add NULL pointers check

Fix regression after commits
  4ad7f5b AGESA: Use pcidev_on_root()
  33ff44c binaryPI: Use pcidev_on_root()

Previously used call dev_find_slot() returned
pointers to PCI device nodes that were actually
not present in the hardware at all. Register
reads would come back with invalid (0xff) values
and writes would be ignored.

After change to pcidev_on_root(), attempting to
do register operations with non-present PCI
hardware immediately halts with error
  get_pbus: dev is NULL!

Change-Id: I785350c171a642207c5fab884a953d45a3bfe592
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/31026
Tested-by: build bot (Jenkins) 
Reviewed-by: Michał Żygowski 
Reviewed-by: Paul Menzel 
Reviewed-by: Angel Pons 
---
M src/northbridge/amd/agesa/family16kb/northbridge.c
M src/northbridge/amd/pi/00660F01/northbridge.c
M src/northbridge/amd/pi/00730F01/northbridge.c
3 files changed, 15 insertions(+), 9 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved
  Michał Żygowski: Looks good to me, but someone else must approve

diff --git a/src/northbridge/amd/agesa/family16kb/northbridge.c b/src/northbridge/amd/agesa/family16kb/northbridge.c
index da54fd8..b283094 100644
--- a/src/northbridge/amd/agesa/family16kb/northbridge.c
+++ b/src/northbridge/amd/agesa/family16kb/northbridge.c
@@ -574,9 +574,11 @@

/* disable No Snoop */
    dev = pcidev_on_root(1, 1);
-	value = pci_read_config32(dev, 0x60);
-	value &= ~(1 << 11);
-	pci_write_config32(dev, 0x60, value);
+	if (dev != NULL) {
+		value = pci_read_config32(dev, 0x60);
+		value &= ~(1 << 11);
+		pci_write_config32(dev, 0x60, value);
+	}
 }

struct chip_operations northbridge_amd_agesa_family16kb_ops = {
diff --git a/src/northbridge/amd/pi/00660F01/northbridge.c b/src/northbridge/amd/pi/00660F01/northbridge.c
index 056e701..00558a5 100644
--- a/src/northbridge/amd/pi/00660F01/northbridge.c
+++ b/src/northbridge/amd/pi/00660F01/northbridge.c
@@ -558,9 +558,11 @@

/* disable No Snoop */
    dev = pcidev_on_root(1, 1);
-	value = pci_read_config32(dev, 0x60);
-	value &= ~(1 << 11);
-	pci_write_config32(dev, 0x60, value);
+	if (dev != NULL) {
+		value = pci_read_config32(dev, 0x60);
+		value &= ~(1 << 11);
+		pci_write_config32(dev, 0x60, value);
+	}
 }

struct chip_operations northbridge_amd_pi_00660F01_ops = {
diff --git a/src/northbridge/amd/pi/00730F01/northbridge.c b/src/northbridge/amd/pi/00730F01/northbridge.c
index 2579d37..81517af 100644
--- a/src/northbridge/amd/pi/00730F01/northbridge.c
+++ b/src/northbridge/amd/pi/00730F01/northbridge.c
@@ -792,9 +792,11 @@

/* disable No Snoop */
    dev = pcidev_on_root(1, 1);
-	value = pci_read_config32(dev, 0x60);
-	value &= ~(1 << 11);
-	pci_write_config32(dev, 0x60, value);
+	if (dev != NULL) {
+		value = pci_read_config32(dev, 0x60);
+		value &= ~(1 << 11);
+		pci_write_config32(dev, 0x60, value);
+	}
 }

struct chip_operations northbridge_amd_pi_00730F01_ops = {