Re: [PATCH RFC 2/4] mmc: sdhci-msm: Add and use voltage regulator related APIs

[Vijay Viswanath]

On 5/2/2018 2:19 PM, Ulf Hansson wrote:
> On 1 May 2018 at 12:39, Vijay Viswanath <vviswana@xxxxxxxxxxxxxx> wrote:
> > From: Asutosh Das <asutoshd@xxxxxxxxxxxxxx>
> >
> > Some platforms require that the voltage switching happen only after
> > the register write occurs and controller is ready for the switch. When
> > the controller is ready, it will inform through power irq.
> >
> > Add voltage regulator APIs and use them during power irq to switch
> > voltage instead of relying on core layer voltage switching.
>
> This is way to simplified. 529 lines of new code deserves some more
> explanations.
>
> > Change-Id: Iaa98686e71a5bfe0092c68e9ffa563b060c5ac60
>
> Remove this.
>
> > Signed-off-by: Asutosh Das <asutoshd@xxxxxxxxxxxxxx>
> > Signed-off-by: Venkat Gopalakrishnan <venkatg@xxxxxxxxxxxxxx>
> > Signed-off-by: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
> > Signed-off-by: Vijay Viswanath <vviswana@xxxxxxxxxxxxxx>
> > ---
> >   .../devicetree/bindings/mmc/sdhci-msm.txt          |  27 +-
>
> Split DT changes and add DT maintainers.

Will do

