[PATCH 1/4 v3] MMC/core: Add f_min to mmc_power_on()

[PATCH 1/4 v3] MMC/core: Add f_min to mmc_power_on()

[r66093]

From: Jerry Huang <Chang-Ming.Huang@freescale.com>

When f_init is zero, the SDHC can't work correctly. So f_min will replace
f_init, when f_init is zero.

Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
CC: Anton Vorontsov <cbouatmailru@gmail.com>
CC: Chris Ball <cjb@laptop.org>
---
changes for v2:
	- add the CC
changes for v3:
	- enalbe the controller clock in platform, instead of core

 drivers/mmc/core/core.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 06c42cf..9c162cd 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1319,7 +1319,10 @@ static void mmc_power_up(struct mmc_host *host)
 	 */
 	mmc_delay(10);
 
-	host->ios.clock = host->f_init;
+	if (host->f_init)
+		host->ios.clock = host->f_init;
+	else
+		host->ios.clock = host->f_min;
 
 	host->ios.power_mode = MMC_POWER_ON;
 	mmc_set_ios(host);
-- 
1.7.9.5

[PATCH 2/4 v4] MMC/SD: Add callback function to detect card

[r66093]

From: Jerry Huang <Chang-Ming.Huang@freescale.com>

In order to check whether the card has been removed, the function
mmc_send_status() will send command CMD13 to card and ask the card
to send its status register to sdhc driver, which will generate
many interrupts repeatedly and make the system performance bad.

Therefore, add callback function get_cd() to check whether
the card has been removed when the driver has this callback function.

If the card is present, 1 will return, if the card is absent, 0 will return.
If the controller will not support this feature, -ENOSYS will return.

Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
CC: Anton Vorontsov <cbouatmailru@gmail.com>
CC: Chris Ball <cjb@laptop.org>
---
changes for v2:
        - when controller don't support get_cd, return -ENOSYS
        - add the CC
changes for v3:
        - enalbe the controller clock in platform, instead of core
changes for v4:
	- move the detect code to core.c according to the new structure

 drivers/mmc/core/core.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 9c162cd..6412355 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2073,7 +2073,7 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
 
 int _mmc_detect_card_removed(struct mmc_host *host)
 {
-	int ret;
+	int ret = -ENOSYS;
 
 	if ((host->caps & MMC_CAP_NONREMOVABLE) || !host->bus_ops->alive)
 		return 0;
@@ -2081,7 +2081,13 @@ int _mmc_detect_card_removed(struct mmc_host *host)
 	if (!host->card || mmc_card_removed(host->card))
 		return 1;
 
-	ret = host->bus_ops->alive(host);
+	if (host->ops->get_cd) {
+		ret = host->ops->get_cd(host);
+		if (ret >= 0)
+			ret = !ret;
+	}
+	if (ret < 0)
+		ret = host->bus_ops->alive(host);
 	if (ret) {
 		mmc_card_set_removed(host->card);
 		pr_debug("%s: card remove detected\n", mmc_hostname(host));
-- 
1.7.9.5

[PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[r66093]

From: Jerry Huang <Chang-Ming.Huang@freescale.com>

Add callback function sdhci_get_cd to detect the card.
And one new callback added to implement the card detect in sdhci struncture.
If special platform has the card detect callback, it will return the card
state, the value zero is for absent cardi and one is for present card.
If the controller don't support card detect, sdhci_get_cd will return -ENOSYS.

Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
CC: Anton Vorontsov <cbouatmailru@gmail.com>
CC: Chris Ball <cjb@laptop.org>
---
changes for v2:
	- when controller don't support get_cd, return -ENOSYS
	- add new callback for sdhci to detect the card
	- add the CC
changes for v3:
	- use pin_lock only when get_cd defined
changes for v4:
	- enalbe the controller clock in platform, instead of core
changes for v5:
	- remove the copyright

 drivers/mmc/host/sdhci.c |   21 +++++++++++++++++++++
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 23 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f05a377..cc9c8a6 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1573,6 +1573,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
 	return ret;
 }
 
+/* Return values for the sdjco_get_cd callback:
+ *   0 for a absent card
+ *   1 for a present card
+ *   -ENOSYS when not supported (equal to NULL callback)
+ */
+static int sdhci_get_cd(struct mmc_host *mmc)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	unsigned long flags;
+	int present = -ENOSYS;
+
+	if (host->ops->get_cd) {
+		spin_lock_irqsave(&host->lock, flags);
+		present = host->ops->get_cd(host);
+		spin_unlock_irqrestore(&host->lock, flags);
+	}
+
+	return present;
+}
+
 static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
 {
 	if (host->flags & SDHCI_DEVICE_DEAD)
@@ -1995,6 +2015,7 @@ static const struct mmc_host_ops sdhci_ops = {
 	.request	= sdhci_request,
 	.set_ios	= sdhci_set_ios,
 	.get_ro		= sdhci_get_ro,
+	.get_cd		= sdhci_get_cd,
 	.hw_reset	= sdhci_hw_reset,
 	.enable_sdio_irq = sdhci_enable_sdio_irq,
 	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 71a4a7e..7c8f08d 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -69,6 +69,7 @@
 #define  SDHCI_SPACE_AVAILABLE	0x00000400
 #define  SDHCI_DATA_AVAILABLE	0x00000800
 #define  SDHCI_CARD_PRESENT	0x00010000
+#define  SDHCI_CARD_CDPL	0x00040000
 #define  SDHCI_WRITE_PROTECT	0x00080000
 #define  SDHCI_DATA_LVL_MASK	0x00F00000
 #define   SDHCI_DATA_LVL_SHIFT	20
@@ -263,6 +264,7 @@ struct sdhci_ops {
 
 	void	(*set_clock)(struct sdhci_host *host, unsigned int clock);
 
+	int		(*get_cd)(struct sdhci_host *host);
 	int		(*enable_dma)(struct sdhci_host *host);
 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
 	unsigned int	(*get_min_clock)(struct sdhci_host *host);
-- 
1.7.9.5

[PATCH 4/4 v4] ESDHC: add callback esdhc_of_get_cd to detect card

[r66093]

From: Jerry Huang <Chang-Ming.Huang@freescale.com>

In order to check if the card is present, we will read the PRESENT STATE
register and check the bit13(Card detect pin level) and bit15(CINS).

Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
CC: Anton Vorontsov <cbouatmailru@gmail.com>
CC: Chris Ball <cjb@laptop.org>
---
changes for v2:
	- add new callback for esdhc to detect the card state
	- add the CC
changes for v3:
	- enable the controller clock when detect the card state, not core
changes for v4:
	- based on the latest version

 drivers/mmc/host/sdhci-of-esdhc.c |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 63d219f..7053218 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -179,6 +179,28 @@ static void esdhc_of_platform_init(struct sdhci_host *host)
 		host->quirks2 |= SDHCI_QUIRK2_HOST_NO_CMD23;
 }
 
+/* Return: none zero - the card is presetn; 0 - card is absent */
+static int esdhc_of_get_cd(struct sdhci_host *host)
+{
+	int present;
+
+	if (host->flags & SDHCI_DEVICE_DEAD)
+		present = 0;
+	else {
+		int sysctl = sdhci_be32bs_readl(host, SDHCI_CLOCK_CONTROL);
+		/* Enable the controller clock to update the present state */
+		sdhci_be32bs_writel(host, sysctl | SDHCI_CLOCK_INT_EN,
+				SDHCI_CLOCK_CONTROL);
+		/* Detect the card present or absent */
+		present = sdhci_be32bs_readl(host, SDHCI_PRESENT_STATE);
+		present &= (SDHCI_CARD_PRESENT | SDHCI_CARD_CDPL);
+		/* Resve the previous to System control register */
+		sdhci_be32bs_writel(host, sysctl, SDHCI_CLOCK_CONTROL);
+	}
+
+	return present;
+}
+
 static struct sdhci_ops sdhci_esdhc_ops = {
 	.read_l = esdhc_readl,
 	.read_w = esdhc_readw,
@@ -191,6 +213,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
 	.get_max_clock = esdhc_of_get_max_clock,
 	.get_min_clock = esdhc_of_get_min_clock,
 	.platform_init = esdhc_of_platform_init,
+	.get_cd = esdhc_of_get_cd,
 #ifdef CONFIG_PM
 	.platform_suspend = esdhc_of_suspend,
 	.platform_resume = esdhc_of_resume,
-- 
1.7.9.5

RE: [PATCH 4/4 v4] ESDHC: add callback esdhc_of_get_cd to detect card

[Huang Changming-R66093]

Hi, Anton,
No any feedback since posted.
Could you give some comment about it?

Best Regards
Jerry Huang


> -----Original Message-----
> From: Huang Changming-R66093
> Sent: Tuesday, October 30, 2012 4:13 PM
> To: linux-mmc@vger.kernel.org
> Cc: Huang Changming-R66093; Anton Vorontsov; Chris Ball
> Subject: [PATCH 4/4 v4] ESDHC: add callback esdhc_of_get_cd to detect
> card
> 
> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> 
> In order to check if the card is present, we will read the PRESENT STATE
> register and check the bit13(Card detect pin level) and bit15(CINS).
> 
> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> CC: Anton Vorontsov <cbouatmailru@gmail.com>
> CC: Chris Ball <cjb@laptop.org>
> ---
> changes for v2:
> 	- add new callback for esdhc to detect the card state
> 	- add the CC
> changes for v3:
> 	- enable the controller clock when detect the card state, not core
> changes for v4:
> 	- based on the latest version
> 
>  drivers/mmc/host/sdhci-of-esdhc.c |   23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-
> of-esdhc.c
> index 63d219f..7053218 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -179,6 +179,28 @@ static void esdhc_of_platform_init(struct sdhci_host
> *host)
>  		host->quirks2 |= SDHCI_QUIRK2_HOST_NO_CMD23;
>  }
> 
> +/* Return: none zero - the card is presetn; 0 - card is absent */
> +static int esdhc_of_get_cd(struct sdhci_host *host)
> +{
> +	int present;
> +
> +	if (host->flags & SDHCI_DEVICE_DEAD)
> +		present = 0;
> +	else {
> +		int sysctl = sdhci_be32bs_readl(host, SDHCI_CLOCK_CONTROL);
> +		/* Enable the controller clock to update the present state */
> +		sdhci_be32bs_writel(host, sysctl | SDHCI_CLOCK_INT_EN,
> +				SDHCI_CLOCK_CONTROL);
> +		/* Detect the card present or absent */
> +		present = sdhci_be32bs_readl(host, SDHCI_PRESENT_STATE);
> +		present &= (SDHCI_CARD_PRESENT | SDHCI_CARD_CDPL);
> +		/* Resve the previous to System control register */
> +		sdhci_be32bs_writel(host, sysctl, SDHCI_CLOCK_CONTROL);
> +	}
> +
> +	return present;
> +}
> +
>  static struct sdhci_ops sdhci_esdhc_ops = {
>  	.read_l = esdhc_readl,
>  	.read_w = esdhc_readw,
> @@ -191,6 +213,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
>  	.get_max_clock = esdhc_of_get_max_clock,
>  	.get_min_clock = esdhc_of_get_min_clock,
>  	.platform_init = esdhc_of_platform_init,
> +	.get_cd = esdhc_of_get_cd,
>  #ifdef CONFIG_PM
>  	.platform_suspend = esdhc_of_suspend,
>  	.platform_resume = esdhc_of_resume,
> --
> 1.7.9.5

Re: [PATCH 4/4 v4] ESDHC: add callback esdhc_of_get_cd to detect card

[Anton Vorontsov]

On Mon, Nov 19, 2012 at 02:50:49AM +0000, Huang Changming-R66093 wrote:
[...]
> > --- a/drivers/mmc/host/sdhci-of-esdhc.c
> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> > @@ -179,6 +179,28 @@ static void esdhc_of_platform_init(struct sdhci_host
> > *host)
> >  		host->quirks2 |= SDHCI_QUIRK2_HOST_NO_CMD23;
> >  }
> > 
> > +/* Return: none zero - the card is presetn; 0 - card is absent */
> > +static int esdhc_of_get_cd(struct sdhci_host *host)
> > +{
> > +	int present;
> > +
> > +	if (host->flags & SDHCI_DEVICE_DEAD)
> > +		present = 0;

return 0;

> > +	else {

then you don't need the 'else', which reduces indentation level.

> > +		int sysctl = sdhci_be32bs_readl(host, SDHCI_CLOCK_CONTROL);

empty line is needed here.

> > +		/* Enable the controller clock to update the present state */
> > +		sdhci_be32bs_writel(host, sysctl | SDHCI_CLOCK_INT_EN,
> > +				SDHCI_CLOCK_CONTROL);

empty line for better readability.

> > +		/* Detect the card present or absent */
> > +		present = sdhci_be32bs_readl(host, SDHCI_PRESENT_STATE);
> > +		present &= (SDHCI_CARD_PRESENT | SDHCI_CARD_CDPL);

ditto here.

> > +		/* Resve the previous to System control register */
> > +		sdhci_be32bs_writel(host, sysctl, SDHCI_CLOCK_CONTROL);
> > +	}
> > +
> > +	return present;
> > +}
> > +
> >  static struct sdhci_ops sdhci_esdhc_ops = {
> >  	.read_l = esdhc_readl,
> >  	.read_w = esdhc_readw,
> > @@ -191,6 +213,7 @@ static struct sdhci_ops sdhci_esdhc_ops = {
> >  	.get_max_clock = esdhc_of_get_max_clock,
> >  	.get_min_clock = esdhc_of_get_min_clock,
> >  	.platform_init = esdhc_of_platform_init,
> > +	.get_cd = esdhc_of_get_cd,
> >  #ifdef CONFIG_PM
> >  	.platform_suspend = esdhc_of_suspend,
> >  	.platform_resume = esdhc_of_resume,
> > --
> > 1.7.9.5

Otherwise, it looks good to me.

Reviewed-by: Anton Vorontsov <cbouatmailru@gmail.com>

RE: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Huang Changming-R66093]

Anton,
No any feedback since posted.
Could you give some comment about it?

Best Regards
Jerry Huang


> -----Original Message-----
> From: Huang Changming-R66093
> Sent: Tuesday, October 30, 2012 4:13 PM
> To: linux-mmc@vger.kernel.org
> Cc: Huang Changming-R66093; Anton Vorontsov; Chris Ball
> Subject: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the
> card
> 
> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> 
> Add callback function sdhci_get_cd to detect the card.
> And one new callback added to implement the card detect in sdhci
> struncture.
> If special platform has the card detect callback, it will return the card
> state, the value zero is for absent cardi and one is for present card.
> If the controller don't support card detect, sdhci_get_cd will return -
> ENOSYS.
> 
> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> CC: Anton Vorontsov <cbouatmailru@gmail.com>
> CC: Chris Ball <cjb@laptop.org>
> ---
> changes for v2:
> 	- when controller don't support get_cd, return -ENOSYS
> 	- add new callback for sdhci to detect the card
> 	- add the CC
> changes for v3:
> 	- use pin_lock only when get_cd defined changes for v4:
> 	- enalbe the controller clock in platform, instead of core changes
> for v5:
> 	- remove the copyright
> 
>  drivers/mmc/host/sdhci.c |   21 +++++++++++++++++++++
>  drivers/mmc/host/sdhci.h |    2 ++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index
> f05a377..cc9c8a6 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1573,6 +1573,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
>  	return ret;
>  }
> 
> +/* Return values for the sdjco_get_cd callback:
> + *   0 for a absent card
> + *   1 for a present card
> + *   -ENOSYS when not supported (equal to NULL callback)
> + */
> +static int sdhci_get_cd(struct mmc_host *mmc) {
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	unsigned long flags;
> +	int present = -ENOSYS;
> +
> +	if (host->ops->get_cd) {
> +		spin_lock_irqsave(&host->lock, flags);
> +		present = host->ops->get_cd(host);
> +		spin_unlock_irqrestore(&host->lock, flags);
> +	}
> +
> +	return present;
> +}
> +
>  static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int
> enable)  {
>  	if (host->flags & SDHCI_DEVICE_DEAD)
> @@ -1995,6 +2015,7 @@ static const struct mmc_host_ops sdhci_ops = {
>  	.request	= sdhci_request,
>  	.set_ios	= sdhci_set_ios,
>  	.get_ro		= sdhci_get_ro,
> +	.get_cd		= sdhci_get_cd,
>  	.hw_reset	= sdhci_hw_reset,
>  	.enable_sdio_irq = sdhci_enable_sdio_irq,
>  	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index
> 71a4a7e..7c8f08d 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -69,6 +69,7 @@
>  #define  SDHCI_SPACE_AVAILABLE	0x00000400
>  #define  SDHCI_DATA_AVAILABLE	0x00000800
>  #define  SDHCI_CARD_PRESENT	0x00010000
> +#define  SDHCI_CARD_CDPL	0x00040000
>  #define  SDHCI_WRITE_PROTECT	0x00080000
>  #define  SDHCI_DATA_LVL_MASK	0x00F00000
>  #define   SDHCI_DATA_LVL_SHIFT	20
> @@ -263,6 +264,7 @@ struct sdhci_ops {
> 
>  	void	(*set_clock)(struct sdhci_host *host, unsigned int clock);
> 
> +	int		(*get_cd)(struct sdhci_host *host);
>  	int		(*enable_dma)(struct sdhci_host *host);
>  	unsigned int	(*get_max_clock)(struct sdhci_host *host);
>  	unsigned int	(*get_min_clock)(struct sdhci_host *host);
> --
> 1.7.9.5

Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Anton Vorontsov]

On Mon, Nov 19, 2012 at 02:50:18AM +0000, Huang Changming-R66093 wrote:
[...]
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index
> > f05a377..cc9c8a6 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1573,6 +1573,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
> >  	return ret;
> >  }
> > 
> > +/* Return values for the sdjco_get_cd callback:
> > + *   0 for a absent card
> > + *   1 for a present card
> > + *   -ENOSYS when not supported (equal to NULL callback)
> > + */

Incorrect style.

> > +static int sdhci_get_cd(struct mmc_host *mmc) {

ditto

> > +	struct sdhci_host *host = mmc_priv(mmc);
> > +	unsigned long flags;
> > +	int present = -ENOSYS;

	if (!host->ops->get_cd)
		return -ENOSYS;

	...
	...

Otherwise, it looks good.

Reviewed-by: Anton Vorontsov <cbouatmailru@gmail.com>

RE: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Huang Changming-R66093]

> -----Original Message-----
> From: Anton Vorontsov [mailto:cbouatmailru@gmail.com]
> Sent: Monday, November 19, 2012 10:58 AM
> To: Huang Changming-R66093
> Cc: Chris Ball; linux-mmc@vger.kernel.org
> Subject: Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect
> the card
> 
> On Mon, Nov 19, 2012 at 02:50:18AM +0000, Huang Changming-R66093 wrote:
> [...]
> > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index
> > > f05a377..cc9c8a6 100644
> > > --- a/drivers/mmc/host/sdhci.c
> > > +++ b/drivers/mmc/host/sdhci.c
> > > @@ -1573,6 +1573,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
> > >  	return ret;
> > >  }
> > >
> > > +/* Return values for the sdjco_get_cd callback:
> > > + *   0 for a absent card
> > > + *   1 for a present card
> > > + *   -ENOSYS when not supported (equal to NULL callback)
> > > + */
> 
> Incorrect style.
Hi, Anton, you mean the comment style is incrorrect?
It should be:
/*
 * xxxxx
 * xxxxxx
 */

> > > +static int sdhci_get_cd(struct mmc_host *mmc) {
> 
> ditto
I don't see the issue, could you explain it?

> 
> > > +	struct sdhci_host *host = mmc_priv(mmc);
> > > +	unsigned long flags;
> > > +	int present = -ENOSYS;
> 
> 	if (!host->ops->get_cd)
> 		return -ENOSYS;
> 
> 	...
> 	...
> 
> Otherwise, it looks good.
> 
> Reviewed-by: Anton Vorontsov <cbouatmailru@gmail.com>

Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Anton Vorontsov]

On Mon, Nov 19, 2012 at 03:15:53AM +0000, Huang Changming-R66093 wrote:
[...]
> > > > @@ -1573,6 +1573,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
> > > >  	return ret;
> > > >  }
> > > >
> > > > +/* Return values for the sdjco_get_cd callback:
> > > > + *   0 for a absent card
> > > > + *   1 for a present card
> > > > + *   -ENOSYS when not supported (equal to NULL callback)
> > > > + */
> > 
> > Incorrect style.
> Hi, Anton, you mean the comment style is incrorrect?
> It should be:
> /*
>  * xxxxx
>  * xxxxxx
>  */

Yup.

> 
> > > > +static int sdhci_get_cd(struct mmc_host *mmc) {
> > 
> > ditto
> I don't see the issue, could you explain it?

The issue is in the brace placement. It should be

static int sdhci_get_cd(struct mmc_host *mmc)
{
	...
}

Please refer to Documentation/CodingStyle -- it's a great document.

Thanks,
Anton.

RE: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Huang Changming-R66093]

Best Regards
Jerry Huang


> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Anton Vorontsov
> Sent: Monday, November 19, 2012 11:31 AM
> To: Huang Changming-R66093
> Cc: Chris Ball; linux-mmc@vger.kernel.org
> Subject: Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect
> the card
> 
> On Mon, Nov 19, 2012 at 03:15:53AM +0000, Huang Changming-R66093 wrote:
> [...]
> > > > > @@ -1573,6 +1573,26 @@ static int sdhci_get_ro(struct mmc_host
> *mmc)
> > > > >  	return ret;
> > > > >  }
> > > > >
> > > > > +/* Return values for the sdjco_get_cd callback:
> > > > > + *   0 for a absent card
> > > > > + *   1 for a present card
> > > > > + *   -ENOSYS when not supported (equal to NULL callback)
> > > > > + */
> > >
> > > Incorrect style.
> > Hi, Anton, you mean the comment style is incrorrect?
> > It should be:
> > /*
> >  * xxxxx
> >  * xxxxxx
> >  */
> 
> Yup.

Thanks, Anton, I see.

> >
> > > > > +static int sdhci_get_cd(struct mmc_host *mmc) {
> > >
> > > ditto
> > I don't see the issue, could you explain it?
> 
> The issue is in the brace placement. It should be
> 
> static int sdhci_get_cd(struct mmc_host *mmc)
> {
> 	...
> }
> 
> Please refer to Documentation/CodingStyle -- it's a great document.
> 
Yes, my first post is that you say,
Maybe the extra lines breaks has been removed in you message, so you see the incorrect style.

Anyway, Thank you very much.
I will post new version with your Reviewed.

Re: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card

[Girish K S]

On 30 October 2012 13:42,  <r66093@freescale.com> wrote:
> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
>
> In order to check whether the card has been removed, the function
> mmc_send_status() will send command CMD13 to card and ask the card
> to send its status register to sdhc driver, which will generate
> many interrupts repeatedly and make the system performance bad.
>
> Therefore, add callback function get_cd() to check whether
> the card has been removed when the driver has this callback function.
>
> If the card is present, 1 will return, if the card is absent, 0 will return.
> If the controller will not support this feature, -ENOSYS will return.
>
> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> CC: Anton Vorontsov <cbouatmailru@gmail.com>
> CC: Chris Ball <cjb@laptop.org>
> ---
> changes for v2:
>         - when controller don't support get_cd, return -ENOSYS
>         - add the CC
> changes for v3:
>         - enalbe the controller clock in platform, instead of core
> changes for v4:
>         - move the detect code to core.c according to the new structure
>
>  drivers/mmc/core/core.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 9c162cd..6412355 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2073,7 +2073,7 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
>
>  int _mmc_detect_card_removed(struct mmc_host *host)
>  {
> -       int ret;
> +       int ret = -ENOSYS;
>
>         if ((host->caps & MMC_CAP_NONREMOVABLE) || !host->bus_ops->alive)
>                 return 0;
> @@ -2081,7 +2081,13 @@ int _mmc_detect_card_removed(struct mmc_host *host)
>         if (!host->card || mmc_card_removed(host->card))
>                 return 1;
>
> -       ret = host->bus_ops->alive(host);
> +       if (host->ops->get_cd) {
> +               ret = host->ops->get_cd(host);
> +               if (ret >= 0)
> +                       ret = !ret;
> +       }
> +       if (ret < 0)
> +               ret = host->bus_ops->alive(host);
why should we check for negative here? can move this code into else
path of   if (host->ops->get_cd).
>         if (ret) {
>                 mmc_card_set_removed(host->card);
>                 pr_debug("%s: card remove detected\n", mmc_hostname(host));
> --
> 1.7.9.5
>
>
> --
> 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 2/4 v4] MMC/SD: Add callback function to detect card

[Huang Changming-R66093]

Best Regards
Jerry Huang


> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Girish K S
> Sent: Tuesday, October 30, 2012 7:35 PM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Huang Changming-R66093; Anton Vorontsov;
> Chris Ball
> Subject: Re: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card
> 
> On 30 October 2012 13:42,  <r66093@freescale.com> wrote:
> > From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> >
> > In order to check whether the card has been removed, the function
> > mmc_send_status() will send command CMD13 to card and ask the card
> > to send its status register to sdhc driver, which will generate
> > many interrupts repeatedly and make the system performance bad.
> >
> > Therefore, add callback function get_cd() to check whether
> > the card has been removed when the driver has this callback function.
> >
> > If the card is present, 1 will return, if the card is absent, 0 will
> return.
> > If the controller will not support this feature, -ENOSYS will return.
> >
> > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > CC: Anton Vorontsov <cbouatmailru@gmail.com>
> > CC: Chris Ball <cjb@laptop.org>
> > ---
> > changes for v2:
> >         - when controller don't support get_cd, return -ENOSYS
> >         - add the CC
> > changes for v3:
> >         - enalbe the controller clock in platform, instead of core
> > changes for v4:
> >         - move the detect code to core.c according to the new structure
> >
> >  drivers/mmc/core/core.c |   10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 9c162cd..6412355 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2073,7 +2073,7 @@ static int mmc_rescan_try_freq(struct mmc_host
> *host, unsigned freq)
> >
> >  int _mmc_detect_card_removed(struct mmc_host *host)
> >  {
> > -       int ret;
> > +       int ret = -ENOSYS;
> >
> >         if ((host->caps & MMC_CAP_NONREMOVABLE) || !host->bus_ops-
> >alive)
> >                 return 0;
> > @@ -2081,7 +2081,13 @@ int _mmc_detect_card_removed(struct mmc_host
> *host)
> >         if (!host->card || mmc_card_removed(host->card))
> >                 return 1;
> >
> > -       ret = host->bus_ops->alive(host);
> > +       if (host->ops->get_cd) {
> > +               ret = host->ops->get_cd(host);
> > +               if (ret >= 0)
> > +                       ret = !ret;
> > +       }
> > +       if (ret < 0)
> > +               ret = host->bus_ops->alive(host);
> why should we check for negative here? can move this code into else
> path of   if (host->ops->get_cd).
> >         if (ret) {

I did this comment:
If the card is present, 1 will return, if the card is absent, 0 will return.
If the controller will not support this feature, -ENOSYS will return.

Can't move to other address, because these codes check the card present state.

Re: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card

[Jaehoon Chung]

Hi,

>> *host)
>>>         if (!host->card || mmc_card_removed(host->card))
>>>                 return 1;
>>>
>>> -       ret = host->bus_ops->alive(host);
>>> +       if (host->ops->get_cd) {
>>> +               ret = host->ops->get_cd(host);
>>> +               if (ret >= 0)
>>> +                       ret = !ret;
>>> +       }
>>> +       if (ret < 0)
>>> +               ret = host->bus_ops->alive(host);
>> why should we check for negative here? can move this code into else
>> path of   if (host->ops->get_cd).
>>>         if (ret) {
> 
> I did this comment:
> If the card is present, 1 will return, if the card is absent, 0 will return.
> If the controller will not support this feature, -ENOSYS will return.
If checked whether card is absent or not with get_cd(), need to check the host->bus_ops->alive?
> 
> Can't move to other address, because these codes check the card present state.
> 
> 
> --
> 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 2/4 v4] MMC/SD: Add callback function to detect card

[Huang Changming-R66093]

Best Regards
Jerry Huang


> -----Original Message-----
> From: Jaehoon Chung [mailto:jh80.chung@samsung.com]
> Sent: Wednesday, October 31, 2012 12:29 PM
> To: Huang Changming-R66093
> Cc: Girish K S; linux-mmc@vger.kernel.org; Anton Vorontsov; Chris Ball
> Subject: Re: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card
> 
> Hi,
> 
> >> *host)
> >>>         if (!host->card || mmc_card_removed(host->card))
> >>>                 return 1;
> >>>
> >>> -       ret = host->bus_ops->alive(host);
> >>> +       if (host->ops->get_cd) {
> >>> +               ret = host->ops->get_cd(host);
> >>> +               if (ret >= 0)
> >>> +                       ret = !ret;
> >>> +       }
> >>> +       if (ret < 0)
> >>> +               ret = host->bus_ops->alive(host);
> >> why should we check for negative here? can move this code into else
> >> path of   if (host->ops->get_cd).
> >>>         if (ret) {
> >
> > I did this comment:
> > If the card is present, 1 will return, if the card is absent, 0 will
> return.
> > If the controller will not support this feature, -ENOSYS will return.
> If checked whether card is absent or not with get_cd(), need to check the
> host->bus_ops->alive?

When get_cd is not supported, need to check it.

Re: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card

[Johan Rudholm]

Hi Jerry,

2012/10/30  <r66093@freescale.com>:
> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
>
> In order to check whether the card has been removed, the function
> mmc_send_status() will send command CMD13 to card and ask the card
> to send its status register to sdhc driver, which will generate
> many interrupts repeatedly and make the system performance bad.
>
> Therefore, add callback function get_cd() to check whether
> the card has been removed when the driver has this callback function.
>
> If the card is present, 1 will return, if the card is absent, 0 will return.
> If the controller will not support this feature, -ENOSYS will return.

In what cases is the performance affected by this? I believe this
function is called only on some errors and on detect?

Also, we've seen cases where the card detect GPIO cannot be trusted to
tell wether the card is present or not, for instance in the corner
case when an SD-card is removed very slowly...

Kind regards, Johan

RE: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card

[Huang Changming-R66093]

Hi, Johan,
When quicks SDHCI_QUIRK_BROKEN_CARD_DETECTION is set, the driver will poll the card status with the command CMD13 periodically, then many interrupts will be generated, which affect the performance.

Yes, some cases to detect GPIO can't be trusted, so I only just implement this callback in eSDHC-of driver. that is to say, just when the platform support it, this callback can be implement, if not, continue to send the command CMD13.  

> Hi Jerry,
> 
> 2012/10/30  <r66093@freescale.com>:
> > From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> >
> > In order to check whether the card has been removed, the function
> > mmc_send_status() will send command CMD13 to card and ask the card to
> > send its status register to sdhc driver, which will generate many
> > interrupts repeatedly and make the system performance bad.
> >
> > Therefore, add callback function get_cd() to check whether the card
> > has been removed when the driver has this callback function.
> >
> > If the card is present, 1 will return, if the card is absent, 0 will
> return.
> > If the controller will not support this feature, -ENOSYS will return.
> 
> In what cases is the performance affected by this? I believe this
> function is called only on some errors and on detect?
> 
> Also, we've seen cases where the card detect GPIO cannot be trusted to
> tell wether the card is present or not, for instance in the corner case
> when an SD-card is removed very slowly...
> 
> Kind regards, Johan

Re: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card

[Johan Rudholm]

Hi Jerry,

2012/11/2 Huang Changming-R66093 <r66093@freescale.com>:
> Hi, Johan,
> When quicks SDHCI_QUIRK_BROKEN_CARD_DETECTION is set, the driver will poll the card status with the command CMD13 periodically, then many interrupts will be generated, which affect the performance.

Ok, just to see if I understand the scenario correctly:
SDHCI_QUIRK_BROKEN_CARD_DETECTION causes the MMC_CAP_NEEDS_POLL cap to
be set, which will cause mmc_rescan to be run at a one second
interval. mmc_rescan calls bus_ops->detect (mmc_sd_detect) and this in
turn calls _mmc_detect_card_removed, which will do the bus_ops->alive
and send CMD13. So in this case, one CMD13 is sent per second, right?
Is this the cause of the performance issue?

A thought: if the host hardware does have a GPIO card detect pin
hooked up, would it not be possible to make this pin generate an IRQ
whenever a card is inserted or removed? This is what we do in the MMCI
driver, for instance (mmci_cd_irq).

> Yes, some cases to detect GPIO can't be trusted, so I only just implement this callback in eSDHC-of driver. that is to say, just when the platform support it, this callback can be implement, if not, continue to send the command CMD13.

I'm just wondering how this will affect our system, where we use the
cd GPIO to generate detect jobs on card insert/remove, but where the
cd pin is not 100% synchronized with the electrical connection of the
power and cmd line of the SD-card. So if I remember correctly, the cd
pin may report that the card has been removed, but there is still an
electric connection and the card is alive...

Kind regards, Johan

RE: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card

[Huang Changming-R66093]

> 
> 2012/11/2 Huang Changming-R66093 <r66093@freescale.com>:
> > Hi, Johan,
> > When quicks SDHCI_QUIRK_BROKEN_CARD_DETECTION is set, the driver will
> poll the card status with the command CMD13 periodically, then many
> interrupts will be generated, which affect the performance.
> 
> Ok, just to see if I understand the scenario correctly:
> SDHCI_QUIRK_BROKEN_CARD_DETECTION causes the MMC_CAP_NEEDS_POLL cap to be
> set, which will cause mmc_rescan to be run at a one second interval.
> mmc_rescan calls bus_ops->detect (mmc_sd_detect) and this in turn calls
> _mmc_detect_card_removed, which will do the bus_ops->alive and send CMD13.
> So in this case, one CMD13 is sent per second, right?
> Is this the cause of the performance issue?

Yes, You are right.

> 
> A thought: if the host hardware does have a GPIO card detect pin hooked
> up, would it not be possible to make this pin generate an IRQ whenever a
> card is inserted or removed? This is what we do in the MMCI driver, for
> instance (mmci_cd_irq).

Our silicones has this pin to do card detect, but some boards don't generate the interrupt when card is inserted or removed. So I have to use the poll mode.

> > Yes, some cases to detect GPIO can't be trusted, so I only just
> implement this callback in eSDHC-of driver. that is to say, just when the
> platform support it, this callback can be implement, if not, continue to
> send the command CMD13.
> 
> I'm just wondering how this will affect our system, where we use the cd
> GPIO to generate detect jobs on card insert/remove, but where the cd pin
> is not 100% synchronized with the electrical connection of the power and
> cmd line of the SD-card. So if I remember correctly, the cd pin may
> report that the card has been removed, but there is still an electric
> connection and the card is alive...
> 
I don't see this on our board, when card is inserted or removed, the register field can reflect this state correctly.

Re: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card

[Johan Rudholm]

Hi,

2012/11/5 Huang Changming-R66093 <r66093@freescale.com>:
>>
>> 2012/11/2 Huang Changming-R66093 <r66093@freescale.com>:
>> > Hi, Johan,
>> > When quicks SDHCI_QUIRK_BROKEN_CARD_DETECTION is set, the driver will
>> poll the card status with the command CMD13 periodically, then many
>> interrupts will be generated, which affect the performance.
>>
>> Ok, just to see if I understand the scenario correctly:
>> SDHCI_QUIRK_BROKEN_CARD_DETECTION causes the MMC_CAP_NEEDS_POLL cap to be
>> set, which will cause mmc_rescan to be run at a one second interval.
>> mmc_rescan calls bus_ops->detect (mmc_sd_detect) and this in turn calls
>> _mmc_detect_card_removed, which will do the bus_ops->alive and send CMD13.
>> So in this case, one CMD13 is sent per second, right?
>> Is this the cause of the performance issue?
>
> Yes, You are right.

Ok, it's a bit hard to understand how this can lead to a noticeable
performance impact, but OK. :)

>> A thought: if the host hardware does have a GPIO card detect pin hooked
>> up, would it not be possible to make this pin generate an IRQ whenever a
>> card is inserted or removed? This is what we do in the MMCI driver, for
>> instance (mmci_cd_irq).
>
> Our silicones has this pin to do card detect, but some boards don't generate the interrupt when card is inserted or removed. So I have to use the poll mode.
>
>> > Yes, some cases to detect GPIO can't be trusted, so I only just
>> implement this callback in eSDHC-of driver. that is to say, just when the
>> platform support it, this callback can be implement, if not, continue to
>> send the command CMD13.
>>
>> I'm just wondering how this will affect our system, where we use the cd
>> GPIO to generate detect jobs on card insert/remove, but where the cd pin
>> is not 100% synchronized with the electrical connection of the power and
>> cmd line of the SD-card. So if I remember correctly, the cd pin may
>> report that the card has been removed, but there is still an electric
>> connection and the card is alive...
>>
> I don't see this on our board, when card is inserted or removed, the register field can reflect this state correctly.

We see a difference on our boards with this patch, but no functional
change (actually a removed card is detected earlier now), so from my
perspective:

Reviewed-By: Johan Rudholm <johan.rudholm@stericsson.com>

//Johan

RE: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card

[Huang Changming-R66093]

Thanks a lot, Johan.

Best Regards
Jerry Huang


> -----Original Message-----
> From: Johan Rudholm [mailto:jrudholm@gmail.com]
> Sent: Monday, November 05, 2012 10:08 PM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Anton Vorontsov; Chris Ball
> Subject: Re: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card
> 
> Hi,
> 
> 2012/11/5 Huang Changming-R66093 <r66093@freescale.com>:
> >>
> >> 2012/11/2 Huang Changming-R66093 <r66093@freescale.com>:
> >> > Hi, Johan,
> >> > When quicks SDHCI_QUIRK_BROKEN_CARD_DETECTION is set, the driver
> will
> >> poll the card status with the command CMD13 periodically, then many
> >> interrupts will be generated, which affect the performance.
> >>
> >> Ok, just to see if I understand the scenario correctly:
> >> SDHCI_QUIRK_BROKEN_CARD_DETECTION causes the MMC_CAP_NEEDS_POLL cap to
> be
> >> set, which will cause mmc_rescan to be run at a one second interval.
> >> mmc_rescan calls bus_ops->detect (mmc_sd_detect) and this in turn
> calls
> >> _mmc_detect_card_removed, which will do the bus_ops->alive and send
> CMD13.
> >> So in this case, one CMD13 is sent per second, right?
> >> Is this the cause of the performance issue?
> >
> > Yes, You are right.
> 
> Ok, it's a bit hard to understand how this can lead to a noticeable
> performance impact, but OK. :)
> 
> >> A thought: if the host hardware does have a GPIO card detect pin
> hooked
> >> up, would it not be possible to make this pin generate an IRQ whenever
> a
> >> card is inserted or removed? This is what we do in the MMCI driver,
> for
> >> instance (mmci_cd_irq).
> >
> > Our silicones has this pin to do card detect, but some boards don't
> generate the interrupt when card is inserted or removed. So I have to use
> the poll mode.
> >
> >> > Yes, some cases to detect GPIO can't be trusted, so I only just
> >> implement this callback in eSDHC-of driver. that is to say, just when
> the
> >> platform support it, this callback can be implement, if not, continue
> to
> >> send the command CMD13.
> >>
> >> I'm just wondering how this will affect our system, where we use the
> cd
> >> GPIO to generate detect jobs on card insert/remove, but where the cd
> pin
> >> is not 100% synchronized with the electrical connection of the power
> and
> >> cmd line of the SD-card. So if I remember correctly, the cd pin may
> >> report that the card has been removed, but there is still an electric
> >> connection and the card is alive...
> >>
> > I don't see this on our board, when card is inserted or removed, the
> register field can reflect this state correctly.
> 
> We see a difference on our boards with this patch, but no functional
> change (actually a removed card is detected earlier now), so from my
> perspective:
> 
> Reviewed-By: Johan Rudholm <johan.rudholm@stericsson.com>
> 
> //Johan

RE: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card

[Huang Changming-R66093]

Hi, Anton and Chris
Have you any comment about the serial patch [patch 2/3/4, please ignore patch1]?
Reviewed-By: Johan Rudholm <johan.rudholm@stericsson.com>

Best Regards
Jerry Huang


> -----Original Message-----
> From: Johan Rudholm [mailto:jrudholm@gmail.com]
> Sent: Monday, November 05, 2012 10:08 PM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Anton Vorontsov; Chris Ball
> Subject: Re: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card
> 
> Hi,
> 
> 2012/11/5 Huang Changming-R66093 <r66093@freescale.com>:
> >>
> >> 2012/11/2 Huang Changming-R66093 <r66093@freescale.com>:
> >> > Hi, Johan,
> >> > When quicks SDHCI_QUIRK_BROKEN_CARD_DETECTION is set, the driver
> will
> >> poll the card status with the command CMD13 periodically, then many
> >> interrupts will be generated, which affect the performance.
> >>
> >> Ok, just to see if I understand the scenario correctly:
> >> SDHCI_QUIRK_BROKEN_CARD_DETECTION causes the MMC_CAP_NEEDS_POLL cap to
> be
> >> set, which will cause mmc_rescan to be run at a one second interval.
> >> mmc_rescan calls bus_ops->detect (mmc_sd_detect) and this in turn
> calls
> >> _mmc_detect_card_removed, which will do the bus_ops->alive and send
> CMD13.
> >> So in this case, one CMD13 is sent per second, right?
> >> Is this the cause of the performance issue?
> >
> > Yes, You are right.
> 
> Ok, it's a bit hard to understand how this can lead to a noticeable
> performance impact, but OK. :)
> 
> >> A thought: if the host hardware does have a GPIO card detect pin
> hooked
> >> up, would it not be possible to make this pin generate an IRQ whenever
> a
> >> card is inserted or removed? This is what we do in the MMCI driver,
> for
> >> instance (mmci_cd_irq).
> >
> > Our silicones has this pin to do card detect, but some boards don't
> generate the interrupt when card is inserted or removed. So I have to use
> the poll mode.
> >
> >> > Yes, some cases to detect GPIO can't be trusted, so I only just
> >> implement this callback in eSDHC-of driver. that is to say, just when
> the
> >> platform support it, this callback can be implement, if not, continue
> to
> >> send the command CMD13.
> >>
> >> I'm just wondering how this will affect our system, where we use the
> cd
> >> GPIO to generate detect jobs on card insert/remove, but where the cd
> pin
> >> is not 100% synchronized with the electrical connection of the power
> and
> >> cmd line of the SD-card. So if I remember correctly, the cd pin may
> >> report that the card has been removed, but there is still an electric
> >> connection and the card is alive...
> >>
> > I don't see this on our board, when card is inserted or removed, the
> register field can reflect this state correctly.
> 
> We see a difference on our boards with this patch, but no functional
> change (actually a removed card is detected earlier now), so from my
> perspective:
> 
> Reviewed-By: Johan Rudholm <johan.rudholm@stericsson.com>
> 
> //Johan

