diff mbox

[RFT,1/3] usb: misc: usb3503: Fix HUB mode after bootloader initialization

Message ID 1461927591-7864-2-git-send-email-k.kozlowski@samsung.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Krzysztof Kozlowski April 29, 2016, 10:59 a.m. UTC
On Odroid U3 (Exynos4412-based) board if USB was initialized by
bootloader (in U-Boot "usb start" before tftpboot), the HUB after after
successful probing was not visible in the system ("lsusb"). Connected
devices were not visible neither.

In such case the USB3503 has to be fully reset before configuring to HUB
mode. Reset by GPIO (called RESET_N pin) and by RESET field in STCD
register are not sufficient. Instead full reset has to be done by
disabling and enabling regulator.

The USB3503 can work with different regulator configurations, however
toggling of only one is relevant in mentioned case: the VDD 3.3V.

The patch adds:
1. New binding for optional regulator (VDD33),
2. Code for toggling the regulator on/off before doing reset by GPIO,
3. Initial reset during probing before configuring HUB mode.

Patch is very loosely based on Tobias Jakobi's similar work for usb3503.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

---

Tested on Odroid U3 so far. Please kindly test on X2 or other
configurations and bootloaders.
---
 Documentation/devicetree/bindings/usb/usb3503.txt |  1 +
 drivers/usb/misc/usb3503.c                        | 81 +++++++++++++++++++++++
 2 files changed, 82 insertions(+)

Comments

Mark Brown April 29, 2016, 11:30 a.m. UTC | #1
On Fri, Apr 29, 2016 at 12:59:49PM +0200, Krzysztof Kozlowski wrote:

> +++ b/Documentation/devicetree/bindings/usb/usb3503.txt
> @@ -24,6 +24,7 @@ Optional properties:
>  	pins (optional, if not provided, driver will not set rate of the
>  	REFCLK signal and assume that a value from the primary reference
>  	clock frequencies table is used)
> +- vdd33-supply: Optional supply for VDD 3.3 V power source.

Supplies are only optional if they may be physically absent.  In this
case it's possible that on device regulators may be used instead, a
pattern more like that used for arizona-ldo1 where we represent those
regulators might be better as it's more clearly describing the
situation.  I'm just wondering if the supply lookup stuff there should
be factored out as this is not an uncommon pattern..

It should at least be clearly stated what's going on, ignoring failure
to get supplies is generally a bug and people will tend to blindly cut
and paste things (witness all the breakage in graphics drivers with
this).

>  static int usb3503_reset(struct usb3503 *hub, int state)
>  {
> +	int err;
> +
> +	err = usb3503_regulator(hub, state);
> +	if (err) {
> +		dev_err(hub->dev, "unable to %s VDD33 regulator to (%d)\n",
> +			(state ? "enable" : "disable"), err);
> +	}

Are we sure that the callers all balance enables and disables and we
don't ever end up going through reset more than once on the way down?

> +		hub->vdd_reg = devm_regulator_get_optional(dev, "vdd33");
> +		if (IS_ERR(hub->vdd_reg)) {
> +			if (PTR_ERR(hub->vdd_reg) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;

This should explicitly check for -ENODEV and return the error if it gets
anything else, that will mean that if the supply is needed but lookup
fails somehow due to a non-deferral error we'll handle it properly.

> +	err = usb3503_regulator(hub, true);

The naming on this function is very obscure (and there's also a couple
of other supplies).  I'd suggest just folding this into the reset
function, or at least renaming so the reader can tell what these calls
do.
Krzysztof Kozlowski April 29, 2016, 11:55 a.m. UTC | #2
On 04/29/2016 01:30 PM, Mark Brown wrote:
> On Fri, Apr 29, 2016 at 12:59:49PM +0200, Krzysztof Kozlowski wrote:
> 
>> +++ b/Documentation/devicetree/bindings/usb/usb3503.txt
>> @@ -24,6 +24,7 @@ Optional properties:
>>  	pins (optional, if not provided, driver will not set rate of the
>>  	REFCLK signal and assume that a value from the primary reference
>>  	clock frequencies table is used)
>> +- vdd33-supply: Optional supply for VDD 3.3 V power source.
> 
> Supplies are only optional if they may be physically absent.  In this
> case it's possible that on device regulators may be used instead, a
> pattern more like that used for arizona-ldo1 where we represent those
> regulators might be better as it's more clearly describing the
> situation.  I'm just wondering if the supply lookup stuff there should
> be factored out as this is not an uncommon pattern..
> 
> It should at least be clearly stated what's going on, ignoring failure
> to get supplies is generally a bug and people will tend to blindly cut
> and paste things (witness all the breakage in graphics drivers with
> this).

The device has four power input lines (called VBAT, VDD33, VDD_CORE and
VDD_12). Datasheet describes 4 valid configurations... but impression of
the Odroid U3 board schematics is that they used another (custom?)
configuration.

I did not add rest of regulators on purpose:
1. I don't have other configurations to test.
2. It is rather old device, so I don't expect active development.

The VDD33 is really optional. The device can work in different
configuration, e.g. only on VBAT. How the reset logic would work then? I
don't know... I would suspect that it could be exactly the same (just
replace VDD33 with VBAT) but I am not sure.

>>  static int usb3503_reset(struct usb3503 *hub, int state)
>>  {
>> +	int err;
>> +
>> +	err = usb3503_regulator(hub, state);
>> +	if (err) {
>> +		dev_err(hub->dev, "unable to %s VDD33 regulator to (%d)\n",
>> +			(state ? "enable" : "disable"), err);
>> +	}
> 
> Are we sure that the callers all balance enables and disables and we
> don't ever end up going through reset more than once on the way down?

I double checked the code and there might be in-balance if DT or
platform data sets initial mode to suspend. Otherwise it should be balanced.

I'll re-think the patch and fix this.

> 
>> +		hub->vdd_reg = devm_regulator_get_optional(dev, "vdd33");
>> +		if (IS_ERR(hub->vdd_reg)) {
>> +			if (PTR_ERR(hub->vdd_reg) == -EPROBE_DEFER)
>> +				return -EPROBE_DEFER;
> 
> This should explicitly check for -ENODEV and return the error if it gets
> anything else, that will mean that if the supply is needed but lookup
> fails somehow due to a non-deferral error we'll handle it properly.

I must admit I wasn't sure about handling the ENODEV and some other
examples (drivers) were handling this just like that.

Thanks for pointing this out.

> 
>> +	err = usb3503_regulator(hub, true);
> 
> The naming on this function is very obscure (and there's also a couple
> of other supplies).  I'd suggest just folding this into the reset
> function, or at least renaming so the reader can tell what these calls
> do.

Okay.

Thanks for feedback!

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown April 29, 2016, 4:44 p.m. UTC | #3
On Fri, Apr 29, 2016 at 01:55:14PM +0200, Krzysztof Kozlowski wrote:
> On 04/29/2016 01:30 PM, Mark Brown wrote:

> > Supplies are only optional if they may be physically absent.  In this
> > case it's possible that on device regulators may be used instead, a
> > pattern more like that used for arizona-ldo1 where we represent those
> > regulators might be better as it's more clearly describing the
> > situation.  I'm just wondering if the supply lookup stuff there should
> > be factored out as this is not an uncommon pattern..

> > It should at least be clearly stated what's going on, ignoring failure
> > to get supplies is generally a bug and people will tend to blindly cut
> > and paste things (witness all the breakage in graphics drivers with
> > this).

> The VDD33 is really optional. The device can work in different
> configuration, e.g. only on VBAT. How the reset logic would work then? I
> don't know... I would suspect that it could be exactly the same (just
> replace VDD33 with VBAT) but I am not sure.

What the Arizona example I mentioned does is look for the property
specifying an external supply in DT and if there isn't one assumes that
it must be using the internal regulator.  That's a bit icky but it does
the right thing and is much simpler from a user point of view.
Krzysztof Kozlowski May 2, 2016, 9:49 a.m. UTC | #4
On 04/29/2016 06:44 PM, Mark Brown wrote:
> On Fri, Apr 29, 2016 at 01:55:14PM +0200, Krzysztof Kozlowski wrote:
>> On 04/29/2016 01:30 PM, Mark Brown wrote:
> 
>>> Supplies are only optional if they may be physically absent.  In this
>>> case it's possible that on device regulators may be used instead, a
>>> pattern more like that used for arizona-ldo1 where we represent those
>>> regulators might be better as it's more clearly describing the
>>> situation.  I'm just wondering if the supply lookup stuff there should
>>> be factored out as this is not an uncommon pattern..
> 
>>> It should at least be clearly stated what's going on, ignoring failure
>>> to get supplies is generally a bug and people will tend to blindly cut
>>> and paste things (witness all the breakage in graphics drivers with
>>> this).
> 
>> The VDD33 is really optional. The device can work in different
>> configuration, e.g. only on VBAT. How the reset logic would work then? I
>> don't know... I would suspect that it could be exactly the same (just
>> replace VDD33 with VBAT) but I am not sure.
> 
> What the Arizona example I mentioned does is look for the property
> specifying an external supply in DT and if there isn't one assumes that
> it must be using the internal regulator.  That's a bit icky but it does
> the right thing and is much simpler from a user point of view.

Okay, that indeed looks similar... in case of lack of external supplies
the usb3503 pins should be tied to the internal regulators.

However it seems I was wrong at the beginning. We've been looking here
at the schematics and the datasheet. The design is unfortunately a
little bit confusing but finally I think we got the impression how does
it work.

This VDD regulator supply actually is not a usb3503 USB HUB regulator
supply... but a supply to the LAN attached to this HUB. Regulator off/on
is needed for LAN to show up. The hub will show up with typical reset
(which is also missing before my patchset btw).

The LAN, as a USB device, is auto-probed so it cannot take the regulator
and play with it. The simplest idea I have is to add it as "external
supply"  to the parent: usb3503.

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski May 2, 2016, 10:49 a.m. UTC | #5
On 05/02/2016 11:49 AM, Krzysztof Kozlowski wrote:
> On 04/29/2016 06:44 PM, Mark Brown wrote:
>> On Fri, Apr 29, 2016 at 01:55:14PM +0200, Krzysztof Kozlowski wrote:
>>> On 04/29/2016 01:30 PM, Mark Brown wrote:
>>
>>>> Supplies are only optional if they may be physically absent.  In this
>>>> case it's possible that on device regulators may be used instead, a
>>>> pattern more like that used for arizona-ldo1 where we represent those
>>>> regulators might be better as it's more clearly describing the
>>>> situation.  I'm just wondering if the supply lookup stuff there should
>>>> be factored out as this is not an uncommon pattern..
>>
>>>> It should at least be clearly stated what's going on, ignoring failure
>>>> to get supplies is generally a bug and people will tend to blindly cut
>>>> and paste things (witness all the breakage in graphics drivers with
>>>> this).
>>
>>> The VDD33 is really optional. The device can work in different
>>> configuration, e.g. only on VBAT. How the reset logic would work then? I
>>> don't know... I would suspect that it could be exactly the same (just
>>> replace VDD33 with VBAT) but I am not sure.
>>
>> What the Arizona example I mentioned does is look for the property
>> specifying an external supply in DT and if there isn't one assumes that
>> it must be using the internal regulator.  That's a bit icky but it does
>> the right thing and is much simpler from a user point of view.
> 
> Okay, that indeed looks similar... in case of lack of external supplies
> the usb3503 pins should be tied to the internal regulators.
> 
> However it seems I was wrong at the beginning. We've been looking here
> at the schematics and the datasheet. The design is unfortunately a
> little bit confusing but finally I think we got the impression how does
> it work.
> 
> This VDD regulator supply actually is not a usb3503 USB HUB regulator
> supply... but a supply to the LAN attached to this HUB. Regulator off/on
> is needed for LAN to show up. The hub will show up with typical reset
> (which is also missing before my patchset btw).
> 
> The LAN, as a USB device, is auto-probed so it cannot take the regulator
> and play with it. The simplest idea I have is to add it as "external
> supply"  to the parent: usb3503.

I run some more tests which adds more confusion. If the usb3503 hub does
not turn off/on the supply (LAN regulator supply in reality) it usually
does not show up as USB device (lsusb) neither. In such case sometimes
it is present, sometimes not.

"Hardware" reset with regulator fixes all the problems: with LAN and
with USB hub... It really does not match the schematics but apparently
usb3503 also needs the regulator.

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown May 2, 2016, 10:55 a.m. UTC | #6
On Mon, May 02, 2016 at 11:49:12AM +0200, Krzysztof Kozlowski wrote:

> This VDD regulator supply actually is not a usb3503 USB HUB regulator
> supply... but a supply to the LAN attached to this HUB. Regulator off/on
> is needed for LAN to show up. The hub will show up with typical reset
> (which is also missing before my patchset btw).

> The LAN, as a USB device, is auto-probed so it cannot take the regulator
> and play with it. The simplest idea I have is to add it as "external
> supply"  to the parent: usb3503.

This is common enough that that just isn't going to scale well I fear
without some generic handling, either walking child devices at the bus
level or at the device level with a pre-probe() callback to get the
device to power on.  The latter is more appropriate to things like
Slimbus where the device is more likely to do active management at
runtime, it's not clear people are building USB devices like that.
Rob Herring (Arm) May 3, 2016, 6 p.m. UTC | #7
On Mon, May 02, 2016 at 11:55:01AM +0100, Mark Brown wrote:
> On Mon, May 02, 2016 at 11:49:12AM +0200, Krzysztof Kozlowski wrote:
> 
> > This VDD regulator supply actually is not a usb3503 USB HUB regulator
> > supply... but a supply to the LAN attached to this HUB. Regulator off/on
> > is needed for LAN to show up. The hub will show up with typical reset
> > (which is also missing before my patchset btw).
> 
> > The LAN, as a USB device, is auto-probed so it cannot take the regulator
> > and play with it. The simplest idea I have is to add it as "external
> > supply"  to the parent: usb3503.
> 
> This is common enough that that just isn't going to scale well I fear
> without some generic handling, either walking child devices at the bus
> level or at the device level with a pre-probe() callback to get the
> device to power on.  The latter is more appropriate to things like
> Slimbus where the device is more likely to do active management at
> runtime, it's not clear people are building USB devices like that.

There's a new binding and support in -next (.../usb/usb-device.txt) for 
USB devices that should help here. Though, how to handle a hub on USB 
and I2C buses would need to be worked out.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski May 4, 2016, 12:01 p.m. UTC | #8
On 05/03/2016 08:00 PM, Rob Herring wrote:
> On Mon, May 02, 2016 at 11:55:01AM +0100, Mark Brown wrote:
>> On Mon, May 02, 2016 at 11:49:12AM +0200, Krzysztof Kozlowski wrote:
>>
>>> This VDD regulator supply actually is not a usb3503 USB HUB regulator
>>> supply... but a supply to the LAN attached to this HUB. Regulator off/on
>>> is needed for LAN to show up. The hub will show up with typical reset
>>> (which is also missing before my patchset btw).
>>
>>> The LAN, as a USB device, is auto-probed so it cannot take the regulator
>>> and play with it. The simplest idea I have is to add it as "external
>>> supply"  to the parent: usb3503.
>>
>> This is common enough that that just isn't going to scale well I fear
>> without some generic handling, either walking child devices at the bus
>> level or at the device level with a pre-probe() callback to get the
>> device to power on.  The latter is more appropriate to things like
>> Slimbus where the device is more likely to do active management at
>> runtime, it's not clear people are building USB devices like that.
> 
> There's a new binding and support in -next (.../usb/usb-device.txt) for 
> USB devices that should help here. Though, how to handle a hub on USB 
> and I2C buses would need to be worked out.

This binding might help, especially with other idea we have - usage of
pwrseq for the USB hub.

The USB hub, without toggling the regulator off/on, appears as USB
device only sometimes. Apparently there is some timing or other
behavior. not yet known to me.

When playing with regulator, the USB hub and child USB device (LAN) will
appear correctly.

This looks like pwrseq for MMC devices so the idea is to:
1. Move drivers/mmc/core/pwrseq* to separate directory
(drivers/power/pwrseq ?)
2. Add support for pwrseq to USB core,
3. Add new pwrseq driver (or extend existing one): toggling the regulator.
4. Add pwrseq phandle to the DT node of USB hub (to the binding
mentioned by Rob?).


Does it look sensible?

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown May 4, 2016, 6:25 p.m. UTC | #9
On Wed, May 04, 2016 at 02:01:57PM +0200, Krzysztof Kozlowski wrote:

> This looks like pwrseq for MMC devices so the idea is to:
> 1. Move drivers/mmc/core/pwrseq* to separate directory
> (drivers/power/pwrseq ?)
> 2. Add support for pwrseq to USB core,
> 3. Add new pwrseq driver (or extend existing one): toggling the regulator.
> 4. Add pwrseq phandle to the DT node of USB hub (to the binding
> mentioned by Rob?).

> Does it look sensible?

It certainly seems like a similar problem space, USB seems very much
like MMC in both the spec and the way it tends to get used.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/usb/usb3503.txt b/Documentation/devicetree/bindings/usb/usb3503.txt
index c1a0a9191d26..36516ade9467 100644
--- a/Documentation/devicetree/bindings/usb/usb3503.txt
+++ b/Documentation/devicetree/bindings/usb/usb3503.txt
@@ -24,6 +24,7 @@  Optional properties:
 	pins (optional, if not provided, driver will not set rate of the
 	REFCLK signal and assume that a value from the primary reference
 	clock frequencies table is used)
+- vdd33-supply: Optional supply for VDD 3.3 V power source.
 
 Examples:
 	usb3503@08 {
diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c
index b45cb77c0744..8905e8b2439d 100644
--- a/drivers/usb/misc/usb3503.c
+++ b/drivers/usb/misc/usb3503.c
@@ -28,6 +28,7 @@ 
 #include <linux/platform_device.h>
 #include <linux/platform_data/usb3503.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 
 #define USB3503_VIDL		0x00
 #define USB3503_VIDM		0x01
@@ -59,6 +60,7 @@  struct usb3503 {
 	struct regmap		*regmap;
 	struct device		*dev;
 	struct clk		*clk;
+	struct regulator	*vdd_reg;
 	u8	port_off_mask;
 	int	gpio_intn;
 	int	gpio_reset;
@@ -66,8 +68,31 @@  struct usb3503 {
 	bool	secondary_ref_clk;
 };
 
+static int usb3503_regulator(struct usb3503 *hub, int state)
+{
+	int ret;
+
+	if (!hub->vdd_reg)
+		return 0;
+
+	if (state)
+		ret = regulator_enable(hub->vdd_reg);
+	else
+		ret = regulator_disable(hub->vdd_reg);
+
+	return ret;
+}
+
 static int usb3503_reset(struct usb3503 *hub, int state)
 {
+	int err;
+
+	err = usb3503_regulator(hub, state);
+	if (err) {
+		dev_err(hub->dev, "unable to %s VDD33 regulator to (%d)\n",
+			(state ? "enable" : "disable"), err);
+	}
+
 	if (!state && gpio_is_valid(hub->gpio_connect))
 		gpio_set_value_cansleep(hub->gpio_connect, 0);
 
@@ -260,6 +285,15 @@  static int usb3503_probe(struct usb3503 *hub)
 			return -EPROBE_DEFER;
 		of_property_read_u32(np, "initial-mode", &mode);
 		hub->mode = mode;
+
+		hub->vdd_reg = devm_regulator_get_optional(dev, "vdd33");
+		if (IS_ERR(hub->vdd_reg)) {
+			if (PTR_ERR(hub->vdd_reg) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			hub->vdd_reg = NULL;
+		} else {
+			dev_dbg(dev, "Using VDD33 regulator for full reset\n");
+		}
 	}
 
 	if (hub->port_off_mask && !hub->regmap)
@@ -299,6 +333,21 @@  static int usb3503_probe(struct usb3503 *hub)
 			return err;
 		}
 	}
+	err = usb3503_regulator(hub, true);
+	if (err) {
+		dev_err(dev, "unable to enable VDD33 regulator (%d)\n", err);
+		return err;
+	}
+
+	/*
+	 * Perform real full reset before configuring.
+	 * On some boards (e.g. on Odroid U3 board with LAN9730/SMSC95xx)
+	 * after enabling the USB by bootloader it has to be fully reset
+	 * here to be visible.
+	 */
+	usb3503_reset(hub, 0);
+	/* Settle down before powering on again */
+	usleep_range(4000, 10000);
 
 	usb3503_switch_mode(hub, hub->mode);
 
@@ -330,6 +379,21 @@  static int usb3503_i2c_probe(struct i2c_client *i2c,
 	return usb3503_probe(hub);
 }
 
+static int usb3503_i2c_remove(struct i2c_client *i2c)
+{
+	struct usb3503 *hub;
+
+	hub = i2c_get_clientdata(i2c);
+	if (hub) {
+		if (hub->clk)
+			clk_disable_unprepare(hub->clk);
+
+		usb3503_regulator(hub, false);
+	}
+
+	return 0;
+}
+
 static int usb3503_platform_probe(struct platform_device *pdev)
 {
 	struct usb3503 *hub;
@@ -342,6 +406,21 @@  static int usb3503_platform_probe(struct platform_device *pdev)
 	return usb3503_probe(hub);
 }
 
+static int usb3503_platform_remove(struct platform_device *pdev)
+{
+	struct usb3503 *hub;
+
+	hub = platform_get_drvdata(pdev);
+	if (hub) {
+		if (hub->clk)
+			clk_disable_unprepare(hub->clk);
+
+		usb3503_regulator(hub, false);
+	}
+
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int usb3503_i2c_suspend(struct device *dev)
 {
@@ -395,6 +474,7 @@  static struct i2c_driver usb3503_i2c_driver = {
 		.of_match_table = of_match_ptr(usb3503_of_match),
 	},
 	.probe		= usb3503_i2c_probe,
+	.remove		= usb3503_i2c_remove,
 	.id_table	= usb3503_id,
 };
 
@@ -404,6 +484,7 @@  static struct platform_driver usb3503_platform_driver = {
 		.of_match_table = of_match_ptr(usb3503_of_match),
 	},
 	.probe		= usb3503_platform_probe,
+	.remove		= usb3503_platform_remove,
 };
 
 static int __init usb3503_init(void)