Re: [PATCH v2 3/4] clk: qcom: ipq5332: Use icc-clk for enabling NoC related clocks

[Varadarajan Narayanan <quic_varada@quicinc.com>]

On Sat, Jul 20, 2024 at 12:05:59PM +0300, Dmitry Baryshkov wrote:
> On Sat, 20 Jul 2024 at 12:02, Varadarajan Narayanan
> <quic_varada@quicinc.com> wrote:
> >
> > On Thu, Jul 18, 2024 at 01:47:32PM +0300, Dmitry Baryshkov wrote:
> > > On Thu, 18 Jul 2024 at 13:42, Varadarajan Narayanan
> > > <quic_varada@quicinc.com> wrote:
> > > >
> > > > On Sat, Jul 13, 2024 at 07:21:29PM +0300, Dmitry Baryshkov wrote:
> > > > > On Thu, Jul 11, 2024 at 05:02:38PM GMT, Varadarajan Narayanan wrote:
> > > > > > Use the icc-clk framework to enable few clocks to be able to
> > > > > > create paths and use the peripherals connected on those NoCs.
> > > > > >
> > > > > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > > > > > ---
> > > > > >  drivers/clk/qcom/gcc-ipq5332.c | 36 +++++++++++++++++++++++++++++-----
> > > > > >  1 file changed, 31 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/clk/qcom/gcc-ipq5332.c b/drivers/clk/qcom/gcc-ipq5332.c
> > > > > > index f98591148a97..6d7672cae0f7 100644
> > > > > > --- a/drivers/clk/qcom/gcc-ipq5332.c
> > > > > > +++ b/drivers/clk/qcom/gcc-ipq5332.c
> > > > > > @@ -4,12 +4,14 @@
> > > > > >   */
> > > > > >
> > > > > >  #include <linux/clk-provider.h>
> > > > > > +#include <linux/interconnect-provider.h>
> > > > > >  #include <linux/mod_devicetable.h>
> > > > > >  #include <linux/module.h>
> > > > > >  #include <linux/platform_device.h>
> > > > > >  #include <linux/regmap.h>
> > > > > >
> > > > > >  #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> > > > > > +#include <dt-bindings/interconnect/qcom,ipq5332.h>
> > > > > >
> > > > > >  #include "clk-alpha-pll.h"
> > > > > >  #include "clk-branch.h"
> > > > > > @@ -131,12 +133,14 @@ static struct clk_alpha_pll gpll4_main = {
> > > > > >                      * (will be added soon), so the clock framework
> > > > > >                      * disables this source. But some of the clocks
> > > > > >                      * initialized by boot loaders uses this source. So we
> > > > > > -                    * need to keep this clock ON. Add the
> > > > > > -                    * CLK_IGNORE_UNUSED flag so the clock will not be
> > > > > > -                    * disabled. Once the consumer in kernel is added, we
> > > > > > -                    * can get rid of this flag.
> > > > > > +                    * need to keep this clock ON.
> > > > > > +                    *
> > > > > > +                    * After initial bootup, when the ICC framework turns
> > > > > > +                    * off unused paths, as part of the icc-clk dependencies
> > > > > > +                    * this clock gets disabled resulting in a hang. Marking
> > > > > > +                    * it as critical to ensure it is not turned off.
> > > > >
> > > > > Previous comment was pretty clear: there are missing consumers, the flag
> > > > > will be removed once they are added. Current comment doesn't make sense.
> > > > > What is the reason for the device hang if we have all the consumers in
> > > > > place?
> > > >
> > > > Earlier, since there were no consumers for this clock, it got
> > > > disabled via clk_disable_unused() and CLK_IGNORE_UNUSED helped
> > > > prevent that.
> > > >
> > > > Now, since this clk is getting used indirectly via icc-clk
> > > > framework, it doesn't qualify for being disabled by
> > > > clk_disable_unused(). However, when icc_sync_state is called, if
> > > > it sees there are no consumers for a path and that path gets
> > > > disabled, the relevant clocks get disabled and eventually this
> > > > clock also gets disabled. To avoid this have changed 'flags' from
> > > > CLK_IGNORE_UNUSED -> CLK_IS_CRITICAL.
> > >
> > > You don't seem to be answering my question: "What is the reason for
> > > the device hang if we have all the consumers in place?"
> > > Could you please answer it rather than describing the CCF / icc-clk behaviour?
> >
> > Sorry if I hadn't expressed myself clearly. All the consumers are
> > not there in place yet.
> >
> > > Are there any actual consumers for GPLL4 at this point? If not, why do
> > > you want to keep it running? Usually all PLLs are shut down when there
> > > are no consumers and then restarted when required. This is the
> > > expected and correct behaviour.
> >
> > There are consumers for GPLL4, but they are getting disabled by
> > clk_disable_unused (this is expected). There seems to be some
> > consumer that got enabled in boot loader itself but not accounted
> > in Linux because of which we are relying on CLK_IGNORE_UNUSED.
> >
> > If missing consumer(s) is identified, we can do away with this
> > flag. Till that is done, was hoping CLK_IS_CRITICAL could help.
>
> NAK, please identify missing consumers instead of landing workarounds.

Please review v3, have identified the missing consumer.

Thanks
Varada