From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934016AbcIHI0Y (ORCPT ); Thu, 8 Sep 2016 04:26:24 -0400 Received: from mout.kundenserver.de ([212.227.17.13]:57168 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932881AbcIHI0V (ORCPT ); Thu, 8 Sep 2016 04:26:21 -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 10:26:05 +0200 Message-ID: <6739240.VDAnDBppH0@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <8737lakedd.fsf@linux.intel.com> References: <3189648.KnWLgq0lTY@wuerfel> <8737lakedd.fsf@linux.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:y9mxvHb2xyeFIzk/8ohOQOlzDtsqGkHQJb11EmtSdwleGEsU7NH ihuV7TA2mJFkaGwsb3XJOnTzTKcFNzhuMc3yb0C5CtoCSqAuS+/3c7/gPoX1YpWE8PQ2Ane BC7zebI//+qij8BVcyqC2Kf0z9wJFkty2OCfjKaPXNyyPJdVZJB+AhxGGjJinT7Z18ztGoh 81NoAV9tpixjkB9XpnYdA== X-UI-Out-Filterresults: notjunk:1;V01:K0:jE3DPkHkm/o=:IcQhxLNp5r4PeU7MTLGJe0 jN2jbF9ZMOOJXKVemNTEtcC43EmKX5Aj5qTPFWfrzoig7oZSsodTh0LrchA6KCyYhn8gVWgfD whRMKMEPfFltJQIajoWfpkNbV6/MBGRQRKb4MxHyb7NQKlzjfx2otN2kjAnGP+uC2g4lZ7n2n qFLiey/7kuIzt2J7TasQhWkD76TMXBj9UbBHT1O4WiXz8cfXbb8olTnXFlXt8/Q+UWOjZR/Wq zlruMgb+7OqAnXNipKnBeKj9CkXwjVPHRrcJsW4cAqIRKApRZGroUZ3pSfnGUNCv5cmS+DBh3 nti/Xv6ngdUZyRfPPpQEaVtV7sxzhBCgQoMuM2sVdgUOg115f7LN0Ewso2c4txw7Vf3ljBCyM 40gNImlrc4EfC8cLYgnoVBJcexBuwt51/b6kf/mQEeFY9/N6y9gpSyB6GZEMAx8UmoLX5MUIv VDCvvSBMT1seuWJiF1qWtqIg+FK8wXprw0oyshlDNjNIMXQFyZrtX/tu1E8FwunxQNEJWawzg Y/osx4faiw/BwuudhzHFbDmVYpbuilz+nAK7OTNLQJrz0DprtbsM0AVpfmhKW9FVbplCX5Q5k 4kCmQTS92zul89dAyeAvNTqwISCXfVBzp3oJvcIRsjD3H+KMnpkXlSinHI5db1WkUYkshPMza bc4zckmbM8ULXlWuxqM+RjFHd9KVAy0uEe21dS0d/0y67LkrRnx278qF9mksEk+IsY4kH3uao Ro/RZ3bHOZ7ICy/L Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thursday, September 8, 2016 11:03:10 AM CEST Felipe Balbi wrote: > Arnd Bergmann writes: > >> Arnd Bergmann writes: > 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 Right, Peter pointed that one out too. > > 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. Agreed. This was just a first draft and I couldn't come up with a better way to detect the case in which the parent device is probed from another HW bus and the child is not known to the firmware. > > #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? The two other things we have discussed in this thread are: - connecting of_node pointers to usb_device structures for children of sysdev->of_node. Note that this can happen even for PCI devices in case you have a USB ethernet device hardwired to a PCI-USB bridge and put the mac address in DT. - finding the PHY device for a HCD There might be others. Basically sysdev here is what the USB core code can use for looking up any kind of properties provided by the firmware. > > @@ -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"); If we do that, we have to put child devices of the dwc3 devices into the platform glue, and it also breaks those dwc3 devices that don't have a parent driver. > > 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? Yes, this is probably just a cleanup that we can apply regardless. We have not needed this in a long time. > > 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. With this patch, I don't think we even need that any more, as the device that we use the dma-mapping API is the one that already gets configured correctly by the platform code for all cases: PCI, OF, ACPI and combinations of those. Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Thu, 08 Sep 2016 10:26:05 +0200 Subject: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev In-Reply-To: <8737lakedd.fsf@linux.intel.com> References: <3189648.KnWLgq0lTY@wuerfel> <8737lakedd.fsf@linux.intel.com> Message-ID: <6739240.VDAnDBppH0@wuerfel> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thursday, September 8, 2016 11:03:10 AM CEST Felipe Balbi wrote: > Arnd Bergmann writes: > >> Arnd Bergmann writes: > 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 Right, Peter pointed that one out too. > > 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. Agreed. This was just a first draft and I couldn't come up with a better way to detect the case in which the parent device is probed from another HW bus and the child is not known to the firmware. > > #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? The two other things we have discussed in this thread are: - connecting of_node pointers to usb_device structures for children of sysdev->of_node. Note that this can happen even for PCI devices in case you have a USB ethernet device hardwired to a PCI-USB bridge and put the mac address in DT. - finding the PHY device for a HCD There might be others. Basically sysdev here is what the USB core code can use for looking up any kind of properties provided by the firmware. > > @@ -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"); If we do that, we have to put child devices of the dwc3 devices into the platform glue, and it also breaks those dwc3 devices that don't have a parent driver. > > 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? Yes, this is probably just a cleanup that we can apply regardless. We have not needed this in a long time. > > 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. With this patch, I don't think we even need that any more, as the device that we use the dma-mapping API is the one that already gets configured correctly by the platform code for all cases: PCI, OF, ACPI and combinations of those. Arnd