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.c...@samsung.com>
>> [linux.amoon: Tested on Odroid U3]
>> Tested-by: Anand Moon <linux.am...@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.
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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
