Re: [PATCH v5 03/10] soc: qcom: spm: add support for voltage regulator

[Stephan Gerhold <stephan@gerhold.net>]

On Thu, Dec 07, 2023 at 10:33:31PM +0200, Dmitry Baryshkov wrote:
> On Thu, 7 Dec 2023 at 19:12, Stephan Gerhold <stephan@gerhold.net> wrote:
> > On Thu, Dec 07, 2023 at 04:06:56PM +0300, Dmitry Baryshkov wrote:
> > > The SPM / SAW2 device also provides a voltage regulator functionality
> > > with optional AVS (Adaptive Voltage Scaling) support. The exact register
> > > sequence and voltage ranges differs from device to device.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >
> > There is some overlap here with the spmi_saw_set_vdd() functionality in
> > qcom_spmi-regulator.c, at least for the SoCs with SPMI PMICs (e.g.
> > MSM8974 etc). You don't add support for these in this patch series yet
> > but I think it would be good to clarify how we would expect to handle
> > those. In other words:
> >
> >  - Would we handle them in qcom_spmi-regulator.c and keep the code in
> >    the spm.c driver only for the non-SPMI platforms?
> >
> >  - Should we add this in a SSBI regulator driver instead for consistency?
> >
> >  - Or should we move the existing functionality in qcom_spmi-regulator.c
> >    to here?
> 
> The spmi_saw_set_vdd() is a definite hack and ideally it should be
> dropped. It is not possible, though, existing msm8996 DT uses that
> spmi/saw regulator to power on the CPU cores. We have to remain
> compatible with that hack.
> But my intent is to have all other platforms use the spm.c (and
> migrate msm8996 at some point too).

Thanks, sounds reasonable.

