diff mbox

[v13,06/12] usb: xhci: use bus->sysdev for DMA configuration

Message ID 1486776443-2280-7-git-send-email-peter.chen@nxp.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Chen Feb. 11, 2017, 1:27 a.m. UTC
From: Arnd Bergmann <arnd@arndb.de>

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.

Cc: Baolin Wang <baolin.wang@linaro.org>
Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
Cc: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Cc: Mathias Nyman <mathias.nyman@linux.intel.com>

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
---
Hi, Baolin, Vivek and Alexander,
I removed your tested-by tag due to add one change that adding sysdev
for shared hcd too, if your test shows this change works for you or
has no effect for you, please consider adding tested-by tag again,
thanks.

 drivers/usb/host/xhci-mem.c  | 12 ++++++------
 drivers/usb/host/xhci-plat.c | 35 +++++++++++++++++++++++++++--------
 drivers/usb/host/xhci.c      | 15 +++++++++++----
 3 files changed, 44 insertions(+), 18 deletions(-)

Comments

Alexander Sverdlin Feb. 13, 2017, 8:25 a.m. UTC | #1
Hi!

On 11/02/17 02:27, Peter Chen wrote:
> 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.
> 
> Cc: Baolin Wang <baolin.wang@linaro.org>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Cc: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
> ---
> Hi, Baolin, Vivek and Alexander,
> I removed your tested-by tag due to add one change that adding sysdev
> for shared hcd too, if your test shows this change works for you or
> has no effect for you, please consider adding tested-by tag again,
> thanks.

This was exactly the missing/requiring part in your patch for my case.
I had to add it extra for the first test.
So, again, here is my
Tested-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

>  drivers/usb/host/xhci-mem.c  | 12 ++++++------
>  drivers/usb/host/xhci-plat.c | 35 +++++++++++++++++++++++++++--------
>  drivers/usb/host/xhci.c      | 15 +++++++++++----
>  3 files changed, 44 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index ba1853f4..032a702 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)
> @@ -1686,7 +1686,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,
> @@ -1758,7 +1758,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;
> @@ -1831,7 +1831,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;
>  
> @@ -2373,7 +2373,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 6d33b42..4ecb3fd 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -14,6 +14,7 @@
>  #include <linux/clk.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/module.h>
> +#include <linux/pci.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/usb/phy.h>
> @@ -148,6 +149,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;
> @@ -164,22 +166,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 (!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);
>  	if (!hcd)
>  		return -ENOMEM;
>  
> @@ -222,20 +241,20 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  
>  	xhci->clk = clk;
>  	xhci->main_hcd = hcd;
> -	xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
> +	xhci->shared_hcd = __usb_create_hcd(driver, sysdev, &pdev->dev,
>  			dev_name(&pdev->dev), hcd);
>  	if (!xhci->shared_hcd) {
>  		ret = -ENOMEM;
>  		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 (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
>  		xhci->quirks |= XHCI_BROKEN_PORT_PED;
>  
> -	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 6d6c460..ab0839c 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -237,6 +237,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);
> @@ -263,7 +266,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 */
> @@ -748,7 +751,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);
> @@ -765,7 +768,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
> @@ -4801,7 +4804,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 */
Vivek Gautam Feb. 14, 2017, 5:33 a.m. UTC | #2
On 02/11/2017 06:57 AM, Peter Chen wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> 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.
>
> Cc: Baolin Wang <baolin.wang@linaro.org>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Cc: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
> ---
> Hi, Baolin, Vivek and Alexander,
> I removed your tested-by tag due to add one change that adding sysdev
> for shared hcd too, if your test shows this change works for you or
> has no effect for you, please consider adding tested-by tag again,
> thanks.

Works well with 4.10-rc8 branch merged with usb-next and the
set of phy patches for msm8996 and corresponding dt patches.

Tested-by: Vivek Gautam <vivek.gautam@codeaurora.org>

Thanks
Vivek

