[coreboot-gerrit] Change in coreboot[master]: mainboard: Add ASRock G41C-GS

Sat May 6 01:04:40 CEST 2017

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/ich7_pci_irqs.asl
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/platform.asl
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/x4x_pci_irqs.asl
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_info.txt
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.layout
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.c
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/devicetree.cb
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.asl
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/romstage.c
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...

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I992ee07b742dfc59733ce0f3a9be202a530ec6cc
Gerrit-PatchSet: 6
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h at gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes