diff mbox series

[5/8] mhi: pci_generic: Add suspend/resume/recovery procedure

Message ID 1605279602-18749-6-git-send-email-loic.poulain@linaro.org
State Superseded
Headers show
Series mhi: pci_generic: Misc improvements | expand

Commit Message

Loic Poulain Nov. 13, 2020, 2:59 p.m. UTC
Add support for system wide suspend/resume. During suspend, MHI
device controller must be put in M3 state and PCI bus in D3 state.

Add a recovery procedure allowing to reinitialize the device in case
of error during resume steps, which can happen if device loses power
(and so its context) while system suspend.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

---
 drivers/bus/mhi/pci_generic.c | 100 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

-- 
2.7.4

Comments

Hemant Kumar Nov. 19, 2020, 2:21 a.m. UTC | #1
Hi Loic,

On 11/13/20 6:59 AM, Loic Poulain wrote:
> Add support for system wide suspend/resume. During suspend, MHI

> device controller must be put in M3 state and PCI bus in D3 state.

> 

> Add a recovery procedure allowing to reinitialize the device in case

> of error during resume steps, which can happen if device loses power

> (and so its context) while system suspend.

> 

> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

> ---

[..]
>   

> +static void mhi_pci_recovery_work(struct work_struct *work)

> +{

> +	struct mhi_pci_device *mhi_pdev = container_of(work, struct mhi_pci_device,

> +						       recovery_work);

> +	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;

> +	struct pci_dev *pdev = to_pci_dev(mhi_cntrl->cntrl_dev);

> +	int err;

> +

> +	dev_warn(&pdev->dev, "device recovery started\n");

> +

> +	/* Clean up MHI state */

> +	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {

> +		mhi_power_down(mhi_cntrl, false);

> +		mhi_unprepare_after_power_down(mhi_cntrl);

> +	}

> +

> +	/* Check if we can recover without full reset */

> +	pci_set_power_state(pdev, PCI_D0);

> +	pci_load_saved_state(pdev, mhi_pdev->pci_state);

> +	pci_restore_state(pdev);

> +

> +	if (!mhi_pci_is_alive(mhi_cntrl))

> +		goto err_try_reset;

> +

> +	err = mhi_prepare_for_power_up(mhi_cntrl);

> +	if (err)

> +		goto err_try_reset;

> +

> +	err = mhi_sync_power_up(mhi_cntrl);

> +	if (err)

> +		goto err_unprepare;

> +

> +	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);

> +	return;

> +

> +err_unprepare:

> +	mhi_unprepare_after_power_down(mhi_cntrl);

> +err_try_reset:

> +	if (pci_reset_function(pdev))

> +		dev_err(&pdev->dev, "Recovery failed\n");

> +}

> +

>   static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)

>   {

>   	const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;

> @@ -333,6 +377,8 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)

>   	if (!mhi_pdev)

>   		return -ENOMEM;

>   

> +	INIT_WORK(&mhi_pdev->recovery_work, mhi_pci_recovery_work);

> +

>   	mhi_cntrl_config = info->config;

>   	mhi_cntrl = &mhi_pdev->mhi_cntrl;

>   	mhi_cntrl->cntrl_dev = &pdev->dev;

> @@ -395,6 +441,8 @@ static void mhi_pci_remove(struct pci_dev *pdev)

>   	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);

>   	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;

>   

> +	cancel_work_sync(&mhi_pdev->recovery_work);

> +

