| Message ID | 1466487339-13211-3-git-send-email-jh80.chung@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2016/6/21 13:35, Jaehoon Chung wrote: > If there ins no vqmmc, voltage should not be changed. > Then it nees to maintain the previous status for USH_REG register. > To use the standard spec, it should be returned error when voltage > can't switch. > > Note: > Dwmmc controller will not make any exception case.(like exynos.) > If card isn't working well after appling this patch, it needs to check > the regulator side. > > Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> > --- > Changelog V2: > -Fix the typo > > drivers/mmc/host/dw_mmc.c | 40 ++++++++++++++++++++++------------------ > 1 file changed, 22 insertions(+), 18 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index ec3f0a8..56c48fb 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -1404,33 +1404,37 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios) > struct dw_mci_slot *slot = mmc_priv(mmc); > struct dw_mci *host = slot->host; > const struct dw_mci_drv_data *drv_data = host->drv_data; > - u32 uhs; > + u32 uhs = 0; > u32 v18 = SDMMC_UHS_18V << slot->id; > int ret; > > if (drv_data && drv_data->switch_voltage) > return drv_data->switch_voltage(mmc, ios); > > - /* > - * Program the voltage. Note that some instances of dw_mmc may use > - * the UHS_REG for this. For other instances (like exynos) the UHS_REG > - * does no harm but you need to set the regulator directly. Try both. > - */ > + if (IS_ERR(mmc->supply.vqmmc)) { > + dev_dbg(&mmc->class_dev, > + "vqmmc not available.(Skip the switching voltage)\n"); > + return -EINVAL; > + } > + > + ret = mmc_regulator_set_vqmmc(mmc, ios); > + if (ret) { > + dev_err(&mmc->class_dev, > + "Regulator set error %d - %s V\n", > + ret, uhs & v18 ? "1.8" : "3.3"); > + return ret; > + } > + > uhs = mci_readl(host, UHS_REG); > - if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) uhs is still zero now, so maybe we can remove this switch branch and only need a simple checking like this: if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) uhs |= v18; otherwise, it looks good to me. > + switch (ios->signal_voltage) { > + case MMC_SIGNAL_VOLTAGE_330: > uhs &= ~v18; > - else > + break; > + case MMC_SIGNAL_VOLTAGE_180: > uhs |= v18; > - > - if (!IS_ERR(mmc->supply.vqmmc)) { > - ret = mmc_regulator_set_vqmmc(mmc, ios); > - > - if (ret) { > - dev_dbg(&mmc->class_dev, > - "Regulator set error %d - %s V\n", > - ret, uhs & v18 ? "1.8" : "3.3"); > - return ret; > - } > + break; > + default: > + uhs &= ~v18; > } > mci_writel(host, UHS_REG, uhs); > >
On 06/21/2016 05:13 PM, Shawn Lin wrote: > On 2016/6/21 13:35, Jaehoon Chung wrote: >> If there ins no vqmmc, voltage should not be changed. >> Then it nees to maintain the previous status for USH_REG register. >> To use the standard spec, it should be returned error when voltage >> can't switch. >> >> Note: >> Dwmmc controller will not make any exception case.(like exynos.) >> If card isn't working well after appling this patch, it needs to check >> the regulator side. >> >> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> >> --- >> Changelog V2: >> -Fix the typo >> >> drivers/mmc/host/dw_mmc.c | 40 ++++++++++++++++++++++------------------ >> 1 file changed, 22 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index ec3f0a8..56c48fb 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -1404,33 +1404,37 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios) >> struct dw_mci_slot *slot = mmc_priv(mmc); >> struct dw_mci *host = slot->host; >> const struct dw_mci_drv_data *drv_data = host->drv_data; >> - u32 uhs; >> + u32 uhs = 0; >> u32 v18 = SDMMC_UHS_18V << slot->id; >> int ret; >> >> if (drv_data && drv_data->switch_voltage) >> return drv_data->switch_voltage(mmc, ios); >> >> - /* >> - * Program the voltage. Note that some instances of dw_mmc may use >> - * the UHS_REG for this. For other instances (like exynos) the UHS_REG >> - * does no harm but you need to set the regulator directly. Try both. >> - */ >> + if (IS_ERR(mmc->supply.vqmmc)) { >> + dev_dbg(&mmc->class_dev, >> + "vqmmc not available.(Skip the switching voltage)\n"); >> + return -EINVAL; >> + } >> + >> + ret = mmc_regulator_set_vqmmc(mmc, ios); >> + if (ret) { >> + dev_err(&mmc->class_dev, >> + "Regulator set error %d - %s V\n", >> + ret, uhs & v18 ? "1.8" : "3.3"); >> + return ret; >> + } >> + >> uhs = mci_readl(host, UHS_REG); >> - if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) > > uhs is still zero now, so maybe we can remove this switch branch and > only need a simple checking like this: > > if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) > uhs |= v18; It's not enough to set the correct value. So i will maintain this patch.. In future, i will add the MMC_SIGNAL_VOLTAGE_120 for USH_REG_EXT register. At that time, switch statement should be better than "if" statement.. :) Anyway, I applied this patch on my repository. If you have any objection, i will accept your opinion. :) Best Regards, Jaehoon Chung > > otherwise, it looks good to me. > >> + switch (ios->signal_voltage) { >> + case MMC_SIGNAL_VOLTAGE_330: >> uhs &= ~v18; >> - else >> + break; >> + case MMC_SIGNAL_VOLTAGE_180: >> uhs |= v18; >> - >> - if (!IS_ERR(mmc->supply.vqmmc)) { >> - ret = mmc_regulator_set_vqmmc(mmc, ios); >> - >> - if (ret) { >> - dev_dbg(&mmc->class_dev, >> - "Regulator set error %d - %s V\n", >> - ret, uhs & v18 ? "1.8" : "3.3"); >> - return ret; >> - } >> + break; >> + default: >> + uhs &= ~v18; >> } >> mci_writel(host, UHS_REG, uhs); >> >> > > -- 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/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index ec3f0a8..56c48fb 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -1404,33 +1404,37 @@ static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios) struct dw_mci_slot *slot = mmc_priv(mmc); struct dw_mci *host = slot->host; const struct dw_mci_drv_data *drv_data = host->drv_data; - u32 uhs; + u32 uhs = 0; u32 v18 = SDMMC_UHS_18V << slot->id; int ret; if (drv_data && drv_data->switch_voltage) return drv_data->switch_voltage(mmc, ios); - /* - * Program the voltage. Note that some instances of dw_mmc may use - * the UHS_REG for this. For other instances (like exynos) the UHS_REG - * does no harm but you need to set the regulator directly. Try both. - */ + if (IS_ERR(mmc->supply.vqmmc)) { + dev_dbg(&mmc->class_dev, + "vqmmc not available.(Skip the switching voltage)\n"); + return -EINVAL; + } + + ret = mmc_regulator_set_vqmmc(mmc, ios); + if (ret) { + dev_err(&mmc->class_dev, + "Regulator set error %d - %s V\n", + ret, uhs & v18 ? "1.8" : "3.3"); + return ret; + } + uhs = mci_readl(host, UHS_REG); - if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) + switch (ios->signal_voltage) { + case MMC_SIGNAL_VOLTAGE_330: uhs &= ~v18; - else + break; + case MMC_SIGNAL_VOLTAGE_180: uhs |= v18; - - if (!IS_ERR(mmc->supply.vqmmc)) { - ret = mmc_regulator_set_vqmmc(mmc, ios); - - if (ret) { - dev_dbg(&mmc->class_dev, - "Regulator set error %d - %s V\n", - ret, uhs & v18 ? "1.8" : "3.3"); - return ret; - } + break; + default: + uhs &= ~v18; } mci_writel(host, UHS_REG, uhs);
If there ins no vqmmc, voltage should not be changed. Then it nees to maintain the previous status for USH_REG register. To use the standard spec, it should be returned error when voltage can't switch. Note: Dwmmc controller will not make any exception case.(like exynos.) If card isn't working well after appling this patch, it needs to check the regulator side. Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> --- Changelog V2: -Fix the typo drivers/mmc/host/dw_mmc.c | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-)