Patchwork [Maverick] UBUNTU - ARM: Reverting patch that break mmc init

login
register
mail settings
Submitter Mathieu Poirier
Date Aug. 9, 2010, 4:58 p.m.
Message ID <1281373098.32137.26.camel@black>
Download mbox | patch
Permalink /patch/61293/
State Rejected
Delegated to: Leann Ogasawara
Headers show

Comments

Mathieu Poirier - Aug. 9, 2010, 4:58 p.m.
>From 7b666254b3b52989749916094629f3efc8d50a7b Mon Sep 17 00:00:00 2001
From: Mathieu J. Poirier <mathieu.poirier@canonical.com>
Date: Mon, 9 Aug 2010 12:49:51 -0400
Subject: [PATCH] UBUNTU - ARM: Reverting patch that break mmc init

This patch breaks mmc initialisation when the following flags
are set:

CONFIG_PREEMPT_VOLUNTARY
CONFIG_CPU_FREQ
CONFIG_CPU_IDLE
CONFIG_SND_SOC.

The power management subsystem will skip the card initialisation
when power_mode is equal to "MMC_POWER_OFF", something that was
done in the second portion of the patch.

BugLink: https://bugs/launchpad.net/bugs/591941

Signed-off-by: Mathieu Poirier <mathieu.poirier@canonical.com>

Revert "omap_hsmmc: Ensure regulator enable / disable are paired"

This reverts commit 6da20c89af64b75302399369a90b9d50c1a87665.
---
 drivers/mmc/host/omap_hsmmc.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
Tim Gardner - Aug. 9, 2010, 11 p.m.
On 08/09/2010 09:58 AM, Mathieu Poirier wrote:
> Ensure regulator enable

What does TI think about this? Its not been reverted upstream. Is there 
a better way to implement the functionality in the original patch?

rtg
Leann Ogasawara - Aug. 9, 2010, 11:11 p.m.
On Mon, 2010-08-09 at 10:58 -0600, Mathieu Poirier wrote:
> >From 7b666254b3b52989749916094629f3efc8d50a7b Mon Sep 17 00:00:00 2001
> From: Mathieu J. Poirier <mathieu.poirier@canonical.com>
> Date: Mon, 9 Aug 2010 12:49:51 -0400
> Subject: [PATCH] UBUNTU - ARM: Reverting patch that break mmc init
> 
> This patch breaks mmc initialisation when the following flags
> are set:
> 
> CONFIG_PREEMPT_VOLUNTARY
> CONFIG_CPU_FREQ
> CONFIG_CPU_IDLE
> CONFIG_SND_SOC.
> 
> The power management subsystem will skip the card initialisation
> when power_mode is equal to "MMC_POWER_OFF", something that was
> done in the second portion of the patch.

Is it necessary to revert the entire patch?  It seems the first portion
fixes up the reference counts by properly pairing regulator
enables/disables.  It doesn't seem correct to revert this.  Is this
being reverted upstream?  Has there been any upstream discussion about
this?

Thanks,
Leann

> BugLink: https://bugs/launchpad.net/bugs/591941
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@canonical.com>
> 
> Revert "omap_hsmmc: Ensure regulator enable / disable are paired"
> 
> This reverts commit 6da20c89af64b75302399369a90b9d50c1a87665.
> ---
>  drivers/mmc/host/omap_hsmmc.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index b032828..1c37d0d 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -297,8 +297,11 @@ static int omap_hsmmc_23_set_power(struct device *dev, int slot, int power_on,
>  				ret = mmc_regulator_set_ocr(host->vcc, 0);
>  		}
>  	} else {
> -		if (host->vcc_aux)
> -			ret = regulator_disable(host->vcc_aux);
> +		if (host->vcc_aux) {
> +			ret = regulator_is_enabled(host->vcc_aux);
> +			if (ret > 0)
> +				ret = regulator_disable(host->vcc_aux);
> +		}
>  		if (ret == 0)
>  			ret = mmc_regulator_set_ocr(host->vcc, 0);
>  	}
> @@ -2010,7 +2013,7 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>  	host->slot_id	= 0;
>  	host->mapbase	= res->start;
>  	host->base	= ioremap(host->mapbase, SZ_4K);
> -	host->power_mode = MMC_POWER_OFF;
> +	host->power_mode = -1;
>  
>  	platform_set_drvdata(pdev, host);
>  	INIT_WORK(&host->mmc_carddetect_work, omap_hsmmc_detect);
> -- 
> 1.7.0.4
> 
> 
> 
>
Amit Kucheria - Aug. 10, 2010, 5:58 a.m.
On 10 Aug 09, Mathieu Poirier wrote:
> >From 7b666254b3b52989749916094629f3efc8d50a7b Mon Sep 17 00:00:00 2001
> From: Mathieu J. Poirier <mathieu.poirier@canonical.com>
> Date: Mon, 9 Aug 2010 12:49:51 -0400
> Subject: [PATCH] UBUNTU - ARM: Reverting patch that break mmc init
> 
> This patch breaks mmc initialisation when the following flags
> are set:
> 
> CONFIG_PREEMPT_VOLUNTARY
> CONFIG_CPU_FREQ
> CONFIG_CPU_IDLE
> CONFIG_SND_SOC.
> 
> The power management subsystem will skip the card initialisation
> when power_mode is equal to "MMC_POWER_OFF", something that was
> done in the second portion of the patch.
> 
> BugLink: https://bugs/launchpad.net/bugs/591941
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@canonical.com>
> 
> Revert "omap_hsmmc: Ensure regulator enable / disable are paired"
> 
> This reverts commit 6da20c89af64b75302399369a90b9d50c1a87665.
> ---
>  drivers/mmc/host/omap_hsmmc.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index b032828..1c37d0d 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -297,8 +297,11 @@ static int omap_hsmmc_23_set_power(struct device *dev, int slot, int power_on,
>  				ret = mmc_regulator_set_ocr(host->vcc, 0);
>  		}
>  	} else {
> -		if (host->vcc_aux)
> -			ret = regulator_disable(host->vcc_aux);
> +		if (host->vcc_aux) {
> +			ret = regulator_is_enabled(host->vcc_aux);
> +			if (ret > 0)
> +				ret = regulator_disable(host->vcc_aux);
> +		}
>  		if (ret == 0)
>  			ret = mmc_regulator_set_ocr(host->vcc, 0);
>  	}
> @@ -2010,7 +2013,7 @@ static int __init omap_hsmmc_probe(struct platform_device *pdev)
>  	host->slot_id	= 0;
>  	host->mapbase	= res->start;
>  	host->base	= ioremap(host->mapbase, SZ_4K);
> -	host->power_mode = MMC_POWER_OFF;
> +	host->power_mode = -1;
>  
>  	platform_set_drvdata(pdev, host);
>  	INIT_WORK(&host->mmc_carddetect_work, omap_hsmmc_detect);
> -- 
> 1.7.0.4


