[PATCH] mmc: mxs-mmc: implement broken-cd

[PATCH] mmc: mxs-mmc: implement broken-cd

[Lauri Hintsala]

This feature allows to use SDIO bus without wiring card detect
signal. This is relevant in cases where SDIO device is connected to
SDIO bus and there is no way to disconnect device from bus (device
is always present).

Signed-off-by: Lauri Hintsala <lauri.hintsala@bluegiga.com>
---
 drivers/mmc/host/mxs-mmc.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
index ad3fcea..992760e 100644
--- a/drivers/mmc/host/mxs-mmc.c
+++ b/drivers/mmc/host/mxs-mmc.c
@@ -165,6 +165,7 @@ struct mxs_mmc_host {
 	int				sdio_irq_en;
 	int				wp_gpio;
 	bool				wp_inverted;
+	bool				broken_cd;
 };
 
 static int mxs_mmc_get_ro(struct mmc_host *mmc)
@@ -187,6 +188,9 @@ static int mxs_mmc_get_cd(struct mmc_host *mmc)
 {
 	struct mxs_mmc_host *host = mmc_priv(mmc);
 
+	if (host->broken_cd)
+		return -ENOSYS;
+
 	return !(readl(host->base + HW_SSP_STATUS(host)) &
 		 BM_SSP_STATUS_CARD_DETECT);
 }
@@ -808,6 +812,9 @@ static int mxs_mmc_probe(struct platform_device *pdev)
 							&flags);
 		if (flags & OF_GPIO_ACTIVE_LOW)
 			host->wp_inverted = 1;