RE: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card

[Huang Changming-R66093]

Hi, Anton and Chris
Have you any comment about this serial patches[patch 2/3/4, except 1]?

Best Regards
Jerry Huang


> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Huang Changming-R66093
> Sent: Tuesday, November 06, 2012 9:56 AM
> To: Anton Vorontsov; Chris Ball
> Cc: linux-mmc@vger.kernel.org; Johan Rudholm
> Subject: RE: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card
> 
> Hi, Anton and Chris
> Have you any comment about the serial patch [patch 2/3/4, please ignore
> patch1]?
> Reviewed-By: Johan Rudholm <johan.rudholm@stericsson.com>
> 
> Best Regards
> Jerry Huang
> 
> 
> > -----Original Message-----
> > From: Johan Rudholm [mailto:jrudholm@gmail.com]
> > Sent: Monday, November 05, 2012 10:08 PM
> > To: Huang Changming-R66093
> > Cc: linux-mmc@vger.kernel.org; Anton Vorontsov; Chris Ball
> > Subject: Re: [PATCH 2/4 v4] MMC/SD: Add callback function to detect
> > card
> >
> > Hi,
> >
> > 2012/11/5 Huang Changming-R66093 <r66093@freescale.com>:
> > >>
> > >> 2012/11/2 Huang Changming-R66093 <r66093@freescale.com>:
> > >> > Hi, Johan,
> > >> > When quicks SDHCI_QUIRK_BROKEN_CARD_DETECTION is set, the driver
> > will
> > >> poll the card status with the command CMD13 periodically, then many
> > >> interrupts will be generated, which affect the performance.
> > >>
> > >> Ok, just to see if I understand the scenario correctly:
> > >> SDHCI_QUIRK_BROKEN_CARD_DETECTION causes the MMC_CAP_NEEDS_POLL cap
> > >> to
> > be
> > >> set, which will cause mmc_rescan to be run at a one second interval.
> > >> mmc_rescan calls bus_ops->detect (mmc_sd_detect) and this in turn
> > calls
> > >> _mmc_detect_card_removed, which will do the bus_ops->alive and send
> > CMD13.
> > >> So in this case, one CMD13 is sent per second, right?
> > >> Is this the cause of the performance issue?
> > >
> > > Yes, You are right.
> >
> > Ok, it's a bit hard to understand how this can lead to a noticeable
> > performance impact, but OK. :)
> >
> > >> A thought: if the host hardware does have a GPIO card detect pin
> > hooked
> > >> up, would it not be possible to make this pin generate an IRQ
> > >> whenever
> > a
> > >> card is inserted or removed? This is what we do in the MMCI driver,
> > for
> > >> instance (mmci_cd_irq).
> > >
> > > Our silicones has this pin to do card detect, but some boards don't
> > generate the interrupt when card is inserted or removed. So I have to
> > use the poll mode.
> > >
> > >> > Yes, some cases to detect GPIO can't be trusted, so I only just
> > >> implement this callback in eSDHC-of driver. that is to say, just
> > >> when
> > the
> > >> platform support it, this callback can be implement, if not,
> > >> continue
> > to
> > >> send the command CMD13.
> > >>
> > >> I'm just wondering how this will affect our system, where we use
> > >> the
> > cd
> > >> GPIO to generate detect jobs on card insert/remove, but where the
> > >> cd
> > pin
> > >> is not 100% synchronized with the electrical connection of the
> > >> power
> > and
> > >> cmd line of the SD-card. So if I remember correctly, the cd pin may
> > >> report that the card has been removed, but there is still an
> > >> electric connection and the card is alive...
> > >>
> > > I don't see this on our board, when card is inserted or removed, the
> > register field can reflect this state correctly.
> >
> > We see a difference on our boards with this patch, but no functional
> > change (actually a removed card is detected earlier now), so from my
> > perspective:
> >
> > Reviewed-By: Johan Rudholm <johan.rudholm@stericsson.com>
> >
> > //Johan
> 
> 
> --
> 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 2/4 v4] MMC/SD: Add callback function to detect card

