This thread has been locked.

If you have a related question, please click the "Ask a related question" button in the top right corner. The newly created question will be automatically linked to this question.

Linux/AM3352: USB device unbind issue

Part Number: AM3352


Tool/software: Linux

Hello team,

I'm working on an am335x based board with linux kernel 4.9.69. I have two USB interfaces(USB0 and USB1) having a cell modem and a USB stick attached to them respectively.

# lsusb
Bus 002 Device 002: ID 0781:5567 SanDisk Corp. Cruzer Blade
Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 001 Device 002: ID 1bc7:0021 Telit HE910
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

 Now I try to unbind cell modem using the below command.

# echo musb-hdrc.0 > /sys/bus/platform/drivers/musb-hdrc/unbind

I could trace that the driver remove method (musb_remove() ) is called the the device unbind happens.

# lsusb
Bus 002 Device 002: ID 0781:5567 SanDisk Corp. Cruzer Blade
Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

However, the second device, a USB stick attached to USB1 interface, is no longer accessible. I'm unable to mount or use it. If I remove and reinsert the USB stick, it is detected but not properly enumerated(/dev/sd<x> entry is missing).

If I try to bind the device again, the system hangs and goes for a reboot.

Some points to note:

1. The behaviour is not observed with 4.1.18 kernel

2. Using softconnect works

    #echo 1 > /sys/kernel/debug/musb-hdrc.0/softconnect

     But I need to go with the bind-unbind method.

Is there some known issue around this? Can someone provide me on how to possibly debug this ?

Thanks and Regards,

