| Message ID | 20230206071217.29313-1-quic_kathirav@quicinc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add minimal boot support for IPQ5332 | expand |
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? > + .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? > + .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? > + .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 > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > +
Hi Kathiravan, thanks for your patches! On Mon, Feb 6, 2023 at 8:12 AM Kathiravan T <quic_kathirav@quicinc.com> wrote: > Kathiravan T (9): > dt-bindings: pinctrl: qcom: add IPQ5332 pinctrl > pinctrl: qcom: Introduce IPQ5332 TLMM driver I have applied these two patches to the pin control tree for v6.3. I see no reason to wait for more review since Krzysztof acked the bindings and the driver isn't invasive at all, any problems can certainly be fixed up in-tree. Yours, Linus Walleij
On 2/6/2023 4:55 PM, Linus Walleij wrote: > Hi Kathiravan, > > thanks for your patches! > > On Mon, Feb 6, 2023 at 8:12 AM Kathiravan T <quic_kathirav@quicinc.com> wrote: > >> Kathiravan T (9): >> dt-bindings: pinctrl: qcom: add IPQ5332 pinctrl >> pinctrl: qcom: Introduce IPQ5332 TLMM driver > I have applied these two patches to the pin control tree for v6.3. Thanks a lot Linus! > > I see no reason to wait for more review since Krzysztof acked the > bindings and the driver isn't invasive at all, any problems can certainly > be fixed up in-tree. > > Yours, > Linus Walleij
On Mon, Feb 06, 2023 at 12:42:13PM +0530, Kathiravan T wrote: > diff --git a/drivers/clk/qcom/gcc-ipq5332.c b/drivers/clk/qcom/gcc-ipq5332.c [..] > + > +enum { > + DT_SLEEP_CLK, > + DT_XO, > + DT_PCIE_2LANE_PHY_PIPE_CLK, > + DT_PCIE_2LANE_PHY_PIPE_X1_CLK, > + DT_USB_PCIE_WRAPPER_PIPE_CLK, This list does not match the clocks as defined in the binding. > +}; > + > +enum { > + P_PCIE3X2_PIPE, > + P_PCIE3X1_0_PIPE, > + P_PCIE3X1_1_PIPE, > + P_USB3PHY_0_PIPE, > + P_CORE_BI_PLL_TEST_SE, > + P_GCC_GPLL0_OUT_MAIN_DIV_CLK_SRC, > + P_GPLL0_OUT_AUX, > + P_GPLL0_OUT_MAIN, > + P_GPLL2_OUT_AUX, > + P_GPLL2_OUT_MAIN, > + P_GPLL4_OUT_AUX, > + P_GPLL4_OUT_MAIN, > + P_SLEEP_CLK, > + P_XO, > +}; > + > +static const struct clk_parent_data gcc_parent_data_xo = { .index = DT_XO }; > + > +static struct clk_alpha_pll gpll0_main = { > + .offset = 0x20000, > + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_STROMER_PLUS], > + .clkr = { > + .enable_reg = 0xb000, > + .enable_mask = BIT(0), > + .hw.init = &(const struct clk_init_data){ Please add a space between ')' and '{ on all these. > + .name = "gpll0_main", > + .parent_data = &gcc_parent_data_xo, > + .num_parents = 1, > + .ops = &clk_alpha_pll_stromer_ops, > + }, > + }, > +}; [..] > +static const struct qcom_cc_desc gcc_ipq5332_desc = { > + .config = &gcc_ipq5332_regmap_config, > + .clks = gcc_ipq5332_clocks, > + .num_clks = ARRAY_SIZE(gcc_ipq5332_clocks), > + .resets = gcc_ipq5332_resets, > + .num_resets = ARRAY_SIZE(gcc_ipq5332_resets), > + .clk_hws = gcc_ipq5332_hws, > + .num_clk_hws = ARRAY_SIZE(gcc_ipq5332_hws), No GDSCs? Regards, Bjorn
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, > > + }, > > + }, > > +}; > > + >
Thanks Bjorn for the review! On 2/7/2023 9:11 AM, Bjorn Andersson wrote: > On Mon, Feb 06, 2023 at 12:42:13PM +0530, Kathiravan T wrote: >> diff --git a/drivers/clk/qcom/gcc-ipq5332.c b/drivers/clk/qcom/gcc-ipq5332.c > [..] >> + >> +enum { >> + DT_SLEEP_CLK, >> + DT_XO, >> + DT_PCIE_2LANE_PHY_PIPE_CLK, >> + DT_PCIE_2LANE_PHY_PIPE_X1_CLK, >> + DT_USB_PCIE_WRAPPER_PIPE_CLK, > This list does not match the clocks as defined in the binding. Ack. will fix this in next spin. > >> +}; >> + >> +enum { >> + P_PCIE3X2_PIPE, >> + P_PCIE3X1_0_PIPE, >> + P_PCIE3X1_1_PIPE, >> + P_USB3PHY_0_PIPE, >> + P_CORE_BI_PLL_TEST_SE, >> + P_GCC_GPLL0_OUT_MAIN_DIV_CLK_SRC, >> + P_GPLL0_OUT_AUX, >> + P_GPLL0_OUT_MAIN, >> + P_GPLL2_OUT_AUX, >> + P_GPLL2_OUT_MAIN, >> + P_GPLL4_OUT_AUX, >> + P_GPLL4_OUT_MAIN, >> + P_SLEEP_CLK, >> + P_XO, >> +}; >> + >> +static const struct clk_parent_data gcc_parent_data_xo = { .index = DT_XO }; >> + >> +static struct clk_alpha_pll gpll0_main = { >> + .offset = 0x20000, >> + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_STROMER_PLUS], >> + .clkr = { >> + .enable_reg = 0xb000, >> + .enable_mask = BIT(0), >> + .hw.init = &(const struct clk_init_data){ > Please add a space between ')' and '{ on all these. Ack. > >> + .name = "gpll0_main", >> + .parent_data = &gcc_parent_data_xo, >> + .num_parents = 1, >> + .ops = &clk_alpha_pll_stromer_ops, >> + }, >> + }, >> +}; > [..] >> +static const struct qcom_cc_desc gcc_ipq5332_desc = { >> + .config = &gcc_ipq5332_regmap_config, >> + .clks = gcc_ipq5332_clocks, >> + .num_clks = ARRAY_SIZE(gcc_ipq5332_clocks), >> + .resets = gcc_ipq5332_resets, >> + .num_resets = ARRAY_SIZE(gcc_ipq5332_resets), >> + .clk_hws = gcc_ipq5332_hws, >> + .num_clk_hws = ARRAY_SIZE(gcc_ipq5332_hws), > No GDSCs? No, there is no GDSC support. > > Regards, > Bjorn