Re: [PATCH V4 5/7] clk: qcom: Add NSS clock Controller driver for IPQ9574

[Andrew Lunn]

On Fri, Oct 04, 2024 at 01:25:52PM +0530, Manikanta Mylavarapu wrote:
> 
> 
> On 6/26/2024 8:09 PM, Devi Priya wrote:
> > 
> > 
> > On 6/25/2024 8:09 PM, Andrew Lunn wrote:
> >>> +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
> >>     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.

> 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