[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Zhangfei Gao]

Introdeced from commit b7db86adfa58b
CDETECT is ingored since IS_ERR_VALUE(!mmc_gpio_get_cd(mmc)) does not work
Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Reported-by: Kevin Hilman <khilman@linaro.org>
---
 drivers/mmc/host/dw_mmc.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index a776f24f4311..9ded62c8225e 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	int present;
 	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci_board *brd = slot->host->pdata;
-	int gpio_cd = !mmc_gpio_get_cd(mmc);
+	struct dw_mci *host = slot->host;
+	int gpio_cd = mmc_gpio_get_cd(mmc);
 
 	/* Use platform get_cd function, else try onboard card detect */
 	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)
@@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	else if (brd->get_cd)
 		present = !brd->get_cd(slot->id);
 	else if (!IS_ERR_VALUE(gpio_cd))
-		present = !!gpio_cd;
+		present = !gpio_cd;
 	else
 		present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
 			== 0 ? 1 : 0;
 
+	spin_lock_bh(&host->lock);
 	if (present) {
 		set_bit(DW_MMC_CARD_PRESENT, &slot->flags);
 		dev_dbg(&mmc->class_dev, "card is present\n");
@@ -1053,6 +1055,7 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 		clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
 		dev_dbg(&mmc->class_dev, "card is not present\n");
 	}
+	spin_unlock_bh(&host->lock);
 
 	return present;
 }
@@ -2081,7 +2084,7 @@ static int dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
 	return gpio;
 }
 
-/* find the cd gpio for a given slot; or -1 if none specified */
+/* find the cd gpio for a given slot */
 static void dw_mci_of_get_cd_gpio(struct device *dev, u8 slot,
 					struct mmc_host *mmc)
 {
-- 
1.7.9.5

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[zhangfei]

Hi Kevin

Would you mind check whehter this patch solve the issue of "The Samsung 
Arndale board started failing boot from MMC".

I am sorry about that.

On 01/15/2014 03:15 PM, Zhangfei Gao wrote:
> Introdeced from commit b7db86adfa58b
commit should be bf626e5550f24aec24975a0e85ad8e572ca76a6b, from mmc-next

> CDETECT is ingored since IS_ERR_VALUE(!mmc_gpio_get_cd(mmc)) does not work
> Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT
>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Reported-by: Kevin Hilman <khilman@linaro.org>
> ---
>   drivers/mmc/host/dw_mmc.c |    9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Zhangfei Gao]

Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b
CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc)
can not be checked by IS_ERR_VALUE.
Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT,
otherwise sd detect may occasionally fail.

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Reported-by: Kevin Hilman <khilman@linaro.org>
Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
Tested-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/mmc/host/dw_mmc.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index a776f24f4311..9ded62c8225e 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	int present;
 	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci_board *brd = slot->host->pdata;
-	int gpio_cd = !mmc_gpio_get_cd(mmc);
+	struct dw_mci *host = slot->host;
+	int gpio_cd = mmc_gpio_get_cd(mmc);
 
 	/* Use platform get_cd function, else try onboard card detect */
 	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)
@@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	else if (brd->get_cd)
 		present = !brd->get_cd(slot->id);
 	else if (!IS_ERR_VALUE(gpio_cd))
-		present = !!gpio_cd;
+		present = !gpio_cd;
 	else
 		present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
 			== 0 ? 1 : 0;
 
+	spin_lock_bh(&host->lock);
 	if (present) {
 		set_bit(DW_MMC_CARD_PRESENT, &slot->flags);
 		dev_dbg(&mmc->class_dev, "card is present\n");
@@ -1053,6 +1055,7 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 		clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
 		dev_dbg(&mmc->class_dev, "card is not present\n");
 	}
+	spin_unlock_bh(&host->lock);
 
 	return present;
 }
@@ -2081,7 +2084,7 @@ static int dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
 	return gpio;
 }
 
-/* find the cd gpio for a given slot; or -1 if none specified */
+/* find the cd gpio for a given slot */
 static void dw_mci_of_get_cd_gpio(struct device *dev, u8 slot,
 					struct mmc_host *mmc)
 {
-- 
1.7.9.5

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Seungwon Jeon]

On Wed, January 15, 2014, Zhangfei Gao wrote:
> Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b
> CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc)
> can not be checked by IS_ERR_VALUE.
> Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT,
> otherwise sd detect may occasionally fail.
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Reported-by: Kevin Hilman <khilman@linaro.org>
> Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
> Tested-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  drivers/mmc/host/dw_mmc.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index a776f24f4311..9ded62c8225e 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>  	int present;
>  	struct dw_mci_slot *slot = mmc_priv(mmc);
>  	struct dw_mci_board *brd = slot->host->pdata;
> -	int gpio_cd = !mmc_gpio_get_cd(mmc);
> +	struct dw_mci *host = slot->host;
> +	int gpio_cd = mmc_gpio_get_cd(mmc);
> 
>  	/* Use platform get_cd function, else try onboard card detect */
>  	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)
> @@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>  	else if (brd->get_cd)
>  		present = !brd->get_cd(slot->id);
>  	else if (!IS_ERR_VALUE(gpio_cd))
> -		present = !!gpio_cd;
> +		present = !gpio_cd;
!!gpio_cd or gpio_cd is correct, isn't it?

Thanks,
Seungwon Jeon

>  	else
>  		present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
>  			== 0 ? 1 : 0;
> 
> +	spin_lock_bh(&host->lock);
>  	if (present) {
>  		set_bit(DW_MMC_CARD_PRESENT, &slot->flags);
>  		dev_dbg(&mmc->class_dev, "card is present\n");
> @@ -1053,6 +1055,7 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>  		clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
>  		dev_dbg(&mmc->class_dev, "card is not present\n");
>  	}
> +	spin_unlock_bh(&host->lock);
> 
>  	return present;
>  }
> @@ -2081,7 +2084,7 @@ static int dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
>  	return gpio;
>  }
> 
> -/* find the cd gpio for a given slot; or -1 if none specified */
> +/* find the cd gpio for a given slot */
>  static void dw_mci_of_get_cd_gpio(struct device *dev, u8 slot,
>  					struct mmc_host *mmc)
>  {
> --
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[zhangfei]

On 01/15/2014 07:58 PM, Seungwon Jeon wrote:
> On Wed, January 15, 2014, Zhangfei Gao wrote:
>> Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b
>> CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc)
>> can not be checked by IS_ERR_VALUE.
>> Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT,
>> otherwise sd detect may occasionally fail.
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> Reported-by: Kevin Hilman <khilman@linaro.org>
>> Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
>> Tested-by: Sachin Kamat <sachin.kamat@linaro.org>
>> ---
>>   drivers/mmc/host/dw_mmc.c |    9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index a776f24f4311..9ded62c8225e 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>>   	int present;
>>   	struct dw_mci_slot *slot = mmc_priv(mmc);
>>   	struct dw_mci_board *brd = slot->host->pdata;
>> -	int gpio_cd = !mmc_gpio_get_cd(mmc);
>> +	struct dw_mci *host = slot->host;
>> +	int gpio_cd = mmc_gpio_get_cd(mmc);
>>
>>   	/* Use platform get_cd function, else try onboard card detect */
>>   	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)
>> @@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>>   	else if (brd->get_cd)
>>   		present = !brd->get_cd(slot->id);
>>   	else if (!IS_ERR_VALUE(gpio_cd))
>> -		present = !!gpio_cd;
>> +		present = !gpio_cd;
> !!gpio_cd or gpio_cd is correct, isn't it?
>

No, mmc_gpio_get_cd(mmc) has to revert.
!!gpio_cd is used before since gpio_cd = !mmc_gpio_get_cd(mmc) is used
before.

Thanks

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Seungwon Jeon]

On Wed, January 15, 2014, Zhangfei Gao wrote:
> On 01/15/2014 07:58 PM, Seungwon Jeon wrote:
> > On Wed, January 15, 2014, Zhangfei Gao wrote:
> >> Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b
> >> CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc)
> >> can not be checked by IS_ERR_VALUE.
> >> Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT,
> >> otherwise sd detect may occasionally fail.
> >>
> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >> Reported-by: Kevin Hilman <khilman@linaro.org>
> >> Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
> >> Tested-by: Sachin Kamat <sachin.kamat@linaro.org>
> >> ---
> >>   drivers/mmc/host/dw_mmc.c |    9 ++++++---
> >>   1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> >> index a776f24f4311..9ded62c8225e 100644
> >> --- a/drivers/mmc/host/dw_mmc.c
> >> +++ b/drivers/mmc/host/dw_mmc.c
> >> @@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
> >>   	int present;
> >>   	struct dw_mci_slot *slot = mmc_priv(mmc);
> >>   	struct dw_mci_board *brd = slot->host->pdata;
> >> -	int gpio_cd = !mmc_gpio_get_cd(mmc);
> >> +	struct dw_mci *host = slot->host;
> >> +	int gpio_cd = mmc_gpio_get_cd(mmc);
> >>
> >>   	/* Use platform get_cd function, else try onboard card detect */
> >>   	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)
> >> @@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
> >>   	else if (brd->get_cd)
> >>   		present = !brd->get_cd(slot->id);
> >>   	else if (!IS_ERR_VALUE(gpio_cd))
> >> -		present = !!gpio_cd;
> >> +		present = !gpio_cd;
> > !!gpio_cd or gpio_cd is correct, isn't it?
> >
> 
> No, mmc_gpio_get_cd(mmc) has to revert.
I'm missing something?
If card is detected, mmc_gpio_get_cd() returns non-zero, right?
I guess gpio_cd should be kept.

> !!gpio_cd is used before since gpio_cd = !mmc_gpio_get_cd(mmc) is used
> before.
> 
> Thanks
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[zhangfei]

On 01/15/2014 08:26 PM, Seungwon Jeon wrote:

>>>> @@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>>>>    	int present;
>>>>    	struct dw_mci_slot *slot = mmc_priv(mmc);
>>>>    	struct dw_mci_board *brd = slot->host->pdata;
>>>> -	int gpio_cd = !mmc_gpio_get_cd(mmc);
>>>> +	struct dw_mci *host = slot->host;
>>>> +	int gpio_cd = mmc_gpio_get_cd(mmc);
>>>>
>>>>    	/* Use platform get_cd function, else try onboard card detect */
>>>>    	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)
>>>> @@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>>>>    	else if (brd->get_cd)
>>>>    		present = !brd->get_cd(slot->id);
>>>>    	else if (!IS_ERR_VALUE(gpio_cd))
>>>> -		present = !!gpio_cd;
>>>> +		present = !gpio_cd;
>>> !!gpio_cd or gpio_cd is correct, isn't it?
>>>
>>
>> No, mmc_gpio_get_cd(mmc) has to revert.
> I'm missing something?
> If card is detected, mmc_gpio_get_cd() returns non-zero, right?
> I guess gpio_cd should be kept.
> 

Hmm, looks you are right.
Though not see clearly mmc_gpio_get_cd declaratoin, other drivers
directly set get_cd as mmc_gpio_get_cd.
.get_cd	= mmc_gpio_get_cd

However, in our board cd =0 when card is deteced while cd=1 when card is
removed.
In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be
set, as well as new property "caps2-mmc-cd-active-low".

--- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
+++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
@@ -73,6 +73,8 @@ Optional properties:
+* caps2-mmc-cd-active-low: cd pin is low when card active
+

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
+       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
+               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+

But it looks strange "cd-active-low" describing "CD_ACTIVE_HIGH" flag.
When card active, cd = 0, and ACTIVE_HIGH is required to make
mmc_gpio_get_cd return 1.
int mmc_gpio_get_cd(struct mmc_host *host)
{
        return !gpio_get_value_cansleep(ctx->cd_gpio) ^
                !!(host->caps2 & MMC_CAP2_CD_ACTIVE_HIGH);
}

Thanks

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Arnd Bergmann]

On Wednesday 15 January 2014 21:56:26 zhangfei wrote:
> However, in our board cd =0 when card is deteced while cd=1 when card is
> removed.
> In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be
> set, as well as new property "caps2-mmc-cd-active-low".
> 
> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> @@ -73,6 +73,8 @@ Optional properties:
> +* caps2-mmc-cd-active-low: cd pin is low when card active
> +
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> +       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
> +               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
> +

The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for
legacy platforms. With DT parsing, you can normally specify
the polarity of the GPIO line in the GPIO specifier in DT.


	Arnd

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Arnd Bergmann]

On Wednesday 15 January 2014 14:59:59 Arnd Bergmann wrote:
> On Wednesday 15 January 2014 21:56:26 zhangfei wrote:
> > However, in our board cd =0 when card is deteced while cd=1 when card is
> > removed.
> > In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be
> > set, as well as new property "caps2-mmc-cd-active-low".
> > 
> > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> > @@ -73,6 +73,8 @@ Optional properties:
> > +* caps2-mmc-cd-active-low: cd pin is low when card active
> > +
> > 
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > +       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
> > +               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
> > +
> 
> The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for
> legacy platforms. With DT parsing, you can normally specify
> the polarity of the GPIO line in the GPIO specifier in DT.

I missed the fact that we already have a "cd-inverted" property
as specified in bindings/mmc/mmc.txt. If your GPIO controller
does not handle polarity, you can use that.

	Arnd

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Seungwon Jeon]

On Wed, January 15, 2014, Zhangfei wrote:
> On 01/15/2014 08:26 PM, Seungwon Jeon wrote:
> 
> >>>> @@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
> >>>>    	int present;
> >>>>    	struct dw_mci_slot *slot = mmc_priv(mmc);
> >>>>    	struct dw_mci_board *brd = slot->host->pdata;
> >>>> -	int gpio_cd = !mmc_gpio_get_cd(mmc);
> >>>> +	struct dw_mci *host = slot->host;
> >>>> +	int gpio_cd = mmc_gpio_get_cd(mmc);
> >>>>
> >>>>    	/* Use platform get_cd function, else try onboard card detect */
> >>>>    	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)
> >>>> @@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
> >>>>    	else if (brd->get_cd)
> >>>>    		present = !brd->get_cd(slot->id);
> >>>>    	else if (!IS_ERR_VALUE(gpio_cd))
> >>>> -		present = !!gpio_cd;
> >>>> +		present = !gpio_cd;
> >>> !!gpio_cd or gpio_cd is correct, isn't it?
> >>>
> >>
> >> No, mmc_gpio_get_cd(mmc) has to revert.
> > I'm missing something?
> > If card is detected, mmc_gpio_get_cd() returns non-zero, right?
> > I guess gpio_cd should be kept.
> >
> 
> Hmm, looks you are right.
> Though not see clearly mmc_gpio_get_cd declaratoin, other drivers
> directly set get_cd as mmc_gpio_get_cd.
> .get_cd	= mmc_gpio_get_cd
> 
> However, in our board cd =0 when card is deteced while cd=1 when card is
> removed.
> In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be
> set, as well as new property "caps2-mmc-cd-active-low".

Ok, you could do more.
mmc_gpio_get_cd() is expected to return non-zero if card is detection.

> 
> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> @@ -73,6 +73,8 @@ Optional properties:
> +* caps2-mmc-cd-active-low: cd pin is low when card active
> +
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> +       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
> +               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
> +
> 
> But it looks strange "cd-active-low" describing "CD_ACTIVE_HIGH" flag.
> When card active, cd = 0, and ACTIVE_HIGH is required to make
> mmc_gpio_get_cd return 1.

I think your board seems not to use pull-up on GPIO line for card detection.
So, MMC_CAP2_CD_ACTIVE_HIGH would be needed.

Thanks,
Seungwon Jeon

> int mmc_gpio_get_cd(struct mmc_host *host)
> {
>         return !gpio_get_value_cansleep(ctx->cd_gpio) ^
>                 !!(host->caps2 & MMC_CAP2_CD_ACTIVE_HIGH);
> }
> 
> Thanks
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[zhangfei]

On 01/15/2014 10:38 PM, Seungwon Jeon wrote:
>> Hmm, looks you are right.
>> Though not see clearly mmc_gpio_get_cd declaratoin, other drivers
>> directly set get_cd as mmc_gpio_get_cd.
>> .get_cd	= mmc_gpio_get_cd
>>
>> However, in our board cd =0 when card is deteced while cd=1 when card is
>> removed.
>> In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be
>> set, as well as new property "caps2-mmc-cd-active-low".
> 
> Ok, you could do more.
> mmc_gpio_get_cd() is expected to return non-zero if card is detection.

> I think your board seems not to use pull-up on GPIO line for card detection.
> So, MMC_CAP2_CD_ACTIVE_HIGH would be needed.

Unfortunately, the specific cd pin can not be configured.
Suspect on our board the cd =1 when card is inserted, can not measure as
no clear resister and need check with hardware guy tomorrow.

"cd-inverted" may be required as mentioned by Arnd.
Will update the patch as following if no problem.

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 9ded62c8225e..8326e54b96a8 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1042,7 +1042,7 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
        else if (brd->get_cd)
                present = !brd->get_cd(slot->id);
        else if (!IS_ERR_VALUE(gpio_cd))
-               present = !gpio_cd;
+               present = gpio_cd ^ brd->cd_inverted;
        else
                present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
                        == 0 ? 1 : 0;
@@ -2414,6 +2414,9 @@ static struct dw_mci_board *dw_mci_parse_dt(struct
dw_mci *host)
        if (of_find_property(np, "caps2-mmc-hs200-1_2v", NULL))
                pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR;

+       if (of_get_property(np, "cd-inverted", NULL))
+               pdata->cd_inverted = 1;
+
        return pdata;
 }

diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 6ce7d2cd3c7a..4535282589ab 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -238,6 +238,7 @@ struct dw_mci_board {
        u32 caps;       /* Capabilities */
        u32 caps2;      /* More capabilities */
        u32 pm_caps;    /* PM capabilities */
+       u8 cd_inverted;

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[zhangfei]

On 01/15/2014 10:20 PM, Arnd Bergmann wrote:
> On Wednesday 15 January 2014 14:59:59 Arnd Bergmann wrote:
>> On Wednesday 15 January 2014 21:56:26 zhangfei wrote:
>>> However, in our board cd =0 when card is deteced while cd=1 when card is
>>> removed.
>>> In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be
>>> set, as well as new property "caps2-mmc-cd-active-low".
>>>
>>> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
>>> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
>>> @@ -73,6 +73,8 @@ Optional properties:
>>> +* caps2-mmc-cd-active-low: cd pin is low when card active
>>> +
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> +       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
>>> +               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
>>> +
>>
>> The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for
>> legacy platforms. With DT parsing, you can normally specify
>> the polarity of the GPIO line in the GPIO specifier in DT.
>
> I missed the fact that we already have a "cd-inverted" property
> as specified in bindings/mmc/mmc.txt. If your GPIO controller
> does not handle polarity, you can use that.
>

Thanks Arnd

GPIO controller drivers/gpio/gpio-pl061.c still not handle polarity, so 
use "cd-inverted" instead.

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Zhangfei Gao]

Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b
CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc)
can not be checked by IS_ERR_VALUE.
Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT,
otherwise sd detect may occasionally fail.

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Reported-by: Kevin Hilman <khilman@linaro.org>
Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
Tested-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/mmc/host/dw_mmc.c  |   12 +++++++++---
 include/linux/mmc/dw_mmc.h |    1 +
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index a776f24f4311..8326e54b96a8 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	int present;
 	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci_board *brd = slot->host->pdata;
-	int gpio_cd = !mmc_gpio_get_cd(mmc);
+	struct dw_mci *host = slot->host;
+	int gpio_cd = mmc_gpio_get_cd(mmc);
 
 	/* Use platform get_cd function, else try onboard card detect */
 	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)
@@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	else if (brd->get_cd)
 		present = !brd->get_cd(slot->id);
 	else if (!IS_ERR_VALUE(gpio_cd))
-		present = !!gpio_cd;
+		present = gpio_cd ^ brd->cd_inverted;
 	else
 		present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
 			== 0 ? 1 : 0;
 
+	spin_lock_bh(&host->lock);
 	if (present) {
 		set_bit(DW_MMC_CARD_PRESENT, &slot->flags);
 		dev_dbg(&mmc->class_dev, "card is present\n");
@@ -1053,6 +1055,7 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 		clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
 		dev_dbg(&mmc->class_dev, "card is not present\n");
 	}
+	spin_unlock_bh(&host->lock);
 
 	return present;
 }
@@ -2081,7 +2084,7 @@ static int dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
 	return gpio;
 }
 
-/* find the cd gpio for a given slot; or -1 if none specified */
+/* find the cd gpio for a given slot */
 static void dw_mci_of_get_cd_gpio(struct device *dev, u8 slot,
 					struct mmc_host *mmc)
 {
@@ -2411,6 +2414,9 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
 	if (of_find_property(np, "caps2-mmc-hs200-1_2v", NULL))
 		pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR;
 
+	if (of_get_property(np, "cd-inverted", NULL))
+		pdata->cd_inverted = 1;
+
 	return pdata;
 }
 
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 6ce7d2cd3c7a..4535282589ab 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -238,6 +238,7 @@ struct dw_mci_board {
 	u32 caps;	/* Capabilities */
 	u32 caps2;	/* More capabilities */
 	u32 pm_caps;	/* PM capabilities */
+	u8 cd_inverted;
 	/*
 	 * Override fifo depth. If 0, autodetect it from the FIFOTH register,
 	 * but note that this may not be reliable after a bootloader has used
-- 
1.7.9.5

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Arnd Bergmann]

On Wednesday 15 January 2014, zhangfei wrote:
> 
> On 01/15/2014 10:38 PM, Seungwon Jeon wrote:
> >> Hmm, looks you are right.
> >> Though not see clearly mmc_gpio_get_cd declaratoin, other drivers
> >> directly set get_cd as mmc_gpio_get_cd.
> >> .get_cd	= mmc_gpio_get_cd
> >>
> >> However, in our board cd =0 when card is deteced while cd=1 when card is
> >> removed.
> >> In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be
> >> set, as well as new property "caps2-mmc-cd-active-low".
> > 
> > Ok, you could do more.
> > mmc_gpio_get_cd() is expected to return non-zero if card is detection.
> 
> > I think your board seems not to use pull-up on GPIO line for card detection.
> > So, MMC_CAP2_CD_ACTIVE_HIGH would be needed.
> 
> Unfortunately, the specific cd pin can not be configured.
> Suspect on our board the cd =1 when card is inserted, can not measure as
> no clear resister and need check with hardware guy tomorrow.
> 
> "cd-inverted" may be required as mentioned by Arnd.
> Will update the patch as following if no problem.

The patch looks right, but I wonder if a better longer-term
solution would be to just the common mmc_of_parse() function
rather than duplicating it in dw_mmc.

	Arnd

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Russell King - ARM Linux]

On Wed, Jan 15, 2014 at 11:51:01PM +0800, Zhangfei Gao wrote:
> Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b
> CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc)
> can not be checked by IS_ERR_VALUE.
> Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT,
> otherwise sd detect may occasionally fail.
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Reported-by: Kevin Hilman <khilman@linaro.org>
> Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
> Tested-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  drivers/mmc/host/dw_mmc.c  |   12 +++++++++---
>  include/linux/mmc/dw_mmc.h |    1 +
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index a776f24f4311..8326e54b96a8 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>  	int present;
>  	struct dw_mci_slot *slot = mmc_priv(mmc);
>  	struct dw_mci_board *brd = slot->host->pdata;
> -	int gpio_cd = !mmc_gpio_get_cd(mmc);
> +	struct dw_mci *host = slot->host;
> +	int gpio_cd = mmc_gpio_get_cd(mmc);
>  
>  	/* Use platform get_cd function, else try onboard card detect */
>  	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)
> @@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>  	else if (brd->get_cd)
>  		present = !brd->get_cd(slot->id);
>  	else if (!IS_ERR_VALUE(gpio_cd))
> -		present = !!gpio_cd;
> +		present = gpio_cd ^ brd->cd_inverted;

I assume you haven't read the contents of mmc_gpio_get_cd()?

int mmc_gpio_get_cd(struct mmc_host *host)
{
        struct mmc_gpio *ctx = host->slot.handler_priv;

        if (!ctx || !gpio_is_valid(ctx->cd_gpio))
                return -ENOSYS;

        return !gpio_get_value_cansleep(ctx->cd_gpio) ^
                !!(host->caps2 & MMC_CAP2_CD_ACTIVE_HIGH);
}
EXPORT_SYMBOL(mmc_gpio_get_cd);

What facility does host->caps2 give you here?  Yes, you don't need
your own cd_inverted stuff.  You should check that you're interpreting
this correctly.  When this returns true, there is a card present.

When MMC_CAP2_CD_ACTIVE_HIGH is set, gpio_get_value_cansleep() must
return non-zero when a card is inserted.  When it isn't set,
gpio_get_value_cansleep must return zero when a card is inserted.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Russell King - ARM Linux]

On Wed, Jan 15, 2014 at 02:59:59PM +0100, Arnd Bergmann wrote:
> On Wednesday 15 January 2014 21:56:26 zhangfei wrote:
> > However, in our board cd =0 when card is deteced while cd=1 when card is
> > removed.
> > In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be
> > set, as well as new property "caps2-mmc-cd-active-low".
> > 
> > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> > @@ -73,6 +73,8 @@ Optional properties:
> > +* caps2-mmc-cd-active-low: cd pin is low when card active
> > +
> > 
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > +       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
> > +               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
> > +
> 
> The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for
> legacy platforms. With DT parsing, you can normally specify
> the polarity of the GPIO line in the GPIO specifier in DT.

Since when?

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Arnd Bergmann]

On Wednesday 15 January 2014 16:01:46 Russell King - ARM Linux wrote:
> On Wed, Jan 15, 2014 at 02:59:59PM +0100, Arnd Bergmann wrote:
> > On Wednesday 15 January 2014 21:56:26 zhangfei wrote:
> > > However, in our board cd =0 when card is deteced while cd=1 when card is
> > > removed.
> > > In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be
> > > set, as well as new property "caps2-mmc-cd-active-low".
> > > 
> > > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> > > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> > > @@ -73,6 +73,8 @@ Optional properties:
> > > +* caps2-mmc-cd-active-low: cd pin is low when card active
> > > +
> > > 
> > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > > +       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
> > > +               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
> > > +
> > 
> > The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for
> > legacy platforms. With DT parsing, you can normally specify
> > the polarity of the GPIO line in the GPIO specifier in DT.
> 
> Since when?

I just looked through the bindings and found that about half
of them allow passing polarity in the gpio specifier. I thought
it was more than that and IIRC the outcome of a previous discussion
was that when the gpio controller allows passing polarity, you
should use that rather than a separate flag.

Not a problem though, since we can use the cd-inverted and
wp-inverted properties.

	Arnd

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Russell King - ARM Linux]

On Wed, Jan 15, 2014 at 05:07:49PM +0100, Arnd Bergmann wrote:
> On Wednesday 15 January 2014 16:01:46 Russell King - ARM Linux wrote:
> > On Wed, Jan 15, 2014 at 02:59:59PM +0100, Arnd Bergmann wrote:
> > > On Wednesday 15 January 2014 21:56:26 zhangfei wrote:
> > > > However, in our board cd =0 when card is deteced while cd=1 when card is
> > > > removed.
> > > > In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be
> > > > set, as well as new property "caps2-mmc-cd-active-low".
> > > > 
> > > > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> > > > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> > > > @@ -73,6 +73,8 @@ Optional properties:
> > > > +* caps2-mmc-cd-active-low: cd pin is low when card active
> > > > +
> > > > 
> > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > > > +       if (of_find_property(np, "caps2-mmc-cd-active-low", NULL))
> > > > +               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
> > > > +
> > > 
> > > The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for
> > > legacy platforms. With DT parsing, you can normally specify
> > > the polarity of the GPIO line in the GPIO specifier in DT.
> > 
> > Since when?
> 
> I just looked through the bindings and found that about half
> of them allow passing polarity in the gpio specifier. I thought
> it was more than that and IIRC the outcome of a previous discussion
> was that when the gpio controller allows passing polarity, you
> should use that rather than a separate flag.
> 
> Not a problem though, since we can use the cd-inverted and
> wp-inverted properties.

Well, having this in the gpio level as well as in subsystems like MMC
is going to create confusion - and from the results of this patch _IS_
causing confusion.  You've just encouraged one implementation to have
things setup such that mmc_gpio_get_cd() returns false when a card is
inserted, rather than it correctly returning true.

The issue here is that we'll potentially now end up with _three_
inversions.  One in the GPIO layers.  One in mmc_gpio_get_cd(), and
another in every driver which uses the GPIO layer inversion.  This
is hardly the right approach as it leads to multiple points where
confusion about the inverted status can occur.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[zhangfei]

On 01/16/2014 12:22 AM, Russell King - ARM Linux wrote:

> Well, having this in the gpio level as well as in subsystems like MMC
> is going to create confusion - and from the results of this patch _IS_
> causing confusion.  You've just encouraged one implementation to have
> things setup such that mmc_gpio_get_cd() returns false when a card is
> inserted, rather than it correctly returning true.
>
> The issue here is that we'll potentially now end up with _three_
> inversions.  One in the GPIO layers.  One in mmc_gpio_get_cd(), and
> another in every driver which uses the GPIO layer inversion.  This
> is hardly the right approach as it leads to multiple points where
> confusion about the inverted status can occur.
>

Thanks Russell, make sense.

Double checked with the hardware guy, cd pin is high when card inserted 
on the board.
The cd pin is originally connected to vout via a resister, and 
internally short to gound, which is broken when card inserted.

Then Optional properties "caps2-mmc-cd-active-high" has to be added.

--- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
+++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt

+* caps2-mmc-cd-active-high: cd pin is high when card present

@@ -2411,6 +2414,9 @@ static struct dw_mci_board *dw_mci_parse_dt(struct 
dw_mci *host)
         if (of_find_property(np, "caps2-mmc-hs200-1_2v", NULL))
                 pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR;

+       if (of_find_property(np, "caps2-mmc-cd-active-high", NULL))
+               pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+

Thanks

[PATCH v2] mmc: dw_mmc: fix dw_mci_get_cd

[Zhangfei Gao]

Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b
CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc)
can not be checked by IS_ERR_VALUE.
Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT,
otherwise sd detect may occasionally fail.

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Reported-by: Kevin Hilman <khilman@linaro.org>
Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
Tested-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 .../devicetree/bindings/mmc/synopsys-dw-mshc.txt   |    2 ++
 drivers/mmc/host/dw_mmc.c                          |   12 +++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
index 8f3f13315358..18b0819bafe0 100644
--- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
+++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
@@ -73,6 +73,8 @@ Optional properties:
 
 * caps2-mmc-hs200-1_2v: Supports mmc HS200 SDR 1.2V mode
 
+* caps2-mmc-cd-active-high: cd pin is high when card present
+
 * broken-cd: as documented in mmc core bindings.
 
 * vmmc-supply: The phandle to the regulator to use for vmmc.  If this is
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index a776f24f4311..ad65b895d34c 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	int present;
 	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci_board *brd = slot->host->pdata;
-	int gpio_cd = !mmc_gpio_get_cd(mmc);
+	struct dw_mci *host = slot->host;
+	int gpio_cd = mmc_gpio_get_cd(mmc);
 
 	/* Use platform get_cd function, else try onboard card detect */
 	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)
@@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	else if (brd->get_cd)
 		present = !brd->get_cd(slot->id);
 	else if (!IS_ERR_VALUE(gpio_cd))
-		present = !!gpio_cd;
+		present = gpio_cd;
 	else
 		present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
 			== 0 ? 1 : 0;
 
+	spin_lock_bh(&host->lock);
 	if (present) {
 		set_bit(DW_MMC_CARD_PRESENT, &slot->flags);
 		dev_dbg(&mmc->class_dev, "card is present\n");
@@ -1053,6 +1055,7 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 		clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
 		dev_dbg(&mmc->class_dev, "card is not present\n");
 	}
+	spin_unlock_bh(&host->lock);
 
 	return present;
 }
@@ -2081,7 +2084,7 @@ static int dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
 	return gpio;
 }
 
-/* find the cd gpio for a given slot; or -1 if none specified */
+/* find the cd gpio for a given slot */
 static void dw_mci_of_get_cd_gpio(struct device *dev, u8 slot,
 					struct mmc_host *mmc)
 {
@@ -2411,6 +2414,9 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
 	if (of_find_property(np, "caps2-mmc-hs200-1_2v", NULL))
 		pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR;
 
+	if (of_find_property(np, "caps2-mmc-cd-active-high", NULL))
+		pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+
 	return pdata;
 }
 
-- 
1.7.9.5

[PATCH v2] mmc: dw_mmc: fix dw_mci_get_cd

[Arnd Bergmann]

On Thursday 16 January 2014, Zhangfei Gao wrote:
> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> @@ -73,6 +73,8 @@ Optional properties:
>  
>  * caps2-mmc-hs200-1_2v: Supports mmc HS200 SDR 1.2V mode
>  
> +* caps2-mmc-cd-active-high: cd pin is high when card present
> +
>  * broken-cd: as documented in mmc core bindings.
>  
>  * vmmc-supply: The phandle to the regulator to use for vmmc.  If this is

No, don't introduce a new flag when there is already one defined
in the generic mmc binding.

	Arnd

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Arnd Bergmann]

On Wednesday 15 January 2014, Russell King - ARM Linux wrote:
> The issue here is that we'll potentially now end up with three
> inversions.  One in the GPIO layers.  One in mmc_gpio_get_cd(), and
> another in every driver which uses the GPIO layer inversion.  This
> is hardly the right approach as it leads to multiple points where
> confusion about the inverted status can occur.

I did not mean to suggest adding a third inversion. We already have
to deal with the one in the GPIO layer and the one in mmc_gpio_get_cd(),
which I absolutely agree is unfortunate. As was pointed out already,
the gpio driver used in this system does not support the inversion
in the DT binding, so the only proper solution is to use the
inversion from mmc_gpio_get_cd if MMC_CAP2_CD_ACTIVE_HIGH is set
in host->caps2. This flag gets set from mmc_of_parse based
on the presence of the "cd-inverted" flag, in the absence of the
gpio flags. The dw_mmc driver does not use mmc_of_parse() at the
moment, and while it probably should use that in the future,
it should definitely read the same DT properties with the
same semantics already and not introduce new properties.

	Arnd

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Russell King - ARM Linux]

On Thu, Jan 16, 2014 at 12:12:10PM +0100, Arnd Bergmann wrote:
> On Wednesday 15 January 2014, Russell King - ARM Linux wrote:
> > The issue here is that we'll potentially now end up with three
> > inversions.  One in the GPIO layers.  One in mmc_gpio_get_cd(), and
> > another in every driver which uses the GPIO layer inversion.  This
> > is hardly the right approach as it leads to multiple points where
> > confusion about the inverted status can occur.
> 
> I did not mean to suggest adding a third inversion. We already have
> to deal with the one in the GPIO layer and the one in mmc_gpio_get_cd(),
> which I absolutely agree is unfortunate. As was pointed out already,
> the gpio driver used in this system does not support the inversion
> in the DT binding, so the only proper solution is to use the
> inversion from mmc_gpio_get_cd if MMC_CAP2_CD_ACTIVE_HIGH is set
> in host->caps2. This flag gets set from mmc_of_parse based
> on the presence of the "cd-inverted" flag, in the absence of the
> gpio flags. The dw_mmc driver does not use mmc_of_parse() at the
> moment, and while it probably should use that in the future,
> it should definitely read the same DT properties with the
> same semantics already and not introduce new properties.

Arnd,

The issue I'm pointing out is that _if_ you use the GPIO layer to do
the inversion, then you end up with _three_ inversions of the signal,
which is excessive and confusing - and makes the sense of
mmc_gpio_get_cd() completely indeterminant without reading the DT
files and the code.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v2] mmc: dw_mmc: fix dw_mci_get_cd

[zhangfei]

