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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
