2018-11-30 06:58:14

by Bjorn Andersson

[permalink] [raw]
Subject: [PATCH 0/3] clk: qcom: gcc-msm8998: Fixes and clkref clocks

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



2018-11-30 06:56:00

by Bjorn Andersson

[permalink] [raw]
Subject: [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical

Keep the two clocks enabled, so that the platform passes
clk_disable_unused().

Signed-off-by: Bjorn Andersson <[email protected]>
---
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


2018-11-30 06:56:01

by Bjorn Andersson

[permalink] [raw]
Subject: [PATCH 3/3] clk: qcom: gcc-msm8998: Add clkref clocks

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 <[email protected]>
---
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


2018-11-30 06:56:09

by Bjorn Andersson

[permalink] [raw]
Subject: [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks

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 <[email protected]>
---
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


2018-11-30 07:06:05

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical

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 <[email protected]>
> ---
> 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,
> },
> },

2018-11-30 07:08:43

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks

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 <[email protected]>

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?


2018-11-30 07:26:27

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical

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 <[email protected]>
> > ---
> > 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,
> > },
> > },

2018-11-30 07:29:29

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks

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 <[email protected]>
>
> 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

2018-11-30 08:14:40

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks

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 <[email protected]>
> >
> > 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.


2018-11-30 08:14:56

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical

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 <[email protected]>
> > > ---
> > > 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.

2018-11-30 09:25:50

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical

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 <[email protected]>
>>>> ---
>>>> 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.

2018-11-30 10:57:27

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks

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 <[email protected]>
> ---
> 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/[email protected]/

(NB: gcc_ufs_tx_symbol_0_clk not mentioned)

Your patch makes the two ufs_rx warnings go away.

Regards.

2018-11-30 13:10:06

by Marc Gonzalez

[permalink] [raw]
Subject: Re: [PATCH 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks

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 <[email protected]>
>> ---
>> 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/[email protected]/

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.

2018-11-30 15:42:02

by Jeffrey Hugo

[permalink] [raw]
Subject: Re: [PATCH 1/3] clk: qcom: gcc-msm8998: Mark hmss_dvm and lpass_at critical

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 <[email protected]>
>>>>> ---
>>>>> 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.