| Message ID | 20181130065259.26497-2-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: qcom: gcc-msm8998: Fixes and clkref clocks | expand |
Quoting Bjorn Andersson (2018-11-29 22:52:57) > Keep the two clocks enabled, so that the platform passes > clk_disable_unused(). > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/clk/qcom/gcc-msm8998.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c > index 9f0ae403d5f5..d89f8e7c2a59 100644 > --- a/drivers/clk/qcom/gcc-msm8998.c > +++ b/drivers/clk/qcom/gcc-msm8998.c > @@ -1972,6 +1972,7 @@ static struct clk_branch gcc_hmss_dvm_bus_clk = { > .enable_mask = BIT(0), > .hw.init = &(struct clk_init_data){ > .name = "gcc_hmss_dvm_bus_clk", > + .flags = CLK_IS_CRITICAL, Please add a comment about why they're critical. This is a temporary solution? > .ops = &clk_branch2_ops, > }, > },
On Thu 29 Nov 23:05 PST 2018, Stephen Boyd wrote: > Quoting Bjorn Andersson (2018-11-29 22:52:57) > > Keep the two clocks enabled, so that the platform passes > > clk_disable_unused(). > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > drivers/clk/qcom/gcc-msm8998.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c > > index 9f0ae403d5f5..d89f8e7c2a59 100644 > > --- a/drivers/clk/qcom/gcc-msm8998.c > > +++ b/drivers/clk/qcom/gcc-msm8998.c > > @@ -1972,6 +1972,7 @@ static struct clk_branch gcc_hmss_dvm_bus_clk = { > > .enable_mask = BIT(0), > > .hw.init = &(struct clk_init_data){ > > .name = "gcc_hmss_dvm_bus_clk", > > + .flags = CLK_IS_CRITICAL, > > Please add a comment about why they're critical. This is a temporary > solution? > Disabling them in clk_disable_unused() are bad, mkay... SDM845 marks the equivalent clocks as critical with a comment that they must be on for system operation... I'm uncertain what the exact purpose of these two clocks are, so I don't have a better explanation right now. Regards, Bjorn > > .ops = &clk_branch2_ops, > > }, > > },
Quoting Bjorn Andersson (2018-11-29 23:24:20) > On Thu 29 Nov 23:05 PST 2018, Stephen Boyd wrote: > > > Quoting Bjorn Andersson (2018-11-29 22:52:57) > > > Keep the two clocks enabled, so that the platform passes > > > clk_disable_unused(). > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > --- > > > drivers/clk/qcom/gcc-msm8998.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c > > > index 9f0ae403d5f5..d89f8e7c2a59 100644 > > > --- a/drivers/clk/qcom/gcc-msm8998.c > > > +++ b/drivers/clk/qcom/gcc-msm8998.c > > > @@ -1972,6 +1972,7 @@ static struct clk_branch gcc_hmss_dvm_bus_clk = { > > > .enable_mask = BIT(0), > > > .hw.init = &(struct clk_init_data){ > > > .name = "gcc_hmss_dvm_bus_clk", > > > + .flags = CLK_IS_CRITICAL, > > > > Please add a comment about why they're critical. This is a temporary > > solution? > > > > Disabling them in clk_disable_unused() are bad, mkay... Ugh sad. > > > SDM845 marks the equivalent clocks as critical with a comment that they > must be on for system operation... I'm uncertain what the exact purpose > of these two clocks are, so I don't have a better explanation right now. > Ok. But does any driver ever want to use it? It may make more sense to just remove it entirely and not touch it.
On 30/11/2018 09:12, Stephen Boyd wrote: > Quoting Bjorn Andersson (2018-11-29 23:24:20) > >> On Thu 29 Nov 23:05 PST 2018, Stephen Boyd wrote: >> >>> Quoting Bjorn Andersson (2018-11-29 22:52:57) >>> >>>> Keep the two clocks enabled, so that the platform passes >>>> clk_disable_unused(). >>>> >>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> >>>> --- >>>> drivers/clk/qcom/gcc-msm8998.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c >>>> index 9f0ae403d5f5..d89f8e7c2a59 100644 >>>> --- a/drivers/clk/qcom/gcc-msm8998.c >>>> +++ b/drivers/clk/qcom/gcc-msm8998.c >>>> @@ -1972,6 +1972,7 @@ static struct clk_branch gcc_hmss_dvm_bus_clk = { >>>> .enable_mask = BIT(0), >>>> .hw.init = &(struct clk_init_data){ >>>> .name = "gcc_hmss_dvm_bus_clk", >>>> + .flags = CLK_IS_CRITICAL, >>> >>> Please add a comment about why they're critical. This is a temporary >>> solution? >> >> Disabling them in clk_disable_unused() are bad, mkay... > > Ugh sad. > >> SDM845 marks the equivalent clocks as critical with a comment that they >> must be on for system operation... I'm uncertain what the exact purpose >> of these two clocks are, so I don't have a better explanation right now. > > Ok. But does any driver ever want to use it? It may make more sense to > just remove it entirely and not touch it. FWIW, gcc_hmss_dvm_bus_clk is flagged "always on" downstream: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/drivers/clk/msm/clock-gcc-8998.c?h=LE.UM.1.3.r3.25#n1715 config REGULATOR_CPR3_HMSS bool "CPR3 regulator for HMSS" depends on OF select REGULATOR_CPR3 help This driver supports Qualcomm Technologies, Inc. HMSS application processor specific features including memory array power mux (APM) switching, two CPR3 threads which monitor the two HMSS clusters that are both powered by a shared supply, and hardware closed-loop auto voltage stepping. This driver reads both initial voltage and CPR target quotient values out of hardware fuses. I wasn't able to find the meaning of the HMSS acronym via git grep, pdfgrep, or a web search. It should be forbidden to use an undefined acronyms in bindings documentation, IMHO. I couldn't find gcc_lpass_at_clk in the downstream 4.4 kernel... LPASS = Low Power Audio Subsystem Regards.
On 11/30/2018 2:23 AM, Marc Gonzalez wrote: > On 30/11/2018 09:12, Stephen Boyd wrote: > >> Quoting Bjorn Andersson (2018-11-29 23:24:20) >> >>> On Thu 29 Nov 23:05 PST 2018, Stephen Boyd wrote: >>> >>>> Quoting Bjorn Andersson (2018-11-29 22:52:57) >>>> >>>>> Keep the two clocks enabled, so that the platform passes >>>>> clk_disable_unused(). >>>>> >>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> >>>>> --- >>>>> drivers/clk/qcom/gcc-msm8998.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c >>>>> index 9f0ae403d5f5..d89f8e7c2a59 100644 >>>>> --- a/drivers/clk/qcom/gcc-msm8998.c >>>>> +++ b/drivers/clk/qcom/gcc-msm8998.c >>>>> @@ -1972,6 +1972,7 @@ static struct clk_branch gcc_hmss_dvm_bus_clk = { >>>>> .enable_mask = BIT(0), >>>>> .hw.init = &(struct clk_init_data){ >>>>> .name = "gcc_hmss_dvm_bus_clk", >>>>> + .flags = CLK_IS_CRITICAL, >>>> >>>> Please add a comment about why they're critical. This is a temporary >>>> solution? >>> >>> Disabling them in clk_disable_unused() are bad, mkay... >> >> Ugh sad. >> >>> SDM845 marks the equivalent clocks as critical with a comment that they >>> must be on for system operation... I'm uncertain what the exact purpose >>> of these two clocks are, so I don't have a better explanation right now. >> >> Ok. But does any driver ever want to use it? It may make more sense to >> just remove it entirely and not touch it. > > FWIW, gcc_hmss_dvm_bus_clk is flagged "always on" downstream: > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/drivers/clk/msm/clock-gcc-8998.c?h=LE.UM.1.3.r3.25#n1715 > > > config REGULATOR_CPR3_HMSS > bool "CPR3 regulator for HMSS" > depends on OF > select REGULATOR_CPR3 > help > This driver supports Qualcomm Technologies, Inc. HMSS application > processor specific features including memory array power mux (APM) > switching, two CPR3 threads which monitor the two HMSS clusters that > are both powered by a shared supply, and hardware closed-loop auto > voltage stepping. This driver reads both initial voltage and CPR > target quotient values out of hardware fuses. > > I wasn't able to find the meaning of the HMSS acronym via git grep, pdfgrep, > or a web search. It should be forbidden to use an undefined acronyms in > bindings documentation, IMHO. HMSS = Hydra Monaco SubSystem > > > I couldn't find gcc_lpass_at_clk in the downstream 4.4 kernel... > LPASS = Low Power Audio Subsystem > > Regards. >
diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c index 9f0ae403d5f5..d89f8e7c2a59 100644 --- a/drivers/clk/qcom/gcc-msm8998.c +++ b/drivers/clk/qcom/gcc-msm8998.c @@ -1972,6 +1972,7 @@ static struct clk_branch gcc_hmss_dvm_bus_clk = { .enable_mask = BIT(0), .hw.init = &(struct clk_init_data){ .name = "gcc_hmss_dvm_bus_clk", + .flags = CLK_IS_CRITICAL, .ops = &clk_branch2_ops, }, }, @@ -2015,6 +2016,7 @@ static struct clk_branch gcc_lpass_at_clk = { .enable_mask = BIT(0), .hw.init = &(struct clk_init_data){ .name = "gcc_lpass_at_clk", + .flags = CLK_IS_CRITICAL, .ops = &clk_branch2_ops, }, },
Keep the two clocks enabled, so that the platform passes clk_disable_unused(). Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/clk/qcom/gcc-msm8998.c | 2 ++ 1 file changed, 2 insertions(+)