Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30414 )
Change subject: mainboard/facebook/fbg1701: Do initial mainboard commit
......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/#/c/30414/19/src/mainboard/facebook/fbg1701/Kco…
File src/mainboard/facebook/fbg1701/Kconfig:
https://review.coreboot.org/#/c/30414/19/src/mainboard/facebook/fbg1701/Kco…
PS19, Line 31: choice
> Use GENERIC_SPD_BIN and please refer to my comments in Portwell mainboard patch. […]
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/30414
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I28ac78a630ee705b1e546031f024bfe7f952ab39
Gerrit-Change-Number: 30414
Gerrit-PatchSet: 19
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 13:42:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-MessageType: comment
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29470 )
Change subject: mainboard/portwell/m107: Do initial mainboard commit
......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/#/c/29470/11/src/mainboard/portwell/m107/Kconfig
File src/mainboard/portwell/m107/Kconfig:
https://review.coreboot.org/#/c/29470/11/src/mainboard/portwell/m107/Kconfi…
PS11, Line 30: choice
> There are not GPIO or other possibility to be used to determine the memory type. […]
This line https://review.coreboot.org/c/coreboot/+/29470/11/src/mainboard/portwell/m1…
says RAMID 2, so I deducted there is a way to determine memory option.
https://review.coreboot.org/#/c/29470/11/src/mainboard/portwell/m107/com_in…
File src/mainboard/portwell/m107/com_init.c:
https://review.coreboot.org/#/c/29470/11/src/mainboard/portwell/m107/com_in…
PS11, Line 24: void car_mainboard_pre_console_init(void)
> The UART needs to be configured here as early as possible. […]
It can be also used to disable the onboard UART and enable the SuperIO UART in one sway. I am not suggesting to remove the SuperIO UART configuration. Since You are already configuring the UART in bootblock, it may be necessary to reenable it in mainboard_after_memory_init in order to not loose any console output. In my case (with other board) I had to do so, because disabling the onboard UART port was not enough. What I suggest is to rename car_mainboard_pre_console_init to mainboard_after_memory_init and add also the code to disable the onboard UART in order to not loose any console.
--
To view, visit https://review.coreboot.org/c/coreboot/+/29470
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I7d3173fdcf881f894a75cd9798ba173b425d4e62
Gerrit-Change-Number: 29470
Gerrit-PatchSet: 11
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Wed, 17 Apr 2019 12:42:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Frans Hendriks <fhendriks(a)eltan.com>
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-MessageType: comment
Hello Patrick Rudolph, Aaron Durbin, Piotr Król, Paul Menzel, build bot (Jenkins), Hannah Williams, Nico Huber, Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/29284
to look at the new patch set (#8).
Change subject: soc/intel/braswell/chip.c: Configure LPSS devices in correct mode
......................................................................
soc/intel/braswell/chip.c: Configure LPSS devices in correct mode
LPSS devices can be configured in ACPI or PCI mode.
Use config structure to report the correct mode.
FSP configures child devices to ACPI mode when (mother) DMA device is
configured in ACPI mode. Supplying incorrect configuration will be corrected by
FSP, but to be futher proof supply correct configuration to FSP.
BUG=N/A
TEST=Intel Cherry Hill
Change-Id: Ie271d8cb9f30f0c0ba538f1530cfb82f1306fea8
Signed-off-by: Frans Hendriks <fhendriks(a)eltan.com>
---
M src/soc/intel/braswell/chip.c
1 file changed, 23 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/29284/8
--
To view, visit https://review.coreboot.org/c/coreboot/+/29284
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie271d8cb9f30f0c0ba538f1530cfb82f1306fea8
Gerrit-Change-Number: 29284
Gerrit-PatchSet: 8
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-MessageType: newpatchset
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29470 )
Change subject: mainboard/portwell/m107: Do initial mainboard commit
......................................................................
Patch Set 11:
(4 comments)
https://review.coreboot.org/#/c/29470/11/src/mainboard/portwell/m107/Kconfig
File src/mainboard/portwell/m107/Kconfig:
https://review.coreboot.org/#/c/29470/11/src/mainboard/portwell/m107/Kconfi…
PS11, Line 30: choice
> These options are used anywhere? […]
There are not GPIO or other possibility to be used to determine the memory type. For this reason this choice is used. romstage.c checks for memory type using ONBOARD_MICRON_MEM.
Using this method different binaries can be generated for the HW revisions.
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/acpi/su…
File src/mainboard/portwell/m107/acpi/superio.asl:
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/acpi/su…
PS8, Line 21: Device (COM1) {
> What I meant is why the ASL code is shifted by 1 TAB?
My fault. Sorry, indented != intended.
Will corrrect this.
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/board_i…
File src/mainboard/portwell/m107/board_info.txt:
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/board_i…
PS8, Line 3: Category: misc
> Here's more detailed description of board_info: […]
Will change it to sbc.
https://review.coreboot.org/#/c/29470/11/src/mainboard/portwell/m107/com_in…
File src/mainboard/portwell/m107/com_init.c:
https://review.coreboot.org/#/c/29470/11/src/mainboard/portwell/m107/com_in…
PS11, Line 24: void car_mainboard_pre_console_init(void)
> Since https://review.coreboot. […]
The UART needs to be configured here as early as possible.
Function mainboard_after_memory_init() is used to disable the onboard UART. Otherwise not console output in later stage.
--
To view, visit https://review.coreboot.org/c/coreboot/+/29470
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I7d3173fdcf881f894a75cd9798ba173b425d4e62
Gerrit-Change-Number: 29470
Gerrit-PatchSet: 11
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Wed, 17 Apr 2019 12:32:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Frans Hendriks <fhendriks(a)eltan.com>
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-MessageType: comment
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29284 )
Change subject: soc/intel/braswell/chip.c: Configure LPSS devices in correct mode
......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/29284/7//COMMIT_MSG
Commit Message:
https://review.coreboot.org/#/c/29284/7//COMMIT_MSG@13
PS7, Line 13: Mode for LPSS can be PCI or ACPI. The child devices need to operate in ACPI
The first sentence is almost a duplicate of a sentence in line 9. Please rewrite the commit message
--
To view, visit https://review.coreboot.org/c/coreboot/+/29284
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie271d8cb9f30f0c0ba538f1530cfb82f1306fea8
Gerrit-Change-Number: 29284
Gerrit-PatchSet: 7
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 12:26:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30414 )
Change subject: mainboard/facebook/fbg1701: Do initial mainboard commit
......................................................................
Patch Set 19:
(2 comments)
https://review.coreboot.org/#/c/30414/19/src/mainboard/facebook/fbg1701/Kco…
File src/mainboard/facebook/fbg1701/Kconfig:
https://review.coreboot.org/#/c/30414/19/src/mainboard/facebook/fbg1701/Kco…
PS19, Line 31: choice
Use GENERIC_SPD_BIN and please refer to my comments in Portwell mainboard patch. I have explained how to switch between SPDs.
https://review.coreboot.org/#/c/30414/19/src/mainboard/facebook/fbg1701/Mak…
File src/mainboard/facebook/fbg1701/Makefile.inc:
https://review.coreboot.org/#/c/30414/19/src/mainboard/facebook/fbg1701/Mak…
PS19, Line 44: SPD_SOURCES += MICRON_MT41K512M16HA-125A
no SPD file for MICRON present in the patch
--
To view, visit https://review.coreboot.org/c/coreboot/+/30414
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I28ac78a630ee705b1e546031f024bfe7f952ab39
Gerrit-Change-Number: 30414
Gerrit-PatchSet: 19
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 12:22:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Hello Patrick Rudolph, Aaron Durbin, Piotr Król, Paul Menzel, build bot (Jenkins), Hannah Williams, Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/29284
to look at the new patch set (#7).
Change subject: soc/intel/braswell/chip.c: Configure LPSS devices in correct mode
......................................................................
soc/intel/braswell/chip.c: Configure LPSS devices in correct mode
LPSS devices can be configured in ACPI or PCI mode. The correct mode
must be reported to FSP.
Use config structure to report the correct mode.
Mode for LPSS can be PCI or ACPI. The child devices need to operate in ACPI
mode when the LPSS is operating in ACPI mode.
FSP checks ACPI mode and changes the device mode. Supplying incorrect
configuration will be corrected by FSP, but to be futher proof supply correct
configuration to FSP.
BUG=N/A
TEST=Intel Cherry Hill
Change-Id: Ie271d8cb9f30f0c0ba538f1530cfb82f1306fea8
Signed-off-by: Frans Hendriks <fhendriks(a)eltan.com>
---
M src/soc/intel/braswell/chip.c
1 file changed, 23 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/29284/7
--
To view, visit https://review.coreboot.org/c/coreboot/+/29284
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie271d8cb9f30f0c0ba538f1530cfb82f1306fea8
Gerrit-Change-Number: 29284
Gerrit-PatchSet: 7
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-MessageType: newpatchset