On Thu, Dec 07, 2023 at 10:33:31PM +0200, Dmitry Baryshkov wrote:
> On Thu, 7 Dec 2023 at 19:12, Stephan Gerhold <stephan@xxxxxxxxxxx> 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@xxxxxxxxxx>
> >
> > 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
