On 10/4/2024 7:32 PM, Andrew Lunn wrote:

On Fri, Oct 04, 2024 at 01:25:52PM +0530, Manikanta Mylavarapu wrote:

quoted

On 6/26/2024 8:09 PM, Devi Priya wrote:

quoted

On 6/25/2024 8:09 PM, Andrew Lunn wrote:

quoted

quoted

+static struct clk_alpha_pll ubi32_pll_main = {
+    .offset = 0x28000,
+    .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_NSS_HUAYRA],
+    .flags = SUPPORTS_DYNAMIC_UPDATE,
+    .clkr = {
+        .hw.init = &(const struct clk_init_data) {
+            .name = "ubi32_pll_main",
+            .parent_data = &(const struct clk_parent_data) {
+                .index = DT_XO,
+            },
+            .num_parents = 1,
+            .ops = &clk_alpha_pll_huayra_ops,
+        },
+    },
+};
+
+static struct clk_alpha_pll_postdiv ubi32_pll = {
+    .offset = 0x28000,
+    .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_NSS_HUAYRA],
+    .width = 2,
+    .clkr.hw.init = &(const struct clk_init_data) {
+        .name = "ubi32_pll",
+        .parent_hws = (const struct clk_hw *[]) {
+            &ubi32_pll_main.clkr.hw
+        },
+        .num_parents = 1,
+        .ops = &clk_alpha_pll_postdiv_ro_ops,
+        .flags = CLK_SET_RATE_PARENT,
+    },
+};

Can these structures be made const? You have quite a few different
structures in this driver, some of which are const, and some which are
not.

Sure, will check and update this in V6

Thanks,
Devi Priya

quoted

    Andrew

Hi Andrew,

Sorry for the delayed response.

The ubi32_pll_main structure should be passed to clk_alpha_pll_configure() API to configure UBI32 PLL. clk_alpha_pll_configure() API expects a non-const structure. Declaring it as const will result in the following compilation warning

drivers/clk/qcom/nsscc-ipq9574.c:3067:26: warning: passing argument 1 of ‘clk_alpha_pll_configure’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  clk_alpha_pll_configure(&ubi32_pll_main, regmap, &ubi32_pll_config);
                          ^
In file included from drivers/clk/qcom/nsscc-ipq9574.c:22:0:
drivers/clk/qcom/clk-alpha-pll.h:200:6: note: expected ‘struct clk_alpha_pll *’ but argument is of type ‘const struct clk_alpha_pll *’
 void clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
      ^~~~~~~~~~~~~~~~~~~~~~~

As far as i can see, clk_alpha_pll_configure() does not modify pll.

https://elixir.bootlin.com/linux/v6.12-rc1/source/drivers/clk/qcom/clk-alpha-pll.c#L391

So you can add a const there as well.

It appears that multiple targets are invoking clk_alpha_pll_configure().
I will make changes in a separate series after this.

quoted

The ubi32_pll is the source for nss_cc_ubi0_clk_src, nss_cc_ubi1_clk_src, nss_cc_ubi2_clk_src, nss_cc_ubi3_clk_src. Therefore, to register ubi32_pll with clock framework, it should be assigned to UBI32_PLL index of nss_cc_ipq9574_clocks array. This assignment will result in the following compilation warning if the ubi32_pll structure is declared as const.

drivers/clk/qcom/nsscc-ipq9574.c:2893:16: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  [UBI32_PLL] = &ubi32_pll.clkr,

Which suggests you are missing a const somewhere else.

Getting these structures const correct has a few benefits. It makes
you code smaller, since at the moment at load time it needs to copy
these structures in to the BSS so they are writable, rather than
keeping them in the .rodata segment. Also, by making them const, you
avoid a few security issues, they cannot be overwritten, the MMU will
protect them. The compiler can also make some optimisations, since it
knows the values cannot change.

Now, it could be getting this all const correct needs lots of patches,
because it has knock-on effects in other places. If so, then don't
bother. But if it is simple to do, please spend a little time to get
this right.

	Andrew

We can make these structures const. However, it will require exhaustive changes. Below is the rationale.

To declare the ubi32_pll and ubi32_pll_main (since it is also passed to nss_cc_ipq9574_clocks[] array) structures as const,
the 'clks' member of 'struct qcom_cc_desc' needs to be const.

The following compilation errors are observed after making the 'clks' member of 'struct qcom_cc_desc' as const:
drivers/clk/qcom/common.c: In function ‘qcom_cc_icc_register’:
drivers/clk/qcom/common.c:274:7: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
hws = &desc->clks[desc->icc_hws[i].clk_id]->hw;
       ^

drivers/clk/qcom/common.c: In function ‘qcom_cc_really_probe’:
drivers/clk/qcom/common.c:294:30: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
struct clk_regmap **rclks = desc->clks;
                              ^~~~

The common.c code is designed in such a way that the 'clks' member needs to be assigned to non-const variables/pointers.

We need to update common.c and the drivers that utilize 'struct qcom_cc_desc'.
I will make changes in a separate series after this.

Thanks & Regards,
Manikanta.