[coreboot-gerrit] Change in coreboot[master]: mb/foxconn/g41s-k: add new mainboard

Sun Jun 4 09:50:23 CEST 2017

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/ich7_pci_irqs.asl
File src/mainboard/foxconn/g41s-k/acpi/ich7_pci_irqs.asl:

https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/acpi/ich7_pci_irqs.asl@1
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/x4x_pci_irqs.asl
File src/mainboard/foxconn/g41s-k/acpi/x4x_pci_irqs.asl:

https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/acpi/x4x_pci_irqs.asl@26
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/x4x_pci_irqs.asl@43
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/x4x_pci_irqs.asl@54
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/devicetree.cb
File src/mainboard/foxconn/g41s-k/devicetree.cb:

https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicetree.cb@41
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/devicetree.cb@118
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/devicetree.cb@125
PS1, Line 125: irq 0xf1 = 0x50
default.

https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicetree.cb@133
PS1, Line 133: irq 0xf1 = 0x00
default

https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicetree.cb@135
PS1, Line 135: irq 0xf3 = 0x00
default

https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicetree.cb@137
PS1, Line 137: rq 0xf5 = 0x00
             : 						irq 0xf6 = 0x00
default

https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicetree.cb@148
PS1, Line 148: irq 0xf0 = 0x00
default

https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/devicetree.cb@154
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/devicetree.cb@172
PS1, Line 172: irq 0xf0 = 0x00
default

https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/dsdt.asl
File src/mainboard/foxconn/g41s-k/dsdt.asl:

https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/dsdt.asl@36
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/romstage.c
File src/mainboard/foxconn/g41s-k/romstage.c:

https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/romstage.c@60
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/romstage.c@80
PS1, Line 80: a0
is actually 0xa00

https://review.coreboot.org/#/c/20027/1/src/mainboard/foxconn/g41s-k/romstage.c@87
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.

-- 
To view, visit https://review.coreboot.org/20027
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc4c8935b1a11e55f4bf6cfa484a8a8d09b1adda
Gerrit-Change-Number: 20027
Gerrit-PatchSet: 1
Gerrit-Owner: Samuel Holland <samuel at sholland.org>
Gerrit-Reviewer: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Comment-Date: Sun, 04 Jun 2017 07:50:23 +0000
Gerrit-HasComments: Yes