Message-ID: <603b56ce-3d86-7639-977f-00730e5f21ec@nokia.com>
Date:   Tue, 13 Dec 2016 17:09:22 +0100
From:   Alexander Sverdlin <alexander.sverdlin@...ia.com>
To:     Sriram Dash <sriram.dash@....com>, <linux-kernel@...r.kernel.org>,
        <linux-usb@...r.kernel.org>
CC:     <mathias.nyman@...el.com>, <gregkh@...uxfoundation.org>,
        <suresh.gupta@....com>, <felipe.balbi@...ux.intel.com>,
        <stern@...land.harvard.edu>, <pku.leo@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Grygorii Strashko <grygorii.strashko@...com>,
        Sinjan Kumar <sinjank@...eaurora.org>,
        David Fisher <david.fisher1@...opsys.com>,
        "Catalin Marinas" <catalin.marinas@....com>,
        "Thang Q. Nguyen" <tqnguyen@....com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        "Ming Lei" <tom.leiming@...il.com>, Jon Masters <jcm@...hat.com>,
        Dann Frazier <dann.frazier@...onical.com>,
        Peter Chen <peter.chen@....com>
Subject: Re: [v5,1/6] usb: separate out sysdev pointer from usb_bus

Hi!

On 17/11/16 12:43, Sriram Dash wrote:
> From: Arnd Bergmann <arnd@...db.de>
> 
> For xhci-hcd platform device, all the DMA parameters are not
> configured properly, notably dma ops for dwc3 devices.
> 
> The idea here is that you pass in the parent of_node along with
> the child device pointer, so it would behave exactly like the
> parent already does. The difference is that it also handles all
> the other attributes besides the mask.
> 
> sysdev will represent the physical device, as seen from firmware
> or bus.Splitting the usb_bus->controller field into the
> Linux-internal device (used for the sysfs hierarchy, for printks
> and for power management) and a new pointer (used for DMA,
> DT enumeration and phy lookup) probably covers all that we really
> need.
> 
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> Signed-off-by: Sriram Dash <sriram.dash@....com>
> Tested-by: Baolin Wang <baolin.wang@...aro.org>

Successfully tested on arm64/axxia with DWC3 USB host, XHCIs properly inherit
DMA configuration. Therefore:

Tested-by: Alexander Sverdlin <alexander.sverdlin@...ia.com>

