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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On Fri, 17 Dec 2021 at 04:37, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Loic Poulain (2021-12-16 11:21:51)
> > 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.
>
> Ok then please remove it.
>
> >
> > 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.
>
> That's quite odd. We migrate the prepare and enable count to the new
> parent in the core framework so is the rcg on, but doesn't look like it
> is on to the core and set_rate is being called?

It's a part of a typical problem for some clocks (that we were
discussing in a thread regarding Bjorn's ASSUME_ENABLED and my 'park
clocks' proposal). For set_rate and set_parent to succeed, both old
and new parrents must be running at the time of the operation/



-- 
With best wishes
Dmitry