Re: [PATCH net-next v4 10/13] net: wwan: t7xx: Introduce power management support
flat view
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Date: 2022-02-10 10:58:42
Also in:
linux-wireless
On Thu, 13 Jan 2022, Ricardo Martinez wrote:
From: Haijun Liu <haijun.liu@mediatek.com> Implements suspend, resumes, freeze, thaw, poweroff, and restore `dev_pm_ops` callbacks.quoted
From the host point of view, the t7xx driver is one entity. But, thedevice 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@mediatek.com> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> ---
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ 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"?
quoted hunk ↗ jump to hunk
+ } + + 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.