Re: [PATCH v3 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SC7180

[Stephen Boyd <sboyd@kernel.org>]

Quoting Taniya Das (2019-09-23 01:01:11)
> Hi Stephen,
> 
> Thanks for your comments.
> 
> On 9/19/2019 3:09 AM, Stephen Boyd wrote:
> > Quoting Taniya Das (2019-09-18 02:50:18)
> >> diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
> >> new file mode 100644
> >> index 000000000000..d47865d5408f
> >> --- /dev/null
> >> +++ b/drivers/clk/qcom/gcc-sc7180.c
> >> @@ -0,0 +1,2515 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +#include <linux/err.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_device.h>
> >> +#include <linux/regmap.h>
> > 
> > include clk-provider.h
> > 
> 
> will add this header.
> Currently the <drivers/clk/qcom/clk-regmap.h> already includes it.

Yes but it should be included in any clk-provider drivers too.

> >> +
> >> +/* Leave the clock ON for parent config_noc_clk to be kept enabled */
> >> +static struct clk_branch gcc_disp_ahb_clk = {
> >> +       .halt_reg = 0xb00c,
> >> +       .halt_check = BRANCH_HALT,
> >> +       .hwcg_reg = 0xb00c,
> >> +       .hwcg_bit = 1,
> >> +       .clkr = {
> >> +               .enable_reg = 0xb00c,
> >> +               .enable_mask = BIT(0),
> >> +               .hw.init = &(struct clk_init_data){
> >> +                       .name = "gcc_disp_ahb_clk",
> >> +                       .flags = CLK_IS_CRITICAL,
> > 
> > Does this assume the display is left enabled out of the bootloader? Why
> > is this critical to system operation? Maybe it is really
> > CLK_IGNORE_UNUSED?
> > 
> 
> This clock is not kept enabled by bootloader. But leaving this ON for 
> clients on config noc.

Please see below comment for the other critical clk with no parent.

> 
> >> +                       .ops = &clk_branch2_ops,
> >> +               },
> >> +       },
> >> +};
> >> +
[...]
> >> +static struct clk_branch gcc_ufs_phy_phy_aux_clk = {
> >> +       .halt_reg = 0x77094,
> >> +       .halt_check = BRANCH_HALT,
> >> +       .hwcg_reg = 0x77094,
> >> +       .hwcg_bit = 1,
> >> +       .clkr = {
> >> +               .enable_reg = 0x77094,
> >> +               .enable_mask = BIT(0),
> >> +               .hw.init = &(struct clk_init_data){
> >> +                       .name = "gcc_ufs_phy_phy_aux_clk",
> >> +                       .parent_data = &(const struct clk_parent_data){
> >> +                               .hw = &gcc_ufs_phy_phy_aux_clk_src.clkr.hw,
> >> +                       },
> >> +                       .num_parents = 1,
> >> +                       .flags = CLK_SET_RATE_PARENT,
> >> +                       .ops = &clk_branch2_ops,
> >> +               },
> >> +       },
> >> +};
> >> +
> >> +static struct clk_branch gcc_ufs_phy_rx_symbol_0_clk = {
> >> +       .halt_reg = 0x7701c,
> >> +       .halt_check = BRANCH_HALT_SKIP,
> > 
> > Again, nobody has fixed the UFS driver to not need to do this halt skip
> > check for these clks? It's been over a year.
> > 
> 
> The UFS_PHY_RX/TX clocks could be left enabled due to certain HW boot 
> configuration and thus during the late initcall of clk_disable there 
> could be warnings of "clock stuck ON" in the dmesg. That is the reason 
> also to use the BRANCH_HALT_SKIP flag.

Oh that's bad. Why do the clks stay on when we try to turn them off?

> 
> I would also check internally for the UFS driver fix you are referring here.

Sure. I keep asking but nothing is done :(

> 
> >> +       .clkr = {
> >> +               .enable_reg = 0x7701c,
> >> +               .enable_mask = BIT(0),
> >> +               .hw.init = &(struct clk_init_data){
> >> +                       .name = "gcc_ufs_phy_rx_symbol_0_clk",
> >> +                       .ops = &clk_branch2_ops,
> >> +               },
> >> +       },
> >> +};
> >> +
[...]
> >> +
> >> +static struct clk_branch gcc_usb3_prim_phy_pipe_clk = {
> >> +       .halt_reg = 0xf058,
> >> +       .halt_check = BRANCH_HALT_SKIP,
> > 
> > Why does this need halt_skip?
> 
> This is required as the source is external PHY, so we want to not check 
> for HALT.

This doesn't really answer my question. If the source is an external phy
then it should be listed as a clock in the DT binding and the parent
should be specified here. Unless something doesn't work because of that?

> 
> > 
> >> +       .clkr = {
> >> +               .enable_reg = 0xf058,
> >> +               .enable_mask = BIT(0),
> >> +               .hw.init = &(struct clk_init_data){
> >> +                       .name = "gcc_usb3_prim_phy_pipe_clk",
> >> +                       .ops = &clk_branch2_ops,
> >> +               },
> >> +       },
> >> +};
> >> +
> >> +static struct clk_branch gcc_usb_phy_cfg_ahb2phy_clk = {
> >> +       .halt_reg = 0x6a004,
> >> +       .halt_check = BRANCH_HALT,
> >> +       .hwcg_reg = 0x6a004,
> >> +       .hwcg_bit = 1,
> >> +       .clkr = {
> >> +               .enable_reg = 0x6a004,
> >> +               .enable_mask = BIT(0),
> >> +               .hw.init = &(struct clk_init_data){
> >> +                       .name = "gcc_usb_phy_cfg_ahb2phy_clk",
> >> +                       .ops = &clk_branch2_ops,
> >> +               },
> >> +       },
> >> +};
> >> +
> >> +/* Leave the clock ON for parent config_noc_clk to be kept enabled */
> > 
> > There's no parent though... So I guess this means it keeps it enabled
> > implicitly in hardware?
> > 
> 
> These are not left enabled, but want to leave them enabled for clients 
> on config NOC.

Sure. It just doesn't make sense to create clk structures and expose
them in the kernel when we just want to turn the bits on and leave them
on forever. Why not just do some register writes in probe for this
driver? Doesn't that work just as well and use less memory?