Jayadev

    

  • Hi Jayadev,

    One way to debug this it to check the kernel git commits between v4.1.18 and v4.9.69 on musb_core.c, and pay attention on changes to musb_remove(), musb_disable(), and other musb teardown functions.

    You mentioned you have to use the bind/unbind method, but can use do so on the usb buses instead musb-hdrc?
    /sys/bus/usb/drivers/usb/

  • Hi Bin Liu,

    Thanks for the reply. I'll check the commits following 4.1.18. Unbinding the bus might not help me as I can see once I plug the device back, the bus is bind back again.

    Regards,

    Jayadev

  • Jayadev,

    I am unable to replicate the issue on AM335x GP EVM with Processor SDK v4.3.0.5, which has kernel v4.9.69.
    I attached a Sandisk USB drive and a USB-Ethernet dongle, unbind musb-hdrc driver one USB port doesn't affect any function on the other port.
    It seems your issue is not in kernel but somewhere else.
  • Hi Bin Liu,

    I was investigating the patches in the TI kernel and found that the attached patch introduces the issues in v4.9.21. It is due to a merge from 'connectivity-ti-linux-4.9.y' branch of 'git://git.ti.com/connectivity-integration-tree/connectivity-ti-linux-kernelinto ti-linux-4.9.y.

    Patch header:

    Subject: [PATCH] usb: musb: dsps: Manage CPPI 4.1 DMA interrupt in DSPS

    commit 255348289f714b96e0e95f9b702f435a638e6fd4 upstream.

    Despite the CPPI 4.1 is a generic DMA, it is tied to USB.
    On the DSPS, CPPI 4.1 interrupt's registers are in USBSS (the MUSB glue).
    Currently, to enable / disable and clear interrupts, the CPPI 4.1 driver maps and accesses to USBSS's register, which making CPPI 4.1 driver not really generic.
    Move the interrupt management to DSPS driver.

    Also could you specify the steps you used for unbinding the port. 

    From c1b32ac75ef78eb0fe22bc1e7f09755744f1c670 Mon Sep 17 00:00:00 2001
    From: Alexandre Bailon <abailon@baylibre.com>
    Date: Fri, 17 Mar 2017 19:41:57 +0530
    Subject: [PATCH] usb: musb: dsps: Manage CPPI 4.1 DMA interrupt in DSPS
    
    commit 255348289f714b96e0e95f9b702f435a638e6fd4 upstream.
    
    Despite the CPPI 4.1 is a generic DMA, it is tied to USB.
    On the DSPS, CPPI 4.1 interrupt's registers are in USBSS (the MUSB glue).
    Currently, to enable / disable and clear interrupts, the CPPI 4.1 driver
    maps and accesses to USBSS's register, which making CPPI 4.1 driver not
    really generic.
    Move the interrupt management to DSPS driver.
    
    Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
    Acked-by: Vinod Koul <vinod.koul@intel.com>
    Signed-off-by: Bin Liu <b-liu@ti.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Signed-off-by: Sekhar Nori <nsekhar@ti.com>
    ---
     drivers/dma/cppi41.c         | 28 ++++-----------
     drivers/usb/musb/musb_dsps.c | 81 ++++++++++++++++++++++++++++++++++++++++++--
     2 files changed, 86 insertions(+), 23 deletions(-)
    
    diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
    index 200828c..d74cee0 100644
    --- a/drivers/dma/cppi41.c
    +++ b/drivers/dma/cppi41.c
    @@ -79,14 +79,6 @@
     #define QMGR_QUEUE_C(n)	(0x2008 + (n) * 0x10)
     #define QMGR_QUEUE_D(n)	(0x200c + (n) * 0x10)
     
    -/* Glue layer specific */
    -/* USBSS  / USB AM335x */
    -#define USBSS_IRQ_STATUS	0x28
    -#define USBSS_IRQ_ENABLER	0x2c
    -#define USBSS_IRQ_CLEARR	0x30
    -
    -#define USBSS_IRQ_PD_COMP	(1 <<  2)
    -
     /* Packet Descriptor */
     #define PD2_ZERO_LENGTH		(1 << 19)
     
    @@ -294,14 +286,8 @@ static irqreturn_t cppi41_irq(int irq, void *data)
     {
     	struct cppi41_dd *cdd = data;
     	struct cppi41_channel *c;
    -	u32 status;
     	int i;
     
    -	status = cppi_readl(cdd->usbss_mem + USBSS_IRQ_STATUS);
    -	if (!(status & USBSS_IRQ_PD_COMP))
    -		return IRQ_NONE;
    -	cppi_writel(status, cdd->usbss_mem + USBSS_IRQ_STATUS);
    -
     	for (i = QMGR_PENDING_SLOT_Q(FIST_COMPLETION_QUEUE); i < QMGR_NUM_PEND;
     			i++) {
     		u32 val;
    @@ -618,6 +604,7 @@ static void cppi41_compute_td_desc(struct cppi41_desc *d)
     
     static int cppi41_tear_down_chan(struct cppi41_channel *c)
     {
    +	struct dmaengine_result abort_result;
     	struct cppi41_dd *cdd = c->cdd;
     	struct cppi41_desc *td;
     	u32 reg;
    @@ -701,6 +688,12 @@ static int cppi41_tear_down_chan(struct cppi41_channel *c)
     	c->td_seen = 0;
     	c->td_desc_seen = 0;
     	cppi_writel(0, c->gcr_reg);
    +
    +	/* Invoke the callback to do the necessary clean-up */
    +	abort_result.result = DMA_TRANS_ABORTED;
    +	dma_cookie_complete(&c->txd);
    +	dmaengine_desc_get_callback_invoke(&c->txd, &abort_result);
    +
     	return 0;
     }
     
    @@ -1066,8 +1059,6 @@ static int cppi41_dma_probe(struct platform_device *pdev)
     		goto err_irq;
     	}
     
    -	cppi_writel(USBSS_IRQ_PD_COMP, cdd->usbss_mem + USBSS_IRQ_ENABLER);
    -
     	ret = devm_request_irq(&pdev->dev, irq, glue_info->isr, IRQF_SHARED,
     			dev_name(dev), cdd);
     	if (ret)
    @@ -1091,7 +1082,6 @@ static int cppi41_dma_probe(struct platform_device *pdev)
     	dma_async_device_unregister(&cdd->ddev);
     err_dma_reg:
     err_irq:
    -	cppi_writel(0, cdd->usbss_mem + USBSS_IRQ_CLEARR);
     	cleanup_chans(cdd);
     err_chans:
     	deinit_cppi41(dev, cdd);
    @@ -1119,7 +1109,6 @@ static int cppi41_dma_remove(struct platform_device *pdev)
     	of_dma_controller_free(pdev->dev.of_node);
     	dma_async_device_unregister(&cdd->ddev);
     
    -	cppi_writel(0, cdd->usbss_mem + USBSS_IRQ_CLEARR);
     	devm_free_irq(&pdev->dev, cdd->irq, cdd);
     	cleanup_chans(cdd);
     	deinit_cppi41(&pdev->dev, cdd);
    @@ -1138,7 +1127,6 @@ static int __maybe_unused cppi41_suspend(struct device *dev)
     	struct cppi41_dd *cdd = dev_get_drvdata(dev);
     
     	cdd->dma_tdfdq = cppi_readl(cdd->ctrl_mem + DMA_TDFDQ);
    -	cppi_writel(0, cdd->usbss_mem + USBSS_IRQ_CLEARR);
     	disable_sched(cdd);
     
     	return 0;
    @@ -1164,8 +1152,6 @@ static int __maybe_unused cppi41_resume(struct device *dev)
     	cppi_writel(QMGR_SCRATCH_SIZE, cdd->qmgr_mem + QMGR_LRAM_SIZE);
     	cppi_writel(0, cdd->qmgr_mem + QMGR_LRAM1_BASE);
     
    -	cppi_writel(USBSS_IRQ_PD_COMP, cdd->usbss_mem + USBSS_IRQ_ENABLER);
    -
     	return 0;
     }
     
    diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
    index c171a0f1..7c047c4 100644
    --- a/drivers/usb/musb/musb_dsps.c
    +++ b/drivers/usb/musb/musb_dsps.c
    @@ -122,6 +122,7 @@ struct dsps_glue {
     	struct timer_list timer;	/* otg_workaround timer */
     	unsigned long last_timer;    /* last timer data for each instance */
     	bool sw_babble_enabled;
    +	void __iomem *usbss_base;
     
     	struct dsps_context context;
     	struct debugfs_regset32 regset;
    @@ -169,6 +170,13 @@ static void dsps_mod_timer_optional(struct dsps_glue *glue)
     	dsps_mod_timer(glue, -1);
     }
     
    +/* USBSS  / USB AM335x */
    +#define USBSS_IRQ_STATUS	0x28
    +#define USBSS_IRQ_ENABLER	0x2c
    +#define USBSS_IRQ_CLEARR	0x30
    +
    +#define USBSS_IRQ_PD_COMP	(1 << 2)
    +
     /**
      * dsps_musb_enable - enable interrupts
      */
    @@ -641,14 +649,76 @@ static void dsps_read_fifo32(struct musb_hw_ep *hw_ep, u16 len, u8 *dst)
     	}
     }
     
    +#ifdef CONFIG_USB_TI_CPPI41_DMA
    +static void dsps_dma_controller_callback(struct dma_controller *c)
    +{
    +	struct musb *musb = c->musb;
    +	struct dsps_glue *glue = dev_get_drvdata(musb->controller->parent);
    +	void __iomem *usbss_base = glue->usbss_base;
    +	u32 status;
    +
    +	status = musb_readl(usbss_base, USBSS_IRQ_STATUS);
    +	if (status & USBSS_IRQ_PD_COMP)
    +		musb_writel(usbss_base, USBSS_IRQ_STATUS, USBSS_IRQ_PD_COMP);
    +}
    +
    +static struct dma_controller *
    +dsps_dma_controller_create(struct musb *musb, void __iomem *base)
    +{
    +	struct dma_controller *controller;
    +	struct dsps_glue *glue = dev_get_drvdata(musb->controller->parent);
    +	void __iomem *usbss_base = glue->usbss_base;
    +
    +	controller = cppi41_dma_controller_create(musb, base);
    +	if (IS_ERR_OR_NULL(controller))
    +		return controller;
    +
    +	musb_writel(usbss_base, USBSS_IRQ_ENABLER, USBSS_IRQ_PD_COMP);
    +	controller->dma_callback = dsps_dma_controller_callback;
    +
    +	return controller;
    +}
    +
    +static void dsps_dma_controller_destroy(struct dma_controller *c)
    +{
    +	struct musb *musb = c->musb;
    +	struct dsps_glue *glue = dev_get_drvdata(musb->controller->parent);
    +	void __iomem *usbss_base = glue->usbss_base;
    +
    +	musb_writel(usbss_base, USBSS_IRQ_CLEARR, USBSS_IRQ_PD_COMP);
    +	cppi41_dma_controller_destroy(c);
    +}
    +
    +#ifdef CONFIG_PM_SLEEP
    +static void dsps_dma_controller_suspend(struct dsps_glue *glue)
    +{
    +	void __iomem *usbss_base = glue->usbss_base;
    +
    +	musb_writel(usbss_base, USBSS_IRQ_CLEARR, USBSS_IRQ_PD_COMP);
    +}
    +
    +static void dsps_dma_controller_resume(struct dsps_glue *glue)
    +{
    +	void __iomem *usbss_base = glue->usbss_base;
    +
    +	musb_writel(usbss_base, USBSS_IRQ_ENABLER, USBSS_IRQ_PD_COMP);
    +}
    +#endif
    +#else /* CONFIG_USB_TI_CPPI41_DMA */
    +#ifdef CONFIG_PM_SLEEP
    +static void dsps_dma_controller_suspend(struct dsps_glue *glue) {}
    +static void dsps_dma_controller_resume(struct dsps_glue *glue) {}
    +#endif
    +#endif /* CONFIG_USB_TI_CPPI41_DMA */
    +
     static struct musb_platform_ops dsps_ops = {
     	.quirks		= MUSB_DMA_CPPI41 | MUSB_INDEXED_EP,
     	.init		= dsps_musb_init,
     	.exit		= dsps_musb_exit,
     
     #ifdef CONFIG_USB_TI_CPPI41_DMA
    -	.dma_init	= cppi41_dma_controller_create,
    -	.dma_exit	= cppi41_dma_controller_destroy,
    +	.dma_init	= dsps_dma_controller_create,
    +	.dma_exit	= dsps_dma_controller_destroy,
     #endif
     	.enable		= dsps_musb_enable,
     	.disable	= dsps_musb_disable,
    @@ -856,6 +926,9 @@ static int dsps_probe(struct platform_device *pdev)
     
     	glue->dev = &pdev->dev;
     	glue->wrp = wrp;
    +	glue->usbss_base = of_iomap(pdev->dev.parent->of_node, 0);
    +	if (!glue->usbss_base)
    +		return -ENXIO;
     
     	if (usb_get_dr_mode(&pdev->dev) == USB_DR_MODE_PERIPHERAL) {
     		ret = dsps_setup_optional_vbus_irq(pdev, glue);
    @@ -950,6 +1023,8 @@ static int dsps_suspend(struct device *dev)
     	glue->context.tx_mode = musb_readl(mbase, wrp->tx_mode);
     	glue->context.rx_mode = musb_readl(mbase, wrp->rx_mode);
     
    +	dsps_dma_controller_suspend(glue);
    +
     	return 0;
     }
     
    @@ -963,6 +1038,8 @@ static int dsps_resume(struct device *dev)
     	if (!musb)
     		return 0;
     
    +	dsps_dma_controller_resume(glue);
    +
     	mbase = musb->ctrl_base;
     	musb_writel(mbase, wrp->control, glue->context.control);
     	musb_writel(mbase, wrp->epintr_set, glue->context.epintr);
    -- 
    1.9.1
    
    

    Regards,

    Jayadev

  • Hi Jayadev,

    Sorry, my bad in my testing, I forgot I had disabled CPPI DMA in my uboot env, which caused me not seeing the problem.

    Now I can reproduce the issue. And the patch you pointed above is indeed the root cause. Please use the following temporary patch to work around the issue. I am working on the proper fix.

    diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
    index f6b526606ad1..9403e06bec29 100644
    --- a/drivers/usb/musb/musb_dsps.c
    +++ b/drivers/usb/musb/musb_dsps.c
    @@ -690,7 +690,6 @@ static void dsps_dma_controller_destroy(struct dma_controller *c)
            struct dsps_glue *glue = dev_get_drvdata(musb->controller->parent);
            void __iomem *usbss_base = glue->usbss_base;
     
    -       musb_writel(usbss_base, USBSS_IRQ_CLEARR, USBSS_IRQ_PD_COMP);
            cppi41_dma_controller_destroy(c);
     }
     
    
    
  • Jayadev,

    Can you please confirm if the patch above solves the issue or not?
  • Hi Bin Liu,

    Sorry about the late response, I had been OOO for some time. I didn't comment out those lines, but used the below change that seemed to work

    diff --git a/KERNEL/drivers/usb/musb/musb_dsps.c b/KERNEL/drivers/usb/musb/musb_dsps.c
    index 1ac3ca8..49eb116 100644
    --- a/KERNEL/drivers/usb/musb/musb_dsps.c
    +++ b/KERNEL/drivers/usb/musb/musb_dsps.c
    @@ -691,7 +691,7 @@ static void dsps_dma_controller_destroy(struct dma_controller *c)
    struct dsps_glue *glue = dev_get_drvdata(musb->controller->parent);
    void __iomem *usbss_base = glue->usbss_base;

    - musb_writel(usbss_base, USBSS_IRQ_CLEARR, USBSS_IRQ_PD_COMP);
    + musb_writel(usbss_base, USBSS_IRQ_CLEARR, 0);
    cppi41_dma_controller_destroy(c);
    }

    @@ -700,7 +700,7 @@ static void dsps_dma_controller_suspend(struct dsps_glue *glue)
    {
    void __iomem *usbss_base = glue->usbss_base;

    - musb_writel(usbss_base, USBSS_IRQ_CLEARR, USBSS_IRQ_PD_COMP);
    + musb_writel(usbss_base, USBSS_IRQ_CLEARR, 0);
    }

    static void dsps_dma_controller_resume(struct dsps_glue *glue)
    --
    1.9.1

    Regards,

    Jayadev

  • Jayadev,

    Writing 0 to IRQ_CLEAR register has no hw effect, so your patch is the same as mine.

    But you shouldn't need to change it in dsps_dma_controller_suspend(). Does the issue still happen if you don't change it?