lkml.org 
[lkml]   [2022]   [Dec]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/2] scsi: ufs: core: Add hibernation callbacks
From
On 12/21/22 09:12, Anjana Hari wrote:
> Add hibernation call backs - freeze, restore and thaw.
> Add the respective prototypes.
>
> Signed-off-by: Anjana Hari <quic_ahari@quicinc.com>
> ---
> drivers/ufs/core/ufshcd.c | 80 +++++++++++++++++++++++++++++++++++++++
> include/ufs/ufshcd.h | 6 +++
> 2 files changed, 86 insertions(+)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index bda61be5f035..c216a97dc1dd 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9453,6 +9453,25 @@ 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);
> + /* Commit the registers */
> + mb();
> + }

The comment above the mb() call is wrong. No matter the type of a
barrier, a barrier controls the order in which load, store and/or MMIO
operations happen. A barrier does not "commit" register writes.

The comment above mb() should explain which load, store and/or MMIO
accesses are ordered and also why the ordering is necessary.

> + /*
> + * Ensure no runtime PM operations take
> + * place in the hibernation and restore sequence
> + * on successful freeze operation.
> + */
> + if (!ret)
> + pm_runtime_disable(hba->dev);

I think the power management core already serializes system-wide power
management state transitions and runtime power management and hence that
the above code can be left out.

> + /*
> + * Now any runtime PM operations can be
> + * allowed on successful restore operation
> + */
> + if (!ret)
> + pm_runtime_enable(hba->dev);

Same comment for the above code.

> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_system_restore);
> +
> +int ufshcd_system_thaw(struct device *dev)
> +{
> +
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + ret = ufshcd_system_resume(dev);
> + if (!ret)
> + pm_runtime_enable(hba->dev);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_system_thaw);

Also here, please remove the code related to runtime power management.

> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 5cf81dff60aa..dadb3c732be4 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -978,6 +978,9 @@ struct ufs_hba {
> #endif
> u32 luns_avail;
> bool complete_put;
> +
> + /* Distinguish between resume and restore */
> + bool restore;
> };

Please convert this member variable into a function argument. This
probably will require adding an argument to existing functions.

Thanks,

Bart.

\
 
 \ /
  Last update: 2023-03-26 23:23    [W:0.142 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site