| Message ID | 20181130065259.26497-3-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: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?
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
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.
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.
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.
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),
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(-)