From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bin Meng Date: Mon, 13 Aug 2018 10:24:01 +0800 Subject: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes In-Reply-To: <972158e3-e0bd-4798-699f-06a97d7100d1@gmail.com> References: <20180808130302.23327-1-marek.vasut+renesas@gmail.com> <25f52264-9bbc-f2d8-b3df-f2a164ad9881@gmail.com> <1858e8ca-9374-be2d-e102-1defeacbad8c@gmail.com> <20180810120135.GH29229@bill-the-cat> <972158e3-e0bd-4798-699f-06a97d7100d1@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 Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut wrote: > On 08/10/2018 02:01 PM, Tom Rini wrote: >> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote: >>> On 08/08/2018 05:32 PM, Bin Meng wrote: >>>> Hi Marek, >>>> >>>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut wrote: >>>>> On 08/08/2018 03:39 PM, Bin Meng wrote: >>>>>> Hi Marek, >>>>>> >>>>>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut wrote: >>>>>>> On 08/08/2018 03:14 PM, Bin Meng wrote: >>>>>>>> Hi Marek, >>>>>>>> >>>>>>>> On Wed, Aug 8, 2018 at 9:03 PM, Marek Vasut wrote: >>>>>>>>> The PCI controller can have DT subnodes describing extra properties >>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller >>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node >>>>>>>>> to the PCI device instance, so that the driver can extract details >>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem. >>>>>>>>> >>>>>>>>> Signed-off-by: Marek Vasut >>>>>>>>> Cc: Simon Glass >>>>>>>>> --- >>>>>>>>> drivers/pci/pci-uclass.c | 14 ++++++++++++++ >>>>>>>>> 1 file changed, 14 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c >>>>>>>>> index 46e9c71bdf..306bea0dbf 100644 >>>>>>>>> --- a/drivers/pci/pci-uclass.c >>>>>>>>> +++ b/drivers/pci/pci-uclass.c >>>>>>>>> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct udevice *parent, >>>>>>>>> for (id = entry->match; >>>>>>>>> id->vendor || id->subvendor || id->class_mask; >>>>>>>>> id++) { >>>>>>>>> + ofnode node; >>>>>>>>> + >>>>>>>>> if (!pci_match_one_id(id, find_id)) >>>>>>>>> continue; >>>>>>>>> >>>>>>>>> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct udevice *parent, >>>>>>>>> goto error; >>>>>>>>> debug("%s: Match found: %s\n", __func__, drv->name); >>>>>>>>> dev->driver_data = find_id->driver_data; >>>>>>>>> + >>>>>>>>> + dev_for_each_subnode(node, parent) { >>>>>>>>> + phys_addr_t df, size; >>>>>>>>> + df = ofnode_get_addr_size(node, "reg", &size); >>>>>>>>> + >>>>>>>>> + if (PCI_FUNC(df) == PCI_FUNC(bdf) && >>>>>>>>> + PCI_DEV(df) == PCI_DEV(bdf)) { >>>>>>>>> + dev->node = node; >>>>>>>>> + break; >>>>>>>>> + } >>>>>>>> >>>>>>>> The function pci_find_and_bind_driver() is supposed to bind devices >>>>>>>> that are NOT in the device tree. Adding device tree access in this >>>>>>>> routine is quite odd. You can add the EHCI controller that need such >>>>>>>> PHY subnodes in the device tree and there is no need to modify >>>>>>>> anything I believe. If you are looking for an example, please check >>>>>>>> pciuart0 in arch/x86/dts/crownbay.dts. >>>>>>> >>>>>>> Well this does not work for me, the EHCI PCI doesn't get a DT node >>>>>>> assigned, check r8a7794.dtsi for the PCI devices I use. >>>>>>> >>>>>> >>>>>> I think that's because you don't specify a "compatible" string for >>>>>> these two EHCI PCI nodes. >>>>> >>>>> That's perfectly fine, why should I specify it ? Linux has no problem >>>>> with it either. >>>>> >>>> >>>> Without a "compatible" string, DM does not bind any device in the >>>> device tree to a driver, hence no device node created. This is not >>>> Linux. >>> >>> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and >>> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and >>> must be fixed. >>> >>> This is a fix. If there is a better fix, I am open to it. >> >> DT should but isn't always OS agnostic. DTS files that reside in the >> Linux Kernel are in practice is Linux-centric with the expectation that >> even if you could solve a given problem with valid DTS changes you make >> whatever is parsing it do additional logic instead. That, >> approximately, is what your patch is doing. If you added some HW >> description information to the dtsi file everything would work as >> expected as your DTS is describing the hardware and U-Boot is reading >> that description and figuring out what to do with it. > > Yes, you need additional logic to match the PCI controller subnode in DT > with PCI device BFD, that's expected. You do NOT need extra compatibles, > the PCI bus gives you enough information to match a driver on them. In > fact, adding a compatible can interfere with this matching. > Please, read U-Boot's doc/driver-model/pci-info.txt. You really don't understand current implementation in U-Boot. In short, 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 can choose either two when you support PCI devices on your board, but you cannot mix both support together and make them a mess. In this patch, you hacked pci_find_and_bind_driver() which is the 2nd scenario to support the 1st scenario. >> The problem here is that in Linux, something that sees the compatible >> renesas,pci-r8a7794 or renesas,pci-rcar-gen2 is doing whatever else >> needs to be done. Or something else? Please explain how what you want >> to have work here in U-Boot is working in the Linux kernel. Thanks! > > This has nothing to do with a specific controller. iMX6 does the same > thing, see ie arch/arm/boot/dts/imx6q-utilite-pro.dts . > Regards, Bin