| Message ID | 1281373098.32137.26.camel@black |
|---|---|
| State | Rejected |
| Delegated to: | Leann Ogasawara |
| Headers | show |
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
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 > > > >
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
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.
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
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.
(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
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);