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.
>> > > +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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
