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.