Hello Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/18993
to look at the new patch set (#7).
Change subject: mainboard: Add ASRock G41C-GS
......................................................................
mainboard: Add ASRock G41C-GS
Start-point is Gigabyte GA-G41M-ES2L.
This board features a G41 northbridge and an ICH7 southbridge. This
board has slots for both DDR2 and DDR3 (cannot run concurrently
though) but only DDR2 is implemented in coreboot. The SPI flash
resides in a DIP-8 socket.
Tested and working:
* DDR2 dual channel (PC2 5300 and PC2 6400, though raminit is picky
with assymetric dimm setups);
* 3,5" IDE;
* SATA;
* PCIe x16 (with some patches up for review);
* Uart, PS2 Keyboard;
* USB, ethernet, audio;
* Native graphic init;
* Fan control;
* Reboot, poweroff;
* Flashrom (vendor and coreboot).
Tested but fails:
* Raminit on resume from S3 (might work with latest patch enabling SIO
3VSBSW# but UNTESTED);
* DDR3 (not implemented in coreboot).
Tests were run with SeaBIOS and Debian sid, using Linux 4.9.0.
Change-Id: I992ee07b742dfc59733ce0f3a9be202a530ec6cc
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
A src/mainboard/asrock/g41c-gs/Kconfig
A src/mainboard/asrock/g41c-gs/Kconfig.name
A src/mainboard/asrock/g41c-gs/Makefile.inc
A src/mainboard/asrock/g41c-gs/acpi/ec.asl
A src/mainboard/asrock/g41c-gs/acpi/ich7_pci_irqs.asl
A src/mainboard/asrock/g41c-gs/acpi/platform.asl
A src/mainboard/asrock/g41c-gs/acpi/superio.asl
A src/mainboard/asrock/g41c-gs/acpi/x4x_pci_irqs.asl
A src/mainboard/asrock/g41c-gs/acpi_tables.c
A src/mainboard/asrock/g41c-gs/board_info.txt
A src/mainboard/asrock/g41c-gs/cmos.default
A src/mainboard/asrock/g41c-gs/cmos.layout
A src/mainboard/asrock/g41c-gs/cstates.c
A src/mainboard/asrock/g41c-gs/devicetree.cb
A src/mainboard/asrock/g41c-gs/dsdt.asl
A src/mainboard/asrock/g41c-gs/gpio.c
A src/mainboard/asrock/g41c-gs/hda_verb.c
A src/mainboard/asrock/g41c-gs/romstage.c
18 files changed, 869 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/18993/7
--
To view, visit https://review.coreboot.org/18993
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I992ee07b742dfc59733ce0f3a9be202a530ec6cc
Gerrit-PatchSet: 7
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>
Hello Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/19594
to look at the new patch set (#2).
Change subject: sb/intel/i82801gx: Implement smbus block r/w functions
......................................................................
sb/intel/i82801gx: Implement smbus block r/w functions
Uses common hardware access functions to make smbus block read and
write available in romstage.
Those are needed to reconfigure the clockgen on smbus offset 0x69,
which is sometimes needed for things like CPU C-states or analog
display out to work properly.
Change-Id: I0a06178d2474ce65972de157cb437b42f3354da0
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/southbridge/intel/i82801gx/early_smbus.c
M src/southbridge/intel/i82801gx/i82801gx.h
2 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/19594/2
--
To view, visit https://review.coreboot.org/19594
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0a06178d2474ce65972de157cb437b42f3354da0
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/19567 )
Change subject: intelmetool: Use correct type for pointer
......................................................................
intelmetool: Use correct type for pointer
Use `uintptr_t` instead of `uint32_t`, fixing the error below on 64-bit
systems, where pointers are 64-bit wide.
```
cc -O0 -g -Wall -W -Wno-unused-parameter -Wno-unused-but-set-variable -Wno-sign-compare -Wno-unused-function -c -o intelmetool.o intelmetool.c
intelmetool.c: In function ‘dump_me_memory’:
intelmetool.c:85:45: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
dump = map_physical_exact((off_t)me_clone, (void *)me_clone, 0x2000000);
^
```
BUG=https://ticket.coreboot.org/issues/111
Change-Id: Id8d778e97090668ad9308a82b44c6b2b599fd6c3
Signed-off-by: Paul Menzel <pmenzel(a)molgen.mpg.de>
Reviewed-on: https://review.coreboot.org/19567
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Aaron Durbin <adurbin(a)chromium.org>
Reviewed-by: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philippe.mathieu.daude(a)gmail.com>
Reviewed-by: Paul Wise (Debian) <pabs(a)debian.org>
---
M util/intelmetool/intelmetool.c
1 file changed, 1 insertion(+), 1 deletion(-)
Approvals:
Aaron Durbin: Looks good to me, approved
Philippe Mathieu-Daudé: Looks good to me, but someone else must approve
Philipp Deppenwiese: Looks good to me, approved
Paul Wise (Debian): Looks good to me, but someone else must approve
build bot (Jenkins): Verified
diff --git a/util/intelmetool/intelmetool.c b/util/intelmetool/intelmetool.c
index 11c2120..b918c3e 100644
--- a/util/intelmetool/intelmetool.c
+++ b/util/intelmetool/intelmetool.c
@@ -79,7 +79,7 @@
* so we avoid cloning to this part.
*/
static void dump_me_memory() {
- uint32_t me_clone = 0x60000000;
+ uintptr_t me_clone = 0x60000000;
uint8_t *dump;
dump = map_physical_exact((off_t)me_clone, (void *)me_clone, 0x2000000);
--
To view, visit https://review.coreboot.org/19567
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id8d778e97090668ad9308a82b44c6b2b599fd6c3
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Paul Wise (Debian) <pabs(a)debian.org>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Caesar Wang has posted comments on this change. ( https://review.coreboot.org/19557 )
Change subject: rockchip/rk3399: enable DPLL SSC for DDR EMI test on bob
......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/#/c/19557/4/src/soc/rockchip/rk3399/clock.c
File src/soc/rockchip/rk3399/clock.c:
Line 360: udelay(30);
> Is this delay needed? There's nothing coming before here? If it's still in
>From the actual test, we need 10us delay at least.
Yes, we need find the root cause, but I believe it's no related to enable the SSC
The other way we cat put the rkclk_set_dpllssc() into src/soc/rockchip/rk3399/sdram.c or src/mainboard/google/gru/romstage.c to enable later
Line 373: PLL_FRAC_MODE << PLL_DSMPD_SHIFT));
> So we're enabling fractional mode here, but where are we setting the fracti
I believe this fractional mode is different with the pll fractional divider.
The pll fractional divider is configured by the other interface.
--
To view, visit https://review.coreboot.org/19557
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I75461d4235bcf55324e6664a1220754e770b4786
Gerrit-PatchSet: 4
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Philip Chen <philipchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes
Julius Werner has posted comments on this change. ( https://review.coreboot.org/19477 )
Change subject: rockchip/rk3399: Add MIPI driver
......................................................................
Patch Set 4:
(9 comments)
https://review.coreboot.org/#/c/19477/4/src/soc/rockchip/rk3399/include/soc…
File src/soc/rockchip/rk3399/include/soc/mipi.h:
Line 225: #define DIV_ROUND_UP(x, y) (((x) + (y) - 1) / (y))
Oh, I only just noticed that you're redefining these here. Please don't do that... instead, use the existing implementations from <stdlib.h> and <timer.h> (this changes the naming from MSEC_PER_SEC to MSECS_PER_SEC, etc.).
https://review.coreboot.org/#/c/19477/2/src/soc/rockchip/rk3399/mipi.c
File src/soc/rockchip/rk3399/mipi.c:
Line 68: return dptdin_map[i].testdin;
> if max_mbps = 100; when we use >= will return 0x10,but we want 0x20
Yeah... right. I don't know what I was thinking there. Sorry.
Line 406:
> rk3288 and rk3399 have the same mipi ip,it will be more convenient if we wa
That doesn't require you to have this here, though. You can just put a
static struct rk_mipi_regs *mipi_regs = (void *)MIPI_BASE;
at the top of this file and use that... it will still pick the right address for 3288 and 3399 because MIPI_BASE comes from the right <soc/addressmap.h> header.
(That reminds me, btw... if this code is already compatible with 3288, just put it in rockchip/common/mipi.c instead.)
https://review.coreboot.org/#/c/19477/4/src/soc/rockchip/rk3399/mipi.c
File src/soc/rockchip/rk3399/mipi.c:
PS4, Line 182: target_bps
target_bps should also be unsigned long, just in case. Also, I think it would be clearer to use u32 and u64 instead of unsigned int and unsigned long so there can be no confusion about how long the types are.
PS4, Line 183: 1500000000
1500 * MHz
PS4, Line 206: 5000000
5 * MHz and 40 * MHz
PS4, Line 216: lane_mbps
This should also be lane_bps instead and all subsequent calculations with it should be in bits/Hz if possible.
Line 405: int ret;
Remove this and just do
if (function_call(...) < 0)
return;
directly.
Line 436: mdelay(500);
I feel like I must have asked this somewhere already... where do the 500ms come from and are they really necessary? We don't usually care about display init that much because we skip it on normal boot, but 500ms is a *really* long time...
--
To view, visit https://review.coreboot.org/19477
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02475eefb187c619c614b1cd20e97074bc8d917f
Gerrit-PatchSet: 4
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: nickey.yang(a)rock-chips.com
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Sean Paul <seanpaul(a)chromium.org>
Gerrit-Reviewer: Shunqian Zheng <zhengsq(a)rock-chips.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: nickey.yang(a)rock-chips.com
Gerrit-HasComments: Yes
Julius Werner has posted comments on this change. ( https://review.coreboot.org/19558 )
Change subject: google/gru: support 800M/928M frequency for bob
......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/#/c/19558/3/src/mainboard/google/gru/sdram_conf…
File src/mainboard/google/gru/sdram_configs.c:
Line 49: if (IS_ENABLED(CONFIG_BOARD_GOOGLE_BOB) && board_id() < 4)
> Then, can the CONFIG_GRU_SDRAM_FREQ be defined by the board_id()?
Oh, yeah, right. I forgot why we've been doing this whole thing. Sorry.
https://review.coreboot.org/#/c/19558/4/src/mainboard/google/gru/sdram_conf…
File src/mainboard/google/gru/sdram_configs.c:
Line 49: if (IS_ENABLED(CONFIG_BOARD_GOOGLE_BOB) && board_id() < 4)
Okay, you were right about the Kconfig, of course (although it might still be nice to have a user-configurable Kconfig and then just do max(CONFIG_GRU_SDRAM_FREQ, 800) for the older Bob boards). But I'd still like to do this with a single table and an snprintf() instead. Just write an int get_sdram_freq(void) function and put the Bob board ID stuff in there.
https://review.coreboot.org/#/c/19558/4/src/mainboard/google/gru/sdram_para…
File src/mainboard/google/gru/sdram_params_800/Makefile.inc:
Line 16: freq := 800
And the stuff I said about file names here still applies, too.
--
To view, visit https://review.coreboot.org/19558
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I613050292a09ff56f4636d7af285075e32259ef4
Gerrit-PatchSet: 4
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Philip Chen <philipchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes
Julius Werner has posted comments on this change. ( https://review.coreboot.org/19557 )
Change subject: rockchip/rk3399: enable DPLL SSC for DDR EMI test on bob
......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/#/c/19557/3/src/soc/rockchip/rk3399/clock.c
File src/soc/rockchip/rk3399/clock.c:
Line 633: switch (hz) {
> done.
...yeah, I wouldn't know, sorry. You guys are the experts. But we can't land this until we have a clear understanding how/why it works and confidence that it won't break any devices. Some random inconsequential EMI isn't as bad as boot failures.
https://review.coreboot.org/#/c/19557/4/src/soc/rockchip/rk3399/clock.c
File src/soc/rockchip/rk3399/clock.c:
Line 360: udelay(30);
Is this delay needed? There's nothing coming before here? If it's still in response to the rkclk_set_pll(dpll), then it should probably go in rkclk_configure_ddr(). (But we're already waiting for PLL_LOCK_STATUS in rkclk_set_pll(), isn't that enough?)
Line 373: PLL_FRAC_MODE << PLL_DSMPD_SHIFT));
So we're enabling fractional mode here, but where are we setting the fractional divider (in DPLL_CON2)? In fact, the TRM says that the power-on reset value is 0x31f... so wouldn't that mean that once you enable this, the PLL will only run with 0x31f / (2 << 24) == 0.00024% of the intended rate (i.e. 22KHz)?
--
To view, visit https://review.coreboot.org/19557
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I75461d4235bcf55324e6664a1220754e770b4786
Gerrit-PatchSet: 4
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Philip Chen <philipchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes