| Message ID | 1545039990-19984-9-git-send-email-jorge.ramirez-ortiz@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Support CPU frequency scaling on QCS404 | expand |
On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote: > When COMMON_CLK_DISABLED_UNUSED is set, in an effort to save power and > to keep the software model of the clock in line with reality, the > framework transverses the clock tree and disables those clocks that > were enabled by the firmware but have not been enabled by any device > driver. > > If CPUFREQ is enabled, early during the system boot, it might attempt > to change the CPU frequency ("set_rate"). If the HFPLL is selected as > a provider, it will then change the rate for this clock. > > As boot continues, clk_disable_unused_subtree will run. Since it wont > find a valid counter (enable_count) for a clock that is actually > enabled it will attempt to disable it which will cause the CPU to > stop. Notice that in this driver, calls to check whether the clock is > enabled are routed via the is_enabled callback which queries the > hardware. > With the CPUFREQ referencing the CPU clock driver, that has decided to run off this clock, why is it not refcounted? Regards, Bjorn > The following commit, rather than marking the clock critical and > forcing the clock to be always enabled, addresses the above scenario > making sure the clock is not disabled but it continues to rely on the > firmware to enable the clock. > > Co-developed-by: Niklas Cassel <niklas.cassel@linaro.org> > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> > --- > drivers/clk/qcom/hfpll.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c > index 0ffed0d..9d92f5d 100644 > --- a/drivers/clk/qcom/hfpll.c > +++ b/drivers/clk/qcom/hfpll.c > @@ -58,6 +58,7 @@ static int qcom_hfpll_probe(struct platform_device *pdev) > .parent_names = (const char *[]){ "xo" }, > .num_parents = 1, > .ops = &clk_ops_hfpll, > + .flags = CLK_IGNORE_UNUSED, > }; > > h = devm_kzalloc(dev, sizeof(*h), GFP_KERNEL); > -- > 2.7.4 >
On 1/17/19 07:33, Bjorn Andersson wrote: > On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote: > >> When COMMON_CLK_DISABLED_UNUSED is set, in an effort to save power and >> to keep the software model of the clock in line with reality, the >> framework transverses the clock tree and disables those clocks that >> were enabled by the firmware but have not been enabled by any device >> driver. >> >> If CPUFREQ is enabled, early during the system boot, it might attempt >> to change the CPU frequency ("set_rate"). If the HFPLL is selected as >> a provider, it will then change the rate for this clock. >> >> As boot continues, clk_disable_unused_subtree will run. Since it wont >> find a valid counter (enable_count) for a clock that is actually >> enabled it will attempt to disable it which will cause the CPU to >> stop. Notice that in this driver, calls to check whether the clock is >> enabled are routed via the is_enabled callback which queries the >> hardware. >> > > With the CPUFREQ referencing the CPU clock driver, that has decided to > run off this clock, why is it not refcounted? COMMON_CLK_DISABLED_UNUSED relies on the enable_count reference counter to disable the clocks that were enabled by the firwmare and not by the drivers. the cpufreq driver does not enable the cpu clock. so when clk_change_rate is called, the enable_count counter is not incremented and therefore it just remains null since this was enabled by the firmware. I tried doing: diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index e58bfcb..5a9f83e 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -124,6 +124,10 @@ static int resources_available(void) return ret; } + ret = clk_prepare_enable(cpu_clk); + if (ret) + return ret; + clk_put(cpu_clk); name = find_supply_name(cpu_dev); and that removed the need for CLK_IGNORE_UNUSED. But I am not sure of the system wide consequences of that change to cpufreq. maybe Viresh can comment? > > Regards, > Bjorn > >> The following commit, rather than marking the clock critical and >> forcing the clock to be always enabled, addresses the above scenario >> making sure the clock is not disabled but it continues to rely on the >> firmware to enable the clock. >> >> Co-developed-by: Niklas Cassel <niklas.cassel@linaro.org> >> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> >> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> >> --- >> drivers/clk/qcom/hfpll.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c >> index 0ffed0d..9d92f5d 100644 >> --- a/drivers/clk/qcom/hfpll.c >> +++ b/drivers/clk/qcom/hfpll.c >> @@ -58,6 +58,7 @@ static int qcom_hfpll_probe(struct platform_device *pdev) >> .parent_names = (const char *[]){ "xo" }, >> .num_parents = 1, >> .ops = &clk_ops_hfpll, >> + .flags = CLK_IGNORE_UNUSED, >> }; >> >> h = devm_kzalloc(dev, sizeof(*h), GFP_KERNEL); >> -- >> 2.7.4 >> >
On 17-01-19, 09:38, Jorge Ramirez wrote: > COMMON_CLK_DISABLED_UNUSED relies on the enable_count reference counter > to disable the clocks that were enabled by the firwmare and not by the > drivers. > > the cpufreq driver does not enable the cpu clock. > > so when clk_change_rate is called, the enable_count counter is not > incremented and therefore it just remains null since this was enabled by > the firmware. > > I tried doing: > > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > index e58bfcb..5a9f83e 100644 > --- a/drivers/cpufreq/cpufreq-dt.c > +++ b/drivers/cpufreq/cpufreq-dt.c > @@ -124,6 +124,10 @@ static int resources_available(void) > return ret; > } > > + ret = clk_prepare_enable(cpu_clk); > + if (ret) > + return ret; > + > clk_put(cpu_clk); > > name = find_supply_name(cpu_dev); > > > and that removed the need for CLK_IGNORE_UNUSED. But I am not sure of > the system wide consequences of that change to cpufreq. If the cpufreq driver enables it then it should disable it on exit as well, right ? And in that case if you unload your driver's module, you will hang the system as the clock will get disabled :) Every other platform must either be marking it with CLK_IGNORE_UNUSED or they must be doing clk_enable from somewhere, maybe the CPU online path, not sure though.
On 1/17/19 11:08, Viresh Kumar wrote: > On 17-01-19, 09:38, Jorge Ramirez wrote: >> COMMON_CLK_DISABLED_UNUSED relies on the enable_count reference counter >> to disable the clocks that were enabled by the firwmare and not by the >> drivers. >> >> the cpufreq driver does not enable the cpu clock. >> >> so when clk_change_rate is called, the enable_count counter is not >> incremented and therefore it just remains null since this was enabled by >> the firmware. >> >> I tried doing: >> >> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c >> index e58bfcb..5a9f83e 100644 >> --- a/drivers/cpufreq/cpufreq-dt.c >> +++ b/drivers/cpufreq/cpufreq-dt.c >> @@ -124,6 +124,10 @@ static int resources_available(void) >> return ret; >> } >> >> + ret = clk_prepare_enable(cpu_clk); >> + if (ret) >> + return ret; >> + >> clk_put(cpu_clk); >> >> name = find_supply_name(cpu_dev); >> >> >> and that removed the need for CLK_IGNORE_UNUSED. But I am not sure of >> the system wide consequences of that change to cpufreq. > > If the cpufreq driver enables it then it should disable it on exit as > well, right ? And in that case if you unload your driver's module, you > will hang the system as the clock will get disabled :) ah, of course, sorry was over-thinking this thing :) > > Every other platform must either be marking it with CLK_IGNORE_UNUSED > or they must be doing clk_enable from somewhere, maybe the CPU online > path, not sure though. > since this clock is enabled by the firmware, it seems to me that using CLK_IGNORE_UNUSED remains the best option.
Quoting Jorge Ramirez (2019-01-17 02:46:21) > On 1/17/19 11:08, Viresh Kumar wrote: > > On 17-01-19, 09:38, Jorge Ramirez wrote: > >> COMMON_CLK_DISABLED_UNUSED relies on the enable_count reference counter > >> to disable the clocks that were enabled by the firwmare and not by the > >> drivers. > >> > >> the cpufreq driver does not enable the cpu clock. > >> > >> so when clk_change_rate is called, the enable_count counter is not > >> incremented and therefore it just remains null since this was enabled by > >> the firmware. > >> > >> I tried doing: > >> > >> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > >> index e58bfcb..5a9f83e 100644 > >> --- a/drivers/cpufreq/cpufreq-dt.c > >> +++ b/drivers/cpufreq/cpufreq-dt.c > >> @@ -124,6 +124,10 @@ static int resources_available(void) > >> return ret; > >> } > >> > >> + ret = clk_prepare_enable(cpu_clk); > >> + if (ret) > >> + return ret; > >> + > >> clk_put(cpu_clk); > >> > >> name = find_supply_name(cpu_dev); > >> > >> > >> and that removed the need for CLK_IGNORE_UNUSED. But I am not sure of > >> the system wide consequences of that change to cpufreq. > > > > If the cpufreq driver enables it then it should disable it on exit as > > well, right ? And in that case if you unload your driver's module, you > > will hang the system as the clock will get disabled :) > > ah, of course, sorry was over-thinking this thing :) > > > > > Every other platform must either be marking it with CLK_IGNORE_UNUSED > > or they must be doing clk_enable from somewhere, maybe the CPU online > > path, not sure though. > > > > since this clock is enabled by the firmware, it seems to me that using > CLK_IGNORE_UNUSED remains the best option. > What do you do about CPUs being offlined? Presumably when the CPU is gone the system doesn't need to keep the clk enabled anymore.
On 1/22/19 19:47, Stephen Boyd wrote: > Quoting Jorge Ramirez (2019-01-17 02:46:21) >> On 1/17/19 11:08, Viresh Kumar wrote: >>> On 17-01-19, 09:38, Jorge Ramirez wrote: >>>> COMMON_CLK_DISABLED_UNUSED relies on the enable_count reference counter >>>> to disable the clocks that were enabled by the firwmare and not by the >>>> drivers. >>>> >>>> the cpufreq driver does not enable the cpu clock. >>>> >>>> so when clk_change_rate is called, the enable_count counter is not >>>> incremented and therefore it just remains null since this was enabled by >>>> the firmware. >>>> >>>> I tried doing: >>>> >>>> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c >>>> index e58bfcb..5a9f83e 100644 >>>> --- a/drivers/cpufreq/cpufreq-dt.c >>>> +++ b/drivers/cpufreq/cpufreq-dt.c >>>> @@ -124,6 +124,10 @@ static int resources_available(void) >>>> return ret; >>>> } >>>> >>>> + ret = clk_prepare_enable(cpu_clk); >>>> + if (ret) >>>> + return ret; >>>> + >>>> clk_put(cpu_clk); >>>> >>>> name = find_supply_name(cpu_dev); >>>> >>>> >>>> and that removed the need for CLK_IGNORE_UNUSED. But I am not sure of >>>> the system wide consequences of that change to cpufreq. >>> >>> If the cpufreq driver enables it then it should disable it on exit as >>> well, right ? And in that case if you unload your driver's module, you >>> will hang the system as the clock will get disabled :) >> >> ah, of course, sorry was over-thinking this thing :) >> >>> >>> Every other platform must either be marking it with CLK_IGNORE_UNUSED >>> or they must be doing clk_enable from somewhere, maybe the CPU online >>> path, not sure though. >>> >> >> since this clock is enabled by the firmware, it seems to me that using >> CLK_IGNORE_UNUSED remains the best option. >> > > What do you do about CPUs being offlined? Presumably when the CPU is > gone the system doesn't need to keep the clk enabled anymore. > > the kernel does not take any action - it is under firmware control; and since the clock is shared between all cores I there will have to be some involvement not only at TF-A/firmware level but also at the TrustedOS level for when the last core is offlined. I dont have visibility on either of those projects though.
diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c index 0ffed0d..9d92f5d 100644 --- a/drivers/clk/qcom/hfpll.c +++ b/drivers/clk/qcom/hfpll.c @@ -58,6 +58,7 @@ static int qcom_hfpll_probe(struct platform_device *pdev) .parent_names = (const char *[]){ "xo" }, .num_parents = 1, .ops = &clk_ops_hfpll, + .flags = CLK_IGNORE_UNUSED, }; h = devm_kzalloc(dev, sizeof(*h), GFP_KERNEL);
When COMMON_CLK_DISABLED_UNUSED is set, in an effort to save power and to keep the software model of the clock in line with reality, the framework transverses the clock tree and disables those clocks that were enabled by the firmware but have not been enabled by any device driver. If CPUFREQ is enabled, early during the system boot, it might attempt to change the CPU frequency ("set_rate"). If the HFPLL is selected as a provider, it will then change the rate for this clock. As boot continues, clk_disable_unused_subtree will run. Since it wont find a valid counter (enable_count) for a clock that is actually enabled it will attempt to disable it which will cause the CPU to stop. Notice that in this driver, calls to check whether the clock is enabled are routed via the is_enabled callback which queries the hardware. The following commit, rather than marking the clock critical and forcing the clock to be always enabled, addresses the above scenario making sure the clock is not disabled but it continues to rely on the firmware to enable the clock. Co-developed-by: Niklas Cassel <niklas.cassel@linaro.org> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> --- drivers/clk/qcom/hfpll.c | 1 + 1 file changed, 1 insertion(+)