diff mbox series

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

Message ID 20240711113239.3063546-4-quic_varada@quicinc.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series Add interconnect driver for IPQ5332 SoC | expand

Commit Message

Varadarajan Narayanan July 11, 2024, 11:32 a.m. UTC
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(-)

Comments

Konrad Dybcio July 12, 2024, 12:24 p.m. UTC | #1
On 11.07.2024 1:32 PM, 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>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
Dmitry Baryshkov July 13, 2024, 4:21 p.m. UTC | #2
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?

>  			 */
> -			.flags = CLK_IGNORE_UNUSED,
> +			.flags = CLK_IS_CRITICAL,
>  		},
>  	},
>  };
Varadarajan Narayanan July 18, 2024, 10:42 a.m. UTC | #3
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
Dmitry Baryshkov July 18, 2024, 10:47 a.m. UTC | #4
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?

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.
Varadarajan Narayanan July 20, 2024, 9:02 a.m. UTC | #5
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.

Thanks
Varada
Dmitry Baryshkov July 20, 2024, 9:05 a.m. UTC | #6
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.
Varadarajan Narayanan July 22, 2024, 6:05 a.m. UTC | #7
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
diff mbox series

Patch

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.
 			 */
-			.flags = CLK_IGNORE_UNUSED,
+			.flags = CLK_IS_CRITICAL,
 		},
 	},
 };
@@ -3628,6 +3632,24 @@  static const struct qcom_reset_map gcc_ipq5332_resets[] = {
 	[GCC_UNIPHY1_XPCS_ARES] = { 0x16060 },
 };
 
+#define IPQ_APPS_ID			5332	/* some unique value */
+
+static struct qcom_icc_hws_data icc_ipq5332_hws[] = {
+	{ MASTER_SNOC_PCIE3_1_M, SLAVE_SNOC_PCIE3_1_M, GCC_SNOC_PCIE3_1LANE_M_CLK },
+	{ MASTER_ANOC_PCIE3_1_S, SLAVE_ANOC_PCIE3_1_S, GCC_SNOC_PCIE3_1LANE_S_CLK },
+	{ MASTER_SNOC_PCIE3_2_M, SLAVE_SNOC_PCIE3_2_M, GCC_SNOC_PCIE3_2LANE_M_CLK },
+	{ MASTER_ANOC_PCIE3_2_S, SLAVE_ANOC_PCIE3_2_S, GCC_SNOC_PCIE3_2LANE_S_CLK },
+	{ MASTER_SNOC_USB, SLAVE_SNOC_USB, GCC_SNOC_USB_CLK },
+	{ MASTER_NSSNOC_NSSCC, SLAVE_NSSNOC_NSSCC, GCC_NSSNOC_NSSCC_CLK },
+	{ MASTER_NSSNOC_SNOC_0, SLAVE_NSSNOC_SNOC_0, GCC_NSSNOC_SNOC_CLK },
+	{ MASTER_NSSNOC_SNOC_1, SLAVE_NSSNOC_SNOC_1, GCC_NSSNOC_SNOC_1_CLK },
+	{ MASTER_NSSNOC_ATB, SLAVE_NSSNOC_ATB, GCC_NSSNOC_ATB_CLK },
+	{ MASTER_NSSNOC_PCNOC_1, SLAVE_NSSNOC_PCNOC_1, GCC_NSSNOC_PCNOC_1_CLK },
+	{ MASTER_NSSNOC_QOSGEN_REF, SLAVE_NSSNOC_QOSGEN_REF, GCC_NSSNOC_QOSGEN_REF_CLK },
+	{ MASTER_NSSNOC_TIMEOUT_REF, SLAVE_NSSNOC_TIMEOUT_REF, GCC_NSSNOC_TIMEOUT_REF_CLK },
+	{ MASTER_NSSNOC_XO_DCD, SLAVE_NSSNOC_XO_DCD, GCC_NSSNOC_XO_DCD_CLK },
+};
+
 static const struct regmap_config gcc_ipq5332_regmap_config = {
 	.reg_bits = 32,
 	.reg_stride = 4,
@@ -3656,6 +3678,9 @@  static const struct qcom_cc_desc gcc_ipq5332_desc = {
 	.num_resets = ARRAY_SIZE(gcc_ipq5332_resets),
 	.clk_hws = gcc_ipq5332_hws,
 	.num_clk_hws = ARRAY_SIZE(gcc_ipq5332_hws),
+	.icc_hws = icc_ipq5332_hws,
+	.num_icc_hws = ARRAY_SIZE(icc_ipq5332_hws),
+	.icc_first_node_id = IPQ_APPS_ID,
 };
 
 static int gcc_ipq5332_probe(struct platform_device *pdev)
@@ -3674,6 +3699,7 @@  static struct platform_driver gcc_ipq5332_driver = {
 	.driver = {
 		.name = "gcc-ipq5332",
 		.of_match_table = gcc_ipq5332_match_table,
+		.sync_state = icc_sync_state,
 	},
 };