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

[Patrice CHOTARD]

Hi Simon

On 05/15/2017 05:02 AM, Simon Glass wrote:
> 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?

Sorry, i didn't understand what you mean by "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
>