Re: [PATCH v2 2/2] memory: samsung: exynos5422-dmc: Add module param to control IRQ mode

[Lukasz Luba <lukasz.luba@arm.com>]

Hi Willy,

On 7/14/20 10:32 AM, Willy Wolff wrote:
> Hi Lukasz and Bartek,
> 
> On 2020-07-14-10-01-16, Lukasz Luba wrote:
>> Hi Bartek,
>>
>> On 7/14/20 8:42 AM, Bartlomiej Zolnierkiewicz wrote:
>>>
>>> Hi,
>>>
>>> On 7/10/20 9:11 PM, Lukasz Luba wrote:
>>>> The driver can operate in two modes relaying on devfreq monitoring
>>>> mechanism which periodically checks the device status or it can use
>>>> interrupts when they are provided by loaded Device Tree. The newly
>>>> introduced module parameter can be used to choose between devfreq
>>>> monitoring and internal interrupts without modifying the Device Tree.
>>>> It also sets devfreq monitoring as default when the parameter is not 
>>>> set
>>>> (also the case for default when the driver is not built as a module).
>>>
>>> Could you please explain why should we leave the IRQ mode
>>> support in the dmc driver?
>>
>> I am still experimenting with the IRQ mode in DMC, but have limited time
>> for it and no TRM.
>> The IRQ mode in memory controller or bus controller has one major
>> advantage: is more interactive. In polling we have fixed period, i.e.
>> 100ms - that's a lot when we have a sudden, latency sensitive workload.
>> There might be no check of the device load for i.e. 99ms, but the tasks
>> with such workload started running. That's a long period of a few frames
>> which are likely to be junked. Should we adjust polling interval to i.e.
>> 10ms, I don't think so. There is no easy way to address all of the
>> scenarios.
>>
>>>
>>> What are the advantages over the polling mode?
>>
>> As described above: more reactive to sudden workload, which might be
>> latency sensitive and cause junk frames.
>> Drawback: not best in benchmarks which are randomly jumping
>> over the data set, causing low traffic on memory.
>> It could be mitigated as Sylwester described with not only one type
>> of interrupt, but another, which could 'observe' also other information
>> type in the counters and fire.
>>
>>>
>>> In what scenarios it should be used?
>>
>> System like Android with GUI, when there is this sudden workload
>> quite often.
>>
>> I think the interconnect could help here and would adjust the DMC
>> freq upfront. Although I don't know if interconnect on Exynos5422 is in
>> your scope in near future. Of course the interconnect will not cover
>> all scenarios either.
>>
> 
> The interconnect (CCI-400) will not help much, you still have the same 
> problem
> of setting interrupts at the right threshold, or to poll it to see any 
> activity
> through it.

I was referring to 'interconnect' framework, the work Artur and now
Sylwester is doing [1]. Together with devfreq passive governors, proper
description of device dependencies and required bandwidth, should be
able to address the typical scenarios in the system.

My bad, I haven't explained which interconnect I have in mind.
I agree with you that HW interconnect won't solve this.

Regards,
Lukasz

[1] https://lkml.org/lkml/2020/7/2/861

> 
>>
>>>
>>> [ If this is only for documentation purposes then it should be
>>>   removed as it would stay in (easily accessible) git history
>>>   anyway.. ]
>>
>> The current interrupt mode is definitely not perfect and switching
>> to devfreq monitoring mode has more sense. On the other hand, it
>> still has potential, until there is no interconnect for this SoC.
>> I will continue experimenting with irq mode, so I would like to
>> still have the code in the driver.
>>
>> Regards,
>> Lukasz
>>
>>>
>>> Best regards,
>>> -- 
>>> Bartlomiej Zolnierkiewicz
>>> Samsung R&D Institute Poland
>>> Samsung Electronics
>>>
>>>> Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com>
>>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>>> ---
>>>>  drivers/memory/samsung/exynos5422-dmc.c | 9 +++++++--
>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/memory/samsung/exynos5422-dmc.c 
>>>> b/drivers/memory/samsung/exynos5422-dmc.c
>>>> index e03ee35f0ab5..53bfe6b7b703 100644
>>>> --- a/drivers/memory/samsung/exynos5422-dmc.c
>>>> +++ b/drivers/memory/samsung/exynos5422-dmc.c
>>>> @@ -12,6 +12,7 @@
>>>>  #include <linux/io.h>
>>>>  #include <linux/mfd/syscon.h>
>>>>  #include <linux/module.h>
>>>> +#include <linux/moduleparam.h>
>>>>  #include <linux/of_device.h>
>>>>  #include <linux/pm_opp.h>
>>>>  #include <linux/platform_device.h>
>>>> @@ -21,6 +22,10 @@
>>>>  #include "../jedec_ddr.h"
>>>>  #include "../of_memory.h"
>>>> +static int irqmode;
>>>> +module_param(irqmode, int, 0644);
>>>> +MODULE_PARM_DESC(irqmode, "Enable IRQ mode (0=off [default], 1=on)");
>>>> +
>>>>  #define EXYNOS5_DREXI_TIMINGAREF        (0x0030)
>>>>  #define EXYNOS5_DREXI_TIMINGROW0        (0x0034)
>>>>  #define EXYNOS5_DREXI_TIMINGDATA0        (0x0038)
>>>> @@ -1428,7 +1433,7 @@ static int exynos5_dmc_probe(struct 
>>>> platform_device *pdev)
>>>>      /* There is two modes in which the driver works: polling or IRQ */
>>>>      irq[0] = platform_get_irq_byname(pdev, "drex_0");
>>>>      irq[1] = platform_get_irq_byname(pdev, "drex_1");
>>>> -    if (irq[0] > 0 && irq[1] > 0) {
>>>> +    if (irq[0] > 0 && irq[1] > 0 && irqmode) {
>>>>          ret = devm_request_threaded_irq(dev, irq[0], NULL,
>>>>                          dmc_irq_thread, IRQF_ONESHOT,
>>>>                          dev_name(dev), dmc);
>>>> @@ -1485,7 +1490,7 @@ static int exynos5_dmc_probe(struct 
>>>> platform_device *pdev)
>>>>      if (dmc->in_irq_mode)
>>>>          exynos5_dmc_start_perf_events(dmc, PERF_COUNTER_START_VALUE);
>>>> -    dev_info(dev, "DMC initialized\n");
>>>> +    dev_info(dev, "DMC initialized, in irq mode: %d\n", 
>>>> dmc->in_irq_mode);
>>>>      return 0;
>>>>
>>>