Re: [PATCH v2 1/2] clk: qcom: Add display clock controller driver for QCM2290

[Loic Poulain <loic.poulain@linaro.org>]

Hi Stephen,


On Thu, 16 Dec 2021 at 04:49, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Loic Poulain (2021-12-09 06:09:10)
> > diff --git a/drivers/clk/qcom/dispcc-qcm2290.c b/drivers/clk/qcom/dispcc-qcm2290.c
> > new file mode 100644
> > index 00000000..8aa5d31
> > --- /dev/null
> > +++ b/drivers/clk/qcom/dispcc-qcm2290.c
> > @@ -0,0 +1,602 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2021, Linaro Ltd.
> > + */
> > +
[...]
> > +static struct clk_rcg2 disp_cc_mdss_pclk0_clk_src = {
> > +       .cmd_rcgr = 0x205c,
> > +       .mnd_width = 8,
> > +       .hid_width = 5,
> > +       .parent_map = disp_cc_parent_map_4,
> > +       .clkr.hw.init = &(struct clk_init_data){
> > +               .name = "disp_cc_mdss_pclk0_clk_src",
> > +               .parent_data = disp_cc_parent_data_4,
> > +               .num_parents = ARRAY_SIZE(disp_cc_parent_data_4),
> > +               .flags = CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE | CLK_OPS_PARENT_ENABLE,
>
> These last two flags are needed for what?

NOCACHE is probably useless with mainline.

I've added OPS_PARENT_ENABLE because AFAIU changing clock rate can
lead to parent switch, and parent switch can only be done if parent
clocks are enabled for rcg2 clocks. Otherwise the update fails and we
get the following:
    disp_cc_mdss_pclk0_clk_src: rcg didn't update its configuration.
    WARNING: CPU: 2 PID: 77 at drivers/clk/qcom/clk-rcg2.c:122
update_config+0xe0/0xf0

I'm a bit surprised other similar dispcc drivers don't use the same
flags though.


>
> > +               .ops = &clk_pixel_ops,
> > +       },
> > +};
> > +
> > +static struct clk_rcg2 disp_cc_mdss_vsync_clk_src = {
> > +       .cmd_rcgr = 0x208c,
> > +       .mnd_width = 0,
> > +       .hid_width = 5,
> > +       .parent_map = disp_cc_parent_map_1,
> > +       .freq_tbl = ftbl_disp_cc_mdss_esc0_clk_src,
> > +       .clkr.hw.init = &(struct clk_init_data){
> > +               .name = "disp_cc_mdss_vsync_clk_src",
> > +               .parent_data = disp_cc_parent_data_1,
> > +               .num_parents = ARRAY_SIZE(disp_cc_parent_data_1),
> > +               .flags = CLK_SET_RATE_PARENT,
> > +               .ops = &clk_rcg2_ops,
> > +       },
> > +};
> > +
[...]
> > +
> > +static struct clk_branch disp_cc_xo_clk = {
> > +       .halt_reg = 0x604c,
> > +       .halt_check = BRANCH_HALT,
> > +       .clkr = {
> > +               .enable_reg = 0x604c,
> > +               .enable_mask = BIT(0),
> > +               .hw.init = &(struct clk_init_data){
> > +                       .name = "disp_cc_xo_clk",
> > +                       .parent_hws = (const struct clk_hw*[]){
> > +                               &disp_cc_xo_clk_src.clkr.hw,
> > +                       },
> > +                       .num_parents = 1,
> > +                       .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,
>
> We need a comment why it's critical. Also I'm not sure why we would ever
> turn this clk off or change the rate. Can't we just hit some registers
> during probe to make sure it's on and drop this clk?

Yes, good point, no objection, we will lose the hierarchical clk view
for this clk (up to bi_tcxo_ao), but it does not really matter.

Regards,
Loic