Hi,
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.
> > > +/* 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.
> > > +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.
> > > +{
> > > + 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
Attachment:
signature.asc
Description: Digital signature
