[PATCH V1 0/3] Internal voltage control for qcom SDHC

[PATCH V1 0/3] Internal voltage control for qcom SDHC

[Veerabhadrarao Badiganti]

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.

So added support to register voltage regulators from the msm driver
and use them.

This patchset was posted long back but not actively pursued
https://lore.kernel.org/linux-arm-msm/1539004739-32060-1-git-send-email-vbadigan@codeaurora.org/
So posting it as fresh patchset.  

Veerabhadrarao Badiganti (1):
  dt-bindings: mmc: Supply max load for mmc supplies

Vijay Viswanath (2):
  mmc: sdhci-msm: Use internal voltage control
  mmc: sdhci: Allow platform controlled voltage switching

 .../devicetree/bindings/mmc/mmc-controller.yaml    |  16 ++
 drivers/mmc/host/sdhci-msm.c                       | 215 ++++++++++++++++++++-
 drivers/mmc/host/sdhci.c                           |  32 +--
 drivers/mmc/host/sdhci.h                           |   1 +
 4 files changed, 243 insertions(+), 21 deletions(-)

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V1 1/3] dt-bindings: mmc: Supply max load for mmc supplies

[Veerabhadrarao Badiganti]

Supply the max load needed for driving the mmc supplies.

Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
---
 .../devicetree/bindings/mmc/mmc-controller.yaml          | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
index acc9f10..9058b82 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
+++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
@@ -290,6 +290,22 @@ properties:
     description:
       Supply for the bus IO line power
 
+  vmmc-max-load-microamp:
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - minimum: 0
+      - maximum: 1000000
+    description:
+      Maximum load for the card power.
+
+  vqmmc-max-load-microamp:
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - minimum: 0
+      - maximum: 1000000
+    description:
+      Maximum load for the bus IO line power.
+
   mmc-pwrseq:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V1 2/3] mmc: sdhci-msm: Use internal voltage control

[Veerabhadrarao Badiganti]

From: Vijay Viswanath <vviswana@codeaurora.org>

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.

Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 215 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 207 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 97758fa..a10e955 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,9 @@ struct sdhci_msm_host {
 	bool use_cdr;
 	u32 transfer_mode;
 	bool updated_ddr_cfg;
+	u32 vmmc_load;
+	u32 vqmmc_load;
+	bool vqmmc_enabled;
 };
 
 static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
@@ -1298,6 +1303,78 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
 		sdhci_msm_hs400(host, &mmc->ios);
 }
 
+static int sdhci_msm_set_vmmc(struct sdhci_msm_host *msm_host,
+			      struct mmc_host *mmc, int level)
+{
+	int load, ret;
+
+	if (IS_ERR(mmc->supply.vmmc))
+		return 0;
+
+	if (msm_host->vmmc_load) {
+		load = level ? msm_host->vmmc_load : 0;
+		ret = regulator_set_load(mmc->supply.vmmc, load);
+		if (ret)
+			goto out;
+	}
+
+	ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
+out:
+	if (ret)
+		pr_err("%s: vmmc set load/ocr failed: %d\n",
+				mmc_hostname(mmc), ret);
+
+	return ret;
+}
+
+static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
+			      struct mmc_host *mmc, int 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)
+			goto out;
+	}
+
+	/*
+	 * The IO voltage regulator may not always support a voltage close to
+	 * vdd. Set IO voltage based on capability of the regulator.
+	 */
+	if (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) {
+			pr_debug("%s: %s: setting signal voltage: %d\n",
+					mmc_hostname(mmc), __func__,
+					ios.signal_voltage);
+			ret = mmc_regulator_set_vqmmc(mmc, &ios);
+			if (ret < 0)
+				goto out;
+		}
+		ret = regulator_enable(mmc->supply.vqmmc);
+	} else {
+		ret = regulator_disable(mmc->supply.vqmmc);
+	}
+out:
+	if (ret)
+		pr_err("%s: vqmmc failed: %d\n", mmc_hostname(mmc), ret);
+	else
+		msm_host->vqmmc_enabled = level;
+
+	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 +1478,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 = 0;
 	u32 pwr_state = 0, io_level = 0;
 	u32 config;
 	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
@@ -1438,14 +1516,35 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 
 	/* Handle BUS ON/OFF*/
 	if (irq_status & CORE_PWRCTL_BUS_ON) {
-		pwr_state = REQ_BUS_ON;
-		io_level = REQ_IO_HIGH;
-		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
+		ret = sdhci_msm_set_vmmc(msm_host, mmc, 1);
+		if (!ret)
+			ret = sdhci_msm_set_vqmmc(msm_host, mmc, 1);
+
+		if (!ret) {
+			pwr_state = REQ_BUS_ON;
+			io_level = REQ_IO_HIGH;
+			irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
+		} else {
+			pr_err("%s: BUS_ON req failed(%d). irq_status: 0x%08x\n",
+					mmc_hostname(mmc), ret, irq_status);
+			irq_ack |= CORE_PWRCTL_BUS_FAIL;
+			sdhci_msm_set_vmmc(msm_host, mmc, 0);
+		}
 	}
 	if (irq_status & CORE_PWRCTL_BUS_OFF) {
-		pwr_state = REQ_BUS_OFF;
-		io_level = REQ_IO_LOW;
-		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
+		ret = sdhci_msm_set_vmmc(msm_host, mmc, 0);
+		if (!ret)
+			ret = sdhci_msm_set_vqmmc(msm_host, mmc, 0);
+
+		if (!ret) {
+			pwr_state = REQ_BUS_OFF;
+			io_level = REQ_IO_LOW;
+			irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
+		} else {
+			pr_err("%s: BUS_ON req failed(%d). irq_status: 0x%08x\n",
+					mmc_hostname(mmc), ret, irq_status);
+			irq_ack |= CORE_PWRCTL_BUS_FAIL;
+		}
 	}
 	/* Handle IO LOW/HIGH */
 	if (irq_status & CORE_PWRCTL_IO_LOW) {
@@ -1457,6 +1556,15 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 		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)
+			pr_err("%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);
+	}
+
 	/*
 	 * The driver has to acknowledge the interrupt, switch voltages and
 	 * report back if it succeded or not to this register. The voltage
@@ -1833,6 +1941,91 @@ 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 = 0;
+	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",
+			&msm_host->vmmc_load);
+	device_property_read_u32(&msm_host->pdev->dev,
+			"vqmmc-max-load-microamp",
+			&msm_host->vqmmc_load);
+
+	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;
+
+	/*
+	 * 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;
+		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+
+		/* 3.3V regulator output should be stable within 5 ms */
+		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		if (!(ctrl & SDHCI_CTRL_VDD_180))
+			return 0;
+
+		pr_warn("%s: 3.3V regulator output did not became stable\n",
+			mmc_hostname(mmc));
+
+		return -EAGAIN;
+	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;
+		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+
+		/* 1.8V regulator output should be stable within 5 ms */
+		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		if (ctrl & SDHCI_CTRL_VDD_180)
+			return 0;
+
+		pr_warn("%s: 1.8V regulator output did not became stable\n",
+			mmc_hostname(mmc));
+
+		return -EAGAIN;
+	case MMC_SIGNAL_VOLTAGE_120:
+		if (!(host->flags & SDHCI_SIGNALING_120))
+			return -EINVAL;
+		return 0;
+	default:
+		/* No signal voltage switch required */
+		return 0;
+	}
+
+}
+
 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 +2073,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 +2266,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 +2314,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 +2323,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

[PATCH V1 3/3] mmc: sdhci: Allow platform controlled voltage switching

[Veerabhadrarao Badiganti]

From: Vijay Viswanath <vviswana@codeaurora.org>

If vendor platform drivers are controlling whole logic of voltage
switching, then sdhci driver no need control vqmmc regulator.
So skip enabling/disable vqmmc from SDHC driver.

Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
---
 drivers/mmc/host/sdhci.c | 32 +++++++++++++++++++-------------
 drivers/mmc/host/sdhci.h |  1 +
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1bb6b67..c010823 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -4098,6 +4098,7 @@ int sdhci_setup_host(struct sdhci_host *host)
 	unsigned int override_timeout_clk;
 	u32 max_clk;
 	int ret;
+	bool enable_vqmmc = false;
 
 	WARN_ON(host == NULL);
 	if (host == NULL)
@@ -4111,9 +4112,12 @@ int sdhci_setup_host(struct sdhci_host *host)
 	 * the host can take the appropriate action if regulators are not
 	 * available.
 	 */
-	ret = mmc_regulator_get_supply(mmc);
-	if (ret)
-		return ret;
+	if (!mmc->supply.vqmmc) {
+		ret = mmc_regulator_get_supply(mmc);
+		if (ret)
+			return ret;
+		enable_vqmmc  = true;
+	}
 
 	DBG("Version:   0x%08x | Present:  0x%08x\n",
 	    sdhci_readw(host, SDHCI_HOST_VERSION),
@@ -4373,7 +4377,15 @@ int sdhci_setup_host(struct sdhci_host *host)
 		mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	if (!IS_ERR(mmc->supply.vqmmc)) {
-		ret = regulator_enable(mmc->supply.vqmmc);
+		if (enable_vqmmc) {
+			ret = regulator_enable(mmc->supply.vqmmc);
+			if (ret) {
+				pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
+					mmc_hostname(mmc), ret);
+				mmc->supply.vqmmc = ERR_PTR(-EINVAL);
+			}
+			host->vqmmc_enabled = !ret;
+		}
 
 		/* If vqmmc provides no 1.8V signalling, then there's no UHS */
 		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
@@ -4386,12 +4398,6 @@ int sdhci_setup_host(struct sdhci_host *host)
 		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 2700000,
 						    3600000))
 			host->flags &= ~SDHCI_SIGNALING_330;
-
-		if (ret) {
-			pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
-				mmc_hostname(mmc), ret);
-			mmc->supply.vqmmc = ERR_PTR(-EINVAL);
-		}
 	}
 
 	if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) {
@@ -4625,7 +4631,7 @@ int sdhci_setup_host(struct sdhci_host *host)
 	return 0;
 
 unreg:
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (host->vqmmc_enabled)
 		regulator_disable(mmc->supply.vqmmc);
 undma:
 	if (host->align_buffer)
@@ -4643,7 +4649,7 @@ void sdhci_cleanup_host(struct sdhci_host *host)
 {
 	struct mmc_host *mmc = host->mmc;
 
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (host->vqmmc_enabled)
 		regulator_disable(mmc->supply.vqmmc);
 
 	if (host->align_buffer)
@@ -4780,7 +4786,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 
 	destroy_workqueue(host->complete_wq);
 
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (host->vqmmc_enabled)
 		regulator_disable(mmc->supply.vqmmc);
 
 	if (host->align_buffer)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 8d2a096..24d27e1 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -570,6 +570,7 @@ struct sdhci_host {
 	u32 caps1;		/* CAPABILITY_1 */
 	bool read_caps;		/* Capability flags have been read */
 
+	bool vqmmc_enabled;	/* Vqmmc is enabled */
 	unsigned int            ocr_avail_sdio;	/* OCR bit masks */
 	unsigned int            ocr_avail_sd;
 	unsigned int            ocr_avail_mmc;
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V1 2/3] mmc: sdhci-msm: Use internal voltage control

[Bjorn Andersson]

On Fri 15 May 04:18 PDT 2020, Veerabhadrarao Badiganti wrote:

> From: Vijay Viswanath <vviswana@codeaurora.org>
> 
> 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.
> 
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>

Please note that per Documentation/process/submitting-patches.rst
section 11) this states:

1) You wrote the patch (From:) without stating that its Certificate of
origin.

2) Then Asutosh took your patch (in full or part) and guarantees that
he's allowed to contribute it to the project.

3) Then you took his patch (in full or part) and guarantee that you're
allowed to contribute it to the project.

4) Then Veerabhadrarao took your patch (in full or part) and guarantees
that he's allowed to contribute it to the project

5) Then somehow it came out of your inbox - even if Veerabhadrarao was
the one who handled it last.


As author you should be the first one to certify, and as poster to LKML
you should be the last one.

If you worked together on this, then list Asutosh and Veerabhadrarao as
Co-developed-by.

> ---
>  drivers/mmc/host/sdhci-msm.c | 215 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 207 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 97758fa..a10e955 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,9 @@ struct sdhci_msm_host {
>  	bool use_cdr;
>  	u32 transfer_mode;
>  	bool updated_ddr_cfg;
> +	u32 vmmc_load;
> +	u32 vqmmc_load;
> +	bool vqmmc_enabled;
>  };
>  
>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> @@ -1298,6 +1303,78 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>  		sdhci_msm_hs400(host, &mmc->ios);
>  }
>  
> +static int sdhci_msm_set_vmmc(struct sdhci_msm_host *msm_host,
> +			      struct mmc_host *mmc, int level)
> +{
> +	int load, ret;
> +
> +	if (IS_ERR(mmc->supply.vmmc))
> +		return 0;
> +
> +	if (msm_host->vmmc_load) {
> +		load = level ? msm_host->vmmc_load : 0;
> +		ret = regulator_set_load(mmc->supply.vmmc, load);

I started on the comment about regulator_set_load() that you can find
below...

> +		if (ret)
> +			goto out;
> +	}
> +
> +	ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);

...but I don't see that mmc->ios.vdd necessarily is in sync with
"level". Or do you here simply set the load based on what the hardware
tell you and then orthogonally to that let the core enable/disable the
regulator?

Perhaps I'm just missing something obvious, but if not I believe this
warrants a comment describing that you're lowering the power level
regardless of the actual power being disabled.

> +out:
> +	if (ret)
> +		pr_err("%s: vmmc set load/ocr failed: %d\n",
> +				mmc_hostname(mmc), ret);

Please use:
	dev_err(mmc_dev(mmc), ...);

> +
> +	return ret;
> +}
> +
> +static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
> +			      struct mmc_host *mmc, int level)

vqmmc_enabled is a bool and "level" sounds like an int with several
possible values. So please make level bool here as well, to make it
easer to read..

> +{
> +	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);

Since v5.0 the "load" of a regulator consumer is only taken into
consideration if the consumer is enabled. So given that you're toggling
the regulator below there's no need to change this here.

Just specify the "active load" at probe time.

> +		if (ret)
> +			goto out;
> +	}
> +
> +	/*
> +	 * The IO voltage regulator may not always support a voltage close to
> +	 * vdd. Set IO voltage based on capability of the regulator.
> +	 */

Is this comment related to the if/else-if inside the conditional? If so
please move it one line down.

> +	if (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;

Please add a space here, to indicate that the if statement on the next
line is unrelated to the if/elseif above.

> +		if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
> +			pr_debug("%s: %s: setting signal voltage: %d\n",
> +					mmc_hostname(mmc), __func__,
> +					ios.signal_voltage);

I strongly believe you should replace these debug prints with
tracepoints, throughout the driver.

> +			ret = mmc_regulator_set_vqmmc(mmc, &ios);
> +			if (ret < 0)
> +				goto out;

> +		}
> +		ret = regulator_enable(mmc->supply.vqmmc);
> +	} else {
> +		ret = regulator_disable(mmc->supply.vqmmc);
> +	}

Given that you don't need to regulator_set_load() this function is now
just one large if/else condition on a constant passed as an argument.
Please split it into sdhci_msm_enable_vqmmc() and
sdhci_msm_disable_vqmmc().

> +out:
> +	if (ret)
> +		pr_err("%s: vqmmc failed: %d\n", mmc_hostname(mmc), ret);

I think it would be useful to know if this error came from
mmc_regulator_set_vqmmc() or regulator_enable() - or
regulator_disable().

So please move this up and add some context in the error message, and
please use dev_err().

> +	else
> +		msm_host->vqmmc_enabled = level;
> +
> +	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 +1478,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 = 0;

There's no need to initialize ret, in all occasions it's assigned before
being read.

>  	u32 pwr_state = 0, io_level = 0;
>  	u32 config;
>  	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
> @@ -1438,14 +1516,35 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  
>  	/* Handle BUS ON/OFF*/
>  	if (irq_status & CORE_PWRCTL_BUS_ON) {
> -		pwr_state = REQ_BUS_ON;
> -		io_level = REQ_IO_HIGH;
> -		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> +		ret = sdhci_msm_set_vmmc(msm_host, mmc, 1);
> +		if (!ret)
> +			ret = sdhci_msm_set_vqmmc(msm_host, mmc, 1);

I find this quite complex to follow. Wouldn't it be cleaner to retain
the 4 checks on irq_status as they are and then before the writel of
irq_ack check pwr_state and io_level and call sdhci_msm_set_{vmmc,vqmmc}
accordingly?

> +
> +		if (!ret) {
> +			pwr_state = REQ_BUS_ON;
> +			io_level = REQ_IO_HIGH;
> +			irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> +		} else {
> +			pr_err("%s: BUS_ON req failed(%d). irq_status: 0x%08x\n",
> +					mmc_hostname(mmc), ret, irq_status);

You already printed that this failed in sdhci_msm_set_{vmmc,vqmmc}, no
need to print again.

> +			irq_ack |= CORE_PWRCTL_BUS_FAIL;
> +			sdhci_msm_set_vmmc(msm_host, mmc, 0);
> +		}
>  	}
>  	if (irq_status & CORE_PWRCTL_BUS_OFF) {
> -		pwr_state = REQ_BUS_OFF;
> -		io_level = REQ_IO_LOW;
> -		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> +		ret = sdhci_msm_set_vmmc(msm_host, mmc, 0);
> +		if (!ret)
> +			ret = sdhci_msm_set_vqmmc(msm_host, mmc, 0);
> +
> +		if (!ret) {
> +			pwr_state = REQ_BUS_OFF;
> +			io_level = REQ_IO_LOW;
> +			irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> +		} else {
> +			pr_err("%s: BUS_ON req failed(%d). irq_status: 0x%08x\n",
> +					mmc_hostname(mmc), ret, irq_status);
> +			irq_ack |= CORE_PWRCTL_BUS_FAIL;
> +		}
>  	}
>  	/* Handle IO LOW/HIGH */
>  	if (irq_status & CORE_PWRCTL_IO_LOW) {
> @@ -1457,6 +1556,15 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>  	}
>  
> +	if (io_level && !IS_ERR(mmc->supply.vqmmc) && !pwr_state) {
> +		ret = mmc_regulator_set_vqmmc(mmc, &mmc->ios);

Didn't you already call this through sdhci_msm_set_vqmmc()?

> +		if (ret < 0)
> +			pr_err("%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);
> +	}
> +
>  	/*
>  	 * The driver has to acknowledge the interrupt, switch voltages and
>  	 * report back if it succeded or not to this register. The voltage
> @@ -1833,6 +1941,91 @@ 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 = 0;

No need to initialize ret, first use is an assignment.

> +	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",
> +			&msm_host->vmmc_load);
> +	device_property_read_u32(&msm_host->pdev->dev,
> +			"vqmmc-max-load-microamp",
> +			&msm_host->vqmmc_load);

These properties are not documented. Do they vary enough to mandate
being read from DT or could they simply be approximated by a define
instead?

> +
> +	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;
> +
> +	/*
> +	 * 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;
> +		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +
> +		/* 3.3V regulator output should be stable within 5 ms */

What mechanism ensures that the readw won't return withing 5ms from the
writew above?

> +		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +		if (!(ctrl & SDHCI_CTRL_VDD_180))
> +			return 0;
> +
> +		pr_warn("%s: 3.3V regulator output did not became stable\n",
> +			mmc_hostname(mmc));
> +
> +		return -EAGAIN;

The body of the 330 and 180 cases are quite similar, can you perhaps
deal with those after the switch, once?

> +	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;
> +		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +
> +		/* 1.8V regulator output should be stable within 5 ms */
> +		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +		if (ctrl & SDHCI_CTRL_VDD_180)
> +			return 0;
> +
> +		pr_warn("%s: 1.8V regulator output did not became stable\n",
> +			mmc_hostname(mmc));
> +
> +		return -EAGAIN;
> +	case MMC_SIGNAL_VOLTAGE_120:
> +		if (!(host->flags & SDHCI_SIGNALING_120))
> +			return -EINVAL;
> +		return 0;
> +	default:
> +		/* No signal voltage switch required */
> +		return 0;
> +	}
> +

Empty line.

Regards,
Bjorn

> +}
> +
>  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 +2073,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 +2266,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 +2314,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 +2323,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
> 

Re: [PATCH V1 2/3] mmc: sdhci-msm: Use internal voltage control

[Bjorn Andersson]

On Mon 18 May 12:57 PDT 2020, Bjorn Andersson wrote:
> On Fri 15 May 04:18 PDT 2020, Veerabhadrarao Badiganti wrote:
> > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
[..]
> > +static int sdhci_msm_register_vreg(struct sdhci_msm_host *msm_host)
> > +{
> > +	int ret = 0;
> 
> No need to initialize ret, first use is an assignment.
> 
> > +	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",
> > +			&msm_host->vmmc_load);
> > +	device_property_read_u32(&msm_host->pdev->dev,
> > +			"vqmmc-max-load-microamp",
> > +			&msm_host->vqmmc_load);
> 
> These properties are not documented. Do they vary enough to mandate
> being read from DT or could they simply be approximated by a define
> instead?
> 

Disregard my comment about them not being documented, I wasn't Cc'ed on
the binding patch and missed it.

My question about just defining them statically still stands.

Thanks,
Bjorn

Re: [PATCH V1 3/3] mmc: sdhci: Allow platform controlled voltage switching

[Adrian Hunter]

On 15/05/20 2:18 pm, Veerabhadrarao Badiganti wrote:
> From: Vijay Viswanath <vviswana@codeaurora.org>
> 
> If vendor platform drivers are controlling whole logic of voltage
> switching, then sdhci driver no need control vqmmc regulator.
> So skip enabling/disable vqmmc from SDHC driver.
> 
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci.c | 32 +++++++++++++++++++-------------
>  drivers/mmc/host/sdhci.h |  1 +
>  2 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1bb6b67..c010823 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -4098,6 +4098,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>  	unsigned int override_timeout_clk;
>  	u32 max_clk;
>  	int ret;
> +	bool enable_vqmmc = false;
>  
>  	WARN_ON(host == NULL);
>  	if (host == NULL)
> @@ -4111,9 +4112,12 @@ int sdhci_setup_host(struct sdhci_host *host)
>  	 * the host can take the appropriate action if regulators are not
>  	 * available.
>  	 */
> -	ret = mmc_regulator_get_supply(mmc);
> -	if (ret)
> -		return ret;
> +	if (!mmc->supply.vqmmc) {
> +		ret = mmc_regulator_get_supply(mmc);
> +		if (ret)
> +			return ret;
> +		enable_vqmmc  = true;
> +	}
>  
>  	DBG("Version:   0x%08x | Present:  0x%08x\n",
>  	    sdhci_readw(host, SDHCI_HOST_VERSION),
> @@ -4373,7 +4377,15 @@ int sdhci_setup_host(struct sdhci_host *host)
>  		mmc->caps |= MMC_CAP_NEEDS_POLL;
>  
>  	if (!IS_ERR(mmc->supply.vqmmc)) {
> -		ret = regulator_enable(mmc->supply.vqmmc);
> +		if (enable_vqmmc) {
> +			ret = regulator_enable(mmc->supply.vqmmc);
> +			if (ret) {
> +				pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
> +					mmc_hostname(mmc), ret);
> +				mmc->supply.vqmmc = ERR_PTR(-EINVAL);
> +			}
> +			host->vqmmc_enabled = !ret;
> +		}
>  
>  		/* If vqmmc provides no 1.8V signalling, then there's no UHS */
>  		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
> @@ -4386,12 +4398,6 @@ int sdhci_setup_host(struct sdhci_host *host)
>  		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 2700000,
>  						    3600000))
>  			host->flags &= ~SDHCI_SIGNALING_330;
> -
> -		if (ret) {
> -			pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
> -				mmc_hostname(mmc), ret);
> -			mmc->supply.vqmmc = ERR_PTR(-EINVAL);
> -		}
>  	}
>  
>  	if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) {
> @@ -4625,7 +4631,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>  	return 0;
>  
>  unreg:
> -	if (!IS_ERR(mmc->supply.vqmmc))
> +	if (host->vqmmc_enabled)
>  		regulator_disable(mmc->supply.vqmmc);
>  undma:
>  	if (host->align_buffer)
> @@ -4643,7 +4649,7 @@ void sdhci_cleanup_host(struct sdhci_host *host)
>  {
>  	struct mmc_host *mmc = host->mmc;
>  
> -	if (!IS_ERR(mmc->supply.vqmmc))
> +	if (host->vqmmc_enabled)
>  		regulator_disable(mmc->supply.vqmmc);
>  
>  	if (host->align_buffer)
> @@ -4780,7 +4786,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>  
>  	destroy_workqueue(host->complete_wq);
>  
> -	if (!IS_ERR(mmc->supply.vqmmc))
> +	if (host->vqmmc_enabled)
>  		regulator_disable(mmc->supply.vqmmc);
>  
>  	if (host->align_buffer)
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 8d2a096..24d27e1 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -570,6 +570,7 @@ struct sdhci_host {
>  	u32 caps1;		/* CAPABILITY_1 */
>  	bool read_caps;		/* Capability flags have been read */
>  
> +	bool vqmmc_enabled;	/* Vqmmc is enabled */

Last time around there was dissatisfaction with this variable name.  Perhaps
change it to sdhci_core_to_disable_vqmmc

>  	unsigned int            ocr_avail_sdio;	/* OCR bit masks */
>  	unsigned int            ocr_avail_sd;
>  	unsigned int            ocr_avail_mmc;
> 

Re: [PATCH V1 2/3] mmc: sdhci-msm: Use internal voltage control

[Veerabhadrarao Badiganti]

Thanks Bjorn for the review. For major comments I'm responding.
Other comments, i will take care of them in my next patch-set.

On 5/19/2020 1:27 AM, Bjorn Andersson wrote:
> On Fri 15 May 04:18 PDT 2020, Veerabhadrarao Badiganti wrote:
>
>> From: Vijay Viswanath <vviswana@codeaurora.org>
>>
>> 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.
>>
>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> Please note that per Documentation/process/submitting-patches.rst
> section 11) this states:
>
> 1) You wrote the patch (From:) without stating that its Certificate of
> origin.
>
> 2) Then Asutosh took your patch (in full or part) and guarantees that
> he's allowed to contribute it to the project.
>
> 3) Then you took his patch (in full or part) and guarantee that you're
> allowed to contribute it to the project.
>
> 4) Then Veerabhadrarao took your patch (in full or part) and guarantees
> that he's allowed to contribute it to the project
>
> 5) Then somehow it came out of your inbox - even if Veerabhadrarao was
> the one who handled it last.
>
>
> As author you should be the first one to certify, and as poster to LKML
> you should be the last one.
>
> If you worked together on this, then list Asutosh and Veerabhadrarao as
> Co-developed-by.
>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 215 +++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 207 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 97758fa..a10e955 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,9 @@ struct sdhci_msm_host {
>>   	bool use_cdr;
>>   	u32 transfer_mode;
>>   	bool updated_ddr_cfg;
>> +	u32 vmmc_load;
>> +	u32 vqmmc_load;
>> +	bool vqmmc_enabled;
>>   };
>>   
>>   static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>> @@ -1298,6 +1303,78 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>>   		sdhci_msm_hs400(host, &mmc->ios);
>>   }
>>   
>> +static int sdhci_msm_set_vmmc(struct sdhci_msm_host *msm_host,
>> +			      struct mmc_host *mmc, int level)
>> +{
>> +	int load, ret;
>> +
>> +	if (IS_ERR(mmc->supply.vmmc))
>> +		return 0;
>> +
>> +	if (msm_host->vmmc_load) {
>> +		load = level ? msm_host->vmmc_load : 0;
>> +		ret = regulator_set_load(mmc->supply.vmmc, load);
> I started on the comment about regulator_set_load() that you can find
> below...
>
>> +		if (ret)
>> +			goto out;
>> +	}
>> +
>> +	ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
> ...but I don't see that mmc->ios.vdd necessarily is in sync with
> "level". Or do you here simply set the load based on what the hardware
> tell you and then orthogonally to that let the core enable/disable the
> regulator?
>
> Perhaps I'm just missing something obvious, but if not I believe this
> warrants a comment describing that you're lowering the power level
> regardless of the actual power being disabled.

ios.vdd will be in sync with level. Vdd will be either 0 or a valid 
voltage (3v).

This indirectly gets triggered/invoked through power-irq when driver 
writes 0
or valid voltage to SDHCI_POWER_CONTROL register from 
sdhci_set_power_noreg().
>> +out:
>> +	if (ret)
>> +		pr_err("%s: vmmc set load/ocr failed: %d\n",
>> +				mmc_hostname(mmc), ret);
> Please use:
> 	dev_err(mmc_dev(mmc), ...);
>
>> +
>> +	return ret;
>> +}
>> +
>> +static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
>> +			      struct mmc_host *mmc, int level)
> vqmmc_enabled is a bool and "level" sounds like an int with several
> possible values. So please make level bool here as well, to make it
> easer to read..
>
>> +{
>> +	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);
> Since v5.0 the "load" of a regulator consumer is only taken into
> consideration if the consumer is enabled. So given that you're toggling
> the regulator below there's no need to change this here.
>
> Just specify the "active load" at probe time.

For eMMC case, we don't disable this Vccq2 regulator by having always-on 
flag
in the regulator node. Only for SDcard Vccq2 will be disabled.
Sice driver is common for both eMMC and SDcard, I have to set 0 load to make
it generic and to ensure eMMC Vccq2 regulator will be in LPM mode.

