Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39658 )
Change subject: mb/asrock/q1900m: Add new mainboard ......................................................................
Patch Set 17:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39658/16/src/mainboard/asrock/q1900... File src/mainboard/asrock/q1900m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/39658/16/src/mainboard/asrock/q1900... PS16, Line 108: 4e.0
I might be wrong about this... This is usually set to 0c31. […]
Probably not, definitely not on TPM 2.0 stuff. I haven't tested this, but Asrock B85M Pro4 works just fine (even better than on vendor BIOS) with the same code and a TPM 2.0.
I could try using `device mmio 0xfed40000` instead (the actual device address).
https://review.coreboot.org/c/coreboot/+/39658/16/src/mainboard/asrock/q1900... File src/mainboard/asrock/q1900m/fadt.c:
https://review.coreboot.org/c/coreboot/+/39658/16/src/mainboard/asrock/q1900... PS16, Line 7: acpi_create_fadt
Please, why don't you use the one we already have here: src/acpi/acpi. […]
Why does this even build in the first place? Wait, did anyone select HAVE_ACPI_TABLES somewhere?
https://review.coreboot.org/c/coreboot/+/39658/16/src/mainboard/asrock/q1900... File src/mainboard/asrock/q1900m/irqroute.h:
https://review.coreboot.org/c/coreboot/+/39658/16/src/mainboard/asrock/q1900... PS16, Line 10: PCI_DEV_PIRQ_ROUTE(SD_DEV, C, D, E, F), \
This is off in DT?
Sure. I haven't checked these at all, so they are completely wrong.
https://review.coreboot.org/c/coreboot/+/39658/16/src/mainboard/asrock/q1900... PS16, Line 13: PCI_DEV_PIRQ_ROUTE(LPE_DEV, A, B, C, D), \ : PCI_DEV_PIRQ_ROUTE(MMC_DEV, D, E, F, G), \ : PCI_DEV_PIRQ_ROUTE(SIO1_DEV, A, B, C, D), \ : PCI_DEV_PIRQ_ROUTE(TXE_DEV, A, B, C, D), \
off in DT?
I want to enable TXE, but it doesn't respond and mei_txe shits a brick. The rest shall go away.
https://review.coreboot.org/c/coreboot/+/39658/16/src/mainboard/asrock/q1900... File src/mainboard/asrock/q1900m/romstage.c:
https://review.coreboot.org/c/coreboot/+/39658/16/src/mainboard/asrock/q1900... PS16, Line 8: static uint8_t spd[2][256]; #define SPD_SIZE 256 static uint8_t spd[NUM_CHANNELS][SPD_SIZE];
https://review.coreboot.org/c/coreboot/+/39658/16/src/mainboard/asrock/q1900... PS16, Line 26: 0x03
Are there defines somewhere for this?
probably in src/include/device/dram/ddr3.h
https://review.coreboot.org/c/coreboot/+/39658/16/src/mainboard/asrock/q1900... PS16, Line 27: spd[1]
spd[1] ends up unused?
No, MRC will access this data even though the two dram_data pointers are the same. See the comment on line 32. It accesses the contents of spd[0] through dram_data[0] and the contents of spd[1] through dram_data[1].
I can change `spd[0]` to `spd` on lines 33 and 34, though.