Re: [PATCH v4 01/20] PM / devfreq: exynos: Add generic exynos bus frequency driver

[MyungJoo Ham <myungjoo.ham@samsung.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 5235 bytes --]

>   
>  This patch adds the generic exynos bus frequency driver for AMBA AXI bus
> of sub-blocks in exynos SoC with DEVFREQ framework. The Samsung Exynos SoC
> have the common architecture for bus between DRAM and sub-blocks in SoC.
> This driver can support the generic bus frequency driver for Exynos SoCs.
> 
> In devicetree, Each bus block has a bus clock, regulator, operation-point
> and devfreq-event devices which measure the utilization of each bus block.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> [linux.amoon: Tested on Odroid U3]
> Tested-by: Anand Moon <linux.amoon@gmail.com>
> 

Chanwoo, could you please show me testing this set of patches in your site?
Please let me know when is ok to visit you.
(I do not have exynos machines right now.)

> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 5134f9ee983d..375ebbb4fcfb 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE)	+= governor_powersave.o
>  obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
>  
>  # DEVFREQ Drivers
> +obj-$(CONFIG_ARCH_EXYNOS)		+= exynos/
>  obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ)	+= exynos/
>  obj-$(CONFIG_ARM_EXYNOS5_BUS_DEVFREQ)	+= exynos/

CONFIG_ARCH_EXYNOS is true if
	CONFIG_ARM_EXYNOS4_BUS_DEVFREQ is true 
	or
	CONFIG_ARM_EXYNOS5_BUS_DEVFREQ is true
Thus, the two lines after you've added have become useless. (dead code)

Please delete them.

[]
> --- /dev/null
> +++ b/drivers/devfreq/exynos/exynos-bus.c
[]
> +static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
> +{
> +	struct exynos_bus *bus = dev_get_drvdata(dev);
> +	struct dev_pm_opp *new_opp;
> +	unsigned long old_freq, new_freq, old_volt, new_volt;
> +	int ret = 0;
> +
> +	/* Get new opp-bus instance according to new bus clock */
> +	rcu_read_lock();
> +	new_opp = devfreq_recommended_opp(dev, freq, flags);
> +	if (IS_ERR_OR_NULL(new_opp)) {
> +		dev_err(dev, "failed to get recommed opp instance\n");
> +		rcu_read_unlock();
> +		return PTR_ERR(new_opp);
> +	}
> +
> +	new_freq = dev_pm_opp_get_freq(new_opp);
> +	new_volt = dev_pm_opp_get_voltage(new_opp);
> +	old_freq = dev_pm_opp_get_freq(bus->curr_opp);
> +	old_volt = dev_pm_opp_get_voltage(bus->curr_opp);
> +	rcu_read_unlock();
> +
> +	if (old_freq == new_freq)
> +		return 0;
> +
> +	/* Change voltage and frequency according to new OPP level */
> +	mutex_lock(&bus->lock);
> +
> +	if (old_freq < new_freq) {
> +		ret = regulator_set_voltage(bus->regulator, new_volt, new_volt);

Setting the maximum volt same as the minimum volt is not recommended.
Especially for any DVFS mechanisms, I recommend to set values as:
min_volt = minimum voltage that does not harm the stability
max_volt = maximum voltage that does not break the circuit

Please refer to /include/linux/regulator/driver.h
"@set_voltage" comments.

For the rest of regulator_set_voltage usages, I'd say the same.

[]
> +static int exynos_bus_get_dev_status(struct device *dev,
> +				     struct devfreq_dev_status *stat)
> +{
> +	struct exynos_bus *bus = dev_get_drvdata(dev);
> +	struct devfreq_event_data edata;
> +	int ret;
> +
> +	rcu_read_lock();
> +	stat->current_frequency = dev_pm_opp_get_freq(bus->curr_opp);
> +	rcu_read_unlock();
> +
> +	ret = exynos_bus_get_event(bus, &edata);
> +	if (ret < 0) {
> +		stat->total_time = stat->busy_time = 0;
> +		goto err;
> +	}
> +
> +	stat->busy_time = (edata.load_count * 100) / bus->ratio;
> +	stat->total_time = edata.total_count;
> +
> +	dev_dbg(dev, "Usage of devfreq-event : %ld/%ld\n", stat->busy_time,
> +							stat->total_time);

These two values are unsigned long.

[]
> +static int exynos_bus_parse_of(struct device_node *np,
> +			      struct exynos_bus *bus)
> +{
> +	struct device *dev = bus->dev;
> +	unsigned long rate;
> +	int i, ret, count, size;
> +
> +	/* Get the clock to provide each bus with source clock */
> +	bus->clk = devm_clk_get(dev, "bus");
> +	if (IS_ERR(bus->clk)) {
> +		dev_err(dev, "failed to get bus clock\n");
> +		return PTR_ERR(bus->clk);
> +	}
> +
> +	ret = clk_prepare_enable(bus->clk);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get enable clock\n");
> +		return ret;
> +	}

[]

> +err_regulator:
> +	regulator_disable(bus->regulator);
> +err_opp:
> +	dev_pm_opp_of_remove_table(dev);
> +
> +	return ret;

No clk_disable_unprepare() somewhere in the error handling routines?

[]

> +#ifdef CONFIG_PM_SLEEP
> +static int exynos_bus_resume(struct device *dev)
> +{
[]
> +		ret = regulator_enable(bus->regulator);
[]
> +}
> +
> +static int exynos_bus_suspend(struct device *dev)
> +{
[]
> +		regulator_disable(bus->regulator);
[]
> +}
> +#endif

Isn't there any possibility that you should not disable at suspend callbacks?
If I remember correctly, we should not disable VDD-INT/VDD-MIF of Exynos4412
for suspend-to-RAM although it is "mostly" ok to do so, but not "always" ok.

In such cases, I guess it should be "selectively" disabled for suspend.
(some regulators support special "low power if suspended" modes for such cases)

[]
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