[v3,2/4] drivers: devfreq: events: extend events by type of counted data
diff mbox series

Message ID 1555681688-19643-3-git-send-email-l.luba@partner.samsung.com
State Not Applicable
Headers show
Series
  • [v3,1/4] include: dt-bindings: add Performance Monitoring Unit for Exynos
Related show

Commit Message

Lukasz Luba April 19, 2019, 1:48 p.m. UTC
This patch adds posibility to choose what type of data should be counted
by the PPMU counter. Now the type comes from DT where the event has been
defined. When there is no 'event-data-type' the default value is used,
which is 'read data in bytes'.
It is needed when you want to know not only read+write data bytes but
i.e. only write data in byte, or number of read requests, etc.

Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 drivers/devfreq/event/exynos-ppmu.c | 61 +++++++++++++++++++++++++------------
 include/linux/devfreq-event.h       |  6 ++++
 2 files changed, 48 insertions(+), 19 deletions(-)

Comments

Chanwoo Choi April 30, 2019, 7:34 a.m. UTC | #1
Hi Lukasz,

On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
> This patch adds posibility to choose what type of data should be counted
> by the PPMU counter. Now the type comes from DT where the event has been
> defined. When there is no 'event-data-type' the default value is used,
> which is 'read data in bytes'.
> It is needed when you want to know not only read+write data bytes but
> i.e. only write data in byte, or number of read requests, etc.
> 
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  drivers/devfreq/event/exynos-ppmu.c | 61 +++++++++++++++++++++++++------------
>  include/linux/devfreq-event.h       |  6 ++++
>  2 files changed, 48 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
> index c61de0b..073bf2c 100644
> --- a/drivers/devfreq/event/exynos-ppmu.c
> +++ b/drivers/devfreq/event/exynos-ppmu.c
> @@ -154,9 +154,9 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
>  	if (ret < 0)
>  		return ret;
>  
> -	/* Set the event of Read/Write data count  */
> +	/* Set the event of proper data type monitoring */
>  	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id),
> -				PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT);
> +			   edev->desc->data_type);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -368,23 +368,11 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
>  	if (ret < 0)
>  		return ret;
>  
> -	/* Set the event of Read/Write data count  */
> -	switch (id) {
> -	case PPMU_PMNCNT0:
> -	case PPMU_PMNCNT1:
> -	case PPMU_PMNCNT2:
> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
> -				PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT);
> -		if (ret < 0)
> -			return ret;
> -		break;
> -	case PPMU_PMNCNT3:
> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
> -				PPMU_V2_EVT3_RW_DATA_CNT);
> -		if (ret < 0)
> -			return ret;
> -		break;
> -	}
> +	/* Set the event of proper data type monitoring */
> +	ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
> +			   edev->desc->data_type);
> +	if (ret < 0)
> +		return ret;
>  
>  	/* Reset cycle counter/performance counter and enable PPMU */
>  	ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
> @@ -508,6 +496,7 @@ static int of_get_devfreq_events(struct device_node *np,
>  	struct device *dev = info->dev;
>  	struct device_node *events_np, *node;
>  	int i, j, count;
> +	int ret;
>  
>  	events_np = of_get_child_by_name(np, "events");
>  	if (!events_np) {
> @@ -544,6 +533,40 @@ static int of_get_devfreq_events(struct device_node *np,
>  		desc[j].driver_data = info;
>  
>  		of_property_read_string(node, "event-name", &desc[j].name);
> +		ret = of_property_read_u32(node, "event-data-type",
> +					   &desc[j].data_type);
> +		if (ret) {
> +			/* Set the event of proper data type counting.
> +			 * Check if the data type has been defined in DT,
> +			 * use default if not.
> +			 */
> +			if (of_device_is_compatible(np,
> +					"samsung,exynos-ppmu-v2")) {

It is not proper to compare the compatible string again
in the device driver. Instead, you can define the ppmu device type
as following and then use 'struct of_device_id' in order to
identify the device type.

	enum exynos_ppmu_type {
		EXYNOS_TYPE_PPMU,
		EXYNOS_TYPE_PPMU_V2,
	};

 static const struct of_device_id exynos_ppmu_id_match[] = {
        {
                .compatible = "samsung,exynos-ppmu",
-               .data = (void *)&exynos_ppmu_ops,
+               .data = (void *)EXYNOS_TYPE_PPMU,
        }, {
                .compatible = "samsung,exynos-ppmu-v2",
-               .data = (void *)&exynos_ppmu_v2_ops,
+               .data = (void *)EXYNOS_TYPE_PPMU_V2,
        },


The many device drivers in the mainline uses this code
in order to get the device type. You can refer the example
in the drivers/mfd/max14577.c.

(snip)


Example, I add the example. but it is not tested. 

--- a/drivers/devfreq/event/exynos-ppmu.c
+++ b/drivers/devfreq/event/exynos-ppmu.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/suspend.h>
@@ -23,6 +24,11 @@
 
 #include "exynos-ppmu.h"
 
+enum exynos_ppmu_type {
+       EXYNOS_TYPE_PPMU,
+       EXYNOS_TYPE_PPMU_V2,
+};
+
 struct exynos_ppmu_data {
        struct clk *clk;
 };
@@ -36,6 +42,7 @@ struct exynos_ppmu {
        struct regmap *regmap;
 
        struct exynos_ppmu_data ppmu;
+       enum exynos_ppmu_type ppmu_type;
 };
 
 #define PPMU_EVENT(name)                       \
 
        /* Reset cycle counter/performance counter and enable PPMU */
        ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
@@ -483,31 +476,23 @@ static const struct devfreq_event_ops exynos_ppmu_v2_ops = {
 static const struct of_device_id exynos_ppmu_id_match[] = {
        {
                .compatible = "samsung,exynos-ppmu",
-               .data = (void *)&exynos_ppmu_ops,
+               .data = (void *)EXYNOS_TYPE_PPMU,
        }, {
                .compatible = "samsung,exynos-ppmu-v2",
-               .data = (void *)&exynos_ppmu_v2_ops,
+               .data = (void *)EXYNOS_TYPE_PPMU_V2,
        },
        { /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, exynos_ppmu_id_match);
 
-static struct devfreq_event_ops *exynos_bus_get_ops(struct device_node *np)
-{
-       const struct of_device_id *match;
-
-       match = of_match_node(exynos_ppmu_id_match, np);
-       return (struct devfreq_event_ops *)match->data;
-}
-
 static int of_get_devfreq_events(struct device_node *np,
                                 struct exynos_ppmu *info)
 {
        struct devfreq_event_desc *desc;
-       struct devfreq_event_ops *event_ops;
        struct device *dev = info->dev;
        struct device_node *events_np, *node;
        int i, j, count;
 
        events_np = of_get_child_by_name(np, "events");
        if (!events_np) {
@@ -515,7 +500,6 @@ static int of_get_devfreq_events(struct device_node *np,
                        "failed to get child node of devfreq-event devices\n");
                return -EINVAL;
        }
-       event_ops = exynos_bus_get_ops(np);
 
        count = of_get_child_count(events_np);
        desc = devm_kcalloc(dev, count, sizeof(*desc), GFP_KERNEL);
@@ -540,11 +524,38 @@ static int of_get_devfreq_events(struct device_node *np,
                        continue;
                }
 
-               desc[j].ops = event_ops;
+               switch (info->ppmu_type) {
+               case EXYNOS_TYPE_PPMU:
+                       desc[j].ops = &exynos_ppmu_ops;
+                       break;
+               case EXYNOS_TYPE_PPMU_V2:
+                       desc[j].ops = &exynos_ppmu_v2_ops;
+                       break;
+               }
+
Lukasz Luba April 30, 2019, 9:19 p.m. UTC | #2
Hi Chanwoo,

On 4/30/19 9:34 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
>> This patch adds posibility to choose what type of data should be counted
>> by the PPMU counter. Now the type comes from DT where the event has been
>> defined. When there is no 'event-data-type' the default value is used,
>> which is 'read data in bytes'.
>> It is needed when you want to know not only read+write data bytes but
>> i.e. only write data in byte, or number of read requests, etc.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   drivers/devfreq/event/exynos-ppmu.c | 61 +++++++++++++++++++++++++------------
>>   include/linux/devfreq-event.h       |  6 ++++
>>   2 files changed, 48 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
>> index c61de0b..073bf2c 100644
>> --- a/drivers/devfreq/event/exynos-ppmu.c
>> +++ b/drivers/devfreq/event/exynos-ppmu.c
>> @@ -154,9 +154,9 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	/* Set the event of Read/Write data count  */
>> +	/* Set the event of proper data type monitoring */
>>   	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id),
>> -				PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT);
>> +			   edev->desc->data_type);
>>   	if (ret < 0)
>>   		return ret;
>>   
>> @@ -368,23 +368,11 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	/* Set the event of Read/Write data count  */
>> -	switch (id) {
>> -	case PPMU_PMNCNT0:
>> -	case PPMU_PMNCNT1:
>> -	case PPMU_PMNCNT2:
>> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>> -				PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT);
>> -		if (ret < 0)
>> -			return ret;
>> -		break;
>> -	case PPMU_PMNCNT3:
>> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>> -				PPMU_V2_EVT3_RW_DATA_CNT);
>> -		if (ret < 0)
>> -			return ret;
>> -		break;
>> -	}
>> +	/* Set the event of proper data type monitoring */
>> +	ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>> +			   edev->desc->data_type);
>> +	if (ret < 0)
>> +		return ret;
>>   
>>   	/* Reset cycle counter/performance counter and enable PPMU */
>>   	ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
>> @@ -508,6 +496,7 @@ static int of_get_devfreq_events(struct device_node *np,
>>   	struct device *dev = info->dev;
>>   	struct device_node *events_np, *node;
>>   	int i, j, count;
>> +	int ret;
>>   
>>   	events_np = of_get_child_by_name(np, "events");
>>   	if (!events_np) {
>> @@ -544,6 +533,40 @@ static int of_get_devfreq_events(struct device_node *np,
>>   		desc[j].driver_data = info;
>>   
>>   		of_property_read_string(node, "event-name", &desc[j].name);
>> +		ret = of_property_read_u32(node, "event-data-type",
>> +					   &desc[j].data_type);
>> +		if (ret) {
>> +			/* Set the event of proper data type counting.
>> +			 * Check if the data type has been defined in DT,
>> +			 * use default if not.
>> +			 */
>> +			if (of_device_is_compatible(np,
>> +					"samsung,exynos-ppmu-v2")) {
> 
> It is not proper to compare the compatible string again
> in the device driver. Instead, you can define the ppmu device type
> as following and then use 'struct of_device_id' in order to
> identify the device type.
I have been thinking about modifying the code in similar fashion as you
did. Good to see similar approach. I'll take your changes with a small
additional code, which sets the 'info->ppmu_type' before the for
loop, as an additional patch. Would it be OK if I add you as an author
and add Sign-off-by: Chanwoo Choi <cw00.choi@samsung.com>?

Regards,
Lukasz
> 
> 	enum exynos_ppmu_type {
> 		EXYNOS_TYPE_PPMU,
> 		EXYNOS_TYPE_PPMU_V2,
> 	};
> 
>   static const struct of_device_id exynos_ppmu_id_match[] = {
>          {
>                  .compatible = "samsung,exynos-ppmu",
> -               .data = (void *)&exynos_ppmu_ops,
> +               .data = (void *)EXYNOS_TYPE_PPMU,
>          }, {
>                  .compatible = "samsung,exynos-ppmu-v2",
> -               .data = (void *)&exynos_ppmu_v2_ops,
> +               .data = (void *)EXYNOS_TYPE_PPMU_V2,
>          },
> 
> 
> The many device drivers in the mainline uses this code
> in order to get the device type. You can refer the example
> in the drivers/mfd/max14577.c.
> 
> (snip)
> 
> 
> Example, I add the example. but it is not tested.
> 
> --- a/drivers/devfreq/event/exynos-ppmu.c
> +++ b/drivers/devfreq/event/exynos-ppmu.c
> @@ -16,6 +16,7 @@
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/of_address.h>
> +#include <linux/of_device.h>
>   #include <linux/platform_device.h>
>   #include <linux/regmap.h>
>   #include <linux/suspend.h>
> @@ -23,6 +24,11 @@
>   
>   #include "exynos-ppmu.h"
>   
> +enum exynos_ppmu_type {
> +       EXYNOS_TYPE_PPMU,
> +       EXYNOS_TYPE_PPMU_V2,
> +};
> +
>   struct exynos_ppmu_data {
>          struct clk *clk;
>   };
> @@ -36,6 +42,7 @@ struct exynos_ppmu {
>          struct regmap *regmap;
>   
>          struct exynos_ppmu_data ppmu;
> +       enum exynos_ppmu_type ppmu_type;
>   };
>   
>   #define PPMU_EVENT(name)                       \
>   
>          /* Reset cycle counter/performance counter and enable PPMU */
>          ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
> @@ -483,31 +476,23 @@ static const struct devfreq_event_ops exynos_ppmu_v2_ops = {
>   static const struct of_device_id exynos_ppmu_id_match[] = {
>          {
>                  .compatible = "samsung,exynos-ppmu",
> -               .data = (void *)&exynos_ppmu_ops,
> +               .data = (void *)EXYNOS_TYPE_PPMU,
>          }, {
>                  .compatible = "samsung,exynos-ppmu-v2",
> -               .data = (void *)&exynos_ppmu_v2_ops,
> +               .data = (void *)EXYNOS_TYPE_PPMU_V2,
>          },
>          { /* sentinel */ },
>   };
>   MODULE_DEVICE_TABLE(of, exynos_ppmu_id_match);
>   
> -static struct devfreq_event_ops *exynos_bus_get_ops(struct device_node *np)
> -{
> -       const struct of_device_id *match;
> -
> -       match = of_match_node(exynos_ppmu_id_match, np);
> -       return (struct devfreq_event_ops *)match->data;
> -}
> -
>   static int of_get_devfreq_events(struct device_node *np,
>                                   struct exynos_ppmu *info)
>   {
>          struct devfreq_event_desc *desc;
> -       struct devfreq_event_ops *event_ops;
>          struct device *dev = info->dev;
>          struct device_node *events_np, *node;
>          int i, j, count;
>   
>          events_np = of_get_child_by_name(np, "events");
>          if (!events_np) {
> @@ -515,7 +500,6 @@ static int of_get_devfreq_events(struct device_node *np,
>                          "failed to get child node of devfreq-event devices\n");
>                  return -EINVAL;
>          }
> -       event_ops = exynos_bus_get_ops(np);
>   
>          count = of_get_child_count(events_np);
>          desc = devm_kcalloc(dev, count, sizeof(*desc), GFP_KERNEL);
> @@ -540,11 +524,38 @@ static int of_get_devfreq_events(struct device_node *np,
>                          continue;
>                  }
>   
> -               desc[j].ops = event_ops;
> +               switch (info->ppmu_type) {
> +               case EXYNOS_TYPE_PPMU:
> +                       desc[j].ops = &exynos_ppmu_ops;
> +                       break;
> +               case EXYNOS_TYPE_PPMU_V2:
> +                       desc[j].ops = &exynos_ppmu_v2_ops;
> +                       break;
> +               }
> +
> 
>
Chanwoo Choi May 2, 2019, 1:25 a.m. UTC | #3
Hi Lukasz,

On 19. 5. 1. 오전 6:19, Lukasz Luba wrote:
> Hi Chanwoo,
> 
> On 4/30/19 9:34 AM, Chanwoo Choi wrote:
>> Hi Lukasz,
>>
>> On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
>>> This patch adds posibility to choose what type of data should be counted
>>> by the PPMU counter. Now the type comes from DT where the event has been
>>> defined. When there is no 'event-data-type' the default value is used,
>>> which is 'read data in bytes'.
>>> It is needed when you want to know not only read+write data bytes but
>>> i.e. only write data in byte, or number of read requests, etc.
>>>
>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>> ---
>>>   drivers/devfreq/event/exynos-ppmu.c | 61 +++++++++++++++++++++++++------------
>>>   include/linux/devfreq-event.h       |  6 ++++
>>>   2 files changed, 48 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
>>> index c61de0b..073bf2c 100644
>>> --- a/drivers/devfreq/event/exynos-ppmu.c
>>> +++ b/drivers/devfreq/event/exynos-ppmu.c
>>> @@ -154,9 +154,9 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
>>>   	if (ret < 0)
>>>   		return ret;
>>>   
>>> -	/* Set the event of Read/Write data count  */
>>> +	/* Set the event of proper data type monitoring */
>>>   	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id),
>>> -				PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT);
>>> +			   edev->desc->data_type);
>>>   	if (ret < 0)
>>>   		return ret;
>>>   
>>> @@ -368,23 +368,11 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
>>>   	if (ret < 0)
>>>   		return ret;
>>>   
>>> -	/* Set the event of Read/Write data count  */
>>> -	switch (id) {
>>> -	case PPMU_PMNCNT0:
>>> -	case PPMU_PMNCNT1:
>>> -	case PPMU_PMNCNT2:
>>> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>>> -				PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT);
>>> -		if (ret < 0)
>>> -			return ret;
>>> -		break;
>>> -	case PPMU_PMNCNT3:
>>> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>>> -				PPMU_V2_EVT3_RW_DATA_CNT);
>>> -		if (ret < 0)
>>> -			return ret;
>>> -		break;
>>> -	}
>>> +	/* Set the event of proper data type monitoring */
>>> +	ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>>> +			   edev->desc->data_type);
>>> +	if (ret < 0)
>>> +		return ret;
>>>   
>>>   	/* Reset cycle counter/performance counter and enable PPMU */
>>>   	ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
>>> @@ -508,6 +496,7 @@ static int of_get_devfreq_events(struct device_node *np,
>>>   	struct device *dev = info->dev;
>>>   	struct device_node *events_np, *node;
>>>   	int i, j, count;
>>> +	int ret;
>>>   
>>>   	events_np = of_get_child_by_name(np, "events");
>>>   	if (!events_np) {
>>> @@ -544,6 +533,40 @@ static int of_get_devfreq_events(struct device_node *np,
>>>   		desc[j].driver_data = info;
>>>   
>>>   		of_property_read_string(node, "event-name", &desc[j].name);
>>> +		ret = of_property_read_u32(node, "event-data-type",
>>> +					   &desc[j].data_type);
>>> +		if (ret) {
>>> +			/* Set the event of proper data type counting.
>>> +			 * Check if the data type has been defined in DT,
>>> +			 * use default if not.
>>> +			 */
>>> +			if (of_device_is_compatible(np,
>>> +					"samsung,exynos-ppmu-v2")) {
>>
>> It is not proper to compare the compatible string again
>> in the device driver. Instead, you can define the ppmu device type
>> as following and then use 'struct of_device_id' in order to
>> identify the device type.
> I have been thinking about modifying the code in similar fashion as you
> did. Good to see similar approach. I'll take your changes with a small
> additional code, which sets the 'info->ppmu_type' before the for
> loop, as an additional patch. Would it be OK if I add you as an author
> and add Sign-off-by: Chanwoo Choi <cw00.choi@samsung.com>?

If you agree, just add the my signed-off-by on second line.

> 
> Regards,
> Lukasz
>>
>> 	enum exynos_ppmu_type {
>> 		EXYNOS_TYPE_PPMU,
>> 		EXYNOS_TYPE_PPMU_V2,
>> 	};
>>
>>   static const struct of_device_id exynos_ppmu_id_match[] = {
>>          {
>>                  .compatible = "samsung,exynos-ppmu",
>> -               .data = (void *)&exynos_ppmu_ops,
>> +               .data = (void *)EXYNOS_TYPE_PPMU,
>>          }, {
>>                  .compatible = "samsung,exynos-ppmu-v2",
>> -               .data = (void *)&exynos_ppmu_v2_ops,
>> +               .data = (void *)EXYNOS_TYPE_PPMU_V2,
>>          },
>>
>>
>> The many device drivers in the mainline uses this code
>> in order to get the device type. You can refer the example
>> in the drivers/mfd/max14577.c.
>>
>> (snip)
>>
>>
>> Example, I add the example. but it is not tested.
>>
>> --- a/drivers/devfreq/event/exynos-ppmu.c
>> +++ b/drivers/devfreq/event/exynos-ppmu.c
>> @@ -16,6 +16,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/of_address.h>
>> +#include <linux/of_device.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/regmap.h>
>>   #include <linux/suspend.h>
>> @@ -23,6 +24,11 @@
>>   
>>   #include "exynos-ppmu.h"
>>   
>> +enum exynos_ppmu_type {
>> +       EXYNOS_TYPE_PPMU,
>> +       EXYNOS_TYPE_PPMU_V2,
>> +};
>> +
>>   struct exynos_ppmu_data {
>>          struct clk *clk;
>>   };
>> @@ -36,6 +42,7 @@ struct exynos_ppmu {
>>          struct regmap *regmap;
>>   
>>          struct exynos_ppmu_data ppmu;
>> +       enum exynos_ppmu_type ppmu_type;
>>   };
>>   
>>   #define PPMU_EVENT(name)                       \
>>   
>>          /* Reset cycle counter/performance counter and enable PPMU */
>>          ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
>> @@ -483,31 +476,23 @@ static const struct devfreq_event_ops exynos_ppmu_v2_ops = {
>>   static const struct of_device_id exynos_ppmu_id_match[] = {
>>          {
>>                  .compatible = "samsung,exynos-ppmu",
>> -               .data = (void *)&exynos_ppmu_ops,
>> +               .data = (void *)EXYNOS_TYPE_PPMU,
>>          }, {
>>                  .compatible = "samsung,exynos-ppmu-v2",
>> -               .data = (void *)&exynos_ppmu_v2_ops,
>> +               .data = (void *)EXYNOS_TYPE_PPMU_V2,
>>          },
>>          { /* sentinel */ },
>>   };
>>   MODULE_DEVICE_TABLE(of, exynos_ppmu_id_match);
>>   
>> -static struct devfreq_event_ops *exynos_bus_get_ops(struct device_node *np)
>> -{
>> -       const struct of_device_id *match;
>> -
>> -       match = of_match_node(exynos_ppmu_id_match, np);
>> -       return (struct devfreq_event_ops *)match->data;
>> -}
>> -
>>   static int of_get_devfreq_events(struct device_node *np,
>>                                   struct exynos_ppmu *info)
>>   {
>>          struct devfreq_event_desc *desc;
>> -       struct devfreq_event_ops *event_ops;
>>          struct device *dev = info->dev;
>>          struct device_node *events_np, *node;
>>          int i, j, count;
>>   
>>          events_np = of_get_child_by_name(np, "events");
>>          if (!events_np) {
>> @@ -515,7 +500,6 @@ static int of_get_devfreq_events(struct device_node *np,
>>                          "failed to get child node of devfreq-event devices\n");
>>                  return -EINVAL;
>>          }
>> -       event_ops = exynos_bus_get_ops(np);
>>   
>>          count = of_get_child_count(events_np);
>>          desc = devm_kcalloc(dev, count, sizeof(*desc), GFP_KERNEL);
>> @@ -540,11 +524,38 @@ static int of_get_devfreq_events(struct device_node *np,
>>                          continue;
>>                  }
>>   
>> -               desc[j].ops = event_ops;
>> +               switch (info->ppmu_type) {
>> +               case EXYNOS_TYPE_PPMU:
>> +                       desc[j].ops = &exynos_ppmu_ops;
>> +                       break;
>> +               case EXYNOS_TYPE_PPMU_V2:
>> +                       desc[j].ops = &exynos_ppmu_v2_ops;
>> +                       break;
>> +               }
>> +
>>
>>
> 
>
Lukasz Luba May 2, 2019, 9:07 a.m. UTC | #4
On 5/2/19 3:25 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> On 19. 5. 1. 오전 6:19, Lukasz Luba wrote:
>> Hi Chanwoo,
>>
>> On 4/30/19 9:34 AM, Chanwoo Choi wrote:
>>> Hi Lukasz,
>>>
>>> On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
>>>> This patch adds posibility to choose what type of data should be counted
>>>> by the PPMU counter. Now the type comes from DT where the event has been
>>>> defined. When there is no 'event-data-type' the default value is used,
>>>> which is 'read data in bytes'.
>>>> It is needed when you want to know not only read+write data bytes but
>>>> i.e. only write data in byte, or number of read requests, etc.
>>>>
>>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>>> ---
>>>>    drivers/devfreq/event/exynos-ppmu.c | 61 +++++++++++++++++++++++++------------
>>>>    include/linux/devfreq-event.h       |  6 ++++
>>>>    2 files changed, 48 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
>>>> index c61de0b..073bf2c 100644
>>>> --- a/drivers/devfreq/event/exynos-ppmu.c
>>>> +++ b/drivers/devfreq/event/exynos-ppmu.c
>>>> @@ -154,9 +154,9 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
>>>>    	if (ret < 0)
>>>>    		return ret;
>>>>    
>>>> -	/* Set the event of Read/Write data count  */
>>>> +	/* Set the event of proper data type monitoring */
>>>>    	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id),
>>>> -				PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT);
>>>> +			   edev->desc->data_type);
>>>>    	if (ret < 0)
>>>>    		return ret;
>>>>    
>>>> @@ -368,23 +368,11 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
>>>>    	if (ret < 0)
>>>>    		return ret;
>>>>    
>>>> -	/* Set the event of Read/Write data count  */
>>>> -	switch (id) {
>>>> -	case PPMU_PMNCNT0:
>>>> -	case PPMU_PMNCNT1:
>>>> -	case PPMU_PMNCNT2:
>>>> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>>>> -				PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT);
>>>> -		if (ret < 0)
>>>> -			return ret;
>>>> -		break;
>>>> -	case PPMU_PMNCNT3:
>>>> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>>>> -				PPMU_V2_EVT3_RW_DATA_CNT);
>>>> -		if (ret < 0)
>>>> -			return ret;
>>>> -		break;
>>>> -	}
>>>> +	/* Set the event of proper data type monitoring */
>>>> +	ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>>>> +			   edev->desc->data_type);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>>    
>>>>    	/* Reset cycle counter/performance counter and enable PPMU */
>>>>    	ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
>>>> @@ -508,6 +496,7 @@ static int of_get_devfreq_events(struct device_node *np,
>>>>    	struct device *dev = info->dev;
>>>>    	struct device_node *events_np, *node;
>>>>    	int i, j, count;
>>>> +	int ret;
>>>>    
>>>>    	events_np = of_get_child_by_name(np, "events");
>>>>    	if (!events_np) {
>>>> @@ -544,6 +533,40 @@ static int of_get_devfreq_events(struct device_node *np,
>>>>    		desc[j].driver_data = info;
>>>>    
>>>>    		of_property_read_string(node, "event-name", &desc[j].name);
>>>> +		ret = of_property_read_u32(node, "event-data-type",
>>>> +					   &desc[j].data_type);
>>>> +		if (ret) {
>>>> +			/* Set the event of proper data type counting.
>>>> +			 * Check if the data type has been defined in DT,
>>>> +			 * use default if not.
>>>> +			 */
>>>> +			if (of_device_is_compatible(np,
>>>> +					"samsung,exynos-ppmu-v2")) {
>>>
>>> It is not proper to compare the compatible string again
>>> in the device driver. Instead, you can define the ppmu device type
>>> as following and then use 'struct of_device_id' in order to
>>> identify the device type.
>> I have been thinking about modifying the code in similar fashion as you
>> did. Good to see similar approach. I'll take your changes with a small
>> additional code, which sets the 'info->ppmu_type' before the for
>> loop, as an additional patch. Would it be OK if I add you as an author
>> and add Sign-off-by: Chanwoo Choi <cw00.choi@samsung.com>?
> 
> If you agree, just add the my signed-off-by on second line.
OK

Regards,
Lukasz

Patch
diff mbox series

diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
index c61de0b..073bf2c 100644
--- a/drivers/devfreq/event/exynos-ppmu.c
+++ b/drivers/devfreq/event/exynos-ppmu.c
@@ -154,9 +154,9 @@  static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
 	if (ret < 0)
 		return ret;
 
-	/* Set the event of Read/Write data count  */
+	/* Set the event of proper data type monitoring */
 	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id),
-				PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT);
+			   edev->desc->data_type);
 	if (ret < 0)
 		return ret;
 
@@ -368,23 +368,11 @@  static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
 	if (ret < 0)
 		return ret;
 
-	/* Set the event of Read/Write data count  */
-	switch (id) {
-	case PPMU_PMNCNT0:
-	case PPMU_PMNCNT1:
-	case PPMU_PMNCNT2:
-		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
-				PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT);
-		if (ret < 0)
-			return ret;
-		break;
-	case PPMU_PMNCNT3:
-		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
-				PPMU_V2_EVT3_RW_DATA_CNT);
-		if (ret < 0)
-			return ret;
-		break;
-	}
+	/* Set the event of proper data type monitoring */
+	ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
+			   edev->desc->data_type);
+	if (ret < 0)
+		return ret;
 
 	/* Reset cycle counter/performance counter and enable PPMU */
 	ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
@@ -508,6 +496,7 @@  static int of_get_devfreq_events(struct device_node *np,
 	struct device *dev = info->dev;
 	struct device_node *events_np, *node;
 	int i, j, count;
+	int ret;
 
 	events_np = of_get_child_by_name(np, "events");
 	if (!events_np) {
@@ -544,6 +533,40 @@  static int of_get_devfreq_events(struct device_node *np,
 		desc[j].driver_data = info;
 
 		of_property_read_string(node, "event-name", &desc[j].name);
+		ret = of_property_read_u32(node, "event-data-type",
+					   &desc[j].data_type);
+		if (ret) {
+			/* Set the event of proper data type counting.
+			 * Check if the data type has been defined in DT,
+			 * use default if not.
+			 */
+			if (of_device_is_compatible(np,
+					"samsung,exynos-ppmu-v2")) {
+				struct devfreq_event_dev edev;
+				int id;
+				/* Not all registers take the same value for
+				 * read+write data count.
+				 */
+				edev.desc = &desc[j];
+				id = exynos_ppmu_find_ppmu_id(&edev);
+
+				switch (id) {
+				case PPMU_PMNCNT0:
+				case PPMU_PMNCNT1:
+				case PPMU_PMNCNT2:
+					desc[j].data_type = PPMU_V2_RO_DATA_CNT
+						| PPMU_V2_WO_DATA_CNT;
+					break;
+				case PPMU_PMNCNT3:
+					desc[j].data_type =
+						PPMU_V2_EVT3_RW_DATA_CNT;
+					break;
+				}
+			} else {
+				desc[j].data_type = PPMU_RO_DATA_CNT |
+					PPMU_WO_DATA_CNT;
+			}
+		}
 
 		j++;
 	}
diff --git a/include/linux/devfreq-event.h b/include/linux/devfreq-event.h
index 4db00b0..cc160b1 100644
--- a/include/linux/devfreq-event.h
+++ b/include/linux/devfreq-event.h
@@ -81,14 +81,20 @@  struct devfreq_event_ops {
  * struct devfreq_event_desc - the descriptor of devfreq-event device
  *
  * @name	: the name of devfreq-event device.
+ * @data_type	: the data type which is going to be counted in the register.
  * @driver_data	: the private data for devfreq-event driver.
  * @ops		: the operation to control devfreq-event device.
  *
  * Each devfreq-event device is described with a this structure.
  * This structure contains the various data for devfreq-event device.
+ * The data_type describes what is going to be counted in the register.
+ * It might choose to count e.g. read requests, write data in bytes, etc.
+ * The full supported list of types is present in specyfic header in:
+ * include/dt-bindings/pmu/.
  */
 struct devfreq_event_desc {
 	const char *name;
+	u32 data_type;
 	void *driver_data;
 
 	const struct devfreq_event_ops *ops;