Date:   Fri, 27 Jan 2023 18:15:22 +0200
From:   Abel Vesa <abel.vesa@linaro.org>
To:     Bjorn Andersson <quic_bjorande@quicinc.com>
Cc:     Bjorn Andersson <andersson@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Taniya Das <quic_tdas@quicinc.com>,
        linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] clk: qcom: gdsc: Disable HW control until supported
Message-ID: <Y9P4mhSIK1BvZ38k@linaro.org>
References: <20230112135224.3837820-1-quic_bjorande@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230112135224.3837820-1-quic_bjorande@quicinc.com>
Precedence: bulk

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
>