On 6.08.2022 17:50, 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@xxxxxxxxxx>
> ---
Must have been fun to debug..
Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxx>
Konrad
> 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);
> + }
> + 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,
> };
