On qcom SD host controllers voltage switching be done after the HW
is ready for it. The HW informs its readiness through power irq.
The voltage switching should happen only then.
Use the internal voltage switching and then control the voltage
switching using power irq.
Set the regulator load as well so that regulator can be configured
in LPM mode when in is not being used.
Co-developed-by: Asutosh Das <[email protected]>
Signed-off-by: Asutosh Das <[email protected]>
Co-developed-by: Vijay Viswanath <[email protected]>
Signed-off-by: Vijay Viswanath <[email protected]>
Co-developed-by: Veerabhadrarao Badiganti <[email protected]>
Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
---
drivers/mmc/host/sdhci-msm.c | 207 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 198 insertions(+), 9 deletions(-)
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 97758fa..6211ab4 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -36,7 +36,9 @@
#define CORE_PWRCTL_IO_LOW BIT(2)
#define CORE_PWRCTL_IO_HIGH BIT(3)
#define CORE_PWRCTL_BUS_SUCCESS BIT(0)
+#define CORE_PWRCTL_BUS_FAIL BIT(1)
#define CORE_PWRCTL_IO_SUCCESS BIT(2)
+#define CORE_PWRCTL_IO_FAIL BIT(3)
#define REQ_BUS_OFF BIT(0)
#define REQ_BUS_ON BIT(1)
#define REQ_IO_LOW BIT(2)
@@ -263,6 +265,8 @@ struct sdhci_msm_host {
bool use_cdr;
u32 transfer_mode;
bool updated_ddr_cfg;
+ u32 vqmmc_load;
+ bool vqmmc_enabled;
};
static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
@@ -1298,6 +1302,71 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
sdhci_msm_hs400(host, &mmc->ios);
}
+static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
+{
+ int ret;
+
+ if (IS_ERR(mmc->supply.vmmc))
+ return 0;
+
+ ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
+ if (ret)
+ dev_err(mmc_dev(mmc), "%s: vmmc set ocr with vdd=%d failed: %d\n",
+ mmc_hostname(mmc), mmc->ios.vdd, ret);
+
+ return ret;
+}
+
+static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
+ struct mmc_host *mmc, bool level)
+{
+ int load, ret;
+ struct mmc_ios ios;
+
+ if (IS_ERR(mmc->supply.vqmmc) ||
+ (mmc->ios.power_mode == MMC_POWER_UNDEFINED) ||
+ (msm_host->vqmmc_enabled == level))
+ return 0;
+
+ if (msm_host->vqmmc_load) {
+ load = level ? msm_host->vqmmc_load : 0;
+ ret = regulator_set_load(mmc->supply.vqmmc, load);
+ if (ret) {
+ dev_err(mmc_dev(mmc), "%s: vqmmc set load failed: %d\n",
+ mmc_hostname(mmc), ret);
+ goto out;
+ }
+ }
+
+ if (level) {
+ /* Set the IO voltage regulator to default voltage level */
+ if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
+ ios.signal_voltage = MMC_SIGNAL_VOLTAGE_330;
+ else if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
+ ios.signal_voltage = MMC_SIGNAL_VOLTAGE_180;
+
+ if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
+ ret = mmc_regulator_set_vqmmc(mmc, &ios);
+ if (ret < 0) {
+ dev_err(mmc_dev(mmc), "%s: vqmmc set volgate failed: %d\n",
+ mmc_hostname(mmc), ret);
+ goto out;
+ }
+ }
+ ret = regulator_enable(mmc->supply.vqmmc);
+ } else {
+ ret = regulator_disable(mmc->supply.vqmmc);
+ }
+
+ if (ret)
+ dev_err(mmc_dev(mmc), "%s: vqmm %sable failed: %d\n",
+ mmc_hostname(mmc), level ? "en":"dis", ret);
+ else
+ msm_host->vqmmc_enabled = level;
+out:
+ return ret;
+}
+
static inline void sdhci_msm_init_pwr_irq_wait(struct sdhci_msm_host *msm_host)
{
init_waitqueue_head(&msm_host->pwr_irq_wait);
@@ -1401,8 +1470,9 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
{
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+ struct mmc_host *mmc = host->mmc;
u32 irq_status, irq_ack = 0;
- int retry = 10;
+ int retry = 10, ret;
u32 pwr_state = 0, io_level = 0;
u32 config;
const struct sdhci_msm_offset *msm_offset = msm_host->offset;
@@ -1440,21 +1510,42 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
if (irq_status & CORE_PWRCTL_BUS_ON) {
pwr_state = REQ_BUS_ON;
io_level = REQ_IO_HIGH;
- irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
}
if (irq_status & CORE_PWRCTL_BUS_OFF) {
pwr_state = REQ_BUS_OFF;
io_level = REQ_IO_LOW;
- irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
}
+
+ if (pwr_state) {
+ ret = sdhci_msm_set_vmmc(mmc);
+ if (!ret)
+ ret = sdhci_msm_set_vqmmc(msm_host, mmc,
+ pwr_state & REQ_BUS_ON);
+ if (!ret)
+ irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
+ else
+ irq_ack |= CORE_PWRCTL_BUS_FAIL;
+ }
+
/* Handle IO LOW/HIGH */
- if (irq_status & CORE_PWRCTL_IO_LOW) {
+ if (irq_status & CORE_PWRCTL_IO_LOW)
io_level = REQ_IO_LOW;
- irq_ack |= CORE_PWRCTL_IO_SUCCESS;
- }
- if (irq_status & CORE_PWRCTL_IO_HIGH) {
+
+ if (irq_status & CORE_PWRCTL_IO_HIGH)
io_level = REQ_IO_HIGH;
+
+ if (io_level)
irq_ack |= CORE_PWRCTL_IO_SUCCESS;
+
+ if (io_level && !IS_ERR(mmc->supply.vqmmc) && !pwr_state) {
+ ret = mmc_regulator_set_vqmmc(mmc, &mmc->ios);
+ if (ret < 0) {
+ dev_err(mmc_dev(mmc), "%s: IO_level setting failed(%d). signal_voltage: %d, vdd: %d irq_status: 0x%08x\n",
+ mmc_hostname(mmc), ret,
+ mmc->ios.signal_voltage, mmc->ios.vdd,
+ irq_status);
+ irq_ack |= CORE_PWRCTL_IO_FAIL;
+ }
}
/*
@@ -1503,7 +1594,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
if (io_level)
msm_host->curr_io_level = io_level;
- pr_debug("%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
+ dev_dbg(mmc_dev(mmc), "%s: %s: Handled IRQ(%d), irq_status=0x%x, ack=0x%x\n",
mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
irq_ack);
}
@@ -1833,6 +1924,98 @@ static void sdhci_msm_reset(struct sdhci_host *host, u8 mask)
sdhci_reset(host, mask);
}
+static int sdhci_msm_register_vreg(struct sdhci_msm_host *msm_host)
+{
+ int ret;
+ u32 vmmc_load;
+ struct mmc_host *mmc = msm_host->mmc;
+
+ ret = mmc_regulator_get_supply(msm_host->mmc);
+ if (ret)
+ return ret;
+ device_property_read_u32(&msm_host->pdev->dev,
+ "vmmc-max-load-microamp",
+ &vmmc_load);
+ device_property_read_u32(&msm_host->pdev->dev,
+ "vqmmc-max-load-microamp",
+ &msm_host->vqmmc_load);
+
+ /* Vmmc regulator can be turned off. So just set active load once */
+ if (!IS_ERR(mmc->supply.vmmc) && vmmc_load) {
+ ret = regulator_set_load(mmc->supply.vmmc, vmmc_load);
+ if (ret) {
+ dev_err(mmc_dev(mmc), "%s: vmmc set active load failed: %d\n",
+ mmc_hostname(mmc), ret);
+ return ret;
+ }
+ }
+
+ sdhci_msm_set_regulator_caps(msm_host);
+ mmc->ios.power_mode = MMC_POWER_UNDEFINED;
+
+ return 0;
+
+}
+
+static int sdhci_msm_start_signal_voltage_switch(struct mmc_host *mmc,
+ struct mmc_ios *ios)
+{
+ struct sdhci_host *host = mmc_priv(mmc);
+ u16 ctrl, status;
+
+ /*
+ * Signal Voltage Switching is only applicable for Host Controllers
+ * v3.00 and above.
+ */
+ if (host->version < SDHCI_SPEC_300)
+ return 0;
+
+ ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+
+ switch (ios->signal_voltage) {
+ case MMC_SIGNAL_VOLTAGE_330:
+ if (!(host->flags & SDHCI_SIGNALING_330))
+ return -EINVAL;
+
+ /* Set 1.8V Signal Enable in the Host Control2 register to 0 */
+ ctrl &= ~SDHCI_CTRL_VDD_180;
+ break;
+ case MMC_SIGNAL_VOLTAGE_180:
+ if (!(host->flags & SDHCI_SIGNALING_180))
+ return -EINVAL;
+
+ /*
+ * Enable 1.8V Signal Enable in the Host Control2
+ * register
+ */
+ ctrl |= SDHCI_CTRL_VDD_180;
+ break;
+ case MMC_SIGNAL_VOLTAGE_120:
+ if (!(host->flags & SDHCI_SIGNALING_120))
+ return -EINVAL;
+ return 0;
+ default:
+ /* No signal voltage switch required */
+ return 0;
+ }
+
+ sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+
+ /* Wait for 5ms */
+ usleep_range(5000, 5500);
+
+ /* regulator output should be stable within 5 ms */
+ status = !!(ctrl & SDHCI_CTRL_VDD_180);
+ ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+ if (!!(ctrl & SDHCI_CTRL_VDD_180) == status)
+ return 0;
+
+ dev_warn(mmc_dev(mmc), "%s: Regulator output did not became stable\n",
+ mmc_hostname(mmc));
+
+ return -EAGAIN;
+}
+
static const struct sdhci_msm_variant_ops mci_var_ops = {
.msm_readl_relaxed = sdhci_msm_mci_variant_readl_relaxed,
.msm_writel_relaxed = sdhci_msm_mci_variant_writel_relaxed,
@@ -1880,6 +2063,7 @@ static void sdhci_msm_reset(struct sdhci_host *host, u8 mask)
.write_w = sdhci_msm_writew,
.write_b = sdhci_msm_writeb,
.irq = sdhci_msm_cqe_irq,
+ .set_power = sdhci_set_power_noreg,
};
static const struct sdhci_pltfm_data sdhci_msm_pdata = {
@@ -2072,6 +2256,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
if (core_major == 1 && core_minor >= 0x49)
msm_host->updated_ddr_cfg = true;
+ ret = sdhci_msm_register_vreg(msm_host);
+ if (ret)
+ goto clk_disable;
+
/*
* Power on reset state may trigger power irq if previous status of
* PWRCTL was either BUS_ON or IO_HIGH_V. So before enabling pwr irq
@@ -2116,6 +2304,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
MSM_MMC_AUTOSUSPEND_DELAY_MS);
pm_runtime_use_autosuspend(&pdev->dev);
+ host->mmc_host_ops.start_signal_voltage_switch =
+ sdhci_msm_start_signal_voltage_switch;
host->mmc_host_ops.execute_tuning = sdhci_msm_execute_tuning;
if (of_property_read_bool(node, "supports-cqe"))
ret = sdhci_msm_cqe_add_host(host, pdev);
@@ -2123,7 +2313,6 @@ static int sdhci_msm_probe(struct platform_device *pdev)
ret = sdhci_add_host(host);
if (ret)
goto pm_runtime_disable;
- sdhci_msm_set_regulator_caps(msm_host);
pm_runtime_mark_last_busy(&pdev->dev);
pm_runtime_put_autosuspend(&pdev->dev);
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On Thu 21 May 08:23 PDT 2020, Veerabhadrarao Badiganti wrote:
> On qcom SD host controllers voltage switching be done after the HW
> is ready for it. The HW informs its readiness through power irq.
> The voltage switching should happen only then.
>
> Use the internal voltage switching and then control the voltage
> switching using power irq.
>
> Set the regulator load as well so that regulator can be configured
> in LPM mode when in is not being used.
>
> Co-developed-by: Asutosh Das <[email protected]>
> Signed-off-by: Asutosh Das <[email protected]>
> Co-developed-by: Vijay Viswanath <[email protected]>
> Signed-off-by: Vijay Viswanath <[email protected]>
> Co-developed-by: Veerabhadrarao Badiganti <[email protected]>
> Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
Looks better, thanks.
> ---
> drivers/mmc/host/sdhci-msm.c | 207 +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 198 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
[..]
> static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> @@ -1298,6 +1302,71 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
> sdhci_msm_hs400(host, &mmc->ios);
> }
>
> +static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
> +{
> + int ret;
> +
> + if (IS_ERR(mmc->supply.vmmc))
> + return 0;
> +
> + ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
> + if (ret)
> + dev_err(mmc_dev(mmc), "%s: vmmc set ocr with vdd=%d failed: %d\n",
> + mmc_hostname(mmc), mmc->ios.vdd, ret);
Missed this one on v1, in the event that mmc_regulator_set_ocr() return
a non-zero value it has already printed an error message. So please
replace the tail with just:
return mmc_regulator_set_ocr(...);
> +
> + return ret;
> +}
> +
> +static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
> + struct mmc_host *mmc, bool level)
> +{
> + int load, ret;
> + struct mmc_ios ios;
> +
> + if (IS_ERR(mmc->supply.vqmmc) ||
> + (mmc->ios.power_mode == MMC_POWER_UNDEFINED) ||
> + (msm_host->vqmmc_enabled == level))
> + return 0;
> +
> + if (msm_host->vqmmc_load) {
> + load = level ? msm_host->vqmmc_load : 0;
> + ret = regulator_set_load(mmc->supply.vqmmc, load);
Sorry for the late reply on v1, but please see my explanation regarding
load and always-on regulators there.
> + if (ret) {
> + dev_err(mmc_dev(mmc), "%s: vqmmc set load failed: %d\n",
> + mmc_hostname(mmc), ret);
> + goto out;
> + }
> + }
> +
> + if (level) {
> + /* Set the IO voltage regulator to default voltage level */
> + if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_330;
> + else if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_180;
> +
> + if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
> + ret = mmc_regulator_set_vqmmc(mmc, &ios);
> + if (ret < 0) {
> + dev_err(mmc_dev(mmc), "%s: vqmmc set volgate failed: %d\n",
> + mmc_hostname(mmc), ret);
> + goto out;
> + }
> + }
> + ret = regulator_enable(mmc->supply.vqmmc);
> + } else {
> + ret = regulator_disable(mmc->supply.vqmmc);
> + }
> +
> + if (ret)
> + dev_err(mmc_dev(mmc), "%s: vqmm %sable failed: %d\n",
> + mmc_hostname(mmc), level ? "en":"dis", ret);
> + else
> + msm_host->vqmmc_enabled = level;
> +out:
> + return ret;
> +}
[..]
> +static int sdhci_msm_start_signal_voltage_switch(struct mmc_host *mmc,
> + struct mmc_ios *ios)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> + u16 ctrl, status;
> +
> + /*
> + * Signal Voltage Switching is only applicable for Host Controllers
> + * v3.00 and above.
> + */
> + if (host->version < SDHCI_SPEC_300)
> + return 0;
> +
> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +
> + switch (ios->signal_voltage) {
> + case MMC_SIGNAL_VOLTAGE_330:
> + if (!(host->flags & SDHCI_SIGNALING_330))
> + return -EINVAL;
> +
> + /* Set 1.8V Signal Enable in the Host Control2 register to 0 */
> + ctrl &= ~SDHCI_CTRL_VDD_180;
> + break;
> + case MMC_SIGNAL_VOLTAGE_180:
> + if (!(host->flags & SDHCI_SIGNALING_180))
> + return -EINVAL;
> +
> + /*
> + * Enable 1.8V Signal Enable in the Host Control2
> + * register
> + */
> + ctrl |= SDHCI_CTRL_VDD_180;
> + break;
> + case MMC_SIGNAL_VOLTAGE_120:
> + if (!(host->flags & SDHCI_SIGNALING_120))
> + return -EINVAL;
> + return 0;
> + default:
> + /* No signal voltage switch required */
> + return 0;
> + }
> +
> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +
> + /* Wait for 5ms */
> + usleep_range(5000, 5500);
> +
> + /* regulator output should be stable within 5 ms */
> + status = !!(ctrl & SDHCI_CTRL_VDD_180);
> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + if (!!(ctrl & SDHCI_CTRL_VDD_180) == status)
You should be able to drop the !! both here and when assigning status.
Overall this looks neater, thanks for reworking it.
Regards,
Bjorn
Hi Bjorn,
On 5/22/2020 12:37 AM, Bjorn Andersson wrote:
> On Thu 21 May 08:23 PDT 2020, Veerabhadrarao Badiganti wrote:
>
>> On qcom SD host controllers voltage switching be done after the HW
>> is ready for it. The HW informs its readiness through power irq.
>> The voltage switching should happen only then.
>>
>> Use the internal voltage switching and then control the voltage
>> switching using power irq.
>>
>> Set the regulator load as well so that regulator can be configured
>> in LPM mode when in is not being used.
>>
>> Co-developed-by: Asutosh Das <[email protected]>
>> Signed-off-by: Asutosh Das <[email protected]>
>> Co-developed-by: Vijay Viswanath <[email protected]>
>> Signed-off-by: Vijay Viswanath <[email protected]>
>> Co-developed-by: Veerabhadrarao Badiganti <[email protected]>
>> Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
> Looks better, thanks.
>
>> ---
>> drivers/mmc/host/sdhci-msm.c | 207 +++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 198 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> [..]
>> static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>> @@ -1298,6 +1302,71 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>> sdhci_msm_hs400(host, &mmc->ios);
>> }
>>
>> +static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
>> +{
>> + int ret;
>> +
>> + if (IS_ERR(mmc->supply.vmmc))
>> + return 0;
>> +
>> + ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
>> + if (ret)
>> + dev_err(mmc_dev(mmc), "%s: vmmc set ocr with vdd=%d failed: %d\n",
>> + mmc_hostname(mmc), mmc->ios.vdd, ret);
> Missed this one on v1, in the event that mmc_regulator_set_ocr() return
> a non-zero value it has already printed an error message. So please
> replace the tail with just:
>
> return mmc_regulator_set_ocr(...);
>
>> +
>> + return ret;
>> +}
>> +
>> +static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
>> + struct mmc_host *mmc, bool level)
>> +{
>> + int load, ret;
>> + struct mmc_ios ios;
>> +
>> + if (IS_ERR(mmc->supply.vqmmc) ||
>> + (mmc->ios.power_mode == MMC_POWER_UNDEFINED) ||
>> + (msm_host->vqmmc_enabled == level))
>> + return 0;
>> +
>> + if (msm_host->vqmmc_load) {
>> + load = level ? msm_host->vqmmc_load : 0;
>> + ret = regulator_set_load(mmc->supply.vqmmc, load);
> Sorry for the late reply on v1, but please see my explanation regarding
> load and always-on regulators there.
<Merging your comment from V1 here>
>> You should still call regulator_enable()/regulator_disable() on your
>> consumer regulator in this driver. When you do this the regulator core
>> will conclude that the regulator_dev (i.e. the part that represents the
>> hardware) is marked always_on and will not enable/disable the regulator.
>> But it will still invoke _regulator_handle_consumer_enable() and
>> _regulator_handle_consumer_disable(), which will aggregate the "load" of
>> all client regulators and update the regulator's load.
>> So this will apply the load as you expect regardless of it being
>> supplied by a regulator marked as always_on.
Since I'm not turning off this regulator for eMMC, I wanted to keep it
in LPM mode
to save some power.
When the regulator configured in auto mode (RPMH_REGULATOR_MODE_AUTO) it
switches to LPM/HPM mode based on the active load.
So i have to minimize my driver load requirement so that I can let this
regulator
in LPM mode.
So i need to set load every-time I disable/enable the regulator.
>> + if (ret) {
>> + dev_err(mmc_dev(mmc), "%s: vqmmc set load failed: %d\n",
>> + mmc_hostname(mmc), ret);
>> + goto out;
>> + }
>> + }
>> +
>> + if (level) {
>> + /* Set the IO voltage regulator to default voltage level */
>> + if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
>> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_330;
>> + else if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
>> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_180;
>> +
>> + if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
>> + ret = mmc_regulator_set_vqmmc(mmc, &ios);
>> + if (ret < 0) {
>> + dev_err(mmc_dev(mmc), "%s: vqmmc set volgate failed: %d\n",
>> + mmc_hostname(mmc), ret);
>> + goto out;
>> + }
>> + }
>> + ret = regulator_enable(mmc->supply.vqmmc);
>> + } else {
>> + ret = regulator_disable(mmc->supply.vqmmc);
>> + }
>> +
>> + if (ret)
>> + dev_err(mmc_dev(mmc), "%s: vqmm %sable failed: %d\n",
>> + mmc_hostname(mmc), level ? "en":"dis", ret);
>> + else
>> + msm_host->vqmmc_enabled = level;
>> +out:
>> + return ret;
>> +}
> [..]
>> +static int sdhci_msm_start_signal_voltage_switch(struct mmc_host *mmc,
>> + struct mmc_ios *ios)
>> +{
>> + struct sdhci_host *host = mmc_priv(mmc);
>> + u16 ctrl, status;
>> +
>> + /*
>> + * Signal Voltage Switching is only applicable for Host Controllers
>> + * v3.00 and above.
>> + */
>> + if (host->version < SDHCI_SPEC_300)
>> + return 0;
>> +
>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> +
>> + switch (ios->signal_voltage) {
>> + case MMC_SIGNAL_VOLTAGE_330:
>> + if (!(host->flags & SDHCI_SIGNALING_330))
>> + return -EINVAL;
>> +
>> + /* Set 1.8V Signal Enable in the Host Control2 register to 0 */
>> + ctrl &= ~SDHCI_CTRL_VDD_180;
>> + break;
>> + case MMC_SIGNAL_VOLTAGE_180:
>> + if (!(host->flags & SDHCI_SIGNALING_180))
>> + return -EINVAL;
>> +
>> + /*
>> + * Enable 1.8V Signal Enable in the Host Control2
>> + * register
>> + */
>> + ctrl |= SDHCI_CTRL_VDD_180;
>> + break;
>> + case MMC_SIGNAL_VOLTAGE_120:
>> + if (!(host->flags & SDHCI_SIGNALING_120))
>> + return -EINVAL;
>> + return 0;
>> + default:
>> + /* No signal voltage switch required */
>> + return 0;
>> + }
>> +
>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +
>> + /* Wait for 5ms */
>> + usleep_range(5000, 5500);
>> +
>> + /* regulator output should be stable within 5 ms */
>> + status = !!(ctrl & SDHCI_CTRL_VDD_180);
>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> + if (!!(ctrl & SDHCI_CTRL_VDD_180) == status)
> You should be able to drop the !! both here and when assigning status.
>
> Overall this looks neater, thanks for reworking it.
>
> Regards,
> Bjorn
Thanks
Veera
On Fri 22 May 06:27 PDT 2020, Veerabhadrarao Badiganti wrote:
> Hi Bjorn,
>
> On 5/22/2020 12:37 AM, Bjorn Andersson wrote:
> > On Thu 21 May 08:23 PDT 2020, Veerabhadrarao Badiganti wrote:
> >
> > > On qcom SD host controllers voltage switching be done after the HW
> > > is ready for it. The HW informs its readiness through power irq.
> > > The voltage switching should happen only then.
> > >
> > > Use the internal voltage switching and then control the voltage
> > > switching using power irq.
> > >
> > > Set the regulator load as well so that regulator can be configured
> > > in LPM mode when in is not being used.
> > >
> > > Co-developed-by: Asutosh Das <[email protected]>
> > > Signed-off-by: Asutosh Das <[email protected]>
> > > Co-developed-by: Vijay Viswanath <[email protected]>
> > > Signed-off-by: Vijay Viswanath <[email protected]>
> > > Co-developed-by: Veerabhadrarao Badiganti <[email protected]>
> > > Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
> > Looks better, thanks.
> >
> > > ---
> > > drivers/mmc/host/sdhci-msm.c | 207 +++++++++++++++++++++++++++++++++++++++++--
> > > 1 file changed, 198 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> > [..]
> > > static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> > > @@ -1298,6 +1302,71 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
> > > sdhci_msm_hs400(host, &mmc->ios);
> > > }
> > > +static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
> > > +{
> > > + int ret;
> > > +
> > > + if (IS_ERR(mmc->supply.vmmc))
> > > + return 0;
> > > +
> > > + ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
> > > + if (ret)
> > > + dev_err(mmc_dev(mmc), "%s: vmmc set ocr with vdd=%d failed: %d\n",
> > > + mmc_hostname(mmc), mmc->ios.vdd, ret);
> > Missed this one on v1, in the event that mmc_regulator_set_ocr() return
> > a non-zero value it has already printed an error message. So please
> > replace the tail with just:
> >
> > return mmc_regulator_set_ocr(...);
> >
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
> > > + struct mmc_host *mmc, bool level)
> > > +{
> > > + int load, ret;
> > > + struct mmc_ios ios;
> > > +
> > > + if (IS_ERR(mmc->supply.vqmmc) ||
> > > + (mmc->ios.power_mode == MMC_POWER_UNDEFINED) ||
> > > + (msm_host->vqmmc_enabled == level))
> > > + return 0;
> > > +
> > > + if (msm_host->vqmmc_load) {
> > > + load = level ? msm_host->vqmmc_load : 0;
> > > + ret = regulator_set_load(mmc->supply.vqmmc, load);
> > Sorry for the late reply on v1, but please see my explanation regarding
> > load and always-on regulators there.
>
> <Merging your comment from V1 here>
>
> >> You should still call regulator_enable()/regulator_disable() on your
> >> consumer regulator in this driver. When you do this the regulator core
> >> will conclude that the regulator_dev (i.e. the part that represents the
> >> hardware) is marked always_on and will not enable/disable the regulator.
>
> >> But it will still invoke _regulator_handle_consumer_enable() and
> >> _regulator_handle_consumer_disable(), which will aggregate the "load" of
> >> all client regulators and update the regulator's load.
>
> >> So this will apply the load as you expect regardless of it being
> >> supplied by a regulator marked as always_on.
>
> Since I'm not turning off this regulator for eMMC, I wanted to keep it in
> LPM mode
> to save some power.
> When the regulator configured in auto mode (RPMH_REGULATOR_MODE_AUTO) it
> switches to LPM/HPM mode based on the active load.
> So i have to minimize my driver load requirement so that I can let this
> regulator
> in LPM mode.
> So i need to set load every-time I disable/enable the regulator.
>
You call regulator_enable(vqmmc) and regulator_disable() below, so you
are telling the regulator framework that your struct regulator should be
"on" or "off".
This will cause the sum of all struct regulator's for the underlying
struct regulator_dev to be recalculated. So after calling
regulator_disable() below your effective addition to the load
calculation is 0, regardless of which load you have specified.
Independent of this the property regulator-always-on (always_on in
struct regulator_dev) will determine if the enable/disable request will
actually be sent to the RPMh.
So, if you where to not call regulator_disable() for eMMC your argument
is correct, but as far as I can see you are and you're relying on the
regulator core to keep it always-on - and then the load logic is in
effect still.
Regards,
Bjorn
> > > + if (ret) {
> > > + dev_err(mmc_dev(mmc), "%s: vqmmc set load failed: %d\n",
> > > + mmc_hostname(mmc), ret);
> > > + goto out;
> > > + }
> > > + }
> > > +
> > > + if (level) {
> > > + /* Set the IO voltage regulator to default voltage level */
> > > + if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
> > > + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_330;
> > > + else if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
> > > + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_180;
> > > +
> > > + if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
> > > + ret = mmc_regulator_set_vqmmc(mmc, &ios);
> > > + if (ret < 0) {
> > > + dev_err(mmc_dev(mmc), "%s: vqmmc set volgate failed: %d\n",
> > > + mmc_hostname(mmc), ret);
> > > + goto out;
> > > + }
> > > + }
> > > + ret = regulator_enable(mmc->supply.vqmmc);
> > > + } else {
> > > + ret = regulator_disable(mmc->supply.vqmmc);
> > > + }
> > > +
> > > + if (ret)
> > > + dev_err(mmc_dev(mmc), "%s: vqmm %sable failed: %d\n",
> > > + mmc_hostname(mmc), level ? "en":"dis", ret);
> > > + else
> > > + msm_host->vqmmc_enabled = level;
> > > +out:
> > > + return ret;
> > > +}
On 5/22/2020 10:34 PM, Bjorn Andersson wrote:
> On Fri 22 May 06:27 PDT 2020, Veerabhadrarao Badiganti wrote:
>
>> Hi Bjorn,
>>
>> On 5/22/2020 12:37 AM, Bjorn Andersson wrote:
>>> On Thu 21 May 08:23 PDT 2020, Veerabhadrarao Badiganti wrote:
>>>
>>>> On qcom SD host controllers voltage switching be done after the HW
>>>> is ready for it. The HW informs its readiness through power irq.
>>>> The voltage switching should happen only then.
>>>>
>>>> Use the internal voltage switching and then control the voltage
>>>> switching using power irq.
>>>>
>>>> Set the regulator load as well so that regulator can be configured
>>>> in LPM mode when in is not being used.
>>>>
>>>> Co-developed-by: Asutosh Das <[email protected]>
>>>> Signed-off-by: Asutosh Das <[email protected]>
>>>> Co-developed-by: Vijay Viswanath <[email protected]>
>>>> Signed-off-by: Vijay Viswanath <[email protected]>
>>>> Co-developed-by: Veerabhadrarao Badiganti <[email protected]>
>>>> Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
>>> Looks better, thanks.
>>>
>>>> ---
>>>> drivers/mmc/host/sdhci-msm.c | 207 +++++++++++++++++++++++++++++++++++++++++--
>>>> 1 file changed, 198 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>> [..]
>>>> static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>>>> @@ -1298,6 +1302,71 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>>>> sdhci_msm_hs400(host, &mmc->ios);
>>>> }
>>>> +static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + if (IS_ERR(mmc->supply.vmmc))
>>>> + return 0;
>>>> +
>>>> + ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
>>>> + if (ret)
>>>> + dev_err(mmc_dev(mmc), "%s: vmmc set ocr with vdd=%d failed: %d\n",
>>>> + mmc_hostname(mmc), mmc->ios.vdd, ret);
>>> Missed this one on v1, in the event that mmc_regulator_set_ocr() return
>>> a non-zero value it has already printed an error message. So please
>>> replace the tail with just:
>>>
>>> return mmc_regulator_set_ocr(...);
>>>
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
>>>> + struct mmc_host *mmc, bool level)
>>>> +{
>>>> + int load, ret;
>>>> + struct mmc_ios ios;
>>>> +
>>>> + if (IS_ERR(mmc->supply.vqmmc) ||
>>>> + (mmc->ios.power_mode == MMC_POWER_UNDEFINED) ||
>>>> + (msm_host->vqmmc_enabled == level))
>>>> + return 0;
>>>> +
>>>> + if (msm_host->vqmmc_load) {
>>>> + load = level ? msm_host->vqmmc_load : 0;
>>>> + ret = regulator_set_load(mmc->supply.vqmmc, load);
>>> Sorry for the late reply on v1, but please see my explanation regarding
>>> load and always-on regulators there.
>> <Merging your comment from V1 here>
>>
>>>> You should still call regulator_enable()/regulator_disable() on your
>>>> consumer regulator in this driver. When you do this the regulator core
>>>> will conclude that the regulator_dev (i.e. the part that represents the
>>>> hardware) is marked always_on and will not enable/disable the regulator.
>>>> But it will still invoke _regulator_handle_consumer_enable() and
>>>> _regulator_handle_consumer_disable(), which will aggregate the "load" of
>>>> all client regulators and update the regulator's load.
>>>> So this will apply the load as you expect regardless of it being
>>>> supplied by a regulator marked as always_on.
>> Since I'm not turning off this regulator for eMMC, I wanted to keep it in
>> LPM mode
>> to save some power.
>> When the regulator configured in auto mode (RPMH_REGULATOR_MODE_AUTO) it
>> switches to LPM/HPM mode based on the active load.
>> So i have to minimize my driver load requirement so that I can let this
>> regulator
>> in LPM mode.
>> So i need to set load every-time I disable/enable the regulator.
>>
> You call regulator_enable(vqmmc) and regulator_disable() below, so you
> are telling the regulator framework that your struct regulator should be
> "on" or "off".
>
> This will cause the sum of all struct regulator's for the underlying
> struct regulator_dev to be recalculated. So after calling
> regulator_disable() below your effective addition to the load
> calculation is 0, regardless of which load you have specified.
>
> Independent of this the property regulator-always-on (always_on in
> struct regulator_dev) will determine if the enable/disable request will
> actually be sent to the RPMh.
>
>
> So, if you where to not call regulator_disable() for eMMC your argument
> is correct, but as far as I can see you are and you're relying on the
> regulator core to keep it always-on - and then the load logic is in
> effect still.
Thanks for the details Bjorn.
My requirement is, for eMMC i shouldn't be turning this regulator off.
But has to configure in LPM mode.
For SD-card, i have to turn-off this regulator.
So I'm planning to update the logic as below, let me know if you have
any other suggestions.
+static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
+ struct mmc_host *mmc, bool level)
+{
+ int ret;
+ bool always_on;
+
+ if (IS_ERR(mmc->supply.vqmmc) ||
+ (mmc->ios.power_mode == MMC_POWER_UNDEFINED))
+ return 0;
+ /*
+ * For eMMC don't turn off Vqmmc, Instead just configure it in LPM
+ * and HPM modes by setting the right amount of load.
+ */
+ always_on = mmc->card && mmc_card_mmc(mmc->card);
+
+ if (always_on)
+ ret = msm_config_vqmmc_mode(msm_host, mmc, level);
+ else
+ ret = msm_toggle_vqmmc(msm_host, mmc, level);
+
+ return ret;
+}
> Regards,
> Bjorn
>
>>>> + if (ret) {
>>>> + dev_err(mmc_dev(mmc), "%s: vqmmc set load failed: %d\n",
>>>> + mmc_hostname(mmc), ret);
>>>> + goto out;
>>>> + }
>>>> + }
>>>> +
>>>> + if (level) {
>>>> + /* Set the IO voltage regulator to default voltage level */
>>>> + if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
>>>> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_330;
>>>> + else if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
>>>> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_180;
>>>> +
>>>> + if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
>>>> + ret = mmc_regulator_set_vqmmc(mmc, &ios);
>>>> + if (ret < 0) {
>>>> + dev_err(mmc_dev(mmc), "%s: vqmmc set volgate failed: %d\n",
>>>> + mmc_hostname(mmc), ret);
>>>> + goto out;
>>>> + }
>>>> + }
>>>> + ret = regulator_enable(mmc->supply.vqmmc);
>>>> + } else {
>>>> + ret = regulator_disable(mmc->supply.vqmmc);
>>>> + }
>>>> +
>>>> + if (ret)
>>>> + dev_err(mmc_dev(mmc), "%s: vqmm %sable failed: %d\n",
>>>> + mmc_hostname(mmc), level ? "en":"dis", ret);
>>>> + else
>>>> + msm_host->vqmmc_enabled = level;
>>>> +out:
>>>> + return ret;
>>>> +}