Skip to contentSkip to conversation

mmc: mxs-mmc: implement broken-cd

PATCH New Lauri Hintsala 4 participants 10 messages Sep 7, 2012 – Sep 20, 2012

Subsystems (2)
Arm i.MX and Layerscape MMC, SD and SDIO
lore
Share & tools
lore

Lauri Hintsala

author
#0 lore raw

PATCH 1/1 mmc: mxs-mmc: implement broken-cd

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 <[email protected]>
---
 drivers/mmc/host/mxs-mmc.c |    7 +++++++
 1 file changed, 7 insertions(+)
drivers/mmc/host/mxs-mmc.c +7 -0
index ad3fcea..992760e 100644
@@ -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;
Signature · 1 line
1.7.9.5
#1 lore raw

PATCH 1/1 mmc: mxs-mmc: implement broken-cd

Quoted 2 lines · Lauri Hintsala

This feature allows to use SDIO bus without wiring card detect signal.

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

Quoted 4 lines

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".

Quoted 18 lines
Signed-off-by Lauri Hintsala <[email protected]>
---
 drivers/mmc/host/mxs-mmc.c |    7 +++++++
 1 file changed, 7 insertions(+)
drivers/mmc/host/mxs-mmc.c +1 -0
index ad3fcea..992760e 100644
@@ -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)

drivers/mmc/host/mxs-mmc.c static int mxs_mmc_get_cd(struct mmc_host *mmc) · line 193

Quoted 6 lines
drivers/mmc/host/mxs-mmc.c +3 -0
@@ -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

Quoted 16 lines
drivers/mmc/host/mxs-mmc.c +3 -0
 	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;
Signature · 1 line
1.7.9.5
author
#2 lore raw

PATCH 1/1 mmc: mxs-mmc: implement broken-cd

Hi Shawn,

Quoted 11 lines · Shawn Guo
Quoted 2 lines · Lauri Hintsala

This feature allows to use SDIO bus without wiring card detect signal.

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

Quoted 4 lines

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.

Quoted 21 lines
Quoted 21 lines
Signed-off-by Lauri Hintsala <[email protected]>
---
  drivers/mmc/host/mxs-mmc.c |    7 +++++++
  1 file changed, 7 insertions(+)
drivers/mmc/host/mxs-mmc.c +1 -0
index ad3fcea..992760e 100644
@@ -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);
Quoted 6 lines

drivers/mmc/host/mxs-mmc.c static int mxs_mmc_get_cd(struct mmc_host *mmc) · line 193

Quoted 3 lines
drivers/mmc/host/mxs-mmc.c +3 -0
+	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.

Quoted 2 lines

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

Quoted 25 lines

Regards,
Shawn

Quoted 14 lines
drivers/mmc/host/mxs-mmc.c +3 -1
  	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;
--
Quoted 2 lines

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

#3 lore raw

PATCH 1/1 mmc: mxs-mmc: implement broken-cd

Quoted 11 lines · Lauri Hintsala
Quoted 10 lines

drivers/mmc/host/mxs-mmc.c static int mxs_mmc_get_cd(struct mmc_host *mmc) · line 193

Quoted 7 lines
drivers/mmc/host/mxs-mmc.c +3 -0
@@ -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.

Quoted 7 lines

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.

Quoted 6 lines
Quoted 3 lines

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 <[email protected]>
Signature · 2 lines
Regards,
Shawn
author
#4 lore raw

PATCH 1/1 mmc: mxs-mmc: implement broken-cd

Hi,

Quoted 8 lines · Shawn Guo
Quoted 7 lines · Lauri Hintsala
Quoted 7 lines
Quoted 7 lines
drivers/mmc/host/mxs-mmc.c +3 -0
@@ -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;
+

...

Quoted 3 lines

But for this patch:

Acked-by Shawn Guo <[email protected]>

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

Regards,
Lauri

#5 lore raw

PATCH 1/1 mmc: mxs-mmc: implement broken-cd

In reply to Lauri Hintsala

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.

Signature · 5 lines
Matt Sealey <[email protected]>
Product Development Analyst, Genesi USA, Inc.


On Mon, Sep 10, 2012 at 5:04 AM, Lauri Hintsala
Quoted 110 lines · <[email protected]> wrote:

Hi Shawn,

Quoted 7 lines · Shawn Guo
Quoted 3 lines · Lauri Hintsala

This feature allows to use SDIO bus without wiring card detect signal.

Quoted 7 lines

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

Quoted 4 lines

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.

Quoted 21 lines
Quoted 21 lines
Signed-off-by Lauri Hintsala <[email protected]>
---
  drivers/mmc/host/mxs-mmc.c |    7 +++++++
  1 file changed, 7 insertions(+)
drivers/mmc/host/mxs-mmc.c +1 -0
index ad3fcea..992760e 100644
@@ -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);
Quoted 7 lines

drivers/mmc/host/mxs-mmc.c static int mxs_mmc_get_cd(struct mmc_host *mmc) · line 193

Quoted 3 lines
drivers/mmc/host/mxs-mmc.c +3 -0
+       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.

Quoted 2 lines

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

Quoted 18 lines

Regards,
Shawn

Quoted 14 lines
drivers/mmc/host/mxs-mmc.c +3 -0
        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 |
Quoted 9 lines
Quoted 4 lines

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

author
#6 lore raw

PATCH 1/1 mmc: mxs-mmc: implement broken-cd

Hi,

Quoted 23 lines · 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)

Quoted 9 lines

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

Quoted 5 lines

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.

author
#7 lore raw

PATCH 1/1 mmc: mxs-mmc: implement broken-cd

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

Quoted 56 lines · Lauri Hintsala

Hi,

Quoted 23 lines · 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)

Quoted 9 lines

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

Quoted 5 lines

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

#8 lore raw

PATCH 1/1 mmc: mxs-mmc: implement broken-cd

Hi,

Quoted 6 lines · Lauri Hintsala

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.

Signature · 2 lines
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child
author
#9 lore raw

PATCH 1/1 mmc: mxs-mmc: implement broken-cd

Hi,

Quoted 10 lines · Chris Ball

Hi,

Quoted 6 lines · Lauri Hintsala

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

Quoted 7 lines

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