Re: [PATCH v7 2/8] power: add power sequence library
On Thu, Oct 13, 2016 at 09:04:42AM +0200, Heiko Stuebner wrote: > > > > +static int __init pwrseq_generic_register(void) > > > > +{ > > > > + struct pwrseq_generic *pwrseq_gen; > > > > + int i; > > > > + > > > > + for (i = 0; i < CONFIG_PWRSEQ_GENERIC_INSTANCE_NUMBER; i++) { > > > > + pwrseq_gen = kzalloc(sizeof(*pwrseq_gen), GFP_KERNEL); > > > > + if (!pwrseq_gen) > > > > + return -ENOMEM; > > > > + > > > > + pwrseq_gen->pwrseq.pwrseq_of_match_table = > > > > generic_id_table; > > > > + pwrseq_gen->pwrseq.get = pwrseq_generic_get; > > > > + pwrseq_gen->pwrseq.on = pwrseq_generic_on; > > > > + pwrseq_gen->pwrseq.off = pwrseq_generic_off; > > > > + pwrseq_gen->pwrseq.put = pwrseq_generic_put; > > > > + pwrseq_gen->pwrseq.free = pwrseq_generic_free; > > > > + > > > > + pwrseq_register(_gen->pwrseq); > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > +postcore_initcall(pwrseq_generic_register) > > > > > > I see that you need to have it preallocated for the compatible matching, > > > but wouldn't it also work to either just register the type and allocate > > > dynamically or otherwise just allocate a new spare everytime > > > pwrseq_generic_get() picks up the previous spare? > > > > Before compatible matching, the host driver doesn't know which pwrseq type > > for its child node, then doesn't know which pwrseq instance needs to be > > allocated. From dts, we don't know which pwrseq type for the node. > > yes, that is why I was suggesting allocating one (or two) here in > pwrseq_generic_register() and every time pwrseq_generic_get() grabs the last > free sequence instance, you allocate a new free spare one in that function. Good idea. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] power: add power sequence library
Am Donnerstag, 13. Oktober 2016, 09:22:16 CEST schrieb Peter Chen: > On Wed, Oct 12, 2016 at 12:30:29PM +0200, Heiko Stuebner wrote: > > Hi, > > > > Am Dienstag, 20. September 2016, 11:36:41 CEST schrieb Peter Chen: > > > We have an well-known problem that the device needs to do some power > > > sequence before it can be recognized by related host, the typical > > > example like hard-wired mmc devices and usb devices. > > > > > > This power sequence is hard to be described at device tree and handled > > > by > > > related host driver, so we have created a common power sequence > > > library to cover this requirement. The core code has supplied > > > some common helpers for host driver, and individual power sequence > > > libraries handle kinds of power sequence for devices. > > > > > > pwrseq_generic is intended for general purpose of power sequence, which > > > handles gpios and clocks currently, and can cover regulator and pinctrl > > > in future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off > > > if only one power sequence is needed, else call of_pwrseq_on_list > > > /of_pwrseq_off_list instead (eg, USB hub driver). > > > > > > Signed-off-by: Peter Chen> > > Tested-by Joshua Clayton > > > Reviewed-by: Matthias Kaehlcke > > > Tested-by: Matthias Kaehlcke > > > > first of all, glad to see this move forward. I've only some qualms with > > the > > static number of allocated power sequences below. > > Thanks for commenting it, the preallocate way is not a good way, but I > can't find suitable way. See below comments. > > > > +static int __init pwrseq_generic_register(void) > > > +{ > > > + struct pwrseq_generic *pwrseq_gen; > > > + int i; > > > + > > > + for (i = 0; i < CONFIG_PWRSEQ_GENERIC_INSTANCE_NUMBER; i++) { > > > + pwrseq_gen = kzalloc(sizeof(*pwrseq_gen), GFP_KERNEL); > > > + if (!pwrseq_gen) > > > + return -ENOMEM; > > > + > > > + pwrseq_gen->pwrseq.pwrseq_of_match_table = generic_id_table; > > > + pwrseq_gen->pwrseq.get = pwrseq_generic_get; > > > + pwrseq_gen->pwrseq.on = pwrseq_generic_on; > > > + pwrseq_gen->pwrseq.off = pwrseq_generic_off; > > > + pwrseq_gen->pwrseq.put = pwrseq_generic_put; > > > + pwrseq_gen->pwrseq.free = pwrseq_generic_free; > > > + > > > + pwrseq_register(_gen->pwrseq); > > > + } > > > + > > > + return 0; > > > +} > > > +postcore_initcall(pwrseq_generic_register) > > > > I see that you need to have it preallocated for the compatible matching, > > but wouldn't it also work to either just register the type and allocate > > dynamically or otherwise just allocate a new spare everytime > > pwrseq_generic_get() picks up the previous spare? > > Before compatible matching, the host driver doesn't know which pwrseq type > for its child node, then doesn't know which pwrseq instance needs to be > allocated. From dts, we don't know which pwrseq type for the node. yes, that is why I was suggesting allocating one (or two) here in pwrseq_generic_register() and every time pwrseq_generic_get() grabs the last free sequence instance, you allocate a new free spare one in that function. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] power: add power sequence library
On Wed, Oct 12, 2016 at 12:30:29PM +0200, Heiko Stuebner wrote: > Hi, > > Am Dienstag, 20. September 2016, 11:36:41 CEST schrieb Peter Chen: > > We have an well-known problem that the device needs to do some power > > sequence before it can be recognized by related host, the typical > > example like hard-wired mmc devices and usb devices. > > > > This power sequence is hard to be described at device tree and handled by > > related host driver, so we have created a common power sequence > > library to cover this requirement. The core code has supplied > > some common helpers for host driver, and individual power sequence > > libraries handle kinds of power sequence for devices. > > > > pwrseq_generic is intended for general purpose of power sequence, which > > handles gpios and clocks currently, and can cover regulator and pinctrl > > in future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off > > if only one power sequence is needed, else call of_pwrseq_on_list > > /of_pwrseq_off_list instead (eg, USB hub driver). > > > > Signed-off-by: Peter Chen> > Tested-by Joshua Clayton > > Reviewed-by: Matthias Kaehlcke > > Tested-by: Matthias Kaehlcke > > first of all, glad to see this move forward. I've only some qualms with the > static number of allocated power sequences below. > Thanks for commenting it, the preallocate way is not a good way, but I can't find suitable way. See below comments. > > +static int __init pwrseq_generic_register(void) > > +{ > > + struct pwrseq_generic *pwrseq_gen; > > + int i; > > + > > + for (i = 0; i < CONFIG_PWRSEQ_GENERIC_INSTANCE_NUMBER; i++) { > > + pwrseq_gen = kzalloc(sizeof(*pwrseq_gen), GFP_KERNEL); > > + if (!pwrseq_gen) > > + return -ENOMEM; > > + > > + pwrseq_gen->pwrseq.pwrseq_of_match_table = generic_id_table; > > + pwrseq_gen->pwrseq.get = pwrseq_generic_get; > > + pwrseq_gen->pwrseq.on = pwrseq_generic_on; > > + pwrseq_gen->pwrseq.off = pwrseq_generic_off; > > + pwrseq_gen->pwrseq.put = pwrseq_generic_put; > > + pwrseq_gen->pwrseq.free = pwrseq_generic_free; > > + > > + pwrseq_register(_gen->pwrseq); > > + } > > + > > + return 0; > > +} > > +postcore_initcall(pwrseq_generic_register) > > I see that you need to have it preallocated for the compatible matching, but > wouldn't it also work to either just register the type and allocate > dynamically or otherwise just allocate a new spare everytime > pwrseq_generic_get() picks up the previous spare? Before compatible matching, the host driver doesn't know which pwrseq type for its child node, then doesn't know which pwrseq instance needs to be allocated. From dts, we don't know which pwrseq type for the node. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] power: add power sequence library
Hi, Am Dienstag, 20. September 2016, 11:36:41 CEST schrieb Peter Chen: > We have an well-known problem that the device needs to do some power > sequence before it can be recognized by related host, the typical > example like hard-wired mmc devices and usb devices. > > This power sequence is hard to be described at device tree and handled by > related host driver, so we have created a common power sequence > library to cover this requirement. The core code has supplied > some common helpers for host driver, and individual power sequence > libraries handle kinds of power sequence for devices. > > pwrseq_generic is intended for general purpose of power sequence, which > handles gpios and clocks currently, and can cover regulator and pinctrl > in future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off > if only one power sequence is needed, else call of_pwrseq_on_list > /of_pwrseq_off_list instead (eg, USB hub driver). > > Signed-off-by: Peter Chen> Tested-by Joshua Clayton > Reviewed-by: Matthias Kaehlcke > Tested-by: Matthias Kaehlcke first of all, glad to see this move forward. I've only some qualms with the static number of allocated power sequences below. [...] > diff --git a/drivers/power/pwrseq/Kconfig b/drivers/power/pwrseq/Kconfig > new file mode 100644 > index 000..dff5e35 > --- /dev/null > +++ b/drivers/power/pwrseq/Kconfig > @@ -0,0 +1,45 @@ > +# > +# Power Sequence library > +# > + > +config POWER_SEQUENCE > + bool > + > +menu "Power Sequence Support" > + > +config PWRSEQ_GENERIC > + bool "Generic power sequence control" > + depends on OF > + select POWER_SEQUENCE > + help > +It is used for drivers which needs to do power sequence > +(eg, turn on clock, toggle reset gpio) before the related > +devices can be found by hardware. This generic one can be > +used for common power sequence control. > + > +config PWRSEQ_GENERIC_INSTANCE_NUMBER > + int "Number of Generic Power Sequence Instance" > + depends on PWRSEQ_GENERIC > + range 1 10 > + default 2 > + help > +Usually, there are not so many devices needs power sequence, we set > two > +as default value. limiting this to some arbitary compile-time number somehow seems crippling for the single-image approach. I.e. a distribution might select something and during its lifetime the board requiring n+1 power-sequences appears and thus needs a different kernel version just to support that additional sequence. Also, board designers are creative, and there were already complex examples mentioned elsewhere, so nothing keeps people from inventing something even more complex. [...] > diff --git a/drivers/power/pwrseq/pwrseq_generic.c > b/drivers/power/pwrseq/pwrseq_generic.c new file mode 100644 > index 000..bcd16c3 > --- /dev/null > +++ b/drivers/power/pwrseq/pwrseq_generic.c [...] > +static int pwrseq_generic_get(struct device_node *np, struct pwrseq > *pwrseq) +{ > + struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq); > + enum of_gpio_flags flags; > + int reset_gpio, clk, ret = 0; > + > + for (clk = 0; clk < PWRSEQ_MAX_CLKS; clk++) { > + pwrseq_gen->clks[clk] = of_clk_get(np, clk); > + if (IS_ERR(pwrseq_gen->clks[clk])) { > + ret = PTR_ERR(pwrseq_gen->clks[clk]); > + if (ret != -ENOENT) > + goto err_put_clks; > + pwrseq_gen->clks[clk] = NULL; > + break; > + } > + } > + > + reset_gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, ); > + if (gpio_is_valid(reset_gpio)) { > + unsigned long gpio_flags; > + > + if (flags & OF_GPIO_ACTIVE_LOW) > + gpio_flags = GPIOF_ACTIVE_LOW | GPIOF_OUT_INIT_LOW; > + else > + gpio_flags = GPIOF_OUT_INIT_HIGH; > + > + ret = gpio_request_one(reset_gpio, gpio_flags, > + "pwrseq-reset-gpios"); > + if (ret) > + goto err_put_clks; > + > + pwrseq_gen->gpiod_reset = gpio_to_desc(reset_gpio); > + of_property_read_u32(np, "reset-duration-us", > + _gen->duration_us); > + } else { > + if (reset_gpio == -ENOENT) > + return 0; > + > + ret = reset_gpio; > + pr_err("Failed to get reset gpio on %s, err = %d\n", > + np->full_name, reset_gpio); > + goto err_put_clks; > + } > + > + return ret; > + > +err_put_clks: > + while (--clk >= 0) > + clk_put(pwrseq_gen->clks[clk]); > + return ret; > +} > + > +static const struct of_device_id generic_id_table[] = { > + { .compatible = "generic",}, > + { /* sentinel */ } > +}; > + >
[PATCH v7 2/8] power: add power sequence library
We have an well-known problem that the device needs to do some power sequence before it can be recognized by related host, the typical example like hard-wired mmc devices and usb devices. This power sequence is hard to be described at device tree and handled by related host driver, so we have created a common power sequence library to cover this requirement. The core code has supplied some common helpers for host driver, and individual power sequence libraries handle kinds of power sequence for devices. pwrseq_generic is intended for general purpose of power sequence, which handles gpios and clocks currently, and can cover regulator and pinctrl in future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off if only one power sequence is needed, else call of_pwrseq_on_list /of_pwrseq_off_list instead (eg, USB hub driver). Signed-off-by: Peter ChenTested-by Joshua Clayton Reviewed-by: Matthias Kaehlcke Tested-by: Matthias Kaehlcke --- MAINTAINERS | 9 ++ drivers/power/Kconfig | 1 + drivers/power/Makefile | 1 + drivers/power/pwrseq/Kconfig| 45 ++ drivers/power/pwrseq/Makefile | 3 + drivers/power/pwrseq/core.c | 190 drivers/power/pwrseq/pwrseq_compatible_sample.c | 178 ++ drivers/power/pwrseq/pwrseq_generic.c | 177 ++ include/linux/power/pwrseq.h| 73 + 9 files changed, 677 insertions(+) create mode 100644 drivers/power/pwrseq/Kconfig create mode 100644 drivers/power/pwrseq/Makefile create mode 100644 drivers/power/pwrseq/core.c create mode 100644 drivers/power/pwrseq/pwrseq_compatible_sample.c create mode 100644 drivers/power/pwrseq/pwrseq_generic.c create mode 100644 include/linux/power/pwrseq.h diff --git a/MAINTAINERS b/MAINTAINERS index b3e9395..b353769 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9343,6 +9343,15 @@ F: include/linux/pm_* F: include/linux/powercap.h F: drivers/powercap/ +POWER SEQUENCE LIBRARY +M: Peter Chen +T: git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git +L: linux...@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/power/pwrseq/ +F: drivers/power/pwrseq/ +F: include/linux/power/pwrseq.h/ + POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS M: Sebastian Reichel M: Dmitry Eremin-Solenikov diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index acd4a15..f6aa4fd 100644 --- a/drivers/power/Kconfig +++ b/drivers/power/Kconfig @@ -515,3 +515,4 @@ endif # POWER_SUPPLY source "drivers/power/reset/Kconfig" source "drivers/power/avs/Kconfig" +source "drivers/power/pwrseq/Kconfig" diff --git a/drivers/power/Makefile b/drivers/power/Makefile index e46b75d..4ed2e12 100644 --- a/drivers/power/Makefile +++ b/drivers/power/Makefile @@ -74,3 +74,4 @@ obj-$(CONFIG_CHARGER_TPS65217)+= tps65217_charger.o obj-$(CONFIG_POWER_RESET) += reset/ obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o +obj-$(CONFIG_POWER_SEQUENCE) += pwrseq/ diff --git a/drivers/power/pwrseq/Kconfig b/drivers/power/pwrseq/Kconfig new file mode 100644 index 000..dff5e35 --- /dev/null +++ b/drivers/power/pwrseq/Kconfig @@ -0,0 +1,45 @@ +# +# Power Sequence library +# + +config POWER_SEQUENCE + bool + +menu "Power Sequence Support" + +config PWRSEQ_GENERIC + bool "Generic power sequence control" + depends on OF + select POWER_SEQUENCE + help + It is used for drivers which needs to do power sequence + (eg, turn on clock, toggle reset gpio) before the related + devices can be found by hardware. This generic one can be + used for common power sequence control. + +config PWRSEQ_GENERIC_INSTANCE_NUMBER + int "Number of Generic Power Sequence Instance" + depends on PWRSEQ_GENERIC + range 1 10 + default 2 + help + Usually, there are not so many devices needs power sequence, we set two + as default value. + +config PWRSEQ_SAMPLE + bool "sample power sequence control using compatible string" + depends on OF + select POWER_SEQUENCE + help + It is a sample library which implements power sequence for device id, + it is an example purpose. + +config PWRSEQ_SAMPLE_INSTANCE_NUMBER + int "Number of Sample Power Sequence Instance" + depends on PWRSEQ_SAMPLE + range 1 5 + default 1 + help + Usually, this file is special for certain device, so the default for this number + is 1. +endmenu diff --git a/drivers/power/pwrseq/Makefile