Messages in this thread | | | Date | Thu, 29 Dec 2022 07:06:38 -0800 | Subject | Re: [PATCH 1/2] scsi: ufs: core: Add hibernation callbacks | From | Bart Van Assche <> |
| |
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.
| |