| Message ID | 20230120113321.30433-1-quic_ahari@quicinc.com |
|---|---|
| Headers | show |
| Series | scsi: ufs: Add hibernation callbacks | expand |
On 1/20/23 03:33, Anjana Hari wrote: > - Note to Bart: Regrading the comment to pass "restore" as an > argument instead of adding a new member to ufs_hba structure, adding > new function argument in core file (ufshcd.c) is forcing us to make > changes to other vendor files to fix the compilation errors. Hence > we have retained our original change. Please let us know your inputs > on this. Storing state information in a structure member that can be passed as a function argument makes code harder to read and to maintain than necessary. Please address my request before this patch goes upstream. I'm concerned if someone would try to address my request after this patch went upstream that there would be no motivation from your side to help with testing the refactoring patch. I think the patch below shows that it is easy to eliminate the new 'restore' member variable. Please note that the patch below has not been tested in any way. --- drivers/ufs/core/ufshcd.c | 48 +++++++++++++++++++-------------------- include/ufs/ufshcd.h | 3 --- 2 files changed, 23 insertions(+), 28 deletions(-) diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 19608f3a38f9..b5cfbc1fccc6 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -9801,34 +9801,11 @@ static int ufshcd_resume(struct ufs_hba *hba) /* enable the host irq as host controller would be active soon */ ufshcd_enable_irq(hba); - if (hba->restore) { - /* Configure UTRL and UTMRL base address registers */ - ufshcd_writel(hba, lower_32_bits(hba->utrdl_dma_addr), - REG_UTP_TRANSFER_REQ_LIST_BASE_L); - ufshcd_writel(hba, upper_32_bits(hba->utrdl_dma_addr), - REG_UTP_TRANSFER_REQ_LIST_BASE_H); - ufshcd_writel(hba, lower_32_bits(hba->utmrdl_dma_addr), - REG_UTP_TASK_REQ_LIST_BASE_L); - ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr), - REG_UTP_TASK_REQ_LIST_BASE_H); - /* Make sure that UTRL and UTMRL base address registers - * are updated with the latest queue addresses. Only after - * updating these addresses, we can queue the new commands. - */ - mb(); - } - - /* Resuming from hibernate, assume that link was OFF */ - if (hba->restore) - ufshcd_set_link_off(hba); - goto out; disable_vreg: ufshcd_vreg_set_lpm(hba); out: - if (hba->restore) - hba->restore = false; if (ret) ufshcd_update_evt_hist(hba, UFS_EVT_RESUME_ERR, (u32)ret); @@ -10012,10 +9989,31 @@ int ufshcd_system_restore(struct device *dev) { struct ufs_hba *hba = dev_get_drvdata(dev); + int ret; - hba->restore = true; - return ufshcd_system_resume(dev); + ret = ufshcd_system_resume(dev); + if (ret) + return ret; + + /* Configure UTRL and UTMRL base address registers */ + ufshcd_writel(hba, lower_32_bits(hba->utrdl_dma_addr), + REG_UTP_TRANSFER_REQ_LIST_BASE_L); + ufshcd_writel(hba, upper_32_bits(hba->utrdl_dma_addr), + REG_UTP_TRANSFER_REQ_LIST_BASE_H); + ufshcd_writel(hba, lower_32_bits(hba->utmrdl_dma_addr), + REG_UTP_TASK_REQ_LIST_BASE_L); + ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr), + REG_UTP_TASK_REQ_LIST_BASE_H); + /* Make sure that UTRL and UTMRL base address registers + * are updated with the latest queue addresses. Only after + * updating these addresses, we can queue the new commands. + */ + mb(); + /* Resuming from hibernate, assume that link was OFF */ + ufshcd_set_link_off(hba); + + return 0; } EXPORT_SYMBOL_GPL(ufshcd_system_restore); diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index 6f50390ca262..1d6dd13e1651 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -1071,9 +1071,6 @@ struct ufs_hba { struct ufs_hw_queue *uhq; struct ufs_hw_queue *dev_cmd_queue; struct ufshcd_mcq_opr_info_t mcq_opr[OPR_MAX]; - - /* Distinguish between resume and restore */ - bool restore; }; /**
On 1/20/23 03:33, Anjana Hari wrote: > + /* Make sure that UTRL and UTMRL base address registers > + * are updated with the latest queue addresses. Only after > + * updating these addresses, we can queue the new commands. > + */ Please follow the kernel coding style and start comment blocks with "/*" on a line by its own. > + /* Resuming from hibernate, assume that link was OFF */ > + if (hba->restore) > + ufshcd_set_link_off(hba); Why two successive 'if (hba->restore)' statements? Can these two if-statements be combined into a single if-statement? > +int ufshcd_system_freeze(struct device *dev) > +{ > + > + struct ufs_hba *hba = dev_get_drvdata(dev); > + int ret = 0; > + > + /* > + * Run time resume the controller to make sure > + * the PM work queue threads do not try to resume > + * the child (scsi host), which leads to errors as > + * the controller is not yet resumed. > + */ > + pm_runtime_get_sync(hba->dev); > + ret = ufshcd_system_suspend(dev); > + pm_runtime_put_sync(hba->dev); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(ufshcd_system_freeze); Why does the above function use 'hba->dev' instead of 'dev'? I do not understand the comment in the above function. My understanding is that the power management core serializes hibernation and runtime power management. How could runtime resume be in progress when ufshcd_system_freeze() is called? Additionally, shouldn't ufshcd_system_freeze() skip the UFS controller if it is already runtime suspended instead of runtime resuming it? Some time ago I added the following code in sd_suspend_system() (commit 9131bff6a9f1 ("scsi: core: pm: Only runtime resume if necessary"): if (pm_runtime_suspended(dev)) return 0; Thanks, Bart.