>   	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {

>   		mhi_power_down(mhi_cntrl, true);

>   		mhi_unprepare_after_power_down(mhi_cntrl);

> @@ -463,12 +511,64 @@ static const struct pci_error_handlers mhi_pci_err_handler = {

>   	.reset_done = mhi_pci_reset_done,

>   };

>   

> +int  __maybe_unused mhi_pci_suspend(struct device *dev)

> +{

> +	struct pci_dev *pdev = to_pci_dev(dev);

> +	struct mhi_pci_device *mhi_pdev = dev_get_drvdata(dev);

> +	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;

> +

is it possible we come here and work still did not get a chance to run ? 
If so we can flush it before going through suspend.
> +	/* Transition to M3 state */

> +	mhi_pm_suspend(mhi_cntrl);

> +

> +	pci_save_state(pdev);

> +	pci_disable_device(pdev);

> +	pci_wake_from_d3(pdev, true);

> +	pci_set_power_state(pdev, PCI_D3hot);

are you planning to park the device in D3hot all the time ?
Is there a plan to move to D3Cold in your use case ?
> +

> +	return 0;

> +}

> +

> +static int __maybe_unused mhi_pci_resume(struct device *dev)

> +{

> +	struct pci_dev *pdev = to_pci_dev(dev);

> +	struct mhi_pci_device *mhi_pdev = dev_get_drvdata(dev);

> +	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;

> +	int err;

> +

> +	pci_set_power_state(pdev, PCI_D0);

> +	pci_restore_state(pdev);

> +	pci_set_master(pdev);

> +

> +	err = pci_enable_device(pdev);

> +	if (err)

> +		goto err_recovery;

> +

> +	/* Exit M3, transition to M0 state */

> +	err = mhi_pm_resume(mhi_cntrl);

> +	if (err) {

> +		dev_err(&pdev->dev, "failed to resume device: %d\n", err);

> +		goto err_recovery;

> +	}

> +

> +	return 0;

> +

> +err_recovery:

> +	/* The device may have loose power or crashed, try recovering it */

> +	queue_work(system_long_wq, &mhi_pdev->recovery_work);

> +	return 0;

> +}

> +

[..]

Thanks,
Hemant

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Loic Poulain Nov. 19, 2020, 9:54 a.m. UTC | #2
Hi Hemant,

On Thu, 19 Nov 2020 at 03:21, Hemant Kumar <hemantk@codeaurora.org> wrote:
>

> Hi Loic,

>

> On 11/13/20 6:59 AM, Loic Poulain wrote:

> > Add support for system wide suspend/resume. During suspend, MHI

> > device controller must be put in M3 state and PCI bus in D3 state.

> >

> > Add a recovery procedure allowing to reinitialize the device in case

> > of error during resume steps, which can happen if device loses power

> > (and so its context) while system suspend.

> >

> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

> > ---

> [..]

> >

> > +static void mhi_pci_recovery_work(struct work_struct *work)

> > +{

> > +     struct mhi_pci_device *mhi_pdev = container_of(work, struct mhi_pci_device,

> > +                                                    recovery_work);

> > +     struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;

> > +     struct pci_dev *pdev = to_pci_dev(mhi_cntrl->cntrl_dev);

> > +     int err;

> > +

> > +     dev_warn(&pdev->dev, "device recovery started\n");

> > +

> > +     /* Clean up MHI state */

> > +     if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {

> > +             mhi_power_down(mhi_cntrl, false);

> > +             mhi_unprepare_after_power_down(mhi_cntrl);

> > +     }

> > +

> > +     /* Check if we can recover without full reset */

> > +     pci_set_power_state(pdev, PCI_D0);

> > +     pci_load_saved_state(pdev, mhi_pdev->pci_state);

> > +     pci_restore_state(pdev);

> > +

> > +     if (!mhi_pci_is_alive(mhi_cntrl))

> > +             goto err_try_reset;

> > +

> > +     err = mhi_prepare_for_power_up(mhi_cntrl);

> > +     if (err)

> > +             goto err_try_reset;

> > +

> > +     err = mhi_sync_power_up(mhi_cntrl);

> > +     if (err)

> > +             goto err_unprepare;

> > +

> > +     set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);

> > +     return;

> > +

> > +err_unprepare:

> > +     mhi_unprepare_after_power_down(mhi_cntrl);

> > +err_try_reset:

> > +     if (pci_reset_function(pdev))

> > +             dev_err(&pdev->dev, "Recovery failed\n");

> > +}

> > +

> >   static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)

> >   {

> >       const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;

> > @@ -333,6 +377,8 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)

> >       if (!mhi_pdev)

> >               return -ENOMEM;

> >

> > +     INIT_WORK(&mhi_pdev->recovery_work, mhi_pci_recovery_work);

> > +

> >       mhi_cntrl_config = info->config;

> >       mhi_cntrl = &mhi_pdev->mhi_cntrl;

> >       mhi_cntrl->cntrl_dev = &pdev->dev;

> > @@ -395,6 +441,8 @@ static void mhi_pci_remove(struct pci_dev *pdev)

> >       struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);

> >       struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;

> >

> > +     cancel_work_sync(&mhi_pdev->recovery_work);

> > +

