devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 4/4] ARM: dts: exynos5422-odroidxu3: add eMMC reset line
       [not found] <1422453595-21160-1-git-send-email-m.szyprowski@samsung.com>
@ 2015-01-28 13:59 ` Marek Szyprowski
       [not found] ` <1422453595-21160-2-git-send-email-m.szyprowski@samsung.com>
  1 sibling, 0 replies; 9+ messages in thread
From: Marek Szyprowski @ 2015-01-28 13:59 UTC (permalink / raw)
  To: linux-mmc, devicetree, linux-samsung-soc
  Cc: Marek Szyprowski, Kukjin Kim, Tobias Jakobi, Daniel Drake,
	Sebastian Reichel, Seungwon Jeon, Jaehoon Chung, Ulf Hansson,
	Chris Ball, Joonyoung Shim

This patch adds reset-gpios property to the eMMC slot, so the MMC driver
is able to properly reset eMMC card on system restart and thus fixes
system hang on software reboot.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/boot/dts/exynos5422-odroidxu3.dts | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos5422-odroidxu3.dts b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
index a519c863248d..bafbc4e19adc 100644
--- a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
+++ b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
@@ -298,13 +298,15 @@
 
 &mmc_0 {
 	status = "okay";
+	reset-gpios = <&gpd1 0 0>;
+	reset-invered;
 	broken-cd;
 	card-detect-delay = <200>;
 	samsung,dw-mshc-ciu-div = <3>;
 	samsung,dw-mshc-sdr-timing = <0 4>;
 	samsung,dw-mshc-ddr-timing = <0 2>;
 	pinctrl-names = "default";
-	pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus4 &sd0_bus8>;
+	pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus4 &sd0_bus8 &emmc_nrst>;
 	bus-width = <8>;
 	cap-mmc-highspeed;
 };
@@ -330,6 +332,15 @@
 	};
 };
 
+&pinctrl_1 {
+	emmc_nrst: emmc-nrst {
+		samsung,pins = "gpd1-0";
+		samsung,pin-function = <0>;
+		samsung,pin-pud = <0>;
+		samsung,pin-drv = <0>;
+	};
+};
+
 &usbdrd_dwc3_0 {
 	dr_mode = "host";
 };
-- 
1.9.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] mmc: core: add support for hardware reset gpio line
       [not found] ` <1422453595-21160-2-git-send-email-m.szyprowski@samsung.com>
@ 2015-01-28 14:24   ` Ulf Hansson
  2015-01-28 14:41     ` Tobias Jakobi
       [not found]     ` <CAPDyKFpowidghwadJS+gYP0EVo6gRUhAG8s14CMEbwatFadP2Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 2 replies; 9+ messages in thread
From: Ulf Hansson @ 2015-01-28 14:24 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-mmc, devicetree@vger.kernel.org, linux-samsung-soc,
	Kukjin Kim, Tobias Jakobi, Daniel Drake, Sebastian Reichel,
	Seungwon Jeon, Jaehoon Chung, Chris Ball, Joonyoung Shim

On 28 January 2015 at 14:59, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> There are boards (like Hardkernel's Odroid boards) on which eMMC card's
> reset line is connected to GPIO line instead of the hardware reset
> logic. In case of such boards, if first stage of bootloader is loaded
> from such eMMC card, before performing system reboot, additional reset
> of eMMC card is required to properly boot the whole system again. This
> patch adds code for handling reset gpio lines defined in device tree.


I don't follow the above sequence. Can you try to elaborate and
describe for what exact scenario we require the hardware reset to be
done?

Also, I wonder whether we could extend the mmc-pwrseq to cover your
case? Did you consider that as an option?

Kind regards
Uffe

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  Documentation/devicetree/bindings/mmc/mmc.txt |  35 +++++----
>  drivers/mmc/core/core.c                       |   2 +
>  drivers/mmc/core/host.c                       |  11 +++
>  drivers/mmc/core/slot-gpio.c                  | 101 +++++++++++++++++++++++++-
>  include/linux/mmc/slot-gpio.h                 |   7 ++
>  5 files changed, 139 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> index 438899e8829b..c952cdea0201 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
> @@ -21,6 +21,9 @@ Optional properties:
>    below for the case, when a GPIO is used for the CD line
>  - wp-inverted: when present, polarity on the WP line is inverted. See the note
>    below for the case, when a GPIO is used for the WP line
> +- reset-gpios: Specify GPIO for hardware reset of a card, see gpio binding
> +- reset-inverted: when present, polarity on the reset line is inverted. See
> +  the note below for the case, when a GPIO is used for the reset line
>  - max-frequency: maximum operating clock frequency
>  - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
>    this system, even if the controller claims it is.
> @@ -43,22 +46,24 @@ Optional properties:
>  - dsr: Value the card's (optional) Driver Stage Register (DSR) should be
>    programmed with. Valid range: [0 .. 0xffff].
>
> -*NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
> -polarity properties, we have to fix the meaning of the "normal" and "inverted"
> -line levels. We choose to follow the SDHCI standard, which specifies both those
> -lines as "active low." Therefore, using the "cd-inverted" property means, that
> -the CD line is active high, i.e. it is high, when a card is inserted. Similar
> -logic applies to the "wp-inverted" property.
> -
> -CD and WP lines can be implemented on the hardware in one of two ways: as GPIOs,
> -specified in cd-gpios and wp-gpios properties, or as dedicated pins. Polarity of
> -dedicated pins can be specified, using *-inverted properties. GPIO polarity can
> -also be specified using the OF_GPIO_ACTIVE_LOW flag. This creates an ambiguity
> -in the latter case. We choose to use the XOR logic for GPIO CD and WP lines.
> -This means, the two properties are "superimposed," for example leaving the
> +*NOTE* on CD, WP and RESET polarity. To use common for all SD/MMC host
> +controllers line polarity properties, we have to fix the meaning of the
> +"normal" and "inverted" line levels. We choose to follow the SDHCI
> +standard, which specifies both those lines as "active low." Therefore,
> +using the "cd-inverted" property means, that the CD line is active high,
> +i.e. it is high, when a card is inserted. Similar logic applies to the
> +"wp-inverted" and "reset-inverted" property.
> +
> +CD, WP and RESET lines can be implemented on the hardware in one of two
> +ways: as GPIOs, specified in cd-gpios and wp-gpios properties, or as
> +dedicated pins. Polarity of dedicated pins can be specified, using
> +*-inverted properties. GPIO polarity can also be specified using the
> +OF_GPIO_ACTIVE_LOW flag. This creates an ambiguity in the latter case.
> +We choose to use the XOR logic for GPIO CD and WP lines. This means, the
> +two properties are "superimposed," for example leaving the
>  OF_GPIO_ACTIVE_LOW flag clear and specifying the respective *-inverted
> -property results in a double-inversion and actually means the "normal" line
> -polarity is in effect.
> +property results in a double-inversion and actually means the "normal"
> +line polarity is in effect.
>
>  Optional SDIO properties:
>  - keep-power-in-suspend: Preserves card power during a suspend/resume cycle
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 1be7055548cb..b556c3b5d83d 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2520,6 +2520,7 @@ void mmc_start_host(struct mmc_host *host)
>                 mmc_power_off(host);
>         else
>                 mmc_power_up(host, host->ocr_avail);
> +       mmc_gpiod_request_restart_handler(host);
>         mmc_gpiod_request_cd_irq(host);
>         _mmc_detect_change(host, 0, false);
>  }
> @@ -2557,6 +2558,7 @@ void mmc_stop_host(struct mmc_host *host)
>
>         BUG_ON(host->card);
>
> +       mmc_gpiod_free_restart_handler(host);
>         mmc_power_off(host);
>  }
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 8be0df758e68..644d0f41b8f6 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -317,6 +317,7 @@ int mmc_of_parse(struct mmc_host *host)
>         int len, ret;
>         bool cd_cap_invert, cd_gpio_invert = false;
>         bool ro_cap_invert, ro_gpio_invert = false;
> +       bool rst_cap_invert;
>
>         if (!host->parent || !host->parent->of_node)
>                 return 0;
> @@ -404,6 +405,16 @@ int mmc_of_parse(struct mmc_host *host)
>         if (ro_cap_invert ^ ro_gpio_invert)
>                 host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
>
> +       /* Parse HW Reset */
> +       rst_cap_invert = of_property_read_bool(np, "reset-inverted");
> +
> +       ret = mmc_gpiod_request_rst(host, "reset", 0, rst_cap_invert);
> +       if (!ret) {
> +               dev_info(host->parent, "Got HW Reset GPIO\n");
> +               host->caps |= MMC_CAP_HW_RESET;
> +       } else if (ret != -ENOENT)
> +               return ret;
> +
>         if (of_find_property(np, "cap-sd-highspeed", &len))
>                 host->caps |= MMC_CAP_SD_HIGHSPEED;
>         if (of_find_property(np, "cap-mmc-highspeed", &len))
> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
> index 27117ba47073..5a2ca43e0e8d 100644
> --- a/drivers/mmc/core/slot-gpio.c
> +++ b/drivers/mmc/core/slot-gpio.c
> @@ -8,6 +8,7 @@
>   * published by the Free Software Foundation.
>   */
>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/gpio.h>
>  #include <linux/gpio/consumer.h>
> @@ -17,17 +18,22 @@
>  #include <linux/mmc/slot-gpio.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> +#include <linux/reboot.h>
>
>  #include "slot-gpio.h"
>
>  struct mmc_gpio {
>         struct gpio_desc *ro_gpio;
>         struct gpio_desc *cd_gpio;
> +       struct gpio_desc *rst_gpio;
>         bool override_ro_active_level;
>         bool override_cd_active_level;
> +       bool rst_raw_active_level;
> +       struct notifier_block rst_nb;
>         irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id);
>         char *ro_label;
> -       char cd_label[0];
> +       char *cd_label;
> +       char rst_label[0];
>  };
>
>  static irqreturn_t mmc_gpio_cd_irqt(int irq, void *dev_id)
> @@ -45,12 +51,14 @@ int mmc_gpio_alloc(struct mmc_host *host)
>  {
>         size_t len = strlen(dev_name(host->parent)) + 4;
>         struct mmc_gpio *ctx = devm_kzalloc(host->parent,
> -                               sizeof(*ctx) + 2 * len, GFP_KERNEL);
> +                               sizeof(*ctx) + 3 * len + 1, GFP_KERNEL);
>
>         if (ctx) {
> +               ctx->cd_label = ctx->rst_label + len + 1;
>                 ctx->ro_label = ctx->cd_label + len;
>                 snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent));
>                 snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent));
> +               snprintf(ctx->rst_label, len, "%s rst", dev_name(host->parent));
>                 host->slot.handler_priv = ctx;
>                 host->slot.cd_irq = -EINVAL;
>         }
> @@ -88,6 +96,21 @@ int mmc_gpio_get_cd(struct mmc_host *host)
>  }
>  EXPORT_SYMBOL(mmc_gpio_get_cd);
>
> +void mmc_gpio_do_hw_reset(struct mmc_host *host)
> +{
> +       struct mmc_gpio *ctx = host->slot.handler_priv;
> +       if (!ctx || !ctx->rst_gpio)
> +               return;
> +
> +       gpiod_set_raw_value_cansleep(ctx->rst_gpio, ctx->rst_raw_active_level);
> +       /* For eMMC, minimum is 1us but give it 10us for good measure */
> +       udelay(10);
> +       gpiod_set_raw_value_cansleep(ctx->rst_gpio, !ctx->rst_raw_active_level);
> +       /* For eMMC, minimum is 200us but give it 300us for good measure */
> +       usleep_range(300, 1000);
> +}
> +EXPORT_SYMBOL(mmc_gpio_do_hw_reset);
> +
>  /**
>   * mmc_gpio_request_ro - request a gpio for write-protection
>   * @host: mmc host
> @@ -167,6 +190,48 @@ void mmc_gpio_set_cd_isr(struct mmc_host *host,
>  }
>  EXPORT_SYMBOL(mmc_gpio_set_cd_isr);
>
> +static int mmc_gpio_rst_nb(struct notifier_block *this, unsigned long mode,
> +                          void *cmd)
> +{
> +       struct mmc_gpio *ctx = container_of(this, struct mmc_gpio, rst_nb);
> +
> +       gpiod_set_raw_value(ctx->rst_gpio, ctx->rst_raw_active_level);
> +       /* For eMMC, minimum is 1us but give it 10us for good measure */
> +       udelay(10);
> +       gpiod_set_raw_value(ctx->rst_gpio, !ctx->rst_raw_active_level);
> +       /* For eMMC, minimum is 200us but give it 300us for good measure */
> +       udelay(300);
> +
> +       return NOTIFY_DONE;
> +}
> +
> +void mmc_gpiod_request_restart_handler(struct mmc_host *host)
> +{
> +       struct mmc_gpio *ctx = host->slot.handler_priv;
> +
> +       if (!ctx || !ctx->rst_gpio)
> +               return;
> +
> +       ctx->rst_nb.notifier_call = mmc_gpio_rst_nb;
> +       ctx->rst_nb.priority = 255;
> +
> +       register_restart_handler(&ctx->rst_nb);
> +       return;
> +}
> +EXPORT_SYMBOL(mmc_gpiod_request_restart_handler);
> +
> +void mmc_gpiod_free_restart_handler(struct mmc_host *host)
> +{
> +       struct mmc_gpio *ctx = host->slot.handler_priv;
> +
> +       if (!ctx || !ctx->rst_gpio)
> +               return;
> +
> +       unregister_restart_handler(&ctx->rst_nb);
> +       return;
> +}
> +EXPORT_SYMBOL(mmc_gpiod_free_restart_handler);
> +
>  /**
>   * mmc_gpio_request_cd - request a gpio for card-detection
>   * @host: mmc host
> @@ -303,3 +368,35 @@ int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id,
>         return 0;
>  }
>  EXPORT_SYMBOL(mmc_gpiod_request_ro);
> +
> +/**
> + * mmc_gpiod_request_rst - request a gpio descriptor for hw reset
> + * @host: mmc host
> + * @con_id: function within the GPIO consumer
> + * @idx: index of the GPIO to obtain in the consumer
> + * @inverted: indicated the polarity of reset line should be inverted
> + * (xored with GPIO_ACTIVE_LOW flag of gpio descriptor)
> + *
> + * Returns zero on success, else an error.
> + */
> +int mmc_gpiod_request_rst(struct mmc_host *host, const char *con_id,
> +                         unsigned int idx, bool inverted)
> +{
> +       struct mmc_gpio *ctx = host->slot.handler_priv;
> +       struct gpio_desc *desc;
> +
> +       if (!con_id)
> +               con_id = ctx->rst_label;
> +
> +       desc = devm_gpiod_get_index(host->parent, con_id, idx);
> +       if (IS_ERR(desc))
> +               return PTR_ERR(desc);
> +
> +       ctx->rst_raw_active_level = !inverted ^ gpiod_is_active_low(desc);
> +       ctx->rst_gpio = desc;
> +
> +       gpiod_direction_output_raw(desc, !ctx->rst_raw_active_level);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(mmc_gpiod_request_rst);
> diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h
> index 3945a8c9d3cb..48f814e1bbeb 100644
> --- a/include/linux/mmc/slot-gpio.h
> +++ b/include/linux/mmc/slot-gpio.h
> @@ -26,8 +26,15 @@ int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id,
>  int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id,
>                          unsigned int idx, bool override_active_level,
>                          unsigned int debounce, bool *gpio_invert);
> +int mmc_gpiod_request_rst(struct mmc_host *host, const char *con_id,
> +                        unsigned int idx, bool inverted);
>  void mmc_gpio_set_cd_isr(struct mmc_host *host,
>                          irqreturn_t (*isr)(int irq, void *dev_id));
>  void mmc_gpiod_request_cd_irq(struct mmc_host *host);
>
> +void mmc_gpio_do_hw_reset(struct mmc_host *host);
> +void mmc_gpiod_request_restart_handler(struct mmc_host *host);
> +void mmc_gpiod_free_restart_handler(struct mmc_host *host);
> +
> +
>  #endif
> --
> 1.9.2
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] mmc: core: add support for hardware reset gpio line
  2015-01-28 14:24   ` [PATCH 1/4] mmc: core: add support for hardware reset gpio line Ulf Hansson
@ 2015-01-28 14:41     ` Tobias Jakobi
       [not found]     ` <CAPDyKFpowidghwadJS+gYP0EVo6gRUhAG8s14CMEbwatFadP2Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 0 replies; 9+ messages in thread
From: Tobias Jakobi @ 2015-01-28 14:41 UTC (permalink / raw)
  To: Ulf Hansson, Marek Szyprowski
  Cc: linux-mmc, devicetree@vger.kernel.org, linux-samsung-soc,
	Kukjin Kim, Daniel Drake, Sebastian Reichel, Seungwon Jeon,
	Jaehoon Chung, Chris Ball, Joonyoung Shim

Hello!

Ulf Hansson wrote:
> On 28 January 2015 at 14:59, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> There are boards (like Hardkernel's Odroid boards) on which eMMC card's
>> reset line is connected to GPIO line instead of the hardware reset
>> logic. In case of such boards, if first stage of bootloader is loaded
>> from such eMMC card, before performing system reboot, additional reset
>> of eMMC card is required to properly boot the whole system again. This
>> patch adds code for handling reset gpio lines defined in device tree.
> 
> 
> I don't follow the above sequence. Can you try to elaborate and
> describe for what exact scenario we require the hardware reset to be
> done?
The Odroid board can either 'boot' from the eMMC or from a SD card. By
'boot', I mean that the iROM in the SoC either sources the bootloader
data (BL1, BL2, trustzone, etc.) from eMMC or SD card.
Let's take a board that is configured to boot from eMMC. If we reboot
that board without doing a hw reset to the eMMC, then the iROM won't be
able to read the bootloader when the board comes up again. Hence the
need to do this reset during the kernel reboot procedure. Doing this
during kernel initialization is already too late.


With best wishes,
Tobias


> Also, I wonder whether we could extend the mmc-pwrseq to cover your
> case? Did you consider that as an option?
> 
> Kind regards
> Uffe
> 
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>  Documentation/devicetree/bindings/mmc/mmc.txt |  35 +++++----
>>  drivers/mmc/core/core.c                       |   2 +
>>  drivers/mmc/core/host.c                       |  11 +++
>>  drivers/mmc/core/slot-gpio.c                  | 101 +++++++++++++++++++++++++-
>>  include/linux/mmc/slot-gpio.h                 |   7 ++
>>  5 files changed, 139 insertions(+), 17 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
>> index 438899e8829b..c952cdea0201 100644
>> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
>> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
>> @@ -21,6 +21,9 @@ Optional properties:
>>    below for the case, when a GPIO is used for the CD line
>>  - wp-inverted: when present, polarity on the WP line is inverted. See the note
>>    below for the case, when a GPIO is used for the WP line
>> +- reset-gpios: Specify GPIO for hardware reset of a card, see gpio binding
>> +- reset-inverted: when present, polarity on the reset line is inverted. See
>> +  the note below for the case, when a GPIO is used for the reset line
>>  - max-frequency: maximum operating clock frequency
>>  - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
>>    this system, even if the controller claims it is.
>> @@ -43,22 +46,24 @@ Optional properties:
>>  - dsr: Value the card's (optional) Driver Stage Register (DSR) should be
>>    programmed with. Valid range: [0 .. 0xffff].
>>
>> -*NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
>> -polarity properties, we have to fix the meaning of the "normal" and "inverted"
>> -line levels. We choose to follow the SDHCI standard, which specifies both those
>> -lines as "active low." Therefore, using the "cd-inverted" property means, that
>> -the CD line is active high, i.e. it is high, when a card is inserted. Similar
>> -logic applies to the "wp-inverted" property.
>> -
>> -CD and WP lines can be implemented on the hardware in one of two ways: as GPIOs,
>> -specified in cd-gpios and wp-gpios properties, or as dedicated pins. Polarity of
>> -dedicated pins can be specified, using *-inverted properties. GPIO polarity can
>> -also be specified using the OF_GPIO_ACTIVE_LOW flag. This creates an ambiguity
>> -in the latter case. We choose to use the XOR logic for GPIO CD and WP lines.
>> -This means, the two properties are "superimposed," for example leaving the
>> +*NOTE* on CD, WP and RESET polarity. To use common for all SD/MMC host
>> +controllers line polarity properties, we have to fix the meaning of the
>> +"normal" and "inverted" line levels. We choose to follow the SDHCI
>> +standard, which specifies both those lines as "active low." Therefore,
>> +using the "cd-inverted" property means, that the CD line is active high,
>> +i.e. it is high, when a card is inserted. Similar logic applies to the
>> +"wp-inverted" and "reset-inverted" property.
>> +
>> +CD, WP and RESET lines can be implemented on the hardware in one of two
>> +ways: as GPIOs, specified in cd-gpios and wp-gpios properties, or as
>> +dedicated pins. Polarity of dedicated pins can be specified, using
>> +*-inverted properties. GPIO polarity can also be specified using the
>> +OF_GPIO_ACTIVE_LOW flag. This creates an ambiguity in the latter case.
>> +We choose to use the XOR logic for GPIO CD and WP lines. This means, the
>> +two properties are "superimposed," for example leaving the
>>  OF_GPIO_ACTIVE_LOW flag clear and specifying the respective *-inverted
>> -property results in a double-inversion and actually means the "normal" line
>> -polarity is in effect.
>> +property results in a double-inversion and actually means the "normal"
>> +line polarity is in effect.
>>
>>  Optional SDIO properties:
>>  - keep-power-in-suspend: Preserves card power during a suspend/resume cycle
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 1be7055548cb..b556c3b5d83d 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -2520,6 +2520,7 @@ void mmc_start_host(struct mmc_host *host)
>>                 mmc_power_off(host);
>>         else
>>                 mmc_power_up(host, host->ocr_avail);
>> +       mmc_gpiod_request_restart_handler(host);
>>         mmc_gpiod_request_cd_irq(host);
>>         _mmc_detect_change(host, 0, false);
>>  }
>> @@ -2557,6 +2558,7 @@ void mmc_stop_host(struct mmc_host *host)
>>
>>         BUG_ON(host->card);
>>
>> +       mmc_gpiod_free_restart_handler(host);
>>         mmc_power_off(host);
>>  }
>>
>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>> index 8be0df758e68..644d0f41b8f6 100644
>> --- a/drivers/mmc/core/host.c
>> +++ b/drivers/mmc/core/host.c
>> @@ -317,6 +317,7 @@ int mmc_of_parse(struct mmc_host *host)
>>         int len, ret;
>>         bool cd_cap_invert, cd_gpio_invert = false;
>>         bool ro_cap_invert, ro_gpio_invert = false;
>> +       bool rst_cap_invert;
>>
>>         if (!host->parent || !host->parent->of_node)
>>                 return 0;
>> @@ -404,6 +405,16 @@ int mmc_of_parse(struct mmc_host *host)
>>         if (ro_cap_invert ^ ro_gpio_invert)
>>                 host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
>>
>> +       /* Parse HW Reset */
>> +       rst_cap_invert = of_property_read_bool(np, "reset-inverted");
>> +
>> +       ret = mmc_gpiod_request_rst(host, "reset", 0, rst_cap_invert);
>> +       if (!ret) {
>> +               dev_info(host->parent, "Got HW Reset GPIO\n");
>> +               host->caps |= MMC_CAP_HW_RESET;
>> +       } else if (ret != -ENOENT)
>> +               return ret;
>> +
>>         if (of_find_property(np, "cap-sd-highspeed", &len))
>>                 host->caps |= MMC_CAP_SD_HIGHSPEED;
>>         if (of_find_property(np, "cap-mmc-highspeed", &len))
>> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
>> index 27117ba47073..5a2ca43e0e8d 100644
>> --- a/drivers/mmc/core/slot-gpio.c
>> +++ b/drivers/mmc/core/slot-gpio.c
>> @@ -8,6 +8,7 @@
>>   * published by the Free Software Foundation.
>>   */
>>
>> +#include <linux/delay.h>
>>  #include <linux/err.h>
>>  #include <linux/gpio.h>
>>  #include <linux/gpio/consumer.h>
>> @@ -17,17 +18,22 @@
>>  #include <linux/mmc/slot-gpio.h>
>>  #include <linux/module.h>
>>  #include <linux/slab.h>
>> +#include <linux/reboot.h>
>>
>>  #include "slot-gpio.h"
>>
>>  struct mmc_gpio {
>>         struct gpio_desc *ro_gpio;
>>         struct gpio_desc *cd_gpio;
>> +       struct gpio_desc *rst_gpio;
>>         bool override_ro_active_level;
>>         bool override_cd_active_level;
>> +       bool rst_raw_active_level;
>> +       struct notifier_block rst_nb;
>>         irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id);
>>         char *ro_label;
>> -       char cd_label[0];
>> +       char *cd_label;
>> +       char rst_label[0];
>>  };
>>
>>  static irqreturn_t mmc_gpio_cd_irqt(int irq, void *dev_id)
>> @@ -45,12 +51,14 @@ int mmc_gpio_alloc(struct mmc_host *host)
>>  {
>>         size_t len = strlen(dev_name(host->parent)) + 4;
>>         struct mmc_gpio *ctx = devm_kzalloc(host->parent,
>> -                               sizeof(*ctx) + 2 * len, GFP_KERNEL);
>> +                               sizeof(*ctx) + 3 * len + 1, GFP_KERNEL);
>>
>>         if (ctx) {
>> +               ctx->cd_label = ctx->rst_label + len + 1;
>>                 ctx->ro_label = ctx->cd_label + len;
>>                 snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent));
>>                 snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent));
>> +               snprintf(ctx->rst_label, len, "%s rst", dev_name(host->parent));
>>                 host->slot.handler_priv = ctx;
>>                 host->slot.cd_irq = -EINVAL;
>>         }
>> @@ -88,6 +96,21 @@ int mmc_gpio_get_cd(struct mmc_host *host)
>>  }
>>  EXPORT_SYMBOL(mmc_gpio_get_cd);
>>
>> +void mmc_gpio_do_hw_reset(struct mmc_host *host)
>> +{
>> +       struct mmc_gpio *ctx = host->slot.handler_priv;
>> +       if (!ctx || !ctx->rst_gpio)
>> +               return;
>> +
>> +       gpiod_set_raw_value_cansleep(ctx->rst_gpio, ctx->rst_raw_active_level);
>> +       /* For eMMC, minimum is 1us but give it 10us for good measure */
>> +       udelay(10);
>> +       gpiod_set_raw_value_cansleep(ctx->rst_gpio, !ctx->rst_raw_active_level);
>> +       /* For eMMC, minimum is 200us but give it 300us for good measure */
>> +       usleep_range(300, 1000);
>> +}
>> +EXPORT_SYMBOL(mmc_gpio_do_hw_reset);
>> +
>>  /**
>>   * mmc_gpio_request_ro - request a gpio for write-protection
>>   * @host: mmc host
>> @@ -167,6 +190,48 @@ void mmc_gpio_set_cd_isr(struct mmc_host *host,
>>  }
>>  EXPORT_SYMBOL(mmc_gpio_set_cd_isr);
>>
>> +static int mmc_gpio_rst_nb(struct notifier_block *this, unsigned long mode,
>> +                          void *cmd)
>> +{
>> +       struct mmc_gpio *ctx = container_of(this, struct mmc_gpio, rst_nb);
>> +
>> +       gpiod_set_raw_value(ctx->rst_gpio, ctx->rst_raw_active_level);
>> +       /* For eMMC, minimum is 1us but give it 10us for good measure */
>> +       udelay(10);
>> +       gpiod_set_raw_value(ctx->rst_gpio, !ctx->rst_raw_active_level);
>> +       /* For eMMC, minimum is 200us but give it 300us for good measure */
>> +       udelay(300);
>> +
>> +       return NOTIFY_DONE;
>> +}
>> +
>> +void mmc_gpiod_request_restart_handler(struct mmc_host *host)
>> +{
>> +       struct mmc_gpio *ctx = host->slot.handler_priv;
>> +
>> +       if (!ctx || !ctx->rst_gpio)
>> +               return;
>> +
>> +       ctx->rst_nb.notifier_call = mmc_gpio_rst_nb;
>> +       ctx->rst_nb.priority = 255;
>> +
>> +       register_restart_handler(&ctx->rst_nb);
>> +       return;
>> +}
>> +EXPORT_SYMBOL(mmc_gpiod_request_restart_handler);
>> +
>> +void mmc_gpiod_free_restart_handler(struct mmc_host *host)
>> +{
>> +       struct mmc_gpio *ctx = host->slot.handler_priv;
>> +
>> +       if (!ctx || !ctx->rst_gpio)
>> +               return;
>> +
>> +       unregister_restart_handler(&ctx->rst_nb);
>> +       return;
>> +}
>> +EXPORT_SYMBOL(mmc_gpiod_free_restart_handler);
>> +
>>  /**
>>   * mmc_gpio_request_cd - request a gpio for card-detection
>>   * @host: mmc host
>> @@ -303,3 +368,35 @@ int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id,
>>         return 0;
>>  }
>>  EXPORT_SYMBOL(mmc_gpiod_request_ro);
>> +
>> +/**
>> + * mmc_gpiod_request_rst - request a gpio descriptor for hw reset
>> + * @host: mmc host
>> + * @con_id: function within the GPIO consumer
>> + * @idx: index of the GPIO to obtain in the consumer
>> + * @inverted: indicated the polarity of reset line should be inverted
>> + * (xored with GPIO_ACTIVE_LOW flag of gpio descriptor)
>> + *
>> + * Returns zero on success, else an error.
>> + */
>> +int mmc_gpiod_request_rst(struct mmc_host *host, const char *con_id,
>> +                         unsigned int idx, bool inverted)
>> +{
>> +       struct mmc_gpio *ctx = host->slot.handler_priv;
>> +       struct gpio_desc *desc;
>> +
>> +       if (!con_id)
>> +               con_id = ctx->rst_label;
>> +
>> +       desc = devm_gpiod_get_index(host->parent, con_id, idx);
>> +       if (IS_ERR(desc))
>> +               return PTR_ERR(desc);
>> +
>> +       ctx->rst_raw_active_level = !inverted ^ gpiod_is_active_low(desc);
>> +       ctx->rst_gpio = desc;
>> +
>> +       gpiod_direction_output_raw(desc, !ctx->rst_raw_active_level);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(mmc_gpiod_request_rst);
>> diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h
>> index 3945a8c9d3cb..48f814e1bbeb 100644
>> --- a/include/linux/mmc/slot-gpio.h
>> +++ b/include/linux/mmc/slot-gpio.h
>> @@ -26,8 +26,15 @@ int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id,
>>  int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id,
>>                          unsigned int idx, bool override_active_level,
>>                          unsigned int debounce, bool *gpio_invert);
>> +int mmc_gpiod_request_rst(struct mmc_host *host, const char *con_id,
>> +                        unsigned int idx, bool inverted);
>>  void mmc_gpio_set_cd_isr(struct mmc_host *host,
>>                          irqreturn_t (*isr)(int irq, void *dev_id));
>>  void mmc_gpiod_request_cd_irq(struct mmc_host *host);
>>
>> +void mmc_gpio_do_hw_reset(struct mmc_host *host);
>> +void mmc_gpiod_request_restart_handler(struct mmc_host *host);
>> +void mmc_gpiod_free_restart_handler(struct mmc_host *host);
>> +
>> +
>>  #endif
>> --
>> 1.9.2
>>
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] mmc: core: add support for hardware reset gpio line
       [not found]     ` <CAPDyKFpowidghwadJS+gYP0EVo6gRUhAG8s14CMEbwatFadP2Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2015-01-29  9:15       ` Marek Szyprowski
       [not found]         ` <54C9FA48.5050705-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
  0 siblings, 1 reply; 9+ messages in thread
From: Marek Szyprowski @ 2015-01-29  9:15 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: linux-mmc, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-samsung-soc, Kukjin Kim, Tobias Jakobi, Daniel Drake,
	Sebastian Reichel, Seungwon Jeon, Jaehoon Chung, Chris Ball,
	Joonyoung Shim

Hello,

On 2015-01-28 15:24, Ulf Hansson wrote:
> On 28 January 2015 at 14:59, Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>> There are boards (like Hardkernel's Odroid boards) on which eMMC card's
>> reset line is connected to GPIO line instead of the hardware reset
>> logic. In case of such boards, if first stage of bootloader is loaded
>> from such eMMC card, before performing system reboot, additional reset
>> of eMMC card is required to properly boot the whole system again. This
>> patch adds code for handling reset gpio lines defined in device tree.
>
> I don't follow the above sequence. Can you try to elaborate and
> describe for what exact scenario we require the hardware reset to be
> done?

Odroid boards uses multi stage bootloaders. The very first stage is in 
SoC ROM.
That code loads next stages (called bl1, bl2, tz) from either eMMC or SD 
card
(depending on jumper configuration or some card detect pins). This ROM 
code is
very simple and assumes that the mmc card has been reset to the default
configuration. However, eMMC card is not being reset by the board 
'reboot' logic.
Instead the nreset line from emmc card is connected to gpio line of SoC. 
Thus to
perform full system reboot procedure, before triggering reboot inside 
the SoC,
one need to manually toggle nreset line of this emmc card to 'zero' for 
a while.
Only then the card is put back to the default state, so ROM code is able 
to read
bootloaders from it.

My patch adds code for managing gpio line connected to nreset signal of 
emmc card.
It registers reset handler, which is being triggered from Linux reboot 
code. This
handler toggles such gpio line before the final reboot is triggered. My 
patch also
provides a function for registering as a mmc host hw_reset callback, so 
it can be
used to reset mmc card before initializing it with kernel driver (this 
is already
implemented), which also might help to get it working if broken 
bootloader left
the emmc card in some unknown/broken state (already observed such issue, 
but it
has been fixed finally by patching bootloader).


> Also, I wonder whether we could extend the mmc-pwrseq to cover your
> case? Did you consider that as an option?

I didn't consider mmc-pwrseq yet. For me it looked straightforward to 
implement
it just like card detect or write-protection gpio pins. I already 
noticed that
there is code for some mmc host driver, which perform mmc hardware 
reset. If you
think that extending pwrseq is the better approach, I will try to update my
patches. This will add reboot handler to mmc-pwrseq then. The only 
question is
weather to use it always when mmc-pwrseq has been enabled or only if some
additional property like 'reset-on-reboot' has been provided.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] mmc: core: add support for hardware reset gpio line
       [not found]         ` <54C9FA48.5050705-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2015-01-29 10:56           ` Javier Martinez Canillas
  2015-01-29 11:01             ` Marek Szyprowski
  2015-01-30 10:37             ` Marek Szyprowski
  0 siblings, 2 replies; 9+ messages in thread
From: Javier Martinez Canillas @ 2015-01-29 10:56 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Ulf Hansson, linux-mmc,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-samsung-soc, Kukjin Kim, Tobias Jakobi, Daniel Drake,
	Sebastian Reichel, Seungwon Jeon, Jaehoon Chung, Chris Ball,
	Joonyoung Shim

Hello Marek,

On Thu, Jan 29, 2015 at 10:15 AM, Marek Szyprowski
<m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>> Also, I wonder whether we could extend the mmc-pwrseq to cover your
>> case? Did you consider that as an option?
>
>
> I didn't consider mmc-pwrseq yet. For me it looked straightforward to

I agree with Ulf that using mmc-pwrseq would be a good solution and in
fact I think the pwrseq_simple [0] driver will fit your use case since
it supports a reset GPIO pin which is what many WLAN chips attached to
a SDIO interface use.

> implement
> it just like card detect or write-protection gpio pins. I already noticed
> that
> there is code for some mmc host driver, which perform mmc hardware reset. If
> you
> think that extending pwrseq is the better approach, I will try to update my
> patches. This will add reboot handler to mmc-pwrseq then. The only question
> is

I don't think that adding a reboot handler to mmc-pwrseq is needed.
AFAICT the call chain is:

sys_reboot() -> kernel_restart() -> device_shutdown() ->
mmc_bus_shutdown() -> _mmc_suspend() -> mmc_power_off() ->
mmc_pwrseq_power_off() -> struct mmc_pwrseq_ops .power_off

So since the pwrseq_simple already asserts the reset GPIO in
.power_off, it should be enough to ensure the eMMC will be reset to
its default configuration for the iROM to work properly.

It also asserts the GPIO pin in .pre_power_on and de-asserts in
.post_power_on which should be needed as well for the other case you
mentioned when a broken bootloader left the emmc card in some unknown
state.

> weather to use it always when mmc-pwrseq has been enabled or only if some
> additional property like 'reset-on-reboot' has been provided.
>

Having a reset GPIO is optional AFAIK so I don't think there is a need
for an additional "reset-on-reboot" propery.

> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Best regards,
Javier

[0]:
FYI, these are the relevant commits in linux-next:
fe1922d5d4d0 mmc: pwrseq_simple: Add support for a reset GPIO pin
ec2017f2491f mmc: pwrseq: Initial support for the simple MMC power
sequence provider
fe1686658f9c mmc: pwrseq: Document DT bindings for the simple MMC power sequence
1b745e8a4627 mmc: core: Initial support for MMC power sequences
--
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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] mmc: core: add support for hardware reset gpio line
  2015-01-29 10:56           ` Javier Martinez Canillas
@ 2015-01-29 11:01             ` Marek Szyprowski
  2015-01-30 10:37             ` Marek Szyprowski
  1 sibling, 0 replies; 9+ messages in thread
From: Marek Szyprowski @ 2015-01-29 11:01 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Ulf Hansson, linux-mmc, devicetree@vger.kernel.org,
	linux-samsung-soc, Kukjin Kim, Tobias Jakobi, Daniel Drake,
	Sebastian Reichel, Seungwon Jeon, Jaehoon Chung, Chris Ball,
	Joonyoung Shim

Hello,

On 2015-01-29 11:56, Javier Martinez Canillas wrote:
> On Thu, Jan 29, 2015 at 10:15 AM, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>>> Also, I wonder whether we could extend the mmc-pwrseq to cover your
>>> case? Did you consider that as an option?
>> I didn't consider mmc-pwrseq yet. For me it looked straightforward to
> I agree with Ulf that using mmc-pwrseq would be a good solution and in
> fact I think the pwrseq_simple [0] driver will fit your use case since
> it supports a reset GPIO pin which is what many WLAN chips attached to
> a SDIO interface use.
>> implement
>> it just like card detect or write-protection gpio pins. I already noticed
>> that
>> there is code for some mmc host driver, which perform mmc hardware reset. If
>> you
>> think that extending pwrseq is the better approach, I will try to update my
>> patches. This will add reboot handler to mmc-pwrseq then. The only question
>> is
> I don't think that adding a reboot handler to mmc-pwrseq is needed.
> AFAICT the call chain is:
>
> sys_reboot() -> kernel_restart() -> device_shutdown() ->
> mmc_bus_shutdown() -> _mmc_suspend() -> mmc_power_off() ->
> mmc_pwrseq_power_off() -> struct mmc_pwrseq_ops .power_off
>
> So since the pwrseq_simple already asserts the reset GPIO in
> .power_off, it should be enough to ensure the eMMC will be reset to
> its default configuration for the iROM to work properly.

I think that I had to add reset handler, because device_shutdown() was not
called if reset was triggered from magic sysrq, but I will check it again.

> It also asserts the GPIO pin in .pre_power_on and de-asserts in
> .post_power_on which should be needed as well for the other case you
> mentioned when a broken bootloader left the emmc card in some unknown
> state.
>
>> weather to use it always when mmc-pwrseq has been enabled or only if some
>> additional property like 'reset-on-reboot' has been provided.
>>
> Having a reset GPIO is optional AFAIK so I don't think there is a need
> for an additional "reset-on-reboot" propery.
>
>> Best regards
>> --
>> Marek Szyprowski, PhD
>> Samsung R&D Institute Poland
>>
> Best regards,
> Javier
>
> [0]:
> FYI, these are the relevant commits in linux-next:
> fe1922d5d4d0 mmc: pwrseq_simple: Add support for a reset GPIO pin
> ec2017f2491f mmc: pwrseq: Initial support for the simple MMC power
> sequence provider
> fe1686658f9c mmc: pwrseq: Document DT bindings for the simple MMC power sequence
> 1b745e8a4627 mmc: core: Initial support for MMC power sequences

Ok, I will check it.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] mmc: core: add support for hardware reset gpio line
  2015-01-29 10:56           ` Javier Martinez Canillas
  2015-01-29 11:01             ` Marek Szyprowski
@ 2015-01-30 10:37             ` Marek Szyprowski
  2015-01-30 11:33               ` Ulf Hansson
  2015-01-30 11:35               ` Sjoerd Simons
  1 sibling, 2 replies; 9+ messages in thread
From: Marek Szyprowski @ 2015-01-30 10:37 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Ulf Hansson, linux-mmc, devicetree@vger.kernel.org,
	linux-samsung-soc, Kukjin Kim, Tobias Jakobi, Daniel Drake,
	Sebastian Reichel, Seungwon Jeon, Jaehoon Chung, Chris Ball,
	Joonyoung Shim

Hello,

On 2015-01-29 11:56, Javier Martinez Canillas wrote:
> On Thu, Jan 29, 2015 at 10:15 AM, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>>> Also, I wonder whether we could extend the mmc-pwrseq to cover your
>>> case? Did you consider that as an option?
>>
>> I didn't consider mmc-pwrseq yet. For me it looked straightforward to
> I agree with Ulf that using mmc-pwrseq would be a good solution and in
> fact I think the pwrseq_simple [0] driver will fit your use case since
> it supports a reset GPIO pin which is what many WLAN chips attached to
> a SDIO interface use.

Ok, I've checked mmc-prwseq and mmc-pwrseq-simple. I also checked the
hardware and it mmc-pwrseq-simple cannot be used directly.

Although the signal is called RSTN (on Odroid U3 schema), the eMMC card
gets resetted not on low line level, but during the rising edge. This RSTN
line is also pulled up by the external resistor. However, the strangest
thing is the fact that the default SoC configuration (which is applied
during hw reset) for this GPIO line is input, pulled-down. The SoC
internal pull-down is stronger than the external pull up, so in the end,
during the SoC reboot the RSTN signal is set to zero. Later bootloader
disables the internal pull-down.

To sum up - to perform proper reboot on Odroid U3/XU3, one need to set
RSTN to zero, wait a while and the set it back to 1.

To achieve this with mmc-pwrseq-simple, I would need to modify the power_off
callback to toggle reset line to zero and back to one. This however might
not be desired to other sd/mmc cards used with mmc-pwrseq-simple.

I can also provide separate mmc-pwrsrq-odroid driver, which will be very
similar to mmc-pwrseq-simple.

Ulf - which approach would you prefer?


>
>> implement
>> it just like card detect or write-protection gpio pins. I already noticed
>> that
>> there is code for some mmc host driver, which perform mmc hardware reset. If
>> you
>> think that extending pwrseq is the better approach, I will try to update my
>> patches. This will add reboot handler to mmc-pwrseq then. The only question
>> is
> I don't think that adding a reboot handler to mmc-pwrseq is needed.
> AFAICT the call chain is:
>
> sys_reboot() -> kernel_restart() -> device_shutdown() ->
> mmc_bus_shutdown() -> _mmc_suspend() -> mmc_power_off() ->
> mmc_pwrseq_power_off() -> struct mmc_pwrseq_ops .power_off
>
> So since the pwrseq_simple already asserts the reset GPIO in
> .power_off, it should be enough to ensure the eMMC will be reset to
> its default configuration for the iROM to work properly.
>
> It also asserts the GPIO pin in .pre_power_on and de-asserts in
> .post_power_on which should be needed as well for the other case you
> mentioned when a broken bootloader left the emmc card in some unknown
> state.

emergency_restart() doesn't call device_shutdown(), so I think it still 
makes
sense to add real reset handler to mmc-pwrseq to ensure that power_off will
be called even during emergency_restart().

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] mmc: core: add support for hardware reset gpio line
  2015-01-30 10:37             ` Marek Szyprowski
@ 2015-01-30 11:33               ` Ulf Hansson
  2015-01-30 11:35               ` Sjoerd Simons
  1 sibling, 0 replies; 9+ messages in thread
From: Ulf Hansson @ 2015-01-30 11:33 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Javier Martinez Canillas, linux-mmc, devicetree@vger.kernel.org,
	linux-samsung-soc, Kukjin Kim, Tobias Jakobi, Daniel Drake,
	Sebastian Reichel, Seungwon Jeon, Jaehoon Chung, Chris Ball,
	Joonyoung Shim

On 30 January 2015 at 11:37, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> Hello,
>
> On 2015-01-29 11:56, Javier Martinez Canillas wrote:
>>
>> On Thu, Jan 29, 2015 at 10:15 AM, Marek Szyprowski
>> <m.szyprowski@samsung.com> wrote:
>>>>
>>>> Also, I wonder whether we could extend the mmc-pwrseq to cover your
>>>> case? Did you consider that as an option?
>>>
>>>
>>> I didn't consider mmc-pwrseq yet. For me it looked straightforward to
>>
>> I agree with Ulf that using mmc-pwrseq would be a good solution and in
>> fact I think the pwrseq_simple [0] driver will fit your use case since
>> it supports a reset GPIO pin which is what many WLAN chips attached to
>> a SDIO interface use.
>
>
> Ok, I've checked mmc-prwseq and mmc-pwrseq-simple. I also checked the
> hardware and it mmc-pwrseq-simple cannot be used directly.
>
> Although the signal is called RSTN (on Odroid U3 schema), the eMMC card
> gets resetted not on low line level, but during the rising edge. This RSTN
> line is also pulled up by the external resistor. However, the strangest
> thing is the fact that the default SoC configuration (which is applied
> during hw reset) for this GPIO line is input, pulled-down. The SoC
> internal pull-down is stronger than the external pull up, so in the end,
> during the SoC reboot the RSTN signal is set to zero. Later bootloader
> disables the internal pull-down.
>
> To sum up - to perform proper reboot on Odroid U3/XU3, one need to set
> RSTN to zero, wait a while and the set it back to 1.
>
> To achieve this with mmc-pwrseq-simple, I would need to modify the power_off
> callback to toggle reset line to zero and back to one. This however might
> not be desired to other sd/mmc cards used with mmc-pwrseq-simple.
>
> I can also provide separate mmc-pwrsrq-odroid driver, which will be very
> similar to mmc-pwrseq-simple.
>
> Ulf - which approach would you prefer?

To me this seems like a quite generic eMMC hw-reset thing, thus we
should maybe add a new pwrseq "driver" for it.

In principle you need to toogle a GPIO in the ->pre_power_on()
callback, right? And you are also proposing to register a restart
handler from the ->alloc() callback!?

I suppose this should work nicely.

Kind regards
Uffe

>
>
>>
>>> implement
>>> it just like card detect or write-protection gpio pins. I already noticed
>>> that
>>> there is code for some mmc host driver, which perform mmc hardware reset.
>>> If
>>> you
>>> think that extending pwrseq is the better approach, I will try to update
>>> my
>>> patches. This will add reboot handler to mmc-pwrseq then. The only
>>> question
>>> is
>>
>> I don't think that adding a reboot handler to mmc-pwrseq is needed.
>> AFAICT the call chain is:
>>
>> sys_reboot() -> kernel_restart() -> device_shutdown() ->
>> mmc_bus_shutdown() -> _mmc_suspend() -> mmc_power_off() ->
>> mmc_pwrseq_power_off() -> struct mmc_pwrseq_ops .power_off
>>
>> So since the pwrseq_simple already asserts the reset GPIO in
>> .power_off, it should be enough to ensure the eMMC will be reset to
>> its default configuration for the iROM to work properly.
>>
>> It also asserts the GPIO pin in .pre_power_on and de-asserts in
>> .post_power_on which should be needed as well for the other case you
>> mentioned when a broken bootloader left the emmc card in some unknown
>> state.
>
>
> emergency_restart() doesn't call device_shutdown(), so I think it still
> makes
> sense to add real reset handler to mmc-pwrseq to ensure that power_off will
> be called even during emergency_restart().
>
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] mmc: core: add support for hardware reset gpio line
  2015-01-30 10:37             ` Marek Szyprowski
  2015-01-30 11:33               ` Ulf Hansson
@ 2015-01-30 11:35               ` Sjoerd Simons
  1 sibling, 0 replies; 9+ messages in thread
From: Sjoerd Simons @ 2015-01-30 11:35 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Javier Martinez Canillas, Ulf Hansson, linux-mmc,
	devicetree@vger.kernel.org, linux-samsung-soc, Kukjin Kim,
	Tobias Jakobi, Daniel Drake, Sebastian Reichel, Seungwon Jeon,
	Jaehoon Chung, Chris Ball, Joonyoung Shim

[-- Attachment #1: Type: text/plain, Size: 2449 bytes --]

On Fri, 2015-01-30 at 11:37 +0100, Marek Szyprowski wrote:
> Hello,
> 
> On 2015-01-29 11:56, Javier Martinez Canillas wrote:
> > On Thu, Jan 29, 2015 at 10:15 AM, Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >>> Also, I wonder whether we could extend the mmc-pwrseq to cover your
> >>> case? Did you consider that as an option?
> >>
> >> I didn't consider mmc-pwrseq yet. For me it looked straightforward to
> > I agree with Ulf that using mmc-pwrseq would be a good solution and in
> > fact I think the pwrseq_simple [0] driver will fit your use case since
> > it supports a reset GPIO pin which is what many WLAN chips attached to
> > a SDIO interface use.
> 
> Ok, I've checked mmc-prwseq and mmc-pwrseq-simple. I also checked the
> hardware and it mmc-pwrseq-simple cannot be used directly.
> 
> Although the signal is called RSTN (on Odroid U3 schema), the eMMC card
> gets resetted not on low line level, but during the rising edge. This RSTN
> line is also pulled up by the external resistor. However, the strangest
> thing is the fact that the default SoC configuration (which is applied
> during hw reset) for this GPIO line is input, pulled-down. The SoC
> internal pull-down is stronger than the external pull up, so in the end,
> during the SoC reboot the RSTN signal is set to zero. Later bootloader
> disables the internal pull-down.
> 
> To sum up - to perform proper reboot on Odroid U3/XU3, one need to set
> RSTN to zero, wait a while and the set it back to 1.

> To achieve this with mmc-pwrseq-simple, I would need to modify the power_off
> callback to toggle reset line to zero and back to one. This however might
> not be desired to other sd/mmc cards used with mmc-pwrseq-simple.
> 
> I can also provide separate mmc-pwrsrq-odroid driver, which will be very
> similar to mmc-pwrseq-simple.

Apart from the initial odd setup of the SoC pins this is the standard
H/W reset operation specified by Jedec 4.4:

* pull the reset line down for at least 1us
* pull the line up again
* Wait at least 200us before sending commands

(sdhci_pci_int_hw_reset also implements this).

I'm not sure what the determining factor is for a pwrseq driver vs. it
being part of the mmc core would be. But if you do a pwrseq driver it
shouldn't be board specific e.g. call it mmc-pwrseq-emmc4.4 instead of
-odroid?


-- 
Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Collabora Ltd.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6170 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2015-01-30 11:35 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <1422453595-21160-1-git-send-email-m.szyprowski@samsung.com>
2015-01-28 13:59 ` [PATCH 4/4] ARM: dts: exynos5422-odroidxu3: add eMMC reset line Marek Szyprowski
     [not found] ` <1422453595-21160-2-git-send-email-m.szyprowski@samsung.com>
2015-01-28 14:24   ` [PATCH 1/4] mmc: core: add support for hardware reset gpio line Ulf Hansson
2015-01-28 14:41     ` Tobias Jakobi
     [not found]     ` <CAPDyKFpowidghwadJS+gYP0EVo6gRUhAG8s14CMEbwatFadP2Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-01-29  9:15       ` Marek Szyprowski
     [not found]         ` <54C9FA48.5050705-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2015-01-29 10:56           ` Javier Martinez Canillas
2015-01-29 11:01             ` Marek Szyprowski
2015-01-30 10:37             ` Marek Szyprowski
2015-01-30 11:33               ` Ulf Hansson
2015-01-30 11:35               ` Sjoerd Simons

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).