Re: [PATCH v5 1/1] mfd: arizona: Export functions to control subsystem DVFS

[Charles Keepax]

On Mon, May 18, 2015 at 03:15:05PM +0100, Richard Fitzgerald wrote:
> The WM5102, WM8997, WM8998 and WM1814 codecs have an internal dynamic
> clock booster. When this booster is active, the DCVDD voltage must be
> increased. If all the currently active audio paths can run with the root
> SYSCLK we can disable the booster, allowing us to turn down DCVDD voltage
> to save power.
> 
> Previously this was being done by having the booster enable bit set
> as a side-effect of the LDO1 regulator driver, which is unexpected
> behaviour of a regulator and not compatible with using an external
> regulator.
> 
> This patch exports functions to handle the booster enable and
> DCVDD voltage, with each relevant subsystem flagging whether it can
> currently run without the booster. Note that these subsystems are
> stateless and none of them are nestable, so there's no need for
> reference counting, we only need a simple boolean for each subsystem
> of whether their current condition could require the booster or will
> allow us to turn the codec down to lower operating power.
> 
> Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/mfd/arizona-core.c       |  172 +++++++++++++++++++++++++++++++++++++-
>  include/linux/mfd/arizona/core.h |   13 +++
>  2 files changed, 183 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index 16828cc..0586624 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -94,6 +94,125 @@ int arizona_clk32k_disable(struct arizona *arizona)
>  }
>  EXPORT_SYMBOL_GPL(arizona_clk32k_disable);
>  
> +static int arizona_dvfs_apply_boost(struct arizona *arizona)
> +{
> +	int ret;
> +
> +	ret = regulator_set_voltage(arizona->dcvdd, 1800000, 1800000);
> +	if (ret != 0) {
> +		dev_err(arizona->dev,
> +			"Failed to boost DCVDD: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(arizona->regmap,
> +				ARIZONA_DYNAMIC_FREQUENCY_SCALING_1,
> +				ARIZONA_SUBSYS_MAX_FREQ, 1);
> +	if (ret != 0) {
> +		dev_err(arizona->dev,
> +			"Failed to enable subsys max: %d\n", ret);
> +
> +		regulator_set_voltage(arizona->dcvdd, 1200000, 1800000);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int arizona_dvfs_remove_boost(struct arizona *arizona)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(arizona->regmap,
> +				ARIZONA_DYNAMIC_FREQUENCY_SCALING_1,
> +				ARIZONA_SUBSYS_MAX_FREQ, 0);
> +	if (ret != 0) {
> +		dev_err(arizona->dev,
> +			"Failed to disable subsys max: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_set_voltage(arizona->dcvdd, 1200000, 1800000);
> +	if (ret != 0) {
> +		dev_err(arizona->dev,
> +			"Failed to unboost DCVDD : %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int arizona_dvfs_up(struct arizona *arizona, unsigned int flags)
> +{
> +	unsigned int old_flags;
> +	int ret = 0;
> +
> +	mutex_lock(&arizona->subsys_max_lock);
> +
> +	old_flags = arizona->subsys_max_rq;
> +	arizona->subsys_max_rq |= flags;
> +
> +	/* If currently caching the change will be applied in runtime resume */
> +	if (arizona->subsys_max_cached) {
> +		dev_dbg(arizona->dev, "subsys_max_cached (dvfs up)\n");
> +		goto out;
> +	}
> +
> +	if (arizona->subsys_max_rq != old_flags) {

Don't we want a similar check here to dvfs_down? Might as well
skip apply_boost if we can.

> +		switch (arizona->type) {
> +		case WM5102:
> +		case WM8997:
> +		case WM8998:
> +		case WM1814:
> +			ret = arizona_dvfs_apply_boost(arizona);
> +			break;
> +
> +		default:
> +			break;
> +		}

Would it make more sense to check the chip type at the start of
the function outside the mutex? Then we can simple return and do
no work if it is a chip that doesn't have DVFS?

> +
> +	}
> +out:
> +	mutex_unlock(&arizona->subsys_max_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(arizona_dvfs_up);
> +
> +int arizona_dvfs_down(struct arizona *arizona, unsigned int flags)
> +{
> +	unsigned int old_flags;
> +	int ret = 0;
> +
> +	mutex_lock(&arizona->subsys_max_lock);
> +
> +	old_flags = arizona->subsys_max_rq;
> +	arizona->subsys_max_rq &= ~flags;
> +
> +	/* If currently caching the change will be applied in runtime resume */
> +	if (arizona->subsys_max_cached) {
> +		dev_dbg(arizona->dev, "subsys_max_cached (dvfs down)\n");
> +		goto out;
> +	}
> +
> +	if ((old_flags != 0) && (arizona->subsys_max_rq == 0)) {
> +		switch (arizona->type) {
> +		case WM5102:
> +		case WM8997:
> +		case WM8998:
> +		case WM1814:
> +			ret = arizona_dvfs_remove_boost(arizona);
> +			break;
> +
> +		default:
> +			break;
> +		}

ditto here.

> +	}
> +out:
> +	mutex_unlock(&arizona->subsys_max_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(arizona_dvfs_down);
> +
>  static irqreturn_t arizona_clkgen_err(int irq, void *data)
>  {
>  	struct arizona *arizona = data;
> @@ -462,6 +581,31 @@ static int wm5102_clear_write_sequencer(struct arizona *arizona)
>  }
>  
>  #ifdef CONFIG_PM
> +static int arizona_restore_dvfs(struct arizona *arizona)
> +{
> +	int ret;
> +
> +	switch (arizona->type) {
> +	default:
> +		return 0;	/* no DVFS */
> +
> +	case WM5102:
> +	case WM8997:
> +	case WM8998:
> +	case WM1814:
> +		ret = 0;
> +		mutex_lock(&arizona->subsys_max_lock);
> +		if (arizona->subsys_max_rq != 0) {
> +			dev_dbg(arizona->dev, "Restore subsys_max boost\n");
> +			ret = arizona_dvfs_apply_boost(arizona);
> +		}
> +
> +		arizona->subsys_max_cached = false;
> +		mutex_unlock(&arizona->subsys_max_lock);
> +		return ret;
> +	}
> +}
> +
>  static int arizona_runtime_resume(struct device *dev)
>  {
>  	struct arizona *arizona = dev_get_drvdata(dev);
> @@ -590,6 +734,10 @@ static int arizona_runtime_resume(struct device *dev)
>  		goto err;
>  	}
>  
> +	ret = arizona_restore_dvfs(arizona);
> +	if (ret < 0)
> +		goto err;
> +
>  	return 0;
>  
>  err:
> @@ -612,6 +760,21 @@ static int arizona_runtime_suspend(struct device *dev)
>  		return ret;
>  	}
>  
> +	switch (arizona->type) {
> +	case WM5102:
> +	case WM8997:
> +	case WM8998:
> +	case WM1814:
> +		/* Must disable DVFS boost before powering down DCVDD */
> +		mutex_lock(&arizona->subsys_max_lock);
> +		arizona->subsys_max_cached = true;
> +		arizona_dvfs_remove_boost(arizona);
> +		mutex_unlock(&arizona->subsys_max_lock);
> +		break;
> +	default:
> +		break;
> +	}
> +

I would be tempted to factor this out into a function as well
since we have a restore_dvfs for resume.

Thanks,
Charles
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/