[RFC] usb-phy-generic: Add support to SMSC USB3315

[RFC] usb-phy-generic: Add support to SMSC USB3315

[Peter Senna Tschudin]

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

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Sergei Shtylyov]

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

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Peter Senna Tschudin]

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?


> 

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Sergei Shtylyov]

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

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Peter Chen]

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

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Fabien Lahoudere]

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 = <&reg_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
> > 
> 
> 

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Stephen Boyd]

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 = <&reg_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

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Fabien Lahoudere]

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 = <&reg_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

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Fabien Lahoudere]

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 = <&reg_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
> 

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Stephen Boyd]

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

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Fabien Lahoudere]

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.

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Peter Chen]

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

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Fabien Lahoudere]

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.

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Peter Chen]

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

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Fabien Lahoudere]

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

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Peter Chen]

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

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Fabien Lahoudere]

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.


> 

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Fabien Lahoudere]

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

RE: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Peter Chen]

 
>
>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

Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Fabien Lahoudere]

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

RE: [RFC] usb-phy-generic: Add support to SMSC USB3315

[Peter Chen]

 
>
>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