| Message ID | 20241016-topic-sm8x50-mmc-vqmmc-v1-1-767af5351d37@linaro.org |
|---|---|
| State | Accepted |
| Commit | 615dfe1aeaf6e536a123cd79f6fcbe47efe5ff07 |
| Headers | show |
| Series | mmc: msm_sdhci: enable vqmmc at probe if available | expand |
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>
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,
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, >
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, >> >
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,
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);
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(+) --- base-commit: d5cab0d6adc26ec1bbd45c2fed101184d04454ae change-id: 20241016-topic-sm8x50-mmc-vqmmc-b7cf8176ec51 Best regards,