Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <cb0c6fbd-bf5c-b5ce-b4c5-8a605836204d@quicinc.com>
Date:   Mon, 8 Aug 2022 13:47:02 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] soc: qcom: rpmhpd: Use highest corner until sync_state
Content-Language: en-US
To:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>
CC:     <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Abel Vesa <abel.vesa@linaro.org>
References: <20220806155035.968340-1-bjorn.andersson@linaro.org>
From:   Rajendra Nayak <quic_rjendra@quicinc.com>
In-Reply-To: <20220806155035.968340-1-bjorn.andersson@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 8/6/2022 9:20 PM, Bjorn Andersson wrote:
> In some cases the hardware that the bootloader has left configured
> depends on RPMH power domains for their operation up until the point
> where the related Linux device driver probes and can inherit that
> configuration, or power down the hardware gracefully.
> 
> Unfortunately as Linux probes the releavant drivers in sequence there
> are periods during the Linux boot flow where either the genpd refcount
> will reach 0, or worse where the active performance_state votes does not
> meet the requirements of the state that the hardware was left in.
> 
> One specific example of this is during boot of e.g. SM8150/SC8180X,
> where the display clock controller probes, without any particular
> performance state needs (to access its registers). This will drop the
> MMCX rail to MIN_SVS, which isn't sufficient to sustain the clock rates
> that the later probing MDP is configured to. This results in an
> unrecoverable system state.
> 
> Handle both these cases by keeping the RPMH power-domais that are
> referenced voted for highest state, until sync_state indicates that all
> devices referencing the RPMH power-domain driver has been probed.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>   drivers/soc/qcom/rpmhpd.c | 35 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
> index 092f6ab09acf..494bb6c75ed7 100644
> --- a/drivers/soc/qcom/rpmhpd.c
> +++ b/drivers/soc/qcom/rpmhpd.c
> @@ -39,6 +39,7 @@
>    * @res_name:		Resource name used for cmd-db lookup
>    * @addr:		Resource address as looped up using resource name from
>    *			cmd-db
> + * @state_synced:       Indicator that sync_state has been invoked for the rpmhpd resource
>    */
>   struct rpmhpd {
>   	struct device	*dev;
> @@ -54,6 +55,7 @@ struct rpmhpd {
>   	bool		enabled;
>   	const char	*res_name;
>   	u32		addr;
> +	bool		state_synced;
>   };
>   
>   struct rpmhpd_desc {
> @@ -493,7 +495,13 @@ static int rpmhpd_aggregate_corner(struct rpmhpd *pd, unsigned int corner)
>   	unsigned int this_active_corner = 0, this_sleep_corner = 0;
>   	unsigned int peer_active_corner = 0, peer_sleep_corner = 0;
>   
> -	to_active_sleep(pd, corner, &this_active_corner, &this_sleep_corner);
> +	if (pd->state_synced) {
> +		to_active_sleep(pd, corner, &this_active_corner, &this_sleep_corner);
> +	} else {
> +		/* Clamp to highest corner if sync_state hasn't happened */
> +		this_active_corner = pd->level_count - 1;
> +		this_sleep_corner = pd->level_count - 1;
> +	}
>   
>   	if (peer && peer->enabled)
>   		to_active_sleep(peer, peer->corner, &peer_active_corner,
> @@ -708,11 +716,36 @@ static int rpmhpd_probe(struct platform_device *pdev)
>   	return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
>   }
>   
> +static void rpmhpd_sync_state(struct device *dev)
> +{
> +	const struct rpmhpd_desc *desc = of_device_get_match_data(dev);
> +	struct rpmhpd **rpmhpds = desc->rpmhpds;
> +	unsigned int corner;
> +	struct rpmhpd *pd;
> +	unsigned int i;
> +
> +	mutex_lock(&rpmhpd_lock);
> +	for (i = 0; i < desc->num_pds; i++) {
> +		pd = rpmhpds[i];
> +		if (!pd)
> +			continue;
> +
> +		pd->state_synced = true;
[]

> +		if (pd->enabled)
> +			corner = max(pd->corner, pd->enable_corner);
> +		else
> +			corner = 0;
> +		rpmhpd_aggregate_corner(pd, corner);

can't this be
		if (pd->enabled) {
			corner = max(pd->corner, pd->enable_corner);
			rpmhpd_aggregate_corner(pd, corner);
		}

also, we do check for return value of rpmhpd_aggregate_corner() but I guess
here there isn't much different we would do even if there is an error?
			
> +	}
> +	mutex_unlock(&rpmhpd_lock);
> +}
> +
>   static struct platform_driver rpmhpd_driver = {
>   	.driver = {
>   		.name = "qcom-rpmhpd",
>   		.of_match_table = rpmhpd_match_table,
>   		.suppress_bind_attrs = true,
> +		.sync_state = rpmhpd_sync_state,
>   	},
>   	.probe = rpmhpd_probe,
>   };