From: Felipe Balbi <balbi@kernel.org> To: Arnd Bergmann <arnd@arndb.de> Cc: Peter Chen <hzpeterchen@gmail.com>, Leo Li <pku.leo@gmail.com>, Grygorii Strashko <grygorii.strashko@ti.com>, Russell King - ARM Linux <linux@arm.linux.org.uk>, Catalin Marinas <catalin.marinas@arm.com>, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>, "linux-usb\@vger.kernel.org" <linux-usb@vger.kernel.org>, Sekhar Nori <nsekhar@ti.com>, lkml <linux-kernel@vger.kernel.org>, Stuart Yoder <stuart.yoder@nxp.com>, Scott Wood <oss@buserror.net>, David Fisher <david.fisher1@synopsys.com>, "Thang Q. Nguyen" <tqnguyen@apm.com>, Alan Stern <stern@rowland.harvard.edu>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "linux-arm-kernel\@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Date: Thu, 08 Sep 2016 14:20:58 +0300 [thread overview] Message-ID: <871t0uiqn9.fsf@linux.intel.com> (raw) In-Reply-To: <3757738.rjzYG3yq8e@wuerfel> [-- Attachment #1: Type: text/plain, Size: 5563 bytes --] Hi, Arnd Bergmann <arnd@arndb.de> writes: > On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote: >> Arnd Bergmann <arnd@arndb.de> writes: >> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: >> >> Arnd Bergmann <arnd@arndb.de> writes: >> >> > That sounds a bit clumsy for the sake of consistency with PCI. >> >> > The advantage is that xhci can always use the grandparent device >> >> > as sysdev whenever it isn't probed through PCI or firmware >> >> > itself, but the purpose of the dwc3-glue is otherwise questionable. >> >> > >> >> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3 >> >> > device when that is created from the PCI driver and checking for that >> >> > with the device property interface instead? If it's "snps,dwc3" >> >> > we use the device itself while for "snps,dwc3-pci", we use the parent? >> >> >> >> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev? >> > >> > That would be incompatible with the USB binding, as the sysdev >> > is assumed to be a USB host controller with #address-cells=<1> >> > and #size-cells=<0> in order to hold the child devices, for >> > example: >> > >> > / { >> > omap_dwc3_1: omap_dwc3_1@48880000 { >> > compatible = "ti,dwc3"; >> > #address-cells = <1>; >> > #size-cells = <1>; >> > ranges; >> > usb1: usb@48890000 { >> > compatible = "snps,dwc3"; >> > reg = <0x48890000 0x17000>; >> > #address-cells = <1>; >> > #size-cells = <0>; >> > interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>, >> > <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>, >> > <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>; >> > interrupt-names = "peripheral", >> > "host", >> > "otg"; >> > phys = <&usb2_phy1>, <&usb3_phy1>; >> > phy-names = "usb2-phy", "usb3-phy"; >> > >> > hub@1 { >> > compatible = "usb5e3,608"; >> > reg = <1>; >> > #address-cells = <1>; >> > #size-cells = <0>; >> > >> > ethernet@1 { >> > compatible = "usb424,ec00"; >> > mac-address = [00 11 22 33 44 55]; >> > reg = <1>; >> > }; >> > }; >> > }; >> > }; >> > >> > It's also the node that contains the "phys" properties and >> > presumably other properties like "otg-rev", "maximum-speed" >> > etc. >> > >> > If we make the sysdev point to the parent, then we can no longer >> > look up those properties and child devices from the USB core code >> > by looking at "sysdev->of_node". >> >> this also makes things more interesting. I can't of anything other than >> having some type of flag passed via e.g. device_properties by dwc3-pci.c >> :-s > > Ok. man, I have been skipping words rather frequently when typing lately. I meant "I can't THINK of anything other ...." >> It's quite a hack, though. I still think that inheriting DMA (or >> manually initializing a child with parent's DMA bits and pieces) is the >> best way to go. So we're back to of_dma_configure() and >> acpi_dma_configure(), right? > > That won't solve the problems with the DT properties or the > dma configuration for PCI devices though. acpi_dma_configure() is supposed to pass along DMA bits from PCI to child devices, no? >> But this needs to be done before dwc3_probe() executes. For dwc3-pci >> that's easy, but for DT devices, seems like it should be in of >> core. Below is, clearly, not enough but should show the idea: >> >> diff --git a/drivers/of/device.c b/drivers/of/device.c >> index fd5cfad7c403..a54610198946 100644 >> --- a/drivers/of/device.c >> +++ b/drivers/of/device.c >> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct device_node *np) >> * Set default coherent_dma_mask to 32 bit. Drivers are expected to >> * setup the correct supported mask. >> */ >> - if (!dev->coherent_dma_mask) >> - dev->coherent_dma_mask = DMA_BIT_MASK(32); >> + if (!dev->coherent_dma_mask) { >> + if (!dev->parent->coherent_dma_mask) >> + dev->coherent_dma_mask = DMA_BIT_MASK(32); >> + else >> + dev->coherent_dma_mask = dev->parent->coherent_dma_mask; >> + } >> > > As the comment above that code says, the default 32-bit mask is intentional, > and you need the driver to ask for the mask it wants using > dma_set_mask_and_coherent(), while the platform code should be able to use > dev->of_node to figure out whether that mask is supported. > > Just setting the initial mask to something else based on what the parent > supports will not do the right thing elsewhere. oh man, it gets more and more complex. Seems like either path we take will cause problems somewhere :-s If we make dwc3.ko a library which glue calls directly then all these problems are solved but we break all current DTs and fall into the trap of having another MUSB. If we try to pass DMA bits from parent to child, then we have the fact that DT ends up, in practice, always having a parent device. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 800 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, 08 Sep 2016 14:20:58 +0300 [thread overview] Message-ID: <871t0uiqn9.fsf@linux.intel.com> (raw) In-Reply-To: <3757738.rjzYG3yq8e@wuerfel> Hi, Arnd Bergmann <arnd@arndb.de> writes: > On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote: >> Arnd Bergmann <arnd@arndb.de> writes: >> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote: >> >> Arnd Bergmann <arnd@arndb.de> writes: >> >> > That sounds a bit clumsy for the sake of consistency with PCI. >> >> > The advantage is that xhci can always use the grandparent device >> >> > as sysdev whenever it isn't probed through PCI or firmware >> >> > itself, but the purpose of the dwc3-glue is otherwise questionable. >> >> > >> >> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3 >> >> > device when that is created from the PCI driver and checking for that >> >> > with the device property interface instead? If it's "snps,dwc3" >> >> > we use the device itself while for "snps,dwc3-pci", we use the parent? >> >> >> >> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev? >> > >> > That would be incompatible with the USB binding, as the sysdev >> > is assumed to be a USB host controller with #address-cells=<1> >> > and #size-cells=<0> in order to hold the child devices, for >> > example: >> > >> > / { >> > omap_dwc3_1: omap_dwc3_1 at 48880000 { >> > compatible = "ti,dwc3"; >> > #address-cells = <1>; >> > #size-cells = <1>; >> > ranges; >> > usb1: usb at 48890000 { >> > compatible = "snps,dwc3"; >> > reg = <0x48890000 0x17000>; >> > #address-cells = <1>; >> > #size-cells = <0>; >> > interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>, >> > <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>, >> > <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>; >> > interrupt-names = "peripheral", >> > "host", >> > "otg"; >> > phys = <&usb2_phy1>, <&usb3_phy1>; >> > phy-names = "usb2-phy", "usb3-phy"; >> > >> > hub at 1 { >> > compatible = "usb5e3,608"; >> > reg = <1>; >> > #address-cells = <1>; >> > #size-cells = <0>; >> > >> > ethernet at 1 { >> > compatible = "usb424,ec00"; >> > mac-address = [00 11 22 33 44 55]; >> > reg = <1>; >> > }; >> > }; >> > }; >> > }; >> > >> > It's also the node that contains the "phys" properties and >> > presumably other properties like "otg-rev", "maximum-speed" >> > etc. >> > >> > If we make the sysdev point to the parent, then we can no longer >> > look up those properties and child devices from the USB core code >> > by looking at "sysdev->of_node". >> >> this also makes things more interesting. I can't of anything other than >> having some type of flag passed via e.g. device_properties by dwc3-pci.c >> :-s > > Ok. man, I have been skipping words rather frequently when typing lately. I meant "I can't THINK of anything other ...." >> It's quite a hack, though. I still think that inheriting DMA (or >> manually initializing a child with parent's DMA bits and pieces) is the >> best way to go. So we're back to of_dma_configure() and >> acpi_dma_configure(), right? > > That won't solve the problems with the DT properties or the > dma configuration for PCI devices though. acpi_dma_configure() is supposed to pass along DMA bits from PCI to child devices, no? >> But this needs to be done before dwc3_probe() executes. For dwc3-pci >> that's easy, but for DT devices, seems like it should be in of >> core. Below is, clearly, not enough but should show the idea: >> >> diff --git a/drivers/of/device.c b/drivers/of/device.c >> index fd5cfad7c403..a54610198946 100644 >> --- a/drivers/of/device.c >> +++ b/drivers/of/device.c >> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct device_node *np) >> * Set default coherent_dma_mask to 32 bit. Drivers are expected to >> * setup the correct supported mask. >> */ >> - if (!dev->coherent_dma_mask) >> - dev->coherent_dma_mask = DMA_BIT_MASK(32); >> + if (!dev->coherent_dma_mask) { >> + if (!dev->parent->coherent_dma_mask) >> + dev->coherent_dma_mask = DMA_BIT_MASK(32); >> + else >> + dev->coherent_dma_mask = dev->parent->coherent_dma_mask; >> + } >> > > As the comment above that code says, the default 32-bit mask is intentional, > and you need the driver to ask for the mask it wants using > dma_set_mask_and_coherent(), while the platform code should be able to use > dev->of_node to figure out whether that mask is supported. > > Just setting the initial mask to something else based on what the parent > supports will not do the right thing elsewhere. oh man, it gets more and more complex. Seems like either path we take will cause problems somewhere :-s If we make dwc3.ko a library which glue calls directly then all these problems are solved but we break all current DTs and fall into the trap of having another MUSB. If we try to pass DMA bits from parent to child, then we have the fact that DT ends up, in practice, always having a parent device. -- balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 800 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160908/1babda94/attachment.sig>
next prev parent reply other threads:[~2016-09-08 11:21 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 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 [this message] 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=871t0uiqn9.fsf@linux.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=hzpeterchen@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=nsekhar@ti.com \ --cc=oss@buserror.net \ --cc=pku.leo@gmail.com \ --cc=stern@rowland.harvard.edu \ --cc=stuart.yoder@nxp.com \ --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.