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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 05 Jun 2017 10:57:00 +0200
From:   Fabien Lahoudere <fabien.lahoudere@...labora.co.uk>
To:     Stephen Boyd <sboyd@...eaurora.org>
Cc:     Peter Chen <peter.chen@....com>,
        Peter Senna Tschudin <peter.senna@...labora.com>,
        Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "open list:USB PHY LAYER" <linux-usb@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] usb-phy-generic: Add support to SMSC USB3315

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.