Re: [PATCH v4 1/9] usb: dwc3: add dual-role support

From: Felipe Balbi <balbi@ti.com>
To: Roger Quadros <rogerq@ti.com>
Cc: <balbi@ti.com>, <tony@atomide.com>, <Joao.Pinto@synopsys.com>,
	<sergei.shtylyov@cogentembedded.com>, <peter.chen@freescale.com>,
	<jun.li@freescale.com>, <linux-usb@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-omap@vger.kernel.org>
Subject: Re: [PATCH v4 1/9] usb: dwc3: add dual-role support
Date: Thu, 3 Sep 2015 10:44:44 -0500	[thread overview]
Message-ID: <20150903154444.GA4031@saruman.tx.rr.com> (raw)
In-Reply-To: <55E83B5C.7050001@ti.com>

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

Hi,

On Thu, Sep 03, 2015 at 03:21:48PM +0300, Roger Quadros wrote:
> >> +	dwc->fsm->id = id;
> >> +	dwc->fsm->b_sess_vld = vbus;
> >> +	usb_otg_sync_inputs(dwc->fsm);
> >> +}
> >> +
> >> +static int dwc3_drd_start_host(struct otg_fsm *fsm, int on)
> >> +{
> >> +	struct device *dev = usb_otg_fsm_to_dev(fsm);
> >> +	struct dwc3 *dwc = dev_get_drvdata(dev);
> > 
> > how about adding a usb_otg_get_drvdata(fsm) ?
> 
> You meant for otg core? That can be done.

yeah. BTW, I think otg core needs quite a few changes to become actually
useful. Currently it's just too much pointer ping-pong going back and
forth between phy, otg core, udc and hcd.

Also, I caught a ton of issues with it and suspend/resume. You might
want to fix them before adding more users to it.

It's also rather racy and that needs fixing too. On top of all that, I
think there's too much being added to UDC just to get Dual-Role, let's
see if we can improve that too.

> >> @@ -843,6 +998,16 @@ static int dwc3_probe(struct platform_device *pdev)
> >>  	hird_threshold = 12;
> >>  
> >>  	if (node) {
> >> +		if (of_property_read_bool(node, "extcon"))
> >> +			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
> >> +		else if (of_property_read_bool(dev->parent->of_node, "extcon"))
> >> +			dwc->edev = extcon_get_edev_by_phandle(dev->parent, 0);
> > 
> > why do you need to check the parent ? Why isn't that done on the glue
> > layer ?
> 
> On DRA7-evm, the extcon device is defined in the glue layer node. But
> we need the device both at the glue layer and at the core layer.

why do you need extcon here ? Glue updates core via UTMI about the
states, right ? So you should get proper VBUS and ID status via OTG IRQ.
Is that not working ?

> We do get the extcon device in dwc3-omap.c
> 
> Any suggestion how to pass the extcon device from glue layer to core.c?
> Or should I add the extcon property to dwc3 USB node as well in the DT?

GPIO toggles
  dwc3-omap extcon event
    update status via UTMI STATUS register
      OTG IRQ on core
        Horray!

:-)

> >> +
> >> +		if (IS_ERR(dwc->edev)) {
> >> +			dev_vdbg(dev, "couldn't get extcon device\n");
> > 
> > dev_err() ??
> 
> Is it ok to print it even in EPROBE_DEFER case?

hmm, probably pointless, indeed.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]