subject:"\[PATCH v4 07\/24\] PM \/ devfreq\: tegra30\: Use CPUFreq notifier"

Re: [PATCH v4 07/24] PM / devfreq: tegra30: Use CPUFreq notifier

2019-07-16 Thread Dmitry Osipenko

16.07.2019 15:08, Chanwoo Choi пишет:
> On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
>> The CPU's client need to take into account that CPUFreq may change
>> while memory activity not, staying high. Thus an appropriate frequency
>> notifier should be used in addition to the clk-notifier.
>>
>> Signed-off-by: Dmitry Osipenko 
>> ---
>>  drivers/devfreq/tegra30-devfreq.c | 105 +-
>>  1 file changed, 88 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c 
>> b/drivers/devfreq/tegra30-devfreq.c
>> index 2bf65409ddd8..48a799fa5f63 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -17,6 +17,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #include "governor.h"
>>  
>> @@ -154,7 +155,10 @@ struct tegra_devfreq {
>>  
>>  struct clk  *emc_clock;
>>  unsigned long   max_freq;
>> -struct notifier_block   rate_change_nb;
>> +struct notifier_block   clk_rate_change_nb;
>> +
>> +struct work_struct  update_work;
> 
> nitpick.
> I think 'update_work' is not clear to indicate the work
> for changing the clock according to the cpu clock change.
> You better to change the name for more clearly.

Okay, I'll rename it to 'cpufreq_update_work' or something like that.

>> +struct notifier_block   cpu_rate_change_nb;
>>  
>>  struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
>>  
>> @@ -456,8 +460,8 @@ static irqreturn_t actmon_thread_isr(int irq, void *data)
>>  return handled ? IRQ_HANDLED : IRQ_NONE;
>>  }
>>  
>> -static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
>> -   unsigned long action, void *ptr)
>> +static int tegra_actmon_clk_notify_cb(struct notifier_block *nb,
>> +  unsigned long action, void *ptr)
>>  {
>>  struct clk_notifier_data *data = ptr;
>>  struct tegra_devfreq_device *dev;
>> @@ -467,7 +471,7 @@ static int tegra_actmon_rate_notify_cb(struct 
>> notifier_block *nb,
>>  if (action != POST_RATE_CHANGE)
>>  return NOTIFY_OK;
>>  
>> -tegra = container_of(nb, struct tegra_devfreq, rate_change_nb);
>> +tegra = container_of(nb, struct tegra_devfreq, clk_rate_change_nb);
>>  
>>  /*
>>   * EMC rate could change due to three reasons:
>> @@ -496,6 +500,37 @@ static int tegra_actmon_rate_notify_cb(struct 
>> notifier_block *nb,
>>  return NOTIFY_OK;
>>  }
>>  
>> +static void tegra_actmon_delayed_update(struct work_struct *work)
>> +{
>> +struct tegra_devfreq *tegra = container_of(work, struct tegra_devfreq,
>> +   update_work);
>> +
>> +mutex_lock(&tegra->devfreq->lock);
>> +update_devfreq(tegra->devfreq);
>> +mutex_unlock(&tegra->devfreq->lock);
>> +}
>> +
>> +static int tegra_actmon_cpu_notify_cb(struct notifier_block *nb,
>> +  unsigned long action, void *ptr)
>> +{
>> +struct tegra_devfreq *tegra;
>> +
>> +if (action != CPUFREQ_POSTCHANGE)
>> +return NOTIFY_OK;
>> +
>> +tegra = container_of(nb, struct tegra_devfreq, cpu_rate_change_nb);
> 
> nitpick. Better to check whether 'tegra' is NULL or not.

It can't be NULL, unless 'nb' is NULL and even in that case
container_of() will cause kernel crash. Hence it's absolutely
unnecessary to check for NULL here.

>> +
>> +/*
>> + * CPUFreq driver should support CPUFREQ_ASYNC_NOTIFICATION in order
>> + * to allow asynchronous notifications. This means we can't block
>> + * here for too long, otherwise CPUFreq's core will complain with a
>> + * warning splat.
>> + */
>> +schedule_work(&tegra->update_work);
>> +
>> +return NOTIFY_OK;
>> +}
>> +
>>  static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
>>struct tegra_devfreq_device *dev)
>>  {
>> @@ -527,9 +562,16 @@ static void tegra_actmon_configure_device(struct 
>> tegra_devfreq *tegra,
>>  device_writel(dev, val, ACTMON_DEV_CTRL);
>>  }
>>  
>> -static void tegra_actmon_start(struct tegra_devfreq *tegra)
>> +static void tegra_actmon_stop_device(struct tegra_devfreq_device *dev)
>> +{
>> +device_writel(dev, 0x, ACTMON_DEV_CTRL);
> 
> Better to define the constant definition of 0x
> in order to explain the correct meaning.
> 
> For example,
> #define ACTMON_DEV_RESET  0x

Okay.

Re: [PATCH v4 07/24] PM / devfreq: tegra30: Use CPUFreq notifier

2019-07-16 Thread Chanwoo Choi

On 19. 7. 8. 오전 7:32, Dmitry Osipenko wrote:
> The CPU's client need to take into account that CPUFreq may change
> while memory activity not, staying high. Thus an appropriate frequency
> notifier should be used in addition to the clk-notifier.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/devfreq/tegra30-devfreq.c | 105 +-
>  1 file changed, 88 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c 
> b/drivers/devfreq/tegra30-devfreq.c
> index 2bf65409ddd8..48a799fa5f63 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "governor.h"
>  
> @@ -154,7 +155,10 @@ struct tegra_devfreq {
>  
>   struct clk  *emc_clock;
>   unsigned long   max_freq;
> - struct notifier_block   rate_change_nb;
> + struct notifier_block   clk_rate_change_nb;
> +
> + struct work_struct  update_work;

nitpick.
I think 'update_work' is not clear to indicate the work
for changing the clock according to the cpu clock change.
You better to change the name for more clearly.

> + struct notifier_block   cpu_rate_change_nb;
>  
>   struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
>  
> @@ -456,8 +460,8 @@ static irqreturn_t actmon_thread_isr(int irq, void *data)
>   return handled ? IRQ_HANDLED : IRQ_NONE;
>  }
>  
> -static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
> -unsigned long action, void *ptr)
> +static int tegra_actmon_clk_notify_cb(struct notifier_block *nb,
> +   unsigned long action, void *ptr)
>  {
>   struct clk_notifier_data *data = ptr;
>   struct tegra_devfreq_device *dev;
> @@ -467,7 +471,7 @@ static int tegra_actmon_rate_notify_cb(struct 
> notifier_block *nb,
>   if (action != POST_RATE_CHANGE)
>   return NOTIFY_OK;
>  
> - tegra = container_of(nb, struct tegra_devfreq, rate_change_nb);
> + tegra = container_of(nb, struct tegra_devfreq, clk_rate_change_nb);
>  
>   /*
>* EMC rate could change due to three reasons:
> @@ -496,6 +500,37 @@ static int tegra_actmon_rate_notify_cb(struct 
> notifier_block *nb,
>   return NOTIFY_OK;
>  }
>  
> +static void tegra_actmon_delayed_update(struct work_struct *work)
> +{
> + struct tegra_devfreq *tegra = container_of(work, struct tegra_devfreq,
> +update_work);
> +
> + mutex_lock(&tegra->devfreq->lock);
> + update_devfreq(tegra->devfreq);
> + mutex_unlock(&tegra->devfreq->lock);
> +}
> +
> +static int tegra_actmon_cpu_notify_cb(struct notifier_block *nb,
> +   unsigned long action, void *ptr)
> +{
> + struct tegra_devfreq *tegra;
> +
> + if (action != CPUFREQ_POSTCHANGE)
> + return NOTIFY_OK;
> +
> + tegra = container_of(nb, struct tegra_devfreq, cpu_rate_change_nb);

nitpick. Better to check whether 'tegra' is NULL or not.

> +
> + /*
> +  * CPUFreq driver should support CPUFREQ_ASYNC_NOTIFICATION in order
> +  * to allow asynchronous notifications. This means we can't block
> +  * here for too long, otherwise CPUFreq's core will complain with a
> +  * warning splat.
> +  */
> + schedule_work(&tegra->update_work);
> +
> + return NOTIFY_OK;
> +}
> +
>  static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
> struct tegra_devfreq_device *dev)
>  {
> @@ -527,9 +562,16 @@ static void tegra_actmon_configure_device(struct 
> tegra_devfreq *tegra,
>   device_writel(dev, val, ACTMON_DEV_CTRL);
>  }
>  
> -static void tegra_actmon_start(struct tegra_devfreq *tegra)
> +static void tegra_actmon_stop_device(struct tegra_devfreq_device *dev)
> +{
> + device_writel(dev, 0x, ACTMON_DEV_CTRL);

Better to define the constant definition of 0x
in order to explain the correct meaning.

For example,
#define ACTMON_DEV_RESET0x

> + device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
> +}
> +
> +static int tegra_actmon_start(struct tegra_devfreq *tegra)
>  {
>   unsigned int i;
> + int err;
>  
>   actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
> ACTMON_GLB_PERIOD_CTRL);
> @@ -537,7 +579,30 @@ static void tegra_actmon_start(struct tegra_devfreq 
> *tegra)
>   for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
>   tegra_actmon_configure_device(tegra, &tegra->devices[i]);
>  
> + /*
> +  * We are estimating CPU's memory bandwidth requirement based on
> +  * amount of memory accesses and system's load, judging by CPU's
> +  * frequency. We also don't want to receive events about CPU's
> +  * frequency transaction when governor is stopped, hence notifier
> +  * is registered

[PATCH v4 07/24] PM / devfreq: tegra30: Use CPUFreq notifier

2019-07-07 Thread Dmitry Osipenko

The CPU's client need to take into account that CPUFreq may change
while memory activity not, staying high. Thus an appropriate frequency
notifier should be used in addition to the clk-notifier.

Signed-off-by: Dmitry Osipenko 
---
 drivers/devfreq/tegra30-devfreq.c | 105 +-
 1 file changed, 88 insertions(+), 17 deletions(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c 
b/drivers/devfreq/tegra30-devfreq.c
index 2bf65409ddd8..48a799fa5f63 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -17,6 +17,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "governor.h"
 
@@ -154,7 +155,10 @@ struct tegra_devfreq {
 
struct clk  *emc_clock;
unsigned long   max_freq;
-   struct notifier_block   rate_change_nb;
+   struct notifier_block   clk_rate_change_nb;
+
+   struct work_struct  update_work;
+   struct notifier_block   cpu_rate_change_nb;
 
struct tegra_devfreq_device devices[ARRAY_SIZE(actmon_device_configs)];
 
@@ -456,8 +460,8 @@ static irqreturn_t actmon_thread_isr(int irq, void *data)
return handled ? IRQ_HANDLED : IRQ_NONE;
 }
 
-static int tegra_actmon_rate_notify_cb(struct notifier_block *nb,
-  unsigned long action, void *ptr)
+static int tegra_actmon_clk_notify_cb(struct notifier_block *nb,
+ unsigned long action, void *ptr)
 {
struct clk_notifier_data *data = ptr;
struct tegra_devfreq_device *dev;
@@ -467,7 +471,7 @@ static int tegra_actmon_rate_notify_cb(struct 
notifier_block *nb,
if (action != POST_RATE_CHANGE)
return NOTIFY_OK;
 
-   tegra = container_of(nb, struct tegra_devfreq, rate_change_nb);
+   tegra = container_of(nb, struct tegra_devfreq, clk_rate_change_nb);
 
/*
 * EMC rate could change due to three reasons:
@@ -496,6 +500,37 @@ static int tegra_actmon_rate_notify_cb(struct 
notifier_block *nb,
return NOTIFY_OK;
 }
 
+static void tegra_actmon_delayed_update(struct work_struct *work)
+{
+   struct tegra_devfreq *tegra = container_of(work, struct tegra_devfreq,
+  update_work);
+
+   mutex_lock(&tegra->devfreq->lock);
+   update_devfreq(tegra->devfreq);
+   mutex_unlock(&tegra->devfreq->lock);
+}
+
+static int tegra_actmon_cpu_notify_cb(struct notifier_block *nb,
+ unsigned long action, void *ptr)
+{
+   struct tegra_devfreq *tegra;
+
+   if (action != CPUFREQ_POSTCHANGE)
+   return NOTIFY_OK;
+
+   tegra = container_of(nb, struct tegra_devfreq, cpu_rate_change_nb);
+
+   /*
+* CPUFreq driver should support CPUFREQ_ASYNC_NOTIFICATION in order
+* to allow asynchronous notifications. This means we can't block
+* here for too long, otherwise CPUFreq's core will complain with a
+* warning splat.
+*/
+   schedule_work(&tegra->update_work);
+
+   return NOTIFY_OK;
+}
+
 static void tegra_actmon_configure_device(struct tegra_devfreq *tegra,
  struct tegra_devfreq_device *dev)
 {
@@ -527,9 +562,16 @@ static void tegra_actmon_configure_device(struct 
tegra_devfreq *tegra,
device_writel(dev, val, ACTMON_DEV_CTRL);
 }
 
-static void tegra_actmon_start(struct tegra_devfreq *tegra)
+static void tegra_actmon_stop_device(struct tegra_devfreq_device *dev)
+{
+   device_writel(dev, 0x, ACTMON_DEV_CTRL);
+   device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
+}
+
+static int tegra_actmon_start(struct tegra_devfreq *tegra)
 {
unsigned int i;
+   int err;
 
actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
  ACTMON_GLB_PERIOD_CTRL);
@@ -537,7 +579,30 @@ static void tegra_actmon_start(struct tegra_devfreq *tegra)
for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
tegra_actmon_configure_device(tegra, &tegra->devices[i]);
 
+   /*
+* We are estimating CPU's memory bandwidth requirement based on
+* amount of memory accesses and system's load, judging by CPU's
+* frequency. We also don't want to receive events about CPU's
+* frequency transaction when governor is stopped, hence notifier
+* is registered dynamically.
+*/
+   err = cpufreq_register_notifier(&tegra->cpu_rate_change_nb,
+   CPUFREQ_TRANSITION_NOTIFIER);
+   if (err) {
+   dev_err(tegra->devfreq->dev.parent,
+   "Failed to register rate change notifier: %d\n", err);
+   goto err_stop;
+   }
+
enable_irq(tegra->irq);
+
+   return 0;
+
+err_stop:
+   for (i = 0; i < ARRAY_SIZE(tegra->devices); i++)
+   tegra_actmon_stop_device(&tegra->devices[i]);
+
+   return err;
 }
 
 static

Re: [PATCH v4 07/24] PM / devfreq: tegra30: Use CPUFreq notifier

Re: [PATCH v4 07/24] PM / devfreq: tegra30: Use CPUFreq notifier

[PATCH v4 07/24] PM / devfreq: tegra30: Use CPUFreq notifier

3 matches

Site Navigation

Mail list logo

Footer information