>   drivers/usb/host/xhci-mem.c  | 12 ++++++------
>   drivers/usb/host/xhci-plat.c | 35 +++++++++++++++++++++++++++--------
>   drivers/usb/host/xhci.c      | 15 +++++++++++----
>   3 files changed, 44 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index ba1853f4..032a702 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)
> @@ -1686,7 +1686,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,
> @@ -1758,7 +1758,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;
> @@ -1831,7 +1831,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;
>   
> @@ -2373,7 +2373,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 6d33b42..4ecb3fd 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -14,6 +14,7 @@
>   #include <linux/clk.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/module.h>
> +#include <linux/pci.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
>   #include <linux/usb/phy.h>
> @@ -148,6 +149,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;
> @@ -164,22 +166,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 (!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);
>   	if (!hcd)
>   		return -ENOMEM;
>   
> @@ -222,20 +241,20 @@ static int xhci_plat_probe(struct platform_device *pdev)
>   
>   	xhci->clk = clk;
>   	xhci->main_hcd = hcd;
> -	xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
> +	xhci->shared_hcd = __usb_create_hcd(driver, sysdev, &pdev->dev,
>   			dev_name(&pdev->dev), hcd);
>   	if (!xhci->shared_hcd) {
>   		ret = -ENOMEM;
>   		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 (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
>   		xhci->quirks |= XHCI_BROKEN_PORT_PED;
>   
> -	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 6d6c460..ab0839c 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -237,6 +237,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);
> @@ -263,7 +266,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 */
> @@ -748,7 +751,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);
> @@ -765,7 +768,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
> @@ -4801,7 +4804,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 */
Roger Quadros Feb. 14, 2017, 10:36 a.m. UTC | #3
Hi,

On 11/02/17 03:27, Peter Chen wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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.
> 
> Cc: Baolin Wang <baolin.wang@linaro.org>
> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
> Cc: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
> ---
> Hi, Baolin, Vivek and Alexander,
> I removed your tested-by tag due to add one change that adding sysdev
> for shared hcd too, if your test shows this change works for you or
> has no effect for you, please consider adding tested-by tag again,
> thanks.
> 
>  drivers/usb/host/xhci-mem.c  | 12 ++++++------
>  drivers/usb/host/xhci-plat.c | 35 +++++++++++++++++++++++++++--------
>  drivers/usb/host/xhci.c      | 15 +++++++++++----
>  3 files changed, 44 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index ba1853f4..032a702 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)
> @@ -1686,7 +1686,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,
> @@ -1758,7 +1758,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;
> @@ -1831,7 +1831,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;
>  
> @@ -2373,7 +2373,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 6d33b42..4ecb3fd 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -14,6 +14,7 @@
>  #include <linux/clk.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/module.h>
> +#include <linux/pci.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/usb/phy.h>
> @@ -148,6 +149,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;
> @@ -164,22 +166,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 (!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);
>  	if (!hcd)
>  		return -ENOMEM;
>  
> @@ -222,20 +241,20 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  
>  	xhci->clk = clk;
>  	xhci->main_hcd = hcd;
> -	xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
> +	xhci->shared_hcd = __usb_create_hcd(driver, sysdev, &pdev->dev,
>  			dev_name(&pdev->dev), hcd);
>  	if (!xhci->shared_hcd) {
>  		ret = -ENOMEM;
>  		goto disable_clk;
>  	}
>  
> -	if (device_property_read_bool(&pdev->dev, "usb3-lpm-capable"))
> +	if (device_property_read_bool(sysdev, "usb3-lpm-capable"))

Why are we using sysdev to read DT property? We should be using the
XHCI device (&pdev->dev) here, no?

>  		xhci->quirks |= XHCI_LPM_SUPPORT;
>  
>  	if (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
>  		xhci->quirks |= XHCI_BROKEN_PORT_PED;
>  
> -	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);

Same here. The PHY is linked to the XHCI device (pdev->dev) and not its parent.

>  	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 6d6c460..ab0839c 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -237,6 +237,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);
> @@ -263,7 +266,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 */
> @@ -748,7 +751,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);
> @@ -765,7 +768,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
> @@ -4801,7 +4804,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 */
>
Arnd Bergmann Feb. 14, 2017, 11:44 a.m. UTC | #4
On Tue, Feb 14, 2017 at 11:36 AM, Roger Quadros <rogerq@ti.com> wrote:
> On 11/02/17 03:27, Peter Chen wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> 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.
>>
>> Cc: Baolin Wang <baolin.wang@linaro.org>
>> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
>> Cc: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
>> ---
>> Hi, Baolin, Vivek and Alexander,
>> I removed your tested-by tag due to add one change that adding sysdev
>> for shared hcd too, if your test shows this change works for you or
>> has no effect for you, please consider adding tested-by tag again,
>> thanks.

>> @@ -222,20 +241,20 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>
>>       xhci->clk = clk;
>>       xhci->main_hcd = hcd;
>> -     xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
>> +     xhci->shared_hcd = __usb_create_hcd(driver, sysdev, &pdev->dev,
>>                       dev_name(&pdev->dev), hcd);
>>       if (!xhci->shared_hcd) {
>>               ret = -ENOMEM;
>>               goto disable_clk;
>>       }
>>
>> -     if (device_property_read_bool(&pdev->dev, "usb3-lpm-capable"))
>> +     if (device_property_read_bool(sysdev, "usb3-lpm-capable"))
>
> Why are we using sysdev to read DT property? We should be using the
> XHCI device (&pdev->dev) here, no?

If I remember correctly, this is one of the cases where pdev does not
have a device node attached to it because it was created by the driver
of the parent device on the fly in case of dwc3. When you have a pure xhci
device in DT, the two pointers are the same.

    Arnd
Roger Quadros Feb. 14, 2017, 12:26 p.m. UTC | #5
On 14/02/17 13:44, Arnd Bergmann wrote:
> On Tue, Feb 14, 2017 at 11:36 AM, Roger Quadros <rogerq@ti.com> wrote:
>> On 11/02/17 03:27, Peter Chen wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> 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.
>>>
>>> Cc: Baolin Wang <baolin.wang@linaro.org>
>>> Cc: Vivek Gautam <vivek.gautam@codeaurora.org>
>>> Cc: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>>> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
>>>
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
>>> ---
>>> Hi, Baolin, Vivek and Alexander,
>>> I removed your tested-by tag due to add one change that adding sysdev
>>> for shared hcd too, if your test shows this change works for you or
>>> has no effect for you, please consider adding tested-by tag again,
>>> thanks.
> 
>>> @@ -222,20 +241,20 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>>
>>>       xhci->clk = clk;
>>>       xhci->main_hcd = hcd;
>>> -     xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
>>> +     xhci->shared_hcd = __usb_create_hcd(driver, sysdev, &pdev->dev,
>>>                       dev_name(&pdev->dev), hcd);
>>>       if (!xhci->shared_hcd) {
>>>               ret = -ENOMEM;
>>>               goto disable_clk;
>>>       }
>>>
>>> -     if (device_property_read_bool(&pdev->dev, "usb3-lpm-capable"))
>>> +     if (device_property_read_bool(sysdev, "usb3-lpm-capable"))
>>
>> Why are we using sysdev to read DT property? We should be using the
>> XHCI device (&pdev->dev) here, no?
> 
> If I remember correctly, this is one of the cases where pdev does not
> have a device node attached to it because it was created by the driver
> of the parent device on the fly in case of dwc3. When you have a pure xhci
> device in DT, the two pointers are the same.

From drivers/usb/dwc3/host.c

>         if (dwc->usb3_lpm_capable) {
>                 props[0].name = "usb3-lpm-capable";
>                 ret = platform_device_add_properties(xhci, props);
>                 if (ret) {
>                         dev_err(dwc->dev, "failed to add properties to xHCI\n");
>                         goto err1;
>                 }
>         }

So it is setting the usb3-lpm-capable property into the xhci platform device
and we should be reading the property from there.
Arnd Bergmann Feb. 14, 2017, 12:58 p.m. UTC | #6
On Tue, Feb 14, 2017 at 1:26 PM, Roger Quadros <rogerq@ti.com> wrote:
> On 14/02/17 13:44, Arnd Bergmann wrote:
>> On Tue, Feb 14, 2017 at 11:36 AM, Roger Quadros <rogerq@ti.com> wrote:

