[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie Delaunay]

On some boards, especially when vbus supply requires large current,
and the charge pump on the PHY isn't enough, an external vbus power switch
may be used.
Add support for this optional external vbus supply in ehci-platform.

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>

---
Changes in v2:
 * Address Roger Quadros comments: move regulator_enable/disable from
ehci_platform_power_on/off to ehci_platform_port_power.
---
 Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
 drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
index 3efde12..fc480cd 100644
--- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
@@ -19,6 +19,7 @@ Optional properties:
  - phys : phandle + phy specifier pair
  - phy-names : "usb"
  - resets : phandle + reset specifier pair
+ - vbus-supply : phandle of regulator supplying vbus
 
 Example (Sequoia 440EPx):
     ehci@e0000300 {
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index b065a96..05be100 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -29,6 +29,7 @@
 #include <linux/of.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
@@ -46,6 +47,7 @@ struct ehci_platform_priv {
 	struct reset_control *rsts;
 	struct phy **phys;
 	int num_phys;
+	struct regulator *vbus_supply;
 	bool reset_on_resume;
 };
 
@@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
 	return 0;
 }
 
+static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
+				    bool enable)
+{
+	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
+	int ret = 0;
+
+	if (priv->vbus_supply) {
+		if (enable)
+			ret = regulator_enable(priv->vbus_supply);
+		else
+			ret = regulator_disable(priv->vbus_supply);
+		if (ret)
+			dev_err(hcd->self.controller,
+				"failed to %s vbus supply: %d\n",
+				enable ? "enable" : "disable", ret);
+	}
+	return ret;
+}
+
 static int ehci_platform_power_on(struct platform_device *dev)
 {
 	struct usb_hcd *hcd = platform_get_drvdata(dev);
@@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
 static const struct ehci_driver_overrides platform_overrides __initconst = {
 	.reset =		ehci_platform_reset,
 	.extra_priv_size =	sizeof(struct ehci_platform_priv),
+	.port_power =		ehci_platform_port_power,
 };
 
 static struct usb_ehci_pdata ehci_platform_defaults = {
@@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
 	if (err)
 		goto err_put_clks;
 
+	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
+	if (IS_ERR(priv->vbus_supply)) {
+		err = PTR_ERR(priv->vbus_supply);
+		if (err == -ENODEV)
+			priv->vbus_supply = NULL;
+		else
+			goto err_reset;
+	}
+
 	if (pdata->big_endian_desc)
 		ehci->big_endian_desc = 1;
 	if (pdata->big_endian_mmio)
-- 
2.7.4

[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie Delaunay]

On some boards, especially when vbus supply requires large current,
and the charge pump on the PHY isn't enough, an external vbus power switch
may be used.
Add support for this optional external vbus supply in ehci-platform.

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>

---
Changes in v2:
 * Address Roger Quadros comments: move regulator_enable/disable from
ehci_platform_power_on/off to ehci_platform_port_power.
---
 Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
 drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
index 3efde12..fc480cd 100644
--- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
@@ -19,6 +19,7 @@ Optional properties:
  - phys : phandle + phy specifier pair
  - phy-names : "usb"
  - resets : phandle + reset specifier pair
+ - vbus-supply : phandle of regulator supplying vbus
 
 Example (Sequoia 440EPx):
     ehci@e0000300 {
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index b065a96..05be100 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -29,6 +29,7 @@
 #include <linux/of.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
@@ -46,6 +47,7 @@ struct ehci_platform_priv {
 	struct reset_control *rsts;
 	struct phy **phys;
 	int num_phys;
+	struct regulator *vbus_supply;
 	bool reset_on_resume;
 };
 
@@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
 	return 0;
 }
 
+static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
+				    bool enable)
+{
+	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
+	int ret = 0;
+
+	if (priv->vbus_supply) {
+		if (enable)
+			ret = regulator_enable(priv->vbus_supply);
+		else
+			ret = regulator_disable(priv->vbus_supply);
+		if (ret)
+			dev_err(hcd->self.controller,
+				"failed to %s vbus supply: %d\n",
+				enable ? "enable" : "disable", ret);
+	}
+	return ret;
+}
+
 static int ehci_platform_power_on(struct platform_device *dev)
 {
 	struct usb_hcd *hcd = platform_get_drvdata(dev);
@@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
 static const struct ehci_driver_overrides platform_overrides __initconst = {
 	.reset =		ehci_platform_reset,
 	.extra_priv_size =	sizeof(struct ehci_platform_priv),
+	.port_power =		ehci_platform_port_power,
 };
 
 static struct usb_ehci_pdata ehci_platform_defaults = {
@@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
 	if (err)
 		goto err_put_clks;
 
+	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
+	if (IS_ERR(priv->vbus_supply)) {
+		err = PTR_ERR(priv->vbus_supply);
+		if (err == -ENODEV)
+			priv->vbus_supply = NULL;
+		else
+			goto err_reset;
+	}
+
 	if (pdata->big_endian_desc)
 		ehci->big_endian_desc = 1;
 	if (pdata->big_endian_mmio)
-- 
2.7.4

[v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie Delaunay]

On some boards, especially when vbus supply requires large current,
and the charge pump on the PHY isn't enough, an external vbus power switch
may be used.
Add support for this optional external vbus supply in ehci-platform.

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
---
Changes in v2:
 * Address Roger Quadros comments: move regulator_enable/disable from
ehci_platform_power_on/off to ehci_platform_port_power.
---
 Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
 drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
index 3efde12..fc480cd 100644
--- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
@@ -19,6 +19,7 @@ Optional properties:
  - phys : phandle + phy specifier pair
  - phy-names : "usb"
  - resets : phandle + reset specifier pair
+ - vbus-supply : phandle of regulator supplying vbus
 
 Example (Sequoia 440EPx):
     ehci@e0000300 {
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index b065a96..05be100 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -29,6 +29,7 @@
 #include <linux/of.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
@@ -46,6 +47,7 @@ struct ehci_platform_priv {
 	struct reset_control *rsts;
 	struct phy **phys;
 	int num_phys;
+	struct regulator *vbus_supply;
 	bool reset_on_resume;
 };
 
@@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
 	return 0;
 }
 
+static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
+				    bool enable)
+{
+	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
+	int ret = 0;
+
+	if (priv->vbus_supply) {
+		if (enable)
+			ret = regulator_enable(priv->vbus_supply);
+		else
+			ret = regulator_disable(priv->vbus_supply);
+		if (ret)
+			dev_err(hcd->self.controller,
+				"failed to %s vbus supply: %d\n",
+				enable ? "enable" : "disable", ret);
+	}
+	return ret;
+}
+
 static int ehci_platform_power_on(struct platform_device *dev)
 {
 	struct usb_hcd *hcd = platform_get_drvdata(dev);
@@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
 static const struct ehci_driver_overrides platform_overrides __initconst = {
 	.reset =		ehci_platform_reset,
 	.extra_priv_size =	sizeof(struct ehci_platform_priv),
+	.port_power =		ehci_platform_port_power,
 };
 
 static struct usb_ehci_pdata ehci_platform_defaults = {
@@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
 	if (err)
 		goto err_put_clks;
 
+	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
+	if (IS_ERR(priv->vbus_supply)) {
+		err = PTR_ERR(priv->vbus_supply);
+		if (err == -ENODEV)
+			priv->vbus_supply = NULL;
+		else
+			goto err_reset;
+	}
+
 	if (pdata->big_endian_desc)
 		ehci->big_endian_desc = 1;
 	if (pdata->big_endian_mmio)

[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie Delaunay]

On some boards, especially when vbus supply requires large current,
and the charge pump on the PHY isn't enough, an external vbus power switch
may be used.
Add support for this optional external vbus supply in ehci-platform.

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>

---
Changes in v2:
 * Address Roger Quadros comments: move regulator_enable/disable from
ehci_platform_power_on/off to ehci_platform_port_power.
---
 Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
 drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
index 3efde12..fc480cd 100644
--- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
@@ -19,6 +19,7 @@ Optional properties:
  - phys : phandle + phy specifier pair
  - phy-names : "usb"
  - resets : phandle + reset specifier pair
+ - vbus-supply : phandle of regulator supplying vbus
 
 Example (Sequoia 440EPx):
     ehci at e0000300 {
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index b065a96..05be100 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -29,6 +29,7 @@
 #include <linux/of.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
@@ -46,6 +47,7 @@ struct ehci_platform_priv {
 	struct reset_control *rsts;
 	struct phy **phys;
 	int num_phys;
+	struct regulator *vbus_supply;
 	bool reset_on_resume;
 };
 
@@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
 	return 0;
 }
 
+static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
+				    bool enable)
+{
+	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
+	int ret = 0;
+
+	if (priv->vbus_supply) {
+		if (enable)
+			ret = regulator_enable(priv->vbus_supply);
+		else
+			ret = regulator_disable(priv->vbus_supply);
+		if (ret)
+			dev_err(hcd->self.controller,
+				"failed to %s vbus supply: %d\n",
+				enable ? "enable" : "disable", ret);
+	}
+	return ret;
+}
+
 static int ehci_platform_power_on(struct platform_device *dev)
 {
 	struct usb_hcd *hcd = platform_get_drvdata(dev);
@@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
 static const struct ehci_driver_overrides platform_overrides __initconst = {
 	.reset =		ehci_platform_reset,
 	.extra_priv_size =	sizeof(struct ehci_platform_priv),
+	.port_power =		ehci_platform_port_power,
 };
 
 static struct usb_ehci_pdata ehci_platform_defaults = {
@@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
 	if (err)
 		goto err_put_clks;
 
+	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
+	if (IS_ERR(priv->vbus_supply)) {
+		err = PTR_ERR(priv->vbus_supply);
+		if (err == -ENODEV)
+			priv->vbus_supply = NULL;
+		else
+			goto err_reset;
+	}
+
 	if (pdata->big_endian_desc)
 		ehci->big_endian_desc = 1;
 	if (pdata->big_endian_mmio)
-- 
2.7.4

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Felipe Balbi]

Hi,

Amelie Delaunay <amelie.delaunay@st.com> writes:
> On some boards, especially when vbus supply requires large current,
> and the charge pump on the PHY isn't enough, an external vbus power switch
> may be used.
> Add support for this optional external vbus supply in ehci-platform.
>
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>
> ---
> Changes in v2:
>  * Address Roger Quadros comments: move regulator_enable/disable from
> ehci_platform_power_on/off to ehci_platform_port_power.
> ---
>  Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>  drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> index 3efde12..fc480cd 100644
> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> @@ -19,6 +19,7 @@ Optional properties:
>   - phys : phandle + phy specifier pair
>   - phy-names : "usb"
>   - resets : phandle + reset specifier pair
> + - vbus-supply : phandle of regulator supplying vbus
>  
>  Example (Sequoia 440EPx):
>      ehci@e0000300 {
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index b065a96..05be100 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -29,6 +29,7 @@
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>  	struct reset_control *rsts;
>  	struct phy **phys;
>  	int num_phys;
> +	struct regulator *vbus_supply;
>  	bool reset_on_resume;
>  };
>  
> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>  	return 0;
>  }
>  
> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> +				    bool enable)
> +{
> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> +	int ret = 0;
> +
> +	if (priv->vbus_supply) {

you can reduce indentation here:

	if (!priv->vbus_supply)
        	return 0;

-- 
balbi

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Felipe Balbi]

Hi,

Amelie Delaunay <amelie.delaunay@st.com> writes:
> On some boards, especially when vbus supply requires large current,
> and the charge pump on the PHY isn't enough, an external vbus power switch
> may be used.
> Add support for this optional external vbus supply in ehci-platform.
>
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>
> ---
> Changes in v2:
>  * Address Roger Quadros comments: move regulator_enable/disable from
> ehci_platform_power_on/off to ehci_platform_port_power.
> ---
>  Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>  drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> index 3efde12..fc480cd 100644
> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> @@ -19,6 +19,7 @@ Optional properties:
>   - phys : phandle + phy specifier pair
>   - phy-names : "usb"
>   - resets : phandle + reset specifier pair
> + - vbus-supply : phandle of regulator supplying vbus
>  
>  Example (Sequoia 440EPx):
>      ehci@e0000300 {
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index b065a96..05be100 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -29,6 +29,7 @@
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>  	struct reset_control *rsts;
>  	struct phy **phys;
>  	int num_phys;
> +	struct regulator *vbus_supply;
>  	bool reset_on_resume;
>  };
>  
> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>  	return 0;
>  }
>  
> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> +				    bool enable)
> +{
> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> +	int ret = 0;
> +
> +	if (priv->vbus_supply) {

you can reduce indentation here:

	if (!priv->vbus_supply)
        	return 0;

-- 
balbi

[v2] usb: host: ehci-platform: add support for optional external vbus supply

[Felipe Balbi]

Hi,

Amelie Delaunay <amelie.delaunay@st.com> writes:
> On some boards, especially when vbus supply requires large current,
> and the charge pump on the PHY isn't enough, an external vbus power switch
> may be used.
> Add support for this optional external vbus supply in ehci-platform.
>
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>
> ---
> Changes in v2:
>  * Address Roger Quadros comments: move regulator_enable/disable from
> ehci_platform_power_on/off to ehci_platform_port_power.
> ---
>  Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>  drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> index 3efde12..fc480cd 100644
> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> @@ -19,6 +19,7 @@ Optional properties:
>   - phys : phandle + phy specifier pair
>   - phy-names : "usb"
>   - resets : phandle + reset specifier pair
> + - vbus-supply : phandle of regulator supplying vbus
>  
>  Example (Sequoia 440EPx):
>      ehci@e0000300 {
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index b065a96..05be100 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -29,6 +29,7 @@
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>  	struct reset_control *rsts;
>  	struct phy **phys;
>  	int num_phys;
> +	struct regulator *vbus_supply;
>  	bool reset_on_resume;
>  };
>  
> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>  	return 0;
>  }
>  
> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> +				    bool enable)
> +{
> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> +	int ret = 0;
> +
> +	if (priv->vbus_supply) {

you can reduce indentation here:

	if (!priv->vbus_supply)
        	return 0;

[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Felipe Balbi]

Hi,

Amelie Delaunay <amelie.delaunay@st.com> writes:
> On some boards, especially when vbus supply requires large current,
> and the charge pump on the PHY isn't enough, an external vbus power switch
> may be used.
> Add support for this optional external vbus supply in ehci-platform.
>
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>
> ---
> Changes in v2:
>  * Address Roger Quadros comments: move regulator_enable/disable from
> ehci_platform_power_on/off to ehci_platform_port_power.
> ---
>  Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>  drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> index 3efde12..fc480cd 100644
> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> @@ -19,6 +19,7 @@ Optional properties:
>   - phys : phandle + phy specifier pair
>   - phy-names : "usb"
>   - resets : phandle + reset specifier pair
> + - vbus-supply : phandle of regulator supplying vbus
>  
>  Example (Sequoia 440EPx):
>      ehci at e0000300 {
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index b065a96..05be100 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -29,6 +29,7 @@
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>  	struct reset_control *rsts;
>  	struct phy **phys;
>  	int num_phys;
> +	struct regulator *vbus_supply;
>  	bool reset_on_resume;
>  };
>  
> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>  	return 0;
>  }
>  
> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> +				    bool enable)
> +{
> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> +	int ret = 0;
> +
> +	if (priv->vbus_supply) {

you can reduce indentation here:

	if (!priv->vbus_supply)
        	return 0;

-- 
balbi

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

Hi,

On 02/20/2018 02:02 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Amelie Delaunay <amelie.delaunay@st.com> writes:
>> On some boards, especially when vbus supply requires large current,
>> and the charge pump on the PHY isn't enough, an external vbus power switch
>> may be used.
>> Add support for this optional external vbus supply in ehci-platform.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>
>> ---
>> Changes in v2:
>>   * Address Roger Quadros comments: move regulator_enable/disable from
>> ehci_platform_power_on/off to ehci_platform_port_power.
>> ---
>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>   2 files changed, 32 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> index 3efde12..fc480cd 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> @@ -19,6 +19,7 @@ Optional properties:
>>    - phys : phandle + phy specifier pair
>>    - phy-names : "usb"
>>    - resets : phandle + reset specifier pair
>> + - vbus-supply : phandle of regulator supplying vbus
>>   
>>   Example (Sequoia 440EPx):
>>       ehci@e0000300 {
>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>> index b065a96..05be100 100644
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -29,6 +29,7 @@
>>   #include <linux/of.h>
>>   #include <linux/phy/phy.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>>   #include <linux/reset.h>
>>   #include <linux/usb.h>
>>   #include <linux/usb/hcd.h>
>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>   	struct reset_control *rsts;
>>   	struct phy **phys;
>>   	int num_phys;
>> +	struct regulator *vbus_supply;
>>   	bool reset_on_resume;
>>   };
>>   
>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>   	return 0;
>>   }
>>   
>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>> +				    bool enable)
>> +{
>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>> +	int ret = 0;
>> +
>> +	if (priv->vbus_supply) {
> 
> you can reduce indentation here:
> 
> 	if (!priv->vbus_supply)
>          	return 0;
> 

You're right, thanks! I will fix it in upcoming v3, waiting for other 
additional comments.
Amelie

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

Hi,

On 02/20/2018 02:02 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Amelie Delaunay <amelie.delaunay@st.com> writes:
>> On some boards, especially when vbus supply requires large current,
>> and the charge pump on the PHY isn't enough, an external vbus power switch
>> may be used.
>> Add support for this optional external vbus supply in ehci-platform.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>
>> ---
>> Changes in v2:
>>   * Address Roger Quadros comments: move regulator_enable/disable from
>> ehci_platform_power_on/off to ehci_platform_port_power.
>> ---
>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>   2 files changed, 32 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> index 3efde12..fc480cd 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> @@ -19,6 +19,7 @@ Optional properties:
>>    - phys : phandle + phy specifier pair
>>    - phy-names : "usb"
>>    - resets : phandle + reset specifier pair
>> + - vbus-supply : phandle of regulator supplying vbus
>>   
>>   Example (Sequoia 440EPx):
>>       ehci@e0000300 {
>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>> index b065a96..05be100 100644
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -29,6 +29,7 @@
>>   #include <linux/of.h>
>>   #include <linux/phy/phy.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>>   #include <linux/reset.h>
>>   #include <linux/usb.h>
>>   #include <linux/usb/hcd.h>
>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>   	struct reset_control *rsts;
>>   	struct phy **phys;
>>   	int num_phys;
>> +	struct regulator *vbus_supply;
>>   	bool reset_on_resume;
>>   };
>>   
>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>   	return 0;
>>   }
>>   
>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>> +				    bool enable)
>> +{
>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>> +	int ret = 0;
>> +
>> +	if (priv->vbus_supply) {
> 
> you can reduce indentation here:
> 
> 	if (!priv->vbus_supply)
>          	return 0;
> 

You're right, thanks! I will fix it in upcoming v3, waiting for other 
additional comments.
Amelie

[v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie Delaunay]

Hi,

On 02/20/2018 02:02 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Amelie Delaunay <amelie.delaunay@st.com> writes:
>> On some boards, especially when vbus supply requires large current,
>> and the charge pump on the PHY isn't enough, an external vbus power switch
>> may be used.
>> Add support for this optional external vbus supply in ehci-platform.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>
>> ---
>> Changes in v2:
>>   * Address Roger Quadros comments: move regulator_enable/disable from
>> ehci_platform_power_on/off to ehci_platform_port_power.
>> ---
>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>   2 files changed, 32 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> index 3efde12..fc480cd 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> @@ -19,6 +19,7 @@ Optional properties:
>>    - phys : phandle + phy specifier pair
>>    - phy-names : "usb"
>>    - resets : phandle + reset specifier pair
>> + - vbus-supply : phandle of regulator supplying vbus
>>   
>>   Example (Sequoia 440EPx):
>>       ehci@e0000300 {
>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>> index b065a96..05be100 100644
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -29,6 +29,7 @@
>>   #include <linux/of.h>
>>   #include <linux/phy/phy.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>>   #include <linux/reset.h>
>>   #include <linux/usb.h>
>>   #include <linux/usb/hcd.h>
>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>   	struct reset_control *rsts;
>>   	struct phy **phys;
>>   	int num_phys;
>> +	struct regulator *vbus_supply;
>>   	bool reset_on_resume;
>>   };
>>   
>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>   	return 0;
>>   }
>>   
>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>> +				    bool enable)
>> +{
>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>> +	int ret = 0;
>> +
>> +	if (priv->vbus_supply) {
> 
> you can reduce indentation here:
> 
> 	if (!priv->vbus_supply)
>          	return 0;
> 

You're right, thanks! I will fix it in upcoming v3, waiting for other 
additional comments.
Amelie

[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

Hi,

On 02/20/2018 02:02 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Amelie Delaunay <amelie.delaunay@st.com> writes:
>> On some boards, especially when vbus supply requires large current,
>> and the charge pump on the PHY isn't enough, an external vbus power switch
>> may be used.
>> Add support for this optional external vbus supply in ehci-platform.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>
>> ---
>> Changes in v2:
>>   * Address Roger Quadros comments: move regulator_enable/disable from
>> ehci_platform_power_on/off to ehci_platform_port_power.
>> ---
>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>   2 files changed, 32 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> index 3efde12..fc480cd 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> @@ -19,6 +19,7 @@ Optional properties:
>>    - phys : phandle + phy specifier pair
>>    - phy-names : "usb"
>>    - resets : phandle + reset specifier pair
>> + - vbus-supply : phandle of regulator supplying vbus
>>   
>>   Example (Sequoia 440EPx):
>>       ehci at e0000300 {
>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>> index b065a96..05be100 100644
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -29,6 +29,7 @@
>>   #include <linux/of.h>
>>   #include <linux/phy/phy.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>>   #include <linux/reset.h>
>>   #include <linux/usb.h>
>>   #include <linux/usb/hcd.h>
>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>   	struct reset_control *rsts;
>>   	struct phy **phys;
>>   	int num_phys;
>> +	struct regulator *vbus_supply;
>>   	bool reset_on_resume;
>>   };
>>   
>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>   	return 0;
>>   }
>>   
>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>> +				    bool enable)
>> +{
>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>> +	int ret = 0;
>> +
>> +	if (priv->vbus_supply) {
> 
> you can reduce indentation here:
> 
> 	if (!priv->vbus_supply)
>          	return 0;
> 

You're right, thanks! I will fix it in upcoming v3, waiting for other 
additional comments.
Amelie

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Roger Quadros]

Hi,

On 20/02/18 14:58, Amelie Delaunay wrote:
> On some boards, especially when vbus supply requires large current,
> and the charge pump on the PHY isn't enough, an external vbus power switch
> may be used.
> Add support for this optional external vbus supply in ehci-platform.
> 
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> 
> ---
> Changes in v2:
>  * Address Roger Quadros comments: move regulator_enable/disable from
> ehci_platform_power_on/off to ehci_platform_port_power.
> ---
>  Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>  drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> index 3efde12..fc480cd 100644
> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> @@ -19,6 +19,7 @@ Optional properties:
>   - phys : phandle + phy specifier pair
>   - phy-names : "usb"
>   - resets : phandle + reset specifier pair
> + - vbus-supply : phandle of regulator supplying vbus
>  

Can platforms have more than one regulator e.g. one regulator per port?

>  Example (Sequoia 440EPx):
>      ehci@e0000300 {
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index b065a96..05be100 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -29,6 +29,7 @@
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>  	struct reset_control *rsts;
>  	struct phy **phys;
>  	int num_phys;
> +	struct regulator *vbus_supply;
>  	bool reset_on_resume;
>  };
>  
> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>  	return 0;
>  }
>  
> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> +				    bool enable)
> +{
> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> +	int ret = 0;
> +
> +	if (priv->vbus_supply) {
> +		if (enable)
> +			ret = regulator_enable(priv->vbus_supply);
> +		else
> +			ret = regulator_disable(priv->vbus_supply);
> +		if (ret)
> +			dev_err(hcd->self.controller,
> +				"failed to %s vbus supply: %d\n",
> +				enable ? "enable" : "disable", ret);
> +	}
> +	return ret;
> +}
> +
>  static int ehci_platform_power_on(struct platform_device *dev)
>  {
>  	struct usb_hcd *hcd = platform_get_drvdata(dev);
> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>  static const struct ehci_driver_overrides platform_overrides __initconst = {
>  	.reset =		ehci_platform_reset,
>  	.extra_priv_size =	sizeof(struct ehci_platform_priv),
> +	.port_power =		ehci_platform_port_power,
>  };
>  
>  static struct usb_ehci_pdata ehci_platform_defaults = {
> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>  	if (err)
>  		goto err_put_clks;
>  
> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
> +	if (IS_ERR(priv->vbus_supply)) {
> +		err = PTR_ERR(priv->vbus_supply);
> +		if (err == -ENODEV)
> +			priv->vbus_supply = NULL;
> +		else
> +			goto err_reset;
> +	}
> +
>  	if (pdata->big_endian_desc)
>  		ehci->big_endian_desc = 1;
>  	if (pdata->big_endian_mmio)
> 

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Roger Quadros]

Hi,

On 20/02/18 14:58, Amelie Delaunay wrote:
> On some boards, especially when vbus supply requires large current,
> and the charge pump on the PHY isn't enough, an external vbus power switch
> may be used.
> Add support for this optional external vbus supply in ehci-platform.
> 
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> 
> ---
> Changes in v2:
>  * Address Roger Quadros comments: move regulator_enable/disable from
> ehci_platform_power_on/off to ehci_platform_port_power.
> ---
>  Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>  drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> index 3efde12..fc480cd 100644
> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> @@ -19,6 +19,7 @@ Optional properties:
>   - phys : phandle + phy specifier pair
>   - phy-names : "usb"
>   - resets : phandle + reset specifier pair
> + - vbus-supply : phandle of regulator supplying vbus
>  

Can platforms have more than one regulator e.g. one regulator per port?

>  Example (Sequoia 440EPx):
>      ehci@e0000300 {
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index b065a96..05be100 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -29,6 +29,7 @@
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>  	struct reset_control *rsts;
>  	struct phy **phys;
>  	int num_phys;
> +	struct regulator *vbus_supply;
>  	bool reset_on_resume;
>  };
>  
> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>  	return 0;
>  }
>  
> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> +				    bool enable)
> +{
> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> +	int ret = 0;
> +
> +	if (priv->vbus_supply) {
> +		if (enable)
> +			ret = regulator_enable(priv->vbus_supply);
> +		else
> +			ret = regulator_disable(priv->vbus_supply);
> +		if (ret)
> +			dev_err(hcd->self.controller,
> +				"failed to %s vbus supply: %d\n",
> +				enable ? "enable" : "disable", ret);
> +	}
> +	return ret;
> +}
> +
>  static int ehci_platform_power_on(struct platform_device *dev)
>  {
>  	struct usb_hcd *hcd = platform_get_drvdata(dev);
> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>  static const struct ehci_driver_overrides platform_overrides __initconst = {
>  	.reset =		ehci_platform_reset,
>  	.extra_priv_size =	sizeof(struct ehci_platform_priv),
> +	.port_power =		ehci_platform_port_power,
>  };
>  
>  static struct usb_ehci_pdata ehci_platform_defaults = {
> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>  	if (err)
>  		goto err_put_clks;
>  
> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
> +	if (IS_ERR(priv->vbus_supply)) {
> +		err = PTR_ERR(priv->vbus_supply);
> +		if (err == -ENODEV)
> +			priv->vbus_supply = NULL;
> +		else
> +			goto err_reset;
> +	}
> +
>  	if (pdata->big_endian_desc)
>  		ehci->big_endian_desc = 1;
>  	if (pdata->big_endian_mmio)
> 

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[v2] usb: host: ehci-platform: add support for optional external vbus supply

[Roger Quadros]

Hi,

On 20/02/18 14:58, Amelie Delaunay wrote:
> On some boards, especially when vbus supply requires large current,
> and the charge pump on the PHY isn't enough, an external vbus power switch
> may be used.
> Add support for this optional external vbus supply in ehci-platform.
> 
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> 
> ---
> Changes in v2:
>  * Address Roger Quadros comments: move regulator_enable/disable from
> ehci_platform_power_on/off to ehci_platform_port_power.
> ---
>  Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>  drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> index 3efde12..fc480cd 100644
> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> @@ -19,6 +19,7 @@ Optional properties:
>   - phys : phandle + phy specifier pair
>   - phy-names : "usb"
>   - resets : phandle + reset specifier pair
> + - vbus-supply : phandle of regulator supplying vbus
>  

Can platforms have more than one regulator e.g. one regulator per port?

>  Example (Sequoia 440EPx):
>      ehci@e0000300 {
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index b065a96..05be100 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -29,6 +29,7 @@
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>  	struct reset_control *rsts;
>  	struct phy **phys;
>  	int num_phys;
> +	struct regulator *vbus_supply;
>  	bool reset_on_resume;
>  };
>  
> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>  	return 0;
>  }
>  
> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> +				    bool enable)
> +{
> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> +	int ret = 0;
> +
> +	if (priv->vbus_supply) {
> +		if (enable)
> +			ret = regulator_enable(priv->vbus_supply);
> +		else
> +			ret = regulator_disable(priv->vbus_supply);
> +		if (ret)
> +			dev_err(hcd->self.controller,
> +				"failed to %s vbus supply: %d\n",
> +				enable ? "enable" : "disable", ret);
> +	}
> +	return ret;
> +}
> +
>  static int ehci_platform_power_on(struct platform_device *dev)
>  {
>  	struct usb_hcd *hcd = platform_get_drvdata(dev);
> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>  static const struct ehci_driver_overrides platform_overrides __initconst = {
>  	.reset =		ehci_platform_reset,
>  	.extra_priv_size =	sizeof(struct ehci_platform_priv),
> +	.port_power =		ehci_platform_port_power,
>  };
>  
>  static struct usb_ehci_pdata ehci_platform_defaults = {
> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>  	if (err)
>  		goto err_put_clks;
>  
> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
> +	if (IS_ERR(priv->vbus_supply)) {
> +		err = PTR_ERR(priv->vbus_supply);
> +		if (err == -ENODEV)
> +			priv->vbus_supply = NULL;
> +		else
> +			goto err_reset;
> +	}
> +
>  	if (pdata->big_endian_desc)
>  		ehci->big_endian_desc = 1;
>  	if (pdata->big_endian_mmio)
>

[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Roger Quadros]

Hi,

On 20/02/18 14:58, Amelie Delaunay wrote:
> On some boards, especially when vbus supply requires large current,
> and the charge pump on the PHY isn't enough, an external vbus power switch
> may be used.
> Add support for this optional external vbus supply in ehci-platform.
> 
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> 
> ---
> Changes in v2:
>  * Address Roger Quadros comments: move regulator_enable/disable from
> ehci_platform_power_on/off to ehci_platform_port_power.
> ---
>  Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>  drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> index 3efde12..fc480cd 100644
> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> @@ -19,6 +19,7 @@ Optional properties:
>   - phys : phandle + phy specifier pair
>   - phy-names : "usb"
>   - resets : phandle + reset specifier pair
> + - vbus-supply : phandle of regulator supplying vbus
>  

Can platforms have more than one regulator e.g. one regulator per port?

>  Example (Sequoia 440EPx):
>      ehci at e0000300 {
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index b065a96..05be100 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -29,6 +29,7 @@
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>  	struct reset_control *rsts;
>  	struct phy **phys;
>  	int num_phys;
> +	struct regulator *vbus_supply;
>  	bool reset_on_resume;
>  };
>  
> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>  	return 0;
>  }
>  
> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> +				    bool enable)
> +{
> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> +	int ret = 0;
> +
> +	if (priv->vbus_supply) {
> +		if (enable)
> +			ret = regulator_enable(priv->vbus_supply);
> +		else
> +			ret = regulator_disable(priv->vbus_supply);
> +		if (ret)
> +			dev_err(hcd->self.controller,
> +				"failed to %s vbus supply: %d\n",
> +				enable ? "enable" : "disable", ret);
> +	}
> +	return ret;
> +}
> +
>  static int ehci_platform_power_on(struct platform_device *dev)
>  {
>  	struct usb_hcd *hcd = platform_get_drvdata(dev);
> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>  static const struct ehci_driver_overrides platform_overrides __initconst = {
>  	.reset =		ehci_platform_reset,
>  	.extra_priv_size =	sizeof(struct ehci_platform_priv),
> +	.port_power =		ehci_platform_port_power,
>  };
>  
>  static struct usb_ehci_pdata ehci_platform_defaults = {
> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>  	if (err)
>  		goto err_put_clks;
>  
> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
> +	if (IS_ERR(priv->vbus_supply)) {
> +		err = PTR_ERR(priv->vbus_supply);
> +		if (err == -ENODEV)
> +			priv->vbus_supply = NULL;
> +		else
> +			goto err_reset;
> +	}
> +
>  	if (pdata->big_endian_desc)
>  		ehci->big_endian_desc = 1;
>  	if (pdata->big_endian_mmio)
> 

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

Hi,

On 02/20/2018 03:00 PM, Roger Quadros wrote:
> Hi,
> 
> On 20/02/18 14:58, Amelie Delaunay wrote:
>> On some boards, especially when vbus supply requires large current,
>> and the charge pump on the PHY isn't enough, an external vbus power switch
>> may be used.
>> Add support for this optional external vbus supply in ehci-platform.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>
>> ---
>> Changes in v2:
>>   * Address Roger Quadros comments: move regulator_enable/disable from
>> ehci_platform_power_on/off to ehci_platform_port_power.
>> ---
>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>   2 files changed, 32 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> index 3efde12..fc480cd 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> @@ -19,6 +19,7 @@ Optional properties:
>>    - phys : phandle + phy specifier pair
>>    - phy-names : "usb"
>>    - resets : phandle + reset specifier pair
>> + - vbus-supply : phandle of regulator supplying vbus
>>   
> 
> Can platforms have more than one regulator e.g. one regulator per port?
> 

I imagine that yes, platforms could have one regulator per port.
Regulator consumers bindings impose a <name>-supply property per 
regulator, so, what do you think about :
vbus0-supply for port#0
vbus1-supply for port#1
...
vbusN-supply for port#N

And then in probe, allocate 'struct regulator *vbus_supplies' with a 
size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct 
regulator *)'.
And loop to get optional regulator vbus0, vbus1,..., vbusN.
And then enable/disable the corresponding regulator in 
ehci_platform_port_power thanks to portnum.

>>   Example (Sequoia 440EPx):
>>       ehci@e0000300 {
>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>> index b065a96..05be100 100644
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -29,6 +29,7 @@
>>   #include <linux/of.h>
>>   #include <linux/phy/phy.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>>   #include <linux/reset.h>
>>   #include <linux/usb.h>
>>   #include <linux/usb/hcd.h>
>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>   	struct reset_control *rsts;
>>   	struct phy **phys;
>>   	int num_phys;
>> +	struct regulator *vbus_supply;
>>   	bool reset_on_resume;
>>   };
>>   
>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>   	return 0;
>>   }
>>   
>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>> +				    bool enable)
>> +{
>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>> +	int ret = 0;
>> +
>> +	if (priv->vbus_supply) {
>> +		if (enable)
>> +			ret = regulator_enable(priv->vbus_supply);
>> +		else
>> +			ret = regulator_disable(priv->vbus_supply);
>> +		if (ret)
>> +			dev_err(hcd->self.controller,
>> +				"failed to %s vbus supply: %d\n",
>> +				enable ? "enable" : "disable", ret);
>> +	}
>> +	return ret;
>> +}
>> +
>>   static int ehci_platform_power_on(struct platform_device *dev)
>>   {
>>   	struct usb_hcd *hcd = platform_get_drvdata(dev);
>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>>   static const struct ehci_driver_overrides platform_overrides __initconst = {
>>   	.reset =		ehci_platform_reset,
>>   	.extra_priv_size =	sizeof(struct ehci_platform_priv),
>> +	.port_power =		ehci_platform_port_power,
>>   };
>>   
>>   static struct usb_ehci_pdata ehci_platform_defaults = {
>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>>   	if (err)
>>   		goto err_put_clks;
>>   
>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
>> +	if (IS_ERR(priv->vbus_supply)) {
>> +		err = PTR_ERR(priv->vbus_supply);
>> +		if (err == -ENODEV)
>> +			priv->vbus_supply = NULL;
>> +		else
>> +			goto err_reset;
>> +	}
>> +
>>   	if (pdata->big_endian_desc)
>>   		ehci->big_endian_desc = 1;
>>   	if (pdata->big_endian_mmio)
>>
> 

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

Hi,

On 02/20/2018 03:00 PM, Roger Quadros wrote:
> Hi,
> 
> On 20/02/18 14:58, Amelie Delaunay wrote:
>> On some boards, especially when vbus supply requires large current,
>> and the charge pump on the PHY isn't enough, an external vbus power switch
>> may be used.
>> Add support for this optional external vbus supply in ehci-platform.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>
>> ---
>> Changes in v2:
>>   * Address Roger Quadros comments: move regulator_enable/disable from
>> ehci_platform_power_on/off to ehci_platform_port_power.
>> ---
>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>   2 files changed, 32 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> index 3efde12..fc480cd 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> @@ -19,6 +19,7 @@ Optional properties:
>>    - phys : phandle + phy specifier pair
>>    - phy-names : "usb"
>>    - resets : phandle + reset specifier pair
>> + - vbus-supply : phandle of regulator supplying vbus
>>   
> 
> Can platforms have more than one regulator e.g. one regulator per port?
> 

I imagine that yes, platforms could have one regulator per port.
Regulator consumers bindings impose a <name>-supply property per 
regulator, so, what do you think about :
vbus0-supply for port#0
vbus1-supply for port#1
...
vbusN-supply for port#N

And then in probe, allocate 'struct regulator *vbus_supplies' with a 
size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct 
regulator *)'.
And loop to get optional regulator vbus0, vbus1,..., vbusN.
And then enable/disable the corresponding regulator in 
ehci_platform_port_power thanks to portnum.

>>   Example (Sequoia 440EPx):
>>       ehci@e0000300 {
>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>> index b065a96..05be100 100644
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -29,6 +29,7 @@
>>   #include <linux/of.h>
>>   #include <linux/phy/phy.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>>   #include <linux/reset.h>
>>   #include <linux/usb.h>
>>   #include <linux/usb/hcd.h>
>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>   	struct reset_control *rsts;
>>   	struct phy **phys;
>>   	int num_phys;
>> +	struct regulator *vbus_supply;
>>   	bool reset_on_resume;
>>   };
>>   
>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>   	return 0;
>>   }
>>   
>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>> +				    bool enable)
>> +{
>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>> +	int ret = 0;
>> +
>> +	if (priv->vbus_supply) {
>> +		if (enable)
>> +			ret = regulator_enable(priv->vbus_supply);
>> +		else
>> +			ret = regulator_disable(priv->vbus_supply);
>> +		if (ret)
>> +			dev_err(hcd->self.controller,
>> +				"failed to %s vbus supply: %d\n",
>> +				enable ? "enable" : "disable", ret);
>> +	}
>> +	return ret;
>> +}
>> +
>>   static int ehci_platform_power_on(struct platform_device *dev)
>>   {
>>   	struct usb_hcd *hcd = platform_get_drvdata(dev);
>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>>   static const struct ehci_driver_overrides platform_overrides __initconst = {
>>   	.reset =		ehci_platform_reset,
>>   	.extra_priv_size =	sizeof(struct ehci_platform_priv),
>> +	.port_power =		ehci_platform_port_power,
>>   };
>>   
>>   static struct usb_ehci_pdata ehci_platform_defaults = {
>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>>   	if (err)
>>   		goto err_put_clks;
>>   
>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
>> +	if (IS_ERR(priv->vbus_supply)) {
>> +		err = PTR_ERR(priv->vbus_supply);
>> +		if (err == -ENODEV)
>> +			priv->vbus_supply = NULL;
>> +		else
>> +			goto err_reset;
>> +	}
>> +
>>   	if (pdata->big_endian_desc)
>>   		ehci->big_endian_desc = 1;
>>   	if (pdata->big_endian_mmio)
>>
> 

[v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie Delaunay]

SGksDQoNCk9uIDAyLzIwLzIwMTggMDM6MDAgUE0sIFJvZ2VyIFF1YWRyb3Mgd3JvdGU6DQo+IEhp
LA0KPiANCj4gT24gMjAvMDIvMTggMTQ6NTgsIEFtZWxpZSBEZWxhdW5heSB3cm90ZToNCj4+IE9u
IHNvbWUgYm9hcmRzLCBlc3BlY2lhbGx5IHdoZW4gdmJ1cyBzdXBwbHkgcmVxdWlyZXMgbGFyZ2Ug
Y3VycmVudCwNCj4+IGFuZCB0aGUgY2hhcmdlIHB1bXAgb24gdGhlIFBIWSBpc24ndCBlbm91Z2gs
IGFuIGV4dGVybmFsIHZidXMgcG93ZXIgc3dpdGNoDQo+PiBtYXkgYmUgdXNlZC4NCj4+IEFkZCBz
dXBwb3J0IGZvciB0aGlzIG9wdGlvbmFsIGV4dGVybmFsIHZidXMgc3VwcGx5IGluIGVoY2ktcGxh
dGZvcm0uDQo+Pg0KPj4gU2lnbmVkLW9mZi1ieTogQW1lbGllIERlbGF1bmF5IDxhbWVsaWUuZGVs
YXVuYXlAc3QuY29tPg0KPj4NCj4+IC0tLQ0KPj4gQ2hhbmdlcyBpbiB2MjoNCj4+ICAgKiBBZGRy
ZXNzIFJvZ2VyIFF1YWRyb3MgY29tbWVudHM6IG1vdmUgcmVndWxhdG9yX2VuYWJsZS9kaXNhYmxl
IGZyb20NCj4+IGVoY2lfcGxhdGZvcm1fcG93ZXJfb24vb2ZmIHRvIGVoY2lfcGxhdGZvcm1fcG9y
dF9wb3dlci4NCj4+IC0tLQ0KPj4gICBEb2N1bWVudGF0aW9uL2RldmljZXRyZWUvYmluZGluZ3Mv
dXNiL3VzYi1laGNpLnR4dCB8ICAxICsNCj4+ICAgZHJpdmVycy91c2IvaG9zdC9laGNpLXBsYXRm
b3JtLmMgICAgICAgICAgICAgICAgICAgfCAzMSArKysrKysrKysrKysrKysrKysrKysrDQo+PiAg
IDIgZmlsZXMgY2hhbmdlZCwgMzIgaW5zZXJ0aW9ucygrKQ0KPj4NCj4+IGRpZmYgLS1naXQgYS9E
b2N1bWVudGF0aW9uL2RldmljZXRyZWUvYmluZGluZ3MvdXNiL3VzYi1laGNpLnR4dCBiL0RvY3Vt
ZW50YXRpb24vZGV2aWNldHJlZS9iaW5kaW5ncy91c2IvdXNiLWVoY2kudHh0DQo+PiBpbmRleCAz
ZWZkZTEyLi5mYzQ4MGNkIDEwMDY0NA0KPj4gLS0tIGEvRG9jdW1lbnRhdGlvbi9kZXZpY2V0cmVl
L2JpbmRpbmdzL3VzYi91c2ItZWhjaS50eHQNCj4+ICsrKyBiL0RvY3VtZW50YXRpb24vZGV2aWNl
dHJlZS9iaW5kaW5ncy91c2IvdXNiLWVoY2kudHh0DQo+PiBAQCAtMTksNiArMTksNyBAQCBPcHRp
b25hbCBwcm9wZXJ0aWVzOg0KPj4gICAgLSBwaHlzIDogcGhhbmRsZSArIHBoeSBzcGVjaWZpZXIg
cGFpcg0KPj4gICAgLSBwaHktbmFtZXMgOiAidXNiIg0KPj4gICAgLSByZXNldHMgOiBwaGFuZGxl
ICsgcmVzZXQgc3BlY2lmaWVyIHBhaXINCj4+ICsgLSB2YnVzLXN1cHBseSA6IHBoYW5kbGUgb2Yg
cmVndWxhdG9yIHN1cHBseWluZyB2YnVzDQo+PiAgIA0KPiANCj4gQ2FuIHBsYXRmb3JtcyBoYXZl
IG1vcmUgdGhhbiBvbmUgcmVndWxhdG9yIGUuZy4gb25lIHJlZ3VsYXRvciBwZXIgcG9ydD8NCj4g
DQoNCkkgaW1hZ2luZSB0aGF0IHllcywgcGxhdGZvcm1zIGNvdWxkIGhhdmUgb25lIHJlZ3VsYXRv
ciBwZXIgcG9ydC4NClJlZ3VsYXRvciBjb25zdW1lcnMgYmluZGluZ3MgaW1wb3NlIGEgPG5hbWU+
LXN1cHBseSBwcm9wZXJ0eSBwZXIgDQpyZWd1bGF0b3IsIHNvLCB3aGF0IGRvIHlvdSB0aGluayBh
Ym91dCA6DQp2YnVzMC1zdXBwbHkgZm9yIHBvcnQjMA0KdmJ1czEtc3VwcGx5IGZvciBwb3J0IzEN
Ci4uLg0KdmJ1c04tc3VwcGx5IGZvciBwb3J0I04NCg0KQW5kIHRoZW4gaW4gcHJvYmUsIGFsbG9j
YXRlICdzdHJ1Y3QgcmVndWxhdG9yICp2YnVzX3N1cHBsaWVzJyB3aXRoIGEgDQpzaXplIGNvcnJl
c3BvbmRpbmcgdG8gJ0hDU19OX1BPUlRTKGVoY2ktPmhjc19wYXJhbXMpICogc2l6ZW9mKHN0cnVj
dCANCnJlZ3VsYXRvciAqKScuDQpBbmQgbG9vcCB0byBnZXQgb3B0aW9uYWwgcmVndWxhdG9yIHZi
dXMwLCB2YnVzMSwuLi4sIHZidXNOLg0KQW5kIHRoZW4gZW5hYmxlL2Rpc2FibGUgdGhlIGNvcnJl
c3BvbmRpbmcgcmVndWxhdG9yIGluIA0KZWhjaV9wbGF0Zm9ybV9wb3J0X3Bvd2VyIHRoYW5rcyB0
byBwb3J0bnVtLg0KDQo+PiAgIEV4YW1wbGUgKFNlcXVvaWEgNDQwRVB4KToNCj4+ICAgICAgIGVo
Y2lAZTAwMDAzMDAgew0KPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvdXNiL2hvc3QvZWhjaS1wbGF0
Zm9ybS5jIGIvZHJpdmVycy91c2IvaG9zdC9laGNpLXBsYXRmb3JtLmMNCj4+IGluZGV4IGIwNjVh
OTYuLjA1YmUxMDAgMTAwNjQ0DQo+PiAtLS0gYS9kcml2ZXJzL3VzYi9ob3N0L2VoY2ktcGxhdGZv
cm0uYw0KPj4gKysrIGIvZHJpdmVycy91c2IvaG9zdC9laGNpLXBsYXRmb3JtLmMNCj4+IEBAIC0y
OSw2ICsyOSw3IEBADQo+PiAgICNpbmNsdWRlIDxsaW51eC9vZi5oPg0KPj4gICAjaW5jbHVkZSA8
bGludXgvcGh5L3BoeS5oPg0KPj4gICAjaW5jbHVkZSA8bGludXgvcGxhdGZvcm1fZGV2aWNlLmg+
DQo+PiArI2luY2x1ZGUgPGxpbnV4L3JlZ3VsYXRvci9jb25zdW1lci5oPg0KPj4gICAjaW5jbHVk
ZSA8bGludXgvcmVzZXQuaD4NCj4+ICAgI2luY2x1ZGUgPGxpbnV4L3VzYi5oPg0KPj4gICAjaW5j
bHVkZSA8bGludXgvdXNiL2hjZC5oPg0KPj4gQEAgLTQ2LDYgKzQ3LDcgQEAgc3RydWN0IGVoY2lf
cGxhdGZvcm1fcHJpdiB7DQo+PiAgIAlzdHJ1Y3QgcmVzZXRfY29udHJvbCAqcnN0czsNCj4+ICAg
CXN0cnVjdCBwaHkgKipwaHlzOw0KPj4gICAJaW50IG51bV9waHlzOw0KPj4gKwlzdHJ1Y3QgcmVn
dWxhdG9yICp2YnVzX3N1cHBseTsNCj4+ICAgCWJvb2wgcmVzZXRfb25fcmVzdW1lOw0KPj4gICB9
Ow0KPj4gICANCj4+IEBAIC03Niw2ICs3OCwyNSBAQCBzdGF0aWMgaW50IGVoY2lfcGxhdGZvcm1f
cmVzZXQoc3RydWN0IHVzYl9oY2QgKmhjZCkNCj4+ICAgCXJldHVybiAwOw0KPj4gICB9DQo+PiAg
IA0KPj4gK3N0YXRpYyBpbnQgZWhjaV9wbGF0Zm9ybV9wb3J0X3Bvd2VyKHN0cnVjdCB1c2JfaGNk
ICpoY2QsIGludCBwb3J0bnVtLA0KPj4gKwkJCQkgICAgYm9vbCBlbmFibGUpDQo+PiArew0KPj4g
KwlzdHJ1Y3QgZWhjaV9wbGF0Zm9ybV9wcml2ICpwcml2ID0gaGNkX3RvX2VoY2lfcHJpdihoY2Qp
Ow0KPj4gKwlpbnQgcmV0ID0gMDsNCj4+ICsNCj4+ICsJaWYgKHByaXYtPnZidXNfc3VwcGx5KSB7
DQo+PiArCQlpZiAoZW5hYmxlKQ0KPj4gKwkJCXJldCA9IHJlZ3VsYXRvcl9lbmFibGUocHJpdi0+
dmJ1c19zdXBwbHkpOw0KPj4gKwkJZWxzZQ0KPj4gKwkJCXJldCA9IHJlZ3VsYXRvcl9kaXNhYmxl
KHByaXYtPnZidXNfc3VwcGx5KTsNCj4+ICsJCWlmIChyZXQpDQo+PiArCQkJZGV2X2VycihoY2Qt
PnNlbGYuY29udHJvbGxlciwNCj4+ICsJCQkJImZhaWxlZCB0byAlcyB2YnVzIHN1cHBseTogJWRc
biIsDQo+PiArCQkJCWVuYWJsZSA/ICJlbmFibGUiIDogImRpc2FibGUiLCByZXQpOw0KPj4gKwl9
DQo+PiArCXJldHVybiByZXQ7DQo+PiArfQ0KPj4gKw0KPj4gICBzdGF0aWMgaW50IGVoY2lfcGxh
dGZvcm1fcG93ZXJfb24oc3RydWN0IHBsYXRmb3JtX2RldmljZSAqZGV2KQ0KPj4gICB7DQo+PiAg
IAlzdHJ1Y3QgdXNiX2hjZCAqaGNkID0gcGxhdGZvcm1fZ2V0X2RydmRhdGEoZGV2KTsNCj4+IEBA
IC0xMzQsNiArMTU1LDcgQEAgc3RhdGljIHN0cnVjdCBoY19kcml2ZXIgX19yZWFkX21vc3RseSBl
aGNpX3BsYXRmb3JtX2hjX2RyaXZlcjsNCj4+ICAgc3RhdGljIGNvbnN0IHN0cnVjdCBlaGNpX2Ry
aXZlcl9vdmVycmlkZXMgcGxhdGZvcm1fb3ZlcnJpZGVzIF9faW5pdGNvbnN0ID0gew0KPj4gICAJ
LnJlc2V0ID0JCWVoY2lfcGxhdGZvcm1fcmVzZXQsDQo+PiAgIAkuZXh0cmFfcHJpdl9zaXplID0J
c2l6ZW9mKHN0cnVjdCBlaGNpX3BsYXRmb3JtX3ByaXYpLA0KPj4gKwkucG9ydF9wb3dlciA9CQll
aGNpX3BsYXRmb3JtX3BvcnRfcG93ZXIsDQo+PiAgIH07DQo+PiAgIA0KPj4gICBzdGF0aWMgc3Ry
dWN0IHVzYl9laGNpX3BkYXRhIGVoY2lfcGxhdGZvcm1fZGVmYXVsdHMgPSB7DQo+PiBAQCAtMjQ3
LDYgKzI2OSwxNSBAQCBzdGF0aWMgaW50IGVoY2lfcGxhdGZvcm1fcHJvYmUoc3RydWN0IHBsYXRm
b3JtX2RldmljZSAqZGV2KQ0KPj4gICAJaWYgKGVycikNCj4+ICAgCQlnb3RvIGVycl9wdXRfY2xr
czsNCj4+ICAgDQo+PiArCXByaXYtPnZidXNfc3VwcGx5ID0gZGV2bV9yZWd1bGF0b3JfZ2V0X29w
dGlvbmFsKCZkZXYtPmRldiwgInZidXMiKTsNCj4+ICsJaWYgKElTX0VSUihwcml2LT52YnVzX3N1
cHBseSkpIHsNCj4+ICsJCWVyciA9IFBUUl9FUlIocHJpdi0+dmJ1c19zdXBwbHkpOw0KPj4gKwkJ
aWYgKGVyciA9PSAtRU5PREVWKQ0KPj4gKwkJCXByaXYtPnZidXNfc3VwcGx5ID0gTlVMTDsNCj4+
ICsJCWVsc2UNCj4+ICsJCQlnb3RvIGVycl9yZXNldDsNCj4+ICsJfQ0KPj4gKw0KPj4gICAJaWYg
KHBkYXRhLT5iaWdfZW5kaWFuX2Rlc2MpDQo+PiAgIAkJZWhjaS0+YmlnX2VuZGlhbl9kZXNjID0g
MTsNCj4+ICAgCWlmIChwZGF0YS0+YmlnX2VuZGlhbl9tbWlvKQ0KPj4NCj4g
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

Hi,

On 02/20/2018 03:00 PM, Roger Quadros wrote:
> Hi,
> 
> On 20/02/18 14:58, Amelie Delaunay wrote:
>> On some boards, especially when vbus supply requires large current,
>> and the charge pump on the PHY isn't enough, an external vbus power switch
>> may be used.
>> Add support for this optional external vbus supply in ehci-platform.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>
>> ---
>> Changes in v2:
>>   * Address Roger Quadros comments: move regulator_enable/disable from
>> ehci_platform_power_on/off to ehci_platform_port_power.
>> ---
>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>   2 files changed, 32 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> index 3efde12..fc480cd 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> @@ -19,6 +19,7 @@ Optional properties:
>>    - phys : phandle + phy specifier pair
>>    - phy-names : "usb"
>>    - resets : phandle + reset specifier pair
>> + - vbus-supply : phandle of regulator supplying vbus
>>   
> 
> Can platforms have more than one regulator e.g. one regulator per port?
> 

I imagine that yes, platforms could have one regulator per port.
Regulator consumers bindings impose a <name>-supply property per 
regulator, so, what do you think about :
vbus0-supply for port#0
vbus1-supply for port#1
...
vbusN-supply for port#N

And then in probe, allocate 'struct regulator *vbus_supplies' with a 
size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct 
regulator *)'.
And loop to get optional regulator vbus0, vbus1,..., vbusN.
And then enable/disable the corresponding regulator in 
ehci_platform_port_power thanks to portnum.

>>   Example (Sequoia 440EPx):
>>       ehci at e0000300 {
>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>> index b065a96..05be100 100644
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -29,6 +29,7 @@
>>   #include <linux/of.h>
>>   #include <linux/phy/phy.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>>   #include <linux/reset.h>
>>   #include <linux/usb.h>
>>   #include <linux/usb/hcd.h>
>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>   	struct reset_control *rsts;
>>   	struct phy **phys;
>>   	int num_phys;
>> +	struct regulator *vbus_supply;
>>   	bool reset_on_resume;
>>   };
>>   
>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>   	return 0;
>>   }
>>   
>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>> +				    bool enable)
>> +{
>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>> +	int ret = 0;
>> +
>> +	if (priv->vbus_supply) {
>> +		if (enable)
>> +			ret = regulator_enable(priv->vbus_supply);
>> +		else
>> +			ret = regulator_disable(priv->vbus_supply);
>> +		if (ret)
>> +			dev_err(hcd->self.controller,
>> +				"failed to %s vbus supply: %d\n",
>> +				enable ? "enable" : "disable", ret);
>> +	}
>> +	return ret;
>> +}
>> +
>>   static int ehci_platform_power_on(struct platform_device *dev)
>>   {
>>   	struct usb_hcd *hcd = platform_get_drvdata(dev);
>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>>   static const struct ehci_driver_overrides platform_overrides __initconst = {
>>   	.reset =		ehci_platform_reset,
>>   	.extra_priv_size =	sizeof(struct ehci_platform_priv),
>> +	.port_power =		ehci_platform_port_power,
>>   };
>>   
>>   static struct usb_ehci_pdata ehci_platform_defaults = {
>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>>   	if (err)
>>   		goto err_put_clks;
>>   
>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
>> +	if (IS_ERR(priv->vbus_supply)) {
>> +		err = PTR_ERR(priv->vbus_supply);
>> +		if (err == -ENODEV)
>> +			priv->vbus_supply = NULL;
>> +		else
>> +			goto err_reset;
>> +	}
>> +
>>   	if (pdata->big_endian_desc)
>>   		ehci->big_endian_desc = 1;
>>   	if (pdata->big_endian_mmio)
>>
> 

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Roger Quadros]

On 20/02/18 16:46, Amelie DELAUNAY wrote:
> Hi,
> 
> On 02/20/2018 03:00 PM, Roger Quadros wrote:
>> Hi,
>>
>> On 20/02/18 14:58, Amelie Delaunay wrote:
>>> On some boards, especially when vbus supply requires large current,
>>> and the charge pump on the PHY isn't enough, an external vbus power switch
>>> may be used.
>>> Add support for this optional external vbus supply in ehci-platform.
>>>
>>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>>
>>> ---
>>> Changes in v2:
>>>   * Address Roger Quadros comments: move regulator_enable/disable from
>>> ehci_platform_power_on/off to ehci_platform_port_power.
>>> ---
>>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>>   2 files changed, 32 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>> index 3efde12..fc480cd 100644
>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>> @@ -19,6 +19,7 @@ Optional properties:
>>>    - phys : phandle + phy specifier pair
>>>    - phy-names : "usb"
>>>    - resets : phandle + reset specifier pair
>>> + - vbus-supply : phandle of regulator supplying vbus
>>>   
>>
>> Can platforms have more than one regulator e.g. one regulator per port?
>>
> 
> I imagine that yes, platforms could have one regulator per port.
> Regulator consumers bindings impose a <name>-supply property per 
> regulator, so, what do you think about :
> vbus0-supply for port#0
> vbus1-supply for port#1
> ...
> vbusN-supply for port#N
> 
> And then in probe, allocate 'struct regulator *vbus_supplies' with a 
> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct 
> regulator *)'.
> And loop to get optional regulator vbus0, vbus1,..., vbusN.
> And then enable/disable the corresponding regulator in 
> ehci_platform_port_power thanks to portnum.

