[PATCH V1 0/3] Internal voltage control for platform drivers

[PATCH V1 0/3] Internal voltage control for platform drivers

[Vijay Viswanath]

Certain SDHC controllers may require that voltage switching happen after
sepcial conditions. Added a QUIRK for such controllers to use.

For SDHCI-MSM controllers, power irq is a signal from controller to SW
that it is ready for voltage switch. So added support to register
voltage regulators from the msm driver and use them.
Voltage switching from core layer is causing CRC/cmd timeout errors in
some chipsets.

Changes since RFC:
	- Added DT option to pass regulator load values the sdhci-msm driver
	should vote during BUS_ON and BUS_OFF power irq.
	- Removed quirk and used local flags in sdhci_add_host to to avoid
	scenario of both sdhci & sdhci-msm layer controlling the regulators.
	- Introduced two sdhci msm layer APIs to replace the respective
	sdhci layer APIs. This is again to stop sdhci layer from
	controlling regulators if sdhci_msm layer is present.

Tested on: sdm845
Requies patch series:"[PATCH V3 0/4] Changes for SDCC5 version"

Vijay Viswanath (3):
  mmc: sdhci: Allow platform controlled voltage switching
  Documentation: sdhci-msm: Add entries for passing load values
  mmc: sdhci-msm: Use internal voltage control

 .../devicetree/bindings/mmc/sdhci-msm.txt          |   6 +
 drivers/mmc/host/sdhci-msm.c                       | 220 +++++++++++++++++++--
 drivers/mmc/host/sdhci.c                           |  21 +-
 3 files changed, 231 insertions(+), 16 deletions(-)

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

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

[Vijay Viswanath]

Some controllers can have internal mechanism to inform the SW that it
is ready for voltage switching. For such controllers, changing voltage
before the HW is ready can result in various issues.

During setup/cleanup of host, check whether regulator enable/disable
was already done by platform driver.

Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 drivers/mmc/host/sdhci.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1c828e0..494a1e2 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3472,6 +3472,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)
@@ -3485,7 +3486,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 (!mmc->supply.vmmc) {
+		ret = mmc_regulator_get_supply(mmc);
+		enable_vqmmc  = true;
+	} else {
+		ret = 0;
+	}
 	if (ret)
 		return ret;
 
@@ -3736,7 +3742,10 @@ int sdhci_setup_host(struct sdhci_host *host)
 
 	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
 	if (!IS_ERR(mmc->supply.vqmmc)) {
-		ret = regulator_enable(mmc->supply.vqmmc);
+		if (enable_vqmmc)
+			ret = regulator_enable(mmc->supply.vqmmc);
+		else
+			ret = 0;
 		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
 						    1950000))
 			host->caps1 &= ~(SDHCI_SUPPORT_SDR104 |
@@ -3984,7 +3993,7 @@ int sdhci_setup_host(struct sdhci_host *host)
 	return 0;
 
 unreg:
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (!IS_ERR(mmc->supply.vqmmc) && enable_vqmmc)
 		regulator_disable(mmc->supply.vqmmc);
 undma:
 	if (host->align_buffer)
@@ -4002,7 +4011,8 @@ void sdhci_cleanup_host(struct sdhci_host *host)
 {
 	struct mmc_host *mmc = host->mmc;
 
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (!IS_ERR(mmc->supply.vqmmc) &&
+			(regulator_is_enabled(mmc->supply.vqmmc) > 0))
 		regulator_disable(mmc->supply.vqmmc);
 
 	if (host->align_buffer)
@@ -4135,7 +4145,8 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 
 	tasklet_kill(&host->finish_tasklet);
 
-	if (!IS_ERR(mmc->supply.vqmmc))
+	if (!IS_ERR(mmc->supply.vqmmc) &&
+			(regulator_is_enabled(mmc->supply.vqmmc) > 0))
 		regulator_disable(mmc->supply.vqmmc);
 
 	if (host->align_buffer)
-- 
 Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V1 2/3] Documentation: sdhci-msm: Add entries for passing load values

[Vijay Viswanath]

The load a particular sdhc controller should request from a regulator
is device specific and hence each device should individually vote for
the required load.

Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
---
 Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
index 502b3b8..ee34f28 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -26,6 +26,11 @@ Required properties:
 	"cal"	- reference clock for RCLK delay calibration (optional)
 	"sleep"	- sleep clock for RCLK delay calibration (optional)
 
+Optional properties:
+- <supply>-current-level - specifies load levels for supply during BUS_ON
+			and BUS_OFF states in power irq. Should be specified in
+			pairs (lpm, hpm), for BUS_OFF and BUS_ON respectively.
+			Units uA.
 Example:
 
 	sdhc_1: sdhci@f9824900 {
@@ -37,6 +42,7 @@ Example:
 
 		vmmc-supply = <&pm8941_l20>;
 		vqmmc-supply = <&pm8941_s3>;
+		vqmmc-current-level = <200 22000>;
 
 		pinctrl-names = "default";
 		pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data>;
-- 
 Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Vijay Viswanath]

Some sdhci-msm controllers require that 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 quirk for internal voltage switching and then control the
voltage switching using power irq.

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

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index a0dc3e1..47732a2 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -43,7 +43,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)
@@ -236,6 +238,14 @@ struct sdhci_msm_variant_info {
 	const struct sdhci_msm_offset *offset;
 };
 
+/* Holds voltage regulator information to be used by the driver*/
+struct sdhci_msm_vreg_info {
+	bool vmmc_load;
+	u32 vmmc_level[2];
+	bool vqmmc_load;
+	u32 vqmmc_level[2];
+};
+
 struct sdhci_msm_host {
 	struct platform_device *pdev;
 	void __iomem *core_mem;	/* MSM SDCC mapped address */
@@ -258,6 +268,8 @@ struct sdhci_msm_host {
 	bool mci_removed;
 	const struct sdhci_msm_variant_ops *var_ops;
 	const struct sdhci_msm_offset *offset;
+	struct sdhci_msm_vreg_info vreg_info;
+	bool pltfm_init_done;
 };
 
 static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
@@ -1314,11 +1326,13 @@ 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;
+	struct sdhci_msm_vreg_info vreg_info = msm_host->vreg_info;
 
 	irq_status = msm_host_readl(msm_host, host,
 			msm_offset->core_pwrctl_status);
@@ -1351,14 +1365,91 @@ 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;
+		if (!IS_ERR(mmc->supply.vmmc)) {
+			if (vreg_info.vmmc_load)
+				ret = regulator_set_load(mmc->supply.vmmc,
+						vreg_info.vmmc_level[1]);
+			ret |= mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
+					mmc->ios.vdd);
+			if (ret)
+				pr_err("%s: vmmc enable failed: %d\n",
+						mmc_hostname(mmc), ret);
+		}
+
+		if (!IS_ERR(mmc->supply.vqmmc) && !ret) {
+			struct mmc_ios ios;
+
+			if (vreg_info.vqmmc_load)
+				ret = regulator_set_load(mmc->supply.vqmmc,
+						vreg_info.vqmmc_level[1]);
+			/*
+			 * The IO voltage regulator maynot always support
+			 * a voltage close to vdd. Set IO voltage based on
+			 * capability of the regulator.
+			 */
+			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_err("%s: %s: setting signal voltage: %d\n",
+						mmc_hostname(mmc), __func__,
+						ios.signal_voltage);
+				ret |= mmc_regulator_set_vqmmc(mmc, &ios);
+			}
+
+			if (!ret)
+				ret = regulator_enable(mmc->supply.vqmmc);
+			if (ret)
+				pr_err("%s: vqmmc enable failed: %d\n",
+						mmc_hostname(mmc), ret);
+		}
+		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;
+		}
 	}
 	if (irq_status & CORE_PWRCTL_BUS_OFF) {
-		pwr_state = REQ_BUS_OFF;
-		io_level = REQ_IO_LOW;
-		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
+		/*
+		 * During platform initialization, we may get BUS_OFF request
+		 * and turn off the supply. EMMC devices expect Power off
+		 * notification before being turned off. So wait till platform
+		 * initialization is over to actually turn off power.
+		 */
+		if (!IS_ERR(mmc->supply.vmmc) && msm_host->pltfm_init_done) {
+			if (vreg_info.vmmc_load)
+				ret = regulator_set_load(mmc->supply.vmmc,
+						vreg_info.vmmc_level[0]);
+			ret |= mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
+					mmc->ios.vdd);
+			if (ret)
+				pr_err("%s: vmmc disabling failed: %d\n",
+						mmc_hostname(mmc), ret);
+		}
+		if (!IS_ERR(mmc->supply.vqmmc) && msm_host->pltfm_init_done &&
+				!ret) {
+			if (vreg_info.vqmmc_load)
+				ret = regulator_set_load(mmc->supply.vqmmc,
+						vreg_info.vqmmc_level[0]);
+			ret |= regulator_disable(mmc->supply.vqmmc);
+			if (ret)
+				pr_err("%s: vqmmc disabling failed: %d\n",
+						mmc_hostname(mmc), ret);
+		}
+		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) {
@@ -1370,6 +1461,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)
+			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
@@ -1415,10 +1515,9 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 		msm_host->curr_pwr_state = pwr_state;
 	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",
-		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
-		irq_ack);
+			mmc_hostname(msm_host->mmc), __func__,
+			irq, irq_status, irq_ack);
 }
 
 static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
@@ -1563,6 +1662,13 @@ static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg)
 		sdhci_msm_check_power_status(host, req_type);
 }
 
+static void sdhci_msm_set_power(struct sdhci_host *host, unsigned char mode,
+		     unsigned short vdd)
+{
+	sdhci_set_power_noreg(host, mode, vdd);
+
+}
+
 static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
 {
 	struct mmc_host *mmc = msm_host->mmc;
@@ -1605,6 +1711,91 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
 	pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
 }
 
+static int sdhci_msm_register_vreg(struct sdhci_msm_host *msm_host)
+{
+	int ret = 0;
+
+	ret = mmc_regulator_get_supply(msm_host->mmc);
+	if (ret)
+		return ret;
+	if (device_property_read_u32_array(&msm_host->pdev->dev,
+			"vmmc-current-level",
+			msm_host->vreg_info.vmmc_level, 2) >= 0)
+		msm_host->vreg_info.vmmc_load = true;
+	if (device_property_read_u32_array(&msm_host->pdev->dev,
+			"vqmmc-current-level",
+			msm_host->vreg_info.vqmmc_level, 2) >= 0)
+		msm_host->vreg_info.vqmmc_load = true;
+
+	sdhci_msm_set_regulator_caps(msm_host);
+
+	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,
@@ -1644,6 +1835,7 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
 	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
 	.write_w = sdhci_msm_writew,
 	.write_b = sdhci_msm_writeb,
+	.set_power = sdhci_msm_set_power,
 };
 
 static const struct sdhci_pltfm_data sdhci_msm_pdata = {
@@ -1672,6 +1864,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	if (IS_ERR(host))
 		return PTR_ERR(host);
 
+	host->mmc_host_ops.start_signal_voltage_switch =
+		sdhci_msm_start_signal_voltage_switch;
 	host->sdma_boundary = 0;
 	pltfm_host = sdhci_priv(host);
 	msm_host = sdhci_pltfm_priv(pltfm_host);
@@ -1819,6 +2013,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 				msm_offset->core_vendor_spec_capabilities0);
 	}
 
+	ret = sdhci_msm_register_vreg(msm_host);
+	if (ret == -EPROBE_DEFER)
+		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
@@ -1867,7 +2065,7 @@ 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);
+	msm_host->pltfm_init_done = true;
 
 	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
-- 
 Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Adrian Hunter]

On 20/07/18 13:46, Vijay Viswanath wrote:
> Some controllers can have internal mechanism to inform the SW that it
> is ready for voltage switching. For such controllers, changing voltage
> before the HW is ready can result in various issues.
> 
> During setup/cleanup of host, check whether regulator enable/disable
> was already done by platform driver.
> 
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1c828e0..494a1e2 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3472,6 +3472,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)
> @@ -3485,7 +3486,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 (!mmc->supply.vmmc) {
> +		ret = mmc_regulator_get_supply(mmc);
> +		enable_vqmmc  = true;
> +	} else {
> +		ret = 0;
> +	}
>  	if (ret)
>  		return ret;

Why not

	if (!mmc->supply.vmmc) {
		ret = mmc_regulator_get_supply(mmc);
	  	if (ret)
  			return ret;
		enable_vqmmc  = true;
	}