>>> Why are we using sysdev to read DT property? We should be using the
>>> XHCI device (&pdev->dev) here, no?
>>
>> If I remember correctly, this is one of the cases where pdev does not
>> have a device node attached to it because it was created by the driver
>> of the parent device on the fly in case of dwc3. When you have a pure xhci
>> device in DT, the two pointers are the same.
>
> From drivers/usb/dwc3/host.c
>
>>         if (dwc->usb3_lpm_capable) {
>>                 props[0].name = "usb3-lpm-capable";
>>                 ret = platform_device_add_properties(xhci, props);
>>                 if (ret) {
>>                         dev_err(dwc->dev, "failed to add properties to xHCI\n");
>>                         goto err1;
>>                 }
>>         }
>
> So it is setting the usb3-lpm-capable property into the xhci platform device
> and we should be reading the property from there.

Hmm, ideally we would only have properties on one of the two, since we
refer to the sysdev for the properties regarding DMA and PHY among other
things, but I guess that's not an option here, since we can't call
platform_device_add_properties() on a dwc_pci device and have to
use the xhci pdev instead.

     Arnd
Peter Chen Feb. 15, 2017, 1:35 a.m. UTC | #7
On Tue, Feb 14, 2017 at 01:58:40PM +0100, Arnd Bergmann wrote:
> On Tue, Feb 14, 2017 at 1:26 PM, Roger Quadros <rogerq@ti.com> wrote:
> > On 14/02/17 13:44, Arnd Bergmann wrote:
> >> On Tue, Feb 14, 2017 at 11:36 AM, Roger Quadros <rogerq@ti.com> wrote:
> 
> >>> Why are we using sysdev to read DT property? We should be using the
> >>> XHCI device (&pdev->dev) here, no?
> >>
> >> If I remember correctly, this is one of the cases where pdev does not
> >> have a device node attached to it because it was created by the driver
> >> of the parent device on the fly in case of dwc3. When you have a pure xhci
> >> device in DT, the two pointers are the same.
> >
> > From drivers/usb/dwc3/host.c
> >
> >>         if (dwc->usb3_lpm_capable) {
> >>                 props[0].name = "usb3-lpm-capable";
> >>                 ret = platform_device_add_properties(xhci, props);
> >>                 if (ret) {
> >>                         dev_err(dwc->dev, "failed to add properties to xHCI\n");
> >>                         goto err1;
> >>                 }
> >>         }
> >
> > So it is setting the usb3-lpm-capable property into the xhci platform device
> > and we should be reading the property from there.

Why dwc3 needs another "snps,usb3_lpm_capable"? Why not using
"usb3-lpm-capable" at firmware directly?

Peter

> 
> Hmm, ideally we would only have properties on one of the two, since we
> refer to the sysdev for the properties regarding DMA and PHY among other
> things, but I guess that's not an option here, since we can't call
> platform_device_add_properties() on a dwc_pci device and have to
> use the xhci pdev instead.
> 
>      Arnd
>
Roger Quadros Feb. 15, 2017, 8:18 a.m. UTC | #8
Peter,

On 15/02/17 03:35, Peter Chen wrote:
> On Tue, Feb 14, 2017 at 01:58:40PM +0100, Arnd Bergmann wrote:
>> On Tue, Feb 14, 2017 at 1:26 PM, Roger Quadros <rogerq@ti.com> wrote:
>>> On 14/02/17 13:44, Arnd Bergmann wrote:
>>>> On Tue, Feb 14, 2017 at 11:36 AM, Roger Quadros <rogerq@ti.com> wrote:
>>
>>>>> Why are we using sysdev to read DT property? We should be using the
>>>>> XHCI device (&pdev->dev) here, no?
>>>>
>>>> If I remember correctly, this is one of the cases where pdev does not
>>>> have a device node attached to it because it was created by the driver
>>>> of the parent device on the fly in case of dwc3. When you have a pure xhci
>>>> device in DT, the two pointers are the same.
>>>
>>> From drivers/usb/dwc3/host.c
>>>
>>>>         if (dwc->usb3_lpm_capable) {
>>>>                 props[0].name = "usb3-lpm-capable";
>>>>                 ret = platform_device_add_properties(xhci, props);
>>>>                 if (ret) {
>>>>                         dev_err(dwc->dev, "failed to add properties to xHCI\n");
>>>>                         goto err1;
>>>>                 }
>>>>         }
>>>
>>> So it is setting the usb3-lpm-capable property into the xhci platform device
>>> and we should be reading the property from there.
> 
> Why dwc3 needs another "snps,usb3_lpm_capable"? Why not using
> "usb3-lpm-capable" at firmware directly?