Looks fine to me but we need to get Alan's opinion if this is worth the effort.
If there isn't a single platform needing it we could probably do without it
but the DT binding must be scalable to add this feature in the future.

And what if it is ganged power? i.e. one regulator for more than one port.
Probably they all can point to the same regulator instance and it should work.

> 
>>>   Example (Sequoia 440EPx):
>>>       ehci@e0000300 {
>>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>>> index b065a96..05be100 100644
>>> --- a/drivers/usb/host/ehci-platform.c
>>> +++ b/drivers/usb/host/ehci-platform.c
>>> @@ -29,6 +29,7 @@
>>>   #include <linux/of.h>
>>>   #include <linux/phy/phy.h>
>>>   #include <linux/platform_device.h>
>>> +#include <linux/regulator/consumer.h>
>>>   #include <linux/reset.h>
>>>   #include <linux/usb.h>
>>>   #include <linux/usb/hcd.h>
>>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>>   	struct reset_control *rsts;
>>>   	struct phy **phys;
>>>   	int num_phys;
>>> +	struct regulator *vbus_supply;
>>>   	bool reset_on_resume;
>>>   };
>>>   
>>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>>   	return 0;
>>>   }
>>>   
>>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>>> +				    bool enable)
>>> +{
>>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>>> +	int ret = 0;
>>> +
>>> +	if (priv->vbus_supply) {
>>> +		if (enable)
>>> +			ret = regulator_enable(priv->vbus_supply);
>>> +		else
>>> +			ret = regulator_disable(priv->vbus_supply);
>>> +		if (ret)
>>> +			dev_err(hcd->self.controller,
>>> +				"failed to %s vbus supply: %d\n",
>>> +				enable ? "enable" : "disable", ret);
>>> +	}
>>> +	return ret;
>>> +}
>>> +
>>>   static int ehci_platform_power_on(struct platform_device *dev)
>>>   {
>>>   	struct usb_hcd *hcd = platform_get_drvdata(dev);
>>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>>>   static const struct ehci_driver_overrides platform_overrides __initconst = {
>>>   	.reset =		ehci_platform_reset,
>>>   	.extra_priv_size =	sizeof(struct ehci_platform_priv),
>>> +	.port_power =		ehci_platform_port_power,
>>>   };
>>>   
>>>   static struct usb_ehci_pdata ehci_platform_defaults = {
>>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>>>   	if (err)
>>>   		goto err_put_clks;
>>>   
>>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
>>> +	if (IS_ERR(priv->vbus_supply)) {
>>> +		err = PTR_ERR(priv->vbus_supply);
>>> +		if (err == -ENODEV)
>>> +			priv->vbus_supply = NULL;
>>> +		else
>>> +			goto err_reset;
>>> +	}
>>> +
>>>   	if (pdata->big_endian_desc)
>>>   		ehci->big_endian_desc = 1;
>>>   	if (pdata->big_endian_mmio)
>>>

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Roger Quadros]

On 20/02/18 16:46, Amelie DELAUNAY wrote:
> Hi,
> 
> On 02/20/2018 03:00 PM, Roger Quadros wrote:
>> Hi,
>>
>> On 20/02/18 14:58, Amelie Delaunay wrote:
>>> On some boards, especially when vbus supply requires large current,
>>> and the charge pump on the PHY isn't enough, an external vbus power switch
>>> may be used.
>>> Add support for this optional external vbus supply in ehci-platform.
>>>
>>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>>
>>> ---
>>> Changes in v2:
>>>   * Address Roger Quadros comments: move regulator_enable/disable from
>>> ehci_platform_power_on/off to ehci_platform_port_power.
>>> ---
>>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>>   2 files changed, 32 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>> index 3efde12..fc480cd 100644
>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>> @@ -19,6 +19,7 @@ Optional properties:
>>>    - phys : phandle + phy specifier pair
>>>    - phy-names : "usb"
>>>    - resets : phandle + reset specifier pair
>>> + - vbus-supply : phandle of regulator supplying vbus
>>>   
>>
>> Can platforms have more than one regulator e.g. one regulator per port?
>>
> 
> I imagine that yes, platforms could have one regulator per port.
> Regulator consumers bindings impose a <name>-supply property per 
> regulator, so, what do you think about :
> vbus0-supply for port#0
> vbus1-supply for port#1
> ...
> vbusN-supply for port#N
> 
> And then in probe, allocate 'struct regulator *vbus_supplies' with a 
> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct 
> regulator *)'.
> And loop to get optional regulator vbus0, vbus1,..., vbusN.
> And then enable/disable the corresponding regulator in 
> ehci_platform_port_power thanks to portnum.

Looks fine to me but we need to get Alan's opinion if this is worth the effort.
If there isn't a single platform needing it we could probably do without it
but the DT binding must be scalable to add this feature in the future.

And what if it is ganged power? i.e. one regulator for more than one port.
Probably they all can point to the same regulator instance and it should work.

> 
>>>   Example (Sequoia 440EPx):
>>>       ehci@e0000300 {
>>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>>> index b065a96..05be100 100644
>>> --- a/drivers/usb/host/ehci-platform.c
>>> +++ b/drivers/usb/host/ehci-platform.c
>>> @@ -29,6 +29,7 @@
>>>   #include <linux/of.h>
>>>   #include <linux/phy/phy.h>
>>>   #include <linux/platform_device.h>
>>> +#include <linux/regulator/consumer.h>
>>>   #include <linux/reset.h>
>>>   #include <linux/usb.h>
>>>   #include <linux/usb/hcd.h>
>>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>>   	struct reset_control *rsts;
>>>   	struct phy **phys;
>>>   	int num_phys;
>>> +	struct regulator *vbus_supply;
>>>   	bool reset_on_resume;
>>>   };
>>>   
>>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>>   	return 0;
>>>   }
>>>   
>>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>>> +				    bool enable)
>>> +{
>>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>>> +	int ret = 0;
>>> +
>>> +	if (priv->vbus_supply) {
>>> +		if (enable)
>>> +			ret = regulator_enable(priv->vbus_supply);
>>> +		else
>>> +			ret = regulator_disable(priv->vbus_supply);
>>> +		if (ret)
>>> +			dev_err(hcd->self.controller,
>>> +				"failed to %s vbus supply: %d\n",
>>> +				enable ? "enable" : "disable", ret);
>>> +	}
>>> +	return ret;
>>> +}
>>> +
>>>   static int ehci_platform_power_on(struct platform_device *dev)
>>>   {
>>>   	struct usb_hcd *hcd = platform_get_drvdata(dev);
>>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>>>   static const struct ehci_driver_overrides platform_overrides __initconst = {
>>>   	.reset =		ehci_platform_reset,
>>>   	.extra_priv_size =	sizeof(struct ehci_platform_priv),
>>> +	.port_power =		ehci_platform_port_power,
>>>   };
>>>   
>>>   static struct usb_ehci_pdata ehci_platform_defaults = {
>>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>>>   	if (err)
>>>   		goto err_put_clks;
>>>   
>>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
>>> +	if (IS_ERR(priv->vbus_supply)) {
>>> +		err = PTR_ERR(priv->vbus_supply);
>>> +		if (err == -ENODEV)
>>> +			priv->vbus_supply = NULL;
>>> +		else
>>> +			goto err_reset;
>>> +	}
>>> +
>>>   	if (pdata->big_endian_desc)
>>>   		ehci->big_endian_desc = 1;
>>>   	if (pdata->big_endian_mmio)
>>>

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[v2] usb: host: ehci-platform: add support for optional external vbus supply

[Roger Quadros]

On 20/02/18 16:46, Amelie DELAUNAY wrote:
> Hi,
> 
> On 02/20/2018 03:00 PM, Roger Quadros wrote:
>> Hi,
>>
>> On 20/02/18 14:58, Amelie Delaunay wrote:
>>> On some boards, especially when vbus supply requires large current,
>>> and the charge pump on the PHY isn't enough, an external vbus power switch
>>> may be used.
>>> Add support for this optional external vbus supply in ehci-platform.
>>>
>>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>>
>>> ---
>>> Changes in v2:
>>>   * Address Roger Quadros comments: move regulator_enable/disable from
>>> ehci_platform_power_on/off to ehci_platform_port_power.
>>> ---
>>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>>   2 files changed, 32 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>> index 3efde12..fc480cd 100644
>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>> @@ -19,6 +19,7 @@ Optional properties:
>>>    - phys : phandle + phy specifier pair
>>>    - phy-names : "usb"
>>>    - resets : phandle + reset specifier pair
>>> + - vbus-supply : phandle of regulator supplying vbus
>>>   
>>
>> Can platforms have more than one regulator e.g. one regulator per port?
>>
> 
> I imagine that yes, platforms could have one regulator per port.
> Regulator consumers bindings impose a <name>-supply property per 
> regulator, so, what do you think about :
> vbus0-supply for port#0
> vbus1-supply for port#1
> ...
> vbusN-supply for port#N
> 
> And then in probe, allocate 'struct regulator *vbus_supplies' with a 
> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct 
> regulator *)'.
> And loop to get optional regulator vbus0, vbus1,..., vbusN.
> And then enable/disable the corresponding regulator in 
> ehci_platform_port_power thanks to portnum.

Looks fine to me but we need to get Alan's opinion if this is worth the effort.
If there isn't a single platform needing it we could probably do without it
but the DT binding must be scalable to add this feature in the future.

And what if it is ganged power? i.e. one regulator for more than one port.
Probably they all can point to the same regulator instance and it should work.

> 
>>>   Example (Sequoia 440EPx):
>>>       ehci@e0000300 {
>>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>>> index b065a96..05be100 100644
>>> --- a/drivers/usb/host/ehci-platform.c
>>> +++ b/drivers/usb/host/ehci-platform.c
>>> @@ -29,6 +29,7 @@
>>>   #include <linux/of.h>
>>>   #include <linux/phy/phy.h>
>>>   #include <linux/platform_device.h>
>>> +#include <linux/regulator/consumer.h>
>>>   #include <linux/reset.h>
>>>   #include <linux/usb.h>
>>>   #include <linux/usb/hcd.h>
>>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>>   	struct reset_control *rsts;
>>>   	struct phy **phys;
>>>   	int num_phys;
>>> +	struct regulator *vbus_supply;
>>>   	bool reset_on_resume;
>>>   };
>>>   
>>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>>   	return 0;
>>>   }
>>>   
>>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>>> +				    bool enable)
>>> +{
>>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>>> +	int ret = 0;
>>> +
>>> +	if (priv->vbus_supply) {
>>> +		if (enable)
>>> +			ret = regulator_enable(priv->vbus_supply);
>>> +		else
>>> +			ret = regulator_disable(priv->vbus_supply);
>>> +		if (ret)
>>> +			dev_err(hcd->self.controller,
>>> +				"failed to %s vbus supply: %d\n",
>>> +				enable ? "enable" : "disable", ret);
>>> +	}
>>> +	return ret;
>>> +}
>>> +
>>>   static int ehci_platform_power_on(struct platform_device *dev)
>>>   {
>>>   	struct usb_hcd *hcd = platform_get_drvdata(dev);
>>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>>>   static const struct ehci_driver_overrides platform_overrides __initconst = {
>>>   	.reset =		ehci_platform_reset,
>>>   	.extra_priv_size =	sizeof(struct ehci_platform_priv),
>>> +	.port_power =		ehci_platform_port_power,
>>>   };
>>>   
>>>   static struct usb_ehci_pdata ehci_platform_defaults = {
>>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>>>   	if (err)
>>>   		goto err_put_clks;
>>>   
>>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
>>> +	if (IS_ERR(priv->vbus_supply)) {
>>> +		err = PTR_ERR(priv->vbus_supply);
>>> +		if (err == -ENODEV)
>>> +			priv->vbus_supply = NULL;
>>> +		else
>>> +			goto err_reset;
>>> +	}
>>> +
>>>   	if (pdata->big_endian_desc)
>>>   		ehci->big_endian_desc = 1;
>>>   	if (pdata->big_endian_mmio)
>>>

[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Roger Quadros]

On 20/02/18 16:46, Amelie DELAUNAY wrote:
> Hi,
> 
> On 02/20/2018 03:00 PM, Roger Quadros wrote:
>> Hi,
>>
>> On 20/02/18 14:58, Amelie Delaunay wrote:
>>> On some boards, especially when vbus supply requires large current,
>>> and the charge pump on the PHY isn't enough, an external vbus power switch
>>> may be used.
>>> Add support for this optional external vbus supply in ehci-platform.
>>>
>>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>>
>>> ---
>>> Changes in v2:
>>>   * Address Roger Quadros comments: move regulator_enable/disable from
>>> ehci_platform_power_on/off to ehci_platform_port_power.
>>> ---
>>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>>   2 files changed, 32 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>> index 3efde12..fc480cd 100644
>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>> @@ -19,6 +19,7 @@ Optional properties:
>>>    - phys : phandle + phy specifier pair
>>>    - phy-names : "usb"
>>>    - resets : phandle + reset specifier pair
>>> + - vbus-supply : phandle of regulator supplying vbus
>>>   
>>
>> Can platforms have more than one regulator e.g. one regulator per port?
>>
> 
> I imagine that yes, platforms could have one regulator per port.
> Regulator consumers bindings impose a <name>-supply property per 
> regulator, so, what do you think about :
> vbus0-supply for port#0
> vbus1-supply for port#1
> ...
> vbusN-supply for port#N
> 
> And then in probe, allocate 'struct regulator *vbus_supplies' with a 
> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct 
> regulator *)'.
> And loop to get optional regulator vbus0, vbus1,..., vbusN.
> And then enable/disable the corresponding regulator in 
> ehci_platform_port_power thanks to portnum.

Looks fine to me but we need to get Alan's opinion if this is worth the effort.
If there isn't a single platform needing it we could probably do without it
but the DT binding must be scalable to add this feature in the future.

And what if it is ganged power? i.e. one regulator for more than one port.
Probably they all can point to the same regulator instance and it should work.

> 
>>>   Example (Sequoia 440EPx):
>>>       ehci at e0000300 {
>>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>>> index b065a96..05be100 100644
>>> --- a/drivers/usb/host/ehci-platform.c
>>> +++ b/drivers/usb/host/ehci-platform.c
>>> @@ -29,6 +29,7 @@
>>>   #include <linux/of.h>
>>>   #include <linux/phy/phy.h>
>>>   #include <linux/platform_device.h>
>>> +#include <linux/regulator/consumer.h>
>>>   #include <linux/reset.h>
>>>   #include <linux/usb.h>
>>>   #include <linux/usb/hcd.h>
>>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>>   	struct reset_control *rsts;
>>>   	struct phy **phys;
>>>   	int num_phys;
>>> +	struct regulator *vbus_supply;
>>>   	bool reset_on_resume;
>>>   };
>>>   
>>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>>   	return 0;
>>>   }
>>>   
>>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>>> +				    bool enable)
>>> +{
>>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>>> +	int ret = 0;
>>> +
>>> +	if (priv->vbus_supply) {
>>> +		if (enable)
>>> +			ret = regulator_enable(priv->vbus_supply);
>>> +		else
>>> +			ret = regulator_disable(priv->vbus_supply);
>>> +		if (ret)
>>> +			dev_err(hcd->self.controller,
>>> +				"failed to %s vbus supply: %d\n",
>>> +				enable ? "enable" : "disable", ret);
>>> +	}
>>> +	return ret;
>>> +}
>>> +
>>>   static int ehci_platform_power_on(struct platform_device *dev)
>>>   {
>>>   	struct usb_hcd *hcd = platform_get_drvdata(dev);
>>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>>>   static const struct ehci_driver_overrides platform_overrides __initconst = {
>>>   	.reset =		ehci_platform_reset,
>>>   	.extra_priv_size =	sizeof(struct ehci_platform_priv),
>>> +	.port_power =		ehci_platform_port_power,
>>>   };
>>>   
>>>   static struct usb_ehci_pdata ehci_platform_defaults = {
>>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>>>   	if (err)
>>>   		goto err_put_clks;
>>>   
>>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
>>> +	if (IS_ERR(priv->vbus_supply)) {
>>> +		err = PTR_ERR(priv->vbus_supply);
>>> +		if (err == -ENODEV)
>>> +			priv->vbus_supply = NULL;
>>> +		else
>>> +			goto err_reset;
>>> +	}
>>> +
>>>   	if (pdata->big_endian_desc)
>>>   		ehci->big_endian_desc = 1;
>>>   	if (pdata->big_endian_mmio)
>>>

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Alan Stern]

On Tue, 20 Feb 2018, Roger Quadros wrote:

> On 20/02/18 16:46, Amelie DELAUNAY wrote:
> > Hi,
> > 
> > On 02/20/2018 03:00 PM, Roger Quadros wrote:
> >> Hi,
> >>
> >> On 20/02/18 14:58, Amelie Delaunay wrote:
> >>> On some boards, especially when vbus supply requires large current,
> >>> and the charge pump on the PHY isn't enough, an external vbus power switch
> >>> may be used.
> >>> Add support for this optional external vbus supply in ehci-platform.
> >>>
> >>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> >>>
> >>> ---
> >>> Changes in v2:
> >>>   * Address Roger Quadros comments: move regulator_enable/disable from
> >>> ehci_platform_power_on/off to ehci_platform_port_power.
> >>> ---
> >>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
> >>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
> >>>   2 files changed, 32 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>> index 3efde12..fc480cd 100644
> >>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>> @@ -19,6 +19,7 @@ Optional properties:
> >>>    - phys : phandle + phy specifier pair
> >>>    - phy-names : "usb"
> >>>    - resets : phandle + reset specifier pair
> >>> + - vbus-supply : phandle of regulator supplying vbus
> >>>   
> >>
> >> Can platforms have more than one regulator e.g. one regulator per port?
> >>
> > 
> > I imagine that yes, platforms could have one regulator per port.
> > Regulator consumers bindings impose a <name>-supply property per 
> > regulator, so, what do you think about :
> > vbus0-supply for port#0
> > vbus1-supply for port#1
> > ...
> > vbusN-supply for port#N
> > 
> > And then in probe, allocate 'struct regulator *vbus_supplies' with a 
> > size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct 
> > regulator *)'.
> > And loop to get optional regulator vbus0, vbus1,..., vbusN.
> > And then enable/disable the corresponding regulator in 
> > ehci_platform_port_power thanks to portnum.
> 
> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
> If there isn't a single platform needing it we could probably do without it
> but the DT binding must be scalable to add this feature in the future.

