| Message ID | 20230112135224.3837820-1-quic_bjorande@quicinc.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | clk: qcom: gdsc: Disable HW control until supported | expand |
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.
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
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(-)