>  
> @@ -3736,7 +3742,10 @@ int sdhci_setup_host(struct sdhci_host *host)
>  
>  	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
>  	if (!IS_ERR(mmc->supply.vqmmc)) {
> -		ret = regulator_enable(mmc->supply.vqmmc);
> +		if (enable_vqmmc)
> +			ret = regulator_enable(mmc->supply.vqmmc);

Please introduce host->vqmmc_enabled = !ret;

> +		else
> +			ret = 0;
>  		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
>  						    1950000))
>  			host->caps1 &= ~(SDHCI_SUPPORT_SDR104 |
> @@ -3984,7 +3993,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>  	return 0;
>  
>  unreg:
> -	if (!IS_ERR(mmc->supply.vqmmc))
> +	if (!IS_ERR(mmc->supply.vqmmc) && enable_vqmmc)

And just make this

	if (host->vqmmc_enabled)

>  		regulator_disable(mmc->supply.vqmmc);
>  undma:
>  	if (host->align_buffer)
> @@ -4002,7 +4011,8 @@ void sdhci_cleanup_host(struct sdhci_host *host)
>  {
>  	struct mmc_host *mmc = host->mmc;
>  
> -	if (!IS_ERR(mmc->supply.vqmmc))
> +	if (!IS_ERR(mmc->supply.vqmmc) &&
> +			(regulator_is_enabled(mmc->supply.vqmmc) > 0))

	if (host->vqmmc_enabled)

>  		regulator_disable(mmc->supply.vqmmc);
>  
>  	if (host->align_buffer)
> @@ -4135,7 +4145,8 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>  
>  	tasklet_kill(&host->finish_tasklet);
>  
> -	if (!IS_ERR(mmc->supply.vqmmc))
> +	if (!IS_ERR(mmc->supply.vqmmc) &&
> +			(regulator_is_enabled(mmc->supply.vqmmc) > 0))

	if (host->vqmmc_enabled)

>  		regulator_disable(mmc->supply.vqmmc);
>  
>  	if (host->align_buffer)
> 

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

[Vijay Viswanath]

Hi Adrian,


On 7/25/2018 5:23 PM, Adrian Hunter wrote:
> On 20/07/18 13:46, Vijay Viswanath wrote:
>> Some controllers can have internal mechanism to inform the SW that it
>> is ready for voltage switching. For such controllers, changing voltage
>> before the HW is ready can result in various issues.
>>
>> During setup/cleanup of host, check whether regulator enable/disable
>> was already done by platform driver.
>>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   drivers/mmc/host/sdhci.c | 21 ++++++++++++++++-----
>>   1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 1c828e0..494a1e2 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -3472,6 +3472,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)
>> @@ -3485,7 +3486,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 (!mmc->supply.vmmc) {
>> +		ret = mmc_regulator_get_supply(mmc);
>> +		enable_vqmmc  = true;
>> +	} else {
>> +		ret = 0;
>> +	}
>>   	if (ret)
>>   		return ret;
> 
> Why not
> 
> 	if (!mmc->supply.vmmc) {
> 		ret = mmc_regulator_get_supply(mmc);
> 	  	if (ret)
>    			return ret;
> 		enable_vqmmc  = true;
> 	}
> 

looks neater. Will do.

>>   
>> @@ -3736,7 +3742,10 @@ int sdhci_setup_host(struct sdhci_host *host)
>>   
>>   	/* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
>>   	if (!IS_ERR(mmc->supply.vqmmc)) {
>> -		ret = regulator_enable(mmc->supply.vqmmc);
>> +		if (enable_vqmmc)
>> +			ret = regulator_enable(mmc->supply.vqmmc);
> 
> Please introduce host->vqmmc_enabled = !ret;
> 

Any reason to introduce vqmmc_enabled variable in sdhci_host structure ? 
We can get around with a local variable and using regulator_is_enabled API.

>> +		else
>> +			ret = 0;
>>   		if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
>>   						    1950000))
>>   			host->caps1 &= ~(SDHCI_SUPPORT_SDR104 |
>> @@ -3984,7 +3993,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>>   	return 0;
>>   
>>   unreg:
>> -	if (!IS_ERR(mmc->supply.vqmmc))
>> +	if (!IS_ERR(mmc->supply.vqmmc) && enable_vqmmc)
> 
> And just make this
> 
> 	if (host->vqmmc_enabled)
> 
>>   		regulator_disable(mmc->supply.vqmmc);
>>   undma:
>>   	if (host->align_buffer)
>> @@ -4002,7 +4011,8 @@ void sdhci_cleanup_host(struct sdhci_host *host)
>>   {
>>   	struct mmc_host *mmc = host->mmc;
>>   
>> -	if (!IS_ERR(mmc->supply.vqmmc))
>> +	if (!IS_ERR(mmc->supply.vqmmc) &&
>> +			(regulator_is_enabled(mmc->supply.vqmmc) > 0))
> 
> 	if (host->vqmmc_enabled)
> 
>>   		regulator_disable(mmc->supply.vqmmc);
>>   
>>   	if (host->align_buffer)
>> @@ -4135,7 +4145,8 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>>   
>>   	tasklet_kill(&host->finish_tasklet);
>>   
>> -	if (!IS_ERR(mmc->supply.vqmmc))
>> +	if (!IS_ERR(mmc->supply.vqmmc) &&
>> +			(regulator_is_enabled(mmc->supply.vqmmc) > 0))
> 
> 	if (host->vqmmc_enabled)
> 
>>   		regulator_disable(mmc->supply.vqmmc);
>>   
>>   	if (host->align_buffer)
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Thanks,
Vijay

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

[Adrian Hunter]

On 25/07/18 15:23, Vijay Viswanath wrote:
> Hi Adrian,
> 
> 
> On 7/25/2018 5:23 PM, Adrian Hunter wrote:
>> On 20/07/18 13:46, Vijay Viswanath wrote:
>>> Some controllers can have internal mechanism to inform the SW that it
>>> is ready for voltage switching. For such controllers, changing voltage
>>> before the HW is ready can result in various issues.
>>>
>>> During setup/cleanup of host, check whether regulator enable/disable
>>> was already done by platform driver.
>>>
>>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>>> ---
>>>   drivers/mmc/host/sdhci.c | 21 ++++++++++++++++-----
>>>   1 file changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 1c828e0..494a1e2 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -3472,6 +3472,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)
>>> @@ -3485,7 +3486,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 (!mmc->supply.vmmc) {
>>> +        ret = mmc_regulator_get_supply(mmc);
>>> +        enable_vqmmc  = true;
>>> +    } else {
>>> +        ret = 0;
>>> +    }
>>>       if (ret)
>>>           return ret;
>>
>> Why not
>>
>>     if (!mmc->supply.vmmc) {
>>         ret = mmc_regulator_get_supply(mmc);
>>           if (ret)
>>                return ret;
>>         enable_vqmmc  = true;
>>     }
>>
> 
> looks neater. Will do.
> 
>>>   @@ -3736,7 +3742,10 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>         /* If vqmmc regulator and no 1.8V signalling, then there's no UHS */
>>>       if (!IS_ERR(mmc->supply.vqmmc)) {
>>> -        ret = regulator_enable(mmc->supply.vqmmc);
>>> +        if (enable_vqmmc)
>>> +            ret = regulator_enable(mmc->supply.vqmmc);
>>
>> Please introduce host->vqmmc_enabled = !ret;
>>
> 
> Any reason to introduce vqmmc_enabled variable in sdhci_host structure ? We
> can get around with a local variable and using regulator_is_enabled API.

regulator_enable() uses reference counting, so we have to use
regulator_disable() exactly the same number of times that we use
regulator_enable().

> 
>>> +        else
>>> +            ret = 0;
>>>           if (!regulator_is_supported_voltage(mmc->supply.vqmmc, 1700000,
>>>                               1950000))
>>>               host->caps1 &= ~(SDHCI_SUPPORT_SDR104 |
>>> @@ -3984,7 +3993,7 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>       return 0;
>>>     unreg:
>>> -    if (!IS_ERR(mmc->supply.vqmmc))
>>> +    if (!IS_ERR(mmc->supply.vqmmc) && enable_vqmmc)
>>
>> And just make this
>>
>>     if (host->vqmmc_enabled)
>>
>>>           regulator_disable(mmc->supply.vqmmc);
>>>   undma:
>>>       if (host->align_buffer)
>>> @@ -4002,7 +4011,8 @@ void sdhci_cleanup_host(struct sdhci_host *host)
>>>   {
>>>       struct mmc_host *mmc = host->mmc;
>>>   -    if (!IS_ERR(mmc->supply.vqmmc))
>>> +    if (!IS_ERR(mmc->supply.vqmmc) &&
>>> +            (regulator_is_enabled(mmc->supply.vqmmc) > 0))
>>
>>     if (host->vqmmc_enabled)
>>
>>>           regulator_disable(mmc->supply.vqmmc);
>>>         if (host->align_buffer)
>>> @@ -4135,7 +4145,8 @@ void sdhci_remove_host(struct sdhci_host *host, int
>>> dead)
>>>         tasklet_kill(&host->finish_tasklet);
>>>   -    if (!IS_ERR(mmc->supply.vqmmc))
>>> +    if (!IS_ERR(mmc->supply.vqmmc) &&
>>> +            (regulator_is_enabled(mmc->supply.vqmmc) > 0))
>>
>>     if (host->vqmmc_enabled)
>>
>>>           regulator_disable(mmc->supply.vqmmc);
>>>         if (host->align_buffer)
>>>
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> Thanks,
> Vijay
> 

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

[Adrian Hunter]

On 20/07/18 13:46, Vijay Viswanath wrote:
> Some sdhci-msm controllers require that 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 quirk for internal voltage switching and then control the
> voltage switching using power irq.
> 
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 220 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 209 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index a0dc3e1..47732a2 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -43,7 +43,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)
> @@ -236,6 +238,14 @@ struct sdhci_msm_variant_info {
>  	const struct sdhci_msm_offset *offset;
>  };
>  
> +/* Holds voltage regulator information to be used by the driver*/
> +struct sdhci_msm_vreg_info {
> +	bool vmmc_load;
> +	u32 vmmc_level[2];
> +	bool vqmmc_load;
> +	u32 vqmmc_level[2];
> +};

Kernel style is not to introduce structures that are only ever wholly
contained within one other structure i.e. the members of struct
sdhci_msm_vreg_info could just be in struct sdhci_msm_host instead.

> +
>  struct sdhci_msm_host {
>  	struct platform_device *pdev;
>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
> @@ -258,6 +268,8 @@ struct sdhci_msm_host {
>  	bool mci_removed;
>  	const struct sdhci_msm_variant_ops *var_ops;
>  	const struct sdhci_msm_offset *offset;
> +	struct sdhci_msm_vreg_info vreg_info;
> +	bool pltfm_init_done;
>  };
>  
>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> @@ -1314,11 +1326,13 @@ 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;
> +	struct sdhci_msm_vreg_info vreg_info = msm_host->vreg_info;
>  
>  	irq_status = msm_host_readl(msm_host, host,
>  			msm_offset->core_pwrctl_status);
> @@ -1351,14 +1365,91 @@ 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;
> +		if (!IS_ERR(mmc->supply.vmmc)) {
> +			if (vreg_info.vmmc_load)
> +				ret = regulator_set_load(mmc->supply.vmmc,
> +						vreg_info.vmmc_level[1]);
> +			ret |= mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
> +					mmc->ios.vdd);

Or-ing error numbers seems a bit weird.  The code is a bit crammed also,
so I suggest adding helper functions.  For example consider something like:

static int sdhci_msm_set_vmmc(struct sdhci_msm_host *msm_host,
			      struct mmc_host *mmc, int level)
{
	int load = msm_host->vmmc_level[level];
	int ret;

	if (IS_ERR(mmc->supply.vmmc))
		return 0;

	if (msm_host->vmmc_load) {
		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 failed: %d\n", mmc_hostname(mmc), ret);

	return ret;
}

> +			if (ret)
> +				pr_err("%s: vmmc enable failed: %d\n",
> +						mmc_hostname(mmc), ret);
> +		}
> +
> +		if (!IS_ERR(mmc->supply.vqmmc) && !ret) {
> +			struct mmc_ios ios;
> +
> +			if (vreg_info.vqmmc_load)
> +				ret = regulator_set_load(mmc->supply.vqmmc,
> +						vreg_info.vqmmc_level[1]);
> +			/*
> +			 * The IO voltage regulator maynot always support
> +			 * a voltage close to vdd. Set IO voltage based on
> +			 * capability of the regulator.
> +			 */
> +			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_err("%s: %s: setting signal voltage: %d\n",
> +						mmc_hostname(mmc), __func__,
> +						ios.signal_voltage);

Why pr_err()?

> +				ret |= mmc_regulator_set_vqmmc(mmc, &ios);

Again I suggest createing a helper function and not or-ing error numbers.

> +			}
> +
> +			if (!ret)
> +				ret = regulator_enable(mmc->supply.vqmmc);
> +			if (ret)
> +				pr_err("%s: vqmmc enable failed: %d\n",
> +						mmc_hostname(mmc), ret);
> +		}
> +		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;
> +		}
>  	}
>  	if (irq_status & CORE_PWRCTL_BUS_OFF) {
> -		pwr_state = REQ_BUS_OFF;
> -		io_level = REQ_IO_LOW;
> -		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> +		/*
> +		 * During platform initialization, we may get BUS_OFF request
> +		 * and turn off the supply. EMMC devices expect Power off
> +		 * notification before being turned off. So wait till platform
> +		 * initialization is over to actually turn off power.
> +		 */
> +		if (!IS_ERR(mmc->supply.vmmc) && msm_host->pltfm_init_done) {
> +			if (vreg_info.vmmc_load)
> +				ret = regulator_set_load(mmc->supply.vmmc,
> +						vreg_info.vmmc_level[0]);
> +			ret |= mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
> +					mmc->ios.vdd);
> +			if (ret)
> +				pr_err("%s: vmmc disabling failed: %d\n",
> +						mmc_hostname(mmc), ret);
> +		}

So with the function above, that would be:

		if (msm_host->pltfm_init_done)
			ret = sdhci_msm_set_vmmc(msm_host, mmc, 0)

> +		if (!IS_ERR(mmc->supply.vqmmc) && msm_host->pltfm_init_done &&
> +				!ret) {
> +			if (vreg_info.vqmmc_load)
> +				ret = regulator_set_load(mmc->supply.vqmmc,
> +						vreg_info.vqmmc_level[0]);
> +			ret |= regulator_disable(mmc->supply.vqmmc);
> +			if (ret)
> +				pr_err("%s: vqmmc disabling failed: %d\n",
> +						mmc_hostname(mmc), ret);
> +		}
> +		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) {
> @@ -1370,6 +1461,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)
> +			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
> @@ -1415,10 +1515,9 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  		msm_host->curr_pwr_state = pwr_state;
>  	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",
> -		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
> -		irq_ack);
> +			mmc_hostname(msm_host->mmc), __func__,
> +			irq, irq_status, irq_ack);

