Nico Huber 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 8:
> > > 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.
> >
> > I can't say that I understand this yet. You are saying FSP
> > allows a value of 2 (PCH_ACPI_MODE). But then you say FSP
> > would be correcting incorrect configuration, which translates
> > to me as it reads the hardware state and ignores the diffe-
> > rence between a 1 and a 2.
> >
> > And about being future proof. That's generally a good idea,
> > but how can we anticipate the future here? How can we tell
> > that if FSP is being corrected, if the code will be made to
> > match the header file or the other way around?
>
> The goal is to supply correct mode settings to FSP. For this reason the modification is made.
> On the other hand FSP (MR2) will modify settings of the child devices if incorrect.
> What FSP is doing is not wrong, but for those without access to FSP sources, we should ensure that coreboot is configuring the correct mode.
Ah, there's that about child devices again. Sometimes you say
it's corrected (see topmost quote here) and the change is only
needed to be future proof, sometimes you say it's corrected for
child devices (only?). That's confusing.
But let's back up a little. How does the device hierarchy look
like here? I couldn't find anything about child devices and
actually nothing about the DMA devices in general. Can you
point to a datasheet that explains this a little?
I think the first thing to do in such a case (documentation /
comments in header files don't match the code) is to file a bug
report for FSP. Was that done yet? And in case we can't get
the official documentation fixed, we have to document things
on our own, e.g. by adding a reasonable enum to our code and
comment that the documentation is wrong.
--
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-Comment-Date: Fri, 26 Apr 2019 14:15:29 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Krystian Hebel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30117 )
Change subject: arch/x86: Support x86_64 exceptions
......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/30117/13/src/arch/x86/idt.S
File src/arch/x86/idt.S:
https://review.coreboot.org/#/c/30117/13/src/arch/x86/idt.S@145
PS13, Line 145: lea 64(%rsp), %rbp
This is also wrong, should be 128(%rsp) due to 8 new registers on stack.
--
To view, visit https://review.coreboot.org/c/coreboot/+/30117
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idd12c90a95cc2989eb9b2a718740a84222193f48
Gerrit-Change-Number: 30117
Gerrit-PatchSet: 13
Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Fri, 26 Apr 2019 14:00:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Hello Patrick Rudolph, Piotr Król, Matt DeVillier, build bot (Jenkins), Hannah Williams, Nico Huber, Martin Roth, Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/29414
to look at the new patch set (#12).
Change subject: soc/intel/braswell: Move LPE ACPI code to mainboard
......................................................................
soc/intel/braswell: Move LPE ACPI code to mainboard
The ACPI code of LPE device is included regardless of the
availability of the LPE controller.
Linux remains requesting the status of device LPEA even if
this device is disabled.
Include ACPI LPE controller code at Braswell mainboards with
LPE enabled.
BUG=N/A
TEST=Linux 4.17+ on Portwell PQ7-M107
Change-Id: Ic8acf9ea9e9b0ba9b272e20beb2023b7a4716a73
Signed-off-by: Frans Hendriks <fhendriks(a)eltan.com>
---
M src/mainboard/google/cyan/dsdt.asl
M src/mainboard/intel/strago/dsdt.asl
M src/soc/intel/braswell/acpi/southcluster.asl
3 files changed, 2 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/29414/12
--
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: 12
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: 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: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: newpatchset
Krystian Hebel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30117 )
Change subject: arch/x86: Support x86_64 exceptions
......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/#/c/30117/13/src/arch/x86/idt.S
File src/arch/x86/idt.S:
https://review.coreboot.org/#/c/30117/13/src/arch/x86/idt.S@131
PS13, Line 131: */
Perhaps SS should be listed as well.
https://review.coreboot.org/#/c/30117/13/src/arch/x86/idt.S@159
PS13, Line 159: pop %rax
Stack was aligned two instructions earlier, this pops wrong values.
--
To view, visit https://review.coreboot.org/c/coreboot/+/30117
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idd12c90a95cc2989eb9b2a718740a84222193f48
Gerrit-Change-Number: 30117
Gerrit-PatchSet: 13
Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Fri, 26 Apr 2019 12:39:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29414 )
Change subject: soc/intel/braswell: Move LPE ACPI code to mainboard
......................................................................
Patch Set 11: Code-Review+1
(1 comment)
> > (1 comment)
>
> Have removed the copyright lines.
Looks like they are still there?
https://review.coreboot.org/#/c/29414/11//COMMIT_MSG
Commit Message:
https://review.coreboot.org/#/c/29414/11//COMMIT_MSG@12
PS11, Line 12: LPE enabled.
I still miss the information that this works around an
actual problem.
--
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: 11
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: 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: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Fri, 26 Apr 2019 12:19:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Krystian Hebel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30114 )
Change subject: arch/x86/assembly_entry: Enable long mode on x86_64
......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/#/c/30114/11/src/arch/x86/long_mode.S
File src/arch/x86/long_mode.S:
https://review.coreboot.org/#/c/30114/11/src/arch/x86/long_mode.S@86
PS11, Line 86: or %esi, 0x0(%edi)
Repeated line. Perhaps top 32 bits were to be cleared here?
https://review.coreboot.org/#/c/30114/11/src/arch/x86/long_mode.S@90
PS11, Line 90: cmp $(4), %eax
I may be overzealous here, but counting downwards is slightly faster and easier to understand.
https://review.coreboot.org/#/c/30114/11/src/arch/x86/long_mode.S@194
PS11, Line 194: cmpq $(4), %rax
As above.
--
To view, visit https://review.coreboot.org/c/coreboot/+/30114
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I57974a55f3b778c90b3587f39e86e4eb8692ad48
Gerrit-Change-Number: 30114
Gerrit-PatchSet: 11
Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: David Guckian <david.guckian(a)intel.com>
Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Reviewer: York Yang <yyang024(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: HAOUAS Elyes <ehaouas(a)noos.fr>
Gerrit-CC: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-CC: Lijian Zhao <lijian.zhao(a)intel.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Fri, 26 Apr 2019 12:17:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment