[PATCH v2] mmc: sdhci-msm: Add sdhci_reset() implementation

[PATCH v2] mmc: sdhci-msm: Add sdhci_reset() implementation

[Georgi Djakov]

On apq8016, apq8084 and apq8074 platforms, writing to the software
reset register triggers the "power irq". We need to ack and handle
the irq, otherwise the following message appears:

mmc0: Reset 0x1 never completed.
sdhci: =========== REGISTER DUMP (mmc0)===========
sdhci: Sys addr: 0x00000000 | Version:  0x00002e02
sdhci: Blk size: 0x00004000 | Blk cnt:  0x00000000
sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
sdhci: Present:  0x01f80000 | Host ctl: 0x00000000
sdhci: Power:    0x00000000 | Blk gap:  0x00000000
sdhci: Wake-up:  0x00000000 | Clock:    0x00000003
sdhci: Timeout:  0x00000000 | Int stat: 0x00000000
sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
sdhci: Caps:     0x322dc8b2 | Caps_1:   0x00008007
sdhci: Cmd:      0x00000000 | Max curr: 0x00000000
sdhci: Host ctl2: 0x00000000
sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
sdhci: ===========================================

Fix it by implementing the custom sdhci_reset() function,
which performs the software reset and then handles the irq.

Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---

Changes since v1: (https://lkml.org/lkml/2016/11/22/411)
 * Perform the software reset by just writing to the SDHCI_SOFTWARE_RESET
   register and then check for the irq.

 drivers/mmc/host/sdhci-msm.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 32879b845b75..157ae07f9309 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1019,6 +1019,33 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 	__sdhci_msm_set_clock(host, clock);
 }
 
+void sdhci_msm_reset(struct sdhci_host *host, u8 mask)
+{
+	unsigned long timeout = 100;
+
+	sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
+
+	if (mask & SDHCI_RESET_ALL) {
+		host->clock = 0;
+
+		/*
+		 * SDHCI_RESET_ALL triggers the PWR IRQ and we need
+		 * to handle it here.
+		 */
+		sdhci_msm_voltage_switch(host);
+	}
+
+	while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
+		if (timeout == 0) {
+			pr_err("%s: Reset 0x%x never completed.\n",
+			       mmc_hostname(host->mmc), (int)mask);
+			return;
+		}
+		timeout--;
+		mdelay(1);
+	}
+}
+
 static const struct of_device_id sdhci_msm_dt_match[] = {
 	{ .compatible = "qcom,sdhci-msm-v4" },
 	{},
@@ -1028,7 +1055,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
 
 static const struct sdhci_ops sdhci_msm_ops = {
 	.platform_execute_tuning = sdhci_msm_execute_tuning,
-	.reset = sdhci_reset,
+	.reset = sdhci_msm_reset,
 	.set_clock = sdhci_msm_set_clock,
 	.get_min_clock = sdhci_msm_get_min_clock,
 	.get_max_clock = sdhci_msm_get_max_clock,

Re: [PATCH v2] mmc: sdhci-msm: Add sdhci_reset() implementation

[Ritesh Harjani]

Hi Georgi,

On 11/28/2016 11:09 PM, Georgi Djakov wrote:
> On apq8016, apq8084 and apq8074 platforms, writing to the software
> reset register triggers the "power irq". We need to ack and handle
> the irq, otherwise the following message appears:
>
> mmc0: Reset 0x1 never completed.
> sdhci: =========== REGISTER DUMP (mmc0)===========
> sdhci: Sys addr: 0x00000000 | Version:  0x00002e02
> sdhci: Blk size: 0x00004000 | Blk cnt:  0x00000000
> sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
> sdhci: Present:  0x01f80000 | Host ctl: 0x00000000
> sdhci: Power:    0x00000000 | Blk gap:  0x00000000
> sdhci: Wake-up:  0x00000000 | Clock:    0x00000003
> sdhci: Timeout:  0x00000000 | Int stat: 0x00000000
> sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
> sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
> sdhci: Caps:     0x322dc8b2 | Caps_1:   0x00008007
> sdhci: Cmd:      0x00000000 | Max curr: 0x00000000
> sdhci: Host ctl2: 0x00000000
> sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
> sdhci: ===========================================
>
> Fix it by implementing the custom sdhci_reset() function,
> which performs the software reset and then handles the irq.
>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
>
> Changes since v1: (https://lkml.org/lkml/2016/11/22/411)
>  * Perform the software reset by just writing to the SDHCI_SOFTWARE_RESET
>    register and then check for the irq.
I am still not sure about this change. Below change will trigger the 
IRQ, once this call is completed (say on a single core system) -since 
this is called while holding spin_lock_irqsave.

But you will be end up calling sdhci_msm_voltage_switch twice,
which to me does not seems correct.
1. 1st time you are directly calling it in sdhci_msm_reset.
2. 2nd time will be called from within pwr_irq to handle the same case.

Please let me know your thoughts on this ?

Regards
Ritesh

>
>  drivers/mmc/host/sdhci-msm.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 32879b845b75..157ae07f9309 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -1019,6 +1019,33 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  	__sdhci_msm_set_clock(host, clock);
>  }
>
> +void sdhci_msm_reset(struct sdhci_host *host, u8 mask)
> +{
> +	unsigned long timeout = 100;
> +
> +	sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
> +
> +	if (mask & SDHCI_RESET_ALL) {
> +		host->clock = 0;
> +
> +		/*
> +		 * SDHCI_RESET_ALL triggers the PWR IRQ and we need
> +		 * to handle it here.
> +		 */
> +		sdhci_msm_voltage_switch(host);
> +	}
> +
> +	while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
> +		if (timeout == 0) {
> +			pr_err("%s: Reset 0x%x never completed.\n",
> +			       mmc_hostname(host->mmc), (int)mask);
> +			return;
> +		}
> +		timeout--;
> +		mdelay(1);
> +	}
> +}
> +
>  static const struct of_device_id sdhci_msm_dt_match[] = {
>  	{ .compatible = "qcom,sdhci-msm-v4" },
>  	{},
> @@ -1028,7 +1055,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>
>  static const struct sdhci_ops sdhci_msm_ops = {
>  	.platform_execute_tuning = sdhci_msm_execute_tuning,
> -	.reset = sdhci_reset,
> +	.reset = sdhci_msm_reset,
>  	.set_clock = sdhci_msm_set_clock,
>  	.get_min_clock = sdhci_msm_get_min_clock,
>  	.get_max_clock = sdhci_msm_get_max_clock,
> --
> 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
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] mmc: sdhci-msm: Add sdhci_reset() implementation

[Georgi Djakov]

On 11/29/2016 05:40 AM, Ritesh Harjani wrote:
> Hi Georgi,
>
> On 11/28/2016 11:09 PM, Georgi Djakov wrote:
>> On apq8016, apq8084 and apq8074 platforms, writing to the software
>> reset register triggers the "power irq". We need to ack and handle
>> the irq, otherwise the following message appears:
>>
>> mmc0: Reset 0x1 never completed.
>> sdhci: =========== REGISTER DUMP (mmc0)===========
>> sdhci: Sys addr: 0x00000000 | Version:  0x00002e02
>> sdhci: Blk size: 0x00004000 | Blk cnt:  0x00000000
>> sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
>> sdhci: Present:  0x01f80000 | Host ctl: 0x00000000
>> sdhci: Power:    0x00000000 | Blk gap:  0x00000000
>> sdhci: Wake-up:  0x00000000 | Clock:    0x00000003
>> sdhci: Timeout:  0x00000000 | Int stat: 0x00000000
>> sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
>> sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
>> sdhci: Caps:     0x322dc8b2 | Caps_1:   0x00008007
>> sdhci: Cmd:      0x00000000 | Max curr: 0x00000000
>> sdhci: Host ctl2: 0x00000000
>> sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
>> sdhci: ===========================================
>>
>> Fix it by implementing the custom sdhci_reset() function,
>> which performs the software reset and then handles the irq.
>>
>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>> ---
>>
>> Changes since v1: (https://lkml.org/lkml/2016/11/22/411)
>>  * Perform the software reset by just writing to the SDHCI_SOFTWARE_RESET
>>    register and then check for the irq.
> I am still not sure about this change. Below change will trigger the
> IRQ, once this call is completed (say on a single core system) -since
> this is called while holding spin_lock_irqsave.
>
> But you will be end up calling sdhci_msm_voltage_switch twice,
> which to me does not seems correct.
> 1. 1st time you are directly calling it in sdhci_msm_reset.
> 2. 2nd time will be called from within pwr_irq to handle the same case.

Yes, that would be the behavior. This function actually is not doing
anything much than to check irq status and ack it. Testing the patch
on a few different platforms does not show any side effects, however i
could not be 100% sure as i have limited information about the hardware.

Thanks,
Georgi

>
> Please let me know your thoughts on this ?
>
> Regards
> Ritesh
>
>>
>>  drivers/mmc/host/sdhci-msm.c | 29 ++++++++++++++++++++++++++++-
>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 32879b845b75..157ae07f9309 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -1019,6 +1019,33 @@ static void sdhci_msm_set_clock(struct
>> sdhci_host *host, unsigned int clock)
>>      __sdhci_msm_set_clock(host, clock);
>>  }
>>
>> +void sdhci_msm_reset(struct sdhci_host *host, u8 mask)
>> +{
>> +    unsigned long timeout = 100;
>> +
>> +    sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
>> +
>> +    if (mask & SDHCI_RESET_ALL) {
>> +        host->clock = 0;
>> +
>> +        /*
>> +         * SDHCI_RESET_ALL triggers the PWR IRQ and we need
>> +         * to handle it here.
>> +         */
>> +        sdhci_msm_voltage_switch(host);
>> +    }
>> +
>> +    while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
>> +        if (timeout == 0) {
>> +            pr_err("%s: Reset 0x%x never completed.\n",
>> +                   mmc_hostname(host->mmc), (int)mask);
>> +            return;
>> +        }
>> +        timeout--;
>> +        mdelay(1);
>> +    }
>> +}
>> +
>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>      { .compatible = "qcom,sdhci-msm-v4" },
>>      {},
>> @@ -1028,7 +1055,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>
>>  static const struct sdhci_ops sdhci_msm_ops = {
>>      .platform_execute_tuning = sdhci_msm_execute_tuning,
>> -    .reset = sdhci_reset,
>> +    .reset = sdhci_msm_reset,
>>      .set_clock = sdhci_msm_set_clock,
>>      .get_min_clock = sdhci_msm_get_min_clock,
>>      .get_max_clock = sdhci_msm_get_max_clock,
>> --
>> 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
>>
>

Re: [PATCH v2] mmc: sdhci-msm: Add sdhci_reset() implementation

[Ritesh Harjani]

On 12/1/2016 11:38 PM, Georgi Djakov wrote:
> On 11/29/2016 05:40 AM, Ritesh Harjani wrote:
>> Hi Georgi,
>>
>> On 11/28/2016 11:09 PM, Georgi Djakov wrote:
>>> On apq8016, apq8084 and apq8074 platforms, writing to the software
>>> reset register triggers the "power irq". We need to ack and handle
>>> the irq, otherwise the following message appears:
>>>
>>> mmc0: Reset 0x1 never completed.
>>> sdhci: =========== REGISTER DUMP (mmc0)===========
>>> sdhci: Sys addr: 0x00000000 | Version:  0x00002e02
>>> sdhci: Blk size: 0x00004000 | Blk cnt:  0x00000000
>>> sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
>>> sdhci: Present:  0x01f80000 | Host ctl: 0x00000000
>>> sdhci: Power:    0x00000000 | Blk gap:  0x00000000
>>> sdhci: Wake-up:  0x00000000 | Clock:    0x00000003
>>> sdhci: Timeout:  0x00000000 | Int stat: 0x00000000
>>> sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
>>> sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
>>> sdhci: Caps:     0x322dc8b2 | Caps_1:   0x00008007
>>> sdhci: Cmd:      0x00000000 | Max curr: 0x00000000
>>> sdhci: Host ctl2: 0x00000000
>>> sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
>>> sdhci: ===========================================
>>>
>>> Fix it by implementing the custom sdhci_reset() function,
>>> which performs the software reset and then handles the irq.
>>>
>>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>>> ---
>>>
>>> Changes since v1: (https://lkml.org/lkml/2016/11/22/411)
>>>  * Perform the software reset by just writing to the
>>> SDHCI_SOFTWARE_RESET
>>>    register and then check for the irq.
>> I am still not sure about this change. Below change will trigger the
>> IRQ, once this call is completed (say on a single core system) -since
>> this is called while holding spin_lock_irqsave.
>>
>> But you will be end up calling sdhci_msm_voltage_switch twice,
>> which to me does not seems correct.
>> 1. 1st time you are directly calling it in sdhci_msm_reset.
>> 2. 2nd time will be called from within pwr_irq to handle the same case.
>
> Yes, that would be the behavior. This function actually is not doing
> anything much than to check irq status and ack it. Testing the patch
Ok.

> on a few different platforms does not show any side effects, however i
> could not be 100% sure as i have limited information about the hardware.
what about the case when say the IRQ is delayed because of some other 
overheads in the system. Will this approach still works?
I understand this would require some HW knowledge to understand the 
functioning/importance of this pwr_irq. I will try and get in touch with 
HW folk here.

Regards
Ritesh


>
> Thanks,
> Georgi
>
>>
>> Please let me know your thoughts on this ?
>>
>> Regards
>> Ritesh
>>
>>>
>>>  drivers/mmc/host/sdhci-msm.c | 29 ++++++++++++++++++++++++++++-
>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>> index 32879b845b75..157ae07f9309 100644
>>> --- a/drivers/mmc/host/sdhci-msm.c
>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>> @@ -1019,6 +1019,33 @@ static void sdhci_msm_set_clock(struct
>>> sdhci_host *host, unsigned int clock)
>>>      __sdhci_msm_set_clock(host, clock);
>>>  }
>>>
>>> +void sdhci_msm_reset(struct sdhci_host *host, u8 mask)
>>> +{
>>> +    unsigned long timeout = 100;
>>> +
>>> +    sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
>>> +
>>> +    if (mask & SDHCI_RESET_ALL) {
>>> +        host->clock = 0;
>>> +
>>> +        /*
>>> +         * SDHCI_RESET_ALL triggers the PWR IRQ and we need
>>> +         * to handle it here.
>>> +         */
>>> +        sdhci_msm_voltage_switch(host);
>>> +    }
>>> +
>>> +    while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
>>> +        if (timeout == 0) {
>>> +            pr_err("%s: Reset 0x%x never completed.\n",
>>> +                   mmc_hostname(host->mmc), (int)mask);
>>> +            return;
>>> +        }
>>> +        timeout--;
>>> +        mdelay(1);
>>> +    }
>>> +}
>>> +
>>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>>      { .compatible = "qcom,sdhci-msm-v4" },
>>>      {},
>>> @@ -1028,7 +1055,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>>
>>>  static const struct sdhci_ops sdhci_msm_ops = {
>>>      .platform_execute_tuning = sdhci_msm_execute_tuning,
>>> -    .reset = sdhci_reset,
>>> +    .reset = sdhci_msm_reset,
>>>      .set_clock = sdhci_msm_set_clock,
>>>      .get_min_clock = sdhci_msm_get_min_clock,
>>>      .get_max_clock = sdhci_msm_get_max_clock,
>>> --
>>> 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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] mmc: sdhci-msm: Add sdhci_reset() implementation

[Ritesh Harjani]

Hi Georgi,

Sorry for delaying this.
I did check with HW folks here, but it seems like we still don't have 
confirmation. In case if I get any new updates later, we can revisit it.

Also I see that we have the same implementation in current driver for 
changing IO voltage. In that case I think this patch should be fine to 
avoid the error message on reboot.


On 12/3/2016 2:24 PM, Ritesh Harjani wrote:
>
>
> On 12/1/2016 11:38 PM, Georgi Djakov wrote:
>> On 11/29/2016 05:40 AM, Ritesh Harjani wrote:
>>> Hi Georgi,
>>>
>>> On 11/28/2016 11:09 PM, Georgi Djakov wrote:
>>>> On apq8016, apq8084 and apq8074 platforms, writing to the software
>>>> reset register triggers the "power irq". We need to ack and handle
>>>> the irq, otherwise the following message appears:
>>>>
>>>> mmc0: Reset 0x1 never completed.
>>>> sdhci: =========== REGISTER DUMP (mmc0)===========
>>>> sdhci: Sys addr: 0x00000000 | Version:  0x00002e02
>>>> sdhci: Blk size: 0x00004000 | Blk cnt:  0x00000000
>>>> sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
>>>> sdhci: Present:  0x01f80000 | Host ctl: 0x00000000
>>>> sdhci: Power:    0x00000000 | Blk gap:  0x00000000
>>>> sdhci: Wake-up:  0x00000000 | Clock:    0x00000003
>>>> sdhci: Timeout:  0x00000000 | Int stat: 0x00000000
>>>> sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
>>>> sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
>>>> sdhci: Caps:     0x322dc8b2 | Caps_1:   0x00008007
>>>> sdhci: Cmd:      0x00000000 | Max curr: 0x00000000
>>>> sdhci: Host ctl2: 0x00000000
>>>> sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
>>>> sdhci: ===========================================
>>>>
>>>> Fix it by implementing the custom sdhci_reset() function,
>>>> which performs the software reset and then handles the irq.
>>>>
>>>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>>>> ---
Reviewed-by: Ritesh Harjani <riteshh@codeaurora.org>


>>>>
>>>> Changes since v1: (https://lkml.org/lkml/2016/11/22/411)
>>>>  * Perform the software reset by just writing to the
>>>> SDHCI_SOFTWARE_RESET
>>>>    register and then check for the irq.
>>> I am still not sure about this change. Below change will trigger the
>>> IRQ, once this call is completed (say on a single core system) -since
>>> this is called while holding spin_lock_irqsave.
>>>
>>> But you will be end up calling sdhci_msm_voltage_switch twice,
>>> which to me does not seems correct.
>>> 1. 1st time you are directly calling it in sdhci_msm_reset.
>>> 2. 2nd time will be called from within pwr_irq to handle the same case.
>>
>> Yes, that would be the behavior. This function actually is not doing
>> anything much than to check irq status and ack it. Testing the patch
> Ok.
>
>> on a few different platforms does not show any side effects, however i
>> could not be 100% sure as i have limited information about the hardware.
> what about the case when say the IRQ is delayed because of some other
> overheads in the system. Will this approach still works?
> I understand this would require some HW knowledge to understand the
> functioning/importance of this pwr_irq. I will try and get in touch with
> HW folk here.
>
> Regards
> Ritesh
>
>
>>
>> Thanks,
>> Georgi
>>
>>>
>>> Please let me know your thoughts on this ?
>>>
>>> Regards
>>> Ritesh
>>>
>>>>
>>>>  drivers/mmc/host/sdhci-msm.c | 29 ++++++++++++++++++++++++++++-
>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-msm.c
>>>> b/drivers/mmc/host/sdhci-msm.c
>>>> index 32879b845b75..157ae07f9309 100644
>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>> @@ -1019,6 +1019,33 @@ static void sdhci_msm_set_clock(struct
>>>> sdhci_host *host, unsigned int clock)
>>>>      __sdhci_msm_set_clock(host, clock);
>>>>  }
>>>>
>>>> +void sdhci_msm_reset(struct sdhci_host *host, u8 mask)
>>>> +{
>>>> +    unsigned long timeout = 100;
>>>> +
>>>> +    sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
>>>> +
>>>> +    if (mask & SDHCI_RESET_ALL) {
>>>> +        host->clock = 0;
>>>> +
>>>> +        /*
>>>> +         * SDHCI_RESET_ALL triggers the PWR IRQ and we need
>>>> +         * to handle it here.
>>>> +         */
>>>> +        sdhci_msm_voltage_switch(host);
>>>> +    }
>>>> +
>>>> +    while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
>>>> +        if (timeout == 0) {
>>>> +            pr_err("%s: Reset 0x%x never completed.\n",
>>>> +                   mmc_hostname(host->mmc), (int)mask);
>>>> +            return;
>>>> +        }
>>>> +        timeout--;
>>>> +        mdelay(1);
>>>> +    }
>>>> +}
>>>> +
>>>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>>>      { .compatible = "qcom,sdhci-msm-v4" },
>>>>      {},
>>>> @@ -1028,7 +1055,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>>>
>>>>  static const struct sdhci_ops sdhci_msm_ops = {
>>>>      .platform_execute_tuning = sdhci_msm_execute_tuning,
>>>> -    .reset = sdhci_reset,
>>>> +    .reset = sdhci_msm_reset,
>>>>      .set_clock = sdhci_msm_set_clock,
>>>>      .get_min_clock = sdhci_msm_get_min_clock,
>>>>      .get_max_clock = sdhci_msm_get_max_clock,
>>>> --
>>>> 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
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] mmc: sdhci-msm: Add sdhci_reset() implementation