> >       if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {

> >               mhi_power_down(mhi_cntrl, true);

> >               mhi_unprepare_after_power_down(mhi_cntrl);

> > @@ -463,12 +511,64 @@ static const struct pci_error_handlers mhi_pci_err_handler = {

> >       .reset_done = mhi_pci_reset_done,

> >   };

> >

> > +int  __maybe_unused mhi_pci_suspend(struct device *dev)

> > +{

> > +     struct pci_dev *pdev = to_pci_dev(dev);

> > +     struct mhi_pci_device *mhi_pdev = dev_get_drvdata(dev);

> > +     struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;

> > +

> is it possible we come here and work still did not get a chance to run ?

> If so we can flush it before going through suspend.


Yes, would be good to cancel that work, will fix that.

> > +     /* Transition to M3 state */

> > +     mhi_pm_suspend(mhi_cntrl);

> > +

> > +     pci_save_state(pdev);

> > +     pci_disable_device(pdev);

> > +     pci_wake_from_d3(pdev, true);

> > +     pci_set_power_state(pdev, PCI_D3hot);

> are you planning to park the device in D3hot all the time ?

> Is there a plan to move to D3Cold in your use case ?


D3Cold is not a state that is controllable in software.

But here we support both scenarios:

Either the device is kept powered during platform suspend, because
e.g. it has auxiliary power. In that case, context is maintained and
the device can be safely resumed from M3. (e.g The M2 spec specifies
that 3.3V power should be provided even during system suspend).

Or the device has its power cut off during platform suspend, that
actually the case with my desktop, PCI express ports are disabled, and
the modem is powered off, this is D3Cold. In that case, we need to
reinitialize the modem, that what is done in the recovery procedure.

Regards,
Loic
diff mbox series

Patch

diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c
index b48c382..75f565b 100644
--- a/drivers/bus/mhi/pci_generic.c
+++ b/drivers/bus/mhi/pci_generic.c
@@ -13,6 +13,7 @@ 
 #include <linux/mhi.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/workqueue.h>
 
 #define MHI_PCI_DEFAULT_BAR_NUM 0
 
@@ -187,6 +188,7 @@  enum mhi_pci_device_status {
 struct mhi_pci_device {
 	struct mhi_controller mhi_cntrl;
 	struct pci_saved_state *pci_state;
+	struct work_struct recovery_work;
 	unsigned long status;
 };
 
@@ -319,6 +321,48 @@  static void mhi_pci_runtime_put(struct mhi_controller *mhi_cntrl)
 	/* no PM for now */
 }
 
+static void mhi_pci_recovery_work(struct work_struct *work)
+{
+	struct mhi_pci_device *mhi_pdev = container_of(work, struct mhi_pci_device,
+						       recovery_work);
+	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
+	struct pci_dev *pdev = to_pci_dev(mhi_cntrl->cntrl_dev);
+	int err;
+
+	dev_warn(&pdev->dev, "device recovery started\n");
+
+	/* Clean up MHI state */
+	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
+		mhi_power_down(mhi_cntrl, false);
+		mhi_unprepare_after_power_down(mhi_cntrl);
+	}
+
+	/* Check if we can recover without full reset */
+	pci_set_power_state(pdev, PCI_D0);
+	pci_load_saved_state(pdev, mhi_pdev->pci_state);
+	pci_restore_state(pdev);
+
+	if (!mhi_pci_is_alive(mhi_cntrl))
+		goto err_try_reset;
+
+	err = mhi_prepare_for_power_up(mhi_cntrl);
+	if (err)
+		goto err_try_reset;
+
+	err = mhi_sync_power_up(mhi_cntrl);
+	if (err)
+		goto err_unprepare;
+
+	set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status);
+	return;
+
+err_unprepare:
+	mhi_unprepare_after_power_down(mhi_cntrl);
+err_try_reset:
+	if (pci_reset_function(pdev))
+		dev_err(&pdev->dev, "Recovery failed\n");
+}
+
 static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
@@ -333,6 +377,8 @@  static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (!mhi_pdev)
 		return -ENOMEM;
 
+	INIT_WORK(&mhi_pdev->recovery_work, mhi_pci_recovery_work);
+
 	mhi_cntrl_config = info->config;
 	mhi_cntrl = &mhi_pdev->mhi_cntrl;
 	mhi_cntrl->cntrl_dev = &pdev->dev;
@@ -395,6 +441,8 @@  static void mhi_pci_remove(struct pci_dev *pdev)
 	struct mhi_pci_device *mhi_pdev = pci_get_drvdata(pdev);
 	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
 
+	cancel_work_sync(&mhi_pdev->recovery_work);
+
 	if (test_and_clear_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status)) {
 		mhi_power_down(mhi_cntrl, true);
 		mhi_unprepare_after_power_down(mhi_cntrl);
@@ -463,12 +511,64 @@  static const struct pci_error_handlers mhi_pci_err_handler = {
 	.reset_done = mhi_pci_reset_done,
 };
 
+int  __maybe_unused mhi_pci_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct mhi_pci_device *mhi_pdev = dev_get_drvdata(dev);
+	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
+
+	/* Transition to M3 state */
+	mhi_pm_suspend(mhi_cntrl);
+
+	pci_save_state(pdev);
+	pci_disable_device(pdev);
+	pci_wake_from_d3(pdev, true);
+	pci_set_power_state(pdev, PCI_D3hot);
+
+	return 0;
+}
+
+static int __maybe_unused mhi_pci_resume(struct device *dev)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct mhi_pci_device *mhi_pdev = dev_get_drvdata(dev);
+	struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl;
+	int err;
+
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+	pci_set_master(pdev);
+
+	err = pci_enable_device(pdev);
+	if (err)
+		goto err_recovery;
+
+	/* Exit M3, transition to M0 state */
+	err = mhi_pm_resume(mhi_cntrl);
+	if (err) {
+		dev_err(&pdev->dev, "failed to resume device: %d\n", err);
+		goto err_recovery;
+	}
+
+	return 0;
+
+err_recovery:
+	/* The device may have loose power or crashed, try recovering it */
+	queue_work(system_long_wq, &mhi_pdev->recovery_work);
+	return 0;
+}
+
+static const struct dev_pm_ops mhi_pci_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(mhi_pci_suspend, mhi_pci_resume)
+};
+
 static struct pci_driver mhi_pci_driver = {
 	.name		= "mhi-pci-generic",
 	.id_table	= mhi_pci_id_table,
 	.probe		= mhi_pci_probe,
 	.remove		= mhi_pci_remove,
 	.err_handler	= &mhi_pci_err_handler,
+	.driver.pm	= &mhi_pci_pm_ops
 };
 module_pci_driver(mhi_pci_driver);