From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Tue, 14 Aug 2018 10:54:38 +0200 Subject: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes In-Reply-To: References: <02fc6d4a-40ee-317b-da77-a7a5b247fd86@gmail.com> <20180813133902.GT29229@bill-the-cat> <20180813171616.GH29229@bill-the-cat> Message-ID: <83281681-4568-d13d-6d97-8d78f6291382@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 08/14/2018 04:34 AM, Bin Meng wrote: > Hi Tom, > > On Tue, Aug 14, 2018 at 1:16 AM, Tom Rini 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. > 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. > * 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. > * 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 ? > * 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 > -- Best regards, Marek Vasut