Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/18561 )
Change subject: AGESA binaryPI: Flip HUDSON_IMC_FWM default to disabled
......................................................................
Patch Set 2: Code-Review-1
(2 comments)
Do you plan to add selects to mainboards in this patch? It looks like it should not be selected in any Mullins or Kabini. Also not in asus/f2a85, msi/ms7721, pcengines/apu2. I'd probably select all others for consistency.
https://review.coreboot.org/#/c/18561/2/src/southbridge/amd/agesa/hudson/Kc…
File src/southbridge/amd/agesa/hudson/Kconfig:
PS2, Line 68: USE_BLOBS
I would prefer not to have this dependency. Explanation in next file, but I'm less opinionated for this one.
https://review.coreboot.org/#/c/18561/2/src/southbridge/amd/pi/hudson/Kconf…
File src/southbridge/amd/pi/hudson/Kconfig:
PS2, Line 71: USE_BLOBS
I would prefer to not have this dependency. USE_BLOBS is primarily an instruction to the build to pull down the blobs repo vs. permission to use blobs in the build. I might already have blobs locally and not want to set this symbol. Also, all PI systems will require blobs to run anyway.
--
To view, visit https://review.coreboot.org/18561
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I87146e32e1a9a8f8133987d2b26e059065c89b14
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/18537 )
Change subject: AGESA binaryPI: Remove unused IMC ACPI methods IMSP and IMWK
......................................................................
Patch Set 3: Code-Review-1
There may be other software that can use this besides flashrom. I don't see a downside to leaving the methods in.
--
To view, visit https://review.coreboot.org/18537
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifca4e8328c54d1074b4799ddecfece24607214db
Gerrit-PatchSet: 3
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: No
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/18538 )
Change subject: AGESA binaryPI: Consolidate ACPI for IMC
......................................................................
Patch Set 3:
Can this patch go ahead of "AGESA binaryPI: Remove ACPI for IMC we don't run"? Then fixup that one with this?
--
To view, visit https://review.coreboot.org/18538
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieff6041f3c9ad02f9cebae0ec83d0898abb0d601
Gerrit-PatchSet: 3
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: No
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 9:
(1 comment)
https://review.coreboot.org/#/c/19558/9/src/mainboard/google/gru/sdram_para…
File src/mainboard/google/gru/sdram_params/Makefile.inc:
Line 21: sdram-params += sdram-lpddr3-micron-2GB-928
Better:
sdram-params += sdram-lpddr3-micron-2GB-800
sdram-params += sdram-lpddr3-micron-2GB-928
sdram-params += sdram-lpddr3-micron-4GB-800
sdram-params += sdram-lpddr3-micron-4GB-928
(below as well)
--
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: 9
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 9:
(2 comments)
https://review.coreboot.org/#/c/19557/8/src/soc/rockchip/rk3399/clock.c
File src/soc/rockchip/rk3399/clock.c:
Line 361: * hang somewhere with reboot tests.
> Yes, it's related to "assert(dpll_cfg->refdiv && dpll_cfg->refdiv <= 6);"
But that makes absolutely no sense. dpll_cfg->refdiv is hardcoded in rkclk_configure_ddr(). It can only ever be 1 or 2 with the current code. There is no way a delay could affect that in any way.
Please post the log of the assertion error you see without this. Are you *sure* that the assertion below is actually the one that you trigger?
(Note that if you just see it hang with no further output, that's not an assertion error. Assertions will output an "ASSERTION ERROR: file '...', line ..." line to the console. If you're seeing a hang, it must be from some of the register writes below... you could try moving the udelay further down line by line to figure out which one.)
https://review.coreboot.org/#/c/19557/9/src/soc/rockchip/rk3399/clock.c
File src/soc/rockchip/rk3399/clock.c:
Line 424: 8 << PLL_SSMOD_SPREADAMP_SHIFT));
Now you're already setting spreadamp to 8 above, you can get rid of this.
--
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: 9
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 5:
(3 comments)
Thanks. Looks good to me now, essentially, but I'd still like one type change to be sure that we can't have any overflows.
https://review.coreboot.org/#/c/19477/5/src/soc/rockchip/rk3399/include/soc…
File src/soc/rockchip/rk3399/include/soc/mipi.h:
Line 280: unsigned int lane_bps; /* per lane */
Better make this u64 to reduce the risk of transient overflows when we do math on it.
https://review.coreboot.org/#/c/19477/5/src/soc/rockchip/rk3399/mipi.c
File src/soc/rockchip/rk3399/mipi.c:
Line 285: lbcc = hcomponent * dsi->lane_bps / (8 * MSECS_PER_SEC);
This one makes me a bit uncomfortable. Changing lane_bps to u64 should guarantee that it's safe.
Line 432: mdelay(120);
You reduced the delay but you still didn't explain what it's for. Can you please do that (ideally add a comment for that)?
Are all these delays only specific to the SoC or are any of them also dependent on the panel?
--
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: 5
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
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/19642 )
Change subject: drivers/storage: Make DRVR_CAP_8BIT controller independent
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/19642
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I51e4c990d3941a9f31915a5703095f92309760f1
Gerrit-PatchSet: 1
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Lee Leahy <leroy.p.leahy(a)intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: No
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/19504 )
Change subject: lapic/apic_timer.c: Provide a tsc_freq_mhz for platforms using LAPIC_UDELAY
......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/#/c/19504/3/src/cpu/x86/lapic/apic_timer.c
File src/cpu/x86/lapic/apic_timer.c:
Line 85: unsigned long tsc_freq_mhz(void)
> I would prefer to export get_fsb() and place this somewhere about
Seems like a good idea. A quick look told me most/all? core i CPU since sandy bridge have 100MHz base clk so I guess things could be more unified.
Line 90: msr = rdmsr(IA32_PERF_STS);
> Is this available on P4???
Made my computer choke for a few minutes but I did found it "MSRs in the Pentium® 4 and Intel® Xeon® Processors" table in Intel 64 and IA32 Architectures Software Develper's manual.
Line 101: } else {
> Can drop the `else`. I guess checkpatch would complain?
I think it would...
--
To view, visit https://review.coreboot.org/19504
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic643681c3b9646bf7efbcd786c35a9beda9afc49
Gerrit-PatchSet: 3
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