> >   drivers/mmc/host/sdhci-msm.c                       | 537 +++++++++++++++++++--
> >   2 files changed, 529 insertions(+), 35 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> > index c2b7b2b..c454046 100644
> > --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> > +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> > @@ -23,6 +23,22 @@ Required properties:
> >          "xo"    - TCXO clock (optional)
> >          "cal"   - reference clock for RCLK delay calibration (optional)
> >          "sleep" - sleep clock for RCLK delay calibration (optional)
> > +- <supply-name>-supply: phandle to the regulator device tree node if voltage
> > +               regulators needs to be handled from within sdhci-msm layer.
> > +               Supported "supply-name" are "vdd" and "vdd-io".
> > +
> > +Optional Properties:
> > +       - qcom,<supply>-always-on - specifies whether supply should be kept
> > +               "on" always.
> > +       - qcom,<supply>-lpm_sup - specifies whether supply can be kept in low
> > +               power mode (lpm).
> > +       - qcom,<supply>-voltage_level - specifies voltage levels for supply.
> > +               Should be specified in pairs (min, max), units uV.
> > +       - qcom,<supply>-current_level - specifies load levels for supply in lpm
> > +               or high power mode (hpm). Should be specified in
> > +               pairs (lpm, hpm), units uA.
> > +
> > +
>
> This looks really weird.
>
> What's so special here that requires you to have your own specific
> regulator bindings?
>
> >   Example:
> >
> > @@ -33,8 +49,15 @@ Example:
> >                  bus-width = <8>;
> >                  non-removable;
> >
> > -               vmmc-supply = <&pm8941_l20>;
> > -               vqmmc-supply = <&pm8941_s3>;
> > +               vdd-supply = <&pm8941_l20>;
> > +               qcom,vdd-voltage-level = <2950000 2950000>;
> > +               qcom,vdd-current-level = <9000 800000>;
> > +
> > +               vdd-io-supply = <&pm8941_s3>;
> > +               qcom,vdd-io-always-on;
> > +               qcom,vdd-io-lpm-sup;
> > +               qcom,vdd-io-voltage-level = <1800000 2950000>;
> > +               qcom,vdd-io-current-level = <6 22000>;
> >
> >                  pinctrl-names = "default";
> >                  pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data>;
> > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> > index d4d432b..0e0f12d 100644
> > --- a/drivers/mmc/host/sdhci-msm.c
> > +++ b/drivers/mmc/host/sdhci-msm.c
> > @@ -213,6 +213,33 @@ struct sdhci_msm_offset sdhci_msm_offset_mci_present = {
> >          .core_ddr_config_2 = 0x1BC,
> >   };
> >
> > +/* This structure keeps information per regulator */
> > +struct sdhci_msm_reg_data {
> > +       /* voltage regulator handle */
> > +       struct regulator *reg;
> > +       /* regulator name */
> > +       const char *name;
> > +       /* voltage level to be set */
> > +       u32 low_vol_level;
> > +       u32 high_vol_level;
> > +       /* Load values for low power and high power mode */
> > +       u32 lpm_uA;
> > +       u32 hpm_uA;
> > +       /* is this regulator enabled? */
> > +       bool is_enabled;
> > +       /* is this regulator needs to be always on? */
> > +       bool is_always_on;
> > +       /* is low power mode setting required for this regulator? */
> > +       bool lpm_sup;
> > +       bool set_voltage_sup;
> > +};
> > +
> > +struct sdhci_msm_pltfm_data {
> > +       /* Change-Id: Ide3a658ad51a3c3d4a05c47c0e8f013f647c9516 */
> > +       struct sdhci_msm_reg_data *vdd_data;
> > +       struct sdhci_msm_reg_data *vdd_io_data;
> > +};
> > +
> >   struct sdhci_msm_host {
> >          struct platform_device *pdev;
> >          void __iomem *core_mem; /* MSM SDCC mapped address */
> > @@ -234,6 +261,8 @@ struct sdhci_msm_host {
> >          u32 caps_0;
> >          bool mci_removed;
> >          const struct sdhci_msm_offset *offset;
> > +       bool pltfm_init_done;
> > +       struct sdhci_msm_pltfm_data pdata;
> >   };
> >
> >   /*
> > @@ -298,6 +327,336 @@ void sdhci_msm_vendor_writel_relaxed(u32 val, struct sdhci_host *host,
> >          writel_relaxed(val, base_addr + offset);
> >   }
> >
> > +enum vdd_io_level {
> > +       /* set vdd_io_data->low_vol_level */
> > +       VDD_IO_LOW,
> > +       /* set vdd_io_data->high_vol_level */
> > +       VDD_IO_HIGH,
> > +       /*
> > +        * set whatever there in voltage_level (third argument) of
> > +        * sdhci_msm_set_vdd_io_vol() function.
> > +        */
> > +       VDD_IO_SET_LEVEL,
> > +};
> > +
> > +#define MAX_PROP_SIZE 32
> > +static int sdhci_msm_dt_parse_vreg_info(struct device *dev,
> > +               struct sdhci_msm_reg_data **vreg_data, const char *vreg_name)
> > +{
> > +       int len, ret = 0;
> > +       const __be32 *prop;
> > +       char prop_name[MAX_PROP_SIZE];
> > +       struct sdhci_msm_reg_data *vreg;
> > +       struct device_node *np = dev->of_node;
> > +
> > +       snprintf(prop_name, MAX_PROP_SIZE, "%s-supply", vreg_name);
> > +       if (!of_parse_phandle(np, prop_name, 0)) {
> > +               dev_warn(dev, "No internal vreg data found for %s\n",
> > +                               vreg_name);
> > +               ret = -EINVAL;
> > +               return ret;
> > +       }
> > +       vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
> > +       if (!vreg) {
> > +               ret = -ENOMEM;
> > +               return ret;
> > +       }
> > +       vreg->name = vreg_name;
> > +       snprintf(prop_name, MAX_PROP_SIZE,
> > +                       "qcom,%s-always-on", vreg_name);
> > +       if (of_get_property(np, prop_name, NULL))
> > +               vreg->is_always_on = true;
> > +
> > +       snprintf(prop_name, MAX_PROP_SIZE,
> > +                       "qcom,%s-lpm-sup", vreg_name);
> > +       if (of_get_property(np, prop_name, NULL))
> > +               vreg->lpm_sup = true;
> > +       snprintf(prop_name, MAX_PROP_SIZE,
> > +                       "qcom,%s-voltage-level", vreg_name);
> > +       prop = of_get_property(np, prop_name, &len);
> > +       if (!prop || (len != (2 * sizeof(__be32)))) {
> > +               dev_warn(dev, "%s %s property\n",
> > +                               prop ? "invalid format" : "no", prop_name);
> > +       } else {
> > +               vreg->low_vol_level = be32_to_cpup(&prop[0]);
> > +               vreg->high_vol_level = be32_to_cpup(&prop[1]);
> > +       }
> > +       snprintf(prop_name, MAX_PROP_SIZE,
> > +                       "qcom,%s-current-level", vreg_name);
> > +       prop = of_get_property(np, prop_name, &len);
> > +       if (!prop || (len != (2 * sizeof(__be32)))) {
> > +               dev_warn(dev, "%s %s property\n",
> > +                               prop ? "invalid format" : "no", prop_name);
> > +       } else {
> > +               vreg->lpm_uA = be32_to_cpup(&prop[0]);
> > +               vreg->hpm_uA = be32_to_cpup(&prop[1]);
> > +       }
> > +       *vreg_data = vreg;
> > +       dev_dbg(dev, "%s: %s %s vol=[%d %d]uV, curr=[%d %d]uA\n",
> > +                       vreg->name, vreg->is_always_on ? "always_on," : "",
> > +                       vreg->lpm_sup ? "lpm_sup," : "", vreg->low_vol_level,
> > +                       vreg->high_vol_level, vreg->lpm_uA, vreg->hpm_uA);
>
> So again, I don't get why this isn't being implemented as a regular
> regulator and why is this code in the mmc driver?
>
> > +
> > +       return ret;
> > +}
> > +
> > +/* Regulator utility functions */
> > +static int sdhci_msm_vreg_init_reg(struct device *dev,
> > +                                       struct sdhci_msm_reg_data *vreg)
> > +{
> > +       int ret = 0;
> > +
> > +       /* check if regulator is already initialized? */
> > +       if (vreg->reg)
> > +               goto out;
> > +
> > +       /* Get the regulator handle */
> > +       vreg->reg = devm_regulator_get(dev, vreg->name);
> > +       if (IS_ERR(vreg->reg)) {
> > +               ret = PTR_ERR(vreg->reg);
> > +               pr_err("%s: devm_regulator_get(%s) failed. ret=%d\n",
> > +                       __func__, vreg->name, ret);
> > +               goto out;
> > +       }
> > +
> > +       if (regulator_count_voltages(vreg->reg) > 0) {
> > +               vreg->set_voltage_sup = true;
> > +               /* sanity check */
> > +               if (!vreg->high_vol_level || !vreg->hpm_uA) {
> > +                       pr_err("%s: %s invalid constraints specified high_vol_level: %d hpm_uA: %d\n",
> > +                              __func__, vreg->name, vreg->high_vol_level,
> > +                              vreg->hpm_uA);
> > +                       ret = -EINVAL;
> > +               }
> > +       }
> > +
> > +out:
> > +       return ret;
> > +}
> > +
> > +static void sdhci_msm_vreg_deinit_reg(struct sdhci_msm_reg_data *vreg)
> > +{
> > +       if (vreg->reg)
> > +               devm_regulator_put(vreg->reg);
> > +}
> > +
> > +static int sdhci_msm_vreg_set_optimum_mode(struct sdhci_msm_reg_data
> > +                                                 *vreg, int uA_load)
> > +{
> > +       int ret = 0;
> > +
> > +       /*
> > +        * regulators that do not support regulator_set_voltage also
> > +        * do not support regulator_set_optimum_mode
> > +        */
>
> If that's the case, the regulator core should return proper error
> codes and you should need to have these kind of checks here. Right?
>
> Or at least there should be a helper function telling what you can do
> with your regulator, instead of encoding it at in the mmc driver.
>
> > +       if (vreg->set_voltage_sup) {
> > +               ret = regulator_set_load(vreg->reg, uA_load);
> > +               if (ret < 0)
> > +                       pr_err("%s: regulator_set_load(reg=%s,uA_load=%d) failed. ret=%d\n",
> > +                              __func__, vreg->name, uA_load, ret);
> > +               else
> > +                       /*
> > +                        * regulator_set_load() can return non zero
> > +                        * value even for success case.
> > +                        */
> > +                       ret = 0;
> > +       }
> > +       return ret;
> > +}
> > +
> > +static int sdhci_msm_vreg_set_voltage(struct sdhci_msm_reg_data *vreg,
> > +                                       int min_uV, int max_uV)
> > +{
> > +       int ret = 0;
> > +
> > +       if (vreg && vreg->set_voltage_sup) {
>
> Ditto.
>
> > +               ret = regulator_set_voltage(vreg->reg, min_uV, max_uV);
> > +               if (ret) {
> > +                       pr_err("%s: regulator_set_voltage(%s)failed. min_uV=%d,max_uV=%d,ret=%d\n",
> > +                              __func__, vreg->name, min_uV, max_uV, ret);
> > +               }
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +static int sdhci_msm_vreg_enable(struct sdhci_msm_reg_data *vreg)
> > +{
> > +       int ret = 0;
> > +
> > +       /* Put regulator in HPM (high power mode) */
> > +       ret = sdhci_msm_vreg_set_optimum_mode(vreg, vreg->hpm_uA);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (!vreg->is_enabled) {
> > +               /* Set voltage level */
> > +               ret = sdhci_msm_vreg_set_voltage(vreg, vreg->high_vol_level,
> > +                                               vreg->high_vol_level);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +       ret = regulator_enable(vreg->reg);
> > +       if (ret) {
> > +               pr_err("%s: regulator_enable(%s) failed. ret=%d\n",
> > +                               __func__, vreg->name, ret);
> > +               return ret;
> > +       }
> > +       vreg->is_enabled = true;
> > +       return ret;
>
> Why don't you use the mmc_regulator_*() APIs? It seems like lots of
> open coding here.
>
> > +}
> > +
> > +static int sdhci_msm_vreg_disable(struct sdhci_msm_reg_data *vreg)
> > +{
> > +       int ret = 0;
> > +
> > +       /* Never disable regulator marked as always_on */
> > +       if (vreg->is_enabled && !vreg->is_always_on) {
> > +               ret = regulator_disable(vreg->reg);
> > +               if (ret) {
> > +                       pr_err("%s: regulator_disable(%s) failed. ret=%d\n",
> > +                               __func__, vreg->name, ret);
> > +                       goto out;
> > +               }
> > +               vreg->is_enabled = false;
> > +
> > +               ret = sdhci_msm_vreg_set_optimum_mode(vreg, 0);
> > +               if (ret < 0)
> > +                       goto out;
> > +
> > +               /* Set min. voltage level to 0 */
> > +               ret = sdhci_msm_vreg_set_voltage(vreg, 0, vreg->high_vol_level);
> > +               if (ret)
> > +                       goto out;
> > +       } else if (vreg->is_enabled && vreg->is_always_on) {
> > +               if (vreg->lpm_sup) {
> > +                       /* Put always_on regulator in LPM (low power mode) */
> > +                       ret = sdhci_msm_vreg_set_optimum_mode(vreg,
> > +                                                             vreg->lpm_uA);
> > +                       if (ret < 0)
> > +                               goto out;
> > +               }
> > +       }
> > +out:
> > +       return ret;
> > +}
>
> Ditto.
>
> [...]
>
> > +
> > +/* Parse platform data */
> > +static void sdhci_msm_populate_pdata(struct device *dev,
> > +               struct sdhci_msm_pltfm_data *pdata)
> > +{
> > +       int ret = 0;
> > +
> > +       ret = sdhci_msm_dt_parse_vreg_info(dev, &pdata->vdd_data, "vdd");
> > +       if (ret)
> > +               dev_warn(dev, "failed parsing vdd data (%d)\n", ret);
> > +
> > +       ret = sdhci_msm_dt_parse_vreg_info(dev, &pdata->vdd_io_data, "vdd-io");
> > +       if (ret)
> > +               dev_warn(dev, "failed parsing vdd-io data (%d)\n", ret);
> > +
> > +}
> > +
> >   static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
> >                                                      unsigned int clock)
> >   {
> > @@ -1326,6 +1685,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
> >          u32 pwr_state = 0, io_level = 0;
> >          u32 config;
> >          const struct sdhci_msm_offset *msm_offset = msm_host->offset;
> > +       int ret = 0;
> >
> >          irq_status = sdhci_msm_vendor_readl_relaxed(host,
> >                          msm_offset->core_pwrctl_status);
> > @@ -1358,21 +1718,54 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
> >
> >          /* Handle BUS ON/OFF*/
> >          if (irq_status & CORE_PWRCTL_BUS_ON) {
> > +               ret = sdhci_msm_setup_vreg(&msm_host->pdata, true, false);
> > +               if (!ret) {
> > +                       ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata,
> > +                                       VDD_IO_HIGH, 0);
> > +                       if (ret)
> > +                               pr_err("%s: error setting vdd io in BUS_ON: %d\n",
> > +                                               mmc_hostname(host->mmc), ret);
> > +               } else {
> > +                       pr_err("%s: error setting up vreg ON : %d\n",
> > +                                       mmc_hostname(host->mmc), ret);
> > +               }
> >                  pwr_state = REQ_BUS_ON;
> >                  io_level = REQ_IO_HIGH;
> >                  irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> >          }
> >          if (irq_status & CORE_PWRCTL_BUS_OFF) {
> > +               if (msm_host->pltfm_init_done)
> > +                       ret = sdhci_msm_setup_vreg(&msm_host->pdata,
> > +                                       false, false);
> > +               if (!ret) {
> > +                       ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata,
> > +                                       VDD_IO_LOW, 0);
> > +                       if (ret)
> > +                               pr_err("%s: error setting vdd io in BUS_OFF: %d\n",
> > +                                               mmc_hostname(host->mmc), ret);
> > +               } else {
> > +                       pr_err("%s: error setting up vreg OFF: %d\n",
> > +                                       mmc_hostname(host->mmc), ret);
> > +               }
> >                  pwr_state = REQ_BUS_OFF;
> >                  io_level = REQ_IO_LOW;
> >                  irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> >          }
> >          /* Handle IO LOW/HIGH */
> >          if (irq_status & CORE_PWRCTL_IO_LOW) {
> > +               ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata, VDD_IO_LOW, 0);
> > +               if (ret)
> > +                       pr_err("%s: error setting up vdd io low: %d\n",
> > +                                       mmc_hostname(host->mmc), ret);
> >                  io_level = REQ_IO_LOW;
> >                  irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> >          }
> >          if (irq_status & CORE_PWRCTL_IO_HIGH) {
> > +               ret = sdhci_msm_set_vdd_io_vol(&msm_host->pdata,
> > +                               VDD_IO_HIGH, 0);
> > +               if (ret)
> > +                       pr_err("%s: error setting up vdd io high: %d\n",
> > +                                       mmc_hostname(host->mmc), ret);
> >                  io_level = REQ_IO_HIGH;
> >                  irq_ack |= CORE_PWRCTL_IO_SUCCESS;
> >          }
> > @@ -1380,7 +1773,7 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host *host, int irq)
> >          /*
> >           * The driver has to acknowledge the interrupt, switch voltages and
> >           * report back if it succeded or not to this register. The voltage
> > -        * switches are handled by the sdhci core, so just report success.
> > +        * switches may be handled by the sdhci core, so just report success.
> >           */
> >          sdhci_msm_vendor_writel_relaxed(irq_ack, host,
> >                          msm_offset->core_pwrctl_ctl);
> > @@ -1612,6 +2005,74 @@ 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 void sdhci_msm_set_default_hw_caps(struct device *dev,
> > +               struct sdhci_msm_host *msm_host,
> > +               struct sdhci_host *host)
> > +{
> > +       u32 version, config;
> > +       u16 minor;
> > +       u8 major;
> > +       const struct sdhci_msm_offset *msm_offset = msm_host->offset;
> > +       struct sdhci_msm_reg_data *vdd_io_reg = msm_host->pdata.vdd_io_data;
> > +       u32 caps = 0;
> > +
> > +       version =  sdhci_msm_vendor_readl_relaxed(host,
> > +                       msm_offset->core_mci_version);
> > +       major = (version & CORE_VERSION_MAJOR_MASK) >>
> > +               CORE_VERSION_MAJOR_SHIFT;
> > +       minor = version & CORE_VERSION_MINOR_MASK;
> > +       dev_dbg(dev, "MCI Version: 0x%08x, major: 0x%04x, minor: 0x%02x\n",
> > +                       version, major, minor);
> > +
> > +       /*
> > +        * If voltage regulators are controlled by msm host layer and
> > +        * IO voltage regulator can support 1.8V, we need to
> > +        * update the capabilities here before sdhci host is added.
> > +        */
> > +       if (vdd_io_reg && (vdd_io_reg->low_vol_level < 1950000))
> > +               caps |= CORE_1_8V_SUPPORT;
> > +       if (vdd_io_reg && (vdd_io_reg->low_vol_level > 2700000))
> > +               caps |= CORE_3_0V_SUPPORT;
> > +       if (caps) {
> > +               msm_host->caps_0 |= caps;
> > +               config =  readl_relaxed(host->ioaddr +
> > +                               msm_offset->core_vendor_spec);
> > +               config |= CORE_IO_PAD_PWR_SWITCH_EN;
> > +               writel_relaxed(config,
> > +                               host->ioaddr + msm_offset->core_vendor_spec);
> > +       }
> > +
> > +       caps = readl_relaxed(host->ioaddr + SDHCI_CAPABILITIES);
> > +
> > +       if (major == 1 && minor >= 0x42)
> > +               msm_host->use_14lpp_dll_reset = true;
> > +       /*
> > +        * SDCC 5 controller with major version 1, minor version 0x34 and later
> > +        * with HS 400 mode support will use CM DLL instead of CDC LP 533 DLL.
> > +        */
> > +       if (major == 1 && minor < 0x34)
> > +               msm_host->use_cdclp533 = true;
> > +
> > +       /*
> > +        * Support for some capabilities is not advertised by newer
> > +        * controller versions and must be explicitly enabled.
> > +        */
> > +       if (major >= 1 && minor != 0x11 && minor != 0x12) {
> > +
> > +               vdd_io_reg = msm_host->pdata.vdd_io_data;
> > +               caps |= SDHCI_CAN_VDD_300 | SDHCI_CAN_DO_8BIT;
> > +               if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
> > +                       caps |= CORE_1_8V_SUPPORT;
> > +       }
> > +       msm_host->caps_0 |= caps;
> > +       writel_relaxed(msm_host->caps_0, host->ioaddr +
> > +                       msm_offset->core_vendor_spec_capabilities0);
> > +
> > +       /* Set more host capabilities */
> > +       msm_host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
> > +       msm_host->mmc->caps2 |= MMC_CAP2_BOOTPART_NOACC;
>
> Is really all above only related to regulators, as according to the
> change log? Probably not.
>
> Reaching this point in the review, it certainly seems like the change
> needs to be split up in quite a few smaller pieces. Can you please do
> that!?
>
> [...]
>
> Kind regards
> Uffe

I am checking this. Will get back to you.

Thanks,
Vijay