[PATCH 0/1] scsi: ufs-pci: Force a full restore after suspend-to-disk

[PATCH 0/1] scsi: ufs-pci: Force a full restore after suspend-to-disk

[Adrian Hunter]

Hi

This patch ensures suspend-to-disk works with Host Performance Booster.
Since the Host Perfomance Booster feature was added in v5.15, please
consider this for v5.15 fixes.


Adrian Hunter (1):
      scsi: ufs-pci: Force a full restore after suspend-to-disk

 drivers/scsi/ufs/ufshcd-pci.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)


Regards
Adrian

[PATCH 1/1] scsi: ufs-pci: Force a full restore after suspend-to-disk

[Adrian Hunter]

Implement the ->restore() PM operation and set the link to off, which
will force a full reset and restore.  This ensures that Host Performance
Booster is reset after suspend-to-disk.

The Host Performance Booster feature caches logical-to-physical mapping
information in the host memory.  After suspend-to-disk, such information
is not valid, so a full reset and restore is needed.

A full reset and restore is done if the SPM level is 5 or 6, but not for
other SPM levels, so this change fixes those cases.

A full reset and restore also restores base address registers, so that
code is removed.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/scsi/ufs/ufshcd-pci.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
index 149c1aa09103..d65e6cd7a28d 100644
--- a/drivers/scsi/ufs/ufshcd-pci.c
+++ b/drivers/scsi/ufs/ufshcd-pci.c
@@ -370,20 +370,6 @@ static void ufs_intel_common_exit(struct ufs_hba *hba)
 
 static int ufs_intel_resume(struct ufs_hba *hba, enum ufs_pm_op op)
 {
-	/*
-	 * To support S4 (suspend-to-disk) with spm_lvl other than 5, the base
-	 * address registers must be restored because the restore kernel can
-	 * have used different addresses.
-	 */
-	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);
-
 	if (ufshcd_is_link_hibern8(hba)) {
 		int ret = ufshcd_uic_hibern8_exit(hba);
 
@@ -463,6 +449,16 @@ static struct ufs_hba_variant_ops ufs_intel_lkf_hba_vops = {
 	.device_reset		= ufs_intel_device_reset,
 };
 
+static int ufshcd_pci_restore(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	/* Force a full reset and restore */
+	ufshcd_set_link_off(hba);
+
+	return ufshcd_system_resume(dev);
+}
+
 /**
  * ufshcd_pci_shutdown - main function to put the controller in reset state
  * @pdev: pointer to PCI device handle
@@ -546,9 +542,14 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 }
 
 static const struct dev_pm_ops ufshcd_pci_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
 	SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
 #ifdef CONFIG_PM_SLEEP
+	.suspend	= ufshcd_system_suspend,
+	.resume		= ufshcd_system_resume,
+	.freeze		= ufshcd_system_suspend,
+	.thaw		= ufshcd_system_resume,
+	.poweroff	= ufshcd_system_suspend,
+	.restore	= ufshcd_pci_restore,
 	.prepare	= ufshcd_suspend_prepare,
 	.complete	= ufshcd_resume_complete,
 #endif
-- 
2.25.1

RE: [PATCH 0/1] scsi: ufs-pci: Force a full restore after suspend-to-disk

[Avri Altman]

> Hi
> 
> This patch ensures suspend-to-disk works with Host Performance Booster.
What is "suspend-to-disk"?
What power mode is used for that?

Thanks,
Avri

> Since the Host Perfomance Booster feature was added in v5.15, please
> consider this for v5.15 fixes.
> 
> 
> Adrian Hunter (1):
>       scsi: ufs-pci: Force a full restore after suspend-to-disk
> 
>  drivers/scsi/ufs/ufshcd-pci.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> 
> Regards
> Adrian

Re: [PATCH 0/1] scsi: ufs-pci: Force a full restore after suspend-to-disk

[Adrian Hunter]

On 13/10/2021 13:46, Avri Altman wrote:
>> Hi
>>
>> This patch ensures suspend-to-disk works with Host Performance Booster.
> What is "suspend-to-disk"?

Hibernation, refer kernel config CONFIG_HIBERNATION
i.e.

echo disk > /sys/power/state

> What power mode is used for that?

Same as spm_lvl except ufshcd_wl_poweroff() naturally uses the same as
shutdown i.e. POWERDOWN 

> 
> Thanks,
> Avri
> 
>> Since the Host Perfomance Booster feature was added in v5.15, please
>> consider this for v5.15 fixes.
>>
>>
>> Adrian Hunter (1):
>>       scsi: ufs-pci: Force a full restore after suspend-to-disk
>>
>>  drivers/scsi/ufs/ufshcd-pci.c | 31 ++++++++++++++++---------------
>>  1 file changed, 16 insertions(+), 15 deletions(-)
>>
>>
>> Regards
>> Adrian

RE: [PATCH 0/1] scsi: ufs-pci: Force a full restore after suspend-to-disk

[Avri Altman]

> On 13/10/2021 13:46, Avri Altman wrote:
> >> Hi
> >>
> >> This patch ensures suspend-to-disk works with Host Performance Booster.
> > What is "suspend-to-disk"?
> 
> Hibernation, refer kernel config CONFIG_HIBERNATION i.e.
> 
> echo disk > /sys/power/state
> 
> > What power mode is used for that?
> 
> Same as spm_lvl except ufshcd_wl_poweroff() naturally uses the same as
> shutdown i.e. POWERDOWN
If spm_lvl is SSU3 - the device maintains its RAM.
On SSU4 however, the RAM is powered down.

> 
> >
> > Thanks,
> > Avri
> >
> >> Since the Host Perfomance Booster feature was added in v5.15, please
> >> consider this for v5.15 fixes.
> >>
> >>
> >> Adrian Hunter (1):
> >>       scsi: ufs-pci: Force a full restore after suspend-to-disk
> >>
> >>  drivers/scsi/ufs/ufshcd-pci.c | 31 ++++++++++++++++---------------
> >>  1 file changed, 16 insertions(+), 15 deletions(-)
> >>
> >>
> >> Regards
> >> Adrian

RE: [PATCH 1/1] scsi: ufs-pci: Force a full restore after suspend-to-disk

[Avri Altman]

 
> Implement the ->restore() PM operation and set the link to off, which
> will force a full reset and restore.  This ensures that Host Performance
> Booster is reset after suspend-to-disk.
> 
> The Host Performance Booster feature caches logical-to-physical mapping
> information in the host memory.  After suspend-to-disk, such information
> is not valid, so a full reset and restore is needed.
> 
> A full reset and restore is done if the SPM level is 5 or 6, but not for
> other SPM levels, so this change fixes those cases.
It is perfectly fine for you to do that on your platform, if you choose so - 
Hence my reviewed-by.
But the reasoning is a bit odd, because you already set SSU4 for your spm-lvl,
And on SSU3 the device does not dump its internal tables.

Thanks,
Avri

> A full reset and restore also restores base address registers, so that
> code is removed.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Re: [PATCH 1/1] scsi: ufs-pci: Force a full restore after suspend-to-disk

[Adrian Hunter]

On 14/10/2021 09:15, Avri Altman wrote:
>  
>> Implement the ->restore() PM operation and set the link to off, which
>> will force a full reset and restore.  This ensures that Host Performance
>> Booster is reset after suspend-to-disk.
>>
>> The Host Performance Booster feature caches logical-to-physical mapping
>> information in the host memory.  After suspend-to-disk, such information
>> is not valid, so a full reset and restore is needed.
>>
>> A full reset and restore is done if the SPM level is 5 or 6, but not for
>> other SPM levels, so this change fixes those cases.
> It is perfectly fine for you to do that on your platform, if you choose so - 
> Hence my reviewed-by.
> But the reasoning is a bit odd, because you already set SSU4 for your spm-lvl,
> And on SSU3 the device does not dump its internal tables.

Hibernation flow is:

	->freeze()
	create memory image
	->thaw()
	write image
	->poweroff()

	time passes
	someone presses the power button or something else activates the machine

	kernel boots
	detects restore image
	read image
	->freeze()
	restore memory image
	->restore()

It is up to ->restore() to ensure the device state matches the
memory state as it was when the hibernation image was made.

> 
> Thanks,
> Avri
> 
>> A full reset and restore also restores base address registers, so that
>> code is removed.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
> 

RE: [PATCH 1/1] scsi: ufs-pci: Force a full restore after suspend-to-disk

[Avri Altman]

 
> On 14/10/2021 09:15, Avri Altman wrote:
> >
> >> Implement the ->restore() PM operation and set the link to off, which
> >> will force a full reset and restore.  This ensures that Host
> >> Performance Booster is reset after suspend-to-disk.
> >>
> >> The Host Performance Booster feature caches logical-to-physical
> >> mapping information in the host memory.  After suspend-to-disk, such
> >> information is not valid, so a full reset and restore is needed.
> >>
> >> A full reset and restore is done if the SPM level is 5 or 6, but not
> >> for other SPM levels, so this change fixes those cases.
> > It is perfectly fine for you to do that on your platform, if you
> > choose so - Hence my reviewed-by.
> > But the reasoning is a bit odd, because you already set SSU4 for your
> > spm-lvl, And on SSU3 the device does not dump its internal tables.
> 
> Hibernation flow is:
> 
>         ->freeze()
>         create memory image
>         ->thaw()
>         write image
>         ->poweroff()
> 
>         time passes
>         someone presses the power button or something else activates the
> machine
> 
>         kernel boots
>         detects restore image
>         read image
>         ->freeze()
>         restore memory image
>         ->restore()
> 
> It is up to ->restore() to ensure the device state matches the memory state as it
> was when the hibernation image was made.
Oh, ok.  I missed the poweroff part before.

Thanks,
Avri

> 
> >
> > Thanks,
> > Avri
> >
> >> A full reset and restore also restores base address registers, so
> >> that code is removed.
> >>
> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> > Reviewed-by: Avri Altman <avri.altman@wdc.com>
> >

Re: [PATCH 1/1] scsi: ufs-pci: Force a full restore after suspend-to-disk

[Martin K. Petersen]

Adrian,

> Implement the ->restore() PM operation and set the link to off, which
> will force a full reset and restore.  This ensures that Host
> Performance Booster is reset after suspend-to-disk.

Applied to 5.16/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering