diff mbox

mmc: dw_mmc: change to use recommended reset procedure

Message ID 1395834374-1988-1-git-send-email-yuvaraj.cd@samsung.com
State New, archived
Headers show

Commit Message

Yuvaraj CD March 26, 2014, 11:46 a.m. UTC
From: Sonny Rao <sonnyrao@chromium.org>

This patch changes the fifo reset code to follow the reset procedure
outlined in the documentation of Synopsys  Mobile storage host databook
7.2.13.
Without this patch, we could able to see eMMC was not detected after
multiple reboots due to driver hangs while eMMC tuning for HS200.

Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.org>
---
 drivers/mmc/host/dw_mmc.c |   48 ++++++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/dw_mmc.h |    1 +
 2 files changed, 48 insertions(+), 1 deletion(-)

Comments

Yuvaraj CD May 8, 2014, 9:40 a.m. UTC | #1
Any comments on this patch?

On Wed, Mar 26, 2014 at 5:16 PM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote:
> From: Sonny Rao <sonnyrao@chromium.org>
>
> This patch changes the fifo reset code to follow the reset procedure
> outlined in the documentation of Synopsys  Mobile storage host databook
> 7.2.13.
> Without this patch, we could able to see eMMC was not detected after
> multiple reboots due to driver hangs while eMMC tuning for HS200.
>
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.org>
> ---
>  drivers/mmc/host/dw_mmc.c |   48 ++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/mmc/host/dw_mmc.h |    1 +
>  2 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 32dd81d..1d77431 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2220,7 +2220,53 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
>                 host->sg = NULL;
>         }
>
> -       return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
> +       /*
> +        * The recommended method for resetting is to always reset the
> +        * controller and the fifo, but differs slightly depending on the mode.
> +        * Note that this doesn't handle the "generic DMA" (not IDMAC) case.
> +        */
> +       if (dw_mci_ctrl_reset(host, SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET)) {
> +               unsigned long timeout = jiffies + msecs_to_jiffies(500);
> +               u32 status, rint;
> +
> +               /* if using dma we wait for dma_req to clear */
> +               if (host->using_dma) {
> +                       do {
> +                               status = mci_readl(host, STATUS);
> +                               if (!(status & SDMMC_STATUS_DMA_REQ))
> +                                       break;
> +                               cpu_relax();
> +                       } while (time_before(jiffies, timeout));
> +
> +                       if (status & SDMMC_STATUS_DMA_REQ)
> +                               dev_err(host->dev,
> +                                       "%s: Timeout waiting for dma_req to "
> +                                       "clear during reset", __func__);
> +
> +                       /* when using DMA next we reset the fifo again */
> +                       dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
> +               }
> +               /*
> +                * In all cases we clear the RAWINTS register to clear any
> +                * interrupts.
> +                */
> +               rint = mci_readl(host, RINTSTS);
> +               rint = rint & (~mci_readl(host, MINTSTS));
> +               if (rint)
> +                       mci_writel(host, RINTSTS, rint);
> +
> +       } else
> +               dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
> +
> + #ifdef CONFIG_MMC_DW_IDMAC
> +       /* It is also recommended that we reset and reprogram idmac */
> +       dw_mci_idmac_reset(host);
> + #endif
> +
> +       /* After a CTRL reset we need to have CIU set clock registers  */
> +       mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
> +
> +       return true;
>  }
>
>  static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 738fa24..037e47a 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -129,6 +129,7 @@
>  #define SDMMC_CMD_INDX(n)              ((n) & 0x1F)
>  /* Status register defines */
>  #define SDMMC_GET_FCNT(x)              (((x)>>17) & 0x1FFF)
> +#define SDMMC_STATUS_DMA_REQ           BIT(31)
>  /* FIFOTH register defines */
>  #define SDMMC_SET_FIFOTH(m, r, t)      (((m) & 0x7) << 28 | \
>                                          ((r) & 0xFFF) << 16 | \
> --
> 1.7.10.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jaehoon Chung May 9, 2014, 1:15 a.m. UTC | #2
On 05/08/2014 06:40 PM, Yuvaraj Kumar wrote:
> Any comments on this patch?
> 
> On Wed, Mar 26, 2014 at 5:16 PM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote:
>> From: Sonny Rao <sonnyrao@chromium.org>
>>
>> This patch changes the fifo reset code to follow the reset procedure
>> outlined in the documentation of Synopsys  Mobile storage host databook
>> 7.2.13.
>> Without this patch, we could able to see eMMC was not detected after
>> multiple reboots due to driver hangs while eMMC tuning for HS200.
>>
>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.org>
>> ---
>>  drivers/mmc/host/dw_mmc.c |   48 ++++++++++++++++++++++++++++++++++++++++++++-
>>  drivers/mmc/host/dw_mmc.h |    1 +
>>  2 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 32dd81d..1d77431 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -2220,7 +2220,53 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
>>                 host->sg = NULL;
>>         }
>>
>> -       return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>> +       /*
>> +        * The recommended method for resetting is to always reset the
>> +        * controller and the fifo, but differs slightly depending on the mode.
>> +        * Note that this doesn't handle the "generic DMA" (not IDMAC) case.
>> +        */

"not IDMAC" is confused..

>> +       if (dw_mci_ctrl_reset(host, SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET)) {
>> +               unsigned long timeout = jiffies + msecs_to_jiffies(500);
>> +               u32 status, rint;
>> +
>> +               /* if using dma we wait for dma_req to clear */
>> +               if (host->using_dma) {
>> +                       do {
>> +                               status = mci_readl(host, STATUS);
>> +                               if (!(status & SDMMC_STATUS_DMA_REQ))
>> +                                       break;
>> +                               cpu_relax();
>> +                       } while (time_before(jiffies, timeout));
>> +
>> +                       if (status & SDMMC_STATUS_DMA_REQ)
>> +                               dev_err(host->dev,
>> +                                       "%s: Timeout waiting for dma_req to "
>> +                                       "clear during reset", __func__);
>> +
>> +                       /* when using DMA next we reset the fifo again */
>> +                       dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>> +               }
>> +               /*
>> +                * In all cases we clear the RAWINTS register to clear any
>> +                * interrupts.
>> +                */
>> +               rint = mci_readl(host, RINTSTS);
>> +               rint = rint & (~mci_readl(host, MINTSTS));

Just clear the RINTSTS register? why do you add these?

>> +               if (rint)
>> +                       mci_writel(host, RINTSTS, rint);
>> +
>> +       } else
>> +               dev_err(host->dev, "%s: Reset bits didn't clear", __func__);

Just display the error log? I didn't understand this.
If you displayed the error log, then it needs to return the error value.

>> +
>> + #ifdef CONFIG_MMC_DW_IDMAC
>> +       /* It is also recommended that we reset and reprogram idmac */
>> +       dw_mci_idmac_reset(host);
>> + #endif
>> +
>> +       /* After a CTRL reset we need to have CIU set clock registers  */
>> +       mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
>> +
>> +       return true;
>>  }
>>
>>  static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>> index 738fa24..037e47a 100644
>> --- a/drivers/mmc/host/dw_mmc.h
>> +++ b/drivers/mmc/host/dw_mmc.h
>> @@ -129,6 +129,7 @@
>>  #define SDMMC_CMD_INDX(n)              ((n) & 0x1F)
>>  /* Status register defines */
>>  #define SDMMC_GET_FCNT(x)              (((x)>>17) & 0x1FFF)
>> +#define SDMMC_STATUS_DMA_REQ           BIT(31)
>>  /* FIFOTH register defines */
>>  #define SDMMC_SET_FIFOTH(m, r, t)      (((m) & 0x7) << 28 | \
>>                                          ((r) & 0xFFF) << 16 | \
>> --
>> 1.7.10.4
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sonny Rao May 9, 2014, 1:34 a.m. UTC | #3
On Thu, May 8, 2014 at 6:15 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> On 05/08/2014 06:40 PM, Yuvaraj Kumar wrote:
>> Any comments on this patch?
>>
>> On Wed, Mar 26, 2014 at 5:16 PM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote:
>>> From: Sonny Rao <sonnyrao@chromium.org>
>>>
>>> This patch changes the fifo reset code to follow the reset procedure
>>> outlined in the documentation of Synopsys  Mobile storage host databook
>>> 7.2.13.
>>> Without this patch, we could able to see eMMC was not detected after
>>> multiple reboots due to driver hangs while eMMC tuning for HS200.
>>>
>>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.org>
>>> ---
>>>  drivers/mmc/host/dw_mmc.c |   48 ++++++++++++++++++++++++++++++++++++++++++++-
>>>  drivers/mmc/host/dw_mmc.h |    1 +
>>>  2 files changed, 48 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 32dd81d..1d77431 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -2220,7 +2220,53 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
>>>                 host->sg = NULL;
>>>         }
>>>
>>> -       return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>>> +       /*
>>> +        * The recommended method for resetting is to always reset the
>>> +        * controller and the fifo, but differs slightly depending on the mode.
>>> +        * Note that this doesn't handle the "generic DMA" (not IDMAC) case.
>>> +        */
>
> "not IDMAC" is confused..
>