>
>> +		if (ret)
>> +			goto out;
>> +	}
>> +
>> +	/*
>> +	 * The IO voltage regulator may not always support a voltage close to
>> +	 * vdd. Set IO voltage based on capability of the regulator.
>> +	 */
> Is this comment related to the if/else-if inside the conditional? If so
> please move it one line down.
>
>> +	if (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;
> Please add a space here, to indicate that the if statement on the next
> line is unrelated to the if/elseif above.
>
>> +		if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
>> +			pr_debug("%s: %s: setting signal voltage: %d\n",
>> +					mmc_hostname(mmc), __func__,
>> +					ios.signal_voltage);
> I strongly believe you should replace these debug prints with
> tracepoints, throughout the driver.
>
>> +			ret = mmc_regulator_set_vqmmc(mmc, &ios);
>> +			if (ret < 0)
>> +				goto out;
>> +		}
>> +		ret = regulator_enable(mmc->supply.vqmmc);
>> +	} else {
>> +		ret = regulator_disable(mmc->supply.vqmmc);
>> +	}
> Given that you don't need to regulator_set_load() this function is now
> just one large if/else condition on a constant passed as an argument.
> Please split it into sdhci_msm_enable_vqmmc() and
> sdhci_msm_disable_vqmmc().


Same response as above
For eMMC case, we don't disable this Vccq2 regulator by having always-on 
flag
in the regulator node. Only for SDcard Vccq2 will be disabled.
Sice driver is common for both eMMC and SDcard, I have to set 0 load to make
it generic and to ensure eMMC Vccq2 regulator will be in LPM mode.

>> +out:
>> +	if (ret)
>> +		pr_err("%s: vqmmc failed: %d\n", mmc_hostname(mmc), ret);
> I think it would be useful to know if this error came from
> mmc_regulator_set_vqmmc() or regulator_enable() - or
> regulator_disable().
>
> So please move this up and add some context in the error message, and
> please use dev_err().
>
>> +	else
>> +		msm_host->vqmmc_enabled = level;
>> +
>> +	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 +1478,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 = 0;
> There's no need to initialize ret, in all occasions it's assigned before
> being read.
>
>>   	u32 pwr_state = 0, io_level = 0;
>>   	u32 config;
>>   	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
>> @@ -1438,14 +1516,35 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>>   
>>   	/* Handle BUS ON/OFF*/
>>   	if (irq_status & CORE_PWRCTL_BUS_ON) {
>> -		pwr_state = REQ_BUS_ON;
>> -		io_level = REQ_IO_HIGH;
>> -		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>> +		ret = sdhci_msm_set_vmmc(msm_host, mmc, 1);
>> +		if (!ret)
>> +			ret = sdhci_msm_set_vqmmc(msm_host, mmc, 1);
> I find this quite complex to follow. Wouldn't it be cleaner to retain
> the 4 checks on irq_status as they are and then before the writel of
> irq_ack check pwr_state and io_level and call sdhci_msm_set_{vmmc,vqmmc}
> accordingly?

I will see if i can update as you suggested.

>> +
>> +		if (!ret) {
>> +			pwr_state = REQ_BUS_ON;
>> +			io_level = REQ_IO_HIGH;
>> +			irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>> +		} else {
>> +			pr_err("%s: BUS_ON req failed(%d). irq_status: 0x%08x\n",
>> +					mmc_hostname(mmc), ret, irq_status);
> You already printed that this failed in sdhci_msm_set_{vmmc,vqmmc}, no
> need to print again.
>
>> +			irq_ack |= CORE_PWRCTL_BUS_FAIL;
>> +			sdhci_msm_set_vmmc(msm_host, mmc, 0);
>> +		}
>>   	}
>>   	if (irq_status & CORE_PWRCTL_BUS_OFF) {
>> -		pwr_state = REQ_BUS_OFF;
>> -		io_level = REQ_IO_LOW;
>> -		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>> +		ret = sdhci_msm_set_vmmc(msm_host, mmc, 0);
>> +		if (!ret)
>> +			ret = sdhci_msm_set_vqmmc(msm_host, mmc, 0);
>> +
>> +		if (!ret) {
>> +			pwr_state = REQ_BUS_OFF;
>> +			io_level = REQ_IO_LOW;
>> +			irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>> +		} else {
>> +			pr_err("%s: BUS_ON req failed(%d). irq_status: 0x%08x\n",
>> +					mmc_hostname(mmc), ret, irq_status);
>> +			irq_ack |= CORE_PWRCTL_BUS_FAIL;
>> +		}
>>   	}
>>   	/* Handle IO LOW/HIGH */
>>   	if (irq_status & CORE_PWRCTL_IO_LOW) {
>> @@ -1457,6 +1556,15 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>>   		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
>>   	}
>>   
>> +	if (io_level && !IS_ERR(mmc->supply.vqmmc) && !pwr_state) {
>> +		ret = mmc_regulator_set_vqmmc(mmc, &mmc->ios);
> Didn't you already call this through sdhci_msm_set_vqmmc()?

No.sdhci_msm_set_vqmmc handles only vqmmc ON/OFF. While turning it ON it 
sets
Vqmmc to possbile default IO level (1.8v or 3.0v).
Where this is only to make IO lines high (3.0v) or Low (1.8v).
>> +		if (ret < 0)
>> +			pr_err("%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);
>> +	}
>> +
>>   	/*
>>   	 * The driver has to acknowledge the interrupt, switch voltages and
>>   	 * report back if it succeded or not to this register. The voltage
>> @@ -1833,6 +1941,91 @@ 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 = 0;
> No need to initialize ret, first use is an assignment.
>
>> +	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",
>> +			&msm_host->vmmc_load);
>> +	device_property_read_u32(&msm_host->pdev->dev,
>> +			"vqmmc-max-load-microamp",
>> +			&msm_host->vqmmc_load);
> These properties are not documented. Do they vary enough to mandate
> being read from DT or could they simply be approximated by a define
> instead?

I can use defines. But since these values are different for eMMC and SDcard
I will have to maintain two sets and need to have logic during probe to
identify SDcard or eMMC and use the assign the set accordingly.
So we tought, getting from dt is simpler and clean.
In case Rob didn't agree with dt entries, I will go with this approach.

>> +
>> +	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;
>> +
>> +	/*
>> +	 * 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;
>> +		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +
>> +		/* 3.3V regulator output should be stable within 5 ms */
> What mechanism ensures that the readw won't return withing 5ms from the
> writew above?

Thanks for pointing this. This delay got missed. I will add it in next 
patchset.
>> +		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> +		if (!(ctrl & SDHCI_CTRL_VDD_180))
>> +			return 0;
>> +
>> +		pr_warn("%s: 3.3V regulator output did not became stable\n",
>> +			mmc_hostname(mmc));
>> +
>> +		return -EAGAIN;
> The body of the 330 and 180 cases are quite similar, can you perhaps
> deal with those after the switch, once?

Sure. Will check this.

>> +	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;
>> +		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +
>> +		/* 1.8V regulator output should be stable within 5 ms */
>> +		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> +		if (ctrl & SDHCI_CTRL_VDD_180)
>> +			return 0;
>> +
>> +		pr_warn("%s: 1.8V regulator output did not became stable\n",
>> +			mmc_hostname(mmc));
>> +
>> +		return -EAGAIN;
>> +	case MMC_SIGNAL_VOLTAGE_120:
>> +		if (!(host->flags & SDHCI_SIGNALING_120))
>> +			return -EINVAL;
>> +		return 0;
>> +	default:
>> +		/* No signal voltage switch required */
>> +		return 0;
>> +	}
>> +
> Empty line.
>
> Regards,
> Bjorn
>
>> +}
>> +
>>   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 +2073,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 +2266,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 +2314,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 +2323,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
>>

Re: [PATCH V1 3/3] mmc: sdhci: Allow platform controlled voltage switching

[Veerabhadrarao Badiganti]

