usb/gadget: Add driver for Aspeed SoC virtual hub
PATCH v4 2/2 usb/gadget: Add driver for Aspeed SoC virtual hub
Hi,
Quoted 29 lines · Benjamin Herrenschmidt
Quoted 28 lines · Benjamin Herrenschmidt
Quoted 9 lines · Felipe Balbi
drivers/usb/gadget/udc/aspeed-vhub/core.c
Quoted 7 lines
drivers/usb/gadget/udc/aspeed-vhub/core.c new file +0
-0@@ -0,0 +1,429 @@missing SPDX license identifier (all files)
Ah yup, the driver predates me knowing about them, I'll fix.
Quoted 9 lines
drivers/usb/gadget/udc/aspeed-vhub/core.c line 7
Quoted 7 lines
drivers/usb/gadget/udc/aspeed-vhub/core.c new file +7
-0+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).
Quoted 8 lines
Quoted 2 lines
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.
Quoted 27 lines
Quoted 20 lines
Quoted 11 lines
Quoted 11 lines
drivers/usb/gadget/udc/aspeed-vhub/core.c new file +11
-0+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 /* Quoted 8 lines
drivers/usb/gadget/udc/aspeed-vhub/core.c line 24
Quoted 6 lines
drivers/usb/gadget/udc/aspeed-vhub/core.c new file +6
-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.
Quoted 3 lines
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:
Quoted 7 lines
/*
* We do *NOT* set theVHUB_CTRL_CLK_STOP_SUSPENDbit
* 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
Quoted 3 lines
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
Quoted 30 lines
Quoted 25 lines
Quoted 16 lines
Quoted 16 lines
drivers/usb/gadget/udc/aspeed-vhub/core.c new file +16
-0+/* +* 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 */ Quoted 8 lines
drivers/usb/gadget/udc/aspeed-vhub/core.c line 46
Quoted 6 lines
drivers/usb/gadget/udc/aspeed-vhub/core.c new file +6
-0+#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
Quoted 4 lines
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.
Quoted 4 lines
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 ;-)
Quoted 1 line
Do you have more comments for the rest of the driver or that's it ?
so far, that's it.