dwc3 is not setting "snps,usb3_lpm_capable" but "usb3-lpm-capable" for the
xhci platform device.

What did you mean by firmware? Did you mean something like BIOS?
At least TI platforms don't use any firmware like BIOS. So dwc3 driver
needs to create a platform device for xhci on the fly and set the DT properties.

> 
> Peter
> 
>>
>> Hmm, ideally we would only have properties on one of the two, since we
>> refer to the sysdev for the properties regarding DMA and PHY among other
>> things, but I guess that's not an option here, since we can't call
>> platform_device_add_properties() on a dwc_pci device and have to
>> use the xhci pdev instead.
>>
>>      Arnd
>>
Peter Chen Feb. 15, 2017, 8:51 a.m. UTC | #9
On Wed, Feb 15, 2017 at 10:18:03AM +0200, Roger Quadros wrote:
> Peter,
> 
> On 15/02/17 03:35, Peter Chen wrote:
> > On Tue, Feb 14, 2017 at 01:58:40PM +0100, Arnd Bergmann wrote:
> >> On Tue, Feb 14, 2017 at 1:26 PM, Roger Quadros <rogerq@ti.com> wrote:
> >>> On 14/02/17 13:44, Arnd Bergmann wrote:
> >>>> On Tue, Feb 14, 2017 at 11:36 AM, Roger Quadros <rogerq@ti.com> wrote:
> >>
> >>>>> Why are we using sysdev to read DT property? We should be using the
> >>>>> XHCI device (&pdev->dev) here, no?
> >>>>
> >>>> If I remember correctly, this is one of the cases where pdev does not
> >>>> have a device node attached to it because it was created by the driver
> >>>> of the parent device on the fly in case of dwc3. When you have a pure xhci
> >>>> device in DT, the two pointers are the same.
> >>>
> >>> From drivers/usb/dwc3/host.c
> >>>
> >>>>         if (dwc->usb3_lpm_capable) {
> >>>>                 props[0].name = "usb3-lpm-capable";
> >>>>                 ret = platform_device_add_properties(xhci, props);
> >>>>                 if (ret) {
> >>>>                         dev_err(dwc->dev, "failed to add properties to xHCI\n");
> >>>>                         goto err1;
> >>>>                 }
> >>>>         }
> >>>
> >>> So it is setting the usb3-lpm-capable property into the xhci platform device
> >>> and we should be reading the property from there.
> > 
> > Why dwc3 needs another "snps,usb3_lpm_capable"? Why not using
> > "usb3-lpm-capable" at firmware directly?
> 
> dwc3 is not setting "snps,usb3_lpm_capable" but "usb3-lpm-capable" for the
> xhci platform device.
> 
> What did you mean by firmware? Did you mean something like BIOS?
> At least TI platforms don't use any firmware like BIOS. So dwc3 driver
> needs to create a platform device for xhci on the fly and set the DT properties.
> 

By readying code, the dwc3 calls dwc3_get_properties to set
dwc->usb3_lpm_capable, and at dwc3/host.c, it sets property
"usb3-lpm-capable" according to this flag, why not let common
code xhci-plat.c to get this property from sysdev which is DT
nodes for dwc3?
Roger Quadros Feb. 15, 2017, 10:10 a.m. UTC | #10
+Felipe,

