Re: [PATCH v2] clk: qcom: gcc: Add missing clocks in SM8150

[Stephen Boyd]

Quoting Vinod Koul (2019-10-24 07:13:44)
> diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> index 20877214acff..0334b2be5fca 100644
> --- a/drivers/clk/qcom/gcc-sm8150.c
> +++ b/drivers/clk/qcom/gcc-sm8150.c
> @@ -1616,6 +1616,40 @@ static struct clk_branch gcc_gpu_cfg_ahb_clk = {
>         },
>  };
>  
> +/* external clocks so add BRANCH_HALT_SKIP */

Ok. The comment is sort of worthless though. Which clk is external? The
parent of this clk?

And it seems very weird that we need this one to be halt skip because
the parent isn't external and I don't know why this is marked with
CLK_SET_RATE_PARENT. Are we going to allow gpll0 to be modified? gpll0
looks to be a fixed rate PLL or something so probably we don't want the
branch to allow consumers to change the main PLL frequency and it should
be turned on before this clk is enabled.

> +static struct clk_branch gcc_gpu_gpll0_clk_src = {
> +       .halt_check = BRANCH_HALT_SKIP,
> +       .clkr = {
> +               .enable_reg = 0x52004,
> +               .enable_mask = BIT(15),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_gpu_gpll0_clk_src",
> +                       .parent_hws = (const struct clk_hw *[]){
> +                               &gpll0.clkr.hw },
> +                       .num_parents = 1,
> +                       .flags = CLK_SET_RATE_PARENT,
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +/* these are external clocks so add BRANCH_HALT_SKIP */
> +static struct clk_branch gcc_gpu_gpll0_div_clk_src = {
> +       .halt_check = BRANCH_HALT_SKIP,
> +       .clkr = {
> +               .enable_reg = 0x52004,
> +               .enable_mask = BIT(16),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_gpu_gpll0_div_clk_src",
> +                       .parent_hws = (const struct clk_hw *[]){
> +                               &gcc_gpu_gpll0_clk_src.clkr.hw },
> +                       .num_parents = 1,
> +                       .flags = CLK_SET_RATE_PARENT,
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
>  static struct clk_branch gcc_gpu_iref_clk = {
>         .halt_reg = 0x8c010,
>         .halt_check = BRANCH_HALT,
> @@ -1698,6 +1732,40 @@ static struct clk_branch gcc_npu_cfg_ahb_clk = {
>         },
>  };
>  
> +/* external clocks so add BRANCH_HALT_SKIP */
> +static struct clk_branch gcc_npu_gpll0_clk_src = {
> +       .halt_check = BRANCH_HALT_SKIP,
> +       .clkr = {
> +               .enable_reg = 0x52004,
> +               .enable_mask = BIT(18),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_npu_gpll0_clk_src",
> +                       .parent_hws = (const struct clk_hw *[]){
> +                               &gpll0.clkr.hw },
> +                       .num_parents = 1,
> +                       .flags = CLK_SET_RATE_PARENT,
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +/* external clocks so add BRANCH_HALT_SKIP */
> +static struct clk_branch gcc_npu_gpll0_div_clk_src = {
> +       .halt_check = BRANCH_HALT_SKIP,
> +       .clkr = {
> +               .enable_reg = 0x52004,
> +               .enable_mask = BIT(19),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_npu_gpll0_div_clk_src",
> +                       .parent_hws = (const struct clk_hw *[]){
> +                               &gcc_npu_gpll0_clk_src.clkr.hw },
> +                       .num_parents = 1,
> +                       .flags = CLK_SET_RATE_PARENT,
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
>  static struct clk_branch gcc_npu_trig_clk = {
>         .halt_reg = 0x4d00c,
>         .halt_check = BRANCH_VOTED,
> @@ -2812,6 +2880,45 @@ static struct clk_branch gcc_ufs_card_phy_aux_hw_ctl_clk = {
>         },
>  };
>  
> +/* external clocks so add BRANCH_HALT_SKIP */

The UFS ones have always been this way. My understanding is that UFS phy
is the parent clk and it's not one when the driver enables it. I think
Manu has clarified these and I still hope we can just turn them on by
default and not model them in clk framework.

> +static struct clk_branch gcc_ufs_card_rx_symbol_0_clk = {
> +       .halt_check = BRANCH_HALT_SKIP,
> +       .clkr = {
> +               .enable_reg = 0x7501c,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_ufs_card_rx_symbol_0_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
[...]
> +static struct clk_branch gcc_usb3_sec_phy_pipe_clk = {

Same comment for USB as for UFS.

> +       .halt_check = BRANCH_HALT_SKIP,
> +       .clkr = {
> +               .enable_reg = 0x10058,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_usb3_sec_phy_pipe_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
>  static struct clk_branch gcc_usb3_sec_phy_com_aux_clk = {
>         .halt_reg = 0x10054,
>         .halt_check = BRANCH_HALT,