clk: qcom: gdsc: Disable HW control until supported

  Software normally uses the SW_COLLAPSE bit to collapse a GDSC, but in
some scenarios it's beneficial to let the hardware perform this without
software intervention.

This is done by configuring the GDSC in "hardware control" state, in
which case the SW_COLLAPSE bit is ignored and some hardware signal is
relies upon instead.

The GDSCs are modelled as power-domains in Linux and as such it's
reasonable to assume that the device drivers intend for the hardware
block to be accessible when their power domain is active.

But in the current implementation, any GDSC that is marked to support
hardware control, gets hardware control unconditionally while the
client driver requests it to be active. It's therefor conceivable that
the hardware collapses a GDSC while Linux is accessing resources
depending on it.

There are ongoing discussions about how to properly expose this control
to the client drivers, but until conclusion in that discussion is
reached, the safer option would be to keep the GDSC in software control
mode.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/clk/qcom/gdsc.c | 48 ++++++-----------------------------------
 1 file changed, 7 insertions(+), 41 deletions(-)
  

Quoting Bjorn Andersson (2023-01-12 05:52:24)
> Software normally uses the SW_COLLAPSE bit to collapse a GDSC, but in
> some scenarios it's beneficial to let the hardware perform this without
> software intervention.
> 
> This is done by configuring the GDSC in "hardware control" state, in
> which case the SW_COLLAPSE bit is ignored and some hardware signal is
> relies upon instead.
> 
> The GDSCs are modelled as power-domains in Linux and as such it's
> reasonable to assume that the device drivers intend for the hardware
> block to be accessible when their power domain is active.
> 
> But in the current implementation, any GDSC that is marked to support
> hardware control, gets hardware control unconditionally while the
> client driver requests it to be active. It's therefor conceivable that
> the hardware collapses a GDSC while Linux is accessing resources
> depending on it.

Why would software want the GDSC to be enabled and accessing resources
while the hardware signals that it isn't required? It sounds like
hardware control isn't complete?

> 
> There are ongoing discussions about how to properly expose this control

Any link? When we implemented hardware clk gating years ago the design
was to have software override hardware control when the clk was enabled
in software and let the hardware control go into effect when the clk was
disabled in software. Hopefully with power domains this could be
implemented in a better way by connecting hardware mode to some
performance state so that enabling the power domain goes to software
mode and then transitioning to a performance state switches to hardware
control mode.

> to the client drivers, but until conclusion in that discussion is
> reached, the safer option would be to keep the GDSC in software control
> mode.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  drivers/clk/qcom/gdsc.c | 48 ++++++-----------------------------------
>  1 file changed, 7 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 9e4d6ce891aa..6d3b36a52a48 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -439,6 +398,13 @@ static int gdsc_init(struct gdsc *sc)
>                 on = true;
>         }
>  
> +       /* Disable HW trigger mode until propertly supported */
> +       if (sc->flags & HW_CTRL) {
> +               ret = gdsc_hwctrl(sc, false);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +

Is it a problem for all hardware controlled gdscs? Or just some of them?
Should we backport this to stable kernels? I seem to recall that
hardware mode was required for some drivers like camera and video? Are
they going to keep working if we simply knock out the hardware control
mode here?
  

On Thu, Jan 12, 2023 at 11:10:40AM -0800, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2023-01-12 05:52:24)
> > Software normally uses the SW_COLLAPSE bit to collapse a GDSC, but in
> > some scenarios it's beneficial to let the hardware perform this without
> > software intervention.
> > 
> > This is done by configuring the GDSC in "hardware control" state, in
> > which case the SW_COLLAPSE bit is ignored and some hardware signal is
> > relies upon instead.
> > 
> > The GDSCs are modelled as power-domains in Linux and as such it's
> > reasonable to assume that the device drivers intend for the hardware
> > block to be accessible when their power domain is active.
> > 
> > But in the current implementation, any GDSC that is marked to support
> > hardware control, gets hardware control unconditionally while the
> > client driver requests it to be active. It's therefor conceivable that
> > the hardware collapses a GDSC while Linux is accessing resources
> > depending on it.
> 
> Why would software want the GDSC to be enabled and accessing resources
> while the hardware signals that it isn't required?

Wouldn't you want a logical OR between these two? As currently written,
no attention is given to the software's need for keeping the GDSC
active.

> It sounds like hardware control isn't complete?
> 

Correct, we're lacking the means for a client driver to affect the
hardware vs software control.

