Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650365 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> - 2017-05-25 12:40 +0200
    Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> - 2017-05-26 11:10 +0200
      Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 Stephen Boyd <sboyd@codeaurora.org> - 2017-06-03 00:10 +0200
        Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> - 2017-06-05 11:00 +0200
          Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> - 2017-06-05 12:00 +0200
            Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> - 2017-06-06 19:40 +0200
              Re: [RFC] usb-phy-generic: Add support to SMSC USB3315 Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> - 2017-06-07 17:10 +0200

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

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

[toc] | [next] | [standalone]

#1651242

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
> 

[toc] | [prev] | [next] | [standalone]

#1656601

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

[toc] | [prev] | [next] | [standalone]

#1657401

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.

[toc] | [prev] | [next] | [standalone]

#1657470

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.

[toc] | [prev] | [next] | [standalone]

#1658977

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

[toc] | [prev] | [next] | [standalone]

#1659915

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.


> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web