From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965393AbcIHMrO (ORCPT ); Thu, 8 Sep 2016 08:47:14 -0400 Received: from mout.kundenserver.de ([212.227.126.134]:62733 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932478AbcIHMrM (ORCPT ); Thu, 8 Sep 2016 08:47:12 -0400 From: Arnd Bergmann To: Felipe Balbi Cc: Peter Chen , Leo Li , Grygorii Strashko , Russell King - ARM Linux , Catalin Marinas , Yoshihiro Shimoda , "linux-usb@vger.kernel.org" , Sekhar Nori , lkml , Stuart Yoder , Scott Wood , David Fisher , "Thang Q. Nguyen" , Alan Stern , Greg Kroah-Hartman , "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 Message-ID: <7818609.ZAbFcr85D0@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <87r38uhalt.fsf@linux.intel.com> References: <12394164.0mceYlJJBD@wuerfel> <87r38uhalt.fsf@linux.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:/SI5e1TLfFsO7GFkv3m1C8jkcy6Z42TKJzw/ZE0IZB5Nl8O8uhC kT1Z1WnHOH95KE2z+ijQGCMZaISsNCruxPyCxJ901sfbt9z88R79M/VgQ5r4l6KoGPLw+b/ Wb75HsBBFdl5jcBaZZBfwYsO8/x2sG2BSqcVjxOn54lUvtO1tLktT6gNpMb12y3/k+Bk3T1 OcOgA3ySdT/qY1n4dYJ3g== X-UI-Out-Filterresults: notjunk:1;V01:K0:y6IoSH0D1cs=:oAdLtMNaxPcJaY/UmK0e1w 9ErYevkc4yXC5pr/wuKBtHRSH++5D0S2LTyqxxm4Gfy+Md8dFRx+Qzb+wajLNs4ccqqXybiXs kfnReAvsH3KngAG82aoB1lPVrvkzYtRJAVnmsw1lD9ZHE88W6nelGnmc2agzaEMb9cqa425FJ 4cGLXx30np34dJOjRG1KRCfenzkTeh1OzoeuL/2LiYkEOTt4PrDJEVrAM9303XnuU9jknQhd7 SXKxrnm57CY2b1XbXKmHDehJfuR5wdPIPBMs4mtDd30ePLRlv0Iws37Mwj4ih8F3QPuszhWlz e7P5QzorV6cCjGeTsVfySkL+5bgz9gzKER58Ag0n2QBhioUcV3XiDoTfe3YsfDGurAtCVMrre xMYY2XQKRMQuI6GWIbWDtsQLncEogZzvdtMqAXtQRsrDjj8tF2jZWEJj/V2hjwUEMMJGkAtfi 9HsXRY6jmpBUKawzVxKpswoKdtdA8f4lNZavvWk38+9aoip3JTQfmg+VRu2OpIz/F3KO6QKTE eAjdLItFucpxXWS/tB1uI+op9cDCYO2UA837F28bZ5ffAWPmVhDinsA6/ZY/9p75eqH5N4+Ff 0NHldCeJQaS32iWwjGOB5oTKHrbSfL0savK4dWcFZ77nXJh9Rcm71r8NuDoNOaT6ypMKs9lc5 i4n+Ga5JIC7WKWT/gw1wXioqgchsedYGSyNz07zEe+sQHeeeDMSBfrWR7P77u9dlv6jlKonQi 1aLT5yPW4pzTQ50w Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thursday, September 8, 2016 2:52:46 PM CEST Felipe Balbi wrote: > Arnd Bergmann 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Thu, 08 Sep 2016 14:46:51 +0200 Subject: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev In-Reply-To: <87r38uhalt.fsf@linux.intel.com> References: <12394164.0mceYlJJBD@wuerfel> <87r38uhalt.fsf@linux.intel.com> Message-ID: <7818609.ZAbFcr85D0@wuerfel> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thursday, September 8, 2016 2:52:46 PM CEST Felipe Balbi wrote: > Arnd Bergmann 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