From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757937AbcIHIDv (ORCPT ); Thu, 8 Sep 2016 04:03:51 -0400 Received: from mga09.intel.com ([134.134.136.24]:45881 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757270AbcIHIDt (ORCPT ); Thu, 8 Sep 2016 04:03:49 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,298,1470726000"; d="asc'?scan'208";a="6017094" 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: <3189648.KnWLgq0lTY@wuerfel> References: <12021424.cItk3A7CfE@wuerfel> <87inu8kny0.fsf@linux.intel.com> <3189648.KnWLgq0lTY@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 11:03:10 +0300 Message-ID: <8737lakedd.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: >> Arnd Bergmann writes: >>=20 >> [...] >>=20 >> > Regarding the DMA configuration that you mention in ci_hdrc_add_device= (), >> > I think we should replace=20 >> > >> > pdev->dev.dma_mask =3D dev->dma_mask; >> > pdev->dev.dma_parms =3D dev->dma_parms; >> > dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); >> > >> > with of_dma_configure(), which has the chance to configure more than >> > just those three, as the dma API might look into different aspects: >> > >> > - iommu specific configuration >> > - cache coherency information >> > - bus type >> > - dma offset >> > - dma_map_ops pointer >> > >> > We try to handle everything in of_dma_configure() at configuration >> > time, and that would be the place to add anything else that we might >> > need in the future. >>=20 >> There are a couple problems with this: >>=20 >> 1) won't work for PCI-based systems. >>=20 >> DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX >> platform (FPGA that appears like a PCI card to host PC) > > Right, I was specifically talking about the code in chipidea here, > which I think is never used on the PCI bus, and how the current just look at the history of the file, you'll see that an Intel employee was a maintainer of chipidea driver. Also: $ git ls-files drivers/usb/chipidea/ | grep pci drivers/usb/chipidea/ci_hdrc_pci.c > code is broken. We can probably do better than of_dma_configure() > (see below), but it would be an improvement. > >> 2) not very robust solution >>=20 >> of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat because >> that's not created by DT. The only reason why this works at all is >> because of the default 32-bit dma mask thing :-) So, how is it any >> different than copying 32-bit dma mask from parent? > > The idea here is that you pass in the parent of_node along with the child > device pointer, so it would behave exactly like the parent already does. > The difference is that it also handles all the other attributes besides t= he mask. Now we're talking :-) I like that. We just need a matching API for ACPI/PCI-based systems. > However, to summarize the discussion so far, I agree that > of_dma_configure() is not the solution to these problems, and I think > we can do much better: > > Splitting the usb_bus->controller field into the Linux-internal device > (used for the sysfs hierarchy, for printks and for power management) > and a new pointer (used for DMA, DT enumeration and phy lookup) probably > covers all that we really need. > > I've prototyped it below, with the dwc3, xhci and chipidea changes > together with the core changes. I've surely made mistakes there and > don't expect it to work out of the box, but this should give an > idea of how I think this can all be solved in the least invasive > way. > > I noticed that the gadget interface already has a way to handle the > DMA allocation by device, so I added that in as well. yeah, I wanna use that :-) > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 35d092456bec..08db66c64c66 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -25,6 +25,7 @@ > #include > #include > #include > +#include actually, we don't want the core to know what it's attached to. > #include > #include > #include > @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 = *dwc) > static void dwc3_free_one_event_buffer(struct dwc3 *dwc, > struct dwc3_event_buffer *evt) > { > - dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma); > + dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma); how about "dma_dev" instead? Is this used for anything other than DMA? > @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev) > dwc =3D PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1); > dwc->mem =3D mem; > dwc->dev =3D dev; > +#ifdef CONFIG_PCI > + /* TODO: or some other way of detecting this? */ > + if (dwc->dev->parent && dwc->dev->parent->bus =3D=3D &pci_bus_type) > + dwc->sysdev =3D dwc->dev->parent; > + else > +#endif > + dwc->sysdev =3D dwc->dev; Well, we can remove this ifdef and *always* use the parent. We will just require that dwc3 users provide a glue layer. In that case, your check becomes: if (dwc->dev->parent) dwc->sysdev =3D dwc->dev->parent; else dev_info(dwc->dev, "Please provide a glue layer!\n"); > diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exyno= s.c > index 2f1fb7e7aa54..e27899bb5706 100644 > --- a/drivers/usb/dwc3/dwc3-exynos.c > +++ b/drivers/usb/dwc3/dwc3-exynos.c > @@ -20,7 +20,6 @@ > #include > #include > #include > -#include > #include > #include > #include > @@ -117,15 +116,6 @@ static int dwc3_exynos_probe(struct platform_device = *pdev) > if (!exynos) > return -ENOMEM; >=20=20 > - /* > - * Right now device-tree probed devices don't get dma_mask set. > - * Since shared usb code relies on it, set it here for now. > - * Once we move to full device tree support this will vanish off. > - */ > - ret =3D dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); > - if (ret) > - return ret; this is a separate patch, right? > diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c > index 89a2f712fdfe..4d7439cb8cd8 100644 > --- a/drivers/usb/dwc3/dwc3-st.c > +++ b/drivers/usb/dwc3/dwc3-st.c > @@ -218,7 +218,6 @@ static int st_dwc3_probe(struct platform_device *pdev) > if (IS_ERR(regmap)) > return PTR_ERR(regmap); >=20=20 > - dma_set_coherent_mask(dev, dev->coherent_dma_mask); so is this. All in all, I like where you're going with this, we just need a matching acpi_dma_configure() and problems will be sorted out. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJX0Rs+AAoJEMy+uJnhGpkGuLoP/2H7KfoRIzGeo09D6xncX+c1 Qew+LUVchAobBalCuZjeFYKhfpRofsNcP+TXHuxyim+wn2sJ6u4jwHX12vI/7TOG jVVq6uf1pyA1srqUezJ6uYSJlwDyfS/eYqGUhS2H1/OWtTZ7vLDTRBRyzHT+0Iww 8E7Enwqr0SJ2UJ3MH0bJnA0sUzdJ+1AARLFEzLk/fshmFVvF7jH5zmTSLPvXEWnf FTMUnrzMX+wRQrEXlVa42hRLYWUrKaNqdkw6lvbiYAGM8e97/eijbwKnuiutqhbB Urwog+M0kW4CrC4y4v1mRGu5YQojYELoobikgHQR27Z26TNcVQHdeIDadT7pxGDC 58fif/tJIjqPedM8+1h48JdxZkigPJ8PHbC5SzqTatBADgNdZSg1ys779f47y0HI ezAb+MLskUx4eti5/cIwfie9Hz8LXRqOrKr5XV23on06JIom+aSR71jAgfxASMFx 9sojkl748KeyOUhpCdecNi8QoDaPtHoFcafarAndY2raqObUDOMA8D3DUxz8M2T7 EDtGEUJt3Fe8u7T9zwvUdiPXChhQDtcEY9ICIJisrMZM3TlPbID5f9vKvpJgcN53 7D7azhB+Aa9o2L3FDyQgCB1QErwZm4u60XFnCdD+QWtybcQc0wYzbHlXNfkmOud7 k+I7looUhPswJhZJni1M =WM7N -----END PGP SIGNATURE----- --=-=-=-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: balbi@kernel.org (Felipe Balbi) Date: Thu, 08 Sep 2016 11:03:10 +0300 Subject: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev In-Reply-To: <3189648.KnWLgq0lTY@wuerfel> References: <12021424.cItk3A7CfE@wuerfel> <87inu8kny0.fsf@linux.intel.com> <3189648.KnWLgq0lTY@wuerfel> Message-ID: <8737lakedd.fsf@linux.intel.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi, Arnd Bergmann writes: >> Arnd Bergmann writes: >> >> [...] >> >> > Regarding the DMA configuration that you mention in ci_hdrc_add_device(), >> > I think we should replace >> > >> > pdev->dev.dma_mask = dev->dma_mask; >> > pdev->dev.dma_parms = dev->dma_parms; >> > dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); >> > >> > with of_dma_configure(), which has the chance to configure more than >> > just those three, as the dma API might look into different aspects: >> > >> > - iommu specific configuration >> > - cache coherency information >> > - bus type >> > - dma offset >> > - dma_map_ops pointer >> > >> > We try to handle everything in of_dma_configure() at configuration >> > time, and that would be the place to add anything else that we might >> > need in the future. >> >> There are a couple problems with this: >> >> 1) won't work for PCI-based systems. >> >> DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX >> platform (FPGA that appears like a PCI card to host PC) > > Right, I was specifically talking about the code in chipidea here, > which I think is never used on the PCI bus, and how the current just look at the history of the file, you'll see that an Intel employee was a maintainer of chipidea driver. Also: $ git ls-files drivers/usb/chipidea/ | grep pci drivers/usb/chipidea/ci_hdrc_pci.c > code is broken. We can probably do better than of_dma_configure() > (see below), but it would be an improvement. > >> 2) not very robust solution >> >> of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat because >> that's not created by DT. The only reason why this works at all is >> because of the default 32-bit dma mask thing :-) So, how is it any >> different than copying 32-bit dma mask from parent? > > The idea here is that you pass in the parent of_node along with the child > device pointer, so it would behave exactly like the parent already does. > The difference is that it also handles all the other attributes besides the mask. Now we're talking :-) I like that. We just need a matching API for ACPI/PCI-based systems. > However, to summarize the discussion so far, I agree that > of_dma_configure() is not the solution to these problems, and I think > we can do much better: > > Splitting the usb_bus->controller field into the Linux-internal device > (used for the sysfs hierarchy, for printks and for power management) > and a new pointer (used for DMA, DT enumeration and phy lookup) probably > covers all that we really need. > > I've prototyped it below, with the dwc3, xhci and chipidea changes > together with the core changes. I've surely made mistakes there and > don't expect it to work out of the box, but this should give an > idea of how I think this can all be solved in the least invasive > way. > > I noticed that the gadget interface already has a way to handle the > DMA allocation by device, so I added that in as well. yeah, I wanna use that :-) > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 35d092456bec..08db66c64c66 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -25,6 +25,7 @@ > #include > #include > #include > +#include actually, we don't want the core to know what it's attached to. > #include > #include > #include > @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc) > static void dwc3_free_one_event_buffer(struct dwc3 *dwc, > struct dwc3_event_buffer *evt) > { > - dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma); > + dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma); how about "dma_dev" instead? Is this used for anything other than DMA? > @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev) > dwc = PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1); > dwc->mem = mem; > dwc->dev = dev; > +#ifdef CONFIG_PCI > + /* TODO: or some other way of detecting this? */ > + if (dwc->dev->parent && dwc->dev->parent->bus == &pci_bus_type) > + dwc->sysdev = dwc->dev->parent; > + else > +#endif > + dwc->sysdev = dwc->dev; Well, we can remove this ifdef and *always* use the parent. We will just require that dwc3 users provide a glue layer. In that case, your check becomes: if (dwc->dev->parent) dwc->sysdev = dwc->dev->parent; else dev_info(dwc->dev, "Please provide a glue layer!\n"); > diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c > index 2f1fb7e7aa54..e27899bb5706 100644 > --- a/drivers/usb/dwc3/dwc3-exynos.c > +++ b/drivers/usb/dwc3/dwc3-exynos.c > @@ -20,7 +20,6 @@ > #include > #include > #include > -#include > #include > #include > #include > @@ -117,15 +116,6 @@ static int dwc3_exynos_probe(struct platform_device *pdev) > if (!exynos) > return -ENOMEM; > > - /* > - * Right now device-tree probed devices don't get dma_mask set. > - * Since shared usb code relies on it, set it here for now. > - * Once we move to full device tree support this will vanish off. > - */ > - ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32)); > - if (ret) > - return ret; this is a separate patch, right? > diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c > index 89a2f712fdfe..4d7439cb8cd8 100644 > --- a/drivers/usb/dwc3/dwc3-st.c > +++ b/drivers/usb/dwc3/dwc3-st.c > @@ -218,7 +218,6 @@ static int st_dwc3_probe(struct platform_device *pdev) > if (IS_ERR(regmap)) > return PTR_ERR(regmap); > > - dma_set_coherent_mask(dev, dev->coherent_dma_mask); so is this. All in all, I like where you're going with this, we just need a matching acpi_dma_configure() and problems will be sorted out. -- balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 800 bytes Desc: not available URL: