Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

From: Arnd Bergmann <arnd@arndb.de>
To: Sriram Dash <sriram.dash@nxp.com>
Cc: Felipe Balbi <balbi@kernel.org>,
	Peter Chen <hzpeterchen@gmail.com>, Leo Li <pku.leo@gmail.com>,
	Grygorii Strashko <grygorii.strashko@ti.com>,
	Russell King - ARM Linux <linux@arm.linux.org.uk>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	Sekhar Nori <nsekhar@ti.com>, lkml <linux-kernel@vger.kernel.org>,
	Stuart Yoder <stuart.yoder@nxp.com>,
	Scott Wood <oss@buserror.net>,
	David Fisher <david.fisher1@synopsys.com>,
	"Thang Q. Nguyen" <tqnguyen@apm.com>,
	Alan Stern <stern@rowland.harvard.edu>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	Suresh Gupta <suresh.gupta@nxp.com>
Subject: Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Date: Wed, 21 Sep 2016 14:48:07 +0200	[thread overview]
Message-ID: <20042182.GcPMJken6Z@wuerfel> (raw)
In-Reply-To: <DB5PR0401MB1925CDF6CBD45ACBEE95D152F5F60@DB5PR0401MB1925.eurprd04.prod.outlook.com>

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 <sriram.dash@nxp.com>
> 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 <sriram.dash@nxp.com>
> ---
>  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