Re: [PATCH] clk: qcom: gcc-msm8994: Remove NoC clocks

[Dmitry Baryshkov]

On Thu, 30 Dec 2021 at 05:31, Konrad Dybcio
<konrad.dybcio@xxxxxxxxxxxxxx> wrote:
>
> Just like in commit 05cf3ec00d460b50088d421fb878a0f83f57e262
> ("clk: qcom: gcc-msm8996: Drop (again) gcc_aggre1_pnoc_ahb_clk")
> adding NoC clocks turned out to be a huge mistake, as they cause a lot of
> issues at little benefit (basically only letting Linux know about their
> children's frequencies), especially when mishandled or misconfigured.

I'm not against this patch, but it manifests another question to me:
should the NoC driver set these frequencies (as demanded), or are they
set by the hardware/RPM/etc and so are read-only to us?

>
> Adding these ones broke SDCC approx 99 out of 100 times, but that somehow
> went unnoticed. To prevent further issues like this one, remove them.
>
> This commit is effectively a revert of 74a33fac3aab77558ca0d80c9935
> ("clk: qcom: gcc-msm8994: Add missing NoC clocks") with ABI preservation.
>
> Fixes: 74a33fac3aab ("clk: qcom: gcc-msm8994: Add missing NoC clocks")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxx>
> ---
>  drivers/clk/qcom/gcc-msm8994.c | 100 +--------------------------------
>  1 file changed, 3 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/clk/qcom/gcc-msm8994.c b/drivers/clk/qcom/gcc-msm8994.c
> index 702a9bdc0559..18044e8dc732 100644
> --- a/drivers/clk/qcom/gcc-msm8994.c
> +++ b/drivers/clk/qcom/gcc-msm8994.c
> @@ -107,42 +107,6 @@ static const struct clk_parent_data gcc_xo_gpll0_gpll4[] = {
>         { .hw = &gpll4.clkr.hw },
>  };
>
> -static struct clk_rcg2 system_noc_clk_src = {
> -       .cmd_rcgr = 0x0120,
> -       .hid_width = 5,
> -       .parent_map = gcc_xo_gpll0_map,
> -       .clkr.hw.init = &(struct clk_init_data){
> -               .name = "system_noc_clk_src",
> -               .parent_data = gcc_xo_gpll0,
> -               .num_parents = ARRAY_SIZE(gcc_xo_gpll0),
> -               .ops = &clk_rcg2_ops,
> -       },
> -};
> -
> -static struct clk_rcg2 config_noc_clk_src = {
> -       .cmd_rcgr = 0x0150,
> -       .hid_width = 5,
> -       .parent_map = gcc_xo_gpll0_map,
> -       .clkr.hw.init = &(struct clk_init_data){
> -               .name = "config_noc_clk_src",
> -               .parent_data = gcc_xo_gpll0,
> -               .num_parents = ARRAY_SIZE(gcc_xo_gpll0),
> -               .ops = &clk_rcg2_ops,
> -       },
> -};
> -
> -static struct clk_rcg2 periph_noc_clk_src = {
> -       .cmd_rcgr = 0x0190,
> -       .hid_width = 5,
> -       .parent_map = gcc_xo_gpll0_map,
> -       .clkr.hw.init = &(struct clk_init_data){
> -               .name = "periph_noc_clk_src",
> -               .parent_data = gcc_xo_gpll0,
> -               .num_parents = ARRAY_SIZE(gcc_xo_gpll0),
> -               .ops = &clk_rcg2_ops,
> -       },
> -};
> -
>  static struct freq_tbl ftbl_ufs_axi_clk_src[] = {
>         F(50000000, P_GPLL0, 12, 0, 0),
>         F(100000000, P_GPLL0, 6, 0, 0),
> @@ -1149,8 +1113,6 @@ static struct clk_branch gcc_blsp1_ahb_clk = {
>                 .enable_mask = BIT(17),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_blsp1_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &periph_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -1434,8 +1396,6 @@ static struct clk_branch gcc_blsp2_ahb_clk = {
>                 .enable_mask = BIT(15),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_blsp2_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &periph_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -1763,8 +1723,6 @@ static struct clk_branch gcc_lpass_q6_axi_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_lpass_q6_axi_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &system_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -1777,8 +1735,6 @@ static struct clk_branch gcc_mss_q6_bimc_axi_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_mss_q6_bimc_axi_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &system_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -1806,9 +1762,6 @@ static struct clk_branch gcc_pcie_0_cfg_ahb_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_pcie_0_cfg_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &config_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
> -                       .flags = CLK_SET_RATE_PARENT,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -1821,9 +1774,6 @@ static struct clk_branch gcc_pcie_0_mstr_axi_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_pcie_0_mstr_axi_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &system_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
> -                       .flags = CLK_SET_RATE_PARENT,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -1853,9 +1803,6 @@ static struct clk_branch gcc_pcie_0_slv_axi_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_pcie_0_slv_axi_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &system_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
> -                       .flags = CLK_SET_RATE_PARENT,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -1883,9 +1830,6 @@ static struct clk_branch gcc_pcie_1_cfg_ahb_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_pcie_1_cfg_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &config_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
> -                       .flags = CLK_SET_RATE_PARENT,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -1898,9 +1842,6 @@ static struct clk_branch gcc_pcie_1_mstr_axi_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_pcie_1_mstr_axi_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &system_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
> -                       .flags = CLK_SET_RATE_PARENT,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -1929,9 +1870,6 @@ static struct clk_branch gcc_pcie_1_slv_axi_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_pcie_1_slv_axi_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &system_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
> -                       .flags = CLK_SET_RATE_PARENT,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -1959,8 +1897,6 @@ static struct clk_branch gcc_pdm_ahb_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_pdm_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &periph_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -1988,9 +1924,6 @@ static struct clk_branch gcc_sdcc1_ahb_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_sdcc1_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &periph_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
> -                       .flags = CLK_SET_RATE_PARENT,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -2003,9 +1936,6 @@ static struct clk_branch gcc_sdcc2_ahb_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_sdcc2_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &periph_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
> -                       .flags = CLK_SET_RATE_PARENT,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -2033,9 +1963,6 @@ static struct clk_branch gcc_sdcc3_ahb_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_sdcc3_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &periph_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
> -                       .flags = CLK_SET_RATE_PARENT,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -2063,9 +1990,6 @@ static struct clk_branch gcc_sdcc4_ahb_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_sdcc4_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &periph_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
> -                       .flags = CLK_SET_RATE_PARENT,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -2123,8 +2047,6 @@ static struct clk_branch gcc_tsif_ahb_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_tsif_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &periph_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -2152,8 +2074,6 @@ static struct clk_branch gcc_ufs_ahb_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_ufs_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &config_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -2197,8 +2117,6 @@ static struct clk_branch gcc_ufs_rx_symbol_0_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_ufs_rx_symbol_0_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &system_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -2212,8 +2130,6 @@ static struct clk_branch gcc_ufs_rx_symbol_1_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_ufs_rx_symbol_1_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &system_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -2242,8 +2158,6 @@ static struct clk_branch gcc_ufs_tx_symbol_0_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_ufs_tx_symbol_0_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &system_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -2257,8 +2171,6 @@ static struct clk_branch gcc_ufs_tx_symbol_1_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_ufs_tx_symbol_1_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &system_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -2363,8 +2275,6 @@ static struct clk_branch gcc_usb_hs_ahb_clk = {
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_usb_hs_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &periph_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -2487,8 +2397,6 @@ static struct clk_branch gcc_boot_rom_ahb_clk = {
>                 .enable_mask = BIT(10),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_boot_rom_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &config_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -2502,8 +2410,6 @@ static struct clk_branch gcc_prng_ahb_clk = {
>                 .enable_mask = BIT(13),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "gcc_prng_ahb_clk",
> -                       .parent_hws = (const struct clk_hw *[]){ &periph_noc_clk_src.clkr.hw },
> -                       .num_parents = 1,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> @@ -2546,9 +2452,6 @@ static struct clk_regmap *gcc_msm8994_clocks[] = {
>         [GPLL0] = &gpll0.clkr,
>         [GPLL4_EARLY] = &gpll4_early.clkr,
>         [GPLL4] = &gpll4.clkr,
> -       [CONFIG_NOC_CLK_SRC] = &config_noc_clk_src.clkr,
> -       [PERIPH_NOC_CLK_SRC] = &periph_noc_clk_src.clkr,
> -       [SYSTEM_NOC_CLK_SRC] = &system_noc_clk_src.clkr,
>         [UFS_AXI_CLK_SRC] = &ufs_axi_clk_src.clkr,
>         [USB30_MASTER_CLK_SRC] = &usb30_master_clk_src.clkr,
>         [BLSP1_QUP1_I2C_APPS_CLK_SRC] = &blsp1_qup1_i2c_apps_clk_src.clkr,
> @@ -2695,6 +2598,9 @@ static struct clk_regmap *gcc_msm8994_clocks[] = {
>         [USB_SS_PHY_LDO] = &usb_ss_phy_ldo.clkr,
>         [GCC_BOOT_ROM_AHB_CLK] = &gcc_boot_rom_ahb_clk.clkr,
>         [GCC_PRNG_AHB_CLK] = &gcc_prng_ahb_clk.clkr,
> +       [CONFIG_NOC_CLK_SRC] = NULL,
> +       [PERIPH_NOC_CLK_SRC] = NULL,
> +       [SYSTEM_NOC_CLK_SRC] = NULL,

You don't have to init array entries with NULL values (if it's not for
the documentation purposes). Uninitialized entries will get NULL value
anyway.

-- 
With best wishes
Dmitry