| Message ID | 4d34a0a70902200400s252f48ddvfd6e0d83e91fa291@mail.gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Friday 20 February 2009, Kim Kyuwon wrote: > +static void omap_hsmmc_init(struct mmc_omap_host *host) > +{ > +Â Â Â Â Â Â Â u32 hctl, capa, value; > + > +Â Â Â Â Â Â Â /* Only MMC1 supports 3.0V */ > +Â Â Â Â Â Â Â if (host->id == OMAP_MMC1_DEVID) { > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â hctl = SDVS30; Shouldn't it be remembering what voltage it was using, and then restore that, instead of always making MMC1 restart at a 3.0V level? That's pretty awkward to test unless you have a 1.8V-capable card in MMC1... Somewhat related: I think the PBIAS register updates should be moved out of mach-omap2/mmc-twl4030.c into this driver. They're needed no matter what flavor regulator is used to with MMC1 voltage over 1.8V, and it's a bit odd to split the state machine for 1.8V -vs- 3.0V I/O voltages the way it's now done. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Sat, Feb 21, 2009 at 6:11 AM, David Brownell <david-b@pacbell.net> wrote: > On Friday 20 February 2009, Kim Kyuwon wrote: >> +static void omap_hsmmc_init(struct mmc_omap_host *host) >> +{ >> + u32 hctl, capa, value; >> + >> + /* Only MMC1 supports 3.0V */ >> + if (host->id == OMAP_MMC1_DEVID) { >> + hctl = SDVS30; > > Shouldn't it be remembering what voltage it was using, > and then restore that, instead of always making MMC1 > restart at a 3.0V level? That's pretty awkward to test > unless you have a 1.8V-capable card in MMC1... You are somewhat right, thank you. But remebering what voltage it was using doesn't feasible to me, because the card can be changed while in 'Sleep' state. I should have inserted a function that detect the right voltage after intializing. I will resend the patch later.
ext Kim Kyuwon wrote: > Hi, > > On Sat, Feb 21, 2009 at 6:11 AM, David Brownell <david-b@pacbell.net> wrote: >> On Friday 20 February 2009, Kim Kyuwon wrote: >>> +static void omap_hsmmc_init(struct mmc_omap_host *host) >>> +{ >>> + u32 hctl, capa, value; >>> + >>> + /* Only MMC1 supports 3.0V */ >>> + if (host->id == OMAP_MMC1_DEVID) { >>> + hctl = SDVS30; >> Shouldn't it be remembering what voltage it was using, >> and then restore that, instead of always making MMC1 >> restart at a 3.0V level? That's pretty awkward to test >> unless you have a 1.8V-capable card in MMC1... > > You are somewhat right, thank you. > But remebering what voltage it was using doesn't feasible to me, > because the card can be changed while in 'Sleep' state. I should have > inserted a function that detect the right voltage after intializing. I > will resend the patch later. Doesn't it already do that? Can you explain more? Although I have not tested it, I very much doubt dual-voltage cards work. That is because VMMC1_185V is zero, which has the side-effect of turning the regulator off (see arch/arm/mach-omap2/mmc-twl4030.c) -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Feb 23, 2009 at 5:04 PM, Adrian Hunter <ext-adrian.hunter@nokia.com> wrote: > ext Kim Kyuwon wrote: >> Hi, >> >> On Sat, Feb 21, 2009 at 6:11 AM, David Brownell <david-b@pacbell.net> wrote: >>> On Friday 20 February 2009, Kim Kyuwon wrote: >>>> +static void omap_hsmmc_init(struct mmc_omap_host *host) >>>> +{ >>>> + u32 hctl, capa, value; >>>> + >>>> + /* Only MMC1 supports 3.0V */ >>>> + if (host->id == OMAP_MMC1_DEVID) { >>>> + hctl = SDVS30; >>> Shouldn't it be remembering what voltage it was using, >>> and then restore that, instead of always making MMC1 >>> restart at a 3.0V level? That's pretty awkward to test >>> unless you have a 1.8V-capable card in MMC1... >> >> You are somewhat right, thank you. >> But remebering what voltage it was using doesn't feasible to me, >> because the card can be changed while in 'Sleep' state. I should have >> inserted a function that detect the right voltage after intializing. I >> will resend the patch later. > > Doesn't it already do that? Can you explain more? > > Although I have not tested it, I very much doubt > dual-voltage cards work. That is because VMMC1_185V > is zero, which has the side-effect of turning the > regulator off (see arch/arm/mach-omap2/mmc-twl4030.c) It's also to difficult to test in our H/W since it's configured only support 3.0V. How about to separate it two phases, first fix the mmc suspend/resume works again, and then verify dual voltage if there are these hardware How to you think? Thank you, Kyungmin Park -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 23 February 2009, Kyungmin Park wrote: > > Although I have not tested it, I very much doubt > > dual-voltage cards work. Â That is because VMMC1_185V > > is zero, which has the side-effect of turning the > > regulator off (see arch/arm/mach-omap2/mmc-twl4030.c) Right, looks like a longstanding bug in that glue. So IMO, no rush to fix for 2.6.29-rc ... > It's also to difficult to test in our H/W since it's configured only > support 3.0V. Easily tested with a Beagle or SDP though, if you have a 1.8V or dual-voltage card (maybe RS-MMC, as for an N770 tablet). > How about to separate it two phases, first fix the mmc suspend/resume > works again, and then verify dual voltage if there are these hardware Well, two patches for sure; I don't know that the order will matter. I just sent a patch fixing that code in the current OMAP git tree, which has some fixes that are scheduled to merge for 2.6.30-rc. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 23 February 2009, Adrian Hunter wrote: > Although I have not tested it, I very much doubt > dual-voltage cards work. Â That is because VMMC1_185V > is zero, which has the side-effect of turning the > regulator off (see arch/arm/mach-omap2/mmc-twl4030.c) And a second reason to know they don't quite work ... in the file drivers/mmc/host/omap_hsmmc.c, omap_mmc_set_ios() sets the voltage for MMC_POWER_OFF (0) or MMC_POWER_UP (1_, which gives the initial setting -- e.g. 3.15 Volts, so it can enumerate at the high range. But after enumerating the card at that voltage, checking the OCR values, and concluding that the slot and card can both run at 1.85V ... the MMC_POWER_ON (2) code is used. But the driver completely ignores it ... the low voltage (more power-efficient!) voltage range never kicks in. It'd be nice to have a nice unambiguous set_voltage() request from the MMC core. The set_ios() thing has always been confusing. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 10 Mar 2009 19:33:50 -0800 David Brownell <david-b@pacbell.net> wrote: > > It'd be nice to have a nice unambiguous set_voltage() > request from the MMC core. The set_ios() thing has > always been confusing. > set_ios() should be taken out back. But someone has to have the time to reimplement things. :/
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index 56363c5..5a73fa6 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -55,6 +55,7 @@ #define VS30 (1 << 25) #define SDVS18 (0x5 << 9) #define SDVS30 (0x6 << 9) +#define SDVS_MASK 0x00000E00 #define SDVSCLR 0xFFFFF1FF #define SDVSDET 0x00000400 #define AUTOIDLE 0x1 @@ -861,6 +862,34 @@ static int omap_hsmmc_get_ro(struct mmc_host *mmc) return pdata->slots[0].get_ro(host->dev, 0); } +static void omap_hsmmc_init(struct mmc_omap_host *host) +{ + u32 hctl, capa, value; + + /* Only MMC1 supports 3.0V */ + if (host->id == OMAP_MMC1_DEVID) { + hctl = SDVS30; + capa = VS30 | VS18; + } else { + hctl = SDVS18; + capa = VS18; + } + + value = OMAP_HSMMC_READ(host->base, HCTL) & ~SDVS_MASK; + OMAP_HSMMC_WRITE(host->base, HCTL, value | hctl); + + value = OMAP_HSMMC_READ(host->base, CAPA); + OMAP_HSMMC_WRITE(host->base, CAPA, value | capa); + + /* Set the controller to AUTO IDLE mode */ + value = OMAP_HSMMC_READ(host->base, SYSCONFIG); + OMAP_HSMMC_WRITE(host->base, SYSCONFIG, value | AUTOIDLE); + + /* Set SD bus power bit */ + value = OMAP_HSMMC_READ(host->base, HCTL); + OMAP_HSMMC_WRITE(host->base, HCTL, value | SDBP); +} + static struct mmc_host_ops mmc_omap_ops = { .request = omap_mmc_request, .set_ios = omap_mmc_set_ios, @@ -876,7 +905,6 @@ static int __init omap_mmc_probe(struct platform_device *pdev) struct mmc_omap_host *host = NULL; struct resource *res; int ret = 0, irq; - u32 hctl, capa; if (pdata == NULL) {
Most registers lose its state when the processor wakes up from sleep state. Thus registers should be initialized, when the processor wakes up. However the current hsmmc 'resume' function doesn't consider this issue and finally makes deadlock. So this patch fixes this problem. Signed-off-by: Kim Kyuwon <chammoru@gmail.com> --- drivers/mmc/host/omap_hsmmc.c | 155 +++++++++++++++++++++-------------------- 1 files changed, 79 insertions(+), 76 deletions(-) dev_err(&pdev->dev, "Platform Data is missing\n"); @@ -981,28 +1009,7 @@ static int __init omap_mmc_probe(struct platform_device *pdev) if (pdata->slots[host->slot_id].wires >= 4) mmc->caps |= MMC_CAP_4_BIT_DATA; - /* Only MMC1 supports 3.0V */ - if (host->id == OMAP_MMC1_DEVID) { - hctl = SDVS30; - capa = VS30 | VS18; - } else { - hctl = SDVS18; - capa = VS18; - } - - OMAP_HSMMC_WRITE(host->base, HCTL, - OMAP_HSMMC_READ(host->base, HCTL) | hctl); - - OMAP_HSMMC_WRITE(host->base, CAPA, - OMAP_HSMMC_READ(host->base, CAPA) | capa); - - /* Set the controller to AUTO IDLE mode */ - OMAP_HSMMC_WRITE(host->base, SYSCONFIG, - OMAP_HSMMC_READ(host->base, SYSCONFIG) | AUTOIDLE); - - /* Set SD bus power bit */ - OMAP_HSMMC_WRITE(host->base, HCTL, - OMAP_HSMMC_READ(host->base, HCTL) | SDBP); + omap_hsmmc_init(host); /* Request IRQ for MMC operations */ ret = request_irq(host->irq, mmc_omap_irq, IRQF_DISABLED, @@ -1127,41 +1134,38 @@ static int omap_mmc_suspend(struct platform_device *pdev, pm_message_t state) if (host && host->suspended) return 0; - if (host) { - ret = mmc_suspend_host(host->mmc, state); - if (ret == 0) { - host->suspended = 1; - - OMAP_HSMMC_WRITE(host->base, ISE, 0); - OMAP_HSMMC_WRITE(host->base, IE, 0); + ret = mmc_suspend_host(host->mmc, state); + if (ret == 0) { + host->suspended = 1; - if (host->pdata->suspend) { - ret = host->pdata->suspend(&pdev->dev, - host->slot_id); - if (ret) - dev_dbg(mmc_dev(host->mmc), - "Unable to handle MMC board" - " level suspend\n"); - } + OMAP_HSMMC_WRITE(host->base, ISE, 0); + OMAP_HSMMC_WRITE(host->base, IE, 0); - if (!(OMAP_HSMMC_READ(host->base, HCTL) & SDVSDET)) { - OMAP_HSMMC_WRITE(host->base, HCTL, - OMAP_HSMMC_READ(host->base, HCTL) - & SDVSCLR); - OMAP_HSMMC_WRITE(host->base, HCTL, - OMAP_HSMMC_READ(host->base, HCTL) - | SDVS30); - OMAP_HSMMC_WRITE(host->base, HCTL, - OMAP_HSMMC_READ(host->base, HCTL) - | SDBP); - } + if (host->pdata->suspend) { + ret = host->pdata->suspend(&pdev->dev, host->slot_id); + if (ret) + dev_dbg(mmc_dev(host->mmc), + "Unable to handle MMC board" + " level suspend\n"); + } - clk_disable(host->fclk); - clk_disable(host->iclk); - clk_disable(host->dbclk); + if (!(OMAP_HSMMC_READ(host->base, HCTL) & SDVSDET)) { + OMAP_HSMMC_WRITE(host->base, HCTL, + OMAP_HSMMC_READ(host->base, HCTL) + & SDVSCLR); + OMAP_HSMMC_WRITE(host->base, HCTL, + OMAP_HSMMC_READ(host->base, HCTL) + | SDVS30); + OMAP_HSMMC_WRITE(host->base, HCTL, + OMAP_HSMMC_READ(host->base, HCTL) + | SDBP); } + clk_disable(host->fclk); + clk_disable(host->iclk); + clk_disable(host->dbclk); } + return ret; } @@ -1174,36 +1178,35 @@ static int omap_mmc_resume(struct platform_device *pdev) if (host && !host->suspended) return 0; - if (host) { + ret = clk_enable(host->fclk); + if (ret) + goto clk_en_err; - ret = clk_enable(host->fclk); - if (ret) - goto clk_en_err; - - ret = clk_enable(host->iclk); - if (ret) { - clk_disable(host->fclk); - clk_put(host->fclk); - goto clk_en_err; - } + ret = clk_enable(host->iclk); + if (ret) { + clk_disable(host->fclk); + clk_put(host->fclk); + goto clk_en_err; + } - if (clk_enable(host->dbclk) != 0) - dev_dbg(mmc_dev(host->mmc), - "Enabling debounce clk failed\n"); + if (clk_enable(host->dbclk) != 0) + dev_dbg(mmc_dev(host->mmc), + "Enabling debounce clk failed\n"); - if (host->pdata->resume) { - ret = host->pdata->resume(&pdev->dev, host->slot_id); - if (ret) - dev_dbg(mmc_dev(host->mmc), - "Unmask interrupt failed\n"); - } + omap_hsmmc_init(host); - /* Notify the core to resume the host */ - ret = mmc_resume_host(host->mmc); - if (ret == 0) - host->suspended = 0; + if (host->pdata->resume) { + ret = host->pdata->resume(&pdev->dev, host->slot_id); + if (ret) + dev_dbg(mmc_dev(host->mmc), + "Unmask interrupt failed\n"); } + /* Notify the core to resume the host */ + ret = mmc_resume_host(host->mmc); + if (ret == 0) + host->suspended = 0; + return ret; clk_en_err: