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 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.

Thanks
Varada

> >  			 */
> > -			.flags = CLK_IGNORE_UNUSED,
> > +			.flags = CLK_IS_CRITICAL,
> >  		},
> >  	},
> >  };
>
>
> --
> With best wishes
> Dmitry