[PATCH 0/4] mmc: pwrseq-simple: add various functionality for devices

[PATCH 0/4] mmc: pwrseq-simple: add various functionality for devices

[Matt Ranostay]

Primarily these additions are used with wifi cards that have an
power down pin, and timing constraints betweens assertions/deassertions.

Matt Ranostay (4):
  mmc: pwrseq-simple: add an optional pre-power-on-delay
  mmc: pwrseq-simple: add support for power down GPIO pins
  mmc: pwrseq-simple: allow inverting of off state logic
  mmc: pwrseq-simple: add disable-post-power-on option

 .../devicetree/bindings/mmc/mmc-pwrseq-simple.txt  |  8 ++++
 drivers/mmc/core/pwrseq_simple.c                   | 49 +++++++++++++++++-----
 2 files changed, 46 insertions(+), 11 deletions(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] mmc: pwrseq-simple: add an optional pre-power-on-delay

[Matt Ranostay]

Some devices need some time between asserting reset-gpios before can
receive sdio commands.

This commits adds a pre-power-on-delay-ms devicetree property to
mmc-pwrseq-simple for use with such devices.

Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
---
 Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 2 ++
 drivers/mmc/core/pwrseq_simple.c                            | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
index e25436861867..88826f7d1d38 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
@@ -16,6 +16,8 @@ Optional properties:
   See ../clocks/clock-bindings.txt for details.
 - clock-names : Must include the following entry:
   "ext_clock" (External clock provided to the card).
+- pre-power-on-delay-ms : Delay in ms before powering the card which
+	the reset-gpios (if any) are asserted
 - post-power-on-delay-ms : Delay in ms after powering the card and
 	de-asserting the reset-gpios (if any)
 
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index 1304160de168..cb2ba7b11383 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -26,6 +26,7 @@
 struct mmc_pwrseq_simple {
 	struct mmc_pwrseq pwrseq;
 	bool clk_enabled;
+	u32 pre_power_on_delay_ms;
 	u32 post_power_on_delay_ms;
 	struct clk *ext_clk;
 	struct gpio_descs *reset_gpios;
@@ -60,6 +61,9 @@ static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
 	}
 
 	mmc_pwrseq_simple_set_gpios_value(pwrseq, 1);
+
+	if (pwrseq->pre_power_on_delay_ms)
+		msleep(pwrseq->pre_power_on_delay_ms);
 }
 
 static void mmc_pwrseq_simple_post_power_on(struct mmc_host *host)
@@ -117,6 +121,9 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
 		return PTR_ERR(pwrseq->reset_gpios);
 	}
 
+	device_property_read_u32(dev, "pre-power-on-delay-ms",
+				 &pwrseq->pre_power_on_delay_ms);
+
 	device_property_read_u32(dev, "post-power-on-delay-ms",
 				 &pwrseq->post_power_on_delay_ms);
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] mmc: pwrseq-simple: add support for power down GPIO pins

[Matt Ranostay]

Some devices have additional pins that control power but need to
asserted separately from reset-gpios using a defined time delay.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
---
 .../devicetree/bindings/mmc/mmc-pwrseq-simple.txt  |  2 ++
 drivers/mmc/core/pwrseq_simple.c                   | 26 +++++++++++++---------
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
index 88826f7d1d38..703a714201d8 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
@@ -12,6 +12,8 @@ Optional properties:
 	at initialization and prior we start the power up procedure of the card.
 	They will be de-asserted right after the power has been provided to the
 	card.
+- pwrdn-gpios : contains a list of GPIO specifiers. The pwrdn GPIOs are asserted
+	at initialization and prior we start the power up procedure of the card.
 - clocks : Must contain an entry for the entry in clock-names.
   See ../clocks/clock-bindings.txt for details.
 - clock-names : Must include the following entry:
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index cb2ba7b11383..adf1f7161fe3 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -30,24 +30,23 @@ struct mmc_pwrseq_simple {
 	u32 post_power_on_delay_ms;
 	struct clk *ext_clk;
 	struct gpio_descs *reset_gpios;
+	struct gpio_descs *pwrdn_gpios;
 };
 
 #define to_pwrseq_simple(p) container_of(p, struct mmc_pwrseq_simple, pwrseq)
 
 static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq,
+					      struct gpio_descs *gpios,
 					      int value)
 {
-	struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
-
-	if (!IS_ERR(reset_gpios)) {
+	if (!IS_ERR(gpios)) {
 		int i;
-		int values[reset_gpios->ndescs];
+		int values[gpios->ndescs];
 
-		for (i = 0; i < reset_gpios->ndescs; i++)
+		for (i = 0; i < gpios->ndescs; i++)
 			values[i] = value;
 
-		gpiod_set_array_value_cansleep(
-			reset_gpios->ndescs, reset_gpios->desc, values);
+		gpiod_set_array_value_cansleep(gpios->ndescs, gpios->desc, values);
 	}
 }
 
@@ -60,17 +59,19 @@ static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
 		pwrseq->clk_enabled = true;
 	}
 
-	mmc_pwrseq_simple_set_gpios_value(pwrseq, 1);
+	mmc_pwrseq_simple_set_gpios_value(pwrseq, pwrseq->reset_gpios, 1);
 
 	if (pwrseq->pre_power_on_delay_ms)
 		msleep(pwrseq->pre_power_on_delay_ms);
+
+	mmc_pwrseq_simple_set_gpios_value(pwrseq, pwrseq->pwrdn_gpios, 1);
 }
 
 static void mmc_pwrseq_simple_post_power_on(struct mmc_host *host)
 {
 	struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
 
-	mmc_pwrseq_simple_set_gpios_value(pwrseq, 0);
+	mmc_pwrseq_simple_set_gpios_value(pwrseq, pwrseq->reset_gpios, 0);
 
 	if (pwrseq->post_power_on_delay_ms)
 		msleep(pwrseq->post_power_on_delay_ms);
@@ -80,12 +81,14 @@ static void mmc_pwrseq_simple_power_off(struct mmc_host *host)
 {
 	struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
 
-	mmc_pwrseq_simple_set_gpios_value(pwrseq, 1);
+	mmc_pwrseq_simple_set_gpios_value(pwrseq, pwrseq->reset_gpios, 1);
+	mmc_pwrseq_simple_set_gpios_value(pwrseq, pwrseq->pwrdn_gpios, 1);
 
 	if (!IS_ERR(pwrseq->ext_clk) && pwrseq->clk_enabled) {
 		clk_disable_unprepare(pwrseq->ext_clk);
 		pwrseq->clk_enabled = false;
 	}
+
 }
 
 static const struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
@@ -121,6 +124,9 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
 		return PTR_ERR(pwrseq->reset_gpios);
 	}
 
+	pwrseq->pwrdn_gpios = devm_gpiod_get_array(dev, "pwrdn",
+							GPIOD_OUT_HIGH);
+
 	device_property_read_u32(dev, "pre-power-on-delay-ms",
 				 &pwrseq->pre_power_on_delay_ms);
 
-- 
2.7.4

[PATCH 3/4] mmc: pwrseq-simple: allow inverting of off state logic

[Matt Ranostay]

Some devices need a logic level low instead of high to be in the
off state.

Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
---
 .../devicetree/bindings/mmc/mmc-pwrseq-simple.txt         |  2 ++
 drivers/mmc/core/pwrseq_simple.c                          | 15 +++++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
index 703a714201d8..bea306d772d1 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
@@ -22,6 +22,8 @@ Optional properties:
 	the reset-gpios (if any) are asserted
 - post-power-on-delay-ms : Delay in ms after powering the card and
 	de-asserting the reset-gpios (if any)
+- invert-off-state: Invert the power down state for the reset-gpios (if any)
+	and pwrdn-gpios (if any)
 
 Example:
 
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index adf1f7161fe3..857dde3e0456 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -26,6 +26,7 @@
 struct mmc_pwrseq_simple {
 	struct mmc_pwrseq pwrseq;
 	bool clk_enabled;
+	bool invert_off_state;
 	u32 pre_power_on_delay_ms;
 	u32 post_power_on_delay_ms;
 	struct clk *ext_clk;
@@ -80,9 +81,10 @@ static void mmc_pwrseq_simple_post_power_on(struct mmc_host *host)
 static void mmc_pwrseq_simple_power_off(struct mmc_host *host)
 {
 	struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
+	unsigned int off_state = !(pwrseq->invert_off_state);
 
-	mmc_pwrseq_simple_set_gpios_value(pwrseq, pwrseq->reset_gpios, 1);
-	mmc_pwrseq_simple_set_gpios_value(pwrseq, pwrseq->pwrdn_gpios, 1);
+	mmc_pwrseq_simple_set_gpios_value(pwrseq, pwrseq->reset_gpios, off_state);
+	mmc_pwrseq_simple_set_gpios_value(pwrseq, pwrseq->pwrdn_gpios, off_state);
 
 	if (!IS_ERR(pwrseq->ext_clk) && pwrseq->clk_enabled) {
 		clk_disable_unprepare(pwrseq->ext_clk);
@@ -107,6 +109,7 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
 {
 	struct mmc_pwrseq_simple *pwrseq;
 	struct device *dev = &pdev->dev;
+	bool invert_off_state = false;
 
 	pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
 	if (!pwrseq)
@@ -116,8 +119,11 @@ 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);
 
+	if (device_property_read_bool(dev, "invert-off-state"))
+		invert_off_state = true;
+
 	pwrseq->reset_gpios = devm_gpiod_get_array(dev, "reset",
-							GPIOD_OUT_HIGH);
+			invert_off_state ? GPIOD_OUT_LOW : GPIOD_OUT_HIGH);
 	if (IS_ERR(pwrseq->reset_gpios) &&
 	    PTR_ERR(pwrseq->reset_gpios) != -ENOENT &&
 	    PTR_ERR(pwrseq->reset_gpios) != -ENOSYS) {
@@ -125,7 +131,7 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
 	}
 
 	pwrseq->pwrdn_gpios = devm_gpiod_get_array(dev, "pwrdn",
-							GPIOD_OUT_HIGH);
+			invert_off_state ? GPIOD_OUT_LOW : GPIOD_OUT_HIGH);
 
 	device_property_read_u32(dev, "pre-power-on-delay-ms",
 				 &pwrseq->pre_power_on_delay_ms);
@@ -136,6 +142,7 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
 	pwrseq->pwrseq.dev = dev;
 	pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
 	pwrseq->pwrseq.owner = THIS_MODULE;
+	pwrseq->invert_off_state = invert_off_state;
 	platform_set_drvdata(pdev, pwrseq);
 
 	return mmc_pwrseq_register(&pwrseq->pwrseq);
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Matt Ranostay]

Add optional device-post-power-on parameters to disable post_power_on
function from being called since this breaks some device.

Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
---
 Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 2 ++
 drivers/mmc/core/pwrseq_simple.c                            | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
index bea306d772d1..09fa153f743e 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
@@ -24,6 +24,8 @@ Optional properties:
 	de-asserting the reset-gpios (if any)
 - invert-off-state: Invert the power down state for the reset-gpios (if any)
 	and pwrdn-gpios (if any)
+- disable-post-power-on : Avoid post_power_on function from being called since
+	this breaks some devices
 
 Example:
 
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index 857dde3e0456..d34084c51f9c 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -26,6 +26,7 @@
 struct mmc_pwrseq_simple {
 	struct mmc_pwrseq pwrseq;
 	bool clk_enabled;
+	bool disable_post_power_on;
 	bool invert_off_state;
 	u32 pre_power_on_delay_ms;
 	u32 post_power_on_delay_ms;
@@ -72,6 +73,9 @@ static void mmc_pwrseq_simple_post_power_on(struct mmc_host *host)
 {
 	struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
 
+	if (pwrseq->disable_post_power_on)
+		return;
+
 	mmc_pwrseq_simple_set_gpios_value(pwrseq, pwrseq->reset_gpios, 0);
 
 	if (pwrseq->post_power_on_delay_ms)
@@ -139,6 +143,9 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
 	device_property_read_u32(dev, "post-power-on-delay-ms",
 				 &pwrseq->post_power_on_delay_ms);
 
+	if (device_property_read_bool(dev, "disable-post-power-on"))
+		pwrseq->disable_post_power_on = true;
+
 	pwrseq->pwrseq.dev = dev;
 	pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
 	pwrseq->pwrseq.owner = THIS_MODULE;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] mmc: pwrseq-simple: add support for power down GPIO pins

[Ulf Hansson]

On 2 December 2016 at 07:17, Matt Ranostay <matt@ranostay.consulting> wrote:
> Some devices have additional pins that control power but need to
> asserted separately from reset-gpios using a defined time delay.
>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> ---
>  .../devicetree/bindings/mmc/mmc-pwrseq-simple.txt  |  2 ++
>  drivers/mmc/core/pwrseq_simple.c                   | 26 +++++++++++++---------
>  2 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> index 88826f7d1d38..703a714201d8 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> @@ -12,6 +12,8 @@ Optional properties:
>         at initialization and prior we start the power up procedure of the card.
>         They will be de-asserted right after the power has been provided to the
>         card.
> +- pwrdn-gpios : contains a list of GPIO specifiers. The pwrdn GPIOs are asserted
> +       at initialization and prior we start the power up procedure of the card.

I think "powerdown-gpios" would be better, as Rob earlier also suggested.

Moreover I think the DT doc change be a separate change. This comment
applies to the other changes in this series as well.

[...]

Kind regards
Uffe

Re: [PATCH 3/4] mmc: pwrseq-simple: allow inverting of off state logic

[Ulf Hansson]

On 2 December 2016 at 07:17, Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org> wrote:
> Some devices need a logic level low instead of high to be in the
> off state.
>
> Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
> ---
>  .../devicetree/bindings/mmc/mmc-pwrseq-simple.txt         |  2 ++
>  drivers/mmc/core/pwrseq_simple.c                          | 15 +++++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> index 703a714201d8..bea306d772d1 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> @@ -22,6 +22,8 @@ Optional properties:
>         the reset-gpios (if any) are asserted
>  - post-power-on-delay-ms : Delay in ms after powering the card and
>         de-asserting the reset-gpios (if any)
> +- invert-off-state: Invert the power down state for the reset-gpios (if any)
> +       and pwrdn-gpios (if any)

We already have DT bindings to describe GPIO pins as active high or
low. I think we should be able to use that instead, don't you think?

[...]

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Ulf Hansson]

On 2 December 2016 at 07:17, Matt Ranostay <matt@ranostay.consulting> wrote:
> Add optional device-post-power-on parameters to disable post_power_on
> function from being called since this breaks some device.
>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> ---
>  Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 2 ++
>  drivers/mmc/core/pwrseq_simple.c                            | 7 +++++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> index bea306d772d1..09fa153f743e 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> @@ -24,6 +24,8 @@ Optional properties:
>         de-asserting the reset-gpios (if any)
>  - invert-off-state: Invert the power down state for the reset-gpios (if any)
>         and pwrdn-gpios (if any)
> +- disable-post-power-on : Avoid post_power_on function from being called since
> +       this breaks some devices

This is a bit weird. I would like to avoid this, if possible.

Perhaps if you simply describe the sequence in detail for your device
we can figure out the best option together.

[...]

Kind regards
Uffe

Re: [PATCH 3/4] mmc: pwrseq-simple: allow inverting of off state logic

[Matt Ranostay]

Sent from my iPhone
> On Dec 1, 2016, at 22:57, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 
>> On 2 December 2016 at 07:17, Matt Ranostay <matt@ranostay.consulting> wrote:
>> Some devices need a logic level low instead of high to be in the
>> off state.
>> 
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>> ---
>> .../devicetree/bindings/mmc/mmc-pwrseq-simple.txt         |  2 ++
>> drivers/mmc/core/pwrseq_simple.c                          | 15 +++++++++++----
>> 2 files changed, 13 insertions(+), 4 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>> index 703a714201d8..bea306d772d1 100644
>> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>> @@ -22,6 +22,8 @@ Optional properties:
>>        the reset-gpios (if any) are asserted
>> - post-power-on-delay-ms : Delay in ms after powering the card and
>>        de-asserting the reset-gpios (if any)
>> +- invert-off-state: Invert the power down state for the reset-gpios (if any)
>> +       and pwrdn-gpios (if any)
> 
> We already have DT bindings to describe GPIO pins as active high or
> low. I think we should be able to use that instead, don't you think?

Ah issue is we are using inverse logic in the power down from anything else. But I guess we could read the gpios active high and low stats from device tree and deduce it from that

> 
> [...]
> 
> Kind regards
> Uffe

Re: [PATCH 3/4] mmc: pwrseq-simple: allow inverting of off state logic

[Matt Ranostay]

On Thu, Dec 1, 2016 at 11:28 PM, Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org> wrote:
>
>
>
>>> On Dec 1, 2016, at 22:57, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>
>>> On 2 December 2016 at 07:17, Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org> wrote:
>>> Some devices need a logic level low instead of high to be in the
>>> off state.
>>>
>>> Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
>>> Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
>>> ---
>>> .../devicetree/bindings/mmc/mmc-pwrseq-simple.txt         |  2 ++
>>> drivers/mmc/core/pwrseq_simple.c                          | 15 +++++++++++----
>>> 2 files changed, 13 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>> index 703a714201d8..bea306d772d1 100644
>>> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>> @@ -22,6 +22,8 @@ Optional properties:
>>>        the reset-gpios (if any) are asserted
>>> - post-power-on-delay-ms : Delay in ms after powering the card and
>>>        de-asserting the reset-gpios (if any)
>>> +- invert-off-state: Invert the power down state for the reset-gpios (if any)
>>> +       and pwrdn-gpios (if any)
>>
>> We already have DT bindings to describe GPIO pins as active high or
>> low. I think we should be able to use that instead, don't you think?
>
> Ah issue is we are using inverse logic in the power down from anything else. But I guess we could read the gpios active high and low stats from device tree and deduce it from that
>

BTW this quirkiness is which why me and Lindgren went the route of an
another pwrseq driver versus hacking the pwrseq-simple initially...

In any case we can change the active high or low in the gpio
descriptors but doesn't change the timing we need or the inverse state
for power down.

Thanks,

Matt
>
>>
>> [...]
>>
>> Kind regards
>> Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Matt Ranostay]

On Fri, Dec 2, 2016 at 12:28 AM, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 2 December 2016 at 08:22, Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org> wrote:
>>
>>
>>> On Dec 1, 2016, at 23:13, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>
>>>> On 2 December 2016 at 07:17, Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org> wrote:
>>>> Add optional device-post-power-on parameters to disable post_power_on
>>>> function from being called since this breaks some device.
>>>>
>>>> Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
>>>> Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
>>>> ---
>>>> Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 2 ++
>>>> drivers/mmc/core/pwrseq_simple.c                            | 7 +++++++
>>>> 2 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>>> index bea306d772d1..09fa153f743e 100644
>>>> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>>> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>>> @@ -24,6 +24,8 @@ Optional properties:
>>>>        de-asserting the reset-gpios (if any)
>>>> - invert-off-state: Invert the power down state for the reset-gpios (if any)
>>>>        and pwrdn-gpios (if any)
>>>> +- disable-post-power-on : Avoid post_power_on function from being called since
>>>> +       this breaks some devices
>>>
>>> This is a bit weird. I would like to avoid this, if possible.
>>>
>>> Perhaps if you simply describe the sequence in detail for your device
>>> we can figure out the best option together.
>>
>> Yeah I know it is a bit weird but we need to keep that reset pin high for at least some time after pre power on.   So any case it would be another property
>
> This went offlist, please resend.
>
> Moreover, please try to describe the sequence you need in detail
> according to your HW spec.
>

Yeah oops....

So basically we need to drive the reset and powerdown lines high with
a 300 milliseconds delay between both...
can't have the reset line low with post power on (pretty sure but
would need a delay anyway), and we need both reset + powerdown line
set low on powerdown.

So the power down sequence would need to be reversed for this
application in pwrseq-simple.

> Br
> Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Rob Herring]

On Fri, Dec 02, 2016 at 01:06:47AM -0800, Matt Ranostay wrote:
> On Fri, Dec 2, 2016 at 12:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On 2 December 2016 at 08:22, Matt Ranostay <matt@ranostay.consulting> wrote:
> >>
> >>
> >>> On Dec 1, 2016, at 23:13, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>>
> >>>> On 2 December 2016 at 07:17, Matt Ranostay <matt@ranostay.consulting> wrote:
> >>>> Add optional device-post-power-on parameters to disable post_power_on
> >>>> function from being called since this breaks some device.
> >>>>
> >>>> Cc: Tony Lindgren <tony@atomide.com>
> >>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> >>>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> >>>> ---
> >>>> Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 2 ++
> >>>> drivers/mmc/core/pwrseq_simple.c                            | 7 +++++++
> >>>> 2 files changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> >>>> index bea306d772d1..09fa153f743e 100644
> >>>> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> >>>> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> >>>> @@ -24,6 +24,8 @@ Optional properties:
> >>>>        de-asserting the reset-gpios (if any)
> >>>> - invert-off-state: Invert the power down state for the reset-gpios (if any)
> >>>>        and pwrdn-gpios (if any)
> >>>> +- disable-post-power-on : Avoid post_power_on function from being called since
> >>>> +       this breaks some devices
> >>>
> >>> This is a bit weird. I would like to avoid this, if possible.
> >>>
> >>> Perhaps if you simply describe the sequence in detail for your device
> >>> we can figure out the best option together.
> >>
> >> Yeah I know it is a bit weird but we need to keep that reset pin high for at least some time after pre power on.   So any case it would be another property
> >
> > This went offlist, please resend.
> >
> > Moreover, please try to describe the sequence you need in detail
> > according to your HW spec.
> >
> 
> Yeah oops....
> 
> So basically we need to drive the reset and powerdown lines high with
> a 300 milliseconds delay between both...
> can't have the reset line low with post power on (pretty sure but
> would need a delay anyway), and we need both reset + powerdown line
> set low on powerdown.
> 
> So the power down sequence would need to be reversed for this
> application in pwrseq-simple.

This sounds like you need a device specific sequence to me. Otherwise, 
write a language to describe any power control waveforms rather than 
trying to bolt on more and more properties. (Don't really go write a 
language.) 

Rob

Re: [PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Matt Ranostay]

On Fri, Dec 9, 2016 at 10:09 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Dec 02, 2016 at 01:06:47AM -0800, Matt Ranostay wrote:
>> On Fri, Dec 2, 2016 at 12:28 AM, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> > On 2 December 2016 at 08:22, Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org> wrote:
>> >>
>> >>
>> >>> On Dec 1, 2016, at 23:13, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> >>>
>> >>>> On 2 December 2016 at 07:17, Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org> wrote:
>> >>>> Add optional device-post-power-on parameters to disable post_power_on
>> >>>> function from being called since this breaks some device.
>> >>>>
>> >>>> Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
>> >>>> Cc: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> >>>> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
>> >>>> ---
>> >>>> Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 2 ++
>> >>>> drivers/mmc/core/pwrseq_simple.c                            | 7 +++++++
>> >>>> 2 files changed, 9 insertions(+)
>> >>>>
>> >>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>> >>>> index bea306d772d1..09fa153f743e 100644
>> >>>> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>> >>>> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>> >>>> @@ -24,6 +24,8 @@ Optional properties:
>> >>>>        de-asserting the reset-gpios (if any)
>> >>>> - invert-off-state: Invert the power down state for the reset-gpios (if any)
>> >>>>        and pwrdn-gpios (if any)
>> >>>> +- disable-post-power-on : Avoid post_power_on function from being called since
>> >>>> +       this breaks some devices
>> >>>
>> >>> This is a bit weird. I would like to avoid this, if possible.
>> >>>
>> >>> Perhaps if you simply describe the sequence in detail for your device
>> >>> we can figure out the best option together.
>> >>
>> >> Yeah I know it is a bit weird but we need to keep that reset pin high for at least some time after pre power on.   So any case it would be another property
>> >
>> > This went offlist, please resend.
>> >
>> > Moreover, please try to describe the sequence you need in detail
>> > according to your HW spec.
>> >
>>
>> Yeah oops....
>>
>> So basically we need to drive the reset and powerdown lines high with
>> a 300 milliseconds delay between both...
>> can't have the reset line low with post power on (pretty sure but
>> would need a delay anyway), and we need both reset + powerdown line
>> set low on powerdown.
>>
>> So the power down sequence would need to be reversed for this
>> application in pwrseq-simple.
>
> This sounds like you need a device specific sequence to me. Otherwise,
> write a language to describe any power control waveforms rather than
> trying to bolt on more and more properties. (Don't really go write a
> language.)
>

Yeah I no interest in that as well :).

So I'll resubmit with the patchset comments so far.

> Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Ulf Hansson]

On 9 December 2016 at 19:09, Rob Herring <robh@kernel.org> wrote:
> On Fri, Dec 02, 2016 at 01:06:47AM -0800, Matt Ranostay wrote:
>> On Fri, Dec 2, 2016 at 12:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> > On 2 December 2016 at 08:22, Matt Ranostay <matt@ranostay.consulting> wrote:
>> >>
>> >>
>> >>> On Dec 1, 2016, at 23:13, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> >>>
>> >>>> On 2 December 2016 at 07:17, Matt Ranostay <matt@ranostay.consulting> wrote:
>> >>>> Add optional device-post-power-on parameters to disable post_power_on
>> >>>> function from being called since this breaks some device.
>> >>>>
>> >>>> Cc: Tony Lindgren <tony@atomide.com>
>> >>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> >>>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>> >>>> ---
>> >>>> Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 2 ++
>> >>>> drivers/mmc/core/pwrseq_simple.c                            | 7 +++++++
>> >>>> 2 files changed, 9 insertions(+)
>> >>>>
>> >>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>> >>>> index bea306d772d1..09fa153f743e 100644
>> >>>> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>> >>>> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>> >>>> @@ -24,6 +24,8 @@ Optional properties:
>> >>>>        de-asserting the reset-gpios (if any)
>> >>>> - invert-off-state: Invert the power down state for the reset-gpios (if any)
>> >>>>        and pwrdn-gpios (if any)
>> >>>> +- disable-post-power-on : Avoid post_power_on function from being called since
>> >>>> +       this breaks some devices
>> >>>
>> >>> This is a bit weird. I would like to avoid this, if possible.
>> >>>
>> >>> Perhaps if you simply describe the sequence in detail for your device
>> >>> we can figure out the best option together.
>> >>
>> >> Yeah I know it is a bit weird but we need to keep that reset pin high for at least some time after pre power on.   So any case it would be another property
>> >
>> > This went offlist, please resend.
>> >
>> > Moreover, please try to describe the sequence you need in detail
>> > according to your HW spec.
>> >
>>
>> Yeah oops....
>>
>> So basically we need to drive the reset and powerdown lines high with
>> a 300 milliseconds delay between both...
>> can't have the reset line low with post power on (pretty sure but
>> would need a delay anyway), and we need both reset + powerdown line
>> set low on powerdown.
>>
>> So the power down sequence would need to be reversed for this
>> application in pwrseq-simple.
>
> This sounds like you need a device specific sequence to me. Otherwise,
> write a language to describe any power control waveforms rather than
> trying to bolt on more and more properties. (Don't really go write a
> language.)

Actually this isn't so device specific. The cw1200 wifi chip which is
being used with ux500 SoCs has a very similar sequence. Allow me to
check the details and get back.

Anyway, my point is that I think we can figure out a common sequence
to support these kind required sequences. That is to me preferred over
adding a new (two to support cw1200) device specific pwrseq driver.

Kind regards
Uffe

Re: [PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Matt Ranostay]

On Tue, Dec 13, 2016 at 12:01 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 9 December 2016 at 19:09, Rob Herring <robh@kernel.org> wrote:
>> On Fri, Dec 02, 2016 at 01:06:47AM -0800, Matt Ranostay wrote:
>>> On Fri, Dec 2, 2016 at 12:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> > On 2 December 2016 at 08:22, Matt Ranostay <matt@ranostay.consulting> wrote:
>>> >>
>>> >>
>>> >>> On Dec 1, 2016, at 23:13, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> >>>
>>> >>>> On 2 December 2016 at 07:17, Matt Ranostay <matt@ranostay.consulting> wrote:
>>> >>>> Add optional device-post-power-on parameters to disable post_power_on
>>> >>>> function from being called since this breaks some device.
>>> >>>>
>>> >>>> Cc: Tony Lindgren <tony@atomide.com>
>>> >>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>> >>>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>>> >>>> ---
>>> >>>> Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 2 ++
>>> >>>> drivers/mmc/core/pwrseq_simple.c                            | 7 +++++++
>>> >>>> 2 files changed, 9 insertions(+)
>>> >>>>
>>> >>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>> >>>> index bea306d772d1..09fa153f743e 100644
>>> >>>> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>> >>>> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>> >>>> @@ -24,6 +24,8 @@ Optional properties:
>>> >>>>        de-asserting the reset-gpios (if any)
>>> >>>> - invert-off-state: Invert the power down state for the reset-gpios (if any)
>>> >>>>        and pwrdn-gpios (if any)
>>> >>>> +- disable-post-power-on : Avoid post_power_on function from being called since
>>> >>>> +       this breaks some devices
>>> >>>
>>> >>> This is a bit weird. I would like to avoid this, if possible.
>>> >>>
>>> >>> Perhaps if you simply describe the sequence in detail for your device
>>> >>> we can figure out the best option together.
>>> >>
>>> >> Yeah I know it is a bit weird but we need to keep that reset pin high for at least some time after pre power on.   So any case it would be another property
>>> >
>>> > This went offlist, please resend.
>>> >
>>> > Moreover, please try to describe the sequence you need in detail
>>> > according to your HW spec.
>>> >
>>>
>>> Yeah oops....
>>>
>>> So basically we need to drive the reset and powerdown lines high with
>>> a 300 milliseconds delay between both...
>>> can't have the reset line low with post power on (pretty sure but
>>> would need a delay anyway), and we need both reset + powerdown line
>>> set low on powerdown.
>>>
>>> So the power down sequence would need to be reversed for this
>>> application in pwrseq-simple.
>>
>> This sounds like you need a device specific sequence to me. Otherwise,
>> write a language to describe any power control waveforms rather than
>> trying to bolt on more and more properties. (Don't really go write a
>> language.)
>
> Actually this isn't so device specific. The cw1200 wifi chip which is
> being used with ux500 SoCs has a very similar sequence. Allow me to
> check the details and get back.

Ok. It would be good to have something common than a bunch of one-off
solutions for sure.

>
> Anyway, my point is that I think we can figure out a common sequence
> to support these kind required sequences. That is to me preferred over
> adding a new (two to support cw1200) device specific pwrseq driver.
>
> Kind regards
> Uffe

Re: [PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Matt Ranostay]

On Wed, Dec 14, 2016 at 5:46 PM, Matt Ranostay <matt@ranostay.consulting> wrote:
> On Tue, Dec 13, 2016 at 12:01 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 9 December 2016 at 19:09, Rob Herring <robh@kernel.org> wrote:
>>> On Fri, Dec 02, 2016 at 01:06:47AM -0800, Matt Ranostay wrote:
>>>> On Fri, Dec 2, 2016 at 12:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> > On 2 December 2016 at 08:22, Matt Ranostay <matt@ranostay.consulting> wrote:
>>>> >>
>>>> >>
>>>> >>> On Dec 1, 2016, at 23:13, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> >>>
>>>> >>>> On 2 December 2016 at 07:17, Matt Ranostay <matt@ranostay.consulting> wrote:
>>>> >>>> Add optional device-post-power-on parameters to disable post_power_on
>>>> >>>> function from being called since this breaks some device.
>>>> >>>>
>>>> >>>> Cc: Tony Lindgren <tony@atomide.com>
>>>> >>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>>> >>>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>>>> >>>> ---
>>>> >>>> Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 2 ++
>>>> >>>> drivers/mmc/core/pwrseq_simple.c                            | 7 +++++++
>>>> >>>> 2 files changed, 9 insertions(+)
>>>> >>>>
>>>> >>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>>> >>>> index bea306d772d1..09fa153f743e 100644
>>>> >>>> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>>> >>>> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
>>>> >>>> @@ -24,6 +24,8 @@ Optional properties:
>>>> >>>>        de-asserting the reset-gpios (if any)
>>>> >>>> - invert-off-state: Invert the power down state for the reset-gpios (if any)
>>>> >>>>        and pwrdn-gpios (if any)
>>>> >>>> +- disable-post-power-on : Avoid post_power_on function from being called since
>>>> >>>> +       this breaks some devices
>>>> >>>
>>>> >>> This is a bit weird. I would like to avoid this, if possible.
>>>> >>>
>>>> >>> Perhaps if you simply describe the sequence in detail for your device
>>>> >>> we can figure out the best option together.
>>>> >>
>>>> >> Yeah I know it is a bit weird but we need to keep that reset pin high for at least some time after pre power on.   So any case it would be another property
>>>> >
>>>> > This went offlist, please resend.
>>>> >
>>>> > Moreover, please try to describe the sequence you need in detail
>>>> > according to your HW spec.
>>>> >
>>>>
>>>> Yeah oops....
>>>>
>>>> So basically we need to drive the reset and powerdown lines high with
>>>> a 300 milliseconds delay between both...
>>>> can't have the reset line low with post power on (pretty sure but
>>>> would need a delay anyway), and we need both reset + powerdown line
>>>> set low on powerdown.
>>>>
>>>> So the power down sequence would need to be reversed for this
>>>> application in pwrseq-simple.
>>>
>>> This sounds like you need a device specific sequence to me. Otherwise,
>>> write a language to describe any power control waveforms rather than
>>> trying to bolt on more and more properties. (Don't really go write a
>>> language.)
>>
>> Actually this isn't so device specific. The cw1200 wifi chip which is
>> being used with ux500 SoCs has a very similar sequence. Allow me to
>> check the details and get back.
>
> Ok. It would be good to have something common than a bunch of one-off
> solutions for sure.
>
>>
>> Anyway, my point is that I think we can figure out a common sequence
>> to support these kind required sequences. That is to me preferred over
>> adding a new (two to support cw1200) device specific pwrseq driver.
>>

So briefly looked at the cw1200 driver (I'm guessing the datasheet is
NDA since I couldn't find it) and it seems like it would be a good fit
to have a common pwrseq driver.

So minor things I noticed that the cw1200 device has different from
our SD8787 chipset.

* SD8787 has a "powerdown" line, and CW1200 has a "powerup" line.. I
know this is a simple logic inversion.
* CW1200 has a clock control line, that seems to be asserted between
the powerup/reset steps.

Would there need to be some per driver functions that could be
registered for edge cases like this?

>> Kind regards
>> Uffe

Re: [PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Linus Walleij]

On Mon, Dec 19, 2016 at 1:01 AM, Matt Ranostay <matt@ranostay.consulting> wrote:

> * SD8787 has a "powerdown" line, and CW1200 has a "powerup" line.. I
> know this is a simple logic inversion.

If this is a GPIO line, the GPIO subsystem can flag a line for
inverted logic. GPIO_ACTIVE_LOW from device tree for example.

Yours,
Linus Walleij

Re: [PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Matt Ranostay]

On Fri, Dec 30, 2016 at 12:05 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Mon, Dec 19, 2016 at 1:01 AM, Matt Ranostay <matt@ranostay.consulting> wrote:
>
>> * SD8787 has a "powerdown" line, and CW1200 has a "powerup" line.. I
>> know this is a simple logic inversion.
>
> If this is a GPIO line, the GPIO subsystem can flag a line for
> inverted logic. GPIO_ACTIVE_LOW from device tree for example.

Slight ping on Ulf on this thread :).

I do understand the inverted logic flag but that doesn't help if there
are different logic states between various chipsets.

>
> Yours,
> Linus Walleij

Re: [PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Ulf Hansson]

On 9 January 2017 at 05:53, Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org> wrote:
> On Fri, Dec 30, 2016 at 12:05 AM, Linus Walleij
> <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Mon, Dec 19, 2016 at 1:01 AM, Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org> wrote:
>>
>>> * SD8787 has a "powerdown" line, and CW1200 has a "powerup" line.. I
>>> know this is a simple logic inversion.
>>
>> If this is a GPIO line, the GPIO subsystem can flag a line for
>> inverted logic. GPIO_ACTIVE_LOW from device tree for example.
>
> Slight ping on Ulf on this thread :).

Thanks, sorry for the delay!

>
> I do understand the inverted logic flag but that doesn't help if there
> are different logic states between various chipsets.

For cw1200 (I looked at code from an old ST-Ericsson vendor tree), the
sequence is as follows:

1) Enable clock/power to the card/chip.
2) Assert GPIO pin. I assume this also can be done before the
clock/power is enabled.
3) Wait some time (~50ms).
4) De-assert GPIO pin.
5) Wait some time (~20ms)
6) Assert GPIO pin.
7) Wait some time (~32ms).

At power off, the GPIO pin is de-asserted and of course also the
clock/power is disabled. Just to make sure we have all the relevant
logic.

Looking at mmc pwrseq simple, perhaps we can extend this to deal with
GPIOs that needs to be *toggled*, as this is not just reset GPIOs.
Then we also need to deal with the delays in-between the toggling.

Thoughts?

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Matt Ranostay]

On Wed, Jan 11, 2017 at 2:55 PM, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 9 January 2017 at 05:53, Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org> wrote:
>> On Fri, Dec 30, 2016 at 12:05 AM, Linus Walleij
>> <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>> On Mon, Dec 19, 2016 at 1:01 AM, Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org> wrote:
>>>
>>>> * SD8787 has a "powerdown" line, and CW1200 has a "powerup" line.. I
>>>> know this is a simple logic inversion.
>>>
>>> If this is a GPIO line, the GPIO subsystem can flag a line for
>>> inverted logic. GPIO_ACTIVE_LOW from device tree for example.
>>
>> Slight ping on Ulf on this thread :).
>
> Thanks, sorry for the delay!
>
>>
>> I do understand the inverted logic flag but that doesn't help if there
>> are different logic states between various chipsets.
>
> For cw1200 (I looked at code from an old ST-Ericsson vendor tree), the
> sequence is as follows:
>
> 1) Enable clock/power to the card/chip.
> 2) Assert GPIO pin. I assume this also can be done before the
> clock/power is enabled.
> 3) Wait some time (~50ms).
> 4) De-assert GPIO pin.
> 5) Wait some time (~20ms)
> 6) Assert GPIO pin.
> 7) Wait some time (~32ms).
>
> At power off, the GPIO pin is de-asserted and of course also the
> clock/power is disabled. Just to make sure we have all the relevant
> logic.
>
> Looking at mmc pwrseq simple, perhaps we can extend this to deal with
> GPIOs that needs to be *toggled*, as this is not just reset GPIOs.
> Then we also need to deal with the delays in-between the toggling.

Wouldn't we need to have a per chip function for each device
supported? As well document gpios? I suspect we'd need an array of
gpios defined in device tree since different devices will likely have
various numbers of pins to toggle

- Matt

>
> Thoughts?
>
> Kind regards
> Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Ulf Hansson]

On 12 January 2017 at 05:43, Matt Ranostay <matt@ranostay.consulting> wrote:
> On Wed, Jan 11, 2017 at 2:55 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 9 January 2017 at 05:53, Matt Ranostay <matt@ranostay.consulting> wrote:
>>> On Fri, Dec 30, 2016 at 12:05 AM, Linus Walleij
>>> <linus.walleij@linaro.org> wrote:
>>>> On Mon, Dec 19, 2016 at 1:01 AM, Matt Ranostay <matt@ranostay.consulting> wrote:
>>>>
>>>>> * SD8787 has a "powerdown" line, and CW1200 has a "powerup" line.. I
>>>>> know this is a simple logic inversion.
>>>>
>>>> If this is a GPIO line, the GPIO subsystem can flag a line for
>>>> inverted logic. GPIO_ACTIVE_LOW from device tree for example.
>>>
>>> Slight ping on Ulf on this thread :).
>>
>> Thanks, sorry for the delay!
>>
>>>
>>> I do understand the inverted logic flag but that doesn't help if there
>>> are different logic states between various chipsets.
>>
>> For cw1200 (I looked at code from an old ST-Ericsson vendor tree), the
>> sequence is as follows:
>>
>> 1) Enable clock/power to the card/chip.
>> 2) Assert GPIO pin. I assume this also can be done before the
>> clock/power is enabled.
>> 3) Wait some time (~50ms).
>> 4) De-assert GPIO pin.
>> 5) Wait some time (~20ms)
>> 6) Assert GPIO pin.
>> 7) Wait some time (~32ms).
>>
>> At power off, the GPIO pin is de-asserted and of course also the
>> clock/power is disabled. Just to make sure we have all the relevant
>> logic.
>>
>> Looking at mmc pwrseq simple, perhaps we can extend this to deal with
>> GPIOs that needs to be *toggled*, as this is not just reset GPIOs.
>> Then we also need to deal with the delays in-between the toggling.
>
> Wouldn't we need to have a per chip function for each device
> supported? As well document gpios? I suspect we'd need an array of

I was hoping to avoid this, but the more I look at it, it seems inevitable.

> gpios defined in device tree since different devices will likely have
> various numbers of pins to toggle

Maybe we can think of a clever way to combine cw1200 with your case?
Perhaps a "mmc pwrseq toggle". :-) If not, please go ahead an repost a
device specific mmc pwrseq for your case.

I realize this would be a good opportunity for me to work on enabling
support for cw1200 on ux500, but as the implementation of the
clock/power control also needs to be up-streamed, I am quite far from
being done.

Kind regards
Uffe

Re: [PATCH 4/4] mmc: pwrseq-simple: add disable-post-power-on option

[Matt Ranostay]

On Thu, Jan 12, 2017 at 7:57 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 12 January 2017 at 05:43, Matt Ranostay <matt@ranostay.consulting> wrote:
>> On Wed, Jan 11, 2017 at 2:55 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 9 January 2017 at 05:53, Matt Ranostay <matt@ranostay.consulting> wrote:
>>>> On Fri, Dec 30, 2016 at 12:05 AM, Linus Walleij
>>>> <linus.walleij@linaro.org> wrote:
>>>>> On Mon, Dec 19, 2016 at 1:01 AM, Matt Ranostay <matt@ranostay.consulting> wrote:
>>>>>
>>>>>> * SD8787 has a "powerdown" line, and CW1200 has a "powerup" line.. I
>>>>>> know this is a simple logic inversion.
>>>>>
>>>>> If this is a GPIO line, the GPIO subsystem can flag a line for
>>>>> inverted logic. GPIO_ACTIVE_LOW from device tree for example.
>>>>
>>>> Slight ping on Ulf on this thread :).
>>>
>>> Thanks, sorry for the delay!
>>>
>>>>
>>>> I do understand the inverted logic flag but that doesn't help if there
>>>> are different logic states between various chipsets.
>>>
>>> For cw1200 (I looked at code from an old ST-Ericsson vendor tree), the
>>> sequence is as follows:
>>>
>>> 1) Enable clock/power to the card/chip.
>>> 2) Assert GPIO pin. I assume this also can be done before the
>>> clock/power is enabled.
>>> 3) Wait some time (~50ms).
>>> 4) De-assert GPIO pin.
>>> 5) Wait some time (~20ms)
>>> 6) Assert GPIO pin.
>>> 7) Wait some time (~32ms).
>>>
>>> At power off, the GPIO pin is de-asserted and of course also the
>>> clock/power is disabled. Just to make sure we have all the relevant
>>> logic.
>>>
>>> Looking at mmc pwrseq simple, perhaps we can extend this to deal with
>>> GPIOs that needs to be *toggled*, as this is not just reset GPIOs.
>>> Then we also need to deal with the delays in-between the toggling.
>>
>> Wouldn't we need to have a per chip function for each device
>> supported? As well document gpios? I suspect we'd need an array of
>
> I was hoping to avoid this, but the more I look at it, it seems inevitable.
>
>> gpios defined in device tree since different devices will likely have
>> various numbers of pins to toggle
>
> Maybe we can think of a clever way to combine cw1200 with your case?
> Perhaps a "mmc pwrseq toggle". :-) If not, please go ahead an repost a
> device specific mmc pwrseq for your case.

I think the best solution for now. We can merge the two solutions
latter down the road.

Thanks,

Matt

>
> I realize this would be a good opportunity for me to work on enabling
> support for cw1200 on ux500, but as the implementation of the
> clock/power control also needs to be up-streamed, I am quite far from
> being done.
>
> Kind regards
> Uffe