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

[Chanwoo Choi <cw00.choi@samsung.com>]

On 2015년 12월 14일 17:28, MyungJoo Ham wrote:
>>   
>>  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.)

Sure. I can show it tomorrow whenever you want.
Before visiting to me, just let me know. I'll prepare the demonstartion.

Regards,
Chanwo oChoi

> 
>> 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�����r��y���b�X��ǧv�^�)޺{.n�+����{�����x,�ȧ�\x17��ܨ}���Ơz�&j:+v���\a����zZ+��+zf���h���~����i���z�\x1e�w���?����&�)ߢ^[fl===
>