Hello Patrick Rudolph, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/19258
to look at the new patch set (#16).
Change subject: sb/intel/*: Use common SMBus functions
......................................................................
sb/intel/*: Use common SMBus functions
All Intel southbridges implement the same SMBus functions.
This patch replaces all these similar and mostly identical
implementations with a common file.
This also makes i2c block read available to all those southbridges.
If the northbridge has to read a lot of SPD bytes sequentially, using
this function can reduce the time being spent to read SPD five-fold.
Change-Id: I93bb186e04e8c32dff04fc1abe4b5ecbc4c9c962
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/northbridge/intel/sandybridge/raminit.c
M src/southbridge/intel/bd82x6x/Kconfig
M src/southbridge/intel/bd82x6x/early_smbus.c
M src/southbridge/intel/bd82x6x/pch.h
M src/southbridge/intel/bd82x6x/smbus.c
D src/southbridge/intel/bd82x6x/smbus.h
M src/southbridge/intel/common/Kconfig
M src/southbridge/intel/common/Makefile.inc
R src/southbridge/intel/common/smbus.c
A src/southbridge/intel/common/smbus.h
M src/southbridge/intel/fsp_bd82x6x/Kconfig
M src/southbridge/intel/fsp_i89xx/Kconfig
M src/southbridge/intel/fsp_i89xx/early_smbus.c
M src/southbridge/intel/fsp_i89xx/pch.h
D src/southbridge/intel/fsp_i89xx/smbus.h
M src/southbridge/intel/fsp_rangeley/Kconfig
M src/southbridge/intel/fsp_rangeley/early_smbus.c
M src/southbridge/intel/fsp_rangeley/smbus.c
D src/southbridge/intel/fsp_rangeley/smbus.h
M src/southbridge/intel/fsp_rangeley/soc.h
M src/southbridge/intel/i3100/Kconfig
M src/southbridge/intel/i3100/early_smbus.c
M src/southbridge/intel/i3100/smbus.c
D src/southbridge/intel/i3100/smbus.h
M src/southbridge/intel/i82371eb/Kconfig
M src/southbridge/intel/i82371eb/early_smbus.c
M src/southbridge/intel/i82371eb/smbus.c
D src/southbridge/intel/i82371eb/smbus.h
M src/southbridge/intel/i82801ax/Kconfig
M src/southbridge/intel/i82801ax/early_smbus.c
M src/southbridge/intel/i82801ax/i82801ax.h
M src/southbridge/intel/i82801ax/smbus.c
D src/southbridge/intel/i82801ax/smbus.h
M src/southbridge/intel/i82801bx/Kconfig
M src/southbridge/intel/i82801bx/early_smbus.c
M src/southbridge/intel/i82801bx/i82801bx.h
M src/southbridge/intel/i82801bx/smbus.c
D src/southbridge/intel/i82801bx/smbus.h
M src/southbridge/intel/i82801dx/Kconfig
M src/southbridge/intel/i82801dx/early_smbus.c
M src/southbridge/intel/i82801dx/i82801dx.h
D src/southbridge/intel/i82801dx/smbus.c
M src/southbridge/intel/i82801ex/Kconfig
M src/southbridge/intel/i82801ex/early_smbus.c
M src/southbridge/intel/i82801ex/smbus.c
D src/southbridge/intel/i82801ex/smbus.h
M src/southbridge/intel/i82801gx/Kconfig
M src/southbridge/intel/i82801gx/early_smbus.c
M src/southbridge/intel/i82801gx/i82801gx.h
M src/southbridge/intel/i82801gx/smbus.c
D src/southbridge/intel/i82801gx/smbus.h
M src/southbridge/intel/i82801ix/Kconfig
M src/southbridge/intel/i82801ix/early_smbus.c
M src/southbridge/intel/i82801ix/i82801ix.h
M src/southbridge/intel/i82801ix/smbus.c
D src/southbridge/intel/i82801ix/smbus.h
M src/southbridge/intel/ibexpeak/Kconfig
M src/southbridge/intel/ibexpeak/early_smbus.c
M src/southbridge/intel/ibexpeak/pch.h
M src/southbridge/intel/ibexpeak/smbus.c
M src/southbridge/intel/lynxpoint/Kconfig
M src/southbridge/intel/lynxpoint/early_smbus.c
M src/southbridge/intel/lynxpoint/pch.h
M src/southbridge/intel/lynxpoint/smbus.c
D src/southbridge/intel/lynxpoint/smbus.h
65 files changed, 237 insertions(+), 1,913 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/19258/16
--
To view, visit https://review.coreboot.org/19258
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I93bb186e04e8c32dff04fc1abe4b5ecbc4c9c962
Gerrit-PatchSet: 16
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
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/i…
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/p…
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/x…
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_…
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.l…
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/cstate…
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/device…
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.a…
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/romsta…
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(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes
Martin Roth has posted comments on this change. ( https://review.coreboot.org/19258 )
Change subject: sb/intel/*: Use common SMBus functions
......................................................................
Patch Set 15:
(6 comments)
https://review.coreboot.org/#/c/19258/15/src/southbridge/intel/common/smbus…
File src/southbridge/intel/common/smbus.c:
PS15, Line 6: * Copyright (C) 2013 Vladimir Serbinenko
I was going to comment on the copyright removal, but it looks like this is just an issue with git choosing which of the multiple deleted versions is the rename.
PS15, Line 96: {
Braces aren't needed for a single line if. Same for all the following cases.
PS15, Line 176: Setup
May as well fix it while we're here, i guess.
/* Set up transaction */
https://review.coreboot.org/#/c/19258/15/src/southbridge/intel/common/smbus…
File src/southbridge/intel/common/smbus.h:
Line 3: *
can we make sure this retains the correct copyrights if possible?
PS15, Line 14: SMBUS_H
INTEL_COMMON_SMBUS_H maybe?
It won't actually conflict with anything, but we have a lot of smbus.h files in the tree, and it'd be a pain if they all just used SMBUS_H for the guards.
PS15, Line 32:
Remove some extra lines?
--
To view, visit https://review.coreboot.org/19258
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I93bb186e04e8c32dff04fc1abe4b5ecbc4c9c962
Gerrit-PatchSet: 15
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes
Arthur Heymans has uploaded a new change for review. ( https://review.coreboot.org/19602 )
Change subject: mb/asus/p5gc-mx: Implement flawed resume from S3 support
......................................................................
mb/asus/p5gc-mx: Implement flawed resume from S3 support
On s3 resume BSEL straps are not configured which require a hard reset
to work which conflicts with s3 resume. The result is that on s3
resume the cpu is run at 800MHz FSB. This however does not work with
each CPU, but it's better than no S3 support at all.
Change-Id: I6ac927ee9dcce15fc7621aad57969fae8f5805ca
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/mainboard/asus/p5gc-mx/Kconfig
M src/mainboard/asus/p5gc-mx/devicetree.cb
M src/mainboard/asus/p5gc-mx/romstage.c
3 files changed, 14 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/19602/1
diff --git a/src/mainboard/asus/p5gc-mx/Kconfig b/src/mainboard/asus/p5gc-mx/Kconfig
index 445d932..83001e0 100644
--- a/src/mainboard/asus/p5gc-mx/Kconfig
+++ b/src/mainboard/asus/p5gc-mx/Kconfig
@@ -21,7 +21,6 @@
select CPU_INTEL_SOCKET_LGA775
select NORTHBRIDGE_INTEL_I945
select NORTHBRIDGE_INTEL_SUBTYPE_I945GC
- select CHECK_SLFRCS_ON_RESUME
select SOUTHBRIDGE_INTEL_I82801GX
select SUPERIO_WINBOND_W83627DHG
select HAVE_OPTION_TABLE
diff --git a/src/mainboard/asus/p5gc-mx/devicetree.cb b/src/mainboard/asus/p5gc-mx/devicetree.cb
index 824beed..a7394e0 100644
--- a/src/mainboard/asus/p5gc-mx/devicetree.cb
+++ b/src/mainboard/asus/p5gc-mx/devicetree.cb
@@ -116,6 +116,7 @@
device pnp 2e.9 on end # GPIO2-5
device pnp 2e.a on # ACPI
irq 0x70 = 0
+ irq 0xe4 = 0x10 # VSBGATE# to power dram during S3
end
device pnp 2e.b on # HWM
io 0x60 = 0x290
diff --git a/src/mainboard/asus/p5gc-mx/romstage.c b/src/mainboard/asus/p5gc-mx/romstage.c
index efeaaec..a29478c 100644
--- a/src/mainboard/asus/p5gc-mx/romstage.c
+++ b/src/mainboard/asus/p5gc-mx/romstage.c
@@ -49,7 +49,7 @@
* BSEL1 is connected with GPIO33 with inversed logic
* BSEL2 is connected with GPIO55
*/
-static void setup_sio_gpio(u8 bsel)
+static int setup_sio_gpio(u8 bsel)
{
int need_reset = 0;
u8 reg, old_reg;
@@ -83,13 +83,7 @@
pnp_exit_ext_func_mode(GPIO_DEV);
- if (need_reset) {
- int i = 1000;
- while (i--)
- outb(i & 0xff, 0x80);
- outb(0xe, 0xcf9);
- halt();
- }
+ return need_reset;
}
static u8 msr_get_fsb(void)
@@ -197,7 +191,6 @@
{
int s3resume = 0, boot_mode = 0;
- u8 m_bsel;
u8 c_bsel = msr_get_fsb();
timestamp_init(get_initial_timestamp());
@@ -209,8 +202,6 @@
ich7_enable_lpc();
winbond_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE);
-
- setup_sio_gpio(c_bsel);
/* Set up the console */
console_init();
@@ -228,11 +219,19 @@
*/
i945_early_initialization();
- m_bsel = MCHBAR32(CLKCFG) & 7;
- printk(BIOS_DEBUG, "CPU BSEL: 0x%x\nMCH BSEL: 0x%x\n", c_bsel, m_bsel);
-
s3resume = southbridge_detect_s3_resume();
+ /*
+ * Result is that FSB is incorrect on s3 resume (fixed at 800MHz).
+ * Some CPU accept this others don't.
+ */
+ if (!s3resume && setup_sio_gpio(c_bsel)) {
+ printk(BIOS_DEBUG,
+ "Needs reset to configure CPU BSEL straps\n");
+ outb(0xe, 0xcf9);
+ halt();
+ }
+
/* Enable SPD ROMs and DDR-II DRAM */
enable_smbus();
--
To view, visit https://review.coreboot.org/19602
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6ac927ee9dcce15fc7621aad57969fae8f5805ca
Gerrit-PatchSet: 1
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>