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
