From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Wed, 29 Aug 2018 18:29:23 -0600 Subject: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes In-Reply-To: <510fa718-a5bb-bdec-b6bb-828acf10a13a@gmail.com> References: <98561a43-18bf-3c76-d3c6-3320cdafdf4b@gmail.com> <7a1aa6ed-7ddd-551a-f445-171465dbbe46@gmail.com> <92b4b0cc-b3fb-45c8-20f9-a232c2891edf@gmail.com> <20180815112540.GG30947@bill-the-cat> <911fd0e4-de0e-f1af-0314-0583f1c1d4e0@gmail.com> <510fa718-a5bb-bdec-b6bb-828acf10a13a@gmail.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Marek, On 23 August 2018 at 06:58, Marek Vasut wrote: > On 08/23/2018 12:45 PM, Simon Glass wrote: > [...] >>>> Why is pci_bus_find_devfn() failing? >>> >>> Because this function is a hack to force-bind drivers to PCI devices >>> which are described in the DT with a compatible string. This does not >>> apply to this case. >> >> pci_bus_find_devfn() does not check compatible strings. It checks the >> PCI devfn stuff. If the node is in the DT and has the correct devfn, >> then pci_bus_find_devfn() should find it. >> >> It does require that the node produced a device, which does indeed >> require a compatible string, I think. > > Well, yeah, I skipped the middle step for the sake of brevity. > Ultimately, you need a compat string there. > >> We support either using DT with a compatible string or using >> PCI_DEVICE() without a DT node. But you already know that. > > Right > >>> If this function fails (correctly, no force-binding needs to happen >>> because there is no compatible string, we want to match on PCI IDs), >>> the code will continue and match on PCI ID/class with >>> pci_find_and_bind_driver() . That function is where we need to verify if >>> there isn't a PCI controller subnode in DT with a matching BFD which >>> should be associated with the new driver instance. >> >> So all of this is to avoid having a compatible string in the DT node for PCI? > > Yes, because it is redundant and possibly harmful. And because I want to > parse existing DTs without special U-Boot modifications or > hacking/twisting U-Boot in some obscure way which doesn't scale. > >> Earlier I said: >> >>>> So can you just add a compatible string and everything works? >> >> and you replied: >> >>> No, because that makes no sense. The code can very well match the driver >>> on the PCI IDs or classes, the DT node is supplementary information and >>> it needs to be made available to the matched driver if present in the >>> DT. The DT node is matched on the BFD (the reg property in the DT). >> >> Yes the DT node is matched on the BFD. Yes we can match the node as you say. >> >>> The compatible string would only add redundancy and can in fact be >>> harmful in case the PCI device could be replaced (ie. swapping USB EHCI >>> controller for USB xHCI controller while retaining the USB PHY ; the >>> controller itself could be discovered on the PCI bus, but the PHY can >>> not and must be described in the DT). >> >> This seems a bit odd. Without the compatible string we don't know what >> driver to use, except through the PCI_DEVICE() mechanism. Is that what >> you are using? > > Of course, you can match on PCI IDs or PCI class already, PCI is a > discoverable bus. > >> So is this a reason why we need this 3rd option? For your example are >> you saying that there are two different settings for a device, which >> result in using a different driver? > > What I am saying is the following needs to be supported: > > pci-controller at 100 { > ... > usb at 1,0 { > reg = <0x800 0 0 0 0>; > phys = <&usb0 0>; > phy-names = "usb"; > }; > ... > }; > > You want to match the PCI device in BFD x.1.0 using PCI ID (ie. the > driver that matches in this case is *hci-pci) and then associate the > node usb at 1,0 , which contains PHY information for that *hci-pci driver, > with it. > >> How would DT normally handle this? > > That's how DTs do it, even for other than r8axxxx devices. There are iMX > devices which use the same approach for associating ethernet MAC with a > PCI NIC for example. > >> Overlays? Having two nodes and marking one status = "disabled"? This >> problem does not seem unique to PCI. I could have a similar problem >> with USB or even with a standard SoC having a memory-mapped USB >> controller that can support both protocols (controlled by a setting >> somewhere). > > You don't need overlays at all, the base DT is sufficient as is. > Everything up to here seems OK. >> If you have both EHCI and a xHCI controller which can occupy the same >> BFD, then how would you supply in the DT options needed by the >> controller itself? Don't you need two nodes in that case? > > For the PHY case, it's controller-type-independent. What do you mean? Your example of why you can't use compatible strings says you might have two different PHYs. But I think you should answer my questions: >> If you have both EHCI and a xHCI controller which can occupy the same >> BFD, then how would you supply in the DT options needed by the >> controller itself? Don't you need two nodes in that case? > >> Also, what if these two devices each had their own PHY and there were >> settings in that PHY DT node? You would need a separate node for each >> one, right? >> >> I suggest that we don't NEED this third way. The question in my mind >> is whether it is necessary and doesn't just add confusion. >> [...] >> In any case, re Bin's list of things that need doing, I worry about >> having different code for sandbox than other archs. It invalidates our >> sandbox testing. Granted, we have to support the PCI emulators, but >> that's OK since that code is not used except in sandbox. We still want >> to support compatible strings in the DT for PCI devices, right? > > We do, since there seems to be usecase for those too. OK, well let's make sure we have some compatible notes too in sandbox, so we retain testing. Regards, Simon