* [RFC] usb-phy-generic: Add support to SMSC USB3315 @ 2017-04-19 6:14 Peter Senna Tschudin 2017-04-19 10:03 ` Sergei Shtylyov 2017-04-20 8:50 ` Peter Chen 0 siblings, 2 replies; 21+ messages in thread From: Peter Senna Tschudin @ 2017-04-19 6:14 UTC (permalink / raw) Cc: Peter Senna Tschudin, Peter Chen, Stephen Boyd, Fabien Lahoudere, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list We need the SMSC USB3315 clock and regulator to always be initialized. We also need the PHY driver to take the PHY out of reset. This patch extends the existing USB generic nop phy driver to include a new initialization path. A new compatible string "smsc,usb3315" is used to decide which initialization path to use. CC: Peter Chen <peter.chen@nxp.com> CC: Stephen Boyd <sboyd@codeaurora.org> CC: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com> --- This is a follow-up of previous discussion: https://www.spinics.net/lists/linux-usb/msg146680.html drivers/usb/phy/phy-generic.c | 33 +++++++++++++++++++++++++++++---- drivers/usb/phy/phy-generic.h | 1 + 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c index 89d6e7a..6ea9ce4 100644 --- a/drivers/usb/phy/phy-generic.c +++ b/drivers/usb/phy/phy-generic.c @@ -151,6 +151,9 @@ int usb_gen_phy_init(struct usb_phy *phy) struct usb_phy_generic *nop = dev_get_drvdata(phy->dev); int ret; + if (nop->init_done) + return 0; + if (!IS_ERR(nop->vcc)) { if (regulator_enable(nop->vcc)) dev_err(phy->dev, "Failed to enable power\n"); @@ -164,6 +167,8 @@ int usb_gen_phy_init(struct usb_phy *phy) nop_reset(nop); + nop->init_done = true; + return 0; } EXPORT_SYMBOL_GPL(usb_gen_phy_init); @@ -216,18 +221,29 @@ static int nop_set_host(struct usb_otg *otg, struct usb_bus *host) otg->host = host; return 0; } +int smsc_usb3315_init(struct usb_phy_generic *nop) +{ + /* + * If the gpio for controlling reset state is not available, try again + * later + */ + if(!nop->gpiod_reset) + return -EPROBE_DEFER; + + return usb_gen_phy_init(&nop->phy); +} int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop, struct usb_phy_generic_platform_data *pdata) { + struct device_node *node = NULL; enum usb_phy_type type = USB_PHY_TYPE_USB2; int err = 0; - u32 clk_rate = 0; bool needs_vcc = false; if (dev->of_node) { - struct device_node *node = dev->of_node; + node = dev->of_node; if (of_property_read_u32(node, "clock-frequency", &clk_rate)) clk_rate = 0; @@ -304,6 +320,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop, nop->phy.otg->set_host = nop_set_host; nop->phy.otg->set_peripheral = nop_set_peripheral; + if(node && of_device_is_compatible(node, "smsc,usb3315")) { + err = smsc_usb3315_init(nop); + if (err) + return err; + } + return 0; } EXPORT_SYMBOL_GPL(usb_phy_gen_create_phy); @@ -318,6 +340,10 @@ static int usb_phy_generic_probe(struct platform_device *pdev) if (!nop) return -ENOMEM; + platform_set_drvdata(pdev, nop); + + nop->init_done = false; + err = usb_phy_gen_create_phy(dev, nop, dev_get_platdata(&pdev->dev)); if (err) return err; @@ -346,8 +372,6 @@ static int usb_phy_generic_probe(struct platform_device *pdev) return err; } - platform_set_drvdata(pdev, nop); - return 0; } @@ -362,6 +386,7 @@ static int usb_phy_generic_remove(struct platform_device *pdev) static const struct of_device_id nop_xceiv_dt_ids[] = { { .compatible = "usb-nop-xceiv" }, + { .compatible = "smsc,usb3315" }, { } }; diff --git a/drivers/usb/phy/phy-generic.h b/drivers/usb/phy/phy-generic.h index 0d0eadd..db4ade6 100644 --- a/drivers/usb/phy/phy-generic.h +++ b/drivers/usb/phy/phy-generic.h @@ -14,6 +14,7 @@ struct usb_phy_generic { struct gpio_desc *gpiod_vbus; struct regulator *vbus_draw; bool vbus_draw_enabled; + bool init_done; unsigned long mA; unsigned int vbus; }; -- 2.9.3 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-04-19 6:14 [RFC] usb-phy-generic: Add support to SMSC USB3315 Peter Senna Tschudin @ 2017-04-19 10:03 ` Sergei Shtylyov 2017-04-19 10:24 ` Peter Senna Tschudin 2017-04-20 8:50 ` Peter Chen 1 sibling, 1 reply; 21+ messages in thread From: Sergei Shtylyov @ 2017-04-19 10:03 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Peter Chen, Stephen Boyd, Fabien Lahoudere, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list Hello! On 4/19/2017 9:14 AM, Peter Senna Tschudin wrote: > We need the SMSC USB3315 clock and regulator to always be initialized. > We also need the PHY driver to take the PHY out of reset. This patch > extends the existing USB generic nop phy driver to include a new > initialization path. > > A new compatible string "smsc,usb3315" is used to decide which > initialization path to use. > > CC: Peter Chen <peter.chen@nxp.com> > CC: Stephen Boyd <sboyd@codeaurora.org> > CC: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> > Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com> > --- > > This is a follow-up of previous discussion: > https://www.spinics.net/lists/linux-usb/msg146680.html > > drivers/usb/phy/phy-generic.c | 33 +++++++++++++++++++++++++++++---- > drivers/usb/phy/phy-generic.h | 1 + > 2 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c > index 89d6e7a..6ea9ce4 100644 > --- a/drivers/usb/phy/phy-generic.c > +++ b/drivers/usb/phy/phy-generic.c [...] > @@ -216,18 +221,29 @@ static int nop_set_host(struct usb_otg *otg, struct usb_bus *host) > otg->host = host; > return 0; > } Need empty line here. > +int smsc_usb3315_init(struct usb_phy_generic *nop) > +{ > + /* > + * If the gpio for controlling reset state is not available, try again > + * later > + */ > + if(!nop->gpiod_reset) You hadn't run the patch thru scripts/checkpatch.pl before posting -- need space between *if* and (. [...] > @@ -304,6 +320,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop, > nop->phy.otg->set_host = nop_set_host; > nop->phy.otg->set_peripheral = nop_set_peripheral; > > + if(node && of_device_is_compatible(node, "smsc,usb3315")) { Same here. [...] MBR, Sergei ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-04-19 10:03 ` Sergei Shtylyov @ 2017-04-19 10:24 ` Peter Senna Tschudin 2017-04-19 17:23 ` Sergei Shtylyov 0 siblings, 1 reply; 21+ messages in thread From: Peter Senna Tschudin @ 2017-04-19 10:24 UTC (permalink / raw) To: Sergei Shtylyov Cc: Peter Chen, Stephen Boyd, Fabien Lahoudere, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list On Wed, Apr 19, 2017 at 01:03:23PM +0300, Sergei Shtylyov wrote: > Hello! > > On 4/19/2017 9:14 AM, Peter Senna Tschudin wrote: > > > We need the SMSC USB3315 clock and regulator to always be initialized. > > We also need the PHY driver to take the PHY out of reset. This patch > > extends the existing USB generic nop phy driver to include a new > > initialization path. > > > > A new compatible string "smsc,usb3315" is used to decide which > > initialization path to use. > > > > CC: Peter Chen <peter.chen@nxp.com> > > CC: Stephen Boyd <sboyd@codeaurora.org> > > CC: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> > > Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com> > > --- > > > > This is a follow-up of previous discussion: > > https://www.spinics.net/lists/linux-usb/msg146680.html > > > > drivers/usb/phy/phy-generic.c | 33 +++++++++++++++++++++++++++++---- > > drivers/usb/phy/phy-generic.h | 1 + > > 2 files changed, 30 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c > > index 89d6e7a..6ea9ce4 100644 > > --- a/drivers/usb/phy/phy-generic.c > > +++ b/drivers/usb/phy/phy-generic.c > [...] > > @@ -216,18 +221,29 @@ static int nop_set_host(struct usb_otg *otg, struct usb_bus *host) > > otg->host = host; > > return 0; > > } > > Need empty line here. > > > +int smsc_usb3315_init(struct usb_phy_generic *nop) > > +{ > > + /* > > + * If the gpio for controlling reset state is not available, try again > > + * later > > + */ > > + if(!nop->gpiod_reset) > > You hadn't run the patch thru scripts/checkpatch.pl before posting -- > need space between *if* and (. > > [...] > > @@ -304,6 +320,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop, > > nop->phy.otg->set_host = nop_set_host; > > nop->phy.otg->set_peripheral = nop_set_peripheral; > > > > + if(node && of_device_is_compatible(node, "smsc,usb3315")) { > > Same here. > > [...] > > MBR, Sergei Thank you for the review Sergei! Should I send V2 of this RFC fixing these issues or wait for comments on the validity of this approach? > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-04-19 10:24 ` Peter Senna Tschudin @ 2017-04-19 17:23 ` Sergei Shtylyov 0 siblings, 0 replies; 21+ messages in thread From: Sergei Shtylyov @ 2017-04-19 17:23 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Peter Chen, Stephen Boyd, Fabien Lahoudere, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list On 04/19/2017 01:24 PM, Peter Senna Tschudin wrote: >>> We need the SMSC USB3315 clock and regulator to always be initialized. >>> We also need the PHY driver to take the PHY out of reset. This patch >>> extends the existing USB generic nop phy driver to include a new >>> initialization path. >>> >>> A new compatible string "smsc,usb3315" is used to decide which >>> initialization path to use. >>> >>> CC: Peter Chen <peter.chen@nxp.com> >>> CC: Stephen Boyd <sboyd@codeaurora.org> >>> CC: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> >>> Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com> >>> --- >>> >>> This is a follow-up of previous discussion: >>> https://www.spinics.net/lists/linux-usb/msg146680.html >>> >>> drivers/usb/phy/phy-generic.c | 33 +++++++++++++++++++++++++++++---- >>> drivers/usb/phy/phy-generic.h | 1 + >>> 2 files changed, 30 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c >>> index 89d6e7a..6ea9ce4 100644 >>> --- a/drivers/usb/phy/phy-generic.c >>> +++ b/drivers/usb/phy/phy-generic.c >> [...] >>> @@ -216,18 +221,29 @@ static int nop_set_host(struct usb_otg *otg, struct usb_bus *host) >>> otg->host = host; >>> return 0; >>> } >> >> Need empty line here. >> >>> +int smsc_usb3315_init(struct usb_phy_generic *nop) >>> +{ >>> + /* >>> + * If the gpio for controlling reset state is not available, try again >>> + * later >>> + */ >>> + if(!nop->gpiod_reset) >> >> You hadn't run the patch thru scripts/checkpatch.pl before posting -- >> need space between *if* and (. >> >> [...] >>> @@ -304,6 +320,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop, >>> nop->phy.otg->set_host = nop_set_host; >>> nop->phy.otg->set_peripheral = nop_set_peripheral; >>> >>> + if(node && of_device_is_compatible(node, "smsc,usb3315")) { >> >> Same here. >> >> [...] >> >> MBR, Sergei > > Thank you for the review Sergei! Should I send V2 of this RFC fixing > these issues or wait for comments on the validity of this approach? Wait, of course, if this is RFC... WBR, Sergei ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-04-19 6:14 [RFC] usb-phy-generic: Add support to SMSC USB3315 Peter Senna Tschudin 2017-04-19 10:03 ` Sergei Shtylyov @ 2017-04-20 8:50 ` Peter Chen 2017-05-23 18:16 ` Fabien Lahoudere 1 sibling, 1 reply; 21+ messages in thread From: Peter Chen @ 2017-04-20 8:50 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Stephen Boyd, Fabien Lahoudere, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list On Wed, Apr 19, 2017 at 06:14:13AM +0000, Peter Senna Tschudin wrote: > We need the SMSC USB3315 clock and regulator to always be initialized. > We also need the PHY driver to take the PHY out of reset. This patch > extends the existing USB generic nop phy driver to include a new > initialization path. > > A new compatible string "smsc,usb3315" is used to decide which > initialization path to use. > Hi Peter, This is an ULPI PHY, so it is not suitable using generic USB PHY. Taking a look of drivers/phy/phy-qcom-usb-hs.c, you may have some clues. Peter > CC: Peter Chen <peter.chen@nxp.com> > CC: Stephen Boyd <sboyd@codeaurora.org> > CC: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> > Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com> > --- > > This is a follow-up of previous discussion: > https://www.spinics.net/lists/linux-usb/msg146680.html > > drivers/usb/phy/phy-generic.c | 33 +++++++++++++++++++++++++++++---- > drivers/usb/phy/phy-generic.h | 1 + > 2 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c > index 89d6e7a..6ea9ce4 100644 > --- a/drivers/usb/phy/phy-generic.c > +++ b/drivers/usb/phy/phy-generic.c > @@ -151,6 +151,9 @@ int usb_gen_phy_init(struct usb_phy *phy) > struct usb_phy_generic *nop = dev_get_drvdata(phy->dev); > int ret; > > + if (nop->init_done) > + return 0; > + > if (!IS_ERR(nop->vcc)) { > if (regulator_enable(nop->vcc)) > dev_err(phy->dev, "Failed to enable power\n"); > @@ -164,6 +167,8 @@ int usb_gen_phy_init(struct usb_phy *phy) > > nop_reset(nop); > > + nop->init_done = true; > + > return 0; > } > EXPORT_SYMBOL_GPL(usb_gen_phy_init); > @@ -216,18 +221,29 @@ static int nop_set_host(struct usb_otg *otg, struct usb_bus *host) > otg->host = host; > return 0; > } > +int smsc_usb3315_init(struct usb_phy_generic *nop) > +{ > + /* > + * If the gpio for controlling reset state is not available, try again > + * later > + */ > + if(!nop->gpiod_reset) > + return -EPROBE_DEFER; > + > + return usb_gen_phy_init(&nop->phy); > +} > > int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop, > struct usb_phy_generic_platform_data *pdata) > { > + struct device_node *node = NULL; > enum usb_phy_type type = USB_PHY_TYPE_USB2; > int err = 0; > - > u32 clk_rate = 0; > bool needs_vcc = false; > > if (dev->of_node) { > - struct device_node *node = dev->of_node; > + node = dev->of_node; > > if (of_property_read_u32(node, "clock-frequency", &clk_rate)) > clk_rate = 0; > @@ -304,6 +320,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop, > nop->phy.otg->set_host = nop_set_host; > nop->phy.otg->set_peripheral = nop_set_peripheral; > > + if(node && of_device_is_compatible(node, "smsc,usb3315")) { > + err = smsc_usb3315_init(nop); > + if (err) > + return err; > + } > + > return 0; > } > EXPORT_SYMBOL_GPL(usb_phy_gen_create_phy); > @@ -318,6 +340,10 @@ static int usb_phy_generic_probe(struct platform_device *pdev) > if (!nop) > return -ENOMEM; > > + platform_set_drvdata(pdev, nop); > + > + nop->init_done = false; > + > err = usb_phy_gen_create_phy(dev, nop, dev_get_platdata(&pdev->dev)); > if (err) > return err; > @@ -346,8 +372,6 @@ static int usb_phy_generic_probe(struct platform_device *pdev) > return err; > } > > - platform_set_drvdata(pdev, nop); > - > return 0; > } > > @@ -362,6 +386,7 @@ static int usb_phy_generic_remove(struct platform_device *pdev) > > static const struct of_device_id nop_xceiv_dt_ids[] = { > { .compatible = "usb-nop-xceiv" }, > + { .compatible = "smsc,usb3315" }, > { } > }; > > diff --git a/drivers/usb/phy/phy-generic.h b/drivers/usb/phy/phy-generic.h > index 0d0eadd..db4ade6 100644 > --- a/drivers/usb/phy/phy-generic.h > +++ b/drivers/usb/phy/phy-generic.h > @@ -14,6 +14,7 @@ struct usb_phy_generic { > struct gpio_desc *gpiod_vbus; > struct regulator *vbus_draw; > bool vbus_draw_enabled; > + bool init_done; > unsigned long mA; > unsigned int vbus; > }; > -- > 2.9.3 > -- Best Regards, Peter Chen ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-04-20 8:50 ` Peter Chen @ 2017-05-23 18:16 ` Fabien Lahoudere 2017-05-23 21:00 ` Stephen Boyd 0 siblings, 1 reply; 21+ messages in thread From: Fabien Lahoudere @ 2017-05-23 18:16 UTC (permalink / raw) To: Peter Chen, Peter Senna Tschudin Cc: Stephen Boyd, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list Hi, We investigate on the topic and now our device tree look like: in imx53.dtsi: usbh2: usb@53f80400 { compatible = "fsl,imx53-usb", "fsl,imx27-usb"; reg = <0x53f80400 0x0200>; interrupts = <16>; clocks = <&clks IMX5_CLK_USBOH3_GATE>; fsl,usbmisc = <&usbmisc 2>; dr_mode = "host"; status = "disabled"; }; usbmisc: usbmisc@53f80800 { #index-cells = <1>; compatible = "fsl,imx53-usbmisc"; reg = <0x53f80800 0x200>; clocks = <&clks IMX5_CLK_USBOH3_GATE>; }; and in our dts: &usbh2 { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_usbh2>; disable-int60ck; dr_mode = "host"; //fsl,usbphy = <&usbphy2>; vbus-supply = <®_usbh2_vbus>; status = "okay"; ulpi { phy { compatible = "smsc,usb3315-ulpi"; reset-gpios = <&gpio4 4 GPIO_ACTIVE_LOW>; clock-names = "main_clk"; /* * Hardware uses CKO2 at 24MHz at several places. Set the parent * clock of CKO2 to OSC. */ clock-frequency = <24000000>; clocks = <&clks IMX5_CLK_CKO2>; assigned-clocks = <&clks IMX5_CLK_CKO2_SEL>, <&clks IMX5_CLK_OSC>; assigned-clock-parents = <&clks IMX5_CLK_OSC>; status = "okay"; }; }; }; And we create a basic driver to check what happened: static int smsc_usb3315_phy_probe(struct ulpi *ulpi) { printk(KERN_ERR "Fabien: %s:%d-%s\n", __FILE__, __LINE__, __func__); return 0; } static const struct of_device_id smsc_usb3315_phy_match[] = { { .compatible = "smsc,usb3315-phy", }, { } }; MODULE_DEVICE_TABLE(of, smsc_usb3315_phy_match); static struct ulpi_driver smsc_usb3315_phy_driver = { .probe = smsc_usb3315_phy_probe, .driver = { .name = "smsc_usb3315_phy", .of_match_table = smsc_usb3315_phy_match, }, }; module_ulpi_driver(smsc_usb3315_phy_driver); /*MODULE_ALIAS("platform:usb_phy_generic");*/ MODULE_AUTHOR("GE Healthcare"); MODULE_DESCRIPTION("SMSC USB 3315 ULPI Phy driver"); MODULE_LICENSE("GPL v2"); I checked that the driver is registered by drivers/usb/common/ulpi.c:__ulpi_register_driver successfully. But our probe function (smsc_usb3315_phy_probe) is never called. Our understanding is that we need to use the ulpi_bus instead of devm_usb_get_phy_by_phandle(&pdev- >dev, "fsl,usbphy", 0); in drivers/usb/chipidea/ci_hdrc_imx.c. Is our approach good? How can we use this bus from our controller probe function ? Thanks Fabien On Thu, 2017-04-20 at 16:50 +0800, Peter Chen wrote: > On Wed, Apr 19, 2017 at 06:14:13AM +0000, Peter Senna Tschudin wrote: > > We need the SMSC USB3315 clock and regulator to always be initialized. > > We also need the PHY driver to take the PHY out of reset. This patch > > extends the existing USB generic nop phy driver to include a new > > initialization path. > > > > A new compatible string "smsc,usb3315" is used to decide which > > initialization path to use. > > > > Hi Peter, > > This is an ULPI PHY, so it is not suitable using generic USB PHY. > Taking a look of drivers/phy/phy-qcom-usb-hs.c, you may have some > clues. > > Peter > > > CC: Peter Chen <peter.chen@nxp.com> > > CC: Stephen Boyd <sboyd@codeaurora.org> > > CC: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> > > Signed-off-by: Peter Senna Tschudin <peter.senna@collabora.com> > > --- > > > > This is a follow-up of previous discussion: > > https://www.spinics.net/lists/linux-usb/msg146680.html > > > > drivers/usb/phy/phy-generic.c | 33 +++++++++++++++++++++++++++++---- > > drivers/usb/phy/phy-generic.h | 1 + > > 2 files changed, 30 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c > > index 89d6e7a..6ea9ce4 100644 > > --- a/drivers/usb/phy/phy-generic.c > > +++ b/drivers/usb/phy/phy-generic.c > > @@ -151,6 +151,9 @@ int usb_gen_phy_init(struct usb_phy *phy) > > struct usb_phy_generic *nop = dev_get_drvdata(phy->dev); > > int ret; > > > > + if (nop->init_done) > > + return 0; > > + > > if (!IS_ERR(nop->vcc)) { > > if (regulator_enable(nop->vcc)) > > dev_err(phy->dev, "Failed to enable power\n"); > > @@ -164,6 +167,8 @@ int usb_gen_phy_init(struct usb_phy *phy) > > > > nop_reset(nop); > > > > + nop->init_done = true; > > + > > return 0; > > } > > EXPORT_SYMBOL_GPL(usb_gen_phy_init); > > @@ -216,18 +221,29 @@ static int nop_set_host(struct usb_otg *otg, struct usb_bus *host) > > otg->host = host; > > return 0; > > } > > +int smsc_usb3315_init(struct usb_phy_generic *nop) > > +{ > > + /* > > + * If the gpio for controlling reset state is not available, try again > > + * later > > + */ > > + if(!nop->gpiod_reset) > > + return -EPROBE_DEFER; > > + > > + return usb_gen_phy_init(&nop->phy); > > +} > > > > int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop, > > struct usb_phy_generic_platform_data *pdata) > > { > > + struct device_node *node = NULL; > > enum usb_phy_type type = USB_PHY_TYPE_USB2; > > int err = 0; > > - > > u32 clk_rate = 0; > > bool needs_vcc = false; > > > > if (dev->of_node) { > > - struct device_node *node = dev->of_node; > > + node = dev->of_node; > > > > if (of_property_read_u32(node, "clock-frequency", &clk_rate)) > > clk_rate = 0; > > @@ -304,6 +320,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop, > > nop->phy.otg->set_host = nop_set_host; > > nop->phy.otg->set_peripheral = nop_set_peripheral; > > > > + if(node && of_device_is_compatible(node, "smsc,usb3315")) { > > + err = smsc_usb3315_init(nop); > > + if (err) > > + return err; > > + } > > + > > return 0; > > } > > EXPORT_SYMBOL_GPL(usb_phy_gen_create_phy); > > @@ -318,6 +340,10 @@ static int usb_phy_generic_probe(struct platform_device *pdev) > > if (!nop) > > return -ENOMEM; > > > > + platform_set_drvdata(pdev, nop); > > + > > + nop->init_done = false; > > + > > err = usb_phy_gen_create_phy(dev, nop, dev_get_platdata(&pdev->dev)); > > if (err) > > return err; > > @@ -346,8 +372,6 @@ static int usb_phy_generic_probe(struct platform_device *pdev) > > return err; > > } > > > > - platform_set_drvdata(pdev, nop); > > - > > return 0; > > } > > > > @@ -362,6 +386,7 @@ static int usb_phy_generic_remove(struct platform_device *pdev) > > > > static const struct of_device_id nop_xceiv_dt_ids[] = { > > { .compatible = "usb-nop-xceiv" }, > > + { .compatible = "smsc,usb3315" }, > > { } > > }; > > > > diff --git a/drivers/usb/phy/phy-generic.h b/drivers/usb/phy/phy-generic.h > > index 0d0eadd..db4ade6 100644 > > --- a/drivers/usb/phy/phy-generic.h > > +++ b/drivers/usb/phy/phy-generic.h > > @@ -14,6 +14,7 @@ struct usb_phy_generic { > > struct gpio_desc *gpiod_vbus; > > struct regulator *vbus_draw; > > bool vbus_draw_enabled; > > + bool init_done; > > unsigned long mA; > > unsigned int vbus; > > }; > > -- > > 2.9.3 > > > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-05-23 18:16 ` Fabien Lahoudere @ 2017-05-23 21:00 ` Stephen Boyd 2017-05-25 10:36 ` Fabien Lahoudere 0 siblings, 1 reply; 21+ messages in thread From: Stephen Boyd @ 2017-05-23 21:00 UTC (permalink / raw) To: Fabien Lahoudere Cc: Peter Chen, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list On 05/23, Fabien Lahoudere wrote: > Hi, > > We investigate on the topic and now our device tree look like: > > in imx53.dtsi: > > usbh2: usb@53f80400 { > compatible = "fsl,imx53-usb", "fsl,imx27-usb"; > reg = <0x53f80400 0x0200>; > interrupts = <16>; > clocks = <&clks IMX5_CLK_USBOH3_GATE>; > fsl,usbmisc = <&usbmisc 2>; > dr_mode = "host"; > status = "disabled"; > }; > > usbmisc: usbmisc@53f80800 { > #index-cells = <1>; > compatible = "fsl,imx53-usbmisc"; > reg = <0x53f80800 0x200>; > clocks = <&clks IMX5_CLK_USBOH3_GATE>; > }; > > and in our dts: > > &usbh2 { > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_usbh2>; > disable-int60ck; > dr_mode = "host"; > //fsl,usbphy = <&usbphy2>; > vbus-supply = <®_usbh2_vbus>; > status = "okay"; > ulpi { > phy { > compatible = "smsc,usb3315-ulpi"; > reset-gpios = <&gpio4 4 GPIO_ACTIVE_LOW>; > clock-names = "main_clk"; > /* > * Hardware uses CKO2 at 24MHz at several places. Set the parent > * clock of CKO2 to OSC. > */ > clock-frequency = <24000000>; > clocks = <&clks IMX5_CLK_CKO2>; > assigned-clocks = <&clks IMX5_CLK_CKO2_SEL>, <&clks IMX5_CLK_OSC>; > assigned-clock-parents = <&clks IMX5_CLK_OSC>; > status = "okay"; > }; > }; > }; > > And we create a basic driver to check what happened: > > static int smsc_usb3315_phy_probe(struct ulpi *ulpi) > { > printk(KERN_ERR "Fabien: %s:%d-%s\n", __FILE__, __LINE__, __func__); > > return 0; > } > > static const struct of_device_id smsc_usb3315_phy_match[] = { > { .compatible = "smsc,usb3315-phy", }, > { } > }; > MODULE_DEVICE_TABLE(of, smsc_usb3315_phy_match); > > static struct ulpi_driver smsc_usb3315_phy_driver = { > .probe = smsc_usb3315_phy_probe, > .driver = { > .name = "smsc_usb3315_phy", > .of_match_table = smsc_usb3315_phy_match, > }, > }; > module_ulpi_driver(smsc_usb3315_phy_driver); > > /*MODULE_ALIAS("platform:usb_phy_generic");*/ > MODULE_AUTHOR("GE Healthcare"); > MODULE_DESCRIPTION("SMSC USB 3315 ULPI Phy driver"); > MODULE_LICENSE("GPL v2"); > > I checked that the driver is registered by drivers/usb/common/ulpi.c:__ulpi_register_driver > successfully. Does the ulpi device have some vendor/product ids associated with it? The design is made to only fallback to matching the device to driver based on DT if the ulpi vendor id is 0. Otherwise, if vendor is non-zero you'll need to have a ulpi_device_id id table in your ulpi_driver structure. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-05-23 21:00 ` Stephen Boyd @ 2017-05-25 10:36 ` Fabien Lahoudere 2017-05-26 9:00 ` Fabien Lahoudere 0 siblings, 1 reply; 21+ messages in thread From: Fabien Lahoudere @ 2017-05-25 10:36 UTC (permalink / raw) To: Stephen Boyd Cc: Peter Chen, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list On Tue, 2017-05-23 at 14:00 -0700, Stephen Boyd wrote: > On 05/23, Fabien Lahoudere wrote: > > Hi, > > > > We investigate on the topic and now our device tree look like: > > > > in imx53.dtsi: > > > > usbh2: usb@53f80400 { > > compatible = "fsl,imx53-usb", "fsl,imx27-usb"; > > reg = <0x53f80400 0x0200>; > > interrupts = <16>; > > clocks = <&clks IMX5_CLK_USBOH3_GATE>; > > fsl,usbmisc = <&usbmisc 2>; > > dr_mode = "host"; > > status = "disabled"; > > }; > > > > usbmisc: usbmisc@53f80800 { > > #index-cells = <1>; > > compatible = "fsl,imx53-usbmisc"; > > reg = <0x53f80800 0x200>; > > clocks = <&clks IMX5_CLK_USBOH3_GATE>; > > }; > > > > and in our dts: > > > > &usbh2 { > > pinctrl-names = "default"; > > pinctrl-0 = <&pinctrl_usbh2>; > > disable-int60ck; > > dr_mode = "host"; > > //fsl,usbphy = <&usbphy2>; > > vbus-supply = <®_usbh2_vbus>; > > status = "okay"; > > ulpi { > > phy { > > compatible = "smsc,usb3315-ulpi"; > > reset-gpios = <&gpio4 4 GPIO_ACTIVE_LOW>; > > clock-names = "main_clk"; > > /* > > * Hardware uses CKO2 at 24MHz at several places. Set the parent > > * clock of CKO2 to OSC. > > */ > > clock-frequency = <24000000>; > > clocks = <&clks IMX5_CLK_CKO2>; > > assigned-clocks = <&clks IMX5_CLK_CKO2_SEL>, <&clks IMX5_CLK_OSC>; > > assigned-clock-parents = <&clks IMX5_CLK_OSC>; > > status = "okay"; > > }; > > }; > > }; > > > > And we create a basic driver to check what happened: > > > > static int smsc_usb3315_phy_probe(struct ulpi *ulpi) > > { > > printk(KERN_ERR "Fabien: %s:%d-%s\n", __FILE__, __LINE__, __func__); > > > > return 0; > > } > > > > static const struct of_device_id smsc_usb3315_phy_match[] = { > > { .compatible = "smsc,usb3315-phy", }, > > { } > > }; > > MODULE_DEVICE_TABLE(of, smsc_usb3315_phy_match); > > > > static struct ulpi_driver smsc_usb3315_phy_driver = { > > .probe = smsc_usb3315_phy_probe, > > .driver = { > > .name = "smsc_usb3315_phy", > > .of_match_table = smsc_usb3315_phy_match, > > }, > > }; > > module_ulpi_driver(smsc_usb3315_phy_driver); > > > > /*MODULE_ALIAS("platform:usb_phy_generic");*/ > > MODULE_AUTHOR("GE Healthcare"); > > MODULE_DESCRIPTION("SMSC USB 3315 ULPI Phy driver"); > > MODULE_LICENSE("GPL v2"); > > > > I checked that the driver is registered by drivers/usb/common/ulpi.c:__ulpi_register_driver > > successfully. > > Does the ulpi device have some vendor/product ids associated > with it? The design is made to only fallback to matching the > device to driver based on DT if the ulpi vendor id is 0. > Otherwise, if vendor is non-zero you'll need to have a > ulpi_device_id id table in your ulpi_driver structure. > Hi, Thanks Stephen for your reply. Indeed we have a vendor/product so I modify my code but without effect. After looking at the ulpi source code in the kernel, it seems that I need to call ulpi_register_interface. ci_hdrc_probe should be called to execute the ulpi init. The problem is that we replace "fsl,usbphy = <&usbphy2>;" by an ulpi node but ci_hdrc_imx_probe fail because of "data->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "fsl,usbphy", 0);" So I will try to adapt ci_hdrc_imx_probe to continue phy initialisation if fsl,usbphy is missing. Is it the good way to proceed? Thanks for any advice Fabien ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-05-25 10:36 ` Fabien Lahoudere @ 2017-05-26 9:00 ` Fabien Lahoudere 2017-06-02 22:00 ` Stephen Boyd 0 siblings, 1 reply; 21+ messages in thread From: Fabien Lahoudere @ 2017-05-26 9:00 UTC (permalink / raw) To: Stephen Boyd, Peter Chen Cc: Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list Hello I modify ci_hrdc_imx_probe to bypass "data->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init. The problem is that in ci_ulpi_init, before calling "ci->ulpi = ulpi_register_interface(ci->dev, &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is the original function that make our system to hang. Our phy is not initialised before calling ulpi_register_interface so I don't understand how the phy can reply if it is not out of reset state. The conclusion is that using ulpi_bus to manage our phy doesn't improve and we reach the same issue. I will try to check if we don't do bad things in hw_phymode_configure. If anyone have an idea it is welcome?? Fabien On Thu, 2017-05-25 at 12:36 +0200, Fabien Lahoudere wrote: > On Tue, 2017-05-23 at 14:00 -0700, Stephen Boyd wrote: > > On 05/23, Fabien Lahoudere wrote: > > > Hi, > > > > > > We investigate on the topic and now our device tree look like: > > > > > > in imx53.dtsi: > > > > > > usbh2: usb@53f80400 { > > > compatible = "fsl,imx53-usb", "fsl,imx27-usb"; > > > reg = <0x53f80400 0x0200>; > > > interrupts = <16>; > > > clocks = <&clks IMX5_CLK_USBOH3_GATE>; > > > fsl,usbmisc = <&usbmisc 2>; > > > dr_mode = "host"; > > > status = "disabled"; > > > }; > > > > > > usbmisc: usbmisc@53f80800 { > > > #index-cells = <1>; > > > compatible = "fsl,imx53-usbmisc"; > > > reg = <0x53f80800 0x200>; > > > clocks = <&clks IMX5_CLK_USBOH3_GATE>; > > > }; > > > > > > and in our dts: > > > > > > &usbh2 { > > > pinctrl-names = "default"; > > > pinctrl-0 = <&pinctrl_usbh2>; > > > disable-int60ck; > > > dr_mode = "host"; > > > //fsl,usbphy = <&usbphy2>; > > > vbus-supply = <®_usbh2_vbus>; > > > status = "okay"; > > > ulpi { > > > phy { > > > compatible = "smsc,usb3315-ulpi"; > > > reset-gpios = <&gpio4 4 GPIO_ACTIVE_LOW>; > > > clock-names = "main_clk"; > > > /* > > > * Hardware uses CKO2 at 24MHz at several places. Set the parent > > > * clock of CKO2 to OSC. > > > */ > > > clock-frequency = <24000000>; > > > clocks = <&clks IMX5_CLK_CKO2>; > > > assigned-clocks = <&clks IMX5_CLK_CKO2_SEL>, <&clks IMX5_CLK_OSC>; > > > assigned-clock-parents = <&clks IMX5_CLK_OSC>; > > > status = "okay"; > > > }; > > > }; > > > }; > > > > > > And we create a basic driver to check what happened: > > > > > > static int smsc_usb3315_phy_probe(struct ulpi *ulpi) > > > { > > > printk(KERN_ERR "Fabien: %s:%d-%s\n", __FILE__, __LINE__, __func__); > > > > > > return 0; > > > } > > > > > > static const struct of_device_id smsc_usb3315_phy_match[] = { > > > { .compatible = "smsc,usb3315-phy", }, > > > { } > > > }; > > > MODULE_DEVICE_TABLE(of, smsc_usb3315_phy_match); > > > > > > static struct ulpi_driver smsc_usb3315_phy_driver = { > > > .probe = smsc_usb3315_phy_probe, > > > .driver = { > > > .name = "smsc_usb3315_phy", > > > .of_match_table = smsc_usb3315_phy_match, > > > }, > > > }; > > > module_ulpi_driver(smsc_usb3315_phy_driver); > > > > > > /*MODULE_ALIAS("platform:usb_phy_generic");*/ > > > MODULE_AUTHOR("GE Healthcare"); > > > MODULE_DESCRIPTION("SMSC USB 3315 ULPI Phy driver"); > > > MODULE_LICENSE("GPL v2"); > > > > > > I checked that the driver is registered by drivers/usb/common/ulpi.c:__ulpi_register_driver > > > successfully. > > > > Does the ulpi device have some vendor/product ids associated > > with it? The design is made to only fallback to matching the > > device to driver based on DT if the ulpi vendor id is 0. > > Otherwise, if vendor is non-zero you'll need to have a > > ulpi_device_id id table in your ulpi_driver structure. > > > > Hi, > > Thanks Stephen for your reply. > Indeed we have a vendor/product so I modify my code but without effect. > > After looking at the ulpi source code in the kernel, it seems that I need to call > ulpi_register_interface. ci_hdrc_probe should be called to execute the ulpi init. > > The problem is that we replace "fsl,usbphy = <&usbphy2>;" by an ulpi node but ci_hdrc_imx_probe > fail > because of "data->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "fsl,usbphy", 0);" > > So I will try to adapt ci_hdrc_imx_probe to continue phy initialisation if fsl,usbphy is missing. > Is it the good way to proceed? > > Thanks for any advice > Fabien > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-05-26 9:00 ` Fabien Lahoudere @ 2017-06-02 22:00 ` Stephen Boyd 2017-06-05 8:57 ` Fabien Lahoudere 0 siblings, 1 reply; 21+ messages in thread From: Stephen Boyd @ 2017-06-02 22:00 UTC (permalink / raw) To: Fabien Lahoudere Cc: Peter Chen, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list On 05/26, Fabien Lahoudere wrote: > Hello > > I modify ci_hrdc_imx_probe to bypass "data->phy = devm_usb_get_phy_by_phandle(&pdev->dev, > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init. > > The problem is that in ci_ulpi_init, before calling "ci->ulpi = ulpi_register_interface(ci->dev, > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is the > original function that make our system to hang. > > Our phy is not initialised before calling ulpi_register_interface so I don't understand how the phy > can reply if it is not out of reset state. I haven't see any problem in hw_phymode_configure(). What's the value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If you phy needs to be taken out of reset to reply to the ulpi reads of the vendor/product ids, then it sounds like you have a similar situation to what I had. I needed to turn on some regulators to get those reads to work, otherwise they would fail, but knowing what needed to be turned on basically meant I needed to probe the ulpi driver so probing the ids wasn't going to be useful. So on my device the reads for the ids go through, but they get all zeroes back, which is actually ok because there aren't any bits set on my devices anyway. After the reads see 0, we fallback to DT matching, which avoids the "bring it out of reset/power it on" sorts of problems entirely. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-06-02 22:00 ` Stephen Boyd @ 2017-06-05 8:57 ` Fabien Lahoudere 2017-06-05 9:43 ` Peter Chen 0 siblings, 1 reply; 21+ messages in thread From: Fabien Lahoudere @ 2017-06-05 8:57 UTC (permalink / raw) To: Stephen Boyd Cc: Peter Chen, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote: > On 05/26, Fabien Lahoudere wrote: > > Hello > > > > I modify ci_hrdc_imx_probe to bypass "data->phy = devm_usb_get_phy_by_phandle(&pdev->dev, > > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init. > > > > The problem is that in ci_ulpi_init, before calling "ci->ulpi = ulpi_register_interface(ci->dev, > > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is the > > original function that make our system to hang. > > > > Our phy is not initialised before calling ulpi_register_interface so I don't understand how the > > phy > > can reply if it is not out of reset state. > > I haven't see any problem in hw_phymode_configure(). What's the > value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If > you phy needs to be taken out of reset to reply to the ulpi reads > of the vendor/product ids, then it sounds like you have a similar > situation to what I had. I needed to turn on some regulators to > get those reads to work, otherwise they would fail, but knowing > what needed to be turned on basically meant I needed to probe the > ulpi driver so probing the ids wasn't going to be useful. So on > my device the reads for the ids go through, but they get all > zeroes back, which is actually ok because there aren't any bits > set on my devices anyway. After the reads see 0, we fallback to > DT matching, which avoids the "bring it out of reset/power it on" > sorts of problems entirely. > Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI. Indeed, this phy need to be out of reset to work. For example everything works fine if I call "_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);" This function only init reset GPIO and clock. For information, the original patch I have to fix the issue: diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 79ad8e9..21aaff1 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) case USBPHY_INTERFACE_MODE_UTMI: case USBPHY_INTERFACE_MODE_UTMIW: case USBPHY_INTERFACE_MODE_HSIC: + case USBPHY_INTERFACE_MODE_ULPI: ret = _ci_usb_phy_init(ci); if (!ret) hw_wait_phy_stable(); @@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) return ret; hw_phymode_configure(ci); break; - case USBPHY_INTERFACE_MODE_ULPI: case USBPHY_INTERFACE_MODE_SERIAL: hw_phymode_configure(ci); ret = _ci_usb_phy_init(ci); -- So if some ULPI phys need to be initialised before calling "hw_phymode_configure", is it acceptable if I separate "case USBPHY_INTERFACE_MODE_ULPI:" and add a DT binding ("init_phy_first") to define the order to call both functions? Something like: case USBPHY_INTERFACE_MODE_ULPI: if (ci->platdata->init_phy_first) { ret = _ci_usb_phy_init(ci); if (!ret) hw_wait_phy_stable(); else return ret; } hw_phymode_configure(ci); if (!ci->platdata->init_phy_first) { ret = _ci_usb_phy_init(ci); if (ret) return ret; } break; This approach will not modify current behaviour but allow to initialize phy first on demand. ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-06-05 8:57 ` Fabien Lahoudere @ 2017-06-05 9:43 ` Peter Chen 2017-06-05 9:52 ` Fabien Lahoudere 0 siblings, 1 reply; 21+ messages in thread From: Peter Chen @ 2017-06-05 9:43 UTC (permalink / raw) To: Fabien Lahoudere Cc: Stephen Boyd, Peter Chen, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list On Mon, Jun 05, 2017 at 10:57:00AM +0200, Fabien Lahoudere wrote: > On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote: > > On 05/26, Fabien Lahoudere wrote: > > > Hello > > > > > > I modify ci_hrdc_imx_probe to bypass "data->phy = devm_usb_get_phy_by_phandle(&pdev->dev, > > > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init. > > > > > > The problem is that in ci_ulpi_init, before calling "ci->ulpi = ulpi_register_interface(ci->dev, > > > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is the > > > original function that make our system to hang. > > > > > > Our phy is not initialised before calling ulpi_register_interface so I don't understand how the > > > phy > > > can reply if it is not out of reset state. > > > > I haven't see any problem in hw_phymode_configure(). What's the > > value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If > > you phy needs to be taken out of reset to reply to the ulpi reads > > of the vendor/product ids, then it sounds like you have a similar > > situation to what I had. I needed to turn on some regulators to > > get those reads to work, otherwise they would fail, but knowing > > what needed to be turned on basically meant I needed to probe the > > ulpi driver so probing the ids wasn't going to be useful. So on > > my device the reads for the ids go through, but they get all > > zeroes back, which is actually ok because there aren't any bits > > set on my devices anyway. After the reads see 0, we fallback to > > DT matching, which avoids the "bring it out of reset/power it on" > > sorts of problems entirely. > > > > Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI. > Indeed, this phy need to be out of reset to work. For example everything works fine if I call > "_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);" > This function only init reset GPIO and clock. > > For information, the original patch I have to fix the issue: > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > index 79ad8e9..21aaff1 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) > case USBPHY_INTERFACE_MODE_UTMI: > case USBPHY_INTERFACE_MODE_UTMIW: > case USBPHY_INTERFACE_MODE_HSIC: > + case USBPHY_INTERFACE_MODE_ULPI: > ret = _ci_usb_phy_init(ci); > if (!ret) > hw_wait_phy_stable(); > @@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) > return ret; > hw_phymode_configure(ci); > break; > - case USBPHY_INTERFACE_MODE_ULPI: > case USBPHY_INTERFACE_MODE_SERIAL: > hw_phymode_configure(ci); > ret = _ci_usb_phy_init(ci); > -- Currently, the hw_phymode_configure is called twice for ULPI PHY, the two execution are between _ci_usb_phy_init, would you test which one causes hang? If the second causes hang, you can make a patch for hw_phymode_configure that if the required PORTSC_PTS is the same the value in register, do noop. -- Best Regards, Peter Chen ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-06-05 9:43 ` Peter Chen @ 2017-06-05 9:52 ` Fabien Lahoudere 2017-06-06 1:55 ` Peter Chen 0 siblings, 1 reply; 21+ messages in thread From: Fabien Lahoudere @ 2017-06-05 9:52 UTC (permalink / raw) To: Peter Chen Cc: Stephen Boyd, Peter Chen, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list On Mon, 2017-06-05 at 17:43 +0800, Peter Chen wrote: > On Mon, Jun 05, 2017 at 10:57:00AM +0200, Fabien Lahoudere wrote: > > On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote: > > > On 05/26, Fabien Lahoudere wrote: > > > > Hello > > > > > > > > I modify ci_hrdc_imx_probe to bypass "data->phy = devm_usb_get_phy_by_phandle(&pdev->dev, > > > > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init. > > > > > > > > The problem is that in ci_ulpi_init, before calling "ci->ulpi = ulpi_register_interface(ci- > > > > >dev, > > > > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is the > > > > original function that make our system to hang. > > > > > > > > Our phy is not initialised before calling ulpi_register_interface so I don't understand how > > > > the > > > > phy > > > > can reply if it is not out of reset state. > > > > > > I haven't see any problem in hw_phymode_configure(). What's the > > > value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If > > > you phy needs to be taken out of reset to reply to the ulpi reads > > > of the vendor/product ids, then it sounds like you have a similar > > > situation to what I had. I needed to turn on some regulators to > > > get those reads to work, otherwise they would fail, but knowing > > > what needed to be turned on basically meant I needed to probe the > > > ulpi driver so probing the ids wasn't going to be useful. So on > > > my device the reads for the ids go through, but they get all > > > zeroes back, which is actually ok because there aren't any bits > > > set on my devices anyway. After the reads see 0, we fallback to > > > DT matching, which avoids the "bring it out of reset/power it on" > > > sorts of problems entirely. > > > > > > > Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI. > > Indeed, this phy need to be out of reset to work. For example everything works fine if I call > > "_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);" > > This function only init reset GPIO and clock. > > > > For information, the original patch I have to fix the issue: > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > index 79ad8e9..21aaff1 100644 > > --- a/drivers/usb/chipidea/core.c > > +++ b/drivers/usb/chipidea/core.c > > @@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) > > case USBPHY_INTERFACE_MODE_UTMI: > > case USBPHY_INTERFACE_MODE_UTMIW: > > case USBPHY_INTERFACE_MODE_HSIC: > > + case USBPHY_INTERFACE_MODE_ULPI: > > ret = _ci_usb_phy_init(ci); > > if (!ret) > > hw_wait_phy_stable(); > > @@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) > > return ret; > > hw_phymode_configure(ci); > > break; > > - case USBPHY_INTERFACE_MODE_ULPI: > > case USBPHY_INTERFACE_MODE_SERIAL: > > hw_phymode_configure(ci); > > ret = _ci_usb_phy_init(ci); > > -- > > Currently, the hw_phymode_configure is called twice for ULPI PHY, the > two execution are between _ci_usb_phy_init, would you test which one > causes hang? If the second causes hang, you can make a patch for > hw_phymode_configure that if the required PORTSC_PTS is the same > the value in register, do noop. The first one hangs, _ci_usb_phy_init is not called due to hang. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-06-05 9:52 ` Fabien Lahoudere @ 2017-06-06 1:55 ` Peter Chen 2017-06-06 17:36 ` Fabien Lahoudere 0 siblings, 1 reply; 21+ messages in thread From: Peter Chen @ 2017-06-06 1:55 UTC (permalink / raw) To: Fabien Lahoudere Cc: Stephen Boyd, Peter Chen, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list On Mon, Jun 05, 2017 at 11:52:26AM +0200, Fabien Lahoudere wrote: > On Mon, 2017-06-05 at 17:43 +0800, Peter Chen wrote: > > On Mon, Jun 05, 2017 at 10:57:00AM +0200, Fabien Lahoudere wrote: > > > On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote: > > > > On 05/26, Fabien Lahoudere wrote: > > > > > Hello > > > > > > > > > > I modify ci_hrdc_imx_probe to bypass "data->phy = devm_usb_get_phy_by_phandle(&pdev->dev, > > > > > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init. > > > > > > > > > > The problem is that in ci_ulpi_init, before calling "ci->ulpi = ulpi_register_interface(ci- > > > > > >dev, > > > > > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is the > > > > > original function that make our system to hang. > > > > > > > > > > Our phy is not initialised before calling ulpi_register_interface so I don't understand how > > > > > the > > > > > phy > > > > > can reply if it is not out of reset state. > > > > > > > > I haven't see any problem in hw_phymode_configure(). What's the > > > > value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If > > > > you phy needs to be taken out of reset to reply to the ulpi reads > > > > of the vendor/product ids, then it sounds like you have a similar > > > > situation to what I had. I needed to turn on some regulators to > > > > get those reads to work, otherwise they would fail, but knowing > > > > what needed to be turned on basically meant I needed to probe the > > > > ulpi driver so probing the ids wasn't going to be useful. So on > > > > my device the reads for the ids go through, but they get all > > > > zeroes back, which is actually ok because there aren't any bits > > > > set on my devices anyway. After the reads see 0, we fallback to > > > > DT matching, which avoids the "bring it out of reset/power it on" > > > > sorts of problems entirely. > > > > > > > > > > Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI. > > > Indeed, this phy need to be out of reset to work. For example everything works fine if I call > > > "_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);" > > > This function only init reset GPIO and clock. > > > > > > For information, the original patch I have to fix the issue: > > > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > > index 79ad8e9..21aaff1 100644 > > > --- a/drivers/usb/chipidea/core.c > > > +++ b/drivers/usb/chipidea/core.c > > > @@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) > > > case USBPHY_INTERFACE_MODE_UTMI: > > > case USBPHY_INTERFACE_MODE_UTMIW: > > > case USBPHY_INTERFACE_MODE_HSIC: > > > + case USBPHY_INTERFACE_MODE_ULPI: > > > ret = _ci_usb_phy_init(ci); > > > if (!ret) > > > hw_wait_phy_stable(); > > > @@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) > > > return ret; > > > hw_phymode_configure(ci); > > > break; > > > - case USBPHY_INTERFACE_MODE_ULPI: > > > case USBPHY_INTERFACE_MODE_SERIAL: > > > hw_phymode_configure(ci); > > > ret = _ci_usb_phy_init(ci); > > > -- > > > > Currently, the hw_phymode_configure is called twice for ULPI PHY, the > > two execution are between _ci_usb_phy_init, would you test which one > > causes hang? If the second causes hang, you can make a patch for > > hw_phymode_configure that if the required PORTSC_PTS is the same > > the value in register, do noop. > > The first one hangs, _ci_usb_phy_init is not called due to hang. > So, you need to comment out hw_phymode_configure at ci_ulpi_init, and you can't get vid/pid correctly, right? If it is, we may need to add power on sequence at chipidea core driver (ci_hdrc_probe) for clock and reset things. http://www.spinics.net/lists/linux-usb/msg157134.html I am wondering if we can call ci_usb_phy_init before calling ci_ulpi_init, since we need to let hardware be ready before reading vid/pid. Stephen & Fabien, does that work for you? -- Best Regards, Peter Chen ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-06-06 1:55 ` Peter Chen @ 2017-06-06 17:36 ` Fabien Lahoudere 2017-06-07 1:43 ` Peter Chen 0 siblings, 1 reply; 21+ messages in thread From: Fabien Lahoudere @ 2017-06-06 17:36 UTC (permalink / raw) To: Peter Chen Cc: Stephen Boyd, Peter Chen, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list Hi Peter, On Tue, 2017-06-06 at 09:55 +0800, Peter Chen wrote: > On Mon, Jun 05, 2017 at 11:52:26AM +0200, Fabien Lahoudere wrote: > > On Mon, 2017-06-05 at 17:43 +0800, Peter Chen wrote: > > > On Mon, Jun 05, 2017 at 10:57:00AM +0200, Fabien Lahoudere wrote: > > > > On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote: > > > > > On 05/26, Fabien Lahoudere wrote: > > > > > > Hello > > > > > > > > > > > > I modify ci_hrdc_imx_probe to bypass "data->phy = devm_usb_get_phy_by_phandle(&pdev- > > > > > > >dev, > > > > > > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init. > > > > > > > > > > > > The problem is that in ci_ulpi_init, before calling "ci->ulpi = > > > > > > ulpi_register_interface(ci- > > > > > > > dev, > > > > > > > > > > > > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is > > > > > > the > > > > > > original function that make our system to hang. > > > > > > > > > > > > Our phy is not initialised before calling ulpi_register_interface so I don't understand > > > > > > how > > > > > > the > > > > > > phy > > > > > > can reply if it is not out of reset state. > > > > > > > > > > I haven't see any problem in hw_phymode_configure(). What's the > > > > > value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If > > > > > you phy needs to be taken out of reset to reply to the ulpi reads > > > > > of the vendor/product ids, then it sounds like you have a similar > > > > > situation to what I had. I needed to turn on some regulators to > > > > > get those reads to work, otherwise they would fail, but knowing > > > > > what needed to be turned on basically meant I needed to probe the > > > > > ulpi driver so probing the ids wasn't going to be useful. So on > > > > > my device the reads for the ids go through, but they get all > > > > > zeroes back, which is actually ok because there aren't any bits > > > > > set on my devices anyway. After the reads see 0, we fallback to > > > > > DT matching, which avoids the "bring it out of reset/power it on" > > > > > sorts of problems entirely. > > > > > > > > > > > > > Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI. > > > > Indeed, this phy need to be out of reset to work. For example everything works fine if I > > > > call > > > > "_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);" > > > > This function only init reset GPIO and clock. > > > > > > > > For information, the original patch I have to fix the issue: > > > > > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > > > index 79ad8e9..21aaff1 100644 > > > > --- a/drivers/usb/chipidea/core.c > > > > +++ b/drivers/usb/chipidea/core.c > > > > @@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) > > > > case USBPHY_INTERFACE_MODE_UTMI: > > > > case USBPHY_INTERFACE_MODE_UTMIW: > > > > case USBPHY_INTERFACE_MODE_HSIC: > > > > + case USBPHY_INTERFACE_MODE_ULPI: > > > > ret = _ci_usb_phy_init(ci); > > > > if (!ret) > > > > hw_wait_phy_stable(); > > > > @@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) > > > > return ret; > > > > hw_phymode_configure(ci); > > > > break; > > > > - case USBPHY_INTERFACE_MODE_ULPI: > > > > case USBPHY_INTERFACE_MODE_SERIAL: > > > > hw_phymode_configure(ci); > > > > ret = _ci_usb_phy_init(ci); > > > > -- > > > > > > Currently, the hw_phymode_configure is called twice for ULPI PHY, the > > > two execution are between _ci_usb_phy_init, would you test which one > > > causes hang? If the second causes hang, you can make a patch for > > > hw_phymode_configure that if the required PORTSC_PTS is the same > > > the value in register, do noop. > > > > The first one hangs, _ci_usb_phy_init is not called due to hang. > > > > So, you need to comment out hw_phymode_configure at ci_ulpi_init, and you > can't get vid/pid correctly, right? If it is, we may need to add power on > sequence at chipidea core driver (ci_hdrc_probe) for clock and reset things. > > http://www.spinics.net/lists/linux-usb/msg157134.html > > I am wondering if we can call ci_usb_phy_init before calling ci_ulpi_init, > since we need to let hardware be ready before reading vid/pid. > Stephen & Fabien, does that work for you? > I test the following patch and it works. But I am not sure that we can move safely ci_ulpi_init. I will investigate more tomorrow if it is a problem for other phys. Is it a good approach? Subject: [PATCH 1/1] power on phy before getting vid/pid Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> --- drivers/usb/chipidea/core.c | 12 ++++++++---- drivers/usb/chipidea/ulpi.c | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 79ad8e9..26889e1 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -879,10 +879,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) return -ENODEV; } - ret = ci_ulpi_init(ci); - if (ret) - return ret; - if (ci->platdata->phy) { ci->phy = ci->platdata->phy; } else if (ci->platdata->usb_phy) { @@ -909,6 +905,14 @@ static int ci_hdrc_probe(struct platform_device *pdev) ci->usb_phy = NULL; } + /* + We move this in order to have phy reset and gpio information + before calling ci_ulpi_init. + */ + ret = ci_ulpi_init(ci); + if (ret) + return ret; + ret = ci_usb_phy_init(ci); if (ret) { dev_err(dev, "unable to init phy: %d\n", ret); diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c index 1219583..1c272e4 100644 --- a/drivers/usb/chipidea/ulpi.c +++ b/drivers/usb/chipidea/ulpi.c @@ -73,9 +73,28 @@ static int ci_ulpi_write(struct device *dev, u8 addr, u8 val) int ci_ulpi_init(struct ci_hdrc *ci) { + int ret; if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI) return 0; + /* + This is the content of _ci_usb_phy_init from core.c to power on the phy. + Duplicated for test purpose only. + */ + if (ci->phy) { + ret = phy_init(ci->phy); + if (ret) + return ret; + + ret = phy_power_on(ci->phy); + if (ret) { + phy_exit(ci->phy); + return ret; + } + } else { + ret = usb_phy_init(ci->usb_phy); + } + /* * Set PORTSC correctly so we can read/write ULPI registers for * identification purposes -- 1.8.3.1 Thanks Fabien ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-06-06 17:36 ` Fabien Lahoudere @ 2017-06-07 1:43 ` Peter Chen 2017-06-07 15:00 ` Fabien Lahoudere 2017-06-08 12:27 ` Fabien Lahoudere 0 siblings, 2 replies; 21+ messages in thread From: Peter Chen @ 2017-06-07 1:43 UTC (permalink / raw) To: Fabien Lahoudere Cc: Stephen Boyd, Peter Chen, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list On Tue, Jun 06, 2017 at 07:36:10PM +0200, Fabien Lahoudere wrote: > Hi Peter, > > On Tue, 2017-06-06 at 09:55 +0800, Peter Chen wrote: > > On Mon, Jun 05, 2017 at 11:52:26AM +0200, Fabien Lahoudere wrote: > > > On Mon, 2017-06-05 at 17:43 +0800, Peter Chen wrote: > > > > On Mon, Jun 05, 2017 at 10:57:00AM +0200, Fabien Lahoudere wrote: > > > > > On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote: > > > > > > On 05/26, Fabien Lahoudere wrote: > > > > > > > Hello > > > > > > > > > > > > > > I modify ci_hrdc_imx_probe to bypass "data->phy = devm_usb_get_phy_by_phandle(&pdev- > > > > > > > >dev, > > > > > > > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init. > > > > > > > > > > > > > > The problem is that in ci_ulpi_init, before calling "ci->ulpi = > > > > > > > ulpi_register_interface(ci- > > > > > > > > dev, > > > > > > > > > > > > > > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called which is > > > > > > > the > > > > > > > original function that make our system to hang. > > > > > > > > > > > > > > Our phy is not initialised before calling ulpi_register_interface so I don't understand > > > > > > > how > > > > > > > the > > > > > > > phy > > > > > > > can reply if it is not out of reset state. > > > > > > > > > > > > I haven't see any problem in hw_phymode_configure(). What's the > > > > > > value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If > > > > > > you phy needs to be taken out of reset to reply to the ulpi reads > > > > > > of the vendor/product ids, then it sounds like you have a similar > > > > > > situation to what I had. I needed to turn on some regulators to > > > > > > get those reads to work, otherwise they would fail, but knowing > > > > > > what needed to be turned on basically meant I needed to probe the > > > > > > ulpi driver so probing the ids wasn't going to be useful. So on > > > > > > my device the reads for the ids go through, but they get all > > > > > > zeroes back, which is actually ok because there aren't any bits > > > > > > set on my devices anyway. After the reads see 0, we fallback to > > > > > > DT matching, which avoids the "bring it out of reset/power it on" > > > > > > sorts of problems entirely. > > > > > > > > > > > > > > > > Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI. > > > > > Indeed, this phy need to be out of reset to work. For example everything works fine if I > > > > > call > > > > > "_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);" > > > > > This function only init reset GPIO and clock. > > > > > > > > > > For information, the original patch I have to fix the issue: > > > > > > > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > > > > index 79ad8e9..21aaff1 100644 > > > > > --- a/drivers/usb/chipidea/core.c > > > > > +++ b/drivers/usb/chipidea/core.c > > > > > @@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) > > > > > case USBPHY_INTERFACE_MODE_UTMI: > > > > > case USBPHY_INTERFACE_MODE_UTMIW: > > > > > case USBPHY_INTERFACE_MODE_HSIC: > > > > > + case USBPHY_INTERFACE_MODE_ULPI: > > > > > ret = _ci_usb_phy_init(ci); > > > > > if (!ret) > > > > > hw_wait_phy_stable(); > > > > > @@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) > > > > > return ret; > > > > > hw_phymode_configure(ci); > > > > > break; > > > > > - case USBPHY_INTERFACE_MODE_ULPI: > > > > > case USBPHY_INTERFACE_MODE_SERIAL: > > > > > hw_phymode_configure(ci); > > > > > ret = _ci_usb_phy_init(ci); > > > > > -- > > > > > > > > Currently, the hw_phymode_configure is called twice for ULPI PHY, the > > > > two execution are between _ci_usb_phy_init, would you test which one > > > > causes hang? If the second causes hang, you can make a patch for > > > > hw_phymode_configure that if the required PORTSC_PTS is the same > > > > the value in register, do noop. > > > > > > The first one hangs, _ci_usb_phy_init is not called due to hang. > > > > > > > So, you need to comment out hw_phymode_configure at ci_ulpi_init, and you > > can't get vid/pid correctly, right? If it is, we may need to add power on > > sequence at chipidea core driver (ci_hdrc_probe) for clock and reset things. > > > > http://www.spinics.net/lists/linux-usb/msg157134.html > > > > I am wondering if we can call ci_usb_phy_init before calling ci_ulpi_init, > > since we need to let hardware be ready before reading vid/pid. > > Stephen & Fabien, does that work for you? > > > > I test the following patch and it works. But I am not sure that we can move safely ci_ulpi_init. > I will investigate more tomorrow if it is a problem for other phys. > > Is it a good approach? > > Subject: [PATCH 1/1] power on phy before getting vid/pid > > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> > --- > drivers/usb/chipidea/core.c | 12 ++++++++---- > drivers/usb/chipidea/ulpi.c | 19 +++++++++++++++++++ > 2 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > index 79ad8e9..26889e1 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -879,10 +879,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) > return -ENODEV; > } > > - ret = ci_ulpi_init(ci); > - if (ret) > - return ret; > - > if (ci->platdata->phy) { > ci->phy = ci->platdata->phy; > } else if (ci->platdata->usb_phy) { > @@ -909,6 +905,14 @@ static int ci_hdrc_probe(struct platform_device *pdev) > ci->usb_phy = NULL; > } > > + /* > + We move this in order to have phy reset and gpio information > + before calling ci_ulpi_init. > + */ > + ret = ci_ulpi_init(ci); > + if (ret) > + return ret; > + > ret = ci_usb_phy_init(ci); > if (ret) { > dev_err(dev, "unable to init phy: %d\n", ret); > diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c > index 1219583..1c272e4 100644 > --- a/drivers/usb/chipidea/ulpi.c > +++ b/drivers/usb/chipidea/ulpi.c > @@ -73,9 +73,28 @@ static int ci_ulpi_write(struct device *dev, u8 addr, u8 val) > > int ci_ulpi_init(struct ci_hdrc *ci) > { > + int ret; > if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI) > return 0; > > + /* > + This is the content of _ci_usb_phy_init from core.c to power on the phy. > + Duplicated for test purpose only. > + */ > + if (ci->phy) { > + ret = phy_init(ci->phy); > + if (ret) > + return ret; > + > + ret = phy_power_on(ci->phy); > + if (ret) { > + phy_exit(ci->phy); > + return ret; > + } > + } else { > + ret = usb_phy_init(ci->usb_phy); > + } > + > /* > * Set PORTSC correctly so we can read/write ULPI registers for > * identification purposes Your above patch is not accepted. I don't know ULPI PHY behavior at imx53, would you please make clear below things: - Before setting phy mode at portsc, which you need to do? If they can't be in phy_init, you may register a power sequence instance. - Can you read pid/vid correctly, and which way you would like to match your ulpi device, pid/vid or using device tree, see ulpi_match -- Best Regards, Peter Chen ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-06-07 1:43 ` Peter Chen @ 2017-06-07 15:00 ` Fabien Lahoudere 2017-06-08 12:27 ` Fabien Lahoudere 1 sibling, 0 replies; 21+ messages in thread From: Fabien Lahoudere @ 2017-06-07 15:00 UTC (permalink / raw) To: Peter Chen Cc: Stephen Boyd, Peter Chen, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list On Wed, 2017-06-07 at 09:43 +0800, Peter Chen wrote: > On Tue, Jun 06, 2017 at 07:36:10PM +0200, Fabien Lahoudere wrote: > > Hi Peter, > > > > On Tue, 2017-06-06 at 09:55 +0800, Peter Chen wrote: > > > On Mon, Jun 05, 2017 at 11:52:26AM +0200, Fabien Lahoudere wrote: > > > > On Mon, 2017-06-05 at 17:43 +0800, Peter Chen wrote: > > > > > On Mon, Jun 05, 2017 at 10:57:00AM +0200, Fabien Lahoudere wrote: > > > > > > On Fri, 2017-06-02 at 15:00 -0700, Stephen Boyd wrote: > > > > > > > On 05/26, Fabien Lahoudere wrote: > > > > > > > > Hello > > > > > > > > > > > > > > > > I modify ci_hrdc_imx_probe to bypass "data->phy = devm_usb_get_phy_by_phandle(&pdev- > > > > > > > > > dev, > > > > > > > > > > > > > > > > "fsl,usbphy", 0);". Everything works as expected and call ci_ulpi_init. > > > > > > > > > > > > > > > > The problem is that in ci_ulpi_init, before calling "ci->ulpi = > > > > > > > > ulpi_register_interface(ci- > > > > > > > > > dev, > > > > > > > > > > > > > > > > &ci->ulpi_ops);" (to initialize our phy), "hw_phymode_configure(ci);" is called > > > > > > > > which is > > > > > > > > the > > > > > > > > original function that make our system to hang. > > > > > > > > > > > > > > > > Our phy is not initialised before calling ulpi_register_interface so I don't > > > > > > > > understand > > > > > > > > how > > > > > > > > the > > > > > > > > phy > > > > > > > > can reply if it is not out of reset state. > > > > > > > > > > > > > > I haven't see any problem in hw_phymode_configure(). What's the > > > > > > > value of ci->platdata->phy_mode? USBPHY_INTERFACE_MODE_ULPI? If > > > > > > > you phy needs to be taken out of reset to reply to the ulpi reads > > > > > > > of the vendor/product ids, then it sounds like you have a similar > > > > > > > situation to what I had. I needed to turn on some regulators to > > > > > > > get those reads to work, otherwise they would fail, but knowing > > > > > > > what needed to be turned on basically meant I needed to probe the > > > > > > > ulpi driver so probing the ids wasn't going to be useful. So on > > > > > > > my device the reads for the ids go through, but they get all > > > > > > > zeroes back, which is actually ok because there aren't any bits > > > > > > > set on my devices anyway. After the reads see 0, we fallback to > > > > > > > DT matching, which avoids the "bring it out of reset/power it on" > > > > > > > sorts of problems entirely. > > > > > > > > > > > > > > > > > > > Yes the phy mode is configured to USBPHY_INTERFACE_MODE_ULPI. > > > > > > Indeed, this phy need to be out of reset to work. For example everything works fine if I > > > > > > call > > > > > > "_ci_usb_phy_init(ci);" before calling "hw_phymode_configure(ci);" > > > > > > This function only init reset GPIO and clock. > > > > > > > > > > > > For information, the original patch I have to fix the issue: > > > > > > > > > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > > > > > index 79ad8e9..21aaff1 100644 > > > > > > --- a/drivers/usb/chipidea/core.c > > > > > > +++ b/drivers/usb/chipidea/core.c > > > > > > @@ -391,6 +391,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) > > > > > > case USBPHY_INTERFACE_MODE_UTMI: > > > > > > case USBPHY_INTERFACE_MODE_UTMIW: > > > > > > case USBPHY_INTERFACE_MODE_HSIC: > > > > > > + case USBPHY_INTERFACE_MODE_ULPI: > > > > > > ret = _ci_usb_phy_init(ci); > > > > > > if (!ret) > > > > > > hw_wait_phy_stable(); > > > > > > @@ -398,7 +399,6 @@ static int ci_usb_phy_init(struct ci_hdrc *ci) > > > > > > return ret; > > > > > > hw_phymode_configure(ci); > > > > > > break; > > > > > > - case USBPHY_INTERFACE_MODE_ULPI: > > > > > > case USBPHY_INTERFACE_MODE_SERIAL: > > > > > > hw_phymode_configure(ci); > > > > > > ret = _ci_usb_phy_init(ci); > > > > > > -- > > > > > > > > > > Currently, the hw_phymode_configure is called twice for ULPI PHY, the > > > > > two execution are between _ci_usb_phy_init, would you test which one > > > > > causes hang? If the second causes hang, you can make a patch for > > > > > hw_phymode_configure that if the required PORTSC_PTS is the same > > > > > the value in register, do noop. > > > > > > > > The first one hangs, _ci_usb_phy_init is not called due to hang. > > > > > > > > > > So, you need to comment out hw_phymode_configure at ci_ulpi_init, and you > > > can't get vid/pid correctly, right? If it is, we may need to add power on > > > sequence at chipidea core driver (ci_hdrc_probe) for clock and reset things. > > > > > > http://www.spinics.net/lists/linux-usb/msg157134.html > > > > > > I am wondering if we can call ci_usb_phy_init before calling ci_ulpi_init, > > > since we need to let hardware be ready before reading vid/pid. > > > Stephen & Fabien, does that work for you? > > > > > > > I test the following patch and it works. But I am not sure that we can move safely ci_ulpi_init. > > I will investigate more tomorrow if it is a problem for other phys. > > > > Is it a good approach? > > > > Subject: [PATCH 1/1] power on phy before getting vid/pid > > > > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> > > --- > > drivers/usb/chipidea/core.c | 12 ++++++++---- > > drivers/usb/chipidea/ulpi.c | 19 +++++++++++++++++++ > > 2 files changed, 27 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > index 79ad8e9..26889e1 100644 > > --- a/drivers/usb/chipidea/core.c > > +++ b/drivers/usb/chipidea/core.c > > @@ -879,10 +879,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > return -ENODEV; > > } > > > > - ret = ci_ulpi_init(ci); > > - if (ret) > > - return ret; > > - > > if (ci->platdata->phy) { > > ci->phy = ci->platdata->phy; > > } else if (ci->platdata->usb_phy) { > > @@ -909,6 +905,14 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > ci->usb_phy = NULL; > > } > > > > + /* > > + We move this in order to have phy reset and gpio information > > + before calling ci_ulpi_init. > > + */ > > + ret = ci_ulpi_init(ci); > > + if (ret) > > + return ret; > > + > > ret = ci_usb_phy_init(ci); > > if (ret) { > > dev_err(dev, "unable to init phy: %d\n", ret); > > diff --git a/drivers/usb/chipidea/ulpi.c b/drivers/usb/chipidea/ulpi.c > > index 1219583..1c272e4 100644 > > --- a/drivers/usb/chipidea/ulpi.c > > +++ b/drivers/usb/chipidea/ulpi.c > > @@ -73,9 +73,28 @@ static int ci_ulpi_write(struct device *dev, u8 addr, u8 val) > > > > int ci_ulpi_init(struct ci_hdrc *ci) > > { > > + int ret; > > if (ci->platdata->phy_mode != USBPHY_INTERFACE_MODE_ULPI) > > return 0; > > > > + /* > > + This is the content of _ci_usb_phy_init from core.c to power on the phy. > > + Duplicated for test purpose only. > > + */ > > + if (ci->phy) { > > + ret = phy_init(ci->phy); > > + if (ret) > > + return ret; > > + > > + ret = phy_power_on(ci->phy); > > + if (ret) { > > + phy_exit(ci->phy); > > + return ret; > > + } > > + } else { > > + ret = usb_phy_init(ci->usb_phy); > > + } > > + > > /* > > * Set PORTSC correctly so we can read/write ULPI registers for > > * identification purposes > > Your above patch is not accepted. I don't know ULPI PHY behavior at > imx53, would you please make clear below things: > > - Before setting phy mode at portsc, which you need to do? I need to prepare clock and enable vcc. Calling "usb_phy_init" before "ci_ulpi_init" works. In my case usb_phy_init points to usb_gen_phy_init() from phy-generic.c which basically do: regulator_enable(nop->vcc); clk_prepare_enable(nop->clk); nop_reset(nop); > If they can't be in phy_init, you may register a power sequence > instance. How can I register a power sequence? by adding clock in a subnode of usbh2 node? > - Can you read pid/vid correctly, and which way you would > like to match your ulpi device, pid/vid or using device > tree, see ulpi_match Yes I can read pid/vid (ulpi ci_hdrc.2.ulpi: registered ULPI PHY: vendor 0424, product 0006) I am not sure which way I prefer. The better seems to be pid/vid. > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-06-07 1:43 ` Peter Chen 2017-06-07 15:00 ` Fabien Lahoudere @ 2017-06-08 12:27 ` Fabien Lahoudere 2017-06-09 8:26 ` Peter Chen 1 sibling, 1 reply; 21+ messages in thread From: Fabien Lahoudere @ 2017-06-08 12:27 UTC (permalink / raw) To: Peter Chen Cc: Stephen Boyd, Peter Chen, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list Hi Peter I have a look at your patches (http://www.spinics.net/lists/linux-usb/msg157134.html) and I wonder if power sequence is applicable only on hub node? hub are probed too late (after ci_ulpi_init). Do you think it is possible to read a power sequence in dts from ci_ulpi_init? Thanks Fabien ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-06-08 12:27 ` Fabien Lahoudere @ 2017-06-09 8:26 ` Peter Chen 2017-06-09 11:17 ` Fabien Lahoudere 0 siblings, 1 reply; 21+ messages in thread From: Peter Chen @ 2017-06-09 8:26 UTC (permalink / raw) To: Fabien Lahoudere, Peter Chen Cc: Stephen Boyd, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list > >I have a look at your patches (http://www.spinics.net/lists/linux-usb/msg157134.html) >and I wonder if power sequence is applicable only on hub node? No, power sequence can be used for any devices which needs to carry out power on before probe. > hub are probed too >late (after ci_ulpi_init). Do you think it is possible to read a power sequence in dts >from ci_ulpi_init? > I think the suitable place for power sequence is at ulpi_of_register, some ULPI PHYs need to carry out power on before reading ID. Peter ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-06-09 8:26 ` Peter Chen @ 2017-06-09 11:17 ` Fabien Lahoudere 2017-06-12 1:20 ` Peter Chen 0 siblings, 1 reply; 21+ messages in thread From: Fabien Lahoudere @ 2017-06-09 11:17 UTC (permalink / raw) To: Peter Chen Cc: Stephen Boyd, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list Hi Peter On Fri, 2017-06-09 at 08:26 +0000, Peter Chen wrote: > > > > > I have a look at your patches (http://www.spinics.net/lists/linux-usb/msg157134.html) > > and I wonder if power sequence is applicable only on hub node? > > No, power sequence can be used for any devices which needs to carry out power on > before probe. > > > hub are probed too > > late (after ci_ulpi_init). Do you think it is possible to read a power sequence in dts > > from ci_ulpi_init? > > > > I think the suitable place for power sequence is at ulpi_of_register, some ULPI PHYs > need to carry out power on before reading ID. > I do some test to verify that ulpi_of_register is called before hw_phymode_configure but it is not the case. Can you confirm that ULPI phys works with IMX6? It seems that IMX53 and IMX6 use the same chipidea controller and should have the same behaviour. So I wonder if the issue I encounter can be a SOC issue. Thanks > Peter ^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [RFC] usb-phy-generic: Add support to SMSC USB3315 2017-06-09 11:17 ` Fabien Lahoudere @ 2017-06-12 1:20 ` Peter Chen 0 siblings, 0 replies; 21+ messages in thread From: Peter Chen @ 2017-06-12 1:20 UTC (permalink / raw) To: Fabien Lahoudere Cc: Stephen Boyd, Peter Senna Tschudin, Felipe Balbi, Greg Kroah-Hartman, open list:USB PHY LAYER, open list > >Can you confirm that ULPI phys works with IMX6? > >It seems that IMX53 and IMX6 use the same chipidea controller and should have the >same behaviour. So I wonder if the issue I encounter can be a SOC issue. > Fabien, all imx6 series uses UTMI PHYs, so I am afraid I can't verify it. Peter ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-06-12 1:20 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-19 6:14 [RFC] usb-phy-generic: Add support to SMSC USB3315 Peter Senna Tschudin 2017-04-19 10:03 ` Sergei Shtylyov 2017-04-19 10:24 ` Peter Senna Tschudin 2017-04-19 17:23 ` Sergei Shtylyov 2017-04-20 8:50 ` Peter Chen 2017-05-23 18:16 ` Fabien Lahoudere 2017-05-23 21:00 ` Stephen Boyd 2017-05-25 10:36 ` Fabien Lahoudere 2017-05-26 9:00 ` Fabien Lahoudere 2017-06-02 22:00 ` Stephen Boyd 2017-06-05 8:57 ` Fabien Lahoudere 2017-06-05 9:43 ` Peter Chen 2017-06-05 9:52 ` Fabien Lahoudere 2017-06-06 1:55 ` Peter Chen 2017-06-06 17:36 ` Fabien Lahoudere 2017-06-07 1:43 ` Peter Chen 2017-06-07 15:00 ` Fabien Lahoudere 2017-06-08 12:27 ` Fabien Lahoudere 2017-06-09 8:26 ` Peter Chen 2017-06-09 11:17 ` Fabien Lahoudere 2017-06-12 1:20 ` Peter Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).