On 01/16/2014 06:50 PM, Arnd Bergmann wrote:
> On Thursday 16 January 2014, Zhangfei Gao wrote:
>> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
>> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
>> @@ -73,6 +73,8 @@ Optional properties:
>>
>>   * caps2-mmc-hs200-1_2v: Supports mmc HS200 SDR 1.2V mode
>>
>> +* caps2-mmc-cd-active-high: cd pin is high when card present
>> +
>>   * broken-cd: as documented in mmc core bindings.
>>
>>   * vmmc-supply: The phandle to the regulator to use for vmmc.  If this is
>
> No, don't introduce a new flag when there is already one defined
> in the generic mmc binding.

Got it.

Update with "cd-inverted".

      if (of_get_property(np, "cd-inverted", NULL))
                 pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;


Thanks

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Zhangfei Gao]

Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b
CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc)
can not be checked by IS_ERR_VALUE.
Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT,
otherwise sd detect may occasionally fail.

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Reported-by: Kevin Hilman <khilman@linaro.org>
Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
Tested-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/mmc/host/dw_mmc.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index a776f24f4311..55cd110a49c4 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	int present;
 	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci_board *brd = slot->host->pdata;
-	int gpio_cd = !mmc_gpio_get_cd(mmc);
+	struct dw_mci *host = slot->host;
+	int gpio_cd = mmc_gpio_get_cd(mmc);
 
 	/* Use platform get_cd function, else try onboard card detect */
 	if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)
@@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 	else if (brd->get_cd)
 		present = !brd->get_cd(slot->id);
 	else if (!IS_ERR_VALUE(gpio_cd))
-		present = !!gpio_cd;
+		present = gpio_cd;
 	else
 		present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
 			== 0 ? 1 : 0;
 
+	spin_lock_bh(&host->lock);
 	if (present) {
 		set_bit(DW_MMC_CARD_PRESENT, &slot->flags);
 		dev_dbg(&mmc->class_dev, "card is present\n");
@@ -1053,6 +1055,7 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
 		clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
 		dev_dbg(&mmc->class_dev, "card is not present\n");
 	}
+	spin_unlock_bh(&host->lock);
 
 	return present;
 }
@@ -2081,7 +2084,7 @@ static int dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
 	return gpio;
 }
 
-/* find the cd gpio for a given slot; or -1 if none specified */
+/* find the cd gpio for a given slot */
 static void dw_mci_of_get_cd_gpio(struct device *dev, u8 slot,
 					struct mmc_host *mmc)
 {
@@ -2411,6 +2414,9 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
 	if (of_find_property(np, "caps2-mmc-hs200-1_2v", NULL))
 		pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR;
 
+	if (of_get_property(np, "cd-inverted", NULL))
+		pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+
 	return pdata;
 }
 
-- 
1.7.9.5

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Kevin Hilman]

Zhangfei Gao <zhangfei.gao@linaro.org> writes:

> Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b
> CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc)
> can not be checked by IS_ERR_VALUE.
> Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT,
> otherwise sd detect may occasionally fail.
>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Reported-by: Kevin Hilman <khilman@linaro.org>
> Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
> Tested-by: Sachin Kamat <sachin.kamat@linaro.org>

I didn't look at the patch in detail, but can at least confirm that
Arndale boots again in -next with this patch applied.

Tested-by: Kevin Hilman <khilman@linaro.org>

Kevin

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Seungwon Jeon]

On Fri, January 17, 2014, Kevin Hilman wrote:
> Zhangfei Gao <zhangfei.gao@linaro.org> writes:
> 
> > Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b
> > CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc)
> > can not be checked by IS_ERR_VALUE.
> > Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT,
> > otherwise sd detect may occasionally fail.
> >
> > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> > Reported-by: Kevin Hilman <khilman@linaro.org>
> > Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
> > Tested-by: Sachin Kamat <sachin.kamat@linaro.org>
> 
> I didn't look at the patch in detail, but can at least confirm that
> Arndale boots again in -next with this patch applied.
> 
> Tested-by: Kevin Hilman <khilman@linaro.org>

Acked-by: Seungwon Jeon <tgih.jun@samsung.com>

Thanks,
Seungwon Jeon

[PATCH] mmc: dw_mmc: fix dw_mci_get_cd

[Chris Ball]

Hi,

On Fri, Jan 17 2014, Seungwon Jeon wrote:
> On Fri, January 17, 2014, Kevin Hilman wrote:
>> Zhangfei Gao <zhangfei.gao@linaro.org> writes:
>> 
>> > Introduced from commit bf626e5550f24aec24975a0e85ad8e572ca76a6b
>> > CDETECT is ignored since negated return value of mmc_gpio_get_cd(mmc)
>> > can not be checked by IS_ERR_VALUE.
>> > Add spin_lock_bh(&host->lock) for atomic accessing DW_MMC_CARD_PRESENT,
>> > otherwise sd detect may occasionally fail.
>> >
>> > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> > Reported-by: Kevin Hilman <khilman@linaro.org>
>> > Reviewed-by: Sachin Kamat <sachin.kamat@linaro.org>
>> > Tested-by: Sachin Kamat <sachin.kamat@linaro.org>
>> 
>> I didn't look at the patch in detail, but can at least confirm that
>> Arndale boots again in -next with this patch applied.
>> 
>> Tested-by: Kevin Hilman <khilman@linaro.org>
>
> Acked-by: Seungwon Jeon <tgih.jun@samsung.com>

Thanks, pushed to mmc-next for 3.14.

- Chris.
-- 
Chris Ball   <chris@printf.net>   <http://printf.net/>