mbox series

[RFC,00/19] Add interconnect and devfreq support for i.MX8MQ

Message ID 1631554694-9599-1-git-send-email-abel.vesa@nxp.com (mailing list archive)
Headers show
Series Add interconnect and devfreq support for i.MX8MQ | expand

Message

Abel Vesa Sept. 13, 2021, 5:37 p.m. UTC
In vendor tree there is something called busfreq but it is
non-upstreamable. This approach with interconnect+devfreq gives us the
same functionality by using already upstream and mature subsystems.
This series is more of a proof-of-concept.

Abel Vesa (19):
  dt-bindings: interconnect: imx8mq: Add missing pl301 and SAI ids
  devfreq: imx-bus: Switch governor to powersave
  devfreq: imx-bus: Decouple imx-bus from icc made
  devfreq: imx8m-ddrc: Change governor to powersave
  devfreq: imx8m-ddrc: Use the opps acquired from EL3
  devfreq: imx8m-ddrc: Add late system sleep PM ops
  interconnect: imx: Switch from imx_icc_node_adj_desc to fsl,icc-id
    node assignment
  interconnect: imx8: Remove the imx_icc_node_adj_desc
  interconnect: imx8mq: Add the pl301_per_m and pl301_wakeup nodes and
    subnodes
  interconnect: imx8mq: Add of_match_table
  interconnect: imx: Add imx_icc_get_bw and imx_icc_aggregate functions
  arm64: dts: imx8mq: Add fsl,icc-id property to ddrc node
  arm64: dts: imx8mq: Add fsl,icc-id to noc node
  arm64: dts: imx8mq: Add all pl301 nodes
  arm64: dts: imx8mq: Add the interconnect node
  arm64: dts: imx8mq: Add interconnect properties to icc consumer nodes
  net: ethernet: fec_main: Add interconnect support
  mmc: sdhci-esdhc-imx: Add interconnect support
  arm64: defconfig: Add necessary configs for icc+devfreq on i.MX8MQ

 arch/arm64/boot/dts/freescale/imx8mq.dtsi | 222 +++++++++++++++++++++-
 arch/arm64/configs/defconfig              |   9 +-
 drivers/devfreq/imx-bus.c                 |  42 +---
 drivers/devfreq/imx8m-ddrc.c              |  78 +++-----
 drivers/interconnect/imx/imx.c            |  92 +++++----
 drivers/interconnect/imx/imx.h            |  19 +-
 drivers/interconnect/imx/imx8mm.c         |  32 +---
 drivers/interconnect/imx/imx8mn.c         |  28 +--
 drivers/interconnect/imx/imx8mq.c         |  59 +++---
 drivers/mmc/host/sdhci-esdhc-imx.c        |  27 +++
 drivers/net/ethernet/freescale/fec.h      |   3 +
 drivers/net/ethernet/freescale/fec_main.c |  19 ++
 include/dt-bindings/interconnect/imx8mq.h |   9 +
 13 files changed, 422 insertions(+), 217 deletions(-)

Comments

Martin Kepplinger Sept. 24, 2021, 10:20 a.m. UTC | #1
hi Abel,

thank you for the update (this is actually v2 of this RFC right?)!

all in all this runs fine on the imx8mq (Librem 5 and devkit) I use. For all
the pl301 nodes I'm not yet sure what I can actually test / switch frequencies.

But I still have one problem: lcdif/mxfb already has the interconnect dram
DT property and I use the following call to request bandwidth:
https://source.puri.sm/martin.kepplinger/linux-next/-/commit/d690e4c021293f938eb2253607f92f5a64f15688
(mainlining this is on our todo list).

With your patchset, I get:

[    0.792960] genirq: Flags mismatch irq 30. 00000004 (mxsfb-drm) vs. 00000004 (mxsfb-drm)
[    0.801143] mxsfb 30320000.lcd-controller: Failed to install IRQ handler
[    0.808058] mxsfb: probe of 30320000.lcd-controller failed with error -16

so the main devfreq user (mxsfb) is not there :) why?

and when I remove the interconnect property from the lcdif DT node, mxsfb
probes again, but of course it doesn't lower dram freq as needed.

Do I do the icc calls wrong in mxsfb despite it working without your
patchset, or may there be something wrong on your side that breaks
the mxsfb IRQ?

again thanks a lot for working on this! I'm always happy to test.

                          martin



---
 .../boot/dts/freescale/imx8mq-librem5.dtsi    | 20 -------------------
 1 file changed, 20 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi b/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
index 6fac6676f412..8496a90f23bf 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
@@ -381,26 +381,6 @@ &A53_3 {
 	cpu-supply = <&buck2_reg>;
 };
 
-&ddrc {
-	operating-points-v2 = <&ddrc_opp_table>;
-
-	ddrc_opp_table: ddrc-opp-table {
-		compatible = "operating-points-v2";
-
-		opp-25M {
-			opp-hz = /bits/ 64 <25000000>;
-		};
-
-		opp-100M {
-			opp-hz = /bits/ 64 <100000000>;
-		};
-
-		opp-800M {
-			opp-hz = /bits/ 64 <800000000>;
-		};
-	};
-};
-
 &dphy {
 	status = "okay";
 };
Abel Vesa Sept. 29, 2021, 11:44 a.m. UTC | #2
On 21-09-24 12:20:26, Martin Kepplinger wrote:
> hi Abel,
> 
> thank you for the update (this is actually v2 of this RFC right?)!
> 
> all in all this runs fine on the imx8mq (Librem 5 and devkit) I use. For all
> the pl301 nodes I'm not yet sure what I can actually test / switch frequencies.
> 

You can start by looking into each of the following:

 $ ls -1d /sys/devices/platform/soc@0/*/devfreq/*/trans_stat

and look if the transitions happen when a specific driver that is a icc user suspends.

You can also look at:

 /sys/kernel/debug/interconnect/interconnect_summary 

and:

 /sys/kernel/debug/interconnect/interconnect_graph

> But I still have one problem: lcdif/mxfb already has the interconnect dram
> DT property and I use the following call to request bandwidth:
> https://source.puri.sm/martin.kepplinger/linux-next/-/commit/d690e4c021293f938eb2253607f92f5a64f15688
> (mainlining this is on our todo list).
> 
> With your patchset, I get:
> 
> [    0.792960] genirq: Flags mismatch irq 30. 00000004 (mxsfb-drm) vs. 00000004 (mxsfb-drm)
> [    0.801143] mxsfb 30320000.lcd-controller: Failed to install IRQ handler
> [    0.808058] mxsfb: probe of 30320000.lcd-controller failed with error -16
> 
> so the main devfreq user (mxsfb) is not there :) why?
> 

OK, I admit, this patchset doesn't provide support for all the icc consumer drivers.
But that should come at a later stage. I only provided example like fec and usdhc, to show
how it all fits together.

> and when I remove the interconnect property from the lcdif DT node, mxsfb
> probes again, but of course it doesn't lower dram freq as needed.
> 
> Do I do the icc calls wrong in mxsfb despite it working without your
> patchset, or may there be something wrong on your side that breaks
> the mxsfb IRQ?
> 

Do you have the following changes into your tree?

diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi               
index 00dd8e39a595..c43a84622af5 100644                                                                          
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi                                                                  
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi                                                                  
@@ -524,7 +524,7 @@ lcdif: lcd-controller@30320000 {                                                             
                                                  <&clk IMX8MQ_VIDEO_PLL1>,                                      
                                                  <&clk IMX8MQ_VIDEO_PLL1_OUT>;                                  
                                assigned-clock-rates = <0>, <0>, <0>, <594000000>;                               
-                               interconnects = <&noc IMX8MQ_ICM_LCDIF &noc IMX8MQ_ICS_DRAM>;                    
+                               interconnects = <&icc IMX8MQ_ICM_LCDIF &icc IMX8MQ_ICS_DRAM>;                    
                                interconnect-names = "dram";                                                     
                                status = "disabled";                                                             
                                                                                                                 
@@ -1117,7 +1117,7 @@ mipi_csi1: csi@30a70000 {                                                                  
                                         <&src IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET>,                            
                                         <&src IMX8MQ_RESET_MIPI_CSI1_ESC_RESET>;                                
                                fsl,mipi-phy-gpr = <&iomuxc_gpr 0x88>;                                           
-                               interconnects = <&noc IMX8MQ_ICM_CSI1 &noc IMX8MQ_ICS_DRAM>;                     
+                               interconnects = <&icc IMX8MQ_ICM_CSI1 &icc IMX8MQ_ICS_DRAM>;                     
                                interconnect-names = "dram";                                                     
                                status = "disabled";                                                             
                                                                                                                 
@@ -1169,7 +1169,7 @@ mipi_csi2: csi@30b60000 {                                                                  
                                         <&src IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET>,                            
                                         <&src IMX8MQ_RESET_MIPI_CSI2_ESC_RESET>;                                
                                fsl,mipi-phy-gpr = <&iomuxc_gpr 0xa4>;                                           
-                               interconnects = <&noc IMX8MQ_ICM_CSI2 &noc IMX8MQ_ICS_DRAM>;                     
+                               interconnects = <&icc IMX8MQ_ICM_CSI2 &icc IMX8MQ_ICS_DRAM>;                     
                                interconnect-names = "dram";                                                     
                                status = "disabled";                                                             

I forgot to update these in the current version of the patchset. Will do in the next version.

Also, would help a lot if you could give me a link to a tree you're testing with.
That way I can look exactly at what's going on.

> again thanks a lot for working on this! I'm always happy to test.
> 
>                           martin
> 
> 
> 
> ---
>  .../boot/dts/freescale/imx8mq-librem5.dtsi    | 20 -------------------
>  1 file changed, 20 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi b/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
> index 6fac6676f412..8496a90f23bf 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
> @@ -381,26 +381,6 @@ &A53_3 {
>  	cpu-supply = <&buck2_reg>;
>  };
>  
> -&ddrc {
> -	operating-points-v2 = <&ddrc_opp_table>;
> -
> -	ddrc_opp_table: ddrc-opp-table {
> -		compatible = "operating-points-v2";
> -
> -		opp-25M {
> -			opp-hz = /bits/ 64 <25000000>;
> -		};
> -
> -		opp-100M {
> -			opp-hz = /bits/ 64 <100000000>;
> -		};
> -
> -		opp-800M {
> -			opp-hz = /bits/ 64 <800000000>;
> -		};
> -	};
> -};
> -
>  &dphy {
>  	status = "okay";
>  };
> -- 
> 2.30.2
>
Martin Kepplinger Sept. 30, 2021, 8:03 a.m. UTC | #3
Am Mittwoch, dem 29.09.2021 um 14:44 +0300 schrieb Abel Vesa:
> On 21-09-24 12:20:26, Martin Kepplinger wrote:
> > hi Abel,
> > 
> > thank you for the update (this is actually v2 of this RFC right?)!
> > 
> > all in all this runs fine on the imx8mq (Librem 5 and devkit) I
> > use. For all
> > the pl301 nodes I'm not yet sure what I can actually test / switch
> > frequencies.
> > 
> 
> You can start by looking into each of the following:
> 
>  $ ls -1d /sys/devices/platform/soc@0/*/devfreq/*/trans_stat
> 
> and look if the transitions happen when a specific driver that is a
> icc user suspends.
> 
> You can also look at:
> 
>  /sys/kernel/debug/interconnect/interconnect_summary 
> 
> and:
> 
>  /sys/kernel/debug/interconnect/interconnect_graph
> 
> > But I still have one problem: lcdif/mxfb already has the
> > interconnect dram
> > DT property and I use the following call to request bandwidth:
> > https://source.puri.sm/martin.kepplinger/linux-next/-/commit/d690e4c021293f938eb2253607f92f5a64f15688
> > (mainlining this is on our todo list).
> > 
> > With your patchset, I get:
> > 
> > [    0.792960] genirq: Flags mismatch irq 30. 00000004 (mxsfb-drm)
> > vs. 00000004 (mxsfb-drm)
> > [    0.801143] mxsfb 30320000.lcd-controller: Failed to install IRQ
> > handler
> > [    0.808058] mxsfb: probe of 30320000.lcd-controller failed with
> > error -16
> > 
> > so the main devfreq user (mxsfb) is not there :) why?
> > 
> 
> OK, I admit, this patchset doesn't provide support for all the icc
> consumer drivers.
> But that should come at a later stage. I only provided example like
> fec and usdhc, to show
> how it all fits together.
> 
> > and when I remove the interconnect property from the lcdif DT node,
> > mxsfb
> > probes again, but of course it doesn't lower dram freq as needed.
> > 
> > Do I do the icc calls wrong in mxsfb despite it working without
> > your
> > patchset, or may there be something wrong on your side that breaks
> > the mxsfb IRQ?
> > 
> 
> Do you have the following changes into your tree?
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mq.dtsi               
> index 00dd8e39a595..c43a84622af5
> 100644                                                               
>            
> ---
> a/arch/arm64/boot/dts/freescale/imx8mq.dtsi                          
>                                         
> +++
> b/arch/arm64/boot/dts/freescale/imx8mq.dtsi                          
>                                         
> @@ -524,7 +524,7 @@ lcdif: lcd-controller@30320000
> {                                                             
>                                                   <&clk
> IMX8MQ_VIDEO_PLL1>,                                      
>                                                   <&clk
> IMX8MQ_VIDEO_PLL1_OUT>;                                  
>                                 assigned-clock-rates = <0>, <0>, <0>,
> <594000000>;                               
> -                               interconnects = <&noc
> IMX8MQ_ICM_LCDIF &noc IMX8MQ_ICS_DRAM>;                    
> +                               interconnects = <&icc
> IMX8MQ_ICM_LCDIF &icc IMX8MQ_ICS_DRAM>;                    
>                                 interconnect-names =
> "dram";                                                     
>                                 status =
> "disabled";                                                          
>    
>                                                                      
>                                             
> @@ -1117,7 +1117,7 @@ mipi_csi1: csi@30a70000
> {                                                                  
>                                          <&src
> IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET>,                            
>                                          <&src
> IMX8MQ_RESET_MIPI_CSI1_ESC_RESET>;                                
>                                 fsl,mipi-phy-gpr = <&iomuxc_gpr
> 0x88>;                                           
> -                               interconnects = <&noc IMX8MQ_ICM_CSI1
> &noc IMX8MQ_ICS_DRAM>;                     
> +                               interconnects = <&icc IMX8MQ_ICM_CSI1
> &icc IMX8MQ_ICS_DRAM>;                     
>                                 interconnect-names =
> "dram";                                                     
>                                 status =
> "disabled";                                                          
>    
>                                                                      
>                                             
> @@ -1169,7 +1169,7 @@ mipi_csi2: csi@30b60000
> {                                                                  
>                                          <&src
> IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET>,                            
>                                          <&src
> IMX8MQ_RESET_MIPI_CSI2_ESC_RESET>;                                
>                                 fsl,mipi-phy-gpr = <&iomuxc_gpr
> 0xa4>;                                           
> -                               interconnects = <&noc IMX8MQ_ICM_CSI2
> &noc IMX8MQ_ICS_DRAM>;                     
> +                               interconnects = <&icc IMX8MQ_ICM_CSI2
> &icc IMX8MQ_ICS_DRAM>;                     
>                                 interconnect-names =
> "dram";                                                     
>                                 status =
> "disabled";                                                          
>    
> 
> I forgot to update these in the current version of the patchset. Will
> do in the next version.
> 
> Also, would help a lot if you could give me a link to a tree you're
> testing with.
> That way I can look exactly at what's going on.
> 
> 


thanks Abel, with the above fix of existing interconnects properties my
system runs as expected and here's the output of

for each in `ls -1d /sys/devices/platform/soc@0/*/devfreq/*`; do echo
$each; cat $each/trans_stat; done

for mxsfb requesting (max) bandwith (display on):

/sys/devices/platform/soc@0/32700000.noc/devfreq/32700000.noc
     From  :   To
           : 133333333 400000000 800000000   time(ms)
  133333333:         0         1         0       624
  400000000:         0         0         1        28
* 800000000:         1         0         0     30624
Total transition : 3
/sys/devices/platform/soc@0/3d400000.memory-
controller/devfreq/3d400000.memory-controller
     From  :   To
           :  25000000 100000000 800000000   time(ms)
   25000000:         0         0         1       620
  100000000:         0         0         0         0
* 800000000:         1         0         0     30652
Total transition : 2
/sys/devices/platform/soc@0/soc@0:pl301@0/devfreq/soc@0:pl301@0
     From  :   To
           :  25000000 133333333 333333333   time(ms)
   25000000:         0         0         1       616
  133333333:         0         0         0         0
* 333333333:         1         0         0     30668
Total transition : 2
/sys/devices/platform/soc@0/soc@0:pl301@1/devfreq/soc@0:pl301@1
     From  :   To
           :  25000000 266666666   time(ms)
*  25000000:         0         0     31284
  266666666:         0         0         0
Total transition : 0
/sys/devices/platform/soc@0/soc@0:pl301@2/devfreq/soc@0:pl301@2
     From  :   To
           :  25000000 800000000   time(ms)
*  25000000:         0         0     31288
  800000000:         1         0         0
Total transition : 1
/sys/devices/platform/soc@0/soc@0:pl301@3/devfreq/soc@0:pl301@3
     From  :   To
           :  25000000 800000000   time(ms)
*  25000000:         0         0     31292
  800000000:         1         0         0
Total transition : 1
/sys/devices/platform/soc@0/soc@0:pl301@4/devfreq/soc@0:pl301@4
     From  :   To
           :  25000000 333333333   time(ms)
   25000000:         0         1       648
* 333333333:         0         0     30652
Total transition : 1
/sys/devices/platform/soc@0/soc@0:pl301@5/devfreq/soc@0:pl301@5
     From  :   To
           :  25000000 500000000   time(ms)
*  25000000:         0         0     31304
  500000000:         1         0         0
Total transition : 1
/sys/devices/platform/soc@0/soc@0:pl301@6/devfreq/soc@0:pl301@6
     From  :   To
           :  25000000 500000000   time(ms)
*  25000000:         0         0     31308
  500000000:         0         0         0
Total transition : 0
/sys/devices/platform/soc@0/soc@0:pl301@7/devfreq/soc@0:pl301@7
     From  :   To
           :  25000000 128000000 500000000   time(ms)
*  25000000:         0         0         0     31312
  128000000:         0         0         0         0
  500000000:         1         0         0         0
Total transition : 1
/sys/devices/platform/soc@0/soc@0:pl301@8/devfreq/soc@0:pl301@8
     From  :   To
           :  25000000 133333333   time(ms)
*  25000000:         0         0     31316
  133333333:         0         0         0
Total transition : 0
/sys/devices/platform/soc@0/soc@0:pl301@9/devfreq/soc@0:pl301@9
     From  :   To
           :  25000000 133333333 266666666   time(ms)
   25000000:         0         0         5      1052
  133333333:         0         0         0         0
* 266666666:         5         0         0     30268
Total transition : 10


but with display off (mxsfb not requesting anything), I get the same
fast freqs for noc and memory-controller. They should use the lowest
freqs. Only pl301@4 switches to 25mhz in that case. That's odd.

said (still) out-of-tree mxsfb request is
https://source.puri.sm/martin.kepplinger/linux-next/-/commit/ee7b1453295932da1e292b734afa7a03651ad9ba

and the exact tree I'm running for the above is
https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.15-rc3/librem5__integration_byzantium_test_new_devfreq_interconnect

thanks,

                              martin
Abel Vesa Sept. 30, 2021, 2:22 p.m. UTC | #4
On 21-09-30 10:03:46, Martin Kepplinger wrote:
> Am Mittwoch, dem 29.09.2021 um 14:44 +0300 schrieb Abel Vesa:
> > On 21-09-24 12:20:26, Martin Kepplinger wrote:
> > > hi Abel,
> > > 
> > > thank you for the update (this is actually v2 of this RFC right?)!
> > > 
> > > all in all this runs fine on the imx8mq (Librem 5 and devkit) I
> > > use. For all
> > > the pl301 nodes I'm not yet sure what I can actually test / switch
> > > frequencies.
> > > 
> > 
> > You can start by looking into each of the following:
> > 
> >  $ ls -1d /sys/devices/platform/soc@0/*/devfreq/*/trans_stat
> > 
> > and look if the transitions happen when a specific driver that is a
> > icc user suspends.
> > 
> > You can also look at:
> > 
> >  /sys/kernel/debug/interconnect/interconnect_summary 
> > 
> > and:
> > 
> >  /sys/kernel/debug/interconnect/interconnect_graph
> > 
> > > But I still have one problem: lcdif/mxfb already has the
> > > interconnect dram
> > > DT property and I use the following call to request bandwidth:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.puri.sm%2Fmartin.kepplinger%2Flinux-next%2F-%2Fcommit%2Fd690e4c021293f938eb2253607f92f5a64f15688&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7C7fab8aca3a5f43d56f5608d983e8da67%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637685858400552603%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=%2FzyEQdOLU8jQuUpqJ74GTWyfrDvavz%2BxZAgv1tcIu9Y%3D&amp;reserved=0
> > > (mainlining this is on our todo list).
> > > 
> > > With your patchset, I get:
> > > 
> > > [    0.792960] genirq: Flags mismatch irq 30. 00000004 (mxsfb-drm)
> > > vs. 00000004 (mxsfb-drm)
> > > [    0.801143] mxsfb 30320000.lcd-controller: Failed to install IRQ
> > > handler
> > > [    0.808058] mxsfb: probe of 30320000.lcd-controller failed with
> > > error -16
> > > 
> > > so the main devfreq user (mxsfb) is not there :) why?
> > > 
> > 
> > OK, I admit, this patchset doesn't provide support for all the icc
> > consumer drivers.
> > But that should come at a later stage. I only provided example like
> > fec and usdhc, to show
> > how it all fits together.
> > 
> > > and when I remove the interconnect property from the lcdif DT node,
> > > mxsfb
> > > probes again, but of course it doesn't lower dram freq as needed.
> > > 
> > > Do I do the icc calls wrong in mxsfb despite it working without
> > > your
> > > patchset, or may there be something wrong on your side that breaks
> > > the mxsfb IRQ?
> > > 
> > 
> > Do you have the following changes into your tree?
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi               
> > index 00dd8e39a595..c43a84622af5
> > 100644                                                               
> >            
> > ---
> > a/arch/arm64/boot/dts/freescale/imx8mq.dtsi                          
> >                                         
> > +++
> > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi                          
> >                                         
> > @@ -524,7 +524,7 @@ lcdif: lcd-controller@30320000
> > {                                                             
> >                                                   <&clk
> > IMX8MQ_VIDEO_PLL1>,                                      
> >                                                   <&clk
> > IMX8MQ_VIDEO_PLL1_OUT>;                                  
> >                                 assigned-clock-rates = <0>, <0>, <0>,
> > <594000000>;                               
> > -                               interconnects = <&noc
> > IMX8MQ_ICM_LCDIF &noc IMX8MQ_ICS_DRAM>;                    
> > +                               interconnects = <&icc
> > IMX8MQ_ICM_LCDIF &icc IMX8MQ_ICS_DRAM>;                    
> >                                 interconnect-names =
> > "dram";                                                     
> >                                 status =
> > "disabled";                                                          
> >    
> >                                                                      
> >                                             
> > @@ -1117,7 +1117,7 @@ mipi_csi1: csi@30a70000
> > {                                                                  
> >                                          <&src
> > IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET>,                            
> >                                          <&src
> > IMX8MQ_RESET_MIPI_CSI1_ESC_RESET>;                                
> >                                 fsl,mipi-phy-gpr = <&iomuxc_gpr
> > 0x88>;                                           
> > -                               interconnects = <&noc IMX8MQ_ICM_CSI1
> > &noc IMX8MQ_ICS_DRAM>;                     
> > +                               interconnects = <&icc IMX8MQ_ICM_CSI1
> > &icc IMX8MQ_ICS_DRAM>;                     
> >                                 interconnect-names =
> > "dram";                                                     
> >                                 status =
> > "disabled";                                                          
> >    
> >                                                                      
> >                                             
> > @@ -1169,7 +1169,7 @@ mipi_csi2: csi@30b60000
> > {                                                                  
> >                                          <&src
> > IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET>,                            
> >                                          <&src
> > IMX8MQ_RESET_MIPI_CSI2_ESC_RESET>;                                
> >                                 fsl,mipi-phy-gpr = <&iomuxc_gpr
> > 0xa4>;                                           
> > -                               interconnects = <&noc IMX8MQ_ICM_CSI2
> > &noc IMX8MQ_ICS_DRAM>;                     
> > +                               interconnects = <&icc IMX8MQ_ICM_CSI2
> > &icc IMX8MQ_ICS_DRAM>;                     
> >                                 interconnect-names =
> > "dram";                                                     
> >                                 status =
> > "disabled";                                                          
> >    
> > 
> > I forgot to update these in the current version of the patchset. Will
> > do in the next version.
> > 
> > Also, would help a lot if you could give me a link to a tree you're
> > testing with.
> > That way I can look exactly at what's going on.
> > 
> > 
> 
> 
> thanks Abel, with the above fix of existing interconnects properties my
> system runs as expected and here's the output of
> 
> for each in `ls -1d /sys/devices/platform/soc@0/*/devfreq/*`; do echo
> $each; cat $each/trans_stat; done
> 
> for mxsfb requesting (max) bandwith (display on):
> 
> /sys/devices/platform/soc@0/32700000.noc/devfreq/32700000.noc
>      From  :   To
>            : 133333333 400000000 800000000   time(ms)
>   133333333:         0         1         0       624
>   400000000:         0         0         1        28
> * 800000000:         1         0         0     30624
> Total transition : 3
> /sys/devices/platform/soc@0/3d400000.memory-
> controller/devfreq/3d400000.memory-controller
>      From  :   To
>            :  25000000 100000000 800000000   time(ms)
>    25000000:         0         0         1       620
>   100000000:         0         0         0         0
> * 800000000:         1         0         0     30652
> Total transition : 2
> /sys/devices/platform/soc@0/soc@0:pl301@0/devfreq/soc@0:pl301@0
>      From  :   To
>            :  25000000 133333333 333333333   time(ms)
>    25000000:         0         0         1       616
>   133333333:         0         0         0         0
> * 333333333:         1         0         0     30668
> Total transition : 2
> /sys/devices/platform/soc@0/soc@0:pl301@1/devfreq/soc@0:pl301@1
>      From  :   To
>            :  25000000 266666666   time(ms)
> *  25000000:         0         0     31284
>   266666666:         0         0         0
> Total transition : 0
> /sys/devices/platform/soc@0/soc@0:pl301@2/devfreq/soc@0:pl301@2
>      From  :   To
>            :  25000000 800000000   time(ms)
> *  25000000:         0         0     31288
>   800000000:         1         0         0
> Total transition : 1
> /sys/devices/platform/soc@0/soc@0:pl301@3/devfreq/soc@0:pl301@3
>      From  :   To
>            :  25000000 800000000   time(ms)
> *  25000000:         0         0     31292
>   800000000:         1         0         0
> Total transition : 1
> /sys/devices/platform/soc@0/soc@0:pl301@4/devfreq/soc@0:pl301@4
>      From  :   To
>            :  25000000 333333333   time(ms)
>    25000000:         0         1       648
> * 333333333:         0         0     30652
> Total transition : 1
> /sys/devices/platform/soc@0/soc@0:pl301@5/devfreq/soc@0:pl301@5
>      From  :   To
>            :  25000000 500000000   time(ms)
> *  25000000:         0         0     31304
>   500000000:         1         0         0
> Total transition : 1
> /sys/devices/platform/soc@0/soc@0:pl301@6/devfreq/soc@0:pl301@6
>      From  :   To
>            :  25000000 500000000   time(ms)
> *  25000000:         0         0     31308
>   500000000:         0         0         0
> Total transition : 0
> /sys/devices/platform/soc@0/soc@0:pl301@7/devfreq/soc@0:pl301@7
>      From  :   To
>            :  25000000 128000000 500000000   time(ms)
> *  25000000:         0         0         0     31312
>   128000000:         0         0         0         0
>   500000000:         1         0         0         0
> Total transition : 1
> /sys/devices/platform/soc@0/soc@0:pl301@8/devfreq/soc@0:pl301@8
>      From  :   To
>            :  25000000 133333333   time(ms)
> *  25000000:         0         0     31316
>   133333333:         0         0         0
> Total transition : 0
> /sys/devices/platform/soc@0/soc@0:pl301@9/devfreq/soc@0:pl301@9
>      From  :   To
>            :  25000000 133333333 266666666   time(ms)
>    25000000:         0         0         5      1052
>   133333333:         0         0         0         0
> * 266666666:         5         0         0     30268
> Total transition : 10
> 
> 
> but with display off (mxsfb not requesting anything), I get the same
> fast freqs for noc and memory-controller. They should use the lowest
> freqs. Only pl301@4 switches to 25mhz in that case. That's odd.
> 

Well, have a look at: 

/sys/devices/platform/soc@0/soc@0:pl301@9/devfreq/soc@0:pl301@9

even in the output you gave here, you can see that there are 5
transisions between 25MHz and 266MHz. BTW, that is the USDHC pl301.

I'm assuming you're booting with rootfs from usdhc not through nfs,
right? Anyway, the noc and dram clocks rate only drop when there is
no user enabling its own icc path to the dram.

Keep in mind that the benefit of this approach is not only to drop the
dram clock rate, but also to drop the rates of all the bus clocks on
whenever possible. 

Yes, the perfect scenario would be, from power consumption point of view at least,
have dram clock rate as low as possible and as long as possible, which
implicitly means there is no one requesting the higher rate.

If you want to observe the transitions number change for the dram
devfreq node as well, you can run a simple sync from userspace and that will 
trigger a "high rate" request for the usdhc. Note, this will only happen
if there are no other users asking for the higher rate.

> said (still) out-of-tree mxsfb request is
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.puri.sm%2Fmartin.kepplinger%2Flinux-next%2F-%2Fcommit%2Fee7b1453295932da1e292b734afa7a03651ad9ba&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7C7fab8aca3a5f43d56f5608d983e8da67%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637685858400552603%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=tY9IN8nAgYrPRD0BRLW%2FZbWEps9DTVIQi8G5jY5aw3Q%3D&amp;reserved=0
> 
> and the exact tree I'm running for the above is
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.puri.sm%2Fmartin.kepplinger%2Flinux-next%2F-%2Fcommits%2F5.15-rc3%2Flibrem5__integration_byzantium_test_new_devfreq_interconnect&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7C7fab8aca3a5f43d56f5608d983e8da67%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637685858400552603%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Kpo7sVLdgzwMv8MPX7X%2FNUxoLcvWjFVIuerlh7Cr2D4%3D&amp;reserved=0
> 
> thanks,
> 
>                               martin
>
Martin Kepplinger Oct. 5, 2021, 1:34 p.m. UTC | #5
Am Donnerstag, dem 30.09.2021 um 17:22 +0300 schrieb Abel Vesa:
> On 21-09-30 10:03:46, Martin Kepplinger wrote:
> > Am Mittwoch, dem 29.09.2021 um 14:44 +0300 schrieb Abel Vesa:
> > > On 21-09-24 12:20:26, Martin Kepplinger wrote:
> > > > hi Abel,
> > > > 
> > > > thank you for the update (this is actually v2 of this RFC
> > > > right?)!
> > > > 
> > > > all in all this runs fine on the imx8mq (Librem 5 and devkit) I
> > > > use. For all
> > > > the pl301 nodes I'm not yet sure what I can actually test /
> > > > switch
> > > > frequencies.
> > > > 
> > > 
> > > You can start by looking into each of the following:
> > > 
> > >  $ ls -1d /sys/devices/platform/soc@0/*/devfreq/*/trans_stat
> > > 
> > > and look if the transitions happen when a specific driver that is
> > > a
> > > icc user suspends.
> > > 
> > > You can also look at:
> > > 
> > >  /sys/kernel/debug/interconnect/interconnect_summary 
> > > 
> > > and:
> > > 
> > >  /sys/kernel/debug/interconnect/interconnect_graph
> > > 
> > > > But I still have one problem: lcdif/mxfb already has the
> > > > interconnect dram
> > > > DT property and I use the following call to request bandwidth:
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.puri.sm%2Fmartin.kepplinger%2Flinux-next%2F-%2Fcommit%2Fd690e4c021293f938eb2253607f92f5a64f15688&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7C7fab8aca3a5f43d56f5608d983e8da67%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637685858400552603%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=%2FzyEQdOLU8jQuUpqJ74GTWyfrDvavz%2BxZAgv1tcIu9Y%3D&amp;reserved=0
> > > > (mainlining this is on our todo list).
> > > > 
> > > > With your patchset, I get:
> > > > 
> > > > [    0.792960] genirq: Flags mismatch irq 30. 00000004 (mxsfb-
> > > > drm)
> > > > vs. 00000004 (mxsfb-drm)
> > > > [    0.801143] mxsfb 30320000.lcd-controller: Failed to install
> > > > IRQ
> > > > handler
> > > > [    0.808058] mxsfb: probe of 30320000.lcd-controller failed
> > > > with
> > > > error -16
> > > > 
> > > > so the main devfreq user (mxsfb) is not there :) why?
> > > > 
> > > 
> > > OK, I admit, this patchset doesn't provide support for all the
> > > icc
> > > consumer drivers.
> > > But that should come at a later stage. I only provided example
> > > like
> > > fec and usdhc, to show
> > > how it all fits together.
> > > 
> > > > and when I remove the interconnect property from the lcdif DT
> > > > node,
> > > > mxsfb
> > > > probes again, but of course it doesn't lower dram freq as
> > > > needed.
> > > > 
> > > > Do I do the icc calls wrong in mxsfb despite it working without
> > > > your
> > > > patchset, or may there be something wrong on your side that
> > > > breaks
> > > > the mxsfb IRQ?
> > > > 
> > > 
> > > Do you have the following changes into your tree?
> > > 
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi               
> > > index 00dd8e39a595..c43a84622af5
> > > 100644                                                           
> > >     
> > >            
> > > ---
> > > a/arch/arm64/boot/dts/freescale/imx8mq.dtsi                      
> > >     
> > >                                         
> > > +++
> > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi                      
> > >     
> > >                                         
> > > @@ -524,7 +524,7 @@ lcdif: lcd-controller@30320000
> > > {                                                             
> > >                                                   <&clk
> > > IMX8MQ_VIDEO_PLL1>,                                      
> > >                                                   <&clk
> > > IMX8MQ_VIDEO_PLL1_OUT>;                                  
> > >                                 assigned-clock-rates = <0>, <0>,
> > > <0>,
> > > <594000000>;                               
> > > -                               interconnects = <&noc
> > > IMX8MQ_ICM_LCDIF &noc IMX8MQ_ICS_DRAM>;                    
> > > +                               interconnects = <&icc
> > > IMX8MQ_ICM_LCDIF &icc IMX8MQ_ICS_DRAM>;                    
> > >                                 interconnect-names =
> > > "dram";                                                     
> > >                                 status =
> > > "disabled";                                                      
> > >     
> > >    
> > >                                                                  
> > >     
> > >                                             
> > > @@ -1117,7 +1117,7 @@ mipi_csi1: csi@30a70000
> > > {                                                                
> > >   
> > >                                          <&src
> > > IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET>,                           
> > >                                          <&src
> > > IMX8MQ_RESET_MIPI_CSI1_ESC_RESET>;                               
> > >                                 fsl,mipi-phy-gpr = <&iomuxc_gpr
> > > 0x88>;                                           
> > > -                               interconnects = <&noc
> > > IMX8MQ_ICM_CSI1
> > > &noc IMX8MQ_ICS_DRAM>;                     
> > > +                               interconnects = <&icc
> > > IMX8MQ_ICM_CSI1
> > > &icc IMX8MQ_ICS_DRAM>;                     
> > >                                 interconnect-names =
> > > "dram";                                                     
> > >                                 status =
> > > "disabled";                                                      
> > >     
> > >    
> > >                                                                  
> > >     
> > >                                             
> > > @@ -1169,7 +1169,7 @@ mipi_csi2: csi@30b60000
> > > {                                                                
> > >   
> > >                                          <&src
> > > IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET>,                           
> > >                                          <&src
> > > IMX8MQ_RESET_MIPI_CSI2_ESC_RESET>;                               
> > >                                 fsl,mipi-phy-gpr = <&iomuxc_gpr
> > > 0xa4>;                                           
> > > -                               interconnects = <&noc
> > > IMX8MQ_ICM_CSI2
> > > &noc IMX8MQ_ICS_DRAM>;                     
> > > +                               interconnects = <&icc
> > > IMX8MQ_ICM_CSI2
> > > &icc IMX8MQ_ICS_DRAM>;                     
> > >                                 interconnect-names =
> > > "dram";                                                     
> > >                                 status =
> > > "disabled";                                                      
> > >     
> > >    
> > > 
> > > I forgot to update these in the current version of the patchset.
> > > Will
> > > do in the next version.
> > > 
> > > Also, would help a lot if you could give me a link to a tree
> > > you're
> > > testing with.
> > > That way I can look exactly at what's going on.
> > > 
> > > 
> > 
> > 
> > thanks Abel, with the above fix of existing interconnects
> > properties my
> > system runs as expected and here's the output of
> > 
> > for each in `ls -1d /sys/devices/platform/soc@0/*/devfreq/*`; do
> > echo
> > $each; cat $each/trans_stat; done
> > 
> > for mxsfb requesting (max) bandwith (display on):
> > 
> > /sys/devices/platform/soc@0/32700000.noc/devfreq/32700000.noc
> >      From  :   To
> >            : 133333333 400000000 800000000   time(ms)
> >   133333333:         0         1         0       624
> >   400000000:         0         0         1        28
> > * 800000000:         1         0         0     30624
> > Total transition : 3
> > /sys/devices/platform/soc@0/3d400000.memory-
> > controller/devfreq/3d400000.memory-controller
> >      From  :   To
> >            :  25000000 100000000 800000000   time(ms)
> >    25000000:         0         0         1       620
> >   100000000:         0         0         0         0
> > * 800000000:         1         0         0     30652
> > Total transition : 2
> > /sys/devices/platform/soc@0/soc@0:pl301@0/devfreq/soc@0:pl301@0
> >      From  :   To
> >            :  25000000 133333333 333333333   time(ms)
> >    25000000:         0         0         1       616
> >   133333333:         0         0         0         0
> > * 333333333:         1         0         0     30668
> > Total transition : 2
> > /sys/devices/platform/soc@0/soc@0:pl301@1/devfreq/soc@0:pl301@1
> >      From  :   To
> >            :  25000000 266666666   time(ms)
> > *  25000000:         0         0     31284
> >   266666666:         0         0         0
> > Total transition : 0
> > /sys/devices/platform/soc@0/soc@0:pl301@2/devfreq/soc@0:pl301@2
> >      From  :   To
> >            :  25000000 800000000   time(ms)
> > *  25000000:         0         0     31288
> >   800000000:         1         0         0
> > Total transition : 1
> > /sys/devices/platform/soc@0/soc@0:pl301@3/devfreq/soc@0:pl301@3
> >      From  :   To
> >            :  25000000 800000000   time(ms)
> > *  25000000:         0         0     31292
> >   800000000:         1         0         0
> > Total transition : 1
> > /sys/devices/platform/soc@0/soc@0:pl301@4/devfreq/soc@0:pl301@4
> >      From  :   To
> >            :  25000000 333333333   time(ms)
> >    25000000:         0         1       648
> > * 333333333:         0         0     30652
> > Total transition : 1
> > /sys/devices/platform/soc@0/soc@0:pl301@5/devfreq/soc@0:pl301@5
> >      From  :   To
> >            :  25000000 500000000   time(ms)
> > *  25000000:         0         0     31304
> >   500000000:         1         0         0
> > Total transition : 1
> > /sys/devices/platform/soc@0/soc@0:pl301@6/devfreq/soc@0:pl301@6
> >      From  :   To
> >            :  25000000 500000000   time(ms)
> > *  25000000:         0         0     31308
> >   500000000:         0         0         0
> > Total transition : 0
> > /sys/devices/platform/soc@0/soc@0:pl301@7/devfreq/soc@0:pl301@7
> >      From  :   To
> >            :  25000000 128000000 500000000   time(ms)
> > *  25000000:         0         0         0     31312
> >   128000000:         0         0         0         0
> >   500000000:         1         0         0         0
> > Total transition : 1
> > /sys/devices/platform/soc@0/soc@0:pl301@8/devfreq/soc@0:pl301@8
> >      From  :   To
> >            :  25000000 133333333   time(ms)
> > *  25000000:         0         0     31316
> >   133333333:         0         0         0
> > Total transition : 0
> > /sys/devices/platform/soc@0/soc@0:pl301@9/devfreq/soc@0:pl301@9
> >      From  :   To
> >            :  25000000 133333333 266666666   time(ms)
> >    25000000:         0         0         5      1052
> >   133333333:         0         0         0         0
> > * 266666666:         5         0         0     30268
> > Total transition : 10
> > 
> > 
> > but with display off (mxsfb not requesting anything), I get the
> > same
> > fast freqs for noc and memory-controller. They should use the
> > lowest
> > freqs. Only pl301@4 switches to 25mhz in that case. That's odd.
> > 
> 
> Well, have a look at: 
> 
> /sys/devices/platform/soc@0/soc@0:pl301@9/devfreq/soc@0:pl301@9
> 
> even in the output you gave here, you can see that there are 5
> transisions between 25MHz and 266MHz. BTW, that is the USDHC pl301.
> 
> I'm assuming you're booting with rootfs from usdhc not through nfs,
> right? Anyway, the noc and dram clocks rate only drop when there is
> no user enabling its own icc path to the dram.
> 
> Keep in mind that the benefit of this approach is not only to drop
> the
> dram clock rate, but also to drop the rates of all the bus clocks on
> whenever possible. 
> 
> Yes, the perfect scenario would be, from power consumption point of
> view at least,
> have dram clock rate as low as possible and as long as possible,
> which
> implicitly means there is no one requesting the higher rate.
> 
> If you want to observe the transitions number change for the dram
> devfreq node as well, you can run a simple sync from userspace and
> that will 
> trigger a "high rate" request for the usdhc. Note, this will only
> happen
> if there are no other users asking for the higher rate.


correct, I boot from usdhc. and when I disable interconnect in usdhc
the behaviour actually makes sense. tree:
https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.15-rc4/librem5__integration_byzantium_new_devfreq_interconnect

And I can see the system using a bit less power in the "display off"
case now (and various freqs switching to the lowest).

I didn't yet test whether the new "consumers" (for example usb)
correctly request more bandwidth now.

The only thing I see is with the "display on" case, that
"32700000.interconnect" is switched to 800mhz now, where it used 400mhz
before this patchset. I should be able to find out why though :)

so, for a proof of concept (and after what you mentioned to change for
a next revision) this looks good to me.

thanks a lot!
                            martin