* [PATCH 0/3] clk: qcom: gcc-msm8998: Fixes and clkref clocks
@ 2018-11-30 6:52 Bjorn Andersson
2018-11-30 6:52 ` [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical Bjorn Andersson
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Bjorn Andersson @ 2018-11-30 6:52 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Andy Gross, David Brown, linux-arm-msm, linux-soc, linux-clk,
linux-kernel, Marc Gonzalez, Amit Kucheria
Mark critical clocks critical, don't halt-check UFS clocks and add clkref
branches.
Bjorn Andersson (3):
clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical
clk: qcom: gcc-msm8998: Disable halt check of UFS clocks
clk: qcom: gcc-msm8998: Add clkref clocks
drivers/clk/qcom/gcc-msm8998.c | 83 +++++++++++++++++++-
include/dt-bindings/clock/qcom,gcc-msm8998.h | 5 ++
2 files changed, 85 insertions(+), 3 deletions(-)
--
2.18.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical
2018-11-30 6:52 [PATCH 0/3] clk: qcom: gcc-msm8998: Fixes and clkref clocks Bjorn Andersson
@ 2018-11-30 6:52 ` Bjorn Andersson
2018-11-30 7:05 ` Stephen Boyd
2018-11-30 6:52 ` [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks Bjorn Andersson
2018-11-30 6:52 ` [PATCH 3/3] clk: qcom: gcc-msm8998: Add clkref clocks Bjorn Andersson
2 siblings, 1 reply; 14+ messages in thread
From: Bjorn Andersson @ 2018-11-30 6:52 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Andy Gross, David Brown, linux-arm-msm, linux-soc, linux-clk,
linux-kernel, Marc Gonzalez, Amit Kucheria
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,
.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,
},
},
--
2.18.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks
2018-11-30 6:52 [PATCH 0/3] clk: qcom: gcc-msm8998: Fixes and clkref clocks Bjorn Andersson
2018-11-30 6:52 ` [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical Bjorn Andersson
@ 2018-11-30 6:52 ` Bjorn Andersson
2018-11-30 7:06 ` Stephen Boyd
2018-11-30 10:55 ` Marc Gonzalez
2018-11-30 6:52 ` [PATCH 3/3] clk: qcom: gcc-msm8998: Add clkref clocks Bjorn Andersson
2 siblings, 2 replies; 14+ messages in thread
From: Bjorn Andersson @ 2018-11-30 6:52 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Andy Gross, David Brown, linux-arm-msm, linux-soc, linux-clk,
linux-kernel, Marc Gonzalez, Amit Kucheria
Drop the halt check of the UFS symbol clocks, in accordance with other
platforms. This makes clk_disable_unused() happy and makes it possible
to turn the clocks on again without an error.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
drivers/clk/qcom/gcc-msm8998.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
index d89f8e7c2a59..3d232d266d72 100644
--- a/drivers/clk/qcom/gcc-msm8998.c
+++ b/drivers/clk/qcom/gcc-msm8998.c
@@ -2403,7 +2403,7 @@ static struct clk_branch gcc_ufs_phy_aux_clk = {
static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
.halt_reg = 0x75014,
- .halt_check = BRANCH_HALT,
+ .halt_check = BRANCH_HALT_SKIP,
.clkr = {
.enable_reg = 0x75014,
.enable_mask = BIT(0),
@@ -2416,7 +2416,7 @@ static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
static struct clk_branch gcc_ufs_rx_symbol_1_clk = {
.halt_reg = 0x7605c,
- .halt_check = BRANCH_HALT,
+ .halt_check = BRANCH_HALT_SKIP,
.clkr = {
.enable_reg = 0x7605c,
.enable_mask = BIT(0),
@@ -2429,7 +2429,7 @@ static struct clk_branch gcc_ufs_rx_symbol_1_clk = {
static struct clk_branch gcc_ufs_tx_symbol_0_clk = {
.halt_reg = 0x75010,
- .halt_check = BRANCH_HALT,
+ .halt_check = BRANCH_HALT_SKIP,
.clkr = {
.enable_reg = 0x75010,
.enable_mask = BIT(0),
--
2.18.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] clk: qcom: gcc-msm8998: Add clkref clocks
2018-11-30 6:52 [PATCH 0/3] clk: qcom: gcc-msm8998: Fixes and clkref clocks Bjorn Andersson
2018-11-30 6:52 ` [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical Bjorn Andersson
2018-11-30 6:52 ` [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks Bjorn Andersson
@ 2018-11-30 6:52 ` Bjorn Andersson
2 siblings, 0 replies; 14+ messages in thread
From: Bjorn Andersson @ 2018-11-30 6:52 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Andy Gross, David Brown, linux-arm-msm, linux-soc, linux-clk,
linux-kernel, Marc Gonzalez, Amit Kucheria
Add clkref clocks for usb3, hdmi, ufs, pcie, and usb2. They are all
sourced off CXO_IN, so parent them off "xo" until a proper link to the
rpmcc can be described in DT.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
drivers/clk/qcom/gcc-msm8998.c | 75 ++++++++++++++++++++
include/dt-bindings/clock/qcom,gcc-msm8998.h | 5 ++
2 files changed, 80 insertions(+)
diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
index 3d232d266d72..3cf16a4f931c 100644
--- a/drivers/clk/qcom/gcc-msm8998.c
+++ b/drivers/clk/qcom/gcc-msm8998.c
@@ -2543,6 +2543,76 @@ static struct clk_branch gcc_usb_phy_cfg_ahb2phy_clk = {
},
};
+static struct clk_branch gcc_hdmi_clkref_clk = {
+ .halt_reg = 0x88000,
+ .clkr = {
+ .enable_reg = 0x88000,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_hdmi_clkref_clk",
+ .parent_names = (const char *[]){ "xo" },
+ .num_parents = 1,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch gcc_ufs_clkref_clk = {
+ .halt_reg = 0x88004,
+ .clkr = {
+ .enable_reg = 0x88004,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_ufs_clkref_clk",
+ .parent_names = (const char *[]){ "xo" },
+ .num_parents = 1,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch gcc_usb3_clkref_clk = {
+ .halt_reg = 0x88008,
+ .clkr = {
+ .enable_reg = 0x88008,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_usb3_clkref_clk",
+ .parent_names = (const char *[]){ "xo" },
+ .num_parents = 1,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch gcc_pcie_clkref_clk = {
+ .halt_reg = 0x8800c,
+ .clkr = {
+ .enable_reg = 0x8800c,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_pcie_clkref_clk",
+ .parent_names = (const char *[]){ "xo" },
+ .num_parents = 1,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch gcc_rx1_usb2_clkref_clk = {
+ .halt_reg = 0x88014,
+ .clkr = {
+ .enable_reg = 0x88014,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_rx1_usb2_clkref_clk",
+ .parent_names = (const char *[]){ "xo" },
+ .num_parents = 1,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
static struct gdsc pcie_0_gdsc = {
.gdscr = 0x6b004,
.gds_hw_ctrl = 0x0,
@@ -2735,6 +2805,11 @@ static struct clk_regmap *gcc_msm8998_clocks[] = {
[USB30_MASTER_CLK_SRC] = &usb30_master_clk_src.clkr,
[USB30_MOCK_UTMI_CLK_SRC] = &usb30_mock_utmi_clk_src.clkr,
[USB3_PHY_AUX_CLK_SRC] = &usb3_phy_aux_clk_src.clkr,
+ [GCC_HDMI_CLKREF_CLK] = &gcc_hdmi_clkref_clk.clkr,
+ [GCC_UFS_CLKREF_CLK] = &gcc_ufs_clkref_clk.clkr,
+ [GCC_USB3_CLKREF_CLK] = &gcc_usb3_clkref_clk.clkr,
+ [GCC_PCIE_CLKREF_CLK] = &gcc_pcie_clkref_clk.clkr,
+ [GCC_RX1_USB2_CLKREF_CLK] = &gcc_rx1_usb2_clkref_clk.clkr,
};
static struct gdsc *gcc_msm8998_gdscs[] = {
diff --git a/include/dt-bindings/clock/qcom,gcc-msm8998.h b/include/dt-bindings/clock/qcom,gcc-msm8998.h
index 58a242e656b1..b3448800980a 100644
--- a/include/dt-bindings/clock/qcom,gcc-msm8998.h
+++ b/include/dt-bindings/clock/qcom,gcc-msm8998.h
@@ -180,6 +180,11 @@
#define USB30_MASTER_CLK_SRC 163
#define USB30_MOCK_UTMI_CLK_SRC 164
#define USB3_PHY_AUX_CLK_SRC 165
+#define GCC_USB3_CLKREF_CLK 166
+#define GCC_HDMI_CLKREF_CLK 167
+#define GCC_UFS_CLKREF_CLK 168
+#define GCC_PCIE_CLKREF_CLK 169
+#define GCC_RX1_USB2_CLKREF_CLK 170
#define PCIE_0_GDSC 0
#define UFS_GDSC 1
--
2.18.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical
2018-11-30 6:52 ` [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical Bjorn Andersson
@ 2018-11-30 7:05 ` Stephen Boyd
2018-11-30 7:24 ` Bjorn Andersson
0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2018-11-30 7:05 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette
Cc: Andy Gross, David Brown, linux-arm-msm, linux-soc, linux-clk,
linux-kernel, Marc Gonzalez, Amit Kucheria
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,
> },
> },
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks
2018-11-30 6:52 ` [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks Bjorn Andersson
@ 2018-11-30 7:06 ` Stephen Boyd
2018-11-30 7:27 ` Bjorn Andersson
2018-11-30 10:55 ` Marc Gonzalez
1 sibling, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2018-11-30 7:06 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette
Cc: Andy Gross, David Brown, linux-arm-msm, linux-soc, linux-clk,
linux-kernel, Marc Gonzalez, Amit Kucheria
Quoting Bjorn Andersson (2018-11-29 22:52:58)
> Drop the halt check of the UFS symbol clocks, in accordance with other
> platforms. This makes clk_disable_unused() happy and makes it possible
> to turn the clocks on again without an error.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Someone was supposed to figure out why we needed to SKIP here instead of
doing things in the proper order. Is anyone attempting to figure that
out?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical
2018-11-30 7:05 ` Stephen Boyd
@ 2018-11-30 7:24 ` Bjorn Andersson
2018-11-30 8:12 ` Stephen Boyd
0 siblings, 1 reply; 14+ messages in thread
From: Bjorn Andersson @ 2018-11-30 7:24 UTC (permalink / raw)
To: Stephen Boyd
Cc: Michael Turquette, Andy Gross, David Brown, linux-arm-msm,
linux-soc, linux-clk, linux-kernel, Marc Gonzalez, Amit Kucheria
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,
> > },
> > },
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks
2018-11-30 7:06 ` Stephen Boyd
@ 2018-11-30 7:27 ` Bjorn Andersson
2018-11-30 8:13 ` Stephen Boyd
0 siblings, 1 reply; 14+ messages in thread
From: Bjorn Andersson @ 2018-11-30 7:27 UTC (permalink / raw)
To: Stephen Boyd, Vivek Gautam
Cc: Michael Turquette, Andy Gross, David Brown, linux-arm-msm,
linux-soc, linux-clk, linux-kernel, Marc Gonzalez, Amit Kucheria
On Thu 29 Nov 23:06 PST 2018, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2018-11-29 22:52:58)
> > Drop the halt check of the UFS symbol clocks, in accordance with other
> > platforms. This makes clk_disable_unused() happy and makes it possible
> > to turn the clocks on again without an error.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> Someone was supposed to figure out why we needed to SKIP here instead of
> doing things in the proper order. Is anyone attempting to figure that
> out?
>
I'm not aware of any such efforts, but looping in Vivek here.
I would be happy to revert this change in 8996, 8998 and 845 once such
fix arrives. But as this is needed to make progress on getting UFS up on
8998 it would be nice to get it merged for now...
Regards,
Bjorn
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical
2018-11-30 7:24 ` Bjorn Andersson
@ 2018-11-30 8:12 ` Stephen Boyd
2018-11-30 9:23 ` Marc Gonzalez
0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2018-11-30 8:12 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Michael Turquette, Andy Gross, David Brown, linux-arm-msm,
linux-soc, linux-clk, linux-kernel, Marc Gonzalez, Amit Kucheria
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks
2018-11-30 7:27 ` Bjorn Andersson
@ 2018-11-30 8:13 ` Stephen Boyd
0 siblings, 0 replies; 14+ messages in thread
From: Stephen Boyd @ 2018-11-30 8:13 UTC (permalink / raw)
To: Bjorn Andersson, Vivek Gautam
Cc: Michael Turquette, Andy Gross, David Brown, linux-arm-msm,
linux-soc, linux-clk, linux-kernel, Marc Gonzalez, Amit Kucheria
Quoting Bjorn Andersson (2018-11-29 23:27:25)
> On Thu 29 Nov 23:06 PST 2018, Stephen Boyd wrote:
>
> > Quoting Bjorn Andersson (2018-11-29 22:52:58)
> > > Drop the halt check of the UFS symbol clocks, in accordance with other
> > > platforms. This makes clk_disable_unused() happy and makes it possible
> > > to turn the clocks on again without an error.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >
> > Someone was supposed to figure out why we needed to SKIP here instead of
> > doing things in the proper order. Is anyone attempting to figure that
> > out?
> >
>
> I'm not aware of any such efforts, but looping in Vivek here.
>
> I would be happy to revert this change in 8996, 8998 and 845 once such
> fix arrives. But as this is needed to make progress on getting UFS up on
> 8998 it would be nice to get it merged for now...
>
That's fine. Just doing my periodic ping on this topic.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical
2018-11-30 8:12 ` Stephen Boyd
@ 2018-11-30 9:23 ` Marc Gonzalez
2018-11-30 15:39 ` Jeffrey Hugo
0 siblings, 1 reply; 14+ messages in thread
From: Marc Gonzalez @ 2018-11-30 9:23 UTC (permalink / raw)
To: Stephen Boyd, Bjorn Andersson
Cc: Michael Turquette, Andy Gross, David Brown, linux-arm-msm,
linux-soc, linux-clk, linux-kernel, Amit Kucheria
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks
2018-11-30 6:52 ` [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks Bjorn Andersson
2018-11-30 7:06 ` Stephen Boyd
@ 2018-11-30 10:55 ` Marc Gonzalez
2018-11-30 13:08 ` Marc Gonzalez
1 sibling, 1 reply; 14+ messages in thread
From: Marc Gonzalez @ 2018-11-30 10:55 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette, Stephen Boyd
Cc: Andy Gross, David Brown, linux-arm-msm, linux-soc, linux-clk,
linux-kernel, Amit Kucheria
On 30/11/2018 07:52, Bjorn Andersson wrote:
> Drop the halt check of the UFS symbol clocks, in accordance with other
> platforms. This makes clk_disable_unused() happy and makes it possible
> to turn the clocks on again without an error.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/clk/qcom/gcc-msm8998.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
> index d89f8e7c2a59..3d232d266d72 100644
> --- a/drivers/clk/qcom/gcc-msm8998.c
> +++ b/drivers/clk/qcom/gcc-msm8998.c
> @@ -2403,7 +2403,7 @@ static struct clk_branch gcc_ufs_phy_aux_clk = {
>
> static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
> .halt_reg = 0x75014,
> - .halt_check = BRANCH_HALT,
> + .halt_check = BRANCH_HALT_SKIP,
> .clkr = {
> .enable_reg = 0x75014,
> .enable_mask = BIT(0),
> @@ -2416,7 +2416,7 @@ static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
>
> static struct clk_branch gcc_ufs_rx_symbol_1_clk = {
> .halt_reg = 0x7605c,
> - .halt_check = BRANCH_HALT,
> + .halt_check = BRANCH_HALT_SKIP,
> .clkr = {
> .enable_reg = 0x7605c,
> .enable_mask = BIT(0),
> @@ -2429,7 +2429,7 @@ static struct clk_branch gcc_ufs_rx_symbol_1_clk = {
>
> static struct clk_branch gcc_ufs_tx_symbol_0_clk = {
> .halt_reg = 0x75010,
> - .halt_check = BRANCH_HALT,
> + .halt_check = BRANCH_HALT_SKIP,
> .clkr = {
> .enable_reg = 0x75010,
> .enable_mask = BIT(0),
FWIW, before applying your patch, the kernel printed the following
warnings at boot:
[ 1.820756] gcc_ufs_rx_symbol_1_clk status stuck at 'on'
[ 1.992977] gcc_ufs_rx_symbol_0_clk status stuck at 'on'
[ 2.165113] gcc_gpu_bimc_gfx_clk status stuck at 'on'
https://lore.kernel.org/linux-clk/f4271471-b60a-f056-b453-1cfa593a0fc4@free.fr/
(NB: gcc_ufs_tx_symbol_0_clk not mentioned)
Your patch makes the two ufs_rx warnings go away.
Regards.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks
2018-11-30 10:55 ` Marc Gonzalez
@ 2018-11-30 13:08 ` Marc Gonzalez
0 siblings, 0 replies; 14+ messages in thread
From: Marc Gonzalez @ 2018-11-30 13:08 UTC (permalink / raw)
To: Bjorn Andersson, Michael Turquette, Stephen Boyd
Cc: Andy Gross, David Brown, linux-arm-msm, linux-soc, linux-clk,
linux-kernel, Amit Kucheria
On 30/11/2018 11:55, Marc Gonzalez wrote:
> On 30/11/2018 07:52, Bjorn Andersson wrote:
>
>> Drop the halt check of the UFS symbol clocks, in accordance with other
>> platforms. This makes clk_disable_unused() happy and makes it possible
>> to turn the clocks on again without an error.
>>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> ---
>> drivers/clk/qcom/gcc-msm8998.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c
>> index d89f8e7c2a59..3d232d266d72 100644
>> --- a/drivers/clk/qcom/gcc-msm8998.c
>> +++ b/drivers/clk/qcom/gcc-msm8998.c
>> @@ -2403,7 +2403,7 @@ static struct clk_branch gcc_ufs_phy_aux_clk = {
>>
>> static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
>> .halt_reg = 0x75014,
>> - .halt_check = BRANCH_HALT,
>> + .halt_check = BRANCH_HALT_SKIP,
>> .clkr = {
>> .enable_reg = 0x75014,
>> .enable_mask = BIT(0),
>> @@ -2416,7 +2416,7 @@ static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
>>
>> static struct clk_branch gcc_ufs_rx_symbol_1_clk = {
>> .halt_reg = 0x7605c,
>> - .halt_check = BRANCH_HALT,
>> + .halt_check = BRANCH_HALT_SKIP,
>> .clkr = {
>> .enable_reg = 0x7605c,
>> .enable_mask = BIT(0),
>> @@ -2429,7 +2429,7 @@ static struct clk_branch gcc_ufs_rx_symbol_1_clk = {
>>
>> static struct clk_branch gcc_ufs_tx_symbol_0_clk = {
>> .halt_reg = 0x75010,
>> - .halt_check = BRANCH_HALT,
>> + .halt_check = BRANCH_HALT_SKIP,
>> .clkr = {
>> .enable_reg = 0x75010,
>> .enable_mask = BIT(0),
>
> FWIW, before applying your patch, the kernel printed the following
> warnings at boot:
>
> [ 1.820756] gcc_ufs_rx_symbol_1_clk status stuck at 'on'
> [ 1.992977] gcc_ufs_rx_symbol_0_clk status stuck at 'on'
> [ 2.165113] gcc_gpu_bimc_gfx_clk status stuck at 'on'
>
> https://lore.kernel.org/linux-clk/f4271471-b60a-f056-b453-1cfa593a0fc4@free.fr/
As far as gcc_gpu_bimc_gfx_clk is concerned, downstream flags it as
"no_halt_check_on_disable", and sets CLKFLAG_RETAIN_MEM.
Regards.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical
2018-11-30 9:23 ` Marc Gonzalez
@ 2018-11-30 15:39 ` Jeffrey Hugo
0 siblings, 0 replies; 14+ messages in thread
From: Jeffrey Hugo @ 2018-11-30 15:39 UTC (permalink / raw)
To: Marc Gonzalez, Stephen Boyd, Bjorn Andersson
Cc: Michael Turquette, Andy Gross, David Brown, linux-arm-msm,
linux-soc, linux-clk, linux-kernel, Amit Kucheria
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.
>
--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-11-30 15:39 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-30 6:52 [PATCH 0/3] clk: qcom: gcc-msm8998: Fixes and clkref clocks Bjorn Andersson
2018-11-30 6:52 ` [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical Bjorn Andersson
2018-11-30 7:05 ` Stephen Boyd
2018-11-30 7:24 ` Bjorn Andersson
2018-11-30 8:12 ` Stephen Boyd
2018-11-30 9:23 ` Marc Gonzalez
2018-11-30 15:39 ` Jeffrey Hugo
2018-11-30 6:52 ` [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks Bjorn Andersson
2018-11-30 7:06 ` Stephen Boyd
2018-11-30 7:27 ` Bjorn Andersson
2018-11-30 8:13 ` Stephen Boyd
2018-11-30 10:55 ` Marc Gonzalez
2018-11-30 13:08 ` Marc Gonzalez
2018-11-30 6:52 ` [PATCH 3/3] clk: qcom: gcc-msm8998: Add clkref clocks Bjorn Andersson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).