Hi Felipe,
I Just sent you the V3 PATCH, I think it should be good for your next
submission to Greg for 3.18-rc6, I guess it's too late for 3.18-rc5. I
really appreciate all your feedback from v1/v2.
Thanks
Ashwini
On Wed, Nov 12, 2014 at 12:35 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
