Hi,
On 19. 4. 17. 오전 12:23, Dmitry Osipenko wrote:
> 16.04.2019 8:56, Chanwoo Choi пишет:
>> Hi,
>>
>> It looks good to me to drop the primary interrupt handler
>> but I have some comments. Please check it.
>>
>> On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
>>> There is no real need in the primary interrupt handler, hence move
>>> everything to the secondary (threaded) handler. In a result locking
>>> is consistent now and there are no potential races with the interrupt
>>> handler because it is protected with the devfreq's mutex.
>>>
>>> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
>>> ---
>>> drivers/devfreq/tegra-devfreq.c | 61 ++++++++++++---------------------
>>> 1 file changed, 22 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/tegra-devfreq.c
>>> b/drivers/devfreq/tegra-devfreq.c
>>> index 2a1464098200..69b557df5084 100644
>>> --- a/drivers/devfreq/tegra-devfreq.c
>>> +++ b/drivers/devfreq/tegra-devfreq.c
>>> @@ -144,7 +144,6 @@ static struct tegra_devfreq_device_config
>>> actmon_device_configs[] = {
>>> struct tegra_devfreq_device {
>>> const struct tegra_devfreq_device_config *config;
>>> void __iomem *regs;
>>> - spinlock_t lock;
>>>
>>> /* Average event count sampled in the last interrupt */
>>> u32 avg_count;
>>> @@ -249,11 +248,8 @@ static void actmon_write_barrier(struct tegra_devfreq
>>> *tegra)
>>> static void actmon_isr_device(struct tegra_devfreq *tegra,
>>> struct tegra_devfreq_device *dev)
>>> {
>>> - unsigned long flags;
>>> u32 intr_status, dev_ctrl;
>>>
>>> - spin_lock_irqsave(&dev->lock, flags);
>>> -
>>> dev->avg_count = device_readl(dev, ACTMON_DEV_AVG_COUNT);
>>> tegra_devfreq_update_avg_wmark(tegra, dev);
>>>
>>> @@ -302,26 +298,6 @@ static void actmon_isr_device(struct tegra_devfreq
>>> *tegra,
>>> device_writel(dev, ACTMON_INTR_STATUS_CLEAR, ACTMON_DEV_INTR_STATUS);
>>>
>>> actmon_write_barrier(tegra);
>>> -
>>> - spin_unlock_irqrestore(&dev->lock, flags);
>>> -}
>>> -
>>> -static irqreturn_t actmon_isr(int irq, void *data)
>>> -{
>>> - struct tegra_devfreq *tegra = data;
>>> - bool handled = false;
>>> - unsigned int i;
>>> - u32 val;
>>> -
>>> - val = actmon_readl(tegra, ACTMON_GLB_STATUS);
>>> - for (i = 0; i < ARRAY_SIZE(tegra->devices); i++) {
>>> - if (val & tegra->devices[i].config->irq_mask) {
>>> - actmon_isr_device(tegra, tegra->devices + i);
>>> - handled = true;
>>> - }
>>> - }
>>> -
>>> - return handled ? IRQ_WAKE_THREAD : IRQ_NONE;
>>> }
>>>
>>> static unsigned long actmon_cpu_to_emc_rate(struct tegra_devfreq *tegra,
>>> @@ -348,35 +324,46 @@ static void actmon_update_target(struct tegra_devfreq
>>> *tegra,
>>> unsigned long cpu_freq = 0;
>>> unsigned long static_cpu_emc_freq = 0;
>>> unsigned int avg_sustain_coef;
>>> - unsigned long flags;
>>> + u32 avg_count;
>>>
>>> if (dev->config->avg_dependency_threshold) {
>>> cpu_freq = cpufreq_get(0);
>>> static_cpu_emc_freq = actmon_cpu_to_emc_rate(tegra, cpu_freq);
>>> }
>>>
>>> - spin_lock_irqsave(&dev->lock, flags);
>>> -
>>> - dev->target_freq = dev->avg_count / ACTMON_SAMPLING_PERIOD;
>>> + avg_count = dev->avg_count;
>>> + dev->target_freq = avg_count / ACTMON_SAMPLING_PERIOD;
>>
>> Actually, this change is not related to this patch.
>> Please keep the original code.
>>
>>> avg_sustain_coef = 100 * 100 / dev->config->boost_up_threshold;
>>> dev->target_freq = do_percent(dev->target_freq, avg_sustain_coef);
>>> dev->target_freq += dev->boost_freq;
>>>
>>> - if (dev->avg_count >= dev->config->avg_dependency_threshold)
>>> + if (avg_count >= dev->config->avg_dependency_threshold)
>>
>> ditto.
>
> Good catch, it's a leftover from v1 that I forgot to revert. Thank you.
>
> [snip]
>
>>
>> When I review this patch, I have a question
>> about why tegra_actmon_rate_notify_cb is needed.
>>
>> tegra_actmon_rate_notify_cb() do something
>> when the clock rate of emc_clock is changed.
>> I think that 'emc_clock' is changed by this driver.
>> It means that the this driver can catch the change timing
>> of emc_clock rate without notifier.
>
> The devfreq driver isn't the only driver of the EMC clock rate. The devfreq
> driver changes EMC freq dynamically based of on average memory usage
> activity, but for some hardware units (like display controller for example)
> there is a requirement for a minimum memory bandwidth (isochronous
> transactions) and hence when display is waking up from suspend it immediately
> requires an amount of memory bandwidth that could be higher than ACTMON
> hardware unit suggests and besides the ACTMON's reaction is delayed by the
> sampling period (12ms).
Thanks for explaining it. If EMC clock is used on multiple point,
I understand why the clock notifier is necessary.
>
>> IMO, it is possible to call tegra_devfreq_update_wmark()
>> directly without clock notifier before calling
>> 'clk_set_min_rate()/clk_set_rate()' in the tegra_devfreq_target().
>>
>> With clock notifier, it cannot restore something for
>> tegra_devfreq_update_wmark(tegra, dev) when failed to
>> set the rate of emc_clk by 'clk_set_min_rate()/clk_set_rate()'.
>
> The watermarks should be changed in accordance to the actual EMC clock rate.
> Given that EMC rate could be changed by something else than the devfreq
> driver, we need to re-adjust the watermarks to the actual values after the
> EMC rate-change completion. Please note that the clock notifier uses the
> POST_RATE_CHANGE event that happens only after successful completion of EMC
> clock rate change.
I understand the needs of clock notifier. Thanks.
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
