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 (#6).
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.
Values 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/6
--
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: 6
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
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29417 )
Change subject: soc/intel/braswell/acpi/lpss.asl: Remove SPI1 and PWM asl code
......................................................................
Patch Set 5: Code-Review+2
Well if we can not use it now, we may bring it back later if needed.
--
To view, visit https://review.coreboot.org/c/coreboot/+/29417
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iec2ca7520081d00bf7a53d58ee054aa6f23e5606
Gerrit-Change-Number: 29417
Gerrit-PatchSet: 5
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.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: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 17 Apr 2019 11:40:11 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
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 5:
> Patch Set 3:
>
> > Patch Set 3:
> >
> > > Patch Set 3:
> > >
> > > > Patch Set 3:
> > > >
> > > > is the default PCI mode? (since you're not explicitly setting it)
> > > > what are the consequences of not passing the correct mode to FSP?
> > >
> > > Default can be PCI or ACPI, depending on values devicetree.cb.
> > > The child devices need to operate ACPI mode when the LPSS is operating in ACPI mode. For this reason the patch is uploaded.
> > >
> > > FSP MR2 does a check for ACPI mode and change the device mode. Supplying incorrect configuration will be corrected by FSP, but to be futher proof supply correct configuration to FSP.
> >
> > By looking at the BSF file for Braswell FSP, the I2C and other LPSS devices are defined as Enable/Disable switches, so only binary configuration (0 or 1). Does it mean that that Intel did not document the ACPI mode as a 3rd option in the BSF and Integration Guide?
>
> In BSF only enable/disable is supported. In FSP code the mode will follow the DMA1Enabled.
DMA settings are also of enable/disable type. Please mention Your discovery in the commit message so that it is clear why the change is introduced.
--
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: 5
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-Comment-Date: Wed, 17 Apr 2019 11:36:52 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29414 )
Change subject: soc/intel/braswell: Include LPE ACPI code in mainboard
......................................................................
Patch Set 10: Code-Review+2
Definitely, looks much better.
--
To view, visit https://review.coreboot.org/c/coreboot/+/29414
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic8acf9ea9e9b0ba9b272e20beb2023b7a4716a73
Gerrit-Change-Number: 29414
Gerrit-PatchSet: 10
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.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: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Wed, 17 Apr 2019 11:33:17 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29371 )
Change subject: drivers/intel/fsp1_1/raminit.c: Make check FSP HOBs independent of CONFIG_DISPLAY_HOBS
......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/#/c/29371/4/src/drivers/intel/fsp1_1/raminit.c
File src/drivers/intel/fsp1_1/raminit.c:
https://review.coreboot.org/#/c/29371/4/src/drivers/intel/fsp1_1/raminit.c@…
PS4, Line 190: } else if (IS_ENABLED(CONFIG_DISPLAY_HOBS)) {
IS_ENABLED is deprecated, use CONFIG() macro in the file
https://review.coreboot.org/#/c/29371/4/src/drivers/intel/fsp1_1/ramstage.c
File src/drivers/intel/fsp1_1/ramstage.c:
https://review.coreboot.org/#/c/29371/4/src/drivers/intel/fsp1_1/ramstage.c…
PS4, Line 64: if (IS_ENABLED(CONFIG_DISPLAY_HOBS))
IS_ENABLED is deprecated, please use CONFIG() macro
--
To view, visit https://review.coreboot.org/c/coreboot/+/29371
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3776fa37866c7ef3aea090842387660c22bbdd4d
Gerrit-Change-Number: 29371
Gerrit-PatchSet: 4
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com>
Gerrit-Reviewer: Lee Leahy <leroy.p.leahy(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: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: York Yang <yyang024(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 17 Apr 2019 11:24:22 +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/+/29371 )
Change subject: drivers/intel/fsp1_1/raminit.c: Make check FSP HOBs independent of CONFIG_DISPLAY_HOBS
......................................................................
Patch Set 4:
> Patch Set 3:
>
> > Patch Set 3:
> >
> > (3 comments)
> >
> > Please clean up and separate HOB checking and HOB displaying. You have also enabled prints that are a part of displaying the hobs after removing the macro.
>
> I don't agree 100% with your comment. The Hobs are handled the same way as FSP_BOOTLOADER_TOLUM_HOB and FSP_SMBIOS_MEMORY_INFO_HOB now.
>
> We might consider if CONFIG_DISPLAY_HOBS should disable and hob information, but don't disable verification of the required hobs.
We may have a misunderstanding here. What I meant is to move all prints in cases where we only print HOB's pointer. I agree that FSP_BOOTLOADER_TOLUM_HOB is somewhat different because it is related to cbmem root. The prints for this HOB should remain where they are. However I do not see the point of printing simple HOB pointer without HOB display option enabled. I am especially talking about FSP_NON_VOLATILE_STORAGE_HOB, FSP_SMBIOS_MEMORY_INFO where we simply print the HOB pointer if the check passes. IMO not necessary and we should even remove those prints, beause print_hob_type_structure prints the pointers either way.
--
To view, visit https://review.coreboot.org/c/coreboot/+/29371
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3776fa37866c7ef3aea090842387660c22bbdd4d
Gerrit-Change-Number: 29371
Gerrit-PatchSet: 4
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com>
Gerrit-Reviewer: Lee Leahy <leroy.p.leahy(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: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: York Yang <yyang024(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 17 Apr 2019 11:22:56 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Hello Patrick Rudolph, Piotr Król, Huang Jin, York Yang, Lee Leahy, build bot (Jenkins), Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/29371
to look at the new patch set (#4).
Change subject: drivers/intel/fsp1_1/raminit.c: Make check FSP HOBs independent of CONFIG_DISPLAY_HOBS
......................................................................
drivers/intel/fsp1_1/raminit.c: Make check FSP HOBs independent of CONFIG_DISPLAY_HOBS
Check for FSP HOBs is depending on CONFIG_DISPLAY_HOBS.
Use the CONFIG_DISPLAY_HOBS for display HOB info only and always check HOBs.
Use BIOS_ERR of printk() for FSP errors.
BUG=N/A
TEST=Check console output on Facebook FBG1701.
Change-Id: I3776fa37866c7ef3aea090842387660c22bbdd4d
Signed-off-by: Frans Hendriks <fhendriks(a)eltan.com>
---
M src/drivers/intel/fsp1_1/raminit.c
M src/drivers/intel/fsp1_1/ramstage.c
2 files changed, 25 insertions(+), 32 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/29371/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/29371
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3776fa37866c7ef3aea090842387660c22bbdd4d
Gerrit-Change-Number: 29371
Gerrit-PatchSet: 4
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com>
Gerrit-Reviewer: Lee Leahy <leroy.p.leahy(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: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: York Yang <yyang024(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: newpatchset
David Guckian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/26796 )
Change subject: src: include <assert.h> when appropriate
......................................................................
Patch Set 14: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/26796
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib843eb7144b7dc2932931b9e8f3f1d816bcc1e1a
Gerrit-Change-Number: 26796
Gerrit-PatchSet: 14
Gerrit-Owner: HAOUAS Elyes <ehaouas(a)noos.fr>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: David Guckian
Gerrit-Reviewer: David Guckian <david.guckian(a)intel.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: HAOUAS Elyes <ehaouas(a)noos.fr>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Vanny E <vanessa.f.eusebio(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Wed, 17 Apr 2019 08:21:33 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29661 )
Change subject: {drivers,mb,soc/intel/braswell}: Add support for Braswell FSP MR2
......................................................................
Patch Set 9:
> Patch Set 9:
>
> (2 comments)
>
> I like the solution with the move of UPDs to the
> mainboards. But that still leaves the settings in
> the soc's chip.h, right?
The common 'MR1 and MR2' settings are still in soc. MR1 specific items moved to mainboard.
--
To view, visit https://review.coreboot.org/c/coreboot/+/29661
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id40b5d46ddda93845d9739b56aaf7ad24ee89246
Gerrit-Change-Number: 29661
Gerrit-PatchSet: 9
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: David Guckian
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com>
Gerrit-Reviewer: Lee Leahy <leroy.p.leahy(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Nathaniel L Desimone <nathaniel.l.desimone(a)intel.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Wed, 17 Apr 2019 07:52:07 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment