| Message ID | 20190704061403.8249-2-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] arm64: dts: imx8mq: Assign highest opp as suspend opp | expand |
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
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
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.
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 :)
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
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 ?
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
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.
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
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; }; };