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

[Sriram Dash]

>From: Arnd Bergmann [mailto:arnd@xxxxxxxx]
>On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote:
>> >From: Arnd Bergmann [mailto:arnd@xxxxxxxx] 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@xxxxxxx>
>> 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@xxxxxxx>
>> ---
>>  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

Yes. Some sanity is required over this patch.

>> @@ -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