RE: [PATCH 1/3] mmc: esdhc: enable polling to detect card by itself

2012-10-18 Thread Yong Ding
Shawn,
Thanks, I will update the patch for sdhci-esdhc-imx.c and also fix my mail 
ASAP:-)

-Original Message-
From: Shawn Guo [mailto:shawn@linaro.org] 
Sent: 2012年10月18日 14:38
To: Yong Ding
Cc: Chris Ball; Anton Vorontsov; Marek Szyprowski; Wolfram Sang; Daniel Drake; 
Sascha Hauer; Wilson Callan; Ben Dooks; Zhangfei Gao; Kevin Liu; Jialing Fu; 
linux-mmc@vger.kernel.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH 1/3] mmc: esdhc: enable polling to detect card by itself

Can you fix your mailer to use bottom posting rather than top posting,
and have texts wrap around column 70?  Otherwise, you message stands
a good chance to be ignored by people.

On Wed, Oct 17, 2012 at 11:27:17PM -0700, Yong Ding wrote:
> Shawn,
> Thanks. Oh, sorry I really have missed the fact u mentioned. U are right in 
> the current code, the bit will also be cleared for ESDHC_CD_GPIO. 
> But I think this is improper since for GPIO detection type, we don't use the 
> host controller internal card detection(ESDHC_CD_CONTROLLER), but with 
> SDHCI_QUIRK_BROKEN_CARD_DETECTION cleared, we'll still enable/disable 
> relevant INT bits (in sdhci_set_card_detection in sdhci.c). This is my 
> biggest concern. And I think the SDHCI_QUIRK_BROKEN_CARD_DETECTION shall be 
> purely used to notify whether the host controller detection method is used or 
> not. So even for the ESDHC_CD_GPIO type, we should still set this flag. How 
> do u think?
> 
I'm fine with that.  Just remind you a fact you seem missed in case
you need to change sdhci-esdhc-imx.c to adapt the changes you are going
to make on sdhci.c.

Shawn 
N�Р骒r��yb�X�肚�v�^�)藓{.n�+�伐�{��g"��^n�r■�z���h�ㄨ��&Ⅷ�G���h�(�茛j"���m赇z罐��帼f"�h���~�m�

RE: [PATCH 1/3] mmc: esdhc: enable polling to detect card by itself

2012-10-17 Thread Yong Ding
Shawn,
Thanks. Oh, sorry I really have missed the fact u mentioned. U are right in the 
current code, the bit will also be cleared for ESDHC_CD_GPIO. 
But I think this is improper since for GPIO detection type, we don't use the 
host controller internal card detection(ESDHC_CD_CONTROLLER), but with 
SDHCI_QUIRK_BROKEN_CARD_DETECTION cleared, we'll still enable/disable relevant 
INT bits (in sdhci_set_card_detection in sdhci.c). This is my biggest concern. 
And I think the SDHCI_QUIRK_BROKEN_CARD_DETECTION shall be purely used to 
notify whether the host controller detection method is used or not. So even for 
the ESDHC_CD_GPIO type, we should still set this flag. How do u think?

-Original Message-
From: Shawn Guo [mailto:shawn@linaro.org] 
Sent: 2012年10月18日 13:51
To: Yong Ding
Cc: Chris Ball; Anton Vorontsov; Marek Szyprowski; Wolfram Sang; Daniel Drake; 
Sascha Hauer; Wilson Callan; Ben Dooks; Zhangfei Gao; Kevin Liu; Jialing Fu; 
linux-mmc@vger.kernel.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH 1/3] mmc: esdhc: enable polling to detect card by itself

On Tue, Oct 16, 2012 at 09:01:40PM -0700, Yong Ding wrote:
> Shawn,
> Thanks for your comment. And sorry for my so late due to illness:-)
> SDHCI_QUIRK_BROKEN_CARD_DETECTION is used for notifying we don't use the host 
> internal card detection method so that we don't need enable/disable those 
> relevant interrupt bits of host(sdhci_set_card_detection in sdhci.c). 
> And as I double-checked the latest kernel code, actually sdhci-esdhc-imx sets 
> this flag by default, and then will clear it only when the detection type is 
> ESDHC_CD_CONTROLLER. So this aligns with my understanding. 