> > 
> > There are ongoing discussions about how to properly expose this control
> 
> Any link? When we implemented hardware clk gating years ago the design
> was to have software override hardware control when the clk was enabled
> in software and let the hardware control go into effect when the clk was
> disabled in software.

That sounds very reasonable, but it is not what's implemented in this
file.

In gdsc_enable() we disable SW_COLLAPSE and then immediately give the
control to the hardware, and in gdsc_disable() we disable hardware
control and then set SW_COLLAPSE.

So effectively the GDSC state is either off when Linux says so, or in
hardware control.

> Hopefully with power domains this could be
> implemented in a better way by connecting hardware mode to some
> performance state so that enabling the power domain goes to software
> mode and then transitioning to a performance state switches to hardware
> control mode.
> 

Right, this would allow the software to keep the GDSC on, give the
control to the hardware or collapse it.

The question is how the "some performance state" should be implemented.

> > to the client drivers, but until conclusion in that discussion is
> > reached, the safer option would be to keep the GDSC in software control
> > mode.
> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> >  drivers/clk/qcom/gdsc.c | 48 ++++++-----------------------------------
> >  1 file changed, 7 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > index 9e4d6ce891aa..6d3b36a52a48 100644
> > --- a/drivers/clk/qcom/gdsc.c
> > +++ b/drivers/clk/qcom/gdsc.c
> > @@ -439,6 +398,13 @@ static int gdsc_init(struct gdsc *sc)
> >                 on = true;
> >         }
> >  
> > +       /* Disable HW trigger mode until propertly supported */
> > +       if (sc->flags & HW_CTRL) {
> > +               ret = gdsc_hwctrl(sc, false);
> > +               if (ret < 0)
> > +                       return ret;
> > +       }
> > +
> 
> Is it a problem for all hardware controlled gdscs? Or just some of them?
> Should we backport this to stable kernels?

Sorry, I probably wasn't clear enough. There is no observed problem,
this simply knocks out the hardware control mode.

The reason for sending this ahead of a design conclusion is that the
current behavior doesn't make sense to me (Linux says "enable!" and we
just ignore that) and consider how the "some performance state" would
relate to this, I don't see that it will be an amendment to the current
flow.

> I seem to recall that hardware mode was required for some drivers like
> camera and video?

Given that the current implementation only adhere to the hardware signal
in-between gdsc_enable() and gdsc_disable(), the drivers for these
blocks must have been written such that the software-state covers the
needs of the hardware.

As mentioned above, the opposite is however not clear. The GDSC might be
collapsed at any time, even if Linux thinks it has the GDSC
non-collapsed. I not clear to me why the current logic hasn't caused
strange issues for us over the years...

> Are they going to keep working if we simply knock out the hardware
> control mode here?

If anything, we might keep the light on longer than today by missing
opportunities where the hardware control currently collapses the GDSC
behind Linux's back - and we haven't noticed.

Regards,
Bjorn
  

On 23-01-12 15:50:38, Bjorn Andersson wrote:
> On Thu, Jan 12, 2023 at 11:10:40AM -0800, Stephen Boyd wrote:
> > Quoting Bjorn Andersson (2023-01-12 05:52:24)
> > > Software normally uses the SW_COLLAPSE bit to collapse a GDSC, but in
> > > some scenarios it's beneficial to let the hardware perform this without
> > > software intervention.
> > > 
> > > This is done by configuring the GDSC in "hardware control" state, in
> > > which case the SW_COLLAPSE bit is ignored and some hardware signal is
> > > relies upon instead.
> > > 
> > > The GDSCs are modelled as power-domains in Linux and as such it's
> > > reasonable to assume that the device drivers intend for the hardware
> > > block to be accessible when their power domain is active.
> > > 
> > > But in the current implementation, any GDSC that is marked to support
> > > hardware control, gets hardware control unconditionally while the
> > > client driver requests it to be active. It's therefor conceivable that
> > > the hardware collapses a GDSC while Linux is accessing resources
> > > depending on it.
> > 
> > Why would software want the GDSC to be enabled and accessing resources
> > while the hardware signals that it isn't required?
> 
> Wouldn't you want a logical OR between these two? As currently written,
> no attention is given to the software's need for keeping the GDSC
> active.

Looking at this more closely, it is weird nobody complained about GDSC
consumers collapsing out of the blue yet.

> 
> > It sounds like hardware control isn't complete?
> > 
> 
> Correct, we're lacking the means for a client driver to affect the
> hardware vs software control.
> 
> > > 
> > > There are ongoing discussions about how to properly expose this control
> > 
> > Any link? When we implemented hardware clk gating years ago the design
> > was to have software override hardware control when the clk was enabled
> > in software and let the hardware control go into effect when the clk was
> > disabled in software.

Discussion is off list for now.

> 
> That sounds very reasonable, but it is not what's implemented in this
> file.
> 
> In gdsc_enable() we disable SW_COLLAPSE and then immediately give the
> control to the hardware, and in gdsc_disable() we disable hardware
> control and then set SW_COLLAPSE.
> 
> So effectively the GDSC state is either off when Linux says so, or in
> hardware control.
> 

The discussed solution is the have a generic genpd API that is
specifically for marking a PD in HW-controlled mode, while keeping other
resources enabled from the consumer driver.

> > Hopefully with power domains this could be
> > implemented in a better way by connecting hardware mode to some
> > performance state so that enabling the power domain goes to software
> > mode and then transitioning to a performance state switches to hardware
> > control mode.
> > 
> 
> Right, this would allow the software to keep the GDSC on, give the
> control to the hardware or collapse it.
> 
> The question is how the "some performance state" should be implemented.
> 
> > > to the client drivers, but until conclusion in that discussion is
> > > reached, the safer option would be to keep the GDSC in software control
> > > mode.
> > > 
> > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > ---
> > >  drivers/clk/qcom/gdsc.c | 48 ++++++-----------------------------------
> > >  1 file changed, 7 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > index 9e4d6ce891aa..6d3b36a52a48 100644
> > > --- a/drivers/clk/qcom/gdsc.c
> > > +++ b/drivers/clk/qcom/gdsc.c
> > > @@ -439,6 +398,13 @@ static int gdsc_init(struct gdsc *sc)
> > >                 on = true;
> > >         }
> > >  
> > > +       /* Disable HW trigger mode until propertly supported */
> > > +       if (sc->flags & HW_CTRL) {
> > > +               ret = gdsc_hwctrl(sc, false);
> > > +               if (ret < 0)
> > > +                       return ret;
> > > +       }
> > > +
> > 
> > Is it a problem for all hardware controlled gdscs? Or just some of them?
> > Should we backport this to stable kernels?
> 
> Sorry, I probably wasn't clear enough. There is no observed problem,
> this simply knocks out the hardware control mode.
> 
> The reason for sending this ahead of a design conclusion is that the
> current behavior doesn't make sense to me (Linux says "enable!" and we
> just ignore that) and consider how the "some performance state" would
> relate to this, I don't see that it will be an amendment to the current
> flow.

I agree. The fact that this did not create any issues yet doesn't mean
we should stick with the current implementation. In fact, disabling
HW-control altogether (for now) is more reasonable.

> 
> > I seem to recall that hardware mode was required for some drivers like
> > camera and video?
> 
> Given that the current implementation only adhere to the hardware signal
> in-between gdsc_enable() and gdsc_disable(), the drivers for these
> blocks must have been written such that the software-state covers the
> needs of the hardware.
> 
> As mentioned above, the opposite is however not clear. The GDSC might be
> collapsed at any time, even if Linux thinks it has the GDSC
> non-collapsed. I not clear to me why the current logic hasn't caused
> strange issues for us over the years...
> 
> > Are they going to keep working if we simply knock out the hardware
> > control mode here?
> 
> If anything, we might keep the light on longer than today by missing
> opportunities where the hardware control currently collapses the GDSC
> behind Linux's back - and we haven't noticed.
> 
> Regards,
> Bjorn
  

On 23-01-12 05:52:24, Bjorn Andersson wrote:
> Software normally uses the SW_COLLAPSE bit to collapse a GDSC, but in
> some scenarios it's beneficial to let the hardware perform this without
> software intervention.
> 
> This is done by configuring the GDSC in "hardware control" state, in
> which case the SW_COLLAPSE bit is ignored and some hardware signal is
> relies upon instead.
> 
> The GDSCs are modelled as power-domains in Linux and as such it's
> reasonable to assume that the device drivers intend for the hardware
> block to be accessible when their power domain is active.
> 
> But in the current implementation, any GDSC that is marked to support
> hardware control, gets hardware control unconditionally while the
> client driver requests it to be active. It's therefor conceivable that
> the hardware collapses a GDSC while Linux is accessing resources
> depending on it.
> 
> There are ongoing discussions about how to properly expose this control
> to the client drivers, but until conclusion in that discussion is
> reached, the safer option would be to keep the GDSC in software control
> mode.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

