diff mbox

[PATCHv2] mmc: dw_mmc: change to use recommended reset procedure

Message ID 1399945121-7387-1-git-send-email-sonnyrao@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sonny Rao May 13, 2014, 1:38 a.m. UTC
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.

v2: Add Generic DMA support
    per the documentation, move interrupt clear before wait
    make the test for DMA host->use_dma rather than host->using_dma
    add proper return values (although it appears no caller checks)

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

Comments

Seungwon Jeon May 13, 2014, 5:02 a.m. UTC | #1
As I mentioned in previous version, you put all reset stuff in existing fifo_reset function.
Although databook mentions ciu_reset case for SBE error, it's not obvious when ciu_reset is needed in other error cases.
If you intend to add some robust error handing, it would be better to make another function.
Please check my comments below.

On Tue, May 13, 2014, Sonny Rao wrote:
> 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.
> 
> v2: Add Generic DMA support
>     per the documentation, move interrupt clear before wait
>     make the test for DMA host->use_dma rather than host->using_dma
>     add proper return values (although it appears no caller checks)
> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
> ---
>  drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/mmc/host/dw_mmc.h |  1 +
>  2 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 55cd110..aff57e1 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2325,6 +2325,7 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
> 
>  static inline bool dw_mci_fifo_reset(struct dw_mci *host)
>  {
> +	u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
>  	/*
>  	 * Reseting generates a block interrupt, hence setting
>  	 * the scatter-gather pointer to NULL.
> @@ -2334,7 +2335,59 @@ 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.
> +	 * The Generic DMA mode (non IDMAC) also needs to reset DMA where IDMAC
> +	 * mode resets IDMAC at the end.
> +	 *
> +	 */
> +#ifndef CONFIG_MMC_DW_IDMAC
Is it added for generic DMA?
IDMAC should be considered for dma_reseet as well.
Please check databook.

> +	if (host->use_dma)
> +		flags |= SDMMC_CTRL_DMA_RESET;
> +#endif
> +	if (dw_mci_ctrl_reset(host, flags)) {
> +		/*
> +		 * In all cases we clear the RAWINTS register to clear any
> +		 * interrupts.
> +		 */
I think interrupt masking is needed before reset because we will not handle it anymore.
And Is there any reason to move interrupt clear here compared with previous version?

> +		mci_writel(host, RINTSTS, 0xFFFFFFFF);
> +
> +		/* if using dma we wait for dma_req to clear */
> +		if (host->use_dma) {
> +			unsigned long timeout = jiffies + msecs_to_jiffies(500);
> +			u32 status;
> +			do {
> +				status = mci_readl(host, STATUS);
> +				if (!(status & SDMMC_STATUS_DMA_REQ))
> +					break;
> +				cpu_relax();
What did you intend here?
If you intent busy-wait, how about using sleep instead?

> +			} 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__);
> +				return false;
> +			}
> +
> +			/* when using DMA next we reset the fifo again */
> +			dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
> +		}
> +	} else {
> +		dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
If ciu_reset is cleared, clk update below is needed?

Thanks,
Seungwon Jeon

> +		return false;
> +	}
> +
> +#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 6bf24ab..2505804 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.9.1.423.g4596e3a
> 
> --
> 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
Kim Kukjin May 13, 2014, 5:19 a.m. UTC | #2
Sonny Rao wrote:
> 
> 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.
> 
> v2: Add Generic DMA support
>     per the documentation, move interrupt clear before wait
>     make the test for DMA host->use_dma rather than host->using_dma
>     add proper return values (although it appears no caller checks)

Above changes should be put after following '---'.

> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
> ---

Here.

- Kukjin

>  drivers/mmc/host/dw_mmc.c | 55
> ++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/mmc/host/dw_mmc.h |  1 +
>  2 files changed, 55 insertions(+), 1 deletion(-)

--
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, 7:16 a.m. UTC | #3
On Mon, May 12, 2014 at 10:02 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> As I mentioned in previous version, you put all reset stuff in existing fifo_reset function.
> Although databook mentions ciu_reset case for SBE error, it's not obvious when ciu_reset is needed in other error cases.
> If you intend to add some robust error handing, it would be better to make another function.

The document I have actually doesn't mention error cases, it describes
this procedure as "Controller/DMA/FIFO Reset Usage" so I believe it is
saying this is the correct procedure in all cases, and based on our
testing it seems to work.  I understand the skepticism, as I shared it
initially when I saw this, but based on our experience, this is
correct and it doesn't need to live in a separate function.

> Please check my comments below.
>
> On Tue, May 13, 2014, Sonny Rao wrote:
>> 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.
>>
>> v2: Add Generic DMA support
>>     per the documentation, move interrupt clear before wait
>>     make the test for DMA host->use_dma rather than host->using_dma
>>     add proper return values (although it appears no caller checks)
>>
>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
>> ---
>>  drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  drivers/mmc/host/dw_mmc.h |  1 +
>>  2 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 55cd110..aff57e1 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -2325,6 +2325,7 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
>>
>>  static inline bool dw_mci_fifo_reset(struct dw_mci *host)
>>  {
>> +     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
>>       /*
>>        * Reseting generates a block interrupt, hence setting
>>        * the scatter-gather pointer to NULL.
>> @@ -2334,7 +2335,59 @@ 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.
>> +      * The Generic DMA mode (non IDMAC) also needs to reset DMA where IDMAC
>> +      * mode resets IDMAC at the end.
>> +      *
>> +      */
>> +#ifndef CONFIG_MMC_DW_IDMAC
> Is it added for generic DMA?
> IDMAC should be considered for dma_reseet as well.
> Please check databook.
>

Yeah it's a little unclear.  In the "7.2.13 Controller/DMA/FIFO Reset
Usage" part of the document they mention It is set for what they call
"generic" DMA, which I think is when there is an external DMA
controller, and the part below that it says for "DW-DMA/Non-DW-DMA"
that controller_reset and fifo_reset should be set.   I believe this
"DW-DMA" case refers to what is called IDMAC.  So, I think it's not
required for this case, but I admit I'm not sure why they also say
"Non-DW-DMA". If you know of a good way to differentiate the "Generic
DMA" case  I can implement it.  It wasn't clear to me if the driver
even supported this "generic" dma case, but it sounds like it might,
so I added this code.  In practice, on the Exynos Systems we have,
which are using IDMAC, the reset procedure seems to work okay without
the dma_reset bit set, but I cannot test this "generic dma" case.

The other places in the doc where I see them mention the dma_reset bit
are "7.2.21 Transmission and Reception with Internal DMAC (IDMAC)"
and the description of the CTRL register, and in "7.1
Software/Hardware Restrictions" where they say it will terminate any
pending transfer.

>> +     if (host->use_dma)
>> +             flags |= SDMMC_CTRL_DMA_RESET;
>> +#endif
>> +     if (dw_mci_ctrl_reset(host, flags)) {
>> +             /*
>> +              * In all cases we clear the RAWINTS register to clear any
>> +              * interrupts.
>> +              */
> I think interrupt masking is needed before reset because we will not handle it anymore.
> And Is there any reason to move interrupt clear here compared with previous version?

Yeah I moved it to match the description in the document more closely.
 The documents mentioned doing the interrupt clear after setting the
reset bits, and before waiting for the dma_req bit in the status
register to clear.  We've been running it with the interrupt clear
below the loop, for a while, and I just tested it with the clear above
the wait to make sure it still works properly and I was able to pass
the tuning process with some errors, so I believe this works fine too,
and more closely matches the description in the doc that I have.

>> +             mci_writel(host, RINTSTS, 0xFFFFFFFF);
>> +
>> +             /* if using dma we wait for dma_req to clear */
>> +             if (host->use_dma) {
>> +                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
>> +                     u32 status;
>> +                     do {
>> +                             status = mci_readl(host, STATUS);
>> +                             if (!(status & SDMMC_STATUS_DMA_REQ))
>> +                                     break;
>> +                             cpu_relax();
> What did you intend here?
> If you intent busy-wait, how about using sleep instead?

Yes it is a busy-wait.  There is similar code in dw_mci_ctrl_reset,
where that function is polling without sleeps, so I was trying to
match that.  The cpu_relax() is something I saw in other busy-waits in
the kernel, but I'm happy to take it out if you'd like.

>> +                     } 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__);
>> +                             return false;
>> +                     }
>> +
>> +                     /* when using DMA next we reset the fifo again */
>> +                     dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>> +             }
>> +     } else {
>> +             dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
> If ciu_reset is cleared, clk update below is needed?

I'm honestly not sure what happens if the reset bits don't clear.  The
docs say controller_reset and dma_reset will auto clear after a number
of clocks, but fifo_reset will clear "after completion of the reset
operation"  So in this particular error case I'm not sure if it's
possible to recover properly or not and might hang, so I thought it
best to just return the error immediately.

> Thanks,
> Seungwon Jeon
>
>> +             return false;
>> +     }
>> +
>> +#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 6bf24ab..2505804 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.9.1.423.g4596e3a
>>
>> --
>> 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
Arnd Bergmann May 13, 2014, 8:46 a.m. UTC | #4
On Monday 12 May 2014 18:38:41 Sonny Rao wrote:
> +#ifndef CONFIG_MMC_DW_IDMAC
> +       if (host->use_dma)
> +               flags |= SDMMC_CTRL_DMA_RESET;
> +#endif

Can you change the above like this?

	if (IS_ENABLED(CONFIG_MMC_DW_IDMAC) && host->use_dma)
		flags |= SDMMC_CTRL_DMA_RESET;

That is generally considered the preferred style these days.

	Arnd
--
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 13, 2014, 11:09 a.m. UTC | #5
On Tuesday, May 13, Sonny Rao wrote:
> On Mon, May 12, 2014 at 10:02 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> > As I mentioned in previous version, you put all reset stuff in existing fifo_reset function.
> > Although databook mentions ciu_reset case for SBE error, it's not obvious when ciu_reset is needed
> in other error cases.
> > If you intend to add some robust error handing, it would be better to make another function.
> 
> The document I have actually doesn't mention error cases, it describes
> this procedure as "Controller/DMA/FIFO Reset Usage" so I believe it is
> saying this is the correct procedure in all cases, and based on our
> testing it seems to work.  I understand the skepticism, as I shared it
> initially when I saw this, but based on our experience, this is
> correct and it doesn't need to live in a separate function.
I agree this active error handling. 
But, existing fifo_reset function is focused on fifo reset purely.
I think your change is close to error recovery and it seems overloaded to basic function.
So, you suggest renaming function for new sequence.
And look into dw_mci_work_routine_card(). dw_mci_idmac_reset() is redundancy.
I expect it can be cleaned.

<Quot>
                                /* Clear down the FIFO */
                                dw_mci_fifo_reset(host);
#ifdef CONFIG_MMC_DW_IDMAC
                                dw_mci_idmac_reset(host);
#endif
</Quot>

> 
> > Please check my comments below.
> >
> > On Tue, May 13, 2014, Sonny Rao wrote:
> >> 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.
Please remove this section number.
No needed. It's old version.

