Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34424 )
Change subject: soc/amd/picasso: Add northbridge pci driver ......................................................................
Uploaded patch set 27.
(7 comments)
https://review.coreboot.org/c/coreboot/+/34424/26//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34424/26//COMMIT_MSG@12 PS26, Line 12:
I thought same parent commit wrote, there isn’t a northbridge.
lol, I guess I didn't know what to call PCI0?
https://review.coreboot.org/c/coreboot/+/34424/26/src/soc/amd/picasso/northb... File src/soc/amd/picasso/northbridge.c:
https://review.coreboot.org/c/coreboot/+/34424/26/src/soc/amd/picasso/northb... PS26, Line 18: 0x10
What was 0x10 chosen here?
No idea. Changed to 0.
https://review.coreboot.org/c/coreboot/+/34424/26/src/soc/amd/picasso/northb... PS26, Line 18: int idx = 0x10;
unsigned
Done
https://review.coreboot.org/c/coreboot/+/34424/26/src/soc/amd/picasso/northb... PS26, Line 32: ram_resource(dev, idx++, 1 * MiB / KiB, (mem_usable - 1 * MiB) / KiB);
MiB / KiB = KiB?
Correct. Since the call to ram_resources takes `basek`, I think it's easier to reason having the /KiB at the end. It's also consistent with all the other calls.
https://review.coreboot.org/c/coreboot/+/34424/26/src/soc/amd/picasso/northb... PS26, Line 67: char pscope[] = "\_SB.PCI0";
acpi_device_scope() is more correct. so one would just call the following on line 69: […]
Done
https://review.coreboot.org/c/coreboot/+/34424/26/src/soc/amd/picasso/northb... PS26, Line 88: .acpi_fill_ssdt = northbridge_fill_ssdt_generator,
Please use tabs as in the first line.
Done
https://review.coreboot.org/c/coreboot/+/34424/26/src/soc/amd/picasso/northb... PS26, Line 94: .device = PCI_DEVICE_ID_AMD_17H_MODEL_101F_NB,
Ditto.
Done