What I was saying is the bit will be cleared when the detection type is
ESDHC_CD_CONTROLLE or ESDHC_CD_GPIO.  You may have missed the fact that
there is no "break" in case ESDHC_CD_GPIO but a "fall through" comment.

switch (boarddata->cd_type) {
case ESDHC_CD_GPIO:
err = gpio_request_one(boarddata->cd_gpio, GPIOF_IN, 
"ESDHC_CD");
if (err) {
dev_err(mmc_dev(host->mmc),
"no card-detect pin available!\n");
goto no_card_detect_pin;
}

err = request_irq(gpio_to_irq(boarddata->cd_gpio), cd_irq,
 IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
 mmc_hostname(host->mmc), host);
if (err) {
dev_err(mmc_dev(host->mmc), "request irq error\n");
goto no_card_detect_irq;
}
/* fall through */

case ESDHC_CD_CONTROLLER:
/* we have a working card_detect back */
host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
break;

case ESDHC_CD_PERMANENT:
host->mmc->caps = MMC_CAP_NONREMOVABLE;
break;

case ESDHC_CD_NONE:
break;
}

Shawn

> What I want to do is that 1st we shall set MMC_CAP_NEEDS_POLL by our host 
> driver itself and 2nd remove the improper logic in sdhci_add_host() . How do 
> u think? Thanks.
> 


RE: [PATCH 1/3] mmc: esdhc: enable polling to detect card by itself

2012-10-16 Thread Yong Ding
Shawn,
Thanks for your comment. And sorry for my so late due to illness:-)
SDHCI_QUIRK_BROKEN_CARD_DETECTION is used for notifying we don't use the host 
internal card detection method so that we don't need enable/disable those 
relevant interrupt bits of host(sdhci_set_card_detection in sdhci.c). 
And as I double-checked the latest kernel code, actually sdhci-esdhc-imx sets 
this flag by default, and then will clear it only when the detection type is 
ESDHC_CD_CONTROLLER. So this aligns with my understanding. 
What I want to do is that 1st we shall set MMC_CAP_NEEDS_POLL by our host 
driver itself and 2nd remove the improper logic in sdhci_add_host() . How do u 
think? Thanks.


From: Shawn Guo [shawn@linaro.org]
Sent: Sunday, October 07, 2012 22:06
To: Yong Ding
Cc: Chris Ball; Anton Vorontsov; Marek Szyprowski; Wolfram Sang; Daniel Drake; 
Sascha Hauer; Wilson Callan; Ben Dooks; Zhangfei Gao; Kevin Liu; Jialing Fu; 
linux-mmc@vger.kernel.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH 1/3] mmc: esdhc: enable polling to detect card by itself

On Fri, Sep 28, 2012 at 06:28:31PM +0800, yongd wrote:
> In the current code logic, sdhci_add_host() will enable the polling
> method (set MMC_CAP_NEEDS_POLL) for a removable card (MMC_CAP_
> NONREMOVABLE is not set) whose host's internal card detection method
> is disabled for some reason (SDHCI_QUIRK_BROKEN_CARD_DETECTION is set).
>
> However, this is improper since we can have some other card detection
> methods besides host internal card detection and polling method. For
> example, if the card detection type is ESDHC_CD_GPIO (external gpio pin
> for CD), we will keep SDHCI_QUIRK_BROKEN_CARD_DETECTION set. This is right.
> But, just as above said, sdhci_add_host() will also enable polling for such
> a card. This is redundant.
>
At least for sdhci-esdhc-imx, SDHCI_QUIRK_BROKEN_CARD_DETECTION will
be set only when neither ESDHC_CD_GPIO nor ESDHC_CD_CONTROLLER works.