> >>
> >> v2: Add Generic DMA support
> >>     per the documentation, move interrupt clear before wait
> >>     make the test for DMA host->use_dma rather than host->using_dma
> >>     add proper return values (although it appears no caller checks)
> >>
> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> >> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
> >> ---
> >>  drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-
> >>  drivers/mmc/host/dw_mmc.h |  1 +
> >>  2 files changed, 55 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> >> index 55cd110..aff57e1 100644
> >> --- a/drivers/mmc/host/dw_mmc.c
> >> +++ b/drivers/mmc/host/dw_mmc.c
> >> @@ -2325,6 +2325,7 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
> >>
> >>  static inline bool dw_mci_fifo_reset(struct dw_mci *host)
> >>  {
> >> +     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
> >>       /*
> >>        * Reseting generates a block interrupt, hence setting
> >>        * the scatter-gather pointer to NULL.
> >> @@ -2334,7 +2335,59 @@ 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.
> >> +      * The Generic DMA mode (non IDMAC) also needs to reset DMA where IDMAC
> >> +      * mode resets IDMAC at the end.
> >> +      *
> >> +      */
> >> +#ifndef CONFIG_MMC_DW_IDMAC
> > Is it added for generic DMA?
> > IDMAC should be considered for dma_reseet as well.
> > Please check databook.
> >
> 
> Yeah it's a little unclear.  In the "7.2.13 Controller/DMA/FIFO Reset
> Usage" part of the document they mention It is set for what they call
> "generic" DMA, which I think is when there is an external DMA
> controller, and the part below that it says for "DW-DMA/Non-DW-DMA"
> that controller_reset and fifo_reset should be set.   I believe this
> "DW-DMA" case refers to what is called IDMAC.  So, I think it's not
> required for this case, but I admit I'm not sure why they also say
> "Non-DW-DMA". If you know of a good way to differentiate the "Generic
> DMA" case  I can implement it.  It wasn't clear to me if the driver
> even supported this "generic" dma case, but it sounds like it might,
> so I added this code.  In practice, on the Exynos Systems we have,
> which are using IDMAC, the reset procedure seems to work okay without
> the dma_reset bit set, but I cannot test this "generic dma" case.
> 
> The other places in the doc where I see them mention the dma_reset bit
> are "7.2.21 Transmission and Reception with Internal DMAC (IDMAC)"
> and the description of the CTRL register, and in "7.1
> Software/Hardware Restrictions" where they say it will terminate any
> pending transfer.
"DW-DMA" means Synopsys's DMA controller not IDMAC.
SDMMC_CTRL_DMA_RESET can apply in all type DMA interface.

> 
> >> +     if (host->use_dma)
> >> +             flags |= SDMMC_CTRL_DMA_RESET;
> >> +#endif
> >> +     if (dw_mci_ctrl_reset(host, flags)) {
> >> +             /*
> >> +              * In all cases we clear the RAWINTS register to clear any
> >> +              * interrupts.
> >> +              */
> > I think interrupt masking is needed before reset because we will not handle it anymore.
> > And Is there any reason to move interrupt clear here compared with previous version?
> 
> Yeah I moved it to match the description in the document more closely.
>  The documents mentioned doing the interrupt clear after setting the
> reset bits, and before waiting for the dma_req bit in the status
> register to clear.  We've been running it with the interrupt clear
> below the loop, for a while, and I just tested it with the clear above
> the wait to make sure it still works properly and I was able to pass
> the tuning process with some errors, so I believe this works fine too,
> and more closely matches the description in the doc that I have.
When I tried ciu_reset, I found some unexpected interrupts occurred.
It means that interrupt handler will run to handle extra interrupts.
Then, we may need mask the interrupt.
Can you check it for test?

> 
> >> +             mci_writel(host, RINTSTS, 0xFFFFFFFF);
> >> +
> >> +             /* if using dma we wait for dma_req to clear */
> >> +             if (host->use_dma) {
> >> +                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
> >> +                     u32 status;
> >> +                     do {
> >> +                             status = mci_readl(host, STATUS);
> >> +                             if (!(status & SDMMC_STATUS_DMA_REQ))
> >> +                                     break;
> >> +                             cpu_relax();
> > What did you intend here?
> > If you intent busy-wait, how about using sleep instead?
> 
> Yes it is a busy-wait.  There is similar code in dw_mci_ctrl_reset,
> where that function is polling without sleeps, so I was trying to
> match that.  The cpu_relax() is something I saw in other busy-waits in
> the kernel, but I'm happy to take it out if you'd like.
In case of ctrl_reset, waiting is during 2 clock.
If this polling status is not long, I'm OK.

> 
> >> +                     } 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__);
> >> +                             return false;
> >> +                     }
> >> +
> >> +                     /* when using DMA next we reset the fifo again */
> >> +                     dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
> >> +             }
> >> +     } else {
> >> +             dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
> > If ciu_reset is cleared, clk update below is needed?
> 
> I'm honestly not sure what happens if the reset bits don't clear.  The
> docs say controller_reset and dma_reset will auto clear after a number
> of clocks, but fifo_reset will clear "after completion of the reset
> operation"  So in this particular error case I'm not sure if it's
> possible to recover properly or not and might hang, so I thought it
> best to just return the error immediately.
Yes.
But at least if ciu_reset is done successfully, it may need clock update sequence again.
In addition, printing reset bit[2:0] will be helpful for debug information.

Thanks,
Seungwon Jeon

> 
> > Thanks,
> > Seungwon Jeon
> >
> >> +             return false;
> >> +     }
> >> +
> >> +#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 6bf24ab..2505804 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.9.1.423.g4596e3a
> >>
> >> --
> >> 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 17, 2014, 12:36 a.m. UTC | #6
On Tue, May 13, 2014 at 4:09 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> On Tuesday, May 13, Sonny Rao wrote:
>> On Mon, May 12, 2014 at 10:02 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
>> > As I mentioned in previous version, you put all reset stuff in existing fifo_reset function.
>> > Although databook mentions ciu_reset case for SBE error, it's not obvious when ciu_reset is needed
>> in other error cases.
>> > If you intend to add some robust error handing, it would be better to make another function.
>>
>> The document I have actually doesn't mention error cases, it describes
>> this procedure as "Controller/DMA/FIFO Reset Usage" so I believe it is
>> saying this is the correct procedure in all cases, and based on our
>> testing it seems to work.  I understand the skepticism, as I shared it
>> initially when I saw this, but based on our experience, this is
>> correct and it doesn't need to live in a separate function.
> I agree this active error handling.
> But, existing fifo_reset function is focused on fifo reset purely.
> I think your change is close to error recovery and it seems overloaded to basic function.
> So, you suggest renaming function for new sequence.

I think the documentation says it should always be done, not just in
error recovery.  I can rename the function to dw_mci_reset rather than
dw_mci_fifo_reset.  Is that what you mean? Or do you mean make a new
function that is only called in error cases?

> And look into dw_mci_work_routine_card(). dw_mci_idmac_reset() is redundancy.
> I expect it can be cleaned.
>
> <Quot>
>                                 /* Clear down the FIFO */
>                                 dw_mci_fifo_reset(host);
> #ifdef CONFIG_MMC_DW_IDMAC
>                                 dw_mci_idmac_reset(host);
> #endif
> </Quot>
>

Ok, I'll remove that extra reset, thanks for catching.

>>
>> > Please check my comments below.
>> >
>> > On Tue, May 13, 2014, Sonny Rao wrote:
>> >> 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.
> Please remove this section number.
> No needed. It's old version.
>

Ok

>> >>
>> >> v2: Add Generic DMA support
>> >>     per the documentation, move interrupt clear before wait
>> >>     make the test for DMA host->use_dma rather than host->using_dma
>> >>     add proper return values (although it appears no caller checks)
>> >>
>> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> >> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
>> >> ---
>> >>  drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-
>> >>  drivers/mmc/host/dw_mmc.h |  1 +
>> >>  2 files changed, 55 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> >> index 55cd110..aff57e1 100644
>> >> --- a/drivers/mmc/host/dw_mmc.c
>> >> +++ b/drivers/mmc/host/dw_mmc.c
>> >> @@ -2325,6 +2325,7 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
>> >>
>> >>  static inline bool dw_mci_fifo_reset(struct dw_mci *host)
>> >>  {
>> >> +     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
>> >>       /*
>> >>        * Reseting generates a block interrupt, hence setting
>> >>        * the scatter-gather pointer to NULL.
>> >> @@ -2334,7 +2335,59 @@ 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.
>> >> +      * The Generic DMA mode (non IDMAC) also needs to reset DMA where IDMAC
>> >> +      * mode resets IDMAC at the end.
>> >> +      *
>> >> +      */
>> >> +#ifndef CONFIG_MMC_DW_IDMAC
>> > Is it added for generic DMA?
>> > IDMAC should be considered for dma_reseet as well.
>> > Please check databook.
>> >
>>
>> Yeah it's a little unclear.  In the "7.2.13 Controller/DMA/FIFO Reset
>> Usage" part of the document they mention It is set for what they call
>> "generic" DMA, which I think is when there is an external DMA
>> controller, and the part below that it says for "DW-DMA/Non-DW-DMA"
>> that controller_reset and fifo_reset should be set.   I believe this
>> "DW-DMA" case refers to what is called IDMAC.  So, I think it's not
>> required for this case, but I admit I'm not sure why they also say
>> "Non-DW-DMA". If you know of a good way to differentiate the "Generic
>> DMA" case  I can implement it.  It wasn't clear to me if the driver
>> even supported this "generic" dma case, but it sounds like it might,
>> so I added this code.  In practice, on the Exynos Systems we have,
>> which are using IDMAC, the reset procedure seems to work okay without
>> the dma_reset bit set, but I cannot test this "generic dma" case.
>>
>> The other places in the doc where I see them mention the dma_reset bit
>> are "7.2.21 Transmission and Reception with Internal DMAC (IDMAC)"
>> and the description of the CTRL register, and in "7.1
>> Software/Hardware Restrictions" where they say it will terminate any
>> pending transfer.
> "DW-DMA" means Synopsys's DMA controller not IDMAC.
> SDMMC_CTRL_DMA_RESET can apply in all type DMA interface.

So, do you think I should always set that bit if we're using DMA?

>
>>
>> >> +     if (host->use_dma)
>> >> +             flags |= SDMMC_CTRL_DMA_RESET;
>> >> +#endif
>> >> +     if (dw_mci_ctrl_reset(host, flags)) {
>> >> +             /*
>> >> +              * In all cases we clear the RAWINTS register to clear any
>> >> +              * interrupts.
>> >> +              */
>> > I think interrupt masking is needed before reset because we will not handle it anymore.
>> > And Is there any reason to move interrupt clear here compared with previous version?
>>
>> Yeah I moved it to match the description in the document more closely.
>>  The documents mentioned doing the interrupt clear after setting the
>> reset bits, and before waiting for the dma_req bit in the status
>> register to clear.  We've been running it with the interrupt clear
>> below the loop, for a while, and I just tested it with the clear above
>> the wait to make sure it still works properly and I was able to pass
>> the tuning process with some errors, so I believe this works fine too,
>> and more closely matches the description in the doc that I have.
> When I tried ciu_reset, I found some unexpected interrupts occurred.
> It means that interrupt handler will run to handle extra interrupts.
> Then, we may need mask the interrupt.
> Can you check it for test?

You're right.  When I have the controller reset bit set, I see one
interrupt coming in shortly after we write the reset bits, which has
status bits of 0x180.  This has Response timeout (RTO) and Data CRC
error set.  It sounds like some interrupt ocurring may be expected
since they say to clear RINTSTS as part of the procedure though.


>
>>
>> >> +             mci_writel(host, RINTSTS, 0xFFFFFFFF);
>> >> +
>> >> +             /* if using dma we wait for dma_req to clear */
>> >> +             if (host->use_dma) {
>> >> +                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
>> >> +                     u32 status;
>> >> +                     do {
>> >> +                             status = mci_readl(host, STATUS);
>> >> +                             if (!(status & SDMMC_STATUS_DMA_REQ))
>> >> +                                     break;
>> >> +                             cpu_relax();
>> > What did you intend here?
>> > If you intent busy-wait, how about using sleep instead?
>>
>> Yes it is a busy-wait.  There is similar code in dw_mci_ctrl_reset,
>> where that function is polling without sleeps, so I was trying to
>> match that.  The cpu_relax() is something I saw in other busy-waits in
>> the kernel, but I'm happy to take it out if you'd like.
> In case of ctrl_reset, waiting is during 2 clock.
> If this polling status is not long, I'm OK.
>
>>
>> >> +                     } 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__);
>> >> +                             return false;
>> >> +                     }
>> >> +
>> >> +                     /* when using DMA next we reset the fifo again */
>> >> +                     dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>> >> +             }
>> >> +     } else {
>> >> +             dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
>> > If ciu_reset is cleared, clk update below is needed?
>>
>> I'm honestly not sure what happens if the reset bits don't clear.  The
>> docs say controller_reset and dma_reset will auto clear after a number
>> of clocks, but fifo_reset will clear "after completion of the reset
>> operation"  So in this particular error case I'm not sure if it's
>> possible to recover properly or not and might hang, so I thought it
>> best to just return the error immediately.
> Yes.
> But at least if ciu_reset is done successfully, it may need clock update sequence again.
> In addition, printing reset bit[2:0] will be helpful for debug information.

The dw_mci_ctrl_reset() function would run this code if some of the
bits didn't clear:

        dev_err(host->dev,
                "Timeout resetting block (ctrl reset %#x)\n",
                ctrl & reset);

Does that give you what you're looking for?

>
> Thanks,
> Seungwon Jeon
>
>>
>> > Thanks,
>> > Seungwon Jeon
>> >
>> >> +             return false;
>> >> +     }
>> >> +
>> >> +#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 6bf24ab..2505804 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.9.1.423.g4596e3a
>> >>
>> >> --
>> >> 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 20, 2014, 1:49 a.m. UTC | #7
On Sat, May 17, 2014, Sonny Rao wrote:
> On Tue, May 13, 2014 at 4:09 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> > On Tuesday, May 13, Sonny Rao wrote:
> >> On Mon, May 12, 2014 at 10:02 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> >> > As I mentioned in previous version, you put all reset stuff in existing fifo_reset function.
> >> > Although databook mentions ciu_reset case for SBE error, it's not obvious when ciu_reset is
> needed
> >> in other error cases.
> >> > If you intend to add some robust error handing, it would be better to make another function.
> >>
> >> The document I have actually doesn't mention error cases, it describes
> >> this procedure as "Controller/DMA/FIFO Reset Usage" so I believe it is
> >> saying this is the correct procedure in all cases, and based on our
> >> testing it seems to work.  I understand the skepticism, as I shared it
> >> initially when I saw this, but based on our experience, this is
> >> correct and it doesn't need to live in a separate function.
> > I agree this active error handling.
> > But, existing fifo_reset function is focused on fifo reset purely.
> > I think your change is close to error recovery and it seems overloaded to basic function.
> > So, you suggest renaming function for new sequence.
> 
> I think the documentation says it should always be done, not just in
> error recovery.  I can rename the function to dw_mci_reset rather than
> dw_mci_fifo_reset.  Is that what you mean? Or do you mean make a new
> function that is only called in error cases?
Both are okay.