IMHO, this seems to address the symptom rather than the real problem. Have
you talked to Adrian about this issue with PREEMPT_VOLUNTARY set?

/Amit
Mathieu Poirier - Aug. 10, 2010, 2:28 p.m.
On Mon, 2010-08-09 at 16:00 -0700, Tim Gardner wrote:
> On 08/09/2010 09:58 AM, Mathieu Poirier wrote:
> > Ensure regulator enable
> 
> What does TI think about this? Its not been reverted upstream. Is there 
> a better way to implement the functionality in the original patch?
> 
> rtg

All,

The patch hasn't been reverted upstream because no one upstream is
seeing this issue.  Only Ubuntu compiles with all 4 flags enabled.  I
tried to discuss my findings on #beagle but only received rude comments
like "ubuntu and their crazy ways".

I thought about leaving the patch intact and reverting only the
initialization of "host->power_mode" but decided against it for
consistency reason.  Fixing only the second part of the patch was my
preferred choice.

I am well aware that reverting the patch is not the real solution.  On
the other hand there is no denying the feature is broken and probably
would not have been accepted upstream had anyone been running their
system with the flags enabled.

I submitted the patch to allow the mobile team to move forward. The
system doesn't boot if the mmc card doesn't get initialized properly,
something that blocks them in all their omap3 endeavours.  

All that being said, I can submit another patch that will only address
the "power_mode" initialization and leave the pairing of regulator
enables/disables intact.  I will also get in touch with the implementer
of the patch and expose the problem to him.

Question/comments are welcomed.

Mathieu.
Tim Gardner - Aug. 10, 2010, 3:59 p.m.
On 08/10/2010 07:28 AM, Mathieu Poirier wrote:
> On Mon, 2010-08-09 at 16:00 -0700, Tim Gardner wrote:
>> On 08/09/2010 09:58 AM, Mathieu Poirier wrote:
>>> Ensure regulator enable
>>
>> What does TI think about this? Its not been reverted upstream. Is there
>> a better way to implement the functionality in the original patch?
>>
>> rtg
>
> All,
>
> The patch hasn't been reverted upstream because no one upstream is
> seeing this issue.  Only Ubuntu compiles with all 4 flags enabled.  I
> tried to discuss my findings on #beagle but only received rude comments
> like "ubuntu and their crazy ways".
>
> I thought about leaving the patch intact and reverting only the
> initialization of "host->power_mode" but decided against it for
> consistency reason.  Fixing only the second part of the patch was my
> preferred choice.
>
> I am well aware that reverting the patch is not the real solution.  On
> the other hand there is no denying the feature is broken and probably
> would not have been accepted upstream had anyone been running their
> system with the flags enabled.
>
> I submitted the patch to allow the mobile team to move forward. The
> system doesn't boot if the mmc card doesn't get initialized properly,
> something that blocks them in all their omap3 endeavours.
>
> All that being said, I can submit another patch that will only address
> the "power_mode" initialization and leave the pairing of regulator
> enables/disables intact.  I will also get in touch with the implementer
> of the patch and expose the problem to him.
>
> Question/comments are welcomed.
>
> Mathieu.
>

I like that approach better. As for the rude comments, you might point 
out that the 4 flags we are using _are_ provided by upstream and there 
are _no_ rules against their simultaneous use.

