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

[Richard Spiegel (Code Review)]

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

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


Patch Set 4:

(4 comments)

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@28 
PS4, Line 28: 	outl(reg_2_cf8(dev, reg), 0xCF8);
> > Existing access functions assume bus 0. […]
I could not make it work. I can try again, but so far I could not make pci_read_configx work on a non-zero bus. Also, at ramstage it assumes that we are pointing to a structure which contains the information about the exact PCI... That's not the case for any device behind a bridge (that I'm aware of), as the exact bus is dependent on the exact PCI tree. Add/remove a card, or change the bridge being used for the device and bus will change.


https://review.coreboot.org/#/c/30215/4/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@66 
PS4, Line 66: 		value &= start;
> > It does not matter. […]
If you follow the code, it'll never go to an address bellow next_mem (except if you assign a range that's not aligned) The code will actually force holes to align any other BAR to the next 1K above the last used by simply forcing any size to be at least 1K. If the actual required size is less then 1K, it'll create a hole between 2 BAR.

It does not matter - because this happens before PCI enumeration, used briefly, and destroyed before the enumeration happens.


https://review.coreboot.org/#/c/30215/4/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@79 
PS4, Line 79: 	pci_write32(dev, TEMP_BASE(number), value);
> Which code? I haven't seen it.
I see, the bits I thought I was forcing to 0 are actually read only. The code works because on my particular bridge the high address is reset to 0... but there's no warranty that all bridges will follow. Will force the high address to 0.


https://review.coreboot.org/#/c/30215/4/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@80 
PS4, Line 80: 	command |= PCI_COMMAND_MASTER;
> Please elaborate, I'm not used to it. […]
I don't know the exact answer to your question. What I do know is that PCI enumeration always set this bit on any bridge/device that has at least one BAR (memory or IO) being used.



-- 
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: 4
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: Mon, 17 Dec 2018 15:26:44 +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/20181217/ae3818b6/attachment.html>