| Message ID | 1461927591-7864-2-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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.
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
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.
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
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
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.
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
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
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 --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)
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(+)