* RE: [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available
2024-10-16 9:17 ` [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available Neil Armstrong
@ 2024-10-22 22:49 ` Jaehoon Chung
2024-10-24 13:11 ` Caleb Connolly
2024-11-14 18:13 ` Caleb Connolly
2 siblings, 0 replies; 6+ messages in thread
From: Jaehoon Chung @ 2024-10-22 22:49 UTC (permalink / raw)
To: 'Neil Armstrong', 'Caleb Connolly',
'Sumit Garg', 'Peng Fan', 'Tom Rini'
Cc: u-boot-qcom, u-boot
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available
2024-10-16 9:17 ` [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available Neil Armstrong
2024-10-22 22:49 ` Jaehoon Chung
@ 2024-10-24 13:11 ` Caleb Connolly
2024-10-24 14:16 ` Neil Armstrong
2024-11-14 18:13 ` Caleb Connolly
2 siblings, 1 reply; 6+ messages in thread
From: Caleb Connolly @ 2024-10-24 13:11 UTC (permalink / raw)
To: Neil Armstrong, Sumit Garg, Peng Fan, Jaehoon Chung, Tom Rini
Cc: u-boot-qcom, u-boot
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)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available
2024-10-24 13:11 ` Caleb Connolly
@ 2024-10-24 14:16 ` Neil Armstrong
2024-10-26 22:48 ` Caleb Connolly
0 siblings, 1 reply; 6+ messages in thread
From: Neil Armstrong @ 2024-10-24 14:16 UTC (permalink / raw)
To: Caleb Connolly, Sumit Garg, Peng Fan, Jaehoon Chung, Tom Rini
Cc: u-boot-qcom, u-boot
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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available
2024-10-24 14:16 ` Neil Armstrong
@ 2024-10-26 22:48 ` Caleb Connolly
0 siblings, 0 replies; 6+ messages in thread
From: Caleb Connolly @ 2024-10-26 22:48 UTC (permalink / raw)
To: neil.armstrong, Sumit Garg, Peng Fan, Jaehoon Chung, Tom Rini
Cc: u-boot-qcom, u-boot
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)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available
2024-10-16 9:17 ` [PATCH] mmc: msm_sdhci: enable vqmmc at probe if available Neil Armstrong
2024-10-22 22:49 ` Jaehoon Chung
2024-10-24 13:11 ` Caleb Connolly
@ 2024-11-14 18:13 ` Caleb Connolly
2 siblings, 0 replies; 6+ messages in thread
From: Caleb Connolly @ 2024-11-14 18:13 UTC (permalink / raw)
To: Sumit Garg, Peng Fan, Jaehoon Chung, Tom Rini, Neil Armstrong
Cc: u-boot-qcom, u-boot
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)
^ permalink raw reply [flat|nested] 6+ messages in thread