On 5/19/2020 11:36 AM, Adrian Hunter wrote:
> On 15/05/20 2:18 pm, Veerabhadrarao Badiganti wrote:
>> From: Vijay Viswanath <vviswana@codeaurora.org>
>>
>> If vendor platform drivers are controlling whole logic of voltage
>> switching, then sdhci driver no need control vqmmc regulator.
>> So skip enabling/disable vqmmc from SDHC driver.
>>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>> ---
>>   drivers/mmc/host/sdhci.c | 32 +++++++++++++++++++-------------
>>   drivers/mmc/host/sdhci.h |  1 +
>>   2 files changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 1bb6b67..c010823 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -4098,6 +4098,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>>   	unsigned int override_timeout_clk;
>>   	u32 max_clk;
>>   	int ret;
>> +	bool enable_vqmmc = false;
>>   
>>   	WARN_ON(host == NULL);
>>   	if (host == NULL)
>> @@ -4111,9 +4112,12 @@ int sdhci_setup_host(struct sdhci_host *host)
>>   	 * the host can take the appropriate action if regulators are not
>>   	 * available.
>>   	 */
>> -	ret = mmc_regulator_get_supply(mmc);
>> -	if (ret)
>> -		return ret;
>> +	if (!mmc->supply.vqmmc) {
>> +		ret = mmc_regulator_get_supply(mmc);
>> +		if (ret)
>> +			return ret;
>> +		enable_vqmmc  = true;
>> +	}
>>   
>>   	DBG("Version:   0x%08x | Present:  0x%08x\n",
>>   	    sdhci_readw(host, SDHCI_HOST_VERSION),
>> @@ -4373,7 +4377,15 @@ int sdhci_setup_host(struct sdhci_host *host)
>>   		mmc->caps |= MMC_CAP_NEEDS_POLL;
>>   
>>   	if (!IS_ERR(mmc->supply.vqmmc)) {
>> -		ret = regulator_enable(mmc->supply.vqmmc);
>> +		if (enable_vqmmc) {
>> +			ret = regulator_enable(mmc->supply.vqmmc);
>> +			if (ret) {
>> +				pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
>> +					mmc_hostname(mmc), ret);
>> +				mmc->supply.vqmmc = ERR_PTR(-EINVAL);
>> +			}
>> +			host->vqmmc_enabled = !ret;
>> +		}
>>   
>>   		/* If vqmmc provides no 1.8V signalling, then there's no UHS */
>>   		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
>> @@ -4386,12 +4398,6 @@ int sdhci_setup_host(struct sdhci_host *host)
>>   		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 2700000,
>>   						    3600000))
>>   			host->flags &= ~SDHCI_SIGNALING_330;
>> -
>> -		if (ret) {
>> -			pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
>> -				mmc_hostname(mmc), ret);
>> -			mmc->supply.vqmmc = ERR_PTR(-EINVAL);
>> -		}
>>   	}
>>   
>>   	if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) {
>> @@ -4625,7 +4631,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>>   	return 0;
>>   
>>   unreg:
>> -	if (!IS_ERR(mmc->supply.vqmmc))
>> +	if (host->vqmmc_enabled)
>>   		regulator_disable(mmc->supply.vqmmc);
>>   undma:
>>   	if (host->align_buffer)
>> @@ -4643,7 +4649,7 @@ void sdhci_cleanup_host(struct sdhci_host *host)
>>   {
>>   	struct mmc_host *mmc = host->mmc;
>>   
>> -	if (!IS_ERR(mmc->supply.vqmmc))
>> +	if (host->vqmmc_enabled)
>>   		regulator_disable(mmc->supply.vqmmc);
>>   
>>   	if (host->align_buffer)
>> @@ -4780,7 +4786,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>>   
>>   	destroy_workqueue(host->complete_wq);
>>   
>> -	if (!IS_ERR(mmc->supply.vqmmc))
>> +	if (host->vqmmc_enabled)
>>   		regulator_disable(mmc->supply.vqmmc);
>>   
>>   	if (host->align_buffer)
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 8d2a096..24d27e1 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -570,6 +570,7 @@ struct sdhci_host {
>>   	u32 caps1;		/* CAPABILITY_1 */
>>   	bool read_caps;		/* Capability flags have been read */
>>   
>> +	bool vqmmc_enabled;	/* Vqmmc is enabled */
> Last time around there was dissatisfaction with this variable name.  Perhaps
> change it to sdhci_core_to_disable_vqmmc

Sure Adrian. Will update this variable name.

>
>>   	unsigned int            ocr_avail_sdio;	/* OCR bit masks */
>>   	unsigned int            ocr_avail_sd;
>>   	unsigned int            ocr_avail_mmc;
>>

[PATCH V2 0/3] Internal voltage control for qcom SDHC

[Veerabhadrarao Badiganti]

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.

So added support to register voltage regulators from the msm driver
and use them.

This patchset was posted long back but not actively pursued
https://lore.kernel.org/linux-arm-msm/1539004739-32060-1-git-send-email-vbadigan@codeaurora.org/
So posting it as fresh patchset.  

Changes since V1:
	- Removed setting load for Vmmc regulator while turning it on/off.
	  Instead setting the active load once during probe.
	- Simplified handlng of supplies for BUS_ON/OFF cases in shci_msm_handle_pwr_irq().
	- Moved common code out of switch case in sdhci_msm_start_signal_voltage_switch().
	- Updated variable name to sdhci_core_to_disable_vqmmc.
	- Updated pr_err logs to dev_err logs.

Veerabhadrarao Badiganti (1):
  dt-bindings: mmc: Supply max load for mmc supplies
  mmc: sdhci-msm: Use internal voltage control

Vijay Viswanath (1):
  mmc: sdhci: Allow platform controlled voltage switching

 .../devicetree/bindings/mmc/mmc-controller.yaml    |  16 ++
 drivers/mmc/host/sdhci-msm.c                       | 207 ++++++++++++++++++++-
 drivers/mmc/host/sdhci.c                           |  32 ++--
 drivers/mmc/host/sdhci.h                           |   1 +
 4 files changed, 234 insertions(+), 22 deletions(-)

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V2 1/3] dt-bindings: mmc: Supply max load for mmc supplies

[Veerabhadrarao Badiganti]

Supply the max load needed for driving the mmc supplies.

Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
---
 .../devicetree/bindings/mmc/mmc-controller.yaml          | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
index acc9f10..9058b82 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
+++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
@@ -290,6 +290,22 @@ properties:
     description:
       Supply for the bus IO line power
 
+  vmmc-max-load-microamp:
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - minimum: 0
+      - maximum: 1000000
+    description:
+      Maximum load for the card power.
+
+  vqmmc-max-load-microamp:
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - minimum: 0
+      - maximum: 1000000
+    description:
+      Maximum load for the bus IO line power.
+
   mmc-pwrseq:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V2 2/3] mmc: sdhci-msm: Use internal voltage control

[Veerabhadrarao Badiganti]

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 <asutoshd@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
Co-developed-by: Vijay Viswanath <vviswana@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
Co-developed-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
---
 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

[PATCH V2 3/3] mmc: sdhci: Allow platform controlled voltage switching

[Veerabhadrarao Badiganti]

From: Vijay Viswanath <vviswana@codeaurora.org>

If vendor platform drivers are controlling whole logic of voltage
switching, then sdhci driver no need control vqmmc regulator.
So skip enabling/disable vqmmc from SDHC driver.

Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
---
 drivers/mmc/host/sdhci.c | 32 +++++++++++++++++++-------------
 drivers/mmc/host/sdhci.h |  1 +
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1bb6b67..88e5312 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -4098,6 +4098,7 @@ int sdhci_setup_host(struct sdhci_host *host)
 	unsigned int override_timeout_clk;
 	u32 max_clk;
 	int ret;
+	bool enable_vqmmc = false;
 
 	WARN_ON(host == NULL);
 	if (host == NULL)
@@ -4111,9 +4112,12 @@ int sdhci_setup_host(struct sdhci_host *host)
 	 * the host can take the appropriate action if regulators are not
 	 * available.
 	 */
-	ret = mmc_regulator_get_supply(mmc);
-	if (ret)
-		return ret;
+	if (!mmc->supply.vqmmc) {
+		ret = mmc_regulator_get_supply(mmc);
+		if (ret)
+			return ret;
+		enable_vqmmc  = true;
+	}
 
 	DBG("Version:   0x%08x | Present:  0x%08x\n",
 	    sdhci_readw(host, SDHCI_HOST_VERSION),
@@ -4373,7 +4377,15 @@ int sdhci_setup_host(struct sdhci_host *host)
 		mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	if (!IS_ERR(mmc->supply.vqmmc)) {
-		ret = regulator_enable(mmc->supply.vqmmc);
+		if (enable_vqmmc) {
+			ret = regulator_enable(mmc->supply.vqmmc);
+			if (ret) {
+				pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
+					mmc_hostname(mmc), ret);
+				mmc->supply.vqmmc = ERR_PTR(-EINVAL);
+			}
+			host->sdhci_core_to_disable_vqmmc = !ret;
+		}
 
 		/* If vqmmc provides no 1.8V signalling, then there's no UHS */
 		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
@@ -4386,12 +4398,6 @@ int sdhci_setup_host(struct sdhci_host *host)
 		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 2700000,
 						    3600000))
 			host->flags &= ~SDHCI_SIGNALING_330;