Shawn

> On the other hand, for the card with ESDHC_CD_NONE detection type(no CD, 
> neither
> controller nor gpio, so use polling), we currently rely on sdhci_add_host() to
> enable polling for us.
>
> Here proposed a solution for such an embarrassing case. 1st, this patch will
> de-couple polling enabling with sdhci_add_host() by doing this in host driver
> itself, just as some other vendors. 2nd, one more patch will remove such 
> improper
> MMC_CAP_NEEDS_POLL enabling in sdhci_add_host().
>
> Change-Id: Ia7525009d8fd188e3f0169f225e4a76ff9e94b47
> Signed-off-by: yongd 
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c |1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c 
> b/drivers/mmc/host/sdhci-esdhc-imx.c
> index e23f813..f70079c 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -569,6 +569,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct 
> platform_device *pdev)
>   break;
>
>   case ESDHC_CD_NONE:
> + host->mmc->caps = MMC_CAP_NEEDS_POLL;
>   break;
>   }
>
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [PATCH] mmc: remove MMC_CAP_NEEDS_POLL setting in sdhci_add_host

2012-09-28 Thread Yong Ding
Anton,
For adding a new QUICK which is something like SDHCI_QUICK_NEEDS_POLL, I think 
it is unnecessary since it actually has the same meaning as 
MMC_CAP_NEEDS_POLLING. Actually, we can directly set MMC_CAP_NEEDS_POLL in host 
driver.

So I prefer to review all the host drivers, and proposed relevant changes. And 
some vendors already have been like this(setting polling by themselves). And as 
u can see, I have already sent the new patch set. Pls also help review. Thanks 
a lot.


Best Wishes,

Yong Ding
Operating Systems Engineering,
Application Processor Systems Engineering

-Original Message-
From: Anton Vorontsov [mailto:anton.voront...@linaro.org] 
Sent: Thursday, September 27, 2012 7:50 AM
To: Yong Ding
Cc: Chris Ball; linux-mmc@vger.kernel.org; Daniel Drake; Zhangfei Gao
Subject: Re: [PATCH] mmc: remove MMC_CAP_NEEDS_POLL setting in sdhci_add_host

On Tue, Sep 25, 2012 at 02:13:10AM -0700, Yong Ding wrote:
[...]
> So, in all, u are right if with my current patch, some host drivers need
> some improvement to add MMC_CAP_NEEDS_POLL when it is actually needed.
> But I think this shall be the right way to follow. Or, we might enable
> polling for some cases in which it is unnecessary, and maybe this is a
> potential issue-bomb. How do u think?

I think if you carefully review and fixup all the drivers, it will be
fine.

But you'd have to add MMC_CAP_NEEDS_POLL into drivers' code (while w/ the
quirk it's less lines of code for drivers).

So, here is another idea: how about something like this

#define SDHCI_QUIRK_NEEDS_POLL \
(SDHCI_QUIRK_BROKEN_CARD_DETECTION | (1 << NN))

And changing the logic to:

if ((host->quirks & SDHCI_QUIRK_NEEDS_POLL) &&
!(host->mmc->caps & MMC_CAP_NONREMOVABLE))
mmc->caps |= MMC_CAP_NEEDS_POLL;

And then, you'd just convert all the current drivers to
SDHCI_QUIRK_NEEDS_POLL (which would be 100% safe), and for your driver,
you'd only set SDHCI_QUIRK_BROKEN_CARD_DETECTION.

Thanks,
Anton.


RE: [PATCH] mmc: remove MMC_CAP_NEEDS_POLL setting in sdhci_add_host

2012-09-25 Thread Yong Ding
Anton,
Thanks a lot for your comments. I've just reviewed those host drivers under 
drivers/mmc/host who use SDHCI_QUIRK_BROKEN_CARD_DETECTION. And here are the 
details,

1. sdhci-pxav2/3.c
Actually they set "SDHCI_QUIRK_BROKEN_CARD_DETECTION and MMC_CAP_NONREMOVABLE" 
together for a permanent card who is always wired to host and with power always 
on, eg, emmc card. So, they don't set SDHCI_QUIRK_BROKEN_CARD_DETECTION for 
polling:-)

