Hi Tom,
On Tue, Aug 14, 2018 at 1:16 AM, Tom Rini trini@konsulko.com wrote:
On Mon, Aug 13, 2018 at 06:07:14PM +0200, Marek Vasut wrote:
On 08/13/2018 03:39 PM, Tom Rini wrote: [...]
Next step is to upstream the DT changes to Linux kernel, then sync the changes to U-Boot to satisfy this obsession - using exactly the same DT as Linux.
This is not gonna happen.
Sorry, you're really just wasting my time with this foolishness. If U-Boot cannot parse valid DT bindings while other OSes can, U-Boot is broken and must be fixed. So far I only see you attacking this patch and trying to pull in everything you can do avoid accepting this patch or providing a better alternative. This is not a constructive discussion, so I stop here.
The fix in this patch is purely hack, period.
Lets step back for a moment.
First, U-Boot intends to be, in the case where a relevant DTS file exists, the Linux kernel one PLUS additions we require (u-boot,dm-spl, u-boot,dm-pre-reloc, but also sometimes stdout-path or properties that are omitted for various reasons).
Right, which doesn't apply here. None of those u-boot,... props are needed in this case.
Which is why I also mentioned the non-u-boot specific props we also need sometimes. My point is two-fold: 1: We can and will _add_ information to the dts files that come from Linux. 2: Not all information that we add is U-Boot prefixed.
It would be better if we document such DT expectation somewhere.
Second, I've asked before (both in this thread and on IRC), and not gotten an answer yet, as to how Linux goes "Oh, _this_ PCI device and _this_ DT node need to be matched and populate some data structures".
You did get an answer to that on irc from George. Looks like of_pci_find_child_device() in drivers/pci/of.c
Yeah, George said he thought that might be it but didn't have time to confirm.
Marek's patch seems to be, in short "here's where U-Boot needs to wire things up". Bin has said that no, the function in question is for other things.
I disagree with this. It's a bind function and assigns other parameters of the driver instance too.
I think knowing where Linux does this would be instructive to figure out where we need to have some additional logic added OR we can make some cost/benefit analysis to see if it makes more sense overall to add compatibles to some nodes rather than add to the binary size.
Adding compatible does not make any sense, the PCI ID provides that information. Adding compatible would only add redundancy which could possibly be even harmful (ie. if the controller got replaced with another one).
To try and move things along rather than re-argue the same point, you're saying that our pci_find_and_bind_driver() is the rough equivalent of of_pci_find_child_device() or at least pci_set_of_node() (which calls of_pci_find_child_device()).
So, Bin, if this isn't the right place to start down this path, where would be? Given that Linux can take a DTB and PCI bus with devices and get things right, what would it look like for U-Boot to replicate the same behavior? Instead of having to add explicit compatible nodes for each PCI device, as I understand (but correct me if I'm wrong) we're doing today. Thanks!
So is this a requirement for all U-Boot driver subsystems to replicate the same Linux behavior? If yes, can we have it officially documented somewhere?
Since Marek refused to take the original U-Boot option 1 to support his case, and asked U-Boot to follow Linux's practice on PCI device binding, if we go that way, here is what we can do:
* Keep pci-uclass driver's post_bind() and child_post_bind() only for Sandbox configuration * Keep the call to pci_bus_find_devfn() in pci_bind_bus_devices() only for Sandbox configuration * Sandbox is special. We should limit the mechanism of matching PCI emulation device via "compatible" to sandbox only * Assign the DT node to the bound device in pci_find_and_bind_driver() if there is a valid PCI "reg" encoding for a specific PCI device in the device tree * Create DM PCI test case against the DT node assignment * Remove all compatible string in U-Boot's PCI device drivers: eg: ehci_pci_ids[], xhci_pci_ids[], etc. IOW, all PCI device drivers should only use U_BOOT_PCI_DEVICE(), aka the original U-Boot option 2 * Fork a "pci-ns16550" driver to support U_BOOT_PCI_DEVICE(), as currently PCI ns16550 device driver uses "compatible" string to do the matching, and update crownbay.dts and galileo.dts (so far I only know two boards are using PCI ns16550 serial port) * Make sure all DM PCI test cases are not broken * Document all of the above changes in doc/driver-model/pci-info.txt
I am not sure if I missed anything. Simon, could you also comment on it?
Regards, Bin