Re: [PATCH v2] usb: chipidea: Hook into mux framework to toggle usb switch
On 2018-04-18 07:48, yos...@codeaurora.org wrote: > On 2018-04-17 17:11, Peter Rosin wrote: >> On 2018-04-17 15:52, Yossi Mansharoff 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. >> >> Ok, so this is v2. Please describe what is different from v1. >> I have told you before that this information helps. >> >>> [1] >>> https://github.com/96boards/documentation/raw/master/ConsumerEdition/DragonBoard-410c/HardwareDocs/Schematics_DragonBoard.pdf >> >> This link returns 404 for me. >> >> Cheers, >> Peter > > > Hi, > This patch was split apart from the original patch into two patches > one for chipidea and the other for bindings. > this patch has no other changes two the code. > > I will update the link. Just a note: I will not feed the mux_control_get_optional patch upstream until I feel confident that this patch is going also heading upstream. Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] usb: chipidea: Hook into mux framework to toggle usb switch
> >> @@ -3,6 +3,8 @@ 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
> >> + select MUX_GPIO
> >
> > The above two configurations are only used at your specific platforms,
> > please add them at either your platform defconfig or the related hardware
> > driver's
> Kconfig.
>
> "select MUX_GPIO" is indeed questionable and should be somewhere else because
> this driver will work with any other mux as well. It's simply something else
> that
> requires it. If it was the case that MUX_GPIO is indeed required then the
> whole use
> of the mux subsystem is questionable and the thing controlled might as well be
> controlled directly with the GPIO line. The mux subsystem is good when a
> single mux
> "controller" is shared between several unrelated drivers utilizing different
> muxes
> controlled by that same mux "controller" (think several muxes controlled by
> the same
> GPIO line/lines). The mux subsystem is also useful when the driver does not
> want to
> handle/know how the specific mux is controlled. That said, it's of course not
> wrong to
> use the mux subsystem in cases like this either, but I think it might be much
> easier
> and direct to just twiddle the single GPIO line directly here?
>
> Or do you expect some future HW variant that will use some other means to
> control
> this mux?
>
No, this mux is only used at Yossi's boards, I expect these two configurations
is under
MUX's hardware driver Kconfig if it is existed, or under its platform
defconfig. Someone
has already complained too many selects increase the code size:
https://patchwork.kernel.org/patch/10349293/
> >>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
> >> 33ae87f..8fa0991 100644
> >> --- a/drivers/usb/chipidea/core.c
> >> +++ b/drivers/usb/chipidea/core.c
> >> @@ -61,6 +61,7 @@
> >> #include
> >> #include #include
> >>
> >> +#include
> >>
> >> #include "ci.h"
> >> #include "udc.h"
> >> @@ -687,6 +688,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
> >> af45aa32..d9d2d00
> >> 100644
> >> --- a/drivers/usb/chipidea/host.c
> >> +++ b/drivers/usb/chipidea/host.c
> >> @@ -13,6 +13,7 @@
> >> #include
> >> #include
> >> #include
> >> +#include
> >>
> >> #include "../host/ehci.h"
> >>
> >> @@ -161,6 +162,10 @@ static int host_start(struct ci_hdrc *ci)
> >>if (ci_otg_is_fsm_mode(ci)) {
> >>otg->host = >self;
> >>hcd->self.otg_port = 1;
> >> + } else {
> >> + ret = mux_control_select(ci->platdata->usb_switch, 1);
> >> + if (ret)
> >> + goto disable_reg;
> >
> > What will happen if ci->platdata->usb_switch is NULL?
>
> What has not been mentioned in this patch is that it depends on another patch
> which
> is not yet upstream. You can google for
>
> mux: add mux_control_get_optional() API
>
> to get an idea (it's also in linux-next). Anyway, with that patch this is not
> a problem.
>
Thanks, I see it judges NULL pointer, then it is OK.
> >> /**
> >> * struct ci_hdrc_cable - structure for external connector cable
> >> state tracking @@ -
> >> 76,6 +77,7 @@ struct ci_hdrc_platform_data {
> >>/* VBUS and ID signal state tracking, using extcon framework */
> >>struct ci_hdrc_cablevbus_extcon;
> >>struct ci_hdrc_cableid_extcon;
> >> + struct mux_control *usb_switch;
> >>u32 phy_clkgate_delay_us;
> >
> > If CONFIG_USB_CHIPIDEA_HOST is not defined, it may cause build error
>
> How is that related? There is a forward declaration above?
>
Sorry, I did not notice drivers/usb/chipidea/core.c also includes
.
Peter
Re: [PATCH v2] usb: chipidea: Hook into mux framework to toggle usb switch
On 2018-04-20 04:00, Peter Chen wrote:
>
>
>> --- a/drivers/usb/chipidea/Kconfig
>> +++ b/drivers/usb/chipidea/Kconfig
>> @@ -3,6 +3,8 @@ 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
>> +select MUX_GPIO
>
> The above two configurations are only used at your specific platforms, please
> add
> them at either your platform defconfig or the related hardware driver's
> Kconfig.
"select MUX_GPIO" is indeed questionable and should be somewhere else because
this driver will work with any other mux as well. It's simply something else
that requires it. If it was the case that MUX_GPIO is indeed required then the
whole use of the mux subsystem is questionable and the thing controlled might
as well be controlled directly with the GPIO line. The mux subsystem is good
when a single mux "controller" is shared between several unrelated drivers
utilizing different muxes controlled by that same mux "controller" (think
several muxes controlled by the same GPIO line/lines). The mux subsystem is
also useful when the driver does not want to handle/know how the specific mux
is controlled. That said, it's of course not wrong to use the mux subsystem
in cases like this either, but I think it might be much easier and direct to
just twiddle the single GPIO line directly here?
Or do you expect some future HW variant that will use some other means to
control this mux?
>> 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
>> 33ae87f..8fa0991 100644
>> --- a/drivers/usb/chipidea/core.c
>> +++ b/drivers/usb/chipidea/core.c
>> @@ -61,6 +61,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "ci.h"
>> #include "udc.h"
>> @@ -687,6 +688,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
>> af45aa32..d9d2d00
>> 100644
>> --- a/drivers/usb/chipidea/host.c
>> +++ b/drivers/usb/chipidea/host.c
>> @@ -13,6 +13,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "../host/ehci.h"
>>
>> @@ -161,6 +162,10 @@ static int host_start(struct ci_hdrc *ci)
>> if (ci_otg_is_fsm_mode(ci)) {
>> otg->host = >self;
>> hcd->self.otg_port = 1;
>> +} else {
>> +ret = mux_control_select(ci->platdata->usb_switch, 1);
>> +if (ret)
>> +goto disable_reg;
>
> What will happen if ci->platdata->usb_switch is NULL?
What has not been mentioned in this patch is that it depends on another
patch which is not yet upstream. You can google for
mux: add mux_control_get_optional() API
to get an idea (it's also in linux-next). Anyway, with that patch this
is not a problem.
>> }
>> }
>>
>> @@ -181,6 +186,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);
>
> Ditto.
>
>> 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
>> 9852ec5..209d3f6 100644
>> --- a/drivers/usb/chipidea/udc.c
>> +++ b/drivers/usb/chipidea/udc.c
>> @@ -19,6 +19,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "ci.h"
>> #include "udc.h"
>> @@ -1965,16 +1966,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);
>> +
>
> Ditto
>
>> +if (ci->is_otg && ret)
>> +hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS,
>> OTGSC_BSVIS);
>> +
>> +return ret;
>> }
>>
>> static void
RE: [PATCH v2] usb: chipidea: Hook into mux framework to toggle usb switch
> --- a/drivers/usb/chipidea/Kconfig
> +++ b/drivers/usb/chipidea/Kconfig
> @@ -3,6 +3,8 @@ 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
> + select MUX_GPIO
The above two configurations are only used at your specific platforms, please
add
them at either your platform defconfig or the related hardware driver's
Kconfig.
> 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
> 33ae87f..8fa0991 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -61,6 +61,7 @@
> #include
> #include
> #include
> +#include
>
> #include "ci.h"
> #include "udc.h"
> @@ -687,6 +688,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
> af45aa32..d9d2d00
> 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -13,6 +13,7 @@
> #include
> #include
> #include
> +#include
>
> #include "../host/ehci.h"
>
> @@ -161,6 +162,10 @@ static int host_start(struct ci_hdrc *ci)
> if (ci_otg_is_fsm_mode(ci)) {
> otg->host = >self;
> hcd->self.otg_port = 1;
> + } else {
> + ret = mux_control_select(ci->platdata->usb_switch, 1);
> + if (ret)
> + goto disable_reg;
What will happen if ci->platdata->usb_switch is NULL?
> }
> }
>
> @@ -181,6 +186,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);
Ditto.
> 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
> 9852ec5..209d3f6 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -19,6 +19,7 @@
> #include
> #include
> #include
> +#include
>
> #include "ci.h"
> #include "udc.h"
> @@ -1965,16 +1966,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);
> +
Ditto
> + 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
> 07f9936..9ea55a1 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -10,6 +10,7 @@
> #include
>
> struct ci_hdrc;
> +struct mux_control;
>
> /**
> * struct ci_hdrc_cable - structure for external connector cable state
> tracking @@ -
> 76,6 +77,7 @@ struct ci_hdrc_platform_data {
> /* VBUS and ID signal state tracking, using extcon framework */
> struct ci_hdrc_cablevbus_extcon;
> struct ci_hdrc_cableid_extcon;
> + struct mux_control *usb_switch;
> u32 phy_clkgate_delay_us;
If CONFIG_USB_CHIPIDEA_HOST is not defined, it may cause build error
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: chipidea: Hook into mux framework to toggle usb switch
On 2018-04-17 17:11, Peter Rosin wrote: On 2018-04-17 15:52, Yossi Mansharoff 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. Ok, so this is v2. Please describe what is different from v1. I have told you before that this information helps. [1] https://github.com/96boards/documentation/raw/master/ConsumerEdition/DragonBoard-410c/HardwareDocs/Schematics_DragonBoard.pdf This link returns 404 for me. Cheers, Peter Hi, This patch was split apart from the original patch into two patches one for chipidea and the other for bindings. this patch has no other changes two the code. I will update the link. thanks Yossi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: chipidea: Hook into mux framework to toggle usb switch
On 2018-04-17 15:52, Yossi Mansharoff 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. Ok, so this is v2. Please describe what is different from v1. I have told you before that this information helps. > [1] > https://github.com/96boards/documentation/raw/master/ConsumerEdition/DragonBoard-410c/HardwareDocs/Schematics_DragonBoard.pdf This link returns 404 for me. Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
