From: Felipe Balbi <balbi@kernel.org> To: Arnd Bergmann <arnd@arndb.de>, linux-arm-kernel@lists.infradead.org Cc: Alan Stern <stern@rowland.harvard.edu>, Grygorii Strashko <grygorii.strashko@ti.com>, Catalin Marinas <catalin.marinas@arm.com>, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>, linux-usb@vger.kernel.org, Sekhar Nori <nsekhar@ti.com>, linux-kernel@vger.kernel.org, David Fisher <david.fisher1@synopsys.com>, "Thang Q. Nguyen" <tqnguyen@apm.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org> Subject: Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Date: Thu, 28 Apr 2016 09:37:08 +0300 [thread overview] Message-ID: <87y47ykzmz.fsf@intel.com> (raw) In-Reply-To: <5129992.Q57qoSDEqC@wuerfel> [-- Attachment #1: Type: text/plain, Size: 6191 bytes --] Hi, Arnd Bergmann <arnd@arndb.de> writes: > On Wednesday 27 April 2016 23:05:42 Felipe Balbi wrote: >> Arnd Bergmann <arnd@arndb.de> writes: >> > On Wednesday 27 April 2016 13:59:13 Alan Stern wrote: >> >> On Wed, 27 Apr 2016, Arnd Bergmann wrote: >> >> >> >> > I've looked at the usb HCD code now and see this: >> >> > >> >> > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver, >> >> > struct device *dev, const char *bus_name, >> >> > struct usb_hcd *primary_hcd) >> >> > { >> >> > ... >> >> > hcd->self.controller = dev; >> >> > hcd->self.uses_dma = (dev->dma_mask != NULL); >> >> > ... >> >> > } >> >> > >> >> > What I think we need to do here is ensure that the device that gets >> >> > passed here and assigned to hcd->self.controller is the actual DMA >> >> > master device, i.e. the pci_device or platform_device that was created >> >> > from outside of the xhci stack. This is after all the pointer that >> >> > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/... >> >> > functions. >> >> >> >> It would be better to add a new field, since self.controller is also >> >> used for lots of other purposes. Something like hcd->self.dma_dev. >> > >> > Ok, fair enough. I only took a brief look and all uses I found were >> > either for the DMA mapping API or some printk logging. >> >> I have a feeling you guys are not considering how the patch to implement >> this will look like. >> >> How are you expecting dwc3 to pass a pointer to the DMA device from >> dwc3.ko to xhci-plat ? platform_data ? That's gonna be horrible > > Not any worse than it already is really. It already uses platform_data > for the exact case that is causing the problem here. there's no use of platform_data for passing around DMA configuration. By platform_data I really mean platform_device_add_data(). >> Also, remember that the DMA device for dwc3 is not always >> dwc3->dev->parent. It might be dwc3->dev itself. How are you expecting >> us to figure that one out ? > > Do you have an example for this? The ones I found here either > create the dwc3 device from PCI or from a platform glue driver. arch/arm64/boot/dts/xilinx/zynqmp.dtsi >> I still think dma_inherit() (or something along those lines) is >> necessary. Specially when you consider that, as I said previously, >> that's pretty much what of_dma_configure() does. > > As I said, this is not an API that can work in general, because > it makes the assumption that everything related to DMA in a > device can be set in that device itself. > > The simplest case where this does not work is a PCI device behind > an IOMMU: when you call dma_map_single() or dma_alloc_coherent(), > the dma_map_ops implementation for the IOMMU has to look at the > PCI device to find out the association with an IOMMU context, > and on most architectures you cannot bind an IOMMU context to > a platform device at all. no, it relies on dev->archdata for IOMMU. In fact, the first "patch" (more of a hack) I wrote to fix IOMMU with dwc3 on Intel platforms was to literally memcpy() pci's archdata to dwc3->dev and it worked just fine with and without IOMMU enabled. >> Anyway, I'd really like to see a patch implementing this >> hcd->self.dma_dev logic. Consider all the duplication with this >> approach, btw. struct dwc3 will *also* need a dwc->dma_dev of its >> own. Will that be passed to dwc3 as platform_data from glue layer ? What >> about platforms which don't even use a glue layer ? > > Let's separate the three problems here. > > a) dwc3 creating a "xhci-hcd" platform_device that is not connected > to any proper bus. We can work around that by adding the "self.dma_dev" platform_bus_type *is* a proper bus. > pointer and pass that in platform_data. This is really easy, it's Sorry but passing a struct device pointer in platform_data is ridiculous. Not to mention that, as I said before, we can't assume which device to pass to xhci_plat in the first place. It might be dwc->dev and it might be dwc->dev->parent. > actually less code (and less iffy) than the current implementation of > copying the parent dma mask. > In the long run, this could be solved by doing away with the extra > platform_device, by calling a variant of xhci_probe() from > xhci_plat_probe() like we do for the normal *HCI drivers. no, that's not something I'll do for dwc3. We have had this talk before and I'm not giving up the benefits of splitting things to separate devices. > b) dwc3-pci creating a "dwc3" platform_device under the covers. This it's not under the covers at all. It's pretty similar to what MFD drivers do. It's just not wrapped in a nice API because there's no need for that. > is pretty much the exact same problem as a) on another layer. In > the short run, we can pass the device pointer as part of > struct dwc3_platform_data (dwc3-pci is the only such user anway), It's incredible that you'd even suggest this at all. Did we not have a big trouble to solve on ARM land because of different mach-* passing function pointers and other pointers from arch/arch/mach-* instead of abstracting things away. Then came DT to the rescue, a setup where you can't even pass code or kernel objects ;-) > and in the long run, it should be easy enough to get rid of the > extra platform device by just calling a variant of dwc3_probe, > which will again simplify the driver also again definitely not something I'll do > c) some SoCs that have two separate device nodes to describe their > dwc3 xhci. I don't think this is causing any additional problems, > but if we want to make this behave more like other drivers in the > long run or deal with machines that are missing a "dma-ranges" > property in the parent node, we can kill off the > of_platform_populate() hack and instead call dwc3_probe() > directly from the glue drivers as in b), and have that > do for_each_child_of_node() or similar to find the child node. no, we cannot. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: balbi@kernel.org (Felipe Balbi) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Date: Thu, 28 Apr 2016 09:37:08 +0300 [thread overview] Message-ID: <87y47ykzmz.fsf@intel.com> (raw) In-Reply-To: <5129992.Q57qoSDEqC@wuerfel> Hi, Arnd Bergmann <arnd@arndb.de> writes: > On Wednesday 27 April 2016 23:05:42 Felipe Balbi wrote: >> Arnd Bergmann <arnd@arndb.de> writes: >> > On Wednesday 27 April 2016 13:59:13 Alan Stern wrote: >> >> On Wed, 27 Apr 2016, Arnd Bergmann wrote: >> >> >> >> > I've looked at the usb HCD code now and see this: >> >> > >> >> > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver, >> >> > struct device *dev, const char *bus_name, >> >> > struct usb_hcd *primary_hcd) >> >> > { >> >> > ... >> >> > hcd->self.controller = dev; >> >> > hcd->self.uses_dma = (dev->dma_mask != NULL); >> >> > ... >> >> > } >> >> > >> >> > What I think we need to do here is ensure that the device that gets >> >> > passed here and assigned to hcd->self.controller is the actual DMA >> >> > master device, i.e. the pci_device or platform_device that was created >> >> > from outside of the xhci stack. This is after all the pointer that >> >> > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/... >> >> > functions. >> >> >> >> It would be better to add a new field, since self.controller is also >> >> used for lots of other purposes. Something like hcd->self.dma_dev. >> > >> > Ok, fair enough. I only took a brief look and all uses I found were >> > either for the DMA mapping API or some printk logging. >> >> I have a feeling you guys are not considering how the patch to implement >> this will look like. >> >> How are you expecting dwc3 to pass a pointer to the DMA device from >> dwc3.ko to xhci-plat ? platform_data ? That's gonna be horrible > > Not any worse than it already is really. It already uses platform_data > for the exact case that is causing the problem here. there's no use of platform_data for passing around DMA configuration. By platform_data I really mean platform_device_add_data(). >> Also, remember that the DMA device for dwc3 is not always >> dwc3->dev->parent. It might be dwc3->dev itself. How are you expecting >> us to figure that one out ? > > Do you have an example for this? The ones I found here either > create the dwc3 device from PCI or from a platform glue driver. arch/arm64/boot/dts/xilinx/zynqmp.dtsi >> I still think dma_inherit() (or something along those lines) is >> necessary. Specially when you consider that, as I said previously, >> that's pretty much what of_dma_configure() does. > > As I said, this is not an API that can work in general, because > it makes the assumption that everything related to DMA in a > device can be set in that device itself. > > The simplest case where this does not work is a PCI device behind > an IOMMU: when you call dma_map_single() or dma_alloc_coherent(), > the dma_map_ops implementation for the IOMMU has to look at the > PCI device to find out the association with an IOMMU context, > and on most architectures you cannot bind an IOMMU context to > a platform device at all. no, it relies on dev->archdata for IOMMU. In fact, the first "patch" (more of a hack) I wrote to fix IOMMU with dwc3 on Intel platforms was to literally memcpy() pci's archdata to dwc3->dev and it worked just fine with and without IOMMU enabled. >> Anyway, I'd really like to see a patch implementing this >> hcd->self.dma_dev logic. Consider all the duplication with this >> approach, btw. struct dwc3 will *also* need a dwc->dma_dev of its >> own. Will that be passed to dwc3 as platform_data from glue layer ? What >> about platforms which don't even use a glue layer ? > > Let's separate the three problems here. > > a) dwc3 creating a "xhci-hcd" platform_device that is not connected > to any proper bus. We can work around that by adding the "self.dma_dev" platform_bus_type *is* a proper bus. > pointer and pass that in platform_data. This is really easy, it's Sorry but passing a struct device pointer in platform_data is ridiculous. Not to mention that, as I said before, we can't assume which device to pass to xhci_plat in the first place. It might be dwc->dev and it might be dwc->dev->parent. > actually less code (and less iffy) than the current implementation of > copying the parent dma mask. > In the long run, this could be solved by doing away with the extra > platform_device, by calling a variant of xhci_probe() from > xhci_plat_probe() like we do for the normal *HCI drivers. no, that's not something I'll do for dwc3. We have had this talk before and I'm not giving up the benefits of splitting things to separate devices. > b) dwc3-pci creating a "dwc3" platform_device under the covers. This it's not under the covers at all. It's pretty similar to what MFD drivers do. It's just not wrapped in a nice API because there's no need for that. > is pretty much the exact same problem as a) on another layer. In > the short run, we can pass the device pointer as part of > struct dwc3_platform_data (dwc3-pci is the only such user anway), It's incredible that you'd even suggest this at all. Did we not have a big trouble to solve on ARM land because of different mach-* passing function pointers and other pointers from arch/arch/mach-* instead of abstracting things away. Then came DT to the rescue, a setup where you can't even pass code or kernel objects ;-) > and in the long run, it should be easy enough to get rid of the > extra platform device by just calling a variant of dwc3_probe, > which will again simplify the driver also again definitely not something I'll do > c) some SoCs that have two separate device nodes to describe their > dwc3 xhci. I don't think this is causing any additional problems, > but if we want to make this behave more like other drivers in the > long run or deal with machines that are missing a "dma-ranges" > property in the parent node, we can kill off the > of_platform_populate() hack and instead call dwc3_probe() > directly from the glue drivers as in b), and have that > do for_each_child_of_node() or similar to find the child node. no, we cannot. -- balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 818 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160428/e528cf76/attachment-0001.sig>
next prev parent reply other threads:[~2016-04-28 6:39 UTC|newest] Thread overview: 182+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-04-25 19:21 [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Grygorii Strashko 2016-04-25 19:21 ` Grygorii Strashko 2016-04-26 6:17 ` Felipe Balbi 2016-04-26 6:17 ` Felipe Balbi 2016-04-26 8:14 ` Grygorii Strashko 2016-04-26 8:14 ` Grygorii Strashko 2016-04-27 5:41 ` Felipe Balbi 2016-04-27 5:41 ` Felipe Balbi 2016-04-27 11:55 ` Grygorii Strashko 2016-04-27 11:55 ` Grygorii Strashko 2016-04-27 13:59 ` Catalin Marinas 2016-04-27 13:59 ` Catalin Marinas 2016-04-27 14:11 ` Arnd Bergmann 2016-04-27 14:11 ` Arnd Bergmann 2016-04-27 15:50 ` Catalin Marinas 2016-04-27 15:50 ` Catalin Marinas 2016-04-27 16:04 ` Arnd Bergmann 2016-04-27 16:04 ` Arnd Bergmann 2016-04-27 16:53 ` Felipe Balbi 2016-04-27 16:53 ` Felipe Balbi 2016-04-27 17:42 ` Arnd Bergmann 2016-04-27 17:42 ` Arnd Bergmann 2016-04-27 17:59 ` Alan Stern 2016-04-27 17:59 ` Alan Stern 2016-04-27 18:08 ` Arnd Bergmann 2016-04-27 18:08 ` Arnd Bergmann 2016-04-27 20:05 ` Felipe Balbi 2016-04-27 20:05 ` Felipe Balbi 2016-04-27 21:05 ` Arnd Bergmann 2016-04-27 21:05 ` Arnd Bergmann 2016-04-28 6:37 ` Felipe Balbi [this message] 2016-04-28 6:37 ` Felipe Balbi 2016-04-28 14:16 ` Russell King - ARM Linux 2016-04-28 14:16 ` Russell King - ARM Linux 2016-04-28 14:23 ` Arnd Bergmann 2016-04-28 14:23 ` Arnd Bergmann 2016-04-28 14:27 ` Felipe Balbi 2016-04-28 14:27 ` Felipe Balbi 2016-09-01 22:14 ` Leo Li 2016-09-01 22:14 ` Leo Li 2016-09-02 10:43 ` Arnd Bergmann 2016-09-02 10:43 ` Arnd Bergmann 2016-09-02 10:47 ` Russell King - ARM Linux 2016-09-02 10:47 ` Russell King - ARM Linux 2016-09-02 11:08 ` Felipe Balbi 2016-09-02 11:08 ` Felipe Balbi 2016-09-02 14:11 ` Felipe Balbi 2016-09-02 14:11 ` Felipe Balbi 2016-09-02 14:21 ` Alan Stern 2016-09-02 14:21 ` Alan Stern 2016-09-02 15:51 ` Arnd Bergmann 2016-09-02 15:51 ` Arnd Bergmann 2016-09-07 7:17 ` Roger Quadros 2016-09-07 7:17 ` Roger Quadros 2016-09-07 8:29 ` Arnd Bergmann 2016-09-07 8:29 ` Arnd Bergmann 2016-09-07 13:04 ` Roger Quadros 2016-09-07 13:04 ` Roger Quadros 2016-09-07 14:38 ` Arnd Bergmann 2016-09-07 14:38 ` Arnd Bergmann 2016-09-02 16:23 ` Grygorii Strashko 2016-09-02 16:23 ` Grygorii Strashko 2016-09-02 10:53 ` Felipe Balbi 2016-09-02 10:53 ` Felipe Balbi 2016-09-02 11:55 ` Robin Murphy 2016-09-02 11:55 ` Robin Murphy 2016-09-02 12:56 ` Felipe Balbi 2016-09-02 12:56 ` Felipe Balbi 2016-09-02 13:10 ` Arnd Bergmann 2016-09-02 13:10 ` Arnd Bergmann 2016-09-02 22:16 ` Leo Li 2016-09-02 22:16 ` Leo Li 2016-09-05 15:39 ` Arnd Bergmann 2016-09-05 15:39 ` Arnd Bergmann 2016-09-06 6:35 ` Peter Chen 2016-09-06 6:35 ` Peter Chen 2016-09-06 6:40 ` Felipe Balbi 2016-09-06 6:40 ` Felipe Balbi 2016-09-06 10:46 ` Arnd Bergmann 2016-09-06 10:46 ` Arnd Bergmann 2016-09-06 10:50 ` Felipe Balbi 2016-09-06 10:50 ` Felipe Balbi 2016-09-06 13:27 ` Arnd Bergmann 2016-09-06 13:27 ` Arnd Bergmann 2016-09-07 6:51 ` Felipe Balbi 2016-09-07 6:51 ` Felipe Balbi 2016-09-07 7:44 ` Peter Chen 2016-09-07 7:44 ` Peter Chen 2016-09-07 8:52 ` Arnd Bergmann 2016-09-07 8:52 ` Arnd Bergmann 2016-09-07 9:29 ` Peter Chen 2016-09-07 9:29 ` Peter Chen 2016-09-07 9:35 ` Russell King - ARM Linux 2016-09-07 9:35 ` Russell King - ARM Linux 2016-09-07 10:18 ` Felipe Balbi 2016-09-07 10:18 ` Felipe Balbi 2016-09-06 10:38 ` Arnd Bergmann 2016-09-06 10:38 ` Arnd Bergmann 2016-09-07 6:33 ` Peter Chen 2016-09-07 6:33 ` Peter Chen 2016-09-07 8:48 ` Arnd Bergmann 2016-09-07 8:48 ` Arnd Bergmann 2016-09-07 9:55 ` Peter Chen 2016-09-07 9:55 ` Peter Chen 2016-09-07 10:33 ` Robin Murphy 2016-09-07 10:33 ` Robin Murphy 2016-09-07 10:47 ` Felipe Balbi 2016-09-07 10:47 ` Felipe Balbi 2016-09-14 16:31 ` Lorenzo Pieralisi 2016-09-14 16:31 ` Lorenzo Pieralisi 2016-09-14 21:50 ` Arnd Bergmann 2016-09-14 21:50 ` Arnd Bergmann 2016-09-07 10:24 ` Felipe Balbi 2016-09-07 10:24 ` Felipe Balbi 2016-09-07 15:24 ` Arnd Bergmann 2016-09-07 15:24 ` Arnd Bergmann 2016-09-07 16:08 ` Alan Stern 2016-09-07 16:08 ` Alan Stern 2016-09-07 19:45 ` Arnd Bergmann 2016-09-07 19:45 ` Arnd Bergmann 2016-09-08 1:15 ` Peter Chen 2016-09-08 1:15 ` Peter Chen 2016-09-08 8:02 ` Arnd Bergmann 2016-09-08 8:02 ` Arnd Bergmann 2016-09-08 8:03 ` Felipe Balbi 2016-09-08 8:03 ` Felipe Balbi 2016-09-08 8:26 ` Arnd Bergmann 2016-09-08 8:26 ` Arnd Bergmann 2016-09-08 8:29 ` Felipe Balbi 2016-09-08 8:29 ` Felipe Balbi 2016-09-08 8:45 ` Arnd Bergmann 2016-09-08 8:45 ` Arnd Bergmann 2016-09-08 9:43 ` Felipe Balbi 2016-09-08 9:43 ` Felipe Balbi 2016-09-08 10:17 ` Arnd Bergmann 2016-09-08 10:17 ` Arnd Bergmann 2016-09-08 11:00 ` Felipe Balbi 2016-09-08 11:00 ` Felipe Balbi 2016-09-08 11:11 ` Arnd Bergmann 2016-09-08 11:11 ` Arnd Bergmann 2016-09-08 11:20 ` Felipe Balbi 2016-09-08 11:20 ` Felipe Balbi 2016-09-08 11:39 ` Arnd Bergmann 2016-09-08 11:39 ` Arnd Bergmann 2016-09-08 11:52 ` Felipe Balbi 2016-09-08 11:52 ` Felipe Balbi 2016-09-08 12:46 ` Arnd Bergmann 2016-09-08 12:46 ` Arnd Bergmann 2016-09-08 12:02 ` Grygorii Strashko 2016-09-08 12:02 ` Grygorii Strashko 2016-09-08 12:14 ` Arnd Bergmann 2016-09-08 12:14 ` Arnd Bergmann 2016-09-08 12:28 ` Peter Chen 2016-09-08 12:28 ` Peter Chen 2016-09-08 12:52 ` Arnd Bergmann 2016-09-08 12:52 ` Arnd Bergmann 2016-09-09 1:37 ` Peter Chen 2016-09-09 1:37 ` Peter Chen 2016-09-08 12:59 ` Grygorii Strashko 2016-09-08 12:59 ` Grygorii Strashko 2016-09-09 1:52 ` Peter Chen 2016-09-09 1:52 ` Peter Chen 2016-09-21 11:06 ` Sriram Dash 2016-09-21 11:06 ` Sriram Dash 2016-09-21 11:31 ` Arnd Bergmann 2016-09-21 11:31 ` Arnd Bergmann 2016-09-21 11:43 ` Sriram Dash 2016-09-21 11:43 ` Sriram Dash 2016-09-21 12:48 ` Arnd Bergmann 2016-09-21 12:48 ` Arnd Bergmann 2016-09-22 5:02 ` Sriram Dash 2016-09-22 5:02 ` Sriram Dash 2016-10-07 22:46 ` Leo Li 2016-10-07 22:46 ` Leo Li 2016-09-21 17:14 ` [PATCH] usb: xhci: Fix the patch inherit dma configuration from kbuild test robot 2016-09-21 17:14 ` kbuild test robot 2016-04-27 20:57 ` [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Felipe Balbi 2016-04-27 20:57 ` Felipe Balbi 2016-04-27 14:14 ` Grygorii Strashko 2016-04-27 14:14 ` Grygorii Strashko 2016-05-05 17:07 ` Brian Norris 2016-05-05 17:07 ` Brian Norris
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87y47ykzmz.fsf@intel.com \ --to=balbi@kernel.org \ --cc=arnd@arndb.de \ --cc=catalin.marinas@arm.com \ --cc=david.fisher1@synopsys.com \ --cc=gregkh@linuxfoundation.org \ --cc=grygorii.strashko@ti.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=nsekhar@ti.com \ --cc=stern@rowland.harvard.edu \ --cc=tqnguyen@apm.com \ --cc=yoshihiro.shimoda.uh@renesas.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.