[Huang Changming-R66093]

Hi, Anton and Chris
This patch has been Reviewed-By: Johan Rudholm <johan.rudholm@stericsson.com> on Nov 05.
Could you give some comment about it?

Best Regards
Jerry Huang

> -----Original Message-----
> From: Johan Rudholm [mailto:jrudholm@gmail.com]
> Sent: Monday, November 05, 2012 10:08 PM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Anton Vorontsov; Chris Ball
> Subject: Re: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card
> 
> Hi,
> 
> 2012/11/5 Huang Changming-R66093 <r66093@freescale.com>:
> >>
> >> 2012/11/2 Huang Changming-R66093 <r66093@freescale.com>:
> >> > Hi, Johan,
> >> > When quicks SDHCI_QUIRK_BROKEN_CARD_DETECTION is set, the driver
> will
> >> poll the card status with the command CMD13 periodically, then many
> >> interrupts will be generated, which affect the performance.
> >>
> >> Ok, just to see if I understand the scenario correctly:
> >> SDHCI_QUIRK_BROKEN_CARD_DETECTION causes the MMC_CAP_NEEDS_POLL cap to
> be
> >> set, which will cause mmc_rescan to be run at a one second interval.
> >> mmc_rescan calls bus_ops->detect (mmc_sd_detect) and this in turn
> calls
> >> _mmc_detect_card_removed, which will do the bus_ops->alive and send
> CMD13.
> >> So in this case, one CMD13 is sent per second, right?
> >> Is this the cause of the performance issue?
> >
> > Yes, You are right.
> 
> Ok, it's a bit hard to understand how this can lead to a noticeable
> performance impact, but OK. :)
> 
> >> A thought: if the host hardware does have a GPIO card detect pin
> hooked
> >> up, would it not be possible to make this pin generate an IRQ whenever
> a
> >> card is inserted or removed? This is what we do in the MMCI driver,
> for
> >> instance (mmci_cd_irq).
> >
> > Our silicones has this pin to do card detect, but some boards don't
> generate the interrupt when card is inserted or removed. So I have to use
> the poll mode.
> >
> >> > Yes, some cases to detect GPIO can't be trusted, so I only just
> >> implement this callback in eSDHC-of driver. that is to say, just when
> the
> >> platform support it, this callback can be implement, if not, continue
> to
> >> send the command CMD13.
> >>
> >> I'm just wondering how this will affect our system, where we use the
> cd
> >> GPIO to generate detect jobs on card insert/remove, but where the cd
> pin
> >> is not 100% synchronized with the electrical connection of the power
> and
> >> cmd line of the SD-card. So if I remember correctly, the cd pin may
> >> report that the card has been removed, but there is still an electric
> >> connection and the card is alive...
> >>
> > I don't see this on our board, when card is inserted or removed, the
> register field can reflect this state correctly.
> 
> We see a difference on our boards with this patch, but no functional
> change (actually a removed card is detected earlier now), so from my
> perspective:
> 
> Reviewed-By: Johan Rudholm <johan.rudholm@stericsson.com>
> 
> //Johan

Re: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card

[Anton Vorontsov]

