[U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes

[Tom Rini <trini@konsulko.com>]

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 <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.

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: <http://lists.denx.de/pipermail/u-boot/attachments/20180814/36ff1731/attachment.sig>