From mboxrd@z Thu Jan 1 00:00:00 1970 From: Felipe Balbi Subject: Re: Fwd: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support Date: Tue, 8 Apr 2014 16:24:21 -0500 Message-ID: <20140408212421.GA1431@saruman.home> References: <1396510519-8555-1-git-send-email-sbhatta@xilinx.com> <113d0620-4003-417d-806b-0b79ae692829@VA3EHSMHS023.ehs.local> <20140403145853.GD14162@saruman.home> <20140407163538.GD20228@saruman.home> Reply-To: Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="gKMricLos+KVdGMg" Return-path: Content-Disposition: inline In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: sundeep subbaraya Cc: balbi-l0cyMroinI0@public.gmane.org, Subbaraya Sundeep Bhatta , Greg Kroah-Hartman , Michal Simek , "linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Subbaraya Sundeep Bhatta List-Id: devicetree@vger.kernel.org --gKMricLos+KVdGMg Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Tue, Apr 08, 2014 at 09:31:29PM +0530, sundeep subbaraya wrote: > >> >> +static const struct usb_gadget_ops xusb_udc_ops =3D { > >> >> + .get_frame =3D xudc_get_frame, > >> >> + .wakeup =3D xudc_wakeup, > >> >> + .udc_start =3D xudc_start, > >> >> + .udc_stop =3D xudc_stop, > >> > > >> > no pullup ??? What gives ? This HW doesn't support it ? really ? > >> > >> Yes, there is no pull up. writing to control register to start udc is > >> sufficient for this IP. > > > > right, but is there a bit inside control register which actually starts > > the IP ? You might want to move that to ->pullup(), see how we did on > > drivers/usb/dwc3/gadget.c::dwc3_gadget_pullup(); we're basically using > > that to control RUN/STOP bit in DCTL register. That bit has two > > functions: a) actually enable the ip; b) connect data pullups. > > > > You can actually see with a scope that the line goes high and low when > > you mess with that bit. > > > > The reason I suggest this is because you don't want to let your host see > > a connection until your gadget driver is registered and ready to go and > > that's what the pullup method would guarantee. >=20 > No.There are only two bits in Control register, one for Ready bit, anothe= r for > sending remote wakeup and remaining are reserved as zeroes. Until ready is > set host do not see the gadget. so this READY bit is what you want to toggle on pullup(). > >> >> + } > >> >> + if (intrstatus & XUSB_STATUS_SUSPEND_MASK) { > >> >> + > >> >> + DBG("Suspend\n"); > >> >> + > >> >> + /* Enable the reset and resume */ > >> >> + intrreg =3D udc->read_fn(udc->base_address + XUSB_IER= _OFFSET); > >> >> + intrreg |=3D XUSB_STATUS_RESET_MASK | XUSB_STATUS_RES= UME_MASK; > >> >> + udc->write_fn(udc->base_address, XUSB_IER_OFFSET, int= rreg); > >> >> + udc->usb_state =3D USB_STATE_SUSPENDED; > >> >> + > >> >> + if (udc->driver->suspend) > >> >> + udc->driver->suspend(&udc->gadget); > >> >> + } > >> > > >> > when are you going to call driver->resume() ?? > >> > >> When an interrupt occurs we first check if udc->usb_state =3D > >> USB_STATE_SUSPENDED, > >> if yes driver->resume is called. Also if Resume bit is set then it is > >> cleared too. Resume status bit is set > >> when device is resumed by host after device sends Remote wakeup > >> signalling to host. > > > > > > > >> >> +static irqreturn_t xudc_irq(int irq, void *_udc) > >> >> +{ > >> >> + struct xusb_udc *udc =3D _udc; > >> >> + u32 intrstatus; > >> >> + u32 intrreg; > >> >> + u8 index; > >> >> + u32 bufintr; > >> >> + > >> >> + spin_lock(&(udc->lock)); > >> >> + > >> >> + intrreg =3D udc->read_fn(udc->base_address + XUSB_IER_OFFSET); > >> >> + intrreg &=3D ~XUSB_STATUS_INTR_EVENT_MASK; > >> >> + udc->write_fn(udc->base_address, XUSB_IER_OFFSET, intrreg); > >> >> + > >> >> + /* Read the Interrupt Status Register.*/ > >> >> + intrstatus =3D udc->read_fn(udc->base_address + XUSB_STATUS_O= FFSET); > >> >> + > >> >> + if (udc->usb_state =3D=3D USB_STATE_SUSPENDED) { > >> >> + > >> >> + DBG("Resume\n"); > >> >> + > >> >> + if (intrstatus & XUSB_STATUS_RESUME_MASK) { > >> >> + /* Enable the reset and suspend */ > >> >> + intrreg =3D udc->read_fn(udc->base_address + > >> >> + XUSB_IER_OFFSET); > >> >> + intrreg |=3D XUSB_STATUS_RESET_MASK | > >> >> + XUSB_STATUS_SUSPEND_MASK; > >> >> + udc->write_fn(udc->base_address, XUSB_IER_OFF= SET, > >> >> + intrreg); > >> >> + } > >> >> + udc->usb_state =3D 0; > >> >> + > >> >> + if (udc->driver->resume) > >> >> + udc->driver->resume(&udc->gadget); > > > > this is wrong, note that you would call ->resume() *every time* > > usb_state =3D=3D SUSPENDED and there's an interrupt. This means that if > > gadget is suspended and you remove the cable, then you first call > > ->resume() and then ->disconnect(). > > > >> Here. calling driver->resume. > > > > Here's what I would do: > > > > if (intrstatus & XUSB_STATUS_RESUME_MASK) { > > bool condition =3D udc->usb_state !=3D USB_STATE_SUSPENDED; > > dev_WARN_ONCE(dev, condition, "Resume IRQ while not suspended\n= "); > > > > /* Enable the reset and suspend */ > > intrreg =3D udc->read_fn(udc->base_address + XUSB_IER_OFFSET); > > intrreg |=3D XUSB_STATUS_RESET_MASK | XUSB_STATUS_SUSPEND_MASK; > > udc->write_fn(udc->base_address, XUSB_IER_OFFSET, intrreg); > > > > if (udc->driver->resume) > > udc->driver_resume(&udc->gadget); > > } >=20 > Resume Interrupt bit is set only when Resume happens by device sending > Remote wakeup. so if the host drives resume signalling there will be no interrupt ? Well, this is wrong :-) The gadget driver needs to know about that too. The host side can decide to wake you up. > I am assuming we need to call driver->resume for every > driver->suspend. Hence I implemented no you don't, you need to let host wake you up or you drive remote wakeup on the bus. The thing is: if the bus is suspended, there will be *no* activity on the bus prior to host driving Resume signal or device driving Remote Wakeup signal. Enable RESUME IRQ on probe and never disable it, it'll be a lot easier to implement it. --=20 balbi --gKMricLos+KVdGMg Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJTRGkEAAoJEIaOsuA1yqREihkP/j/g/Ckd7TZRahGge2T4LKuP gm6rWwmh88h/jz0V9UKP2zf7SfhdDsiBmFLwn2C4i49ptAFzrzmDjYdpBMU9RmIp SQDhHMO6z4zyrTeNKPnjwDHQYtNUpm68taud5IHbWBWpQQceYFk1qfmMlo0Cd3y+ TZ3V5GTBfL3NEI8w4VC5ZnYSqpF0SaaeFh+kVJPGadqLFenQMq8E9r7cUreMmYl9 JkxwlPlD4HQOyUXSNZ8BkA8CzuGeM2t4k6Gm5L+yl3XF9F13tGivK7+lMkSA3DzJ NlLE8KVal+cOlBNJDP4Nusn+C8N2H3IuoWXCoPFKtbUDaQIDHuy8Z2DsbrHjWycO LK2tqZFABJ2lOt0a++ArQHelqc/axxFe6ORt2oldzKinD4yAeYRjky/4ik1i5/yb KjAkA0cNFrQMDE/TVFDr46ztZUiN2kskNRnwMm7G71Ir7Kgvc5TWkOG385pxHb9K rhvVmzqEBCKFUuuVHaUcZkn26i+Dv+lhXf3F/t2uE5Rw+s8nBLWTlXDxdu96JRVB p+rj+Vqtwk9CTFXKeML6vmsqfURti/3rxwruuB3tUi6AzEsR5W45Ui8T3cctyejB lMlKp/kCSuL/zQ+Whj9h0CoSTRyooe/PFGRlws0YD80CPsnbgQ7yF3uBd6fJ3Tl2 h+XibIFBd3jQFmsC/odU =JOis -----END PGP SIGNATURE----- --gKMricLos+KVdGMg-- -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html