| Message ID | 20161228225711.698-24-stephen.boyd@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Wed, Dec 28, 2016 at 02:57:09PM -0800, Stephen Boyd wrote: > If the phy supports it, call phy_set_mode() to pull up D+ when > required by setting the mode to PHY_MODE_USB_DEVICE. If we want > to remove the pullup, set the mode to PHY_MODE_USB_HOST. > > Cc: Peter Chen <peter.chen@nxp.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org> > --- > drivers/usb/chipidea/udc.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index 0d532a724d48..6d61fa0689b0 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -18,6 +18,7 @@ > #include <linux/kernel.h> > #include <linux/slab.h> > #include <linux/pm_runtime.h> > +#include <linux/phy/phy.h> > #include <linux/usb/ch9.h> > #include <linux/usb/gadget.h> > #include <linux/usb/otg-fsm.h> > @@ -1609,10 +1610,15 @@ static int ci_udc_pullup(struct usb_gadget *_gadget, int is_on) > return 0; > > pm_runtime_get_sync(&ci->gadget.dev); > - if (is_on) > + if (is_on) { > + if (ci->phy) > + phy_set_mode(ci->phy, PHY_MODE_USB_DEVICE); > hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS); > - else > + } else { > hw_write(ci, OP_USBCMD, USBCMD_RS, 0); > + if (ci->phy) > + phy_set_mode(ci->phy, PHY_MODE_USB_HOST); > + } > pm_runtime_put_sync(&ci->gadget.dev); > > return 0; > -- Would you describe the use case for it? Why not adding it at role switch routine?
On Wed, Jan 11, 2017 at 04:19:53PM -0800, Stephen Boyd wrote: > Quoting Peter Chen (2017-01-02 22:53:19) > > On Wed, Dec 28, 2016 at 02:57:09PM -0800, Stephen Boyd wrote: > > > If the phy supports it, call phy_set_mode() to pull up D+ when > > > required by setting the mode to PHY_MODE_USB_DEVICE. If we want > > > to remove the pullup, set the mode to PHY_MODE_USB_HOST. > > > > [..] > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > > index 0d532a724d48..6d61fa0689b0 100644 > > > --- a/drivers/usb/chipidea/udc.c > > > +++ b/drivers/usb/chipidea/udc.c > > > @@ -1609,10 +1610,15 @@ static int ci_udc_pullup(struct usb_gadget *_gadget, int is_on) > > > return 0; > > > > > > pm_runtime_get_sync(&ci->gadget.dev); > > > - if (is_on) > > > + if (is_on) { > > > + if (ci->phy) > > > + phy_set_mode(ci->phy, PHY_MODE_USB_DEVICE); > > > hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS); > > > - else > > > + } else { > > > hw_write(ci, OP_USBCMD, USBCMD_RS, 0); > > > + if (ci->phy) > > > + phy_set_mode(ci->phy, PHY_MODE_USB_HOST); > > > + } > > > pm_runtime_put_sync(&ci->gadget.dev); > > > > > > return 0; > > > > Would you describe the use case for it? Why not adding it at > > role switch routine? > > > > This is about pulling up D+. The phy I have requires that we manually > pull up D+ by writing a ULPI register before we set the run/stop bit. Afaik, only controller can pull up dp when it is at device mode by setting USBCMD_RS. At host mode, clear USBCMD_RS will only stopping sending SoF from controller side. I am puzzled why you can pull up D+ by writing an ULPI register, perhaps, your phy needs DP to change before switching the mode? Would you double confirm that? > I > thought it would be appropriate to do so in ci_udc_pullup(), where we're > supposed to put that pullup code, unless I'm mistaken? > > It's not exactly about putting the phy into device or host mode, so > phy_set_mode() may not actually be the best API to use. Perhaps we need > some sort of phy_pullup_usb() API here?
On Thu, Jan 12, 2017 at 02:49:51PM -0800, Stephen Boyd wrote: > Quoting Peter Chen (2017-01-12 01:50:40) > > On Wed, Jan 11, 2017 at 04:19:53PM -0800, Stephen Boyd wrote: > > > Quoting Peter Chen (2017-01-02 22:53:19) > > > > On Wed, Dec 28, 2016 at 02:57:09PM -0800, Stephen Boyd wrote: > > > > > If the phy supports it, call phy_set_mode() to pull up D+ when > > > > > required by setting the mode to PHY_MODE_USB_DEVICE. If we want > > > > > to remove the pullup, set the mode to PHY_MODE_USB_HOST. > > > > > > > > [..] > > > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > > > > index 0d532a724d48..6d61fa0689b0 100644 > > > > > --- a/drivers/usb/chipidea/udc.c > > > > > +++ b/drivers/usb/chipidea/udc.c > > > > > @@ -1609,10 +1610,15 @@ static int ci_udc_pullup(struct usb_gadget *_gadget, int is_on) > > > > > return 0; > > > > > > > > > > pm_runtime_get_sync(&ci->gadget.dev); > > > > > - if (is_on) > > > > > + if (is_on) { > > > > > + if (ci->phy) > > > > > + phy_set_mode(ci->phy, PHY_MODE_USB_DEVICE); > > > > > hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS); > > > > > - else > > > > > + } else { > > > > > hw_write(ci, OP_USBCMD, USBCMD_RS, 0); > > > > > + if (ci->phy) > > > > > + phy_set_mode(ci->phy, PHY_MODE_USB_HOST); > > > > > + } > > > > > pm_runtime_put_sync(&ci->gadget.dev); > > > > > > > > > > return 0; > > > > > > > > Would you describe the use case for it? Why not adding it at > > > > role switch routine? > > > > > > > > > > This is about pulling up D+. The phy I have requires that we manually > > > pull up D+ by writing a ULPI register before we set the run/stop bit. > > > > Afaik, only controller can pull up dp when it is at device mode by > > setting USBCMD_RS. At host mode, clear USBCMD_RS will only stopping > > sending SoF from controller side. > > > > I am puzzled why you can pull up D+ by writing an ULPI register, perhaps, > > your phy needs DP to change before switching the mode? Would you > > double confirm that? > > With the boards I have, vbus is not routed to the phy. Instead, there's > a vbus comparator on the PMIC where the vbus line from the usb > receptacle is sent. The vbus extcon driver probes the comparator on the > PMIC to see if vbus is present or not and then notifies extcon users > when vbus changes. > > The ULPI register we write in the phy is a vendor specific register > (called MISC_A) that has two bits. If you look at > qcom_usb_hs_phy_set_mode() in this series you'll see that we set > VBUSVLDEXTSEL and VBUSVLDEXT. VBUSVLDEXTSEL controls a mux in the phy > that chooses between an internal comparator, in the case where vbus goes > to the phy, or an external signal input to the phy, VBUSVLDEXT, to > consider as the "session valid" signal. It looks like the session valid > signal drives the D+ pullup resistor in the phy. These bits in MISC_A > don't matter when the phy is in host mode. > > So when the board doesn't route vbus to the phy, we have to toggle the > VBUSVLDEXT bit to signal to the phy that the vbus is there or not. I > also see that we're not supposed to toggle the VBUSVLDEXTSEL bit when in > "normal" operating mode. So perhaps we should do everything in the > qcom_usb_hs_phy_set_mode() routine during the role switch as you > suggest, except toggle the VBUSVLDEXT bit. Toggling the VBUSVLDEXT bit > can be done via some new phy op when the extcon triggers? Why not call phy_set_mode(phy, DEVICE) directly at ci_handle_vbus_change when you get extcon vbus event?
On Fri, Jan 13, 2017 at 12:03:00PM -0800, Stephen Boyd wrote: > Quoting Peter Chen (2017-01-12 19:35:36) > > On Thu, Jan 12, 2017 at 02:49:51PM -0800, Stephen Boyd wrote: > > > > > > With the boards I have, vbus is not routed to the phy. Instead, there's > > > a vbus comparator on the PMIC where the vbus line from the usb > > > receptacle is sent. The vbus extcon driver probes the comparator on the > > > PMIC to see if vbus is present or not and then notifies extcon users > > > when vbus changes. > > > > > > The ULPI register we write in the phy is a vendor specific register > > > (called MISC_A) that has two bits. If you look at > > > qcom_usb_hs_phy_set_mode() in this series you'll see that we set > > > VBUSVLDEXTSEL and VBUSVLDEXT. VBUSVLDEXTSEL controls a mux in the phy > > > that chooses between an internal comparator, in the case where vbus goes > > > to the phy, or an external signal input to the phy, VBUSVLDEXT, to > > > consider as the "session valid" signal. It looks like the session valid > > > signal drives the D+ pullup resistor in the phy. These bits in MISC_A > > > don't matter when the phy is in host mode. > > > > > > So when the board doesn't route vbus to the phy, we have to toggle the > > > VBUSVLDEXT bit to signal to the phy that the vbus is there or not. I > > > also see that we're not supposed to toggle the VBUSVLDEXTSEL bit when in > > > "normal" operating mode. So perhaps we should do everything in the > > > qcom_usb_hs_phy_set_mode() routine during the role switch as you > > > suggest, except toggle the VBUSVLDEXT bit. Toggling the VBUSVLDEXT bit > > > can be done via some new phy op when the extcon triggers? > > > > Why not call phy_set_mode(phy, DEVICE) directly at ci_handle_vbus_change when > > you get extcon vbus event? > > > > Right, I can call phy_set_mode(phy, DEVICE) there, but is that correct? > How do we signal vbus is gone, with phy_set_mode(phy, HOST)? Mode > doesn't seem the same as "vbus status changed" so this feels wrong. > So when the board doesn't route vbus to the phy, we have to toggle the > VBUSVLDEXT bit to signal to the phy that the vbus is there or not. I > also see that we're not supposed to toggle the VBUSVLDEXTSEL bit when in > "normal" operating mode. So perhaps we should do everything in the > qcom_usb_hs_phy_set_mode() routine during the role switch as you > suggest, except toggle the VBUSVLDEXT bit. Toggling the VBUSVLDEXT bit > can be done via some new phy op when the extcon triggers? So, you need to call phy_set_mode when switching between host and device. Besides, you also need to toggle VBUSVLDEXT when the external vbus is on or off at device mode (doesn't need for host mode), is it correct? At include/linux/usb/phy.h, we have .set_vbus interface, maybe you need to port it to generic phy framework.
On Tue, Jan 17, 2017 at 09:58:33AM -0800, Stephen Boyd wrote: > Quoting Peter Chen (2017-01-15 19:45:51) > > > > So, you need to call phy_set_mode when switching between host and device. > > Besides, you also need to toggle VBUSVLDEXT when the external vbus > > is on or off at device mode (doesn't need for host mode), is it correct? > > Correct. > > > > > At include/linux/usb/phy.h, we have .set_vbus interface, maybe you need > > to port it to generic phy framework. > > > > Ok. I'll look into that. Can the other patches in this series be picked > up? Otherwise I can resend them all again once I fix the phy_set_mode() > call location and introduce a new phy op. I can pick up chipidea patches after you test the patch I supplied at: [PATCH v6 11/25] usb: chipidea: vbus event may exist before starting gadget You may ping other maintainers to pick up other patches.
Quoting Peter Chen (2017-01-17 23:34:32) > On Tue, Jan 17, 2017 at 09:58:33AM -0800, Stephen Boyd wrote: > > Quoting Peter Chen (2017-01-15 19:45:51) > > > > > > At include/linux/usb/phy.h, we have .set_vbus interface, maybe you need > > > to port it to generic phy framework. > > > > > > > Ok. I'll look into that. Can the other patches in this series be picked > > up? Otherwise I can resend them all again once I fix the phy_set_mode() > > call location and introduce a new phy op. > > I can pick up chipidea patches after you test the patch I supplied at: > > [PATCH v6 11/25] usb: chipidea: vbus event may exist before starting > gadget Ok. I've confirmed that this updated patch works fine for me. You can have my Tested-by and Reviewed-by there. > > You may ping other maintainers to pick up other patches. > I was hoping you could pick the beginning of the series up until the PHY drivers, which can go via Kishon's tree. That would mean applying the drivers/of/ part. Rob is that ok? -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 18, 2017 at 2:54 PM, Stephen Boyd <stephen.boyd@linaro.org> wrote: > Quoting Peter Chen (2017-01-17 23:34:32) >> On Tue, Jan 17, 2017 at 09:58:33AM -0800, Stephen Boyd wrote: >> > Quoting Peter Chen (2017-01-15 19:45:51) >> > > >> > > At include/linux/usb/phy.h, we have .set_vbus interface, maybe you need >> > > to port it to generic phy framework. >> > > >> > >> > Ok. I'll look into that. Can the other patches in this series be picked >> > up? Otherwise I can resend them all again once I fix the phy_set_mode() >> > call location and introduce a new phy op. >> >> I can pick up chipidea patches after you test the patch I supplied at: >> >> [PATCH v6 11/25] usb: chipidea: vbus event may exist before starting >> gadget > > Ok. I've confirmed that this updated patch works fine for me. You can > have my Tested-by and Reviewed-by there. > >> >> You may ping other maintainers to pick up other patches. >> > > I was hoping you could pick the beginning of the series up until the PHY > drivers, which can go via Kishon's tree. That would mean applying the > drivers/of/ part. Rob is that ok? Peter, If there's a dependency then please take the patches I've acked. That's why I acked them. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 18, 2017 at 02:57:27PM -0600, Rob Herring wrote: > On Wed, Jan 18, 2017 at 2:54 PM, Stephen Boyd <stephen.boyd@linaro.org> wrote: > > Quoting Peter Chen (2017-01-17 23:34:32) > >> On Tue, Jan 17, 2017 at 09:58:33AM -0800, Stephen Boyd wrote: > >> > Quoting Peter Chen (2017-01-15 19:45:51) > >> > > > >> > > At include/linux/usb/phy.h, we have .set_vbus interface, maybe you need > >> > > to port it to generic phy framework. > >> > > > >> > > >> > Ok. I'll look into that. Can the other patches in this series be picked > >> > up? Otherwise I can resend them all again once I fix the phy_set_mode() > >> > call location and introduce a new phy op. > >> > >> I can pick up chipidea patches after you test the patch I supplied at: > >> > >> [PATCH v6 11/25] usb: chipidea: vbus event may exist before starting > >> gadget > > > > Ok. I've confirmed that this updated patch works fine for me. You can > > have my Tested-by and Reviewed-by there. > > > >> > >> You may ping other maintainers to pick up other patches. > >> > > > > I was hoping you could pick the beginning of the series up until the PHY > > drivers, which can go via Kishon's tree. That would mean applying the > > drivers/of/ part. Rob is that ok? > > Peter, If there's a dependency then please take the patches I've > acked. That's why I acked them. > Ok, I will do it.
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index 0d532a724d48..6d61fa0689b0 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -18,6 +18,7 @@ #include <linux/kernel.h> #include <linux/slab.h> #include <linux/pm_runtime.h> +#include <linux/phy/phy.h> #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> #include <linux/usb/otg-fsm.h> @@ -1609,10 +1610,15 @@ static int ci_udc_pullup(struct usb_gadget *_gadget, int is_on) return 0; pm_runtime_get_sync(&ci->gadget.dev); - if (is_on) + if (is_on) { + if (ci->phy) + phy_set_mode(ci->phy, PHY_MODE_USB_DEVICE); hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS); - else + } else { hw_write(ci, OP_USBCMD, USBCMD_RS, 0); + if (ci->phy) + phy_set_mode(ci->phy, PHY_MODE_USB_HOST); + } pm_runtime_put_sync(&ci->gadget.dev); return 0;
If the phy supports it, call phy_set_mode() to pull up D+ when required by setting the mode to PHY_MODE_USB_DEVICE. If we want to remove the pullup, set the mode to PHY_MODE_USB_HOST. Cc: Peter Chen <peter.chen@nxp.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org> --- drivers/usb/chipidea/udc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)