Hello Martin Roth, Furquan Shaikh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/34422
to review the following change.
Change subject: soc/amd/picasso: Update for vbios ......................................................................
soc/amd/picasso: Update for vbios
Add Picasso's Device ID and default filename.
Only a single Device ID is documented for Picasso so remove the oprom remapper function.
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: Iaf43d7c8da41beb05b58c494f0a6814f8f571b18 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/include/soc/pci_devs.h M src/soc/amd/picasso/northbridge.c 3 files changed, 3 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/34422/1
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 10e1930..840de12 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -93,14 +93,14 @@
config VGA_BIOS_ID string - default "1002,98e4" + default "1002,15d8" help The default VGA BIOS PCI vendor/device ID should be set to the result of the map_oprom_vendev() function in northbridge.c.
config VGA_BIOS_FILE string - default "3rdparty/blobs/soc/amd/picasso/VBIOS.bin" + default "3rdparty/blobs/soc/amd/picasso/PicassoGenericVbios.bin"
config S3_VGA_ROM_RUN bool diff --git a/src/soc/amd/picasso/include/soc/pci_devs.h b/src/soc/amd/picasso/include/soc/pci_devs.h index d6887f9..acde455 100644 --- a/src/soc/amd/picasso/include/soc/pci_devs.h +++ b/src/soc/amd/picasso/include/soc/pci_devs.h @@ -42,7 +42,7 @@ /* Internal Graphics */ #define GFX_DEV 0x1 #define GFX_FUNC 0 -#define GFX_DEVID 0x98e4 /* subject to SKU/OPN variation */ +#define GFX_DEVID 0x15d8 #define GFX_DEVFN PCI_DEVFN(GFX_DEV, GFX_FUNC) #define SOC_GFX_DEV _SOC_DEV(GFX_DEV, GFX_FUNC)
diff --git a/src/soc/amd/picasso/northbridge.c b/src/soc/amd/picasso/northbridge.c index 627ce03..a39e79a 100644 --- a/src/soc/amd/picasso/northbridge.c +++ b/src/soc/amd/picasso/northbridge.c @@ -329,19 +329,3 @@ assign_resources(dev->link_list); }
-/********************************************************************* - * Change the vendor / device IDs to match the generic VBIOS header. * - *********************************************************************/ -u32 map_oprom_vendev(u32 vendev) -{ - u32 new_vendev; - new_vendev = - ((vendev >= 0x100298e0) && (vendev <= 0x100298ef)) ? - 0x100298e0 : vendev; - - if (vendev != new_vendev) - printk(BIOS_NOTICE, "Mapping PCI device %8x to %8x\n", - vendev, new_vendev); - - return new_vendev; -}
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34422
to look at the new patch set (#2).
Change subject: soc/amd/picasso: Update for vbios ......................................................................
soc/amd/picasso: Update for vbios
Add Picasso's Device ID and default filename.
Only a single Device ID is documented for Picasso so remove the oprom remapper function.
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: Iaf43d7c8da41beb05b58c494f0a6814f8f571b18 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/include/soc/pci_devs.h M src/soc/amd/picasso/northbridge.c 3 files changed, 3 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/34422/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34422 )
Change subject: soc/amd/picasso: Update for vbios ......................................................................
Patch Set 2:
Can you add information if a) running it under yabel aka secure mode in coreboot works b) loading (atom?) oprom in coreboot is enough to have OS gfx work
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34422 )
Change subject: soc/amd/picasso: Update for vbios ......................................................................
Patch Set 2:
(1 comment)
I’d prefer if you splitted this in two commits.
https://review.coreboot.org/c/coreboot/+/34422/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34422/2//COMMIT_MSG@7 PS2, Line 7: soc/amd/picasso: Update for vbios Please make it a statement by using a verb (in imperative mood):
Update VBIOS
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34422 )
Change subject: soc/amd/picasso: Update for vbios ......................................................................
Patch Set 2:
(1 comment)
Can you add information if a) running it under yabel aka secure mode in coreboot works b) loading (atom?) oprom in coreboot is enough to have OS gfx work
I only have limited information at this time. a) It runs and returns successfully but I don't know whether the hardware was sufficiently initialized. b) There are some unpublished steps that are buried in AGESA, however I'm not aware of any additional requirements, e.g. no DRAM Ready message to the PSP like in ST. BTW AMD often says it's not necessary to run the vbios at all, but that's not seemed correct for any integrated graphics I've ever seen.
I’d prefer if you splitted this in two commits.
I don't see a natural way to split it, and it seems to me the changes go together pretty well. I'm not worried about ever needing a complicated revert since the files won't be used until later in history.
https://review.coreboot.org/c/coreboot/+/34422/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34422/2//COMMIT_MSG@7 PS2, Line 7: soc/amd/picasso: Update for vbios
Please make it a statement by using a verb (in imperative mood): […]
Done
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34422 )
Change subject: soc/amd/picasso: Add display identification and vbios name ......................................................................
Patch Set 11: Code-Review+2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34422 )
Change subject: soc/amd/picasso: Add display identification and vbios name ......................................................................
Patch Set 13: Code-Review+2
Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/34422 )
Change subject: soc/amd/picasso: Add display identification and vbios name ......................................................................
soc/amd/picasso: Add display identification and vbios name
Add Picasso's Device ID and default filename.
Only a single Device ID is documented for Picasso so remove the oprom remapper function.
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: Iaf43d7c8da41beb05b58c494f0a6814f8f571b18 Reviewed-on: https://review.coreboot.org/c/coreboot/+/34422 Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Richard Spiegel richard.spiegel@silverbackltd.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/include/soc/pci_devs.h M src/soc/amd/picasso/northbridge.c 3 files changed, 3 insertions(+), 20 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Richard Spiegel: Looks good to me, approved
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig index 33e8c81..6d0a3ef 100644 --- a/src/soc/amd/picasso/Kconfig +++ b/src/soc/amd/picasso/Kconfig @@ -111,14 +111,14 @@
config VGA_BIOS_ID string - default "1002,98e4" + default "1002,15d8" help The default VGA BIOS PCI vendor/device ID should be set to the result of the map_oprom_vendev() function in northbridge.c.
config VGA_BIOS_FILE string - default "3rdparty/blobs/soc/amd/picasso/VBIOS.bin" + default "3rdparty/blobs/soc/amd/picasso/PicassoGenericVbios.bin"
config S3_VGA_ROM_RUN bool diff --git a/src/soc/amd/picasso/include/soc/pci_devs.h b/src/soc/amd/picasso/include/soc/pci_devs.h index d6887f9..acde455 100644 --- a/src/soc/amd/picasso/include/soc/pci_devs.h +++ b/src/soc/amd/picasso/include/soc/pci_devs.h @@ -42,7 +42,7 @@ /* Internal Graphics */ #define GFX_DEV 0x1 #define GFX_FUNC 0 -#define GFX_DEVID 0x98e4 /* subject to SKU/OPN variation */ +#define GFX_DEVID 0x15d8 #define GFX_DEVFN PCI_DEVFN(GFX_DEV, GFX_FUNC) #define SOC_GFX_DEV _SOC_DEV(GFX_DEV, GFX_FUNC)
diff --git a/src/soc/amd/picasso/northbridge.c b/src/soc/amd/picasso/northbridge.c index 36135f9..08807f3 100644 --- a/src/soc/amd/picasso/northbridge.c +++ b/src/soc/amd/picasso/northbridge.c @@ -310,20 +310,3 @@
assign_resources(dev->link_list); } - -/********************************************************************* - * Change the vendor / device IDs to match the generic VBIOS header. * - *********************************************************************/ -u32 map_oprom_vendev(u32 vendev) -{ - u32 new_vendev; - new_vendev = - ((vendev >= 0x100298e0) && (vendev <= 0x100298ef)) ? - 0x100298e0 : vendev; - - if (vendev != new_vendev) - printk(BIOS_NOTICE, "Mapping PCI device %8x to %8x\n", - vendev, new_vendev); - - return new_vendev; -}
-
Kyösti Mälkki (Code Review) -
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)
-
Richard Spiegel (Code Review)