Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753511AbcD0U51 (ORCPT ); Wed, 27 Apr 2016 16:57:27 -0400 Received: from mail-lf0-f65.google.com ([209.85.215.65]:33001 "EHLO mail-lf0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752323AbcD0U5Z (ORCPT ); Wed, 27 Apr 2016 16:57:25 -0400 From: Felipe Balbi To: Arnd Bergmann , linux-arm-kernel@lists.infradead.org Cc: Grygorii Strashko , Catalin Marinas , Yoshihiro Shimoda , linux-usb@vger.kernel.org, Sekhar Nori , linux-kernel@vger.kernel.org, David Fisher , "Thang Q. Nguyen" , Greg Kroah-Hartman Subject: Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev In-Reply-To: <6148633.0YXW4XnX4b@wuerfel> References: <1461612094-30939-1-git-send-email-grygorii.strashko@ti.com> <4985498.GT33RtioOA@wuerfel> <877ffjro0w.fsf@ti.com> <6148633.0YXW4XnX4b@wuerfel> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/25.0.92.2 (x86_64-pc-linux-gnu) Date: Wed, 27 Apr 2016 23:57:17 +0300 Message-ID: <87zisercr6.fsf@ti.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6948 Lines: 184 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Arnd Bergmann writes: > On Wednesday 27 April 2016 19:53:51 Felipe Balbi wrote: >> Arnd Bergmann writes: >> > On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote: >> >> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote: >> >> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote: >> >> > >=20 >> >> > > I would be in favour of a dma_inherit() function as well. We coul= d hack >> >> > > something up in the arch code (like below) but I would rather pre= fer an >> >> > > explicit dma_inherit() call by drivers creating such devices. >> >> > >=20 >> >> > > diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/in= clude/asm/dma-mapping.h >> >> > > index ba437f090a74..ea6fb9b0e8fa 100644 >> >> > > --- a/arch/arm64/include/asm/dma-mapping.h >> >> > > +++ b/arch/arm64/include/asm/dma-mapping.h >> >> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops; >> >> > >=20=20 >> >> > > static inline struct dma_map_ops *__generic_dma_ops(struct devic= e *dev) >> >> > > { >> >> > > - if (dev && dev->archdata.dma_ops) >> >> > > - return dev->archdata.dma_ops; >> >> > > + while (dev) { >> >> > > + if (dev->archdata.dma_ops) >> >> > > + return dev->archdata.dma_ops; >> >> > > + dev =3D dev->parent; >> >> > > + } >> >> >=20 >> >> > I think this would be a very bad idea: we don't want to have random >> >> > devices be able to perform DMA just because their parent devices >> >> > have been set up that way. >> >>=20 >> >> I agree, it's a big hack. It would be nice to have a simpler way to do >> >> this in driver code rather than explicitly calling >> >> of_dma_configure/arch_setup_dma_ops as per the original patch in this >> >> thread. >> > >> > I haven't followed the entire discussion, but what's wrong with passing >> > around a pointer to a 'struct device *hwdev' that represents the physi= cal >> > device that does the DMA? >>=20 >> that will likely create duplicated solutions in several drivers and >> it'll be a pain to maintain. There's another complication, dwc3 can be >> integrated in many different ways. See the device child-parent tree >> representations below: >>=20 >> a) with a parent PCI device: >>=20 >> pci_bus_type >> - dwc3-pci >> - dwc3 >> - xhci-plat >>=20 >> b) with a parent platform_device (OF): >>=20 >> platform_bus_type >> - dwc3-${omap,st,of-simple,exynos,keystone} >> - dwc3 >> - xhci-plat >>=20 >> c) without a parent at all (thanks Grygorii): >>=20 >> platform_bus_type >> - dwc3 >> - xhci-plat >>=20 >> (a) and (b) above are the common cases. The DMA-capable device is >> clearly dwc3-${pci,omap,st,of-simple,exynos,keystone} with dwc3 only >> having proper DMA configuration in OF platforms (because of the >> unconditional of_dma_configure() during OF device creation) and >> xhci-plat not knowing about DMA at all and hardcoding some crappy >> defaults. >>=20 >> (c) is the uncommon case which creates some problems. In this case, dwc3 >> itself is the DMA-capable device and dwc3->dev->parent is the >> platform_bus_type itself. Now consider the problem this creates: >>=20 >> i. the patch that I wrote [1] becomes invalid for (c), thanks to >> Grygorii for pointing this out before it was too late. >>=20 >> ii. xhci-plat can also be described directly in DT (and is in some >> cases). This means that assuming xhci-plat's parent's parent to be the >> DMA-capable device is also an invalid assumption. >>=20 >> iii. one might argue that for DT-based platforms *with* a glue layer >> ((b) above), OF already "copies" some sensible DMA defaults during >> device creation. > > But that is exactly the point I was trying to make: instead of copying > all the data into the xhci-plat device, just assign one pointer > inside the xhci-plat device from whoever creates it. and how do you pass that pointer to xhci-plat from another driver ? No, platform_data is not an option ;-) >> The only clean way to fix this up is with a dma_inherit()-like API which >> would allow dwc3's glue layers ((a) and (b) above) to initialize child's >> (dwc3) DMA configuration during child's creation. Something like below: >>=20 >> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c >> index adc1e8a624cb..74b599269e2c 100644 >> --- a/drivers/usb/dwc3/dwc3-pci.c >> +++ b/drivers/usb/dwc3/dwc3-pci.c >> @@ -152,6 +152,8 @@ static int dwc3_pci_probe(struct pci_dev *pci, >> return -ENOMEM; >> } >>=20=20 >> + dma_inherit(&dwc->dev, dev); >> + >> memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res)); >>=20=20 >> res[0].start =3D pci_resource_start(pci, 0); >>=20 >> that's all I'm asking for :-) dma_inherit() should, probably, be >> arch-specific to handle details like IOMMU (which today sits under >> archdata). > > It's still wrong: the archdata in the device can contain all sorts of > additional information about how to do DMA, and some of that information yes, inherit all of that ;-) > is bus specific, e.g. when your dma_map_ops look like the on sparc: okay, let me be clear. The underlying bus is still pci_bus_type or platform_bus_type; that won't change. > static inline struct dma_map_ops *get_dma_ops(struct device *dev) > { > #ifdef CONFIG_SPARC_LEON > if (sparc_cpu_model =3D=3D sparc_leon) > return leon_dma_ops; > #endif > #if defined(CONFIG_SPARC32) && defined(CONFIG_PCI) > if (dev->bus =3D=3D &pci_bus_type) > return &pci32_dma_ops; > #endif > return dma_ops; > } this seems to be a rather fragile assumption, no ? Only works because *_bus_type declarations are exported. I wonder if this is the only reason *why* they are exported, probably not... > There is no way for a device to "inherit" the bus_type of its parent, I don't want to inherit bus_type, I just want DMA configuration to not depend on bus_type directly ;-) =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXISetAAoJEIaOsuA1yqRERo4P/2AVE1l8NfFuMoJKUNF8xspD 8Uhr+FvUUwqRycu3MLq8XXAifPl9QnoHFzx0inoqC3wQ1UTrorF56czlMe81xwVn oadbpvIfUZv4RlO7U50AOjI8z/whak+Q8dHWKVtBHpq329vAoIxPk1SthUytzkLV +fw+EpsJvNK9A1JQr80eg5x2caicU8pf0LpZNzujXldk3JEhZbW6yeOcf7R4bgT9 G2Te6EQdFHk6dzcmnRjecxzzRbLE84ODSe1teIkQsfb+sjt2U404zo6gmXHsQtr3 rno1t1j9xK5qeQ38MKNCzJwzx+MUtB8Y96HD5+6NNUBxPVGWPPAwQFRJTnAq9QAx JtlKcrywWZlt34cypnE8joZhCa9TfAcO6KtTXSXZjzqtBSE7E+c9hlaRnWsVacUM n+vLpjMo7kLaShc0wqyFhCt9n1nm1TncgWz7JRXAPbPJpTZnMwIZ2DAa9Vl4aLQl 3CsVlSlXELMOEJ+5aybLyrYz/qnENDfrpjrVVb4u5qGxlQZU5Xlqc+H650Zt311H qnUjCxv/Ozt52hkaP86NWRTYOoqWUMPLW1RXeGKFRPfx32ztnohoggqasIehcK4x /h9hSrMg0YHJabUADpzZY61+KbUftx7r80tHKGpI7JlStBFOWoU4PLlhb5QLycuZ uQvPtlVfeb2hD/JuaiQg =h8TB -----END PGP SIGNATURE----- --=-=-=--