From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756485AbcIUMtI (ORCPT ); Wed, 21 Sep 2016 08:49:08 -0400 Received: from mout.kundenserver.de ([217.72.192.75]:51711 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754266AbcIUMtH (ORCPT ); Wed, 21 Sep 2016 08:49:07 -0400 From: Arnd Bergmann To: Sriram Dash Cc: Felipe Balbi , 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" , Suresh Gupta Subject: Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev Date: Wed, 21 Sep 2016 14:48:07 +0200 Message-ID: <20042182.GcPMJken6Z@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: References: <3596831.dckG5peiyF@wuerfel> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:ILdxVQuAwwaw9c5PBV65SlzOcZvzBw7zGBGjQ4V1chg3SWbpMQt JAM6vKKeQfj3Dg7JkssgsprCPycRAUptHUqWH2d51C5GgUHBIcWOU9QFp/ElJmC6VkezKoe qRIG2YNok3EIF1Bxg4LewWl179hL8MFG/MS0V/aaymw/BiSDNRb5bYfYd5awEoNQXk9zXGn JNLaNV/pnGcAFCknBypZQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:1jr07X6HDdI=:UjCrLf/E4gTFSKCDMrQtz7 AOXMCgdfAlGJUMJ6F5QTyxlsOcrg/Ft6SBrmhZ1ZXW7ECZjlCTCFjf8WFftt0A3lDNJpKpjfR dQcBV6rNi7NFr2hMCDwL6gD9a46OVzJEcoEtuFyEOS2qGQDvPztqjcW/dhex5jAn/QtdlIFV4 ADRhcdxqayiOmGhyhjpNVXhuWd8vL/h4/kJKwGQxywkFA9PxijdXmoBWtQmFWHc8ljogZs1aS 5s2ZYxH8CcxgRjO05CkNibr3WeZZSb59hJoIDBGYwwFE+72acZFJZVUKPA8r2Jxkbe1O6NmGi bkGns+KZgw5jnlL0Jx3REqbc23yMqd5d5imi6jgThKpwucuucOdg48HhsmZF1ibKysWxv5+/+ MtHFcO3TAy3cphkebZVGhWnWz6CAUJq46tK/Pfz7LQV6U5aClfx4kcA5aDB4HAyO+zpxRKlDW c702eQqsL1AJSg0LssuX6cAPO+3/arYTkVmttO1kHEk0qrmg3ehFUgC7nbO+7/soT4WOEz8tK cGMMBc1w3pQXANdVLsmS3uDg/YD8O65athWbZiKrk03MAvSKFjH4VBFiKTILGQ3R+bm2npCSC iOUZGTbBI6htjBVuLBiIgEl3konRF6ZOEoTyDSz0gSZV5vmUzGuMyR0GpN6uBDwT+GpP0wckx CMpwaSeB8LzlqGtgmUxDGO/bOP52X+9KxkpiTQDmwmAfyoQ== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote: > >From: Arnd Bergmann [mailto:arnd@arndb.de] > >On Wednesday, September 21, 2016 11:06:47 AM CEST Sriram Dash wrote: > > ============================================================== > From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001 > From: Sriram Dash > Date: Wed, 21 Sep 2016 11:39:30 +0530 > Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration from > parent dev > > Fixes the patch https://patchwork.kernel.org/patch/9319527/ > ("usb: dwc3: host: inherit dma configuration from parent dev"). > > Signed-off-by: Sriram Dash > --- > drivers/usb/host/xhci-mem.c | 12 ++++++------ > drivers/usb/host/xhci.c | 20 ++++++++++---------- > 2 files changed, 16 insertions(+), 16 deletions(-) > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > index 6afe323..79608df 100644 > --- a/drivers/usb/host/xhci-mem.c > +++ b/drivers/usb/host/xhci-mem.c All the changes you did to this file seem fine, I completely missed that part. > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 01d96c9..9a1ff09 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -231,7 +231,7 @@ static int xhci_free_msi(struct xhci_hcd *xhci) > static int xhci_setup_msi(struct xhci_hcd *xhci) > { > int ret; > - struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller); > + struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev); > > ret = pci_enable_msi(pdev); > if (ret) { This one is interesting as I stumbled over some code comment mentioning that for dwc3-pci, we don't support MSI. I think with this change, we /can/ actually support MSI, but this could be a separate patch and we'd have to test it on dwc3-pci hardware. Same for most of this file. > @@ -745,7 +745,7 @@ void xhci_shutdown(struct usb_hcd *hcd) > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > > if (xhci->quirks & XHCI_SPURIOUS_REBOOT) > - usb_disable_xhci_ports(to_pci_dev(hcd->self.controller)); > + usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev)); > > spin_lock_irq(&xhci->lock); > xhci_halt(xhci); This seems obviously correct, but I don't yet see why it currently works. We probably don't call this function on dwc3. > #ifdef CONFIG_PM > @@ -3605,7 +3605,7 @@ void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) > * if no devices remain. > */ > if (xhci->quirks & XHCI_RESET_ON_RESUME) > - pm_runtime_put_noidle(hcd->self.controller); > + pm_runtime_put_noidle(hcd->self.sysdev); > #endif > > ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__); I suspect this one is wrong, based on what Felipe explained earlier: the power management should propagate down from the child to the parent device. Someone who understands this better than I do should look at it. > @@ -3745,7 +3745,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev) > * suspend if there is a device attached. > */ > if (xhci->quirks & XHCI_RESET_ON_RESUME) > - pm_runtime_get_noresume(hcd->self.controller); > + pm_runtime_get_noresume(hcd->self.sysdev); > #endif > > Same here. > @@ -4834,7 +4834,7 @@ int xhci_get_frame(struct usb_hcd *hcd) > int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) > { > struct xhci_hcd *xhci; > - struct device *dev = hcd->self.controller; > + struct device *dev = hcd->self.sysdev; > int retval; This one calls get_quirks(dev, xhci); not sure whether this should be called with self.controller or self.sysdev, we should audit every one of the callers here to be sure: drivers/usb/host/xhci-mtk.c: return xhci_gen_setup(hcd, xhci_mtk_quirks); drivers/usb/host/xhci-pci.c: retval = xhci_gen_setup(hcd, xhci_pci_quirks); drivers/usb/host/xhci-plat.c: return xhci_gen_setup(hcd, xhci_plat_quirks); drivers/usb/host/xhci-rcar.c: * xhci_gen_setup(). drivers/usb/host/xhci-tegra.c: return xhci_gen_setup(hcd, tegra_xhci_quirks); Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Wed, 21 Sep 2016 14:48:07 +0200 Subject: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev In-Reply-To: References: <3596831.dckG5peiyF@wuerfel> Message-ID: <20042182.GcPMJken6Z@wuerfel> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote: > >From: Arnd Bergmann [mailto:arnd at arndb.de] > >On Wednesday, September 21, 2016 11:06:47 AM CEST Sriram Dash wrote: > > ============================================================== > From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001 > From: Sriram Dash > Date: Wed, 21 Sep 2016 11:39:30 +0530 > Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration from > parent dev > > Fixes the patch https://patchwork.kernel.org/patch/9319527/ > ("usb: dwc3: host: inherit dma configuration from parent dev"). > > Signed-off-by: Sriram Dash > --- > drivers/usb/host/xhci-mem.c | 12 ++++++------ > drivers/usb/host/xhci.c | 20 ++++++++++---------- > 2 files changed, 16 insertions(+), 16 deletions(-) > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > index 6afe323..79608df 100644 > --- a/drivers/usb/host/xhci-mem.c > +++ b/drivers/usb/host/xhci-mem.c All the changes you did to this file seem fine, I completely missed that part. > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 01d96c9..9a1ff09 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -231,7 +231,7 @@ static int xhci_free_msi(struct xhci_hcd *xhci) > static int xhci_setup_msi(struct xhci_hcd *xhci) > { > int ret; > - struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller); > + struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev); > > ret = pci_enable_msi(pdev); > if (ret) { This one is interesting as I stumbled over some code comment mentioning that for dwc3-pci, we don't support MSI. I think with this change, we /can/ actually support MSI, but this could be a separate patch and we'd have to test it on dwc3-pci hardware. Same for most of this file. > @@ -745,7 +745,7 @@ void xhci_shutdown(struct usb_hcd *hcd) > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > > if (xhci->quirks & XHCI_SPURIOUS_REBOOT) > - usb_disable_xhci_ports(to_pci_dev(hcd->self.controller)); > + usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev)); > > spin_lock_irq(&xhci->lock); > xhci_halt(xhci); This seems obviously correct, but I don't yet see why it currently works. We probably don't call this function on dwc3. > #ifdef CONFIG_PM > @@ -3605,7 +3605,7 @@ void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) > * if no devices remain. > */ > if (xhci->quirks & XHCI_RESET_ON_RESUME) > - pm_runtime_put_noidle(hcd->self.controller); > + pm_runtime_put_noidle(hcd->self.sysdev); > #endif > > ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__); I suspect this one is wrong, based on what Felipe explained earlier: the power management should propagate down from the child to the parent device. Someone who understands this better than I do should look at it. > @@ -3745,7 +3745,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev) > * suspend if there is a device attached. > */ > if (xhci->quirks & XHCI_RESET_ON_RESUME) > - pm_runtime_get_noresume(hcd->self.controller); > + pm_runtime_get_noresume(hcd->self.sysdev); > #endif > > Same here. > @@ -4834,7 +4834,7 @@ int xhci_get_frame(struct usb_hcd *hcd) > int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) > { > struct xhci_hcd *xhci; > - struct device *dev = hcd->self.controller; > + struct device *dev = hcd->self.sysdev; > int retval; This one calls get_quirks(dev, xhci); not sure whether this should be called with self.controller or self.sysdev, we should audit every one of the callers here to be sure: drivers/usb/host/xhci-mtk.c: return xhci_gen_setup(hcd, xhci_mtk_quirks); drivers/usb/host/xhci-pci.c: retval = xhci_gen_setup(hcd, xhci_pci_quirks); drivers/usb/host/xhci-plat.c: return xhci_gen_setup(hcd, xhci_plat_quirks); drivers/usb/host/xhci-rcar.c: * xhci_gen_setup(). drivers/usb/host/xhci-tegra.c: return xhci_gen_setup(hcd, tegra_xhci_quirks); Arnd