I agree that for now there don't seem to be any platforms requiring 
more than one regulator, but this should be implemented in a way that 
could be expanded if necessary.

Anyway, the basic idea is reasonable.  I don't know to what extent 
people want to power-off their EHCI ports, but if they do then we ought 
to turn off external regulators at the same time.

Is there a real-life use case for this?

Alan Stern

> And what if it is ganged power? i.e. one regulator for more than one port.
> Probably they all can point to the same regulator instance and it should work.
> 
> > 
> >>>   Example (Sequoia 440EPx):
> >>>       ehci@e0000300 {
> >>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> >>> index b065a96..05be100 100644
> >>> --- a/drivers/usb/host/ehci-platform.c
> >>> +++ b/drivers/usb/host/ehci-platform.c
> >>> @@ -29,6 +29,7 @@
> >>>   #include <linux/of.h>
> >>>   #include <linux/phy/phy.h>
> >>>   #include <linux/platform_device.h>
> >>> +#include <linux/regulator/consumer.h>
> >>>   #include <linux/reset.h>
> >>>   #include <linux/usb.h>
> >>>   #include <linux/usb/hcd.h>
> >>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
> >>>   	struct reset_control *rsts;
> >>>   	struct phy **phys;
> >>>   	int num_phys;
> >>> +	struct regulator *vbus_supply;
> >>>   	bool reset_on_resume;
> >>>   };
> >>>   
> >>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
> >>>   	return 0;
> >>>   }
> >>>   
> >>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> >>> +				    bool enable)
> >>> +{
> >>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> >>> +	int ret = 0;
> >>> +
> >>> +	if (priv->vbus_supply) {
> >>> +		if (enable)
> >>> +			ret = regulator_enable(priv->vbus_supply);
> >>> +		else
> >>> +			ret = regulator_disable(priv->vbus_supply);
> >>> +		if (ret)
> >>> +			dev_err(hcd->self.controller,
> >>> +				"failed to %s vbus supply: %d\n",
> >>> +				enable ? "enable" : "disable", ret);
> >>> +	}
> >>> +	return ret;
> >>> +}
> >>> +
> >>>   static int ehci_platform_power_on(struct platform_device *dev)
> >>>   {
> >>>   	struct usb_hcd *hcd = platform_get_drvdata(dev);
> >>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
> >>>   static const struct ehci_driver_overrides platform_overrides __initconst = {
> >>>   	.reset =		ehci_platform_reset,
> >>>   	.extra_priv_size =	sizeof(struct ehci_platform_priv),
> >>> +	.port_power =		ehci_platform_port_power,
> >>>   };
> >>>   
> >>>   static struct usb_ehci_pdata ehci_platform_defaults = {
> >>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
> >>>   	if (err)
> >>>   		goto err_put_clks;
> >>>   
> >>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
> >>> +	if (IS_ERR(priv->vbus_supply)) {
> >>> +		err = PTR_ERR(priv->vbus_supply);
> >>> +		if (err == -ENODEV)
> >>> +			priv->vbus_supply = NULL;
> >>> +		else
> >>> +			goto err_reset;
> >>> +	}
> >>> +
> >>>   	if (pdata->big_endian_desc)
> >>>   		ehci->big_endian_desc = 1;
> >>>   	if (pdata->big_endian_mmio)
> >>>
> 
> 

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Alan Stern]

On Tue, 20 Feb 2018, Roger Quadros wrote:

> On 20/02/18 16:46, Amelie DELAUNAY wrote:
> > Hi,
> > 
> > On 02/20/2018 03:00 PM, Roger Quadros wrote:
> >> Hi,
> >>
> >> On 20/02/18 14:58, Amelie Delaunay wrote:
> >>> On some boards, especially when vbus supply requires large current,
> >>> and the charge pump on the PHY isn't enough, an external vbus power switch
> >>> may be used.
> >>> Add support for this optional external vbus supply in ehci-platform.
> >>>
> >>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> >>>
> >>> ---
> >>> Changes in v2:
> >>>   * Address Roger Quadros comments: move regulator_enable/disable from
> >>> ehci_platform_power_on/off to ehci_platform_port_power.
> >>> ---
> >>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
> >>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
> >>>   2 files changed, 32 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>> index 3efde12..fc480cd 100644
> >>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>> @@ -19,6 +19,7 @@ Optional properties:
> >>>    - phys : phandle + phy specifier pair
> >>>    - phy-names : "usb"
> >>>    - resets : phandle + reset specifier pair
> >>> + - vbus-supply : phandle of regulator supplying vbus
> >>>   
> >>
> >> Can platforms have more than one regulator e.g. one regulator per port?
> >>
> > 
> > I imagine that yes, platforms could have one regulator per port.
> > Regulator consumers bindings impose a <name>-supply property per 
> > regulator, so, what do you think about :
> > vbus0-supply for port#0
> > vbus1-supply for port#1
> > ...
> > vbusN-supply for port#N
> > 
> > And then in probe, allocate 'struct regulator *vbus_supplies' with a 
> > size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct 
> > regulator *)'.
> > And loop to get optional regulator vbus0, vbus1,..., vbusN.
> > And then enable/disable the corresponding regulator in 
> > ehci_platform_port_power thanks to portnum.
> 
> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
> If there isn't a single platform needing it we could probably do without it
> but the DT binding must be scalable to add this feature in the future.

