Re: [PATCH 2/2] clk: qcom: gcc-qdu1000: Update GCC clocks and add support for GDSCs

[Dmitry Baryshkov]

On Fri, 23 Jun 2023 at 13:08, Imran Shaik <quic_imrashai@xxxxxxxxxxx> wrote:
>
>
>
> On 6/16/2023 4:50 PM, Dmitry Baryshkov wrote:
> > On 16/06/2023 13:49, Imran Shaik wrote:
> >> Update the GCC clocks and add support for GDSCs for QDU1000 and
> >> QRU1000 SoCs. While at it, fix the PCIe pipe clock handling and
> >> add support for v2 variant.
> >
> > Please split this into individual chunks instead of squashing everything
> > together. For each change please describe the logic behind the change in
> > the commit message. Please describe why, not what is changed.
> >
>
> Sure, will split this patch and post the next series.
>
> >>
> >> Signed-off-by: Taniya Das <quic_tdas@xxxxxxxxxxx>
> >> Signed-off-by: Imran Shaik <quic_imrashai@xxxxxxxxxxx>
> >
> > This doesn't look fully logical. Who is the author of the patch? If
> > there are two authors, please add Co-developed-by tag.
> >
>
> Yes, will use Co-developed-by tag in the next patch series.
>
> >> ---
> >>   drivers/clk/qcom/gcc-qdu1000.c | 162 ++++++++++++++++++++++-----------
> >>   1 file changed, 110 insertions(+), 52 deletions(-)
> >>
> >> diff --git a/drivers/clk/qcom/gcc-qdu1000.c
> >> b/drivers/clk/qcom/gcc-qdu1000.c
> >> index 5051769ad90c..5d8125c0eacc 100644
> >> --- a/drivers/clk/qcom/gcc-qdu1000.c
> >> +++ b/drivers/clk/qcom/gcc-qdu1000.c
> >> @@ -1,6 +1,6 @@
> >>   // SPDX-License-Identifier: GPL-2.0-only
> >>   /*
> >> - * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights
> >> reserved.
> >> + * Copyright (c) 2022-2023, Qualcomm Innovation Center, Inc. All
> >> rights reserved.
> >>    */
> >>   #include <linux/clk-provider.h>
> >> @@ -17,6 +17,7 @@
> >>   #include "clk-regmap-divider.h"
> >>   #include "clk-regmap-mux.h"
> >>   #include "clk-regmap-phy-mux.h"
> >> +#include "gdsc.h"
> >>   #include "reset.h"
> >>   enum {
> >> @@ -370,16 +371,6 @@ static const struct clk_parent_data
> >> gcc_parent_data_6[] = {
> >>       { .index = DT_TCXO_IDX },
> >>   };
> >> -static const struct parent_map gcc_parent_map_7[] = {
> >> -    { P_PCIE_0_PIPE_CLK, 0 },
> >> -    { P_BI_TCXO, 2 },
> >> -};
> >> -
> >> -static const struct clk_parent_data gcc_parent_data_7[] = {
> >> -    { .index = DT_PCIE_0_PIPE_CLK_IDX },
> >> -    { .index = DT_TCXO_IDX },
> >> -};
> >> -
> >>   static const struct parent_map gcc_parent_map_8[] = {
> >>       { P_BI_TCXO, 0 },
> >>       { P_GCC_GPLL0_OUT_MAIN, 1 },
> >> @@ -439,16 +430,15 @@ static struct clk_regmap_mux
> >> gcc_pcie_0_phy_aux_clk_src = {
> >>       },
> >>   };
> >> -static struct clk_regmap_mux gcc_pcie_0_pipe_clk_src = {
> >> +static struct clk_regmap_phy_mux gcc_pcie_0_pipe_clk_src = {
> >>       .reg = 0x9d064,
> >> -    .shift = 0,
> >> -    .width = 2,
> >> -    .parent_map = gcc_parent_map_7,
> >>       .clkr = {
> >>           .hw.init = &(const struct clk_init_data) {
> >>               .name = "gcc_pcie_0_pipe_clk_src",
> >> -            .parent_data = gcc_parent_data_7,
> >> -            .num_parents = ARRAY_SIZE(gcc_parent_data_7),
> >> +            .parent_data = &(const struct clk_parent_data){
> >> +                .index = DT_PCIE_0_PIPE_CLK_IDX,
> >> +            },
> >> +            .num_parents = 1,
> >>               .ops = &clk_regmap_phy_mux_ops,
> >>           },
> >>       },
> >> @@ -485,7 +475,7 @@ static struct clk_rcg2
> >> gcc_aggre_noc_ecpri_dma_clk_src = {
> >>           .name = "gcc_aggre_noc_ecpri_dma_clk_src",
> >>           .parent_data = gcc_parent_data_4,
> >>           .num_parents = ARRAY_SIZE(gcc_parent_data_4),
> >> -        .ops = &clk_rcg2_ops,
> >> +        .ops = &clk_rcg2_shared_ops,
> >>       },
> >>   };
> >> @@ -505,7 +495,7 @@ static struct clk_rcg2
> >> gcc_aggre_noc_ecpri_gsi_clk_src = {
> >>           .name = "gcc_aggre_noc_ecpri_gsi_clk_src",
> >>           .parent_data = gcc_parent_data_5,
> >>           .num_parents = ARRAY_SIZE(gcc_parent_data_5),
> >> -        .ops = &clk_rcg2_ops,
> >> +        .ops = &clk_rcg2_shared_ops,
> >
> > This is probably some kind of NoC or NIU clock. If it is not to be
> > touched by Linux, the recent clk_rcg2_ro_ops patch looks promising here.
> >
>
> This is not a NoC or NIU clock and Linux will use this clock.

The clock name ("gcc_aggre_noc_ecpri_gsi_clk_src") seems to disagree with you.

> >>       },
> >>   };
> >> @@ -524,7 +514,7 @@ static struct clk_rcg2 gcc_gp1_clk_src = {
> >>           .name = "gcc_gp1_clk_src",
> >>           .parent_data = gcc_parent_data_1,
> >>           .num_parents = ARRAY_SIZE(gcc_parent_data_1),
> >> -        .ops = &clk_rcg2_ops,
> >> +        .ops = &clk_rcg2_shared_ops,
> >
> > But why? GP clocks are not shared.
> > The 'why?' question applies to all such changes. As I wrote, please
> > split & describe the reason.
> >
>
> We want to park all the RCGs at safe clock source (XO) during disable.
> Hence using the clk_rcg2_shared_ops for all the RCGs.
>
> Will split and post the next patch series.

For this (and for all other changes) please describe the reason behind
the change in the commit message.
For example, for GP clocks there seems to be no reason to park them.
On other platforms it was perfectly fine to disable them instead.


-- 
With best wishes
Dmitry