[PATCH] mmc: dw_mmc: change to use recommended reset procedure
Sonny Rao
sonnyrao at chromium.org
Mon May 12 14:14:43 PDT 2014
On Sat, May 10, 2014 at 7:08 AM, Seungwon Jeon <tgih.jun at 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 at 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 at 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 at gmail.com> wrote:
>> >>>>>> From: Sonny Rao <sonnyrao at 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 at chromium.org>
>> >>>>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd at 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 at 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 at 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 at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
More information about the linux-arm-kernel
mailing list