Attention is currently required from: Furquan Shaikh, Martin Roth, Tim Wawrzynczak, Arthur Heymans, Aaron Durbin, Patrick Rudolph, Karthik Ramasubramanian.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52788 )
Change subject: soc/intel: Don't select VBOOT_SEPARATE_VERSTAGE
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
Patchset:
PS4:
> Seems okay to me. My understanding is that we use VBOOT_SEPARATE_VERSTAGE in a couple of scenarios: […]
+1 to all Furquan said. Linking into bootblock should be more efficient on XIP devices with no other restrictions.
Have you actually tested this on a board? I seem to recall that once upon a time I found this broken on x86. Don't remember any details though, may well be fixed at this point.
--
To view, visit https://review.coreboot.org/c/coreboot/+/52788
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5e02961183b5bcc37365458a3b10342e5bc2b525
Gerrit-Change-Number: 52788
Gerrit-PatchSet: 4
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
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: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Tue, 04 May 2021 00:53:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Tim Wawrzynczak, Angel Pons.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50858 )
Change subject: soc/intel/common: Add function to lpc_lib to return PIRQ routing
......................................................................
Patch Set 14:
(1 comment)
File src/soc/intel/common/block/include/intelblocks/itss.h:
https://review.coreboot.org/c/coreboot/+/50858/comment/dfbabc67_ee9f30ae
PS14, Line 7: MAX_PXRC_CONFIG
I think this can be dropped now and we can use `PIRQ_COUNT` which was added in CB:50857.
--
To view, visit https://review.coreboot.org/c/coreboot/+/50858
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib215fba54573c50a88aa4584442bd8d27ae017be
Gerrit-Change-Number: 50858
Gerrit-PatchSet: 14
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Tue, 04 May 2021 00:37:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Tim Wawrzynczak.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49408 )
Change subject: soc/intel/common: Add new IRQ module
......................................................................
Patch Set 26:
(21 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49408/comment/f8f778d9_8ce63cfe
PS26, Line 25: meet the FSPs rules
nit: I think most(all but 1?) rules are what the BWG requires.
File src/soc/intel/common/block/include/intelblocks/irq.h:
https://review.coreboot.org/c/coreboot/+/49408/comment/f959b867_6d9ac248
PS26, Line 14: irq_constraint_type
Can a single device have multiple constraints? e.g. FIXED_INT_PIN and IDENTITY? or FIXED_INT_PIN and UNIQUE_IRQ?
https://review.coreboot.org/c/coreboot/+/49408/comment/4bc2cf4b_4180c9a3
PS26, Line 26: IDENTITY
Haven't read rest of the CL yet, but it is not very clear from the comment above how `IDENTITY` is different than PIRQ?
https://review.coreboot.org/c/coreboot/+/49408/comment/ed5ba410_de6b958d
PS26, Line 30: pci_devfn_t
Shouldn't this be `unsigned int`?
https://review.coreboot.org/c/coreboot/+/49408/comment/6c3e5e5f_bd814b55
PS26, Line 32: pci_pin
Is this set only in case of `FIXED_INT_PIN`?
https://review.coreboot.org/c/coreboot/+/49408/comment/4567e035_b6c642fc
PS26, Line 40: pci_devfn_t
`unsigned int`?
File src/soc/intel/common/block/irq/irq.c:
https://review.coreboot.org/c/coreboot/+/49408/comment/d6bf0969_39939fab
PS26, Line 13: MIN_IRQ
Should this be `MIN_SHARED_IRQ` so that it is consistent with other _SHARED_IRQ macros below?
https://review.coreboot.org/c/coreboot/+/49408/comment/072c0bc3_78dc6f64
PS26, Line 17: #define TOTAL_IRQ (MAX_IRQ - MIN_IRQ + 1)
: #define MAX_IRQ_ENTRIES 120
These macros are unused. Are these required?
https://review.coreboot.org/c/coreboot/+/49408/comment/5778773e_c027f3fe
PS26, Line 21: MAX_PINS
Is this the same as PCI_INT_MAX?
https://review.coreboot.org/c/coreboot/+/49408/comment/70fd5960_23ab58d3
PS26, Line 82: >
Shouldn't this be ==?
https://review.coreboot.org/c/coreboot/+/49408/comment/5ea881d6_0bc1f976
PS26, Line 91: used
Instead of determining the pin state here and in `find_shareable_pin`, do you think if it would make sense to have another structure tracking the pin state:
```
struct pin_info {
enum pin_state {
FREE_PIN,
SHARED_PIN,
UNIQUE_IRQ_PIN,
} pin_state;
int usage_count;
bool identity_map;
int irq;
} pin_info[MAX_PINS];
```
It is basically just combining the information you are already tracking/using in different places:
* used[]
* share_count[]
* pin_irq_map[]
It helps with a few things:
1. Don't need to calculate pin state (used/free) or usage count every time the function is called.
2. Don't need to track pin_irq_map and MAX_SHARED_IRQ to determine if a pin is being used for unique irq. Makes couple of checks in the `find_shareable_pin()` cleaner.
3. With #2 addressed, I think we can have a clean split of pin allocation followed by irq allocation (probably makes it easier to read?):
```
assign_slot()
{
assign_pins(); // Assigns pin to each valid function. Generates pin_info and fn_pin_map
assign_irqs(); // Assigns IRQs to each pin using information from pin_info
add_slot_entries(); // Adds entry for each slot function using fn_pin_map and pin_info
}
```
where
```
assign_pins()
{
assign_fixed_pins();
assign_unique_irq_pins();
assign_shareable_pins();
}
```
I think these functions can also make use of some helper function to set pin_info[] which does the required sanity check.
https://review.coreboot.org/c/coreboot/+/49408/comment/668460ae_1717938c
PS26, Line 136: unsigned int share_count[TOTAL_SHARED_IRQ] = {0};
Should this be a global array to avoid having to calculate the share_count every time? IIUC, one of the reasons why you are recalculating the share_count every time is because we haven't yet added entries for the current slot. But, if we don't rely on `entries[]` to determine this and instead add a step in `assign_irqs()` to `increment_shared_count()` then share_count will always have the up-to-date information.
```
assign_irqs()
{
// If unique, assign unique irq.
// If shared:
// 1. get least used
// 2. call increment_shared_count()
}
```
https://review.coreboot.org/c/coreboot/+/49408/comment/39318f2e_b2b8f36e
PS26, Line 175: fn_pin_map
I am trying to understand what state `fn_pin_map[]` would be in. By default, you set all functions 0 - 7 as `EMPTY_FN` (0xff) which I think means that the function is not present in the slot.
And then in the loop below, if `constraints[i].type` is `UNKNOWN`, then the function is left as `EMPTY_FN`, else it is set to `PCI_INT_NONE`. So, IIUC, what you want after this function is:
* Non-existent functions in the slot ==> Set as EMPTY_FN
* Existent functions in the slot ==> Set as PCI_INT_NONE
But, the loop below assumes that all functions exist in the order 0 - 7(in the constraints list) and there are no holes in between. I think that assumption is not really clear at the API level. It might be better to check for the function # in the loop and set `fn_pin_map[i]` accordingly.
```
for (i = 0; i < MAX_FNS; i++) {
func = PCI_FUNC(constraints[i].devfn);
if (constraints[i].type != UNKNOWN)
fn_pin_map[func] = PCI_INT_NONE;
...
}
```
BTW, is there any advantage of using `EMPTY_FN`? I don't really see it getting used anywhere outside of this function. Can we simply initialize `fn_pin_map` to `PCI_INT_NONE`? Ideally, there should not be any slots that are all empty in the constraint table when we are in this function. You can use the slot count(as I mentioned on line 342) to ensure that.
https://review.coreboot.org/c/coreboot/+/49408/comment/d065dcfb_8095b529
PS26, Line 198: constraints[i].type == FIXED_INT_PIN || constraints[i].type == IDENTITY
I wonder if the constraints type should be a bit-field so that multiple can be set e.g. `FIXED_INT_PIN` and `IDENTITY`. `FIXED_INT_PIN` indicates that a fixed pin is allocated for the slot-function. `IDENTITY` indicates that the pin is mapped 1:1 to PIRQ #. Then, we don't need to make the assumption that IDENTITY implies fixed pin allocation.
Also, do we need to handle cases like a `FIXED_INT_PIN` also requires a `UNIQUE_IRQ`?
This needs some more thought because I understand that having multiple bits set at the same time also requires more paths to handle in `assign_pins()`. Probably, will require bits in the same order as we will process them and use something like `fls()` to properly handle it.
https://review.coreboot.org/c/coreboot/+/49408/comment/50d4c9bf_fca5111d
PS26, Line 200: return false;
printk(BIOS_ERR, ""); to indicate that the SoC constraint is incorrect.
https://review.coreboot.org/c/coreboot/+/49408/comment/9acd4b58_d5abfefc
PS26, Line 202: i
Same comment as above. This will have to be PCI_FUNC(...)
https://review.coreboot.org/c/coreboot/+/49408/comment/ade6bd08_3462db89
PS26, Line 208: (pin - PCI_INT_A)
PIN2IDX?
https://review.coreboot.org/c/coreboot/+/49408/comment/de73a357_c191c001
PS26, Line 214: fn_pin_map[0] = PCI_INT_A;
Should there be a check to ensure that fn_pin_map[0] is not already assigned to something other than PCI_INT_A?
https://review.coreboot.org/c/coreboot/+/49408/comment/020ee4a0_27754dd2
PS26, Line 226: if (constraints[i].type == UNIQUE_IRQ) {
If you invert this check, additional tab on rest of the lines can be avoided.
```
if (constraints[i].type != UNIQUE_IRQ)
continue;
```
https://review.coreboot.org/c/coreboot/+/49408/comment/0f2fbe6c_ff24cd66
PS26, Line 337: entry_count
Should `entry_count` be initialized to -1 by default? If there is a failure in `assign_slot()`, `entry_count` can be set to 0 and we don't have to go through the assignment functions again since it is anyways going to fail.
https://review.coreboot.org/c/coreboot/+/49408/comment/e3c1b533_af3246a0
PS26, Line 342: soc_irq_constraints
Since `assign_pci_irqs()` is called by SoC code, can we accept this list as an input parameter rather than making a callback into SoC for getting the IRQ constraints? Also, I think you will need a `count` parameter to indicate the number of slots that the SoC has provided in the constraints list.
--
To view, visit https://review.coreboot.org/c/coreboot/+/49408
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I0c22a08ce589fa80d0bb1e637422304a3af2045c
Gerrit-Change-Number: 49408
Gerrit-PatchSet: 26
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Duncan Laurie <duncan(a)iceblink.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.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)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Comment-Date: Tue, 04 May 2021 00:37:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Shelley Chen, Ravi kumar, Sajida Bhanu, mturney mturney.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50583 )
Change subject: herobrine: Enable macronix SPI config
......................................................................
Patch Set 32:
(1 comment)
Patchset:
PS32:
Why Macronix? Are we planning to use that for Herobrine? (This would be the first I heard of us using them on any Chrome OS device...)
Last I heard was that Herobrine rev0 was supposed to reuse the Trogdor rev2 I/O board, and on my schematics that is listed as GigaDevice. I would suggest you at least leave the Kconfigs for Winbond and GigaDevice in there, because we tend to use those two very often. (If you're using Macronix on your IDP then feel free to add that in addition, but I don't think you should remove the other too.)
--
To view, visit https://review.coreboot.org/c/coreboot/+/50583
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I505ee95d9f2ca16baf244135b3e2e8fe72f93491
Gerrit-Change-Number: 50583
Gerrit-PatchSet: 32
Gerrit-Owner: Ravi kumar <rbokka(a)codeaurora.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Sajida Bhanu <sbhanu(a)codeaurora.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: mturney mturney <mturney(a)codeaurora.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Ravi Kumar Bokka <c_rbokka(a)qualcomm.corp-partner.google.com>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Ravi kumar <rbokka(a)codeaurora.org>
Gerrit-Attention: Sajida Bhanu <sbhanu(a)codeaurora.org>
Gerrit-Attention: mturney mturney <mturney(a)codeaurora.org>
Gerrit-Comment-Date: Tue, 04 May 2021 00:01:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Martin Roth, Jakub Czapiga, Angel Pons.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52444 )
Change subject: tests: enable code coverage for unit tests
......................................................................
Patch Set 4:
(1 comment)
File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/52444/comment/262456f6_59e0ca4b
PS4, Line 164: clean-unit-tests
I added a 'clean' here because if some of the code has already been built without gcov, it will cause build errors. At least for the first iteration of building coverage, I think we want coverage over all of the unit tests. Later, when we add options to build coverage just for one test, we can revisit building the code into a separate build directory so that we don't have to clean when switching between coverage and no-coverage.
--
To view, visit https://review.coreboot.org/c/coreboot/+/52444
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6ea780ee9e246c0bb8c35b8e0de4252431dabbff
Gerrit-Change-Number: 52444
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Fagerburg <pfagerburg(a)chromium.org>
Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Mon, 03 May 2021 23:28:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Martin Roth, Jakub Czapiga, Angel Pons.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Jakub Czapiga,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/52444
to look at the new patch set (#4).
Change subject: tests: enable code coverage for unit tests
......................................................................
tests: enable code coverage for unit tests
Add a new `coverage-unit-tests` make target that builds the unit
tests for code coverage, runs the tests, and generates a coverage
report.
Signed-off-by: Paul Fagerburg <pfagerburg(a)google.com>
Change-Id: I6ea780ee9e246c0bb8c35b8e0de4252431dabbff
---
M tests/Makefile.inc
1 file changed, 16 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/52444/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/52444
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6ea780ee9e246c0bb8c35b8e0de4252431dabbff
Gerrit-Change-Number: 52444
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Fagerburg <pfagerburg(a)chromium.org>
Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Martin Roth, Jakub Czapiga, Angel Pons.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52444 )
Change subject: tests: enable code coverage for unit tests
......................................................................
Patch Set 3:
(2 comments)
File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/52444/comment/4ca75a77_9cd2edef
PS3, Line 165: build/tests
> I think you can use $(testobj) instead. […]
Done
https://review.coreboot.org/c/coreboot/+/52444/comment/337a5a75_91ba5f7a
PS3, Line 205: Generate a code coverage report
> This might be misleading. […]
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/52444
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6ea780ee9e246c0bb8c35b8e0de4252431dabbff
Gerrit-Change-Number: 52444
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Fagerburg <pfagerburg(a)chromium.org>
Gerrit-Reviewer: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Mon, 03 May 2021 23:25:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-MessageType: comment
Attention is currently required from: Jakub Czapiga, Julius Werner, Jan Dabros.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52708 )
Change subject: tests/lib/crc_byte-test: Fix incorrect variable types
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/52708
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I61ee029a6950a8dfeb54520b634eaf4ed6bac576
Gerrit-Change-Number: 52708
Gerrit-PatchSet: 2
Gerrit-Owner: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-Reviewer: Jan Dabros <jsd(a)semihalf.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Fagerburg <pfagerburg(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jakub Czapiga <jacz(a)semihalf.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Jan Dabros <jsd(a)semihalf.com>
Gerrit-Comment-Date: Mon, 03 May 2021 23:17:10 +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 uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/52870 )
Change subject: soc/amd/cezanne: Generate PCI GPP ACPI names
......................................................................
soc/amd/cezanne: Generate PCI GPP ACPI names
We can generate the names, so there is no need to hard code a table.
This will make the code more generic so it can be reused with picasso in
the future.
BUG=b:184766519
TEST=Dump guybrush ACPI table and verify it looks correct.
Signed-off-by: Raul E Rangel <rrangel(a)chromium.org>
Change-Id: I5134d1dba4fcb9ce8cc4bfad1c619331a95f3b11
---
M src/soc/amd/cezanne/pcie_gpp.c
1 file changed, 7 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/52870/1
diff --git a/src/soc/amd/cezanne/pcie_gpp.c b/src/soc/amd/cezanne/pcie_gpp.c
index 66f20d5..2a85917 100644
--- a/src/soc/amd/cezanne/pcie_gpp.c
+++ b/src/soc/amd/cezanne/pcie_gpp.c
@@ -16,40 +16,16 @@
static const char *pcie_gpp_acpi_name(const struct device *dev)
{
+ char *name;
+
if (dev->path.type != DEVICE_PATH_PCI)
return NULL;
- switch (dev->path.pci.devfn) {
- case PCIE_GPP_1_0_DEVFN:
- return "GP10";
- case PCIE_GPP_1_1_DEVFN:
- return "GP11";
- case PCIE_GPP_1_2_DEVFN:
- return "GP12";
- case PCIE_GPP_2_0_DEVFN:
- return "GP20";
- case PCIE_GPP_2_1_DEVFN:
- return "GP21";
- case PCIE_GPP_2_2_DEVFN:
- return "GP22";
- case PCIE_GPP_2_3_DEVFN:
- return "GP23";
- case PCIE_GPP_2_4_DEVFN:
- return "GP24";
- case PCIE_GPP_2_5_DEVFN:
- return "GP25";
- case PCIE_GPP_2_6_DEVFN:
- return "GP26";
- case PCIE_ABC_A_DEVFN:
- return "GPPA";
- case PCIE_GPP_B_DEVFN:
- return "GPPB";
- case PCIE_GPP_C_DEVFN:
- return "GPPC";
- default:
- printk(BIOS_ERR, "%s: Unhanded devfn 0x%x\n", __func__, dev->path.pci.devfn);
- return NULL;
- }
+ name = malloc(ACPI_NAME_BUFFER_SIZE);
+ snprintf(name, ACPI_NAME_BUFFER_SIZE, "GP%02x", dev->path.pci.devfn);
+ name[4] = '\0';
+
+ return name;
}
/* b/187083211 - Enable GNB IO-APIC */
--
To view, visit https://review.coreboot.org/c/coreboot/+/52870
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5134d1dba4fcb9ce8cc4bfad1c619331a95f3b11
Gerrit-Change-Number: 52870
Gerrit-PatchSet: 1
Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: 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-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-MessageType: newchange