On Tue, Oct 14, 2014 at 11:53 AM, Felipe Balbi <balbi@xxxxxx> wrote: > Hi, sorry for my late response, I was pulled into some other activity. I will be sending out v2 patch this week. > > On Tue, Oct 14, 2014 at 11:30:40AM -0700, Ashwini Pahuja wrote: > > <snip> > >> > > +/* Upstream port status change sr */ >> > > +void bdc_sr_upsc(struct bdc *, struct bdc_sr *); >> > > +/* transfer sr */ >> > > +void bdc_sr_xsf(struct bdc *, struct bdc_sr *); >> > > +/* command completion */ >> > > +void bdc_sr_cmd(struct bdc *, struct bdc_sr *); >> > > +/* Controller exception */ >> > > +void bdc_sr_ce(struct bdc *, struct bdc_sr *); >> > > +/* bus interval adjustment */ >> > > +void bdc_sr_bia(struct bdc *, struct bdc_sr *); >> > > +/* Microframce count wrap */ >> > > +void bdc_sr_mcw(struct bdc *, struct bdc_sr *); >> > > +/* Transfer notification error */ >> > > +void bdc_sr_tne(struct bdc *, struct bdc_sr *); >> > > +/* Buffer descriptor error */ >> > > +void bdc_sr_bde(struct bdc *, struct bdc_sr *); >> > > + >> > > +/* EP0 XSF handlers */ >> > > +void bdc_xsf_ep0_setup_recv(struct bdc *, struct bdc_sr *); >> > > +void bdc_xsf_ep0_data_start(struct bdc *, struct bdc_sr *); >> > > +void bdc_xsf_ep0_status_start(struct bdc *, struct bdc_sr *); >> > > + >> > > +void bdc_func_wake_timer(struct work_struct *); >> > > + >> > > +int ep_disable(struct bdc_ep *); >> > > +int ep_enable(struct bdc_ep *); >> > >> > waaaay too much detail of your driver is exposed. This usually means >> > it's wrong. Figure out if you really, really need all of these >> > implementation details to be exposed like this. >> > >> > (Hint: you don't) >> >> OK, Do you mean a lot of function/variables are global instead of static >> and they shouldn't be exposed to all the files? If yes, I will improve this >> in v2. > > yeah, that needs to be cleaned up. > OK. >> > > +/* default 64 entries in a SRR */ >> > > +unsigned int num_sr_entries = 64; >> > > +module_param(num_sr_entries, uint, S_IRUGO); >> > > +MODULE_PARM_DESC(num_sr_entries, "SR entries in SRR,should be power of >> > 2"); >> > > + >> > > +/* Num of bds per table */ >> > > +unsigned int bds_per_table = 32; >> > > +module_param(bds_per_table, uint, S_IRUGO); >> > > +MODULE_PARM_DESC(bds_per_table, "number of bd per table, default 32"); >> > > + >> > > +/* Num of tables in bd list for control,bulk and Int ep */ >> > > +unsigned int num_tables = 2; >> > > +module_param(num_tables, uint, S_IRUGO); >> > > +MODULE_PARM_DESC(num_tables, "number of tables in a bd list for >> > control/bulk/Int ep, default 1"); > > btw, your default is wrong. > >> > > +/* Num of tables in bd list for Isoch ep */ >> > > +unsigned int num_tables_isoc = 6; >> > > +module_param(num_tables_isoc, uint, S_IRUGO); >> > > +MODULE_PARM_DESC(num_tables_isoc, "number of table in bd list for Isoch >> > ep, default 6"); >> > >> > why are any of these configurable ? >> > >> >> This driver is targeted for various kind of applications, so depending upon >> the application requirement and memory availability the user can pass >> configurable number of dma descriptor tables. If these module parameters >> are not there then user will have to recompile the driver. > > aren't there any read-only registers you can use to figure some of these > out in runtime ? Usually, the more module parameters you have, the > "harder" it is to actually use your driver. > >> > > +/* User can force disable U1/U2 entry/exit due to host/phy issues */ >> > > +bool disable_u1u2 = false; >> > > +module_param(disable_u1u2, bool, S_IRUSR); >> > > +MODULE_PARM_DESC(disable_u1u2, "Forcefully disable U1/U2 entry/exit"); >> > >> > have you really seen this happen ? Which host was broken ? How have you >> > proved it to be a host problem ? >> > >> >> Actually, I should have mentioned hubs in the comments, sometime back we >> came across a hub which had broken u1/u2 and that's why I provided user the >> flexibility to disable u1/u2 optionally. > > that should not be done by the UDC however. USB Host stack should take > care of considering that a quirky device. > >> > > +/* U1 Timeout default: 248usec */ >> > > +unsigned int u1_timeout = 0xf8; >> > > +module_param(u1_timeout, uint, S_IRUSR); >> > > +MODULE_PARM_DESC(u1_timeout, "U1T in usec, valid range 1-255"); >> > >> > this should be configurable per-instance. Rather than globably for >> > everybody. Sure, right now you only have one instance, but things can >> > change. >> > >> > The u1_timeout parameter will depend upon application i.e. how aggressive >> they want the low power modes to be, so this parameter provides flexibility >> to the user without recompiling the driver. and I will like to keep this >> parameter. > > Not arguing against that. My point is that if you have an SoC with more > than one instance of this controller, you could have a PCB layout so > that u1_timeout will be different for each port. So instead of making > this global for everybody, you should allow this to be configured > per-intance. > >> > > +/* Interrupt coalescence in usec */ >> > > +unsigned int int_cls = 500; >> > > +module_param(int_cls, uint, S_IRUSR); >> > > +MODULE_PARM_DESC(int_cls, "Interrupt coalescence in usec, valid range >> > 0-0xffff"); >> > > + >> > > +/* num_eps override, sometimes IP is not configured with rite number of >> > eps */ >> > >> > s/rite/right, you're still in FPGA stage, right ? Or do you have actual >> > silicon with wrong number of endpoints ? If you do have silicon, then >> > you have erratum number for this silicon bug. Without that, I won't >> > accept this. >> > >> >> OK, we don't have any silicon with this issue, I will remove this parameter >> in v2. > > thanks > >> > > +static unsigned int num_eps_override; >> > > +module_param(num_eps_override, uint, S_IRUSR); >> > > +MODULE_PARM_DESC(num_eps_override, "Interrupt coalescence in usec, >> > valid range 0-0xffff"); >> > >> > waaaaaaaay too many module parameters. This usually means they're wrong. >> > >> >> I agree, there are many module parameters but it doesn't mean they are >> wrong. Most of the time default values will be used, but I want to provide >> flexibility to the user in case they want to modify configuration depending >> upon their application needs without recompiling the driver. If there is NO >> restriction then I will like to keep the parameters, Otherwise I am OK with >> removing the dma descriptor related module parameters. Let me know. > > I'd rather remove them and only add when we know they're needed. module > parameters are exposed to userland (through modprobe itself and through > sysfs), so they become an ABI. You'll have to maintain those module > parameters to the end of time as soon as they reach the kernel. > > IOW, it's best to be conservative here: don't add anything until you > know it's needed. > OK. I understand. I will remove module parameters for now and add on need basis. >> > > +static int bdc_set_test_mode(struct bdc *bdc, int mode) >> > > +{ >> > > + u32 usb2_pm; >> > > + >> > > + usb2_pm = bdc_readl(bdc->regs, BDC_USPPM2); >> > > + usb2_pm &= ~BDC_PTC_MASK; >> > > + dev_dbg(bdc->dev, "%s\n", __func__); >> > > + switch (mode) { >> > > + case TEST_J: >> > > + case TEST_K: >> > > + case TEST_SE0_NAK: >> > > + case TEST_PACKET: >> > > + case TEST_FORCE_EN: >> > > + usb2_pm |= mode << 28; >> > > + break; >> > > + default: >> > > + return -EINVAL; >> > > + } >> > > + dev_dbg(bdc->dev, "usb2_pm=%08x", usb2_pm); >> > > + bdc_writel(bdc->regs, BDC_USPPM2, usb2_pm); >> > >> > this is probably wrong. Usually controllers stop working if you enter >> > test mode right away. Have you tested this ? Can you see >> > SetFeature(TEST) actually completing, including status phase ? >> > >> > What usually happens it that controller expects you to cache the test >> > mode and only write to register after completion of status phase. >> > >> > Actually, this function is called when the status stage is completed and >> this is tested. The test_mode is also cached in bdc->test_mode. I will >> remove the mode argument from this function and directly use bdc->test_mode >> so there is no confusion, this change will be in v2. > > ah, that helps, thanks. Completely missed that detail. > >> > > + if (ep->flags & BDC_EP_ENABLED) { >> > > + dev_warn(bdc->dev, "%s is already enabled\n", ep->name); >> > > + /*Experimential for g_audio class*/ >> > >> > huh ? Why do you have class-specific crap here ? So instead of trying to >> > fix the bug on g_audio you work around it ? Lucky you, I've fixed that >> > and will send patches soon: >> > >> > commit c5ef21f43d3109d12b5c36190f89332f86920625 >> > Author: Felipe Balbi <balbi@xxxxxx> >> > Date: Mon Sep 29 14:23:41 2014 -0500 >> > >> > usb: gadget: function: uac2: prevent double ep disable >> > >> > without this check, f_uac2 would try to disable >> > the same endpoint twice. Fix that. >> > >> > Signed-off-by: Felipe Balbi <balbi@xxxxxx> >> > >> > diff --git a/drivers/usb/gadget/function/f_uac2.c >> > b/drivers/usb/gadget/function/f_uac2.c >> > index fa51118..1146f4d 100644 >> > --- a/drivers/usb/gadget/function/f_uac2.c >> > +++ b/drivers/usb/gadget/function/f_uac2.c >> > @@ -951,6 +951,9 @@ free_ep(struct uac2_rtd_params *prm, struct usb_ep *ep) >> > struct snd_uac2_chip *uac2 = prm->uac2; >> > int i; >> > >> > + if (!prm->ep_enabled) >> > + return; >> > + >> > prm->ep_enabled = false; >> > >> > for (i = 0; i < USB_XFERS; i++) { >> > >> >> OK, my bad. I didn't realize this could be a bug in the audio gadget and >> there was also investigation pending from my side, that's why I marked this >> as an experimental code. thanks for the patch. > > no problem, you might want to use my testing/next branch for development > (at least for now) as I have quite a few fixes pending v3.18-rc1 :-) > >> > > +void bdc_sr_cmd(struct bdc *bdc, struct bdc_sr *sreport) >> > > +{ >> > > + dev_warn(bdc->dev, "%s: nothing do do\n", __func__); >> > > +} >> > > +void bdc_sr_bia(struct bdc *bdc, struct bdc_sr *sreport) >> > > +{ >> > > + dev_warn(bdc->dev, "%s: nothing do do\n", __func__); >> > > +} >> > > +void bdc_sr_mcw(struct bdc *bdc, struct bdc_sr *sreport) >> > > +{ >> > > + dev_warn(bdc->dev, "%s: nothing do do\n", __func__); >> > > +} >> > > +void bdc_sr_ce(struct bdc *bdc, struct bdc_sr *sreport) >> > > +{ >> > > + dev_warn(bdc->dev, "%s: nothing do do\n", __func__); >> > > +} >> > > +void bdc_sr_tne(struct bdc *bdc, struct bdc_sr *sreport) >> > > +{ >> > > + dev_warn(bdc->dev, "%s: nothing do do\n", __func__); >> > > +} >> > > +void bdc_sr_bde(struct bdc *bdc, struct bdc_sr *sreport) >> > > +{ >> > > + dev_warn(bdc->dev, "%s: nothing do do\n", __func__); >> > > +} >> > >> > why all these if you don't really implement them ? >> > >> >> These functions are callbacks from interrupt handler, and all the functions >> above are to monitor error scenarios which ideally shouldn't happen on >> silicon and so far I have not seen them during my usage but it's just for >> information purposes only. Hope its OK to keep them in the driver? > > I'd rather see them converted as tracepoints into the IRQ handler. Plus, > dev_warn() will *always* be enabled adding pointless overhead to your > IRQ handler for no reason whatsoever. Also "nothing do do" is both > funny-sounding and completely useless from a debugging stand-point. > OK, fixed in v2. >> > > +{ >> > > + u32 upsc, link_state; >> > > + u32 clear_flags = 0; >> > > + >> > > + upsc = bdc_readl(bdc->regs, BDC_UPSC); >> > > + dev_dbg(bdc->dev, "%s upsc=0x%08x bdc->dev_state=%d\n", >> > > + __func__, upsc, bdc->dev_state); >> > > + >> > > + bdc_dbg_upsc(bdc, upsc); >> > > + if ((upsc & BDC_VBC) && (upsc & BDC_VBS)) { >> > >> > why do it here ? udc-core should be the only one calling pullup() to >> > connect data pullups. >> >> >> This is a requirement from the HW that the software can only enable the >> TERM (softconnect) when the device is plugged into the Host i.e. VBus is >> present. If I do this from pullup() then it will not work for a self >> powered device. so this has to be done from this particular event >> condition. > > but that means we have to fix udc-core to add that requirement. > Certainly you're not the only one with such a requirement ;-) > >> > > + } else if (link_state == BDC_LINK_STATE_RESUME) { >> > > + dev_dbg(bdc->dev, "Resumed from Suspend\n"); >> > > + if (bdc->devstatus & DEVICE_SUSPENDED) { >> > > + bdc->gadget_driver->resume(&bdc->gadget); >> > > + bdc->devstatus &= ~DEVICE_SUSPENDED; >> > > + } >> > > + } >> > > + clear_flags |= BDC_PSC|BDC_PCS; >> > > + } else if ((upsc & BDC_PCC) && !(upsc & BDC_PCS)) { >> > > + dev_dbg(bdc->dev, >> > > + "Disconnected dev_state=%d\n", >> > > + bdc->dev_state); >> > > + bdc_upsc_disconnected(bdc); >> > >> > gadget_driver->disconnect() ?? >> > >> >> The gadget_driver->disconnect() will not disable ep0, right? so >> bdc_upsc_disconnected disables ep0 and calls gadget_driver->disconnect(). > > actually, ep0 is usually always enabled. It's not mandatory, but it's > the usual case. If bdc_upsc_disconnected() call ->disconnect(), that's > fine then. > >> > > +static int bdc_udc_start(struct usb_gadget *gadget, >> > > + struct usb_gadget_driver *driver) >> > > +{ >> > > + struct bdc *bdc = gadget_to_bdc(gadget); >> > > + unsigned long flags; >> > > + int ret = 0; >> > > + >> > > + dev_dbg(bdc->dev, "%s()\n", __func__); >> > > + spin_lock_irqsave(&bdc->lock, flags); >> > > + if (bdc->gadget_driver) { >> > > + dev_err(bdc->dev, "%s is already bound to %s\n", >> > > + bdc->gadget.name, >> > > + bdc->gadget_driver->driver.name); >> > > + ret = -EBUSY; >> > > + goto err; >> > > + } >> > > + /* start the controller from pullup function >> > > + * when we want to present the termination >> > > + */ >> > >> > wrong multi-line comment style. Also, the choice you've made here >> > doesn't really make sense. Care to explain why are you bypassing the >> > layers ? What if I decide to change the framework ? This has the >> > potential of causing regressions. >> > >> >> OK, Will fix the multi-line style. This was done just to avoid this >> condition: gadget driver calls udc_start() and doesn't call the pullup_on() >> OR calls pullup_off after pullup_on. If the user plugs in the the device >> into host, then event handler will see the Vbus present and Vbus change >> condition and present the termination. By delaying the udc_start() to >> pullup() helps in avoiding getting any event till gadget layer calls >> pullup(). But your point is also valid that this might cause regression in >> future if the framework is changed. The above scenario can be avoided by >> having a software flag that is set from pullup() and can be checked from >> Interrupt handler before presenting the TERM. I will implement this flag in >> v2 and move the bdc_run back to udc_start(). Thanks for pointing this out. > > alright, let's see. Not sure a flag is enough. I also, can't see the > problem you describe. Sure, ideally pullups won't be connected until > VBUS is above session valid threshold, but (bear with me :-) other than > causing a failure on USB Certification, it should not break > functionality. > > -- > 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