Re: [PATCHv2] usb: musb: Fix unbalanced platform_disable

[Bin Liu <b-liu-l0cyMroinI0@public.gmane.org>]

On Mon, Sep 12, 2016 at 08:05:30PM +0200, Andreas Kemnade wrote:
> On Mon, 12 Sep 2016 10:34:08 -0700
> Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [160912 10:26]:
> > > * Bin Liu <b-liu-l0cyMroinI0@public.gmane.org> [160912 09:55]:
> > > > Hi Tony,
> > > > 
> > > > On Mon, Sep 12, 2016 at 08:39:49AM -0700, Tony Lindgren wrote:
> > > > > Commit a83e17d0f73b ("usb: musb: Improve PM runtime and phy
> > > > > handling for 2430 glue layer") moved PHY enable/disable calls
> > > > > to happen from omap2430_musb_enable/disable(). That broke
> > > > > enumeration for several devices as PM runtime in the PHY will
> > > > > never enable it.
> > > > > 
> > > > > The root cause of the problem is unpaired calls from
> > > > > musb_core.c to musb_platform_enable/disable in musb_core.c as
> > > > > reported by Andreas Kemnade <andreas-cLv4Z9ELZ06ZuzBka8ofvg@public.gmane.org>.
> > > > > 
> > > > > As musb_platform_enable/disable are being called from various
> > > > > functions, let's not attempt to make them paiered immediately.
> > > > > This would require fixing all the callers like musb_remove.
> > > > > 
> > > > > Instead, let's first fix the regression in a minimal way by
> > > > > removing the initial call to musb_platform_disable.
> > > > > 
> > > > > AFAIK the initial musb_platform_disable call has always been
> > > > > just an attempted workaround for the 2430 glue layer announcing
> > > > > itself too early before the gadgets are configured. And that
> > > > > issue finally
> > > > 
> > > > Many glue layers rely on musb_platform_diable to disable
> > > > interrupts in musb_init_controller() before registering ISR, is
> > > > it safe to assume the interrupts will be masked when musb is
> > > > out-of-reset so that we don't have to call
> > > > musb_platform_disable() in musb_init_controller()?
> > > 
> > > It should be, we do request_irq only later on after this in
> > > musb_core.c. And the glue layers don't do request_irq except for
> > > the separate DMA interrupts in two cases.
> > > 
> > > And as the platform glue layer are the ones doing the probing, they
> > > should initialize things into sane state :)
> > > 
> > > We could add a call irq_set_status_flags(irq, IRQ_NOAUTOEN) before
> > > request_irq. But I'm guessing there's no need to.. Do you have some
> > > example in mind that should be tested?
> > 
> > Oh I see davinci_musb_disable and am35x_musb_disable reset devctl and
> > clear interrupts. I'll try to check on am3517 here today, don't have
> > any davinci boards configured here.
> > 
> Hmm, then the question is: Couldn't the X_musb_disable simply be called
> from X_probe if needed to be an the safe side?

In general, we try not to do so if all possible. We want to put common
code in the core, not repleat them in glue layers.

In this specific case, we cannot do it. For example in dsps glue, the
musb reset is done in dsps_musb_init(), so no place in dsps_probe() to
call dsps_musb_disable().

Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html