[PATCH] mmc: msm_sdhci: enable vqmmc at probe if available

[PATCH] mmc: msm_sdhci: enable vqmmc at probe if available

[Neil Armstrong]

On earlier platforms, the vqmmc regulator was enabled by the
previous bootloader, but on the newest (SM8650) it's not
and we need vqmmc to be enabled in order to have the card
to respond.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/mmc/msm_sdhci.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mmc/msm_sdhci.c b/drivers/mmc/msm_sdhci.c
index 4e5c932c071..27bb7052fca 100644
--- a/drivers/mmc/msm_sdhci.c
+++ b/drivers/mmc/msm_sdhci.c
@@ -15,6 +15,7 @@
 #include <asm/global_data.h>
 #include <asm/io.h>
 #include <linux/bitops.h>
+#include <power/regulator.h>
 
 /* Non-standard registers needed for SDHCI startup */
 #define SDCC_MCI_POWER   0x0
@@ -43,6 +44,7 @@ struct msm_sdhc {
 	struct sdhci_host host;
 	void *base;
 	struct clk_bulk clks;
+	struct udevice *vqmmc;
 };
 
 struct msm_sdhc_variant_info {
@@ -163,6 +165,16 @@ static int msm_sdc_probe(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	/* Get the vqmmc regulator and enable it if available */
+	device_get_supply_regulator(dev, "vqmmc-supply", &prv->vqmmc);
+	if (prv->vqmmc) {
+		ret = regulator_set_enable_if_allowed(prv->vqmmc, true);
+		if (ret) {
+			printf("Failed to enable the VQMMC regulator\n");
+			return ret;
+		}
+	}
+
 	var_info = (void *)dev_get_driver_data(dev);
 	if (!var_info->mci_removed) {
 		ret = msm_sdc_mci_init(prv);

---
base-commit: d5cab0d6adc26ec1bbd45c2fed101184d04454ae
change-id: 20241016-topic-sm8x50-mmc-vqmmc-b7cf8176ec51

Best regards,
-- 
Neil Armstrong <neil.armstrong@linaro.org>

RE: [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available

[Jaehoon Chung]

Hi

> -----Original Message-----
> From: Neil Armstrong <neil.armstrong@linaro.org>
> Sent: Wednesday, October 16, 2024 6:17 PM
> To: Caleb Connolly <caleb.connolly@linaro.org>; Sumit Garg <sumit.garg@linaro.org>; Peng Fan
> <peng.fan@nxp.com>; Jaehoon Chung <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>
> Cc: u-boot-qcom@groups.io; u-boot@lists.denx.de; Neil Armstrong <neil.armstrong@linaro.org>
> Subject: [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available
>
> On earlier platforms, the vqmmc regulator was enabled by the
> previous bootloader, but on the newest (SM8650) it's not
> and we need vqmmc to be enabled in order to have the card
> to respond.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
>  drivers/mmc/msm_sdhci.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/mmc/msm_sdhci.c b/drivers/mmc/msm_sdhci.c
> index 4e5c932c071..27bb7052fca 100644
> --- a/drivers/mmc/msm_sdhci.c
> +++ b/drivers/mmc/msm_sdhci.c
> @@ -15,6 +15,7 @@
>  #include <asm/global_data.h>
>  #include <asm/io.h>
>  #include <linux/bitops.h>
> +#include <power/regulator.h>
>
>  /* Non-standard registers needed for SDHCI startup */
>  #define SDCC_MCI_POWER   0x0
> @@ -43,6 +44,7 @@ struct msm_sdhc {
>  	struct sdhci_host host;
>  	void *base;
>  	struct clk_bulk clks;
> +	struct udevice *vqmmc;
>  };
>
>  struct msm_sdhc_variant_info {
> @@ -163,6 +165,16 @@ static int msm_sdc_probe(struct udevice *dev)
>  	if (ret)
>  		return ret;
>
> +	/* Get the vqmmc regulator and enable it if available */
> +	device_get_supply_regulator(dev, "vqmmc-supply", &prv->vqmmc);
> +	if (prv->vqmmc) {
> +		ret = regulator_set_enable_if_allowed(prv->vqmmc, true);
> +		if (ret) {
> +			printf("Failed to enable the VQMMC regulator\n");
> +			return ret;
> +		}
> +	}
> +
>  	var_info = (void *)dev_get_driver_data(dev);
>  	if (!var_info->mci_removed) {
>  		ret = msm_sdc_mci_init(prv);
>
> ---
> base-commit: d5cab0d6adc26ec1bbd45c2fed101184d04454ae
> change-id: 20241016-topic-sm8x50-mmc-vqmmc-b7cf8176ec51
>
> Best regards,
> --
> Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available

[Caleb Connolly]

Hi Neil,

On 16/10/2024 11:17, Neil Armstrong wrote:
> On earlier platforms, the vqmmc regulator was enabled by the
> previous bootloader, but on the newest (SM8650) it's not
> and we need vqmmc to be enabled in order to have the card
> to respond.

Isn't/shouldn't this be handled in mmc_power_init() ?

Kind regards,
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>   drivers/mmc/msm_sdhci.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/mmc/msm_sdhci.c b/drivers/mmc/msm_sdhci.c
> index 4e5c932c071..27bb7052fca 100644
> --- a/drivers/mmc/msm_sdhci.c
> +++ b/drivers/mmc/msm_sdhci.c
> @@ -15,6 +15,7 @@
>   #include <asm/global_data.h>
>   #include <asm/io.h>
>   #include <linux/bitops.h>
> +#include <power/regulator.h>
>   
>   /* Non-standard registers needed for SDHCI startup */
>   #define SDCC_MCI_POWER   0x0
> @@ -43,6 +44,7 @@ struct msm_sdhc {
>   	struct sdhci_host host;
>   	void *base;
>   	struct clk_bulk clks;
> +	struct udevice *vqmmc;
>   };
>   
>   struct msm_sdhc_variant_info {
> @@ -163,6 +165,16 @@ static int msm_sdc_probe(struct udevice *dev)
>   	if (ret)
>   		return ret;
>   
> +	/* Get the vqmmc regulator and enable it if available */
> +	device_get_supply_regulator(dev, "vqmmc-supply", &prv->vqmmc);
> +	if (prv->vqmmc) {
> +		ret = regulator_set_enable_if_allowed(prv->vqmmc, true);
> +		if (ret) {
> +			printf("Failed to enable the VQMMC regulator\n");
> +			return ret;
> +		}
> +	}
> +
>   	var_info = (void *)dev_get_driver_data(dev);
>   	if (!var_info->mci_removed) {
>   		ret = msm_sdc_mci_init(prv);
> 
> ---
> base-commit: d5cab0d6adc26ec1bbd45c2fed101184d04454ae
> change-id: 20241016-topic-sm8x50-mmc-vqmmc-b7cf8176ec51
> 
> Best regards,

-- 
// Caleb (they/them)

Re: [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available

[Neil Armstrong]

On 24/10/2024 15:11, Caleb Connolly wrote:
> Hi Neil,
> 
> On 16/10/2024 11:17, Neil Armstrong wrote:
>> On earlier platforms, the vqmmc regulator was enabled by the
>> previous bootloader, but on the newest (SM8650) it's not
>> and we need vqmmc to be enabled in order to have the card
>> to respond.
> 
> Isn't/shouldn't this be handled in mmc_power_init() ?

So, yes and no, the device_get_supply_regulator(vqmmc-supply) in mmc_power_init is
done to be used by sdhci_set_voltage() but .... but ... Qualcomm doesn't use
MMC_SIGNAL_VOLTAGE_330 but MMC_SIGNAL_VOLTAGE_300 which is implemented in Linux
but not in Uboot, so it would require a significant mmc/sdhci driver update
but since we 're not even far from supporting any card state requiring
MMC_SIGNAL_VOLTAGE_180 we don't even declare the sdhci_ops and neither
any of the config_dll/set_enhanced_strobe/execute_tuning so in our current
case we just need to enable vqmmc.

Neil
> 
> Kind regards,
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   drivers/mmc/msm_sdhci.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/mmc/msm_sdhci.c b/drivers/mmc/msm_sdhci.c
>> index 4e5c932c071..27bb7052fca 100644
>> --- a/drivers/mmc/msm_sdhci.c
>> +++ b/drivers/mmc/msm_sdhci.c
>> @@ -15,6 +15,7 @@
>>   #include <asm/global_data.h>
>>   #include <asm/io.h>
>>   #include <linux/bitops.h>
>> +#include <power/regulator.h>
>>   /* Non-standard registers needed for SDHCI startup */
>>   #define SDCC_MCI_POWER   0x0
>> @@ -43,6 +44,7 @@ struct msm_sdhc {
>>       struct sdhci_host host;
>>       void *base;
>>       struct clk_bulk clks;
>> +    struct udevice *vqmmc;
>>   };
>>   struct msm_sdhc_variant_info {
>> @@ -163,6 +165,16 @@ static int msm_sdc_probe(struct udevice *dev)
>>       if (ret)
>>           return ret;
>> +    /* Get the vqmmc regulator and enable it if available */
>> +    device_get_supply_regulator(dev, "vqmmc-supply", &prv->vqmmc);
>> +    if (prv->vqmmc) {
>> +        ret = regulator_set_enable_if_allowed(prv->vqmmc, true);
>> +        if (ret) {
>> +            printf("Failed to enable the VQMMC regulator\n");
>> +            return ret;
>> +        }
>> +    }
>> +
>>       var_info = (void *)dev_get_driver_data(dev);
>>       if (!var_info->mci_removed) {
>>           ret = msm_sdc_mci_init(prv);
>>
>> ---
>> base-commit: d5cab0d6adc26ec1bbd45c2fed101184d04454ae
>> change-id: 20241016-topic-sm8x50-mmc-vqmmc-b7cf8176ec51
>>
>> Best regards,
> 

Re: [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available

[Caleb Connolly]

On 24/10/2024 16:16, Neil Armstrong wrote:
> On 24/10/2024 15:11, Caleb Connolly wrote:
>> Hi Neil,
>>
>> On 16/10/2024 11:17, Neil Armstrong wrote:
>>> On earlier platforms, the vqmmc regulator was enabled by the
>>> previous bootloader, but on the newest (SM8650) it's not
>>> and we need vqmmc to be enabled in order to have the card
>>> to respond.
>>
>> Isn't/shouldn't this be handled in mmc_power_init() ?
> 
> So, yes and no, the device_get_supply_regulator(vqmmc-supply) in
> mmc_power_init is
> done to be used by sdhci_set_voltage() but .... but ... Qualcomm doesn't
> use
> MMC_SIGNAL_VOLTAGE_330 but MMC_SIGNAL_VOLTAGE_300 which is implemented
> in Linux
> but not in Uboot, so it would require a significant mmc/sdhci driver update
> but since we 're not even far from supporting any card state requiring
> MMC_SIGNAL_VOLTAGE_180 we don't even declare the sdhci_ops and neither
> any of the config_dll/set_enhanced_strobe/execute_tuning so in our current
> case we just need to enable vqmmc.

Ahhh, welp this presumably explains a bunch of my sdcard woes ;( I
thought the generic code was handling this (and clearly did a bad job at
checking the regulators themselves)

Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>

Kind regards,
> 
> Neil
>>
>> Kind regards,
>>>
>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>> ---
>>>   drivers/mmc/msm_sdhci.c | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/mmc/msm_sdhci.c b/drivers/mmc/msm_sdhci.c
>>> index 4e5c932c071..27bb7052fca 100644
>>> --- a/drivers/mmc/msm_sdhci.c
>>> +++ b/drivers/mmc/msm_sdhci.c
>>> @@ -15,6 +15,7 @@
>>>   #include <asm/global_data.h>
>>>   #include <asm/io.h>
>>>   #include <linux/bitops.h>
>>> +#include <power/regulator.h>
>>>   /* Non-standard registers needed for SDHCI startup */
>>>   #define SDCC_MCI_POWER   0x0
>>> @@ -43,6 +44,7 @@ struct msm_sdhc {
>>>       struct sdhci_host host;
>>>       void *base;
>>>       struct clk_bulk clks;
>>> +    struct udevice *vqmmc;
>>>   };
>>>   struct msm_sdhc_variant_info {
>>> @@ -163,6 +165,16 @@ static int msm_sdc_probe(struct udevice *dev)
>>>       if (ret)
>>>           return ret;
>>> +    /* Get the vqmmc regulator and enable it if available */
>>> +    device_get_supply_regulator(dev, "vqmmc-supply", &prv->vqmmc);
>>> +    if (prv->vqmmc) {
>>> +        ret = regulator_set_enable_if_allowed(prv->vqmmc, true);
>>> +        if (ret) {
>>> +            printf("Failed to enable the VQMMC regulator\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>>       var_info = (void *)dev_get_driver_data(dev);
>>>       if (!var_info->mci_removed) {
>>>           ret = msm_sdc_mci_init(prv);
>>>
>>> ---
>>> base-commit: d5cab0d6adc26ec1bbd45c2fed101184d04454ae
>>> change-id: 20241016-topic-sm8x50-mmc-vqmmc-b7cf8176ec51
>>>
>>> Best regards,
>>
> 

-- 
// Caleb (they/them)

Re: [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available

[Caleb Connolly]

On Wed, 16 Oct 2024 11:17:16 +0200, Neil Armstrong wrote:
> On earlier platforms, the vqmmc regulator was enabled by the
> previous bootloader, but on the newest (SM8650) it's not
> and we need vqmmc to be enabled in order to have the card
> to respond.
> 
> 

Applied, thanks!

[1/1] mmc: msm_sdhci: enable vqmmc at probe if available
      commit: 2d55b9e7319231a72659d620de03a471b92cdef0

Best regards,
-- 
// Caleb (they/them)