+
+		if (of_get_property(np, "broken-cd", NULL))
+			host->broken_cd = 1;
 	} else {
 		if (pdata->flags & SLOTF_8_BIT_CAPABLE)
 			mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
-- 
1.7.9.5

[PATCH] mmc: mxs-mmc: implement broken-cd

[Shawn Guo]

On Fri, Sep 07, 2012 at 02:45:17PM +0300, Lauri Hintsala wrote:
> This feature allows to use SDIO bus without wiring card detect
> signal.

This looks like a case of "broken-cd" ...

> This is relevant in cases where SDIO device is connected to
> SDIO bus and there is no way to disconnect device from bus (device
> is always present).
> 
... while this sounds like a case of "non-removable".

> Signed-off-by: Lauri Hintsala <lauri.hintsala@bluegiga.com>
> ---
>  drivers/mmc/host/mxs-mmc.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
> index ad3fcea..992760e 100644
> --- a/drivers/mmc/host/mxs-mmc.c
> +++ b/drivers/mmc/host/mxs-mmc.c
> @@ -165,6 +165,7 @@ struct mxs_mmc_host {
>  	int				sdio_irq_en;
>  	int				wp_gpio;
>  	bool				wp_inverted;
> +	bool				broken_cd;
>  };
>  
>  static int mxs_mmc_get_ro(struct mmc_host *mmc)
> @@ -187,6 +188,9 @@ static int mxs_mmc_get_cd(struct mmc_host *mmc)
>  {
>  	struct mxs_mmc_host *host = mmc_priv(mmc);
>  
> +	if (host->broken_cd)
> +		return -ENOSYS;
> +

The .get_cd caller does not check for error.  This change makes no
sense to me.  Instead, the driver needs to set MMC_CAP_NEEDS_POLL in
case of "broken-cd".

Regards,
Shawn

>  	return !(readl(host->base + HW_SSP_STATUS(host)) &
>  		 BM_SSP_STATUS_CARD_DETECT);
>  }
> @@ -808,6 +812,9 @@ static int mxs_mmc_probe(struct platform_device *pdev)
>  							&flags);
>  		if (flags & OF_GPIO_ACTIVE_LOW)
>  			host->wp_inverted = 1;
> +
> +		if (of_get_property(np, "broken-cd", NULL))
> +			host->broken_cd = 1;
>  	} else {
>  		if (pdata->flags & SLOTF_8_BIT_CAPABLE)
>  			mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
> -- 
> 1.7.9.5
> 

[PATCH] mmc: mxs-mmc: implement broken-cd

[Lauri Hintsala]

Hi Shawn,

On 09/10/2012 09:08 AM, Shawn Guo wrote:
> On Fri, Sep 07, 2012 at 02:45:17PM +0300, Lauri Hintsala wrote:
>> This feature allows to use SDIO bus without wiring card detect
>> signal.
>
> This looks like a case of "broken-cd" ...
>
>> This is relevant in cases where SDIO device is connected to
>> SDIO bus and there is no way to disconnect device from bus (device
>> is always present).
>>
> ... while this sounds like a case of "non-removable".

Right, I'll remove this comment.

In our case the SDIO device is always physically connected to SDIO bus 
but the power of the device can be controller. So in SW point of view 
the device can be removed by switching the power off.


>> Signed-off-by: Lauri Hintsala <lauri.hintsala@bluegiga.com>
>> ---
>>   drivers/mmc/host/mxs-mmc.c |    7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
>> index ad3fcea..992760e 100644
>> --- a/drivers/mmc/host/mxs-mmc.c
>> +++ b/drivers/mmc/host/mxs-mmc.c
>> @@ -165,6 +165,7 @@ struct mxs_mmc_host {
>>   	int				sdio_irq_en;
>>   	int				wp_gpio;
>>   	bool				wp_inverted;
>> +	bool				broken_cd;
>>   };
>>
>>   static int mxs_mmc_get_ro(struct mmc_host *mmc)
>> @@ -187,6 +188,9 @@ static int mxs_mmc_get_cd(struct mmc_host *mmc)
>>   {
>>   	struct mxs_mmc_host *host = mmc_priv(mmc);
>>
>> +	if (host->broken_cd)
>> +		return -ENOSYS;
>> +
>
> The .get_cd caller does not check for error.  This change makes no
> sense to me.

In our case we don't want to read card detect bit and we have to 
continue MMC probing because card detect pin is routed nowhere.

Could it be better to return zero instead of error? ENOSYS is used to 
indicate "unreachable card detect" in other drivers (e.g. atmel-mci, 
omap_hsmmc) and I followed them.


> Instead, the driver needs to set MMC_CAP_NEEDS_POLL in
> case of "broken-cd".

MMC_CAP_NEEDS_POLL is always set in mxs-mmc.


Best regards,
Lauri


>
> Regards,
> Shawn
>
>>   	return !(readl(host->base + HW_SSP_STATUS(host)) &
>>   		 BM_SSP_STATUS_CARD_DETECT);
>>   }
>> @@ -808,6 +812,9 @@ static int mxs_mmc_probe(struct platform_device *pdev)
>>   							&flags);
>>   		if (flags & OF_GPIO_ACTIVE_LOW)
>>   			host->wp_inverted = 1;
>> +
>> +		if (of_get_property(np, "broken-cd", NULL))
>> +			host->broken_cd = 1;
>>   	} else {
>>   		if (pdata->flags & SLOTF_8_BIT_CAPABLE)
>>   			mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
>> --
>> 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: mxs-mmc: implement broken-cd

[Shawn Guo]

On Mon, Sep 10, 2012 at 01:04:00PM +0300, Lauri Hintsala wrote:
> >>@@ -187,6 +188,9 @@ static int mxs_mmc_get_cd(struct mmc_host *mmc)
> >>  {
> >>  	struct mxs_mmc_host *host = mmc_priv(mmc);
> >>
> >>+	if (host->broken_cd)
> >>+		return -ENOSYS;
> >>+
> >
> >The .get_cd caller does not check for error.  This change makes no
> >sense to me.
> 
> In our case we don't want to read card detect bit and we have to
> continue MMC probing because card detect pin is routed nowhere.
> 
> Could it be better to return zero instead of error? ENOSYS is used
> to indicate "unreachable card detect" in other drivers (e.g.
> atmel-mci, omap_hsmmc) and I followed them.
> 
I was wrong.  Even the kerneldoc of .get_cd suggests return -ENOSYS
for that case.

> 
> >Instead, the driver needs to set MMC_CAP_NEEDS_POLL in
> >case of "broken-cd".
> 
> MMC_CAP_NEEDS_POLL is always set in mxs-mmc.
> 
Oops, I forgot that.  But this is really something that needs to be
fixed.  Per my understanding, flag MMC_CAP_NEEDS_POLL should only
be set when card-detect is not working.

Also the card-detect in gpio support is missing.

But for this patch:

Acked-by: Shawn Guo <shawn.guo@linaro.org>

-- 
Regards,
Shawn

[PATCH] mmc: mxs-mmc: implement broken-cd

[Lauri Hintsala]

Hi,

On 09/10/2012 04:50 PM, Shawn Guo wrote:
> On Mon, Sep 10, 2012 at 01:04:00PM +0300, Lauri Hintsala wrote:
>>>> @@ -187,6 +188,9 @@ static int mxs_mmc_get_cd(struct mmc_host *mmc)
>>>>   {
>>>>   	struct mxs_mmc_host *host = mmc_priv(mmc);
>>>>
>>>> +	if (host->broken_cd)
>>>> +		return -ENOSYS;
>>>> +

...

> But for this patch:
>
> Acked-by: Shawn Guo <shawn.guo@linaro.org>

Any chance to get this patch for 3.7? It makes possible to use the 
driver without a card detect signal.

Regards,
Lauri

[PATCH] mmc: mxs-mmc: implement broken-cd

[Matt Sealey]

I think this describes three use cases which are different, as Shawn
said we have here;

* missing card detect support (card detect is not wired so it's
impossible to tell, and the controller doesn't support the SD standard
card detection)
* non-removable device
* broken card detect support

The third one is what the property sounds like it describes, but this
is not the use case you are describing at all. This kind of property
naming is more like describing "card detect doesn't operate reliably"
which is true of i.MX devices with certain versions of the eSDHC and
uSDHC controllers, where non-GPIO card detection interferes with SDIO
interrupts, but in this case a board designer should add a real card
detect pin to the board (most more expensive push-push SD card slots
come with a pin you can wire for CD). In cases where errata is
published after boards are shipped, broken-cd is a meaningful
description when described in the absense of a gpio cd description, or
presence of some cd-handled-internally style property (forgive me for
not cross-referencing the FSL definition for this property, we don't
use it here at Genesi since all our boards have GPIO CD)

It seems your device is a combination of the top two in the list, it
is not down to a broken feature at all, but one that should be
possible to not implement for devices which are permanently connected.
A non-removable device should be able to be powered down, at least
using runtime PM or clock gating (if this works, remember to whitelist
the card for clock gating!) but card detect shouldn't be used in this
case to detect if the card is powered or not (this is a problem for
your card controller and card driver state tracker. The MMC subsystem
already tracks this state fairly well for power management).

I would refrain from calling a feature "broken card detect" if there
is actually no breakage involved especially if it would be more
prudent instead look into how to figure out how to track power
management state properly without cluttering the device tree.

-- 
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.


On Mon, Sep 10, 2012 at 5:04 AM, Lauri Hintsala
<lauri.hintsala@bluegiga.com> wrote:
> Hi Shawn,
>
>
> On 09/10/2012 09:08 AM, Shawn Guo wrote:
>>
>> On Fri, Sep 07, 2012 at 02:45:17PM +0300, Lauri Hintsala wrote:
>>>
>>> This feature allows to use SDIO bus without wiring card detect
>>> signal.
>>
>>
>> This looks like a case of "broken-cd" ...
>>
>>> This is relevant in cases where SDIO device is connected to
>>> SDIO bus and there is no way to disconnect device from bus (device
>>> is always present).
>>>
>> ... while this sounds like a case of "non-removable".
>
>
> Right, I'll remove this comment.
>
> In our case the SDIO device is always physically connected to SDIO bus but
> the power of the device can be controller. So in SW point of view the device
> can be removed by switching the power off.
>
>
>
>>> Signed-off-by: Lauri Hintsala <lauri.hintsala@bluegiga.com>
>>> ---
>>>   drivers/mmc/host/mxs-mmc.c |    7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
>>> index ad3fcea..992760e 100644
>>> --- a/drivers/mmc/host/mxs-mmc.c
>>> +++ b/drivers/mmc/host/mxs-mmc.c
>>> @@ -165,6 +165,7 @@ struct mxs_mmc_host {
>>>         int                             sdio_irq_en;
>>>         int                             wp_gpio;
>>>         bool                            wp_inverted;
>>> +       bool                            broken_cd;
>>>   };
>>>
>>>   static int mxs_mmc_get_ro(struct mmc_host *mmc)
>>> @@ -187,6 +188,9 @@ static int mxs_mmc_get_cd(struct mmc_host *mmc)
>>>   {
>>>         struct mxs_mmc_host *host = mmc_priv(mmc);
>>>
>>> +       if (host->broken_cd)
>>> +               return -ENOSYS;
>>> +
>>
>>
>> The .get_cd caller does not check for error.  This change makes no
>> sense to me.
>
>
> In our case we don't want to read card detect bit and we have to continue
> MMC probing because card detect pin is routed nowhere.
>
> Could it be better to return zero instead of error? ENOSYS is used to
> indicate "unreachable card detect" in other drivers (e.g. atmel-mci,
> omap_hsmmc) and I followed them.
>
>
>
>> Instead, the driver needs to set MMC_CAP_NEEDS_POLL in
>> case of "broken-cd".
>
>
> MMC_CAP_NEEDS_POLL is always set in mxs-mmc.
>
>
> Best regards,
> Lauri
>
>
>>
>> Regards,
>> Shawn
>>
>>>         return !(readl(host->base + HW_SSP_STATUS(host)) &
>>>                  BM_SSP_STATUS_CARD_DETECT);
>>>   }
>>> @@ -808,6 +812,9 @@ static int mxs_mmc_probe(struct platform_device
>>> *pdev)
>>>                                                         &flags);
>>>                 if (flags & OF_GPIO_ACTIVE_LOW)
>>>                         host->wp_inverted = 1;
>>> +
>>> +               if (of_get_property(np, "broken-cd", NULL))
>>> +                       host->broken_cd = 1;
>>>         } else {
>>>                 if (pdata->flags & SLOTF_8_BIT_CAPABLE)
>>>                         mmc->caps |= MMC_CAP_4_BIT_DATA |
>>> MMC_CAP_8_BIT_DATA;
>>> --
>>> 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
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] mmc: mxs-mmc: implement broken-cd

[Lauri Hintsala]

Hi,

On 09/10/2012 05:58 PM, Matt Sealey wrote:
> I think this describes three use cases which are different, as Shawn
> said we have here;
>
> * missing card detect support (card detect is not wired so it's
> impossible to tell, and the controller doesn't support the SD standard
> card detection)
> * non-removable device
> * broken card detect support
>
> The third one is what the property sounds like it describes, but this
> is not the use case you are describing at all. This kind of property
> naming is more like describing "card detect doesn't operate reliably"
> which is true of i.MX devices with certain versions of the eSDHC and
> uSDHC controllers, where non-GPIO card detection interferes with SDIO
> interrupts, but in this case a board designer should add a real card
> detect pin to the board (most more expensive push-push SD card slots
> come with a pin you can wire for CD). In cases where errata is
> published after boards are shipped, broken-cd is a meaningful
> description when described in the absense of a gpio cd description, or
> presence of some cd-handled-internally style property (forgive me for
> not cross-referencing the FSL definition for this property, we don't
> use it here at Genesi since all our boards have GPIO CD)
>
> It seems your device is a combination of the top two in the list, it
> is not down to a broken feature at all, but one that should be
> possible to not implement for devices which are permanently connected.
> A non-removable device should be able to be powered down, at least
> using runtime PM or clock gating (if this works, remember to whitelist
> the card for clock gating!) but card detect shouldn't be used in this
> case to detect if the card is powered or not (this is a problem for
> your card controller and card driver state tracker. The MMC subsystem
> already tracks this state fairly well for power management).

Do you mean the polling mode shouldn't been used to detect if the device 
is powered?

Is there any examples how PM support could be implemented? I'm not 
familiar with PM subsystem. How is it done in practice? Should I play 
with fixed regulators?

BR,
Lauri


> I would refrain from calling a feature "broken card detect" if there
> is actually no breakage involved especially if it would be more
> prudent instead look into how to figure out how to track power
> management state properly without cluttering the device tree.
>

[PATCH] mmc: mxs-mmc: implement broken-cd

[Lauri Hintsala]

Hi,

Is it OK to use broken-cd? broken-cd feature is documented as "There is 
no card detection available; polling must be used". In this case the 
card detect is not broken but it is unrouted so it is unavailable.

Documentation about broken-cd has been added by commit:
https://git.kernel.org/?p=linux/kernel/git/cjb/mmc.git;a=commitdiff;h=abe1e05da365350ac282ba5f6831aae13d97074e

Any tips about power management implementation are welcome if I have to 
use PM instead of polling method.

Regards,
Lauri


On 09/12/2012 05:06 PM, Lauri Hintsala wrote:
> Hi,
>
> On 09/10/2012 05:58 PM, Matt Sealey wrote:
>> I think this describes three use cases which are different, as Shawn
>> said we have here;
>>
>> * missing card detect support (card detect is not wired so it's
>> impossible to tell, and the controller doesn't support the SD standard
>> card detection)
>> * non-removable device
>> * broken card detect support
>>
>> The third one is what the property sounds like it describes, but this
>> is not the use case you are describing at all. This kind of property
>> naming is more like describing "card detect doesn't operate reliably"
>> which is true of i.MX devices with certain versions of the eSDHC and
>> uSDHC controllers, where non-GPIO card detection interferes with SDIO
>> interrupts, but in this case a board designer should add a real card
>> detect pin to the board (most more expensive push-push SD card slots
>> come with a pin you can wire for CD). In cases where errata is
>> published after boards are shipped, broken-cd is a meaningful
>> description when described in the absense of a gpio cd description, or
>> presence of some cd-handled-internally style property (forgive me for
>> not cross-referencing the FSL definition for this property, we don't
>> use it here at Genesi since all our boards have GPIO CD)
>>
>> It seems your device is a combination of the top two in the list, it
>> is not down to a broken feature at all, but one that should be
>> possible to not implement for devices which are permanently connected.
>> A non-removable device should be able to be powered down, at least
>> using runtime PM or clock gating (if this works, remember to whitelist
>> the card for clock gating!) but card detect shouldn't be used in this
>> case to detect if the card is powered or not (this is a problem for
>> your card controller and card driver state tracker. The MMC subsystem
>> already tracks this state fairly well for power management).
>
> Do you mean the polling mode shouldn't been used to detect if the device
> is powered?
>
> Is there any examples how PM support could be implemented? I'm not
> familiar with PM subsystem. How is it done in practice? Should I play
> with fixed regulators?
>
> BR,
> Lauri
>
>
>> I would refrain from calling a feature "broken card detect" if there
>> is actually no breakage involved especially if it would be more
>> prudent instead look into how to figure out how to track power
>> management state properly without cluttering the device tree.
>>
> --
> 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: mxs-mmc: implement broken-cd

[Chris Ball]

Hi,

On Mon, Sep 17 2012, Lauri Hintsala wrote:
> Is it OK to use broken-cd? broken-cd feature is documented as "There
> is no card detection available; polling must be used". In this case
> the card detect is not broken but it is unrouted so it is unavailable.
>
> Documentation about broken-cd has been added by commit:
> https://git.kernel.org/?p=linux/kernel/git/cjb/mmc.git;a=commitdiff;h=abe1e05da365350ac282ba5f6831aae13d97074e

I'm having trouble following the problem; it sounds like perhaps all you
need is:

        if (of_find_property(np, "non-removable", NULL))
                mmc->caps |= MMC_CAP_NONREMOVABLE;

Have you tried that?

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH] mmc: mxs-mmc: implement broken-cd

