Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/20027 )
Change subject: mb/foxconn/g41s-k: add new mainboard ......................................................................
Patch Set 1:
(17 comments)
Looks quite ok! just a few suggestions for improvements.
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/acpi/ic... File src/mainboard/foxconn/g41s-k/acpi/ich7_pci_irqs.asl:
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/acpi/ic... PS1, Line 1: /* It looks like there are no PCI devices on this board. did you find this in vendor DSDT?
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/acpi/x4... File src/mainboard/foxconn/g41s-k/acpi/x4x_pci_irqs.asl:
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/acpi/x4... PS1, Line 26: Package() { 0x0001ffff, 1, 0, 0x11 }, : Package() { 0x0001ffff, 2, 0, 0x12 }, : Package() { 0x0001ffff, 3, 0, 0x13 }, I don't think it has anything but PINA, so can be removed.
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/acpi/x4... PS1, Line 43: CI Bridge while correct, those IRQ are used for AC ‘97 Audio Pin (AAIP) -> PINA and AC ‘97 Modem Pin (AMIP) -> PINB
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/acpi/x4... PS1, Line 54: Package() { 0x0001ffff, 1, _SB.PCI0.LPCB.LNKB, 0 }, : Package() { 0x0001ffff, 2, _SB.PCI0.LPCB.LNKC, 0 }, : Package() { 0x0001ffff, 3, _SB.PCI0.LPCB.LNKD, 0 }, same.
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicet... File src/mainboard/foxconn/g41s-k/devicetree.cb:
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicet... PS1, Line 41: register "pirqa_routing" = "0x0a" : register "pirqb_routing" = "0x0b" : register "pirqc_routing" = "0x0a" : register "pirqd_routing" = "0x05" : register "pirqe_routing" = "0x0a" : register "pirqf_routing" = "0x0b" : register "pirqg_routing" = "0x0a" : register "pirqh_routing" = "0x03" often we set all these to 11. only matters in PIC mode which modern OS don't use by default.
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicet... PS1, Line 118: irq 0xf0 = 0x00 : irq 0xf1 = 0x50 default. no need to have this here
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicet... PS1, Line 125: irq 0xf1 = 0x50 default.
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicet... PS1, Line 133: irq 0xf1 = 0x00 default
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicet... PS1, Line 135: irq 0xf3 = 0x00 default
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicet... PS1, Line 137: rq 0xf5 = 0x00 : irq 0xf6 = 0x00 default
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicet... PS1, Line 148: irq 0xf0 = 0x00 default
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicet... PS1, Line 154: irq 0xf0 = 0x00 : irq 0xf1 = 0x00 : irq 0xf2 = 0x00 : irq 0xf3 = 0x00 : irq 0xf4 = 0x00 : irq 0xf5 = 0x00 : irq 0xf6 = 0x22 : irq 0xf7 = 0x00 : irq 0xf8 = 0x00 : irq 0xf9 = 0x00 : irq 0xfa = 0x00 : irq 0xfb = 0x00 : irq 0xfd = 0x00 : irq 0xfe = 0x00 default is 0 on all these, so just leave 0xf6?
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicet... PS1, Line 172: irq 0xf0 = 0x00 default
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/dsdt.as... File src/mainboard/foxconn/g41s-k/dsdt.asl:
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/dsdt.as... PS1, Line 36: #include <southbridge/intel/i82801gx/acpi/ich7.asl> maybe remove if there is no PCI device.
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/romstag... File src/mainboard/foxconn/g41s-k/romstage.c:
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/romstag... PS1, Line 60: RCBA16(D31IR) = 0x0132; : RCBA16(D30IR) = 0x3241; : RCBA16(D29IR) = 0x0237; no need to do this but if you want you can leave this at defaults, and have A,B,C,D for each relevant pin in ACPI PIRQ routing.
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/romstag... PS1, Line 80: a0 is actually 0xa00
https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/romstag... PS1, Line 87: 0x50, 0, 0x52 The board has only one dimm slot? removing it would save a bit of time in raminit since it is one dimm less to probe.
-
Arthur Heymans (Code Review)