> ---
>  drivers/clk/qcom/gdsc.c | 48 ++++++-----------------------------------
>  1 file changed, 7 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 9e4d6ce891aa..6d3b36a52a48 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -291,22 +291,6 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>  	 */
>  	udelay(1);
>  
> -	/* Turn on HW trigger mode if supported */
> -	if (sc->flags & HW_CTRL) {
> -		ret = gdsc_hwctrl(sc, true);
> -		if (ret)
> -			return ret;
> -		/*
> -		 * Wait for the GDSC to go through a power down and
> -		 * up cycle.  In case a firmware ends up polling status
> -		 * bits for the gdsc, it might read an 'on' status before
> -		 * the GDSC can finish the power cycle.
> -		 * We wait 1us before returning to ensure the firmware
> -		 * can't immediately poll the status bits.
> -		 */
> -		udelay(1);
> -	}
> -
>  	if (sc->flags & RETAIN_FF_ENABLE)
>  		gdsc_retain_ff_on(sc);
>  
> @@ -321,24 +305,6 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>  	if (sc->pwrsts == PWRSTS_ON)
>  		return gdsc_assert_reset(sc);
>  
> -	/* Turn off HW trigger mode if supported */
> -	if (sc->flags & HW_CTRL) {
> -		ret = gdsc_hwctrl(sc, false);
> -		if (ret < 0)
> -			return ret;
> -		/*
> -		 * Wait for the GDSC to go through a power down and
> -		 * up cycle.  In case we end up polling status
> -		 * bits for the gdsc before the power cycle is completed
> -		 * it might read an 'on' status wrongly.
> -		 */
> -		udelay(1);
> -
> -		ret = gdsc_poll_status(sc, GDSC_ON);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	if (sc->pwrsts & PWRSTS_OFF)
>  		gdsc_clear_mem_on(sc);
>  
> @@ -419,13 +385,6 @@ static int gdsc_init(struct gdsc *sc)
>  				goto err_disable_supply;
>  		}
>  
> -		/* Turn on HW trigger mode if supported */
> -		if (sc->flags & HW_CTRL) {
> -			ret = gdsc_hwctrl(sc, true);
> -			if (ret < 0)
> -				goto err_disable_supply;
> -		}
> -
>  		/*
>  		 * Make sure the retain bit is set if the GDSC is already on,
>  		 * otherwise we end up turning off the GDSC and destroying all
> @@ -439,6 +398,13 @@ static int gdsc_init(struct gdsc *sc)
>  		on = true;
>  	}
>  
> +	/* Disable HW trigger mode until propertly supported */
> +	if (sc->flags & HW_CTRL) {
> +		ret = gdsc_hwctrl(sc, false);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	if (on || (sc->pwrsts & PWRSTS_RET))
>  		gdsc_force_mem_on(sc);
>  	else
> -- 
> 2.37.3
>
  

Quoting Abel Vesa (2023-01-27 08:15:03)
> On 23-01-12 15:50:38, Bjorn Andersson wrote:
> > On Thu, Jan 12, 2023 at 11:10:40AM -0800, Stephen Boyd wrote:
> > > Quoting Bjorn Andersson (2023-01-12 05:52:24)
> > > > Software normally uses the SW_COLLAPSE bit to collapse a GDSC, but in
> > > > some scenarios it's beneficial to let the hardware perform this without
> > > > software intervention.
> > > > 
> > > > This is done by configuring the GDSC in "hardware control" state, in
> > > > which case the SW_COLLAPSE bit is ignored and some hardware signal is
> > > > relies upon instead.
> > > > 
> > > > The GDSCs are modelled as power-domains in Linux and as such it's
> > > > reasonable to assume that the device drivers intend for the hardware
> > > > block to be accessible when their power domain is active.
> > > > 
> > > > But in the current implementation, any GDSC that is marked to support
> > > > hardware control, gets hardware control unconditionally while the
> > > > client driver requests it to be active. It's therefor conceivable that
> > > > the hardware collapses a GDSC while Linux is accessing resources
> > > > depending on it.
> > > 
> > > Why would software want the GDSC to be enabled and accessing resources
> > > while the hardware signals that it isn't required?
> > 
> > Wouldn't you want a logical OR between these two? As currently written,
> > no attention is given to the software's need for keeping the GDSC
> > active.
> 
> Looking at this more closely, it is weird nobody complained about GDSC
> consumers collapsing out of the blue yet.

Probably the hardware requests the gdsc to be enabled by default and
only drops the request when it knows it is idle and not doing anything?
I'm imagining an open drain sort of signal where the hardware pulls the
line when it wants to power down and then lets go when it wants to
operate normally.

> 
> > 
> > > It sounds like hardware control isn't complete?
> > > 
> > 
> > Correct, we're lacking the means for a client driver to affect the
> > hardware vs software control.
> > 
> > > > 
> > > > There are ongoing discussions about how to properly expose this control
> > > 
> > > Any link? When we implemented hardware clk gating years ago the design
> > > was to have software override hardware control when the clk was enabled
> > > in software and let the hardware control go into effect when the clk was
> > > disabled in software.
> 
> Discussion is off list for now.

😎

> 
> > 
> > That sounds very reasonable, but it is not what's implemented in this
> > file.
> > 
> > In gdsc_enable() we disable SW_COLLAPSE and then immediately give the
> > control to the hardware, and in gdsc_disable() we disable hardware
> > control and then set SW_COLLAPSE.
> > 
> > So effectively the GDSC state is either off when Linux says so, or in
> > hardware control.
> > 
> 
> The discussed solution is the have a generic genpd API that is
> specifically for marking a PD in HW-controlled mode, while keeping other
> resources enabled from the consumer driver.

Alright.

> 
> > > Hopefully with power domains this could be
> > > implemented in a better way by connecting hardware mode to some
> > > performance state so that enabling the power domain goes to software
> > > mode and then transitioning to a performance state switches to hardware
> > > control mode.
> > > 
> > 
> > Right, this would allow the software to keep the GDSC on, give the
> > control to the hardware or collapse it.
> > 
> > The question is how the "some performance state" should be implemented.
> > 
> > > > to the client drivers, but until conclusion in that discussion is
> > > > reached, the safer option would be to keep the GDSC in software control
> > > > mode.
> > > > 
> > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > > ---
> > > >  drivers/clk/qcom/gdsc.c | 48 ++++++-----------------------------------
> > > >  1 file changed, 7 insertions(+), 41 deletions(-)
> > > > 
> > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > > index 9e4d6ce891aa..6d3b36a52a48 100644
> > > > --- a/drivers/clk/qcom/gdsc.c
> > > > +++ b/drivers/clk/qcom/gdsc.c
> > > > @@ -439,6 +398,13 @@ static int gdsc_init(struct gdsc *sc)
> > > >                 on = true;
> > > >         }
> > > >  
> > > > +       /* Disable HW trigger mode until propertly supported */
> > > > +       if (sc->flags & HW_CTRL) {
> > > > +               ret = gdsc_hwctrl(sc, false);
> > > > +               if (ret < 0)
> > > > +                       return ret;
> > > > +       }
> > > > +
> > > 
> > > Is it a problem for all hardware controlled gdscs? Or just some of them?
> > > Should we backport this to stable kernels?
> > 
> > Sorry, I probably wasn't clear enough. There is no observed problem,
> > this simply knocks out the hardware control mode.
> > 
> > The reason for sending this ahead of a design conclusion is that the
> > current behavior doesn't make sense to me (Linux says "enable!" and we
> > just ignore that) and consider how the "some performance state" would
> > relate to this, I don't see that it will be an amendment to the current
> > flow.
> 
> I agree. The fact that this did not create any issues yet doesn't mean
> we should stick with the current implementation. In fact, disabling
> HW-control altogether (for now) is more reasonable.

Any change needs testing. For all I know, this breaks drivers or
firmware that are expecting power control to be hardware based (e.g.
maybe video firmware is polling the gdsc to make sure it turns off). Or
it causes a large power regression in the active use case because now
the gdsc is on far more often. Has any of this sort of testing been
done? Changing things because they don't make sense is not a great
argument.

> 
> > 
> > > I seem to recall that hardware mode was required for some drivers like
> > > camera and video?
> > 
> > Given that the current implementation only adhere to the hardware signal
> > in-between gdsc_enable() and gdsc_disable(), the drivers for these
> > blocks must have been written such that the software-state covers the
> > needs of the hardware.
> > 
> > As mentioned above, the opposite is however not clear. The GDSC might be
> > collapsed at any time, even if Linux thinks it has the GDSC
> > non-collapsed. I not clear to me why the current logic hasn't caused
> > strange issues for us over the years...

Ok, but it hasn't caused us any issues so far. For all I know this patch
will cause problems. I don't see the harm in waiting for the final
solution to appear at a later time.

> > 
> > > Are they going to keep working if we simply knock out the hardware
> > > control mode here?
> > 
> > If anything, we might keep the light on longer than today by missing
> > opportunities where the hardware control currently collapses the GDSC
> > behind Linux's back - and we haven't noticed.
> > 

Yep!