I agree that for now there don't seem to be any platforms requiring 
more than one regulator, but this should be implemented in a way that 
could be expanded if necessary.

Anyway, the basic idea is reasonable.  I don't know to what extent 
people want to power-off their EHCI ports, but if they do then we ought 
to turn off external regulators at the same time.

Is there a real-life use case for this?

Alan Stern

> And what if it is ganged power? i.e. one regulator for more than one port.
> Probably they all can point to the same regulator instance and it should work.
> 
> > 
> >>>   Example (Sequoia 440EPx):
> >>>       ehci@e0000300 {
> >>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> >>> index b065a96..05be100 100644
> >>> --- a/drivers/usb/host/ehci-platform.c
> >>> +++ b/drivers/usb/host/ehci-platform.c
> >>> @@ -29,6 +29,7 @@
> >>>   #include <linux/of.h>
> >>>   #include <linux/phy/phy.h>
> >>>   #include <linux/platform_device.h>
> >>> +#include <linux/regulator/consumer.h>
> >>>   #include <linux/reset.h>
> >>>   #include <linux/usb.h>
> >>>   #include <linux/usb/hcd.h>
> >>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
> >>>   	struct reset_control *rsts;
> >>>   	struct phy **phys;
> >>>   	int num_phys;
> >>> +	struct regulator *vbus_supply;
> >>>   	bool reset_on_resume;
> >>>   };
> >>>   
> >>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
> >>>   	return 0;
> >>>   }
> >>>   
> >>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> >>> +				    bool enable)
> >>> +{
> >>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> >>> +	int ret = 0;
> >>> +
> >>> +	if (priv->vbus_supply) {
> >>> +		if (enable)
> >>> +			ret = regulator_enable(priv->vbus_supply);
> >>> +		else
> >>> +			ret = regulator_disable(priv->vbus_supply);
> >>> +		if (ret)
> >>> +			dev_err(hcd->self.controller,
> >>> +				"failed to %s vbus supply: %d\n",
> >>> +				enable ? "enable" : "disable", ret);
> >>> +	}
> >>> +	return ret;
> >>> +}
> >>> +
> >>>   static int ehci_platform_power_on(struct platform_device *dev)
> >>>   {
> >>>   	struct usb_hcd *hcd = platform_get_drvdata(dev);
> >>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
> >>>   static const struct ehci_driver_overrides platform_overrides __initconst = {
> >>>   	.reset =		ehci_platform_reset,
> >>>   	.extra_priv_size =	sizeof(struct ehci_platform_priv),
> >>> +	.port_power =		ehci_platform_port_power,
> >>>   };
> >>>   
> >>>   static struct usb_ehci_pdata ehci_platform_defaults = {
> >>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
> >>>   	if (err)
> >>>   		goto err_put_clks;
> >>>   
> >>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
> >>> +	if (IS_ERR(priv->vbus_supply)) {
> >>> +		err = PTR_ERR(priv->vbus_supply);
> >>> +		if (err == -ENODEV)
> >>> +			priv->vbus_supply = NULL;
> >>> +		else
> >>> +			goto err_reset;
> >>> +	}
> >>> +
> >>>   	if (pdata->big_endian_desc)
> >>>   		ehci->big_endian_desc = 1;
> >>>   	if (pdata->big_endian_mmio)
> >>>
> 
> 

[v2] usb: host: ehci-platform: add support for optional external vbus supply

[Alan Stern]

On Tue, 20 Feb 2018, Roger Quadros wrote:

> On 20/02/18 16:46, Amelie DELAUNAY wrote:
> > Hi,
> > 
> > On 02/20/2018 03:00 PM, Roger Quadros wrote:
> >> Hi,
> >>
> >> On 20/02/18 14:58, Amelie Delaunay wrote:
> >>> On some boards, especially when vbus supply requires large current,
> >>> and the charge pump on the PHY isn't enough, an external vbus power switch
> >>> may be used.
> >>> Add support for this optional external vbus supply in ehci-platform.
> >>>
> >>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> >>>
> >>> ---
> >>> Changes in v2:
> >>>   * Address Roger Quadros comments: move regulator_enable/disable from
> >>> ehci_platform_power_on/off to ehci_platform_port_power.
> >>> ---
> >>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
> >>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
> >>>   2 files changed, 32 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>> index 3efde12..fc480cd 100644
> >>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>> @@ -19,6 +19,7 @@ Optional properties:
> >>>    - phys : phandle + phy specifier pair
> >>>    - phy-names : "usb"
> >>>    - resets : phandle + reset specifier pair
> >>> + - vbus-supply : phandle of regulator supplying vbus
> >>>   
> >>
> >> Can platforms have more than one regulator e.g. one regulator per port?
> >>
> > 
> > I imagine that yes, platforms could have one regulator per port.
> > Regulator consumers bindings impose a <name>-supply property per 
> > regulator, so, what do you think about :
> > vbus0-supply for port#0
> > vbus1-supply for port#1
> > ...
> > vbusN-supply for port#N
> > 
> > And then in probe, allocate 'struct regulator *vbus_supplies' with a 
> > size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct 
> > regulator *)'.
> > And loop to get optional regulator vbus0, vbus1,..., vbusN.
> > And then enable/disable the corresponding regulator in 
> > ehci_platform_port_power thanks to portnum.
> 
> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
> If there isn't a single platform needing it we could probably do without it
> but the DT binding must be scalable to add this feature in the future.

I agree that for now there don't seem to be any platforms requiring 
more than one regulator, but this should be implemented in a way that 
could be expanded if necessary.

Anyway, the basic idea is reasonable.  I don't know to what extent 
people want to power-off their EHCI ports, but if they do then we ought 
to turn off external regulators at the same time.

Is there a real-life use case for this?

Alan Stern

> And what if it is ganged power? i.e. one regulator for more than one port.
> Probably they all can point to the same regulator instance and it should work.
> 
> > 
> >>>   Example (Sequoia 440EPx):
> >>>       ehci@e0000300 {
> >>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> >>> index b065a96..05be100 100644
> >>> --- a/drivers/usb/host/ehci-platform.c
> >>> +++ b/drivers/usb/host/ehci-platform.c
> >>> @@ -29,6 +29,7 @@
> >>>   #include <linux/of.h>
> >>>   #include <linux/phy/phy.h>
> >>>   #include <linux/platform_device.h>
> >>> +#include <linux/regulator/consumer.h>
> >>>   #include <linux/reset.h>
> >>>   #include <linux/usb.h>
> >>>   #include <linux/usb/hcd.h>
> >>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
> >>>   	struct reset_control *rsts;
> >>>   	struct phy **phys;
> >>>   	int num_phys;
> >>> +	struct regulator *vbus_supply;
> >>>   	bool reset_on_resume;
> >>>   };
> >>>   
> >>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
> >>>   	return 0;
> >>>   }
> >>>   
> >>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> >>> +				    bool enable)
> >>> +{
> >>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> >>> +	int ret = 0;
> >>> +
> >>> +	if (priv->vbus_supply) {
> >>> +		if (enable)
> >>> +			ret = regulator_enable(priv->vbus_supply);
> >>> +		else
> >>> +			ret = regulator_disable(priv->vbus_supply);
> >>> +		if (ret)
> >>> +			dev_err(hcd->self.controller,
> >>> +				"failed to %s vbus supply: %d\n",
> >>> +				enable ? "enable" : "disable", ret);
> >>> +	}
> >>> +	return ret;
> >>> +}
> >>> +
> >>>   static int ehci_platform_power_on(struct platform_device *dev)
> >>>   {
> >>>   	struct usb_hcd *hcd = platform_get_drvdata(dev);
> >>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
> >>>   static const struct ehci_driver_overrides platform_overrides __initconst = {
> >>>   	.reset =		ehci_platform_reset,
> >>>   	.extra_priv_size =	sizeof(struct ehci_platform_priv),
> >>> +	.port_power =		ehci_platform_port_power,
> >>>   };
> >>>   
> >>>   static struct usb_ehci_pdata ehci_platform_defaults = {
> >>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
> >>>   	if (err)
> >>>   		goto err_put_clks;
> >>>   
> >>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
> >>> +	if (IS_ERR(priv->vbus_supply)) {
> >>> +		err = PTR_ERR(priv->vbus_supply);
> >>> +		if (err == -ENODEV)
> >>> +			priv->vbus_supply = NULL;
> >>> +		else
> >>> +			goto err_reset;
> >>> +	}
> >>> +
> >>>   	if (pdata->big_endian_desc)
> >>>   		ehci->big_endian_desc = 1;
> >>>   	if (pdata->big_endian_mmio)
> >>>
> 
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Alan Stern]

On Tue, 20 Feb 2018, Roger Quadros wrote:

> On 20/02/18 16:46, Amelie DELAUNAY wrote:
> > Hi,
> > 
> > On 02/20/2018 03:00 PM, Roger Quadros wrote:
> >> Hi,
> >>
> >> On 20/02/18 14:58, Amelie Delaunay wrote:
> >>> On some boards, especially when vbus supply requires large current,
> >>> and the charge pump on the PHY isn't enough, an external vbus power switch
> >>> may be used.
> >>> Add support for this optional external vbus supply in ehci-platform.
> >>>
> >>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> >>>
> >>> ---
> >>> Changes in v2:
> >>>   * Address Roger Quadros comments: move regulator_enable/disable from
> >>> ehci_platform_power_on/off to ehci_platform_port_power.
> >>> ---
> >>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
> >>>   drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
> >>>   2 files changed, 32 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>> index 3efde12..fc480cd 100644
> >>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>> @@ -19,6 +19,7 @@ Optional properties:
> >>>    - phys : phandle + phy specifier pair
> >>>    - phy-names : "usb"
> >>>    - resets : phandle + reset specifier pair
> >>> + - vbus-supply : phandle of regulator supplying vbus
> >>>   
> >>
> >> Can platforms have more than one regulator e.g. one regulator per port?
> >>
> > 
> > I imagine that yes, platforms could have one regulator per port.
> > Regulator consumers bindings impose a <name>-supply property per 
> > regulator, so, what do you think about :
> > vbus0-supply for port#0
> > vbus1-supply for port#1
> > ...
> > vbusN-supply for port#N
> > 
> > And then in probe, allocate 'struct regulator *vbus_supplies' with a 
> > size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct 
> > regulator *)'.
> > And loop to get optional regulator vbus0, vbus1,..., vbusN.
> > And then enable/disable the corresponding regulator in 
> > ehci_platform_port_power thanks to portnum.
> 
> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
> If there isn't a single platform needing it we could probably do without it
> but the DT binding must be scalable to add this feature in the future.

I agree that for now there don't seem to be any platforms requiring 
more than one regulator, but this should be implemented in a way that 
could be expanded if necessary.

Anyway, the basic idea is reasonable.  I don't know to what extent 
people want to power-off their EHCI ports, but if they do then we ought 
to turn off external regulators at the same time.

Is there a real-life use case for this?

Alan Stern