On 15/02/17 10:51, Peter Chen wrote:
> On Wed, Feb 15, 2017 at 10:18:03AM +0200, Roger Quadros wrote:
>> Peter,
>>
>> On 15/02/17 03:35, Peter Chen wrote:
>>> On Tue, Feb 14, 2017 at 01:58:40PM +0100, Arnd Bergmann wrote:
>>>> On Tue, Feb 14, 2017 at 1:26 PM, Roger Quadros <rogerq@ti.com> wrote:
>>>>> On 14/02/17 13:44, Arnd Bergmann wrote:
>>>>>> On Tue, Feb 14, 2017 at 11:36 AM, Roger Quadros <rogerq@ti.com> wrote:
>>>>
>>>>>>> Why are we using sysdev to read DT property? We should be using the
>>>>>>> XHCI device (&pdev->dev) here, no?
>>>>>>
>>>>>> If I remember correctly, this is one of the cases where pdev does not
>>>>>> have a device node attached to it because it was created by the driver
>>>>>> of the parent device on the fly in case of dwc3. When you have a pure xhci
>>>>>> device in DT, the two pointers are the same.
>>>>>
>>>>> From drivers/usb/dwc3/host.c
>>>>>
>>>>>>         if (dwc->usb3_lpm_capable) {
>>>>>>                 props[0].name = "usb3-lpm-capable";
>>>>>>                 ret = platform_device_add_properties(xhci, props);
>>>>>>                 if (ret) {
>>>>>>                         dev_err(dwc->dev, "failed to add properties to xHCI\n");
>>>>>>                         goto err1;
>>>>>>                 }
>>>>>>         }
>>>>>
>>>>> So it is setting the usb3-lpm-capable property into the xhci platform device
>>>>> and we should be reading the property from there.
>>>
>>> Why dwc3 needs another "snps,usb3_lpm_capable"? Why not using
>>> "usb3-lpm-capable" at firmware directly?
>>
>> dwc3 is not setting "snps,usb3_lpm_capable" but "usb3-lpm-capable" for the
>> xhci platform device.
>>
>> What did you mean by firmware? Did you mean something like BIOS?
>> At least TI platforms don't use any firmware like BIOS. So dwc3 driver
>> needs to create a platform device for xhci on the fly and set the DT properties.
>>
> 
> By readying code, the dwc3 calls dwc3_get_properties to set
> dwc->usb3_lpm_capable, and at dwc3/host.c, it sets property
> "usb3-lpm-capable" according to this flag, why not let common
> code xhci-plat.c to get this property from sysdev which is DT
> nodes for dwc3?
> 

Felipe, any comments?
Felipe Balbi Feb. 15, 2017, 11:51 a.m. UTC | #11
Hi,

Roger Quadros <rogerq@ti.com> writes:
>>>>>>>> Why are we using sysdev to read DT property? We should be using the
>>>>>>>> XHCI device (&pdev->dev) here, no?
>>>>>>>
>>>>>>> If I remember correctly, this is one of the cases where pdev does not
>>>>>>> have a device node attached to it because it was created by the driver
>>>>>>> of the parent device on the fly in case of dwc3. When you have a pure xhci
>>>>>>> device in DT, the two pointers are the same.
>>>>>>
>>>>>> From drivers/usb/dwc3/host.c
>>>>>>
>>>>>>>         if (dwc->usb3_lpm_capable) {
>>>>>>>                 props[0].name = "usb3-lpm-capable";
>>>>>>>                 ret = platform_device_add_properties(xhci, props);
>>>>>>>                 if (ret) {
>>>>>>>                         dev_err(dwc->dev, "failed to add properties to xHCI\n");
>>>>>>>                         goto err1;
>>>>>>>                 }
>>>>>>>         }
>>>>>>
>>>>>> So it is setting the usb3-lpm-capable property into the xhci platform device
>>>>>> and we should be reading the property from there.
>>>>
>>>> Why dwc3 needs another "snps,usb3_lpm_capable"? Why not using
>>>> "usb3-lpm-capable" at firmware directly?
>>>
>>> dwc3 is not setting "snps,usb3_lpm_capable" but "usb3-lpm-capable" for the
>>> xhci platform device.
>>>
>>> What did you mean by firmware? Did you mean something like BIOS?
>>> At least TI platforms don't use any firmware like BIOS. So dwc3 driver
>>> needs to create a platform device for xhci on the fly and set the DT properties.
>>>
>> 
>> By readying code, the dwc3 calls dwc3_get_properties to set
>> dwc->usb3_lpm_capable, and at dwc3/host.c, it sets property
>> "usb3-lpm-capable" according to this flag, why not let common
>> code xhci-plat.c to get this property from sysdev which is DT
>> nodes for dwc3?
>> 
>
> Felipe, any comments?

