Re: [PATCH] clk: qcom: gdsc: Disable HW control until supported

[Abel Vesa <abel.vesa@linaro.org>]

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