[coreboot-gerrit] Change in ...coreboot[master]: drivers/generic: Add PCIe bridge helper

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30215 )

Change subject: drivers/generic: Add PCIe bridge helper
......................................................................


Patch Set 5:

(10 comments)

I have exercised a simple number example on PS5 to show you the align-down problem. It's assuming two memory resources, the first 4KiB, the second 8KiB in size.

Please don't put further effort into the implementation at this time, until we have discussed the API.

Your use case is quite simple and I think most are. You just want to access a single memory resource. All you need from the caller is to tell what resource, mapped where. But the API you provide here is much more flexible and obviously results in error-prone implementations. So please, trim this down to what you actually need. If we ever need more flexibility, we can still implement that later.

https://review.coreboot.org/#/c/30215/4/src/drivers/generic/temp_pcie_bridge/bridge_rom.c 
File src/drivers/generic/temp_pcie_bridge/bridge_rom.c:

https://review.coreboot.org/#/c/30215/4/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@80 
PS4, Line 80: 	else
> I don't know the exact answer to your question. […]
Which PCI enumeration? coreboot's?

    git grep PCI_COMMAND_MASTER -- src/device/

shows only two cases: IORESOURCE_PCI_BRIDGE and PCI_BASE_CLASS_SYSTEM.
I guess the first case is only a shortcut so you don't have to walk
the path up to set it everywhere in case you need to set it for a
downstream device (so that upstream requests can be forwarded all
the way to the host bridge).


https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c 
File src/drivers/generic/temp_pcie_bridge/bridge_rom.c:

https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@21 
PS5, Line 21: 		    uint32_t *base, uint32_t start, uint32_t *size)
1. start = 0xf4000000
2. start = 0xf4001000


https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@28 
PS5, Line 28: 	value = pci_read_config32(dev, TEMP_BASE(number));
1. resource (4KiB): value = 0xfffff000
2. resource (8KiB): value = 0xffffe000


https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@33 
PS5, Line 33: 		value &= 0xfffffc00; /* At least 1K between addresses */
1. value = 0xfffff000
2. value = 0xffffe000


https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@34 
PS5, Line 34: 		*size = 1 + ~value;
1. size = 0x1000
2. size = 0x2000


https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@35 
PS5, Line 35: 		value &= start;
1. value = 0xf4000000
2. value = 0xffffe000 & 0xf4001000 = 0xf4000000 oops


https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@36 
PS5, Line 36: 		*base = value;
1. base = 0xf4000000


https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@76 
PS5, Line 76: 		next_mem = TP_DEFAULT_MEM_START;
next_mem = 0xf4000000


https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@87 
PS5, Line 87: 		case MEMORY_ADDRESS:
1. next_mem = 0xf4000000
2. next_mem = 0xf4001000


https://review.coreboot.org/#/c/30215/5/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@93 
PS5, Line 93: 				next_mem = (return_base + return_size);
1. next_mem = 0xf4001000



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/30215
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iaec1fcc4f8db8c3b4cfd3786d3ff589dc9cb22f5
Gerrit-Change-Number: 30215
Gerrit-PatchSet: 5
Gerrit-Owner: Richard Spiegel <richard.spiegel at silverbackltd.com>
Gerrit-Reviewer: Daniel Kurtz <djkurtz at google.com>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi at google.com>
Gerrit-Reviewer: Patrick Rudolph <siro at das-labor.org>
Gerrit-Reviewer: Raul Rangel <rrangel at chromium.org>
Gerrit-Reviewer: Richard Spiegel <richard.spiegel at silverbackltd.com>
Gerrit-Reviewer: Simon Glass <sjg at chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Nico Huber <nico.h at gmx.de>
Gerrit-Comment-Date: Tue, 18 Dec 2018 15:53:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Richard Spiegel <richard.spiegel at silverbackltd.com>
Comment-In-Reply-To: Nico Huber <nico.h at gmx.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20181218/e5b9d5b4/attachment.html>