Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Pawel Laszczak <pawell@cadence.com>
To: Felipe Balbi <felipe.balbi@linux.intel.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Cc: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"hdegoede@redhat.com" <hdegoede@redhat.com>,
	"heikki.krogerus@linux.intel.com"
	<heikki.krogerus@linux.intel.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"rogerq@ti.com" <rogerq@ti.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"jbergsagel@ti.com" <jbergsagel@ti.com>,
	"nsekhar@ti.com" <nsekhar@ti.com>, "nm@ti.com" <nm@ti.com>,
	Suresh Punnoose <sureshp@cadence.com>,
	"peter.chen@nxp.com" <peter.chen@nxp.com>,
	Jayshri Dajiram Pawar <jpawar@cadence.com>,
	Rahul Kumar <kurahul@cadence.com>
Subject: RE: [PATCH v9 5/6] usb:cdns3 Add Cadence USB3 DRD Driver
Date: Sat, 10 Aug 2019 20:39:19 +0000
Message-ID: <BYAPR07MB4709B0A4FADFB76183D651DCDDD10@BYAPR07MB4709.namprd07.prod.outlook.com> (raw)
In-Reply-To: <8736idnu0q.fsf@gmail.com>

Hi,

>
>Pawel Laszczak <pawell@cadence.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

  reply index

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-05 10:57 [PATCH v9 0/6] Introduced new Cadence USBSS " Pawel Laszczak
2019-07-05 10:57 ` [PATCH v9 1/6] dt-bindings: add binding for USBSS-DRD controller Pawel Laszczak
2019-07-05 10:57 ` [PATCH v9 2/6] usb:gadget Separated decoding functions from dwc3 driver Pawel Laszczak
2019-07-05 11:27   ` Greg KH
2019-07-05 11:39     ` Pawel Laszczak
2019-07-05 11:49       ` Greg KH
2019-07-05 12:03         ` Pawel Laszczak
2019-07-05 11:39   ` Felipe Balbi
2019-07-05 11:44     ` Pawel Laszczak
2019-08-07 10:17       ` Roger Quadros
2019-08-07 10:22         ` Felipe Balbi
2019-08-08  3:07           ` Pawel Laszczak
2019-07-05 10:57 ` [PATCH v9 3/6] usb:gadget Patch simplify usb_decode_set_clear_feature function Pawel Laszczak
2019-07-05 10:57 ` [PATCH v9 4/6] usb:gadget Simplify usb_decode_get_set_descriptor function Pawel Laszczak
2019-07-05 10:57 ` [PATCH v9 5/6] usb:cdns3 Add Cadence USB3 DRD Driver Pawel Laszczak
2019-07-05 11:55   ` Felipe Balbi
2019-07-07 17:56     ` Pawel Laszczak
2019-07-08  6:29       ` Felipe Balbi
2019-07-08 10:59         ` Pawel Laszczak
2019-07-08 11:15           ` Felipe Balbi
2019-07-08 11:45             ` Pawel Laszczak
2019-07-08 11:59               ` Felipe Balbi
2019-07-08 12:39                 ` Pawel Laszczak
2019-07-09  4:23         ` Pawel Laszczak
2019-07-09  6:36           ` Felipe Balbi
2019-07-09  7:07             ` Pawel Laszczak
2019-07-09  7:22               ` Felipe Balbi
2019-07-09  7:29                 ` Pawel Laszczak
2019-07-10  8:25       ` Pawel Laszczak
2019-07-08  7:11   ` Felipe Balbi
2019-07-15 11:00     ` Pawel Laszczak
2019-08-07 10:34       ` Felipe Balbi
2019-08-10 20:39         ` Pawel Laszczak [this message]
2019-08-12  1:55           ` Peter Chen
2019-08-12  4:43             ` Pawel Laszczak
2019-08-12  5:24           ` Felipe Balbi
2019-08-12  7:13             ` Pawel Laszczak
2019-08-12  8:19               ` Felipe Balbi
2019-08-12  9:13                 ` Pawel Laszczak
2019-08-12  9:45                   ` Felipe Balbi
2019-08-12 10:26                     ` Pawel Laszczak
2019-08-12 10:34                       ` Felipe Balbi
2019-08-12 14:20                         ` Alan Stern
2019-07-05 10:57 ` [PATCH v9 6/6] usb:cdns3 Fix for stuck packets in on-chip OUT buffer Pawel Laszczak

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BYAPR07MB4709B0A4FADFB76183D651DCDDD10@BYAPR07MB4709.namprd07.prod.outlook.com \
    --to=pawell@cadence.com \
    --cc=devicetree@vger.kernel.org \
    --cc=felipe.balbi@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hdegoede@redhat.com \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=jbergsagel@ti.com \
    --cc=jpawar@cadence.com \
    --cc=kurahul@cadence.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=nm@ti.com \
    --cc=nsekhar@ti.com \
    --cc=peter.chen@nxp.com \
    --cc=robh+dt@kernel.org \
    --cc=rogerq@ti.com \
    --cc=sureshp@cadence.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/ public-inbox