> Cc: Felipe Balbi <felipe.balbi@...ux.intel.com>
> Cc: Grygorii Strashko <grygorii.strashko@...com>
> Cc: Sinjan Kumar <sinjank@...eaurora.org>
> Cc: David Fisher <david.fisher1@...opsys.com>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: "Thang Q. Nguyen" <tqnguyen@....com>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
> Cc: Stephen Boyd <sboyd@...eaurora.org>
> Cc: Bjorn Andersson <bjorn.andersson@...aro.org>
> Cc: Ming Lei <tom.leiming@...il.com>
> Cc: Jon Masters <jcm@...hat.com>
> Cc: Dann Frazier <dann.frazier@...onical.com>
> Cc: Peter Chen <peter.chen@....com>
> Cc: Leo Li <pku.leo@...il.com>
> Tested-by: Brian Norris <briannorris@...omium.org>
> ---
> Changes in v5:
>   - No update
> 
> Changes in v4:
>   - No update
> 
> Changes in v3: 
>   - usb is_device_dma_capable instead of directly accessing 
>     dma props. 
>  
> Changes in v2: 
>   - Split the patch wrt driver
> 
>  drivers/usb/core/buffer.c | 12 ++++++------
>  drivers/usb/core/hcd.c    | 48 ++++++++++++++++++++++++++++-------------------
>  drivers/usb/core/usb.c    | 18 +++++++++---------
>  include/linux/usb.h       |  1 +
>  include/linux/usb/hcd.h   |  3 +++
>  5 files changed, 48 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> index 98e39f9..a6cd44a 100644
> --- a/drivers/usb/core/buffer.c
> +++ b/drivers/usb/core/buffer.c
> @@ -63,7 +63,7 @@ int hcd_buffer_create(struct usb_hcd *hcd)
>  	int		i, size;
>  
>  	if (!IS_ENABLED(CONFIG_HAS_DMA) ||
> -	    (!hcd->self.controller->dma_mask &&
> +	    (!is_device_dma_capable(hcd->self.sysdev) &&
>  	     !(hcd->driver->flags & HCD_LOCAL_MEM)))
>  		return 0;
>  
> @@ -72,7 +72,7 @@ int hcd_buffer_create(struct usb_hcd *hcd)
>  		if (!size)
>  			continue;
>  		snprintf(name, sizeof(name), "buffer-%d", size);
> -		hcd->pool[i] = dma_pool_create(name, hcd->self.controller,
> +		hcd->pool[i] = dma_pool_create(name, hcd->self.sysdev,
>  				size, size, 0);
>  		if (!hcd->pool[i]) {
>  			hcd_buffer_destroy(hcd);
> @@ -127,7 +127,7 @@ void *hcd_buffer_alloc(
>  
>  	/* some USB hosts just use PIO */
>  	if (!IS_ENABLED(CONFIG_HAS_DMA) ||
> -	    (!bus->controller->dma_mask &&
> +	    (!is_device_dma_capable(bus->sysdev) &&
>  	     !(hcd->driver->flags & HCD_LOCAL_MEM))) {
>  		*dma = ~(dma_addr_t) 0;
>  		return kmalloc(size, mem_flags);
> @@ -137,7 +137,7 @@ void *hcd_buffer_alloc(
>  		if (size <= pool_max[i])
>  			return dma_pool_alloc(hcd->pool[i], mem_flags, dma);
>  	}
> -	return dma_alloc_coherent(hcd->self.controller, size, dma, mem_flags);
> +	return dma_alloc_coherent(hcd->self.sysdev, size, dma, mem_flags);
>  }
>  
>  void hcd_buffer_free(
> @@ -154,7 +154,7 @@ void hcd_buffer_free(
>  		return;
>  
>  	if (!IS_ENABLED(CONFIG_HAS_DMA) ||
> -	    (!bus->controller->dma_mask &&
> +	    (!is_device_dma_capable(bus->sysdev) &&
>  	     !(hcd->driver->flags & HCD_LOCAL_MEM))) {
>  		kfree(addr);
>  		return;
> @@ -166,5 +166,5 @@ void hcd_buffer_free(
>  			return;
>  		}
>  	}
> -	dma_free_coherent(hcd->self.controller, size, addr, dma);
> +	dma_free_coherent(hcd->self.sysdev, size, addr, dma);
>  }
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 479e223..f8feb08 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1073,6 +1073,7 @@ static void usb_deregister_bus (struct usb_bus *bus)
>  static int register_root_hub(struct usb_hcd *hcd)
>  {
>  	struct device *parent_dev = hcd->self.controller;
> +	struct device *sysdev = hcd->self.sysdev;
>  	struct usb_device *usb_dev = hcd->self.root_hub;
>  	const int devnum = 1;
>  	int retval;
> @@ -1119,7 +1120,7 @@ static int register_root_hub(struct usb_hcd *hcd)
>  		/* Did the HC die before the root hub was registered? */
>  		if (HCD_DEAD(hcd))
>  			usb_hc_died (hcd);	/* This time clean up */
> -		usb_dev->dev.of_node = parent_dev->of_node;
> +		usb_dev->dev.of_node = sysdev->of_node;
>  	}
>  	mutex_unlock(&usb_bus_idr_lock);
>  
> @@ -1465,19 +1466,19 @@ void usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
>  	dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
>  	if (IS_ENABLED(CONFIG_HAS_DMA) &&
>  	    (urb->transfer_flags & URB_DMA_MAP_SG))
> -		dma_unmap_sg(hcd->self.controller,
> +		dma_unmap_sg(hcd->self.sysdev,
>  				urb->sg,
>  				urb->num_sgs,
>  				dir);
>  	else if (IS_ENABLED(CONFIG_HAS_DMA) &&
>  		 (urb->transfer_flags & URB_DMA_MAP_PAGE))
> -		dma_unmap_page(hcd->self.controller,
> +		dma_unmap_page(hcd->self.sysdev,
>  				urb->transfer_dma,
>  				urb->transfer_buffer_length,
>  				dir);
>  	else if (IS_ENABLED(CONFIG_HAS_DMA) &&
>  		 (urb->transfer_flags & URB_DMA_MAP_SINGLE))
> -		dma_unmap_single(hcd->self.controller,
> +		dma_unmap_single(hcd->self.sysdev,
>  				urb->transfer_dma,
>  				urb->transfer_buffer_length,
>  				dir);
> @@ -1520,11 +1521,11 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
>  			return ret;
>  		if (IS_ENABLED(CONFIG_HAS_DMA) && hcd->self.uses_dma) {
>  			urb->setup_dma = dma_map_single(
> -					hcd->self.controller,
> +					hcd->self.sysdev,
>  					urb->setup_packet,
>  					sizeof(struct usb_ctrlrequest),
>  					DMA_TO_DEVICE);
> -			if (dma_mapping_error(hcd->self.controller,
> +			if (dma_mapping_error(hcd->self.sysdev,
>  						urb->setup_dma))
>  				return -EAGAIN;
>  			urb->transfer_flags |= URB_SETUP_MAP_SINGLE;
> @@ -1555,7 +1556,7 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
>  				}
>  
>  				n = dma_map_sg(
> -						hcd->self.controller,
> +						hcd->self.sysdev,
>  						urb->sg,
>  						urb->num_sgs,
>  						dir);
> @@ -1570,12 +1571,12 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
>  			} else if (urb->sg) {
>  				struct scatterlist *sg = urb->sg;
>  				urb->transfer_dma = dma_map_page(
> -						hcd->self.controller,
> +						hcd->self.sysdev,
>  						sg_page(sg),
>  						sg->offset,
>  						urb->transfer_buffer_length,
>  						dir);
> -				if (dma_mapping_error(hcd->self.controller,
> +				if (dma_mapping_error(hcd->self.sysdev,
>  						urb->transfer_dma))
>  					ret = -EAGAIN;
>  				else
> @@ -1585,11 +1586,11 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
>  				ret = -EAGAIN;
>  			} else {
>  				urb->transfer_dma = dma_map_single(
> -						hcd->self.controller,
> +						hcd->self.sysdev,
>  						urb->transfer_buffer,
>  						urb->transfer_buffer_length,
>  						dir);
> -				if (dma_mapping_error(hcd->self.controller,
> +				if (dma_mapping_error(hcd->self.sysdev,
>  						urb->transfer_dma))
>  					ret = -EAGAIN;
>  				else
> @@ -2511,8 +2512,8 @@ static void init_giveback_urb_bh(struct giveback_urb_bh *bh)
>   * Return: On success, a pointer to the created and initialized HCD structure.
>   * On failure (e.g. if memory is unavailable), %NULL.
>   */
> -struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
> -		struct device *dev, const char *bus_name,
> +struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
> +		struct device *sysdev, struct device *dev, const char *bus_name,
>  		struct usb_hcd *primary_hcd)
>  {
>  	struct usb_hcd *hcd;
> @@ -2553,8 +2554,9 @@ struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
>  
>  	usb_bus_init(&hcd->self);
>  	hcd->self.controller = dev;
> +	hcd->self.sysdev = sysdev;
>  	hcd->self.bus_name = bus_name;
> -	hcd->self.uses_dma = (dev->dma_mask != NULL);
> +	hcd->self.uses_dma = (sysdev->dma_mask != NULL);
>  
>  	init_timer(&hcd->rh_timer);
>  	hcd->rh_timer.function = rh_timer_func;
> @@ -2569,6 +2571,14 @@ struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
>  			"USB Host Controller";
>  	return hcd;
>  }
> +EXPORT_SYMBOL_GPL(__usb_create_hcd);
> +
> +struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
> +		struct device *dev, const char *bus_name,
> +		struct usb_hcd *primary_hcd)
> +{
> +	return __usb_create_hcd(driver, dev, dev, bus_name, primary_hcd);
> +}
>  EXPORT_SYMBOL_GPL(usb_create_shared_hcd);
>  
>  /**
> @@ -2588,7 +2598,7 @@ EXPORT_SYMBOL_GPL(usb_create_shared_hcd);
>  struct usb_hcd *usb_create_hcd(const struct hc_driver *driver,
>  		struct device *dev, const char *bus_name)
>  {
> -	return usb_create_shared_hcd(driver, dev, bus_name, NULL);
> +	return __usb_create_hcd(driver, dev, dev, bus_name, NULL);
>  }
>  EXPORT_SYMBOL_GPL(usb_create_hcd);
>  
> @@ -2715,7 +2725,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	struct usb_device *rhdev;
>  
>  	if (IS_ENABLED(CONFIG_USB_PHY) && !hcd->usb_phy) {
> -		struct usb_phy *phy = usb_get_phy_dev(hcd->self.controller, 0);
> +		struct usb_phy *phy = usb_get_phy_dev(hcd->self.sysdev, 0);
>  
>  		if (IS_ERR(phy)) {
>  			retval = PTR_ERR(phy);
> @@ -2733,7 +2743,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	}
>  
>  	if (IS_ENABLED(CONFIG_GENERIC_PHY) && !hcd->phy) {
> -		struct phy *phy = phy_get(hcd->self.controller, "usb");
> +		struct phy *phy = phy_get(hcd->self.sysdev, "usb");
>  
>  		if (IS_ERR(phy)) {
>  			retval = PTR_ERR(phy);
> @@ -2781,7 +2791,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	 */
>  	retval = hcd_buffer_create(hcd);
>  	if (retval != 0) {
> -		dev_dbg(hcd->self.controller, "pool alloc failed\n");
> +		dev_dbg(hcd->self.sysdev, "pool alloc failed\n");
>  		goto err_create_buf;
>  	}
>  
> @@ -2791,7 +2801,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  
>  	rhdev = usb_alloc_dev(NULL, &hcd->self, 0);
>  	if (rhdev == NULL) {
> -		dev_err(hcd->self.controller, "unable to allocate root hub\n");
> +		dev_err(hcd->self.sysdev, "unable to allocate root hub\n");
>  		retval = -ENOMEM;
>  		goto err_allocate_root_hub;
>  	}
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 5921514..3abe83a 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -450,9 +450,9 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
>  	 * Note: calling dma_set_mask() on a USB device would set the
>  	 * mask for the entire HCD, so don't do that.
>  	 */
> -	dev->dev.dma_mask = bus->controller->dma_mask;
> -	dev->dev.dma_pfn_offset = bus->controller->dma_pfn_offset;
> -	set_dev_node(&dev->dev, dev_to_node(bus->controller));
> +	dev->dev.dma_mask = bus->sysdev->dma_mask;
> +	dev->dev.dma_pfn_offset = bus->sysdev->dma_pfn_offset;
> +	set_dev_node(&dev->dev, dev_to_node(bus->sysdev));
>  	dev->state = USB_STATE_ATTACHED;
>  	dev->lpm_disable_count = 1;
>  	atomic_set(&dev->urbnum, 0);
> @@ -800,7 +800,7 @@ struct urb *usb_buffer_map(struct urb *urb)
>  	if (!urb
>  			|| !urb->dev
>  			|| !(bus = urb->dev->bus)
> -			|| !(controller = bus->controller))
> +			|| !(controller = bus->sysdev))
>  		return NULL;
>  
>  	if (controller->dma_mask) {
> @@ -838,7 +838,7 @@ void usb_buffer_dmasync(struct urb *urb)
>  			|| !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)
>  			|| !urb->dev
>  			|| !(bus = urb->dev->bus)
> -			|| !(controller = bus->controller))
> +			|| !(controller = bus->sysdev))
>  		return;
>  
>  	if (controller->dma_mask) {
> @@ -872,7 +872,7 @@ void usb_buffer_unmap(struct urb *urb)
>  			|| !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)
>  			|| !urb->dev
>  			|| !(bus = urb->dev->bus)
> -			|| !(controller = bus->controller))
> +			|| !(controller = bus->sysdev))
>  		return;
>  
>  	if (controller->dma_mask) {
> @@ -922,7 +922,7 @@ int usb_buffer_map_sg(const struct usb_device *dev, int is_in,
>  
>  	if (!dev
>  			|| !(bus = dev->bus)
> -			|| !(controller = bus->controller)
> +			|| !(controller = bus->sysdev)
>  			|| !controller->dma_mask)
>  		return -EINVAL;
>  
> @@ -958,7 +958,7 @@ void usb_buffer_dmasync_sg(const struct usb_device *dev, int is_in,
>  
>  	if (!dev
>  			|| !(bus = dev->bus)
> -			|| !(controller = bus->controller)
> +			|| !(controller = bus->sysdev)
>  			|| !controller->dma_mask)
>  		return;
>  
> @@ -986,7 +986,7 @@ void usb_buffer_unmap_sg(const struct usb_device *dev, int is_in,
>  
>  	if (!dev
>  			|| !(bus = dev->bus)
> -			|| !(controller = bus->controller)
> +			|| !(controller = bus->sysdev)
>  			|| !controller->dma_mask)
>  		return;
>  
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index eba1f10..f3f5d8a 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -354,6 +354,7 @@ struct usb_devmap {
>   */
>  struct usb_bus {
>  	struct device *controller;	/* host/master side hardware */
> +	struct device *sysdev;		/* as seen from firmware or bus */
>  	int busnum;			/* Bus number (in order of reg) */
>  	const char *bus_name;		/* stable id (PCI slot_name etc) */
>  	u8 uses_dma;			/* Does the host controller use DMA? */
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 66fc137..3860560 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -437,6 +437,9 @@ extern int usb_hcd_alloc_bandwidth(struct usb_device *udev,
>  		struct usb_host_interface *new_alt);
>  extern int usb_hcd_get_frame_number(struct usb_device *udev);
>  
> +struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
> +		struct device *sysdev, struct device *dev, const char *bus_name,
> +		struct usb_hcd *primary_hcd);
>  extern struct usb_hcd *usb_create_hcd(const struct hc_driver *driver,
>  		struct device *dev, const char *bus_name);
>  extern struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
> 

-- 
Best regards,
Alexander Sverdlin.

Powered by blists - more mailing lists