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@xxxxxxxxxxx>
Reviewed-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> ---
> 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
>
