Re: [PATCH v2 1/5] mmc: dw-mmc: ensure that the card is not busy when changing clock

[Jaehoon Chung <jh80.chung@samsung.com>]

On 08/30/2012 10:19 AM, Seungwon Jeon wrote:
> On Tuesday, August 29, 2012, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> On 08/29/2012 10:21 AM, Seungwon Jeon wrote:
>>> On Tuesday, August 28, 2012, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>>>> synopsys spec is mentioned. (7.1 Software/Hardware Restrictions)
>>>> "Before disabling clock, ensure that card is not busy due to any previous data command."
>>>>
>>>> At specific board, i found the problem that didn't inform ciu.
>>>> (It didn't produce always.)
>>>> At that time, i checked the status register whether card is busy or not.
>>>> (Start bit of CMD register is cleared, but card is busy.)
>>>> Current code didn't check the card status. Just check whether the start-bit cleared.
>>>> host regard that command is completed, will send the next command.
>>>> But next command didn't issue because Card is busy.
>>>> This patch can prevent this situation.
>>>>
>>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>>> ---
>>>>  drivers/mmc/host/dw_mmc.c |   27 +++++++++++++++++++++------
>>>>  drivers/mmc/host/dw_mmc.h |    1 +
>>>>  2 files changed, 22 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>> index 36f98c0..e30d3ed 100644
>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>> @@ -623,6 +623,24 @@ static void mci_send_cmd(struct dw_mci_slot *slot, u32 cmd, u32 arg)
>>>>  		cmd, arg, cmd_status);
>>>>  }
>>>>
>>>> +static void dw_mci_inform_ciu(struct dw_mci_slot *slot)
>>>> +{
>>>> +	struct dw_mci *host = slot->host;
>>>> +	unsigned long timeout = jiffies + msecs_to_jiffies(10);
>>> 10ms is from test?
>> It's tested...it's not exactly value..but never spent the time.
>>>
>>>> +	u32 ctrl, status;
>>>> +
>>>> +	do {
>>>> +		status = mci_readl(host, STATUS);
>>>> +		if (!(status & SDMMC_DATA_BUSY))
>>>> +			break;
>>>> +		ctrl = mci_readl(host, CTRL);
>>>> +		ctrl |= SDMMC_CTRL_RESET;
>>>> +		mci_writel(host, CTRL, ctrl);
>>>> +	} while (time_before(jiffies, timeout));
>>>> +
>>>> +	mci_send_cmd(slot, SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
>>>> +}
>>>> +
>>>>  static void dw_mci_setup_bus(struct dw_mci_slot *slot)
>>>>  {
>>>>  	struct dw_mci *host = slot->host;
>>>> @@ -650,15 +668,13 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot)
>>> According to your quotation of manual("Before disabling clock, ...),
>>> Place of dw_mci_inform_ciu is proper here?
>> manual is mentioned "before the disabling clock". but i think no problem before send the command.
>>>
>>> 	dw_mci_inform_ciu(slot);
>>>
>>>           /* disable clock */
>>>          mci_writel(host, CLKENA, 0);
>>>          mci_writel(host, CLKSRC, 0);
>>>
>>>>  		mci_writel(host, CLKSRC, 0);
>>>>
>>>>  		/* inform CIU */
>>>> -		mci_send_cmd(slot,
>>>> -			     SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
>>>> +		dw_mci_inform_ciu(slot);
>>>>
>>>>  		/* set clock to desired speed */
>>>>  		mci_writel(host, CLKDIV, div);
>>>>
>>>>  		/* inform CIU */
>>>> -		mci_send_cmd(slot,
>>>> -			     SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
>>>> +		dw_mci_inform_ciu(slot);
>>> And then, since start to change clock sequence, there is no need to check busy-status.
>>> Just need to update clock with origin code.
>> Right, didn't check the busy-status. but there is also no reason that didn't need to check busy-status.
> Ok, I mentioned no check busy-status because host doesn't send command to card in the middle of host's clock update.
> If you have a opinion, please let me know.
OK, i will apply with your opinion.
Then resend the patch.

Best Regards,
Jaehoon Chung
> 
> Thanks,
> Seungwon Jeon
> 
>>>>
>>>>  		/* enable clock; only low power if no SDIO */
>>>>  		clk_en_a = SDMMC_CLKEN_ENABLE << slot->id;
>>>> @@ -667,8 +683,7 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot)
>>>>  		mci_writel(host, CLKENA, clk_en_a);
>>>>
>>>>  		/* inform CIU */
>>>> -		mci_send_cmd(slot,
>>>> -			     SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT, 0);
>>>> +		dw_mci_inform_ciu(slot);
>>> Same here.
>>>
>>> Thanks,
>>> Seungwon Jeon
>>>>
>>>>  		host->current_speed = slot->clock;
>>>>  	}
>>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>>> index 15c27e1..b6a1a78 100644
>>>> --- a/drivers/mmc/host/dw_mmc.h
>>>> +++ b/drivers/mmc/host/dw_mmc.h
>>>> @@ -127,6 +127,7 @@
>>>>  #define SDMMC_CMD_INDX(n)		((n) & 0x1F)
>>>>  /* Status register defines */
>>>>  #define SDMMC_GET_FCNT(x)		(((x)>>17) & 0x1FFF)
>>>> +#define SDMMC_DATA_BUSY			BIT(9)
>>>>  /* Internal DMAC interrupt defines */
>>>>  #define SDMMC_IDMAC_INT_AI		BIT(9)
>>>>  #define SDMMC_IDMAC_INT_NI		BIT(8)
>>>> --
>>>> 1.7.4.1
>>>
>>> --
>>> 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
> 
>