| Message ID | 56F3E9EB.3090207@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 24 Mar 2016 15:21:47 +0200 Adrian Hunter <adrian.hunter@intel.com> wrote: > On 24/03/16 15:11, Adrian Hunter wrote: > > On 24/03/16 10:42, Krzysztof Kozlowski wrote: > >> On 24.03.2016 17:24, Jisheng Zhang wrote: > >>> Hi, > >>> > >>> On Thu, 24 Mar 2016 17:09:27 +0900 Jaehoon Chung wrote: > >>> > >>>> Hi, > >>>> > >>>> On 03/24/2016 04:58 PM, Jisheng Zhang wrote: > >>>>> Hi, > >>>>> > >>>>> On Thu, 24 Mar 2016 16:28:56 +0900 Krzysztof Kozlowski wrote: > >>>>> > >>>>>> Hi, > >>>>>> > >>>>>> After 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD via > >>>>>> external regulator") On Trats2 board I see warnings for invalid VDD > >>>>>> value (2.8V): > >>>>>> > >>>>>> [ 3.119656] ------------[ cut here ]------------ > >>>>>> [ 3.119666] WARNING: CPU: 3 PID: 90 at > >>>>>> ../drivers/mmc/host/sdhci.c:1234 sdhci_do_set_ios+0x4cc/0x5e0 > >>>>>> [ 3.119669] mmc0: Invalid vdd 0x10 > >>>>> > >>>>> Per my understanding, the wrong vdd indicates a wrong ocr, what's the voltage of > >>>>> this host's vmmc regulator? > >>>> > >>>> As i know, it's fixed-voltage with gpio on trats2. It's 2.8V. > >>>> I didn't check this entirely..need to check ocr value. > >>>> > >>> > >>> I may know the reason. the vmmc is 2.8v, then mmc_regulator_get_supply() convert > >>> the value to a ocr as 0x10. The key here is that the 2.8v is invalid in SDHCI > >>> case and isn't accepted by current sdhci driver. > >> > >> Yeah, I already wrote that. It is the part of the warning and my email. > >> Our regulator is fixed at 2.8 which is 0x10. :) > >> > >>> I dunno the elegant solution to handle this case, let's wait for sdhci maintainers > >>> idea. > >> > >> Hmm... > > > > I haven't tested it, but what about this: > > And now with checkpatch complaints fixed: > > From: Adrian Hunter <adrian.hunter@intel.com> > Date: Thu, 24 Mar 2016 14:29:24 +0200 > Subject: [PATCH V2] mmc: sdhci: Fix regression setting power on Trats2 board > > Several commits relating to setting power have been introducing > problems by putting driver-specific rules into generic SDHCI code. > > Fix by adding a 'set_power' callback and restoring the default > behaviour prior to commit 918f4cbd4340 ("mmc: sdhci: restore > behavior when setting VDD via external regulator"). The desired > behaviour of that commit is gotten by having sdhci-pxav3 provide > its own set_power callback. > > Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > Fixes: 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD...) > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> Reviewed-by: Jisheng Zhang <jszhang@marvell.com> Tested-by: Jisheng Zhang <jszhang@marvell.com> The only problem is atmel's sdhc host also need similar own .set_power Ludovic knows details. Thanks, Jisheng > Cc: stable@vger.kernel.org # v4.5+ > --- > drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++ > drivers/mmc/host/sdhci.c | 38 ++++++++++++++++++++++++++++++-------- > drivers/mmc/host/sdhci.h | 4 ++++ > 3 files changed, 51 insertions(+), 8 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c > index f5edf9d3a18a..673d1e8446a5 100644 > --- a/drivers/mmc/host/sdhci-pxav3.c > +++ b/drivers/mmc/host/sdhci-pxav3.c > @@ -307,8 +307,25 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > __func__, uhs, ctrl_2); > } > > +static void pxav3_set_power(struct sdhci_host *host, unsigned char mode, > + unsigned short vdd) > +{ > + struct mmc_host *mmc = host->mmc; > + u8 pwr = host->pwr; > + > + sdhci_set_power(host, mode, vdd); > + > + if (host->pwr == pwr) > + return; > + > + spin_unlock_irq(&host->lock); > + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); > + spin_lock_irq(&host->lock); > +} > + > static const struct sdhci_ops pxav3_sdhci_ops = { > .set_clock = sdhci_set_clock, > + .set_power = pxav3_set_power, > .platform_send_init_74_clocks = pxav3_gen_init_74_clocks, > .get_max_clock = sdhci_pltfm_clk_get_max_clock, > .set_bus_width = sdhci_set_bus_width, > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index add9fdfd1d8f..282257222121 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -1269,10 +1269,24 @@ clock_set: > } > EXPORT_SYMBOL_GPL(sdhci_set_clock); > > -static void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > - unsigned short vdd) > +static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode, > + unsigned short vdd) > { > struct mmc_host *mmc = host->mmc; > + > + spin_unlock_irq(&host->lock); > + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); > + spin_lock_irq(&host->lock); > + > + if (mode != MMC_POWER_OFF) > + sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL); > + else > + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL); > +} > + > +void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > + unsigned short vdd) > +{ > u8 pwr = 0; > > if (mode != MMC_POWER_OFF) { > @@ -1335,12 +1349,20 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER) > mdelay(10); > } > +} > +EXPORT_SYMBOL_GPL(sdhci_set_power); > > - if (!IS_ERR(mmc->supply.vmmc)) { > - spin_unlock_irq(&host->lock); > - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); > - spin_lock_irq(&host->lock); > - } > +static void __sdhci_set_power(struct sdhci_host *host, unsigned char mode, > + unsigned short vdd) > +{ > + struct mmc_host *mmc = host->mmc; > + > + if (host->ops->set_power) > + host->ops->set_power(host, mode, vdd); > + else if (!IS_ERR(mmc->supply.vmmc)) > + sdhci_set_power_reg(host, mode, vdd); > + else > + sdhci_set_power(host, mode, vdd); > } > > /*****************************************************************************\ > @@ -1490,7 +1512,7 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios) > } > } > > - sdhci_set_power(host, ios->power_mode, ios->vdd); > + __sdhci_set_power(host, ios->power_mode, ios->vdd); > > if (host->ops->platform_send_init_74_clocks) > host->ops->platform_send_init_74_clocks(host, ios->power_mode); > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h > index 0115e9907bf8..033d72b5bbd5 100644 > --- a/drivers/mmc/host/sdhci.h > +++ b/drivers/mmc/host/sdhci.h > @@ -529,6 +529,8 @@ struct sdhci_ops { > #endif > > void (*set_clock)(struct sdhci_host *host, unsigned int clock); > + void (*set_power)(struct sdhci_host *host, unsigned char mode, > + unsigned short vdd); > > int (*enable_dma)(struct sdhci_host *host); > unsigned int (*get_max_clock)(struct sdhci_host *host); > @@ -660,6 +662,8 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host) > } > > void sdhci_set_clock(struct sdhci_host *host, unsigned int clock); > +void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > + unsigned short vdd); > void sdhci_set_bus_width(struct sdhci_host *host, int width); > void sdhci_reset(struct sdhci_host *host, u8 mask); > void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing); -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 24, 2016 at 09:45:56PM +0800, Jisheng Zhang wrote: > On Thu, 24 Mar 2016 15:21:47 +0200 > Adrian Hunter <adrian.hunter@intel.com> wrote: > > > On 24/03/16 15:11, Adrian Hunter wrote: > > > On 24/03/16 10:42, Krzysztof Kozlowski wrote: > > >> On 24.03.2016 17:24, Jisheng Zhang wrote: > > >>> Hi, > > >>> > > >>> On Thu, 24 Mar 2016 17:09:27 +0900 Jaehoon Chung wrote: > > >>> > > >>>> Hi, > > >>>> > > >>>> On 03/24/2016 04:58 PM, Jisheng Zhang wrote: > > >>>>> Hi, > > >>>>> > > >>>>> On Thu, 24 Mar 2016 16:28:56 +0900 Krzysztof Kozlowski wrote: > > >>>>> > > >>>>>> Hi, > > >>>>>> > > >>>>>> After 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD via > > >>>>>> external regulator") On Trats2 board I see warnings for invalid VDD > > >>>>>> value (2.8V): > > >>>>>> > > >>>>>> [ 3.119656] ------------[ cut here ]------------ > > >>>>>> [ 3.119666] WARNING: CPU: 3 PID: 90 at > > >>>>>> ../drivers/mmc/host/sdhci.c:1234 sdhci_do_set_ios+0x4cc/0x5e0 > > >>>>>> [ 3.119669] mmc0: Invalid vdd 0x10 > > >>>>> > > >>>>> Per my understanding, the wrong vdd indicates a wrong ocr, what's the voltage of > > >>>>> this host's vmmc regulator? > > >>>> > > >>>> As i know, it's fixed-voltage with gpio on trats2. It's 2.8V. > > >>>> I didn't check this entirely..need to check ocr value. > > >>>> > > >>> > > >>> I may know the reason. the vmmc is 2.8v, then mmc_regulator_get_supply() convert > > >>> the value to a ocr as 0x10. The key here is that the 2.8v is invalid in SDHCI > > >>> case and isn't accepted by current sdhci driver. > > >> > > >> Yeah, I already wrote that. It is the part of the warning and my email. > > >> Our regulator is fixed at 2.8 which is 0x10. :) > > >> > > >>> I dunno the elegant solution to handle this case, let's wait for sdhci maintainers > > >>> idea. > > >> > > >> Hmm... > > > > > > I haven't tested it, but what about this: > > > > And now with checkpatch complaints fixed: > > > > From: Adrian Hunter <adrian.hunter@intel.com> > > Date: Thu, 24 Mar 2016 14:29:24 +0200 > > Subject: [PATCH V2] mmc: sdhci: Fix regression setting power on Trats2 board > > > > Several commits relating to setting power have been introducing > > problems by putting driver-specific rules into generic SDHCI code. > > > > Fix by adding a 'set_power' callback and restoring the default > > behaviour prior to commit 918f4cbd4340 ("mmc: sdhci: restore > > behavior when setting VDD via external regulator"). The desired > > behaviour of that commit is gotten by having sdhci-pxav3 provide > > its own set_power callback. > > > > Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > Fixes: 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD...) > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > > Reviewed-by: Jisheng Zhang <jszhang@marvell.com> > Tested-by: Jisheng Zhang <jszhang@marvell.com> > > The only problem is atmel's sdhc host also need similar own .set_power > Ludovic knows details. It doesn't introduce regression on my side since I have removed regulators from my device tree. If I introduce then back, I'll need my own set_power callback. I am not in favour of this patch only because I have the feeling that sdhci_set_power_reg() is not conform to the specification. Tested-by: Ludovic Desroches <ludovic.desroches@atmel.com> > > Thanks, > Jisheng > > > Cc: stable@vger.kernel.org # v4.5+ > > --- > > drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++ > > drivers/mmc/host/sdhci.c | 38 ++++++++++++++++++++++++++++++-------- > > drivers/mmc/host/sdhci.h | 4 ++++ > > 3 files changed, 51 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c > > index f5edf9d3a18a..673d1e8446a5 100644 > > --- a/drivers/mmc/host/sdhci-pxav3.c > > +++ b/drivers/mmc/host/sdhci-pxav3.c > > @@ -307,8 +307,25 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) > > __func__, uhs, ctrl_2); > > } > > > > +static void pxav3_set_power(struct sdhci_host *host, unsigned char mode, > > + unsigned short vdd) > > +{ > > + struct mmc_host *mmc = host->mmc; > > + u8 pwr = host->pwr; > > + > > + sdhci_set_power(host, mode, vdd); > > + > > + if (host->pwr == pwr) > > + return; > > + > > + spin_unlock_irq(&host->lock); > > + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); > > + spin_lock_irq(&host->lock); > > +} > > + > > static const struct sdhci_ops pxav3_sdhci_ops = { > > .set_clock = sdhci_set_clock, > > + .set_power = pxav3_set_power, > > .platform_send_init_74_clocks = pxav3_gen_init_74_clocks, > > .get_max_clock = sdhci_pltfm_clk_get_max_clock, > > .set_bus_width = sdhci_set_bus_width, > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > index add9fdfd1d8f..282257222121 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -1269,10 +1269,24 @@ clock_set: > > } > > EXPORT_SYMBOL_GPL(sdhci_set_clock); > > > > -static void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > > - unsigned short vdd) > > +static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode, > > + unsigned short vdd) > > { > > struct mmc_host *mmc = host->mmc; > > + > > + spin_unlock_irq(&host->lock); > > + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); > > + spin_lock_irq(&host->lock); > > + > > + if (mode != MMC_POWER_OFF) > > + sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL); > > + else > > + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL); > > +} > > + > > +void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > > + unsigned short vdd) > > +{ > > u8 pwr = 0; > > > > if (mode != MMC_POWER_OFF) { > > @@ -1335,12 +1349,20 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > > if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER) > > mdelay(10); > > } > > +} > > +EXPORT_SYMBOL_GPL(sdhci_set_power); > > > > - if (!IS_ERR(mmc->supply.vmmc)) { > > - spin_unlock_irq(&host->lock); > > - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); > > - spin_lock_irq(&host->lock); > > - } > > +static void __sdhci_set_power(struct sdhci_host *host, unsigned char mode, > > + unsigned short vdd) > > +{ > > + struct mmc_host *mmc = host->mmc; > > + > > + if (host->ops->set_power) > > + host->ops->set_power(host, mode, vdd); > > + else if (!IS_ERR(mmc->supply.vmmc)) > > + sdhci_set_power_reg(host, mode, vdd); > > + else > > + sdhci_set_power(host, mode, vdd); > > } > > > > /*****************************************************************************\ > > @@ -1490,7 +1512,7 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios) > > } > > } > > > > - sdhci_set_power(host, ios->power_mode, ios->vdd); > > + __sdhci_set_power(host, ios->power_mode, ios->vdd); > > > > if (host->ops->platform_send_init_74_clocks) > > host->ops->platform_send_init_74_clocks(host, ios->power_mode); > > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h > > index 0115e9907bf8..033d72b5bbd5 100644 > > --- a/drivers/mmc/host/sdhci.h > > +++ b/drivers/mmc/host/sdhci.h > > @@ -529,6 +529,8 @@ struct sdhci_ops { > > #endif > > > > void (*set_clock)(struct sdhci_host *host, unsigned int clock); > > + void (*set_power)(struct sdhci_host *host, unsigned char mode, > > + unsigned short vdd); > > > > int (*enable_dma)(struct sdhci_host *host); > > unsigned int (*get_max_clock)(struct sdhci_host *host); > > @@ -660,6 +662,8 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host) > > } > > > > void sdhci_set_clock(struct sdhci_host *host, unsigned int clock); > > +void sdhci_set_power(struct sdhci_host *host, unsigned char mode, > > + unsigned short vdd); > > void sdhci_set_bus_width(struct sdhci_host *host, int width); > > void sdhci_reset(struct sdhci_host *host, u8 mask); > > void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing); > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24.03.2016 22:21, Adrian Hunter wrote: > On 24/03/16 15:11, Adrian Hunter wrote: >> On 24/03/16 10:42, Krzysztof Kozlowski wrote: >>> On 24.03.2016 17:24, Jisheng Zhang wrote: >>>> Hi, >>>> >>>> On Thu, 24 Mar 2016 17:09:27 +0900 Jaehoon Chung wrote: >>>> >>>>> Hi, >>>>> >>>>> On 03/24/2016 04:58 PM, Jisheng Zhang wrote: >>>>>> Hi, >>>>>> >>>>>> On Thu, 24 Mar 2016 16:28:56 +0900 Krzysztof Kozlowski wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> After 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD via >>>>>>> external regulator") On Trats2 board I see warnings for invalid VDD >>>>>>> value (2.8V): >>>>>>> >>>>>>> [ 3.119656] ------------[ cut here ]------------ >>>>>>> [ 3.119666] WARNING: CPU: 3 PID: 90 at >>>>>>> ../drivers/mmc/host/sdhci.c:1234 sdhci_do_set_ios+0x4cc/0x5e0 >>>>>>> [ 3.119669] mmc0: Invalid vdd 0x10 >>>>>> >>>>>> Per my understanding, the wrong vdd indicates a wrong ocr, what's the voltage of >>>>>> this host's vmmc regulator? >>>>> >>>>> As i know, it's fixed-voltage with gpio on trats2. It's 2.8V. >>>>> I didn't check this entirely..need to check ocr value. >>>>> >>>> >>>> I may know the reason. the vmmc is 2.8v, then mmc_regulator_get_supply() convert >>>> the value to a ocr as 0x10. The key here is that the 2.8v is invalid in SDHCI >>>> case and isn't accepted by current sdhci driver. >>> >>> Yeah, I already wrote that. It is the part of the warning and my email. >>> Our regulator is fixed at 2.8 which is 0x10. :) >>> >>>> I dunno the elegant solution to handle this case, let's wait for sdhci maintainers >>>> idea. >>> >>> Hmm... >> >> I haven't tested it, but what about this: > > And now with checkpatch complaints fixed: > > From: Adrian Hunter <adrian.hunter@intel.com> > Date: Thu, 24 Mar 2016 14:29:24 +0200 > Subject: [PATCH V2] mmc: sdhci: Fix regression setting power on Trats2 board > > Several commits relating to setting power have been introducing > problems by putting driver-specific rules into generic SDHCI code. > > Fix by adding a 'set_power' callback and restoring the default > behaviour prior to commit 918f4cbd4340 ("mmc: sdhci: restore > behavior when setting VDD via external regulator"). The desired s/behavior/behaviour/ > behaviour of that commit is gotten by having sdhci-pxav3 provide > its own set_power callback. > > Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > Fixes: 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD...) > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > Cc: stable@vger.kernel.org # v4.5+ > --- > drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++ > drivers/mmc/host/sdhci.c | 38 ++++++++++++++++++++++++++++++-------- > drivers/mmc/host/sdhci.h | 4 ++++ > 3 files changed, 51 insertions(+), 8 deletions(-) > Works for me (sdhci-s3c, Exynos4412 on Trats2 board): Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/03/16 09:07, Krzysztof Kozlowski wrote: > On 24.03.2016 22:21, Adrian Hunter wrote: >> On 24/03/16 15:11, Adrian Hunter wrote: >>> On 24/03/16 10:42, Krzysztof Kozlowski wrote: >>>> On 24.03.2016 17:24, Jisheng Zhang wrote: >>>>> Hi, >>>>> >>>>> On Thu, 24 Mar 2016 17:09:27 +0900 Jaehoon Chung wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> On 03/24/2016 04:58 PM, Jisheng Zhang wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On Thu, 24 Mar 2016 16:28:56 +0900 Krzysztof Kozlowski wrote: >>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> After 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD via >>>>>>>> external regulator") On Trats2 board I see warnings for invalid VDD >>>>>>>> value (2.8V): >>>>>>>> >>>>>>>> [ 3.119656] ------------[ cut here ]------------ >>>>>>>> [ 3.119666] WARNING: CPU: 3 PID: 90 at >>>>>>>> ../drivers/mmc/host/sdhci.c:1234 sdhci_do_set_ios+0x4cc/0x5e0 >>>>>>>> [ 3.119669] mmc0: Invalid vdd 0x10 >>>>>>> >>>>>>> Per my understanding, the wrong vdd indicates a wrong ocr, what's the voltage of >>>>>>> this host's vmmc regulator? >>>>>> >>>>>> As i know, it's fixed-voltage with gpio on trats2. It's 2.8V. >>>>>> I didn't check this entirely..need to check ocr value. >>>>>> >>>>> >>>>> I may know the reason. the vmmc is 2.8v, then mmc_regulator_get_supply() convert >>>>> the value to a ocr as 0x10. The key here is that the 2.8v is invalid in SDHCI >>>>> case and isn't accepted by current sdhci driver. >>>> >>>> Yeah, I already wrote that. It is the part of the warning and my email. >>>> Our regulator is fixed at 2.8 which is 0x10. :) >>>> >>>>> I dunno the elegant solution to handle this case, let's wait for sdhci maintainers >>>>> idea. >>>> >>>> Hmm... >>> >>> I haven't tested it, but what about this: >> >> And now with checkpatch complaints fixed: >> >> From: Adrian Hunter <adrian.hunter@intel.com> >> Date: Thu, 24 Mar 2016 14:29:24 +0200 >> Subject: [PATCH V2] mmc: sdhci: Fix regression setting power on Trats2 board >> >> Several commits relating to setting power have been introducing >> problems by putting driver-specific rules into generic SDHCI code. >> >> Fix by adding a 'set_power' callback and restoring the default >> behaviour prior to commit 918f4cbd4340 ("mmc: sdhci: restore >> behavior when setting VDD via external regulator"). The desired > > s/behavior/behaviour/ It seems checkpatch insists the commit message must be identical to the original, so I am leaving it that way. > >> behaviour of that commit is gotten by having sdhci-pxav3 provide >> its own set_power callback. >> >> Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> Fixes: 918f4cbd4340 ("mmc: sdhci: restore behavior when setting VDD...) >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >> Cc: stable@vger.kernel.org # v4.5+ >> --- >> drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++ >> drivers/mmc/host/sdhci.c | 38 ++++++++++++++++++++++++++++++-------- >> drivers/mmc/host/sdhci.h | 4 ++++ >> 3 files changed, 51 insertions(+), 8 deletions(-) >> > > Works for me (sdhci-s3c, Exynos4412 on Trats2 board): > Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> I made a V3 which has a couple of minor changes for pxav3_set_power() and consequently have not added the Reviewed/Tested-by Jisheng Zhang. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c index f5edf9d3a18a..673d1e8446a5 100644 --- a/drivers/mmc/host/sdhci-pxav3.c +++ b/drivers/mmc/host/sdhci-pxav3.c @@ -307,8 +307,25 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs) __func__, uhs, ctrl_2); } +static void pxav3_set_power(struct sdhci_host *host, unsigned char mode, + unsigned short vdd) +{ + struct mmc_host *mmc = host->mmc; + u8 pwr = host->pwr; + + sdhci_set_power(host, mode, vdd); + + if (host->pwr == pwr) + return; + + spin_unlock_irq(&host->lock); + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); + spin_lock_irq(&host->lock); +} + static const struct sdhci_ops pxav3_sdhci_ops = { .set_clock = sdhci_set_clock, + .set_power = pxav3_set_power, .platform_send_init_74_clocks = pxav3_gen_init_74_clocks, .get_max_clock = sdhci_pltfm_clk_get_max_clock, .set_bus_width = sdhci_set_bus_width, diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index add9fdfd1d8f..282257222121 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -1269,10 +1269,24 @@ clock_set: } EXPORT_SYMBOL_GPL(sdhci_set_clock); -static void sdhci_set_power(struct sdhci_host *host, unsigned char mode, - unsigned short vdd) +static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode, + unsigned short vdd) { struct mmc_host *mmc = host->mmc; + + spin_unlock_irq(&host->lock); + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); + spin_lock_irq(&host->lock); + + if (mode != MMC_POWER_OFF) + sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL); + else + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL); +} + +void sdhci_set_power(struct sdhci_host *host, unsigned char mode, + unsigned short vdd) +{ u8 pwr = 0; if (mode != MMC_POWER_OFF) { @@ -1335,12 +1349,20 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned char mode, if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER) mdelay(10); } +} +EXPORT_SYMBOL_GPL(sdhci_set_power); - if (!IS_ERR(mmc->supply.vmmc)) { - spin_unlock_irq(&host->lock); - mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd); - spin_lock_irq(&host->lock); - } +static void __sdhci_set_power(struct sdhci_host *host, unsigned char mode, + unsigned short vdd) +{ + struct mmc_host *mmc = host->mmc; + + if (host->ops->set_power) + host->ops->set_power(host, mode, vdd); + else if (!IS_ERR(mmc->supply.vmmc)) + sdhci_set_power_reg(host, mode, vdd); + else + sdhci_set_power(host, mode, vdd); } /*****************************************************************************\ @@ -1490,7 +1512,7 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios) } } - sdhci_set_power(host, ios->power_mode, ios->vdd); + __sdhci_set_power(host, ios->power_mode, ios->vdd); if (host->ops->platform_send_init_74_clocks) host->ops->platform_send_init_74_clocks(host, ios->power_mode); diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index 0115e9907bf8..033d72b5bbd5 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -529,6 +529,8 @@ struct sdhci_ops { #endif void (*set_clock)(struct sdhci_host *host, unsigned int clock); + void (*set_power)(struct sdhci_host *host, unsigned char mode, + unsigned short vdd); int (*enable_dma)(struct sdhci_host *host); unsigned int (*get_max_clock)(struct sdhci_host *host); @@ -660,6 +662,8 @@ static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host) } void sdhci_set_clock(struct sdhci_host *host, unsigned int clock); +void sdhci_set_power(struct sdhci_host *host, unsigned char mode, + unsigned short vdd); void sdhci_set_bus_width(struct sdhci_host *host, int width); void sdhci_reset(struct sdhci_host *host, u8 mask); void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);