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
