Hi Patrice, On 17 May 2017 at 01:14, Patrice CHOTARD <patrice.chot...@st.com> wrote: > Hi Simon > > On 05/17/2017 03:38 AM, Simon Glass wrote: >> Hi Patrice, >> >> On 15 May 2017 at 03:21, Patrice CHOTARD <patrice.chot...@st.com> wrote: >>> Hi Simon >>> >>> On 05/15/2017 05:02 AM, Simon Glass wrote: >>>> Hi Patrice, >>>> >>>> On 10 May 2017 at 10:09, <patrice.chot...@st.com> wrote: >>>>> From: Patrice Chotard <patrice.chot...@st.com> >>>>> >>>>> Signed-off-by: Patrice Chotard <patrice.chot...@st.com> >>>>> Reviewed-by: Jaehoon Chung <jh80.ch...@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 <s...@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" >> >> I am wondering if the value of the 'resets' property is 'softtreset'? > > "softreset" is the value of the "reset-names" property. > > resets = <&softreset STIH407_MMC1_SOFTRESET>; > reset-names = "softreset"; > > But i am thinking to simplify this part. >
OK I see. Well, from me: Reviewed-by: Simon Glass <s...@chromium.org> > Patrice > >> If so, you could perhaps use that value instead of the string >> 'softreset'. >> >>> >>> >>>> 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 >> _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot