mbox series

[RFC,0/3] Add support of busfreq

Message ID 20190313193408.23740-1-abailon@baylibre.com (mailing list archive)
Headers show
Series Add support of busfreq | expand

Message

Alexandre Bailon March 13, 2019, 7:34 p.m. UTC
This series implements busfreq, a framework used in MXP's
tree to scale the interconnect and dram frequencies.
In the vendor tree, device's driver request for a
performance level, which is used to scale the frequencies.
This series implements it using the interconnect framework.
Devices' driver request for bandwidth which is use by busfreq
to determine a performance level, and then scale the frequency.

Busfreq is quite generic. It could be used for any i.MX SoC.
A busfreq platform driver just have to define a list of
interconnect nodes, and some OPPs.

This series is sent as RFC mostly because the current support
of i.MX SoC won't benefit of busfreq framework, because the
clocks' driver don't support interconnect / dram frequency
scaling.
As exemple, this series implements busfreq for i.MX8MM whose
upstreaming is in progress. Because this relies on ATF to
do the frequency scaling, it won't be hard make it work.

As exemple, this series implements busfreq for 
Alexandre Bailon (3):
  drivers: interconnect: Add a driver for i.MX SoC
  drivers: interconnect: imx: Add support of i.MX8MM
  dt-bindings: interconnect: Document fsl,busfreq-imx8mm bindings

 .../bindings/interconnect/imx8mm.txt          |  24 +
 drivers/interconnect/Kconfig                  |   1 +
 drivers/interconnect/Makefile                 |   1 +
 drivers/interconnect/imx/Kconfig              |  17 +
 drivers/interconnect/imx/Makefile             |   2 +
 drivers/interconnect/imx/busfreq-imx8mm.c     | 132 ++++
 drivers/interconnect/imx/busfreq.c            | 570 ++++++++++++++++++
 drivers/interconnect/imx/busfreq.h            | 123 ++++
 include/dt-bindings/interconnect/imx8mm.h     |  37 ++
 9 files changed, 907 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interconnect/imx8mm.txt
 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

Comments

Dong Aisheng March 15, 2019, 2:39 a.m. UTC | #1
+Jacky and Leonard, Ranjani

Hi Alexandre,

> From: Alexandre Bailon [mailto:abailon@baylibre.com]
> 
> This series implements busfreq, a framework used in MXP's tree to scale the
> interconnect and dram frequencies.
> In the vendor tree, device's driver request for a performance level, which is
> used to scale the frequencies.
> This series implements it using the interconnect framework.
> Devices' driver request for bandwidth which is use by busfreq to determine a
> performance level, and then scale the frequency.
> 
> Busfreq is quite generic. It could be used for any i.MX SoC.
> A busfreq platform driver just have to define a list of interconnect nodes, and
> some OPPs.
> 

It's really great to see this patch series.
And it should be the correct direction we're heading to upstream busfreq support.

> This series is sent as RFC mostly because the current support of i.MX SoC won't
> benefit of busfreq framework, because the clocks' driver don't support
> interconnect / dram frequency scaling.
> As exemple, this series implements busfreq for i.MX8MM whose upstreaming
> is in progress. Because this relies on ATF to do the frequency scaling, it won't
> be hard make it work.
> 

How can I test this patch series?
Any additional patches you can share with us?
Or what else we need to do to test it? We can help with it.

Regards
Dong Aisheng

> As exemple, this series implements busfreq for Alexandre Bailon (3):
>   drivers: interconnect: Add a driver for i.MX SoC
>   drivers: interconnect: imx: Add support of i.MX8MM
>   dt-bindings: interconnect: Document fsl,busfreq-imx8mm bindings
> 
>  .../bindings/interconnect/imx8mm.txt          |  24 +
>  drivers/interconnect/Kconfig                  |   1 +
>  drivers/interconnect/Makefile                 |   1 +
>  drivers/interconnect/imx/Kconfig              |  17 +
>  drivers/interconnect/imx/Makefile             |   2 +
>  drivers/interconnect/imx/busfreq-imx8mm.c     | 132 ++++
>  drivers/interconnect/imx/busfreq.c            | 570
> ++++++++++++++++++
>  drivers/interconnect/imx/busfreq.h            | 123 ++++
>  include/dt-bindings/interconnect/imx8mm.h     |  37 ++
>  9 files changed, 907 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/interconnect/imx8mm.txt
>  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.19.2
Alexandre Bailon March 15, 2019, 9:31 a.m. UTC | #2
Hi Aisheng

On 3/15/19 3:39 AM, Aisheng Dong wrote:
> +Jacky and Leonard, Ranjani
> 
> Hi Alexandre,
> 
>> From: Alexandre Bailon [mailto:abailon@baylibre.com]
>>
>> This series implements busfreq, a framework used in MXP's tree to scale the
>> interconnect and dram frequencies.
>> In the vendor tree, device's driver request for a performance level, which is
>> used to scale the frequencies.
>> This series implements it using the interconnect framework.
>> Devices' driver request for bandwidth which is use by busfreq to determine a
>> performance level, and then scale the frequency.
>>
>> Busfreq is quite generic. It could be used for any i.MX SoC.
>> A busfreq platform driver just have to define a list of interconnect nodes, and
>> some OPPs.
>>
> 
> It's really great to see this patch series.
> And it should be the correct direction we're heading to upstream busfreq support.
> 
>> This series is sent as RFC mostly because the current support of i.MX SoC won't
>> benefit of busfreq framework, because the clocks' driver don't support
>> interconnect / dram frequency scaling.
>> As exemple, this series implements busfreq for i.MX8MM whose upstreaming
>> is in progress. Because this relies on ATF to do the frequency scaling, it won't
>> be hard make it work.
>>
> 
> How can I test this patch series?
> Any additional patches you can share with us?
> Or what else we need to do to test it? We can help with it.
Many other patches will be required to test the series.
There are a couple of patches that updates i.MX device drivers to 
request for bandwidth (does similar thing as bus_freq_request and 
bus_freq_release).
In addition, a patch to that allow to scale the DRAM
frequency using CCF is required. I'm still working on this patch. It 
works correctly on 4.14, but since I rebased it on linux-next/master, it 
stopped to work. I'm trying to fix it now.
I will share all this patches ASAP.

Best Regards,
Alexandre
> 
> Regards
> Dong Aisheng
> 
>> As exemple, this series implements busfreq for Alexandre Bailon (3):
>>    drivers: interconnect: Add a driver for i.MX SoC
>>    drivers: interconnect: imx: Add support of i.MX8MM
>>    dt-bindings: interconnect: Document fsl,busfreq-imx8mm bindings
>>
>>   .../bindings/interconnect/imx8mm.txt          |  24 +
>>   drivers/interconnect/Kconfig                  |   1 +
>>   drivers/interconnect/Makefile                 |   1 +
>>   drivers/interconnect/imx/Kconfig              |  17 +
>>   drivers/interconnect/imx/Makefile             |   2 +
>>   drivers/interconnect/imx/busfreq-imx8mm.c     | 132 ++++
>>   drivers/interconnect/imx/busfreq.c            | 570
>> ++++++++++++++++++
>>   drivers/interconnect/imx/busfreq.h            | 123 ++++
>>   include/dt-bindings/interconnect/imx8mm.h     |  37 ++
>>   9 files changed, 907 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/interconnect/imx8mm.txt
>>   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.19.2
Michael Turquette March 15, 2019, 4:17 p.m. UTC | #3
Hi Alex,

Some nitpick review comments below.

On Wed, Mar 13, 2019 at 12:33 PM Alexandre Bailon <abailon@baylibre.com> wrote:
>
> This series implements busfreq, a framework used in MXP's

s/MXP/NXP/

> tree to scale the interconnect and dram frequencies.
> In the vendor tree, device's driver request for a
> performance level, which is used to scale the frequencies.
> This series implements it using the interconnect framework.
> Devices' driver request for bandwidth which is use by busfreq
> to determine a performance level, and then scale the frequency.
>
> Busfreq is quite generic. It could be used for any i.MX SoC.
> A busfreq platform driver just have to define a list of
> interconnect nodes, and some OPPs.
>
> This series is sent as RFC mostly because the current support
> of i.MX SoC won't benefit of busfreq framework, because the
> clocks' driver don't support interconnect / dram frequency
> scaling.

In your v2 cover letter could you post a link to a git branch that has
everything integrated that is needed to test the series? I guess this
is similar to what Aisheng asked for already.

> As exemple, this series implements busfreq for i.MX8MM whose
> upstreaming is in progress. Because this relies on ATF to
> do the frequency scaling, it won't be hard make it work.

It's not clear to me whether this series actual scales the dram
frequency based on what you said above. Is it just theoretical or do
you have it working with a pile of out-of-tree patches? Would be good
to include that pile of patches in your integration branch that I
suggested above.

Thanks,
Mike

>
> As exemple, this series implements busfreq for
> Alexandre Bailon (3):
>   drivers: interconnect: Add a driver for i.MX SoC
>   drivers: interconnect: imx: Add support of i.MX8MM
>   dt-bindings: interconnect: Document fsl,busfreq-imx8mm bindings
>
>  .../bindings/interconnect/imx8mm.txt          |  24 +
>  drivers/interconnect/Kconfig                  |   1 +
>  drivers/interconnect/Makefile                 |   1 +
>  drivers/interconnect/imx/Kconfig              |  17 +
>  drivers/interconnect/imx/Makefile             |   2 +
>  drivers/interconnect/imx/busfreq-imx8mm.c     | 132 ++++
>  drivers/interconnect/imx/busfreq.c            | 570 ++++++++++++++++++
>  drivers/interconnect/imx/busfreq.h            | 123 ++++
>  include/dt-bindings/interconnect/imx8mm.h     |  37 ++
>  9 files changed, 907 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interconnect/imx8mm.txt
>  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.19.2
Alexandre Bailon March 15, 2019, 4:55 p.m. UTC | #4
Hi Mike,
On 3/15/19 5:17 PM, Michael Turquette wrote:
> Hi Alex,
> 
> Some nitpick review comments below.
> 
> On Wed, Mar 13, 2019 at 12:33 PM Alexandre Bailon <abailon@baylibre.com> wrote:
>>
>> This series implements busfreq, a framework used in MXP's
> 
> s/MXP/NXP/
> 
>> tree to scale the interconnect and dram frequencies.
>> In the vendor tree, device's driver request for a
>> performance level, which is used to scale the frequencies.
>> This series implements it using the interconnect framework.
>> Devices' driver request for bandwidth which is use by busfreq
>> to determine a performance level, and then scale the frequency.
>>
>> Busfreq is quite generic. It could be used for any i.MX SoC.
>> A busfreq platform driver just have to define a list of
>> interconnect nodes, and some OPPs.
>>
>> This series is sent as RFC mostly because the current support
>> of i.MX SoC won't benefit of busfreq framework, because the
>> clocks' driver don't support interconnect / dram frequency
>> scaling.
> 
> In your v2 cover letter could you post a link to a git branch that has
> everything integrated that is needed to test the series? I guess this
> is similar to what Aisheng asked for already.
I will do it.
> 
>> As exemple, this series implements busfreq for i.MX8MM whose
>> upstreaming is in progress. Because this relies on ATF to
>> do the frequency scaling, it won't be hard make it work.
> 
> It's not clear to me whether this series actual scales the dram
> frequency based on what you said above. Is it just theoretical or do
> you have it working with a pile of out-of-tree patches? Would be good
> to include that pile of patches in your integration branch that I
> suggested above.
The current series only introduce busfreq generic driver, and the 
busfreq driver for the imx8mm.
As is, the imx8mm driver will just be loaded, but do nothing because
none of the drivers have been updated to request bandwidth using the 
interconnect framework.
In addition, the current clock driver of imx8mm doesn't allow dram 
frequency scaling, so if busfreq driver tries, it will fail (should be 
harmless because any other clocks should restored to their previous rate).

My intent was to sent a first draft o busfreq, to get some feedback, 
before to send a more complete, and fully functional series.

Thanks,
Alexandre
> 
> Thanks,
> Mike
> 
>>
>> As exemple, this series implements busfreq for
>> Alexandre Bailon (3):
>>    drivers: interconnect: Add a driver for i.MX SoC
>>    drivers: interconnect: imx: Add support of i.MX8MM
>>    dt-bindings: interconnect: Document fsl,busfreq-imx8mm bindings
>>
>>   .../bindings/interconnect/imx8mm.txt          |  24 +
>>   drivers/interconnect/Kconfig                  |   1 +
>>   drivers/interconnect/Makefile                 |   1 +
>>   drivers/interconnect/imx/Kconfig              |  17 +
>>   drivers/interconnect/imx/Makefile             |   2 +
>>   drivers/interconnect/imx/busfreq-imx8mm.c     | 132 ++++
>>   drivers/interconnect/imx/busfreq.c            | 570 ++++++++++++++++++
>>   drivers/interconnect/imx/busfreq.h            | 123 ++++
>>   include/dt-bindings/interconnect/imx8mm.h     |  37 ++
>>   9 files changed, 907 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/interconnect/imx8mm.txt
>>   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.19.2
> 
> 
>
Leonard Crestez March 15, 2019, 5:17 p.m. UTC | #5
On 3/15/19 11:31 AM, Alexandre Bailon wrote:
>>> This series is sent as RFC mostly because the current support of i.MX SoC won't
>>> benefit of busfreq framework, because the clocks' driver don't support
>>> interconnect / dram frequency scaling.
>>> As exemple, this series implements busfreq for i.MX8MM whose upstreaming
>>> is in progress. Because this relies on ATF to do the frequency scaling, it won't
>>> be hard make it work.

>> How can I test this patch series?
>> Any additional patches you can share with us?
>> Or what else we need to do to test it? We can help with it.

> Many other patches will be required to test the series.
> There are a couple of patches that updates i.MX device drivers to
> request for bandwidth (does similar thing as bus_freq_request and
> bus_freq_release).

The interconnect framework asks for bandwidth in bytes/second but in NXP 
tree most requests are of the form "request_bus_freq(BUS_FREQ_HIGH);". 
In many cases (ethernet) it doesn't seem you can calculate a specific 
bandwidth usefully.

Instead of asking for "infinite bandwidth zero latency" to force 
everything to "high" it would be nicer to "request an opp".

Power-domain bindings mentioned that consumer-devices can specify a 
"required-opps" property but I've found zero users in tree. Maybe some 
helpers could be written to parse that property and automatically 
request ICC opp on device suspend/resume via device-links?

I know that stuff was written for genpd but it looks like a very good 
fit to me.

> In addition, a patch to that allow to scale the DRAM
> frequency using CCF is required. I'm still working on this patch.

I'm not sure what you mean here, do you want a clk set_rate to change 
the DRAM freq? It makes sense for DDRC to be a node in the interconnect 
framework and switch OPP inside icc implementation via an ATF call.

--
Regards,
Leonard
Viresh Kumar April 10, 2019, 5:29 a.m. UTC | #6
On 15-03-19, 17:17, Leonard Crestez wrote:
> On 3/15/19 11:31 AM, Alexandre Bailon wrote:
> >>> This series is sent as RFC mostly because the current support of i.MX SoC won't
> >>> benefit of busfreq framework, because the clocks' driver don't support
> >>> interconnect / dram frequency scaling.
> >>> As exemple, this series implements busfreq for i.MX8MM whose upstreaming
> >>> is in progress. Because this relies on ATF to do the frequency scaling, it won't
> >>> be hard make it work.
> 
> >> How can I test this patch series?
> >> Any additional patches you can share with us?
> >> Or what else we need to do to test it? We can help with it.
> 
> > Many other patches will be required to test the series.
> > There are a couple of patches that updates i.MX device drivers to
> > request for bandwidth (does similar thing as bus_freq_request and
> > bus_freq_release).
> 
> The interconnect framework asks for bandwidth in bytes/second but in NXP 
> tree most requests are of the form "request_bus_freq(BUS_FREQ_HIGH);". 
> In many cases (ethernet) it doesn't seem you can calculate a specific 
> bandwidth usefully.
> 
> Instead of asking for "infinite bandwidth zero latency" to force 
> everything to "high" it would be nicer to "request an opp".
> 
> Power-domain bindings mentioned that consumer-devices can specify a 
> "required-opps" property but I've found zero users in tree. Maybe some 
> helpers could be written to parse that property and automatically 
> request ICC opp on device suspend/resume via device-links?

Documentation/devicetree/bindings/power/qcom,rpmpd.txt is using it currently in
mainline.

> I know that stuff was written for genpd but it looks like a very good 
> fit to me.

Yes, the very first user is genpd but we have designed it with an open mind and
so it shouldn't be difficult to make use of it at other places.

There is some WIP which you can look at :

- Introduce OPP bandwidth bindings
  lore.kernel.org/lkml/20190313090010.20534-1-georgi.djakov@linaro.org

- DVFS in the OPP core
  https://lore.kernel.org/lkml/20190320094918.20234-1-rnayak@codeaurora.org
Krzysztof Kozlowski May 3, 2019, 11:19 a.m. UTC | #7
On Wed, 13 Mar 2019 at 20:35, Alexandre Bailon <abailon@baylibre.com> wrote:
>
> This series implements busfreq, a framework used in MXP's
> tree to scale the interconnect and dram frequencies.
> In the vendor tree, device's driver request for a
> performance level, which is used to scale the frequencies.
> This series implements it using the interconnect framework.
> Devices' driver request for bandwidth which is use by busfreq
> to determine a performance level, and then scale the frequency.
>
> Busfreq is quite generic. It could be used for any i.MX SoC.
> A busfreq platform driver just have to define a list of
> interconnect nodes, and some OPPs.
>
> This series is sent as RFC mostly because the current support
> of i.MX SoC won't benefit of busfreq framework, because the
> clocks' driver don't support interconnect / dram frequency
> scaling.
> As exemple, this series implements busfreq for i.MX8MM whose
> upstreaming is in progress. Because this relies on ATF to
> do the frequency scaling, it won't be hard make it work.
>
> As exemple, this series implements busfreq for
> Alexandre Bailon (3):
>   drivers: interconnect: Add a driver for i.MX SoC
>   drivers: interconnect: imx: Add support of i.MX8MM
>   dt-bindings: interconnect: Document fsl,busfreq-imx8mm bindings

Hi Alexandre,

I am quite late but I just found your email.

This looks very similar to existing framework - devfreq, which purpose
is to scale the system busses based on performance counters/events. It
would be nice if we could avoid duplication of existing subsystems.

Best regards,
Krzysztof
Georgi Djakov May 14, 2019, 6:33 a.m. UTC | #8
On 5/3/19 14:19, Krzysztof Kozlowski wrote:
> On Wed, 13 Mar 2019 at 20:35, Alexandre Bailon <abailon@baylibre.com> wrote:
>>
>> This series implements busfreq, a framework used in MXP's
>> tree to scale the interconnect and dram frequencies.
>> In the vendor tree, device's driver request for a
>> performance level, which is used to scale the frequencies.
>> This series implements it using the interconnect framework.
>> Devices' driver request for bandwidth which is use by busfreq
>> to determine a performance level, and then scale the frequency.
>>
>> Busfreq is quite generic. It could be used for any i.MX SoC.
>> A busfreq platform driver just have to define a list of
>> interconnect nodes, and some OPPs.
>>
>> This series is sent as RFC mostly because the current support
>> of i.MX SoC won't benefit of busfreq framework, because the
>> clocks' driver don't support interconnect / dram frequency
>> scaling.
>> As exemple, this series implements busfreq for i.MX8MM whose
>> upstreaming is in progress. Because this relies on ATF to
>> do the frequency scaling, it won't be hard make it work.
>>
>> As exemple, this series implements busfreq for
>> Alexandre Bailon (3):
>>   drivers: interconnect: Add a driver for i.MX SoC
>>   drivers: interconnect: imx: Add support of i.MX8MM
>>   dt-bindings: interconnect: Document fsl,busfreq-imx8mm bindings
> 
> Hi Alexandre,
> 
> I am quite late but I just found your email.
> 
> This looks very similar to existing framework - devfreq, which purpose
> is to scale the system busses based on performance counters/events. It
> would be nice if we could avoid duplication of existing subsystems.

Hi Krzysztof,

Scaling buses based on performance counters is suboptimal and sometimes might
not work well. In contrast with devfreq, the interconnect API is allowing
drivers to express their needs in advance and be proactive. It is also designed
to deal with multi-tiered bus topologies and to aggregate the requests from the
different consumer drivers.

Thanks,
Georgi

> 
> Best regards,
> Krzysztof
>
Leonard Crestez May 14, 2019, 7:34 p.m. UTC | #9
On 15.03.2019 18:55, Alexandre Bailon wrote:
>> On Wed, Mar 13, 2019 at 12:33 PM Alexandre Bailon <abailon@baylibre.com> wrote:

>>> As exemple, this series implements busfreq for i.MX8MM whose
>>> upstreaming is in progress. Because this relies on ATF to
>>> do the frequency scaling, it won't be hard make it work.
>>
>> It's not clear to me whether this series actual scales the dram
>> frequency based on what you said above. Is it just theoretical or do
>> you have it working with a pile of out-of-tree patches? Would be good
>> to include that pile of patches in your integration branch that I
>> suggested above.

> The current series only introduce busfreq generic driver, and the
> busfreq driver for the imx8mm.
> As is, the imx8mm driver will just be loaded, but do nothing because
> none of the drivers have been updated to request bandwidth using the
> interconnect framework.
> 
> My intent was to sent a first draft o busfreq, to get some feedback,
> before to send a more complete, and fully functional series.

It's been a while since this was first posted and imx8mm now boots fine 
in linux-next. Is there a more up-to-date WIP branch somewhere? 
Otherwise I can try to hack this series into a bootable form.

 > In addition, the current clock driver of imx8mm doesn't allow dram
 > frequency scaling, so if busfreq driver tries, it will fail (should be
 > harmless because any other clocks should restored to their previous
 > rate).

I'm confused about this. In NXP tree the actual DRAM switch is done 
inside ATF via SIP calls and involves corralling all CPUs. Do you want 
an "dram" clk which wraps the SIP calls required to changing dram 
frequency and root switching etc?

I've been looking at the busfreq implementation in the NXP tree and 
refactoring just the "dram freq switch" behind a clk might work nicely.

This would be similar to the imx_cpu clk used for cpufreq-dt and it 
might even be possible to upstream this separately from the rest of 
busfreq logic dealing with device requests.


I haven't done a very careful review but I noticed you're not using the 
OPP framework and instead redefined everything? It's not clear why.

--
Regards,
Leonard
Anson Huang June 4, 2019, 8:44 a.m. UTC | #10
Hi, Alexandre

> -----Original Message-----
> From: Leonard Crestez
> Sent: Wednesday, May 15, 2019 3:34 AM
> To: Alexandre Bailon <abailon@baylibre.com>; Jacky Bai <ping.bai@nxp.com>
> Cc: Michael Turquette <mturquette@baylibre.com>; Linux PM list <linux-
> pm@vger.kernel.org>; Georgi Djakov <georgi.djakov@linaro.org>; Patrick
> Titiano <ptitiano@baylibre.com>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Stephen Boyd <sboyd@codeaurora.org>; Emilio
> Lopez <emilio@elopez.com.ar>; Hans de Goede <hdegoede@redhat.com>;
> linux-clk <linux-clk@vger.kernel.org>; linux-arm-kernel <linux-arm-
> kernel@lists.infradead.org>; Zening Wang <zening.wang@nxp.com>;
> Aisheng Dong <aisheng.dong@nxp.com>; Kevin Hilman
> <khilman@baylibre.com>; Carlo Caione <ccaione@baylibre.com>; dl-linux-
> imx <linux-imx@nxp.com>; Anson Huang <anson.huang@nxp.com>; Viresh
> Kumar <viresh.kumar@linaro.org>
> Subject: Re: [RFC PATCH 0/3] Add support of busfreq
> 
> On 15.03.2019 18:55, Alexandre Bailon wrote:
> >> On Wed, Mar 13, 2019 at 12:33 PM Alexandre Bailon
> <abailon@baylibre.com> wrote:
> 
> >>> As exemple, this series implements busfreq for i.MX8MM whose
> >>> upstreaming is in progress. Because this relies on ATF to do the
> >>> frequency scaling, it won't be hard make it work.

