Hi,

Pawel Laszczak [off-list ref] writes:

quoted

quoted

+void cdns3_role_stop(struct cdns3 *cdns)

not static? Why is it so that _start() is static but _stop() is not?
Looks fishy

This function is used in cdns3_role_stop in debugfs.c file so it can't
be static.

it's still super fishy. Why don't you need _start() from debugfs.c? In
any case, the role framework would remove the need for any of this, I
suppose.

quoted

quoted

+static int cdns3_idle_role_start(struct cdns3 *cnds)
+{	/* Hold PHY in RESET */

huh?

quoted

+	return 0;
+}
+
+static void cdns3_idle_role_stop(struct cdns3 *cnds)
+{
+	/* Program Lane swap and bring PHY out of RESET */

double huh?

These functions were added for consistency with FSM described in controller specification. 
Yes, I know that you don't like empty functions :), but it could be helpful in
understanding how this controller work.

frankly, it really doesn't. An empty function doesn't really help IMHO.

quoted

quoted

+static const char *const cdns3_mode[] = {
+	[USB_DR_MODE_UNKNOWN]		= "unknown",
+	[USB_DR_MODE_OTG]		= "otg",
+	[USB_DR_MODE_HOST]		= "host",
+	[USB_DR_MODE_PERIPHERAL]	= "device",
+};

don't we have a generic version of this under usb/common ?

Yes, there is a similar array, but it is defined also as static 
and there is no function for converting value to string. 
There is only function for converting string to value.

right. You can add the missing interface generically instead of
duplicating the enumeration.

There is also:  
[USB_DR_MODE_UNKNOWN]		= "",
Instead of: 
[USB_DR_MODE_UNKNOWN]		= "unknown",

So, for USB_DR_MODE_UNKNOWN user will not see anything information.

But that's what "unknown" means :-) We don't know the information.

quoted

quoted

+static irqreturn_t cdns3_drd_irq(int irq, void *data)
+{
+	irqreturn_t ret = IRQ_NONE;
+	struct cdns3 *cdns = data;
+	u32 reg;
+
+	if (cdns->dr_mode != USB_DR_MODE_OTG)
+		return ret;
+
+	reg = readl(&cdns->otg_regs->ivect);
+
+	if (!reg)
+		return ret;
+
+	if (reg & OTGIEN_ID_CHANGE_INT) {
+		dev_dbg(cdns->dev, "OTG IRQ: new ID: %d\n",
+			cdns3_get_id(cdns));
+
+		ret = IRQ_HANDLED;
+	}
+
+	if (reg & (OTGIEN_VBUSVALID_RISE_INT | OTGIEN_VBUSVALID_FALL_INT)) {
+		dev_dbg(cdns->dev, "OTG IRQ: new VBUS: %d\n",
+			cdns3_get_vbus(cdns));
+
+		ret = IRQ_HANDLED;
+	}
+
+	if (ret == IRQ_HANDLED)
+		queue_work(system_freezable_wq, &cdns->role_switch_wq);
+
+	writel(~0, &cdns->otg_regs->ivect);
+	return ret;
+}

seems like you could use threaded irq to avoid this workqueue.

This function is also called in cdns3_mode_write (debugfs.c file),
therefor after changing it to threaded irq I will still need workqueue. 

Why? debugfs writes are not atomic. They run in process context, right?
Just don't disable interrupts while running this and you should be fine.

quoted

quoted

+	cdns->desired_dr_mode = cdns->dr_mode;
+	cdns->current_dr_mode = USB_DR_MODE_UNKNOWN;
+
+	ret = devm_request_threaded_irq(cdns->dev, cdns->otg_irq,
+					cdns3_drd_irq,
+					NULL, IRQF_SHARED,

if you don't have a threaded handler, you should set IRQF_ONESHOT. I
would prefer if you implement a threaded handler that doesn't require
IRQF_ONESHOT, though.

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?

quoted

quoted

+	} 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.

-- 
balbi