On Tue, 2018-03-13 at 09:35 +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2018-03-09 at 11:20 +0200, Felipe Balbi wrote:
> >
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > new file mode 100644
> > > index 000000000000..31ed2b6e241b
> > > --- /dev/null
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > > @@ -0,0 +1,429 @@
> >
> > missing SPDX license identifier (all files)
>
> Ah yup, the driver predates me knowing about them, I'll fix.
>
> > > +static bool force_usb1 = false;
> > > +static bool no_dma_desc = false;
> > > +
> > > +module_param_named(force_usb1, force_usb1, bool, 0644);
> > > +MODULE_PARM_DESC(force_usb1, "Set to true to force to USB1 speed");
> > > +module_param_named(no_dma_desc, no_dma_desc, bool, 0644);
> > > +MODULE_PARM_DESC(no_dma_desc, "Set to true to disable use of DMA descriptors");
> >
> > module parameters? Sorry, but no.
>
> Why ? Any suggestion then for the two above ? They are mostly meant as
> diagnostic/debug features if something doesn't work (for example, the
> board has some sub-standard wiring making USB2 unreliable, or a driver
> bug with DMA descriptors).
>
> I can just take them out completely but it feels like module parameters
> are the right thing to do in that specific case.
I've taken out the second one, it isn't that useful. I've left in the
first one however. This is the right thing to do. That setting (force
USB1 mode) needs to be applied as soon as the vhub gets initialized
at driver load time, regardless of what gadgets might be attached to
it later on.
It's purely a diagnostic function, I don't see a point having it
elsewhere.
.../...
> > > +void ast_vhub_init_hw(struct ast_vhub *vhub)
> > > +{
> > > + u32 ctrl;
> > > +
> > > + UDCDBG(vhub,"(Re)Starting HW ...\n");
> > > +
> > > + /* Enable PHY */
> > > + ctrl = VHUB_CTRL_PHY_CLK |
> > > + VHUB_CTRL_PHY_RESET_DIS;
> > > +
> > > +#if 0 /*
> > > + * This causes registers to become inaccessible during
> > > + * suspend. Need to figure out how to bring the controller
> > > + * back into life to issue a wakeup.
> > > + */
> > > + ctrl |= VHUB_CTRL_CLK_STOP_SUSPEND;
> > > +#endif
> >
> > no commented code.
>
> Why ? This documents a HW issue ... ie, one would normally want to set
> this bit but you can't because in practice you can't bring the HW back
> short of doing a full reset.
So I don't want to lose the "HW documentation" part of that, I've
turned the above into a comment:
/*
* We do *NOT* set the VHUB_CTRL_CLK_STOP_SUSPEND bit
* to stop the logic clock during suspend because
* it causes the registers to become inaccessible and
* we haven't yet figured out a good wayt to bring the
* controller back into life to issue a wakeup.
*/
It will be in v5 when I post it (I'll test a bit more and wait
for other replies from you, if I don't hear back I'll probably send
it by end of week or next week).
> > > + /*
> > > + * Set some ISO & split control bits according to Aspeed
> > > + * recommendation
> > > + *
> > > + * VHUB_CTRL_ISO_RSP_CTRL: When set tells the HW to respond
> > > + * with 0 bytes data packet to ISO IN endpoints when no data
> > > + * is available.
> > > + *
> > > + * VHUB_CTRL_SPLIT_IN: This makes a SOF complete a split IN
> > > + * transaction.
> > > + */
> > > + ctrl |= VHUB_CTRL_ISO_RSP_CTRL | VHUB_CTRL_SPLIT_IN;
> > > + writel(ctrl, vhub->regs + AST_VHUB_CTRL);
> > > + udelay(1);
> > > +
> > > + /* Set descriptor ring size */
> > > +#if AST_VHUB_DESCS_COUNT == 256
> > > + ctrl |= VHUB_CTRL_LONG_DESC;
> > > + writel(ctrl, vhub->regs + AST_VHUB_CTRL);
> > > +#elif AST_VHUB_DESCS_COUNT != 32
> > > + #error Invalid AST_VHUB_DESCS_COUNT
> > > +#endif
> >
> > find a better way for this. No ifdefs
>
> Ugh ? What's that rule ? I could do a module parameter but you hate
> that too and honestly keeping that an ifdef makes things easier. It's
> not meant to be changed unless a hardware or performance issues shows
> up, I wanted to keep both mode of operations available.
Oh well, I've just made it an if () and kept the #define, and the
#error turns into a BUILD_BUG_ON(). Same principle... but I don't
like those arbitrary "rules", I try to avoid them for things I
maintain (granted, not much anymore these days).
Do you have more comments for the rest of the driver or that's it ?
Cheers,
Ben.
--
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