On Tue, Oct 30, 2012 at 04:12:47PM +0800, r66093@freescale.com wrote:
[..]
> If the card is present, 1 will return, if the card is absent, 0 will return.
> If the controller will not support this feature, -ENOSYS will return.
> 
> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> CC: Anton Vorontsov <cbouatmailru@gmail.com>
> CC: Chris Ball <cjb@laptop.org>
> ---
[...]
>  int _mmc_detect_card_removed(struct mmc_host *host)
>  {
> -	int ret;
> +	int ret = -ENOSYS;
>  
>  	if ((host->caps & MMC_CAP_NONREMOVABLE) || !host->bus_ops->alive)
>  		return 0;
> @@ -2081,7 +2081,13 @@ int _mmc_detect_card_removed(struct mmc_host *host)
>  	if (!host->card || mmc_card_removed(host->card))
>  		return 1;
>  
> -	ret = host->bus_ops->alive(host);
> +	if (host->ops->get_cd) {
> +		ret = host->ops->get_cd(host);
> +		if (ret >= 0)
> +			ret = !ret;

o_O

Oh, I see...

Reviewed-by: Anton Vorontsov <cbouatmailru@gmail.com>

(But I must confess I didn't follow the whole discussion about
get_cd()-via-gpio being unreliable. I'm assuming you fixed this?)

> +	}
> +	if (ret < 0)
> +		ret = host->bus_ops->alive(host);
>  	if (ret) {
>  		mmc_card_set_removed(host->card);
>  		pr_debug("%s: card remove detected\n", mmc_hostname(host));
> -- 
> 1.7.9.5

RE: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card

[Huang Changming-R66093]

> -----Original Message-----
> From: Anton Vorontsov [mailto:cbouatmailru@gmail.com]
> Sent: Monday, November 19, 2012 11:06 AM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Huang Changming-R66093; Chris Ball
> Subject: Re: [PATCH 2/4 v4] MMC/SD: Add callback function to detect card
> 
> On Tue, Oct 30, 2012 at 04:12:47PM +0800, r66093@freescale.com wrote:
> [..]
> > If the card is present, 1 will return, if the card is absent, 0 will
> return.
> > If the controller will not support this feature, -ENOSYS will return.
> >
> > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > CC: Anton Vorontsov <cbouatmailru@gmail.com>
> > CC: Chris Ball <cjb@laptop.org>
> > ---
> [...]
> >  int _mmc_detect_card_removed(struct mmc_host *host)
> >  {
> > -	int ret;
> > +	int ret = -ENOSYS;
> >
> >  	if ((host->caps & MMC_CAP_NONREMOVABLE) || !host->bus_ops->alive)
> >  		return 0;
> > @@ -2081,7 +2081,13 @@ int _mmc_detect_card_removed(struct mmc_host
> *host)
> >  	if (!host->card || mmc_card_removed(host->card))
> >  		return 1;
> >
> > -	ret = host->bus_ops->alive(host);
> > +	if (host->ops->get_cd) {
> > +		ret = host->ops->get_cd(host);
> > +		if (ret >= 0)
> > +			ret = !ret;
> 
> o_O
> 
> Oh, I see...
> 
> Reviewed-by: Anton Vorontsov <cbouatmailru@gmail.com>
> 
> (But I must confess I didn't follow the whole discussion about
> get_cd()-via-gpio being unreliable. I'm assuming you fixed this?)
> 
If the GPIO is unreliable, the related driver may not implement the callback function get_cd.
For eSDHC, the GPIO is reliable, so I do it.

Re: [PATCH 1/4 v3] MMC/core: Add f_min to mmc_power_on()

[Ulf Hansson]

On 30 October 2012 09:12,  <r66093@freescale.com> wrote:
> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
>
> When f_init is zero, the SDHC can't work correctly. So f_min will replace
> f_init, when f_init is zero.
>
> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> CC: Anton Vorontsov <cbouatmailru@gmail.com>
> CC: Chris Ball <cjb@laptop.org>
> ---
> changes for v2:
>         - add the CC
> changes for v3:
>         - enalbe the controller clock in platform, instead of core
>
>  drivers/mmc/core/core.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 06c42cf..9c162cd 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1319,7 +1319,10 @@ static void mmc_power_up(struct mmc_host *host)
>          */
>         mmc_delay(10);
>
> -       host->ios.clock = host->f_init;
> +       if (host->f_init)
> +               host->ios.clock = host->f_init;
> +       else
> +               host->ios.clock = host->f_min;

This should not be needed. host->f_init should never become zero, I believe.

>
>         host->ios.power_mode = MMC_POWER_ON;
>         mmc_set_ios(host);
> --
> 1.7.9.5
>
>
> --
> 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

Kind regards
Ulf Hansson

RE: [PATCH 1/4 v3] MMC/core: Add f_min to mmc_power_on()

[Huang Changming-R66093]

Best Regards
Jerry Huang


> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Wednesday, October 31, 2012 7:09 AM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Huang Changming-R66093; Anton Vorontsov;
> Chris Ball
> Subject: Re: [PATCH 1/4 v3] MMC/core: Add f_min to mmc_power_on()
> 
> On 30 October 2012 09:12,  <r66093@freescale.com> wrote:
> > From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> >
> > When f_init is zero, the SDHC can't work correctly. So f_min will
> replace
> > f_init, when f_init is zero.
> >
> > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > CC: Anton Vorontsov <cbouatmailru@gmail.com>
> > CC: Chris Ball <cjb@laptop.org>
> > ---
> > changes for v2:
> >         - add the CC
> > changes for v3:
> >         - enalbe the controller clock in platform, instead of core
> >
> >  drivers/mmc/core/core.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 06c42cf..9c162cd 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -1319,7 +1319,10 @@ static void mmc_power_up(struct mmc_host *host)
> >          */
> >         mmc_delay(10);
> >
> > -       host->ios.clock = host->f_init;
> > +       if (host->f_init)
> > +               host->ios.clock = host->f_init;
> > +       else
> > +               host->ios.clock = host->f_min;
> 
> This should not be needed. host->f_init should never become zero, I
> believe.
> 
Uh, this patch can be removed.

Re: [PATCH 1/4 v3] MMC/core: Add f_min to mmc_power_on()

[Jaehoon Chung]

On 10/31/2012 08:08 AM, Ulf Hansson wrote:
> On 30 October 2012 09:12,  <r66093@freescale.com> wrote:
>> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
>>
>> When f_init is zero, the SDHC can't work correctly. So f_min will replace
>> f_init, when f_init is zero.
>>
>> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
>> CC: Anton Vorontsov <cbouatmailru@gmail.com>
>> CC: Chris Ball <cjb@laptop.org>
>> ---
>> changes for v2:
>>         - add the CC
>> changes for v3:
>>         - enalbe the controller clock in platform, instead of core
>>
>>  drivers/mmc/core/core.c |    5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 06c42cf..9c162cd 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1319,7 +1319,10 @@ static void mmc_power_up(struct mmc_host *host)
>>          */
>>         mmc_delay(10);
>>
>> -       host->ios.clock = host->f_init;
>> +       if (host->f_init)
>> +               host->ios.clock = host->f_init;
>> +       else
>> +               host->ios.clock = host->f_min;
> 
> This should not be needed. host->f_init should never become zero, I believe.
I agree for Ulf's comment.
> 
>>
>>         host->ios.power_mode = MMC_POWER_ON;
>>         mmc_set_ios(host);
>> --
>> 1.7.9.5
>>
>>
>> --
>> 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
> 
> Kind regards
> Ulf Hansson
> --
> 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
> 

[PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[r66093]

From: Jerry Huang <Chang-Ming.Huang@freescale.com>

Add callback function sdhci_get_cd to detect the card.
And one new callback added to implement the card detect in sdhci struncture.
If special platform has the card detect callback, it will return the card
state, the value zero is for absent cardi and one is for present card.
If the controller don't support card detect, sdhci_get_cd will return -ENOSYS.

Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
CC: Chris Ball <cjb@laptop.org>
---
changes for v2:
	- when controller don't support get_cd, return -ENOSYS
	- add new callback for sdhci to detect the card
	- add the CC
changes for v3:
	- use pin_lock only when get_cd defined
changes for v4:
	- enalbe the controller clock in platform, instead of core
changes for v5:
	- remove the copyright

 drivers/mmc/host/sdhci.c |   21 ++++++++++++++++++++++
 drivers/mmc/host/sdhci.h |    2 ++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6d8eea3..fbe2f46 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1518,6 +1519,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
 	return ret;
 }
 
+/* Return values for the sdjco_get_cd callback:
+ *   0 for a absent card
+ *   1 for a present card
+ *   -ENOSYS when not supported (equal to NULL callback)
+ */
+static int sdhci_get_cd(struct mmc_host *mmc)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	unsigned long flags;
+	int present = -ENOSYS;
+
+	if (host->ops->get_cd) {
+		spin_lock_irqsave(&host->lock, flags);
+		present = host->ops->get_cd(host);
+		spin_unlock_irqrestore(&host->lock, flags);
+	}
+
+	return present;
+}
+
 static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
 {
 	if (host->flags & SDHCI_DEVICE_DEAD)
@@ -1884,6 +1905,7 @@ static const struct mmc_host_ops sdhci_ops = {
 	.request	= sdhci_request,
 	.set_ios	= sdhci_set_ios,
 	.get_ro		= sdhci_get_ro,
+	.get_cd		= sdhci_get_cd,
 	.hw_reset	= sdhci_hw_reset,
 	.enable_sdio_irq = sdhci_enable_sdio_irq,
 	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0a5b654..82f4d27 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -69,6 +69,7 @@
 #define  SDHCI_SPACE_AVAILABLE	0x00000400
 #define  SDHCI_DATA_AVAILABLE	0x00000800
 #define  SDHCI_CARD_PRESENT	0x00010000
+#define  SDHCI_CARD_CDPL	0x00040000
 #define  SDHCI_WRITE_PROTECT	0x00080000
 #define  SDHCI_DATA_LVL_MASK	0x00F00000
 #define   SDHCI_DATA_LVL_SHIFT	20
@@ -261,6 +262,7 @@ struct sdhci_ops {
 
 	void	(*set_clock)(struct sdhci_host *host, unsigned int clock);
 
+	int		(*get_cd)(struct sdhci_host *host);
 	int		(*enable_dma)(struct sdhci_host *host);
 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
 	unsigned int	(*get_min_clock)(struct sdhci_host *host);
-- 
1.7.5.4

Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Philip Rakity]

On Dec 13, 2011, at 6:18 PM, <r66093@freescale.com> wrote:

> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> 
> Add callback function sdhci_get_cd to detect the card.
> And one new callback added to implement the card detect in sdhci struncture.
> If special platform has the card detect callback, it will return the card
> state, the value zero is for absent cardi and one is for present card.
> If the controller don't support card detect, sdhci_get_cd will return -ENOSYS.
> 
> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> CC: Chris Ball <cjb@laptop.org>
> ---
> changes for v2:
> 	- when controller don't support get_cd, return -ENOSYS
> 	- add new callback for sdhci to detect the card
> 	- add the CC
> changes for v3:
> 	- use pin_lock only when get_cd defined
> changes for v4:
> 	- enalbe the controller clock in platform, instead of core
> changes for v5:
> 	- remove the copyright
> 
> drivers/mmc/host/sdhci.c |   21 ++++++++++++++++++++++
> drivers/mmc/host/sdhci.h |    2 ++
> 2 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 6d8eea3..fbe2f46 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1518,6 +1519,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
> 	return ret;
> }
> 
> +/* Return values for the sdjco_get_cd callback:
> + *   0 for a absent card
> + *   1 for a present card
> + *   -ENOSYS when not supported (equal to NULL callback)
> + */
> +static int sdhci_get_cd(struct mmc_host *mmc)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	unsigned long flags;
> +	int present = -ENOSYS;
> +
> +	if (host->ops->get_cd) {
> +		spin_lock_irqsave(&host->lock, flags);
> +		present = host->ops->get_cd(host);
> +		spin_unlock_irqrestore(&host->lock, flags);
> +	}
> +
> +	return present;
> +}

In  static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)

there is code below to handle broken card detect.

1257
1258         /* If polling, assume that the card is always present. */
1259         if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
1260                 present = true;
1261         else
1262                 present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
1263                                 SDHCI_CARD_PRESENT;

The problem with this code is that it assumes broken card detect is broken since
the present state register cannot be read.

would it make more sense to do something like
        if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) {
		if (host->ops->get_cd)
			present = present = host->ops->get_cd(host);
		else
			present = true;

and adjust the code in host->ops->get_cd() to not -ENOSYS.

Philip

> +
> static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable)
> {
> 	if (host->flags & SDHCI_DEVICE_DEAD)
> @@ -1884,6 +1905,7 @@ static const struct mmc_host_ops sdhci_ops = {
> 	.request	= sdhci_request,
> 	.set_ios	= sdhci_set_ios,
> 	.get_ro		= sdhci_get_ro,
> +	.get_cd		= sdhci_get_cd,
> 	.hw_reset	= sdhci_hw_reset,
> 	.enable_sdio_irq = sdhci_enable_sdio_irq,
> 	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0a5b654..82f4d27 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -69,6 +69,7 @@
> #define  SDHCI_SPACE_AVAILABLE	0x00000400
> #define  SDHCI_DATA_AVAILABLE	0x00000800
> #define  SDHCI_CARD_PRESENT	0x00010000
> +#define  SDHCI_CARD_CDPL	0x00040000
> #define  SDHCI_WRITE_PROTECT	0x00080000
> #define  SDHCI_DATA_LVL_MASK	0x00F00000
> #define   SDHCI_DATA_LVL_SHIFT	20
> @@ -261,6 +262,7 @@ struct sdhci_ops {
> 
> 	void	(*set_clock)(struct sdhci_host *host, unsigned int clock);
> 
> +	int		(*get_cd)(struct sdhci_host *host);
> 	int		(*enable_dma)(struct sdhci_host *host);
> 	unsigned int	(*get_max_clock)(struct sdhci_host *host);
> 	unsigned int	(*get_min_clock)(struct sdhci_host *host);
> -- 
> 1.7.5.4
> 
> 
> --
> 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 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Huang Changming-R66093]

> -----Original Message-----
> From: Philip Rakity [mailto:prakity@marvell.com]
> Sent: Wednesday, December 14, 2011 12:52 PM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Huang Changming-R66093; Chris Ball
> Subject: Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect
> the card
> 
> 
> On Dec 13, 2011, at 6:18 PM, <r66093@freescale.com> wrote:
> 
> > From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> >
> > Add callback function sdhci_get_cd to detect the card.
> > And one new callback added to implement the card detect in sdhci
> struncture.
> > If special platform has the card detect callback, it will return the
> > card state, the value zero is for absent cardi and one is for present
> card.
> > If the controller don't support card detect, sdhci_get_cd will return -
> ENOSYS.
> >
> > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > CC: Chris Ball <cjb@laptop.org>
> > ---
> > changes for v2:
> > 	- when controller don't support get_cd, return -ENOSYS
> > 	- add new callback for sdhci to detect the card
> > 	- add the CC
> > changes for v3:
> > 	- use pin_lock only when get_cd defined changes for v4:
> > 	- enalbe the controller clock in platform, instead of core changes
> > for v5:
> > 	- remove the copyright
> >
> > drivers/mmc/host/sdhci.c |   21 ++++++++++++++++++++++
> > drivers/mmc/host/sdhci.h |    2 ++
> > 2 files changed, 23 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index
> > 6d8eea3..fbe2f46 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1518,6 +1519,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
> > 	return ret;
> > }
> >
> > +/* Return values for the sdjco_get_cd callback:
> > + *   0 for a absent card
> > + *   1 for a present card
> > + *   -ENOSYS when not supported (equal to NULL callback)
> > + */
> > +static int sdhci_get_cd(struct mmc_host *mmc) {
> > +	struct sdhci_host *host = mmc_priv(mmc);
> > +	unsigned long flags;
> > +	int present = -ENOSYS;
> > +
> > +	if (host->ops->get_cd) {
> > +		spin_lock_irqsave(&host->lock, flags);
> > +		present = host->ops->get_cd(host);
> > +		spin_unlock_irqrestore(&host->lock, flags);
> > +	}
> > +
> > +	return present;
> > +}
> 
> In  static void sdhci_request(struct mmc_host *mmc, struct mmc_request
> *mrq)
> 
> there is code below to handle broken card detect.
> 
> 1257
> 1258         /* If polling, assume that the card is always present. */
> 1259         if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
> 1260                 present = true;
> 1261         else
> 1262                 present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
> 1263                                 SDHCI_CARD_PRESENT;
> 
> The problem with this code is that it assumes broken card detect is
> broken since the present state register cannot be read.
> 
> would it make more sense to do something like
>         if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) {
> 		if (host->ops->get_cd)
> 			present = present = host->ops->get_cd(host);
> 		else
> 			present = true;
> 
> and adjust the code in host->ops->get_cd() to not -ENOSYS.

I think we should not detect the card present state in sdhci_request function,
Because if we do it, this detection will be performed with any command to card,
which will down the performance.

And if the quirks has SDHCI_QUIRK_BROKEN_CARD_DETECTION, that means the card is always present,
We don't need to detect the card state.

Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Philip Rakity]

On Dec 14, 2011, at 6:32 PM, Huang Changming-R66093 wrote:

> 
> 
>> -----Original Message-----
>> From: Philip Rakity [mailto:prakity@marvell.com]
>> Sent: Wednesday, December 14, 2011 12:52 PM
>> To: Huang Changming-R66093
>> Cc: linux-mmc@vger.kernel.org; Huang Changming-R66093; Chris Ball
>> Subject: Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect
>> the card
>> 
>> 
>> On Dec 13, 2011, at 6:18 PM, <r66093@freescale.com> wrote:
>> 
>>> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
>>> 
>>> Add callback function sdhci_get_cd to detect the card.
>>> And one new callback added to implement the card detect in sdhci
>> struncture.
>>> If special platform has the card detect callback, it will return the
>>> card state, the value zero is for absent cardi and one is for present
>> card.
>>> If the controller don't support card detect, sdhci_get_cd will return -
>> ENOSYS.
>>> 
>>> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
>>> CC: Chris Ball <cjb@laptop.org>
>>> ---
>>> changes for v2:
>>> 	- when controller don't support get_cd, return -ENOSYS
>>> 	- add new callback for sdhci to detect the card
>>> 	- add the CC
>>> changes for v3:
>>> 	- use pin_lock only when get_cd defined changes for v4:
>>> 	- enalbe the controller clock in platform, instead of core changes
>>> for v5:
>>> 	- remove the copyright
>>> 
>>> drivers/mmc/host/sdhci.c |   21 ++++++++++++++++++++++
>>> drivers/mmc/host/sdhci.h |    2 ++
>>> 2 files changed, 23 insertions(+), 0 deletions(-)
>>> 
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index
>>> 6d8eea3..fbe2f46 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1518,6 +1519,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
>>> 	return ret;
>>> }
>>> 
>>> +/* Return values for the sdjco_get_cd callback:
>>> + *   0 for a absent card
>>> + *   1 for a present card
>>> + *   -ENOSYS when not supported (equal to NULL callback)
>>> + */
>>> +static int sdhci_get_cd(struct mmc_host *mmc) {
>>> +	struct sdhci_host *host = mmc_priv(mmc);
>>> +	unsigned long flags;
>>> +	int present = -ENOSYS;
>>> +
>>> +	if (host->ops->get_cd) {
>>> +		spin_lock_irqsave(&host->lock, flags);
>>> +		present = host->ops->get_cd(host);
>>> +		spin_unlock_irqrestore(&host->lock, flags);
>>> +	}
>>> +
>>> +	return present;
>>> +}
>> 
>> In  static void sdhci_request(struct mmc_host *mmc, struct mmc_request
>> *mrq)
>> 
>> there is code below to handle broken card detect.
>> 
>> 1257
>> 1258         /* If polling, assume that the card is always present. */
>> 1259         if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
>> 1260                 present = true;
>> 1261         else
>> 1262                 present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
>> 1263                                 SDHCI_CARD_PRESENT;
>> 
>> The problem with this code is that it assumes broken card detect is
>> broken since the present state register cannot be read.
>> 
>> would it make more sense to do something like
>>        if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) {
>> 		if (host->ops->get_cd)
>> 			present = present = host->ops->get_cd(host);
>> 		else
>> 			present = true;
>> 
>> and adjust the code in host->ops->get_cd() to not -ENOSYS.
> 
> I think we should not detect the card present state in sdhci_request function,
> Because if we do it, this detection will be performed with any command to card,
> which will down the performance.
> 
> And if the quirks has SDHCI_QUIRK_BROKEN_CARD_DETECTION, that means the card is always present,
> We don't need to detect the card state.

currently WE do read the present state register on every request.  All the above suggestion does is try to use your code

maybe we want something like
>>          if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
>>                  present = true;
>>          else if (host->ops->get_cd)
>> 			present = present = host->ops->get_cd(host);
>> 	else
>>                  present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
>>                                  SDHCI_CARD_PRESENT;







> 

RE: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Huang Changming-R66093]

> -----Original Message-----
> From: Philip Rakity [mailto:prakity@marvell.com]
> Sent: Thursday, December 15, 2011 2:42 PM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Chris Ball
> Subject: Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect
> the card
> 
> 
> On Dec 14, 2011, at 6:32 PM, Huang Changming-R66093 wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Philip Rakity [mailto:prakity@marvell.com]
> >> Sent: Wednesday, December 14, 2011 12:52 PM
> >> To: Huang Changming-R66093
> >> Cc: linux-mmc@vger.kernel.org; Huang Changming-R66093; Chris Ball
> >> Subject: Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to
> >> detect the card
> >>
> >>
> >> On Dec 13, 2011, at 6:18 PM, <r66093@freescale.com> wrote:
> >>
> >>> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> >>>
> >>> Add callback function sdhci_get_cd to detect the card.
> >>> And one new callback added to implement the card detect in sdhci
> >> struncture.
> >>> If special platform has the card detect callback, it will return the
> >>> card state, the value zero is for absent cardi and one is for
> >>> present
> >> card.
> >>> If the controller don't support card detect, sdhci_get_cd will
> >>> return -
> >> ENOSYS.
> >>>
> >>> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> >>> CC: Chris Ball <cjb@laptop.org>
> >>> ---
> >>> changes for v2:
> >>> 	- when controller don't support get_cd, return -ENOSYS
> >>> 	- add new callback for sdhci to detect the card
> >>> 	- add the CC
> >>> changes for v3:
> >>> 	- use pin_lock only when get_cd defined changes for v4:
> >>> 	- enalbe the controller clock in platform, instead of core changes
> >>> for v5:
> >>> 	- remove the copyright
> >>>
> >>> drivers/mmc/host/sdhci.c |   21 ++++++++++++++++++++++
> >>> drivers/mmc/host/sdhci.h |    2 ++
> >>> 2 files changed, 23 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >>> index
> >>> 6d8eea3..fbe2f46 100644
> >>> --- a/drivers/mmc/host/sdhci.c
> >>> +++ b/drivers/mmc/host/sdhci.c
> >>> @@ -1518,6 +1519,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
> >>> 	return ret;
> >>> }
> >>>
> >>> +/* Return values for the sdjco_get_cd callback:
> >>> + *   0 for a absent card
> >>> + *   1 for a present card
> >>> + *   -ENOSYS when not supported (equal to NULL callback)
> >>> + */
> >>> +static int sdhci_get_cd(struct mmc_host *mmc) {
> >>> +	struct sdhci_host *host = mmc_priv(mmc);
> >>> +	unsigned long flags;
> >>> +	int present = -ENOSYS;
> >>> +
> >>> +	if (host->ops->get_cd) {
> >>> +		spin_lock_irqsave(&host->lock, flags);
> >>> +		present = host->ops->get_cd(host);
> >>> +		spin_unlock_irqrestore(&host->lock, flags);
> >>> +	}
> >>> +
> >>> +	return present;
> >>> +}
> >>
> >> In  static void sdhci_request(struct mmc_host *mmc, struct
> >> mmc_request
> >> *mrq)
> >>
> >> there is code below to handle broken card detect.
> >>
> >> 1257
> >> 1258         /* If polling, assume that the card is always present. */
> >> 1259         if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
> >> 1260                 present = true;
> >> 1261         else
> >> 1262                 present = sdhci_readl(host, SDHCI_PRESENT_STATE)
> &
> >> 1263                                 SDHCI_CARD_PRESENT;
> >>
> >> The problem with this code is that it assumes broken card detect is
> >> broken since the present state register cannot be read.
> >>
> >> would it make more sense to do something like
> >>        if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) {
> >> 		if (host->ops->get_cd)
> >> 			present = present = host->ops->get_cd(host);
> >> 		else
> >> 			present = true;
> >>
> >> and adjust the code in host->ops->get_cd() to not -ENOSYS.
> >
> > I think we should not detect the card present state in sdhci_request
> > function, Because if we do it, this detection will be performed with
> > any command to card, which will down the performance.
> >
> > And if the quirks has SDHCI_QUIRK_BROKEN_CARD_DETECTION, that means
> > the card is always present, We don't need to detect the card state.
> 
> currently WE do read the present state register on every request.  All
> the above suggestion does is try to use your code
> 
> maybe we want something like
> >>          if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
> >>                  present = true;
> >>          else if (host->ops->get_cd)
> >> 			present = present = host->ops->get_cd(host);
> >> 	else
> >>                  present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
> >>                                  SDHCI_CARD_PRESENT;
> 
I am very confused, why do we read the present state register on every request?
My codes are added to the function mmc_sd_detect in file core/sd.c
Function mmc_rescan has detect the card present state repeatedly, so I think we don't need to detect the card state on every request.
So I think the codes to detect the card present state in sdhci_request should be removed to improve the performance.

Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Jaehoon Chung]