Seem to be some unrelated whitespace changes here.

>  }
>  
>  static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
> @@ -1563,6 +1662,13 @@ static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg)
>  		sdhci_msm_check_power_status(host, req_type);
>  }
>  
> +static void sdhci_msm_set_power(struct sdhci_host *host, unsigned char mode,
> +		     unsigned short vdd)
> +{
> +	sdhci_set_power_noreg(host, mode, vdd);

Why not just have .set_power = sdhci_set_power_noreg()

> +
> +}
> +
>  static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>  {
>  	struct mmc_host *mmc = msm_host->mmc;
> @@ -1605,6 +1711,91 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>  	pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
>  }
>  
> +static int sdhci_msm_register_vreg(struct sdhci_msm_host *msm_host)
> +{
> +	int ret = 0;
> +
> +	ret = mmc_regulator_get_supply(msm_host->mmc);
> +	if (ret)
> +		return ret;
> +	if (device_property_read_u32_array(&msm_host->pdev->dev,
> +			"vmmc-current-level",
> +			msm_host->vreg_info.vmmc_level, 2) >= 0)
> +		msm_host->vreg_info.vmmc_load = true;
> +	if (device_property_read_u32_array(&msm_host->pdev->dev,
> +			"vqmmc-current-level",
> +			msm_host->vreg_info.vqmmc_level, 2) >= 0)
> +		msm_host->vreg_info.vqmmc_load = true;
> +
> +	sdhci_msm_set_regulator_caps(msm_host);
> +
> +	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,
> @@ -1644,6 +1835,7 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>  	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
>  	.write_w = sdhci_msm_writew,
>  	.write_b = sdhci_msm_writeb,
> +	.set_power = sdhci_msm_set_power,
>  };
>  
>  static const struct sdhci_pltfm_data sdhci_msm_pdata = {
> @@ -1672,6 +1864,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  	if (IS_ERR(host))
>  		return PTR_ERR(host);
>  
> +	host->mmc_host_ops.start_signal_voltage_switch =
> +		sdhci_msm_start_signal_voltage_switch;

Perhaps put this where the other mmc_host_ops assignment is.

>  	host->sdma_boundary = 0;
>  	pltfm_host = sdhci_priv(host);
>  	msm_host = sdhci_pltfm_priv(pltfm_host);
> @@ -1819,6 +2013,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  				msm_offset->core_vendor_spec_capabilities0);
>  	}
>  
> +	ret = sdhci_msm_register_vreg(msm_host);
> +	if (ret == -EPROBE_DEFER)

