| Message ID | 264124e14b966a1bbc07c364fbd89fc55aa765e6.1527244201.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Viresh, Am Freitag, den 25.05.2018, 16:02 +0530 schrieb Viresh Kumar: > The OPP properties, like "operating-points", should either be present > for all the CPUs of a cluster or none. If these are present only for a > subset of CPUs of a cluster then things will start falling apart as soon > as the CPUs are brought online in a different order. For example, this > will happen because the operating system looks for such properties in > the CPU node it is trying to bring up, so that it can create an OPP > table. > > Add such missing properties. > > Fix other missing properties (like clocks, supply, clock latency) as > well to make it all work. This is a lot of duplicate information for what is effectively a shared cluster wide thing. This does absolutely not _feel_ right. What problem are you solving here? Why do we need all this duplicate information? Why can't we fix it by falling back to looking at cpu0 if needed? Regards, Lucas > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > arch/arm/boot/dts/imx6dl.dtsi | 23 ++++++++++ > arch/arm/boot/dts/imx6q-cm-fx6.dts | 66 +++++++++++++++++++++++++++++ > arch/arm/boot/dts/imx6q.dtsi | 87 ++++++++++++++++++++++++++++++++++++-- > arch/arm/boot/dts/imx7d.dtsi | 5 +++ > 4 files changed, 178 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/boot/dts/imx6dl.dtsi b/arch/arm/boot/dts/imx6dl.dtsi > index b384913c34dd..cc8ffc42d128 100644 > --- a/arch/arm/boot/dts/imx6dl.dtsi > +++ b/arch/arm/boot/dts/imx6dl.dtsi > @@ -50,6 +50,29 @@ > > device_type = "cpu"; > > reg = <1>; > > next-level-cache = <&L2>; > > + operating-points = < > > + /* kHz uV */ > > + 996000 1250000 > > + 792000 1175000 > > + 396000 1150000 > > + >; > > + fsl,soc-operating-points = < > > + /* ARM kHz SOC-PU uV */ > > > + 996000 1175000 > > > + 792000 1175000 > > > + 396000 1175000 > > + >; > > + clock-latency = <61036>; /* two CLK32 periods */ > > + clocks = <&clks IMX6QDL_CLK_ARM>, > > + <&clks IMX6QDL_CLK_PLL2_PFD2_396M>, > > + <&clks IMX6QDL_CLK_STEP>, > > + <&clks IMX6QDL_CLK_PLL1_SW>, > > + <&clks IMX6QDL_CLK_PLL1_SYS>; > > + clock-names = "arm", "pll2_pfd2_396m", "step", > > + "pll1_sw", "pll1_sys"; > > + arm-supply = <®_arm>; > > + pu-supply = <®_pu>; > > + soc-supply = <®_soc>; > > }; > > }; > > diff --git a/arch/arm/boot/dts/imx6q-cm-fx6.dts b/arch/arm/boot/dts/imx6q-cm-fx6.dts > index 65ef4cacbc71..18ae4f3be6e3 100644 > --- a/arch/arm/boot/dts/imx6q-cm-fx6.dts > +++ b/arch/arm/boot/dts/imx6q-cm-fx6.dts > @@ -187,6 +187,72 @@ > > >; > }; > > +&cpu1 { > > + /* > > + * Although the imx6q fuse indicates that 1.2GHz operation is possible, > > + * the module behaves unstable at this frequency. Hence, remove the > > + * 1.2GHz operation point here. > > + */ > > + operating-points = < > > > + /* kHz uV */ > > > + 996000 1250000 > > > + 852000 1250000 > > > + 792000 1175000 > > > + 396000 975000 > > + >; > > + fsl,soc-operating-points = < > > > + /* ARM kHz SOC-PU uV */ > > > + 996000 1250000 > > > + 852000 1250000 > > > + 792000 1175000 > > > + 396000 1175000 > > + >; > +}; > + > +&cpu2 { > > + /* > > + * Although the imx6q fuse indicates that 1.2GHz operation is possible, > > + * the module behaves unstable at this frequency. Hence, remove the > > + * 1.2GHz operation point here. > > + */ > > + operating-points = < > > > + /* kHz uV */ > > > + 996000 1250000 > > > + 852000 1250000 > > > + 792000 1175000 > > > + 396000 975000 > > + >; > > + fsl,soc-operating-points = < > > > + /* ARM kHz SOC-PU uV */ > > > + 996000 1250000 > > > + 852000 1250000 > > > + 792000 1175000 > > > + 396000 1175000 > > + >; > +}; > + > +&cpu3 { > > + /* > > + * Although the imx6q fuse indicates that 1.2GHz operation is possible, > > + * the module behaves unstable at this frequency. Hence, remove the > > + * 1.2GHz operation point here. > > + */ > > + operating-points = < > > > + /* kHz uV */ > > > + 996000 1250000 > > > + 852000 1250000 > > > + 792000 1175000 > > > + 396000 975000 > > + >; > > + fsl,soc-operating-points = < > > > + /* ARM kHz SOC-PU uV */ > > > + 996000 1250000 > > > + 852000 1250000 > > > + 792000 1175000 > > > + 396000 1175000 > > + >; > +}; > + > &ecspi1 { > > cs-gpios = <&gpio2 30 GPIO_ACTIVE_HIGH>, <&gpio3 19 GPIO_ACTIVE_HIGH>; > > pinctrl-names = "default"; > diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi > index 70483ce72ba6..78b89bb1bfed 100644 > --- a/arch/arm/boot/dts/imx6q.dtsi > +++ b/arch/arm/boot/dts/imx6q.dtsi > @@ -50,25 +50,106 @@ > > soc-supply = <®_soc>; > > }; > > > - cpu@1 { > > > + cpu1: cpu@1 { > > compatible = "arm,cortex-a9"; > > device_type = "cpu"; > > reg = <1>; > > next-level-cache = <&L2>; > > + operating-points = < > > + /* kHz uV */ > > + 1200000 1275000 > > + 996000 1250000 > > + 852000 1250000 > > + 792000 1175000 > > + 396000 975000 > > + >; > > + fsl,soc-operating-points = < > > + /* ARM kHz SOC-PU uV */ > > + 1200000 1275000 > > > + 996000 1250000 > > > + 852000 1250000 > > > + 792000 1175000 > > > + 396000 1175000 > > + >; > > + clock-latency = <61036>; /* two CLK32 periods */ > > + clocks = <&clks IMX6QDL_CLK_ARM>, > > + <&clks IMX6QDL_CLK_PLL2_PFD2_396M>, > > + <&clks IMX6QDL_CLK_STEP>, > > + <&clks IMX6QDL_CLK_PLL1_SW>, > > + <&clks IMX6QDL_CLK_PLL1_SYS>; > > + clock-names = "arm", "pll2_pfd2_396m", "step", > > + "pll1_sw", "pll1_sys"; > > + arm-supply = <®_arm>; > > + pu-supply = <®_pu>; > > + soc-supply = <®_soc>; > > }; > > > - cpu@2 { > > > + cpu2: cpu@2 { > > compatible = "arm,cortex-a9"; > > device_type = "cpu"; > > reg = <2>; > > next-level-cache = <&L2>; > > + operating-points = < > > + /* kHz uV */ > > + 1200000 1275000 > > + 996000 1250000 > > + 852000 1250000 > > + 792000 1175000 > > + 396000 975000 > > + >; > > + fsl,soc-operating-points = < > > + /* ARM kHz SOC-PU uV */ > > + 1200000 1275000 > > > + 996000 1250000 > > > + 852000 1250000 > > > + 792000 1175000 > > > + 396000 1175000 > > + >; > > + clock-latency = <61036>; /* two CLK32 periods */ > > + clocks = <&clks IMX6QDL_CLK_ARM>, > > + <&clks IMX6QDL_CLK_PLL2_PFD2_396M>, > > + <&clks IMX6QDL_CLK_STEP>, > > + <&clks IMX6QDL_CLK_PLL1_SW>, > > + <&clks IMX6QDL_CLK_PLL1_SYS>; > > + clock-names = "arm", "pll2_pfd2_396m", "step", > > + "pll1_sw", "pll1_sys"; > > + arm-supply = <®_arm>; > > + pu-supply = <®_pu>; > > + soc-supply = <®_soc>; > > }; > > > - cpu@3 { > > > + cpu3: cpu@3 { > > compatible = "arm,cortex-a9"; > > device_type = "cpu"; > > reg = <3>; > > next-level-cache = <&L2>; > > + operating-points = < > > + /* kHz uV */ > > + 1200000 1275000 > > + 996000 1250000 > > + 852000 1250000 > > + 792000 1175000 > > + 396000 975000 > > + >; > > + fsl,soc-operating-points = < > > + /* ARM kHz SOC-PU uV */ > > + 1200000 1275000 > > > + 996000 1250000 > > > + 852000 1250000 > > > + 792000 1175000 > > > + 396000 1175000 > > + >; > > + clock-latency = <61036>; /* two CLK32 periods */ > > + clocks = <&clks IMX6QDL_CLK_ARM>, > > + <&clks IMX6QDL_CLK_PLL2_PFD2_396M>, > > + <&clks IMX6QDL_CLK_STEP>, > > + <&clks IMX6QDL_CLK_PLL1_SW>, > > + <&clks IMX6QDL_CLK_PLL1_SYS>; > > + clock-names = "arm", "pll2_pfd2_396m", "step", > > + "pll1_sw", "pll1_sys"; > > + arm-supply = <®_arm>; > > + pu-supply = <®_pu>; > > + soc-supply = <®_soc>; > > }; > > }; > > diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi > index 4c9877ec29f2..5434a8aa5602 100644 > --- a/arch/arm/boot/dts/imx7d.dtsi > +++ b/arch/arm/boot/dts/imx7d.dtsi > @@ -21,6 +21,11 @@ > > compatible = "arm,cortex-a7"; > > device_type = "cpu"; > > reg = <1>; > > + operating-points = < > > > + /* KHz uV */ > > > + 996000 1075000 > > > + 792000 975000 > > + >; > > clock-frequency = <996000000>; > > }; > > };
Hi Lucas, On 25-05-18, 13:46, Lucas Stach wrote: > This is a lot of duplicate information for what is effectively a shared > cluster wide thing. This does absolutely not _feel_ right. I cannot agree more :) > What problem are you solving here? Why do we need all this duplicate > information? Why can't we fix it by falling back to looking at cpu0 if > needed? Let me try explaining one of the problem scenarios to you as your platform is a single cluster one. Make cpufreq driver as module, don't insert it, hotplug out CPU0 and now insert the cpufreq driver. The cpufreq core will try adding the cpufreq policy for CPU1 but wouldn't find the required information in the DT node of CPU1 and so will fail or behave incorrectly. We can't look at CPU0 as we don't know they are related at all. Nothing tells that to us. The right solution to fix the duplication is to move to OPP-v2 bindings, which allow us to create a single OPP table node and refer to it from all the CPU nodes. Because in case of imx platforms getting updated here, we use the old and some platforms specific frequency tables, we have to duplicate it everywhere. But looking from DT otherwise, all the device should anyway have all the information required right in their node. That can be simplified with things like phandle to opp-v2 node, but still everything needs to be there. We shouldn't really rely on other CPU nodes to make it work. That would be an incomplete definition of the hardware IMHO.
On Mon, May 28, 2018 at 04:37:39PM +0530, Viresh Kumar wrote: > Hi Lucas, > > On 25-05-18, 13:46, Lucas Stach wrote: > > This is a lot of duplicate information for what is effectively a shared > > cluster wide thing. This does absolutely not _feel_ right. > > I cannot agree more :) > > > What problem are you solving here? Why do we need all this duplicate > > information? Why can't we fix it by falling back to looking at cpu0 if > > needed? > > Let me try explaining one of the problem scenarios to you as your > platform is a single cluster one. Make cpufreq driver as module, don't > insert it, hotplug out CPU0 and now insert the cpufreq driver. The > cpufreq core will try adding the cpufreq policy for CPU1 but wouldn't > find the required information in the DT node of CPU1 and so will fail > or behave incorrectly. > > We can't look at CPU0 as we don't know they are related at all. > Nothing tells that to us. The right solution to fix the duplication is > to move to OPP-v2 bindings, which allow us to create a single OPP > table node and refer to it from all the CPU nodes. Because in case of > imx platforms getting updated here, we use the old and some platforms > specific frequency tables, we have to duplicate it everywhere. > > But looking from DT otherwise, all the device should anyway have all > the information required right in their node. That can be simplified > with things like phandle to opp-v2 node, but still everything needs to > be there. We shouldn't really rely on other CPU nodes to make it work. > That would be an incomplete definition of the hardware IMHO. Lucas, Are you fine with the patch now considering the respond from Viresh? Shawn
Hi Shawn, Am Dienstag, den 03.07.2018, 15:12 +0800 schrieb Shawn Guo: > On Mon, May 28, 2018 at 04:37:39PM +0530, Viresh Kumar wrote: > > Hi Lucas, > > > > On 25-05-18, 13:46, Lucas Stach wrote: > > > This is a lot of duplicate information for what is effectively a shared > > > cluster wide thing. This does absolutely not _feel_ right. > > > > I cannot agree more :) > > > > > What problem are you solving here? Why do we need all this duplicate > > > information? Why can't we fix it by falling back to looking at cpu0 if > > > needed? > > > > Let me try explaining one of the problem scenarios to you as your > > platform is a single cluster one. Make cpufreq driver as module, don't > > insert it, hotplug out CPU0 and now insert the cpufreq driver. The > > cpufreq core will try adding the cpufreq policy for CPU1 but wouldn't > > find the required information in the DT node of CPU1 and so will fail > > or behave incorrectly. > > > > We can't look at CPU0 as we don't know they are related at all. > > Nothing tells that to us. The right solution to fix the duplication is > > to move to OPP-v2 bindings, which allow us to create a single OPP > > table node and refer to it from all the CPU nodes. Because in case of > > imx platforms getting updated here, we use the old and some platforms > > specific frequency tables, we have to duplicate it everywhere. > > > > But looking from DT otherwise, all the device should anyway have all > > the information required right in their node. That can be simplified > > with things like phandle to opp-v2 node, but still everything needs to > > be there. We shouldn't really rely on other CPU nodes to make it work. > > That would be an incomplete definition of the hardware IMHO. > > Lucas, > > Are you fine with the patch now considering the respond from Viresh? I still don't like the huge duplication of information in the DT, but I agree that it's currently necessary for correctness. Long term I hope we can get away from cpufreq-imx6 and the custom SoC OPPs, which would allow us to switch over to OPPv2. So patch is: Reluctantly-acked-by: Lucas Stach <l.stach@pengutronix.de> Regards, Lucas
On Fri, May 25, 2018 at 04:02:01PM +0530, Viresh Kumar wrote: > The OPP properties, like "operating-points", should either be present > for all the CPUs of a cluster or none. If these are present only for a > subset of CPUs of a cluster then things will start falling apart as soon > as the CPUs are brought online in a different order. For example, this > will happen because the operating system looks for such properties in > the CPU node it is trying to bring up, so that it can create an OPP > table. > > Add such missing properties. > > Fix other missing properties (like clocks, supply, clock latency) as > well to make it all work. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Applied, thanks.
diff --git a/arch/arm/boot/dts/imx6dl.dtsi b/arch/arm/boot/dts/imx6dl.dtsi index b384913c34dd..cc8ffc42d128 100644 --- a/arch/arm/boot/dts/imx6dl.dtsi +++ b/arch/arm/boot/dts/imx6dl.dtsi @@ -50,6 +50,29 @@ device_type = "cpu"; reg = <1>; next-level-cache = <&L2>; + operating-points = < + /* kHz uV */ + 996000 1250000 + 792000 1175000 + 396000 1150000 + >; + fsl,soc-operating-points = < + /* ARM kHz SOC-PU uV */ + 996000 1175000 + 792000 1175000 + 396000 1175000 + >; + clock-latency = <61036>; /* two CLK32 periods */ + clocks = <&clks IMX6QDL_CLK_ARM>, + <&clks IMX6QDL_CLK_PLL2_PFD2_396M>, + <&clks IMX6QDL_CLK_STEP>, + <&clks IMX6QDL_CLK_PLL1_SW>, + <&clks IMX6QDL_CLK_PLL1_SYS>; + clock-names = "arm", "pll2_pfd2_396m", "step", + "pll1_sw", "pll1_sys"; + arm-supply = <®_arm>; + pu-supply = <®_pu>; + soc-supply = <®_soc>; }; }; diff --git a/arch/arm/boot/dts/imx6q-cm-fx6.dts b/arch/arm/boot/dts/imx6q-cm-fx6.dts index 65ef4cacbc71..18ae4f3be6e3 100644 --- a/arch/arm/boot/dts/imx6q-cm-fx6.dts +++ b/arch/arm/boot/dts/imx6q-cm-fx6.dts @@ -187,6 +187,72 @@ >; }; +&cpu1 { + /* + * Although the imx6q fuse indicates that 1.2GHz operation is possible, + * the module behaves unstable at this frequency. Hence, remove the + * 1.2GHz operation point here. + */ + operating-points = < + /* kHz uV */ + 996000 1250000 + 852000 1250000 + 792000 1175000 + 396000 975000 + >; + fsl,soc-operating-points = < + /* ARM kHz SOC-PU uV */ + 996000 1250000 + 852000 1250000 + 792000 1175000 + 396000 1175000 + >; +}; + +&cpu2 { + /* + * Although the imx6q fuse indicates that 1.2GHz operation is possible, + * the module behaves unstable at this frequency. Hence, remove the + * 1.2GHz operation point here. + */ + operating-points = < + /* kHz uV */ + 996000 1250000 + 852000 1250000 + 792000 1175000 + 396000 975000 + >; + fsl,soc-operating-points = < + /* ARM kHz SOC-PU uV */ + 996000 1250000 + 852000 1250000 + 792000 1175000 + 396000 1175000 + >; +}; + +&cpu3 { + /* + * Although the imx6q fuse indicates that 1.2GHz operation is possible, + * the module behaves unstable at this frequency. Hence, remove the + * 1.2GHz operation point here. + */ + operating-points = < + /* kHz uV */ + 996000 1250000 + 852000 1250000 + 792000 1175000 + 396000 975000 + >; + fsl,soc-operating-points = < + /* ARM kHz SOC-PU uV */ + 996000 1250000 + 852000 1250000 + 792000 1175000 + 396000 1175000 + >; +}; + &ecspi1 { cs-gpios = <&gpio2 30 GPIO_ACTIVE_HIGH>, <&gpio3 19 GPIO_ACTIVE_HIGH>; pinctrl-names = "default"; diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi index 70483ce72ba6..78b89bb1bfed 100644 --- a/arch/arm/boot/dts/imx6q.dtsi +++ b/arch/arm/boot/dts/imx6q.dtsi @@ -50,25 +50,106 @@ soc-supply = <®_soc>; }; - cpu@1 { + cpu1: cpu@1 { compatible = "arm,cortex-a9"; device_type = "cpu"; reg = <1>; next-level-cache = <&L2>; + operating-points = < + /* kHz uV */ + 1200000 1275000 + 996000 1250000 + 852000 1250000 + 792000 1175000 + 396000 975000 + >; + fsl,soc-operating-points = < + /* ARM kHz SOC-PU uV */ + 1200000 1275000 + 996000 1250000 + 852000 1250000 + 792000 1175000 + 396000 1175000 + >; + clock-latency = <61036>; /* two CLK32 periods */ + clocks = <&clks IMX6QDL_CLK_ARM>, + <&clks IMX6QDL_CLK_PLL2_PFD2_396M>, + <&clks IMX6QDL_CLK_STEP>, + <&clks IMX6QDL_CLK_PLL1_SW>, + <&clks IMX6QDL_CLK_PLL1_SYS>; + clock-names = "arm", "pll2_pfd2_396m", "step", + "pll1_sw", "pll1_sys"; + arm-supply = <®_arm>; + pu-supply = <®_pu>; + soc-supply = <®_soc>; }; - cpu@2 { + cpu2: cpu@2 { compatible = "arm,cortex-a9"; device_type = "cpu"; reg = <2>; next-level-cache = <&L2>; + operating-points = < + /* kHz uV */ + 1200000 1275000 + 996000 1250000 + 852000 1250000 + 792000 1175000 + 396000 975000 + >; + fsl,soc-operating-points = < + /* ARM kHz SOC-PU uV */ + 1200000 1275000 + 996000 1250000 + 852000 1250000 + 792000 1175000 + 396000 1175000 + >; + clock-latency = <61036>; /* two CLK32 periods */ + clocks = <&clks IMX6QDL_CLK_ARM>, + <&clks IMX6QDL_CLK_PLL2_PFD2_396M>, + <&clks IMX6QDL_CLK_STEP>, + <&clks IMX6QDL_CLK_PLL1_SW>, + <&clks IMX6QDL_CLK_PLL1_SYS>; + clock-names = "arm", "pll2_pfd2_396m", "step", + "pll1_sw", "pll1_sys"; + arm-supply = <®_arm>; + pu-supply = <®_pu>; + soc-supply = <®_soc>; }; - cpu@3 { + cpu3: cpu@3 { compatible = "arm,cortex-a9"; device_type = "cpu"; reg = <3>; next-level-cache = <&L2>; + operating-points = < + /* kHz uV */ + 1200000 1275000 + 996000 1250000 + 852000 1250000 + 792000 1175000 + 396000 975000 + >; + fsl,soc-operating-points = < + /* ARM kHz SOC-PU uV */ + 1200000 1275000 + 996000 1250000 + 852000 1250000 + 792000 1175000 + 396000 1175000 + >; + clock-latency = <61036>; /* two CLK32 periods */ + clocks = <&clks IMX6QDL_CLK_ARM>, + <&clks IMX6QDL_CLK_PLL2_PFD2_396M>, + <&clks IMX6QDL_CLK_STEP>, + <&clks IMX6QDL_CLK_PLL1_SW>, + <&clks IMX6QDL_CLK_PLL1_SYS>; + clock-names = "arm", "pll2_pfd2_396m", "step", + "pll1_sw", "pll1_sys"; + arm-supply = <®_arm>; + pu-supply = <®_pu>; + soc-supply = <®_soc>; }; }; diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi index 4c9877ec29f2..5434a8aa5602 100644 --- a/arch/arm/boot/dts/imx7d.dtsi +++ b/arch/arm/boot/dts/imx7d.dtsi @@ -21,6 +21,11 @@ compatible = "arm,cortex-a7"; device_type = "cpu"; reg = <1>; + operating-points = < + /* KHz uV */ + 996000 1075000 + 792000 975000 + >; clock-frequency = <996000000>; }; };
The OPP properties, like "operating-points", should either be present for all the CPUs of a cluster or none. If these are present only for a subset of CPUs of a cluster then things will start falling apart as soon as the CPUs are brought online in a different order. For example, this will happen because the operating system looks for such properties in the CPU node it is trying to bring up, so that it can create an OPP table. Add such missing properties. Fix other missing properties (like clocks, supply, clock latency) as well to make it all work. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- arch/arm/boot/dts/imx6dl.dtsi | 23 ++++++++++ arch/arm/boot/dts/imx6q-cm-fx6.dts | 66 +++++++++++++++++++++++++++++ arch/arm/boot/dts/imx6q.dtsi | 87 ++++++++++++++++++++++++++++++++++++-- arch/arm/boot/dts/imx7d.dtsi | 5 +++ 4 files changed, 178 insertions(+), 3 deletions(-)