[v4,2/2] usb/gadget: Add driver for Aspeed SoC virtual hub

[Felipe Balbi <balbi@kernel.org>]

Hi,

Benjamin Herrenschmidt <benh@kernel.crashing.org> 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.

>> > > +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.