clk: qcom: clk-rpmh: Wait for completion when enabling clocks

The current implementation always uses rpmh_write_async, which doesn't
wait for completion. That's fine for disable requests since there's no
immediate need for the clocks and they can be disabled in the
background. However, for enable requests we need to ensure the clocks
are actually enabled before returning to the client. Otherwise, clients
can end up accessing their HW before the necessary clocks are enabled,
which can lead to bus errors.

Use the synchronous version of this API (rpmh_write) for enable requests
in the active set to ensure completion.

Completion isn't required for sleep/wake sets, since they don't take
effect until after we enter sleep. All rpmh requests are automatically
flushed prior to entering sleep.

Fixes: 9c7e47025a6b ("clk: qcom: clk-rpmh: Add QCOM RPMh clock driver")
Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>
---
 drivers/clk/qcom/clk-rpmh.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

On Fri 14 Feb 18:12 PST 2020, Mike Tipton wrote:

> The current implementation always uses rpmh_write_async, which doesn't
> wait for completion. That's fine for disable requests since there's no
> immediate need for the clocks and they can be disabled in the
> background. However, for enable requests we need to ensure the clocks
> are actually enabled before returning to the client. Otherwise, clients
> can end up accessing their HW before the necessary clocks are enabled,
> which can lead to bus errors.
> 
> Use the synchronous version of this API (rpmh_write) for enable requests
> in the active set to ensure completion.
> 
> Completion isn't required for sleep/wake sets, since they don't take
> effect until after we enter sleep. All rpmh requests are automatically
> flushed prior to entering sleep.
> 
> Fixes: 9c7e47025a6b ("clk: qcom: clk-rpmh: Add QCOM RPMh clock driver")
> Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/clk/qcom/clk-rpmh.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> index 12bd8715dece..3137595a736b 100644
> --- a/drivers/clk/qcom/clk-rpmh.c
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -143,6 +143,19 @@ static inline bool has_state_changed(struct clk_rpmh *c, u32 state)
>  		!= (c->aggr_state & BIT(state));
>  }
>  
> +static int clk_rpmh_send(struct clk_rpmh *c, enum rpmh_state state,
> +			 struct tcs_cmd *cmd, bool wait_for_completion)
> +{
> +	int ret;
> +
> +	if (wait_for_completion)
> +		ret = rpmh_write(c->dev, state, cmd, 1);
> +	else
> +		ret = rpmh_write_async(c->dev, state, cmd, 1);
> +
> +	return ret;
> +}
> +
>  static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
>  {
>  	struct tcs_cmd cmd = { 0 };
> @@ -159,7 +172,8 @@ static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
>  			if (cmd_state & BIT(state))
>  				cmd.data = on_val;
>  
> -			ret = rpmh_write_async(c->dev, state, &cmd, 1);
> +			ret = clk_rpmh_send(c, state, &cmd,
> +				cmd_state && state == RPMH_ACTIVE_ONLY_STATE);
>  			if (ret) {
>  				dev_err(c->dev, "set %s state of %s failed: (%d)\n",
>  					!state ? "sleep" :
> @@ -267,7 +281,7 @@ static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
>  	cmd.addr = c->res_addr;
>  	cmd.data = BCM_TCS_CMD(1, enable, 0, cmd_state);
>  
> -	ret = rpmh_write_async(c->dev, RPMH_ACTIVE_ONLY_STATE, &cmd, 1);
> +	ret = clk_rpmh_send(c, RPMH_ACTIVE_ONLY_STATE, &cmd, enable);
>  	if (ret) {
>  		dev_err(c->dev, "set active state of %s failed: (%d)\n",
>  			c->res_name, ret);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Quoting Mike Tipton (2020-02-14 18:12:32)
> The current implementation always uses rpmh_write_async, which doesn't
> wait for completion. That's fine for disable requests since there's no
> immediate need for the clocks and they can be disabled in the
> background. However, for enable requests we need to ensure the clocks
> are actually enabled before returning to the client. Otherwise, clients
> can end up accessing their HW before the necessary clocks are enabled,
> which can lead to bus errors.
> 
> Use the synchronous version of this API (rpmh_write) for enable requests
> in the active set to ensure completion.
> 
> Completion isn't required for sleep/wake sets, since they don't take
> effect until after we enter sleep. All rpmh requests are automatically
> flushed prior to entering sleep.
> 
> Fixes: 9c7e47025a6b ("clk: qcom: clk-rpmh: Add QCOM RPMh clock driver")
> Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>
> ---

Applied to clk-next but I squashed in some changes to make it easier for
me to read.

Hi,

On Mon, Mar 9, 2020 at 2:11 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Mike Tipton (2020-02-14 18:12:32)
> > The current implementation always uses rpmh_write_async, which doesn't
> > wait for completion. That's fine for disable requests since there's no
> > immediate need for the clocks and they can be disabled in the
> > background. However, for enable requests we need to ensure the clocks
> > are actually enabled before returning to the client. Otherwise, clients
> > can end up accessing their HW before the necessary clocks are enabled,
> > which can lead to bus errors.
> >
> > Use the synchronous version of this API (rpmh_write) for enable requests
> > in the active set to ensure completion.
> >
> > Completion isn't required for sleep/wake sets, since they don't take
> > effect until after we enter sleep. All rpmh requests are automatically
> > flushed prior to entering sleep.
> >
> > Fixes: 9c7e47025a6b ("clk: qcom: clk-rpmh: Add QCOM RPMh clock driver")
> > Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>
> > ---
>
> Applied to clk-next but I squashed in some changes to make it easier for
> me to read.

This landed upstream as commit dad4e7fda4bd ("clk: qcom: clk-rpmh:
Wait for completion when enabling clocks") but seemed to have missed
stable.  Can stable pick it up?  It has a Fixes tag so presumably it
should be easy to track down where it needs to go.

Thanks!

-Doug