| Message ID | 20200710191122.11029-3-lukasz.luba@arm.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 4fc9a0470d2dc370289e9d883feb41e5dd2c6303 |
| Headers | show |
| Series | Exynos5422 DMC: adjust to new devfreq monitoring mechanism | expand |
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? What are the advantages over the polling mode? In what scenarios it should be used? [ If this is only for documentation purposes then it should be removed as it would stay in (easily accessible) git history anyway.. ] 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; > >
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; >> >> >
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. > >> >>[ 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; >>> >>
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; >>>> >>>
On 7/14/20 10:01 AM, 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. > > >> >> [ 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 >> Bartek, do you have some objections to the patches or you think they can be taken via devfreq-next? Cheers, Lukasz
On 7/17/20 1:53 PM, Lukasz Luba wrote: > > > On 7/14/20 10:01 AM, 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. >> >> >>> >>> [ 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 >>> > > Bartek, do you have some objections to the patches or you think > they can be taken via devfreq-next? No objections from me, thank you for the IRQ mode explanation. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On Fri, Jul 10, 2020 at 08:11:22PM +0100, 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). > > 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(-) Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof
Hi Lukasz, On 10.07.2020 21:11, 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). > > Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com> > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> I've got back from my holidays and noticed that in meantime this commit got merged as commit 4fc9a0470d2d. It revealed that there is a race between registering exynos5422-dmc driver and exynos-ppmu driver, which can be observed sometimes as the following message and freeze on Odroid XU3 with multi_v7_defconfig: [ 8.767708] exynos5-dmc 10c20000.memory-controller: couldn't probe performance counters I will check this later why the EPROBE_DEFER error is not properly propagated and why it causes a freeze. Best regards
Hi All, On 03.08.2020 17:30, Marek Szyprowski wrote: > On 10.07.2020 21:11, 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). >> >> Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > > I've got back from my holidays and noticed that in meantime this > commit got merged as commit 4fc9a0470d2d. It revealed that there is a > race between registering exynos5422-dmc driver and exynos-ppmu driver, > which can be observed sometimes as the following message and freeze on > Odroid XU3 with multi_v7_defconfig: > > [ 8.767708] exynos5-dmc 10c20000.memory-controller: couldn't probe > performance counters > > I will check this later why the EPROBE_DEFER error is not properly > propagated and why it causes a freeze. It looks that simply propagating return value from exynos5_counters_get() in exynos5_dmc_get_status() fixes the boot: # dmesg | grep dmc [ 8.838754] exynos-ppmu: new PPMU device registered 10d00000.ppmu (ppmu-event3-dmc0_0) [ 8.861344] exynos-ppmu: new PPMU device registered 10d10000.ppmu (ppmu-event3-dmc0_1) [ 8.868488] exynos5-dmc 10c20000.memory-controller: couldn't probe performance counters [ 8.874417] exynos-ppmu: new PPMU device registered 10d60000.ppmu (ppmu-event3-dmc1_0) [ 8.886612] exynos-ppmu: new PPMU device registered 10d70000.ppmu (ppmu-event3-dmc1_1) [ 9.396769] exynos5-dmc 10c20000.memory-controller: DMC initialized, in irq mode: 0 I'm still curious why it freezes if getting performance counters is not possible. Best regards
Hi Marek, On 8/3/20 4:35 PM, Marek Szyprowski wrote: > Hi All, > > On 03.08.2020 17:30, Marek Szyprowski wrote: >> On 10.07.2020 21:11, 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). >>> >>> Reported-by: Willy Wolff <willy.mh.wolff.ml@gmail.com> >>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >> >> I've got back from my holidays and noticed that in meantime this >> commit got merged as commit 4fc9a0470d2d. It revealed that there is a >> race between registering exynos5422-dmc driver and exynos-ppmu driver, >> which can be observed sometimes as the following message and freeze on >> Odroid XU3 with multi_v7_defconfig: >> >> [ 8.767708] exynos5-dmc 10c20000.memory-controller: couldn't probe >> performance counters >> >> I will check this later why the EPROBE_DEFER error is not properly >> propagated and why it causes a freeze. > > It looks that simply propagating return value from > exynos5_counters_get() in exynos5_dmc_get_status() fixes the boot: > > # dmesg | grep dmc > [ 8.838754] exynos-ppmu: new PPMU device registered 10d00000.ppmu > (ppmu-event3-dmc0_0) > [ 8.861344] exynos-ppmu: new PPMU device registered 10d10000.ppmu > (ppmu-event3-dmc0_1) > [ 8.868488] exynos5-dmc 10c20000.memory-controller: couldn't probe > performance counters > [ 8.874417] exynos-ppmu: new PPMU device registered 10d60000.ppmu > (ppmu-event3-dmc1_0) > [ 8.886612] exynos-ppmu: new PPMU device registered 10d70000.ppmu > (ppmu-event3-dmc1_1) > [ 9.396769] exynos5-dmc 10c20000.memory-controller: DMC initialized, > in irq mode: 0 > > I'm still curious why it freezes if getting performance counters is not > possible. > > Best regards > Thank you for investigating this issue. Indeed, it's odd why it freezes. I've seen you patch with the fix. Regards, Lukasz
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;
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). 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(-)