From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrice CHOTARD Date: Wed, 17 May 2017 07:14:09 +0000 Subject: [U-Boot] [PATCH v5 03/14] mmc: sti_sdhci: Use reset framework In-Reply-To: References: <1494432599-17924-1-git-send-email-patrice.chotard@st.com> <1494432599-17924-4-git-send-email-patrice.chotard@st.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon On 05/17/2017 03:38 AM, Simon Glass wrote: > Hi Patrice, > > On 15 May 2017 at 03:21, Patrice CHOTARD wrote: >> Hi Simon >> >> On 05/15/2017 05:02 AM, Simon Glass wrote: >>> Hi Patrice, >>> >>> On 10 May 2017 at 10:09, wrote: >>>> From: Patrice Chotard >>>> >>>> Signed-off-by: Patrice Chotard >>>> Reviewed-by: Jaehoon Chung >>>> --- >>>> >>>> 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 >>> >>> 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 >>>> #include >>>> #include >>>> +#include >>>> #include >>>> #include >>>> >>>> @@ -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. 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 >