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).
>
> > ---
> > 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.
>
> I don't have too much experience with these really old platforms but at
> least on MSM8916 I can't think of any reason that would make the CPU0
> more special for the L2 cache than the others (MSM8916 has the regulator
> stuff only in the L2 SAW since there is just one CPU power rail).
>
> Thanks,
> Stephan
--
With best wishes
Dmitry