On 12/15/2011 03:56 PM, Huang Changming-R66093 wrote:

> 
> 
>> -----Original Message-----
>> From: Philip Rakity [mailto:prakity@marvell.com]
>> Sent: Thursday, December 15, 2011 2:42 PM
>> To: Huang Changming-R66093
>> Cc: linux-mmc@vger.kernel.org; Chris Ball
>> Subject: Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect
>> the card
>>
>>
>> On Dec 14, 2011, at 6:32 PM, Huang Changming-R66093 wrote:
>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Philip Rakity [mailto:prakity@marvell.com]
>>>> Sent: Wednesday, December 14, 2011 12:52 PM
>>>> To: Huang Changming-R66093
>>>> Cc: linux-mmc@vger.kernel.org; Huang Changming-R66093; Chris Ball
>>>> Subject: Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to
>>>> detect the card
>>>>
>>>>
>>>> On Dec 13, 2011, at 6:18 PM, <r66093@freescale.com> wrote:
>>>>
>>>>> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
>>>>>
>>>>> Add callback function sdhci_get_cd to detect the card.
>>>>> And one new callback added to implement the card detect in sdhci
>>>> struncture.
>>>>> If special platform has the card detect callback, it will return the
>>>>> card state, the value zero is for absent cardi and one is for
>>>>> present
>>>> card.
>>>>> If the controller don't support card detect, sdhci_get_cd will
>>>>> return -
>>>> ENOSYS.
>>>>>
>>>>> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
>>>>> CC: Chris Ball <cjb@laptop.org>
>>>>> ---
>>>>> changes for v2:
>>>>> 	- when controller don't support get_cd, return -ENOSYS
>>>>> 	- add new callback for sdhci to detect the card
>>>>> 	- add the CC
>>>>> changes for v3:
>>>>> 	- use pin_lock only when get_cd defined changes for v4:
>>>>> 	- enalbe the controller clock in platform, instead of core changes
>>>>> for v5:
>>>>> 	- remove the copyright
>>>>>
>>>>> drivers/mmc/host/sdhci.c |   21 ++++++++++++++++++++++
>>>>> drivers/mmc/host/sdhci.h |    2 ++
>>>>> 2 files changed, 23 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>>> index
>>>>> 6d8eea3..fbe2f46 100644
>>>>> --- a/drivers/mmc/host/sdhci.c
>>>>> +++ b/drivers/mmc/host/sdhci.c
>>>>> @@ -1518,6 +1519,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
>>>>> 	return ret;
>>>>> }
>>>>>
>>>>> +/* Return values for the sdjco_get_cd callback:
>>>>> + *   0 for a absent card
>>>>> + *   1 for a present card
>>>>> + *   -ENOSYS when not supported (equal to NULL callback)
>>>>> + */
>>>>> +static int sdhci_get_cd(struct mmc_host *mmc) {
>>>>> +	struct sdhci_host *host = mmc_priv(mmc);
>>>>> +	unsigned long flags;
>>>>> +	int present = -ENOSYS;
>>>>> +
>>>>> +	if (host->ops->get_cd) {
>>>>> +		spin_lock_irqsave(&host->lock, flags);
>>>>> +		present = host->ops->get_cd(host);
>>>>> +		spin_unlock_irqrestore(&host->lock, flags);
>>>>> +	}
>>>>> +
>>>>> +	return present;
>>>>> +}
>>>>
>>>> In  static void sdhci_request(struct mmc_host *mmc, struct
>>>> mmc_request
>>>> *mrq)
>>>>
>>>> there is code below to handle broken card detect.
>>>>
>>>> 1257
>>>> 1258         /* If polling, assume that the card is always present. */
>>>> 1259         if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
>>>> 1260                 present = true;
>>>> 1261         else
>>>> 1262                 present = sdhci_readl(host, SDHCI_PRESENT_STATE)
>> &
>>>> 1263                                 SDHCI_CARD_PRESENT;
>>>>
>>>> The problem with this code is that it assumes broken card detect is
>>>> broken since the present state register cannot be read.
>>>>
>>>> would it make more sense to do something like
>>>>        if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) {
>>>> 		if (host->ops->get_cd)
>>>> 			present = present = host->ops->get_cd(host);
>>>> 		else
>>>> 			present = true;
>>>>
>>>> and adjust the code in host->ops->get_cd() to not -ENOSYS.
>>>
>>> I think we should not detect the card present state in sdhci_request
>>> function, Because if we do it, this detection will be performed with
>>> any command to card, which will down the performance.
>>>
>>> And if the quirks has SDHCI_QUIRK_BROKEN_CARD_DETECTION, that means
>>> the card is always present, We don't need to detect the card state.

I think SDHCI_QUIRK_BROKEN_CARD_DETECTION is just _assume_ "the card is present."

>>
>> currently WE do read the present state register on every request.  All
>> the above suggestion does is try to use your code
>>
>> maybe we want something like
>>>>          if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION)
>>>>                  present = true;
>>>>          else if (host->ops->get_cd)
>>>> 			present = present = host->ops->get_cd(host);
>>>> 	else
>>>>                  present = sdhci_readl(host, SDHCI_PRESENT_STATE) &
>>>>                                  SDHCI_CARD_PRESENT;
>>
> I am very confused, why do we read the present state register on every request?

