[PATCH 1/2] arm64: dts: imx8mq: Assign highest opp as suspend opp

[PATCH 1/2] arm64: dts: imx8mq: Assign highest opp as suspend opp

[Anson.Huang]

From: Anson Huang <Anson.Huang@nxp.com>

Assign highest OPP as suspend OPP to reduce suspend/resume
latency on i.MX8MQ.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
This patch is based on https://patchwork.kernel.org/patch/11023815/
---
 arch/arm64/boot/dts/freescale/imx8mq.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index 3187428..c61e968 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -171,6 +171,7 @@
 			opp-microvolt = <1000000>;
 			opp-supported-hw = <0xc>, <0x4>;
 			clock-latency-ns = <150000>;
+			opp-suspend;
 		};
 
 		opp-1500000000 {
@@ -178,6 +179,7 @@
 			opp-microvolt = <1000000>;
 			opp-supported-hw = <0x8>, <0x3>;
 			clock-latency-ns = <150000>;
+			opp-suspend;
 		};
 	};
 
-- 
2.7.4

[PATCH 2/2] arm64: dts: imx8mm: Assign highest opp as suspend opp

[Anson.Huang]

From: Anson Huang <Anson.Huang@nxp.com>

Assign highest OPP as suspend OPP to reduce suspend/resume
latency on i.MX8MM.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
This patch is based on https://patchwork.kernel.org/patch/11023813/
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index b11fc5e..3a62407 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -136,6 +136,7 @@
 			opp-microvolt = <1000000>;
 			opp-supported-hw = <0x8>, <0x3>;
 			clock-latency-ns = <150000>;
+			opp-suspend;
 		};
 	};
 
-- 
2.7.4

Re: [PATCH 2/2] arm64: dts: imx8mm: Assign highest opp as suspend opp

[Leonard Crestez]

On 7/4/2019 9:23 AM, Anson.Huang@nxp.com wrote:
> From: Anson Huang <Anson.Huang@nxp.com>
> 
> Assign highest OPP as suspend OPP to reduce suspend/resume
> latency on i.MX8MM.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>   arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index b11fc5e..3a62407 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -136,6 +136,7 @@
>   			opp-microvolt = <1000000>;
>   			opp-supported-hw = <0x8>, <0x3>;
>   			clock-latency-ns = <150000>;
> +			opp-suspend;
>   		};
>   	};

What if the highest OPP is unavailable due to speed grading? Ideally we 
should find a way to suspend at the highest *supported* OPP.

Maybe the opp-suspend marking could be assigned from imx-cpufreq-dt 
driver code?

--
Regards,
Leonard

RE: [PATCH 2/2] arm64: dts: imx8mm: Assign highest opp as suspend opp

[Anson Huang]

Hi, Leonard

> On 7/4/2019 9:23 AM, Anson.Huang@nxp.com wrote:
> > From: Anson Huang <Anson.Huang@nxp.com>
> >
> > Assign highest OPP as suspend OPP to reduce suspend/resume latency on
> > i.MX8MM.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >   arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > index b11fc5e..3a62407 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > @@ -136,6 +136,7 @@
> >   			opp-microvolt = <1000000>;
> >   			opp-supported-hw = <0x8>, <0x3>;
> >   			clock-latency-ns = <150000>;
> > +			opp-suspend;
> >   		};
> >   	};
> 
> What if the highest OPP is unavailable due to speed grading? Ideally we
> should find a way to suspend at the highest *supported* OPP.
> 
> Maybe the opp-suspend marking could be assigned from imx-cpufreq-dt
> driver code?

Yes, this is also my concern, the current OPP driver does NOT handle it well, and
I was thinking to assigne it from imx-cpufreq-dt driver, 1 option is to runtime add
"suspend-opp" property into DT OPP node after parsing the speed grading fuse and
OPP table, but I do NOT like that very much, as we need to manually create a property,
the other option is to change cpu freq policy inside imx-cpufreq-dt driver in suspend/resume
callback? Which one do you prefer?

