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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxx>
>> [linux.amoon: Tested on Odroid U3]
>> Tested-by: Anand Moon <linux.amoon@xxxxxxxxx>
>>
> 
> 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.

In this series, patch11 deletes all of both exynos4_bus.c and exynos5_bus.c.

> 
> []
>> --- /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.

OK.
I'll add the 'voltage-tolerance' property as cpufreq-dt.c driver.
The cpufreq-dt.c get the percentage value by using 'voltage-tolerance'
devicetree property.

For example,
	if (of_property_read_u32(np, "exynos,voltage-tolerance",
			&bus->voltage_tolerance))
		bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;

	tol = new_volt * bus->voltage_tolerance / 100;
	regulator_set_voltage_tol(regulator, new_volt, tol);

> 
> []
>> +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.

OK. I'll modify it (%ld -> %lu)

> 
> []
>> +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?

OK. I'll handle the error of clock control.

> 
> []
> 
>> +#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.

Yes. It is not always same. I'll pass the role of handling the VDD_INT/VDD_MIF
regulator to regulator framework. The regulator framework handles the state
in suspend state by using 'regulator-off-in-suspend' property as following:

For example, in arch/arm/boot/dts/exynos4412-trats.dts:
	buck1_reg: BUCK1 {
		regulator-name = "vdd_mif";
		regulator-min-microvolt = <850000>;
		regulator-max-microvolt = <1100000>;
		regulator-always-on;
		regulator-boot-on;
		regulator-state-mem {
			regulator-off-in-suspend;
		};
	};

	buck3_reg: BUCK3 {
		regulator-name = "vdd_int";
		regulator-min-microvolt = <850000>;
		regulator-max-microvolt = <1150000>;
		regulator-always-on;
		regulator-boot-on;
		regulator-state-mem {
			regulator-off-in-suspend;
		};
	};

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

Thanks,
Chanwoo Choi


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux