Attention is currently required from: Benjamin Doron, Christian Walter, David Milosevic, Lean Sheng Tan, Maximilian Brune, Naresh Solanki.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79086?usp=email )
Change subject: mainboard/emulation/qemu-sbsa: Add qemu-sbsa board ......................................................................
Patch Set 3:
(29 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/79086/comment/9069c39c_beca04e6 : PS3, Line 7: sbsa Would be good to explain what the SBSA acronym means (Server Base System Architecture) somewhere.
File src/mainboard/emulation/qemu-sbsa/Kconfig:
https://review.coreboot.org/c/coreboot/+/79086/comment/89d8c1b4_e0545c6f : PS3, Line 41: default y Why?
File src/mainboard/emulation/qemu-sbsa/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/79086/comment/ce92cbcb_d2af1280 : PS3, Line 4: o execute, do: : qemu-system-aarch64 -nographic -m 1024 -M sbsa-ref -pflash <TF-A> -pflash ./build/coreboot.rom These instructions would be better placed in documentation
File src/mainboard/emulation/qemu-sbsa/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/79086/comment/679f9e59_769a1825 : PS3, Line 23: truncate -s 256M $(obj)/coreboot.rom Why is this needed?
File src/mainboard/emulation/qemu-sbsa/acpi.c:
https://review.coreboot.org/c/coreboot/+/79086/comment/70fa7a10_5af50f91 : PS3, Line 36: his Typo: his ---> This
File src/mainboard/emulation/qemu-sbsa/board_info.txt:
https://review.coreboot.org/c/coreboot/+/79086/comment/3d81a160_1c1aa6b3 : PS3, Line 3: http HTTPS, please.
Even better, how about linking to the documentation page for this board in particular? https://www.qemu.org/docs/master/system/arm/sbsa.html
File src/mainboard/emulation/qemu-sbsa/dsdt.asl:
PS3: Some parts of this file are indented with tabs, and others with spaces. It'd be great if you could make whitespace usage consistent.
https://review.coreboot.org/c/coreboot/+/79086/comment/01b98517_07507995 : PS3, Line 10: Method (_STA) { \ : Return (0xF) \ : } _STA that always returns 0xf can be removed: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device...
https://review.coreboot.org/c/coreboot/+/79086/comment/2a3eb143_e4e57c03 : PS3, Line 14: Method (_SRS, 1) { } \ Looks like this device is meant to be non-configurable (OSPM can't change the IRQ number), as _SRS does nothing. Then, please remove _SRS and _PRS, and keep _CRS:
``` Name (_CRS, ResourceTemplate() { \ Interrupt (ResourceProducer, Level, ActiveHigh, Exclusive) { Irq } \ }) ```
See https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device... for more information.
https://review.coreboot.org/c/coreboot/+/79086/comment/77cdf3a3_a8a91e49 : PS3, Line 15: Method (_DIS) { } \ If _DIS does nothing, should it even exist?
https://review.coreboot.org/c/coreboot/+/79086/comment/bcbaca13_cb4d9720 : PS3, Line 21: } nit: closing brace is indented a bit too much
https://review.coreboot.org/c/coreboot/+/79086/comment/4bad8752_934ae3e1 : PS3, Line 31: // OEM revision Most instances of this comment are copy-pasta from other boards. If this is the case here, please drop
https://review.coreboot.org/c/coreboot/+/79086/comment/0dd19427_e92e98f1 : PS3, Line 45: Method (_STA) { : Return (0xF) : } _STA that always returns 0xf can be removed: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device...
https://review.coreboot.org/c/coreboot/+/79086/comment/aa0a26db_3fa92a38 : PS3, Line 53: Name (_CLS, Package (3) { : 0x01, : 0x06, : 0x01, : }) Idea for the future: make macros for these magic numbers. See https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device... for where to find them
https://review.coreboot.org/c/coreboot/+/79086/comment/84543058_13795628 : PS3, Line 63: Method (_STA) { : Return (0xF) : } _STA that always returns 0xf can be removed: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device...
https://review.coreboot.org/c/coreboot/+/79086/comment/3ef712b4_a26bca65 : PS3, Line 70: Name (_HID, "LNRO0D20") Indentation went up an extra level here and in the following blocks
https://review.coreboot.org/c/coreboot/+/79086/comment/16527f2a_81a44d08 : PS3, Line 72: Method (_STA) { : Return (0xF) : } _STA that always returns 0xf can be removed: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device...
https://review.coreboot.org/c/coreboot/+/79086/comment/fd0c25d1_90692dfe : PS3, Line 86: Method (_STA) { : Return (0xF) : } _STA that always returns 0xf can be removed: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device...
https://review.coreboot.org/c/coreboot/+/79086/comment/10bec14a_06497875 : PS3, Line 103: Device (PRT1) { Since you've used macros for other things, how about using another macro for the USB ports?
https://review.coreboot.org/c/coreboot/+/79086/comment/aebfcc6b_9dbfe249 : PS3, Line 117: Method (_STA) { : Return (0xF) : } _STA that always returns 0xf can be removed: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device...
https://review.coreboot.org/c/coreboot/+/79086/comment/b952542e_b1ffd578 : PS3, Line 190: Method (_STA) { : Return (0xF) : } _STA that always returns 0xf can be removed: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device...
https://review.coreboot.org/c/coreboot/+/79086/comment/a940c3d5_2d08ac2d : PS3, Line 205: PRT_ENTRY(0x0000FFFF, 0, GSI0), : PRT_ENTRY(0x0000FFFF, 0, GSI1), : PRT_ENTRY(0x0000FFFF, 0, GSI2), : PRT_ENTRY(0x0000FFFF, 0, GSI3), Pin is always 0 for these 4 entries, is this correct? It seems odd.
https://review.coreboot.org/c/coreboot/+/79086/comment/d730eb0b_ff75cde9 : PS3, Line 210: PRT_ENTRY(0x0001FFFF, 0, GSI1), : PRT_ENTRY(0x0001FFFF, 1, GSI2), : PRT_ENTRY(0x0001FFFF, 2, GSI3), : PRT_ENTRY(0x0001FFFF, 3, GSI0), Idea: For the sake of brevity, you could define another helper function:
``` #define PRT_ENTRY_GROUP(Address, Link0, Link1, Link2, Link3) \ PRT_ENTRY(Address, 0, Link0), \ PRT_ENTRY(Address, 1, Link1), \ PRT_ENTRY(Address, 2, Link2), \ PRT_ENTRY(Address, 3, Link3) ```
It would be used as follows:
``` PRT_ENTRY_GROUP(0x0001FFFF, GSI1, GSI2, GSI3, GSI0), ```
https://review.coreboot.org/c/coreboot/+/79086/comment/20fb54b5_fa8ebe33 : PS3, Line 453: Store (CDW2,SUPP) : Store (CDW3,CTRL) Should be equivalent:
``` SUPP = CDW2 CTRL = CDW3 ```
File src/mainboard/emulation/qemu-sbsa/include/mainboard/addressmap.h:
https://review.coreboot.org/c/coreboot/+/79086/comment/26466fb7_13856b64 : PS3, Line 4: * Base addreses for QEMU sbsa-ref machine
`'addreses' may be misspelled - perhaps 'addresses'?`
Please fix.
https://review.coreboot.org/c/coreboot/+/79086/comment/4669c845_d6e395d5 : PS3, Line 32: #define SBSA_PCIE_MMIO_HIGH_SIZE 0xff00000000 Maybe align the values with tabs?
File src/mainboard/emulation/qemu-sbsa/mainboard.c:
https://review.coreboot.org/c/coreboot/+/79086/comment/4c880510_f3413416 : PS3, Line 19: return (size_t)cbmem_top() - (uintptr_t)_dram;;
`Statements terminations use 1 semicolon`
Please fix.
https://review.coreboot.org/c/coreboot/+/79086/comment/bb5ba4f0_baefb91f : PS3, Line 161: struct device *cpu = alloc_dev(dev->link_list, &devpath);
`code indent should use tabs where possible`
Please fix.
https://review.coreboot.org/c/coreboot/+/79086/comment/0b992c1e_ea8a1646 : PS3, Line 168: nit: drop empty line