[PATCH v3] mmc: sdhci-msm: Correctly set the load for the regulator

[PATCH v3] mmc: sdhci-msm: Correctly set the load for the regulator

[Yuanjie Yang]

Qualcomm regulator supports two power supply modes: HPM and LPM.
Currently, the sdhci-msm.c driver does not set the load to adjust
the current for eMMC and SD. If the regulator dont't set correct
load in LPM state, it will lead to the inability to properly
initialize eMMC and SD.

Set the correct regulator current for eMMC and SD to ensure that the
device can work normally even when the regulator is in LPM.

Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
---
Changes in v3:
- Optimize the code logic and separate code for regulator vmmc and vqmmc
- Rebase on tag: next-20241217
- Link to v2: https://lore.kernel.org/all/20241127095029.3918290-1-quic_yuanjiey@quicinc.com/

Changes in v2:
- Add enum msm_reg_type to optimize the code
- Delete redundant emmc type judgment
- Link to v1: https://lore.kernel.org/linux-arm-msm/20241122075048.2006894-1-quic_yuanjiey@quicinc.com/

---
 drivers/mmc/host/sdhci-msm.c | 64 ++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index e00208535bd1..22811964ba61 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -134,9 +134,18 @@
 /* Timeout value to avoid infinite waiting for pwr_irq */
 #define MSM_PWR_IRQ_TIMEOUT_MS 5000
 
+/* Max load for eMMC Vdd supply */
+#define MMC_VMMC_MAX_LOAD_UA	570000
+
 /* Max load for eMMC Vdd-io supply */
 #define MMC_VQMMC_MAX_LOAD_UA	325000
 
+/* Max load for SD Vdd supply */
+#define SD_VMMC_MAX_LOAD_UA	800000
+
+/* Max load for SD Vdd-io supply */
+#define SD_VQMMC_MAX_LOAD_UA	22000
+
 #define msm_host_readl(msm_host, host, offset) \
 	msm_host->var_ops->msm_readl_relaxed(host, offset)
 
@@ -1403,11 +1412,59 @@ static int sdhci_msm_set_pincfg(struct sdhci_msm_host *msm_host, bool level)
 	return ret;
 }
 
-static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
+static void msm_config_vmmc_regulator(struct mmc_host *mmc, bool hpm)
+{
+	int load;
+
+	if (!mmc->card) {
+		regulator_set_mode(mmc->supply.vmmc,
+				   hpm ? REGULATOR_MODE_NORMAL : REGULATOR_MODE_IDLE);
+		return;
+	}
+
+	if (!mmc_card_mmc(mmc->card) &&
+	    !mmc_card_sd(mmc->card))
+		return;
+
+	if (mmc_card_mmc(mmc->card))
+		load = MMC_VMMC_MAX_LOAD_UA;
+	else if (mmc_card_sd(mmc->card))
+		load =  SD_VMMC_MAX_LOAD_UA;
+	load = hpm ? load : 0;
+
+	regulator_set_load(mmc->supply.vmmc, load);
+
+	return;
+}
+
+static void msm_config_vqmmc_regulator(struct mmc_host *mmc, bool hpm)
+{
+	int load;
+
+	if (!mmc->card) {
+		regulator_set_mode(mmc->supply.vqmmc,
+				   hpm ? REGULATOR_MODE_NORMAL : REGULATOR_MODE_IDLE);
+		return;
+	}
+
+	if (!mmc_card_sd(mmc->card))
+		return;
+
+	load =  hpm ? SD_VQMMC_MAX_LOAD_UA : 0;
+
+	regulator_set_load(mmc->supply.vqmmc, load);
+
+	return;
+}
+
+static int sdhci_msm_set_vmmc(struct sdhci_msm_host *msm_host,
+			      struct mmc_host *mmc, bool hpm)
 {
 	if (IS_ERR(mmc->supply.vmmc))
 		return 0;
 
+	msm_config_vmmc_regulator(mmc, hpm);
+
 	return mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
 }
 
