On Thu, 13 Jan 2022, Ricardo Martinez wrote:
> From: Haijun Liu <haijun.liu@xxxxxxxxxxxx>
>
> Implements suspend, resumes, freeze, thaw, poweroff, and restore
> `dev_pm_ops` callbacks.
>
> >From the host point of view, the t7xx driver is one entity. But, the
> device has several modules that need to be addressed in different ways
> during power management (PM) flows.
> The driver uses the term 'PM entities' to refer to the 2 DPMA and
> 2 CLDMA HW blocks that need to be managed during PM flows.
> When a dev_pm_ops function is called, the PM entities list is iterated
> and the matching function is called for each entry in the list.
>
> Signed-off-by: Haijun Liu <haijun.liu@xxxxxxxxxxxx>
> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@xxxxxxxxx>
> Co-developed-by: Ricardo Martinez <ricardo.martinez@xxxxxxxxxxxxxxx>
> Signed-off-by: Ricardo Martinez <ricardo.martinez@xxxxxxxxxxxxxxx>
> ---
> if (ret) {
> dev_err(dev, "Failed to allocate RX/TX SW resources: %d\n", ret);
> + t7xx_dpmaif_pm_entity_release(dpmaif_ctrl);
> return NULL;
Print after release.
> +static int __t7xx_pci_pm_suspend(struct pci_dev *pdev)
> +{
> + struct t7xx_pci_dev *t7xx_dev;
> + struct md_pm_entity *entity;
> + unsigned long wait_ret;
> + enum t7xx_pm_id id;
> + int ret = 0;
> +
> + t7xx_dev = pci_get_drvdata(pdev);
> + if (atomic_read(&t7xx_dev->md_pm_state) <= MTK_PM_INIT) {
> + dev_err(&pdev->dev,
> + "[PM] Exiting suspend, because handshake failure or in an exception\n");
> + return -EFAULT;
> + }
> +
> + iowrite32(L1_DISABLE_BIT(0), IREG_BASE(t7xx_dev) + DIS_ASPM_LOWPWR_SET_0);
> +
> + ret = t7xx_wait_pm_config(t7xx_dev);
> + if (ret)
> + return ret;
Do you need to rollback the iowrite?
> + atomic_set(&t7xx_dev->md_pm_state, MTK_PM_SUSPENDED);
> + t7xx_pcie_mac_clear_int(t7xx_dev, SAP_RGU_INT);
> + t7xx_dev->rgu_pci_irq_en = false;
> +
> + list_for_each_entry(entity, &t7xx_dev->md_pm_entities, entity) {
> + if (entity->suspend) {
> + ret = entity->suspend(t7xx_dev, entity->entity_param);
> + if (ret) {
> + id = entity->id;
> + break;
> + }
> + }
> + }
> +
> + if (ret) {
> + dev_err(&pdev->dev, "[PM] Suspend error: %d, id: %d\n", ret, id);
> +
> + list_for_each_entry(entity, &t7xx_dev->md_pm_entities, entity) {
> + if (id == entity->id)
> + break;
> +
> + if (entity->resume)
> + entity->resume(t7xx_dev, entity->entity_param);
> + }
> +
> + goto suspend_complete;
Suspend failure path(?) gotos to "suspend_complete"?
> + }
> +
> + reinit_completion(&t7xx_dev->pm_sr_ack);
> + t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_SUSPEND_REQ);
> + wait_ret = wait_for_completion_timeout(&t7xx_dev->pm_sr_ack,
> + msecs_to_jiffies(PM_ACK_TIMEOUT_MS));
> + if (!wait_ret)
> + dev_err(&pdev->dev, "[PM] Wait for device suspend ACK timeout-MD\n");
> +
> + reinit_completion(&t7xx_dev->pm_sr_ack);
> + t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_SUSPEND_REQ_AP);
> + wait_ret = wait_for_completion_timeout(&t7xx_dev->pm_sr_ack,
> + msecs_to_jiffies(PM_ACK_TIMEOUT_MS));
> + if (!wait_ret)
> + dev_err(&pdev->dev, "[PM] Wait for device suspend ACK timeout-SAP\n");
> +
> + list_for_each_entry(entity, &t7xx_dev->md_pm_entities, entity) {
> + if (entity->suspend_late)
> + entity->suspend_late(t7xx_dev, entity->entity_param);
> + }
> +
> +suspend_complete:
> + iowrite32(L1_DISABLE_BIT(0), IREG_BASE(t7xx_dev) + DIS_ASPM_LOWPWR_CLR_0);
> +
> + if (ret) {
> + atomic_set(&t7xx_dev->md_pm_state, MTK_PM_RESUMED);
> + t7xx_pcie_mac_set_int(t7xx_dev, SAP_RGU_INT);
> + }
> +
> + return ret;
> +}
Please check all paths in this function. I found enough oddities to not
be able to convince myself I understood it all or found all the problems.
As if, e.g., an ok path return would be missing above misnamed
suspend_complete label (but then there's if (ret) below it which is kind
of counterargument against my reasoning).
I've no comments on patches 11-13.
--
i.
