devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCHv4 1/8] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor
@ 2014-12-18  6:24 MyungJoo Ham
  2014-12-18  7:23 ` Chanwoo Choi
  0 siblings, 1 reply; 4+ messages in thread
From: MyungJoo Ham @ 2014-12-18  6:24 UTC (permalink / raw)
  To: 최찬우
  Cc: 김국진, 박경민,
	rafael.j.wysocki@intel.com, mark.rutland@arm.com,
	ABHILASH KESAVAN, tomasz.figa@gmail.com, Krzysztof Kozlowski,
	대인기, linux-pm@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org

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 0000000..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 || !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?

> +
> +       mutex_unlock(&edev->lock);
> +
> +       return enabled;
> +}
> +EXPORT_SYMBOL_GPL(devfreq_event_is_enabled);


Cheers,
MyungJoo


^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: Re: [PATCHv4 1/8] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor
@ 2014-12-19  2:11 MyungJoo Ham
  2014-12-19  6:46 ` Chanwoo Choi
  0 siblings, 1 reply; 4+ messages in thread
From: MyungJoo Ham @ 2014-12-19  2:11 UTC (permalink / raw)
  To: 최찬우
  Cc: 김국진, 박경민,
	rafael.j.wysocki@intel.com, mark.rutland@arm.com,
	ABHILASH KESAVAN, tomasz.figa@gmail.com, Krzysztof Kozlowski,
	대인기, linux-pm@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org

>   
>  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 0000000..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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [PATCHv4 0/8] devfreq: Add devfreq-event class to provide raw data for devfreq device
@ 2014-12-16 23:26 Chanwoo Choi
  2014-12-16 23:26 ` [PATCHv4 1/8] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor Chanwoo Choi
  0 siblings, 1 reply; 4+ messages in thread
From: Chanwoo Choi @ 2014-12-16 23:26 UTC (permalink / raw)
  To: myungjoo.ham
  Cc: kgene.kim, kyungmin.park, rafael.j.wysocki, mark.rutland,
	a.kesavan, tomasz.figa, k.kozlowski, cw00.choi, inki.dae,
	linux-pm, linux-kernel, devicetree, linux-arm-kernel,
	linux-samsung-soc

This patchset add new devfreq_event class to provide raw data to determine
current utilization of device  which is used for devfreq governor.

[Description of devfreq-event class]
This patchset add new devfreq_event class for devfreq_event device which provide
raw data (e.g., memory bus utilization/GPU utilization). This raw data from
devfreq_event data would be used for the governor of devfreq subsystem.
- devfreq_event device : Provide raw data for governor of existing devfreq device
- devfreq device       : Monitor device state and change frequency/voltage of device
                         using the raw data from devfreq_event device

The devfreq subsystem support generic DVFS(Dynamic Voltage/Frequency Scaling)
for Non-CPU Devices. The devfreq device would dertermine current device state
using various governor (e.g., ondemand, performance, powersave). After completed
determination of system state, devfreq device would change the frequency/voltage
of devfreq device according to the result of governor.

But, devfreq governor must need basic data which indicates current device state.
Existing devfreq subsystem only consider devfreq device which check current system
state and determine proper system state using basic data. There is no subsystem
for device providing basic data to devfreq device.

The devfreq subsystem must need devfreq_event device(data-provider device) for
existing devfreq device. So, this patch add new devfreq_event class for
devfreq_event device which read various basic data(e.g, memory bus utilization,
GPU utilization) and provide measured data to existing devfreq device through
standard APIs of devfreq_event class.

The following description explains the feature of two kind of devfreq class:
- devfreq class (existing)
 : devfreq consumer device use raw data from devfreq_event device for
   determining proper current system state and change voltage/frequency
   dynamically using various governors.
- devfreq_event class (new)
 : Provide measured raw data to devfreq device for governor

Also, the devfreq-event device would support various type event as following:
 : DEVFREQ_EVENT_TYPE_RAW_DATA
 : DEVFREQ_EVENT_TYPE_UTILIZATION
 : DEVFREQ_EVENT_TYPE_BANDWIDTH
 : DEVFREQ_EVENT_TYPE_LATENCY

[For example]
If board dts includes PPMU_DMC0/DMC1/LEFTBUS/RIGHTBUS event node,
would show following sysfs entry. Also devfreq driver(e.g., exynos4_bus.c)
can get the instance of devfreq-event device by using provided API and then
get raw data which reflect the current state of device.

-sh-3.2# cd /sys/class/devfreq-event/
-sh-3.2# ls -al
total 0
drwxr-xr-x  2 root root 0 Jan  9 16:47 .
drwxr-xr-x 37 root root 0 Jan  9 16:47 ..
lrwxrwxrwx  1 root root 0 Jan  9 16:47 event.0 -> ../../devices/soc/106a0000.ppmu_dmc0/devfreq-event/event.0
lrwxrwxrwx  1 root root 0 Jan  9 16:47 event.1 -> ../../devices/soc/106b0000.ppmu_dmc1/devfreq-event/event.1
lrwxrwxrwx  1 root root 0 Jan  9 16:47 event.2 -> ../../devices/soc/112a0000.ppmu_rightbus/devfreq-event/event.2
lrwxrwxrwx  1 root root 0 Jan  9 16:47 event.3 -> ../../devices/soc/116a0000.ppmu_leftbus0/devfreq-event/event.3
-sh-3.2# 

Changes from v3:
1. devfreq-event class driver
- Fix return value of devfreq_event_get_event()
- Add new structure devfreq_event_data for devfreq_event_get_event()
- Modify the prototype of devfreq_event_get_event() function
- Call of_node_put after calling of_parse_phandle() to decrement refcount
2. exynos-ppmu driver
- Modify usage of devfreq_event_get_event() function
  according to new prototype of this funciton
- Add the additional description to exynos-ppmu.txt how to add PPMU node
  in board dts file
- Use 'PPMU_EVENT' macro to remove duplicate codes
- Add the support of PPMU for Exynos5260
3. exynos dts file
- Add missing PPMU_FSYS node to exynos3250.dtsi
- Fix 'ppmu_mfc_l' node name as 'ppmu_mfc' because exynos3250 has only one MFC IP.
- Add missing PPMU_ACP/G3D to exynos4.dtsi
4. etc
- Fix wrong abbreviation of PPMU (PPMU :Platform Performance Monitoring Unit)
- Add new patch to support the PPMU of Exynos5260 SoC

Changes from v2:
1. devfreq-event class driver
- Rename all the helper functions of devfreq-event device
- Add devfreq_event_remove_edev() instead of devfreq_put_event_dev()
- Add devfreq_event_release_edev() to initialize it before put device
- Add the detailed description of devfreq-event API
- Add the attributes of devfreq-event class (enable_count)
- Check the overflow about event/total_event data in devfreq_event_get_event()
- Remove the 'exclusive flag' feature
- Set set_event()/get_event() functions as mandary
- Add missing of_node_put() call
- Change variable type of 'get_event()' funciton from 'int' to 'u64'
2. exynos-ppmu driver
- Remove un-used field (struct devfreq)
- Use 'of_get_child_by_name()' instead of 'of_find_node_by_name()'
- Add missing of_node_put() call
- Fix wrong clock control
- Use devfreq_event_remove_edev() instead of devfreq_remove_device()
- Add the documentation for exynos-ppmu driver
- Remove 'enable/disable/is_enabled/reset' function of exynos-ppmu driver
3. exynos3250-rinato.dts
- Add ppmu_{leftbus|rightbus} dt node and remove ppmu_cpu dt node

Changes from v1:
- Code clean
- Add the description of devfreq-event structure
- Add 'is_enabled' function to devfreq_event_ops structure
- Add 'enable_count' field to devfreq_event_dev structure
- Check whether devfreq-event device is enabled or not
  during calling devfreq_event API
- Define the type of devfreq-event device as following
  : DEVFREQ_EVENT_TYPE_RAW_DATA
  : DEVFREQ_EVENT_TYPE_UTILIZATION
  : DEVFREQ_EVENT_TYPE_BANDWIDTH
  : DEVFREQ_EVENT_TYPE_LATENCY
- Add the exclusive feature of devfreq-event device.
  If devfreq-event device is used on only on devfreq driver,
  should used 'devfreq_enable_event_dev_exclusive()' function
- Add new patch6 for test on Exynos3250-based Rinato board

Chanwoo Choi (8):
  devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor
  devfreq: event: Add the list of supported devfreq-event type
  devfreq: event: Add exynos-ppmu devfreq event driver
  devfreq: event: Add documentation for exynos-ppmu devfreq-event driver
  ARM: dts: Add PPMU dt node for Exynos3250 SoC
  ARM: dts: Add PPMU dt node for Exynos4 SoCs
  ARM: dts: Add PPMU dt node for Exynos5260 SoC
  ARM: dts: exynos: Add PPMU dt node to Exynos3250-based Rinato board

 .../bindings/devfreq/event/exynos-ppmu.txt         | 110 +++++
 arch/arm/boot/dts/exynos3250-rinato.dts            |  40 ++
 arch/arm/boot/dts/exynos3250.dtsi                  |  72 +++
 arch/arm/boot/dts/exynos4.dtsi                     | 108 +++++
 arch/arm/boot/dts/exynos4210.dtsi                  |   8 +
 arch/arm/boot/dts/exynos5260.dtsi                  |  90 ++++
 drivers/devfreq/Kconfig                            |  11 +
 drivers/devfreq/Makefile                           |   5 +-
 drivers/devfreq/devfreq-event.c                    | 493 +++++++++++++++++++++
 drivers/devfreq/event/Makefile                     |   2 +
 drivers/devfreq/event/exynos-ppmu.c                | 397 +++++++++++++++++
 include/linux/devfreq.h                            | 179 ++++++++
 12 files changed, 1514 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
 create mode 100644 drivers/devfreq/devfreq-event.c
 create mode 100644 drivers/devfreq/event/Makefile
 create mode 100644 drivers/devfreq/event/exynos-ppmu.c

-- 
1.8.5.5

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2014-12-19  6:46 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-18  6:24 [PATCHv4 1/8] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor MyungJoo Ham
2014-12-18  7:23 ` Chanwoo Choi
  -- strict thread matches above, loose matches on Subject: below --
2014-12-19  2:11 MyungJoo Ham
2014-12-19  6:46 ` Chanwoo Choi
2014-12-16 23:26 [PATCHv4 0/8] devfreq: Add devfreq-event class to provide raw data for devfreq device Chanwoo Choi
2014-12-16 23:26 ` [PATCHv4 1/8] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor Chanwoo Choi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).