| Message ID | 1464768141-25420-7-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 01, 2016 at 10:02:15AM +0200, Krzysztof Kozlowski wrote: > Some devices need real hard-reset by cutting the power. During power > sequence turn off and on the regulator, if it is provided. > > Additionally add support for instantiating the pwrseq-simple device on a > generic property 'power-sequence'. The device will attach itself to the > node containing the property and parse the node's properties like > reset-gpios, ext-supply etc. > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > --- > .../bindings/power/pwrseq/pwrseq-simple.txt | 29 +++++++- > drivers/power/pwrseq/pwrseq_simple.c | 85 +++++++++++++++++++++- > 2 files changed, 107 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt > index ce0e76749671..a8c3f13ee83f 100644 > --- a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt > +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt > @@ -1,11 +1,17 @@ > -* The simple MMC power sequence provider > +* The simple power sequence provider > > -The purpose of the simple MMC power sequence provider is to supports a set of > +The purpose of the simple power sequence provider is to supports a set of > common properties between various SOC designs. It thus enables us to use the > same provider for several SOC designs. > > -Required properties: > -- compatible : contains "mmc-pwrseq-simple". > +The driver supports two types of bindings: > +1. Separate node > + Required properties: > + - compatible : contains "mmc-pwrseq-simple". Please note that this is not recommended for new users. > + > +2. Property for any node > + Required properties: > + - power-sequence > > Optional properties: > - reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted > @@ -16,6 +22,7 @@ Optional properties: > See ../clocks/clock-bindings.txt for details. > - clock-names : Must include the following entry: > "ext_clock" (External clock provided to the card). > +- ext-supply : External regulator supply What happens when there are 2 supplies? I'd prefer the name not be genericish and use the real supply names. Then the power seq code should just turn on all supplies it finds. If the order or timing to turn on matters, then sorry, no generic sequence. > > Example: > > @@ -24,4 +31,18 @@ Example: > reset-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>; > clocks = <&clk_32768_ck>; > clock-names = "ext_clock"; > + ext-supply = <&buck8>; > } > + > + usb3503@08 { > + compatible = "smsc,usb3503"; > + reg = <0x08>; > + > + intn-gpios = <&gpx3 0 GPIO_ACTIVE_HIGH>; > + connect-gpios = <&gpx3 4 GPIO_ACTIVE_HIGH>; > + reset-gpios = <&gpx3 5 GPIO_ACTIVE_HIGH>; > + initial-mode = <1>; > + > + power-sequence; > + ext-supply = <&buck8_reg>; > + }; -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/03/2016 04:02 AM, Rob Herring wrote: > On Wed, Jun 01, 2016 at 10:02:15AM +0200, Krzysztof Kozlowski wrote: >> Some devices need real hard-reset by cutting the power. During power >> sequence turn off and on the regulator, if it is provided. >> >> Additionally add support for instantiating the pwrseq-simple device on a >> generic property 'power-sequence'. The device will attach itself to the >> node containing the property and parse the node's properties like >> reset-gpios, ext-supply etc. >> >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> --- >> .../bindings/power/pwrseq/pwrseq-simple.txt | 29 +++++++- >> drivers/power/pwrseq/pwrseq_simple.c | 85 +++++++++++++++++++++- >> 2 files changed, 107 insertions(+), 7 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt >> index ce0e76749671..a8c3f13ee83f 100644 >> --- a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt >> +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt >> @@ -1,11 +1,17 @@ >> -* The simple MMC power sequence provider >> +* The simple power sequence provider >> >> -The purpose of the simple MMC power sequence provider is to supports a set of >> +The purpose of the simple power sequence provider is to supports a set of >> common properties between various SOC designs. It thus enables us to use the >> same provider for several SOC designs. >> >> -Required properties: >> -- compatible : contains "mmc-pwrseq-simple". >> +The driver supports two types of bindings: >> +1. Separate node >> + Required properties: >> + - compatible : contains "mmc-pwrseq-simple". > > Please note that this is not recommended for new users. Sure. > >> + >> +2. Property for any node >> + Required properties: >> + - power-sequence >> >> Optional properties: >> - reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted >> @@ -16,6 +22,7 @@ Optional properties: >> See ../clocks/clock-bindings.txt for details. >> - clock-names : Must include the following entry: >> "ext_clock" (External clock provided to the card). >> +- ext-supply : External regulator supply > > What happens when there are 2 supplies? > > I'd prefer the name not be genericish and use the real supply names. > Then the power seq code should just turn on all supplies it finds. If > the order or timing to turn on matters, then sorry, no generic sequence. Understood. I'll change the code to use any supply. As for the genericness of this approach, Sylwester Nawrocki pointed an old thread: [PATCH v6 0/4] Runtime Interpreted Power Sequences https://lkml.org/lkml/2012/9/12/127 How do you like that approach? Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 03, 2016 at 02:35:08PM +0200, Krzysztof Kozlowski wrote: > On 06/03/2016 04:02 AM, Rob Herring wrote: > > On Wed, Jun 01, 2016 at 10:02:15AM +0200, Krzysztof Kozlowski wrote: > >> Some devices need real hard-reset by cutting the power. During power > >> sequence turn off and on the regulator, if it is provided. > >> > >> Additionally add support for instantiating the pwrseq-simple device on a > >> generic property 'power-sequence'. The device will attach itself to the > >> node containing the property and parse the node's properties like > >> reset-gpios, ext-supply etc. > >> > >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > >> --- > >> .../bindings/power/pwrseq/pwrseq-simple.txt | 29 +++++++- > >> drivers/power/pwrseq/pwrseq_simple.c | 85 +++++++++++++++++++++- > >> 2 files changed, 107 insertions(+), 7 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt > >> index ce0e76749671..a8c3f13ee83f 100644 > >> --- a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt > >> +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt > >> @@ -1,11 +1,17 @@ > >> -* The simple MMC power sequence provider > >> +* The simple power sequence provider > >> > >> -The purpose of the simple MMC power sequence provider is to supports a set of > >> +The purpose of the simple power sequence provider is to supports a set of > >> common properties between various SOC designs. It thus enables us to use the > >> same provider for several SOC designs. > >> > >> -Required properties: > >> -- compatible : contains "mmc-pwrseq-simple". > >> +The driver supports two types of bindings: > >> +1. Separate node > >> + Required properties: > >> + - compatible : contains "mmc-pwrseq-simple". > > > > Please note that this is not recommended for new users. > > Sure. > > > > >> + > >> +2. Property for any node > >> + Required properties: > >> + - power-sequence > >> > >> Optional properties: > >> - reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted > >> @@ -16,6 +22,7 @@ Optional properties: > >> See ../clocks/clock-bindings.txt for details. > >> - clock-names : Must include the following entry: > >> "ext_clock" (External clock provided to the card). > >> +- ext-supply : External regulator supply > > > > What happens when there are 2 supplies? > > > > I'd prefer the name not be genericish and use the real supply names. > > Then the power seq code should just turn on all supplies it finds. If > > the order or timing to turn on matters, then sorry, no generic sequence. > > Understood. I'll change the code to use any supply. > > As for the genericness of this approach, Sylwester Nawrocki pointed an > old thread: > [PATCH v6 0/4] Runtime Interpreted Power Sequences > https://lkml.org/lkml/2012/9/12/127 > > How do you like that approach? > Rob, I am trying to implement the dts layout you suggested (see below), but I find it is very hard to it due to the device is still not created, without device, it is hard to manage the resources under this device ( Eg, de-initialization for probe deferral case). So, a common driver is suitable for this power sequence case. &usbotg1 { vbus-supply = <®_usb_otg1_vbus>; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_usb_otg1_id>; status = "okay"; #address-cells = <1>; #size-cells = <0>; hub: genesys@1 { compatible = "usb5e3,608"; reg = <1>; reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; /* hub reset pin */ reset-duration-us = <10>; clocks = <&clks IMX6SX_CLK_CKO>; }; };
On 06/03/2016 04:02 AM, Rob Herring wrote: >> Optional properties: >> - reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted >> @@ -16,6 +22,7 @@ Optional properties: >> See ../clocks/clock-bindings.txt for details. >> - clock-names : Must include the following entry: >> "ext_clock" (External clock provided to the card). >> +- ext-supply : External regulator supply > > What happens when there are 2 supplies? > > I'd prefer the name not be genericish and use the real supply names. > Then the power seq code should just turn on all supplies it finds. If > the order or timing to turn on matters, then sorry, no generic sequence. I think the generic part for regulators might be a problem. Regulator API requires a name for the supply... it cannot get "something" or "everything". The driver could attach itself to any kind of node (where power-sequence property exists) so the supply name depends on the bindings of device (not bindings of power sequence driver). The power sequence driver could however iterate over child properties and get the names of all supplies. It is a little bit ugly... Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 07, 2016 at 11:29:02AM +0200, Krzysztof Kozlowski wrote: > On 06/03/2016 04:02 AM, Rob Herring wrote: > >> Optional properties: > >> - reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted > >> @@ -16,6 +22,7 @@ Optional properties: > >> See ../clocks/clock-bindings.txt for details. > >> - clock-names : Must include the following entry: > >> "ext_clock" (External clock provided to the card). > >> +- ext-supply : External regulator supply > > > > What happens when there are 2 supplies? > > > > I'd prefer the name not be genericish and use the real supply names. > > Then the power seq code should just turn on all supplies it finds. If > > the order or timing to turn on matters, then sorry, no generic sequence. > > I think the generic part for regulators might be a problem. Regulator > API requires a name for the supply... it cannot get "something" or > "everything". That's the downside of variable property names... > The driver could attach itself to any kind of node (where power-sequence > property exists) so the supply name depends on the bindings of device > (not bindings of power sequence driver). > > The power sequence driver could however iterate over child properties > and get the names of all supplies. It is a little bit ugly... Yes. Like this, right? for_each_property_of_node(np, pp) { if (!strstr(pp->name, "-supply")) continue; // found supply } The uglyness can always be improved with a function to do this parsing. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On Thu, Jun 9, 2016 at 3:03 AM, Rob Herring <robh@kernel.org> wrote: > On Tue, Jun 07, 2016 at 11:29:02AM +0200, Krzysztof Kozlowski wrote: >> On 06/03/2016 04:02 AM, Rob Herring wrote: >> >> Optional properties: >> >> - reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted >> >> @@ -16,6 +22,7 @@ Optional properties: >> >> See ../clocks/clock-bindings.txt for details. >> >> - clock-names : Must include the following entry: >> >> "ext_clock" (External clock provided to the card). >> >> +- ext-supply : External regulator supply >> > >> > What happens when there are 2 supplies? >> > >> > I'd prefer the name not be genericish and use the real supply names. >> > Then the power seq code should just turn on all supplies it finds. If >> > the order or timing to turn on matters, then sorry, no generic sequence. >> >> I think the generic part for regulators might be a problem. Regulator >> API requires a name for the supply... it cannot get "something" or >> "everything". > > That's the downside of variable property names... > >> The driver could attach itself to any kind of node (where power-sequence >> property exists) so the supply name depends on the bindings of device >> (not bindings of power sequence driver). >> >> The power sequence driver could however iterate over child properties >> and get the names of all supplies. It is a little bit ugly... > > Yes. Like this, right? > > for_each_property_of_node(np, pp) { > if (!strstr(pp->name, "-supply")) > continue; > // found supply > } > > The uglyness can always be improved with a function to do this parsing. There's already a version of this in simplefb. Maybe it's time to move this to a common function? ChenYu -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/09/2016 04:34 AM, Chen-Yu Tsai wrote: > Hi > > On Thu, Jun 9, 2016 at 3:03 AM, Rob Herring <robh@kernel.org> wrote: >> On Tue, Jun 07, 2016 at 11:29:02AM +0200, Krzysztof Kozlowski wrote: >>> On 06/03/2016 04:02 AM, Rob Herring wrote: >>>>> Optional properties: >>>>> - reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted >>>>> @@ -16,6 +22,7 @@ Optional properties: >>>>> See ../clocks/clock-bindings.txt for details. >>>>> - clock-names : Must include the following entry: >>>>> "ext_clock" (External clock provided to the card). >>>>> +- ext-supply : External regulator supply >>>> >>>> What happens when there are 2 supplies? >>>> >>>> I'd prefer the name not be genericish and use the real supply names. >>>> Then the power seq code should just turn on all supplies it finds. If >>>> the order or timing to turn on matters, then sorry, no generic sequence. >>> >>> I think the generic part for regulators might be a problem. Regulator >>> API requires a name for the supply... it cannot get "something" or >>> "everything". >> >> That's the downside of variable property names... >> >>> The driver could attach itself to any kind of node (where power-sequence >>> property exists) so the supply name depends on the bindings of device >>> (not bindings of power sequence driver). >>> >>> The power sequence driver could however iterate over child properties >>> and get the names of all supplies. It is a little bit ugly... >> >> Yes. Like this, right? >> >> for_each_property_of_node(np, pp) { >> if (!strstr(pp->name, "-supply")) >> continue; >> // found supply >> } >> >> The uglyness can always be improved with a function to do this parsing. > > There's already a version of this in simplefb. Maybe it's time to move > this to a common function? Thanks, I'll make a generic one and let's see how Mark will respond to it. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt index ce0e76749671..a8c3f13ee83f 100644 --- a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt +++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-simple.txt @@ -1,11 +1,17 @@ -* The simple MMC power sequence provider +* The simple power sequence provider -The purpose of the simple MMC power sequence provider is to supports a set of +The purpose of the simple power sequence provider is to supports a set of common properties between various SOC designs. It thus enables us to use the same provider for several SOC designs. -Required properties: -- compatible : contains "mmc-pwrseq-simple". +The driver supports two types of bindings: +1. Separate node + Required properties: + - compatible : contains "mmc-pwrseq-simple". + +2. Property for any node + Required properties: + - power-sequence Optional properties: - reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted @@ -16,6 +22,7 @@ Optional properties: See ../clocks/clock-bindings.txt for details. - clock-names : Must include the following entry: "ext_clock" (External clock provided to the card). +- ext-supply : External regulator supply Example: @@ -24,4 +31,18 @@ Example: reset-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>; clocks = <&clk_32768_ck>; clock-names = "ext_clock"; + ext-supply = <&buck8>; } + + usb3503@08 { + compatible = "smsc,usb3503"; + reg = <0x08>; + + intn-gpios = <&gpx3 0 GPIO_ACTIVE_HIGH>; + connect-gpios = <&gpx3 4 GPIO_ACTIVE_HIGH>; + reset-gpios = <&gpx3 5 GPIO_ACTIVE_HIGH>; + initial-mode = <1>; + + power-sequence; + ext-supply = <&buck8_reg>; + }; diff --git a/drivers/power/pwrseq/pwrseq_simple.c b/drivers/power/pwrseq/pwrseq_simple.c index 93807a6ef162..4096261b16a4 100644 --- a/drivers/power/pwrseq/pwrseq_simple.c +++ b/drivers/power/pwrseq/pwrseq_simple.c @@ -1,12 +1,15 @@ /* - * Copyright (C) 2014 Linaro Ltd + * Copyright (C) 2014 Linaro Ltd + * Copyright (C) 2016 Samsung Electronics * * Author: Ulf Hansson <ulf.hansson@linaro.org> + * Krzysztof Kozlowski <k.kozlowski@samsung.com> * * License terms: GNU General Public License (GPL) version 2 * * Simple MMC power sequence management */ +#include <linux/of.h> #include <linux/clk.h> #include <linux/init.h> #include <linux/kernel.h> @@ -16,13 +19,16 @@ #include <linux/device.h> #include <linux/err.h> #include <linux/gpio/consumer.h> +#include <linux/regulator/consumer.h> #include <linux/pwrseq.h> +#include <linux/delay.h> struct mmc_pwrseq_simple { struct pwrseq pwrseq; bool clk_enabled; struct clk *ext_clk; struct gpio_descs *reset_gpios; + struct regulator *ext_reg; }; #define to_pwrseq_simple(p) container_of(p, struct mmc_pwrseq_simple, pwrseq) @@ -60,6 +66,13 @@ static void mmc_pwrseq_simple_post_power_on(struct pwrseq *_pwrseq) { struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(_pwrseq); + if (pwrseq->ext_reg) { + int err; + + err = regulator_enable(pwrseq->ext_reg); + WARN_ON_ONCE(err); + } + mmc_pwrseq_simple_set_gpios_value(pwrseq, 0); } @@ -73,6 +86,13 @@ static void mmc_pwrseq_simple_power_off(struct pwrseq *_pwrseq) clk_disable_unprepare(pwrseq->ext_clk); pwrseq->clk_enabled = false; } + + if (pwrseq->ext_reg) { + int err; + + err = regulator_disable(pwrseq->ext_reg); + WARN_ON_ONCE(err); + } } static const struct pwrseq_ops mmc_pwrseq_simple_ops = { @@ -100,12 +120,40 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev) if (IS_ERR(pwrseq->ext_clk) && PTR_ERR(pwrseq->ext_clk) != -ENOENT) return PTR_ERR(pwrseq->ext_clk); + pwrseq->ext_reg = devm_regulator_get_optional(dev, "ext"); + if (IS_ERR(pwrseq->ext_reg)) { + if (PTR_ERR(pwrseq->ext_reg) == -ENODEV) + pwrseq->ext_reg = NULL; + else + return PTR_ERR(pwrseq->ext_reg); + } else { + int err; + /* + * Be sure that regulator is off, before the driver will start + * power sequence. It is likely that regulator is on by default + * and it without toggling it here, it would be disabled much + * later by the core. + */ + + err = regulator_enable(pwrseq->ext_reg); + WARN_ON_ONCE(err); + + err = regulator_disable(pwrseq->ext_reg); + WARN_ON_ONCE(err); + } + pwrseq->reset_gpios = devm_gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH); if (IS_ERR(pwrseq->reset_gpios) && PTR_ERR(pwrseq->reset_gpios) != -ENOENT && PTR_ERR(pwrseq->reset_gpios) != -ENOSYS) { - return PTR_ERR(pwrseq->reset_gpios); + /* + * Don't care about errors. If this pwrseq device was added + * to node with existing reset-gpios, then the GPIO reset will + * be handled by other device. + */ + dev_warn(dev, "Cannot get reset gpio: %ld\n", + PTR_ERR(pwrseq->reset_gpios)); } pwrseq->pwrseq.dev = dev; @@ -122,6 +170,13 @@ static int mmc_pwrseq_simple_remove(struct platform_device *pdev) pwrseq_unregister(&pwrseq->pwrseq); + if (pwrseq->ext_reg) { + int err; + + err = regulator_disable(pwrseq->ext_reg); + WARN_ON_ONCE(err); + } + return 0; } @@ -134,5 +189,29 @@ static struct platform_driver mmc_pwrseq_simple_driver = { }, }; -module_platform_driver(mmc_pwrseq_simple_driver); +static int __init mmc_pwrseq_simple_driver_init(void) +{ + struct platform_device *pdev; + struct device_node *np; + + for_each_node_with_property(np, "power-sequence") { + pdev = platform_device_register_simple("pwrseq_simple", + PLATFORM_DEVID_AUTO, + NULL, 0); + if (!IS_ERR(pdev)) { + of_node_get(np); + pdev->dev.of_node = np; + } + } + + return platform_driver_register(&mmc_pwrseq_simple_driver); +} +module_init(mmc_pwrseq_simple_driver_init); + +static void __exit mmc_pwrseq_simple_driver_exit(void) +{ + /* FIXME: of_node_put? */ + platform_driver_unregister(&mmc_pwrseq_simple_driver); +} +module_exit(mmc_pwrseq_simple_driver_exit); MODULE_LICENSE("GPL v2");
Some devices need real hard-reset by cutting the power. During power sequence turn off and on the regulator, if it is provided. Additionally add support for instantiating the pwrseq-simple device on a generic property 'power-sequence'. The device will attach itself to the node containing the property and parse the node's properties like reset-gpios, ext-supply etc. Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- .../bindings/power/pwrseq/pwrseq-simple.txt | 29 +++++++- drivers/power/pwrseq/pwrseq_simple.c | 85 +++++++++++++++++++++- 2 files changed, 107 insertions(+), 7 deletions(-)