Date:   Sat, 10 Aug 2019 20:39:19 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Felipe Balbi <felipe.balbi@...ux.intel.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "hdegoede@...hat.com" <hdegoede@...hat.com>,
        "heikki.krogerus@...ux.intel.com" <heikki.krogerus@...ux.intel.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "rogerq@...com" <rogerq@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jbergsagel@...com" <jbergsagel@...com>,
        "nsekhar@...com" <nsekhar@...com>, "nm@...com" <nm@...com>,
        Suresh Punnoose <sureshp@...ence.com>,
        "peter.chen@....com" <peter.chen@....com>,
        Jayshri Dajiram Pawar <jpawar@...ence.com>,
        Rahul Kumar <kurahul@...ence.com>
Subject: RE: [PATCH v9 5/6] usb:cdns3 Add Cadence USB3 DRD Driver

Hi,

>
>Pawel Laszczak <pawell@...ence.com> writes:
>>>> +static int cdns3_gadget_start(struct cdns3 *cdns)
>>>> +{
>>>> +	struct cdns3_device *priv_dev;
>>>> +	u32 max_speed;
>>>> +	int ret;
>>>> +
>>>> +	priv_dev = kzalloc(sizeof(*priv_dev), GFP_KERNEL);
>>>> +	if (!priv_dev)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	cdns->gadget_dev = priv_dev;
>>>> +	priv_dev->sysdev = cdns->dev;
>>>> +	priv_dev->dev = cdns->dev;
>>>> +	priv_dev->regs = cdns->dev_regs;
>>>> +
>>>> +	device_property_read_u16(priv_dev->dev, "cdns,on-chip-buff-size",
>>>> +				 &priv_dev->onchip_buffers);
>>>> +
>>>> +	if (priv_dev->onchip_buffers <=  0) {
>>>> +		u32 reg = readl(&priv_dev->regs->usb_cap2);
>>>> +
>>>> +		priv_dev->onchip_buffers = USB_CAP2_ACTUAL_MEM_SIZE(reg);
>>>> +	}
>>>> +
>>>> +	if (!priv_dev->onchip_buffers)
>>>> +		priv_dev->onchip_buffers = 256;
>>>> +
>>>> +	max_speed = usb_get_maximum_speed(cdns->dev);
>>>> +
>>>> +	/* Check the maximum_speed parameter */
>>>> +	switch (max_speed) {
>>>> +	case USB_SPEED_FULL:
>>>> +	case USB_SPEED_HIGH:
>>>> +	case USB_SPEED_SUPER:
>>>> +		break;
>>>> +	default:
>>>> +		dev_err(cdns->dev, "invalid maximum_speed parameter %d\n",
>>>> +			max_speed);
>>>> +		/* fall through */
>>>> +	case USB_SPEED_UNKNOWN:
>>>> +		/* default to superspeed */
>>>> +		max_speed = USB_SPEED_SUPER;
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	/* fill gadget fields */
>>>> +	priv_dev->gadget.max_speed = max_speed;
>>>> +	priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
>>>> +	priv_dev->gadget.ops = &cdns3_gadget_ops;
>>>> +	priv_dev->gadget.name = "usb-ss-gadget";
>>>> +	priv_dev->gadget.sg_supported = 1;
>>>> +
>>>> +	spin_lock_init(&priv_dev->lock);
>>>> +	INIT_WORK(&priv_dev->pending_status_wq,
>>>> +		  cdns3_pending_setup_status_handler);
>>>> +
>>>> +	/* initialize endpoint container */
>>>> +	INIT_LIST_HEAD(&priv_dev->gadget.ep_list);
>>>> +	INIT_LIST_HEAD(&priv_dev->aligned_buf_list);
>>>> +
>>>> +	ret = cdns3_init_eps(priv_dev);
>>>> +	if (ret) {
>>>> +		dev_err(priv_dev->dev, "Failed to create endpoints\n");
>>>> +		goto err1;
>>>> +	}
>>>> +
>>>> +	/* allocate memory for setup packet buffer */
>>>> +	priv_dev->setup_buf = dma_alloc_coherent(priv_dev->sysdev, 8,
>>>> +						 &priv_dev->setup_dma, GFP_DMA);
>>>> +	if (!priv_dev->setup_buf) {
>>>> +		ret = -ENOMEM;
>>>> +		goto err2;
>>>> +	}
>>>> +
>>>> +	priv_dev->dev_ver = readl(&priv_dev->regs->usb_cap6);
>>>> +
>>>> +	dev_dbg(priv_dev->dev, "Device Controller version: %08x\n",
>>>> +		readl(&priv_dev->regs->usb_cap6));
>>>> +	dev_dbg(priv_dev->dev, "USB Capabilities:: %08x\n",
>>>> +		readl(&priv_dev->regs->usb_cap1));
>>>> +	dev_dbg(priv_dev->dev, "On-Chip memory cnfiguration: %08x\n",
>>>> +		readl(&priv_dev->regs->usb_cap2));
>>>> +
>>>> +	priv_dev->dev_ver = GET_DEV_BASE_VERSION(priv_dev->dev_ver);
>>>> +
>>>> +	priv_dev->zlp_buf = kzalloc(CDNS3_EP_ZLP_BUF_SIZE, GFP_KERNEL);
>>>> +	if (!priv_dev->zlp_buf) {
>>>> +		ret = -ENOMEM;
>>>> +		goto err3;
>>>> +	}
>>>> +
>>>> +	/* add USB gadget device */
>>>> +	ret = usb_add_gadget_udc(priv_dev->dev, &priv_dev->gadget);
>>>> +	if (ret < 0) {
>>>> +		dev_err(priv_dev->dev,
>>>> +			"Failed to register USB device controller\n");
>>>> +		goto err4;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +err4:
>>>> +	kfree(priv_dev->zlp_buf);
>>>> +err3:
>>>> +	dma_free_coherent(priv_dev->sysdev, 8, priv_dev->setup_buf,
>>>> +			  priv_dev->setup_dma);
>>>> +err2:
>>>> +	cdns3_free_all_eps(priv_dev);
>>>> +err1:
>>>> +	cdns->gadget_dev = NULL;
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int __cdns3_gadget_init(struct cdns3 *cdns)
>>>> +{
>>>> +	struct cdns3_device *priv_dev;
>>>> +	int ret = 0;
>>>> +
>>>> +	cdns3_drd_switch_gadget(cdns, 1);
>>>> +	pm_runtime_get_sync(cdns->dev);
>>>> +
>>>> +	ret = cdns3_gadget_start(cdns);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	priv_dev = cdns->gadget_dev;
>>>> +	ret = devm_request_threaded_irq(cdns->dev, cdns->dev_irq,
>>>> +					cdns3_device_irq_handler,
>>>> +					cdns3_device_thread_irq_handler,
>>>> +					IRQF_SHARED, dev_name(cdns->dev), cdns);
>>>
>>>copied handlers here for commenting. Note that you don't have
>>>IRQF_ONESHOT:
>>
>> I know, I can't use  IRQF_ ONESHOT flag in this case. I have implemented
>> some code for masking/unmasking interrupts in cdns3_device_irq_handler.
>>
>> Some priority interrupts should be handled ASAP so I can't blocked interrupt
>> Line.
>
>You're completely missing my comment. Your top half should be as short
>as possile. It should only check if current device generated
>interrupts. If it did, then you should wake the thread handler.
>
>This is to improve realtime behavior but not keeping preemption disabled
>for longer than necessary.

Ok, I understand. I will move it to thread handler.

I can't use IRQF_ONESHOT flag because it doesn't work when interrupt line is shared. 
I have such situation in which one interrupt line is shared with ehci and cdns3 driver. 
In such case this function returns error code. 

So probably I will need to mask only the reported interrupts. 
I can't mask all interrupt using controller register because I can miss some of them. 
After masking all interrupt  the next new event will not be reported in  usb_ists, ep_ists 
registers.

>
>>>> +static irqreturn_t cdns3_device_irq_handler(int irq, void *data)
>>>> +{
>>>> +	struct cdns3_device *priv_dev;
>>>> +	struct cdns3 *cdns = data;
>>>> +	irqreturn_t ret = IRQ_NONE;
>>>> +	unsigned long flags;
>>>> +	u32 reg;
>>>> +
>>>> +	priv_dev = cdns->gadget_dev;
>>>> +	spin_lock_irqsave(&priv_dev->lock, flags);
>>>
>>>the top half handler runs in hardirq context. You don't need any locks
>>>here. Also IRQs are *already* disabled, you don't need to disable them again.
>>
>> I will remove spin_lock_irqsave but I need to disable only some of the interrupts.
>> I disable interrupts associated with USB endpoints. Handling of them can be
>> deferred to thread handled.
>
>you should defer all of them to thread. Endpoints or otherwise.

I will do this. 

Also I remove spin_lock_irqsave(&priv_dev->lock, flags); 
As I remember it's not needed here. 

>
>>>> +
>>>> +	/* check USB device interrupt */
>>>> +	reg = readl(&priv_dev->regs->usb_ists);
>>>> +
>>>> +	if (reg) {
>>>> +		writel(reg, &priv_dev->regs->usb_ists);
>>>> +		cdns3_check_usb_interrupt_proceed(priv_dev, reg);
>>>> +		ret = IRQ_HANDLED;
>>>
>>>now, because you _don't_ mask this interrupt, you're gonna have
>>>issues. Say we actually get both device and endpoint interrupts while
>>>the thread is already running with previous endpoint interrupts. Now
>>>we're gonna reenter the top half, because device interrupts are *not*
>>>masked, which will read usb_ists and handle it here.
>>
>> Endpoint interrupts are masked in cdns3_device_irq_handler and stay masked
>> until they are not handled in threaded handler.
>
>Quick question, then: these ISTS registers, are they masked interrupt
>status or raw interrupt status?

Yes it's masked, but after masking them the new interrupts will not be reported 
In ISTS registers. Form this reason I can mask only reported interrupt. 

>
>> Of course, not all endpoint interrupts are masked, but only reported in ep_ists.
>> USB interrupt will be handled immediately.
>>
>> Also, I can get next endpoint interrupt from not masked endpoint and driver also again wake
>> the thread. I saw such situation, but threaded interrupt handler has been working correct
>> in such situations.
>>
>> In thread handler driver checks again which endpoint should be handled in ep_ists.
>>
>> I think that such situation should also occurs during our LPM enter/exit test.
>> So, driver has  been tested for such case. During this test driver during
>> transferring data generate a huge number of LPM interrupts which
>> are usb interrupts.
>>
>> I can't block usb interrupts interrupts because:
>> /*
>>  * WORKAROUND: CDNS3 controller has issue with hardware resuming
>>  * from L1. To fix it, if any DMA transfer is pending driver
>>  * must starts driving resume signal immediately.
>>  */
>
>I can't see why this would prevent you from defering handling to thread
>handler.
>

I also will  try to move it, but this change can has impact on performance. 

>
>>>> +	if (priv_dev->run_garbage_colector) {
>>>
>>>wait, what?
>>
>> DMA require data buffer aligned to 8 bytes. So, if buffer data is not aligned
>> driver allocate aligned buffer for data and copy it from unaligned to
>> Aligned.
>>
>>>
>>>ps: correct spelling is "collector" ;-)
>>
>> Ok, thanks.
>>>
>>>> +		struct cdns3_aligned_buf *buf, *tmp;
>>>> +
>>>> +		list_for_each_entry_safe(buf, tmp, &priv_dev->aligned_buf_list,
>>>> +					 list) {
>>>> +			if (!buf->in_use) {
>>>> +				list_del(&buf->list);
>>>> +
>>>> +				spin_unlock_irqrestore(&priv_dev->lock, flags);
>>>
>>>creates the possibility of a race condition
>> Why? In this place the buf can't be used.
>
>but you're reenabling interrupts, right?

Yes, driver frees not used buffers here. 
I think that it's the safest place for this purpose. 

>
>>>> +				dma_free_coherent(priv_dev->sysdev, buf->size,
>>>> +						  buf->buf,
>>>> +						  buf->dma);
>>>> +				spin_lock_irqsave(&priv_dev->lock, flags);
>>>> +
>>>> +				kfree(buf);
>>>
>>>why do you even need this "garbage collector"?
>>
>> I need to free not used memory. The once allocated buffer will be associated with
>> request, but if request.length will be increased in usb_request then driver will
>> must allocate the  bigger buffer. As I remember I couldn't call dma_free_coherent
>> in interrupt context so I had to move it to thread handled. This flag was used to avoid
>> going through whole  aligned_buf_list  every time.
>> In most cases this part will never called int this place
>
>Did you try, btw, setting the quirk flag which tells gadget drivers to
>always allocate buffers aligned to MaxPacketSize? Wouldn't that be enough?

If found only  quirk_ep_out_aligned_size flag, but it align only buffer size. 

DMA used by this controller must have buffer address aligned to 8.
I think that on most architecture kmalloc should guarantee such aligned.
The problem was detected on NXP testing board.  
On my board all buffer address are alignment at least to 8.  

>
>>>> +	TP_printk("%s: req: %p, req buff %p, length: %u/%u %s%s%s, status: %d,"
>>>> +		cd   " trb: [start:%d, end:%d: virt addr %pa], flags:%x ",
>>>> +		__get_str(name), __entry->req, __entry->buf, __entry->actual,
>>>> +		__entry->length,
>>>> +		__entry->zero ? "zero | " : "",
>>>> +		__entry->short_not_ok ? "short | " : "",
>>>> +		__entry->no_interrupt ? "no int" : "",
>>>
>>>I guess you didn't really think the formatting through. Think about what
>>>happens if you get a request with only zero flag or only short flag. How
>>>will this log look like?
>>
>> Like this:
>> cdns3_gadget_giveback: ep0: req: 0000000071a6a5f5, req buff 000000008d40c4db, length: 60/60 zero | , status: 0, trb: [start:0, end:0:
>virt addr (null)], flags:0
>>
>> Is it something wrong with this?. Maybe one extra sign |.
>
>yes, the extra | :-)
>
>This is one reason why I switched to character flags where a lower case
>character means flag is cleared while uppercase means it's set.

I've made it in this way in v10

--

Thanks
Pawell

Powered by blists - more mailing lists