[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 4:

(4 comments)

>> Do you really need this in all its complexity? Many things seem overkill and the ramstage part doesn't fit into our current driver model.
> 
> Yes, I need. I need to access devices behind a bridge, before and after PCI enumeration. The only complexity is the possibility to access/program all the BARs. Currently, I only need BAR0.

I don't see how your eMMC-reset ramstage code does anything useful. You just check a register value without any immediate action. So you can just let the payload read the register.

That aside, you can just query the resource after allocation (e.g. with pci_get_resource()). Manual reading of the register would only be necessary before resource allocation. But that would have to be handled much more precisely (maybe I missed it, but I don't see anything reserving the temporary BAR, so having that enabled during allocation is dangerous).

> I could just force a particular range of addresses, but it does not add much complexity to allow user define a preferred range.

I would handle this very differently. For instance a function that takes a pci path, what resource to enable and a callback pointer. It enables the resource, calls back, takes it down again. Just do the whole procedure every time you want to talk to the device. That would keep coreboot's code flow free of any state to track.

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.

no (cf. PCI_DEV())


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.

it does

> I just need to assign a base address to one or more BAR registers. I don't care about alignment or holes...

You should. If it's aligning down, you might end up with an address below `next_mem`.


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);
> If you see the code, I'm forcing 32-bit resource, as it's temporary anyway.
Which code? I haven't seen it.


https://review.coreboot.org/#/c/30215/4/src/drivers/generic/temp_pcie_bridge/bridge_rom.c@80 
PS4, Line 80: 	command |= PCI_COMMAND_MASTER;
> I'm programming a device behind a bridge, before PCI initialization. So yes, it's needed.
Please elaborate, I'm not used to it. Does it have to be master to be able to send an asynchronous reply?



-- 
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: Sat, 15 Dec 2018 11:27:18 +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/20181215/5be13fd2/attachment.html>