Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754004AbcLIL4z (ORCPT ); Fri, 9 Dec 2016 06:56:55 -0500 Received: from lelnx193.ext.ti.com ([198.47.27.77]:11393 "EHLO lelnx193.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753610AbcLIL4x (ORCPT ); Fri, 9 Dec 2016 06:56:53 -0500 Subject: Re: [PATCH v5 4/6] usb: xhci: use bus->sysdev for DMA configuration To: Sriram Dash , , , , Arnd Bergmann References: <1479383028-27701-1-git-send-email-sriram.dash@nxp.com> <1479383028-27701-5-git-send-email-sriram.dash@nxp.com> CC: , , , , From: Roger Quadros Message-ID: <6807fef7-df05-bfd7-0447-b1abd29cf25e@ti.com> Date: Fri, 9 Dec 2016 13:56:32 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <1479383028-27701-5-git-send-email-sriram.dash@nxp.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10387 Lines: 267 Hi, On 17/11/16 13:43, Sriram Dash wrote: > From: Arnd Bergmann > > For xhci-hcd platform device, all the DMA parameters are not > configured properly, notably dma ops for dwc3 devices. So, set > the dma for xhci from sysdev. sysdev is pointing to device that > is known to the system firmware or hardware. > > Signed-off-by: Arnd Bergmann > Signed-off-by: Sriram Dash > Tested-by: Baolin Wang > --- > Changes in v5: > - No update > > Changes in v4: > - No update > > Changes in v3: > - No update > > Changes in v2: > - Separate out xhci driver changes apart > > drivers/usb/host/xhci-mem.c | 12 ++++++------ > drivers/usb/host/xhci-plat.c | 33 ++++++++++++++++++++++++++------- > drivers/usb/host/xhci.c | 15 +++++++++++---- > 3 files changed, 43 insertions(+), 17 deletions(-) > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > index 48a26d378..eb32de9 100644 > --- a/drivers/usb/host/xhci-mem.c > +++ b/drivers/usb/host/xhci-mem.c > @@ -586,7 +586,7 @@ static void xhci_free_stream_ctx(struct xhci_hcd *xhci, > unsigned int num_stream_ctxs, > struct xhci_stream_ctx *stream_ctx, dma_addr_t dma) > { > - struct device *dev = xhci_to_hcd(xhci)->self.controller; > + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; > size_t size = sizeof(struct xhci_stream_ctx) * num_stream_ctxs; > > if (size > MEDIUM_STREAM_ARRAY_SIZE) > @@ -614,7 +614,7 @@ static struct xhci_stream_ctx *xhci_alloc_stream_ctx(struct xhci_hcd *xhci, > unsigned int num_stream_ctxs, dma_addr_t *dma, > gfp_t mem_flags) > { > - struct device *dev = xhci_to_hcd(xhci)->self.controller; > + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; > size_t size = sizeof(struct xhci_stream_ctx) * num_stream_ctxs; > > if (size > MEDIUM_STREAM_ARRAY_SIZE) > @@ -1644,7 +1644,7 @@ void xhci_slot_copy(struct xhci_hcd *xhci, > static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags) > { > int i; > - struct device *dev = xhci_to_hcd(xhci)->self.controller; > + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; > int num_sp = HCS_MAX_SCRATCHPAD(xhci->hcs_params2); > > xhci_dbg_trace(xhci, trace_xhci_dbg_init, > @@ -1716,7 +1716,7 @@ static void scratchpad_free(struct xhci_hcd *xhci) > { > int num_sp; > int i; > - struct device *dev = xhci_to_hcd(xhci)->self.controller; > + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; > > if (!xhci->scratchpad) > return; > @@ -1792,7 +1792,7 @@ void xhci_free_command(struct xhci_hcd *xhci, > > void xhci_mem_cleanup(struct xhci_hcd *xhci) > { > - struct device *dev = xhci_to_hcd(xhci)->self.controller; > + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; > int size; > int i, j, num_ports; > > @@ -2334,7 +2334,7 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags) > int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) > { > dma_addr_t dma; > - struct device *dev = xhci_to_hcd(xhci)->self.controller; > + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; > unsigned int val, val2; > u64 val_64; > struct xhci_segment *seg; > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index ed56bf9..beb95c8 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -14,6 +14,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -139,6 +140,7 @@ static int xhci_plat_probe(struct platform_device *pdev) > { > const struct of_device_id *match; > const struct hc_driver *driver; > + struct device *sysdev; > struct xhci_hcd *xhci; > struct resource *res; > struct usb_hcd *hcd; > @@ -155,22 +157,39 @@ static int xhci_plat_probe(struct platform_device *pdev) > if (irq < 0) > return -ENODEV; > > + /* > + * sysdev must point to a device that is known to the system firmware > + * or PCI hardware. We handle these three cases here: > + * 1. xhci_plat comes from firmware > + * 2. xhci_plat is child of a device from firmware (dwc3-plat) > + * 3. xhci_plat is grandchild of a pci device (dwc3-pci) > + */ > + sysdev = &pdev->dev; > + if (sysdev->parent && !sysdev->of_node && sysdev->parent->of_node) > + sysdev = sysdev->parent; > +#ifdef CONFIG_PCI > + else if (sysdev->parent && sysdev->parent->parent && > + sysdev->parent->parent->bus == &pci_bus_type) > + sysdev = sysdev->parent->parent; > +#endif > + > /* Try to set 64-bit DMA first */ > - if (WARN_ON(!pdev->dev.dma_mask)) > + if (WARN_ON(!sysdev->dma_mask)) > /* Platform did not initialize dma_mask */ > - ret = dma_coerce_mask_and_coherent(&pdev->dev, > + ret = dma_coerce_mask_and_coherent(sysdev, > DMA_BIT_MASK(64)); > else > - ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > + ret = dma_set_mask_and_coherent(sysdev, DMA_BIT_MASK(64)); > > /* If seting 64-bit DMA mask fails, fall back to 32-bit DMA mask */ > if (ret) { > - ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); > + ret = dma_set_mask_and_coherent(sysdev, DMA_BIT_MASK(32)); > if (ret) > return ret; > } > > - hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev)); > + hcd = __usb_create_hcd(driver, sysdev, &pdev->dev, > + dev_name(&pdev->dev), NULL); This is fine but we're forgetting to set sysdev while creating the shared HCD. This will lead to invalid dma parameters for the shared HCD. So we would need, - xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev, + xhci->shared_hcd = usb_create_shared_hcd(driver, sysdev, dev_name(&pdev->dev), hcd); if (!xhci->shared_hcd) { ret = -ENOMEM; Also, I noticed that Felipe has already picked up the dwc3 patches but the core and xhci patches are missing in linux-next leading to the following warning [ 4.648958] ------------[ cut here ]------------ [ 4.653602] WARNING: CPU: 0 PID: 1 at drivers/usb/host/xhci-plat.c:159 xhci_plat_probe+0x1a0/0x474 [ 4.662560] Modules linked in: [ 4.665612] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W 4.9.0-rc3 #1028 [ 4.673258] Hardware name: Keystone [ 4.676743] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 4.684479] [] (show_stack) from [] (dump_stack+0x78/0x8c) [ 4.691694] [] (dump_stack) from [] (__warn+0xec/0x104) [ 4.698648] [] (__warn) from [] (warn_slowpath_null+0x20/0x28) [ 4.706210] [] (warn_slowpath_null) from [] (xhci_plat_probe+0x1a0/0x474) [ 4.714729] [] (xhci_plat_probe) from [] (platform_drv_probe+0x4c/0xb0) [ 4.723073] [] (platform_drv_probe) from [] (driver_probe_device+0x208/0x2b4) [ 4.731937] [] (driver_probe_device) from [] (__driver_attach+0xb8/0xbc) [ 4.740365] [] (__driver_attach) from [] (bus_for_each_dev+0x68/0x9c) [ 4.748535] [] (bus_for_each_dev) from [] (bus_add_driver+0x1a4/0x21c) [ 4.756791] [] (bus_add_driver) from [] (driver_register+0x78/0xf8) [ 4.764786] [] (driver_register) from [] (do_one_initcall+0x44/0x174) [ 4.772955] [] (do_one_initcall) from [] (kernel_init_freeable+0x154/0x1e4) [ 4.781645] [] (kernel_init_freeable) from [] (kernel_init+0x8/0x10c) [ 4.789815] [] (kernel_init) from [] (ret_from_fork+0x14/0x2c) [ 4.797573] ---[ end trace 0ec4c38d69780113 ]--- cheers, -roger > if (!hcd) > return -ENOMEM; > > @@ -220,13 +239,13 @@ static int xhci_plat_probe(struct platform_device *pdev) > goto disable_clk; > } > > - if (device_property_read_bool(&pdev->dev, "usb3-lpm-capable")) > + if (device_property_read_bool(sysdev, "usb3-lpm-capable")) > xhci->quirks |= XHCI_LPM_SUPPORT; > > if (HCC_MAX_PSA(xhci->hcc_params) >= 4) > xhci->shared_hcd->can_do_streams = 1; > > - hcd->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0); > + hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0); > if (IS_ERR(hcd->usb_phy)) { > ret = PTR_ERR(hcd->usb_phy); > if (ret == -EPROBE_DEFER) > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index cf30cb6..fc76671 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -231,6 +231,9 @@ static int xhci_free_msi(struct xhci_hcd *xhci) > static int xhci_setup_msi(struct xhci_hcd *xhci) > { > int ret; > + /* > + * TODO:Check with MSI Soc for sysdev > + */ > struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller); > > ret = pci_enable_msi(pdev); > @@ -257,7 +260,7 @@ static int xhci_setup_msi(struct xhci_hcd *xhci) > */ > static void xhci_free_irq(struct xhci_hcd *xhci) > { > - 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); > int ret; > > /* return if using legacy interrupt */ > @@ -743,7 +746,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); > @@ -760,7 +763,7 @@ void xhci_shutdown(struct usb_hcd *hcd) > > /* Yet another workaround for spurious wakeups at shutdown with HSW */ > if (xhci->quirks & XHCI_SPURIOUS_WAKEUP) > - pci_set_power_state(to_pci_dev(hcd->self.controller), PCI_D3hot); > + pci_set_power_state(to_pci_dev(hcd->self.sysdev), PCI_D3hot); > } > > #ifdef CONFIG_PM > @@ -4832,7 +4835,11 @@ 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; > + /* > + * TODO: Check with DWC3 clients for sysdev according to > + * quirks > + */ > + struct device *dev = hcd->self.sysdev; > int retval; > > /* Accept arbitrarily long scatter-gather lists */ >