@@ -1420,6 +1477,8 @@ static int msm_toggle_vqmmc(struct sdhci_msm_host *msm_host,
 	if (msm_host->vqmmc_enabled == level)
 		return 0;
 
+	msm_config_vqmmc_regulator(mmc, level);
+
 	if (level) {
 		/* Set the IO voltage regulator to default voltage level */
 		if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
@@ -1642,7 +1701,8 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
 	}
 
 	if (pwr_state) {
-		ret = sdhci_msm_set_vmmc(mmc);
+		ret = sdhci_msm_set_vmmc(msm_host, mmc,
+					 pwr_state & REQ_BUS_ON);
 		if (!ret)
 			ret = sdhci_msm_set_vqmmc(msm_host, mmc,
 					pwr_state & REQ_BUS_ON);
-- 
2.34.1

Re: [PATCH v3] mmc: sdhci-msm: Correctly set the load for the regulator

[Dmitry Baryshkov]

On Thu, Dec 26, 2024 at 11:18:45AM +0800, Yuanjie Yang wrote:
> Qualcomm regulator supports two power supply modes: HPM and LPM.
> Currently, the sdhci-msm.c driver does not set the load to adjust
> the current for eMMC and SD. If the regulator dont't set correct
> load in LPM state, it will lead to the inability to properly
> initialize eMMC and SD.
> 
> Set the correct regulator current for eMMC and SD to ensure that the
> device can work normally even when the regulator is in LPM.
> 
> Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
> ---
> Changes in v3:
> - Optimize the code logic and separate code for regulator vmmc and vqmmc
> - Rebase on tag: next-20241217

10 days old branch

> - Link to v2: https://lore.kernel.org/all/20241127095029.3918290-1-quic_yuanjiey@quicinc.com/
> 
> Changes in v2:
> - Add enum msm_reg_type to optimize the code
> - Delete redundant emmc type judgment
> - Link to v1: https://lore.kernel.org/linux-arm-msm/20241122075048.2006894-1-quic_yuanjiey@quicinc.com/
> 
> ---
>  drivers/mmc/host/sdhci-msm.c | 64 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index e00208535bd1..22811964ba61 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -134,9 +134,18 @@
>  /* Timeout value to avoid infinite waiting for pwr_irq */
>  #define MSM_PWR_IRQ_TIMEOUT_MS 5000
>  
> +/* Max load for eMMC Vdd supply */
> +#define MMC_VMMC_MAX_LOAD_UA	570000
> +
>  /* Max load for eMMC Vdd-io supply */
>  #define MMC_VQMMC_MAX_LOAD_UA	325000
>  
> +/* Max load for SD Vdd supply */
> +#define SD_VMMC_MAX_LOAD_UA	800000
> +
> +/* Max load for SD Vdd-io supply */
> +#define SD_VQMMC_MAX_LOAD_UA	22000
> +
>  #define msm_host_readl(msm_host, host, offset) \
>  	msm_host->var_ops->msm_readl_relaxed(host, offset)
>  
> @@ -1403,11 +1412,59 @@ static int sdhci_msm_set_pincfg(struct sdhci_msm_host *msm_host, bool level)
>  	return ret;
>  }
>  
> -static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
> +static void msm_config_vmmc_regulator(struct mmc_host *mmc, bool hpm)
> +{
> +	int load;
> +
> +	if (!mmc->card) {
> +		regulator_set_mode(mmc->supply.vmmc,
> +				   hpm ? REGULATOR_MODE_NORMAL : REGULATOR_MODE_IDLE);

Can there be other users of the regulator or is it an exclusive one?
Because if there can be other users, you could have pulled the power
from them.

> +		return;
> +	}
> +
> +	if (!mmc_card_mmc(mmc->card) &&
> +	    !mmc_card_sd(mmc->card))
> +		return;
> +
> +	if (mmc_card_mmc(mmc->card))
> +		load = MMC_VMMC_MAX_LOAD_UA;
> +	else if (mmc_card_sd(mmc->card))
> +		load =  SD_VMMC_MAX_LOAD_UA;

if (mmc_card_mmc(mmc->card) ||
    mmc_card_sd(mmc->card))

> +	load = hpm ? load : 0;
> +
> +	regulator_set_load(mmc->supply.vmmc, load);
> +
> +	return;
> +}
> +
> +static void msm_config_vqmmc_regulator(struct mmc_host *mmc, bool hpm)
> +{
> +	int load;
> +
> +	if (!mmc->card) {
> +		regulator_set_mode(mmc->supply.vqmmc,
> +				   hpm ? REGULATOR_MODE_NORMAL : REGULATOR_MODE_IDLE);
> +		return;
> +	}
> +
> +	if (!mmc_card_sd(mmc->card))
> +		return;
> +
> +	load =  hpm ? SD_VQMMC_MAX_LOAD_UA : 0;
> +
> +	regulator_set_load(mmc->supply.vqmmc, load);
> +
> +	return;
> +}
> +
> +static int sdhci_msm_set_vmmc(struct sdhci_msm_host *msm_host,
> +			      struct mmc_host *mmc, bool hpm)
>  {
>  	if (IS_ERR(mmc->supply.vmmc))
>  		return 0;
>  
> +	msm_config_vmmc_regulator(mmc, hpm);
> +
>  	return mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
>  }
>  
> @@ -1420,6 +1477,8 @@ static int msm_toggle_vqmmc(struct sdhci_msm_host *msm_host,
>  	if (msm_host->vqmmc_enabled == level)
>  		return 0;
>  
> +	msm_config_vqmmc_regulator(mmc, level);
> +
>  	if (level) {
>  		/* Set the IO voltage regulator to default voltage level */
>  		if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
> @@ -1642,7 +1701,8 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  	}
>  
>  	if (pwr_state) {
> -		ret = sdhci_msm_set_vmmc(mmc);
> +		ret = sdhci_msm_set_vmmc(msm_host, mmc,
> +					 pwr_state & REQ_BUS_ON);
>  		if (!ret)
>  			ret = sdhci_msm_set_vqmmc(msm_host, mmc,
>  					pwr_state & REQ_BUS_ON);
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3] mmc: sdhci-msm: Correctly set the load for the regulator

[Yuanjie Yang]

On Thu, Dec 26, 2024 at 11:04:11PM +0200, Dmitry Baryshkov wrote:
> On Thu, Dec 26, 2024 at 11:18:45AM +0800, Yuanjie Yang wrote:
> > Qualcomm regulator supports two power supply modes: HPM and LPM.
> > Currently, the sdhci-msm.c driver does not set the load to adjust
> > the current for eMMC and SD. If the regulator dont't set correct
> > load in LPM state, it will lead to the inability to properly
> > initialize eMMC and SD.
> > 
> > Set the correct regulator current for eMMC and SD to ensure that the
> > device can work normally even when the regulator is in LPM.
> > 
> > Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
> > ---
> > Changes in v3:
> > - Optimize the code logic and separate code for regulator vmmc and vqmmc
> > - Rebase on tag: next-20241217
> 
> 10 days old branch
> 
OK I will rebase on the latest tag in next patch.

> > - Link to v2: https://lore.kernel.org/all/20241127095029.3918290-1-quic_yuanjiey@quicinc.com/
> > 
> > Changes in v2:
> > - Add enum msm_reg_type to optimize the code
> > - Delete redundant emmc type judgment
> > - Link to v1: https://lore.kernel.org/linux-arm-msm/20241122075048.2006894-1-quic_yuanjiey@quicinc.com/
> > 
> > ---
> >  drivers/mmc/host/sdhci-msm.c | 64 ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 62 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> > index e00208535bd1..22811964ba61 100644
> > --- a/drivers/mmc/host/sdhci-msm.c
> > +++ b/drivers/mmc/host/sdhci-msm.c
> > @@ -134,9 +134,18 @@
> >  /* Timeout value to avoid infinite waiting for pwr_irq */
> >  #define MSM_PWR_IRQ_TIMEOUT_MS 5000
> >  
> > +/* Max load for eMMC Vdd supply */
> > +#define MMC_VMMC_MAX_LOAD_UA	570000
> > +
> >  /* Max load for eMMC Vdd-io supply */
> >  #define MMC_VQMMC_MAX_LOAD_UA	325000
> >  
> > +/* Max load for SD Vdd supply */
> > +#define SD_VMMC_MAX_LOAD_UA	800000
> > +
> > +/* Max load for SD Vdd-io supply */
> > +#define SD_VQMMC_MAX_LOAD_UA	22000
> > +
> >  #define msm_host_readl(msm_host, host, offset) \
> >  	msm_host->var_ops->msm_readl_relaxed(host, offset)
> >  
> > @@ -1403,11 +1412,59 @@ static int sdhci_msm_set_pincfg(struct sdhci_msm_host *msm_host, bool level)
> >  	return ret;
> >  }
> >  
> > -static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
> > +static void msm_config_vmmc_regulator(struct mmc_host *mmc, bool hpm)
> > +{
> > +	int load;
> > +
> > +	if (!mmc->card) {
> > +		regulator_set_mode(mmc->supply.vmmc,
> > +				   hpm ? REGULATOR_MODE_NORMAL : REGULATOR_MODE_IDLE);
> 
> Can there be other users of the regulator or is it an exclusive one?
> Because if there can be other users, you could have pulled the power
> from them.

1.Yes there may be others users of the regulator.

I searched all the DTS files using the “qcom,sdhci-msm-v5”
driver and found that the following four DTS files have
instances where the SDHCI device and other devices share a
regulator.

qcm2290.dtsi
qrb2210-rb1.dts SDHC2(sd) and usb share pm4125_l21
		pm4125_l21: l21 {
			/* SDHCI2 VMMC */
			regulator-min-microvolt = <2960000>;
			regulator-max-microvolt = <3300000>;
			regulator-allow-set-load;
			regulator-boot-on;
		};

sc7280.dtsi
qcm6490-idp.dts
	SDHC_1(eMMC) and ufs share vreg_l7b_2p952
		vreg_l7b_2p952: ldo7 {
			regulator-name = "vreg_l7b_2p952";
			regulator-min-microvolt = <2400000>;
			regulator-max-microvolt = <3544000>;
			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
		};

sc7280.dtsi
sc7280-idp.dtsi
	SDHC_1(eMMC) and ufs share vreg_l7b_2p9
		vreg_l7b_2p9: ldo7 {
			regulator-min-microvolt = <2960000>;
			regulator-max-microvolt = <2960000>;
		};

qcs615.dtsi
qcs615-ride.dts
	SDHC_1(eMMC) and ufs
		vreg_l17a: ldo17 {
			regulator-name = "vreg_l17a";
			regulator-min-microvolt = <2950000>;
			regulator-max-microvolt = <3312000>;
			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
		};

We can see When SDHC device and other device share a regulator,
the regulator doesn't have regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM>
property.
So even if the code here uses the regulator_set_mode(REGULATOR_MODE_IDLE),
it will have no effect to other device.

2. code (!mmc->card) means SDHC device first intialize.

When the SDHC device initialization does not recognize the
device type, call mmc_power_up will reach condition
(!mmc->card) && hpm == 1. Usually this situation is in SDHC
initialization, and need to power up.
This patch aim to set correct current to let SDHC can properly
initialize even regulator-init-mode is LPM, but in first time
initialization, We don't konw this device type. So we change
regulator mode to HPM.

When the SDHC device initialization does not recognize
the device type, and call mmc_power_off will reach condition
(!mmc->card) && hpm == 0. Usually this situation means SDHC
device initialize fail.
This scenario is rare, and most of the time the code cannot
reach this point.

> > +		return;
> > +	}
> > +
> > +	if (!mmc_card_mmc(mmc->card) &&
> > +	    !mmc_card_sd(mmc->card))
> > +		return;
> > +
> > +	if (mmc_card_mmc(mmc->card))
> > +		load = MMC_VMMC_MAX_LOAD_UA;
> > +	else if (mmc_card_sd(mmc->card))
> > +		load =  SD_VMMC_MAX_LOAD_UA;
> 
> if (mmc_card_mmc(mmc->card) ||
>     mmc_card_sd(mmc->card))

Here, I want to filter out other types of devices,
just select the corrsponding current only for eMMC
and SD card.

"
if (!mmc_card_mmc(mmc->card) &&
    !mmc_card_sd(mmc->card))
    return;
"     filter out

"
if (mmc_card_mmc(mmc->card))
    load = MMC_VMMC_MAX_LOAD_UA;
else if (mmc_card_sd(mmc->card))
    load =  SD_VMMC_MAX_LOAD_UA;
"      select current


> > +	load = hpm ? load : 0;
> > +
> > +	regulator_set_load(mmc->supply.vmmc, load);
> > +
> > +	return;
> > +}
> > +
> > +static void msm_config_vqmmc_regulator(struct mmc_host *mmc, bool hpm)
> > +{
> > +	int load;
> > +
> > +	if (!mmc->card) {
> > +		regulator_set_mode(mmc->supply.vqmmc,
> > +				   hpm ? REGULATOR_MODE_NORMAL : REGULATOR_MODE_IDLE);
> > +		return;
> > +	}
> > +
> > +	if (!mmc_card_sd(mmc->card))
> > +		return;
> > +
> > +	load =  hpm ? SD_VQMMC_MAX_LOAD_UA : 0;
> > +
> > +	regulator_set_load(mmc->supply.vqmmc, load);
> > +
> > +	return;
> > +}
> > +
> > +static int sdhci_msm_set_vmmc(struct sdhci_msm_host *msm_host,
> > +			      struct mmc_host *mmc, bool hpm)
> >  {
> >  	if (IS_ERR(mmc->supply.vmmc))
> >  		return 0;
> >  
> > +	msm_config_vmmc_regulator(mmc, hpm);
> > +
> >  	return mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
> >  }
> >  
> > @@ -1420,6 +1477,8 @@ static int msm_toggle_vqmmc(struct sdhci_msm_host *msm_host,
> >  	if (msm_host->vqmmc_enabled == level)
> >  		return 0;
> >  
> > +	msm_config_vqmmc_regulator(mmc, level);
> > +
> >  	if (level) {
> >  		/* Set the IO voltage regulator to default voltage level */
> >  		if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
> > @@ -1642,7 +1701,8 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
> >  	}
> >  
> >  	if (pwr_state) {
> > -		ret = sdhci_msm_set_vmmc(mmc);
> > +		ret = sdhci_msm_set_vmmc(msm_host, mmc,
> > +					 pwr_state & REQ_BUS_ON);
> >  		if (!ret)
> >  			ret = sdhci_msm_set_vqmmc(msm_host, mmc,
> >  					pwr_state & REQ_BUS_ON);
> > -- 
> > 2.34.1
> > 
> 
> -- 
> With best wishes
> Dmitry

Thanks,
Yuanjie

Re: [PATCH v3] mmc: sdhci-msm: Correctly set the load for the regulator

[Adrian Hunter]

On 26/12/24 05:18, Yuanjie Yang wrote:
> Qualcomm regulator supports two power supply modes: HPM and LPM.
> Currently, the sdhci-msm.c driver does not set the load to adjust
> the current for eMMC and SD. If the regulator dont't set correct
> load in LPM state, it will lead to the inability to properly
> initialize eMMC and SD.
> 
> Set the correct regulator current for eMMC and SD to ensure that the
> device can work normally even when the regulator is in LPM.
> 
> Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
> ---
> Changes in v3:
> - Optimize the code logic and separate code for regulator vmmc and vqmmc
> - Rebase on tag: next-20241217
> - Link to v2: https://lore.kernel.org/all/20241127095029.3918290-1-quic_yuanjiey@quicinc.com/
> 
> Changes in v2:
> - Add enum msm_reg_type to optimize the code
> - Delete redundant emmc type judgment
> - Link to v1: https://lore.kernel.org/linux-arm-msm/20241122075048.2006894-1-quic_yuanjiey@quicinc.com/
> 
> ---
>  drivers/mmc/host/sdhci-msm.c | 64 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index e00208535bd1..22811964ba61 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -134,9 +134,18 @@
>  /* Timeout value to avoid infinite waiting for pwr_irq */
>  #define MSM_PWR_IRQ_TIMEOUT_MS 5000
>  
> +/* Max load for eMMC Vdd supply */
> +#define MMC_VMMC_MAX_LOAD_UA	570000
> +
>  /* Max load for eMMC Vdd-io supply */
>  #define MMC_VQMMC_MAX_LOAD_UA	325000
>  
> +/* Max load for SD Vdd supply */
> +#define SD_VMMC_MAX_LOAD_UA	800000
> +
> +/* Max load for SD Vdd-io supply */
> +#define SD_VQMMC_MAX_LOAD_UA	22000
> +
>  #define msm_host_readl(msm_host, host, offset) \
>  	msm_host->var_ops->msm_readl_relaxed(host, offset)
>  
> @@ -1403,11 +1412,59 @@ static int sdhci_msm_set_pincfg(struct sdhci_msm_host *msm_host, bool level)
>  	return ret;
>  }
>  
> -static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
> +static void msm_config_vmmc_regulator(struct mmc_host *mmc, bool hpm)
> +{
> +	int load;
> +
> +	if (!mmc->card) {
> +		regulator_set_mode(mmc->supply.vmmc,
> +				   hpm ? REGULATOR_MODE_NORMAL : REGULATOR_MODE_IDLE);

Why use regulator_set_mode() in this case but use regulator_set_load()
otherwise?

Previous patches used the maximum 'max(MMC_VMMC_MAX_LOAD_UA, SD_VMMC_MAX_LOAD_UA)'
for the '!mmc->card' bus-on case, so why not:

static void msm_config_vmmc_regulator(struct mmc_host *mmc, bool hpm)
{
	int load;

	if (!hpm)
		load = 0;
	else if (!mmc->card)
		load = max(MMC_VMMC_MAX_LOAD_UA, SD_VMMC_MAX_LOAD_UA);
	else if (mmc_card_mmc(mmc->card))
		load = MMC_VMMC_MAX_LOAD_UA;
	else if (mmc_card_sd(mmc->card))
		load =  SD_VMMC_MAX_LOAD_UA;
	else
		return;

	regulator_set_load(mmc->supply.vmmc, load);
}

> +		return;
> +	}
> +
> +	if (!mmc_card_mmc(mmc->card) &&
> +	    !mmc_card_sd(mmc->card))
> +		return;
> +
> +	if (mmc_card_mmc(mmc->card))
> +		load = MMC_VMMC_MAX_LOAD_UA;
> +	else if (mmc_card_sd(mmc->card))
> +		load =  SD_VMMC_MAX_LOAD_UA;
> +	load = hpm ? load : 0;
> +
> +	regulator_set_load(mmc->supply.vmmc, load);
> +
> +	return;

'return' at the end of a function returning 'void' is not needed.

> +}
> +
> +static void msm_config_vqmmc_regulator(struct mmc_host *mmc, bool hpm)
> +{
> +	int load;
> +
> +	if (!mmc->card) {
> +		regulator_set_mode(mmc->supply.vqmmc,
> +				   hpm ? REGULATOR_MODE_NORMAL : REGULATOR_MODE_IDLE);
> +		return;
> +	}
> +
> +	if (!mmc_card_sd(mmc->card))
> +		return;
> +
> +	load =  hpm ? SD_VQMMC_MAX_LOAD_UA : 0;
> +
> +	regulator_set_load(mmc->supply.vqmmc, load);
> +
> +	return;

'return' at the end of a function returning 'void' is not needed.

> +}
> +
> +static int sdhci_msm_set_vmmc(struct sdhci_msm_host *msm_host,
> +			      struct mmc_host *mmc, bool hpm)
>  {
>  	if (IS_ERR(mmc->supply.vmmc))
>  		return 0;
>  
> +	msm_config_vmmc_regulator(mmc, hpm);
> +
>  	return mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
>  }
>  
> @@ -1420,6 +1477,8 @@ static int msm_toggle_vqmmc(struct sdhci_msm_host *msm_host,
>  	if (msm_host->vqmmc_enabled == level)
>  		return 0;
>  
> +	msm_config_vqmmc_regulator(mmc, level);
> +
>  	if (level) {
>  		/* Set the IO voltage regulator to default voltage level */
>  		if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
> @@ -1642,7 +1701,8 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
>  	}
>  
>  	if (pwr_state) {
> -		ret = sdhci_msm_set_vmmc(mmc);
> +		ret = sdhci_msm_set_vmmc(msm_host, mmc,
> +					 pwr_state & REQ_BUS_ON);
>  		if (!ret)
>  			ret = sdhci_msm_set_vqmmc(msm_host, mmc,
>  					pwr_state & REQ_BUS_ON);

Re: [PATCH v3] mmc: sdhci-msm: Correctly set the load for the regulator

[Yuanjie Yang]

On Thu, Jan 09, 2025 at 10:00:49AM +0200, Adrian Hunter wrote:
> On 26/12/24 05:18, Yuanjie Yang wrote:
> > Qualcomm regulator supports two power supply modes: HPM and LPM.
> > Currently, the sdhci-msm.c driver does not set the load to adjust
> > the current for eMMC and SD. If the regulator dont't set correct
> > load in LPM state, it will lead to the inability to properly
> > initialize eMMC and SD.
> > 
> > Set the correct regulator current for eMMC and SD to ensure that the
> > device can work normally even when the regulator is in LPM.
> > 
> > Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
> > ---
> > Changes in v3:
> > - Optimize the code logic and separate code for regulator vmmc and vqmmc
> > - Rebase on tag: next-20241217
> > - Link to v2: https://lore.kernel.org/all/20241127095029.3918290-1-quic_yuanjiey@quicinc.com/
> > 
> > Changes in v2:
> > - Add enum msm_reg_type to optimize the code
> > - Delete redundant emmc type judgment
> > - Link to v1: https://lore.kernel.org/linux-arm-msm/20241122075048.2006894-1-quic_yuanjiey@quicinc.com/
> > 
> > ---
> >  drivers/mmc/host/sdhci-msm.c | 64 ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 62 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> > index e00208535bd1..22811964ba61 100644
> > --- a/drivers/mmc/host/sdhci-msm.c
> > +++ b/drivers/mmc/host/sdhci-msm.c
> > @@ -134,9 +134,18 @@
> >  /* Timeout value to avoid infinite waiting for pwr_irq */
> >  #define MSM_PWR_IRQ_TIMEOUT_MS 5000
> >  
> > +/* Max load for eMMC Vdd supply */
> > +#define MMC_VMMC_MAX_LOAD_UA	570000
> > +
> >  /* Max load for eMMC Vdd-io supply */
> >  #define MMC_VQMMC_MAX_LOAD_UA	325000
> >  
> > +/* Max load for SD Vdd supply */
> > +#define SD_VMMC_MAX_LOAD_UA	800000
> > +
> > +/* Max load for SD Vdd-io supply */
> > +#define SD_VQMMC_MAX_LOAD_UA	22000
> > +
> >  #define msm_host_readl(msm_host, host, offset) \
> >  	msm_host->var_ops->msm_readl_relaxed(host, offset)
> >  
> > @@ -1403,11 +1412,59 @@ static int sdhci_msm_set_pincfg(struct sdhci_msm_host *msm_host, bool level)
> >  	return ret;
> >  }
> >  
> > -static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
> > +static void msm_config_vmmc_regulator(struct mmc_host *mmc, bool hpm)
> > +{
> > +	int load;
> > +
> > +	if (!mmc->card) {
> > +		regulator_set_mode(mmc->supply.vmmc,
> > +				   hpm ? REGULATOR_MODE_NORMAL : REGULATOR_MODE_IDLE);
> 
> Why use regulator_set_mode() in this case but use regulator_set_load()
> otherwise?
> 
> Previous patches used the maximum 'max(MMC_VMMC_MAX_LOAD_UA, SD_VMMC_MAX_LOAD_UA)'
> for the '!mmc->card' bus-on case, so why not:
> 
> static void msm_config_vmmc_regulator(struct mmc_host *mmc, bool hpm)
> {
> 	int load;
> 
> 	if (!hpm)
> 		load = 0;
> 	else if (!mmc->card)
> 		load = max(MMC_VMMC_MAX_LOAD_UA, SD_VMMC_MAX_LOAD_UA);
> 	else if (mmc_card_mmc(mmc->card))
> 		load = MMC_VMMC_MAX_LOAD_UA;
> 	else if (mmc_card_sd(mmc->card))
> 		load =  SD_VMMC_MAX_LOAD_UA;
> 	else
> 		return;
> 
> 	regulator_set_load(mmc->supply.vmmc, load);
> }
Thanks, my purpose for using regulator_set_mode() in this case is just
to simplify the code logic.
And I readlize that using regulator_set_mode() here might cause side
effect.

Thanks for your code suggestion, I will use your suggestion to modify
this part of code in next version.

> > +		return;
> > +	}
> > +
> > +	if (!mmc_card_mmc(mmc->card) &&
> > +	    !mmc_card_sd(mmc->card))
> > +		return;
> > +
> > +	if (mmc_card_mmc(mmc->card))
> > +		load = MMC_VMMC_MAX_LOAD_UA;
> > +	else if (mmc_card_sd(mmc->card))
> > +		load =  SD_VMMC_MAX_LOAD_UA;
> > +	load = hpm ? load : 0;
> > +
> > +	regulator_set_load(mmc->supply.vmmc, load);
> > +
> > +	return;
> 
> 'return' at the end of a function returning 'void' is not needed.
OK, I will delete return in next version.

> > +}
> > +
> > +static void msm_config_vqmmc_regulator(struct mmc_host *mmc, bool hpm)
> > +{
> > +	int load;
> > +
> > +	if (!mmc->card) {
> > +		regulator_set_mode(mmc->supply.vqmmc,
> > +				   hpm ? REGULATOR_MODE_NORMAL : REGULATOR_MODE_IDLE);
> > +		return;
> > +	}
> > +
> > +	if (!mmc_card_sd(mmc->card))
> > +		return;
> > +
> > +	load =  hpm ? SD_VQMMC_MAX_LOAD_UA : 0;
> > +
> > +	regulator_set_load(mmc->supply.vqmmc, load);
> > +
> > +	return;
> 
> 'return' at the end of a function returning 'void' is not needed.
OK , I will delete return in next version.

> > +}
> > +
> > +static int sdhci_msm_set_vmmc(struct sdhci_msm_host *msm_host,
> > +			      struct mmc_host *mmc, bool hpm)
> >  {
> >  	if (IS_ERR(mmc->supply.vmmc))
> >  		return 0;
> >  
> > +	msm_config_vmmc_regulator(mmc, hpm);
> > +
> >  	return mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
> >  }
> >  
> > @@ -1420,6 +1477,8 @@ static int msm_toggle_vqmmc(struct sdhci_msm_host *msm_host,
> >  	if (msm_host->vqmmc_enabled == level)
> >  		return 0;
> >  
> > +	msm_config_vqmmc_regulator(mmc, level);
> > +
> >  	if (level) {
> >  		/* Set the IO voltage regulator to default voltage level */
> >  		if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
> > @@ -1642,7 +1701,8 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
> >  	}
> >  
> >  	if (pwr_state) {
> > -		ret = sdhci_msm_set_vmmc(mmc);
> > +		ret = sdhci_msm_set_vmmc(msm_host, mmc,
> > +					 pwr_state & REQ_BUS_ON);
> >  		if (!ret)
> >  			ret = sdhci_msm_set_vqmmc(msm_host, mmc,
> >  					pwr_state & REQ_BUS_ON);
> 

Thanks,
Yuanjie