Hi,
On Wed, Nov 12, 2014 at 12:24:11PM -0700, Ashwini Pahuja wrote:
> On Wed, Nov 5, 2014 at 12:18 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> > On Fri, Oct 31, 2014 at 01:14:33PM -0700, Ashwini Pahuja wrote:
> >> This patch adds a UDC driver for Broadcom's USB3.0 Peripheral core
> >> named BDC. BDC is capable of supporting all transfer types control,
> >> bulk, Int and isoch on each endpoint.
> >>
> >> Signed-off-by: Ashwini Pahuja <ashwini.linux@xxxxxxxxx>
> >> ---
> >> Changes for v2:
> >> - Includes all the feedback provided by Felipe on the v1.
> >> - Removed unnecessary out of memory messages.
> >> - Added usb_gadget_giveback_request.
> >> - udc_stop: removed unnecessary driver argument.
> >> - Removed unused defines.
> >> - Renamed upsc to uspc.
> >
> > which platform uses this ? Is the platform working in mainline today ?
>
> Currently our "brcmstb" platform in arch/arm/mach-bcm/brcmstb.c, which
> is in mainline and which supports (to some degree) BCM7439 does have
> this HW block. BDC is a generic IP that will go inside various kinds
> of application and there is no dependency on one particular chip. BDC
> driver is developed/tested on our FPGA-PCIe based platform that
> connects to x86 machine. BDC IP is also USB-IF certified, The 3.6
> kernel version of this driver was used during USB-IF certification in
> the Mass storage mode.
>
> > Can you show me boot up logs and logs of this driver working with g_zero
> > and g_mass_storage ? Also make sure to run USB20CV chapter 9 with both
> > g_zero and mass_storage and post the logs somewhere I can access, or
> > just attach everything to this mail as a tarball or something.
> >
> Sure, I always run the USB30CV, USB20CV before sending out the code.
> Also testusb has been running for several days. I am attaching the
> logs with CONFIG_USB_GADGET_DEBUG enabled. Let me know if you need the
> logs with CONFIG_USB_GADGET_VERBOSE as well.
cool, I see you've been testing on v3.16-rc6, which should be fine, a
little bit on the old side, but still pretty recent :-)
[snip]
> >> + gbdi = 0;
> >> + dev_vdbg(bdc->dev,
> >> + "Dump bd list for %s epnum:%d\n",
> >> + ep->name, ep->ep_num);
> >> +
> >> + dev_vdbg(bdc->dev,
> >> + "tabs:%d max_bdi:%d eqp_bdi:%d hwd_bdi:%d num_bds_table:%d\n",
> >> + bd_list->num_tabs, bd_list->max_bdi, bd_list->eqp_bdi,
> >> + bd_list->hwd_bdi, bd_list->num_bds_table);
> >> +
> >> + for (tbi = 0; tbi < bd_list->num_tabs; tbi++) {
> >> + bd_table = bd_list->bd_table_array[tbi];
> >> + for (bdi = 0; bdi < bd_list->num_bds_table; bdi++) {
> >> + bd = bd_table->start_bd + bdi;
> >> + dma = bd_table->dma + (sizeof(struct bdc_bd) * bdi);
> >> + dev_vdbg(bdc->dev,
> >> + "tbi:%2d bdi:%2d gbdi:%2d virt:%p phys:%llx %08x %08x %08x %08x\n",
> >> + tbi, bdi, gbdi++, bd, (unsigned long long)dma,
> >> + le32_to_cpu(bd->offset[0]),
> >> + le32_to_cpu(bd->offset[1]),
> >> + le32_to_cpu(bd->offset[2]),
> >> + le32_to_cpu(bd->offset[3]));
> >> + }
> >> + dev_vdbg(bdc->dev, "\n\n");
> >> + }
> >> +}
> >
> > all of these debugging features should be done either using tracepoints
> > or debugfs. At a minimum you should stub them out when building without
> > debug to avoid the extra overhead.
> >
> OK, I will stub these debug functions in v3. I will consider adding
> support for tracepoints/debugfs in near future.
cool, thanks.
> >> diff --git a/drivers/usb/gadget/udc/bdc/bdc_ep.c b/drivers/usb/gadget/udc/bdc/bdc_ep.c
> >> new file mode 100644
> >> index 0000000..f5adcb4
> >> --- /dev/null
> >> +++ b/drivers/usb/gadget/udc/bdc/bdc_ep.c
> >> @@ -0,0 +1,2019 @@
> >> +/*
> >> + * bdc_ep.c - BRCM BDC USB3.0 device controller endpoint related functions
> >> + *
> >> + * Copyright (C) 2014 Broadcom Corporation
> >> + *
> >> + * Author: Ashwini Pahuja
> >> + *
> >> + * Based on drivers under drivers/usb/
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify it
> >> + * under the terms of the GNU General Public License as published by the
> >> + * Free Software Foundation; either version 2 of the License, or (at your
> >> + * option) any later version.
> >> + *
> >> + */
> >> +#include <linux/module.h>
> >> +#include <linux/pci.h>
> >> +#include <linux/dma-mapping.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/ioport.h>
> >> +#include <linux/sched.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/init.h>
> >> +#include <linux/timer.h>
> >> +#include <linux/list.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/moduleparam.h>
> >> +#include <linux/device.h>
> >> +#include <linux/usb/ch9.h>
> >> +#include <linux/usb/gadget.h>
> >> +#include <linux/usb/otg.h>
> >> +#include <linux/pm.h>
> >> +#include <linux/io.h>
> >> +#include <linux/irq.h>
> >> +#include <asm/unaligned.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/usb/composite.h>
> >> +
> >> +#include "bdc.h"
> >> +#include "bdc_ep.h"
> >> +#include "bdc_cmd.h"
> >> +#include "bdc_dbg.h"
> >> +
> >> +static const char * const ep0_state_string[] = {
> >
> > Shouldn't this be:
> >
> > static const char const * ep0_state_string
> >
> > instead ?
> >
>
> why? We don’t want to modify where the pointer is pointing to and also
> the string pointed by the pointer, so const char * const is correct.
I was under the impression C convention was to put const before the
type being "constified", maybe GCC accepts both, dunno.
> >> +static void ep_bd_list_free(struct bdc_ep *ep, u32 num_tabs)
> >> +{
> >> + struct bd_list *bd_list = &ep->bd_list;
> >> + struct bdc *bdc = ep->bdc;
> >> + struct bd_table *bd_table;
> >> + int index;
> >> +
> >> + dev_dbg(bdc->dev, "%s ep:%s num_tabs:%d\n",
> >> + __func__, ep->name, num_tabs);
> >> +
> >> + if (!bd_list->bd_table_array) {
> >> + dev_dbg(bdc->dev, "%s already freed\n", ep->name);
> >> + return;
> >> + }
> >> + for (index = 0; index < num_tabs; index++) {
> >> + /*
> >> + * check if the bd_table struct is allocated ?
> >> + * if yes, then check if bd memory has been allocated, then
> >> + * free the dma_pool and also the bd_table struct memory
> >> + */
> >> + bd_table = bd_list->bd_table_array[index];
> >> + dev_dbg(bdc->dev, "bd_table:%p index:%d\n", bd_table, index);
> >> + if (bd_table) {
> >
> > decrease the indentation here by shuffling things around:
> >
> > if (!bd_table) {
> > dev_dbg(bdc->dev, "bd_table not allocated\n");
> > continue;
> > }
> >
> > dma_pool_free(...);
> >
> > Go through the driver and modify it similarly on any other occurence.
> >
> OK thanks, fixed in v3.
alright, tks.
--
balbi
Attachment:
signature.asc
Description: Digital signature
