[PATCH 1/5 v2] MMC/core: Make sure the power is up, when detect the card

[PATCH 1/5 v2] MMC/core: Make sure the power is up, when detect the card

[r66093]

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

Before running get_cd() recall function to detect whether the card is
present, must make sure the power is up.

Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
CC: Chris Ball <cjb@laptop.org>
---
changes for v2:
	- add the CC

 drivers/mmc/core/core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5278ffb..a08e6b1 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2066,8 +2066,10 @@ void mmc_rescan(struct work_struct *work)
 	 */
 	mmc_bus_put(host);
 
+	mmc_power_up(host);
 	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
 		goto out;
+	mmc_power_off(host);
 
 	mmc_claim_host(host);
 	for (i = 0; i < ARRAY_SIZE(freqs); i++) {
-- 
1.7.5.4

[PATCH 2/5 v2] 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: Chris Ball <cjb@laptop.org>
---
changes for v2:
	- add the CC

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index a08e6b1..2d40c04 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1253,7 +1253,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.5.4

Re: [PATCH 2/5 v2] MMC/core: Add f_min to mmc_power_on()

[Hein_Tibosch]

Hi Jerry,

On 12/6/2011 5:18 PM, 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: Chris Ball <cjb@laptop.org>
> ---
> changes for v2:
> 	- add the CC
>
>  drivers/mmc/core/core.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index a08e6b1..2d40c04 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1253,7 +1253,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;

Are you sure f_min can have a value of zero?
It should have been set mmc_rescan_try_freq(), when trying a list
of frequencies: 400000, 300000, 200000, 100000

>  
>  	host->ios.power_mode = MMC_POWER_ON;
>  	mmc_set_ios(host);

Re: [PATCH 2/5 v2] MMC/core: Add f_min to mmc_power_on()

[Hein_Tibosch]

On 12/6/2011 8:13 PM, Hein_Tibosch wrote:
> On 12/6/2011 5:18 PM, r66093@freescale.com wrote:
>> When f_init is zero, the SDHC can't work correctly. So f_min will replace
>> f_init, when f_init is zero.
>>
>> -	host->ios.clock = host->f_init;
>> +	if (host->f_init)
>> +		host->ios.clock = host->f_init;
>> +	else
>> +		host->ios.clock = host->f_min;
> Are you sure f_min can have a value of zero?
> It should have been set mmc_rescan_try_freq(), when trying a list
> of frequencies: 400000, 300000, 200000, 100000

Oops: I meant: Are you sure "f_init" can have a value of zero?

Hein

RE: [PATCH 2/5 v2] MMC/core: Add f_min to mmc_power_on()

[Huang Changming-R66093]

> -----Original Message-----
> From: Hein_Tibosch [mailto:hein_tibosch@yahoo.es]
> Sent: Tuesday, December 06, 2011 8:13 PM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Huang Changming-R66093; Chris Ball
> Subject: Re: [PATCH 2/5 v2] MMC/core: Add f_min to mmc_power_on()
> 
> Hi Jerry,
> 
> On 12/6/2011 5:18 PM, 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: Chris Ball <cjb@laptop.org>
> > ---
> > changes for v2:
> > 	- add the CC
> >
> >  drivers/mmc/core/core.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
> > a08e6b1..2d40c04 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -1253,7 +1253,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;
> 
> Are you sure f_min can have a value of zero?
> It should have been set mmc_rescan_try_freq(), when trying a list of
> frequencies: 400000, 300000, 200000, 100000
> 
But, mmc_power_up is called not only by mmc_rescan_try_freq, so the value of f_init may be zero.

Re: [PATCH 2/5 v2] MMC/core: Add f_min to mmc_power_on()

[Hein_Tibosch]

Hi Jerry,
On 12/7/2011 10:23 AM, Huang Changming-R66093 wrote:
>
>> -----Original Message-----
>> From: Hein_Tibosch [mailto:hein_tibosch@yahoo.es]
>> Sent: Tuesday, December 06, 2011 8:13 PM
>> To: Huang Changming-R66093
>> Cc: linux-mmc@vger.kernel.org; Huang Changming-R66093; Chris Ball
>> Subject: Re: [PATCH 2/5 v2] MMC/core: Add f_min to mmc_power_on()
>>
>> <cut>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
>>> a08e6b1..2d40c04 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -1253,7 +1253,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;
>> Are you sure f_min can have a value of zero?
>> It should have been set mmc_rescan_try_freq(), when trying a list of
>> frequencies: 400000, 300000, 200000, 100000
>>
> But, mmc_power_up is called not only by mmc_rescan_try_freq, so the value of f_init may be zero.

Ok, I see: mmc_power_up() used to be called only after mmc_rescan_try_freq(),
but now you want to call it earlier, because get_cd() *might* need it,
even if get_cd is not defined...

Still I wonder whether it's good to power-up each and every card:

+	mmc_power_up(host);
 	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
 		goto out;
+	mmc_power_off(host);

before initialization starts. Remember earlier complaints that f_min is
extremely low on some platforms and therefor f_init was introduced.

Hein

RE: [PATCH 2/5 v2] MMC/core: Add f_min to mmc_power_on()

[Huang Changming-R66093]

> -----Original Message-----
> From: Hein_Tibosch [mailto:hein_tibosch@yahoo.es]
> Sent: Wednesday, December 07, 2011 11:37 AM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; ulf.hansson@stericsson.com; Chris Ball
> Subject: Re: [PATCH 2/5 v2] MMC/core: Add f_min to mmc_power_on()
> 
> Hi Jerry,
> On 12/7/2011 10:23 AM, Huang Changming-R66093 wrote:
> >
> >> -----Original Message-----
> >> From: Hein_Tibosch [mailto:hein_tibosch@yahoo.es]
> >> Sent: Tuesday, December 06, 2011 8:13 PM
> >> To: Huang Changming-R66093
> >> Cc: linux-mmc@vger.kernel.org; Huang Changming-R66093; Chris Ball
> >> Subject: Re: [PATCH 2/5 v2] MMC/core: Add f_min to mmc_power_on()
> >>
> >> <cut>
> >>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
> >>> a08e6b1..2d40c04 100644
> >>> --- a/drivers/mmc/core/core.c
> >>> +++ b/drivers/mmc/core/core.c
> >>> @@ -1253,7 +1253,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;
> >> Are you sure f_min can have a value of zero?
> >> It should have been set mmc_rescan_try_freq(), when trying a list of
> >> frequencies: 400000, 300000, 200000, 100000
> >>
> > But, mmc_power_up is called not only by mmc_rescan_try_freq, so the
> value of f_init may be zero.
> 
> Ok, I see: mmc_power_up() used to be called only after
> mmc_rescan_try_freq(), but now you want to call it earlier, because
> get_cd() *might* need it, even if get_cd is not defined...
> 
> Still I wonder whether it's good to power-up each and every card:
> 
> +	mmc_power_up(host);
>  	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
>  		goto out;
> +	mmc_power_off(host);
> 
> before initialization starts. Remember earlier complaints that f_min is
> extremely low on some platforms and therefor f_init was introduced.
You can view the v3 patch I posted, only when get_cd is defined, this function is called. 
Some controller may use GPIO to detect the card preset or absent, card can be detected without power on. But the other controller has the pin to detect the card, so we need the power on to update the register.

Function mmc_power_restore_host and mmc_resume_host have called mmc_power_up, these two function are used for PM.
If the controller support get_cd and there is not card present, mmc_rescan_try_freq  can't be called, then the f_init can't be initized. If SDHC enter sleep and resume, mmc_power_up will be called by the two function I mentioned. In this case, the f_init can be zero.
So my code first check if the f_init is zero, if yes, f_min will be used. F_min is always initialized during the SDHC controller. 

Re: [PATCH 1/5 v2] MMC/core: Make sure the power is up, when detect the card

[Hein_Tibosch]

Hi Jerry,

On 12/6/2011 5:18 PM, r66093@freescale.com wrote:
> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
>
> Before running get_cd() recall function to detect whether the card is
> present, must make sure the power is up.
>
> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> CC: Chris Ball <cjb@laptop.org>
> ---
> changes for v2:
> 	- add the CC
>
>  drivers/mmc/core/core.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 5278ffb..a08e6b1 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2066,8 +2066,10 @@ void mmc_rescan(struct work_struct *work)
>  	 */
>  	mmc_bus_put(host);
>  
> +	mmc_power_up(host);

If get_cd is not defined, a power-up for sure isn't necessary:

>  	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
>  		goto out;

Did you intent to leave it powered-up in case get_cd() fails?

> +	mmc_power_off(host);
>  
>  	mmc_claim_host(host);
>  	for (i = 0; i < ARRAY_SIZE(freqs); i++) {

RE: [PATCH 1/5 v2] MMC/core: Make sure the power is up, when detect the card

[Huang Changming-R66093]

> -----Original Message-----
> From: Hein_Tibosch [mailto:hein_tibosch@yahoo.es]
> Sent: Tuesday, December 06, 2011 8:13 PM
> To: Huang Changming-R66093
> Cc: linux-mmc@vger.kernel.org; Huang Changming-R66093; Chris Ball
> Subject: Re: [PATCH 1/5 v2] MMC/core: Make sure the power is up, when
> detect the card
> 
> Hi Jerry,
> 
> On 12/6/2011 5:18 PM, r66093@freescale.com wrote:
> > From: Jerry Huang <Chang-Ming.Huang@freescale.com>
> >
> > Before running get_cd() recall function to detect whether the card is
> > present, must make sure the power is up.
> >
> > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > CC: Chris Ball <cjb@laptop.org>
> > ---
> > changes for v2:
> > 	- add the CC
> >
> >  drivers/mmc/core/core.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
> > 5278ffb..a08e6b1 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2066,8 +2066,10 @@ void mmc_rescan(struct work_struct *work)
> >  	 */
> >  	mmc_bus_put(host);
> >
> > +	mmc_power_up(host);
> 
> If get_cd is not defined, a power-up for sure isn't necessary:
> 
> >  	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
> >  		goto out;
> 
> Did you intent to leave it powered-up in case get_cd() fails?
> 
En, it is one bug, I will fix it