The documentation describes three different possible modes.  There's a
mode that doesn't use IDMAC but still does DMA.  But as far as I can
tell this driver doesn't support that way.  We can just remove that
wording if it's confusing.

>>> +       if (dw_mci_ctrl_reset(host, SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET)) {
>>> +               unsigned long timeout = jiffies + msecs_to_jiffies(500);
>>> +               u32 status, rint;
>>> +
>>> +               /* if using dma we wait for dma_req to clear */
>>> +               if (host->using_dma) {
>>> +                       do {
>>> +                               status = mci_readl(host, STATUS);
>>> +                               if (!(status & SDMMC_STATUS_DMA_REQ))
>>> +                                       break;
>>> +                               cpu_relax();
>>> +                       } while (time_before(jiffies, timeout));
>>> +
>>> +                       if (status & SDMMC_STATUS_DMA_REQ)
>>> +                               dev_err(host->dev,
>>> +                                       "%s: Timeout waiting for dma_req to "
>>> +                                       "clear during reset", __func__);
>>> +
>>> +                       /* when using DMA next we reset the fifo again */
>>> +                       dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>>> +               }
>>> +               /*
>>> +                * In all cases we clear the RAWINTS register to clear any
>>> +                * interrupts.
>>> +                */
>>> +               rint = mci_readl(host, RINTSTS);
>>> +               rint = rint & (~mci_readl(host, MINTSTS));
>
> Just clear the RINTSTS register? why do you add these?
>

This will look at what is not masked, and only clear those bits.

>>> +               if (rint)
>>> +                       mci_writel(host, RINTSTS, rint);
>>> +
>>> +       } else
>>> +               dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
>
> Just display the error log? I didn't understand this.
> If you displayed the error log, then it needs to return the error value.
>
>>> +
>>> + #ifdef CONFIG_MMC_DW_IDMAC
>>> +       /* It is also recommended that we reset and reprogram idmac */
>>> +       dw_mci_idmac_reset(host);
>>> + #endif
>>> +
>>> +       /* After a CTRL reset we need to have CIU set clock registers  */
>>> +       mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
>>> +
>>> +       return true;
>>>  }
>>>
>>>  static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>> index 738fa24..037e47a 100644
>>> --- a/drivers/mmc/host/dw_mmc.h
>>> +++ b/drivers/mmc/host/dw_mmc.h
>>> @@ -129,6 +129,7 @@
>>>  #define SDMMC_CMD_INDX(n)              ((n) & 0x1F)
>>>  /* Status register defines */
>>>  #define SDMMC_GET_FCNT(x)              (((x)>>17) & 0x1FFF)
>>> +#define SDMMC_STATUS_DMA_REQ           BIT(31)
>>>  /* FIFOTH register defines */
>>>  #define SDMMC_SET_FIFOTH(m, r, t)      (((m) & 0x7) << 28 | \
>>>                                          ((r) & 0xFFF) << 16 | \
>>> --
>>> 1.7.10.4
>>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jaehoon Chung May 9, 2014, 4:27 a.m. UTC | #4
Hi, Sonny.

I have checked the Synopsys TRM..

On 05/09/2014 10:34 AM, Sonny Rao wrote:
> On Thu, May 8, 2014 at 6:15 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> On 05/08/2014 06:40 PM, Yuvaraj Kumar wrote:
>>> Any comments on this patch?
>>>
>>> On Wed, Mar 26, 2014 at 5:16 PM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote:
>>>> From: Sonny Rao <sonnyrao@chromium.org>
>>>>
>>>> This patch changes the fifo reset code to follow the reset procedure
>>>> outlined in the documentation of Synopsys  Mobile storage host databook
>>>> 7.2.13.
>>>> Without this patch, we could able to see eMMC was not detected after
>>>> multiple reboots due to driver hangs while eMMC tuning for HS200.
>>>>
>>>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.org>
>>>> ---
>>>>  drivers/mmc/host/dw_mmc.c |   48 ++++++++++++++++++++++++++++++++++++++++++++-
>>>>  drivers/mmc/host/dw_mmc.h |    1 +
>>>>  2 files changed, 48 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>> index 32dd81d..1d77431 100644
>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>> @@ -2220,7 +2220,53 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
>>>>                 host->sg = NULL;
>>>>         }
>>>>
>>>> -       return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>>>> +       /*
>>>> +        * The recommended method for resetting is to always reset the
>>>> +        * controller and the fifo, but differs slightly depending on the mode.
>>>> +        * Note that this doesn't handle the "generic DMA" (not IDMAC) case.
>>>> +        */
>>
>> "not IDMAC" is confused..
>>
> 
> The documentation describes three different possible modes.  There's a
> mode that doesn't use IDMAC but still does DMA.  But as far as I can
> tell this driver doesn't support that way.  We can just remove that
> wording if it's confusing.
> 
>>>> +       if (dw_mci_ctrl_reset(host, SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET)) {
>>>> +               unsigned long timeout = jiffies + msecs_to_jiffies(500);
>>>> +               u32 status, rint;
>>>> +
>>>> +               /* if using dma we wait for dma_req to clear */
>>>> +               if (host->using_dma) {
>>>> +                       do {
>>>> +                               status = mci_readl(host, STATUS);
>>>> +                               if (!(status & SDMMC_STATUS_DMA_REQ))
>>>> +                                       break;
>>>> +                               cpu_relax();
>>>> +                       } while (time_before(jiffies, timeout));
>>>> +
>>>> +                       if (status & SDMMC_STATUS_DMA_REQ)
>>>> +                               dev_err(host->dev,
>>>> +                                       "%s: Timeout waiting for dma_req to "
>>>> +                                       "clear during reset", __func__);
>>>> +
>>>> +                       /* when using DMA next we reset the fifo again */
>>>> +                       dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>>>> +               }
>>>> +               /*
>>>> +                * In all cases we clear the RAWINTS register to clear any
>>>> +                * interrupts.
>>>> +                */
>>>> +               rint = mci_readl(host, RINTSTS);
>>>> +               rint = rint & (~mci_readl(host, MINTSTS));
you use the "status or temp" instead of rint. (you can reuse the variable.)
And can use "status &= ~mci_readl(host,MINTSTS);"

>>
>> Just clear the RINTSTS register? why do you add these?
>>
> 
> This will look at what is not masked, and only clear those bits.
Well, i known if clear the RINTSTS register, 
recommended to use "0xfffffff" and set the value for interrupt.

> 
>>>> +               if (rint)
>>>> +                       mci_writel(host, RINTSTS, rint);
>>>> +
>>>> +       } else
>>>> +               dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
>>
>> Just display the error log? I didn't understand this.
>> If you displayed the error log, then it needs to return the error value.
>>
>>>> +
>>>> + #ifdef CONFIG_MMC_DW_IDMAC
>>>> +       /* It is also recommended that we reset and reprogram idmac */
>>>> +       dw_mci_idmac_reset(host);
>>>> + #endif
>>>> +
>>>> +       /* After a CTRL reset we need to have CIU set clock registers  */
>>>> +       mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);

Well, why do you send the clock update command?
I didn't see that update the value related with clock.

Best Regards,
Jaehoon Chung

>>>> +
>>>> +       return true;
>>>>  }
>>>>
>>>>  static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
>>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>>> index 738fa24..037e47a 100644
>>>> --- a/drivers/mmc/host/dw_mmc.h
>>>> +++ b/drivers/mmc/host/dw_mmc.h
>>>> @@ -129,6 +129,7 @@
>>>>  #define SDMMC_CMD_INDX(n)              ((n) & 0x1F)
>>>>  /* Status register defines */
>>>>  #define SDMMC_GET_FCNT(x)              (((x)>>17) & 0x1FFF)
>>>> +#define SDMMC_STATUS_DMA_REQ           BIT(31)
>>>>  /* FIFOTH register defines */
>>>>  #define SDMMC_SET_FIFOTH(m, r, t)      (((m) & 0x7) << 28 | \
>>>>                                          ((r) & 0xFFF) << 16 | \
>>>> --
>>>> 1.7.10.4
>>>>
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jaehoon Chung May 9, 2014, 7:32 a.m. UTC | #5
Hi, Sonny.

You can discard the my previous some comment.
As you mentioned, this reset sequence is recommended at Synopsys TRM.

Add the minor question.

On 05/09/2014 01:27 PM, Jaehoon Chung wrote:
> Hi, Sonny.
> 
> I have checked the Synopsys TRM..
> 
> On 05/09/2014 10:34 AM, Sonny Rao wrote:
>> On Thu, May 8, 2014 at 6:15 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>> On 05/08/2014 06:40 PM, Yuvaraj Kumar wrote:
>>>> Any comments on this patch?
>>>>
>>>> On Wed, Mar 26, 2014 at 5:16 PM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote:
>>>>> From: Sonny Rao <sonnyrao@chromium.org>
>>>>>
>>>>> This patch changes the fifo reset code to follow the reset procedure
>>>>> outlined in the documentation of Synopsys  Mobile storage host databook
>>>>> 7.2.13.
>>>>> Without this patch, we could able to see eMMC was not detected after
>>>>> multiple reboots due to driver hangs while eMMC tuning for HS200.
>>>>>
>>>>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>>>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.org>
>>>>> ---
>>>>>  drivers/mmc/host/dw_mmc.c |   48 ++++++++++++++++++++++++++++++++++++++++++++-
>>>>>  drivers/mmc/host/dw_mmc.h |    1 +
>>>>>  2 files changed, 48 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>> index 32dd81d..1d77431 100644
>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>> @@ -2220,7 +2220,53 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
>>>>>                 host->sg = NULL;
>>>>>         }
>>>>>
>>>>> -       return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>>>>> +       /*
>>>>> +        * The recommended method for resetting is to always reset the
>>>>> +        * controller and the fifo, but differs slightly depending on the mode.
>>>>> +        * Note that this doesn't handle the "generic DMA" (not IDMAC) case.
>>>>> +        */
>>>
>>> "not IDMAC" is confused..
>>>
>>
>> The documentation describes three different possible modes.  There's a
>> mode that doesn't use IDMAC but still does DMA.  But as far as I can
>> tell this driver doesn't support that way.  We can just remove that
>> wording if it's confusing.

How did it know whether dma is generic DMA or not?

>>
>>>>> +       if (dw_mci_ctrl_reset(host, SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET)) {
>>>>> +               unsigned long timeout = jiffies + msecs_to_jiffies(500);
>>>>> +               u32 status, rint;
>>>>> +
>>>>> +               /* if using dma we wait for dma_req to clear */
>>>>> +               if (host->using_dma) {
>>>>> +                       do {
>>>>> +                               status = mci_readl(host, STATUS);
>>>>> +                               if (!(status & SDMMC_STATUS_DMA_REQ))
>>>>> +                                       break;
>>>>> +                               cpu_relax();
>>>>> +                       } while (time_before(jiffies, timeout));
>>>>> +
>>>>> +                       if (status & SDMMC_STATUS_DMA_REQ)
>>>>> +                               dev_err(host->dev,
>>>>> +                                       "%s: Timeout waiting for dma_req to "
>>>>> +                                       "clear during reset", __func__);
>>>>> +
>>>>> +                       /* when using DMA next we reset the fifo again */
>>>>> +                       dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>>>>> +               }
>>>>> +               /*
>>>>> +                * In all cases we clear the RAWINTS register to clear any
>>>>> +                * interrupts.
>>>>> +                */
>>>>> +               rint = mci_readl(host, RINTSTS);
>>>>> +               rint = rint & (~mci_readl(host, MINTSTS));
> you use the "status or temp" instead of rint. (you can reuse the variable.)
> And can use "status &= ~mci_readl(host,MINTSTS);"
> 
>>>
>>> Just clear the RINTSTS register? why do you add these?
>>>
>>
>> This will look at what is not masked, and only clear those bits.
> Well, i known if clear the RINTSTS register, 
> recommended to use "0xfffffff" and set the value for interrupt.

Can be used "0xfffffff"?

Best Regards,
Jaehoon Chung
> 
>>
>>>>> +               if (rint)
>>>>> +                       mci_writel(host, RINTSTS, rint);
>>>>> +
>>>>> +       } else
>>>>> +               dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
>>>
>>> Just display the error log? I didn't understand this.
>>> If you displayed the error log, then it needs to return the error value.
>>>
>>>>> +
>>>>> + #ifdef CONFIG_MMC_DW_IDMAC
>>>>> +       /* It is also recommended that we reset and reprogram idmac */
>>>>> +       dw_mci_idmac_reset(host);
>>>>> + #endif
>>>>> +
>>>>> +       /* After a CTRL reset we need to have CIU set clock registers  */
>>>>> +       mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
> 
> Well, why do you send the clock update command?
> I didn't see that update the value related with clock.
> 
> Best Regards,
> Jaehoon Chung
> 
>>>>> +
>>>>> +       return true;
>>>>>  }
>>>>>
>>>>>  static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
>>>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>>>> index 738fa24..037e47a 100644
>>>>> --- a/drivers/mmc/host/dw_mmc.h
>>>>> +++ b/drivers/mmc/host/dw_mmc.h
>>>>> @@ -129,6 +129,7 @@
>>>>>  #define SDMMC_CMD_INDX(n)              ((n) & 0x1F)
>>>>>  /* Status register defines */
>>>>>  #define SDMMC_GET_FCNT(x)              (((x)>>17) & 0x1FFF)
>>>>> +#define SDMMC_STATUS_DMA_REQ           BIT(31)
>>>>>  /* FIFOTH register defines */
>>>>>  #define SDMMC_SET_FIFOTH(m, r, t)      (((m) & 0x7) << 28 | \
>>>>>                                          ((r) & 0xFFF) << 16 | \
>>>>> --
>>>>> 1.7.10.4
>>>>>
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sonny Rao May 10, 2014, 3:36 a.m. UTC | #6
On Fri, May 9, 2014 at 12:32 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Hi, Sonny.
>
> You can discard the my previous some comment.
> As you mentioned, this reset sequence is recommended at Synopsys TRM.
>
> Add the minor question.
>
> On 05/09/2014 01:27 PM, Jaehoon Chung wrote:
>> Hi, Sonny.
>>
>> I have checked the Synopsys TRM..
>>
>> On 05/09/2014 10:34 AM, Sonny Rao wrote:
>>> On Thu, May 8, 2014 at 6:15 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>> On 05/08/2014 06:40 PM, Yuvaraj Kumar wrote:
>>>>> Any comments on this patch?
>>>>>
>>>>> On Wed, Mar 26, 2014 at 5:16 PM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote:
>>>>>> From: Sonny Rao <sonnyrao@chromium.org>
>>>>>>
>>>>>> This patch changes the fifo reset code to follow the reset procedure
>>>>>> outlined in the documentation of Synopsys  Mobile storage host databook
>>>>>> 7.2.13.
>>>>>> Without this patch, we could able to see eMMC was not detected after
>>>>>> multiple reboots due to driver hangs while eMMC tuning for HS200.
>>>>>>
>>>>>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>>>>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.org>
>>>>>> ---
>>>>>>  drivers/mmc/host/dw_mmc.c |   48 ++++++++++++++++++++++++++++++++++++++++++++-
>>>>>>  drivers/mmc/host/dw_mmc.h |    1 +
>>>>>>  2 files changed, 48 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>>> index 32dd81d..1d77431 100644
>>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>>> @@ -2220,7 +2220,53 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
>>>>>>                 host->sg = NULL;
>>>>>>         }
>>>>>>
>>>>>> -       return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>>>>>> +       /*
>>>>>> +        * The recommended method for resetting is to always reset the
>>>>>> +        * controller and the fifo, but differs slightly depending on the mode.
>>>>>> +        * Note that this doesn't handle the "generic DMA" (not IDMAC) case.
>>>>>> +        */
>>>>
>>>> "not IDMAC" is confused..
>>>>
>>>
>>> The documentation describes three different possible modes.  There's a
>>> mode that doesn't use IDMAC but still does DMA.  But as far as I can
>>> tell this driver doesn't support that way.  We can just remove that
>>> wording if it's confusing.
>
> How did it know whether dma is generic DMA or not?
>

That's a good question.  I wasn't sure whether the driver supported it
or not.  It looks like it definitely supports IDMAC through the
CONFIG_MMC_DW_IDMAC flag, but I wasn't sure if it was supported the
generic dma.  Maybe if CONFIG_MMC_DW_IDMAC isn't specified but
host->dma_ops is not NULL then we are using the generic dma mode.

>>>
>>>>>> +       if (dw_mci_ctrl_reset(host, SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET)) {
>>>>>> +               unsigned long timeout = jiffies + msecs_to_jiffies(500);
>>>>>> +               u32 status, rint;
>>>>>> +
>>>>>> +               /* if using dma we wait for dma_req to clear */
>>>>>> +               if (host->using_dma) {
>>>>>> +                       do {
>>>>>> +                               status = mci_readl(host, STATUS);
>>>>>> +                               if (!(status & SDMMC_STATUS_DMA_REQ))
>>>>>> +                                       break;
>>>>>> +                               cpu_relax();
>>>>>> +                       } while (time_before(jiffies, timeout));
>>>>>> +
>>>>>> +                       if (status & SDMMC_STATUS_DMA_REQ)
>>>>>> +                               dev_err(host->dev,
>>>>>> +                                       "%s: Timeout waiting for dma_req to "
>>>>>> +                                       "clear during reset", __func__);
>>>>>> +
>>>>>> +                       /* when using DMA next we reset the fifo again */
>>>>>> +                       dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>>>>>> +               }
>>>>>> +               /*
>>>>>> +                * In all cases we clear the RAWINTS register to clear any
>>>>>> +                * interrupts.
>>>>>> +                */
>>>>>> +               rint = mci_readl(host, RINTSTS);
>>>>>> +               rint = rint & (~mci_readl(host, MINTSTS));
>> you use the "status or temp" instead of rint. (you can reuse the variable.)
>> And can use "status &= ~mci_readl(host,MINTSTS);"
>>
>>>>
>>>> Just clear the RINTSTS register? why do you add these?
>>>>
>>>
>>> This will look at what is not masked, and only clear those bits.
>> Well, i known if clear the RINTSTS register,
>> recommended to use "0xfffffff" and set the value for interrupt.
>
> Can be used "0xfffffff"?
>

Yeah we probably can.  We just lose information about interrupts that
were masked, but maybe we just don't care about any of them anyway, so
it doesn't matter.

> Best Regards,
> Jaehoon Chung
>>
>>>
>>>>>> +               if (rint)
>>>>>> +                       mci_writel(host, RINTSTS, rint);
>>>>>> +
>>>>>> +       } else
>>>>>> +               dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
>>>>
>>>> Just display the error log? I didn't understand this.
>>>> If you displayed the error log, then it needs to return the error value.
>>>>
>>>>>> +
>>>>>> + #ifdef CONFIG_MMC_DW_IDMAC
>>>>>> +       /* It is also recommended that we reset and reprogram idmac */
>>>>>> +       dw_mci_idmac_reset(host);
>>>>>> + #endif
>>>>>> +
>>>>>> +       /* After a CTRL reset we need to have CIU set clock registers  */
>>>>>> +       mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
>>
>> Well, why do you send the clock update command?
>> I didn't see that update the value related with clock.
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>>>> +
>>>>>> +       return true;
>>>>>>  }
>>>>>>
>>>>>>  static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
>>>>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>>>>> index 738fa24..037e47a 100644
>>>>>> --- a/drivers/mmc/host/dw_mmc.h
>>>>>> +++ b/drivers/mmc/host/dw_mmc.h
>>>>>> @@ -129,6 +129,7 @@
>>>>>>  #define SDMMC_CMD_INDX(n)              ((n) & 0x1F)
>>>>>>  /* Status register defines */
>>>>>>  #define SDMMC_GET_FCNT(x)              (((x)>>17) & 0x1FFF)
>>>>>> +#define SDMMC_STATUS_DMA_REQ           BIT(31)
>>>>>>  /* FIFOTH register defines */
>>>>>>  #define SDMMC_SET_FIFOTH(m, r, t)      (((m) & 0x7) << 28 | \
>>>>>>                                          ((r) & 0xFFF) << 16 | \
>>>>>> --
>>>>>> 1.7.10.4
>>>>>>
>>>>>
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Seungwon Jeon May 10, 2014, 2:08 p.m. UTC | #7
Hi Sonny,

Can you separate procedure?
Reset all are handled in fifo-reset.
And ciu reset is always needed for error handling?

Thanks,
Seungwon Jeon

On Sat, May 10, 2014, Sonny Rao wrote:
> On Fri, May 9, 2014 at 12:32 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> > Hi, Sonny.
> >
> > You can discard the my previous some comment.
> > As you mentioned, this reset sequence is recommended at Synopsys TRM.
> >
> > Add the minor question.
> >
> > On 05/09/2014 01:27 PM, Jaehoon Chung wrote:
> >> Hi, Sonny.
> >>
> >> I have checked the Synopsys TRM..
> >>
> >> On 05/09/2014 10:34 AM, Sonny Rao wrote:
> >>> On Thu, May 8, 2014 at 6:15 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> >>>> On 05/08/2014 06:40 PM, Yuvaraj Kumar wrote:
> >>>>> Any comments on this patch?
> >>>>>
> >>>>> On Wed, Mar 26, 2014 at 5:16 PM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote:
> >>>>>> From: Sonny Rao <sonnyrao@chromium.org>
> >>>>>>
> >>>>>> This patch changes the fifo reset code to follow the reset procedure
> >>>>>> outlined in the documentation of Synopsys  Mobile storage host databook
> >>>>>> 7.2.13.
> >>>>>> Without this patch, we could able to see eMMC was not detected after
> >>>>>> multiple reboots due to driver hangs while eMMC tuning for HS200.
> >>>>>>
> >>>>>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> >>>>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.org>
> >>>>>> ---
> >>>>>>  drivers/mmc/host/dw_mmc.c |   48 ++++++++++++++++++++++++++++++++++++++++++++-
> >>>>>>  drivers/mmc/host/dw_mmc.h |    1 +
> >>>>>>  2 files changed, 48 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> >>>>>> index 32dd81d..1d77431 100644
> >>>>>> --- a/drivers/mmc/host/dw_mmc.c
> >>>>>> +++ b/drivers/mmc/host/dw_mmc.c
> >>>>>> @@ -2220,7 +2220,53 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
> >>>>>>                 host->sg = NULL;
> >>>>>>         }
> >>>>>>
> >>>>>> -       return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
> >>>>>> +       /*
> >>>>>> +        * The recommended method for resetting is to always reset the
> >>>>>> +        * controller and the fifo, but differs slightly depending on the mode.
> >>>>>> +        * Note that this doesn't handle the "generic DMA" (not IDMAC) case.
> >>>>>> +        */
> >>>>
> >>>> "not IDMAC" is confused..
> >>>>
> >>>
> >>> The documentation describes three different possible modes.  There's a
> >>> mode that doesn't use IDMAC but still does DMA.  But as far as I can
> >>> tell this driver doesn't support that way.  We can just remove that
> >>> wording if it's confusing.
> >
> > How did it know whether dma is generic DMA or not?
> >
> 
> That's a good question.  I wasn't sure whether the driver supported it
> or not.  It looks like it definitely supports IDMAC through the
> CONFIG_MMC_DW_IDMAC flag, but I wasn't sure if it was supported the
> generic dma.  Maybe if CONFIG_MMC_DW_IDMAC isn't specified but
> host->dma_ops is not NULL then we are using the generic dma mode.
> 
> >>>
> >>>>>> +       if (dw_mci_ctrl_reset(host, SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET)) {
> >>>>>> +               unsigned long timeout = jiffies + msecs_to_jiffies(500);
> >>>>>> +               u32 status, rint;
> >>>>>> +
> >>>>>> +               /* if using dma we wait for dma_req to clear */
> >>>>>> +               if (host->using_dma) {
> >>>>>> +                       do {
> >>>>>> +                               status = mci_readl(host, STATUS);
> >>>>>> +                               if (!(status & SDMMC_STATUS_DMA_REQ))
> >>>>>> +                                       break;
> >>>>>> +                               cpu_relax();
> >>>>>> +                       } while (time_before(jiffies, timeout));
> >>>>>> +
> >>>>>> +                       if (status & SDMMC_STATUS_DMA_REQ)
> >>>>>> +                               dev_err(host->dev,
> >>>>>> +                                       "%s: Timeout waiting for dma_req to "
> >>>>>> +                                       "clear during reset", __func__);
> >>>>>> +
> >>>>>> +                       /* when using DMA next we reset the fifo again */
> >>>>>> +                       dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
> >>>>>> +               }
> >>>>>> +               /*
> >>>>>> +                * In all cases we clear the RAWINTS register to clear any
> >>>>>> +                * interrupts.
> >>>>>> +                */
> >>>>>> +               rint = mci_readl(host, RINTSTS);
> >>>>>> +               rint = rint & (~mci_readl(host, MINTSTS));
> >> you use the "status or temp" instead of rint. (you can reuse the variable.)
> >> And can use "status &= ~mci_readl(host,MINTSTS);"
> >>
> >>>>
> >>>> Just clear the RINTSTS register? why do you add these?
> >>>>
> >>>
> >>> This will look at what is not masked, and only clear those bits.
> >> Well, i known if clear the RINTSTS register,
> >> recommended to use "0xfffffff" and set the value for interrupt.
> >
> > Can be used "0xfffffff"?
> >
> 
> Yeah we probably can.  We just lose information about interrupts that
> were masked, but maybe we just don't care about any of them anyway, so
> it doesn't matter.
> 
> > Best Regards,
> > Jaehoon Chung
> >>
> >>>
> >>>>>> +               if (rint)
> >>>>>> +                       mci_writel(host, RINTSTS, rint);
> >>>>>> +
> >>>>>> +       } else
> >>>>>> +               dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
> >>>>
> >>>> Just display the error log? I didn't understand this.
> >>>> If you displayed the error log, then it needs to return the error value.
> >>>>
> >>>>>> +
> >>>>>> + #ifdef CONFIG_MMC_DW_IDMAC
> >>>>>> +       /* It is also recommended that we reset and reprogram idmac */
> >>>>>> +       dw_mci_idmac_reset(host);
> >>>>>> + #endif
> >>>>>> +
> >>>>>> +       /* After a CTRL reset we need to have CIU set clock registers  */
> >>>>>> +       mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
> >>
> >> Well, why do you send the clock update command?
> >> I didn't see that update the value related with clock.
> >>
> >> Best Regards,
> >> Jaehoon Chung
> >>
> >>>>>> +
> >>>>>> +       return true;
> >>>>>>  }
> >>>>>>
> >>>>>>  static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
> >>>>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> >>>>>> index 738fa24..037e47a 100644
> >>>>>> --- a/drivers/mmc/host/dw_mmc.h
> >>>>>> +++ b/drivers/mmc/host/dw_mmc.h
> >>>>>> @@ -129,6 +129,7 @@
> >>>>>>  #define SDMMC_CMD_INDX(n)              ((n) & 0x1F)
> >>>>>>  /* Status register defines */
> >>>>>>  #define SDMMC_GET_FCNT(x)              (((x)>>17) & 0x1FFF)
> >>>>>> +#define SDMMC_STATUS_DMA_REQ           BIT(31)
> >>>>>>  /* FIFOTH register defines */
> >>>>>>  #define SDMMC_SET_FIFOTH(m, r, t)      (((m) & 0x7) << 28 | \
> >>>>>>                                          ((r) & 0xFFF) << 16 | \
> >>>>>> --
> >>>>>> 1.7.10.4
> >>>>>>
> >>>>>
> >>>>
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sonny Rao May 12, 2014, 9:14 p.m. UTC | #8
On Sat, May 10, 2014 at 7:08 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> Hi Sonny,
>
> Can you separate procedure?
> Reset all are handled in fifo-reset.
> And ciu reset is always needed for error handling?

Yes according to the document in the "Controller/DMA/FIFO Reset Usage"
section, the controller_reset bit should be set in all cases, so I
don't think that it should be separated.

>
> Thanks,
> Seungwon Jeon
>
> On Sat, May 10, 2014, Sonny Rao wrote:
>> On Fri, May 9, 2014 at 12:32 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> > Hi, Sonny.
>> >
>> > You can discard the my previous some comment.
>> > As you mentioned, this reset sequence is recommended at Synopsys TRM.
>> >
>> > Add the minor question.
>> >
>> > On 05/09/2014 01:27 PM, Jaehoon Chung wrote:
>> >> Hi, Sonny.
>> >>
>> >> I have checked the Synopsys TRM..
>> >>
>> >> On 05/09/2014 10:34 AM, Sonny Rao wrote:
>> >>> On Thu, May 8, 2014 at 6:15 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> >>>> On 05/08/2014 06:40 PM, Yuvaraj Kumar wrote:
>> >>>>> Any comments on this patch?
>> >>>>>
>> >>>>> On Wed, Mar 26, 2014 at 5:16 PM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote:
>> >>>>>> From: Sonny Rao <sonnyrao@chromium.org>
>> >>>>>>
>> >>>>>> This patch changes the fifo reset code to follow the reset procedure
>> >>>>>> outlined in the documentation of Synopsys  Mobile storage host databook
>> >>>>>> 7.2.13.
>> >>>>>> Without this patch, we could able to see eMMC was not detected after
>> >>>>>> multiple reboots due to driver hangs while eMMC tuning for HS200.
>> >>>>>>
>> >>>>>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> >>>>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.org>
>> >>>>>> ---
>> >>>>>>  drivers/mmc/host/dw_mmc.c |   48 ++++++++++++++++++++++++++++++++++++++++++++-
>> >>>>>>  drivers/mmc/host/dw_mmc.h |    1 +
>> >>>>>>  2 files changed, 48 insertions(+), 1 deletion(-)
>> >>>>>>
>> >>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> >>>>>> index 32dd81d..1d77431 100644
>> >>>>>> --- a/drivers/mmc/host/dw_mmc.c
>> >>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>> >>>>>> @@ -2220,7 +2220,53 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
>> >>>>>>                 host->sg = NULL;
>> >>>>>>         }
>> >>>>>>
>> >>>>>> -       return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>> >>>>>> +       /*
>> >>>>>> +        * The recommended method for resetting is to always reset the
>> >>>>>> +        * controller and the fifo, but differs slightly depending on the mode.
>> >>>>>> +        * Note that this doesn't handle the "generic DMA" (not IDMAC) case.
>> >>>>>> +        */
>> >>>>
>> >>>> "not IDMAC" is confused..
>> >>>>
>> >>>
>> >>> The documentation describes three different possible modes.  There's a
>> >>> mode that doesn't use IDMAC but still does DMA.  But as far as I can
>> >>> tell this driver doesn't support that way.  We can just remove that
>> >>> wording if it's confusing.
>> >
>> > How did it know whether dma is generic DMA or not?
>> >
>>
>> That's a good question.  I wasn't sure whether the driver supported it
>> or not.  It looks like it definitely supports IDMAC through the
>> CONFIG_MMC_DW_IDMAC flag, but I wasn't sure if it was supported the
>> generic dma.  Maybe if CONFIG_MMC_DW_IDMAC isn't specified but
>> host->dma_ops is not NULL then we are using the generic dma mode.
>>
>> >>>
>> >>>>>> +       if (dw_mci_ctrl_reset(host, SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET)) {
>> >>>>>> +               unsigned long timeout = jiffies + msecs_to_jiffies(500);
>> >>>>>> +               u32 status, rint;
>> >>>>>> +
>> >>>>>> +               /* if using dma we wait for dma_req to clear */
>> >>>>>> +               if (host->using_dma) {
>> >>>>>> +                       do {
>> >>>>>> +                               status = mci_readl(host, STATUS);
>> >>>>>> +                               if (!(status & SDMMC_STATUS_DMA_REQ))
>> >>>>>> +                                       break;
>> >>>>>> +                               cpu_relax();
>> >>>>>> +                       } while (time_before(jiffies, timeout));
>> >>>>>> +
>> >>>>>> +                       if (status & SDMMC_STATUS_DMA_REQ)
>> >>>>>> +                               dev_err(host->dev,
>> >>>>>> +                                       "%s: Timeout waiting for dma_req to "
>> >>>>>> +                                       "clear during reset", __func__);
>> >>>>>> +
>> >>>>>> +                       /* when using DMA next we reset the fifo again */
>> >>>>>> +                       dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>> >>>>>> +               }
>> >>>>>> +               /*
>> >>>>>> +                * In all cases we clear the RAWINTS register to clear any
>> >>>>>> +                * interrupts.
>> >>>>>> +                */
>> >>>>>> +               rint = mci_readl(host, RINTSTS);
>> >>>>>> +               rint = rint & (~mci_readl(host, MINTSTS));
>> >> you use the "status or temp" instead of rint. (you can reuse the variable.)
>> >> And can use "status &= ~mci_readl(host,MINTSTS);"
>> >>
>> >>>>
>> >>>> Just clear the RINTSTS register? why do you add these?
>> >>>>
>> >>>
>> >>> This will look at what is not masked, and only clear those bits.
>> >> Well, i known if clear the RINTSTS register,
>> >> recommended to use "0xfffffff" and set the value for interrupt.
>> >
>> > Can be used "0xfffffff"?
>> >
>>
>> Yeah we probably can.  We just lose information about interrupts that
>> were masked, but maybe we just don't care about any of them anyway, so
>> it doesn't matter.
>>
>> > Best Regards,
>> > Jaehoon Chung
>> >>
>> >>>
>> >>>>>> +               if (rint)
>> >>>>>> +                       mci_writel(host, RINTSTS, rint);
>> >>>>>> +
>> >>>>>> +       } else
>> >>>>>> +               dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
>> >>>>
>> >>>> Just display the error log? I didn't understand this.
>> >>>> If you displayed the error log, then it needs to return the error value.
>> >>>>
>> >>>>>> +
>> >>>>>> + #ifdef CONFIG_MMC_DW_IDMAC
>> >>>>>> +       /* It is also recommended that we reset and reprogram idmac */
>> >>>>>> +       dw_mci_idmac_reset(host);
>> >>>>>> + #endif
>> >>>>>> +
>> >>>>>> +       /* After a CTRL reset we need to have CIU set clock registers  */
>> >>>>>> +       mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
>> >>
>> >> Well, why do you send the clock update command?
>> >> I didn't see that update the value related with clock.
>> >>
>> >> Best Regards,
>> >> Jaehoon Chung
>> >>
>> >>>>>> +
>> >>>>>> +       return true;
>> >>>>>>  }
>> >>>>>>
>> >>>>>>  static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
>> >>>>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>> >>>>>> index 738fa24..037e47a 100644
>> >>>>>> --- a/drivers/mmc/host/dw_mmc.h
>> >>>>>> +++ b/drivers/mmc/host/dw_mmc.h
>> >>>>>> @@ -129,6 +129,7 @@
>> >>>>>>  #define SDMMC_CMD_INDX(n)              ((n) & 0x1F)
>> >>>>>>  /* Status register defines */
>> >>>>>>  #define SDMMC_GET_FCNT(x)              (((x)>>17) & 0x1FFF)
>> >>>>>> +#define SDMMC_STATUS_DMA_REQ           BIT(31)
>> >>>>>>  /* FIFOTH register defines */
>> >>>>>>  #define SDMMC_SET_FIFOTH(m, r, t)      (((m) & 0x7) << 28 | \
>> >>>>>>                                          ((r) & 0xFFF) << 16 | \
>> >>>>>> --
>> >>>>>> 1.7.10.4
>> >>>>>>
>> >>>>>
>> >>>>
>> >>> --
>> >>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> >>> the body of a message to majordomo@vger.kernel.org
>> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >>>
>> >>
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> >> the body of a message to majordomo@vger.kernel.org
>> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >>
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sonny Rao May 12, 2014, 9:44 p.m. UTC | #9
On Fri, May 9, 2014 at 8:36 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
> On Fri, May 9, 2014 at 12:32 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> Hi, Sonny.
>>
>> You can discard the my previous some comment.
>> As you mentioned, this reset sequence is recommended at Synopsys TRM.
>>
>> Add the minor question.
>>
>> On 05/09/2014 01:27 PM, Jaehoon Chung wrote:
>>> Hi, Sonny.
>>>
>>> I have checked the Synopsys TRM..
>>>
>>> On 05/09/2014 10:34 AM, Sonny Rao wrote:
>>>> On Thu, May 8, 2014 at 6:15 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>>> On 05/08/2014 06:40 PM, Yuvaraj Kumar wrote:
>>>>>> Any comments on this patch?
>>>>>>
>>>>>> On Wed, Mar 26, 2014 at 5:16 PM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote:
>>>>>>> From: Sonny Rao <sonnyrao@chromium.org>
>>>>>>>
>>>>>>> This patch changes the fifo reset code to follow the reset procedure
>>>>>>> outlined in the documentation of Synopsys  Mobile storage host databook
>>>>>>> 7.2.13.
>>>>>>> Without this patch, we could able to see eMMC was not detected after
>>>>>>> multiple reboots due to driver hangs while eMMC tuning for HS200.
>>>>>>>
>>>>>>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>>>>>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.org>
>>>>>>> ---
>>>>>>>  drivers/mmc/host/dw_mmc.c |   48 ++++++++++++++++++++++++++++++++++++++++++++-
>>>>>>>  drivers/mmc/host/dw_mmc.h |    1 +
>>>>>>>  2 files changed, 48 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>>>> index 32dd81d..1d77431 100644
>>>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>>>> @@ -2220,7 +2220,53 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
>>>>>>>                 host->sg = NULL;
>>>>>>>         }
>>>>>>>
>>>>>>> -       return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>>>>>>> +       /*
>>>>>>> +        * The recommended method for resetting is to always reset the
>>>>>>> +        * controller and the fifo, but differs slightly depending on the mode.
>>>>>>> +        * Note that this doesn't handle the "generic DMA" (not IDMAC) case.
>>>>>>> +        */
>>>>>
>>>>> "not IDMAC" is confused..
>>>>>
>>>>
>>>> The documentation describes three different possible modes.  There's a
>>>> mode that doesn't use IDMAC but still does DMA.  But as far as I can
>>>> tell this driver doesn't support that way.  We can just remove that
>>>> wording if it's confusing.
>>
>> How did it know whether dma is generic DMA or not?
>>
>
> That's a good question.  I wasn't sure whether the driver supported it
> or not.  It looks like it definitely supports IDMAC through the
> CONFIG_MMC_DW_IDMAC flag, but I wasn't sure if it was supported the
> generic dma.  Maybe if CONFIG_MMC_DW_IDMAC isn't specified but
> host->dma_ops is not NULL then we are using the generic dma mode.
>

