Re: [PATCH v2] drm/bridge: cdns-mhdp8546: Add suspend resume support to the bridge driver

["Kumar, Abhash" <a-kumar2@ti.com>]

Hi Devarsh,

Thanks for the review

Posted a v3 here: [PATCH v3] drm/bridge: cdns-mhdp8546: Add suspend 
resume support to the bridge driver - Abhash Kumar Jha 
<https://lore.kernel.org/all/20260601095041.3042950-1-a-kumar2@ti.com/>

>> +    } else {
>> +        ret = cdns_mhdp_set_firmware_active(mhdp, true);
>> +        if (ret) {
>> +            dev_err(mhdp->dev, "Failed to activate firmware 
>> (%pe)\n", ERR_PTR(ret));
>> +            goto phy_off;
>> +        }
>> +    }
>> +
>
> mhdp->powered_off = false ??

Good catch, updated in v3.

>
>> +    }
>> +    mhdp->hw_state = MHDP_HW_STOPPED;
>
> Above should be inside if block i..e if hw state was not ready.
Yes, updated this in v3
>
>> + spin_unlock(&mhdp->start_lock);
>> +
>> +    if (ret == 0) {
>> +        dev_err(mhdp->dev, "%s: Timeout waiting for fw loading\n", 
>> __func__);
>> +        ret = -ETIMEDOUT;
>> +        goto error;
>> +    } else {
>> +        ret = cdns_mhdp_set_firmware_active(mhdp, false);
>> +        if (ret) {
>> +            dev_err(mhdp->dev, "Failed to stop firmware (%pe)\n", 
>> ERR_PTR(ret));
>
> At this point don;t you need to
> phy_power_off/phy_exit/clk_disable_unprepare, and restore hw_state to 
> MHDP_HW_READY ?

I think we should only restore the hw_state to MHDP_HW_READY.

We should not do phy_power_off/phy_exit/clk_disable_unprepare in case of 
a failure in suspend

because the device is still active. So the driver can be used and a 
suspend can be triggered later.


>> +            goto error;
>> +        }
>> +    }
>> +
>> +    phy_power_off(mhdp->phy);
>> +    phy_exit(mhdp->phy);
>> +    clk_disable_unprepare(mhdp->clk);
>> +
>> +error:
>> +    return ret;
>> +}
>> +
>> +static int mhdp_pd_notifier_cb(struct notifier_block *nb,
>> +            unsigned long action, void *data)
>> +{
>> +    struct cdns_mhdp_device *mhdp = container_of(nb, struct 
>> cdns_mhdp_device, pd_nb);
>> +
>> +    if (action == GENPD_NOTIFY_OFF)
>> +        mhdp->powered_off = true;
>
> Should we reset powered_off flag for GENPD_NOTIFY_ON?

I have reset the mhdp->powered_off to false in the resume in the v3.

>> +
>> +    return 0;
>> +}
>> +
>> +static const struct dev_pm_ops cdns_mhdp_pm_ops = {
>> +    SET_SYSTEM_SLEEP_PM_OPS(cdns_mhdp_suspend, cdns_mhdp_resume)
>> +};
>
> Have you considered using below ?
>   static DEFINE_SIMPLE_DEV_PM_OPS(cdns_mhdp_pm_ops,
>
>                                    cdns_mhdp_suspend, cdns_mhdp_resume);
Thanks for the tip, updated it in v3.
>> +
>>   static int cdns_mhdp_probe(struct platform_device *pdev)
>>   {
>>       struct device *dev = &pdev->dev;
>> @@ -2494,6 +2609,11 @@ static int cdns_mhdp_probe(struct 
>> platform_device *pdev)
>>           dev_err(mhdp->dev, "Failed to initialize PHY: %d\n", ret);
>>           goto plat_fini;
>>       }
>>   +    mhdp->powered_off = false;
>> +    mhdp->pd_nb.notifier_call = mhdp_pd_notifier_cb;
>> +    ret = dev_pm_genpd_add_notifier(mhdp->dev, &mhdp->pd_nb);
>> +    if (ret) {
>> +        dev_err_probe(dev, ret, "failed to add power domain 
>> notifier\n");
>> +        dev_pm_genpd_remove_notifier(mhdp->dev);
>
> If add_notifier failed already why need to do above remove ?
Correct, we do not need this.
>
>> +        goto power_off;
>> +    }
>
> Also for non-gen-pd platforms this would pretty much restrict usage of 
> this driver, is that expected/desired behaviour ?

For non-gen-pd platforms, we mark the mhdp->powered_off = true at the 
end of suspend.

So if no gen-pd is available, assume that the platform has taken care of 
powering off the PD at the end of suspend

and hence always reload firmware on resume.

Additionally we only add this notifier for gen-pd platforms.


Thanks and Regards,

Abhash