lkml.org 
[lkml]   [2013]   [Oct]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] usb: gadget: Add UDC driver for Aeroflex Gaisler GRUSBDC
On 2013-10-01 16:19, Felipe Balbi wrote:
> Hi,
>
> On Tue, Oct 01, 2013 at 10:34:47AM +0200, Andreas Larsson wrote:
>>>> +/* #define VERBOSE_DEBUG */
>>>
>>> we don't want this, we want verbose debug to be selectable on Kconfig,
>>> which already is ;-)
>>
>> I was only aware of CONFIG_USB_GADGET_DEBUG leading to DEBUG being
>> defined, not that any Kconfig turned on VERBOSE_DEBUG. Where is this
>> happening?
>
> you're right there :-) My bad. Do you mind adding a patch which sets
> VERBOSE_DEBUG when building drivers/usb/gadget/ directory ?
> drivers/usb/dwc3/ has an example, if you need ;-)

Sure, I'll do that.

>
> Or I can patch that myself, if you prefer. works both ways.
>
>>>> +#include "gr_udc.h"
>>>> +
>>>> +#define DRIVER_NAME "gr_udc"
>>>> +#define DRIVER_DESC "Aeroflex Gaisler GRUSBDC USB Peripheral Controller"
>>>> +
>>>> +static const char driver_name[] = DRIVER_NAME;
>>>> +static const char driver_desc[] = DRIVER_DESC;
>>>> +
>>>> +#define gr_read32(x) (ioread32be((x)))
>>>> +#define gr_write32(x, v) (iowrite32be((v), (x)))
>>>> +
>>>> +/* USB speed and corresponding string calculated from status register value */
>>>> +#define GR_SPEED(status) \
>>>> + ((status & GR_STATUS_SP) ? USB_SPEED_FULL : USB_SPEED_HIGH)
>>>> +#define GR_SPEED_STR(status) usb_speed_string(GR_SPEED(status))
>>>> +
>>>> +/* Size of hardware buffer calculated from epctrl register value */
>>>> +#define GR_BUFFER_SIZE(epctrl) \
>>>> + ((((epctrl) & GR_EPCTRL_BUFSZ_MASK) >> GR_EPCTRL_BUFSZ_POS) * \
>>>> + GR_EPCTRL_BUFSZ_SCALER)
>>>> +
>>>> +/* ---------------------------------------------------------------------- */
>>>> +/* Debug printout functionality */
>>>> +
>>>> +static const char * const gr_modestring[] = {"control", "iso", "bulk", "int"};
>>>> +
>>>> +static const char *gr_ep0state_string(enum gr_ep0state state)
>>>> +{
>>>> + static const char *const names[] = {
>>>> + [GR_EP0_DISCONNECT] = "disconnect",
>>>> + [GR_EP0_SETUP] = "setup",
>>>> + [GR_EP0_IDATA] = "idata",
>>>> + [GR_EP0_ODATA] = "odata",
>>>> + [GR_EP0_ISTATUS] = "istatus",
>>>> + [GR_EP0_OSTATUS] = "ostatus",
>>>> + [GR_EP0_STALL] = "stall",
>>>> + [GR_EP0_SUSPEND] = "suspend",
>>>> + };
>>>> +
>>>> + if (state < 0 || state >= ARRAY_SIZE(names))
>>>> + return "UNKNOWN";
>>>> +
>>>> + return names[state];
>>>> +}
>>>> +
>>>> +#ifdef VERBOSE_DEBUG
>>>> +
>>>> +#define BPRINTF(buf, left, fmt, args...) \
>>>> + do { \
>>>> + int ret = snprintf(buf, left, fmt, ## args); \
>>>> + buf += ret; \
>>>> + left -= ret; \
>>>> + } while (0)
>>>
>>> nack, use dev_vdbg() instead.
>>>
>>>> +static void gr_dbgprint_request(const char *str, struct gr_ep *ep,
>>>> + struct gr_request *req)
>>>> +{
>>>> + char buffer[100];
>>>
>>> NAK^10000000
>>>
>>> use kernel facilities instead. printk() and all its friends already
>>> print to a ring buffer.
>>
>> Alright. The concern was that repeatedly calling printk for multiple
>> parts of the same message could lead to intermixing with other unrelated
>> printouts.
>
> hmm, there are two ways to look at this.
>
> a) we have KERN_CONT to continue printing messages
> b) you might prefer using debugfs and seq_puts() for dumping large(-ish)
> amounts of debugging data ;-)

