| Message ID | 1372238954-29047-1-git-send-email-george.cherian@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, Jun 26, 2013 at 02:59:14PM +0530, George Cherian wrote: > There can be configurations in which DWC3 is hoooked up only to USB2 PHY. > In such cases we should not return -EPROBE_DEFER, rather continue probe > even if there is no USB3 PHY. > > Signed-off-by: George Cherian <george.cherian@ti.com> > --- > drivers/usb/dwc3/core.c | 31 ++++++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index c35d49d..d5e6f3e 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -100,7 +100,9 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc) > dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > > usb_phy_init(dwc->usb2_phy); > - usb_phy_init(dwc->usb3_phy); > + > + if (dwc->usb3_phy) > + usb_phy_init(dwc->usb3_phy); I would feel more comfortable if you would move our maximum_speed module parameter to DT with a property such as: snps,maximum_speed = "highspeed"; then on driver you could: ret = of_property_read_string(np, "snps,maximum_speed", &maximum_speed); if (ret < 0) bailout(); if (strncmp(maximum_speed, "superspeed", 10) == 0) { /* grab USB3 PHY, return EPROBE_DEFER if not found */ grab_usb3_phy(); } if ((strncmp(maximum_speed, "highspeed", 9) == 0) || (strncmp(maximum_speed, "fullspeed", 9) == 0) || (strncmp(maximum_speed, "lowspeed", 8) == 0)) { /* grab USB2 PHY, return EPROBE_DEFER if not found */ grab_usb2_phy(); } this way, we depend solely on setting maximum_speed to highspeed for AM437x :-) ps: don't forget to default to superspeed in case no property is passed.
On 6/26/2013 3:46 PM, Felipe Balbi wrote: > Hi, > > On Wed, Jun 26, 2013 at 02:59:14PM +0530, George Cherian wrote: >> There can be configurations in which DWC3 is hoooked up only to USB2 PHY. >> In such cases we should not return -EPROBE_DEFER, rather continue probe >> even if there is no USB3 PHY. >> >> Signed-off-by: George Cherian <george.cherian@ti.com> >> --- >> drivers/usb/dwc3/core.c | 31 ++++++++++++++++++++++++------- >> 1 file changed, 24 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index c35d49d..d5e6f3e 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -100,7 +100,9 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc) >> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> >> usb_phy_init(dwc->usb2_phy); >> - usb_phy_init(dwc->usb3_phy); >> + >> + if (dwc->usb3_phy) >> + usb_phy_init(dwc->usb3_phy); > I would feel more comfortable if you would move our maximum_speed module > parameter to DT with a property such as: > > snps,maximum_speed = "highspeed"; > > then on driver you could: okay > ret = of_property_read_string(np, "snps,maximum_speed", &maximum_speed); > if (ret < 0) > bailout(); > > if (strncmp(maximum_speed, "superspeed", 10) == 0) { > /* grab USB3 PHY, return EPROBE_DEFER if not found */ > grab_usb3_phy(); > } > > if ((strncmp(maximum_speed, "highspeed", 9) == 0) || > (strncmp(maximum_speed, "fullspeed", 9) == 0) || > (strncmp(maximum_speed, "lowspeed", 8) == 0)) { > /* grab USB2 PHY, return EPROBE_DEFER if not found */ > grab_usb2_phy(); > } > > this way, we depend solely on setting maximum_speed to highspeed for > AM437x :-) In dra7xx one instance is superspeed and one instance highspeed. > > ps: don't forget to default to superspeed in case no property is passed. okay Regards -George -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 26, 2013 at 05:37:19PM +0530, George Cherian wrote: > On 6/26/2013 3:46 PM, Felipe Balbi wrote: > >Hi, > > > >On Wed, Jun 26, 2013 at 02:59:14PM +0530, George Cherian wrote: > >>There can be configurations in which DWC3 is hoooked up only to USB2 PHY. > >>In such cases we should not return -EPROBE_DEFER, rather continue probe > >>even if there is no USB3 PHY. > >> > >>Signed-off-by: George Cherian <george.cherian@ti.com> > >>--- > >> drivers/usb/dwc3/core.c | 31 ++++++++++++++++++++++++------- > >> 1 file changed, 24 insertions(+), 7 deletions(-) > >> > >>diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >>index c35d49d..d5e6f3e 100644 > >>--- a/drivers/usb/dwc3/core.c > >>+++ b/drivers/usb/dwc3/core.c > >>@@ -100,7 +100,9 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc) > >> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > >> usb_phy_init(dwc->usb2_phy); > >>- usb_phy_init(dwc->usb3_phy); > >>+ > >>+ if (dwc->usb3_phy) > >>+ usb_phy_init(dwc->usb3_phy); > >I would feel more comfortable if you would move our maximum_speed module > >parameter to DT with a property such as: > > > >snps,maximum_speed = "highspeed"; > > > >then on driver you could: > okay > >ret = of_property_read_string(np, "snps,maximum_speed", &maximum_speed); > >if (ret < 0) > > bailout(); > > > >if (strncmp(maximum_speed, "superspeed", 10) == 0) { > > /* grab USB3 PHY, return EPROBE_DEFER if not found */ > > grab_usb3_phy(); > >} > > > >if ((strncmp(maximum_speed, "highspeed", 9) == 0) || > > (strncmp(maximum_speed, "fullspeed", 9) == 0) || > > (strncmp(maximum_speed, "lowspeed", 8) == 0)) { > > /* grab USB2 PHY, return EPROBE_DEFER if not found */ > > grab_usb2_phy(); > >} > > > >this way, we depend solely on setting maximum_speed to highspeed for > >AM437x :-) > In dra7xx one instance is superspeed and one instance highspeed. right, but in DT you will define both instances and each instance will have a seaparate snps,maximum_speed attribute :-) I'm now considering if we should make maximum_speed a generic attribute, Kishon ? Alex ? Alan ? anyone else needs such thing ?
Felipe Balbi <balbi@ti.com> writes: > On Wed, Jun 26, 2013 at 05:37:19PM +0530, George Cherian wrote: >> On 6/26/2013 3:46 PM, Felipe Balbi wrote: >> >Hi, >> > >> >On Wed, Jun 26, 2013 at 02:59:14PM +0530, George Cherian wrote: >> >>There can be configurations in which DWC3 is hoooked up only to USB2 PHY. >> >>In such cases we should not return -EPROBE_DEFER, rather continue probe >> >>even if there is no USB3 PHY. >> >> >> >>Signed-off-by: George Cherian <george.cherian@ti.com> >> >>--- >> >> drivers/usb/dwc3/core.c | 31 ++++++++++++++++++++++++------- >> >> 1 file changed, 24 insertions(+), 7 deletions(-) >> >> >> >>diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> >>index c35d49d..d5e6f3e 100644 >> >>--- a/drivers/usb/dwc3/core.c >> >>+++ b/drivers/usb/dwc3/core.c >> >>@@ -100,7 +100,9 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc) >> >> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> >> usb_phy_init(dwc->usb2_phy); >> >>- usb_phy_init(dwc->usb3_phy); >> >>+ >> >>+ if (dwc->usb3_phy) >> >>+ usb_phy_init(dwc->usb3_phy); >> >I would feel more comfortable if you would move our maximum_speed module >> >parameter to DT with a property such as: >> > >> >snps,maximum_speed = "highspeed"; >> > >> >then on driver you could: >> okay >> >ret = of_property_read_string(np, "snps,maximum_speed", &maximum_speed); >> >if (ret < 0) >> > bailout(); >> > >> >if (strncmp(maximum_speed, "superspeed", 10) == 0) { >> > /* grab USB3 PHY, return EPROBE_DEFER if not found */ >> > grab_usb3_phy(); >> >} >> > >> >if ((strncmp(maximum_speed, "highspeed", 9) == 0) || >> > (strncmp(maximum_speed, "fullspeed", 9) == 0) || >> > (strncmp(maximum_speed, "lowspeed", 8) == 0)) { >> > /* grab USB2 PHY, return EPROBE_DEFER if not found */ >> > grab_usb2_phy(); >> >} >> > >> >this way, we depend solely on setting maximum_speed to highspeed for >> >AM437x :-) >> In dra7xx one instance is superspeed and one instance highspeed. > > right, but in DT you will define both instances and each instance will > have a seaparate snps,maximum_speed attribute :-) > > I'm now considering if we should make maximum_speed a generic attribute, > Kishon ? Alex ? Alan ? > > anyone else needs such thing ? We have a force-full-speed attibute for chipidea on the way. This maximum_speed looks like a more generic alternative. Michael, what say you? Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 26, 2013 at 03:46:45PM +0300, Alexander Shishkin wrote: > Felipe Balbi <balbi@ti.com> writes: > > > On Wed, Jun 26, 2013 at 05:37:19PM +0530, George Cherian wrote: > >> On 6/26/2013 3:46 PM, Felipe Balbi wrote: > >> >Hi, > >> > > >> >On Wed, Jun 26, 2013 at 02:59:14PM +0530, George Cherian wrote: > >> >>There can be configurations in which DWC3 is hoooked up only to USB2 PHY. > >> >>In such cases we should not return -EPROBE_DEFER, rather continue probe > >> >>even if there is no USB3 PHY. > >> >> > >> >>Signed-off-by: George Cherian <george.cherian@ti.com> > >> >>--- > >> >> drivers/usb/dwc3/core.c | 31 ++++++++++++++++++++++++------- > >> >> 1 file changed, 24 insertions(+), 7 deletions(-) > >> >> > >> >>diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >> >>index c35d49d..d5e6f3e 100644 > >> >>--- a/drivers/usb/dwc3/core.c > >> >>+++ b/drivers/usb/dwc3/core.c > >> >>@@ -100,7 +100,9 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc) > >> >> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > >> >> usb_phy_init(dwc->usb2_phy); > >> >>- usb_phy_init(dwc->usb3_phy); > >> >>+ > >> >>+ if (dwc->usb3_phy) > >> >>+ usb_phy_init(dwc->usb3_phy); > >> >I would feel more comfortable if you would move our maximum_speed module > >> >parameter to DT with a property such as: > >> > > >> >snps,maximum_speed = "highspeed"; > >> > > >> >then on driver you could: > >> okay > >> >ret = of_property_read_string(np, "snps,maximum_speed", &maximum_speed); > >> >if (ret < 0) > >> > bailout(); > >> > > >> >if (strncmp(maximum_speed, "superspeed", 10) == 0) { > >> > /* grab USB3 PHY, return EPROBE_DEFER if not found */ > >> > grab_usb3_phy(); > >> >} > >> > > >> >if ((strncmp(maximum_speed, "highspeed", 9) == 0) || > >> > (strncmp(maximum_speed, "fullspeed", 9) == 0) || > >> > (strncmp(maximum_speed, "lowspeed", 8) == 0)) { > >> > /* grab USB2 PHY, return EPROBE_DEFER if not found */ > >> > grab_usb2_phy(); > >> >} > >> > > >> >this way, we depend solely on setting maximum_speed to highspeed for > >> >AM437x :-) > >> In dra7xx one instance is superspeed and one instance highspeed. > > > > right, but in DT you will define both instances and each instance will > > have a seaparate snps,maximum_speed attribute :-) > > > > I'm now considering if we should make maximum_speed a generic attribute, > > Kishon ? Alex ? Alan ? > > > > anyone else needs such thing ? > > We have a force-full-speed attibute for chipidea on the way. This > maximum_speed looks like a more generic alternative. Michael, what say > you? The maximum_speed attribute sounds more reasonable. I will change my patch to it. Regards, Michael
Hi, On Thu, Jun 27, 2013 at 08:14:16AM +0200, Michael Grzeschik wrote: > > > right, but in DT you will define both instances and each instance will > > > have a seaparate snps,maximum_speed attribute :-) > > > > > > I'm now considering if we should make maximum_speed a generic attribute, > > > Kishon ? Alex ? Alan ? > > > > > > anyone else needs such thing ? > > > > We have a force-full-speed attibute for chipidea on the way. This > > maximum_speed looks like a more generic alternative. Michael, what say > > you? > > The maximum_speed attribute sounds more reasonable. I will change my > patch to it. thanks, perhaps we can add a generic helper in udc-core or usb-common ?
Hi, On Thu, Jun 27, 2013 at 09:35:26AM +0300, Felipe Balbi wrote: > Hi, > > On Thu, Jun 27, 2013 at 08:14:16AM +0200, Michael Grzeschik wrote: > > > > right, but in DT you will define both instances and each instance will > > > > have a seaparate snps,maximum_speed attribute :-) > > > > > > > > I'm now considering if we should make maximum_speed a generic attribute, > > > > Kishon ? Alex ? Alan ? > > > > > > > > anyone else needs such thing ? > > > > > > We have a force-full-speed attibute for chipidea on the way. This > > > maximum_speed looks like a more generic alternative. Michael, what say > > > you? > > > > The maximum_speed attribute sounds more reasonable. I will change my > > patch to it. > > thanks, perhaps we can add a generic helper in udc-core or usb-common ? As this function is not only udc specific, this should go into usb-common. But right, we should go with a helper here. I will write one in addition to my full-speed patch as first user of it. Thanks, Michael
Hi, On Thu, Jun 27, 2013 at 09:24:08AM +0200, Michael Grzeschik wrote: > > > > > right, but in DT you will define both instances and each instance will > > > > > have a seaparate snps,maximum_speed attribute :-) > > > > > > > > > > I'm now considering if we should make maximum_speed a generic attribute, > > > > > Kishon ? Alex ? Alan ? > > > > > > > > > > anyone else needs such thing ? > > > > > > > > We have a force-full-speed attibute for chipidea on the way. This > > > > maximum_speed looks like a more generic alternative. Michael, what say > > > > you? > > > > > > The maximum_speed attribute sounds more reasonable. I will change my > > > patch to it. > > > > thanks, perhaps we can add a generic helper in udc-core or usb-common ? > > As this function is not only udc specific, this should go into usb-common. > But right, we should go with a helper here. I will write one in addition > to my full-speed patch as first user of it. alright, thanks :-)
Hi, On Thu, Jun 27, 2013 at 10:30:33AM +0300, Felipe Balbi wrote: > Hi, > > On Thu, Jun 27, 2013 at 09:24:08AM +0200, Michael Grzeschik wrote: > > > > > > right, but in DT you will define both instances and each instance will > > > > > > have a seaparate snps,maximum_speed attribute :-) > > > > > > > > > > > > I'm now considering if we should make maximum_speed a generic attribute, > > > > > > Kishon ? Alex ? Alan ? > > > > > > > > > > > > anyone else needs such thing ? > > > > > > > > > > We have a force-full-speed attibute for chipidea on the way. This > > > > > maximum_speed looks like a more generic alternative. Michael, what say > > > > > you? > > > > > > > > The maximum_speed attribute sounds more reasonable. I will change my > > > > patch to it. > > > > > > thanks, perhaps we can add a generic helper in udc-core or usb-common ? > > > > As this function is not only udc specific, this should go into usb-common. > > But right, we should go with a helper here. I will write one in addition > > to my full-speed patch as first user of it. > > alright, thanks :-) Michael, if you need any help with patching in maximum_speed attribute, let me know as I have some time to work on that part. cheers
On Fri, Jun 28, 2013 at 09:00:00PM +0300, Felipe Balbi wrote: > Hi, > > On Thu, Jun 27, 2013 at 10:30:33AM +0300, Felipe Balbi wrote: > > Hi, > > > > On Thu, Jun 27, 2013 at 09:24:08AM +0200, Michael Grzeschik wrote: > > > > > > > right, but in DT you will define both instances and each instance will > > > > > > > have a seaparate snps,maximum_speed attribute :-) > > > > > > > > > > > > > > I'm now considering if we should make maximum_speed a generic attribute, > > > > > > > Kishon ? Alex ? Alan ? > > > > > > > > > > > > > > anyone else needs such thing ? > > > > > > > > > > > > We have a force-full-speed attibute for chipidea on the way. This > > > > > > maximum_speed looks like a more generic alternative. Michael, what say > > > > > > you? > > > > > > > > > > The maximum_speed attribute sounds more reasonable. I will change my > > > > > patch to it. > > > > > > > > thanks, perhaps we can add a generic helper in udc-core or usb-common ? > > > > > > As this function is not only udc specific, this should go into usb-common. > > > But right, we should go with a helper here. I will write one in addition > > > to my full-speed patch as first user of it. > > > > alright, thanks :-) > > Michael, if you need any help with patching in maximum_speed attribute, > let me know as I have some time to work on that part. Feel free to begin with anything. I am currently short on time. Do you need some pointers? Thanks, Michael
Hi, On Fri, Jun 28, 2013 at 10:06:01PM +0200, Michael Grzeschik wrote: > > > > > > > > right, but in DT you will define both instances and each instance will > > > > > > > > have a seaparate snps,maximum_speed attribute :-) > > > > > > > > > > > > > > > > I'm now considering if we should make maximum_speed a generic attribute, > > > > > > > > Kishon ? Alex ? Alan ? > > > > > > > > > > > > > > > > anyone else needs such thing ? > > > > > > > > > > > > > > We have a force-full-speed attibute for chipidea on the way. This > > > > > > > maximum_speed looks like a more generic alternative. Michael, what say > > > > > > > you? > > > > > > > > > > > > The maximum_speed attribute sounds more reasonable. I will change my > > > > > > patch to it. > > > > > > > > > > thanks, perhaps we can add a generic helper in udc-core or usb-common ? > > > > > > > > As this function is not only udc specific, this should go into usb-common. > > > > But right, we should go with a helper here. I will write one in addition > > > > to my full-speed patch as first user of it. > > > > > > alright, thanks :-) > > > > Michael, if you need any help with patching in maximum_speed attribute, > > let me know as I have some time to work on that part. > > Feel free to begin with anything. I am currently short on time. > Do you need some pointers? not, really v1 will come on monday.
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index c35d49d..d5e6f3e 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -100,7 +100,9 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc) dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); usb_phy_init(dwc->usb2_phy); - usb_phy_init(dwc->usb3_phy); + + if (dwc->usb3_phy) + usb_phy_init(dwc->usb3_phy); mdelay(100); /* Clear USB3 PHY reset */ @@ -360,7 +362,9 @@ err0: static void dwc3_core_exit(struct dwc3 *dwc) { usb_phy_shutdown(dwc->usb2_phy); - usb_phy_shutdown(dwc->usb3_phy); + + if (dwc->usb3_phy) + usb_phy_shutdown(dwc->usb3_phy); } #define DWC3_ALIGN_MASK (16 - 1) @@ -460,12 +464,19 @@ static int dwc3_probe(struct platform_device *pdev) if (ret == -ENXIO) return ret; + /* + * In some cases we wont have a USB3 PHY, in such cases + * if we return EPROBE_DEFER we would never complete the + * probe. Just print error and continue. + */ dev_err(dev, "no usb3 phy configured\n"); - return -EPROBE_DEFER; + dwc->usb3_phy = NULL; } usb_phy_set_suspend(dwc->usb2_phy, 0); - usb_phy_set_suspend(dwc->usb3_phy, 0); + + if (dwc->usb3_phy) + usb_phy_set_suspend(dwc->usb3_phy, 0); spin_lock_init(&dwc->lock); platform_set_drvdata(pdev, dwc); @@ -604,7 +615,9 @@ static int dwc3_remove(struct platform_device *pdev) struct dwc3 *dwc = platform_get_drvdata(pdev); usb_phy_set_suspend(dwc->usb2_phy, 1); - usb_phy_set_suspend(dwc->usb3_phy, 1); + + if (dwc->usb3_phy) + usb_phy_set_suspend(dwc->usb3_phy, 1); pm_runtime_put(&pdev->dev); pm_runtime_disable(&pdev->dev); @@ -700,7 +713,9 @@ static int dwc3_suspend(struct device *dev) dwc->gctl = dwc3_readl(dwc->regs, DWC3_GCTL); spin_unlock_irqrestore(&dwc->lock, flags); - usb_phy_shutdown(dwc->usb3_phy); + if (dwc->usb3_phy) + usb_phy_shutdown(dwc->usb3_phy); + usb_phy_shutdown(dwc->usb2_phy); return 0; @@ -711,7 +726,9 @@ static int dwc3_resume(struct device *dev) struct dwc3 *dwc = dev_get_drvdata(dev); unsigned long flags; - usb_phy_init(dwc->usb3_phy); + if (dwc->usb3_phy) + usb_phy_init(dwc->usb3_phy); + usb_phy_init(dwc->usb2_phy); msleep(100);
There can be configurations in which DWC3 is hoooked up only to USB2 PHY. In such cases we should not return -EPROBE_DEFER, rather continue probe even if there is no USB3 PHY. Signed-off-by: George Cherian <george.cherian@ti.com> --- drivers/usb/dwc3/core.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)