I have similar question as previous reviewer, is there any branch that we can test
this series? 

And, from the patch, it has multiple levels description of fabric arch, while we ONLY
intend to scale "bus" frequency per devices' request, here "bus" includes DRAM, NOC and
AHB, AXI, should we make it more flatter, such as just a virtual fabric as a single provider, and then
all other devices as nodes under this provider?

Anson

> >>
> >> It's not clear to me whether this series actual scales the dram
> >> frequency based on what you said above. Is it just theoretical or do
> >> you have it working with a pile of out-of-tree patches? Would be good
> >> to include that pile of patches in your integration branch that I
> >> suggested above.
> 
> > The current series only introduce busfreq generic driver, and the
> > busfreq driver for the imx8mm.
> > As is, the imx8mm driver will just be loaded, but do nothing because
> > none of the drivers have been updated to request bandwidth using the
> > interconnect framework.
> >
> > My intent was to sent a first draft o busfreq, to get some feedback,
> > before to send a more complete, and fully functional series.
> 
> It's been a while since this was first posted and imx8mm now boots fine in
> linux-next. Is there a more up-to-date WIP branch somewhere?
> Otherwise I can try to hack this series into a bootable form.
> 
>  > In addition, the current clock driver of imx8mm doesn't allow dram  >
> frequency scaling, so if busfreq driver tries, it will fail (should be  > harmless
> because any other clocks should restored to their previous  > rate).
> 
> I'm confused about this. In NXP tree the actual DRAM switch is done inside
> ATF via SIP calls and involves corralling all CPUs. Do you want an "dram" clk
> which wraps the SIP calls required to changing dram frequency and root
> switching etc?
> 
> I've been looking at the busfreq implementation in the NXP tree and
> refactoring just the "dram freq switch" behind a clk might work nicely.
> 
> This would be similar to the imx_cpu clk used for cpufreq-dt and it might
> even be possible to upstream this separately from the rest of busfreq logic
> dealing with device requests.
> 
> 
> I haven't done a very careful review but I noticed you're not using the OPP
> framework and instead redefined everything? It's not clear why.
> 
> --
> Regards,
> Leonard
Leonard Crestez June 4, 2019, 8:13 p.m. UTC | #11
On 6/4/2019 11:44 AM, Anson Huang wrote:
>>>>> As exemple, this series implements busfreq for i.MX8MM whose
>>>>> upstreaming is in progress. Because this relies on ATF to do the
>>>>> frequency scaling, it won't be hard make it work.
> 
> I have similar question as previous reviewer, is there any branch that we can test
> this series?

I've been looking at this and pushed a fixed-up functional variant to my 
personal github:

     https://github.com/cdleonard/linux/commits/next_imx8mm_busfreq

It builds and probes and switches DRAM freq between low and high based 
on whether ethernet is down or up (for testing purposes). The pile of 
out-of-tree patches required to get this work is quite small.

The DRAM freq switch is done via a clk wrapper previously sent as RFC:

     https://patchwork.kernel.org/patch/10968303/

That part needs more work but it could serve as a neat encapsulation 
similar to imx_cpu clk used for cpufreq-dt.

> And, from the patch, it has multiple levels description of fabric arch, while we ONLY
> intend to scale "bus" frequency per devices' request, here "bus" includes DRAM, NOC and
> AHB, AXI, should we make it more flatter, such as just a virtual fabric as a single provider, and then
> all other devices as nodes under this provider?

The imx8mm interconnect bindings describe many bus endpoints but all 
requests are aggregated to a single platform-level OPP which is 
equivalent to "low/audio/high mode" from NXP tree.

It might be better to associate clks to several ICC nodes and this way 
scale NOC and DRAM separately? As far as I understand an interconnect 
provider is free to decide on granularity.

As a wilder idea it might even be possible to use a stanard 
"devfreq-with-perfmon" for DDRC and have interconnect request a min freq 
to that instead of doing clk_set_rate on dram directly. That could bring 
features from both worlds, scaling both proactively and reactively.

--
Regards,
Leonard