Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34424 )
Change subject: soc/amd/picasso: Add northbridge pci driver ......................................................................
Patch Set 31:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34424/31/src/soc/amd/picasso/northb... File src/soc/amd/picasso/northbridge.c:
https://review.coreboot.org/c/coreboot/+/34424/31/src/soc/amd/picasso/northb... PS31, Line 41: for (; hob->type != HOB_TYPE_END_OF_HOB_LIST; hob = fsp_next_hob(hob)) {
Here is the memory map I came up with after dumping the GUIDS: […]
This looks much better. Out of this, the following don't really need to be marked as separate reserved resources:
read_resources: RESERVED_MEMORY: addr: 0x00000000cd7fe000 (3287 MB) - BOOTLOADER_TOLUM_START length: 8192 (0 MB) end: 0x00000000cd800000 (3288 MB) - BOOTLOADER_TOLUM_END Owner GUID: 73ff4f56-aa8e-4451-b31636353667ad44 <- fsp_bootloader_tolum_guid BOOTLOADER_TOLUM read_resources: RESERVED_MEMORY: addr: 0x00000000cc7fe000 (3271 MB) length: 16777216 (16 MB) end: 0x00000000cd7fe000 (3287 MB) - BOOTLOADER_TOLUM_START Owner GUID: 69a79759-1373-4367-a6c4c7f59efd986e <- fsp_reserved_memory_guid FSP_RESERVED_MEMORY
They are already part of CBMEM.
I think we should be setting up the resources such that: 0 - 640KiB : ram_resource 640 - 768 KiB : mmio_resource 768KiB - 1MiB : reserved_ram_resource 1MiB - cbmem_top() : ram_resource cbmem_top() - TOM1 : reserved_ram_resource MMIO_CONF_BASE - ... : mmconf_resource 4GiB - TOM2: ram_resource
and not really have to parse the entire hob list in here. The only thing that is currently not understood is the 12MiB below TOM2. If we can identify that, it would help keep the whole map defined correctly here without having to parse the HOBs. Also, makes it simpler to visualize how the entire DRAM resources are laid out.
Anyways, if we don't really understand the 12MiB before TOM2, we can raise a bug for that and update this later on.