> And what if it is ganged power? i.e. one regulator for more than one port.
> Probably they all can point to the same regulator instance and it should work.
> 
> > 
> >>>   Example (Sequoia 440EPx):
> >>>       ehci at e0000300 {
> >>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> >>> index b065a96..05be100 100644
> >>> --- a/drivers/usb/host/ehci-platform.c
> >>> +++ b/drivers/usb/host/ehci-platform.c
> >>> @@ -29,6 +29,7 @@
> >>>   #include <linux/of.h>
> >>>   #include <linux/phy/phy.h>
> >>>   #include <linux/platform_device.h>
> >>> +#include <linux/regulator/consumer.h>
> >>>   #include <linux/reset.h>
> >>>   #include <linux/usb.h>
> >>>   #include <linux/usb/hcd.h>
> >>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
> >>>   	struct reset_control *rsts;
> >>>   	struct phy **phys;
> >>>   	int num_phys;
> >>> +	struct regulator *vbus_supply;
> >>>   	bool reset_on_resume;
> >>>   };
> >>>   
> >>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
> >>>   	return 0;
> >>>   }
> >>>   
> >>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
> >>> +				    bool enable)
> >>> +{
> >>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
> >>> +	int ret = 0;
> >>> +
> >>> +	if (priv->vbus_supply) {
> >>> +		if (enable)
> >>> +			ret = regulator_enable(priv->vbus_supply);
> >>> +		else
> >>> +			ret = regulator_disable(priv->vbus_supply);
> >>> +		if (ret)
> >>> +			dev_err(hcd->self.controller,
> >>> +				"failed to %s vbus supply: %d\n",
> >>> +				enable ? "enable" : "disable", ret);
> >>> +	}
> >>> +	return ret;
> >>> +}
> >>> +
> >>>   static int ehci_platform_power_on(struct platform_device *dev)
> >>>   {
> >>>   	struct usb_hcd *hcd = platform_get_drvdata(dev);
> >>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
> >>>   static const struct ehci_driver_overrides platform_overrides __initconst = {
> >>>   	.reset =		ehci_platform_reset,
> >>>   	.extra_priv_size =	sizeof(struct ehci_platform_priv),
> >>> +	.port_power =		ehci_platform_port_power,
> >>>   };
> >>>   
> >>>   static struct usb_ehci_pdata ehci_platform_defaults = {
> >>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
> >>>   	if (err)
> >>>   		goto err_put_clks;
> >>>   
> >>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
> >>> +	if (IS_ERR(priv->vbus_supply)) {
> >>> +		err = PTR_ERR(priv->vbus_supply);
> >>> +		if (err == -ENODEV)
> >>> +			priv->vbus_supply = NULL;
> >>> +		else
> >>> +			goto err_reset;
> >>> +	}
> >>> +
> >>>   	if (pdata->big_endian_desc)
> >>>   		ehci->big_endian_desc = 1;
> >>>   	if (pdata->big_endian_mmio)
> >>>
> 
> 

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

Hi,

On 02/20/2018 07:10 PM, Alan Stern wrote:
> On Tue, 20 Feb 2018, Roger Quadros wrote:
> 
>> On 20/02/18 16:46, Amelie DELAUNAY wrote:
>>> Hi,
>>>
>>> On 02/20/2018 03:00 PM, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 20/02/18 14:58, Amelie Delaunay wrote:
>>>>> On some boards, especially when vbus supply requires large current,
>>>>> and the charge pump on the PHY isn't enough, an external vbus power switch
>>>>> may be used.
>>>>> Add support for this optional external vbus supply in ehci-platform.
>>>>>
>>>>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>>>>
>>>>> ---
>>>>> Changes in v2:
>>>>>    * Address Roger Quadros comments: move regulator_enable/disable from
>>>>> ehci_platform_power_on/off to ehci_platform_port_power.
>>>>> ---
>>>>>    Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>>>>    drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>>>>    2 files changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> index 3efde12..fc480cd 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> @@ -19,6 +19,7 @@ Optional properties:
>>>>>     - phys : phandle + phy specifier pair
>>>>>     - phy-names : "usb"
>>>>>     - resets : phandle + reset specifier pair
>>>>> + - vbus-supply : phandle of regulator supplying vbus
>>>>>    
>>>>
>>>> Can platforms have more than one regulator e.g. one regulator per port?
>>>>
>>>
>>> I imagine that yes, platforms could have one regulator per port.
>>> Regulator consumers bindings impose a <name>-supply property per
>>> regulator, so, what do you think about :
>>> vbus0-supply for port#0
>>> vbus1-supply for port#1
>>> ...
>>> vbusN-supply for port#N
>>>
>>> And then in probe, allocate 'struct regulator *vbus_supplies' with a
>>> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
>>> regulator *)'.
>>> And loop to get optional regulator vbus0, vbus1,..., vbusN.
>>> And then enable/disable the corresponding regulator in
>>> ehci_platform_port_power thanks to portnum.
>>
>> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
>> If there isn't a single platform needing it we could probably do without it
>> but the DT binding must be scalable to add this feature in the future.
> 
> I agree that for now there don't seem to be any platforms requiring
> more than one regulator, but this should be implemented in a way that
> could be expanded if necessary.
> 
> Anyway, the basic idea is reasonable.  I don't know to what extent
> people want to power-off their EHCI ports, but if they do then we ought
> to turn off external regulators at the same time.
> 
> Is there a real-life use case for this?
> 
> Alan Stern
> 

On my setup I have the following:

                  regulator_____vbus
  _________________             \
| EHCI controller |-port0-----[USB connector]
|_________________|-port1-----X

So, I have one regulator only for port0. But I could I have one more if 
port1 was connected. My current regulator could also supplies port1.

To allocate a vbus_supplies array depending on N_PORTS, I have to move 
this initialization from probe to ehci_platform_reset, after ehci_setup 
is done.
Then, I have to define each regulator id depending on the port number.
This imposes a binding like
- portN_vbus-supply : phandle of regulator supplying vbus for port N
But I don't know if we can describe it like this in dt-bindings ?

&ehci {
	...
	port0_vbus-supply = <&vbus_reg>;
	port1_vbus-supply = <&vbus_reg>; //Could be another regulator, or not 
specified if port is not connected.
	...
};

Is it ok to move vbus_supplies initialization in ehci_platform_reset ?

Regards,
Amelie

>> And what if it is ganged power? i.e. one regulator for more than one port.
>> Probably they all can point to the same regulator instance and it should work.
>>
>>>
>>>>>    Example (Sequoia 440EPx):
>>>>>        ehci@e0000300 {
>>>>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>>>>> index b065a96..05be100 100644
>>>>> --- a/drivers/usb/host/ehci-platform.c
>>>>> +++ b/drivers/usb/host/ehci-platform.c
>>>>> @@ -29,6 +29,7 @@
>>>>>    #include <linux/of.h>
>>>>>    #include <linux/phy/phy.h>
>>>>>    #include <linux/platform_device.h>
>>>>> +#include <linux/regulator/consumer.h>
>>>>>    #include <linux/reset.h>
>>>>>    #include <linux/usb.h>
>>>>>    #include <linux/usb/hcd.h>
>>>>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>>>>    	struct reset_control *rsts;
>>>>>    	struct phy **phys;
>>>>>    	int num_phys;
>>>>> +	struct regulator *vbus_supply;
>>>>>    	bool reset_on_resume;
>>>>>    };
>>>>>    
>>>>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>>>>    	return 0;
>>>>>    }
>>>>>    
>>>>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>>>>> +				    bool enable)
>>>>> +{
>>>>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>>>>> +	int ret = 0;
>>>>> +
>>>>> +	if (priv->vbus_supply) {
>>>>> +		if (enable)
>>>>> +			ret = regulator_enable(priv->vbus_supply);
>>>>> +		else
>>>>> +			ret = regulator_disable(priv->vbus_supply);
>>>>> +		if (ret)
>>>>> +			dev_err(hcd->self.controller,
>>>>> +				"failed to %s vbus supply: %d\n",
>>>>> +				enable ? "enable" : "disable", ret);
>>>>> +	}
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>>    static int ehci_platform_power_on(struct platform_device *dev)
>>>>>    {
>>>>>    	struct usb_hcd *hcd = platform_get_drvdata(dev);
>>>>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>>>>>    static const struct ehci_driver_overrides platform_overrides __initconst = {
>>>>>    	.reset =		ehci_platform_reset,
>>>>>    	.extra_priv_size =	sizeof(struct ehci_platform_priv),
>>>>> +	.port_power =		ehci_platform_port_power,
>>>>>    };
>>>>>    
>>>>>    static struct usb_ehci_pdata ehci_platform_defaults = {
>>>>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>>>>>    	if (err)
>>>>>    		goto err_put_clks;
>>>>>    
>>>>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
>>>>> +	if (IS_ERR(priv->vbus_supply)) {
>>>>> +		err = PTR_ERR(priv->vbus_supply);
>>>>> +		if (err == -ENODEV)
>>>>> +			priv->vbus_supply = NULL;
>>>>> +		else
>>>>> +			goto err_reset;
>>>>> +	}
>>>>> +
>>>>>    	if (pdata->big_endian_desc)
>>>>>    		ehci->big_endian_desc = 1;
>>>>>    	if (pdata->big_endian_mmio)
>>>>>
>>
>>
> 
> 
> 

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

Hi,

On 02/20/2018 07:10 PM, Alan Stern wrote:
> On Tue, 20 Feb 2018, Roger Quadros wrote:
> 
>> On 20/02/18 16:46, Amelie DELAUNAY wrote:
>>> Hi,
>>>
>>> On 02/20/2018 03:00 PM, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 20/02/18 14:58, Amelie Delaunay wrote:
>>>>> On some boards, especially when vbus supply requires large current,
>>>>> and the charge pump on the PHY isn't enough, an external vbus power switch
>>>>> may be used.
>>>>> Add support for this optional external vbus supply in ehci-platform.
>>>>>
>>>>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>>>>
>>>>> ---
>>>>> Changes in v2:
>>>>>    * Address Roger Quadros comments: move regulator_enable/disable from
>>>>> ehci_platform_power_on/off to ehci_platform_port_power.
>>>>> ---
>>>>>    Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>>>>    drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>>>>    2 files changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> index 3efde12..fc480cd 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> @@ -19,6 +19,7 @@ Optional properties:
>>>>>     - phys : phandle + phy specifier pair
>>>>>     - phy-names : "usb"
>>>>>     - resets : phandle + reset specifier pair
>>>>> + - vbus-supply : phandle of regulator supplying vbus
>>>>>    
>>>>
>>>> Can platforms have more than one regulator e.g. one regulator per port?
>>>>
>>>
>>> I imagine that yes, platforms could have one regulator per port.
>>> Regulator consumers bindings impose a <name>-supply property per
>>> regulator, so, what do you think about :
>>> vbus0-supply for port#0
>>> vbus1-supply for port#1
>>> ...
>>> vbusN-supply for port#N
>>>
>>> And then in probe, allocate 'struct regulator *vbus_supplies' with a
>>> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
>>> regulator *)'.
>>> And loop to get optional regulator vbus0, vbus1,..., vbusN.
>>> And then enable/disable the corresponding regulator in
>>> ehci_platform_port_power thanks to portnum.
>>
>> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
>> If there isn't a single platform needing it we could probably do without it
>> but the DT binding must be scalable to add this feature in the future.
> 
> I agree that for now there don't seem to be any platforms requiring
> more than one regulator, but this should be implemented in a way that
> could be expanded if necessary.
> 
> Anyway, the basic idea is reasonable.  I don't know to what extent
> people want to power-off their EHCI ports, but if they do then we ought
> to turn off external regulators at the same time.
> 
> Is there a real-life use case for this?
> 
> Alan Stern
> 

On my setup I have the following:

                  regulator_____vbus
  _________________             \
| EHCI controller |-port0-----[USB connector]
|_________________|-port1-----X

So, I have one regulator only for port0. But I could I have one more if 
port1 was connected. My current regulator could also supplies port1.

To allocate a vbus_supplies array depending on N_PORTS, I have to move 
this initialization from probe to ehci_platform_reset, after ehci_setup 
is done.
Then, I have to define each regulator id depending on the port number.
This imposes a binding like
- portN_vbus-supply : phandle of regulator supplying vbus for port N
But I don't know if we can describe it like this in dt-bindings ?

&ehci {
	...
	port0_vbus-supply = <&vbus_reg>;
	port1_vbus-supply = <&vbus_reg>; //Could be another regulator, or not 
specified if port is not connected.
	...
};

Is it ok to move vbus_supplies initialization in ehci_platform_reset ?

Regards,
Amelie

>> And what if it is ganged power? i.e. one regulator for more than one port.
>> Probably they all can point to the same regulator instance and it should work.
>>
>>>
>>>>>    Example (Sequoia 440EPx):
>>>>>        ehci@e0000300 {
>>>>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>>>>> index b065a96..05be100 100644
>>>>> --- a/drivers/usb/host/ehci-platform.c
>>>>> +++ b/drivers/usb/host/ehci-platform.c
>>>>> @@ -29,6 +29,7 @@
>>>>>    #include <linux/of.h>
>>>>>    #include <linux/phy/phy.h>
>>>>>    #include <linux/platform_device.h>
>>>>> +#include <linux/regulator/consumer.h>
>>>>>    #include <linux/reset.h>
>>>>>    #include <linux/usb.h>
>>>>>    #include <linux/usb/hcd.h>
>>>>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>>>>    	struct reset_control *rsts;
>>>>>    	struct phy **phys;
>>>>>    	int num_phys;
>>>>> +	struct regulator *vbus_supply;
>>>>>    	bool reset_on_resume;
>>>>>    };
>>>>>    
>>>>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>>>>    	return 0;
>>>>>    }
>>>>>    
>>>>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>>>>> +				    bool enable)
>>>>> +{
>>>>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>>>>> +	int ret = 0;
>>>>> +
>>>>> +	if (priv->vbus_supply) {
>>>>> +		if (enable)
>>>>> +			ret = regulator_enable(priv->vbus_supply);
>>>>> +		else
>>>>> +			ret = regulator_disable(priv->vbus_supply);
>>>>> +		if (ret)
>>>>> +			dev_err(hcd->self.controller,
>>>>> +				"failed to %s vbus supply: %d\n",
>>>>> +				enable ? "enable" : "disable", ret);
>>>>> +	}
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>>    static int ehci_platform_power_on(struct platform_device *dev)
>>>>>    {
>>>>>    	struct usb_hcd *hcd = platform_get_drvdata(dev);
>>>>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>>>>>    static const struct ehci_driver_overrides platform_overrides __initconst = {
>>>>>    	.reset =		ehci_platform_reset,
>>>>>    	.extra_priv_size =	sizeof(struct ehci_platform_priv),
>>>>> +	.port_power =		ehci_platform_port_power,
>>>>>    };
>>>>>    
>>>>>    static struct usb_ehci_pdata ehci_platform_defaults = {
>>>>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>>>>>    	if (err)
>>>>>    		goto err_put_clks;
>>>>>    
>>>>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
>>>>> +	if (IS_ERR(priv->vbus_supply)) {
>>>>> +		err = PTR_ERR(priv->vbus_supply);
>>>>> +		if (err == -ENODEV)
>>>>> +			priv->vbus_supply = NULL;
>>>>> +		else
>>>>> +			goto err_reset;
>>>>> +	}
>>>>> +
>>>>>    	if (pdata->big_endian_desc)
>>>>>    		ehci->big_endian_desc = 1;
>>>>>    	if (pdata->big_endian_mmio)
>>>>>
>>
>>
> 
> 
> 

[v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie Delaunay]

Hi,

On 02/20/2018 07:10 PM, Alan Stern wrote:
> On Tue, 20 Feb 2018, Roger Quadros wrote:
> 
>> On 20/02/18 16:46, Amelie DELAUNAY wrote:
>>> Hi,
>>>
>>> On 02/20/2018 03:00 PM, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 20/02/18 14:58, Amelie Delaunay wrote:
>>>>> On some boards, especially when vbus supply requires large current,
>>>>> and the charge pump on the PHY isn't enough, an external vbus power switch
>>>>> may be used.
>>>>> Add support for this optional external vbus supply in ehci-platform.
>>>>>
>>>>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>>>>
>>>>> ---
>>>>> Changes in v2:
>>>>>    * Address Roger Quadros comments: move regulator_enable/disable from
>>>>> ehci_platform_power_on/off to ehci_platform_port_power.
>>>>> ---
>>>>>    Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>>>>    drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>>>>    2 files changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> index 3efde12..fc480cd 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> @@ -19,6 +19,7 @@ Optional properties:
>>>>>     - phys : phandle + phy specifier pair
>>>>>     - phy-names : "usb"
>>>>>     - resets : phandle + reset specifier pair
>>>>> + - vbus-supply : phandle of regulator supplying vbus
>>>>>    
>>>>
>>>> Can platforms have more than one regulator e.g. one regulator per port?
>>>>
>>>
>>> I imagine that yes, platforms could have one regulator per port.
>>> Regulator consumers bindings impose a <name>-supply property per
>>> regulator, so, what do you think about :
>>> vbus0-supply for port#0
>>> vbus1-supply for port#1
>>> ...
>>> vbusN-supply for port#N
>>>
>>> And then in probe, allocate 'struct regulator *vbus_supplies' with a
>>> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
>>> regulator *)'.
>>> And loop to get optional regulator vbus0, vbus1,..., vbusN.
>>> And then enable/disable the corresponding regulator in
>>> ehci_platform_port_power thanks to portnum.
>>
>> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
>> If there isn't a single platform needing it we could probably do without it
>> but the DT binding must be scalable to add this feature in the future.
> 
> I agree that for now there don't seem to be any platforms requiring
> more than one regulator, but this should be implemented in a way that
> could be expanded if necessary.
> 
> Anyway, the basic idea is reasonable.  I don't know to what extent
> people want to power-off their EHCI ports, but if they do then we ought
> to turn off external regulators at the same time.
> 
> Is there a real-life use case for this?
> 
> Alan Stern
> 

On my setup I have the following:

                  regulator_____vbus
  _________________             \
| EHCI controller |-port0-----[USB connector]
|_________________|-port1-----X

So, I have one regulator only for port0. But I could I have one more if 
port1 was connected. My current regulator could also supplies port1.

To allocate a vbus_supplies array depending on N_PORTS, I have to move 
this initialization from probe to ehci_platform_reset, after ehci_setup 
is done.
Then, I have to define each regulator id depending on the port number.
This imposes a binding like
- portN_vbus-supply : phandle of regulator supplying vbus for port N
But I don't know if we can describe it like this in dt-bindings ?

&ehci {
	...
	port0_vbus-supply = <&vbus_reg>;
	port1_vbus-supply = <&vbus_reg>; //Could be another regulator, or not 
specified if port is not connected.
	...
};

Is it ok to move vbus_supplies initialization in ehci_platform_reset ?

Regards,
Amelie

>> And what if it is ganged power? i.e. one regulator for more than one port.
>> Probably they all can point to the same regulator instance and it should work.
>>
>>>
>>>>>    Example (Sequoia 440EPx):
>>>>>        ehci@e0000300 {
>>>>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>>>>> index b065a96..05be100 100644
>>>>> --- a/drivers/usb/host/ehci-platform.c
>>>>> +++ b/drivers/usb/host/ehci-platform.c
>>>>> @@ -29,6 +29,7 @@
>>>>>    #include <linux/of.h>
>>>>>    #include <linux/phy/phy.h>
>>>>>    #include <linux/platform_device.h>
>>>>> +#include <linux/regulator/consumer.h>
>>>>>    #include <linux/reset.h>
>>>>>    #include <linux/usb.h>
>>>>>    #include <linux/usb/hcd.h>
>>>>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>>>>    	struct reset_control *rsts;
>>>>>    	struct phy **phys;
>>>>>    	int num_phys;
>>>>> +	struct regulator *vbus_supply;
>>>>>    	bool reset_on_resume;
>>>>>    };
>>>>>    
>>>>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>>>>    	return 0;
>>>>>    }
>>>>>    
>>>>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>>>>> +				    bool enable)
>>>>> +{
>>>>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>>>>> +	int ret = 0;
>>>>> +
>>>>> +	if (priv->vbus_supply) {
>>>>> +		if (enable)
>>>>> +			ret = regulator_enable(priv->vbus_supply);
>>>>> +		else
>>>>> +			ret = regulator_disable(priv->vbus_supply);
>>>>> +		if (ret)
>>>>> +			dev_err(hcd->self.controller,
>>>>> +				"failed to %s vbus supply: %d\n",
>>>>> +				enable ? "enable" : "disable", ret);
>>>>> +	}
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>>    static int ehci_platform_power_on(struct platform_device *dev)
>>>>>    {
>>>>>    	struct usb_hcd *hcd = platform_get_drvdata(dev);
>>>>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>>>>>    static const struct ehci_driver_overrides platform_overrides __initconst = {
>>>>>    	.reset =		ehci_platform_reset,
>>>>>    	.extra_priv_size =	sizeof(struct ehci_platform_priv),
>>>>> +	.port_power =		ehci_platform_port_power,
>>>>>    };
>>>>>    
>>>>>    static struct usb_ehci_pdata ehci_platform_defaults = {
>>>>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>>>>>    	if (err)
>>>>>    		goto err_put_clks;
>>>>>    
>>>>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
>>>>> +	if (IS_ERR(priv->vbus_supply)) {
>>>>> +		err = PTR_ERR(priv->vbus_supply);
>>>>> +		if (err == -ENODEV)
>>>>> +			priv->vbus_supply = NULL;
>>>>> +		else
>>>>> +			goto err_reset;
>>>>> +	}
>>>>> +
>>>>>    	if (pdata->big_endian_desc)
>>>>>    		ehci->big_endian_desc = 1;
>>>>>    	if (pdata->big_endian_mmio)
>>>>>
>>
>>
> 
> 
>

[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

Hi,

On 02/20/2018 07:10 PM, Alan Stern wrote:
> On Tue, 20 Feb 2018, Roger Quadros wrote:
> 
>> On 20/02/18 16:46, Amelie DELAUNAY wrote:
>>> Hi,
>>>
>>> On 02/20/2018 03:00 PM, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 20/02/18 14:58, Amelie Delaunay wrote:
>>>>> On some boards, especially when vbus supply requires large current,
>>>>> and the charge pump on the PHY isn't enough, an external vbus power switch
>>>>> may be used.
>>>>> Add support for this optional external vbus supply in ehci-platform.
>>>>>
>>>>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>>>>
>>>>> ---
>>>>> Changes in v2:
>>>>>    * Address Roger Quadros comments: move regulator_enable/disable from
>>>>> ehci_platform_power_on/off to ehci_platform_port_power.
>>>>> ---
>>>>>    Documentation/devicetree/bindings/usb/usb-ehci.txt |  1 +
>>>>>    drivers/usb/host/ehci-platform.c                   | 31 ++++++++++++++++++++++
>>>>>    2 files changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> index 3efde12..fc480cd 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>> @@ -19,6 +19,7 @@ Optional properties:
>>>>>     - phys : phandle + phy specifier pair
>>>>>     - phy-names : "usb"
>>>>>     - resets : phandle + reset specifier pair
>>>>> + - vbus-supply : phandle of regulator supplying vbus
>>>>>    
>>>>
>>>> Can platforms have more than one regulator e.g. one regulator per port?
>>>>
>>>
>>> I imagine that yes, platforms could have one regulator per port.
>>> Regulator consumers bindings impose a <name>-supply property per
>>> regulator, so, what do you think about :
>>> vbus0-supply for port#0
>>> vbus1-supply for port#1
>>> ...
>>> vbusN-supply for port#N
>>>
>>> And then in probe, allocate 'struct regulator *vbus_supplies' with a
>>> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
>>> regulator *)'.
>>> And loop to get optional regulator vbus0, vbus1,..., vbusN.
>>> And then enable/disable the corresponding regulator in
>>> ehci_platform_port_power thanks to portnum.
>>
>> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
>> If there isn't a single platform needing it we could probably do without it
>> but the DT binding must be scalable to add this feature in the future.
> 
> I agree that for now there don't seem to be any platforms requiring
> more than one regulator, but this should be implemented in a way that
> could be expanded if necessary.
> 
> Anyway, the basic idea is reasonable.  I don't know to what extent
> people want to power-off their EHCI ports, but if they do then we ought
> to turn off external regulators at the same time.
> 
> Is there a real-life use case for this?
> 
> Alan Stern
> 

On my setup I have the following:

                  regulator_____vbus
  _________________             \
| EHCI controller |-port0-----[USB connector]
|_________________|-port1-----X

So, I have one regulator only for port0. But I could I have one more if 
port1 was connected. My current regulator could also supplies port1.

To allocate a vbus_supplies array depending on N_PORTS, I have to move 
this initialization from probe to ehci_platform_reset, after ehci_setup 
is done.
Then, I have to define each regulator id depending on the port number.
This imposes a binding like
- portN_vbus-supply : phandle of regulator supplying vbus for port N
But I don't know if we can describe it like this in dt-bindings ?

&ehci {
	...
	port0_vbus-supply = <&vbus_reg>;
	port1_vbus-supply = <&vbus_reg>; //Could be another regulator, or not 
specified if port is not connected.
	...
};

Is it ok to move vbus_supplies initialization in ehci_platform_reset ?

Regards,
Amelie

>> And what if it is ganged power? i.e. one regulator for more than one port.
>> Probably they all can point to the same regulator instance and it should work.
>>
>>>
>>>>>    Example (Sequoia 440EPx):
>>>>>        ehci at e0000300 {
>>>>> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
>>>>> index b065a96..05be100 100644
>>>>> --- a/drivers/usb/host/ehci-platform.c
>>>>> +++ b/drivers/usb/host/ehci-platform.c
>>>>> @@ -29,6 +29,7 @@
>>>>>    #include <linux/of.h>
>>>>>    #include <linux/phy/phy.h>
>>>>>    #include <linux/platform_device.h>
>>>>> +#include <linux/regulator/consumer.h>
>>>>>    #include <linux/reset.h>
>>>>>    #include <linux/usb.h>
>>>>>    #include <linux/usb/hcd.h>
>>>>> @@ -46,6 +47,7 @@ struct ehci_platform_priv {
>>>>>    	struct reset_control *rsts;
>>>>>    	struct phy **phys;
>>>>>    	int num_phys;
>>>>> +	struct regulator *vbus_supply;
>>>>>    	bool reset_on_resume;
>>>>>    };
>>>>>    
>>>>> @@ -76,6 +78,25 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>>>>>    	return 0;
>>>>>    }
>>>>>    
>>>>> +static int ehci_platform_port_power(struct usb_hcd *hcd, int portnum,
>>>>> +				    bool enable)
>>>>> +{
>>>>> +	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
>>>>> +	int ret = 0;
>>>>> +
>>>>> +	if (priv->vbus_supply) {
>>>>> +		if (enable)
>>>>> +			ret = regulator_enable(priv->vbus_supply);
>>>>> +		else
>>>>> +			ret = regulator_disable(priv->vbus_supply);
>>>>> +		if (ret)
>>>>> +			dev_err(hcd->self.controller,
>>>>> +				"failed to %s vbus supply: %d\n",
>>>>> +				enable ? "enable" : "disable", ret);
>>>>> +	}
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>>    static int ehci_platform_power_on(struct platform_device *dev)
>>>>>    {
>>>>>    	struct usb_hcd *hcd = platform_get_drvdata(dev);
>>>>> @@ -134,6 +155,7 @@ static struct hc_driver __read_mostly ehci_platform_hc_driver;
>>>>>    static const struct ehci_driver_overrides platform_overrides __initconst = {
>>>>>    	.reset =		ehci_platform_reset,
>>>>>    	.extra_priv_size =	sizeof(struct ehci_platform_priv),
>>>>> +	.port_power =		ehci_platform_port_power,
>>>>>    };
>>>>>    
>>>>>    static struct usb_ehci_pdata ehci_platform_defaults = {
>>>>> @@ -247,6 +269,15 @@ static int ehci_platform_probe(struct platform_device *dev)
>>>>>    	if (err)
>>>>>    		goto err_put_clks;
>>>>>    
>>>>> +	priv->vbus_supply = devm_regulator_get_optional(&dev->dev, "vbus");
>>>>> +	if (IS_ERR(priv->vbus_supply)) {
>>>>> +		err = PTR_ERR(priv->vbus_supply);
>>>>> +		if (err == -ENODEV)
>>>>> +			priv->vbus_supply = NULL;
>>>>> +		else
>>>>> +			goto err_reset;
>>>>> +	}
>>>>> +
>>>>>    	if (pdata->big_endian_desc)
>>>>>    		ehci->big_endian_desc = 1;
>>>>>    	if (pdata->big_endian_mmio)
>>>>>
>>
>>
> 
> 
> 

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Alan Stern]

On Thu, 22 Feb 2018, Amelie DELAUNAY wrote:

> >>>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>>>> @@ -19,6 +19,7 @@ Optional properties:
> >>>>>     - phys : phandle + phy specifier pair
> >>>>>     - phy-names : "usb"
> >>>>>     - resets : phandle + reset specifier pair
> >>>>> + - vbus-supply : phandle of regulator supplying vbus
> >>>>>    
> >>>>
> >>>> Can platforms have more than one regulator e.g. one regulator per port?
> >>>>
> >>>
> >>> I imagine that yes, platforms could have one regulator per port.
> >>> Regulator consumers bindings impose a <name>-supply property per
> >>> regulator, so, what do you think about :
> >>> vbus0-supply for port#0
> >>> vbus1-supply for port#1
> >>> ...
> >>> vbusN-supply for port#N
> >>>
> >>> And then in probe, allocate 'struct regulator *vbus_supplies' with a
> >>> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
> >>> regulator *)'.
> >>> And loop to get optional regulator vbus0, vbus1,..., vbusN.
> >>> And then enable/disable the corresponding regulator in
> >>> ehci_platform_port_power thanks to portnum.
> >>
> >> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
> >> If there isn't a single platform needing it we could probably do without it
> >> but the DT binding must be scalable to add this feature in the future.
> > 
> > I agree that for now there don't seem to be any platforms requiring
> > more than one regulator, but this should be implemented in a way that
> > could be expanded if necessary.
> > 
> > Anyway, the basic idea is reasonable.  I don't know to what extent
> > people want to power-off their EHCI ports, but if they do then we ought
> > to turn off external regulators at the same time.
> > 
> > Is there a real-life use case for this?
> > 
> > Alan Stern
> > 
> 
> On my setup I have the following:
> 
>                   regulator_____vbus
>   _________________             \
> | EHCI controller |-port0-----[USB connector]
> |_________________|-port1-----X
> 
> So, I have one regulator only for port0. But I could I have one more if 
> port1 was connected. My current regulator could also supplies port1.
> 
> To allocate a vbus_supplies array depending on N_PORTS, I have to move 
> this initialization from probe to ehci_platform_reset, after ehci_setup 
> is done.
> Then, I have to define each regulator id depending on the port number.
> This imposes a binding like
> - portN_vbus-supply : phandle of regulator supplying vbus for port N
> But I don't know if we can describe it like this in dt-bindings ?
> 
> &ehci {
> 	...
> 	port0_vbus-supply = <&vbus_reg>;
> 	port1_vbus-supply = <&vbus_reg>; //Could be another regulator, or not 
> specified if port is not connected.
> 	...
> };
> 
> Is it ok to move vbus_supplies initialization in ehci_platform_reset ?

Yes, it's okay to move the code if you need to.

However, I can not speak on the DT implications.  Someone who knows
more about it should chime in.

Alan Stern

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Alan Stern]

On Thu, 22 Feb 2018, Amelie DELAUNAY wrote:

> >>>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>>>> @@ -19,6 +19,7 @@ Optional properties:
> >>>>>     - phys : phandle + phy specifier pair
> >>>>>     - phy-names : "usb"
> >>>>>     - resets : phandle + reset specifier pair
> >>>>> + - vbus-supply : phandle of regulator supplying vbus
> >>>>>    
> >>>>
> >>>> Can platforms have more than one regulator e.g. one regulator per port?
> >>>>
> >>>
> >>> I imagine that yes, platforms could have one regulator per port.
> >>> Regulator consumers bindings impose a <name>-supply property per
> >>> regulator, so, what do you think about :
> >>> vbus0-supply for port#0
> >>> vbus1-supply for port#1
> >>> ...
> >>> vbusN-supply for port#N
> >>>
> >>> And then in probe, allocate 'struct regulator *vbus_supplies' with a
> >>> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
> >>> regulator *)'.
> >>> And loop to get optional regulator vbus0, vbus1,..., vbusN.
> >>> And then enable/disable the corresponding regulator in
> >>> ehci_platform_port_power thanks to portnum.
> >>
> >> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
> >> If there isn't a single platform needing it we could probably do without it
> >> but the DT binding must be scalable to add this feature in the future.
> > 
> > I agree that for now there don't seem to be any platforms requiring
> > more than one regulator, but this should be implemented in a way that
> > could be expanded if necessary.
> > 
> > Anyway, the basic idea is reasonable.  I don't know to what extent
> > people want to power-off their EHCI ports, but if they do then we ought
> > to turn off external regulators at the same time.
> > 
> > Is there a real-life use case for this?
> > 
> > Alan Stern
> > 
> 
> On my setup I have the following:
> 
>                   regulator_____vbus
>   _________________             \
> | EHCI controller |-port0-----[USB connector]
> |_________________|-port1-----X
> 
> So, I have one regulator only for port0. But I could I have one more if 
> port1 was connected. My current regulator could also supplies port1.
> 
> To allocate a vbus_supplies array depending on N_PORTS, I have to move 
> this initialization from probe to ehci_platform_reset, after ehci_setup 
> is done.
> Then, I have to define each regulator id depending on the port number.
> This imposes a binding like
> - portN_vbus-supply : phandle of regulator supplying vbus for port N
> But I don't know if we can describe it like this in dt-bindings ?
> 
> &ehci {
> 	...
> 	port0_vbus-supply = <&vbus_reg>;
> 	port1_vbus-supply = <&vbus_reg>; //Could be another regulator, or not 
> specified if port is not connected.
> 	...
> };
> 
> Is it ok to move vbus_supplies initialization in ehci_platform_reset ?

Yes, it's okay to move the code if you need to.

However, I can not speak on the DT implications.  Someone who knows
more about it should chime in.

Alan Stern

[v2] usb: host: ehci-platform: add support for optional external vbus supply

[Alan Stern]

On Thu, 22 Feb 2018, Amelie DELAUNAY wrote:

> >>>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>>>> @@ -19,6 +19,7 @@ Optional properties:
> >>>>>     - phys : phandle + phy specifier pair
> >>>>>     - phy-names : "usb"
> >>>>>     - resets : phandle + reset specifier pair
> >>>>> + - vbus-supply : phandle of regulator supplying vbus
> >>>>>    
> >>>>
> >>>> Can platforms have more than one regulator e.g. one regulator per port?
> >>>>
> >>>
> >>> I imagine that yes, platforms could have one regulator per port.
> >>> Regulator consumers bindings impose a <name>-supply property per
> >>> regulator, so, what do you think about :
> >>> vbus0-supply for port#0
> >>> vbus1-supply for port#1
> >>> ...
> >>> vbusN-supply for port#N
> >>>
> >>> And then in probe, allocate 'struct regulator *vbus_supplies' with a
> >>> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
> >>> regulator *)'.
> >>> And loop to get optional regulator vbus0, vbus1,..., vbusN.
> >>> And then enable/disable the corresponding regulator in
> >>> ehci_platform_port_power thanks to portnum.
> >>
> >> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
> >> If there isn't a single platform needing it we could probably do without it
> >> but the DT binding must be scalable to add this feature in the future.
> > 
> > I agree that for now there don't seem to be any platforms requiring
> > more than one regulator, but this should be implemented in a way that
> > could be expanded if necessary.
> > 
> > Anyway, the basic idea is reasonable.  I don't know to what extent
> > people want to power-off their EHCI ports, but if they do then we ought
> > to turn off external regulators at the same time.
> > 
> > Is there a real-life use case for this?
> > 
> > Alan Stern
> > 
> 
> On my setup I have the following:
> 
>                   regulator_____vbus
>   _________________             \
> | EHCI controller |-port0-----[USB connector]
> |_________________|-port1-----X
> 
> So, I have one regulator only for port0. But I could I have one more if 
> port1 was connected. My current regulator could also supplies port1.
> 
> To allocate a vbus_supplies array depending on N_PORTS, I have to move 
> this initialization from probe to ehci_platform_reset, after ehci_setup 
> is done.
> Then, I have to define each regulator id depending on the port number.
> This imposes a binding like
> - portN_vbus-supply : phandle of regulator supplying vbus for port N
> But I don't know if we can describe it like this in dt-bindings ?
> 
> &ehci {
> 	...
> 	port0_vbus-supply = <&vbus_reg>;
> 	port1_vbus-supply = <&vbus_reg>; //Could be another regulator, or not 
> specified if port is not connected.
> 	...
> };
> 
> Is it ok to move vbus_supplies initialization in ehci_platform_reset ?

Yes, it's okay to move the code if you need to.

However, I can not speak on the DT implications.  Someone who knows
more about it should chime in.

Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Alan Stern]