-
-		if (ret) {
-			pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
-				mmc_hostname(mmc), ret);
-			mmc->supply.vqmmc = ERR_PTR(-EINVAL);
-		}
 	}
 
 	if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) {
@@ -4625,7 +4631,7 @@ int sdhci_setup_host(struct sdhci_host *host)
 	return 0;
 
 unreg:
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (host->sdhci_core_to_disable_vqmmc)
 		regulator_disable(mmc->supply.vqmmc);
 undma:
 	if (host->align_buffer)
@@ -4643,7 +4649,7 @@ void sdhci_cleanup_host(struct sdhci_host *host)
 {
 	struct mmc_host *mmc = host->mmc;
 
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (host->sdhci_core_to_disable_vqmmc)
 		regulator_disable(mmc->supply.vqmmc);
 
 	if (host->align_buffer)
@@ -4780,7 +4786,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 
 	destroy_workqueue(host->complete_wq);
 
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (host->sdhci_core_to_disable_vqmmc)
 		regulator_disable(mmc->supply.vqmmc);
 
 	if (host->align_buffer)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 8d2a096..c7dbc68 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -570,6 +570,7 @@ struct sdhci_host {
 	u32 caps1;		/* CAPABILITY_1 */
 	bool read_caps;		/* Capability flags have been read */
 
+	bool sdhci_core_to_disable_vqmmc;  /* sdhci core can disable vqmmc */
 	unsigned int            ocr_avail_sdio;	/* OCR bit masks */
 	unsigned int            ocr_avail_sd;
 	unsigned int            ocr_avail_mmc;
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V1 2/3] mmc: sdhci-msm: Use internal voltage control

[Bjorn Andersson]

On Wed 20 May 04:16 PDT 2020, Veerabhadrarao Badiganti wrote:

> 
> Thanks Bjorn for the review. For major comments I'm responding.
> Other comments, i will take care of them in my next patch-set.
> 
> On 5/19/2020 1:27 AM, Bjorn Andersson wrote:
> > On Fri 15 May 04:18 PDT 2020, Veerabhadrarao Badiganti wrote:
[..]
> > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
[..]
> > > +static int sdhci_msm_set_vmmc(struct sdhci_msm_host *msm_host,
> > > +			      struct mmc_host *mmc, int level)
> > > +{
> > > +	int load, ret;
> > > +
> > > +	if (IS_ERR(mmc->supply.vmmc))
> > > +		return 0;
> > > +
> > > +	if (msm_host->vmmc_load) {
> > > +		load = level ? msm_host->vmmc_load : 0;
> > > +		ret = regulator_set_load(mmc->supply.vmmc, load);
> > I started on the comment about regulator_set_load() that you can find
> > below...
> > 
> > > +		if (ret)
> > > +			goto out;
> > > +	}
> > > +
> > > +	ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
> > ...but I don't see that mmc->ios.vdd necessarily is in sync with
> > "level". Or do you here simply set the load based on what the hardware
> > tell you and then orthogonally to that let the core enable/disable the
> > regulator?
> > 
> > Perhaps I'm just missing something obvious, but if not I believe this
> > warrants a comment describing that you're lowering the power level
> > regardless of the actual power being disabled.
> 
> ios.vdd will be in sync with level. Vdd will be either 0 or a valid voltage
> (3v).
> 
> This indirectly gets triggered/invoked through power-irq when driver writes
> 0
> or valid voltage to SDHCI_POWER_CONTROL register from
> sdhci_set_power_noreg().

Ok, thanks for explaining.

> > > +out:
> > > +	if (ret)
> > > +		pr_err("%s: vmmc set load/ocr failed: %d\n",
> > > +				mmc_hostname(mmc), ret);
> > Please use:
> > 	dev_err(mmc_dev(mmc), ...);
> > 
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
> > > +			      struct mmc_host *mmc, int level)
> > vqmmc_enabled is a bool and "level" sounds like an int with several
> > possible values. So please make level bool here as well, to make it
> > easer to read..
> > 
> > > +{
> > > +	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);
> > Since v5.0 the "load" of a regulator consumer is only taken into
> > consideration if the consumer is enabled. So given that you're toggling
> > the regulator below there's no need to change this here.
> > 
> > Just specify the "active load" at probe time.
> 
> For eMMC case, we don't disable this Vccq2 regulator by having always-on
> flag
> in the regulator node. Only for SDcard Vccq2 will be disabled.
> Sice driver is common for both eMMC and SDcard, I have to set 0 load to make
> it generic and to ensure eMMC Vccq2 regulator will be in LPM mode.
> 

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.

> > 
> > > +		if (ret)
> > > +			goto out;
> > > +	}
> > > +
> > > +	/*
> > > +	 * The IO voltage regulator may not always support a voltage close to
> > > +	 * vdd. Set IO voltage based on capability of the regulator.
> > > +	 */
> > Is this comment related to the if/else-if inside the conditional? If so
> > please move it one line down.
> > 
> > > +	if (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;
> > Please add a space here, to indicate that the if statement on the next
> > line is unrelated to the if/elseif above.
> > 
> > > +		if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
> > > +			pr_debug("%s: %s: setting signal voltage: %d\n",
> > > +					mmc_hostname(mmc), __func__,
> > > +					ios.signal_voltage);
> > I strongly believe you should replace these debug prints with
> > tracepoints, throughout the driver.
> > 
> > > +			ret = mmc_regulator_set_vqmmc(mmc, &ios);
> > > +			if (ret < 0)
> > > +				goto out;
> > > +		}
> > > +		ret = regulator_enable(mmc->supply.vqmmc);
> > > +	} else {
> > > +		ret = regulator_disable(mmc->supply.vqmmc);
> > > +	}
> > Given that you don't need to regulator_set_load() this function is now
> > just one large if/else condition on a constant passed as an argument.
> > Please split it into sdhci_msm_enable_vqmmc() and
> > sdhci_msm_disable_vqmmc().
> 
> 
> Same response as above
> For eMMC case, we don't disable this Vccq2 regulator by having always-on
> flag
> in the regulator node. Only for SDcard Vccq2 will be disabled.
> Sice driver is common for both eMMC and SDcard, I have to set 0 load to make
> it generic and to ensure eMMC Vccq2 regulator will be in LPM mode.
> 
> > > +out:
> > > +	if (ret)
> > > +		pr_err("%s: vqmmc failed: %d\n", mmc_hostname(mmc), ret);
> > I think it would be useful to know if this error came from
> > mmc_regulator_set_vqmmc() or regulator_enable() - or
> > regulator_disable().
> > 
> > So please move this up and add some context in the error message, and
> > please use dev_err().
> > 
> > > +	else
> > > +		msm_host->vqmmc_enabled = level;
> > > +
> > > +	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 +1478,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 = 0;
> > There's no need to initialize ret, in all occasions it's assigned before
> > being read.
> > 
> > >   	u32 pwr_state = 0, io_level = 0;
> > >   	u32 config;
> > >   	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
> > > @@ -1438,14 +1516,35 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
> > >   	/* Handle BUS ON/OFF*/
> > >   	if (irq_status & CORE_PWRCTL_BUS_ON) {
> > > -		pwr_state = REQ_BUS_ON;
> > > -		io_level = REQ_IO_HIGH;
> > > -		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> > > +		ret = sdhci_msm_set_vmmc(msm_host, mmc, 1);
> > > +		if (!ret)
> > > +			ret = sdhci_msm_set_vqmmc(msm_host, mmc, 1);
> > I find this quite complex to follow. Wouldn't it be cleaner to retain
> > the 4 checks on irq_status as they are and then before the writel of
> > irq_ack check pwr_state and io_level and call sdhci_msm_set_{vmmc,vqmmc}
> > accordingly?
> 
> I will see if i can update as you suggested.
> 
> > > +
> > > +		if (!ret) {
> > > +			pwr_state = REQ_BUS_ON;
> > > +			io_level = REQ_IO_HIGH;
> > > +			irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> > > +		} else {
> > > +			pr_err("%s: BUS_ON req failed(%d). irq_status: 0x%08x\n",
> > > +					mmc_hostname(mmc), ret, irq_status);
> > You already printed that this failed in sdhci_msm_set_{vmmc,vqmmc}, no
> > need to print again.
> > 
> > > +			irq_ack |= CORE_PWRCTL_BUS_FAIL;
> > > +			sdhci_msm_set_vmmc(msm_host, mmc, 0);
> > > +		}
> > >   	}
> > >   	if (irq_status & CORE_PWRCTL_BUS_OFF) {
> > > -		pwr_state = REQ_BUS_OFF;
> > > -		io_level = REQ_IO_LOW;
> > > -		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> > > +		ret = sdhci_msm_set_vmmc(msm_host, mmc, 0);
> > > +		if (!ret)
> > > +			ret = sdhci_msm_set_vqmmc(msm_host, mmc, 0);
> > > +
> > > +		if (!ret) {
> > > +			pwr_state = REQ_BUS_OFF;
> > > +			io_level = REQ_IO_LOW;
> > > +			irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> > > +		} else {
> > > +			pr_err("%s: BUS_ON req failed(%d). irq_status: 0x%08x\n",
> > > +					mmc_hostname(mmc), ret, irq_status);
> > > +			irq_ack |= CORE_PWRCTL_BUS_FAIL;
> > > +		}
> > >   	}
> > >   	/* Handle IO LOW/HIGH */
> > >   	if (irq_status & CORE_PWRCTL_IO_LOW) {
> > > @@ -1457,6 +1556,15 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
> > >   		irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> > >   	}
> > > +	if (io_level && !IS_ERR(mmc->supply.vqmmc) && !pwr_state) {
> > > +		ret = mmc_regulator_set_vqmmc(mmc, &mmc->ios);
> > Didn't you already call this through sdhci_msm_set_vqmmc()?
> 
> No.sdhci_msm_set_vqmmc handles only vqmmc ON/OFF. While turning it ON it
> sets
> Vqmmc to possbile default IO level (1.8v or 3.0v).
> Where this is only to make IO lines high (3.0v) or Low (1.8v).

If you move both the regulator operations here (below the point where
you figure out pwr_state and io_level), wouldn't it be possible to avoid
the additional, nested, vqmmc voltage request?

> > > +		if (ret < 0)
> > > +			pr_err("%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);
> > > +	}
> > > +
> > >   	/*
> > >   	 * The driver has to acknowledge the interrupt, switch voltages and
> > >   	 * report back if it succeded or not to this register. The voltage
> > > @@ -1833,6 +1941,91 @@ 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 = 0;
> > No need to initialize ret, first use is an assignment.
> > 
> > > +	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",
> > > +			&msm_host->vmmc_load);
> > > +	device_property_read_u32(&msm_host->pdev->dev,
> > > +			"vqmmc-max-load-microamp",
> > > +			&msm_host->vqmmc_load);
> > These properties are not documented. Do they vary enough to mandate
> > being read from DT or could they simply be approximated by a define
> > instead?
> 
> I can use defines. But since these values are different for eMMC and SDcard
> I will have to maintain two sets and need to have logic during probe to
> identify SDcard or eMMC and use the assign the set accordingly.
> So we tought, getting from dt is simpler and clean.
> In case Rob didn't agree with dt entries, I will go with this approach.
> 

Sounds reasonable, let's see what Rob says.

> > > +
> > > +	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;
> > > +
> > > +	/*
> > > +	 * 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;
> > > +		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> > > +
> > > +		/* 3.3V regulator output should be stable within 5 ms */
> > What mechanism ensures that the readw won't return withing 5ms from the
> > writew above?
> 
> Thanks for pointing this. This delay got missed. I will add it in next
> patchset.

Nice, thanks.

Regards,
Bjorn

Re: [PATCH V2 2/3] mmc: sdhci-msm: Use internal voltage control

[Bjorn Andersson]

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 <asutoshd@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Co-developed-by: Vijay Viswanath <vviswana@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> Co-developed-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>

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

Re: [PATCH V2 2/3] mmc: sdhci-msm: Use internal voltage control

[Veerabhadrarao Badiganti]

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 <asutoshd@codeaurora.org>
>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>> Co-developed-by: Vijay Viswanath <vviswana@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> Co-developed-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> 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

Re: [PATCH V2 2/3] mmc: sdhci-msm: Use internal voltage control

[Bjorn Andersson]

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 <asutoshd@codeaurora.org>
> > > Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> > > Co-developed-by: Vijay Viswanath <vviswana@codeaurora.org>
> > > Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> > > Co-developed-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> > > Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> > 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;
> > > +}

Re: [PATCH V2 3/3] mmc: sdhci: Allow platform controlled voltage switching

[Adrian Hunter]

