Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/18993 )
Change subject: mainboard: Add ASRock G41C-GS ......................................................................
Patch Set 6:
(25 comments)
Thanks for reviewing.
https://review.coreboot.org/#/c/18993/6/src/mainboard/asrock/g41c-gs/Kconfig File src/mainboard/asrock/g41c-gs/Kconfig:
Line 30: select MAINBOARD_HAS_NATIVE_VGA_INIT_TEXTMODECFG
This and INTEL_EDID could be selected in nb's Kconfig with `if
Agreed. will do in a follow up patch or base this against one that does since it affects other boards too.
Line 36: select HAVE_ACPI_RESUME
Commit message says no.
True but I have a very much suspect LDN 0xa offset 0xe4 bit4 fixes it (set in devicetree) (I've seen this work on other multiple other similar Intel boards) So I left it in just in case it works.
Line 40: default 0xe0000000
Why is it not set in nb? How many buses are set? if < 256, it could
it seems very popular to set this in board Kconfig... Ofc default should be in nb.
Line 52: default 4
Hmmm, never thought about it, but should this be set per socket?
Total afaict from amd multisocket boards.
https://review.coreboot.org/#/c/18993/6/src/mainboard/asrock/g41c-gs/acpi/ic... File src/mainboard/asrock/g41c-gs/acpi/ich7_pci_irqs.asl:
Why are the devices sorted backwards?
Vendor DSDT... will sort it increasingly.
PS6, Line 16:
line break here, please
ok.
Line 22: Package() { 0x0008ffff, 0, 0, 0x14},
Why only one pin? Is this an onboard device?
nothing in lspci afaict so few ideas: * onboard device that might exist on a variant board * shitty vendor DSDT.
Since I don't have schematics, its probably better to leave it here.
https://review.coreboot.org/#/c/18993/6/src/mainboard/asrock/g41c-gs/acpi/pl... File src/mainboard/asrock/g41c-gs/acpi/platform.asl:
Is this mb specific?
nope and I don't think SMI is being used either.
https://review.coreboot.org/#/c/18993/6/src/mainboard/asrock/g41c-gs/acpi/x4... File src/mainboard/asrock/g41c-gs/acpi/x4x_pci_irqs.asl:
Line 1: /* would be nice if it could be rebased against https://review.coreboot.org/#/c/19017/ (provide a common default for ich7) to simply avoid this file
PS6, Line 39: 0:1f.2, 0:1f.3
PATA is 1f.1
yes comment is incorrect. does smbus have an IRQ?
https://review.coreboot.org/#/c/18993/6/src/mainboard/asrock/g41c-gs/board_i... File src/mainboard/asrock/g41c-gs/board_info.txt:
Line 6
Flashrom support?
ok
https://review.coreboot.org/#/c/18993/6/src/mainboard/asrock/g41c-gs/cmos.la... File src/mainboard/asrock/g41c-gs/cmos.layout:
PS6, Line 60: #424 1 e 2 hyper_threading
stale comment?
Yes those CPUs don't work. I have a vague memory that this option disables SMP/multicore on Thinkpad x60 (yonah)...
https://review.coreboot.org/#/c/18993/6/src/mainboard/asrock/g41c-gs/cstates... File src/mainboard/asrock/g41c-gs/cstates.c:
PS6, Line 17: #include <device/device.h> : #include <southbridge/intel/i82801gx/i82801gx.h>
Are these needed?
will check it.
Line 20: static acpi_cstate_t cst_entries[] = {};
Doesn't work?
I think linux sometimes decides there are 2 c-states with this (so independent of ACPI). could be removed and have the function return 0.
Line 25: return ARRAY_SIZE(cst_entries);
Just return 0?
ok.
https://review.coreboot.org/#/c/18993/6/src/mainboard/asrock/g41c-gs/devicet... File src/mainboard/asrock/g41c-gs/devicetree.cb:
PS6, Line 39: register "pirqa_routing" = "0x8b" : register "pirqb_routing" = "0x8a" : register "pirqc_routing" = "0x86" : register "pirqd_routing" = "0x85" : register "pirqe_routing" = "0x80" : register "pirqf_routing" = "0x80" : register "pirqg_routing" = "0x80" : register "pirqh_routing" = "0x83"
Does it make sense to set values but disable them (bit7)?
probably copied from vendor. In https://review.coreboot.org/#/c/19017/ I want to set them all to 11 like on sandy bridge.
Line 57: register "sata_ahci" = "0x0" # AHCI does not work
why? irq problems?
ICH7 has variants without AHCI. this is one of them sadly...
https://review.coreboot.org/#/c/18993/6/src/mainboard/asrock/g41c-gs/dsdt.as... File src/mainboard/asrock/g41c-gs/dsdt.asl:
Line 37: #include <drivers/intel/gma/acpi/default_brightness_levels.asl>
Would be a bug if it's needed, wouldn't it?
ok
https://review.coreboot.org/#/c/18993/6/src/mainboard/asrock/g41c-gs/romstag... File src/mainboard/asrock/g41c-gs/romstage.c:
PS6, Line 45: pci_devfn_t dev; : : /* Southbridge GPIOs. */ : dev = PCI_DEV(0x0, 0x1f, 0x0);
Could just use a #define.
ok
Line 57:
Settings below are not GPIO related, please move. And please use
ok.
I'd hope to rebase PIRQ settings on https://review.coreboot.org/#/c/19017/
Line 59: RCBA32(0x3100) = 0x00042210;
This is the reset default.
ok.
PS6, Line 60: RCBA32(0x3104) = 0x00002100; : RCBA32(0x3108) = 0x10004321; : RCBA32(0x310c) = 0x00214321; : RCBA32(0x3110) = 0x00000001;
These too.
ok.
Line 87: pci_write_config8(PCI_DEV(0, 0x1f, 0), SERIRQ_CNTL, 0xd0);
Didn't check what it does, though it gets overwritten in ramstage
not sure but probably not.
PS6, Line 91: COMB_LPC_EN
Can't spot this in the devicetree. Also both COMs are set to 3f8 above.
ok
Line 93: pci_write_config32(PCI_DEV(0, 0x1f, 0), 0x84, 0x00fc0281);
Not sure, what the range is exactly used for, is it needed in romstage?
I doubt it...