Won't work. We have quirk flags which are based on DWC3's revision which
is not accessible by xhci-plat. Also, we can't call
device_add_property() because it's not really *adding*. It's *setting*,
meaning that we would loose all other properties.
Peter Chen Feb. 16, 2017, 1:23 a.m. UTC | #12
On Wed, Feb 15, 2017 at 01:51:31PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
> >>>>>>>> Why are we using sysdev to read DT property? We should be using the
> >>>>>>>> XHCI device (&pdev->dev) here, no?
> >>>>>>>
> >>>>>>> If I remember correctly, this is one of the cases where pdev does not
> >>>>>>> have a device node attached to it because it was created by the driver
> >>>>>>> of the parent device on the fly in case of dwc3. When you have a pure xhci
> >>>>>>> device in DT, the two pointers are the same.
> >>>>>>
> >>>>>> From drivers/usb/dwc3/host.c
> >>>>>>
> >>>>>>>         if (dwc->usb3_lpm_capable) {
> >>>>>>>                 props[0].name = "usb3-lpm-capable";
> >>>>>>>                 ret = platform_device_add_properties(xhci, props);
> >>>>>>>                 if (ret) {
> >>>>>>>                         dev_err(dwc->dev, "failed to add properties to xHCI\n");
> >>>>>>>                         goto err1;
> >>>>>>>                 }
> >>>>>>>         }
> >>>>>>
> >>>>>> So it is setting the usb3-lpm-capable property into the xhci platform device
> >>>>>> and we should be reading the property from there.
> >>>>
> >>>> Why dwc3 needs another "snps,usb3_lpm_capable"? Why not using
> >>>> "usb3-lpm-capable" at firmware directly?
> >>>
> >>> dwc3 is not setting "snps,usb3_lpm_capable" but "usb3-lpm-capable" for the
> >>> xhci platform device.
> >>>
> >>> What did you mean by firmware? Did you mean something like BIOS?
> >>> At least TI platforms don't use any firmware like BIOS. So dwc3 driver
> >>> needs to create a platform device for xhci on the fly and set the DT properties.
> >>>
> >> 
> >> By readying code, the dwc3 calls dwc3_get_properties to set
> >> dwc->usb3_lpm_capable, and at dwc3/host.c, it sets property
> >> "usb3-lpm-capable" according to this flag, why not let common
> >> code xhci-plat.c to get this property from sysdev which is DT
> >> nodes for dwc3?
> >> 
> >
> > Felipe, any comments?
> 
> Won't work. We have quirk flags which are based on DWC3's revision which
> is not accessible by xhci-plat. Also, we can't call
> device_add_property() because it's not really *adding*. It's *setting*,
> meaning that we would loose all other properties.
> 

Sorry, I am not clear by reading the code, here we just discuss
"usb3-lpm-capable" property from DT or other firmwares.
diff mbox

Patch

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index ba1853f4..032a702 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)
@@ -1686,7 +1686,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,
@@ -1758,7 +1758,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;
@@ -1831,7 +1831,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;
 
@@ -2373,7 +2373,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 6d33b42..4ecb3fd 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -14,6 +14,7 @@ 
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/module.h>
+#include <linux/pci.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/usb/phy.h>
@@ -148,6 +149,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;
@@ -164,22 +166,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 (!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);
 	if (!hcd)
 		return -ENOMEM;
 
@@ -222,20 +241,20 @@  static int xhci_plat_probe(struct platform_device *pdev)
 
 	xhci->clk = clk;
 	xhci->main_hcd = hcd;
-	xhci->shared_hcd = usb_create_shared_hcd(driver, &pdev->dev,
+	xhci->shared_hcd = __usb_create_hcd(driver, sysdev, &pdev->dev,
 			dev_name(&pdev->dev), hcd);
 	if (!xhci->shared_hcd) {
 		ret = -ENOMEM;
 		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 (device_property_read_bool(&pdev->dev, "quirk-broken-port-ped"))
 		xhci->quirks |= XHCI_BROKEN_PORT_PED;
 
-	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 6d6c460..ab0839c 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -237,6 +237,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);
@@ -263,7 +266,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 */
@@ -748,7 +751,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);
@@ -765,7 +768,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
@@ -4801,7 +4804,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 */