Hi Leonard,

On Fri, Jun 28, 2019 at 12:40 AM Leonard Crestez
[off-list ref] wrote:

This series attempts to add upstream DVFS support for imx8mm, covering dynamic
scaling of internal buses and dram. It uses the interconnect framework for
proactive scaling (in response to explicit bandwidth requests from devices) and
devfreq in order expose the buses and eventually implement reactive scaling (in
response to measuredtraffic).

I'm assuming you took a glance at my patch series [1] adding BW OPP
tables and adding devfreq support for scaling interconnect paths.

Based on looking at your patch series, I think [1] would allow you to
use ICC framework for both proactive and reactive scaling. Proactive
scaling would use the ICC framework directly. Reactive scaling would
go through devfreq (so that you can use various governors/adjust
policy) before it goes to ICC framework.

Actual scaling is performed through the clk framework: The NOC and main NICs
are driven by composite clks and a new 'imx8m-dram' clk is included for
scaling dram using firmware calls.

Seems reasonable. All hardware block in the end run using a clock.

The interconnect and devfreq parts do not communicate explicitly: they both
just call clk_set_min_rate and the clk core picks the minimum value that can
satisfy both. They are thus completely independent.

Two different parts not talking to each other and just setting min
rate can cause problems for some concurrency use cases. ICC framework
is explicitly designed to handle cases like this and aggregate the
needs correctly. You might want to look into that more closely.

This is easily extensible to more members of the imx8m family, some of which
expose more detailed controls over interconnect fabric frequencies.

TODO:
* Clarify DT bindings
* Clarify interconnect OPP picking logic
* Implement devfreq_event for imx8m ddrc
* Expose more dram frequencies

The clk_set_min_rate approach does not mesh very well with the OPP framework.
Some of interconnect nodes on imx8m can run at different voltages: OPP can
handle this well but not in response to a clk_set_min_rate from an unrelated
subsystem. Maybe set voltage on a clk notifier?

I think if you design it something like below, it might make your life
a whole lot easier.
Hopefully the formatting looks okay on your end. The arrow going up is
just connecting devfreq to ICC.

                        Proactive -> ICC -> clk/OPP API to set freq/volt
                                      ^
                                      |
HW measure -> governor -> devfreq ----+

That way, all voltage/frequency requirements are managed cleanly by
clk/OPP frameworks. The ICC deals with aggregating all the
requirements and devfreq lets you handle reactive scaling and policy.

And in the beginning, while you get a governor going, you can use
"performance" governor for the "reactive scaling" users. That way,
your reactive paths will continue to have good performance while the
rest of the "proactive" clients change to use ICC APIs.

Vendor tree does not support voltage switching, independent freqs for
different parts of the fabric or any reactive scaling. I think it's important
to pick an upstreaming approach which can support as much as possible.

Feedback welcome.

If all of this makes sense, please take a look at [2] and provide your
thoughts. I've dropped a few patches from [1] to avoid confusion (too
much going on at once). I think BW OPP tables and having OPP tables
for interconnect paths will be something you'll need (if not now,
eventually) and something you can build on top of nicely.

Thanks,
Saravana

[1] - https://lore.kernel.org/lkml/20190614041733.120807-1-saravanak@google.com/ (local)
[2] - https://lore.kernel.org/lkml/20190703011020.151615-1-saravanak@google.com/ (local)

Some objections were apparently raised to doing DRAM switch inside CLK:
perhaps ICC should make min_freq requests to devfreq instead?

Link to v1 (multiple chunks):
 * https://patchwork.kernel.org/patch/10976897/
 * https://patchwork.kernel.org/patch/10968303/
 * https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=91251

Also as a github branch (with few other changes):
    https://github.com/cdleonard/linux/tree/next_imx8mm_busfreq

Alexandre Bailon (2):
  interconnect: Add generic driver for imx
  interconnect: imx: Add platform driver for imx8mm

Leonard Crestez (6):
  clk: imx8mm: Add dram freq switch support
  clk: imx8m-composite: Switch to determine_rate
  arm64: dts: imx8mm: Add dram dvfs irqs to ccm node
  devfreq: Add imx-devfreq driver
  arm64: dts: imx8mm: Add interconnect node
  arm64: dts: imx8mm: Add devfreq-imx nodes

 arch/arm64/boot/dts/freescale/imx8mm.dtsi |  73 +++
 drivers/clk/imx/Makefile                  |   1 +
 drivers/clk/imx/clk-composite-8m.c        |  34 +-
 drivers/clk/imx/clk-imx8m-dram.c          | 357 ++++++++++++
 drivers/clk/imx/clk-imx8mm.c              |  12 +
 drivers/clk/imx/clk.h                     |  13 +
 drivers/devfreq/Kconfig                   |  10 +
 drivers/devfreq/Makefile                  |   1 +
 drivers/devfreq/imx-devfreq.c             | 142 +++++
 drivers/interconnect/Kconfig              |   1 +
 drivers/interconnect/Makefile             |   1 +
 drivers/interconnect/imx/Kconfig          |  17 +
 drivers/interconnect/imx/Makefile         |   2 +
 drivers/interconnect/imx/busfreq-imx8mm.c | 151 ++++++
 drivers/interconnect/imx/busfreq.c        | 628 ++++++++++++++++++++++
 drivers/interconnect/imx/busfreq.h        | 123 +++++
 include/dt-bindings/clock/imx8mm-clock.h  |   4 +-
 include/dt-bindings/interconnect/imx8mm.h |  49 ++
 18 files changed, 1606 insertions(+), 13 deletions(-)
 create mode 100644 drivers/clk/imx/clk-imx8m-dram.c
 create mode 100644 drivers/devfreq/imx-devfreq.c
 create mode 100644 drivers/interconnect/imx/Kconfig
 create mode 100644 drivers/interconnect/imx/Makefile
 create mode 100644 drivers/interconnect/imx/busfreq-imx8mm.c
 create mode 100644 drivers/interconnect/imx/busfreq.c
 create mode 100644 drivers/interconnect/imx/busfreq.h
 create mode 100644 include/dt-bindings/interconnect/imx8mm.h

--
2.17.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel