Re: [PATCH 3/3] usb: musb: fix setting JZ4740 gadget periphal mode on reset

[Felipe Balbi <balbi@ti.com>]

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

Hi,

On Tue, Dec 17, 2013 at 02:31:00AM +0100, Apelete Seketeli wrote:
> On 16-Dec-13, Felipe Balbi wrote:
> > On Sat, Dec 14, 2013 at 04:48:38AM +0100, Apelete Seketeli wrote:
> > > JZ4740 USB Device Controller is not OTG compatible and does not have DEVCTL
> > > register in silicon.
> > > 
> > > During ethernet-over-usb transactions, on reset, musb driver tries to
> > > read from DEVCTL and consequently sets device as host (A-Device)
> > > instead of peripheral (B-Device), which makes it a composite device to
> > > the USB gadget driver.
> > > This induces a kernel panic during power down where the USB gadget
> > > driver does a null pointer dereference when trying to access the
> > > composite device configuration.
> > > 
> > > On reset, do not rely on DEVCTL value for setting gadget peripheral
> > > mode: hardcode it instead to B-Device.
> > > 
> > > Signed-off-by: Apelete Seketeli <apelete@seketeli.net>
> > > ---
> > >  drivers/usb/musb/musb_gadget.c |    9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> > > index 32fb057..b4bea7a 100644
> > > --- a/drivers/usb/musb/musb_gadget.c
> > > +++ b/drivers/usb/musb/musb_gadget.c
> > > @@ -2119,6 +2119,14 @@ __acquires(musb->lock)
> > >  	/* Normal reset, as B-Device;
> > >  	 * or else after HNP, as A-Device
> > >  	 */
> > > +#if defined(CONFIG_USB_MUSB_JZ4740) || defined(CONFIG_USB_MUSB_JZ4740_MODULE)
> > 
> > NAK, no ifdefs in this driver. Pass a quirk flag through platform_data
> > or something similar.
> 
> I get that, makes sense to me, but problem is the driver has to read a
> valid value from DEVCTL hardware register when musb_g_reset() is
> called, and I do not see how this can be achieved through
> platform_data.

why not ?

	/*
	 * JZ4740 UDC Controller is not OTG compatible as does not
	 * have a DEVCTL register in silicon. Due to that, we must
	 * NOT rely on that register for setting peripheral mode.
	 */
	if (unlikely(musb->quirk_broken_devctl)) {
		musb->xceiv->state = OTG_STATE_B_PERIPHERAL;
		musb->g_is_a_peripheral = 0;
	else if (devctl & MUSB_DEVCTL_BDEVICE) {
		musb->xceiv->state = OTG_STATE_B_PERIPHERAL;
		musb->g_is_a_peripheral = 0;
	} else {
		musb->xceiv->state = OTG_STATE_A_PERIPHERAL;
		musb->g_is_a_peripheral = 1;
	}

> Is it ok to use ifdefs in musb_regs.h to add specific hardware
> register indexes for JZ4740 instead ?

you guys changed the register file ? Why ? Is my pain not enough
already ? :-p

> I am actually thinking about fooling the musb driver into reading a
> valid value from another register instead of DEVCTL.

which register would that be ? If the register file is different, we
need to find a way to support it, but you gotta fix a few other things
first before I look into that, I don't want to see any more hacks and
ifdeffery hell around this driver. It's already painful enough to
support all HW variants it already supports.

cheers

-- 
balbi

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