[Lauri Hintsala]

Hi,

On 09/17/2012 05:01 PM, Chris Ball wrote:
> Hi,
>
> On Mon, Sep 17 2012, Lauri Hintsala wrote:
>> Is it OK to use broken-cd? broken-cd feature is documented as "There
>> is no card detection available; polling must be used". In this case
>> the card detect is not broken but it is unrouted so it is unavailable.
>>
>> Documentation about broken-cd has been added by commit:
>> https://git.kernel.org/?p=linux/kernel/git/cjb/mmc.git;a=commitdiff;h=abe1e05da365350ac282ba5f6831aae13d97074e
>
> I'm having trouble following the problem; it sounds like perhaps all you
> need is:
>
>          if (of_find_property(np, "non-removable", NULL))
>                  mmc->caps |= MMC_CAP_NONREMOVABLE;
>
> Have you tried that?

Yes I have and it does not work. Current mxs-mmc driver uses polling 
method and it reads the CARD_DETECT bit of STATUS register. In our case 
card detect is signal is unrouted so SDIO device is never detect.

Another problem with NONREMOVABLE feature is in case where power cycle 
or reset is done for SDIO device. A device drive doesn't detect HW 
reset. After reset the device driver is still in running state and it 
does not reinitialize the device. So after reset device stops to work.

Lauri