> 
> > And look into dw_mci_work_routine_card(). dw_mci_idmac_reset() is redundancy.
> > I expect it can be cleaned.
> >
> > <Quot>
> >                                 /* Clear down the FIFO */
> >                                 dw_mci_fifo_reset(host);
> > #ifdef CONFIG_MMC_DW_IDMAC
> >                                 dw_mci_idmac_reset(host);
> > #endif
> > </Quot>
> >
> 
> Ok, I'll remove that extra reset, thanks for catching.
> 
> >>
> >> > Please check my comments below.
> >> >
> >> > On Tue, May 13, 2014, Sonny Rao wrote:
> >> >> 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.
> > Please remove this section number.
> > No needed. It's old version.
> >
> 
> Ok
> 
> >> >>
> >> >> v2: Add Generic DMA support
> >> >>     per the documentation, move interrupt clear before wait
> >> >>     make the test for DMA host->use_dma rather than host->using_dma
> >> >>     add proper return values (although it appears no caller checks)
> >> >>
> >> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> >> >> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
> >> >> ---
> >> >>  drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-
> >> >>  drivers/mmc/host/dw_mmc.h |  1 +
> >> >>  2 files changed, 55 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> >> >> index 55cd110..aff57e1 100644
> >> >> --- a/drivers/mmc/host/dw_mmc.c
> >> >> +++ b/drivers/mmc/host/dw_mmc.c
> >> >> @@ -2325,6 +2325,7 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
> >> >>
> >> >>  static inline bool dw_mci_fifo_reset(struct dw_mci *host)
> >> >>  {
> >> >> +     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
> >> >>       /*
> >> >>        * Reseting generates a block interrupt, hence setting
> >> >>        * the scatter-gather pointer to NULL.
> >> >> @@ -2334,7 +2335,59 @@ 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.
> >> >> +      * The Generic DMA mode (non IDMAC) also needs to reset DMA where IDMAC
> >> >> +      * mode resets IDMAC at the end.
> >> >> +      *
> >> >> +      */
> >> >> +#ifndef CONFIG_MMC_DW_IDMAC
> >> > Is it added for generic DMA?
> >> > IDMAC should be considered for dma_reseet as well.
> >> > Please check databook.
> >> >
> >>
> >> Yeah it's a little unclear.  In the "7.2.13 Controller/DMA/FIFO Reset
> >> Usage" part of the document they mention It is set for what they call
> >> "generic" DMA, which I think is when there is an external DMA
> >> controller, and the part below that it says for "DW-DMA/Non-DW-DMA"
> >> that controller_reset and fifo_reset should be set.   I believe this
> >> "DW-DMA" case refers to what is called IDMAC.  So, I think it's not
> >> required for this case, but I admit I'm not sure why they also say
> >> "Non-DW-DMA". If you know of a good way to differentiate the "Generic
> >> DMA" case  I can implement it.  It wasn't clear to me if the driver
> >> even supported this "generic" dma case, but it sounds like it might,
> >> so I added this code.  In practice, on the Exynos Systems we have,
> >> which are using IDMAC, the reset procedure seems to work okay without
> >> the dma_reset bit set, but I cannot test this "generic dma" case.
> >>
> >> The other places in the doc where I see them mention the dma_reset bit
> >> are "7.2.21 Transmission and Reception with Internal DMAC (IDMAC)"
> >> and the description of the CTRL register, and in "7.1
> >> Software/Hardware Restrictions" where they say it will terminate any
> >> pending transfer.
> > "DW-DMA" means Synopsys's DMA controller not IDMAC.
> > SDMMC_CTRL_DMA_RESET can apply in all type DMA interface.
> 
> So, do you think I should always set that bit if we're using DMA?
Yes. Although you used it for general dma at first, IDMAC also needs it.

> 
> >
> >>
> >> >> +     if (host->use_dma)
> >> >> +             flags |= SDMMC_CTRL_DMA_RESET;
> >> >> +#endif
> >> >> +     if (dw_mci_ctrl_reset(host, flags)) {
> >> >> +             /*
> >> >> +              * In all cases we clear the RAWINTS register to clear any
> >> >> +              * interrupts.
> >> >> +              */
> >> > I think interrupt masking is needed before reset because we will not handle it anymore.
> >> > And Is there any reason to move interrupt clear here compared with previous version?
> >>
> >> Yeah I moved it to match the description in the document more closely.
> >>  The documents mentioned doing the interrupt clear after setting the
> >> reset bits, and before waiting for the dma_req bit in the status
> >> register to clear.  We've been running it with the interrupt clear
> >> below the loop, for a while, and I just tested it with the clear above
> >> the wait to make sure it still works properly and I was able to pass
> >> the tuning process with some errors, so I believe this works fine too,
> >> and more closely matches the description in the doc that I have.
> > When I tried ciu_reset, I found some unexpected interrupts occurred.
> > It means that interrupt handler will run to handle extra interrupts.
> > Then, we may need mask the interrupt.
> > Can you check it for test?
> 
> You're right.  When I have the controller reset bit set, I see one
> interrupt coming in shortly after we write the reset bits, which has
> status bits of 0x180.  This has Response timeout (RTO) and Data CRC
> error set.  It sounds like some interrupt ocurring may be expected
> since they say to clear RINTSTS as part of the procedure though.
Yes, it needs to clear RINTSTS.
While this interrupt occurs, interrupt handler is called and thus tasklet function is also called.
Can we handle this unexpected interrupt properly?
To prevent these calling, we may need interrupt mask before resetting.

> 
> 
> >
> >>
> >> >> +             mci_writel(host, RINTSTS, 0xFFFFFFFF);
> >> >> +
> >> >> +             /* if using dma we wait for dma_req to clear */
> >> >> +             if (host->use_dma) {
> >> >> +                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
> >> >> +                     u32 status;
> >> >> +                     do {
> >> >> +                             status = mci_readl(host, STATUS);
> >> >> +                             if (!(status & SDMMC_STATUS_DMA_REQ))
> >> >> +                                     break;
> >> >> +                             cpu_relax();
> >> > What did you intend here?
> >> > If you intent busy-wait, how about using sleep instead?
> >>
> >> Yes it is a busy-wait.  There is similar code in dw_mci_ctrl_reset,
> >> where that function is polling without sleeps, so I was trying to
> >> match that.  The cpu_relax() is something I saw in other busy-waits in
> >> the kernel, but I'm happy to take it out if you'd like.
> > In case of ctrl_reset, waiting is during 2 clock.
> > If this polling status is not long, I'm OK.
> >
> >>
> >> >> +                     } 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__);
> >> >> +                             return false;
> >> >> +                     }
> >> >> +
> >> >> +                     /* when using DMA next we reset the fifo again */
> >> >> +                     dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
> >> >> +             }
> >> >> +     } else {
> >> >> +             dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
> >> > If ciu_reset is cleared, clk update below is needed?
> >>
> >> I'm honestly not sure what happens if the reset bits don't clear.  The
> >> docs say controller_reset and dma_reset will auto clear after a number
> >> of clocks, but fifo_reset will clear "after completion of the reset
> >> operation"  So in this particular error case I'm not sure if it's
> >> possible to recover properly or not and might hang, so I thought it
> >> best to just return the error immediately.
> > Yes.
> > But at least if ciu_reset is done successfully, it may need clock update sequence again.
> > In addition, printing reset bit[2:0] will be helpful for debug information.
> 
> The dw_mci_ctrl_reset() function would run this code if some of the
> bits didn't clear:
> 
>         dev_err(host->dev,
>                 "Timeout resetting block (ctrl reset %#x)\n",
>                 ctrl & reset);
> 
> Does that give you what you're looking for?
Ah, it was there.

Thanks,
Seungwon Jeon

> 
> >
> > Thanks,
> > Seungwon Jeon
> >
> >>
> >> > Thanks,
> >> > Seungwon Jeon
> >> >
> >> >> +             return false;
> >> >> +     }
> >> >> +
> >> >> +#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 6bf24ab..2505804 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.9.1.423.g4596e3a
> >> >>
> >> >> --
> >> >> 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 22, 2014, 6:54 p.m. UTC | #8
On Mon, May 19, 2014 at 6:49 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> On Sat, May 17, 2014, Sonny Rao wrote:
>> On Tue, May 13, 2014 at 4:09 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
>> > On Tuesday, May 13, Sonny Rao wrote:
>> >> On Mon, May 12, 2014 at 10:02 PM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
>> >> > As I mentioned in previous version, you put all reset stuff in existing fifo_reset function.
>> >> > Although databook mentions ciu_reset case for SBE error, it's not obvious when ciu_reset is
>> needed
>> >> in other error cases.
>> >> > If you intend to add some robust error handing, it would be better to make another function.
>> >>
>> >> The document I have actually doesn't mention error cases, it describes
>> >> this procedure as "Controller/DMA/FIFO Reset Usage" so I believe it is
>> >> saying this is the correct procedure in all cases, and based on our
>> >> testing it seems to work.  I understand the skepticism, as I shared it
>> >> initially when I saw this, but based on our experience, this is
>> >> correct and it doesn't need to live in a separate function.
>> > I agree this active error handling.
>> > But, existing fifo_reset function is focused on fifo reset purely.
>> > I think your change is close to error recovery and it seems overloaded to basic function.
>> > So, you suggest renaming function for new sequence.
>>
>> I think the documentation says it should always be done, not just in
>> error recovery.  I can rename the function to dw_mci_reset rather than
>> dw_mci_fifo_reset.  Is that what you mean? Or do you mean make a new
>> function that is only called in error cases?
> Both are okay.
>