On Thu, 22 Feb 2018, Amelie DELAUNAY wrote:

> >>>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> >>>>> @@ -19,6 +19,7 @@ Optional properties:
> >>>>>     - phys : phandle + phy specifier pair
> >>>>>     - phy-names : "usb"
> >>>>>     - resets : phandle + reset specifier pair
> >>>>> + - vbus-supply : phandle of regulator supplying vbus
> >>>>>    
> >>>>
> >>>> Can platforms have more than one regulator e.g. one regulator per port?
> >>>>
> >>>
> >>> I imagine that yes, platforms could have one regulator per port.
> >>> Regulator consumers bindings impose a <name>-supply property per
> >>> regulator, so, what do you think about :
> >>> vbus0-supply for port#0
> >>> vbus1-supply for port#1
> >>> ...
> >>> vbusN-supply for port#N
> >>>
> >>> And then in probe, allocate 'struct regulator *vbus_supplies' with a
> >>> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
> >>> regulator *)'.
> >>> And loop to get optional regulator vbus0, vbus1,..., vbusN.
> >>> And then enable/disable the corresponding regulator in
> >>> ehci_platform_port_power thanks to portnum.
> >>
> >> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
> >> If there isn't a single platform needing it we could probably do without it
> >> but the DT binding must be scalable to add this feature in the future.
> > 
> > I agree that for now there don't seem to be any platforms requiring
> > more than one regulator, but this should be implemented in a way that
> > could be expanded if necessary.
> > 
> > Anyway, the basic idea is reasonable.  I don't know to what extent
> > people want to power-off their EHCI ports, but if they do then we ought
> > to turn off external regulators at the same time.
> > 
> > Is there a real-life use case for this?
> > 
> > Alan Stern
> > 
> 
> On my setup I have the following:
> 
>                   regulator_____vbus
>   _________________             \
> | EHCI controller |-port0-----[USB connector]
> |_________________|-port1-----X
> 
> So, I have one regulator only for port0. But I could I have one more if 
> port1 was connected. My current regulator could also supplies port1.
> 
> To allocate a vbus_supplies array depending on N_PORTS, I have to move 
> this initialization from probe to ehci_platform_reset, after ehci_setup 
> is done.
> Then, I have to define each regulator id depending on the port number.
> This imposes a binding like
> - portN_vbus-supply : phandle of regulator supplying vbus for port N
> But I don't know if we can describe it like this in dt-bindings ?
> 
> &ehci {
> 	...
> 	port0_vbus-supply = <&vbus_reg>;
> 	port1_vbus-supply = <&vbus_reg>; //Could be another regulator, or not 
> specified if port is not connected.
> 	...
> };
> 
> Is it ok to move vbus_supplies initialization in ehci_platform_reset ?

Yes, it's okay to move the code if you need to.

However, I can not speak on the DT implications.  Someone who knows
more about it should chime in.

Alan Stern

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

On 02/22/2018 07:27 PM, Alan Stern wrote:
> On Thu, 22 Feb 2018, Amelie DELAUNAY wrote:
> 
>>>>>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>>>> @@ -19,6 +19,7 @@ Optional properties:
>>>>>>>      - phys : phandle + phy specifier pair
>>>>>>>      - phy-names : "usb"
>>>>>>>      - resets : phandle + reset specifier pair
>>>>>>> + - vbus-supply : phandle of regulator supplying vbus
>>>>>>>     
>>>>>>
>>>>>> Can platforms have more than one regulator e.g. one regulator per port?
>>>>>>
>>>>>
>>>>> I imagine that yes, platforms could have one regulator per port.
>>>>> Regulator consumers bindings impose a <name>-supply property per
>>>>> regulator, so, what do you think about :
>>>>> vbus0-supply for port#0
>>>>> vbus1-supply for port#1
>>>>> ...
>>>>> vbusN-supply for port#N
>>>>>
>>>>> And then in probe, allocate 'struct regulator *vbus_supplies' with a
>>>>> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
>>>>> regulator *)'.
>>>>> And loop to get optional regulator vbus0, vbus1,..., vbusN.
>>>>> And then enable/disable the corresponding regulator in
>>>>> ehci_platform_port_power thanks to portnum.
>>>>
>>>> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
>>>> If there isn't a single platform needing it we could probably do without it
>>>> but the DT binding must be scalable to add this feature in the future.
>>>
>>> I agree that for now there don't seem to be any platforms requiring
>>> more than one regulator, but this should be implemented in a way that
>>> could be expanded if necessary.
>>>
>>> Anyway, the basic idea is reasonable.  I don't know to what extent
>>> people want to power-off their EHCI ports, but if they do then we ought
>>> to turn off external regulators at the same time.
>>>
>>> Is there a real-life use case for this?
>>>
>>> Alan Stern
>>>
>>
>> On my setup I have the following:
>>
>>                    regulator_____vbus
>>    _________________             \
>> | EHCI controller |-port0-----[USB connector]
>> |_________________|-port1-----X
>>
>> So, I have one regulator only for port0. But I could I have one more if
>> port1 was connected. My current regulator could also supplies port1.
>>
>> To allocate a vbus_supplies array depending on N_PORTS, I have to move
>> this initialization from probe to ehci_platform_reset, after ehci_setup
>> is done.
>> Then, I have to define each regulator id depending on the port number.
>> This imposes a binding like
>> - portN_vbus-supply : phandle of regulator supplying vbus for port N
>> But I don't know if we can describe it like this in dt-bindings ?
>>
>> &ehci {
>> 	...
>> 	port0_vbus-supply = <&vbus_reg>;
>> 	port1_vbus-supply = <&vbus_reg>; //Could be another regulator, or not
>> specified if port is not connected.
>> 	...
>> };
>>
>> Is it ok to move vbus_supplies initialization in ehci_platform_reset ?
> 
> Yes, it's okay to move the code if you need to.
> 
> However, I can not speak on the DT implications.  Someone who knows
> more about it should chime in.
> 
> Alan Stern
> 

Thanks Alan,

I'm going to send a v3 including all these changes to ease review.

Regards,
Amelie

Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

On 02/22/2018 07:27 PM, Alan Stern wrote:
> On Thu, 22 Feb 2018, Amelie DELAUNAY wrote:
> 
>>>>>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>>>> @@ -19,6 +19,7 @@ Optional properties:
>>>>>>>      - phys : phandle + phy specifier pair
>>>>>>>      - phy-names : "usb"
>>>>>>>      - resets : phandle + reset specifier pair
>>>>>>> + - vbus-supply : phandle of regulator supplying vbus
>>>>>>>     
>>>>>>
>>>>>> Can platforms have more than one regulator e.g. one regulator per port?
>>>>>>
>>>>>
>>>>> I imagine that yes, platforms could have one regulator per port.
>>>>> Regulator consumers bindings impose a <name>-supply property per
>>>>> regulator, so, what do you think about :
>>>>> vbus0-supply for port#0
>>>>> vbus1-supply for port#1
>>>>> ...
>>>>> vbusN-supply for port#N
>>>>>
>>>>> And then in probe, allocate 'struct regulator *vbus_supplies' with a
>>>>> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
>>>>> regulator *)'.
>>>>> And loop to get optional regulator vbus0, vbus1,..., vbusN.
>>>>> And then enable/disable the corresponding regulator in
>>>>> ehci_platform_port_power thanks to portnum.
>>>>
>>>> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
>>>> If there isn't a single platform needing it we could probably do without it
>>>> but the DT binding must be scalable to add this feature in the future.
>>>
>>> I agree that for now there don't seem to be any platforms requiring
>>> more than one regulator, but this should be implemented in a way that
>>> could be expanded if necessary.
>>>
>>> Anyway, the basic idea is reasonable.  I don't know to what extent
>>> people want to power-off their EHCI ports, but if they do then we ought
>>> to turn off external regulators at the same time.
>>>
>>> Is there a real-life use case for this?
>>>
>>> Alan Stern
>>>
>>
>> On my setup I have the following:
>>
>>                    regulator_____vbus
>>    _________________             \
>> | EHCI controller |-port0-----[USB connector]
>> |_________________|-port1-----X
>>
>> So, I have one regulator only for port0. But I could I have one more if
>> port1 was connected. My current regulator could also supplies port1.
>>
>> To allocate a vbus_supplies array depending on N_PORTS, I have to move
>> this initialization from probe to ehci_platform_reset, after ehci_setup
>> is done.
>> Then, I have to define each regulator id depending on the port number.
>> This imposes a binding like
>> - portN_vbus-supply : phandle of regulator supplying vbus for port N
>> But I don't know if we can describe it like this in dt-bindings ?
>>
>> &ehci {
>> 	...
>> 	port0_vbus-supply = <&vbus_reg>;
>> 	port1_vbus-supply = <&vbus_reg>; //Could be another regulator, or not
>> specified if port is not connected.
>> 	...
>> };
>>
>> Is it ok to move vbus_supplies initialization in ehci_platform_reset ?
> 
> Yes, it's okay to move the code if you need to.
> 
> However, I can not speak on the DT implications.  Someone who knows
> more about it should chime in.
> 
> Alan Stern
> 

Thanks Alan,

I'm going to send a v3 including all these changes to ease review.

Regards,
Amelie

[v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie Delaunay]

DQoNCk9uIDAyLzIyLzIwMTggMDc6MjcgUE0sIEFsYW4gU3Rlcm4gd3JvdGU6DQo+IE9uIFRodSwg
MjIgRmViIDIwMTgsIEFtZWxpZSBERUxBVU5BWSB3cm90ZToNCj4gDQo+Pj4+Pj4+IC0tLSBhL0Rv
Y3VtZW50YXRpb24vZGV2aWNldHJlZS9iaW5kaW5ncy91c2IvdXNiLWVoY2kudHh0DQo+Pj4+Pj4+
ICsrKyBiL0RvY3VtZW50YXRpb24vZGV2aWNldHJlZS9iaW5kaW5ncy91c2IvdXNiLWVoY2kudHh0
DQo+Pj4+Pj4+IEBAIC0xOSw2ICsxOSw3IEBAIE9wdGlvbmFsIHByb3BlcnRpZXM6DQo+Pj4+Pj4+
ICAgICAgLSBwaHlzIDogcGhhbmRsZSArIHBoeSBzcGVjaWZpZXIgcGFpcg0KPj4+Pj4+PiAgICAg
IC0gcGh5LW5hbWVzIDogInVzYiINCj4+Pj4+Pj4gICAgICAtIHJlc2V0cyA6IHBoYW5kbGUgKyBy
ZXNldCBzcGVjaWZpZXIgcGFpcg0KPj4+Pj4+PiArIC0gdmJ1cy1zdXBwbHkgOiBwaGFuZGxlIG9m
IHJlZ3VsYXRvciBzdXBwbHlpbmcgdmJ1cw0KPj4+Pj4+PiAgICAgDQo+Pj4+Pj4NCj4+Pj4+PiBD
YW4gcGxhdGZvcm1zIGhhdmUgbW9yZSB0aGFuIG9uZSByZWd1bGF0b3IgZS5nLiBvbmUgcmVndWxh
dG9yIHBlciBwb3J0Pw0KPj4+Pj4+DQo+Pj4+Pg0KPj4+Pj4gSSBpbWFnaW5lIHRoYXQgeWVzLCBw
bGF0Zm9ybXMgY291bGQgaGF2ZSBvbmUgcmVndWxhdG9yIHBlciBwb3J0Lg0KPj4+Pj4gUmVndWxh
dG9yIGNvbnN1bWVycyBiaW5kaW5ncyBpbXBvc2UgYSA8bmFtZT4tc3VwcGx5IHByb3BlcnR5IHBl
cg0KPj4+Pj4gcmVndWxhdG9yLCBzbywgd2hhdCBkbyB5b3UgdGhpbmsgYWJvdXQgOg0KPj4+Pj4g
dmJ1czAtc3VwcGx5IGZvciBwb3J0IzANCj4+Pj4+IHZidXMxLXN1cHBseSBmb3IgcG9ydCMxDQo+
Pj4+PiAuLi4NCj4+Pj4+IHZidXNOLXN1cHBseSBmb3IgcG9ydCNODQo+Pj4+Pg0KPj4+Pj4gQW5k
IHRoZW4gaW4gcHJvYmUsIGFsbG9jYXRlICdzdHJ1Y3QgcmVndWxhdG9yICp2YnVzX3N1cHBsaWVz
JyB3aXRoIGENCj4+Pj4+IHNpemUgY29ycmVzcG9uZGluZyB0byAnSENTX05fUE9SVFMoZWhjaS0+
aGNzX3BhcmFtcykgKiBzaXplb2Yoc3RydWN0DQo+Pj4+PiByZWd1bGF0b3IgKiknLg0KPj4+Pj4g
QW5kIGxvb3AgdG8gZ2V0IG9wdGlvbmFsIHJlZ3VsYXRvciB2YnVzMCwgdmJ1czEsLi4uLCB2YnVz
Ti4NCj4+Pj4+IEFuZCB0aGVuIGVuYWJsZS9kaXNhYmxlIHRoZSBjb3JyZXNwb25kaW5nIHJlZ3Vs
YXRvciBpbg0KPj4+Pj4gZWhjaV9wbGF0Zm9ybV9wb3J0X3Bvd2VyIHRoYW5rcyB0byBwb3J0bnVt
Lg0KPj4+Pg0KPj4+PiBMb29rcyBmaW5lIHRvIG1lIGJ1dCB3ZSBuZWVkIHRvIGdldCBBbGFuJ3Mg
b3BpbmlvbiBpZiB0aGlzIGlzIHdvcnRoIHRoZSBlZmZvcnQuDQo+Pj4+IElmIHRoZXJlIGlzbid0
IGEgc2luZ2xlIHBsYXRmb3JtIG5lZWRpbmcgaXQgd2UgY291bGQgcHJvYmFibHkgZG8gd2l0aG91
dCBpdA0KPj4+PiBidXQgdGhlIERUIGJpbmRpbmcgbXVzdCBiZSBzY2FsYWJsZSB0byBhZGQgdGhp
cyBmZWF0dXJlIGluIHRoZSBmdXR1cmUuDQo+Pj4NCj4+PiBJIGFncmVlIHRoYXQgZm9yIG5vdyB0
aGVyZSBkb24ndCBzZWVtIHRvIGJlIGFueSBwbGF0Zm9ybXMgcmVxdWlyaW5nDQo+Pj4gbW9yZSB0
aGFuIG9uZSByZWd1bGF0b3IsIGJ1dCB0aGlzIHNob3VsZCBiZSBpbXBsZW1lbnRlZCBpbiBhIHdh
eSB0aGF0DQo+Pj4gY291bGQgYmUgZXhwYW5kZWQgaWYgbmVjZXNzYXJ5Lg0KPj4+DQo+Pj4gQW55
d2F5LCB0aGUgYmFzaWMgaWRlYSBpcyByZWFzb25hYmxlLiAgSSBkb24ndCBrbm93IHRvIHdoYXQg
ZXh0ZW50DQo+Pj4gcGVvcGxlIHdhbnQgdG8gcG93ZXItb2ZmIHRoZWlyIEVIQ0kgcG9ydHMsIGJ1
dCBpZiB0aGV5IGRvIHRoZW4gd2Ugb3VnaHQNCj4+PiB0byB0dXJuIG9mZiBleHRlcm5hbCByZWd1
bGF0b3JzIGF0IHRoZSBzYW1lIHRpbWUuDQo+Pj4NCj4+PiBJcyB0aGVyZSBhIHJlYWwtbGlmZSB1
c2UgY2FzZSBmb3IgdGhpcz8NCj4+Pg0KPj4+IEFsYW4gU3Rlcm4NCj4+Pg0KPj4NCj4+IE9uIG15
IHNldHVwIEkgaGF2ZSB0aGUgZm9sbG93aW5nOg0KPj4NCj4+ICAgICAgICAgICAgICAgICAgICBy
ZWd1bGF0b3JfX19fX3ZidXMNCj4+ICAgIF9fX19fX19fX19fX19fX19fICAgICAgICAgICAgIFwN
Cj4+IHwgRUhDSSBjb250cm9sbGVyIHwtcG9ydDAtLS0tLVtVU0IgY29ubmVjdG9yXQ0KPj4gfF9f
X19fX19fX19fX19fX19ffC1wb3J0MS0tLS0tWA0KPj4NCj4+IFNvLCBJIGhhdmUgb25lIHJlZ3Vs
YXRvciBvbmx5IGZvciBwb3J0MC4gQnV0IEkgY291bGQgSSBoYXZlIG9uZSBtb3JlIGlmDQo+PiBw
b3J0MSB3YXMgY29ubmVjdGVkLiBNeSBjdXJyZW50IHJlZ3VsYXRvciBjb3VsZCBhbHNvIHN1cHBs
aWVzIHBvcnQxLg0KPj4NCj4+IFRvIGFsbG9jYXRlIGEgdmJ1c19zdXBwbGllcyBhcnJheSBkZXBl
bmRpbmcgb24gTl9QT1JUUywgSSBoYXZlIHRvIG1vdmUNCj4+IHRoaXMgaW5pdGlhbGl6YXRpb24g
ZnJvbSBwcm9iZSB0byBlaGNpX3BsYXRmb3JtX3Jlc2V0LCBhZnRlciBlaGNpX3NldHVwDQo+PiBp
cyBkb25lLg0KPj4gVGhlbiwgSSBoYXZlIHRvIGRlZmluZSBlYWNoIHJlZ3VsYXRvciBpZCBkZXBl
bmRpbmcgb24gdGhlIHBvcnQgbnVtYmVyLg0KPj4gVGhpcyBpbXBvc2VzIGEgYmluZGluZyBsaWtl
DQo+PiAtIHBvcnROX3ZidXMtc3VwcGx5IDogcGhhbmRsZSBvZiByZWd1bGF0b3Igc3VwcGx5aW5n
IHZidXMgZm9yIHBvcnQgTg0KPj4gQnV0IEkgZG9uJ3Qga25vdyBpZiB3ZSBjYW4gZGVzY3JpYmUg
aXQgbGlrZSB0aGlzIGluIGR0LWJpbmRpbmdzID8NCj4+DQo+PiAmZWhjaSB7DQo+PiAJLi4uDQo+
PiAJcG9ydDBfdmJ1cy1zdXBwbHkgPSA8JnZidXNfcmVnPjsNCj4+IAlwb3J0MV92YnVzLXN1cHBs
eSA9IDwmdmJ1c19yZWc+OyAvL0NvdWxkIGJlIGFub3RoZXIgcmVndWxhdG9yLCBvciBub3QNCj4+
IHNwZWNpZmllZCBpZiBwb3J0IGlzIG5vdCBjb25uZWN0ZWQuDQo+PiAJLi4uDQo+PiB9Ow0KPj4N
Cj4+IElzIGl0IG9rIHRvIG1vdmUgdmJ1c19zdXBwbGllcyBpbml0aWFsaXphdGlvbiBpbiBlaGNp
X3BsYXRmb3JtX3Jlc2V0ID8NCj4gDQo+IFllcywgaXQncyBva2F5IHRvIG1vdmUgdGhlIGNvZGUg
aWYgeW91IG5lZWQgdG8uDQo+IA0KPiBIb3dldmVyLCBJIGNhbiBub3Qgc3BlYWsgb24gdGhlIERU
IGltcGxpY2F0aW9ucy4gIFNvbWVvbmUgd2hvIGtub3dzDQo+IG1vcmUgYWJvdXQgaXQgc2hvdWxk
IGNoaW1lIGluLg0KPiANCj4gQWxhbiBTdGVybg0KPiANCg0KVGhhbmtzIEFsYW4sDQoNCkknbSBn
b2luZyB0byBzZW5kIGEgdjMgaW5jbHVkaW5nIGFsbCB0aGVzZSBjaGFuZ2VzIHRvIGVhc2UgcmV2
aWV3Lg0KDQpSZWdhcmRzLA0KQW1lbGll
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply

[Amelie DELAUNAY]

On 02/22/2018 07:27 PM, Alan Stern wrote:
> On Thu, 22 Feb 2018, Amelie DELAUNAY wrote:
> 
>>>>>>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>>>>>>> @@ -19,6 +19,7 @@ Optional properties:
>>>>>>>      - phys : phandle + phy specifier pair
>>>>>>>      - phy-names : "usb"
>>>>>>>      - resets : phandle + reset specifier pair
>>>>>>> + - vbus-supply : phandle of regulator supplying vbus
>>>>>>>     
>>>>>>
>>>>>> Can platforms have more than one regulator e.g. one regulator per port?
>>>>>>
>>>>>
>>>>> I imagine that yes, platforms could have one regulator per port.
>>>>> Regulator consumers bindings impose a <name>-supply property per
>>>>> regulator, so, what do you think about :
>>>>> vbus0-supply for port#0
>>>>> vbus1-supply for port#1
>>>>> ...
>>>>> vbusN-supply for port#N
>>>>>
>>>>> And then in probe, allocate 'struct regulator *vbus_supplies' with a
>>>>> size corresponding to 'HCS_N_PORTS(ehci->hcs_params) * sizeof(struct
>>>>> regulator *)'.
>>>>> And loop to get optional regulator vbus0, vbus1,..., vbusN.
>>>>> And then enable/disable the corresponding regulator in
>>>>> ehci_platform_port_power thanks to portnum.
>>>>
>>>> Looks fine to me but we need to get Alan's opinion if this is worth the effort.
>>>> If there isn't a single platform needing it we could probably do without it
>>>> but the DT binding must be scalable to add this feature in the future.
>>>
>>> I agree that for now there don't seem to be any platforms requiring
>>> more than one regulator, but this should be implemented in a way that
>>> could be expanded if necessary.
>>>
>>> Anyway, the basic idea is reasonable.  I don't know to what extent
>>> people want to power-off their EHCI ports, but if they do then we ought
>>> to turn off external regulators at the same time.
>>>
>>> Is there a real-life use case for this?
>>>
>>> Alan Stern
>>>
>>
>> On my setup I have the following:
>>
>>                    regulator_____vbus
>>    _________________             \
>> | EHCI controller |-port0-----[USB connector]
>> |_________________|-port1-----X
>>
>> So, I have one regulator only for port0. But I could I have one more if
>> port1 was connected. My current regulator could also supplies port1.
>>
>> To allocate a vbus_supplies array depending on N_PORTS, I have to move
>> this initialization from probe to ehci_platform_reset, after ehci_setup
>> is done.
>> Then, I have to define each regulator id depending on the port number.
>> This imposes a binding like
>> - portN_vbus-supply : phandle of regulator supplying vbus for port N
>> But I don't know if we can describe it like this in dt-bindings ?
>>
>> &ehci {
>> 	...
>> 	port0_vbus-supply = <&vbus_reg>;
>> 	port1_vbus-supply = <&vbus_reg>; //Could be another regulator, or not
>> specified if port is not connected.
>> 	...
>> };
>>
>> Is it ok to move vbus_supplies initialization in ehci_platform_reset ?
> 
> Yes, it's okay to move the code if you need to.
> 
> However, I can not speak on the DT implications.  Someone who knows
> more about it should chime in.
> 
> Alan Stern
> 

Thanks Alan,

I'm going to send a v3 including all these changes to ease review.

Regards,
Amelie