Re: [PATCH v4 07/20] PM / devfreq: Show the related information according to governor type
>
> This patch modifies the following sysfs entry of DEVFREQ framework
> because the devfreq device using passive governor don't need the same
> information of the devfreq device using rest governor.
> - polling_interval: passive gov don't use the sampling rate.
> - available_governors : passive gov don't be changed on runtime in this
> version.
> - trans_stat : passive governor don't support trans_stat in this
> version.
>
> Signed-off-by: Chanwoo Choi
> [linux.amoon: Tested on Odroid U3]
> Tested-by: Anand Moon
You have a major update that is not mendtioned in the commit message.
(add governor "type")
> ---
> drivers/devfreq/devfreq.c | 31
> +--
> drivers/devfreq/governor.h| 7 +++
> drivers/devfreq/governor_passive.c| 1 +
> drivers/devfreq/governor_performance.c| 1 +
> drivers/devfreq/governor_powersave.c | 1 +
> drivers/devfreq/governor_simpleondemand.c | 1 +
> drivers/devfreq/governor_userspace.c | 1 +
> include/linux/devfreq.h | 2 ++
> 8 files changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 78ea4cdaa82c..18ad956fec93 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -597,7 +597,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
> goto err_init;
> }
>
> - if (!strncmp(devfreq->governor_name, "passive", 7)) {
> + if (devfreq->governor->type == DEVFREQ_GOV_PASSIVE) {
> struct devfreq *parent_devfreq =
> ((struct devfreq_passive_data *)data)->parent;
As mentioned in a previous reply, this code may be removed.
>
> @@ -963,13 +963,23 @@ static ssize_t available_governors_show(struct device
> *d,
> struct device_attribute *attr,
> char *buf)
> {
> - struct devfreq_governor *tmp_governor;
> + struct devfreq *devfreq = to_devfreq(d);
> ssize_t count = 0;
>
> mutex_lock(&devfreq_list_lock);
> - list_for_each_entry(tmp_governor, &devfreq_governor_list, node)
> + if (devfreq->governor->type == DEVFREQ_GOV_PASSIVE) {
> count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> -"%s ", tmp_governor->name);
> +"%s ", devfreq->governor->name);
> + } else {
> + struct devfreq_governor *tmp_governor;
> +
> + list_for_each_entry(tmp_governor, &devfreq_governor_list, node)
> {
> + if (tmp_governor->type == DEVFREQ_GOV_PASSIVE)
> + continue;
> + count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> +"%s ", tmp_governor->name);
> + }
> + }
Uh no. As long as we do not have a list of all possible governors
for each device, let's stick with what we've defined in ABI documentation:
show all available governors "in the system".
You MAY have a device that may run both PASSIVE and USERSPACE.
> mutex_unlock(&devfreq_list_lock);
>
> /* Truncate the trailing space */
> @@ -1006,6 +1016,11 @@ static DEVICE_ATTR_RO(target_freq);
> static ssize_t polling_interval_show(struct device *dev,
>struct device_attribute *attr, char *buf)
> {
> + struct devfreq *df = to_devfreq(dev);
> +
> + if (df->governor->type == DEVFREQ_GOV_PASSIVE)
> + return sprintf(buf, "Not Supported.\n");
> +
> return sprintf(buf, "%d\n", to_devfreq(dev)->profile->polling_ms);
> }
When polling interval is irrlevent with the governor,
you don't need to print "Not Supported". Instead,
printing "0" is enough, (polling_ms=0 == no polling)
which is what devfreq is doing now.
>
> @@ -1020,6 +1035,9 @@ static ssize_t polling_interval_store(struct device
> *dev,
> if (!df->governor)
> return -EINVAL;
>
> + if (df->governor->type == DEVFREQ_GOV_PASSIVE)
> + return -EINVAL;
> +
Please simply return -EINVAL with governor's event_handler with
DEVFREQ_GOV_INTERVAL
(I see the return value checking is missing at df->governor->event_handler().
You may want to add return value checking there to properly handle what you
want.)
> ret = sscanf(buf, "%u", &value);
> if (ret != 1)
> return -EINVAL;
> @@ -1137,11 +1155,12 @@ static ssize_t trans_stat_show(struct device *dev,
> int i, j;
> unsigned int max_state = devfreq->profile->max_state;
>
> + if (max_state == 0 || devfreq->governor->type == DEVFREQ_GOV_PASSIVE)
> + return sprintf(buf, "Not Supported.\n");
> +
> if (!devfreq->stop_polling &&
> devfreq_update_status(devfreq, devfreq->previous_freq))
> return 0;
> - if (max_state == 0)
> -
Re: [PATCH v4 06/20] PM / devfreq: Add devfreq_get_devfreq_by_phandle()
>
> This patch adds the new devfreq_get_devfreq_by_phandle() OF helper function
> which can find the instance of devfreq device by using phandle ("devfreq").
>
> Signed-off-by: Chanwoo Choi
> [linux.amoon: Tested on Odroid U3]
> Tested-by: Anand Moon
Signed-off-by: MyungJoo Ham
> ---
> drivers/devfreq/devfreq.c | 44
> include/linux/devfreq.h | 9 +
> 2 files changed, 53 insertions(+)
>
Re: [PATCH v4 05/20] PM / devfreq: Add new passive governor
>
> This patch adds the new passive governor for DEVFREQ framework. The following
> governors are already present and used for DVFS (Dynamic Voltage and Frequency
> Scaling) drivers. The following governors are independently used for one
> device
> driver which don't give the influence to other device drviers and also don't
> receive the effect from other device drivers.
> - ondemand / performance / powersave / userspace
>
> The passive governor depends on operation of parent driver with specific
> governos extremely and is not able to decide the new frequency by oneself.
> According to the decided new frequency of parent driver with governor,
> the passive governor uses it to decide the appropriate frequency for own
> device driver. The passive governor must need the following information
> from device tree:
> - the source clock and OPP tables
> - the instance of parent device
>
> For exameple,
> there are one more devfreq device drivers which need to change their source
> clock according to their utilization on runtime. But, they share the same
> power line (e.g., regulator). So, specific device driver is operated as parent
> with ondemand governor and then the rest device driver with passive governor
> is influenced by parent device.
>
> Suggested-by: Myungjoo Ham
> Signed-off-by: Chanwoo Choi
> [linux.amoon: Tested on Odroid U3]
> Tested-by: Anand Moon
> ---
> drivers/devfreq/Kconfig| 9
> drivers/devfreq/Makefile | 1 +
> drivers/devfreq/devfreq.c | 47
> drivers/devfreq/governor_passive.c | 108
> +
> include/linux/devfreq.h| 15 ++
> 5 files changed, 180 insertions(+)
> create mode 100644 drivers/devfreq/governor_passive.c
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 55ec774f794c..d03f635a93e1 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -64,6 +64,15 @@ config DEVFREQ_GOV_USERSPACE
> Otherwise, the governor does not change the frequnecy
> given at the initialization.
>
> +config DEVFREQ_GOV_PASSIVE
> + tristate "Passive"
> + help
> + Sets the frequency by other governors (simple_ondemand, performance,
> + powersave, usersapce) of a parent devfreq device. This governor
> + always has the dependency on the chosen frequency from paired
> + governor. This governor does not change the frequency by oneself
> + through sysfs entry.
Sets the frequency based on the frequency of its parent devfreq
device. This governor does not change the frequency by itself
through sysfs entries.
> +
> comment "DEVFREQ Drivers"
>
> config ARM_EXYNOS_BUS_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 375ebbb4fcfb..f81c313b4b79 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
[]
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 984c5e9e7bdd..15e58779e4c0 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -190,6 +190,31 @@ static struct devfreq_governor
> *find_devfreq_governor(const char *name)
>
> /* Load monitoring helper functions for governors use */
>
> +static int update_devfreq_passive(struct devfreq *devfreq, unsigned long
> freq)
> +{
> + struct devfreq *passive;
> + unsigned long rate;
> + int ret;
> +
> + list_for_each_entry(passive, &devfreq->passive_dev_list, passive_node) {
> + if (!passive->governor)
> + continue;
> + rate = freq;
> +
> + ret = passive->governor->get_target_freq(passive, &rate);
> + if (ret)
> + return ret;
> +
> + ret = passive->profile->target(passive->dev.parent, &rate, 0);
> + if (ret)
> + return ret;
> +
> + passive->previous_freq = rate;
> + }
> +
> + return 0;
> +}
> +
> /**
> * update_devfreq() - Reevaluate the device and configure frequency.
> * @devfreq: the devfreq instance.
> @@ -233,10 +258,18 @@ int update_devfreq(struct devfreq *devfreq)
> flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
> }
>
> + if (!list_empty(&devfreq->passive_dev_list)
> + && devfreq->previous_freq > freq)
> + update_devfreq_passive(devfreq, freq);
> +
Could you please comment somewhere appropriate
that the dependent is going to be changed
before its parent if the frequency is going
Re: [PATCH v4 04/20] ARM: dts: Add DMC bus frequency for exynos3250-rinato/monk
> > This patch adds the DMC (Dynamic Memory Controller) bus frequency node > which includes the devfreq-events and regulator properties. The bus > frequency support the DVFS (Dynamic Voltage Frequency Scaling) feature > with ondemand governor. > > The devfreq-events (ppmu_dmc0*) can monitor the utilization of DMC bus > on runtime and the buck1_reg (VDD_MIF power line) supplies the power to > the DMC block. > > Signed-off-by: Chanwoo Choi > Reviewed-by: Krzysztof Kozlowski Acked-by: MyungJoo Ham
Re: [PATCH v4 03/20] ARM: dts: Add DMC bus node for Exynos3250
>
> This patch adds the DMC (Dynamic Memory Controller) bus node for Exynos3250
> SoC.
> The DMC is an AMBA AXI-compliant slave to interface external JEDEC standard
> SDRAM devices. The bus includes the OPP tables and the source clock for DMC
> block.
>
> Following list specifies the detailed relation between the clock and DMC
> block:
> - The source clock of DMC block : div_dmc
>
> Signed-off-by: Chanwoo Choi
> Reviewed-by: Krzysztof Kozlowski
Acked-by: MyungJoo Ham
N�r��yb�X��ǧv�^�){.n�+���z��z��z)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
Re: [PATCH v4 02/20] PM / devfreq: exynos: Add documentation for generic exynos bus frequency driver
>
> This patch adds the documentation for generic exynos bus frequency
> driver.
>
> Signed-off-by: Chanwoo Choi
> Reviewed-by: Krzysztof Kozlowski
A little changes following:
> ---
> .../devicetree/bindings/devfreq/exynos-bus.txt | 93
> ++
> 1 file changed, 93 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/devfreq/exynos-bus.txt
>
> diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> new file mode 100644
> index ..e32daef328da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> @@ -0,0 +1,93 @@
> +* Generic Exynos Bus frequency device
> +
> +The Samsung Exynos SoC have many buses for data transfer between DRAM
+The Samsung Exynos SoC has many buses for data transfer between DRAM
or
+The Samsung Exynos SoCs have many buses for data transfer between DRAM
(because you intend to support mulitple Exynos SoCs)
> +and sub-blocks in SoC. Almost Exynos SoC have the common architecture
+and sub-blocks in SoC. Most Exynos SoCs share the common architecture
> +for buses. Generally, the each bus of Exynos SoC includes the source clock
+for buses. Generally, each bus of Exynos SoC includes a source clock
> +and power line and then is able to change the clock according to the usage
+and a power line, which are able to change the clock frequency
> +of each buses on runtime. When gathering the usage of each buses on runtime,
+of the bus in runtime. To monitor the usage of each bus in runtime,
> +the driver uses the PPMU (Platform Performance Monitoring Unit) which
+the driver uses the PPMU (Platform Performance Monitoring Unit), which
> +is able to measure the current load of sub-blocks.
> +
> +There are a little different composition among Exynos SoC because each Exynos
> +SoC has the different sub-blocks. So, this difference should be specified
+SoC has different sub-blocks. Therefore, such difference should be specified
> +in devicetree file instead of each device driver. In result, this driver
> +is able to support the bus frequency for all Exynos SoCs.
> +
[]
N�r��yb�X��ǧv�^�){.n�+���z��z��z)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
Re: [PATCH v4 01/20] PM / devfreq: exynos: Add generic exynos bus frequency driver
>
> 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
> [linux.amoon: Tested on Odroid U3]
> Tested-by: Anand Moon
>
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(st
Re: Re: [RFC PATCH 02/15] PM / devfreq: exynos: Add documentation for generic exynos bus frequency driver
> Hi Rob, > > On Sat, Nov 28, 2015 at 5:30 AM, Rob Herring wrote: > > On Thu, Nov 26, 2015 at 10:47:26PM +0900, Chanwoo Choi wrote: > >> This patch adds the documentation for generic exynos bus frequency > >> driver. > >> > >> Signed-off-by: Chanwoo Choi > >> --- > >> .../devicetree/bindings/devfreq/exynos-bus.txt | 92 > >> ++ > >> 1 file changed, 92 insertions(+) > >> create mode 100644 > >> Documentation/devicetree/bindings/devfreq/exynos-bus.txt > >> +Example2 : > >> + The bus of DMC block in exynos3250.dtsi are listed below: > > > > What is DMC? > > DMC (DRAM Memory Controller) It's Dynamic Memory Controller. (DRAM =~ Dynamic Memory) You may need to write the full name with the first reference of DMC there in the documentation as "DMC" may confuse a lot of people. Cheers, MyungJoo
Re: [RFC PATCH 01/15] PM / devfreq: exynos: Add generic exynos bus frequency driver
On Thu, Nov 26, 2015 at 10:47 PM, Chanwoo Choi 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 > --- > drivers/devfreq/Kconfig | 15 ++ > drivers/devfreq/Makefile| 1 + > drivers/devfreq/exynos/Makefile | 1 + > drivers/devfreq/exynos/exynos-bus.c | 443 > > 4 files changed, 460 insertions(+) > create mode 100644 drivers/devfreq/exynos/exynos-bus.c > Are we finally getting a common Exynos bus driver with full DT support? (can this replace both Exynos4/5 drivers and support Exynos7 series?) Cheers, MyungJoo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] extcon: gpio: Add the support for Device tree bindings
>
> This patch adds the support for Device tree bindings of extcon-gpio driver.
> The extcon-gpio device tree node must include the both 'extcon-id' and
> 'extcon-gpio' property.
>
[]
>
> Signed-off-by: Chanwoo Choi
Except for some beautification issues described below,
Signed-off-by: MyungJoo Ham
> ---
> This patch is based on following patch[1].
> [1] https://lkml.org/lkml/2015/10/3/304
>
> Changes from v1:
> - Create the include/dt-bindings/extcon/extcon.h including the identification
> of external connector. These definitions are used in dts file.
> - Fix error if CONFIG_OF is disabled.
>
> .../devicetree/bindings/extcon/extcon-gpio.txt | 38 +++
> drivers/extcon/extcon-gpio.c | 110
> -
> include/dt-bindings/extcon/extcon.h| 44 +
> include/linux/extcon/extcon-gpio.h | 6 +-
> 4 files changed, 173 insertions(+), 25 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> create mode 100644 include/dt-bindings/extcon/extcon.h
>
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
> new file mode 100644
> index ..70c36f729963
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio.txt
[]
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index 279ff8f6637d..7f3e24aae0c4 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -1,8 +1,8 @@
> /*
> * extcon_gpio.c - Single-state GPIO extcon driver based on extcon class
> *
> - * Copyright (C) 2008 Google, Inc.
> - * Author: Mike Lockwood
> + * Copyright (C) 2015 Chanwoo Choi , Samsung
> Electronics
> + * Copyright (C) 2008 Mike Lockwood , Google, Inc.
> *
> * Modified by MyungJoo Ham to support extcon
> * (originally switch class is supported)
Let's make it in chronological order.
(may need to "beautify the last two lines as well)
I.e., 2008-->2012-->2015 or 2015-->2012-->2008.
Not 2015-->2008-->2012
> @@ -26,12 +26,14 @@
[]
> diff --git a/include/dt-bindings/extcon/extcon.h
> b/include/dt-bindings/extcon/extcon.h
> new file mode 100644
> index ..14c7f36b2206
> --- /dev/null
> +++ b/include/dt-bindings/extcon/extcon.h
[]
> diff --git a/include/linux/extcon/extcon-gpio.h
> b/include/linux/extcon/extcon-gpio.h
> index 7cacafb78b09..bcc6d7f7116a 100644
> --- a/include/linux/extcon/extcon-gpio.h
> +++ b/include/linux/extcon/extcon-gpio.h
[]
> @@ -38,7 +38,7 @@ struct gpio_extcon_pdata {
> unsigned int extcon_id;
> unsigned gpio;
> bool gpio_active_low;
> - unsigned long debounce;
> + unsigned int debounce;
What about u32, making it more clear?
( > + device_property_read_u32(dev, "debounce-ms", &pdata->debounce); )
> unsigned long irq_flags;
>
> bool check_on_resume;
> --
> 1.8.0
N�r��yb�X��ǧv�^�){.n�+���z��z��z)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
Re: [RFC PATCH 0/2] thermal: Add generic devfreq cooling device
>
> This patchset introduce the generic devfreq cooling device for generic
> thermal
> framework. The devfreq devices are used ad cooling device to reduce the
> overheating temperature. This patch is based on drivers/thermal/cpu_cooling.c.
> The devfreq cooling device can change the ragne of the frequency table of
> devfreq device according to cooling level in device tree file.
Hi,
1. You've exported "update_devfreq()" in 1/2 and didn't use it anywhere.
2. If you've added "update_devfreq()" to notify devfreq driver when a new
max/min is defined, you'll need to add it at set_state, OR
You may do it with opp_enable()/opp_disable() function and let opp
notifiers do the homework for you. (no need to update_devfreq().
Cheers,
MyungJoo
N�r��yb�X��ǧv�^�){.n�+���z��z��z)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
Re: [PATCH v6 0/8] Add support for Tegra Activity Monitor
> Hello,
>
> something happened during the last cycle and an old version of the devfreq
> driver was merged.
>
> This thread contains patches that bring it up to date to the last submitted
> version and also incorporates the feedback that that version received, plus
> some other small fixes and improvements that came up during rebase and
> testing.
>
> These patches implement support for setting the rate of the EMC clock based on
> stats collected from the ACTMON, a piece of hw in the Tegra124 that counts
> memory accesses (among others).
>
> It depends on the following in-flight patches:
>
> * EMC driver: http://thread.gmane.org/gmane.linux.kernel/1907035
> * CPUFreq driver: http://thread.gmane.org/gmane.linux.kernel/1897078
>
> I have pushed a branch here for testing:
>
> http://cgit.collabora.com/git/user/tomeu/linux.git/log/?h=actmon-v6
>
> Regards,
>
> Tomeu
>
> Tomeu Vizoso (8):
> of: Add binding for NVIDIA Tegra ACTMON node
> PM / devfreq: tegra: Update to v5 of the submitted patches
> clk: tegra: Have EMC clock implement determine_rate()
> PM / devfreq: tegra: Use clock rate constraints
> PM / devfreq: tegra: remove operating-points
> PM / devfreq: tegra: Set drvdata before enabling the irq
> PM / devfreq: tegra: Enable interrupts after resuming the devfreq
> monitor
> ARM: tegra: Add Tegra124 ACTMON support
Acked-by: MyungJoo Ham
for all PM / devfreq patches (2, 4, 5, 6, 7)
And merged in for-rc tree with a little modification.
In the patch 2/8, I would like to add "const" in Line 748.
Would it be fine with you?
(You may look at:
https://git.kernel.org/cgit/linux/kernel/git/mzx/devfreq.git/log/?h=for-rc )
The diff after applying all 2/8 to 7/8 will be:
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 0d1edd5..8e633a6 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -740,7 +740,7 @@ static int tegra_devfreq_remove(struct platform_device
*pdev)
return 0;
}
-static struct of_device_id tegra_devfreq_of_match[] = {
+static const struct of_device_id tegra_devfreq_of_match[] = {
{ .compatible = "nvidia,tegra124-actmon" },
{ },
};
>
> .../devicetree/bindings/arm/tegra/actmon.txt | 28 ++
> arch/arm/boot/dts/tegra124.dtsi| 11 +
> drivers/clk/tegra/clk-emc.c| 19 +-
> drivers/devfreq/tegra-devfreq.c| 480
> +++--
> 4 files changed, 316 insertions(+), 222 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/arm/tegra/actmon.txt
>
> --
> 2.1.0
>
N�r��yb�X��ǧv�^�){.n�+���z��z��z)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
Re: Re: [PATCHv4 1/8] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor
>
> Dear Myungjoo,
>
> Thanks for your review.
>
> On 12/18/2014 03:24 PM, MyungJoo Ham wrote:
> > Hi Chanwoo,
> >
> > I love the idea and I now have a little mechanical issues in your code.
> >
> >> ---
> >> drivers/devfreq/Kconfig | 2 +
> >> drivers/devfreq/Makefile| 5 +-
> >> drivers/devfreq/devfreq-event.c | 449
> >>
> >> drivers/devfreq/event/Makefile | 1 +
> >> include/linux/devfreq.h | 160 ++
> >> 5 files changed, 616 insertions(+), 1 deletion(-)
> >> create mode 100644 drivers/devfreq/devfreq-event.c
> >> create mode 100644 drivers/devfreq/event/Makefile
> >>
[]
>
> >
> >
> > [snip]
> >
> >> diff --git a/drivers/devfreq/devfreq-event.c
> >> b/drivers/devfreq/devfreq-event.c
> >> new file mode 100644
> >> index 000..0e1948e
> >> --- /dev/null
> >> +++ b/drivers/devfreq/devfreq-event.c
> >> @@ -0,0 +1,449 @@
> >> +/*
> >> + * devfreq-event: Generic DEVFREQ Event class driver
> >
> > DEVFREQ is a generic DVFS mechanism (or subsystem).
> >
> > Plus, I thought devfreq-event is considered to be a "framework"
> > for devfreq event class drivers. Am I mistaken?
>
> You're right. just "class driver" description is not proper.
> I'll modify the description of devfreq-event.c as following:
> or If you have other opinion, would you please let me know about it?
>
> devfreq-event: DEVFREQ-Event Framework to provide raw data of Non-CPU
> Devices.
devfreq-event: a framework to provide raw data and events of devfreq devices
should be enough.
[]
> > [snip / reversed maybe.. sorry]
> >
> >> +/**
> >> + * devfreq_event_is_enabled() - Check whether devfreq-event dev is
> >> enabled or
> >> + * not.
> >> + * @edev : the devfreq-event device
> >> + *
> >> + * Note that this function check whether devfreq-event dev is enabled or
> >> not.
> >> + * If return true, the devfreq-event dev is enabeld. If return false, the
> >> + * devfreq-event dev is disabled.
> >> + */
> >> +bool devfreq_event_is_enabled(struct devfreq_event_dev *edev)
> >> +{
> >> + bool enabled = false;
> >> +
> >> + if (!edev || !edev->desc)
> >> + return enabled;
> >> +
> >> + mutex_lock(&edev->lock);
> >> +
> >> + if (edev->enable_count > 0)
> >> + enabled = true;
> >> +
> >> + if (edev->desc->ops && edev->desc->ops->is_enabled)
> >> + enabled |= edev->desc->ops->is_enabled(edev);
> >
> > What does it mean when enabled_count > 0 and ops->is_enabled() is false?
> > or..
> > What does it mean when enabled_count = 0 and ops->is_enabled() is true?
> >
> > If you do enable_count in the subsystem, why would we rely on
> > ops->is_enabled()? Are you assuming that a device MAY turn itself off
> > without any kernel control (ops->disable()) and it is still a correct
> > behabior?
>
> You're right. devfreq_event_is_enabled() has ambiguous operation according to
> your comment.
>
> I'll only control the enable_count in the subsystem without ops->is_enabled()
> and then remove the is_enabled function in the structre devfreq_event_ops.
>
> Best Regards,
> Chanwoo Choi
>
[Off-Topic]
The name of devfreq-event may look quite intersecting with irq-driven governors,
which are being proposed these days.
Although they may look intersecting, we can have them independently;
this as a sub-class and that as a governor. And we can consider to
provide a common infrastructure for irq-driven mechanisms in devfreq or
devfreq-event when we irq-driven DVFS become more general, which I
expect in 2 ~ 3 years.
So for now, both can go independently.
Cheers!
MyungJoo
N�r��yb�X��ǧv�^�){.n�+���z��z��z)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
Re: [PATCHv4 1/8] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor
Hi Chanwoo,
I love the idea and I now have a little mechanical issues in your code.
> ---
> drivers/devfreq/Kconfig | 2 +
> drivers/devfreq/Makefile| 5 +-
> drivers/devfreq/devfreq-event.c | 449
>
> drivers/devfreq/event/Makefile | 1 +
> include/linux/devfreq.h | 160 ++
> 5 files changed, 616 insertions(+), 1 deletion(-)
> create mode 100644 drivers/devfreq/devfreq-event.c
> create mode 100644 drivers/devfreq/event/Makefile
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index faf4e70..4d15b62 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -87,4 +87,6 @@ config ARM_EXYNOS5_BUS_DEVFREQ
> It reads PPMU counters of memory controllers and adjusts the
> operating frequencies and voltages with OPP support.
>
> +comment "DEVFREQ Event Drivers"
> +
> endif # PM_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 16138c9..a1ffabe 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -1,4 +1,4 @@
> -obj-$(CONFIG_PM_DEVFREQ) += devfreq.o
> +obj-$(CONFIG_PM_DEVFREQ) += devfreq.o devfreq-event.o
> obj-$(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND)+= governor_simpleondemand.o
> obj-$(CONFIG_DEVFREQ_GOV_PERFORMANCE)+= governor_performance.o
> obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o
> @@ -7,3 +7,6 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o
> # DEVFREQ Drivers
> obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ)+= exynos/
> obj-$(CONFIG_ARM_EXYNOS5_BUS_DEVFREQ)+= exynos/
> +
> +# DEVFREQ Event Drivers
> +obj-$(CONFIG_PM_DEVFREQ) += event/
>
It looks getting mature fast.
However, I would like to suggest you to
allow not to compile devfreq-event.c and not include its compiled object
if devfreq.c is required but devfreq-event.c is not required.
(e.g., add CONFIG_PM_DEVFREQ_EVENT and let it be enabled when needed)
just a little concern for lightweight devices.
(this change might require a bit more work on the header as well)
- Or do you think devfreq-event.c will become almost mandatory for
most devfreq drivers?
[snip]
> diff --git a/drivers/devfreq/devfreq-event.c b/drivers/devfreq/devfreq-event.c
> new file mode 100644
> index 000..0e1948e
> --- /dev/null
> +++ b/drivers/devfreq/devfreq-event.c
> @@ -0,0 +1,449 @@
> +/*
> + * devfreq-event: Generic DEVFREQ Event class driver
DEVFREQ is a generic DVFS mechanism (or subsystem).
Plus, I thought devfreq-event is considered to be a "framework"
for devfreq event class drivers. Am I mistaken?
[snip]
> +struct devfreq_event_dev *devfreq_event_add_edev(struct device *dev,
> + struct devfreq_event_desc
> *desc)
> +{
> + struct devfreq_event_dev *edev;
> + static atomic_t event_no = ATOMIC_INIT(0);
> + int ret;
> +
> + if (!dev || !desc)
> + return ERR_PTR(-EINVAL);
> +
> + if (!desc->name || !desc->ops)
> + return ERR_PTR(-EINVAL);
> +
> + if (!desc->ops->set_event || !desc->ops->get_event)
> + return ERR_PTR(-EINVAL);
> +
> + edev = devm_kzalloc(dev, sizeof(*edev), GFP_KERNEL);
> + if (!edev)
> + return ERR_PTR(-ENOMEM);
> +
> + mutex_lock(&devfreq_event_list_lock);
You seem to lock that global lock too long. That lock is only required
while you operate the list. The data to be protected by this mutex is
devfreq_event_list. Until the new entry is added to the list, the new
entry is free from protection. (may be delayed right before list_add)
> + mutex_init(&edev->lock);
> + edev->desc = desc;
> + edev->dev.parent = dev;
> + edev->dev.class = devfreq_event_class;
> + edev->dev.release = devfreq_event_release_edev;
> +
> + dev_set_name(&edev->dev, "event.%d", atomic_inc_return(&event_no) -
> 1);
> + ret = device_register(&edev->dev);
> + if (ret < 0) {
> + put_device(&edev->dev);
> + mutex_unlock(&devfreq_event_list_lock);
> + return ERR_PTR(ret);
> + }
> + dev_set_drvdata(&edev->dev, edev);
> +
> + INIT_LIST_HEAD(&edev->node);
> + list_add(&edev->node, &devfreq_event_list);
> + mutex_unlock(&devfreq_event_list_lock);
> +
> + return edev;
> +}
[snip / reversed maybe.. sorry]
> +/**
> + * devfreq_event_is_enabled() - Check whether devfreq-event dev is enabled or
> + * not.
> + * @edev : the devfreq-event device
> + *
> + * Note that this function check whether devfreq-event dev is enabled or not.
> + * If return true, the devfreq-event dev is enabeld. If return false, the
> + * devfreq-event dev is disabled.
> + */
> +bool devfreq_event_is_enabled(struct devfreq_event_dev *edev)
> +{
> + bool enabled = false;
> +
> + if (!edev || !
Re: [RFC 1/3] devfreq: dt-bindings: Document Exynos3250 devfreq driver
>
> Add documentation for bindings used by Exynos3250 devfreq driver.
>
> Signed-off-by: Krzysztof Kozlowski
> ---
> .../bindings/arm/samsung/exynos3250-devfreq.txt| 66
> ++
> 1 file changed, 66 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/arm/samsung/exynos3250-devfreq.txt
>
> diff --git
> a/Documentation/devicetree/bindings/arm/samsung/exynos3250-devfreq.txt
> b/Documentation/devicetree/bindings/arm/samsung/exynos3250-devfreq.txt
> new file mode 100644
> index ..047955e9e371
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/samsung/exynos3250-devfreq.txt
> @@ -0,0 +1,66 @@
> +Samsung Exynos3250 devfreq driver
> +=
> +
> +The driver support changing frequencies and voltage for:
> + - memory controller and bus,
> + - peripheral buses (left and right).
> +
> +Memory controller and bus
> +=
> +Required properties:
> + - compatible : should be "samsung,exynos3250-busfreq-mif"
> + - reg : two sets (offset and length of the register) for PPMU registers
> + used by this devfreq driver
> + - clock-names : one clock of name "dmc" to manage frequency
> + - clocks : phandle and specifier for clock listed in clock-names property
> + - vdd_mif-supply : phandle to MIF voltage regulator
> +
> +Peripheral buses
> +
> +Required properties:
> + - compatible : should be "samsung,exynos3250-busfreq-int"
> + - reg : two sets (offset and length of the register) for PPMU registers
> + used by this devfreq driver
> + - clock-names : names for PPMU clocks and bus clocks to manage frequencies;
> + All following clock names (and corresponding phandles) must be
> + provided:
> + - "ppmu_left", "ppmu_right",
> + - "aclk_400", "aclk_266", "aclk_200", "aclk_160", "aclk_gdl",
> "aclk_gdr", "mfc";
> + - clocks : phandles and specifiers for clocks listed in clock-names property
> + - vdd_mif-supply : phandle to INT voltage regulator
> +
> +Example
> +===
> + busfreq_mif: busfreq@106A {
> + compatible = "samsung,exynos3250-busfreq-mif";
> + reg = <0x106A 0x2000>, <0x106B 0x2000>;
> + clocks = <&cmu_dmc CLK_DIV_DMC>;
> + clock-names = "dmc";
> + vdd_mif-supply = <&buck1_reg>;
> + status = "okay";
> + };
The hardware you are binding hereby is "Exynos PPMU".
You may consider to bind PPMU (DMC PPMU or BUS PPMU whichever hardware
you want to use) with DT and then let exynos bus devfreq driver use
the already-bound devices if found, ... in principle.
In other words or point of view, you may implement PPMU driver in
devfreq class device driver so that you let it bind PPMU device with DT.
It may be done similarly with the device below.
Cheers,
MyungJoo.
> +
> + busfreq_int: busfreq@116A {
> + compatible = "samsung,exynos3250-busfreq-int";
> + reg = <0x116A 0x2000>, <0x112A 0x2000>;
> + clocks = <&cmu CLK_PPMULEFT>,
> + <&cmu CLK_PPMURIGHT>,
> + <&cmu CLK_DIV_ACLK_400_MCUISP>,
> + <&cmu CLK_DIV_ACLK_266>,
> + <&cmu CLK_DIV_ACLK_200>,
> + <&cmu CLK_DIV_ACLK_160>,
> + <&cmu CLK_DIV_GDL>,
> + <&cmu CLK_DIV_GDR>,
> + <&cmu CLK_DIV_MFC>;
> + clock-names = "ppmuleft",
> + "ppmuright",
> + "aclk_400",
> + "aclk_266",
> + "aclk_200",
> + "aclk_160",
> + "aclk_gdl",
> + "aclk_gdr",
> + "mfc";
> + vdd_int-supply = <&buck3_reg>;
> + status = "okay";
> + };
> --
> 1.9.1
>
Re: Re: [PATCH v2 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor
> On 25 November 2014 at 08:07, MyungJoo Ham wrote:
> >> The ACTMON block can monitor several counters, providing averaging and
> >> firing
> >> interrupts based on watermarking configuration. This implementation
> >> monitors
> >> the MCALL and MCCPU counters to choose an appropriate frequency for the
> >> external memory clock.
> >>
> >> This patch is based on work by Alex Frid and Mikko
> >> Perttunen .
> >>
> >> Signed-off-by: Tomeu Vizoso
> >
> > Signed-off-by: MyungJoo Ham
> >
> > How are you going to integrate other two patches?
>
> I think they should go through the Tegra tree.
>
> > May I just go ahead with this patch only?
>
> Yes, I think that would be fine as the other patches aren't actual
> build or runtime dependencies. This driver shouldn't cause problems
> either if it's ran without the EMC and CPUFreq patches (though won't
> be fully functional, of course).
Ok, then, as long as the other two patches (for DT support) get
ACKed (seems fairly straightforward as well) or Signed-off by
corresponding tree maintainers, I feel fine to
send this patch along with the next pull request.
Thank you.
Cheers,
MyungJoo
>
> Regards,
>
> Tomeu
>
> > Cheers,
> > MyungJoo.
> >
>
N�r��yb�X��ǧv�^�){.n�+���z��z��z)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
Re: [PATCH v2 2/3] PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor
> The ACTMON block can monitor several counters, providing averaging and firing
> interrupts based on watermarking configuration. This implementation monitors
> the MCALL and MCCPU counters to choose an appropriate frequency for the
> external memory clock.
>
> This patch is based on work by Alex Frid and Mikko
> Perttunen .
>
> Signed-off-by: Tomeu Vizoso
Signed-off-by: MyungJoo Ham
How are you going to integrate other two patches?
May I just go ahead with this patch only?
Cheers,
MyungJoo.
>
> ---
>
> v2: * Use devfreq
> ---
> drivers/devfreq/Kconfig | 10 +
> drivers/devfreq/Makefile| 1 +
> drivers/devfreq/tegra-devfreq.c | 718
>
> 3 files changed, 729 insertions(+)
> create mode 100644 drivers/devfreq/tegra-devfreq.c
>
N�r��yb�X��ǧv�^�){.n�+���z��z��z)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
Re: [PATCH RFC 0/8] Devfreq support for Exynos5250/5420
> This patchset adds devfreq support on Exynos5250 and Exynos5420. > The patches add the missing INT clock support for exynos5250 and > also add a new 5420 driver. We also have patches which add the PPMU > node and fix a typo in the exynos5250 driver. > > The patches have been tested on Exynos5250 based Snow board and SMDK5420 > EVT1 boards. They have been prepared on linux-next(20140424) with the > devfreq for-next branch and Shaik Ameer Basha's clock cleanup patches > merged (http://www.spinics.net/lists/arm-kernel/msg325313.html). > > There are a couple of concerns with these patches: > 1) The patches are using clock aliases to expose the required clocks to the > devfreq driver. > 2) The patches are making use of different compatible strings for the PPMU > node to differentiate between multiple SoCs even though the PPMU is the same. > > Kindly suggest a way to move forward with respect to the above. The Exynos PPMU drivers have been located in drivers/devfreq/exynos as devfreq drivers have been the only users of PPMU. There are developers trying to design common PPMU frameworks, including Chanwoo, it is not yet submitted. Because other Exynos5 SoCs have their bus/memif PPMU defined in drivers/devfreq/exynos, adding another ont in another directory seems not appropriate. Cheers, MyungJoo.
