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

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

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.


> 
> [ 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;
>>   
>>
>