Doug mentioned that James Hogan might have an answer.  James, are
there Imgtec SoCs which use dw_mmc and use DMA but don't use the
IDMAC?  If so, we can add that support into this reset procedure
patch.

>>>>
>>>>>>> +       if (dw_mci_ctrl_reset(host, SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET)) {
>>>>>>> +               unsigned long timeout = jiffies + msecs_to_jiffies(500);
>>>>>>> +               u32 status, rint;
>>>>>>> +
>>>>>>> +               /* if using dma we wait for dma_req to clear */
>>>>>>> +               if (host->using_dma) {
>>>>>>> +                       do {
>>>>>>> +                               status = mci_readl(host, STATUS);
>>>>>>> +                               if (!(status & SDMMC_STATUS_DMA_REQ))
>>>>>>> +                                       break;
>>>>>>> +                               cpu_relax();
>>>>>>> +                       } while (time_before(jiffies, timeout));
>>>>>>> +
>>>>>>> +                       if (status & SDMMC_STATUS_DMA_REQ)
>>>>>>> +                               dev_err(host->dev,
>>>>>>> +                                       "%s: Timeout waiting for dma_req to "
>>>>>>> +                                       "clear during reset", __func__);
>>>>>>> +
>>>>>>> +                       /* when using DMA next we reset the fifo again */
>>>>>>> +                       dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>>>>>>> +               }
>>>>>>> +               /*
>>>>>>> +                * In all cases we clear the RAWINTS register to clear any
>>>>>>> +                * interrupts.
>>>>>>> +                */
>>>>>>> +               rint = mci_readl(host, RINTSTS);
>>>>>>> +               rint = rint & (~mci_readl(host, MINTSTS));
>>> you use the "status or temp" instead of rint. (you can reuse the variable.)
>>> And can use "status &= ~mci_readl(host,MINTSTS);"
>>>
>>>>>
>>>>> Just clear the RINTSTS register? why do you add these?
>>>>>
>>>>
>>>> This will look at what is not masked, and only clear those bits.
>>> Well, i known if clear the RINTSTS register,
>>> recommended to use "0xfffffff" and set the value for interrupt.
>>
>> Can be used "0xfffffff"?
>>
>
> Yeah we probably can.  We just lose information about interrupts that
> were masked, but maybe we just don't care about any of them anyway, so
> it doesn't matter.
>
>> Best Regards,
>> Jaehoon Chung
>>>
>>>>
>>>>>>> +               if (rint)
>>>>>>> +                       mci_writel(host, RINTSTS, rint);
>>>>>>> +
>>>>>>> +       } else
>>>>>>> +               dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
>>>>>
>>>>> Just display the error log? I didn't understand this.
>>>>> If you displayed the error log, then it needs to return the error value.
>>>>>
>>>>>>> +
>>>>>>> + #ifdef CONFIG_MMC_DW_IDMAC
>>>>>>> +       /* It is also recommended that we reset and reprogram idmac */
>>>>>>> +       dw_mci_idmac_reset(host);
>>>>>>> + #endif
>>>>>>> +
>>>>>>> +       /* After a CTRL reset we need to have CIU set clock registers  */
>>>>>>> +       mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
>>>
>>> Well, why do you send the clock update command?
>>> I didn't see that update the value related with clock.
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>>>> +
>>>>>>> +       return true;
>>>>>>>  }
>>>>>>>
>>>>>>>  static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
>>>>>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>>>>>> index 738fa24..037e47a 100644
>>>>>>> --- a/drivers/mmc/host/dw_mmc.h
>>>>>>> +++ b/drivers/mmc/host/dw_mmc.h
>>>>>>> @@ -129,6 +129,7 @@
>>>>>>>  #define SDMMC_CMD_INDX(n)              ((n) & 0x1F)
>>>>>>>  /* Status register defines */
>>>>>>>  #define SDMMC_GET_FCNT(x)              (((x)>>17) & 0x1FFF)
>>>>>>> +#define SDMMC_STATUS_DMA_REQ           BIT(31)
>>>>>>>  /* FIFOTH register defines */
>>>>>>>  #define SDMMC_SET_FIFOTH(m, r, t)      (((m) & 0x7) << 28 | \
>>>>>>>                                          ((r) & 0xFFF) << 16 | \
>>>>>>> --
>>>>>>> 1.7.10.4
>>>>>>>
>>>>>>
>>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sonny Rao May 13, 2014, 12:40 a.m. UTC | #10
On Mon, May 12, 2014 at 2:44 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
> On Fri, May 9, 2014 at 8:36 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
>> On Fri, May 9, 2014 at 12:32 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>> Hi, Sonny.
>>>
>>> You can discard the my previous some comment.
>>> As you mentioned, this reset sequence is recommended at Synopsys TRM.
>>>
>>> Add the minor question.
>>>
>>> On 05/09/2014 01:27 PM, Jaehoon Chung wrote:
>>>> Hi, Sonny.
>>>>
>>>> I have checked the Synopsys TRM..
>>>>
>>>> On 05/09/2014 10:34 AM, Sonny Rao wrote:
>>>>> On Thu, May 8, 2014 at 6:15 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>>>> On 05/08/2014 06:40 PM, Yuvaraj Kumar wrote:
>>>>>>> Any comments on this patch?
>>>>>>>
>>>>>>> On Wed, Mar 26, 2014 at 5:16 PM, Yuvaraj Kumar C D <yuvaraj.cd@gmail.com> wrote:
>>>>>>>> From: Sonny Rao <sonnyrao@chromium.org>
>>>>>>>>
>>>>>>>> This patch changes the fifo reset code to follow the reset procedure
>>>>>>>> outlined in the documentation of Synopsys  Mobile storage host databook
>>>>>>>> 7.2.13.
>>>>>>>> Without this patch, we could able to see eMMC was not detected after
>>>>>>>> multiple reboots due to driver hangs while eMMC tuning for HS200.
>>>>>>>>
>>>>>>>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>>>>>>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.org>
>>>>>>>> ---
>>>>>>>>  drivers/mmc/host/dw_mmc.c |   48 ++++++++++++++++++++++++++++++++++++++++++++-
>>>>>>>>  drivers/mmc/host/dw_mmc.h |    1 +
>>>>>>>>  2 files changed, 48 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>>>>> index 32dd81d..1d77431 100644
>>>>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>>>>> @@ -2220,7 +2220,53 @@ static inline bool dw_mci_fifo_reset(struct dw_mci *host)
>>>>>>>>                 host->sg = NULL;
>>>>>>>>         }
>>>>>>>>
>>>>>>>> -       return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>>>>>>>> +       /*
>>>>>>>> +        * The recommended method for resetting is to always reset the
>>>>>>>> +        * controller and the fifo, but differs slightly depending on the mode.
>>>>>>>> +        * Note that this doesn't handle the "generic DMA" (not IDMAC) case.
>>>>>>>> +        */
>>>>>>
>>>>>> "not IDMAC" is confused..
>>>>>>
>>>>>
>>>>> The documentation describes three different possible modes.  There's a
>>>>> mode that doesn't use IDMAC but still does DMA.  But as far as I can
>>>>> tell this driver doesn't support that way.  We can just remove that
>>>>> wording if it's confusing.
>>>
>>> How did it know whether dma is generic DMA or not?
>>>
>>
>> That's a good question.  I wasn't sure whether the driver supported it
>> or not.  It looks like it definitely supports IDMAC through the
>> CONFIG_MMC_DW_IDMAC flag, but I wasn't sure if it was supported the
>> generic dma.  Maybe if CONFIG_MMC_DW_IDMAC isn't specified but
>> host->dma_ops is not NULL then we are using the generic dma mode.
>>
>
> Doug mentioned that James Hogan might have an answer.  James, are
> there Imgtec SoCs which use dw_mmc and use DMA but don't use the
> IDMAC?  If so, we can add that support into this reset procedure
> patch.
>

In any case, whether on not anybody is using it, it looks like it's
not that hard to support this mode for reset (I was just lazy the
first time), we just need to add a flag to our reset.  I've made that
change and cleaned it up a little bit more and I'll resend this patch.

>>>>>
>>>>>>>> +       if (dw_mci_ctrl_reset(host, SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET)) {
>>>>>>>> +               unsigned long timeout = jiffies + msecs_to_jiffies(500);
>>>>>>>> +               u32 status, rint;
>>>>>>>> +
>>>>>>>> +               /* if using dma we wait for dma_req to clear */
>>>>>>>> +               if (host->using_dma) {
>>>>>>>> +                       do {
>>>>>>>> +                               status = mci_readl(host, STATUS);
>>>>>>>> +                               if (!(status & SDMMC_STATUS_DMA_REQ))
>>>>>>>> +                                       break;
>>>>>>>> +                               cpu_relax();
>>>>>>>> +                       } while (time_before(jiffies, timeout));
>>>>>>>> +
>>>>>>>> +                       if (status & SDMMC_STATUS_DMA_REQ)
>>>>>>>> +                               dev_err(host->dev,
>>>>>>>> +                                       "%s: Timeout waiting for dma_req to "
>>>>>>>> +                                       "clear during reset", __func__);
>>>>>>>> +
>>>>>>>> +                       /* when using DMA next we reset the fifo again */
>>>>>>>> +                       dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>>>>>>>> +               }
>>>>>>>> +               /*
>>>>>>>> +                * In all cases we clear the RAWINTS register to clear any
>>>>>>>> +                * interrupts.
>>>>>>>> +                */
>>>>>>>> +               rint = mci_readl(host, RINTSTS);
>>>>>>>> +               rint = rint & (~mci_readl(host, MINTSTS));
>>>> you use the "status or temp" instead of rint. (you can reuse the variable.)
>>>> And can use "status &= ~mci_readl(host,MINTSTS);"
>>>>
>>>>>>
>>>>>> Just clear the RINTSTS register? why do you add these?
>>>>>>
>>>>>
>>>>> This will look at what is not masked, and only clear those bits.
>>>> Well, i known if clear the RINTSTS register,
>>>> recommended to use "0xfffffff" and set the value for interrupt.
>>>
>>> Can be used "0xfffffff"?
>>>
>>
>> Yeah we probably can.  We just lose information about interrupts that
>> were masked, but maybe we just don't care about any of them anyway, so
>> it doesn't matter.
>>
>>> Best Regards,
>>> Jaehoon Chung
>>>>
>>>>>
>>>>>>>> +               if (rint)
>>>>>>>> +                       mci_writel(host, RINTSTS, rint);
>>>>>>>> +
>>>>>>>> +       } else
>>>>>>>> +               dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
>>>>>>
>>>>>> Just display the error log? I didn't understand this.
>>>>>> If you displayed the error log, then it needs to return the error value.
>>>>>>
>>>>>>>> +
>>>>>>>> + #ifdef CONFIG_MMC_DW_IDMAC
>>>>>>>> +       /* It is also recommended that we reset and reprogram idmac */
>>>>>>>> +       dw_mci_idmac_reset(host);
>>>>>>>> + #endif
>>>>>>>> +
>>>>>>>> +       /* After a CTRL reset we need to have CIU set clock registers  */
>>>>>>>> +       mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
>>>>
>>>> Well, why do you send the clock update command?
>>>> I didn't see that update the value related with clock.
>>>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>>>>>> +
>>>>>>>> +       return true;
>>>>>>>>  }
>>>>>>>>
>>>>>>>>  static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
>>>>>>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>>>>>>> index 738fa24..037e47a 100644
>>>>>>>> --- a/drivers/mmc/host/dw_mmc.h
>>>>>>>> +++ b/drivers/mmc/host/dw_mmc.h
>>>>>>>> @@ -129,6 +129,7 @@
>>>>>>>>  #define SDMMC_CMD_INDX(n)              ((n) & 0x1F)
>>>>>>>>  /* Status register defines */
>>>>>>>>  #define SDMMC_GET_FCNT(x)              (((x)>>17) & 0x1FFF)
>>>>>>>> +#define SDMMC_STATUS_DMA_REQ           BIT(31)
>>>>>>>>  /* FIFOTH register defines */
>>>>>>>>  #define SDMMC_SET_FIFOTH(m, r, t)      (((m) & 0x7) << 28 | \
>>>>>>>>                                          ((r) & 0xFFF) << 16 | \
>>>>>>>> --
>>>>>>>> 1.7.10.4
>>>>>>>>
>>>>>>>
>>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Hogan May 13, 2014, 10:13 a.m. UTC | #11
Hi,

On 12/05/14 22:44, Sonny Rao wrote:
> Doug mentioned that James Hogan might have an answer.  James, are
> there Imgtec SoCs which use dw_mmc and use DMA but don't use the
> IDMAC?  If so, we can add that support into this reset procedure
> patch.

Yes, the Toumaz TZ1090 SoC has the dw_mmc configured without an IDMAC,
so it uses the IMG DMAC block instead.

Cheers
James
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 32dd81d..1d77431 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2220,7 +2220,53 @@  static inline bool dw_mci_fifo_reset(struct dw_mci *host)
 		host->sg = NULL;
 	}
 
-	return dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
+	/*
+	 * The recommended method for resetting is to always reset the
+	 * controller and the fifo, but differs slightly depending on the mode.
+	 * Note that this doesn't handle the "generic DMA" (not IDMAC) case.
+	 */
+	if (dw_mci_ctrl_reset(host, SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET)) {
+		unsigned long timeout = jiffies + msecs_to_jiffies(500);
+		u32 status, rint;
+
+		/* if using dma we wait for dma_req to clear */
+		if (host->using_dma) {
+			do {
+				status = mci_readl(host, STATUS);
+				if (!(status & SDMMC_STATUS_DMA_REQ))
+					break;
+				cpu_relax();
+			} while (time_before(jiffies, timeout));
+
+			if (status & SDMMC_STATUS_DMA_REQ)
+				dev_err(host->dev,
+					"%s: Timeout waiting for dma_req to "
+					"clear during reset", __func__);
+
+			/* when using DMA next we reset the fifo again */
+			dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
+		}
+		/*
+		 * In all cases we clear the RAWINTS register to clear any
+		 * interrupts.
+		 */
+		rint = mci_readl(host, RINTSTS);
+		rint = rint & (~mci_readl(host, MINTSTS));
+		if (rint)
+			mci_writel(host, RINTSTS, rint);
+
+	} else
+		dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
+
+ #ifdef CONFIG_MMC_DW_IDMAC
+	/* It is also recommended that we reset and reprogram idmac */
+	dw_mci_idmac_reset(host);
+ #endif
+
+	/* After a CTRL reset we need to have CIU set clock registers  */
+	mci_send_cmd(host->cur_slot, SDMMC_CMD_UPD_CLK, 0);
+
+	return true;
 }
 
 static inline bool dw_mci_ctrl_all_reset(struct dw_mci *host)
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 738fa24..037e47a 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -129,6 +129,7 @@ 
 #define SDMMC_CMD_INDX(n)		((n) & 0x1F)
 /* Status register defines */
 #define SDMMC_GET_FCNT(x)		(((x)>>17) & 0x1FFF)
+#define SDMMC_STATUS_DMA_REQ		BIT(31)
 /* FIFOTH register defines */
 #define SDMMC_SET_FIFOTH(m, r, t)	(((m) & 0x7) << 28 | \
 					 ((r) & 0xFFF) << 16 | \