On Wed, 2018-03-14 at 10:54 +0200, Felipe Balbi wrote:
> Hi,
>
> Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> writes:
> > 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.
>
> we have DT binding for passing maximum speed. A module parameter is
> global, whereas DT binding (or device property) is per-device.
Hrm, ok I could do that. Less practical for diagnostics, you can't
always easily change the DT (or tell a user to do so over email), but
it would work.
> > > > > +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.
> > */
>
> this is good
>
> > 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).
>
> okay
>
> > > > > + /*
> > > > > + * 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).
>
> they're not arbitrary, actually, and have been around for a long time ;-)
>
> > Do you have more comments for the rest of the driver or that's it ?
>
> so far, that's it.
Ok. I'll re-send.
Thanks !
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