I just found print_hex_dump_debug that takes care of everything
including dynamic debug support, so I'll use that.

>
>>>> +static void gr_finish_request(struct gr_ep *ep, struct gr_request *req,
>>>> + int status)
>>>> +{
>>>> + struct gr_udc *dev;
>>>> +
>>>> + list_del_init(&req->queue);
>>>> +
>>>> + if (likely(req->req.status == -EINPROGRESS))
>>>> + req->req.status = status;
>>>> + else
>>>> + status = req->req.status;
>>>> +
>>>> + dev = ep->dev;
>>>> + usb_gadget_unmap_request(&dev->gadget, &req->req, ep->is_in);
>>>> + gr_free_dma_desc_chain(dev, req);
>>>> +
>>>> + if (ep->is_in) /* For OUT, actual gets updated by the work handler */
>>>> + req->req.actual = req->req.length;
>>>> +
>>>> + if (!status) {
>>>> + if (ep->is_in)
>>>> + gr_dbgprint_request("SENT", ep, req);
>>>> + else
>>>> + gr_dbgprint_request("RECV", ep, req);
>>>> + }
>>>> +
>>>> + /* Prevent changes to ep->queue during callback */
>>>> + ep->callback = 1;
>>>> + if (req == dev->ep0reqo && !status) {
>>>> + if (req->setup)
>>>> + gr_ep0_setup(dev, req);
>>>> + else
>>>> + dev_err(dev->dev,
>>>> + "Unexpected non setup packet on ep0in\n");
>>>> + } else if (req->req.complete) {
>>>> + unsigned long flags;
>>>> +
>>>> + /* Complete should be called with irqs disabled */
>>>> + local_irq_save(flags);
>>>
>>> I guess it'd be better if you called this with spin_lock_irqsave()
>>> called before, then you can remove local_irq_save from here.
>>
>> That would increase the amount of time interrupts are disabled quite a
>> lot, so I would prefer not to.
>
> that's what every other UDC driver is doing. I don't think you need to
> worry about that. Can you run some benchmarks with both constructs just
> so I can have peace of mind ?

I'll look into this.

>
>>>> + spin_unlock(&dev->lock);
>>>> +
>>>> + req->req.complete(&ep->ep, &req->req);
>>>> +
>>>> + spin_lock(&dev->lock);
>>>> + local_irq_restore(flags);
>>>> + }
>>>> + ep->callback = 0;
>>>> +
>>>> + /* Catch up possible prevented ep handling during completion callback */
>>>> + if (!ep->stopped)
>>>> + schedule_work(&dev->work);
>>>
>>> this workqueue is awkward, what's up with that ?
>>
>> The reason for the scheduling here is that during the completion call
>> the handling of endpoint events needs to be stopped. This is
>> accomplished by the ep->callback flag. When that is done we might have
>> ep events that needs to be taken care of.
>>
>> The same situation arises after unhalting an endpoint further down. All
>> potential handling of that endpoint was on pause during halt, and thus
>> the work handler needs to be scheduled to catch up.
>
> not so sure. Other UDC drivers also support EP halt and they don't need
> the workqueue at all.
>
>>>> +/* Call with non-NULL dev to do a devm-allocation */
>>>> +static struct usb_request *__gr_alloc_request(struct device *dev,
>>>> + struct usb_ep *_ep,
>>>> + gfp_t gfp_flags)
>>>> +{
>>>> + struct gr_request *req;
>>>> +
>>>> + if (dev)
>>>> + req = devm_kzalloc(dev, sizeof(*req), gfp_flags);
>>>> + else
>>>> + req = kzalloc(sizeof(*req), gfp_flags);
>>>
>>> why would "dev" ever be NULL ?
>>
>> When the gadget allocates a request it will free it explicitely later
>> on. Thus there is no need for any devm allocation. Therefore, the calls
>> from the gadget to gr_alloc_request then calls this function with a NULL
>> argument so that non-devm allocation is done in that case.
>
> then couldn't you just stick with direct kzalloc() instead of trying to
> use devm_kzalloc() for allocating requests ?

Alright.

>
> That's the righ way to handle usb_request lifetime anyway; leave it to
> the gadget driver. If that gadget driver doesn't free the usb_requests
> it allocated, we want the memory leak as an indication of a buggy
> gadget driver.
>
>>>> + epctrl = gr_read32(&ep->regs->epctrl);
>>>> + if (halt) {
>>>> + /* Set HALT */
>>>> + gr_write32(&ep->regs->epctrl, epctrl | GR_EPCTRL_EH);
>>>> + ep->stopped = 1;
>>>> + if (wedge)
>>>> + ep->wedged = 1;
>>>> + } else {
>>>> + gr_write32(&ep->regs->epctrl, epctrl & ~GR_EPCTRL_EH);
>>>> + ep->stopped = 0;
>>>> + ep->wedged = 0;
>>>> +
>>>> + /* Things might have been queued up in the meantime */
>>>> + if (!ep->dma_start)
>>>> + gr_start_dma(ep);
>>>> +
>>>> + /* Ep handling might have been hindered during halt */
>>>> + schedule_work(&ep->dev->work);
>>
>> Here is the second place where we need to schedule work as mentioned
>> above.
>
> that's fine, but we still have other gadget drivers which don't take the
> route of a workqueue after unhalting the endpoint.
>
> If the endpoint is halted, why do you even have anything to process at
> all for this endpoint ? nothing should have been queued, right ?
>
> And if you did queue requests while EP was halted, you could just
> restart your EP queue right here, instead of scheduling a work_struct to
> do that for you.
>
>>>> + }
>>>> +
>>>> + return retval;
>>>> +}
>>>> +
>>>> +/* Must be called with dev->lock held */
>>>> +static inline void gr_set_ep0state(struct gr_udc *dev, enum gr_ep0state value)
>>>> +{
>>>> + if (dev->ep0state != value)
>>>> + VDBG("STATE: ep0state=%s\n",
>>>> + gr_ep0state_string(value));
>>>
>>> dev_vdbg()
>>>
>>>> + dev->ep0state = value;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Should only be called when endpoints can not generate interrupts.
>>>> + *
>>>> + * Must be called with dev->lock held.
>>>> + */
>>>> +static void gr_disable_interrupts_and_pullup(struct gr_udc *dev)
>>>> +{
>>>> + gr_write32(&dev->regs->control, 0);
>>>> + wmb(); /* Make sure that we do not deny one of our interrupts */
>>>> + dev->irq_enabled = 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Stop all device activity and disable data line pullup.
>>>> + *
>>>> + * Must be called with dev->lock held.
>>>> + */
>>>> +static void gr_stop_activity(struct gr_udc *dev)
>>>> +{
>>>> + struct gr_ep *ep;
>>>> +
>>>> + list_for_each_entry(ep, &dev->ep_list, ep_list)
>>>> + gr_ep_nuke(ep);
>>>> +
>>>> + gr_disable_interrupts_and_pullup(dev);
>>>> +
>>>> + gr_set_ep0state(dev, GR_EP0_DISCONNECT);
>>>> + usb_gadget_set_state(&dev->gadget, USB_STATE_ATTACHED);
>>>
>>> ATTACHED ??
>>
>> Maybe NOTATTACHED is clearer, even if it is the same state in all
>> respects.
>
> for the sake of being clear, yes :-)
>
>>>> +static irqreturn_t gr_irq(int irq, void *_dev)
>>>> +{
>>>> + struct gr_udc *dev = _dev;
>>>> +
>>>> + if (!dev->irq_enabled)
>>>> + return IRQ_NONE;
>>>> +
>>>> + schedule_work(&dev->work);
>>>
>>> why do you need this ? We have threaded IRQ handlers. Why a workqueue ?
>>
>> As mentioned above, to to be able to schedule work after pausing
>> endpoint handling during a completion callback call or during an
>> endpoint halt.
>
> doesn't look like you need that work_struct at all. Handle your IRQ
> directly and for the pieces you need to do after ClearHalt, re-factor
> that to a separate function which you call conditionally on
> ->set_halt().

OK, I'll look into this for v2.

Cheers,
Andreas



\
 
 \ /
  Last update: 2013-10-02 11:21    [W:0.127 / U:0.628 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site