Re: [PATCH V3 5/9] clk: qcom: add Global Clock controller (GCC) driver for IPQ5332 SoC
From: Kathiravan T
Date: Mon Feb 06 2023 - 23:30:42 EST
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@xxxxxxxxxxx>
> ---
> 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,
> + },
> + },
> +};
> +