| Message ID | 1584356082-26769-1-git-send-email-tdas@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add GCC clock driver support | expand |
The subject of this cover letter is too generic. Is it more like clk: qcom: sc7180: Add secure clk and more QUP frequencies or so? Quoting Taniya Das (2020-03-16 03:54:39) > [v1] > * Add a new frequency of 51.2MHz for QUP clock. > * Add support for gcc_sec_ctrl_clk_src RCG for client to be able to request > various frequencies. > > Taniya Das (3): > clk: qcom: gcc: Add support for a new frequency for SC7180 > dt-bindings: clock: Add gcc_sec_ctrl_clk_src clock ID > clk: qcom: gcc: Add support for Secure control source clock Is this for sc7180? Please indicate as such in the subject lines. > > drivers/clk/qcom/gcc-sc7180.c | 94 ++++++++++++++++++----------- > include/dt-bindings/clock/qcom,gcc-sc7180.h | 1 + > 2 files changed, 59 insertions(+), 36 deletions(-)
Quoting Taniya Das (2020-03-16 03:54:40) > There is a requirement to support 51.2MHz from GPLL6 for qup clocks, > thus update the frequency table and parent data/map to use the GPLL6 > source PLL. > > Signed-off-by: Taniya Das <tdas@codeaurora.org> > --- Any Fixes: tag for this? I guess the beginning of this driver being introduced? > drivers/clk/qcom/gcc-sc7180.c | 73 ++++++++++++++++++++++--------------------- > 1 file changed, 37 insertions(+), 36 deletions(-) > > diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c > index 7f59fb8..ad75847 100644 > --- a/drivers/clk/qcom/gcc-sc7180.c > +++ b/drivers/clk/qcom/gcc-sc7180.c > @@ -405,8 +406,8 @@ static const struct freq_tbl ftbl_gcc_qupv3_wrap0_s0_clk_src[] = { > > static struct clk_init_data gcc_qupv3_wrap0_s0_clk_src_init = { > .name = "gcc_qupv3_wrap0_s0_clk_src", > - .parent_data = gcc_parent_data_0, > - .num_parents = 4, > + .parent_data = gcc_parent_data_1, This should have been done initially. We shouldn't need to describe "new" parents when they have always been there. Are there other clks in this driver that actually have more parents than we've currently described? If so, please fix them. > + .num_parents = 5, Can you use ARRAY_SIZE(gcc_parent_data_1) instead? That way this isn't a hard-coded value. > .ops = &clk_rcg2_ops, > }; >
Quoting Taniya Das (2020-03-16 03:54:42) > diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c > index ad75847..3302f19 100644 > --- a/drivers/clk/qcom/gcc-sc7180.c > +++ b/drivers/clk/qcom/gcc-sc7180.c > @@ -817,6 +817,26 @@ static struct clk_rcg2 gcc_usb3_prim_phy_aux_clk_src = { > }, > }; > > +static const struct freq_tbl ftbl_gcc_sec_ctrl_clk_src[] = { > + F(4800000, P_BI_TCXO, 4, 0, 0), > + F(19200000, P_BI_TCXO, 1, 0, 0), > + { } > +}; > + > +static struct clk_rcg2 gcc_sec_ctrl_clk_src = { > + .cmd_rcgr = 0x3d030, > + .mnd_width = 0, > + .hid_width = 5, > + .parent_map = gcc_parent_map_3, > + .freq_tbl = ftbl_gcc_sec_ctrl_clk_src, > + .clkr.hw.init = &(struct clk_init_data){ > + .name = "gcc_sec_ctrl_clk_src", > + .parent_data = gcc_parent_data_3, > + .num_parents = 3, ARRAY_SIZE please. > + .ops = &clk_rcg2_ops, > + }, > +}; > + > static struct clk_branch gcc_aggre_ufs_phy_axi_clk = { > .halt_reg = 0x82024, > .halt_check = BRANCH_HALT_DELAY,
Hello Stephen, Thanks for your review. On 3/16/2020 11:19 PM, Stephen Boyd wrote: > Quoting Taniya Das (2020-03-16 03:54:40) >> There is a requirement to support 51.2MHz from GPLL6 for qup clocks, >> thus update the frequency table and parent data/map to use the GPLL6 >> source PLL. >> >> Signed-off-by: Taniya Das <tdas@codeaurora.org> >> --- > > Any Fixes: tag for this? I guess the beginning of this driver being > introduced? > Sure, will add the same. >> drivers/clk/qcom/gcc-sc7180.c | 73 ++++++++++++++++++++++--------------------- >> 1 file changed, 37 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c >> index 7f59fb8..ad75847 100644 >> --- a/drivers/clk/qcom/gcc-sc7180.c >> +++ b/drivers/clk/qcom/gcc-sc7180.c >> @@ -405,8 +406,8 @@ static const struct freq_tbl ftbl_gcc_qupv3_wrap0_s0_clk_src[] = { >> >> static struct clk_init_data gcc_qupv3_wrap0_s0_clk_src_init = { >> .name = "gcc_qupv3_wrap0_s0_clk_src", >> - .parent_data = gcc_parent_data_0, >> - .num_parents = 4, >> + .parent_data = gcc_parent_data_1, > > This should have been done initially. We shouldn't need to describe > "new" parents when they have always been there. Are there other clks in > this driver that actually have more parents than we've currently > described? If so, please fix them. > The auto generation script does not consider to define the parent unless it is used in the frequency table to derive a frequency. For now I didn't find any other sources missed. >> + .num_parents = 5, > > Can you use ARRAY_SIZE(gcc_parent_data_1) instead? That way this isn't a > hard-coded value. > Yes will take care of it too. >> .ops = &clk_rcg2_ops, >> }; >>
Hello Stephen, Thanks for your review. On 3/16/2020 11:19 PM, Stephen Boyd wrote: > Quoting Taniya Das (2020-03-16 03:54:42) >> diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c >> index ad75847..3302f19 100644 >> --- a/drivers/clk/qcom/gcc-sc7180.c >> +++ b/drivers/clk/qcom/gcc-sc7180.c >> @@ -817,6 +817,26 @@ static struct clk_rcg2 gcc_usb3_prim_phy_aux_clk_src = { >> }, >> }; >> >> +static const struct freq_tbl ftbl_gcc_sec_ctrl_clk_src[] = { >> + F(4800000, P_BI_TCXO, 4, 0, 0), >> + F(19200000, P_BI_TCXO, 1, 0, 0), >> + { } >> +}; >> + >> +static struct clk_rcg2 gcc_sec_ctrl_clk_src = { >> + .cmd_rcgr = 0x3d030, >> + .mnd_width = 0, >> + .hid_width = 5, >> + .parent_map = gcc_parent_map_3, >> + .freq_tbl = ftbl_gcc_sec_ctrl_clk_src, >> + .clkr.hw.init = &(struct clk_init_data){ >> + .name = "gcc_sec_ctrl_clk_src", >> + .parent_data = gcc_parent_data_3, >> + .num_parents = 3, > > ARRAY_SIZE please. > Will take care of the same. >> + .ops = &clk_rcg2_ops, >> + }, >> +}; >> + >> static struct clk_branch gcc_aggre_ufs_phy_axi_clk = { >> .halt_reg = 0x82024, >> .halt_check = BRANCH_HALT_DELAY,