Attention is currently required from: Shelley Chen, Felix Singer, Furquan Shaikh, Martin Roth, Tim Wawrzynczak, Paul Menzel, Julius Werner, Angel Pons, Kyösti Mälkki.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57861 )
Change subject: Documentation/RFC: Generalize PCI support in coreboot
......................................................................
Patch Set 9:
(4 comments)
Patchset:
PS9:
> Hi Nico, I was wondering if you had any open issues with this design doc in it's current state? I b […]
Hi, sorry, didn't mean to stall this. I prefer to review/discuss things step-by-step
which scales much better. This doc requires one to review everything untested,
without visible context of the source code and at once which is a lot additional
work (which I lack the time for and tbh. don't understand why we do it this way).
I tried before and didn't spot an issue in the very first proposed change. Which,
OTOH, was very obvious to me when I looked at the follow-up patch.
File Documentation/RFC/pci_config_access.md:
https://review.coreboot.org/c/coreboot/+/57861/comment/15dc3c57_ccd647ae
PS9, Line 41: #if
: CONFIG(MMCONFIG_SUPPORT)
I think this should be done later when the necessary Kconfig option is introduced
(in step 3.). Adding inconsistent conditions in one step just to fix them up later
won't work well during review.
https://review.coreboot.org/c/coreboot/+/57861/comment/7cd92027_26a70f8d
PS9, Line 54: 3. Add new Kconfigs to distinguish between ECAM and non-ECAM access mechanisms.
If this happens after renaming another Kconfig to `PCI_ECAM` wouldn't there
be two Kconfigs that distinguish between ECAM and non-ECAM?
I think it would be better if we reduce the need for Kconfig. For instance,
we could move the current MMCONF_SUPPORT if into x86 scope, because it's
really only there where we want to include `pci_mmio_cfg.h` and need a
switch to disable it. I'll just try that...
https://review.coreboot.org/c/coreboot/+/57861/comment/04768188_ff394b34
PS9, Line 92: supported and it currently only covers ECAM.
I'm still not convinced that anything is wrong with `MMCONF_SUPPORT`.
You mentioned that people were confused by it in CB:57333. If the
motivation is to avoid confusion that should be mentioned. But first
we need to understand why it was confusing. Did people mix it up
with MMIO in general?
Why I was against changing the name from the very beginning is that
it will likely _create_ confusion. We've been used to grep for MMCONF
for more than a decade (maybe more than two, idk). If that term sud-
denly vanishes, I'd be very confused.
--
To view, visit https://review.coreboot.org/c/coreboot/+/57861
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3a56d932f6e047087c38a7687564065cc1562363
Gerrit-Change-Number: 57861
Gerrit-PatchSet: 9
Gerrit-Owner: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Comment-Date: Thu, 14 Oct 2021 16:09:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Shelley Chen <shchen(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Nico Huber, Martin Roth, Angel Pons, Julius Werner.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56410 )
Change subject: [RFC] kconfig_lint: Drop overly restrictive rules about choice configs
......................................................................
Patch Set 5:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56410/comment/4eadf977_7c6b4134
PS4, Line 19: On top, the linter treats every occurence of a `config` entry as a
: symbol declaration, even when it's just setting a default or adding
: selects.
> > Thanks for the hint - I missed that discussion. I'll have a look later.
> >
> > > they had to debug something for weeks because they had a bad binary in their build environment, AIUI
> >
> > Not sure how that shall work as argument against Python. What am I missing?
>
> The assumption was that we use the Python binaries that people have
> already installed. Same as for any other host tool we just assume to
> be present, we'd have to maintain on our end, i.e. keep compatibility
> to everything people might have installed. We already do that for the
> host toolchain, for instance, and the effort is not negligible,
Uhm, if we don't use that fancy python stuff, we won't have any problem. Ofc there must be some minimum version, same as with perl or whatever.
>
> I guess things would look differently if we would add Python to buildgcc.
> But I have really no idea if that is feasible. And again, we'd have to
> maintain something on our end.
>
> > > It seems, one major issue was the transition from Python 2 to 3.
> >
> > Python2 vs. Python3 never was a real problem - the problem actually was:
> > - that religious `print is not a function but a statement` bs discussion.
> > - Python3 is soooo different. No, it's not. Python2.7 and Python3 are mostly compatible.
> > Anyway, Py2 is dead and we shouldn't use it.
>
> It's not about what changed but that things change. If we'd work with
> the random versions everybody has installed, we'd have to keep our code
> compatible with all these versions at once. Same for any other tool that
> we don't provide ourselves. (kconfiglib is said to achieve that rather
> well, btw.)
Python versions are backwards compatible. Sure, here and there we will have to adapt but that won't be a huge problem.
>
> >
> > However, from my (very naive) point of view, we should just evaluate it and see if it works for us. Theoretical discussions often lead to false assumptions, at least in my experience.
>
> Well, _you_ can evaluate what you like. But you shouldn't push other people
> (you said "we should") to evaluate something that you want. The project is
> simply too big for that. You can't expect everybody to take a break just to
> evaluate something.
No it's different: When I say we should, then it's a proposal. When you don't care, you don't have to. I don't expect anything. I proposed it, if noone cares I don't give a f***. Sorry.
--
To view, visit https://review.coreboot.org/c/coreboot/+/56410
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I48a17f6403470251be6b6d44bb82a8bdcbefe9f6
Gerrit-Change-Number: 56410
Gerrit-PatchSet: 5
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Thu, 14 Oct 2021 15:40:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Martin Roth <martinroth(a)google.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Marshall Dawson, Kangheui Won, Karthik Ramasubramanian, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58316 )
Change subject: psp_verstage: convert relative address in EFS2
......................................................................
Patch Set 1:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/58316/comment/5bad6899_55036607
PS1, Line 10: update_psp_bios_dir
Can you update the documentation on update_psp_bios_dir to say that it only accepts x86 absolute addresses?
File src/soc/amd/common/psp_verstage/include/psp_verstage.h:
https://review.coreboot.org/c/coreboot/+/58316/comment/64611816_be1f0ce7
PS1, Line 46:
Can you tab the values
File src/soc/amd/common/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/58316/comment/abfedf9d_426145b9
PS1, Line 126: (ef_table->efs_gen & EFS_GEN_MASK) == EFS_GEN2
EFS2 uses a version of 0? What's the current value of the reserved1 field? Will this always be true?
https://review.coreboot.org/c/coreboot/+/58316/comment/2921cc96_0b25d8ce
PS1, Line 131: (void *)
Why the void* cast?
--
To view, visit https://review.coreboot.org/c/coreboot/+/58316
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I95813beba7278480e6640599fcf7445923259361
Gerrit-Change-Number: 58316
Gerrit-PatchSet: 1
Gerrit-Owner: Kangheui Won <khwon(a)chromium.org>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Kangheui Won <khwon(a)chromium.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Thu, 14 Oct 2021 15:02:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Rob Barnes, Karthik Ramasubramanian, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58311 )
Change subject: mb/google/guybrush: Fix variant_has_pcie_wwan helper
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/58311
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I07b9dd8fc5c8c3e1557f9268c1176d4a3cade1af
Gerrit-Change-Number: 58311
Gerrit-PatchSet: 1
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Rob Barnes <robbarnes(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Thu, 14 Oct 2021 14:52:24 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Marshall Dawson, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58312 )
Change subject: soc/amd/common/block/lpc: simplify eSPI part of Makefile
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/58312
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I65e07c356aac73c5de2d9ce5582434872a223c19
Gerrit-Change-Number: 58312
Gerrit-PatchSet: 1
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Thu, 14 Oct 2021 14:50:37 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Paul Menzel, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph, Felix Held.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58322 )
Change subject: Doc/mainboard_io_trap_handler_sample.c: Drop file
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/58322
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4a238fbf354926cb6568e1709bfb79cc546dfd73
Gerrit-Change-Number: 58322
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Thu, 14 Oct 2021 14:46:51 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Martin Roth, Angel Pons, Julius Werner, Michael Niewöhner.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56410 )
Change subject: [RFC] kconfig_lint: Drop overly restrictive rules about choice configs
......................................................................
Patch Set 5:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56410/comment/6dc1360b_9ff26344
PS4, Line 19: On top, the linter treats every occurence of a `config` entry as a
: symbol declaration, even when it's just setting a default or adding
: selects.
> Thanks for the hint - I missed that discussion. I'll have a look later.
>
> > they had to debug something for weeks because they had a bad binary in their build environment, AIUI
>
> Not sure how that shall work as argument against Python. What am I missing?
The assumption was that we use the Python binaries that people have
already installed. Same as for any other host tool we just assume to
be present, we'd have to maintain on our end, i.e. keep compatibility
to everything people might have installed. We already do that for the
host toolchain, for instance, and the effort is not negligible.
I guess things would look differently if we would add Python to buildgcc.
But I have really no idea if that is feasible. And again, we'd have to
maintain something on our end.
> > It seems, one major issue was the transition from Python 2 to 3.
>
> Python2 vs. Python3 never was a real problem - the problem actually was:
> - that religious `print is not a function but a statement` bs discussion.
> - Python3 is soooo different. No, it's not. Python2.7 and Python3 are mostly compatible.
> Anyway, Py2 is dead and we shouldn't use it.
It's not about what changed but that things change. If we'd work with
the random versions everybody has installed, we'd have to keep our code
compatible with all these versions at once. Same for any other tool that
we don't provide ourselves. (kconfiglib is said to achieve that rather
well, btw.)
>
> However, from my (very naive) point of view, we should just evaluate it and see if it works for us. Theoretical discussions often lead to false assumptions, at least in my experience.
Well, _you_ can evaluate what you like. But you shouldn't push other people
(you said "we should") to evaluate something that you want. The project is
simply too big for that. You can't expect everybody to take a break just to
evaluate something.
--
To view, visit https://review.coreboot.org/c/coreboot/+/56410
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I48a17f6403470251be6b6d44bb82a8bdcbefe9f6
Gerrit-Change-Number: 56410
Gerrit-PatchSet: 5
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Comment-Date: Thu, 14 Oct 2021 14:39:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Martin Roth <martinroth(a)google.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment