Re: [PATCH 1/4] mmc: core: add support for hardware reset gpio line

Ulf Hansson
2015-01-28T14:24:03Z

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
>