Thanks,
Anson

> 
> --
> Regards,
> Leonard

RE: [PATCH 2/2] arm64: dts: imx8mm: Assign highest opp as suspend opp

[Anson Huang]

Hi, Leonard

> > On 7/4/2019 9:23 AM, Anson.Huang@nxp.com wrote:
> > > From: Anson Huang <Anson.Huang@nxp.com>
> > >
> > > Assign highest OPP as suspend OPP to reduce suspend/resume latency
> > > on i.MX8MM.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > ---
> > >   arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 +
> > >   1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > index b11fc5e..3a62407 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > @@ -136,6 +136,7 @@
> > >   			opp-microvolt = <1000000>;
> > >   			opp-supported-hw = <0x8>, <0x3>;
> > >   			clock-latency-ns = <150000>;
> > > +			opp-suspend;
> > >   		};
> > >   	};
> >
> > What if the highest OPP is unavailable due to speed grading? Ideally
> > we should find a way to suspend at the highest *supported* OPP.
> >
> > Maybe the opp-suspend marking could be assigned from imx-cpufreq-dt
> > driver code?
> 
> Yes, this is also my concern, the current OPP driver does NOT handle it well,
> and I was thinking to assigne it from imx-cpufreq-dt driver, 1 option is to
> runtime add "suspend-opp" property into DT OPP node after parsing the
> speed grading fuse and OPP table, but I do NOT like that very much, as we
> need to manually create a property, the other option is to change cpu freq
> policy inside imx-cpufreq-dt driver in suspend/resume callback? Which one
> do you prefer?

After looking through the cpufreq driver, I think we can use below late init function
to assign the suspend_freq, then no need to add "opp-suspend " in DT, and the freq
get from cpufreq_quick_get_max() is already the max supported freq considering the
speed grading and market segment fuse settings, please ignore these 2 patches, I will
send out a imx-cpufreq-dt.c patch with below change to support all SoCs with imx-cpufreq-dt
driver.

+static int __init imx_cpufreq_dt_setup_suspend_opp(void)
+{
+       struct cpufreq_policy *policy = cpufreq_cpu_get(0);
+
+       policy->suspend_freq = cpufreq_quick_get_max(0);
+
+       return 0;
+}
+late_initcall(imx_cpufreq_dt_setup_suspend_opp);

Anson.

Re: [PATCH 2/2] arm64: dts: imx8mm: Assign highest opp as suspend opp

[Viresh Kumar]

On 04-07-19, 07:49, Leonard Crestez wrote:
> On 7/4/2019 9:23 AM, Anson.Huang@nxp.com wrote:
> > From: Anson Huang <Anson.Huang@nxp.com>
> > 
> > Assign highest OPP as suspend OPP to reduce suspend/resume
> > latency on i.MX8MM.
> > 
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >   arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > index b11fc5e..3a62407 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > @@ -136,6 +136,7 @@
> >   			opp-microvolt = <1000000>;
> >   			opp-supported-hw = <0x8>, <0x3>;
> >   			clock-latency-ns = <150000>;
> > +			opp-suspend;
> >   		};
> >   	};
> 
> What if the highest OPP is unavailable due to speed grading?

What does this exactly mean ? How is the OPP made unavailable in your
case ?

What will dev_pm_opp_get_suspend_opp_freq() return in this case ?

> Ideally we 
> should find a way to suspend at the highest *supported* OPP.
> 
> Maybe the opp-suspend marking could be assigned from imx-cpufreq-dt 
> driver code?

Sorry for jumping in late, the latest patch from Anson drew my
attention to this topic :)

-- 
viresh

RE: [PATCH 2/2] arm64: dts: imx8mm: Assign highest opp as suspend opp

[Anson Huang]

Hi, Viresh

> On 04-07-19, 07:49, Leonard Crestez wrote:
> > On 7/4/2019 9:23 AM, Anson.Huang@nxp.com wrote:
> > > From: Anson Huang <Anson.Huang@nxp.com>
> > >
> > > Assign highest OPP as suspend OPP to reduce suspend/resume latency
> > > on i.MX8MM.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > ---
> > >   arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 +
> > >   1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > index b11fc5e..3a62407 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > @@ -136,6 +136,7 @@
> > >   			opp-microvolt = <1000000>;
> > >   			opp-supported-hw = <0x8>, <0x3>;
> > >   			clock-latency-ns = <150000>;
> > > +			opp-suspend;
> > >   		};
> > >   	};
> >
> > What if the highest OPP is unavailable due to speed grading?
> 
> What does this exactly mean ? How is the OPP made unavailable in your
> case ?

That is because in i.MX8M series SoCs, the speed grading and market segment
fuses settings could affect the OPP defined in DT, in a word, all possible OPPs
are defined in DT, but each parts could ONLY select some of them to be working
OPPs, so if the "opp-suspend" is added for 1 OPP in DT, if the part's speed grading or
market segment fuse settings make that OPP as unavailable,  then that "opp-suspend"
is NOT working at all.

> 
> What will dev_pm_opp_get_suspend_opp_freq() return in this case ?

If the OPP contains "opp-suspend" property is NOT supported by the HW, then there will
be no suspend OPP defined, so it will return 0. The _opp_is_supported() parses the opp-supported-hw
before opp-suspend.

> 
> > Ideally we
> > should find a way to suspend at the highest *supported* OPP.
> >
> > Maybe the opp-suspend marking could be assigned from imx-cpufreq-dt
> > driver code?

I ever tried that, go through the OPP table and check the fuse settings, then runtime add "opp-suspend"
to the opp table, but unfortunately, the " struct opp_table " is NOT opened to be used, it is a private
structure?

> 
> Sorry for jumping in late, the latest patch from Anson drew my attention to
> this topic :)

That is OK😊

Thanks,
Anson.

Re: [PATCH 2/2] arm64: dts: imx8mm: Assign highest opp as suspend opp

[Viresh Kumar]

On 08-07-19, 08:43, Anson Huang wrote:
> Hi, Viresh
> 
> > On 04-07-19, 07:49, Leonard Crestez wrote:
> > > On 7/4/2019 9:23 AM, Anson.Huang@nxp.com wrote:
> > > > From: Anson Huang <Anson.Huang@nxp.com>
> > > >
> > > > Assign highest OPP as suspend OPP to reduce suspend/resume latency
> > > > on i.MX8MM.
> > > >
> > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > > ---
> > > >   arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 +
> > > >   1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > index b11fc5e..3a62407 100644
> > > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > @@ -136,6 +136,7 @@
> > > >   			opp-microvolt = <1000000>;
> > > >   			opp-supported-hw = <0x8>, <0x3>;
> > > >   			clock-latency-ns = <150000>;
> > > > +			opp-suspend;
> > > >   		};
> > > >   	};
> > >
> > > What if the highest OPP is unavailable due to speed grading?
> > 
> > What does this exactly mean ? How is the OPP made unavailable in your
> > case ?
> 
> That is because in i.MX8M series SoCs, the speed grading and market segment
> fuses settings could affect the OPP defined in DT, in a word, all possible OPPs
> are defined in DT, but each parts could ONLY select some of them to be working
> OPPs, so if the "opp-suspend" is added for 1 OPP in DT, if the part's speed grading or
> market segment fuse settings make that OPP as unavailable,  then that "opp-suspend"
> is NOT working at all.

How is this selection done ? You using some OPP helper or something
else ?

-- 
viresh

RE: [PATCH 2/2] arm64: dts: imx8mm: Assign highest opp as suspend opp

[Anson Huang]

Hi, Viresh

> On 08-07-19, 08:43, Anson Huang wrote:
> > Hi, Viresh
> >
> > > On 04-07-19, 07:49, Leonard Crestez wrote:
> > > > On 7/4/2019 9:23 AM, Anson.Huang@nxp.com wrote:
> > > > > From: Anson Huang <Anson.Huang@nxp.com>
> > > > >
> > > > > Assign highest OPP as suspend OPP to reduce suspend/resume
> > > > > latency on i.MX8MM.
> > > > >
> > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > > > ---
> > > > >   arch/arm64/boot/dts/freescale/imx8mm.dtsi | 1 +
> > > > >   1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > > b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > > index b11fc5e..3a62407 100644
> > > > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> > > > > @@ -136,6 +136,7 @@
> > > > >   			opp-microvolt = <1000000>;
> > > > >   			opp-supported-hw = <0x8>, <0x3>;
> > > > >   			clock-latency-ns = <150000>;
> > > > > +			opp-suspend;
> > > > >   		};
> > > > >   	};
> > > >
> > > > What if the highest OPP is unavailable due to speed grading?
> > >
> > > What does this exactly mean ? How is the OPP made unavailable in
> > > your case ?
> >
> > That is because in i.MX8M series SoCs, the speed grading and market
> > segment fuses settings could affect the OPP defined in DT, in a word,
> > all possible OPPs are defined in DT, but each parts could ONLY select
> > some of them to be working OPPs, so if the "opp-suspend" is added for
> > 1 OPP in DT, if the part's speed grading or market segment fuse settings
> make that OPP as unavailable,  then that "opp-suspend"
> > is NOT working at all.
> 
> How is this selection done ? You using some OPP helper or something else ?

Each OPP has "opp-supported-hw" property as below, the first value needs to be
checked with speed grading fuse, and the second one needs to be checked with
market segment fuse, ONLY both of them passed, then this OPP is supported. It
calls dev_pm_opp_set_supported_hw() to tell OPP framework to parse the OPP
table, this is my understanding.

opp-supported-hw = <0x8>, <0x3>;

thanks,
Anson

Re: [PATCH 2/2] arm64: dts: imx8mm: Assign highest opp as suspend opp

[Viresh Kumar]

On 08-07-19, 08:54, Anson Huang wrote:
> Each OPP has "opp-supported-hw" property as below, the first value needs to be
> checked with speed grading fuse, and the second one needs to be checked with
> market segment fuse, ONLY both of them passed, then this OPP is supported. It
> calls dev_pm_opp_set_supported_hw() to tell OPP framework to parse the OPP
> table, this is my understanding.
> 
> opp-supported-hw = <0x8>, <0x3>;

Right, so that's what I was expecting.

One thing we can do is change the binding of OPP core a bit to allow
multiple OPP nodes to contain the "opp-suspend" property and select
the one finally with the highest frequency. That would be a better as
a generic solution IMO.

And then a small OPP core patch will fix it.

-- 
viresh

RE: [PATCH 2/2] arm64: dts: imx8mm: Assign highest opp as suspend opp

[Anson Huang]

Hi, Viresh

> On 08-07-19, 08:54, Anson Huang wrote:
> > Each OPP has "opp-supported-hw" property as below, the first value
> > needs to be checked with speed grading fuse, and the second one needs
> > to be checked with market segment fuse, ONLY both of them passed, then
> > this OPP is supported. It calls dev_pm_opp_set_supported_hw() to tell
> > OPP framework to parse the OPP table, this is my understanding.
> >
> > opp-supported-hw = <0x8>, <0x3>;
> 
> Right, so that's what I was expecting.
> 
> One thing we can do is change the binding of OPP core a bit to allow multiple
> OPP nodes to contain the "opp-suspend" property and select the one finally
> with the highest frequency. That would be a better as a generic solution IMO.
> 
> And then a small OPP core patch will fix it.

Looks good, I will try to generate a patch for of OPP core.

Thanks,
Anson