Ok

>>
>> > And look into dw_mci_work_routine_card(). dw_mci_idmac_reset() is redundancy.
>> > I expect it can be cleaned.
>> >
>> > <Quot>
>> >                                 /* Clear down the FIFO */
>> >                                 dw_mci_fifo_reset(host);
>> > #ifdef CONFIG_MMC_DW_IDMAC
>> >                                 dw_mci_idmac_reset(host);
>> > #endif
>> > </Quot>
>> >
>>
>> Ok, I'll remove that extra reset, thanks for catching.
>>
>> >>
>> >> > Please check my comments below.
>> >> >
>> >> > On Tue, May 13, 2014, Sonny Rao wrote:
>> >> >> 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.
>> > Please remove this section number.
>> > No needed. It's old version.
>> >
>>
>> Ok
>>
>> >> >>
>> >> >> v2: Add Generic DMA support
>> >> >>     per the documentation, move interrupt clear before wait
>> >> >>     make the test for DMA host->use_dma rather than host->using_dma
>> >> >>     add proper return values (although it appears no caller checks)
>> >> >>
>> >> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> >> >> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
>> >> >> ---
>> >> >>  drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-
>> >> >>  drivers/mmc/host/dw_mmc.h |  1 +
>> >> >>  2 files changed, 55 insertions(+), 1 deletion(-)
>> >> >>
>> >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> >> >> index 55cd110..aff57e1 100644
>> >> >> --- a/drivers/mmc/host/dw_mmc.c
>> >> >> +++ b/drivers/mmc/host/dw_mmc.c
>> >> >> @@ -2325,6 +2325,7 @@ static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
>> >> >>
>> >> >>  static inline bool dw_mci_fifo_reset(struct dw_mci *host)
>> >> >>  {
>> >> >> +     u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
>> >> >>       /*
>> >> >>        * Reseting generates a block interrupt, hence setting
>> >> >>        * the scatter-gather pointer to NULL.
>> >> >> @@ -2334,7 +2335,59 @@ 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.
>> >> >> +      * The Generic DMA mode (non IDMAC) also needs to reset DMA where IDMAC
>> >> >> +      * mode resets IDMAC at the end.
>> >> >> +      *
>> >> >> +      */
>> >> >> +#ifndef CONFIG_MMC_DW_IDMAC
>> >> > Is it added for generic DMA?
>> >> > IDMAC should be considered for dma_reseet as well.
>> >> > Please check databook.
>> >> >
>> >>
>> >> Yeah it's a little unclear.  In the "7.2.13 Controller/DMA/FIFO Reset
>> >> Usage" part of the document they mention It is set for what they call
>> >> "generic" DMA, which I think is when there is an external DMA
>> >> controller, and the part below that it says for "DW-DMA/Non-DW-DMA"
>> >> that controller_reset and fifo_reset should be set.   I believe this
>> >> "DW-DMA" case refers to what is called IDMAC.  So, I think it's not
>> >> required for this case, but I admit I'm not sure why they also say
>> >> "Non-DW-DMA". If you know of a good way to differentiate the "Generic
>> >> DMA" case  I can implement it.  It wasn't clear to me if the driver
>> >> even supported this "generic" dma case, but it sounds like it might,
>> >> so I added this code.  In practice, on the Exynos Systems we have,
>> >> which are using IDMAC, the reset procedure seems to work okay without
>> >> the dma_reset bit set, but I cannot test this "generic dma" case.
>> >>
>> >> The other places in the doc where I see them mention the dma_reset bit
>> >> are "7.2.21 Transmission and Reception with Internal DMAC (IDMAC)"
>> >> and the description of the CTRL register, and in "7.1
>> >> Software/Hardware Restrictions" where they say it will terminate any
>> >> pending transfer.
>> > "DW-DMA" means Synopsys's DMA controller not IDMAC.
>> > SDMMC_CTRL_DMA_RESET can apply in all type DMA interface.
>>
>> So, do you think I should always set that bit if we're using DMA?
> Yes. Although you used it for general dma at first, IDMAC also needs it.
>

Ok, I will change that.

>>
>> >
>> >>
>> >> >> +     if (host->use_dma)
>> >> >> +             flags |= SDMMC_CTRL_DMA_RESET;
>> >> >> +#endif
>> >> >> +     if (dw_mci_ctrl_reset(host, flags)) {
>> >> >> +             /*
>> >> >> +              * In all cases we clear the RAWINTS register to clear any
>> >> >> +              * interrupts.
>> >> >> +              */
>> >> > I think interrupt masking is needed before reset because we will not handle it anymore.
>> >> > And Is there any reason to move interrupt clear here compared with previous version?
>> >>
>> >> Yeah I moved it to match the description in the document more closely.
>> >>  The documents mentioned doing the interrupt clear after setting the
>> >> reset bits, and before waiting for the dma_req bit in the status
>> >> register to clear.  We've been running it with the interrupt clear
>> >> below the loop, for a while, and I just tested it with the clear above
>> >> the wait to make sure it still works properly and I was able to pass
>> >> the tuning process with some errors, so I believe this works fine too,
>> >> and more closely matches the description in the doc that I have.
>> > When I tried ciu_reset, I found some unexpected interrupts occurred.
>> > It means that interrupt handler will run to handle extra interrupts.
>> > Then, we may need mask the interrupt.
>> > Can you check it for test?
>>
>> You're right.  When I have the controller reset bit set, I see one
>> interrupt coming in shortly after we write the reset bits, which has
>> status bits of 0x180.  This has Response timeout (RTO) and Data CRC
>> error set.  It sounds like some interrupt ocurring may be expected
>> since they say to clear RINTSTS as part of the procedure though.
> Yes, it needs to clear RINTSTS.
> While this interrupt occurs, interrupt handler is called and thus tasklet function is also called.
> Can we handle this unexpected interrupt properly?
> To prevent these calling, we may need interrupt mask before resetting.
>

Ok, I traced what actually happens for me in this case, and it turns
out for tuning that the extra Data CRC interrupt doesn't matter (it
looks like the RTO bit I mentioned before isn't always there).

This is because we're doing the fifo and controller reset from within
the tasklet, which holds host->lock, it does clear RINTSTS, and also
eventually calls dw_mci_request_end() which ends by setting
host->state to STATE_IDLE.  The tasklet then wakes up again
immediately but sees the host->state as STATE_IDLE and exits.   We
could  mask the Data CRC interrupt before doing the controller reset,
if you'd like.

I'll post an updated patch with the changes so far, thanks.


>>
>>
>> >
>> >>
>> >> >> +             mci_writel(host, RINTSTS, 0xFFFFFFFF);
>> >> >> +
>> >> >> +             /* if using dma we wait for dma_req to clear */
>> >> >> +             if (host->use_dma) {
>> >> >> +                     unsigned long timeout = jiffies + msecs_to_jiffies(500);
>> >> >> +                     u32 status;
>> >> >> +                     do {
>> >> >> +                             status = mci_readl(host, STATUS);
>> >> >> +                             if (!(status & SDMMC_STATUS_DMA_REQ))
>> >> >> +                                     break;
>> >> >> +                             cpu_relax();
>> >> > What did you intend here?
>> >> > If you intent busy-wait, how about using sleep instead?
>> >>
>> >> Yes it is a busy-wait.  There is similar code in dw_mci_ctrl_reset,
>> >> where that function is polling without sleeps, so I was trying to
>> >> match that.  The cpu_relax() is something I saw in other busy-waits in
>> >> the kernel, but I'm happy to take it out if you'd like.
>> > In case of ctrl_reset, waiting is during 2 clock.
>> > If this polling status is not long, I'm OK.
>> >
>> >>
>> >> >> +                     } 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__);
>> >> >> +                             return false;
>> >> >> +                     }
>> >> >> +
>> >> >> +                     /* when using DMA next we reset the fifo again */
>> >> >> +                     dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
>> >> >> +             }
>> >> >> +     } else {
>> >> >> +             dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
>> >> > If ciu_reset is cleared, clk update below is needed?
>> >>
>> >> I'm honestly not sure what happens if the reset bits don't clear.  The
>> >> docs say controller_reset and dma_reset will auto clear after a number
>> >> of clocks, but fifo_reset will clear "after completion of the reset
>> >> operation"  So in this particular error case I'm not sure if it's
>> >> possible to recover properly or not and might hang, so I thought it
>> >> best to just return the error immediately.
>> > Yes.
>> > But at least if ciu_reset is done successfully, it may need clock update sequence again.
>> > In addition, printing reset bit[2:0] will be helpful for debug information.
>>
>> The dw_mci_ctrl_reset() function would run this code if some of the
>> bits didn't clear:
>>
>>         dev_err(host->dev,
>>                 "Timeout resetting block (ctrl reset %#x)\n",
>>                 ctrl & reset);
>>
>> Does that give you what you're looking for?
> Ah, it was there.
>
> Thanks,
> Seungwon Jeon
>
>>
>> >
>> > Thanks,
>> > Seungwon Jeon
>> >
>> >>
>> >> > Thanks,
>> >> > Seungwon Jeon
>> >> >
>> >> >> +             return false;
>> >> >> +     }
>> >> >> +
>> >> >> +#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 6bf24ab..2505804 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.9.1.423.g4596e3a
>> >> >>
>> >> >> --
>> >> >> 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
diff mbox

