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

[Stephan Gerhold <stephan@gerhold.net>]

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?

> ---
>  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.

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