Hi Marek,
On Tue, Aug 14, 2018 at 4:54 PM, Marek Vasut marek.vasut@gmail.com wrote:
On 08/14/2018 04:34 AM, Bin Meng wrote:
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?
No, because we are not replicating Linux behavior.
But you kept mentioning you wanted U-Boot to use exactly the same DT from Linux. And I pointed out that FreeBSD's DT files are not the same as Linux. Here you are saying you don't want U-Boot to replicate Linux behavior, if not the Linux behavior, what do you want to suggest then?
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:
You are inserting words into my mouth and I dislike that. I never said anything about Linux. I said DT is OS agnostic and U-Boot should be able to parse DT as fully as possible.
See above comment. I might have used some words that made you feel uncomfortable, and for me I felt the same way. Part of the reason is that I am not a native English speaker and I may mis-use/interpret words. If that's the case I apologize. Anyway I won't quarrel against this over and over again. This does not help to move things forward.
- 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
I don't think this is limited to sandbox, although I don't see a real-world usecase right now.
You are welcome to send patches against sandbox PCI codes to remove such limitation, to make it behave like a real-world bus device.
- 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)
I cannot test such changes, but I believe there is PCI_CLASS_COMMUNICATION_SERIAL and matching on that would suffice ?
Maybe, but I need check the datasheet to confirm. This can be a good start though. Note I can test the changes, and I can do the whole bunch of such proposed design changes if you don't mind, but let's wait for Simon's comments.
- 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