How long time read the present state register?

> My codes are added to the function mmc_sd_detect in file core/sd.c
> Function mmc_rescan has detect the card present state repeatedly, so I think we don't need to detect the card state on every request.
> So I think the codes to detect the card present state in sdhci_request should be removed to improve the performance.

How did you get the performance benefit? 
And mmc_rescan is repeatedly for SD-card?
If mmc_rescan is repeatedly, that reason is the below code. (just my thinking)

if (host->caps & MMC_CAP_NEEDS_POLL)
	mmc_schedule_delayed_work(&host->detect, HZ);

what is your point related with mmc_rescan?
I didn't understand yours..

Thanks,
Jaehoon Chung

> 
> 
> --
> 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 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Huang Changming-R66093]

> > I am very confused, why do we read the present state register on every
> request?
> 
> How long time read the present state register?
Even if one line code is performed, I think it need time to complete.

> > My codes are added to the function mmc_sd_detect in file core/sd.c
> > Function mmc_rescan has detect the card present state repeatedly, so I
> think we don't need to detect the card state on every request.
> > So I think the codes to detect the card present state in sdhci_request
> should be removed to improve the performance.
> 
> How did you get the performance benefit?
Do you know there are many tools to run the performance?
IPForward/IPSEC for network, iozone for SATA/SDHC...

We have the performance result from our platform, the result will decrease 2%~5%!

If the driver has the option 'MMC_CAP_NEEDS_POLL' (many driver enable this option) and the card is always present, the driver will send the command "mmc_send_status" to ask the card state repeatedly.
Do you know what it means? Many interrupts will be generated even if the system is reading from/writing to the SD/MMC card.

> And mmc_rescan is repeatedly for SD-card?
> If mmc_rescan is repeatedly, that reason is the below code. (just my
> thinking)
> 
> if (host->caps & MMC_CAP_NEEDS_POLL)
> 	mmc_schedule_delayed_work(&host->detect, HZ);
> 
> what is your point related with mmc_rescan?
> I didn't understand yours..

There are some controllers don't support the card detect, the SDHCI_CARD_PRESENT is forever one. And some controller must enable the MMC_CAP_NEEDS_POLL option (about this, you can search the folder mmc/host/), that means mmc_rescan will be run repeatedly.
In these cases, if we detect the card state though command mmc_send_status, a lot of interrupts will be generated.

For the transaction from/to SD card, we don't need to check the card present state, it should be done in other area.
So I don't think it is correct to detect card state in function sdhci_request. 

Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Jaehoon Chung]

On 12/16/2011 12:25 PM, Huang Changming-R66093 wrote:

>>> I am very confused, why do we read the present state register on every
>> request?
>>
>> How long time read the present state register?
> Even if one line code is performed, I think it need time to complete.
> 
>>> My codes are added to the function mmc_sd_detect in file core/sd.c
>>> Function mmc_rescan has detect the card present state repeatedly, so I
>> think we don't need to detect the card state on every request.
>>> So I think the codes to detect the card present state in sdhci_request
>> should be removed to improve the performance.
>>
>> How did you get the performance benefit?
> Do you know there are many tools to run the performance?
> IPForward/IPSEC for network, iozone for SATA/SDHC...
> 
> We have the performance result from our platform, the result will decrease 2%~5%!
> 
> If the driver has the option 'MMC_CAP_NEEDS_POLL' (many driver enable this option) and the card is always present, the driver will send the command "mmc_send_status" to ask the card state repeatedly.
> Do you know what it means? Many interrupts will be generated even if the system is reading from/writing to the SD/MMC card.
> 
>> And mmc_rescan is repeatedly for SD-card?
>> If mmc_rescan is repeatedly, that reason is the below code. (just my
>> thinking)
>>
>> if (host->caps & MMC_CAP_NEEDS_POLL)
>> 	mmc_schedule_delayed_work(&host->detect, HZ);
>>
>> what is your point related with mmc_rescan?
>> I didn't understand yours..
> 
> There are some controllers don't support the card detect, the SDHCI_CARD_PRESENT is forever one. And some controller must enable the MMC_CAP_NEEDS_POLL option (about this, you can search the folder mmc/host/), that means mmc_rescan will be run repeatedly.
> In these cases, if we detect the card state though command mmc_send_status, a lot of interrupts will be generated.

I know that...Some controller didn't support the card detect...But I think you can control this problem with SDHCI_QUIRK_BROKEN_CARD_DETECTION and some flags.
If controller is always polling for card status, that's inefficient.
I use the SDHCI_QUIRK_BROKEN_CARD_DETECTION, but didn't generate the interrupt at every time(didn't polling)
I think you can solve this problem in sdhci-xxx.c, not in sdhci.c

> 
> For the transaction from/to SD card, we don't need to check the card present state, it should be done in other area.
> So I don't think it is correct to detect card state in function sdhci_request. 

> 

> 
> --
> 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 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Huang Changming-R66093]

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Jaehoon Chung
> Sent: Friday, December 16, 2011 12:06 PM
> To: Huang Changming-R66093
> Cc: Jaehoon Chung; Philip Rakity; linux-mmc@vger.kernel.org; Chris Ball
> Subject: Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect
> the card
> 
> On 12/16/2011 12:25 PM, Huang Changming-R66093 wrote:
> 
> >>> I am very confused, why do we read the present state register on
> every
> >> request?
> >>
> >> How long time read the present state register?
> > Even if one line code is performed, I think it need time to complete.
> >
> >>> My codes are added to the function mmc_sd_detect in file core/sd.c
> >>> Function mmc_rescan has detect the card present state repeatedly, so
> I
> >> think we don't need to detect the card state on every request.
> >>> So I think the codes to detect the card present state in
> sdhci_request
> >> should be removed to improve the performance.
> >>
> >> How did you get the performance benefit?
> > Do you know there are many tools to run the performance?
> > IPForward/IPSEC for network, iozone for SATA/SDHC...
> >
> > We have the performance result from our platform, the result will
> decrease 2%~5%!
> >
> > If the driver has the option 'MMC_CAP_NEEDS_POLL' (many driver enable
> this option) and the card is always present, the driver will send the
> command "mmc_send_status" to ask the card state repeatedly.
> > Do you know what it means? Many interrupts will be generated even if
> the system is reading from/writing to the SD/MMC card.
> >
> >> And mmc_rescan is repeatedly for SD-card?
> >> If mmc_rescan is repeatedly, that reason is the below code. (just my
> >> thinking)
> >>
> >> if (host->caps & MMC_CAP_NEEDS_POLL)
> >> 	mmc_schedule_delayed_work(&host->detect, HZ);
> >>
> >> what is your point related with mmc_rescan?
> >> I didn't understand yours..
> >
> > There are some controllers don't support the card detect, the
> SDHCI_CARD_PRESENT is forever one. And some controller must enable the
> MMC_CAP_NEEDS_POLL option (about this, you can search the folder
> mmc/host/), that means mmc_rescan will be run repeatedly.
> > In these cases, if we detect the card state though command
> mmc_send_status, a lot of interrupts will be generated.
> 
> I know that...Some controller didn't support the card detect...But I
> think you can control this problem with SDHCI_QUIRK_BROKEN_CARD_DETECTION
> and some flags.
> If controller is always polling for card status, that's inefficient.
Maybe.
But, many controller will polling the card state as controller designed.

> I use the SDHCI_QUIRK_BROKEN_CARD_DETECTION, but didn't generate the
> interrupt at every time(didn't polling)
> I think you can solve this problem in sdhci-xxx.c, not in sdhci.c
> 
Because there is one interface to detect card state .get_cd in structure mmc_host_ops,
I just implement it in sdhci.c and just run the callback if the controller support the card detection,
and think about some controller don't support card detection,
I add one new interface to detect the card state in special platform.
For FSL eSDHC controller, I do it in sdhci-of-esdhc.c

RE: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Huang Changming-R66093]

Hi, Chris,
Could you have any comment about this patch?
Can it go into 3.3 or 3.4?

Thanks
Jerry Huang

> -----Original Message-----
> From: Huang Changming-R66093
> Sent: Wednesday, December 14, 2011 10:18 AM
> To: linux-mmc@vger.kernel.org
> Cc: Huang Changming-R66093; Chris Ball
> Subject: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the
> card
> 
> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> 
> Add callback function sdhci_get_cd to detect the card.
> And one new callback added to implement the card detect in sdhci
> struncture.
> If special platform has the card detect callback, it will return the card
> state, the value zero is for absent cardi and one is for present card.
> If the controller don't support card detect, sdhci_get_cd will return -
> ENOSYS.
> 
> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> CC: Chris Ball <cjb@laptop.org>
> ---
> changes for v2:
> 	- when controller don't support get_cd, return -ENOSYS
> 	- add new callback for sdhci to detect the card
> 	- add the CC
> changes for v3:
> 	- use pin_lock only when get_cd defined
> changes for v4:
> 	- enalbe the controller clock in platform, instead of core
> changes for v5:
> 	- remove the copyright
> 
>  drivers/mmc/host/sdhci.c |   21 ++++++++++++++++++++++
>  drivers/mmc/host/sdhci.h |    2 ++
>  2 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 6d8eea3..fbe2f46 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1518,6 +1519,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
>  	return ret;
>  }
> 
> +/* Return values for the sdjco_get_cd callback:
> + *   0 for a absent card
> + *   1 for a present card
> + *   -ENOSYS when not supported (equal to NULL callback)
> + */
> +static int sdhci_get_cd(struct mmc_host *mmc)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	unsigned long flags;
> +	int present = -ENOSYS;
> +
> +	if (host->ops->get_cd) {
> +		spin_lock_irqsave(&host->lock, flags);
> +		present = host->ops->get_cd(host);
> +		spin_unlock_irqrestore(&host->lock, flags);
> +	}
> +
> +	return present;
> +}
> +
>  static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int
> enable)
>  {
>  	if (host->flags & SDHCI_DEVICE_DEAD)
> @@ -1884,6 +1905,7 @@ static const struct mmc_host_ops sdhci_ops = {
>  	.request	= sdhci_request,
>  	.set_ios	= sdhci_set_ios,
>  	.get_ro		= sdhci_get_ro,
> +	.get_cd		= sdhci_get_cd,
>  	.hw_reset	= sdhci_hw_reset,
>  	.enable_sdio_irq = sdhci_enable_sdio_irq,
>  	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0a5b654..82f4d27 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -69,6 +69,7 @@
>  #define  SDHCI_SPACE_AVAILABLE	0x00000400
>  #define  SDHCI_DATA_AVAILABLE	0x00000800
>  #define  SDHCI_CARD_PRESENT	0x00010000
> +#define  SDHCI_CARD_CDPL	0x00040000
>  #define  SDHCI_WRITE_PROTECT	0x00080000
>  #define  SDHCI_DATA_LVL_MASK	0x00F00000
>  #define   SDHCI_DATA_LVL_SHIFT	20
> @@ -261,6 +262,7 @@ struct sdhci_ops {
> 
>  	void	(*set_clock)(struct sdhci_host *host, unsigned int clock);
> 
> +	int		(*get_cd)(struct sdhci_host *host);
>  	int		(*enable_dma)(struct sdhci_host *host);
>  	unsigned int	(*get_max_clock)(struct sdhci_host *host);
>  	unsigned int	(*get_min_clock)(struct sdhci_host *host);
> --
> 1.7.5.4

Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Aaron Lu]

Hi,

On Fri, Jan 13, 2012 at 02:25:18AM +0000, Huang Changming-R66093 wrote:
> Hi, Chris,
> Could you have any comment about this patch?
> Can it go into 3.3 or 3.4?
> 
> Thanks
> Jerry Huang
> 
> > -----Original Message-----
> > From: Huang Changming-R66093
> > Sent: Wednesday, December 14, 2011 10:18 AM
> > To: linux-mmc@vger.kernel.org
> > Cc: Huang Changming-R66093; Chris Ball
> > Subject: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the
> > card
> > 
> > From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > 
> > Add callback function sdhci_get_cd to detect the card.
> > And one new callback added to implement the card detect in sdhci
> > struncture.
> > If special platform has the card detect callback, it will return the card
> > state, the value zero is for absent cardi and one is for present card.
> > If the controller don't support card detect, sdhci_get_cd will return -
> > ENOSYS.
> > 
> > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > CC: Chris Ball <cjb@laptop.org>
> > ---
> > changes for v2:
> > 	- when controller don't support get_cd, return -ENOSYS
> > 	- add new callback for sdhci to detect the card
> > 	- add the CC
> > changes for v3:
> > 	- use pin_lock only when get_cd defined
> > changes for v4:
> > 	- enalbe the controller clock in platform, instead of core
> > changes for v5:
> > 	- remove the copyright
> > 
> >  drivers/mmc/host/sdhci.c |   21 ++++++++++++++++++++++
> >  drivers/mmc/host/sdhci.h |    2 ++
> >  2 files changed, 23 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 6d8eea3..fbe2f46 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1518,6 +1519,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
> >  	return ret;
> >  }

snip
> > 
> > +/* Return values for the sdjco_get_cd callback:
> > + *   0 for a absent card
> > + *   1 for a present card
> > + *   -ENOSYS when not supported (equal to NULL callback)
> > + */
> > +static int sdhci_get_cd(struct mmc_host *mmc)
> > +{
> > +	struct sdhci_host *host = mmc_priv(mmc);
> > +	unsigned long flags;
> > +	int present = -ENOSYS;
> > +
> > +	if (host->ops->get_cd) {
> > +		spin_lock_irqsave(&host->lock, flags);
> > +		present = host->ops->get_cd(host);
> > +		spin_unlock_irqrestore(&host->lock, flags);
> > +	}
> > +
> > +	return present;
> > +}
> > +

I think this function has to take care of standard sd host behaviour: if
a specific host has implemented the get_cd callback, then use it; if
not, then see the BROKEN_DETECT quirk; finally, use the present register
to get the value like the sdhci_request function does.

-Aaron

> >  static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int
> > enable)
> >  {
> >  	if (host->flags & SDHCI_DEVICE_DEAD)
> > @@ -1884,6 +1905,7 @@ static const struct mmc_host_ops sdhci_ops = {
> >  	.request	= sdhci_request,
> >  	.set_ios	= sdhci_set_ios,
> >  	.get_ro		= sdhci_get_ro,
> > +	.get_cd		= sdhci_get_cd,
> >  	.hw_reset	= sdhci_hw_reset,
> >  	.enable_sdio_irq = sdhci_enable_sdio_irq,
> >  	.start_signal_voltage_switch	= sdhci_start_signal_voltage_switch,
> > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> > index 0a5b654..82f4d27 100644
> > --- a/drivers/mmc/host/sdhci.h
> > +++ b/drivers/mmc/host/sdhci.h
> > @@ -69,6 +69,7 @@
> >  #define  SDHCI_SPACE_AVAILABLE	0x00000400
> >  #define  SDHCI_DATA_AVAILABLE	0x00000800
> >  #define  SDHCI_CARD_PRESENT	0x00010000
> > +#define  SDHCI_CARD_CDPL	0x00040000
> >  #define  SDHCI_WRITE_PROTECT	0x00080000
> >  #define  SDHCI_DATA_LVL_MASK	0x00F00000
> >  #define   SDHCI_DATA_LVL_SHIFT	20
> > @@ -261,6 +262,7 @@ struct sdhci_ops {
> > 
> >  	void	(*set_clock)(struct sdhci_host *host, unsigned int clock);
> > 
> > +	int		(*get_cd)(struct sdhci_host *host);
> >  	int		(*enable_dma)(struct sdhci_host *host);
> >  	unsigned int	(*get_max_clock)(struct sdhci_host *host);
> >  	unsigned int	(*get_min_clock)(struct sdhci_host *host);
> > --
> > 1.7.5.4
> 
> 
> --
> 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 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Huang Changming-R66093]

> -----Original Message-----
> From: Aaron Lu [mailto:aaron.lu@amd.com]
> Sent: Friday, January 13, 2012 11:27 AM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Chris Ball
> Subject: Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect
> the card
> 
> Hi,
> 
> On Fri, Jan 13, 2012 at 02:25:18AM +0000, Huang Changming-R66093 wrote:
> > Hi, Chris,
> > Could you have any comment about this patch?
> > Can it go into 3.3 or 3.4?
> >
> > Thanks
> > Jerry Huang
> >
> > > -----Original Message-----
> > > From: Huang Changming-R66093
> > > Sent: Wednesday, December 14, 2011 10:18 AM
> > > To: linux-mmc@vger.kernel.org
> > > Cc: Huang Changming-R66093; Chris Ball
> > > Subject: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect
> the
> > > card
> > >
> > > From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > >
> > > Add callback function sdhci_get_cd to detect the card.
> > > And one new callback added to implement the card detect in sdhci
> > > struncture.
> > > If special platform has the card detect callback, it will return the
> card
> > > state, the value zero is for absent cardi and one is for present card.
> > > If the controller don't support card detect, sdhci_get_cd will return
> -
> > > ENOSYS.
> > >
> > > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > > CC: Chris Ball <cjb@laptop.org>
> > > ---
> > > changes for v2:
> > > 	- when controller don't support get_cd, return -ENOSYS
> > > 	- add new callback for sdhci to detect the card
> > > 	- add the CC
> > > changes for v3:
> > > 	- use pin_lock only when get_cd defined
> > > changes for v4:
> > > 	- enalbe the controller clock in platform, instead of core
> > > changes for v5:
> > > 	- remove the copyright
> > >
> > >  drivers/mmc/host/sdhci.c |   21 ++++++++++++++++++++++
> > >  drivers/mmc/host/sdhci.h |    2 ++
> > >  2 files changed, 23 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > > index 6d8eea3..fbe2f46 100644
> > > --- a/drivers/mmc/host/sdhci.c
> > > +++ b/drivers/mmc/host/sdhci.c
> > > @@ -1518,6 +1519,26 @@ static int sdhci_get_ro(struct mmc_host *mmc)
> > >  	return ret;
> > >  }
> 
> snip
> > >
> > > +/* Return values for the sdjco_get_cd callback:
> > > + *   0 for a absent card
> > > + *   1 for a present card
> > > + *   -ENOSYS when not supported (equal to NULL callback)
> > > + */
> > > +static int sdhci_get_cd(struct mmc_host *mmc)
> > > +{
> > > +	struct sdhci_host *host = mmc_priv(mmc);
> > > +	unsigned long flags;
> > > +	int present = -ENOSYS;
> > > +
> > > +	if (host->ops->get_cd) {
> > > +		spin_lock_irqsave(&host->lock, flags);
> > > +		present = host->ops->get_cd(host);
> > > +		spin_unlock_irqrestore(&host->lock, flags);
> > > +	}
> > > +
> > > +	return present;
> > > +}
> > > +
> 
> I think this function has to take care of standard sd host behaviour: if
> a specific host has implemented the get_cd callback, then use it; if
> not, then see the BROKEN_DETECT quirk; finally, use the present register
> to get the value like the sdhci_request function does.
> 
Yes, I have thought about it.
In this patch, only FSL eSDHC use it.
For the other platform, the get_cd is NULL.

Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Aaron Lu]

On Fri, Jan 13, 2012 at 04:50:11AM +0000, Huang Changming-R66093 wrote:
> 
> 
> > I think this function has to take care of standard sd host behaviour: if
> > a specific host has implemented the get_cd callback, then use it; if
> > not, then see the BROKEN_DETECT quirk; finally, use the present register
> > to get the value like the sdhci_request function does.
> > 
> Yes, I have thought about it.
> In this patch, only FSL eSDHC use it.

Only FSL eSDHC defines it, but all sd hosts will use it since the call
is made in core.c with host->ops->get_cd.

If you only cares about FSL eSDHC, then probably you should do this in
sdhci.c instead of core.c, by adding a callback to the sdhci_ops and in
the sdhci_request call, instead of checking present register, call the
get_cd callback if available.

> For the other platform, the get_cd is NULL.
>

RE: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect the card

[Huang Changming-R66093]

> -----Original Message-----
> From: Aaron Lu [mailto:aaron.lu@amd.com]
> Sent: Friday, January 13, 2012 2:21 PM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Chris Ball
> Subject: Re: [PATCH 3/4 v5] SDHCI: add sdhci_get_cd callback to detect
> the card
> 
> On Fri, Jan 13, 2012 at 04:50:11AM +0000, Huang Changming-R66093 wrote:
> >
> >
> > > I think this function has to take care of standard sd host
> > > behaviour: if a specific host has implemented the get_cd callback,
> > > then use it; if not, then see the BROKEN_DETECT quirk; finally, use
> > > the present register to get the value like the sdhci_request function
> does.
> > >
> > Yes, I have thought about it.
> > In this patch, only FSL eSDHC use it.
> 
> Only FSL eSDHC defines it, but all sd hosts will use it since the call is
> made in core.c with host->ops->get_cd.
First, I check if the get_cd is defined, if no, this callback can't be called.

> If you only cares about FSL eSDHC, then probably you should do this in
> sdhci.c instead of core.c, by adding a callback to the sdhci_ops and in
> the sdhci_request call, instead of checking present register, call the
> get_cd callback if available.
> 
Maybe you can read my patch carefully. 
In file "drivers/mmc/host/sdhci.c", I just implement the callback get_cd,
And add new callback or hook (get_cd in structre sdhci_ops) to detect the card in the special platform.