2. sdhci-s3c.c
If the card detection type is S3C_SDHCI_CD_NONE, they are expecting polling 
method to detect card. And in the current code logic, sdhci-s3c.c does not set 
MMC_CAP_NEEDS_POLL itself, but depends on the code which I am preferring to 
remove. So, you are right this is not so that simple.

3. sdhci-esdhc-imx.c
By default, it sets SDHCI_QUIRK_BROKEN_CARD_DETECTION for all hosts. 
And then if the card detection type is ESDHC_CD_CONTROLLER(indicating we use 
the host internal card detection), it will clear this flag. No issue.
If the cd_type is ESDHC_CD_PERMANENT, it will still keep the set of the flag. 
This is just exactly the same case as sdhci-pxav2/3.c. No issue.
If the cd_type is ESDHC_CD_GPIO(using external GPIO for CD), it will keep the 
set of this flag, and I think this is the right logic since we don't use host 
internal card detection. But this will eventually causes the enabling of 
polling method, which is rightly the case mentioned in my patch comment. A +1 
for my patch.
If the cd_type is ESDHC_CD_NONE, this is the same case as sdhci-s3c.c. A +1 for 
your guess/concern:-)

4. sdhci-of-esdhc.c and sdhci-pci.c
Cased are already included in the above 3 samples. So nothing more here.

So, in all, u are right if with my current patch, some host drivers need some 
improvement to add MMC_CAP_NEEDS_POLL when it is actually needed. But I think 
this shall be the right way to follow. Or, we might enable polling for some 
cases in which it is unnecessary, and maybe this is a potential issue-bomb. How 
do u think?


Best Wishes,

Yong Ding
Operating Systems Engineering,
Application Processor Systems Engineering

