Attention is currently required from: Arthur Heymans, Christian Walter, Dinesh Gehlot, Elyes Haouas, Eran Mitrani, Felix Held, Jeff Daly, Johnny Lin, Julius Werner, Kapil Porwal, Martin L Roth, Subrata Banik, Tarun, Tim Chu, Vanessa Eusebio.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80193?usp=email )
Change subject: soc/intel: Use write{64,32,16,8}p and read{64,32,16,8}p
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> Basically, I think when you're trying to call write32() with an integer address then in 99% of the cases that means you already made a mistake beforehand, and should've been keeping that address in a pointer variable instead.
Totally disagree. We often use constant addresses, so 99% seems exaggerated,
maybe 1%? And technically using pointers for MMIO is 100% wrong in C.
I do like the idea of type safety, though. Right now we don't have much. The
*p() functions allow any integer. The others allow any pointer. So yeah, with
one set of functions we could accidentally put in a number that isn't an address.
But with the other set one could accidentally use a real pointer to a real object.
So the argument works both ways.
How about something like this:
```
typedef struct __packed {
uint8_t __dont_care;
} *mmio_addr;
```
That's a type that we can do byte-wise arithmetic with. And the compiler
should always warn if we pass another type. The fact that we are using
pointers for MMIO would be hidden.
What I don't know is if we'd get different (potentially bloated) code
generated for `void *`/`uintptr_t`/`mmio_addr`.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80193?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I4022e3bbfacf35d0529e46dc82cf303dae9438e4
Gerrit-Change-Number: 80193
Gerrit-PatchSet: 1
Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Jeff Daly <jeffd(a)silicom-usa.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tarun <tstuli(a)gmail.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: Martin L Roth <gaumless(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Jeff Daly <jeffd(a)silicom-usa.com>
Gerrit-Attention: Eran Mitrani <mitrani(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Tarun <tstuli(a)gmail.com>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 20:03:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Martin L Roth <gaumless(a)gmail.com>
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: comment
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80270?usp=email )
Change subject: soc/amd/common/data_fabric/domain: introduce add_pci_cfg_resources
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
looks like this patch causes a regression for windows
--
To view, visit https://review.coreboot.org/c/coreboot/+/80270?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ibbcc2aea4f25b6dc68fdf7f360e5a4ce53f6d850
Gerrit-Change-Number: 80270
Gerrit-PatchSet: 2
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 18:19:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Eric Lai, Ivy Jian, Nick Vaccaro.
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80300?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: mb/google/brox: Initialize TCHSCR_RST_L to 0
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
> I still like to set all power related pins to DEEP. Since may have S5 leakage issue. […]
This is from our HW engineer (Charlie Costakis): "My understanding is that PLT_RST_L is actually the more conservative choice here, since PLT_RST_L will always be asserted in S5. Do you see any shortcoming with this approach?"
--
To view, visit https://review.coreboot.org/c/coreboot/+/80300?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5bf1901a3a40a38237b950abcb758f96aebcc1cf
Gerrit-Change-Number: 80300
Gerrit-PatchSet: 4
Gerrit-Owner: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Ivy Jian <ivy.jian(a)quanta.corp-partner.google.com>
Gerrit-Attention: Eric Lai <ericllai(a)google.com>
Gerrit-Attention: Ivy Jian <ivy.jian(a)quanta.corp-partner.google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 18:19:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Eric Lai <ericllai(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Christian Walter, Dinesh Gehlot, Elyes Haouas, Eran Mitrani, Jeff Daly, Johnny Lin, Julius Werner, Kapil Porwal, Martin L Roth, Subrata Banik, Tarun, Tim Chu, Vanessa Eusebio.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80193?usp=email )
Change subject: soc/intel: Use write{64,32,16,8}p and read{64,32,16,8}p
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> I'm with Julius on this. […]
i strongly disagree with Julius and Martin on this; the writep[8,16,32] are very useful, since a lot of the registers are definesand this removes the need for typecasts to void * which makes at least for me the code easier to read
--
To view, visit https://review.coreboot.org/c/coreboot/+/80193?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I4022e3bbfacf35d0529e46dc82cf303dae9438e4
Gerrit-Change-Number: 80193
Gerrit-PatchSet: 1
Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Jeff Daly <jeffd(a)silicom-usa.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tarun <tstuli(a)gmail.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Jeff Daly <jeffd(a)silicom-usa.com>
Gerrit-Attention: Eran Mitrani <mitrani(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Tarun <tstuli(a)gmail.com>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Wed, 07 Feb 2024 18:13:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Martin L Roth <gaumless(a)gmail.com>
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Gerrit-MessageType: comment