| Message ID | 20170714214005.14967-3-stephen.boyd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 14, 2017 at 02:40:04PM -0700, Stephen Boyd wrote: > > @@ -175,6 +176,10 @@ static int host_start(struct ci_hdrc *ci) > if (ci_otg_is_fsm_mode(ci)) { > otg->host = &hcd->self; > hcd->self.otg_port = 1; > + } else { > + ret = mux_control_select(ci->platdata->usb_switch, 1); It is better to use MACRO for 1 and 0. > + if (ret) > + goto disable_reg; > } > } > > @@ -195,6 +200,8 @@ static void host_stop(struct ci_hdrc *ci) > struct usb_hcd *hcd = ci->hcd; > > if (hcd) { > + if (!ci_otg_is_fsm_mode(ci)) > + mux_control_deselect(ci->platdata->usb_switch); > if (ci->platdata->notify_event) > ci->platdata->notify_event(ci, > CI_HDRC_CONTROLLER_STOPPED_EVENT); > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index d68b125796f9..deb18099e168 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -22,6 +22,7 @@ > #include <linux/usb/gadget.h> > #include <linux/usb/otg-fsm.h> > #include <linux/usb/chipidea.h> > +#include <linux/mux/consumer.h> > > #include "ci.h" > #include "udc.h" > @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci) > > static int udc_id_switch_for_device(struct ci_hdrc *ci) > { > + int ret = 0; > + > if (ci->is_otg) > /* Clear and enable BSV irq */ > hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE, > OTGSC_BSVIS | OTGSC_BSVIE); > > - return 0; > + if (!ci_otg_is_fsm_mode(ci)) > + ret = mux_control_select(ci->platdata->usb_switch, 0); > + > + if (ci->is_otg && ret) > + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS); Should use !ret? Peter > + > + return ret; > } > > static void udc_id_switch_for_host(struct ci_hdrc *ci) > { > + mux_control_deselect(ci->platdata->usb_switch); > + > /* > * host doesn't care B_SESSION_VALID event > * so clear and disbale BSV irq > diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h > index c5fdfcf99828..3b27e333de1d 100644 > --- a/include/linux/usb/chipidea.h > +++ b/include/linux/usb/chipidea.h > @@ -9,6 +9,7 @@ > #include <linux/usb/otg.h> > > struct ci_hdrc; > +struct mux_control; > > /** > * struct ci_hdrc_cable - structure for external connector cable state tracking > @@ -74,6 +75,7 @@ struct ci_hdrc_platform_data { > /* VBUS and ID signal state tracking, using extcon framework */ > struct ci_hdrc_cable vbus_extcon; > struct ci_hdrc_cable id_extcon; > + struct mux_control *usb_switch; > u32 phy_clkgate_delay_us; > }; > > -- > 2.10.0.297.gf6727b0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoting Peter Chen (2017-07-17 21:41:11) > On Fri, Jul 14, 2017 at 02:40:04PM -0700, Stephen Boyd wrote: > > > > @@ -175,6 +176,10 @@ static int host_start(struct ci_hdrc *ci) > > if (ci_otg_is_fsm_mode(ci)) { > > otg->host = &hcd->self; > > hcd->self.otg_port = 1; > > + } else { > > + ret = mux_control_select(ci->platdata->usb_switch, 1); > > It is better to use MACRO for 1 and 0. > Ok. > > + if (ret) > > + goto disable_reg; > > } > > } > > > > @@ -195,6 +200,8 @@ static void host_stop(struct ci_hdrc *ci) > > struct usb_hcd *hcd = ci->hcd; > > > > if (hcd) { > > + if (!ci_otg_is_fsm_mode(ci)) > > + mux_control_deselect(ci->platdata->usb_switch); > > if (ci->platdata->notify_event) > > ci->platdata->notify_event(ci, > > CI_HDRC_CONTROLLER_STOPPED_EVENT); > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > index d68b125796f9..deb18099e168 100644 > > --- a/drivers/usb/chipidea/udc.c > > +++ b/drivers/usb/chipidea/udc.c > > @@ -22,6 +22,7 @@ > > #include <linux/usb/gadget.h> > > #include <linux/usb/otg-fsm.h> > > #include <linux/usb/chipidea.h> > > +#include <linux/mux/consumer.h> > > > > #include "ci.h" > > #include "udc.h" > > @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci) > > > > static int udc_id_switch_for_device(struct ci_hdrc *ci) > > { > > + int ret = 0; > > + > > if (ci->is_otg) > > /* Clear and enable BSV irq */ > > hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE, > > OTGSC_BSVIS | OTGSC_BSVIE); > > > > - return 0; > > + if (!ci_otg_is_fsm_mode(ci)) > > + ret = mux_control_select(ci->platdata->usb_switch, 0); > > + > > + if (ci->is_otg && ret) > > + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS); > > Should use !ret? > No? This is intended to unwind the clearing and enabling of the BSV irq on failure (ret is non-zero) and so we clear and disable the BSV irq.
On Tue, Jul 18, 2017 at 06:47:02PM -0700, Stephen Boyd wrote: > Quoting Peter Chen (2017-07-17 21:41:11) > > On Fri, Jul 14, 2017 at 02:40:04PM -0700, Stephen Boyd wrote: > > > > > > @@ -175,6 +176,10 @@ static int host_start(struct ci_hdrc *ci) > > > if (ci_otg_is_fsm_mode(ci)) { > > > otg->host = &hcd->self; > > > hcd->self.otg_port = 1; > > > + } else { > > > + ret = mux_control_select(ci->platdata->usb_switch, 1); > > > > It is better to use MACRO for 1 and 0. > > > > Ok. > > > > + if (ret) > > > + goto disable_reg; > > > } > > > } > > > > > > @@ -195,6 +200,8 @@ static void host_stop(struct ci_hdrc *ci) > > > struct usb_hcd *hcd = ci->hcd; > > > > > > if (hcd) { > > > + if (!ci_otg_is_fsm_mode(ci)) > > > + mux_control_deselect(ci->platdata->usb_switch); > > > if (ci->platdata->notify_event) > > > ci->platdata->notify_event(ci, > > > CI_HDRC_CONTROLLER_STOPPED_EVENT); > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > > index d68b125796f9..deb18099e168 100644 > > > --- a/drivers/usb/chipidea/udc.c > > > +++ b/drivers/usb/chipidea/udc.c > > > @@ -22,6 +22,7 @@ > > > #include <linux/usb/gadget.h> > > > #include <linux/usb/otg-fsm.h> > > > #include <linux/usb/chipidea.h> > > > +#include <linux/mux/consumer.h> > > > > > > #include "ci.h" > > > #include "udc.h" > > > @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci) > > > > > > static int udc_id_switch_for_device(struct ci_hdrc *ci) > > > { > > > + int ret = 0; > > > + > > > if (ci->is_otg) > > > /* Clear and enable BSV irq */ > > > hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE, > > > OTGSC_BSVIS | OTGSC_BSVIE); > > > > > > - return 0; > > > + if (!ci_otg_is_fsm_mode(ci)) > > > + ret = mux_control_select(ci->platdata->usb_switch, 0); > > > + > > > + if (ci->is_otg && ret) > > > + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS); > > > > Should use !ret? > > > > No? This is intended to unwind the clearing and enabling of the BSV irq > on failure (ret is non-zero) and so we clear and disable the BSV irq. I see now, I did not notice we have already enabled BSV above.
On 2017-07-14 23:40, Stephen Boyd wrote: > On the db410c 96boards platform we have a TC7USB40MU on the board > to mux the D+/D- lines coming from the controller between a micro > usb "device" port and a USB hub for "host" roles[1]. During a > role switch, we need to toggle this mux to forward the D+/D- > lines to either the port or the hub. Add the necessary code to do > the role switch in chipidea core via the generic mux framework. > Board configurations like on db410c are expected to change roles > via the sysfs API described in > Documentation/ABI/testing/sysfs-platform-chipidea-usb2. > > [1] https://github.com/96boards/documentation/raw/master/ConsumerEdition/DragonBoard-410c/HardwareDocs/Schematics_DragonBoard.pdf > > Cc: Peter Rosin <peda@axentia.se> > Cc: Peter Chen <peter.chen@nxp.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: <devicetree@vger.kernel.org> > Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org> > --- > Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 6 ++++++ > drivers/usb/chipidea/Kconfig | 1 + > drivers/usb/chipidea/core.c | 5 +++++ > drivers/usb/chipidea/host.c | 7 +++++++ > drivers/usb/chipidea/udc.c | 13 ++++++++++++- > include/linux/usb/chipidea.h | 2 ++ > 6 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > index 0e03344e2e8b..2e9318151df7 100644 > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > @@ -76,6 +76,10 @@ Optional properties: > needs to make sure it does not send more than 90% > maximum_periodic_data_per_frame. The use case is multiple transactions, but > less frame rate. > +- mux-controls: The mux control for toggling host/device output of this > + controller. It's expected that a mux state of 0 indicates device mode and a > + mux state of 1 indicates host mode. > +- mux-control-names: Shall be "usb_switch" if mux-controls is specified. > > i.mx specific properties > - fsl,usbmisc: phandler of non-core register device, with one > @@ -102,4 +106,6 @@ Example: > rx-burst-size-dword = <0x10>; > extcon = <0>, <&usb_id>; > phy-clkgate-delay-us = <400>; > + mux-controls = <&usb_switch>; > + mux-control-names = "usb_switch"; > }; > diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig > index 51f4157bbecf..3798e0e69d57 100644 > --- a/drivers/usb/chipidea/Kconfig > +++ b/drivers/usb/chipidea/Kconfig > @@ -3,6 +3,7 @@ config USB_CHIPIDEA > depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA > select EXTCON > select RESET_CONTROLLER > + select MULTIPLEXER > help > Say Y here if your system has a dual role high speed USB > controller based on ChipIdea silicon IP. It supports: > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > index b17ed3a9a304..d088c262ebb8 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -64,6 +64,7 @@ > #include <linux/of.h> > #include <linux/regulator/consumer.h> > #include <linux/usb/ehci_def.h> > +#include <linux/mux/consumer.h> > > #include "ci.h" > #include "udc.h" > @@ -690,6 +691,10 @@ static int ci_get_platdata(struct device *dev, > if (of_find_property(dev->of_node, "non-zero-ttctrl-ttha", NULL)) > platdata->flags |= CI_HDRC_SET_NON_ZERO_TTHA; > > + platdata->usb_switch = devm_mux_control_get_optional(dev, "usb_switch"); > + if (IS_ERR(platdata->usb_switch)) > + return PTR_ERR(platdata->usb_switch); > + > ext_id = ERR_PTR(-ENODEV); > ext_vbus = ERR_PTR(-ENODEV); > if (of_property_read_bool(dev->of_node, "extcon")) { > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c > index 18cb8e46262d..9ef3ecf27ad3 100644 > --- a/drivers/usb/chipidea/host.c > +++ b/drivers/usb/chipidea/host.c > @@ -25,6 +25,7 @@ > #include <linux/usb/hcd.h> > #include <linux/usb/chipidea.h> > #include <linux/regulator/consumer.h> > +#include <linux/mux/consumer.h> > > #include "../host/ehci.h" > > @@ -175,6 +176,10 @@ static int host_start(struct ci_hdrc *ci) > if (ci_otg_is_fsm_mode(ci)) { > otg->host = &hcd->self; > hcd->self.otg_port = 1; > + } else { > + ret = mux_control_select(ci->platdata->usb_switch, 1); > + if (ret) > + goto disable_reg; > } > } > > @@ -195,6 +200,8 @@ static void host_stop(struct ci_hdrc *ci) > struct usb_hcd *hcd = ci->hcd; > > if (hcd) { > + if (!ci_otg_is_fsm_mode(ci)) > + mux_control_deselect(ci->platdata->usb_switch); > if (ci->platdata->notify_event) > ci->platdata->notify_event(ci, > CI_HDRC_CONTROLLER_STOPPED_EVENT); > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index d68b125796f9..deb18099e168 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -22,6 +22,7 @@ > #include <linux/usb/gadget.h> > #include <linux/usb/otg-fsm.h> > #include <linux/usb/chipidea.h> > +#include <linux/mux/consumer.h> > > #include "ci.h" > #include "udc.h" > @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci) > > static int udc_id_switch_for_device(struct ci_hdrc *ci) > { > + int ret = 0; > + > if (ci->is_otg) > /* Clear and enable BSV irq */ > hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE, > OTGSC_BSVIS | OTGSC_BSVIE); > > - return 0; > + if (!ci_otg_is_fsm_mode(ci)) > + ret = mux_control_select(ci->platdata->usb_switch, 0); > + > + if (ci->is_otg && ret) > + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS); > + > + return ret; > } > > static void udc_id_switch_for_host(struct ci_hdrc *ci) > { > + mux_control_deselect(ci->platdata->usb_switch); > + This looks broken. You conditionally lock the mux and you unconditionally unlock it. Quoting the mux_control_deselect doc: * It is required that a single call is made to mux_control_deselect() for * each and every successful call made to either of mux_control_select() or * mux_control_try_select(). Think of the mux as a semaphore with a max count of one. If you lock it, you have to unlock it when you're done. If you didn't lock it, you have zero business unlocking it. If you try to lock it but fail, you also have no business unlocking it. Just like a semaphore. Another point: I do not know if udc_id_switch_for_host is *only* called when there has been a prior call to udc_id_switch_for_device that succeeded or how this works exactly. But this all looks fragile. Using mux_control_select and mux_control_deselect *must* be done in pairs. If you want a mux to be locked for "a while", such as in this case, you have to make sure you stay within the rules. There is no room for half measures, or you will likely cause a deadlock when something unexpected happens. Cheers, Peter > /* > * host doesn't care B_SESSION_VALID event > * so clear and disbale BSV irq > diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h > index c5fdfcf99828..3b27e333de1d 100644 > --- a/include/linux/usb/chipidea.h > +++ b/include/linux/usb/chipidea.h > @@ -9,6 +9,7 @@ > #include <linux/usb/otg.h> > > struct ci_hdrc; > +struct mux_control; > > /** > * struct ci_hdrc_cable - structure for external connector cable state tracking > @@ -74,6 +75,7 @@ struct ci_hdrc_platform_data { > /* VBUS and ID signal state tracking, using extcon framework */ > struct ci_hdrc_cable vbus_extcon; > struct ci_hdrc_cable id_extcon; > + struct mux_control *usb_switch; > u32 phy_clkgate_delay_us; > }; > >
Quoting Peter Rosin (2017-07-31 03:33:22) > On 2017-07-14 23:40, Stephen Boyd wrote: > > @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci) > > > > static int udc_id_switch_for_device(struct ci_hdrc *ci) > > { > > + int ret = 0; > > + > > if (ci->is_otg) > > /* Clear and enable BSV irq */ > > hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE, > > OTGSC_BSVIS | OTGSC_BSVIE); > > > > - return 0; > > + if (!ci_otg_is_fsm_mode(ci)) > > + ret = mux_control_select(ci->platdata->usb_switch, 0); > > + > > + if (ci->is_otg && ret) > > + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS); > > + > > + return ret; > > } > > > > static void udc_id_switch_for_host(struct ci_hdrc *ci) > > { > > + mux_control_deselect(ci->platdata->usb_switch); > > + > > This looks broken. You conditionally lock the mux and you unconditionally > unlock it. Quoting the mux_control_deselect doc: > > * It is required that a single call is made to mux_control_deselect() for > * each and every successful call made to either of mux_control_select() or > * mux_control_try_select(). > > Think of the mux as a semaphore with a max count of one. If you lock it, > you have to unlock it when you're done. If you didn't lock it, you have > zero business unlocking it. If you try to lock it but fail, you also have > no business unlocking it. Just like a semaphore. Good catch. I've added a if (!ci_otg_is_fsm_mode()) check here. > > Another point: I do not know if udc_id_switch_for_host is *only* called > when there has been a prior call to udc_id_switch_for_device that > succeeded or how this works exactly. But this all looks fragile. Using > mux_control_select and mux_control_deselect *must* be done in pairs. > If you want a mux to be locked for "a while", such as in this case, you > have to make sure you stay within the rules. There is no room for half > measures, or you will likely cause a deadlock when something unexpected > happens. > Can you elaborate? Is it bad that we keep it "locked" while we're in host or device mode? It looked like we paired the start/stop ops with each other so that the mux is properly managed across these ops. My testing hasn't shown a problem, but maybe there's some corner case you're thinking of? I'll double check the code.
On 2017-08-08 03:51, Stephen Boyd wrote: > Quoting Peter Rosin (2017-07-31 03:33:22) >> On 2017-07-14 23:40, Stephen Boyd wrote: >>> @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci) >>> >>> static int udc_id_switch_for_device(struct ci_hdrc *ci) >>> { >>> + int ret = 0; >>> + >>> if (ci->is_otg) >>> /* Clear and enable BSV irq */ >>> hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE, >>> OTGSC_BSVIS | OTGSC_BSVIE); >>> >>> - return 0; >>> + if (!ci_otg_is_fsm_mode(ci)) >>> + ret = mux_control_select(ci->platdata->usb_switch, 0); >>> + >>> + if (ci->is_otg && ret) >>> + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS); >>> + >>> + return ret; >>> } >>> >>> static void udc_id_switch_for_host(struct ci_hdrc *ci) >>> { >>> + mux_control_deselect(ci->platdata->usb_switch); >>> + >> >> This looks broken. You conditionally lock the mux and you unconditionally >> unlock it. Quoting the mux_control_deselect doc: >> >> * It is required that a single call is made to mux_control_deselect() for >> * each and every successful call made to either of mux_control_select() or >> * mux_control_try_select(). >> >> Think of the mux as a semaphore with a max count of one. If you lock it, >> you have to unlock it when you're done. If you didn't lock it, you have >> zero business unlocking it. If you try to lock it but fail, you also have >> no business unlocking it. Just like a semaphore. > > Good catch. I've added a if (!ci_otg_is_fsm_mode()) check here. > >> >> Another point: I do not know if udc_id_switch_for_host is *only* called >> when there has been a prior call to udc_id_switch_for_device that >> succeeded or how this works exactly. But this all looks fragile. Using >> mux_control_select and mux_control_deselect *must* be done in pairs. >> If you want a mux to be locked for "a while", such as in this case, you >> have to make sure you stay within the rules. There is no room for half >> measures, or you will likely cause a deadlock when something unexpected >> happens. >> > > Can you elaborate? Is it bad that we keep it "locked" while we're in > host or device mode? No no, that's good. You do not want some other consumer of the same mux controller to clobber your state (in case it's shared), so you absolutely want to have the mux locked when you want it to remain in a specific position. > It looked like we paired the start/stop ops with > each other so that the mux is properly managed across these ops. Yes, it *looks* ok... > My > testing hasn't shown a problem, but maybe there's some corner case > you're thinking of? I'll double check the code. ...but since I do not know the usb code, I can't tell. What I worry about is the usb core calling udc_id_switch_for_host or udc_id_switch_for_device more than once without any call to the other in between. Maybe that is a guarantee that the usb core makes? Or maybe it isn't? If e.g. there is a third mode (or if one is added in the future), then the calls to mux_control_select and mux_control_deselect would not be paired correctly. Ok, sure, a third mode probably doesn't exist and will probably not be added, but but but... Also, what happens if udc_id_switch_for_device fails? Is it certain that it will be called again before udc_id_switch_for_host is called, or is the failure simply logged? If the latter, you might have a call to mux_control_deselect without a preceding (and successful) call to mux_control_select. That's fatal. I have similar worries for host_start/host_stop, but for that case host_stop is not allowed to fail, and it seems like a safe bet that host_stop will only be called if host_start succeeds. So, I'm not as worried there. In other words, the question is if the usb core is designed to allow this kind of "raw" resource administration in udc_id_switch_for_host and udc_id_switch_for_device, or if you need to keep a local record of the state so that you do not do double resource acquisition or attempt to free resources you don't have? I think I would feel better if the muxing for the device mode could be done in a start/stop pair of function just like the host mode is doing. Again, I don't know the usb code and don't know if such hooks exist or not? Cheers, Peter
Quoting Peter Rosin (2017-08-08 05:46:30) > On 2017-08-08 03:51, Stephen Boyd wrote: > > > It looked like we paired the start/stop ops with > > each other so that the mux is properly managed across these ops. > > Yes, it *looks* ok... > > > My > > testing hasn't shown a problem, but maybe there's some corner case > > you're thinking of? I'll double check the code. > > ...but since I do not know the usb code, I can't tell. What I worry about > is the usb core calling udc_id_switch_for_host or udc_id_switch_for_device > more than once without any call to the other in between. Maybe that is a > guarantee that the usb core makes? Or maybe it isn't? If e.g. there is a > third mode (or if one is added in the future), then the calls to > mux_control_select and mux_control_deselect would not be paired correctly. > Ok, sure, a third mode probably doesn't exist and will probably not be > added, but but but... > > Also, what happens if udc_id_switch_for_device fails? Is it certain that > it will be called again before udc_id_switch_for_host is called, or is > the failure simply logged? If the latter, you might have a call to > mux_control_deselect without a preceding (and successful) call to > mux_control_select. That's fatal. The only thing that could fail right now is the mux selection, so we wouldn't get into some sort of situation where that's locked in and unchangeable. We do rollback the role if it fails to switch, so we also wouldn't go into a half-way state of being in one role but not actually switching all the way over to it. > > I have similar worries for host_start/host_stop, but for that case > host_stop is not allowed to fail, and it seems like a safe bet that > host_stop will only be called if host_start succeeds. So, I'm not as > worried there. > > In other words, the question is if the usb core is designed to allow > this kind of "raw" resource administration in udc_id_switch_for_host and > udc_id_switch_for_device, or if you need to keep a local record of the > state so that you do not do double resource acquisition or attempt to > free resources you don't have? > > I think I would feel better if the muxing for the device mode could > be done in a start/stop pair of function just like the host mode is > doing. Again, I don't know the usb code and don't know if such hooks > exist or not? > The host_start/host_stop functions are assigned to the same struct ci_role_driver ops that udc_idc_switch_for_{device,host} are for the gadget role. Really, these things are called from the same place by the chipidea driver so not much is different between the two files I modify to make the mux calls. Furthermore, we don't want to do this if we have HNP or "true" OTG support so I've put it behind the ci_otg_is_fsm_mode() check to make sure we don't do any muxing stuff based on fsm state changes. It doesn't really make any sense here anyway because this device I have doesn't support OTG, just role switching.
On 2017-08-12 00:26, Stephen Boyd wrote: > Quoting Peter Rosin (2017-08-08 05:46:30) >> On 2017-08-08 03:51, Stephen Boyd wrote: >> >>> It looked like we paired the start/stop ops with >>> each other so that the mux is properly managed across these ops. >> >> Yes, it *looks* ok... >> >>> My >>> testing hasn't shown a problem, but maybe there's some corner case >>> you're thinking of? I'll double check the code. >> >> ...but since I do not know the usb code, I can't tell. What I worry about >> is the usb core calling udc_id_switch_for_host or udc_id_switch_for_device >> more than once without any call to the other in between. Maybe that is a >> guarantee that the usb core makes? Or maybe it isn't? If e.g. there is a >> third mode (or if one is added in the future), then the calls to >> mux_control_select and mux_control_deselect would not be paired correctly. >> Ok, sure, a third mode probably doesn't exist and will probably not be >> added, but but but... >> >> Also, what happens if udc_id_switch_for_device fails? Is it certain that >> it will be called again before udc_id_switch_for_host is called, or is >> the failure simply logged? If the latter, you might have a call to >> mux_control_deselect without a preceding (and successful) call to >> mux_control_select. That's fatal. > > The only thing that could fail right now is the mux selection, so we > wouldn't get into some sort of situation where that's locked in and > unchangeable. We do rollback the role if it fails to switch, so we also > wouldn't go into a half-way state of being in one role but not actually > switching all the way over to it. What do you roll back to if the initial setting of the role fails? Hopefully that causes a probe failure? Cheers, Peter >> I have similar worries for host_start/host_stop, but for that case >> host_stop is not allowed to fail, and it seems like a safe bet that >> host_stop will only be called if host_start succeeds. So, I'm not as >> worried there. >> >> In other words, the question is if the usb core is designed to allow >> this kind of "raw" resource administration in udc_id_switch_for_host and >> udc_id_switch_for_device, or if you need to keep a local record of the >> state so that you do not do double resource acquisition or attempt to >> free resources you don't have? >> >> I think I would feel better if the muxing for the device mode could >> be done in a start/stop pair of function just like the host mode is >> doing. Again, I don't know the usb code and don't know if such hooks >> exist or not? >> > > The host_start/host_stop functions are assigned to the same struct > ci_role_driver ops that udc_idc_switch_for_{device,host} are for the > gadget role. Really, these things are called from the same place by the > chipidea driver so not much is different between the two files I modify > to make the mux calls. Furthermore, we don't want to do this if we have > HNP or "true" OTG support so I've put it behind the ci_otg_is_fsm_mode() > check to make sure we don't do any muxing stuff based on fsm state > changes. It doesn't really make any sense here anyway because this > device I have doesn't support OTG, just role switching.
diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt index 0e03344e2e8b..2e9318151df7 100644 --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt @@ -76,6 +76,10 @@ Optional properties: needs to make sure it does not send more than 90% maximum_periodic_data_per_frame. The use case is multiple transactions, but less frame rate. +- mux-controls: The mux control for toggling host/device output of this + controller. It's expected that a mux state of 0 indicates device mode and a + mux state of 1 indicates host mode. +- mux-control-names: Shall be "usb_switch" if mux-controls is specified. i.mx specific properties - fsl,usbmisc: phandler of non-core register device, with one @@ -102,4 +106,6 @@ Example: rx-burst-size-dword = <0x10>; extcon = <0>, <&usb_id>; phy-clkgate-delay-us = <400>; + mux-controls = <&usb_switch>; + mux-control-names = "usb_switch"; }; diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig index 51f4157bbecf..3798e0e69d57 100644 --- a/drivers/usb/chipidea/Kconfig +++ b/drivers/usb/chipidea/Kconfig @@ -3,6 +3,7 @@ config USB_CHIPIDEA depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA select EXTCON select RESET_CONTROLLER + select MULTIPLEXER help Say Y here if your system has a dual role high speed USB controller based on ChipIdea silicon IP. It supports: diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index b17ed3a9a304..d088c262ebb8 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -64,6 +64,7 @@ #include <linux/of.h> #include <linux/regulator/consumer.h> #include <linux/usb/ehci_def.h> +#include <linux/mux/consumer.h> #include "ci.h" #include "udc.h" @@ -690,6 +691,10 @@ static int ci_get_platdata(struct device *dev, if (of_find_property(dev->of_node, "non-zero-ttctrl-ttha", NULL)) platdata->flags |= CI_HDRC_SET_NON_ZERO_TTHA; + platdata->usb_switch = devm_mux_control_get_optional(dev, "usb_switch"); + if (IS_ERR(platdata->usb_switch)) + return PTR_ERR(platdata->usb_switch); + ext_id = ERR_PTR(-ENODEV); ext_vbus = ERR_PTR(-ENODEV); if (of_property_read_bool(dev->of_node, "extcon")) { diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index 18cb8e46262d..9ef3ecf27ad3 100644 --- a/drivers/usb/chipidea/host.c +++ b/drivers/usb/chipidea/host.c @@ -25,6 +25,7 @@ #include <linux/usb/hcd.h> #include <linux/usb/chipidea.h> #include <linux/regulator/consumer.h> +#include <linux/mux/consumer.h> #include "../host/ehci.h" @@ -175,6 +176,10 @@ static int host_start(struct ci_hdrc *ci) if (ci_otg_is_fsm_mode(ci)) { otg->host = &hcd->self; hcd->self.otg_port = 1; + } else { + ret = mux_control_select(ci->platdata->usb_switch, 1); + if (ret) + goto disable_reg; } } @@ -195,6 +200,8 @@ static void host_stop(struct ci_hdrc *ci) struct usb_hcd *hcd = ci->hcd; if (hcd) { + if (!ci_otg_is_fsm_mode(ci)) + mux_control_deselect(ci->platdata->usb_switch); if (ci->platdata->notify_event) ci->platdata->notify_event(ci, CI_HDRC_CONTROLLER_STOPPED_EVENT); diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index d68b125796f9..deb18099e168 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -22,6 +22,7 @@ #include <linux/usb/gadget.h> #include <linux/usb/otg-fsm.h> #include <linux/usb/chipidea.h> +#include <linux/mux/consumer.h> #include "ci.h" #include "udc.h" @@ -1964,16 +1965,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci) static int udc_id_switch_for_device(struct ci_hdrc *ci) { + int ret = 0; + if (ci->is_otg) /* Clear and enable BSV irq */ hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE, OTGSC_BSVIS | OTGSC_BSVIE); - return 0; + if (!ci_otg_is_fsm_mode(ci)) + ret = mux_control_select(ci->platdata->usb_switch, 0); + + if (ci->is_otg && ret) + hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS); + + return ret; } static void udc_id_switch_for_host(struct ci_hdrc *ci) { + mux_control_deselect(ci->platdata->usb_switch); + /* * host doesn't care B_SESSION_VALID event * so clear and disbale BSV irq diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h index c5fdfcf99828..3b27e333de1d 100644 --- a/include/linux/usb/chipidea.h +++ b/include/linux/usb/chipidea.h @@ -9,6 +9,7 @@ #include <linux/usb/otg.h> struct ci_hdrc; +struct mux_control; /** * struct ci_hdrc_cable - structure for external connector cable state tracking @@ -74,6 +75,7 @@ struct ci_hdrc_platform_data { /* VBUS and ID signal state tracking, using extcon framework */ struct ci_hdrc_cable vbus_extcon; struct ci_hdrc_cable id_extcon; + struct mux_control *usb_switch; u32 phy_clkgate_delay_us; };
On the db410c 96boards platform we have a TC7USB40MU on the board to mux the D+/D- lines coming from the controller between a micro usb "device" port and a USB hub for "host" roles[1]. During a role switch, we need to toggle this mux to forward the D+/D- lines to either the port or the hub. Add the necessary code to do the role switch in chipidea core via the generic mux framework. Board configurations like on db410c are expected to change roles via the sysfs API described in Documentation/ABI/testing/sysfs-platform-chipidea-usb2. [1] https://github.com/96boards/documentation/raw/master/ConsumerEdition/DragonBoard-410c/HardwareDocs/Schematics_DragonBoard.pdf Cc: Peter Rosin <peda@axentia.se> Cc: Peter Chen <peter.chen@nxp.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: <devicetree@vger.kernel.org> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org> --- Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 6 ++++++ drivers/usb/chipidea/Kconfig | 1 + drivers/usb/chipidea/core.c | 5 +++++ drivers/usb/chipidea/host.c | 7 +++++++ drivers/usb/chipidea/udc.c | 13 ++++++++++++- include/linux/usb/chipidea.h | 2 ++ 6 files changed, 33 insertions(+), 1 deletion(-)