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: Tue, 9 Jul 2019 07:07:30 +0000
Message-ID: <BYAPR07MB47092C1E19FD36FB336A55C2DDF10@BYAPR07MB4709.namprd07.prod.outlook.com> (raw)
In-Reply-To: <87pnmj67ee.fsf@linux.intel.com>

>
>Hi,
>
>Pawel Laszczak <pawell@cadence.com> writes:
>>>> IRQF_ONESHOT can be used  only in threaded handled.
>>>> "
>>>>  * IRQF_ONESHOT - Interrupt is not reenabled after the hardirq handler finished.
>>>>  *                Used by threaded interrupts which need to keep the
>>>>  *                irq line disabled until the threaded handler has been run.
>>>> "
>>>
>>>so?
>>
>> I don't understand why If I don't have threaded handler why I need IRQF_ONESHOT.
>> Why interrupt cannot be reenabled after hardirq handler finished ?
>> I do not use threaded handler so this flag seem unnecessary.
>
>Unless this has changed over the years, it was a requirement from IRQ susbystem.
>
>	/*
>	 * Drivers are often written to work w/o knowledge about the
>	 * underlying irq chip implementation, so a request for a
>	 * threaded irq without a primary hard irq context handler
>	 * requires the ONESHOT flag to be set. Some irq chips like
>	 * MSI based interrupts are per se one shot safe. Check the
>	 * chip flags, so we can avoid the unmask dance at the end of
>	 * the threaded handler for those.
>	 */
>	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
>		new->flags &= ~IRQF_ONESHOT;

From description I understand that it should be set when driver uses only 
threaded handler without hard irq handler.
eg. 

	ret = devm_request_threaded_irq(dev, data->usb_id_irq,
					NULL, int3496_thread_isr,
					IRQF_SHARED | IRQF_ONESHOT |
					IRQF_TRIGGER_RISING |
					IRQF_TRIGGER_FALLING,
					dev_name(dev), data);

It make sense, we don't have hard irq handler so we can't clear source of interrupt. 
If we clear it immediately in interrupt controller then the same interrupt could 
be raised again, because it was not cleared e.g in controller register. 


>>>>>> +	} else {
>>>>>> +		struct usb_request *request;
>>>>>> +
>>>>>> +		if (priv_dev->eps[index]->flags & EP_WEDGE) {
>>>>>> +			cdns3_select_ep(priv_dev, 0x00);
>>>>>> +			return 0;
>>>>>> +		}
>>>>>> +
>>>>>> +		cdns3_dbg(priv_ep->cdns3_dev, "Clear Stalled endpoint %s\n",
>>>>>> +			  priv_ep->name);
>>>>>
>>>>>why do you need your own wrapper around dev_dbg()? This looks quite unnecessary.
>>>>
>>>> It's generic function used for adding message to trace.log.  It's not wrapper to dev_dbg
>>>>
>>>> void cdns3_dbg(struct cdns3_device *priv_dev, const char *fmt, ...)
>>>> {
>>>> 	struct va_format vaf;
>>>> 	va_list args;
>>>>
>>>> 	va_start(args, fmt);
>>>> 	vaf.fmt = fmt;
>>>> 	vaf.va = &args;
>>>> 	trace_cdns3_log(priv_dev, &vaf);
>>>> 	va_end(args);
>>>> }
>>>
>>>oh. Don't do it like that. Add a proper trace event that actually
>>>decodes the information you want. These random messages will give you
>>>trouble in the future. I had this sort of construct in dwc3 for a while
>>>and it became clear that it's a bad idea. It's best to have trace events
>>>that decode information coming from the HW. That way your trace logs
>>>have a "predictable" shape/format and you can easily find problem areas.
>>
>> Ok , I will change this.
>> I used such solution because I didn't want to create to many trace events.
>> I used it only for rely used messages.
>
>If you have these messages that are *really* needed, then you should add
>a trace event for it. Look at the events we have on dwc3 if you need
>some inspiration. Could also look at the history of our trace events to
>figure out how things changed over time.
>
>cheers
>
>--
>balbi

  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 [this message]
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
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=BYAPR07MB47092C1E19FD36FB336A55C2DDF10@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
	public-inbox-index linux-usb

Example config snippet for mirrors

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.git