rtg
Amit Kucheria - Aug. 11, 2010, 7:25 a.m.
On 10 Aug 10, Mathieu Poirier wrote:
> On Mon, 2010-08-09 at 16:00 -0700, Tim Gardner wrote:
> > On 08/09/2010 09:58 AM, Mathieu Poirier wrote:
> > > Ensure regulator enable
> > 
> > What does TI think about this? Its not been reverted upstream. Is there 
> > a better way to implement the functionality in the original patch?
> > 
> > rtg
> 
> All,
> 
> The patch hasn't been reverted upstream because no one upstream is
> seeing this issue.  Only Ubuntu compiles with all 4 flags enabled.  I
> tried to discuss my findings on #beagle but only received rude comments
> like "ubuntu and their crazy ways".

The #beagle community has a few juveniles in their midst. I strongly suggest
_not_ using them as the first stop for discussions and discussing this on
linux-omap mailing list which is the upstream for the omap kernels.

> I thought about leaving the patch intact and reverting only the
> initialization of "host->power_mode" but decided against it for
> consistency reason.  Fixing only the second part of the patch was my
> preferred choice.
> 
> I am well aware that reverting the patch is not the real solution.  On
> the other hand there is no denying the feature is broken and probably
> would not have been accepted upstream had anyone been running their
> system with the flags enabled.
> 
> I submitted the patch to allow the mobile team to move forward. The
> system doesn't boot if the mmc card doesn't get initialized properly,
> something that blocks them in all their omap3 endeavours.  
> 
> All that being said, I can submit another patch that will only address
> the "power_mode" initialization and leave the pairing of regulator
> enables/disables intact.  I will also get in touch with the implementer
> of the patch and expose the problem to him.
Amit Kucheria - Aug. 11, 2010, 7:36 a.m.
(Argh. Sent it before I was finished)

On 10 Aug 10, Mathieu Poirier wrote:
> On Mon, 2010-08-09 at 16:00 -0700, Tim Gardner wrote:
> > On 08/09/2010 09:58 AM, Mathieu Poirier wrote:
> > > Ensure regulator enable
> > 
> > What does TI think about this? Its not been reverted upstream. Is there 
> > a better way to implement the functionality in the original patch?
> > 
> > rtg
> 
> All,
> 
> The patch hasn't been reverted upstream because no one upstream is
> seeing this issue.  Only Ubuntu compiles with all 4 flags enabled.  I
> tried to discuss my findings on #beagle but only received rude comments
> like "ubuntu and their crazy ways".
 
The #beagle community has a few juveniles in their midst. I strongly suggest
_not_ using them as the first stop for discussions and discussing this on
linux-omap mailing list which is the upstream for the omap kernels.
 
> I thought about leaving the patch intact and reverting only the
> initialization of "host->power_mode" but decided against it for
> consistency reason.  Fixing only the second part of the patch was my
> preferred choice.
> 
> I am well aware that reverting the patch is not the real solution.  On
> the other hand there is no denying the feature is broken and probably
> would not have been accepted upstream had anyone been running their
> system with the flags enabled.

Not completely true.

Nokia ships the N900 kernel with all those feature enabled, but AFAICT they
ship with CONFIG_PREEMPT (low-latency desktop) instead of VOLUNTARY. Have you
tried with that?

> I submitted the patch to allow the mobile team to move forward. The
> system doesn't boot if the mmc card doesn't get initialized properly,
> something that blocks them in all their omap3 endeavours.  

Please mention it in the patch description - something like "HACK: foo bar"
in that case. That way we can review it in the next cycle.

> All that being said, I can submit another patch that will only address
> the "power_mode" initialization and leave the pairing of regulator
> enables/disables intact.  I will also get in touch with the implementer
> of the patch and expose the problem to him.

Yes, please get in touch with Adrian Hunter about the patch. They've
probably never tested with PREEMPT_VOLUNTARY because N900 is not a desktop.

/Amit

Patch

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index b032828..1c37d0d 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -297,8 +297,11 @@  static int omap_hsmmc_23_set_power(struct device *dev, int slot, int power_on,
 				ret = mmc_regulator_set_ocr(host->vcc, 0);
 		}
 	} else {
-		if (host->vcc_aux)
-			ret = regulator_disable(host->vcc_aux);
+		if (host->vcc_aux) {
+			ret = regulator_is_enabled(host->vcc_aux);
+			if (ret > 0)
+				ret = regulator_disable(host->vcc_aux);
+		}
 		if (ret == 0)
 			ret = mmc_regulator_set_ocr(host->vcc, 0);
 	}
@@ -2010,7 +2013,7 @@  static int __init omap_hsmmc_probe(struct platform_device *pdev)
 	host->slot_id	= 0;
 	host->mapbase	= res->start;
 	host->base	= ioremap(host->mapbase, SZ_4K);
-	host->power_mode = MMC_POWER_OFF;
+	host->power_mode = -1;
 
 	platform_set_drvdata(pdev, host);
 	INIT_WORK(&host->mmc_carddetect_work, omap_hsmmc_detect);