Why not just:

	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
> @@ -1867,7 +2065,7 @@ 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);
> +	msm_host->pltfm_init_done = true;
>  
>  	pm_runtime_mark_last_busy(&pdev->dev);
>  	pm_runtime_put_autosuspend(&pdev->dev);
> 

Re: [PATCH V1 2/3] Documentation: sdhci-msm: Add entries for passing load values

[Rob Herring]

On Fri, Jul 20, 2018 at 04:16:05PM +0530, Vijay Viswanath wrote:
> The load a particular sdhc controller should request from a regulator
> is device specific and hence each device should individually vote for
> the required load.

"dt-bindings: mmc: ..." for the subject.

> 
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> index 502b3b8..ee34f28 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> @@ -26,6 +26,11 @@ Required properties:
>  	"cal"	- reference clock for RCLK delay calibration (optional)
>  	"sleep"	- sleep clock for RCLK delay calibration (optional)
>  
> +Optional properties:
> +- <supply>-current-level - specifies load levels for supply during BUS_ON
> +			and BUS_OFF states in power irq. Should be specified in
> +			pairs (lpm, hpm), for BUS_OFF and BUS_ON respectively.
> +			Units uA.

This should probably be a common regulator property (or maybe one exists 
already).

It is missing vendor prefix and unit suffix as-is.

