[U-Boot] [PATCH v5 03/14] mmc: sti_sdhci: Use reset framework

[Simon Glass]

Hi Patrice,

On 10 May 2017 at 10:09,  <patrice.chotard at st.com> wrote:
> From: Patrice Chotard <patrice.chotard at st.com>
>
> Signed-off-by: Patrice Chotard <patrice.chotard at st.com>
> Reviewed-by: Jaehoon Chung <jh80.chung at samsung.com>
> ---
>
> v5:     _ none
> v4:     _ none
> v3:     _ none
> v2:     _ none
>
>
>  drivers/mmc/sti_sdhci.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
>

Reviewed-by: Simon Glass <sjg at chromium.org>

This is fine as is. But please see questions below.

> diff --git a/drivers/mmc/sti_sdhci.c b/drivers/mmc/sti_sdhci.c
> index d6c4d67..8b1b2c0 100644
> --- a/drivers/mmc/sti_sdhci.c
> +++ b/drivers/mmc/sti_sdhci.c
> @@ -8,6 +8,7 @@
>  #include <common.h>
>  #include <dm.h>
>  #include <mmc.h>
> +#include <reset-uclass.h>
>  #include <sdhci.h>
>  #include <asm/arch/sdhci.h>
>
> @@ -16,6 +17,7 @@ DECLARE_GLOBAL_DATA_PTR;
>  struct sti_sdhci_plat {
>         struct mmc_config cfg;
>         struct mmc mmc;
> +       struct reset_ctl reset;
>         int instance;
>  };
>
> @@ -37,17 +39,19 @@ struct sti_sdhci_plat {
>   * W/o these settings the SDHCI could configure and use the embedded controller
>   * with limited features.
>   */
> -static void sti_mmc_core_config(struct udevice *dev)
> +static int sti_mmc_core_config(struct udevice *dev)
>  {
>         struct sti_sdhci_plat *plat = dev_get_platdata(dev);
>         struct sdhci_host *host = dev_get_priv(dev);
> -       unsigned long *sysconf;
> +       int ret;
>
>         /* only MMC1 has a reset line */
>         if (plat->instance) {
> -               sysconf = (unsigned long *)(STIH410_SYSCONF5_BASE +
> -                         ST_MMC_CCONFIG_REG_5);
> -               generic_set_bit(SYSCONF_MMC1_ENABLE_BIT, sysconf);
> +               ret = reset_deassert(&plat->reset);
> +               if (ret < 0) {
> +                       error("MMC1 deassert failed: %d", ret);
> +                       return ret;
> +               }
>         }
>
>         writel(STI_FLASHSS_MMC_CORE_CONFIG_1,
> @@ -66,6 +70,8 @@ static void sti_mmc_core_config(struct udevice *dev)
>         }
>         writel(STI_FLASHSS_MMC_CORE_CONFIG4,
>                host->ioaddr + FLASHSS_MMC_CORE_CONFIG_4);
> +
> +       return 0;
>  }
>
>  static int sti_sdhci_probe(struct udevice *dev)
> @@ -80,13 +86,20 @@ static int sti_sdhci_probe(struct udevice *dev)
>          * MMC0 is wired to the SD slot,
>          * MMC1 is wired on the high speed connector
>          */
> -
> -       if (fdt_getprop(gd->fdt_blob, dev_of_offset(dev), "resets", NULL))
> +       if (fdt_getprop(gd->fdt_blob, dev_of_offset(dev), "resets", NULL)) {
>                 plat->instance = 1;
> -       else
> +               ret = reset_get_by_name(dev, "softreset", &plat->reset);

Two questions:

1. The name "softreset" is in the "resets" property, isn't it? If so,
can you use it instead of hard-coding "softreset" here?
2. Can you not call reset_get_by_name() and deal with the error return
(-ENOENT I think) if there is no such reset?

> +               if (ret) {
> +                       error("can't get reset for %s (%d)", dev->name, ret);
> +                       return ret;
> +               }
> +       } else {
>                 plat->instance = 0;
> +       }
>
> -       sti_mmc_core_config(dev);
> +       ret = sti_mmc_core_config(dev);
> +       if (ret)
> +               return ret;
>
>         host->quirks = SDHCI_QUIRK_WAIT_SEND_CMD |
>                        SDHCI_QUIRK_32BIT_DMA_ADDR |
> --
> 1.9.1
>

Regards,
Simon