From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758096AbcIHLx1 (ORCPT ); Thu, 8 Sep 2016 07:53:27 -0400 Received: from mga05.intel.com ([192.55.52.43]:3069 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758006AbcIHLx0 (ORCPT ); Thu, 8 Sep 2016 07:53:26 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,300,1470726000"; d="asc'?scan'208";a="1037014651" From: Felipe Balbi To: Arnd Bergmann 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 In-Reply-To: <12394164.0mceYlJJBD@wuerfel> References: <3757738.rjzYG3yq8e@wuerfel> <871t0uiqn9.fsf@linux.intel.com> <12394164.0mceYlJJBD@wuerfel> User-Agent: Notmuch/0.22.1+63~g994277e (https://notmuchmail.org) Emacs/25.1.3 (x86_64-pc-linux-gnu) Date: Thu, 08 Sep 2016 14:52:46 +0300 Message-ID: <87r38uhalt.fsf@linux.intel.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-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Arnd Bergmann writes: >> >> 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: >> >>=20 >> >> 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 d= evice_node *np) >> >> * Set default coherent_dma_mask to 32 bit. Drivers are expe= cted to >> >> * setup the correct supported mask. >> >> */ >> >> - if (!dev->coherent_dma_mask) >> >> - dev->coherent_dma_mask =3D DMA_BIT_MASK(32); >> >> + if (!dev->coherent_dma_mask) { >> >> + if (!dev->parent->coherent_dma_mask) >> >> + dev->coherent_dma_mask =3D DMA_BIT_MASK(32); >> >> + else >> >> + dev->coherent_dma_mask =3D dev->parent->coher= ent_dma_mask; >> >> + } >> >>=20=20 >> > >> > As the comment above that code says, the default 32-bit mask is intent= ional, >> > 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 pare= nt >> > supports will not do the right thing elsewhere. >>=20 >> oh man, it gets more and more complex. Seems like either path we take >> will cause problems somewhere=20 >>=20 >> 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 > 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. >> 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 =3D dwc->dev->parent else dwc->sysdev =3D dwc->dev > 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. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJX0VEOAAoJEMy+uJnhGpkG5fUQAJnuZfmdaHWV3kNnRQaU3eZw aGwbDypAnjqkn2uhoWnwCHbjqUfSSM4hA4rJcMNxTraLH9re9Eai12p0XZmuXZaR rADRsZ2PiuIVMHAMSTPZk+zNQGLHyyI9ICISdTvb04oeonYs1sG3d3NQDYNbKdgq LlJlQBrnPLlFextosY+hsIH2TFOGETr0FEUOZBbdnbjUuJazhxnjgg0J23o1Z0Yd aV86R1W3o8LnYPRFDSLjCJxmX0NhcaHGiBc7EDEjte1YcPtdLvw3au3B1I/bnMM2 Y/e+8wdVg0Xk2pNvyvom/IeTDKbGF0QcQ0X03/7PuOL1QvB5faZP7w6Eunhv82AW SxX4fhQnvHOeFEHqB9Fk06iBAOIyNqoc8wf03dEHUQdf3Mjjv+n+2qfGmCL74LM3 CpoyNoCsqN7v9wI72ET+w+KQN8vUAScRJrLDcllGpKd1NDi88OhDORhggsB1Q7tM Srzz0a0IX49CAvwRpms0y65Yq5c4mkpINSS1LRGmkL220uVUT8A6tGfXkYcapNmc EEDUOR6Q4w8+BWsQYzwgm4OCsysNcm3DhTztAfsLqO5Il6dWKHvRNqjOenU7lZKe 4NDW28b8tdpxV/eJXyGf+kf2iiQsNTDadK+q16kJYx94tIjogBqUmIsQP5cbRe9V ZCWgAHyZjrZB4FUtT93c =6F1U -----END PGP SIGNATURE----- --=-=-=--