Re: [PATCH v6 00/19] More improvements for Tegra30 devfreq driver

[Peter Geis]

Tested on the Ouya (tegra30).

Tested-by: Peter Geis <pgwipeout@xxxxxxxxx>

On Wed, Oct 2, 2019 at 9:56 AM Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>
> 02.10.2019 03:25, Chanwoo Choi ÐÐÑÐÑ:
> > Hello Dmitry and Thierry,
> >
> > On 19. 10. 2. ìì 6:15, Dmitry Osipenko wrote:
> >> 12.08.2019 00:22, Dmitry Osipenko ÐÐÑÐÑ:
> >>> Hello,
> >>>
> >>> This series addresses some additional review comments that were made by
> >>> Thierry Reding to [1], makes several important changes to the driver,
> >>> fixing excessive interrupts activity, and adds new features. In the end
> >>> I'm proposing myself as a maintainer for the Tegra devfreq drivers.
> >>>
> >>> [1] https://lore.kernel.org/lkml/0fb50eb1-a173-1756-6889-2526a10ac707@xxxxxxxxx/T/
> >>>
> >>> Changelog:
> >>>
> >>> v6:  Addressed review comment that was made by Chanwoo Choi to v5 by
> >>>      squashing "Define ACTMON_DEV_CTRL_STOP" patch into the "Use CPUFreq
> >>>      notifier" patch.
> >>>
> >>> v5:  Addressed review comments that were made by Chanwoo Choi to v4 by
> >>>      squashing few patches, dropping some questionable patches, rewording
> >>>      comments to the code, restructuring the code and etc.
> >>>
> >>>      These patches are now dropped from the series:
> >>>
> >>>        PM / devfreq: tegra30: Use tracepoints for debugging
> >>>        PM / devfreq: tegra30: Inline all one-line functions
> >>>
> >>>      The interrupt-optimization patches are squashed into a single patch:
> >>>
> >>>        PM / devfreq: tegra30: Reduce unnecessary interrupts activity
> >>>
> >>>      because it's better to keep the optimizations as a separate change and
> >>>      this also helps to reduce code churning, since the code changes depend
> >>>      on a previous patch in order to stay cleaner.
> >>>
> >>>      Fixed a lockup bug that I spotted recently, which is caused by a
> >>>      clk-notifier->cpufreq_get()->clk_set_rate() sequence. Now a non-blocking
> >>>      variant of CPU's frequency retrieving is used, i.e. cpufreq_quick_get().
> >>>
> >>>      Further optimized the CPUFreq notifier by postponing the delayed
> >>>      updating in accordance to the polling interval, this actually uncovered
> >>>      the above lockup bug.
> >>>
> >>>      Implemented new minor driver feature in the new patch:
> >>>
> >>>        PM / devfreq: tegra30: Support variable polling interval
> >>>
> >>> v4:  Added two new patches to the series:
> >>>
> >>>        PM / devfreq: tegra30: Synchronize average count on target's update
> >>>        PM / devfreq: tegra30: Increase sampling period to 16ms
> >>>
> >>>      The first patch addresses problem where governor could get stuck due
> >>>      to outdated "average count" value which is snapshoted by ISR and there
> >>>      are cases where manual update of the value is required.
> >>>
> >>>      The second patch is just a minor optimization.
> >>>
> >>> v3:  Added support for tracepoints, replacing the debug messages.
> >>>      Fixed few more bugs with the help of tracepoints.
> >>>
> >>>      New patches in this version:
> >>>
> >>>        PM / devfreq: tegra30: Use tracepoints for debugging
> >>>        PM / devfreq: tegra30: Optimize CPUFreq notifier
> >>>        PM / devfreq: tegra30: Optimize upper consecutive watermark selection
> >>>        PM / devfreq: tegra30: Optimize upper average watermark selection
> >>>        PM / devfreq: tegra30: Include appropriate header
> >>>
> >>>      Some of older patches of this series also got some extra minor polish.
> >>>
> >>> v2:  Added more patches that are cleaning driver's code further and
> >>>      squashing another kHz conversion bug.
> >>>
> >>>      The patch "Rework frequency management logic" of the v1 series is now
> >>>      converted to "Set up watermarks properly" because I found some problems
> >>>      in the original patch and then realized that there is no need to change
> >>>      the logic much. So the logic mostly preserved and only got improvements.
> >>>
> >>>      The series is based on the today's linux-next (25 Jun) and takes into
> >>>      account minor changes that MyungJoo Ham made to the already queued
> >>>      patches from the first batch [1].
> >>>
> >>> Dmitry Osipenko (19):
> >>>   PM / devfreq: tegra30: Change irq type to unsigned int
> >>>   PM / devfreq: tegra30: Keep interrupt disabled while governor is
> >>>     stopped
> >>>   PM / devfreq: tegra30: Handle possible round-rate error
> >>>   PM / devfreq: tegra30: Drop write-barrier
> >>>   PM / devfreq: tegra30: Set up watermarks properly
> >>>   PM / devfreq: tegra30: Tune up boosting thresholds
> >>>   PM / devfreq: tegra30: Fix integer overflow on CPU's freq max out
> >>>   PM / devfreq: tegra30: Ensure that target freq won't overflow
> >>>   PM / devfreq: tegra30: Use kHz units uniformly in the code
> >>>   PM / devfreq: tegra30: Reduce unnecessary interrupts activity
> >>>   PM / devfreq: tegra30: Use CPUFreq notifier
> >>>   PM / devfreq: tegra30: Move clk-notifier's registration to governor's
> >>>     start
> >>>   PM / devfreq: tegra30: Reset boosting on startup
> >>>   PM / devfreq: tegra30: Don't enable consecutive-down interrupt on
> >>>     startup
> >>>   PM / devfreq: tegra30: Constify structs
> >>>   PM / devfreq: tegra30: Include appropriate header
> >>>   PM / devfreq: tegra30: Increase sampling period to 16ms
> >>>   PM / devfreq: tegra30: Support variable polling interval
> >>>   PM / devfreq: tegra20/30: Add Dmitry as a maintainer
> >>>
> >>>  MAINTAINERS                       |   9 +
> >>>  drivers/devfreq/tegra30-devfreq.c | 706 +++++++++++++++++++++++-------
> >>>  2 files changed, 555 insertions(+), 160 deletions(-)
> >>>
> >>
> >> Hello Chanwoo,
> >>
> >> I don't have any more updates in regards to this series, everything is
> >> working flawlessly for now. Will be awesome if we could continue the
> >> reviewing and then get the patches into linux-next to get some more testing.
> >>
> >>
> >
> > Hello Dmitry,
> >
> > I'm sorry for late reply. Except for patch5, I reviewed the patches.
> > Please check my comment. Actually, It is difficult to review the patch5
> > without any testing environment and detailed knowledge of watermark of tegra.
> > It is not familiar with me.
>
> Thank you very much! I'll go through yours comments and reply to them.
>
> I understand that it's not easy for you to review patch5, but probably
> you don't need to go into details and a brief-generic review of the code
> will be enough in that case.
>
> The hardware is actually very simple, there are watermarks that
> correspond to a memory activity that hardware accounts over a given
> period of time. Once watermark is reached, hardware generates interrupt.
> There are two types of watermarks: average and consecutive. In case of
> the average, the memory activity is collected over a larger window of
> time. For the consecutive case, the memory activity is collected over
> each period (16ms by default in the driver). Memory client may breach
> average watermark very frequently, although that may not affect much the
> average value and for some memory clients (like CPU) it is more
> preferred to not completely ignore those short bursts of memory
> activity. The consecutive watermarks are used in order to detect those
> short bursts, which we account in the driver in a form of boosting. You
> may notice that boost_up_coeff for the CPU's memory client is set to a
> higher value in the driver.
>
> > Hello Thierry,
> > If possible, Could you review the patch5 related to setting up the watermark
> > and other patches?
> >
>
> Indeed, will be very nice if Thierry could also take a look at this
> series. Although.. I could be wrong here, but it looks to me that
> Thierry also isn't closely familiar with this driver and the hardware.
>
> Thierry, at least please let us know if you're interested in taking a
> look at the patches, I'm pretty sure that you're quite busy with other
> things ;)