linux-kernel - Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY

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]

Message-ID: <20150126125503.GB28539@kuha.fi.intel.com>
Date:	Mon, 26 Jan 2015 14:55:03 +0200
From:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:	David Cohen <david.a.cohen@...ux.intel.com>
Cc:	Felipe Balbi <balbi@...com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Baolu Lu <baolu.lu@...ux.intel.com>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Kishon Vijay Abraham I <kishon@...com>
Subject: Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY

Hi David,

On Sat, Jan 24, 2015 at 03:58:11PM -0800, David Cohen wrote:
> > +static int tusb1210_power_on(struct phy *phy)
> > +{
> > +	struct tusb1210 *tusb = phy_get_drvdata(phy);
> > +
> > +	gpiod_set_value_cansleep(tusb->gpio_reset, 1);
> > +	gpiod_set_value_cansleep(tusb->gpio_cs, 1);
> > +
> > +	/* Restore eye diagram optimisation value */
> > +	ulpi_write(tusb->ulpi, TUSB1210_VENDOR_SPECIFIC2,
> > +		   tusb->eye_diagram_tuning);
> 
> After you power on phy, ulpi bus may not be available right away. In
> intel case, phy power on happens during dwc3 power on. ULPI bus is not
> available until OTG controller and phy are in sync.
> 
> In resume, you can't restore eye diagram from here.

I'm sorry but I don't think I understand? Where do we power on the phy
before dwc3 is powered on? Or is this a Baytrail-CR specific problem?
I can't see any problems with the hardware I have.

In any case, this sounds like purely dwc3 issue and not tusb1210
issue.

> > +static int tusb1210_probe(struct ulpi *ulpi)
> > +{
> > +	struct gpio_desc *gpio;
> > +	struct tusb1210 *tusb;
> > +	int ret;
> > +
> > +	tusb = devm_kzalloc(&ulpi->dev, sizeof(*tusb), GFP_KERNEL);
> > +	if (!tusb)
> > +		return -ENOMEM;
> > +
> > +	gpio = devm_gpiod_get(&ulpi->dev, "reset");
> > +	if (!IS_ERR(gpio)) {
> > +		ret = gpiod_direction_output(gpio, 0);
> > +		if (ret)
> > +			return ret;
> > +		tusb->gpio_reset = gpio;
> > +	}
> 
> You cannot proceed with probe if gpio reset is not available. Different
> from CS, it's a mandatory pin to toggle in order to power on/off phy and
> get it in sync with OTG controller.

Well, let's check -ENOENT and -ENODEV return values separately. The
reset pin is not used on all platforms so getting this gpio is
optional. This is the case even with some Intel's platforms using
TUSB1210.

> > +
> > +	gpio = devm_gpiod_get(&ulpi->dev, "cs");
> > +	if (!IS_ERR(gpio)) {
> > +		ret = gpiod_direction_output(gpio, 0);
> > +		if (ret)
> > +			return ret;
> > +		tusb->gpio_cs = gpio;
> > +	}
> > +
> > +	/* Store initial eye diagram optimisation value */
> > +	ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> 
> It's unclear if ulpi is accessible at this point. You can't read it at
> this point.

We wouldn't have reached this point if ulpi wasn't accessible.
Registering the ulpi interface would have already failed so no driver
would have been probed.


Thanks!

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/