[Ritesh Harjani]

Hi Georgi,

I checked this patch again on internal msm8996 target which has more 
than 1 core. And I can still see below issue with this patch. Please 
check the dmesg log snip. (I enabled card runtime suspend/resume to 
reproduce this more frequently).

In that case we still need to work over this issue.


<dmesg snip>
# [    5.854979] mmc0: mmc_runtime_suspend
[    5.854982] mmc1: mmc_runtime_suspend
[    5.957925] mmc1: Reset 0x1 never completed.
[    6.071244] mmc0: Reset 0x1 never completed.


Regards
Ritesh


On 1/18/2017 2:46 PM, Ritesh Harjani wrote:
> Hi Georgi,
>
> Sorry for delaying this.
> I did check with HW folks here, but it seems like we still don't have
> confirmation. In case if I get any new updates later, we can revisit it.
>
> Also I see that we have the same implementation in current driver for
> changing IO voltage. In that case I think this patch should be fine to
> avoid the error message on reboot.
>
>
> On 12/3/2016 2:24 PM, Ritesh Harjani wrote:
>>
>>
>> On 12/1/2016 11:38 PM, Georgi Djakov wrote:
>>> On 11/29/2016 05:40 AM, Ritesh Harjani wrote:
>>>> Hi Georgi,
>>>>
>>>> On 11/28/2016 11:09 PM, Georgi Djakov wrote:
>>>>> On apq8016, apq8084 and apq8074 platforms, writing to the software
>>>>> reset register triggers the "power irq". We need to ack and handle
>>>>> the irq, otherwise the following message appears:
>>>>>
>>>>> mmc0: Reset 0x1 never completed.
>>>>> sdhci: =========== REGISTER DUMP (mmc0)===========
>>>>> sdhci: Sys addr: 0x00000000 | Version:  0x00002e02
>>>>> sdhci: Blk size: 0x00004000 | Blk cnt:  0x00000000
>>>>> sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
>>>>> sdhci: Present:  0x01f80000 | Host ctl: 0x00000000
>>>>> sdhci: Power:    0x00000000 | Blk gap:  0x00000000
>>>>> sdhci: Wake-up:  0x00000000 | Clock:    0x00000003
>>>>> sdhci: Timeout:  0x00000000 | Int stat: 0x00000000
>>>>> sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
>>>>> sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
>>>>> sdhci: Caps:     0x322dc8b2 | Caps_1:   0x00008007
>>>>> sdhci: Cmd:      0x00000000 | Max curr: 0x00000000
>>>>> sdhci: Host ctl2: 0x00000000
>>>>> sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
>>>>> sdhci: ===========================================
>>>>>
>>>>> Fix it by implementing the custom sdhci_reset() function,
>>>>> which performs the software reset and then handles the irq.
>>>>>
>>>>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>>>>> ---
> Reviewed-by: Ritesh Harjani <riteshh@codeaurora.org>
>
>
>>>>>
>>>>> Changes since v1: (https://lkml.org/lkml/2016/11/22/411)
>>>>>  * Perform the software reset by just writing to the
>>>>> SDHCI_SOFTWARE_RESET
>>>>>    register and then check for the irq.
>>>> I am still not sure about this change. Below change will trigger the
>>>> IRQ, once this call is completed (say on a single core system) -since
>>>> this is called while holding spin_lock_irqsave.
>>>>
>>>> But you will be end up calling sdhci_msm_voltage_switch twice,
>>>> which to me does not seems correct.
>>>> 1. 1st time you are directly calling it in sdhci_msm_reset.
>>>> 2. 2nd time will be called from within pwr_irq to handle the same case.
>>>
>>> Yes, that would be the behavior. This function actually is not doing
>>> anything much than to check irq status and ack it. Testing the patch
>> Ok.
>>
>>> on a few different platforms does not show any side effects, however i
>>> could not be 100% sure as i have limited information about the hardware.
>> what about the case when say the IRQ is delayed because of some other
>> overheads in the system. Will this approach still works?
>> I understand this would require some HW knowledge to understand the
>> functioning/importance of this pwr_irq. I will try and get in touch with
>> HW folk here.
>>
>> Regards
>> Ritesh
>>
>>
>>>
>>> Thanks,
>>> Georgi
>>>
>>>>
>>>> Please let me know your thoughts on this ?
>>>>
>>>> Regards
>>>> Ritesh
>>>>
>>>>>
>>>>>  drivers/mmc/host/sdhci-msm.c | 29 ++++++++++++++++++++++++++++-
>>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci-msm.c
>>>>> b/drivers/mmc/host/sdhci-msm.c
>>>>> index 32879b845b75..157ae07f9309 100644
>>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>>> @@ -1019,6 +1019,33 @@ static void sdhci_msm_set_clock(struct
>>>>> sdhci_host *host, unsigned int clock)
>>>>>      __sdhci_msm_set_clock(host, clock);
>>>>>  }
>>>>>
>>>>> +void sdhci_msm_reset(struct sdhci_host *host, u8 mask)
>>>>> +{
>>>>> +    unsigned long timeout = 100;
>>>>> +
>>>>> +    sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
>>>>> +
>>>>> +    if (mask & SDHCI_RESET_ALL) {
>>>>> +        host->clock = 0;
>>>>> +
>>>>> +        /*
>>>>> +         * SDHCI_RESET_ALL triggers the PWR IRQ and we need
>>>>> +         * to handle it here.
>>>>> +         */
>>>>> +        sdhci_msm_voltage_switch(host);
>>>>> +    }
>>>>> +
>>>>> +    while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
>>>>> +        if (timeout == 0) {
>>>>> +            pr_err("%s: Reset 0x%x never completed.\n",
>>>>> +                   mmc_hostname(host->mmc), (int)mask);
>>>>> +            return;
>>>>> +        }
>>>>> +        timeout--;
>>>>> +        mdelay(1);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>>>>      { .compatible = "qcom,sdhci-msm-v4" },
>>>>>      {},
>>>>> @@ -1028,7 +1055,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>>>>
>>>>>  static const struct sdhci_ops sdhci_msm_ops = {
>>>>>      .platform_execute_tuning = sdhci_msm_execute_tuning,
>>>>> -    .reset = sdhci_reset,
>>>>> +    .reset = sdhci_msm_reset,
>>>>>      .set_clock = sdhci_msm_set_clock,
>>>>>      .get_min_clock = sdhci_msm_get_min_clock,
>>>>>      .get_max_clock = sdhci_msm_get_max_clock,
>>>>> --
>>>>> 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
>>
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] mmc: sdhci-msm: Add sdhci_reset() implementation

[Georgi Djakov]

On 01/18/2017 01:47 PM, Ritesh Harjani wrote:
> Hi Georgi,
>
> I checked this patch again on internal msm8996 target which has more
> than 1 core. And I can still see below issue with this patch. Please
> check the dmesg log snip. (I enabled card runtime suspend/resume to
> reproduce this more frequently).
>
> In that case we still need to work over this issue.

Hi Ritesh,

Thanks for checking this. I will submit a new version.

BR,
Georgi

>
>
> <dmesg snip>
> # [    5.854979] mmc0: mmc_runtime_suspend
> [    5.854982] mmc1: mmc_runtime_suspend
> [    5.957925] mmc1: Reset 0x1 never completed.
> [    6.071244] mmc0: Reset 0x1 never completed.
>
>
> Regards
> Ritesh
>
>
> On 1/18/2017 2:46 PM, Ritesh Harjani wrote:
>> Hi Georgi,
>>
>> Sorry for delaying this.
>> I did check with HW folks here, but it seems like we still don't have
>> confirmation. In case if I get any new updates later, we can revisit it.
>>
>> Also I see that we have the same implementation in current driver for
>> changing IO voltage. In that case I think this patch should be fine to
>> avoid the error message on reboot.
>>
>>
>> On 12/3/2016 2:24 PM, Ritesh Harjani wrote:
>>>
>>>
>>> On 12/1/2016 11:38 PM, Georgi Djakov wrote:
>>>> On 11/29/2016 05:40 AM, Ritesh Harjani wrote:
>>>>> Hi Georgi,
>>>>>
>>>>> On 11/28/2016 11:09 PM, Georgi Djakov wrote:
>>>>>> On apq8016, apq8084 and apq8074 platforms, writing to the software
>>>>>> reset register triggers the "power irq". We need to ack and handle
>>>>>> the irq, otherwise the following message appears:
>>>>>>
>>>>>> mmc0: Reset 0x1 never completed.
>>>>>> sdhci: =========== REGISTER DUMP (mmc0)===========
>>>>>> sdhci: Sys addr: 0x00000000 | Version:  0x00002e02
>>>>>> sdhci: Blk size: 0x00004000 | Blk cnt:  0x00000000
>>>>>> sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
>>>>>> sdhci: Present:  0x01f80000 | Host ctl: 0x00000000
>>>>>> sdhci: Power:    0x00000000 | Blk gap:  0x00000000
>>>>>> sdhci: Wake-up:  0x00000000 | Clock:    0x00000003
>>>>>> sdhci: Timeout:  0x00000000 | Int stat: 0x00000000
>>>>>> sdhci: Int enab: 0x00000000 | Sig enab: 0x00000000
>>>>>> sdhci: AC12 err: 0x00000000 | Slot int: 0x00000000
>>>>>> sdhci: Caps:     0x322dc8b2 | Caps_1:   0x00008007
>>>>>> sdhci: Cmd:      0x00000000 | Max curr: 0x00000000
>>>>>> sdhci: Host ctl2: 0x00000000
>>>>>> sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x0000000000000000
>>>>>> sdhci: ===========================================
>>>>>>
>>>>>> Fix it by implementing the custom sdhci_reset() function,
>>>>>> which performs the software reset and then handles the irq.
>>>>>>
>>>>>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>>>>>> ---
>> Reviewed-by: Ritesh Harjani <riteshh@codeaurora.org>
>>
>>
>>>>>>
>>>>>> Changes since v1: (https://lkml.org/lkml/2016/11/22/411)
>>>>>>  * Perform the software reset by just writing to the
>>>>>> SDHCI_SOFTWARE_RESET
>>>>>>    register and then check for the irq.
>>>>> I am still not sure about this change. Below change will trigger the
>>>>> IRQ, once this call is completed (say on a single core system) -since
>>>>> this is called while holding spin_lock_irqsave.
>>>>>
>>>>> But you will be end up calling sdhci_msm_voltage_switch twice,
>>>>> which to me does not seems correct.
>>>>> 1. 1st time you are directly calling it in sdhci_msm_reset.
>>>>> 2. 2nd time will be called from within pwr_irq to handle the same
>>>>> case.
>>>>
>>>> Yes, that would be the behavior. This function actually is not doing
>>>> anything much than to check irq status and ack it. Testing the patch
>>> Ok.
>>>
>>>> on a few different platforms does not show any side effects, however i
>>>> could not be 100% sure as i have limited information about the
>>>> hardware.
>>> what about the case when say the IRQ is delayed because of some other
>>> overheads in the system. Will this approach still works?
>>> I understand this would require some HW knowledge to understand the
>>> functioning/importance of this pwr_irq. I will try and get in touch with
>>> HW folk here.
>>>
>>> Regards
>>> Ritesh
>>>
>>>
>>>>
>>>> Thanks,
>>>> Georgi
>>>>
>>>>>
>>>>> Please let me know your thoughts on this ?
>>>>>
>>>>> Regards
>>>>> Ritesh
>>>>>
>>>>>>
>>>>>>  drivers/mmc/host/sdhci-msm.c | 29 ++++++++++++++++++++++++++++-
>>>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/sdhci-msm.c
>>>>>> b/drivers/mmc/host/sdhci-msm.c
>>>>>> index 32879b845b75..157ae07f9309 100644
>>>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>>>> @@ -1019,6 +1019,33 @@ static void sdhci_msm_set_clock(struct
>>>>>> sdhci_host *host, unsigned int clock)
>>>>>>      __sdhci_msm_set_clock(host, clock);
>>>>>>  }
>>>>>>
>>>>>> +void sdhci_msm_reset(struct sdhci_host *host, u8 mask)
>>>>>> +{
>>>>>> +    unsigned long timeout = 100;
>>>>>> +
>>>>>> +    sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
>>>>>> +
>>>>>> +    if (mask & SDHCI_RESET_ALL) {
>>>>>> +        host->clock = 0;
>>>>>> +
>>>>>> +        /*
>>>>>> +         * SDHCI_RESET_ALL triggers the PWR IRQ and we need
>>>>>> +         * to handle it here.
>>>>>> +         */
>>>>>> +        sdhci_msm_voltage_switch(host);
>>>>>> +    }
>>>>>> +
>>>>>> +    while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
>>>>>> +        if (timeout == 0) {
>>>>>> +            pr_err("%s: Reset 0x%x never completed.\n",
>>>>>> +                   mmc_hostname(host->mmc), (int)mask);
>>>>>> +            return;
>>>>>> +        }
>>>>>> +        timeout--;
>>>>>> +        mdelay(1);
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>>  static const struct of_device_id sdhci_msm_dt_match[] = {
>>>>>>      { .compatible = "qcom,sdhci-msm-v4" },
>>>>>>      {},
>>>>>> @@ -1028,7 +1055,7 @@ MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
>>>>>>
>>>>>>  static const struct sdhci_ops sdhci_msm_ops = {
>>>>>>      .platform_execute_tuning = sdhci_msm_execute_tuning,
>>>>>> -    .reset = sdhci_reset,
>>>>>> +    .reset = sdhci_msm_reset,
>>>>>>      .set_clock = sdhci_msm_set_clock,
>>>>>>      .get_min_clock = sdhci_msm_get_min_clock,
>>>>>>      .get_max_clock = sdhci_msm_get_max_clock,
>>>>>> --
>>>>>> 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
>>>
>>
>