Messages in this thread | | | Date | Tue, 7 Feb 2023 09:59:57 +0530 | Subject | Re: [PATCH V3 5/9] clk: qcom: add Global Clock controller (GCC) driver for IPQ5332 SoC | From | Kathiravan T <> |
| |
Thanks Dmirty for the review!
On 2/6/2023 3:26 PM, Dmitry Baryshkov wrote: > On 06/02/2023 09:12, Kathiravan T wrote: > > Add support for the global clock controller found on IPQ5332 SoC. > > > > Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com> > > --- > > Changes in V3: > > - As I mentined the bindings, changes need to be done in V2 got > > missed out and same has been done in V3, to call out > > specifically dropped the CLK_IS_CRITICAL and dropped the > > gcc_apss_ahb_clk, its source clock and gcc_apss_axi_clk > > - Used gcc_parent_data_xo wherever applicable and dropped the > > duplicate entries > > - dropped the unused parent_map_10 and parent_data_10 > > - Used qcom_cc_probe instead of qcom_cc_really_probe > > Changes in V2: > > - Added the 'dependes on' for Kconfig symbol > > - Dropped the CLK_IS_CRITICAL flag throughout the file > > - Dropped the gcc_apss_ahb_clk and gcc_apss_axi_clk as these are > > managed by bootloaders > > > [skipped] > > > +static const struct freq_tbl ftbl_gcc_pcie_aux_clk_src[] = { > > + F(2000000, P_XO, 12, 0, 0), > > + { } > > +}; > > + > > +static struct clk_rcg2 gcc_pcie_aux_clk_src = { > > + .cmd_rcgr = 0x28004, > > + .mnd_width = 16, > > + .hid_width = 5, > > + .parent_map = gcc_parent_map_6, > > + .freq_tbl = ftbl_gcc_pcie_aux_clk_src, > > + .clkr.hw.init = &(const struct clk_init_data){ > > + .name = "gcc_pcie_aux_clk_src", > > + .parent_data = gcc_parent_data_6, > > + .num_parents = ARRAY_SIZE(gcc_parent_data_6), > > + .ops = &clk_rcg2_ops, > > + }, > > +}; > > + > > +static struct clk_regmap_mux pcie3x2_pipe_clk_src = { > > + .reg = 0x28064, > > + .shift = 8, > > + .width = 2, > > + .parent_map = gcc_parent_map_14, > > + .clkr = { > > + .hw.init = &(struct clk_init_data){ > > + .name = "pcie3x2_phy_pipe_clk_src", > > + .parent_data = gcc_parent_data_14, > > + .num_parents = ARRAY_SIZE(gcc_parent_data_14), > > + .ops = &clk_regmap_mux_closest_ops, > > Should we use clk_regmap_phy_mux_ops here instead?
Sure, I will check this one. Also looking at the commit, 74e4190cdebe ("clk: qcom: regmap: add PHY clock source implementation"), looks like I can use the clk_regmap_phy_mux struct instead of clk_regmap_mux. I will check into this and update accordingly.
> > > + .flags = CLK_SET_RATE_PARENT, > > + }, > > + }, > > +}; > > + > > +static struct clk_regmap_mux pcie3x1_0_pipe_clk_src = { > > + .reg = 0x29064, > > + .shift = 8, > > + .width = 2, > > + .parent_map = gcc_parent_map_15, > > + .clkr = { > > + .hw.init = &(struct clk_init_data){ > > + .name = "pcie3x1_0_phy_pipe_clk_src", > > + .parent_data = gcc_parent_data_15, > > + .num_parents = ARRAY_SIZE(gcc_parent_data_15), > > + .ops = &clk_regmap_mux_closest_ops, > > And clk_regmap_phy_mux_ops here too?
Ack.
> > > + .flags = CLK_SET_RATE_PARENT, > > + }, > > + }, > > +}; > > + > > +static struct clk_regmap_mux pcie3x1_1_pipe_clk_src = { > > + .reg = 0x2A064, > > + .shift = 8, > > + .width = 2, > > + .parent_map = gcc_parent_map_16, > > + .clkr = { > > + .hw.init = &(struct clk_init_data){ > > + .name = "pcie3x1_1_phy_pipe_clk_src", > > + .parent_data = gcc_parent_data_16, > > + .num_parents = ARRAY_SIZE(gcc_parent_data_16), > > + .ops = &clk_regmap_mux_closest_ops, > > And here?
Ack.
> > > + .flags = CLK_SET_RATE_PARENT, > > + }, > > + }, > > +}; > > + > > [skipped] > > > > + > > +static struct clk_branch gcc_pcie3x1_0_pipe_clk = { > > + .halt_reg = 0x29068, > > + .halt_check = BRANCH_HALT_DELAY, > > + .clkr = { > > + .enable_reg = 0x29068, > > + .enable_mask = BIT(0), > > + .hw.init = &(const struct clk_init_data){ > > + .name = "gcc_pcie3x1_0_pipe_clk", > > + .parent_names = (const char *[]){ > > + "pcie3x1_0_pipe_clk_src" > > + }, > > Nooo. No parent_names please. You have several of them in the driver
I missed this. I will fix in next spin.
> > > + .num_parents = 1, > > + .flags = CLK_SET_RATE_PARENT, > > + .ops = &clk_branch2_ops, > > + }, > > + }, > > +}; > > + >
| |