> 
> >
> > > ---
> > >  drivers/soc/qcom/spm.c | 221 ++++++++++++++++++++++++++++++++++++++++-
> > >  include/soc/qcom/spm.h |   9 ++
> > >  2 files changed, 225 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
> > > index 2f0b1bfe7658..595e2afb2141 100644
> > > --- a/drivers/soc/qcom/spm.c
> > > +++ b/drivers/soc/qcom/spm.c
> > > [...]
> > > @@ -238,6 +273,181 @@ void spm_set_low_power_mode(struct spm_driver_data *drv,
> > >       spm_register_write_sync(drv, SPM_REG_SPM_CTL, ctl_val);
> > >  }
> > >
> > > +static int spm_set_voltage_sel(struct regulator_dev *rdev, unsigned int selector)
> > > +{
> > > +     struct spm_driver_data *drv = rdev_get_drvdata(rdev);
> > > +
> > > +     drv->volt_sel = selector;
> > > +
> > > +     /* Always do the SAW register writes on the corresponding CPU */
> > > +     return smp_call_function_single(drv->reg_cpu, drv->reg_data->set_vdd, drv, true);
> > > +}
> > > +
> > > +static int spm_get_voltage_sel(struct regulator_dev *rdev)
> > > +{
> > > +     struct spm_driver_data *drv = rdev_get_drvdata(rdev);
> > > +
> > > +     return drv->volt_sel;
> > > +}
> > > +
> > > +static const struct regulator_ops spm_reg_ops = {
> > > +     .set_voltage_sel        = spm_set_voltage_sel,
> > > +     .get_voltage_sel        = spm_get_voltage_sel,
> > > +     .list_voltage           = regulator_list_voltage_linear_range,
> > > +     .set_voltage_time_sel   = regulator_set_voltage_time_sel,
> > > +};
> > > +
> > > +static void smp_set_vdd_v1_1(void *data)
> > > +{
> > > +     struct spm_driver_data *drv = data;
> > > +     unsigned int vctl, data0, data1, avs_ctl, sts;
> > > +     unsigned int vlevel, volt_sel;
> > > +     bool avs_enabled;
> > > +
> > > +     volt_sel = drv->volt_sel;
> > > +     vlevel = volt_sel | 0x80; /* band */
> > > +
> > > +     avs_ctl = spm_register_read(drv, SPM_REG_AVS_CTL);
> > > +     vctl = spm_register_read(drv, SPM_REG_VCTL);
> > > +     data0 = spm_register_read(drv, SPM_REG_PMIC_DATA_0);
> > > +     data1 = spm_register_read(drv, SPM_REG_PMIC_DATA_1);
> > > +
> > > +     avs_enabled = avs_ctl & SPM_1_1_AVS_CTL_AVS_ENABLED;
> > > +
> > > +     /* If AVS is enabled, switch it off during the voltage change */
> > > +     if (avs_enabled) {
> > > +             avs_ctl &= ~SPM_1_1_AVS_CTL_AVS_ENABLED;
> > > +             spm_register_write(drv, SPM_REG_AVS_CTL, avs_ctl);
> > > +     }
> > > +
> > > +     /* Kick the state machine back to idle */
> > > +     spm_register_write(drv, SPM_REG_RST, 1);
> > > +
> > > +     vctl = FIELD_SET(vctl, SPM_VCTL_VLVL, vlevel);
> > > +     data0 = FIELD_SET(data0, SPM_PMIC_DATA_0_VLVL, vlevel);
> > > +     data1 = FIELD_SET(data1, SPM_PMIC_DATA_1_MIN_VSEL, volt_sel);
> > > +     data1 = FIELD_SET(data1, SPM_PMIC_DATA_1_MAX_VSEL, volt_sel);
> > > +
> > > +     spm_register_write(drv, SPM_REG_VCTL, vctl);
> > > +     spm_register_write(drv, SPM_REG_PMIC_DATA_0, data0);
> > > +     spm_register_write(drv, SPM_REG_PMIC_DATA_1, data1);
> > > +
> > > +     if (read_poll_timeout_atomic(spm_register_read,
> > > +                                  sts, sts == vlevel,
> > > +                                  1, 200, false,
> > > +                                  drv, SPM_REG_STS1)) {
> > > +             dev_err_ratelimited(drv->dev, "timeout setting the voltage (%x %x)!\n", sts, vlevel);
> > > +             goto enable_avs;
> > > +     }
> > > +
> > > +     if (avs_enabled) {
> > > +             unsigned int max_avs = volt_sel;
> > > +             unsigned int min_avs = max(max_avs, 4U) - 4;
> > > +
> > > +             avs_ctl = FIELD_SET(avs_ctl, SPM_AVS_CTL_MIN_VLVL, min_avs);
> > > +             avs_ctl = FIELD_SET(avs_ctl, SPM_AVS_CTL_MAX_VLVL, max_avs);
> > > +             spm_register_write(drv, SPM_REG_AVS_CTL, avs_ctl);
> > > +     }
> > > +
> > > +enable_avs:
> > > +     if (avs_enabled) {
> > > +             avs_ctl |= SPM_1_1_AVS_CTL_AVS_ENABLED;
> > > +             spm_register_write(drv, SPM_REG_AVS_CTL, avs_ctl);
> > > +     }
> > > +}
> > > +
> > > +static int spm_get_cpu(struct device *dev)
> > > +{
> > > +     int cpu;
> > > +     bool found;
> > > +
> > > +     for_each_possible_cpu(cpu) {
> > > +             struct device_node *cpu_node, *saw_node;
> > > +
> > > +             cpu_node = of_cpu_device_node_get(cpu);
> > > +             if (!cpu_node)
> > > +                     continue;
> > > +
> > > +             saw_node = of_parse_phandle(cpu_node, "qcom,saw", 0);
> > > +             found = (saw_node == dev->of_node);
> > > +             of_node_put(saw_node);
> > > +             of_node_put(cpu_node);
> > > +
> > > +             if (found)
> > > +                     return cpu;
> > > +     }
> > > +
> > > +     /* L2 SPM is not bound to any CPU, tie it to CPU0 */
> >
> > Is this necessary? I would kind of expect that it's only important that
> > this doesn't happen in parallel on multiple CPUs. The lock in the
> > regulator core should already ensure that, though. It's somewhat
> > expensive to schedule on other cores, especially if they are currently
> > idle and power collapsed.
> 
> If I understand correctly, it is the other way around. From the msm
> kernels I see that CPU SPM calls are scheduled to be executed only on
> the corresponding CPU/core. For L2 we didn't have the CPU, so to keep
> the same code path I selected for them to be executed on CPU0. At this
> point I'm not even sure if this cpu0 comes from the downstream tree or
> not.
> 

Hm, does 8064/8960 set any voltages through the L2 SPM at all? From a
quick look I see only the core SPMs being used as regulators, the L2 SPM
seems to be only used for idling.

I found a msm_spm_apcs_set_vdd() [1] but:

 - That one skips the smp_call stuff and calls msm_spm_drv_set_vdd()
   directly. In other words, there is no scheduling to CPU0.
 - It seems to be only used in krait-regulator which is I think
   8974-specific?

If you don't need the regulator on the L2 SPM you could just put an
error here rather than the CPU0 fallback. Or alternatively I'd try to
avoid the smp_call_function_single() for L2 since I don't see why those
should need to be called on a specific CPU.

Thanks,
Stephan

[1]: https://git.codelinaro.org/clo/la/kernel/msm/-/blob/LA.AF.1.2.1-08410-8064.0/arch/arm/mach-msm/spm_devices.c#L257