From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Tue, 21 Aug 2018 12:28:45 +0200 Subject: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes In-Reply-To: References: <92b4b0cc-b3fb-45c8-20f9-a232c2891edf@gmail.com> <20180815112540.GG30947@bill-the-cat> <6aa50a30-df29-07fe-4d12-f9cbdae82df1@gmail.com> <8c47a8b1-13ea-d0b1-cca2-a4d51bc566a4@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 On 08/21/2018 09:16 AM, Bin Meng wrote: > Hi Marek, > > On Tue, Aug 21, 2018 at 1:43 PM, Marek Vasut wrote: >> On 08/21/2018 06:56 AM, Bin Meng wrote: >> [...] >>>>>>> The proposal I made is: >>>>>>> >>>>>>> * 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 >>>>>> >>>>>> The above three points can be done separately and I don't care about >>>>>> this too much. >>>>>> >>>>> >>>>> That's my concern. Only doing this created confusing and incomplete >>>>> design. >>>> >>>> Incomplete how ? >>> >>> I explained several times. I have to repeat here once again: >>> >>> Currently U-Boot supports two scenarios for PCI driver binding: >>> >>> - Declare a PCI device in the device tree. That requires specifying a >>> 'compatible' string as well as 'reg' property as defined by the 'PCI >>> Bus Binding' spec. DM uses the 'compatible' string to bind the driver >>> for the device. >>> - Don't declare a PCI device in the device tree. Instead, using >>> U_BOOT_PCI_DEVICE() to declare a device and driver mapping. >>> >>> You are creating a 3rd scenario which is a mix up of both scenario#1 >>> and #2 in this patch, yet without any documentation, plus any >>> additional changes to impacted drivers. >> >> I am extending both, it's not a "third scenario". >> >> I said in the previous discussion I am willing to update the >> documentation to match the implementation, but that's about it. >> >>> So far almost all PCI device drivers in U-Boot supports both >>> scenarios, except PCI UART which currently only supports scenario#1. >>> If you declare what you do in this patch is pureblood then you should >>> revoke the other 2 at the same time. Leaving such in the mainstream >>> only creates chaos in the future and we should avoid doing that, given >>> we already had lots of discussion here. >> >> No. The compatible is valid for some PCI subdevs, ie. bridges, so that >> has to stay. You also need compatible for sandbox, you said that >> yourself. And declaring a PCI device with BFD in DT is needed, ie. for >> the r8a7794-style bindings. >> > > OK, now new comments :) So you admit "compatible" can be valid for > some cases. _some_ select ones, and that is a _very_ important distinction. > I have to point out that your theory on PCI device probing > is self-contradictory.You refuse to add a "compatible" string to your > PCI device because you said the vendor/device id/class provides enough > information to bind the driver, then why do you want to specify your > devices in the device tree in the first place. Because of the USB PHY , which is attached to that device and can not be probed on the PCI bus, unlike the device itself ? > According to your > theory, "Each PCI device already has a PCI ID and class which is used > to identify it and based on which the drivers bind to it, so a DT > compatible is NOT needed and is actually redundant and harmful.", I > would argue why not just creating a dedicate PCI device driver using > PCI ID and class information to do the driver binding and start from > there. Because the same device with the same PCI ID can be used without that PHY . > As you mentioned PCI bus is probable bus like USB, everything > can be self-contained in the PCI device driver itself. There is no > need to create nothing in the device tree. If you want an example, > check 8250_pci.c in the Linux kernel. Everything that is needed to > configure the driver is included in the driver itself. It does not > read anything from the device tree. Well, see above for why this approach doesn't work. >> So again, how is the state of PCI more incomplete with this patch than >> without it ? >> >>>>> Yes, you probably don't care about this. But I care about this >>>>> as it impacts some other things like PCI uart driver which is used by >>>>> some x86 boards. >>>> >>>> Sandbox changes impact PCI UART driver and x86 boards how ? >>> >>> See above. It's not about sandbox! >> >> So what is it about ? And how does this core change impact anything ? >> Does it change behavior for some board or break some tests for you ? >> >>>>>>> * 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 think you're just adding completely orthogonal stuff to this 5-liner >>>>>> patch into the list and overly complicate the situation. Sure, if you >>>>>> want to do all this, go ahead, but I don't see how this prevents this >>>>>> particular patch from being applied , except maybe for the documentation >>>>>> tweak. >>>>> >>>>> Please, think from the maintainer perspective. It's nothing related to >>>>> number of LOCs. It's that we need create a clean design. You probably >>>>> don't want someone just submits a single patch that changed current >>>>> USB design without a full consideration on all possible impacts. >>>> >>>> OK, after reading all this, what I gather from the discussion is that >>>> this patch somehow breaks the design or the other platforms, which is >>>> why you fight against it so much. Does it, yes or no? And if so, you >>>> still failed to explain how, so, how ? >> >> You did not answer this question. >> > > It's too early to go testing at this point because we are still > discussing the design. OK, so basically you didn't even try this patch to see if it actually has any impact anywhere. Sigh ... > BTW your patch does not check the return value > of ofnode_get_addr_size(). It's possible to write a DT without "reg" > property in a device node. That would be invalid DT, since you won't be able to match on the BFD. But that check can be added in V2. >>>> Improving sandbox and x86 PCI handling is orthogonal to this change, so >>>> are ns16550 driver improvements and if you want to work on them, nothing >>>> prevents you from doing so. >>> >>> First of all, it's not x86 PCI handling. It's PCI device driver clean >>> up to match the new design. >> >> It is not a new design, just an extension of the current one. >> >> Driver cleanups where you match on PCI IDs or classes instead of >> compatible strings are completely orthogonal to this change, you don't >> even need this patch to do such a cleanup. Same applies for the x86 DT >> cleanup, you don't need this patch for those either. >> > > Why would I? If there is no design changes, the clean up work to > current drivers is out of the question as these drivers are supported > user cases by current U-Boot PCI implementation. Then leave them as is. But don't request unrelated cleansup from me either. I really do not understand why I should do those cleanups to get in an completely unrelated patch, that's just nonsense. [...] -- Best regards, Marek Vasut