From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Tue, 14 Aug 2018 15:39:41 -0400 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: <20180814193941.GC30947@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, Aug 14, 2018 at 10:34:23AM +0800, 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. Yes, it should be written down somewhere. But it's been a general expectation for a long time. It's why we even have the -u-boot.dtsi auto-include logic, so that the main dts/dtsi files can be re-synced without losing changes U-Boot needs. > >> > 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? Without very good and documented reasons, yes, it's an expectation that concepts / details we borrow from Linux should work like or close to Linux. Large parts of the MTD subsystem for example are re-synced. > 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 Yes, Sandbox is special and if we #if that part as needed, that might be good too. > * 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? Yes, some further thoughts on how to move this forward would be helpful, thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: