usb/gadget: Add driver for Aspeed SoC virtual hub
PATCH v4 2/2 usb/gadget: Add driver for Aspeed SoC virtual hub
Quoted 35 lines · Felipe Balbi
Hi,
Quoted 32 lines · Benjamin Herrenschmidt
Quoted 30 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).
I can just take them out completely but it feels like module parameters are the right thing to do in that specific case.
Quoted 8 lines
Quoted 5 lines
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.
Quoted 78 lines
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).
Quoted 5 lines
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.
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html