by Alexander Sverdlin

[permalink] [raw]

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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Sriram Dash <[email protected]>
> Tested-by: Baolin Wang <[email protected]>

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

Tested-by: Alexander Sverdlin <[email protected]>

> Cc: Felipe Balbi <[email protected]>
> Cc: Grygorii Strashko <[email protected]>
> Cc: Sinjan Kumar <[email protected]>
> Cc: David Fisher <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: "Thang Q. Nguyen" <[email protected]>
> Cc: Yoshihiro Shimoda <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Ming Lei <[email protected]>
> Cc: Jon Masters <[email protected]>
> Cc: Dann Frazier <[email protected]>
> Cc: Peter Chen <[email protected]>
> Cc: Leo Li <[email protected]>
> Tested-by: Brian Norris <[email protected]>
> ---
> 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.

2016-12-22 06:33:50

by Vivek Gautam

[permalink] [raw]

Subject: Re: [PATCH v5 0/6] inherit dma configuration from parent dev

On Thu, Nov 17, 2016 at 5:13 PM, Sriram Dash <[email protected]> wrote:
> 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.
>
> Arnd Bergmann (6):
> usb: separate out sysdev pointer from usb_bus
> usb: chipidea: use bus->sysdev for DMA configuration
> usb: ehci: fsl: use bus->sysdev for DMA configuration
> usb: xhci: use bus->sysdev for DMA configuration
> usb: dwc3: use bus->sysdev for DMA configuration
> usb: dwc3: Do not set dma coherent mask

Tested patches 1, 4 & 5 on db820c platform with required set of patches [1] for
phy.

Tested-by: Vivek Gautam <[email protected]>
for the above mentioned patches.

[1] https://lkml.org/lkml/2016/12/20/392

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project