Rob

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

[Veerabhadrarao Badiganti]

Hi Adrian,


On 7/25/2018 7:34 PM, Adrian Hunter wrote:
> On 20/07/18 13:46, Vijay Viswanath wrote:
>> Some sdhci-msm controllers require that 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 quirk for internal voltage switching and then control the
>> voltage switching using power irq.
>>
>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 220 ++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 209 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index a0dc3e1..47732a2 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -43,7 +43,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)
>> @@ -236,6 +238,14 @@ struct sdhci_msm_variant_info {
>>   	const struct sdhci_msm_offset *offset;
>>   };
>>   
>> +/* Holds voltage regulator information to be used by the driver*/
>> +struct sdhci_msm_vreg_info {
>> +	bool vmmc_load;
>> +	u32 vmmc_level[2];
>> +	bool vqmmc_load;
>> +	u32 vqmmc_level[2];
>> +};
> Kernel style is not to introduce structures that are only ever wholly
> contained within one other structure i.e. the members of struct
> sdhci_msm_vreg_info could just be in struct sdhci_msm_host instead.
Sure. Will move this to sdhci_msm_host
>> +
>>   struct sdhci_msm_host {
>>   	struct platform_device *pdev;
>>   	void __iomem *core_mem;	/* MSM SDCC mapped address */
>> @@ -258,6 +268,8 @@ struct sdhci_msm_host {
>>   	bool mci_removed;
>>   	const struct sdhci_msm_variant_ops *var_ops;
>>   	const struct sdhci_msm_offset *offset;
>> +	struct sdhci_msm_vreg_info vreg_info;
>> +	bool pltfm_init_done;
>>   };
>>   
>>   static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>> @@ -1314,11 +1326,13 @@ 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;
>> +	struct sdhci_msm_vreg_info vreg_info = msm_host->vreg_info;
>>   
>>   	irq_status = msm_host_readl(msm_host, host,
>>   			msm_offset->core_pwrctl_status);
>> @@ -1351,14 +1365,91 @@ 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;
>> +		if (!IS_ERR(mmc->supply.vmmc)) {
>> +			if (vreg_info.vmmc_load)
>> +				ret = regulator_set_load(mmc->supply.vmmc,
>> +						vreg_info.vmmc_level[1]);
>> +			ret |= mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
>> +					mmc->ios.vdd);
> Or-ing error numbers seems a bit weird.  The code is a bit crammed also,
> so I suggest adding helper functions.  For example consider something like:
>
> static int sdhci_msm_set_vmmc(struct sdhci_msm_host *msm_host,
> 			      struct mmc_host *mmc, int level)
> {
> 	int load = msm_host->vmmc_level[level];
> 	int ret;
>
> 	if (IS_ERR(mmc->supply.vmmc))
> 		return 0;
>
> 	if (msm_host->vmmc_load) {
> 		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 failed: %d\n", mmc_hostname(mmc), ret);
>
> 	return ret;
> }
Will update it as suggested.
>> +			if (ret)
>> +				pr_err("%s: vmmc enable failed: %d\n",
>> +						mmc_hostname(mmc), ret);
>> +		}
>> +
>> +		if (!IS_ERR(mmc->supply.vqmmc) && !ret) {
>> +			struct mmc_ios ios;
>> +
>> +			if (vreg_info.vqmmc_load)
>> +				ret = regulator_set_load(mmc->supply.vqmmc,
>> +						vreg_info.vqmmc_level[1]);
>> +			/*
>> +			 * The IO voltage regulator maynot always support
>> +			 * a voltage close to vdd. Set IO voltage based on
>> +			 * capability of the regulator.
>> +			 */
>> +			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_err("%s: %s: setting signal voltage: %d\n",
>> +						mmc_hostname(mmc), __func__,
>> +						ios.signal_voltage);
> Why pr_err()?
This need not be pr_err. Will change it to pr_debug().
>> +				ret |= mmc_regulator_set_vqmmc(mmc, &ios);
> Again I suggest createing a helper function and not or-ing error numbers.
>
>> +			}
>> +
>> +			if (!ret)
>> +				ret = regulator_enable(mmc->supply.vqmmc);
>> +			if (ret)
>> +				pr_err("%s: vqmmc enable failed: %d\n",
>> +						mmc_hostname(mmc), ret);
>> +		}
>> +		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;
>> +		}
>>   	}
>>   	if (irq_status & CORE_PWRCTL_BUS_OFF) {
>> -		pwr_state = REQ_BUS_OFF;
>> -		io_level = REQ_IO_LOW;
>> -		irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
>> +		/*
>> +		 * During platform initialization, we may get BUS_OFF request
>> +		 * and turn off the supply. EMMC devices expect Power off
>> +		 * notification before being turned off. So wait till platform
>> +		 * initialization is over to actually turn off power.
>> +		 */
>> +		if (!IS_ERR(mmc->supply.vmmc) && msm_host->pltfm_init_done) {
>> +			if (vreg_info.vmmc_load)
>> +				ret = regulator_set_load(mmc->supply.vmmc,
>> +						vreg_info.vmmc_level[0]);
>> +			ret |= mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
>> +					mmc->ios.vdd);
>> +			if (ret)
>> +				pr_err("%s: vmmc disabling failed: %d\n",
>> +						mmc_hostname(mmc), ret);
>> +		}
> So with the function above, that would be:
>
> 		if (msm_host->pltfm_init_done)
> 			ret = sdhci_msm_set_vmmc(msm_host, mmc, 0)
>
>> +		if (!IS_ERR(mmc->supply.vqmmc) && msm_host->pltfm_init_done &&
>> +				!ret) {
>> +			if (vreg_info.vqmmc_load)
>> +				ret = regulator_set_load(mmc->supply.vqmmc,
>> +						vreg_info.vqmmc_level[0]);
>> +			ret |= regulator_disable(mmc->supply.vqmmc);
>> +			if (ret)
>> +				pr_err("%s: vqmmc disabling failed: %d\n",
>> +						mmc_hostname(mmc), ret);
>> +		}
>> +		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) {
>> @@ -1370,6 +1461,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)
>> +			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
>> @@ -1415,10 +1515,9 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>>   		msm_host->curr_pwr_state = pwr_state;
>>   	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",
>> -		mmc_hostname(msm_host->mmc), __func__, irq, irq_status,
>> -		irq_ack);
>> +			mmc_hostname(msm_host->mmc), __func__,
>> +			irq, irq_status, irq_ack);
> Seem to be some unrelated whitespace changes here.
>
>>   }
>>   
>>   static irqreturn_t sdhci_msm_pwr_irq(int irq, void *data)
>> @@ -1563,6 +1662,13 @@ static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg)
>>   		sdhci_msm_check_power_status(host, req_type);
>>   }
>>   
>> +static void sdhci_msm_set_power(struct sdhci_host *host, unsigned char mode,
>> +		     unsigned short vdd)
>> +{
>> +	sdhci_set_power_noreg(host, mode, vdd);
> Why not just have .set_power = sdhci_set_power_noreg()
You are right. Will remove  sdhci_msm_set_power()
>
>> +
>> +}
>> +
>>   static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>>   {
>>   	struct mmc_host *mmc = msm_host->mmc;
>> @@ -1605,6 +1711,91 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>>   	pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
>>   }
>>   
>> +static int sdhci_msm_register_vreg(struct sdhci_msm_host *msm_host)
>> +{
>> +	int ret = 0;
>> +
>> +	ret = mmc_regulator_get_supply(msm_host->mmc);
>> +	if (ret)
>> +		return ret;
>> +	if (device_property_read_u32_array(&msm_host->pdev->dev,
>> +			"vmmc-current-level",
>> +			msm_host->vreg_info.vmmc_level, 2) >= 0)
>> +		msm_host->vreg_info.vmmc_load = true;
>> +	if (device_property_read_u32_array(&msm_host->pdev->dev,
>> +			"vqmmc-current-level",
>> +			msm_host->vreg_info.vqmmc_level, 2) >= 0)
>> +		msm_host->vreg_info.vqmmc_load = true;
>> +
>> +	sdhci_msm_set_regulator_caps(msm_host);
>> +
>> +	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,
>> @@ -1644,6 +1835,7 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>>   	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
>>   	.write_w = sdhci_msm_writew,
>>   	.write_b = sdhci_msm_writeb,
>> +	.set_power = sdhci_msm_set_power,
>>   };
>>   
>>   static const struct sdhci_pltfm_data sdhci_msm_pdata = {
>> @@ -1672,6 +1864,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>   	if (IS_ERR(host))
>>   		return PTR_ERR(host);
>>   
>> +	host->mmc_host_ops.start_signal_voltage_switch =
>> +		sdhci_msm_start_signal_voltage_switch;
> Perhaps put this where the other mmc_host_ops assignment is.
sure. Will move it.
>>   	host->sdma_boundary = 0;
>>   	pltfm_host = sdhci_priv(host);
>>   	msm_host = sdhci_pltfm_priv(pltfm_host);
>> @@ -1819,6 +2013,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>   				msm_offset->core_vendor_spec_capabilities0);
>>   	}
>>   
>> +	ret = sdhci_msm_register_vreg(msm_host);
>> +	if (ret == -EPROBE_DEFER)
> Why not just:
>
> 	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
>> @@ -1867,7 +2065,7 @@ 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);
>> +	msm_host->pltfm_init_done = true;
>>   
>>   	pm_runtime_mark_last_busy(&pdev->dev);
>>   	pm_runtime_put_autosuspend(&pdev->dev);
>>
Thanks for all the comments. Will incorporate all the commented changes.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html