* [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 12:58 ` Amelie Delaunay 0 siblings, 0 replies; 40+ messages in thread From: Amelie Delaunay @ 2018-02-20 12:58 UTC (permalink / raw) To: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel, 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 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 12:58 ` Amelie Delaunay 0 siblings, 0 replies; 40+ messages in thread From: Amelie Delaunay @ 2018-02-20 12:58 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 12:58 ` Amelie Delaunay 0 siblings, 0 replies; 40+ messages in thread From: Amelie Delaunay @ 2018-02-20 12:58 UTC (permalink / raw) To: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel, 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) ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 12:58 ` Amelie Delaunay 0 siblings, 0 replies; 40+ messages in thread From: Amelie Delaunay @ 2018-02-20 12:58 UTC (permalink / raw) To: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: devicetree, Amelie Delaunay, linux-usb, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply 2018-02-20 12:58 ` [PATCH v2] " Amelie Delaunay (?) (?) @ 2018-02-20 13:02 ` Felipe Balbi -1 siblings, 0 replies; 40+ messages in thread From: Felipe Balbi @ 2018-02-20 13:02 UTC (permalink / raw) To: Amelie Delaunay, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel, Amelie Delaunay 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 13:02 ` Felipe Balbi 0 siblings, 0 replies; 40+ messages in thread From: Felipe Balbi @ 2018-02-20 13:02 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 13:02 ` Felipe Balbi 0 siblings, 0 replies; 40+ messages in thread From: Felipe Balbi @ 2018-02-20 13:02 UTC (permalink / raw) To: Amelie Delaunay, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel 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; ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 13:02 ` Felipe Balbi 0 siblings, 0 replies; 40+ messages in thread From: Felipe Balbi @ 2018-02-20 13:02 UTC (permalink / raw) To: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: devicetree, Amelie Delaunay, linux-usb, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply 2018-02-20 13:02 ` [PATCH v2] " Felipe Balbi (?) (?) @ 2018-02-20 13:48 ` Amelie DELAUNAY -1 siblings, 0 replies; 40+ messages in thread From: Amelie DELAUNAY @ 2018-02-20 13:48 UTC (permalink / raw) To: Felipe Balbi, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern, Roger Quadros Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 13:48 ` Amelie DELAUNAY 0 siblings, 0 replies; 40+ messages in thread From: Amelie DELAUNAY @ 2018-02-20 13:48 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 13:48 ` Amelie DELAUNAY 0 siblings, 0 replies; 40+ messages in thread From: Amelie Delaunay @ 2018-02-20 13:48 UTC (permalink / raw) To: Felipe Balbi, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern, Roger Quadros Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 13:48 ` Amelie DELAUNAY 0 siblings, 0 replies; 40+ messages in thread From: Amelie DELAUNAY @ 2018-02-20 13:48 UTC (permalink / raw) To: Felipe Balbi, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern, Roger Quadros Cc: devicetree, linux-usb, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply 2018-02-20 12:58 ` [PATCH v2] " Amelie Delaunay (?) (?) @ 2018-02-20 14:00 ` Roger Quadros -1 siblings, 0 replies; 40+ messages in thread From: Roger Quadros @ 2018-02-20 14:00 UTC (permalink / raw) To: Amelie Delaunay, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 14:00 ` Roger Quadros 0 siblings, 0 replies; 40+ messages in thread From: Roger Quadros @ 2018-02-20 14:00 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 14:00 ` Roger Quadros 0 siblings, 0 replies; 40+ messages in thread From: Roger Quadros @ 2018-02-20 14:00 UTC (permalink / raw) To: Amelie Delaunay, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel 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) > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 14:00 ` Roger Quadros 0 siblings, 0 replies; 40+ messages in thread From: Roger Quadros @ 2018-02-20 14:00 UTC (permalink / raw) To: Amelie Delaunay, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: devicetree, linux-usb, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply 2018-02-20 14:00 ` [PATCH v2] " Roger Quadros (?) (?) @ 2018-02-20 14:46 ` Amelie DELAUNAY -1 siblings, 0 replies; 40+ messages in thread From: Amelie DELAUNAY @ 2018-02-20 14:46 UTC (permalink / raw) To: Roger Quadros, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel 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) >> > ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 14:46 ` Amelie DELAUNAY 0 siblings, 0 replies; 40+ messages in thread From: Amelie DELAUNAY @ 2018-02-20 14:46 UTC (permalink / raw) To: linux-arm-kernel 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) >> > ^ permalink raw reply [flat|nested] 40+ messages in thread
* [v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 14:46 ` Amelie DELAUNAY 0 siblings, 0 replies; 40+ messages in thread From: Amelie Delaunay @ 2018-02-20 14:46 UTC (permalink / raw) To: Roger Quadros, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 14:46 ` Amelie DELAUNAY 0 siblings, 0 replies; 40+ messages in thread From: Amelie DELAUNAY @ 2018-02-20 14:46 UTC (permalink / raw) To: Roger Quadros, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: devicetree, linux-usb, linux-kernel, linux-arm-kernel 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) >> > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply 2018-02-20 14:46 ` [PATCH v2] " Amelie DELAUNAY (?) (?) @ 2018-02-20 16:33 ` Roger Quadros -1 siblings, 0 replies; 40+ messages in thread From: Roger Quadros @ 2018-02-20 16:33 UTC (permalink / raw) To: Amelie DELAUNAY, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 16:33 ` Roger Quadros 0 siblings, 0 replies; 40+ messages in thread From: Roger Quadros @ 2018-02-20 16:33 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 16:33 ` Roger Quadros 0 siblings, 0 replies; 40+ messages in thread From: Roger Quadros @ 2018-02-20 16:33 UTC (permalink / raw) To: Amelie DELAUNAY, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: linux-usb, devicetree, linux-kernel, linux-arm-kernel 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) >>> ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 16:33 ` Roger Quadros 0 siblings, 0 replies; 40+ messages in thread From: Roger Quadros @ 2018-02-20 16:33 UTC (permalink / raw) To: Amelie DELAUNAY, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, Alan Stern Cc: devicetree, linux-usb, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply 2018-02-20 16:33 ` [PATCH v2] " Roger Quadros (?) (?) @ 2018-02-20 18:10 ` Alan Stern -1 siblings, 0 replies; 40+ messages in thread From: Alan Stern @ 2018-02-20 18:10 UTC (permalink / raw) To: Roger Quadros Cc: Amelie DELAUNAY, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, linux-usb, devicetree, linux-kernel, linux-arm-kernel 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) > >>> > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 18:10 ` Alan Stern 0 siblings, 0 replies; 40+ messages in thread From: Alan Stern @ 2018-02-20 18:10 UTC (permalink / raw) To: linux-arm-kernel 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) > >>> > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* [v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 18:10 ` Alan Stern 0 siblings, 0 replies; 40+ messages in thread From: Alan Stern @ 2018-02-20 18:10 UTC (permalink / raw) To: Roger Quadros Cc: Amelie DELAUNAY, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, linux-usb, devicetree, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-20 18:10 ` Alan Stern 0 siblings, 0 replies; 40+ messages in thread From: Alan Stern @ 2018-02-20 18:10 UTC (permalink / raw) To: Roger Quadros Cc: Mark Rutland, devicetree, Amelie DELAUNAY, Greg Kroah-Hartman, linux-usb, linux-kernel, Tony Prisk, Rob Herring, linux-arm-kernel 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) > >>> > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply 2018-02-20 18:10 ` [PATCH v2] " Alan Stern (?) (?) @ 2018-02-22 10:17 ` Amelie DELAUNAY -1 siblings, 0 replies; 40+ messages in thread From: Amelie DELAUNAY @ 2018-02-22 10:17 UTC (permalink / raw) To: Alan Stern, Roger Quadros Cc: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, linux-usb, devicetree, linux-kernel, linux-arm-kernel 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) >>>>> >> >> > > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-22 10:17 ` Amelie DELAUNAY 0 siblings, 0 replies; 40+ messages in thread From: Amelie DELAUNAY @ 2018-02-22 10:17 UTC (permalink / raw) To: linux-arm-kernel 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) >>>>> >> >> > > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* [v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-22 10:17 ` Amelie DELAUNAY 0 siblings, 0 replies; 40+ messages in thread From: Amelie Delaunay @ 2018-02-22 10:17 UTC (permalink / raw) To: Alan Stern, Roger Quadros Cc: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, linux-usb, devicetree, linux-kernel, linux-arm-kernel 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) >>>>> >> >> > > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-22 10:17 ` Amelie DELAUNAY 0 siblings, 0 replies; 40+ messages in thread From: Amelie DELAUNAY @ 2018-02-22 10:17 UTC (permalink / raw) To: Alan Stern, Roger Quadros Cc: Mark Rutland, devicetree, Greg Kroah-Hartman, linux-usb, linux-kernel, Tony Prisk, Rob Herring, linux-arm-kernel 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) >>>>> >> >> > > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply 2018-02-22 10:17 ` [PATCH v2] " Amelie DELAUNAY (?) (?) @ 2018-02-22 18:27 ` Alan Stern -1 siblings, 0 replies; 40+ messages in thread From: Alan Stern @ 2018-02-22 18:27 UTC (permalink / raw) To: Amelie DELAUNAY Cc: Roger Quadros, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, linux-usb, devicetree, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-22 18:27 ` Alan Stern 0 siblings, 0 replies; 40+ messages in thread From: Alan Stern @ 2018-02-22 18:27 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-22 18:27 ` Alan Stern 0 siblings, 0 replies; 40+ messages in thread From: Alan Stern @ 2018-02-22 18:27 UTC (permalink / raw) To: Amelie DELAUNAY Cc: Roger Quadros, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, linux-usb, devicetree, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-22 18:27 ` Alan Stern 0 siblings, 0 replies; 40+ messages in thread From: Alan Stern @ 2018-02-22 18:27 UTC (permalink / raw) To: Amelie DELAUNAY Cc: Mark Rutland, devicetree, Greg Kroah-Hartman, linux-usb, linux-kernel, Tony Prisk, Rob Herring, linux-arm-kernel, Roger Quadros 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply 2018-02-22 18:27 ` [PATCH v2] " Alan Stern (?) (?) @ 2018-02-23 10:46 ` Amelie DELAUNAY -1 siblings, 0 replies; 40+ messages in thread From: Amelie DELAUNAY @ 2018-02-23 10:46 UTC (permalink / raw) To: Alan Stern Cc: Roger Quadros, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, linux-usb, devicetree, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-23 10:46 ` Amelie DELAUNAY 0 siblings, 0 replies; 40+ messages in thread From: Amelie DELAUNAY @ 2018-02-23 10:46 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-23 10:46 ` Amelie DELAUNAY 0 siblings, 0 replies; 40+ messages in thread From: Amelie Delaunay @ 2018-02-23 10:46 UTC (permalink / raw) To: Alan Stern Cc: Roger Quadros, Greg Kroah-Hartman, Rob Herring, Mark Rutland, Tony Prisk, linux-usb, devicetree, linux-kernel, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply @ 2018-02-23 10:46 ` Amelie DELAUNAY 0 siblings, 0 replies; 40+ messages in thread From: Amelie DELAUNAY @ 2018-02-23 10:46 UTC (permalink / raw) To: Alan Stern Cc: Mark Rutland, devicetree, Greg Kroah-Hartman, linux-usb, linux-kernel, Tony Prisk, Rob Herring, linux-arm-kernel, Roger Quadros 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2018-02-23 10:46 UTC | newest] Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-20 12:58 [PATCH v2] usb: host: ehci-platform: add support for optional external vbus supply Amelie Delaunay 2018-02-20 12:58 ` Amelie Delaunay 2018-02-20 12:58 ` [v2] " Amelie Delaunay 2018-02-20 12:58 ` [PATCH v2] " Amelie Delaunay 2018-02-20 13:02 ` Felipe Balbi 2018-02-20 13:02 ` Felipe Balbi 2018-02-20 13:02 ` [v2] " Felipe Balbi 2018-02-20 13:02 ` [PATCH v2] " Felipe Balbi 2018-02-20 13:48 ` Amelie DELAUNAY 2018-02-20 13:48 ` Amelie DELAUNAY 2018-02-20 13:48 ` [v2] " Amelie Delaunay 2018-02-20 13:48 ` [PATCH v2] " Amelie DELAUNAY 2018-02-20 14:00 ` Roger Quadros 2018-02-20 14:00 ` Roger Quadros 2018-02-20 14:00 ` [v2] " Roger Quadros 2018-02-20 14:00 ` [PATCH v2] " Roger Quadros 2018-02-20 14:46 ` Amelie DELAUNAY 2018-02-20 14:46 ` Amelie DELAUNAY 2018-02-20 14:46 ` [v2] " Amelie Delaunay 2018-02-20 14:46 ` [PATCH v2] " Amelie DELAUNAY 2018-02-20 16:33 ` Roger Quadros 2018-02-20 16:33 ` Roger Quadros 2018-02-20 16:33 ` [v2] " Roger Quadros 2018-02-20 16:33 ` [PATCH v2] " Roger Quadros 2018-02-20 18:10 ` Alan Stern 2018-02-20 18:10 ` Alan Stern 2018-02-20 18:10 ` [v2] " Alan Stern 2018-02-20 18:10 ` [PATCH v2] " Alan Stern 2018-02-22 10:17 ` Amelie DELAUNAY 2018-02-22 10:17 ` Amelie DELAUNAY 2018-02-22 10:17 ` [v2] " Amelie Delaunay 2018-02-22 10:17 ` [PATCH v2] " Amelie DELAUNAY 2018-02-22 18:27 ` Alan Stern 2018-02-22 18:27 ` Alan Stern 2018-02-22 18:27 ` [v2] " Alan Stern 2018-02-22 18:27 ` [PATCH v2] " Alan Stern 2018-02-23 10:46 ` Amelie DELAUNAY 2018-02-23 10:46 ` Amelie DELAUNAY 2018-02-23 10:46 ` [v2] " Amelie Delaunay 2018-02-23 10:46 ` [PATCH v2] " Amelie DELAUNAY
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.