> -Original Message-
> From: Anton Vorontsov [mailto:anton.voront...@linaro.org]
> Sent: Tuesday, September 25, 2012 3:04 PM
> To: Yong Ding
> Cc: Chris Ball; linux-mmc@vger.kernel.org; Daniel Drake; Zhangfei Gao
> Subject: Re: [PATCH] mmc: remove MMC_CAP_NEEDS_POLL setting in
> sdhci_add_host
> 
> On Tue, Sep 25, 2012 at 02:34:07PM +0800, yongd wrote:
> > From: yongd 
> [...]
> > And the better one to decide whether we use polling or not should be
> > the host driver itself. Actually, some host driver has already been
> > like this. Eg, in drivers/mmc/host/Au1xmmc.c, polling will be enabled
> > only after the board-specific card detection can't be set up
> successfully.
> 
> I guess it's not that simple. If you remove this, you have to add
> appropriate CAP_NEEDS_POLL for these drivers:
> 
>  linux/drivers/mmc/host$ git grep -l SDHCI_QUIRK_BROKEN_CARD_DETECTION
> | xargs grep -L NEEDS_POLL
>  sdhci-esdhc-imx.c
>  sdhci-of-esdhc.c
>  sdhci-pci.c
>  sdhci-pxav2.c
>  sdhci-pxav3.c
>  sdhci-s3c.c
> 
> > Change-Id: I27774488a7b9191d7bc39699fd7d62ee21bbf157
> > Signed-off-by: yongd 
> > ---
> >  drivers/mmc/host/sdhci.c |4 
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 0e15c79..900d5f4 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -2840,10 +2840,6 @@ int sdhci_add_host(struct sdhci_host *host)
> > if (caps[0] & SDHCI_CAN_DO_HISPD)
> > mmc->caps |= MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED;
> >
> > -   if ((host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) &&
> > -   !(host->mmc->caps & MMC_CAP_NONREMOVABLE))
> > -   mmc->caps |= MMC_CAP_NEEDS_POLL;
> > -
> > /* If vqmmc regulator and no 1.8V signalling, then there's no UHS
> */
> > host->vqmmc = regulator_get(mmc_dev(mmc), "vqmmc");
> > if (IS_ERR(host->vqmmc)) {
> > --
> > 1.7.9.5
N�r��yb�X��ǧv�^�)޺{.n�+{��g"��^n�r���z���h�&���G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�

RE: [PATCH v2] mmc:sdio:guarantee setting card data bus width as 4-bit

2012-06-06 Thread Yong Ding
Chris,
Thanks, and very glad to hear this:-)

Best Wishes,

Yong Ding
Operating Systems Engineering,
Application Processor Systems Engineering

-Original Message-
From: Chris Ball [mailto:c...@laptop.org] 
Sent: Wednesday, June 06, 2012 9:24 PM
To: Yong Ding
Cc: linux-mmc@vger.kernel.org; Philip Rakity; o...@wizery.com; 
linus.wall...@linaro.org; ulf.hans...@stericsson.com; 
linux-ker...@vger.kernel.org
Subject: Re: [PATCH v2] mmc:sdio:guarantee setting card data bus width as 4-bit

Hi,

On Tue, May 15 2012, yongd wrote:
>   SDIO_CCCR_IF[1:0] in SDIO card is used for card data bus width
> setting as below,
>  00b: 1-bit bus
>  01b: Reserved
>  10b: 4-bit bus
>  11b: 8-bit bus(only for embedded SDIO)
>   And sdio_enable_wide is for setting data bus width as 4-bit. But
> currently, it 1stly read the register, 2ndly OR' 1b with SDIO_CCCR_IF[1],
> and then write it back.
>   As we can see, such way is based on such assumption that the
> SDIO_CCCR_IF[0] is always 0. Apparently, this is not right.
>
> Signed-off-by: yongd 
> ---
>  drivers/mmc/core/sdio.c  |7 +++
>  include/linux/mmc/sdio.h |2 ++
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 13d0e95..7b528cf 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -218,6 +218,13 @@ static int sdio_enable_wide(struct mmc_card *card)
>   if (ret)
>   return ret;
>  
> + if ((ctrl & SDIO_BUS_WIDTH_MASK)
> + == SDIO_BUS_WIDTH_RESERVED)
> + printk(KERN_WARNING"%s: SDIO_CCCR_IF is invalid: 0x%02X\n",
> + mmc_hostname(card->host), ctrl);
> +
> + /* set as 4-bit bus width */
> + ctrl &= ~SDIO_BUS_WIDTH_MASK;
>   ctrl |= SDIO_BUS_WIDTH_4BIT;
>  
>   ret = mmc_io_rw_direct(card, 1, 0, SDIO_CCCR_IF, ctrl, NULL);
> diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
> index c9fe66c..17446d3 100644
> --- a/include/linux/mmc/sdio.h
> +++ b/include/linux/mmc/sdio.h
> @@ -98,7 +98,9 @@
>  
>  #define SDIO_CCCR_IF 0x07/* bus interface controls */
>  
> +#define  SDIO_BUS_WIDTH_MASK 0x03/* data bus width setting */
>  #define  SDIO_BUS_WIDTH_1BIT 0x00
> +#define  SDIO_BUS_WIDTH_RESERVED 0x01
>  #define  SDIO_BUS_WIDTH_4BIT 0x02
>  #define  SDIO_BUS_ECSI   0x20/* Enable continuous SPI 
> interrupt */
>  #define  SDIO_BUS_SCSI   0x40/* Support continuous SPI 
> interrupt */

Thanks, pushed to mmc-next for 3.5.

- Chris.
-- 
Chris Ball  <http://printf.net/>
One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [PATCH v2] mmc:sdio:guarantee setting card data bus width as 4-bit

2012-05-17 Thread Yong Ding
Chris,
Thanks. And my full name is Yong Ding.

Best Wishes,

Yong Ding
Operating Systems Engineering,
Application Processor Systems Engineering

-Original Message-
From: Chris Ball [mailto:c...@laptop.org] 
Sent: Thursday, May 17, 2012 8:38 PM
To: Yong Ding
Cc: linux-mmc@vger.kernel.org; Philip Rakity; o...@wizery.com; 
linus.wall...@linaro.org; ulf.hans...@stericsson.com; 
linux-ker...@vger.kernel.org
Subject: Re: [PATCH v2] mmc:sdio:guarantee setting card data bus width as 4-bit

Hi,

On Tue, May 15 2012, yongd wrote:
>   SDIO_CCCR_IF[1:0] in SDIO card is used for card data bus width
> setting as below,
>  00b: 1-bit bus
>  01b: Reserved
>  10b: 4-bit bus
>  11b: 8-bit bus(only for embedded SDIO)
>   And sdio_enable_wide is for setting data bus width as 4-bit. But
> currently, it 1stly read the register, 2ndly OR' 1b with SDIO_CCCR_IF[1],
> and then write it back.
>   As we can see, such way is based on such assumption that the
> SDIO_CCCR_IF[0] is always 0. Apparently, this is not right.
>
> Signed-off-by: yongd 

Thanks.  Can we have your full name, for the copyright record?

- Chris.
-- 
Chris Ball  <http://printf.net/>
One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [PATCH] mmc:sdio:guarantee setting card data bus width as 4-bit

2012-05-14 Thread Yong Ding
Philip,
Thanks for your comment. And have sent out "[PATCH v2] mmc:sdio:guarantee 
setting card data bus width as 4-bit " which uses the below printk.

printk(KERN_WARNING("%s: SDIO_CCCR_IF is invalid: 0x%02X\n", 
mmc_hostname(card->host), ctrl);


Best Wishes,

Yong Ding
Operating Systems Engineering,
Application Processor Systems Engineering


-Original Message-
From: Philip Rakity 
Sent: Monday, May 14, 2012 10:09 PM
To: Yong Ding
Cc: c...@laptop.org; linux-mmc@vger.kernel.org; o...@wizery.com; 
linus.wall...@linaro.org; ulf.hans...@stericsson.com; 
linux-ker...@vger.kernel.org
Subject: Re: [PATCH] mmc:sdio:guarantee setting card data bus width as 4-bit


ACK

but see below for printk change


On May 14, 2012, at 1:35 AM, yongd wrote:

>  SDIO_CCCR_IF[1:0] in SDIO card is used for card data bus width
> setting as below,
> 00b: 1-bit bus
> 01b: Reserved
> 10b: 4-bit bus
> 11b: 8-bit bus(only for embedded SDIO)
>  And sdio_enable_wide is for setting data bus width as 4-bit. But
> currently, it 1stly read the register, 2ndly OR' 1b with SDIO_CCCR_IF[1],
> and then write it back.
>  As we can see, such way is based on such assumption that the
> SDIO_CCCR_IF[0] is always 0. Apparently, this is not right.
> 
> Signed-off-by: yongd 
> ---
> drivers/mmc/core/sdio.c  |6 ++
> include/linux/mmc/sdio.h |2 ++
> 2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 13d0e95..6b6f902 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -218,6 +218,12 @@ static int sdio_enable_wide(struct mmc_card *card)
>   if (ret)
>   return ret;
> 
> + if ((ctrl & SDIO_BUS_WIDTH_MASK)
> + == SDIO_BUS_WIDTH_RESERVED)
> + printk(KERN_WARNING"SDIO_CCCR_IF is invalid: %d\n", ctrl);
printk(KERN_WARNING("%s: SDIO_CCCR_IF is invalid: %02X\n", 
mmc_hostname(card->host), ctrl);

please double check mmc_hostname argument.

> +
> + /* set as 4-bit bus width */
> + ctrl &= ~SDIO_BUS_WIDTH_MASK;
>   ctrl |= SDIO_BUS_WIDTH_4BIT;
> 
>   ret = mmc_io_rw_direct(card, 1, 0, SDIO_CCCR_IF, ctrl, NULL);
> diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
> index c9fe66c..17446d3 100644
> --- a/include/linux/mmc/sdio.h
> +++ b/include/linux/mmc/sdio.h
> @@ -98,7 +98,9 @@
> 
> #define SDIO_CCCR_IF  0x07/* bus interface controls */
> 
> +#define  SDIO_BUS_WIDTH_MASK 0x03/* data bus width setting */
> #define  SDIO_BUS_WIDTH_1BIT  0x00
> +#define  SDIO_BUS_WIDTH_RESERVED 0x01
> #define  SDIO_BUS_WIDTH_4BIT  0x02
> #define  SDIO_BUS_ECSI0x20/* Enable continuous SPI 
> interrupt */
> #define  SDIO_BUS_SCSI0x40/* Support continuous SPI 
> interrupt */
> -- 
> 1.7.9.5
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [PATCH] mmc:sdio:retry CMD52/53 when error happens

2012-05-14 Thread Yong Ding
Hi Ulf Hansson,
Thanks for your comments.

Such retry mechanism is a simple CMD-level or sdio-core-level retry which takes 
advantage of the mmc core internal retry. There is not any side effect, but 
with it, even those SDIO function driver's without its own retrying mechanism 
can benefit in some conditions.

Besides, for CMD52, it can be used during SDIO card initialization/detection 
phase. At that time, SDIO function driver hasn't started to work. Of course we 
can probably encounter CMD52 error due to some reasons(eg, board-level 
schematic bad behavior), and such retry can be a potential workaround. 
Actually, we've encountered exactly such case in our development.


Best Wishes,

Yong Ding
Operating Systems Engineering,
Application Processor Systems Engineering

-Original Message-
From: Ulf Hansson [mailto:ulf.hans...@stericsson.com] 
Sent: Monday, May 14, 2012 8:58 PM
To: Yong Ding
Cc: c...@laptop.org; linux-mmc@vger.kernel.org; 
stefan.xk.nils...@stericsson.com; linus.wall...@linaro.org; svenk...@ti.com; 
linux-ker...@vger.kernel.org
Subject: Re: [PATCH] mmc:sdio:retry CMD52/53 when error happens

Hi Yongd,

 From my perspective I don't think this patch is wanted.

A retry mechanism is likely very much hw-SDIO device dependent. Upper 
layers (SDIO func driver) is thus the only software that is able to 
handle retries correctly.

Kind regards
Ulf Hansson


On 05/14/2012 10:39 AM, yongd wrote:
>Set sdio CMD52/53's retry time as MMC_CMD_RETRIES. As a result,
> sdio might benefit from core-internal CMD retry mechanism when
> some errors happen(CRC, etc).
>
> Signed-off-by: yongd
> ---
>   drivers/mmc/core/sdio_ops.c |3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
> index d29e206..884c27e 100644
> --- a/drivers/mmc/core/sdio_ops.c
> +++ b/drivers/mmc/core/sdio_ops.c
> @@ -86,7 +86,7 @@ static int mmc_io_rw_direct_host(struct mmc_host *host, int 
> write, unsigned fn,
>   cmd.arg |= in;
>   cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_AC;
>
> - err = mmc_wait_for_cmd(host,&cmd, 0);
> + err = mmc_wait_for_cmd(host,&cmd, MMC_CMD_RETRIES);
>   if (err)
>   return err;
>
> @@ -147,6 +147,7 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, 
> unsigned fn,
>   else
>   cmd.arg |= 0x0800 | blocks; /* block mode */
>   cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC;
> + cmd.retries = MMC_CMD_RETRIES;
>
>   data.blksz = blksz;
>   /* Code in host drivers/fwk assumes that "blocks" always is>=1 */

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html