On 21/05/20 6:23 pm, Veerabhadrarao Badiganti wrote:
> From: Vijay Viswanath <vviswana@codeaurora.org>
> 
> If vendor platform drivers are controlling whole logic of voltage
> switching, then sdhci driver no need control vqmmc regulator.
> So skip enabling/disable vqmmc from SDHC driver.
> 
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci.c | 32 +++++++++++++++++++-------------
>  drivers/mmc/host/sdhci.h |  1 +
>  2 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1bb6b67..88e5312 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -4098,6 +4098,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>  	unsigned int override_timeout_clk;
>  	u32 max_clk;
>  	int ret;
> +	bool enable_vqmmc = false;
>  
>  	WARN_ON(host == NULL);
>  	if (host == NULL)
> @@ -4111,9 +4112,12 @@ int sdhci_setup_host(struct sdhci_host *host)
>  	 * the host can take the appropriate action if regulators are not
>  	 * available.
>  	 */
> -	ret = mmc_regulator_get_supply(mmc);
> -	if (ret)
> -		return ret;
> +	if (!mmc->supply.vqmmc) {
> +		ret = mmc_regulator_get_supply(mmc);
> +		if (ret)
> +			return ret;
> +		enable_vqmmc  = true;
> +	}
>  
>  	DBG("Version:   0x%08x | Present:  0x%08x\n",
>  	    sdhci_readw(host, SDHCI_HOST_VERSION),
> @@ -4373,7 +4377,15 @@ int sdhci_setup_host(struct sdhci_host *host)
>  		mmc->caps |= MMC_CAP_NEEDS_POLL;
>  
>  	if (!IS_ERR(mmc->supply.vqmmc)) {
> -		ret = regulator_enable(mmc->supply.vqmmc);
> +		if (enable_vqmmc) {
> +			ret = regulator_enable(mmc->supply.vqmmc);
> +			if (ret) {
> +				pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
> +					mmc_hostname(mmc), ret);
> +				mmc->supply.vqmmc = ERR_PTR(-EINVAL);
> +			}
> +			host->sdhci_core_to_disable_vqmmc = !ret;
> +		}
>  
>  		/* If vqmmc provides no 1.8V signalling, then there's no UHS */
>  		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
> @@ -4386,12 +4398,6 @@ int sdhci_setup_host(struct sdhci_host *host)
>  		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 2700000,
>  						    3600000))
>  			host->flags &= ~SDHCI_SIGNALING_330;
> -
> -		if (ret) {
> -			pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
> -				mmc_hostname(mmc), ret);
> -			mmc->supply.vqmmc = ERR_PTR(-EINVAL);
> -		}
>  	}
>  
>  	if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) {
> @@ -4625,7 +4631,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>  	return 0;
>  
>  unreg:
> -	if (!IS_ERR(mmc->supply.vqmmc))
> +	if (host->sdhci_core_to_disable_vqmmc)
>  		regulator_disable(mmc->supply.vqmmc);
>  undma:
>  	if (host->align_buffer)
> @@ -4643,7 +4649,7 @@ void sdhci_cleanup_host(struct sdhci_host *host)
>  {
>  	struct mmc_host *mmc = host->mmc;
>  
> -	if (!IS_ERR(mmc->supply.vqmmc))
> +	if (host->sdhci_core_to_disable_vqmmc)
>  		regulator_disable(mmc->supply.vqmmc);
>  
>  	if (host->align_buffer)
> @@ -4780,7 +4786,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>  
>  	destroy_workqueue(host->complete_wq);
>  
> -	if (!IS_ERR(mmc->supply.vqmmc))
> +	if (host->sdhci_core_to_disable_vqmmc)
>  		regulator_disable(mmc->supply.vqmmc);
>  
>  	if (host->align_buffer)
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 8d2a096..c7dbc68 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -570,6 +570,7 @@ struct sdhci_host {
>  	u32 caps1;		/* CAPABILITY_1 */
>  	bool read_caps;		/* Capability flags have been read */
>  
> +	bool sdhci_core_to_disable_vqmmc;  /* sdhci core can disable vqmmc */
>  	unsigned int            ocr_avail_sdio;	/* OCR bit masks */
>  	unsigned int            ocr_avail_sd;
>  	unsigned int            ocr_avail_mmc;
> 

Re: [PATCH V2 2/3] mmc: sdhci-msm: Use internal voltage control

[Veerabhadrarao Badiganti]

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 <asutoshd@codeaurora.org>
>>>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>>>> Co-developed-by: Vijay Viswanath <vviswana@codeaurora.org>
>>>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>>>> Co-developed-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>>>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>>> 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;
>>>> +}

Re: [PATCH V2 1/3] dt-bindings: mmc: Supply max load for mmc supplies

[Rob Herring]

On Thu, May 21, 2020 at 08:53:33PM +0530, Veerabhadrarao Badiganti wrote:
> Supply the max load needed for driving the mmc supplies.
> 
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> ---
>  .../devicetree/bindings/mmc/mmc-controller.yaml          | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> index acc9f10..9058b82 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> +++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> @@ -290,6 +290,22 @@ properties:
>      description:
>        Supply for the bus IO line power
>  
> +  vmmc-max-load-microamp:

Seems like this should be a common regulator property (it would have to 
be a suffix to match up with *-supply).

> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32

Properties with unit suffix already have a type.

> +      - minimum: 0
> +      - maximum: 1000000
> +    description:
> +      Maximum load for the card power.
> +
> +  vqmmc-max-load-microamp:
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - minimum: 0
> +      - maximum: 1000000
> +    description:
> +      Maximum load for the bus IO line power.
> +
>    mmc-pwrseq:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description:
> -- 
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
> 

[PATCH V3 0/3] Internal voltage control for qcom SDHC

[Veerabhadrarao Badiganti]

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.

So added support to register voltage regulators from the msm driver
and use them.

This patchset was posted long back but not actively pursued
https://lore.kernel.org/linux-arm-msm/1539004739-32060-1-git-send-email-vbadigan@codeaurora.org/
So posting it as fresh patchset.  

Changes since V2:
	- Removed redundant log from sdhci_msm_set_vmmc.
	- Added the condition for skiping disabling of vqmmc for eMMC.
	- Updated logic such that, setting load for vqmmc only if
	  it is kept ON. 
	- Retained ack by Adrian for second patch.
	- Updated dt properties names as per Robs comments.

Changes since V1:
	- Removed setting load for Vmmc regulator while turning it on/off.
	  Instead setting the active load once during probe.
	- Simplified handlng of supplies for BUS_ON/OFF cases in shci_msm_handle_pwr_irq().
	- Moved common code out of switch case in sdhci_msm_start_signal_voltage_switch().
	- Updated variable name to sdhci_core_to_disable_vqmmc.
	- Updated pr_err logs to dev_err logs.

Veerabhadrarao Badiganti (2):
  dt-bindings: mmc: Supply max load for mmc supplies
  mmc: sdhci-msm: Use internal voltage control

Vijay Viswanath (1):
  mmc: sdhci: Allow platform controlled voltage switching

 .../devicetree/bindings/mmc/mmc-controller.yaml    |   6 +
 drivers/mmc/host/sdhci-msm.c                       | 235 ++++++++++++++++++++-
 drivers/mmc/host/sdhci.c                           |  32 +--
 drivers/mmc/host/sdhci.h                           |   1 +
 4 files changed, 252 insertions(+), 22 deletions(-)

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V3 1/3] dt-bindings: mmc: Supply max load for mmc supplies

[Veerabhadrarao Badiganti]

Supply the max load needed for driving the mmc supplies.

Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
---
 Documentation/devicetree/bindings/mmc/mmc-controller.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
index acc9f10871d4..d95219721fa1 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
+++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
@@ -290,6 +290,12 @@ properties:
     description:
       Supply for the bus IO line power
 
+  vmmc-supply-max-microamp:
+    description: Maximum load for the card power.
+
+  vqmmc-supply-max-microamp:
+    description: Maximum load for the bus IO line power.
+
   mmc-pwrseq:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V3 2/3] mmc: sdhci: Allow platform controlled voltage switching

[Veerabhadrarao Badiganti]

From: Vijay Viswanath <vviswana@codeaurora.org>

If vendor platform drivers are controlling whole logic of voltage
switching, then sdhci driver no need control vqmmc regulator.
So skip enabling/disable vqmmc from SDHC driver.

Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci.c | 32 +++++++++++++++++++-------------
 drivers/mmc/host/sdhci.h |  1 +
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 37b1158c1c0c..e6275c2202b0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -4105,6 +4105,7 @@ int sdhci_setup_host(struct sdhci_host *host)
 	unsigned int override_timeout_clk;
 	u32 max_clk;
 	int ret;
+	bool enable_vqmmc = false;
 
 	WARN_ON(host == NULL);
 	if (host == NULL)
@@ -4118,9 +4119,12 @@ int sdhci_setup_host(struct sdhci_host *host)
 	 * the host can take the appropriate action if regulators are not
 	 * available.
 	 */
-	ret = mmc_regulator_get_supply(mmc);
-	if (ret)
-		return ret;
+	if (!mmc->supply.vqmmc) {
+		ret = mmc_regulator_get_supply(mmc);
+		if (ret)
+			return ret;
+		enable_vqmmc  = true;
+	}
 
 	DBG("Version:   0x%08x | Present:  0x%08x\n",
 	    sdhci_readw(host, SDHCI_HOST_VERSION),
@@ -4377,7 +4381,15 @@ int sdhci_setup_host(struct sdhci_host *host)
 		mmc->caps |= MMC_CAP_NEEDS_POLL;
 
 	if (!IS_ERR(mmc->supply.vqmmc)) {
-		ret = regulator_enable(mmc->supply.vqmmc);
+		if (enable_vqmmc) {
+			ret = regulator_enable(mmc->supply.vqmmc);
+			if (ret) {
+				pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
+					mmc_hostname(mmc), ret);
+				mmc->supply.vqmmc = ERR_PTR(-EINVAL);
+			}
+			host->sdhci_core_to_disable_vqmmc = !ret;
+		}
 
 		/* If vqmmc provides no 1.8V signalling, then there's no UHS */
 		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
@@ -4390,12 +4402,6 @@ int sdhci_setup_host(struct sdhci_host *host)
 		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 2700000,
 						    3600000))
 			host->flags &= ~SDHCI_SIGNALING_330;
-
-		if (ret) {
-			pr_warn("%s: Failed to enable vqmmc regulator: %d\n",
-				mmc_hostname(mmc), ret);
-			mmc->supply.vqmmc = ERR_PTR(-EINVAL);
-		}
 	}
 
 	if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) {
@@ -4626,7 +4632,7 @@ int sdhci_setup_host(struct sdhci_host *host)
 	return 0;
 
 unreg:
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (host->sdhci_core_to_disable_vqmmc)
 		regulator_disable(mmc->supply.vqmmc);
 undma:
 	if (host->align_buffer)
@@ -4644,7 +4650,7 @@ void sdhci_cleanup_host(struct sdhci_host *host)
 {
 	struct mmc_host *mmc = host->mmc;
 
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (host->sdhci_core_to_disable_vqmmc)
 		regulator_disable(mmc->supply.vqmmc);
 
 	if (host->align_buffer)
@@ -4787,7 +4793,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 
 	destroy_workqueue(host->complete_wq);
 
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (host->sdhci_core_to_disable_vqmmc)
 		regulator_disable(mmc->supply.vqmmc);
 
 	if (host->align_buffer)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0008bbd27127..0770c036e2ff 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -567,6 +567,7 @@ struct sdhci_host {
 	u32 caps1;		/* CAPABILITY_1 */
 	bool read_caps;		/* Capability flags have been read */
 
+	bool sdhci_core_to_disable_vqmmc;  /* sdhci core can disable vqmmc */
 	unsigned int            ocr_avail_sdio;	/* OCR bit masks */
 	unsigned int            ocr_avail_sd;
 	unsigned int            ocr_avail_mmc;
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[PATCH V3 3/3] mmc: sdhci-msm: Use internal voltage control

[Veerabhadrarao Badiganti]

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 <asutoshd@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
Co-developed-by: Vijay Viswanath <vviswana@codeaurora.org>
Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
Co-developed-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 235 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 226 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 95cd9735e9a3..20ef90fc7dd7 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)
@@ -277,6 +279,8 @@ struct sdhci_msm_host {
 	bool uses_tassadar_dll;
 	u32 dll_config;
 	u32 ddr_config;
+	u32 vqmmc_load;
+	bool vqmmc_enabled;
 };
 
 static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
@@ -1339,6 +1343,91 @@ 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)
+{
+	if (IS_ERR(mmc->supply.vmmc))
+		return 0;
+
+	return mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
+}
+
+static int msm_toggle_vqmmc(struct sdhci_msm_host *msm_host,
+			      struct mmc_host *mmc, bool level)
+{
+	int ret;
+	struct mmc_ios ios;
+
+	if (msm_host->vqmmc_enabled == level)
+		return 0;
+
+	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 msm_config_vqmmc_mode(struct sdhci_msm_host *msm_host,
+			      struct mmc_host *mmc, bool hpm)
+{
+	int load, ret;
+
+	if (!msm_host->vqmmc_load)
+		return 0;
+
+	load = hpm ? 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);
+	return ret;
+}
+
+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 amonut 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;
+}
+
 static inline void sdhci_msm_init_pwr_irq_wait(struct sdhci_msm_host *msm_host)
 {
 	init_waitqueue_head(&msm_host->pwr_irq_wait);
@@ -1442,8 +1531,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;
@@ -1481,21 +1571,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;
+		}
 	}
 
 	/*
@@ -1544,7 +1655,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);
 }
@@ -1874,6 +1985,106 @@ 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-supply-max-microamp",
+			&vmmc_load);
+	device_property_read_u32(&msm_host->pdev->dev,
+			"vqmmc-supply-max-microamp",
+			&msm_host->vqmmc_load);
+
+	/* Set active load */
+	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;
+		}
+	}
+	if (!IS_ERR(mmc->supply.vqmmc) && msm_host->vqmmc_load) {
+		ret = regulator_set_load(mmc->supply.vmmc,
+					msm_host->vqmmc_load);
+		if (ret) {
+			dev_err(mmc_dev(mmc), "%s: vqmmc 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;
+}
+
 #define DRIVER_NAME "sdhci_msm"
 #define SDHCI_MSM_DUMP(f, x...) \
 	pr_err("%s: " DRIVER_NAME ": " f, mmc_hostname(host->mmc), ## x)
@@ -1960,6 +2171,7 @@ void sdhci_msm_dump_vendor_regs(struct sdhci_host *host)
 	.write_b = sdhci_msm_writeb,
 	.irq	= sdhci_msm_cqe_irq,
 	.dump_vendor_regs = sdhci_msm_dump_vendor_regs,
+	.set_power = sdhci_set_power_noreg,
 };
 
 static const struct sdhci_pltfm_data sdhci_msm_pdata = {
@@ -2169,6 +2381,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
@@ -2213,6 +2429,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);
@@ -2220,7 +2438,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

Re: [PATCH V3 3/3] mmc: sdhci-msm: Use internal voltage control

[Veerabhadrarao Badiganti]

Hi Bjorn,

Do you have any comments on V3 patchset?

Thanks
Veera

On 6/2/2020 4:17 PM, 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 <asutoshd@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Co-developed-by: Vijay Viswanath <vviswana@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> Co-developed-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> ---
>   drivers/mmc/host/sdhci-msm.c | 235 +++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 226 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 95cd9735e9a3..20ef90fc7dd7 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)
> @@ -277,6 +279,8 @@ struct sdhci_msm_host {
>   	bool uses_tassadar_dll;
>   	u32 dll_config;
>   	u32 ddr_config;
> +	u32 vqmmc_load;
> +	bool vqmmc_enabled;
>   };
>   
>   static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> @@ -1339,6 +1343,91 @@ 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)
> +{
> +	if (IS_ERR(mmc->supply.vmmc))
> +		return 0;
> +
> +	return mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
> +}
> +
> +static int msm_toggle_vqmmc(struct sdhci_msm_host *msm_host,
> +			      struct mmc_host *mmc, bool level)
> +{
> +	int ret;
> +	struct mmc_ios ios;
> +
> +	if (msm_host->vqmmc_enabled == level)
> +		return 0;
> +
> +	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 msm_config_vqmmc_mode(struct sdhci_msm_host *msm_host,
> +			      struct mmc_host *mmc, bool hpm)
> +{
> +	int load, ret;
> +
> +	if (!msm_host->vqmmc_load)
> +		return 0;
> +
> +	load = hpm ? 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);
> +	return ret;
> +}
> +
> +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 amonut 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;
> +}
> +
>   static inline void sdhci_msm_init_pwr_irq_wait(struct sdhci_msm_host *msm_host)
>   {
>   	init_waitqueue_head(&msm_host->pwr_irq_wait);
> @@ -1442,8 +1531,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;
> @@ -1481,21 +1571,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;
> +		}
>   	}
>   
>   	/*
> @@ -1544,7 +1655,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);
>   }
> @@ -1874,6 +1985,106 @@ 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-supply-max-microamp",
> +			&vmmc_load);
> +	device_property_read_u32(&msm_host->pdev->dev,
> +			"vqmmc-supply-max-microamp",
> +			&msm_host->vqmmc_load);
> +
> +	/* Set active load */
> +	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;
> +		}
> +	}
> +	if (!IS_ERR(mmc->supply.vqmmc) && msm_host->vqmmc_load) {
> +		ret = regulator_set_load(mmc->supply.vmmc,
> +					msm_host->vqmmc_load);
> +		if (ret) {
> +			dev_err(mmc_dev(mmc), "%s: vqmmc 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;
> +}
> +
>   #define DRIVER_NAME "sdhci_msm"
>   #define SDHCI_MSM_DUMP(f, x...) \
>   	pr_err("%s: " DRIVER_NAME ": " f, mmc_hostname(host->mmc), ## x)
> @@ -1960,6 +2171,7 @@ void sdhci_msm_dump_vendor_regs(struct sdhci_host *host)
>   	.write_b = sdhci_msm_writeb,
>   	.irq	= sdhci_msm_cqe_irq,
>   	.dump_vendor_regs = sdhci_msm_dump_vendor_regs,
> +	.set_power = sdhci_set_power_noreg,
>   };
>   
>   static const struct sdhci_pltfm_data sdhci_msm_pdata = {
> @@ -2169,6 +2381,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
> @@ -2213,6 +2429,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);
> @@ -2220,7 +2438,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);

Re: [PATCH V3 1/3] dt-bindings: mmc: Supply max load for mmc supplies

[Rob Herring]

+Mark B

On Tue, Jun 02, 2020 at 04:17:54PM +0530, Veerabhadrarao Badiganti wrote:
> Supply the max load needed for driving the mmc supplies.
> 
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/mmc/mmc-controller.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> index acc9f10871d4..d95219721fa1 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> +++ b/Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> @@ -290,6 +290,12 @@ properties:
>      description:
>        Supply for the bus IO line power
>  
> +  vmmc-supply-max-microamp:
> +    description: Maximum load for the card power.
> +
> +  vqmmc-supply-max-microamp:
> +    description: Maximum load for the bus IO line power.

By a 'common regulator property' I meant documented with regulator 
binding like *-supply, not common to MMC. How is MMC special?

Thinking about this some more, what's wrong with the max current in the 
regulator nodes? I suppose you could have more than one load and need to 
define the loads separately?

Rob

Re: [PATCH V3 1/3] dt-bindings: mmc: Supply max load for mmc supplies

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

On Tue, Jun 09, 2020 at 05:02:16PM -0600, Rob Herring wrote:
> On Tue, Jun 02, 2020 at 04:17:54PM +0530, Veerabhadrarao Badiganti wrote:

> > +  vmmc-supply-max-microamp:
> > +    description: Maximum load for the card power.

> > +  vqmmc-supply-max-microamp:
> > +    description: Maximum load for the bus IO line power.

> By a 'common regulator property' I meant documented with regulator 
> binding like *-supply, not common to MMC. How is MMC special?

TBH I'm surprised that these aren't defined by the MMC spec or by the ID
information from the part we find connected - I'd not expect the board
to be defining these at all.

> Thinking about this some more, what's wrong with the max current in the 
> regulator nodes? I suppose you could have more than one load and need to 
> define the loads separately?

One of the bigger reasons to think about the loads would be to
dynamically configure the mode the regulator is in to go into a more
efficient mode when some of the devices attached to it are turned off.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]