From: Arnd Bergmann <arnd@arndb.de> To: Felipe Balbi <balbi@kernel.org> 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:46:51 +0200 [thread overview] Message-ID: <7818609.ZAbFcr85D0@wuerfel> (raw) In-Reply-To: <87r38uhalt.fsf@linux.intel.com> On Thursday, September 8, 2016 2:52:46 PM CEST Felipe Balbi wrote: > Arnd Bergmann <arnd@arndb.de> writes: > >> 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. > > > > I don't see how we'd break the current DTs, I'm fairly sure we could turn dwc3 > > well, at a minimum dwc3-{pci,exynos,st,omap,of-simple}.c would have to > look at possible children for their own quirks and properties. > > > into a library without changing the DT representation. However the parts > > that I think would change are > > > > - The sysfs representation for dwc3-pci, as we would no longer have > > a parent-child relationship there. > > that's a no-brainer, I think > > > - The power management handling might need a rework, since you currently > > rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning > > power on and off > > simple enough to do as well. > > > - turning dwc3 into a library probably implies also turning xhci into > > a library, in part for consistency. > > yeah, I considered that too. We could still do it in parts, though. > > > - if we don't do the whole usb_bus->sysdev thing, we need to not just > > do this for dwc3 but also chipidea and maybe a couple of others. > > MUSB comes to mind Right. > > There should not be any show-stoppers here, but it's a lot of work. > > I think the biggest work will making sure people don't abuse functions > just because they're now part of a single binary. Having them as > separate modules helped a lot reducing the maintenance overhead. There > was only one occasion where someone sent a glue layer which iterated > over its children to find struct dwc3 * from child's drvdata. This is where it get a bit philosophical ;-) I understand that you like the strict separation that the current model provides, and I agree that can be an advantage. Changing the abstraction model to a set of library modules the way that other drivers (e.g. ehci, sdhci, or libata) work to me means changing this separation model into a different model and once we do that I would not consider it a mistake for the platform specific driver to take advantage of that. You still get a bit of separation since the drivers would be in separate modules that can only access exported symbols, and the library can still hide its data structures (to some degree). I still think that turning xhci (and dwc3) into a library would be an overall win, but if we solve the problems of DMA settings and usb_device DT properties without it, I'd prefer not to fight over that with you again ;-) > >> 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. > > > > I don't understand what you mean here, but I agree that the various ways > > well, we can't simply use what I pointed out a few emails back: > > if (dwc->dev->parent) > dwc->sysdev = dwc->dev->parent > else > dwc->sysdev = dwc->dev Ok, I see. > > we discussed for copying the DMA flags from one 'struct device' to another > > all turned out to be flawed in at least one way. > > > > Do you see any problems with the patch I posted other than the ugliness > > of the dwc3 and xhci drivers finding out which pointer to use for > > usb_bus->sysdev? If we can solve this, we shouldn't need any new > > of_dma_configure/acpi_dma_configure calls and we won't have to > > turn the drivers into a library, so maybe let's try to come up with > > better ideas for that sub-problem. > > No big problems with that, no. Just the ifdef looking for a PCI bus in > the parent. How about passing a flag via device_properties? I don't > wanna change dwc3 core's device name with a platform_device_id because > there probably already are scripts relying on the names to enable > pm_runtime for example. Sounds ok to me. Grygorii's solution might a be a bit more elegant, but also a bit more error-prone: If we get a platform that mistakenly sets the dma_mask pointer of the child device, or a platform that does not set the dma_mask pointer of the parent, things break. Arnd
WARNING: multiple messages have this Message-ID (diff)
From: arnd@arndb.de (Arnd Bergmann) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Date: Thu, 08 Sep 2016 14:46:51 +0200 [thread overview] Message-ID: <7818609.ZAbFcr85D0@wuerfel> (raw) In-Reply-To: <87r38uhalt.fsf@linux.intel.com> On Thursday, September 8, 2016 2:52:46 PM CEST Felipe Balbi wrote: > Arnd Bergmann <arnd@arndb.de> writes: > >> 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. > > > > I don't see how we'd break the current DTs, I'm fairly sure we could turn dwc3 > > well, at a minimum dwc3-{pci,exynos,st,omap,of-simple}.c would have to > look at possible children for their own quirks and properties. > > > into a library without changing the DT representation. However the parts > > that I think would change are > > > > - The sysfs representation for dwc3-pci, as we would no longer have > > a parent-child relationship there. > > that's a no-brainer, I think > > > - The power management handling might need a rework, since you currently > > rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning > > power on and off > > simple enough to do as well. > > > - turning dwc3 into a library probably implies also turning xhci into > > a library, in part for consistency. > > yeah, I considered that too. We could still do it in parts, though. > > > - if we don't do the whole usb_bus->sysdev thing, we need to not just > > do this for dwc3 but also chipidea and maybe a couple of others. > > MUSB comes to mind Right. > > There should not be any show-stoppers here, but it's a lot of work. > > I think the biggest work will making sure people don't abuse functions > just because they're now part of a single binary. Having them as > separate modules helped a lot reducing the maintenance overhead. There > was only one occasion where someone sent a glue layer which iterated > over its children to find struct dwc3 * from child's drvdata. This is where it get a bit philosophical ;-) I understand that you like the strict separation that the current model provides, and I agree that can be an advantage. Changing the abstraction model to a set of library modules the way that other drivers (e.g. ehci, sdhci, or libata) work to me means changing this separation model into a different model and once we do that I would not consider it a mistake for the platform specific driver to take advantage of that. You still get a bit of separation since the drivers would be in separate modules that can only access exported symbols, and the library can still hide its data structures (to some degree). I still think that turning xhci (and dwc3) into a library would be an overall win, but if we solve the problems of DMA settings and usb_device DT properties without it, I'd prefer not to fight over that with you again ;-) > >> 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. > > > > I don't understand what you mean here, but I agree that the various ways > > well, we can't simply use what I pointed out a few emails back: > > if (dwc->dev->parent) > dwc->sysdev = dwc->dev->parent > else > dwc->sysdev = dwc->dev Ok, I see. > > we discussed for copying the DMA flags from one 'struct device' to another > > all turned out to be flawed in at least one way. > > > > Do you see any problems with the patch I posted other than the ugliness > > of the dwc3 and xhci drivers finding out which pointer to use for > > usb_bus->sysdev? If we can solve this, we shouldn't need any new > > of_dma_configure/acpi_dma_configure calls and we won't have to > > turn the drivers into a library, so maybe let's try to come up with > > better ideas for that sub-problem. > > No big problems with that, no. Just the ifdef looking for a PCI bus in > the parent. How about passing a flag via device_properties? I don't > wanna change dwc3 core's device name with a platform_device_id because > there probably already are scripts relying on the names to enable > pm_runtime for example. Sounds ok to me. Grygorii's solution might a be a bit more elegant, but also a bit more error-prone: If we get a platform that mistakenly sets the dma_mask pointer of the child device, or a platform that does not set the dma_mask pointer of the parent, things break. Arnd
next prev parent reply other threads:[~2016-09-08 12:47 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 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 [this message] 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=7818609.ZAbFcr85D0@wuerfel \ --to=arnd@arndb.de \ --cc=balbi@kernel.org \ --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.