Patch

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 55cd110..aff57e1 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2325,6 +2325,7 @@  static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
 
 static inline bool dw_mci_fifo_reset(struct dw_mci *host)
 {
+	u32 flags = SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET;
 	/*
 	 * Reseting generates a block interrupt, hence setting
 	 * the scatter-gather pointer to NULL.
@@ -2334,7 +2335,59 @@  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.
+	 * The Generic DMA mode (non IDMAC) also needs to reset DMA where IDMAC
+	 * mode resets IDMAC at the end.
+	 *
+	 */
+#ifndef CONFIG_MMC_DW_IDMAC
+	if (host->use_dma)
+		flags |= SDMMC_CTRL_DMA_RESET;
+#endif
+	if (dw_mci_ctrl_reset(host, flags)) {
+		/*
+		 * In all cases we clear the RAWINTS register to clear any
+		 * interrupts.
+		 */
+		mci_writel(host, RINTSTS, 0xFFFFFFFF);
+
+		/* if using dma we wait for dma_req to clear */
+		if (host->use_dma) {
+			unsigned long timeout = jiffies + msecs_to_jiffies(500);
+			u32 status;
+			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__);
+				return false;
+			}
+
+			/* when using DMA next we reset the fifo again */
+			dw_mci_ctrl_reset(host, SDMMC_CTRL_FIFO_RESET);
+		}
+	} else {
+		dev_err(host->dev, "%s: Reset bits didn't clear", __func__);
+		return false;
+	}
+
+#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 6bf24ab..2505804 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 | \