MHI devices may not be destroyed during suspend/hibernation, so need
to unprepare/prepare MHI channels throughout the transition, this is
done by adding suspend/resume callbacks.
The suspend callback is called in the late suspend stage, this means
MHI channels are still alive at suspend stage, and that makes it
possible for an MHI controller driver to communicate with others over
those channels at suspend stage. While the resume callback is called
in the early resume stage, for a similar reason.
Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
Signed-off-by: Baochen Qiang <[email protected]>
---
net/qrtr/mhi.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
index 9ced13c0627a..e96b82a6742c 100644
--- a/net/qrtr/mhi.c
+++ b/net/qrtr/mhi.c
@@ -118,6 +118,32 @@ static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = {
};
MODULE_DEVICE_TABLE(mhi, qcom_mhi_qrtr_id_table);
+static int __maybe_unused qcom_mhi_qrtr_pm_suspend_late(struct device *dev)
+{
+ struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev);
+
+ mhi_unprepare_from_transfer(mhi_dev);
+
+ return 0;
+}
+
+static int __maybe_unused qcom_mhi_qrtr_pm_resume_early(struct device *dev)
+{
+ struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev);
+ int rc;
+
+ rc = mhi_prepare_for_transfer_autoqueue(mhi_dev);
+ if (rc)
+ dev_err(dev, "failed to prepare for autoqueue transfer %d\n", rc);
+
+ return rc;
+}
+
+static const struct dev_pm_ops qcom_mhi_qrtr_pm_ops = {
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(qcom_mhi_qrtr_pm_suspend_late,
+ qcom_mhi_qrtr_pm_resume_early)
+};
+
static struct mhi_driver qcom_mhi_qrtr_driver = {
.probe = qcom_mhi_qrtr_probe,
.remove = qcom_mhi_qrtr_remove,
@@ -126,6 +152,7 @@ static struct mhi_driver qcom_mhi_qrtr_driver = {
.id_table = qcom_mhi_qrtr_id_table,
.driver = {
.name = "qcom_mhi_qrtr",
+ .pm = &qcom_mhi_qrtr_pm_ops,
},
};
--
2.25.1
On Tue, Feb 27, 2024 at 02:36:12PM +0800, Baochen Qiang wrote:
> MHI devices may not be destroyed during suspend/hibernation, so need
> to unprepare/prepare MHI channels throughout the transition, this is
> done by adding suspend/resume callbacks.
>
> The suspend callback is called in the late suspend stage, this means
> MHI channels are still alive at suspend stage, and that makes it
> possible for an MHI controller driver to communicate with others over
> those channels at suspend stage. While the resume callback is called
> in the early resume stage, for a similar reason.
>
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
>
> Signed-off-by: Baochen Qiang <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Could you please confirm if this patch can be applied separately and won't cause
regression?
- Mani
> ---
> net/qrtr/mhi.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
> index 9ced13c0627a..e96b82a6742c 100644
> --- a/net/qrtr/mhi.c
> +++ b/net/qrtr/mhi.c
> @@ -118,6 +118,32 @@ static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = {
> };
> MODULE_DEVICE_TABLE(mhi, qcom_mhi_qrtr_id_table);
>
> +static int __maybe_unused qcom_mhi_qrtr_pm_suspend_late(struct device *dev)
> +{
> + struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev);
> +
> + mhi_unprepare_from_transfer(mhi_dev);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused qcom_mhi_qrtr_pm_resume_early(struct device *dev)
> +{
> + struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev);
> + int rc;
> +
> + rc = mhi_prepare_for_transfer_autoqueue(mhi_dev);
> + if (rc)
> + dev_err(dev, "failed to prepare for autoqueue transfer %d\n", rc);
> +
> + return rc;
> +}
> +
> +static const struct dev_pm_ops qcom_mhi_qrtr_pm_ops = {
> + SET_LATE_SYSTEM_SLEEP_PM_OPS(qcom_mhi_qrtr_pm_suspend_late,
> + qcom_mhi_qrtr_pm_resume_early)
> +};
> +
> static struct mhi_driver qcom_mhi_qrtr_driver = {
> .probe = qcom_mhi_qrtr_probe,
> .remove = qcom_mhi_qrtr_remove,
> @@ -126,6 +152,7 @@ static struct mhi_driver qcom_mhi_qrtr_driver = {
> .id_table = qcom_mhi_qrtr_id_table,
> .driver = {
> .name = "qcom_mhi_qrtr",
> + .pm = &qcom_mhi_qrtr_pm_ops,
> },
> };
>
> --
> 2.25.1
>
--
மணிவண்ணன் சதாசிவம்
On 2/27/2024 3:15 PM, Manivannan Sadhasivam wrote:
> On Tue, Feb 27, 2024 at 02:36:12PM +0800, Baochen Qiang wrote:
>> MHI devices may not be destroyed during suspend/hibernation, so need
>> to unprepare/prepare MHI channels throughout the transition, this is
>> done by adding suspend/resume callbacks.
>>
>> The suspend callback is called in the late suspend stage, this means
>> MHI channels are still alive at suspend stage, and that makes it
>> possible for an MHI controller driver to communicate with others over
>> those channels at suspend stage. While the resume callback is called
>> in the early resume stage, for a similar reason.
>>
>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
>>
>> Signed-off-by: Baochen Qiang <[email protected]>
>
> Reviewed-by: Manivannan Sadhasivam <[email protected]>
>
> Could you please confirm if this patch can be applied separately and won't cause
> regression?
Just searched the kernel and found another two drivers using IPCR
channels, one is pci_epf_mhi_driver in
drivers/pci/endpoint/functions/pci-epf-mhi.c and the other is
mhi_pci_driver in drivers/bus/mhi/host/pci_generic.c.
For pci_epf_mhi_driver, this is not impacted because the devices for
those channels are attached to mhi_ep_bus_type while QRTR MHI driver
attached to mhi_bus_type.
For mhi_pci_driver, I am afraid there would be regressions caused by
this patch. The regression is because when system suspends,
mhi_pci_suspend() is called and then qcom_mhi_qrtr_pm_suspend_late()
called, however the latter would fail because MHI is moved to M3 state
by call mhi_pm_suspend() in mhi_pci_suspend(). To address this, there
might be two options: the first is to move mhi_pci_suspend() to a late
suspend stage which would be called after
qcom_mhi_qrtr_pm_suspend_late(). and the second is to avoid whole QRTR
suspend operation in such cases. I prefer the second one, because if MHI
is going to suspend, instead of power down, it is pointless to unprepare
MHI channels and re-prepare them after resume back. We can achieve this
purpose by adding a status_cb() to QRTR MHI driver which would be called
when MHI goes to low power mode. And then QRTR MHI driver could decide
not to suspend/resume if low power mode is notified.
Your thoughts?
>
> - Mani
>
>> ---
>> net/qrtr/mhi.c | 27 +++++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
>> index 9ced13c0627a..e96b82a6742c 100644
>> --- a/net/qrtr/mhi.c
>> +++ b/net/qrtr/mhi.c
>> @@ -118,6 +118,32 @@ static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = {
>> };
>> MODULE_DEVICE_TABLE(mhi, qcom_mhi_qrtr_id_table);
>>
>> +static int __maybe_unused qcom_mhi_qrtr_pm_suspend_late(struct device *dev)
>> +{
>> + struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev);
>> +
>> + mhi_unprepare_from_transfer(mhi_dev);
>> +
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused qcom_mhi_qrtr_pm_resume_early(struct device *dev)
>> +{
>> + struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev);
>> + int rc;
>> +
>> + rc = mhi_prepare_for_transfer_autoqueue(mhi_dev);
>> + if (rc)
>> + dev_err(dev, "failed to prepare for autoqueue transfer %d\n", rc);
>> +
>> + return rc;
>> +}
>> +
>> +static const struct dev_pm_ops qcom_mhi_qrtr_pm_ops = {
>> + SET_LATE_SYSTEM_SLEEP_PM_OPS(qcom_mhi_qrtr_pm_suspend_late,
>> + qcom_mhi_qrtr_pm_resume_early)
>> +};
>> +
>> static struct mhi_driver qcom_mhi_qrtr_driver = {
>> .probe = qcom_mhi_qrtr_probe,
>> .remove = qcom_mhi_qrtr_remove,
>> @@ -126,6 +152,7 @@ static struct mhi_driver qcom_mhi_qrtr_driver = {
>> .id_table = qcom_mhi_qrtr_id_table,
>> .driver = {
>> .name = "qcom_mhi_qrtr",
>> + .pm = &qcom_mhi_qrtr_pm_ops,
>> },
>> };
>>
>> --
>> 2.25.1
>>
>
On Tue, Feb 27, 2024 at 04:39:41PM +0800, Baochen Qiang wrote:
>
>
> On 2/27/2024 3:15 PM, Manivannan Sadhasivam wrote:
> > On Tue, Feb 27, 2024 at 02:36:12PM +0800, Baochen Qiang wrote:
> > > MHI devices may not be destroyed during suspend/hibernation, so need
> > > to unprepare/prepare MHI channels throughout the transition, this is
> > > done by adding suspend/resume callbacks.
> > >
> > > The suspend callback is called in the late suspend stage, this means
> > > MHI channels are still alive at suspend stage, and that makes it
> > > possible for an MHI controller driver to communicate with others over
> > > those channels at suspend stage. While the resume callback is called
> > > in the early resume stage, for a similar reason.
> > >
> > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
> > >
> > > Signed-off-by: Baochen Qiang <[email protected]>
> >
> > Reviewed-by: Manivannan Sadhasivam <[email protected]>
> >
> > Could you please confirm if this patch can be applied separately and won't cause
> > regression?
> Just searched the kernel and found another two drivers using IPCR channels,
> one is pci_epf_mhi_driver in drivers/pci/endpoint/functions/pci-epf-mhi.c
> and the other is mhi_pci_driver in drivers/bus/mhi/host/pci_generic.c.
>
> For pci_epf_mhi_driver, this is not impacted because the devices for those
> channels are attached to mhi_ep_bus_type while QRTR MHI driver attached to
> mhi_bus_type.
>
> For mhi_pci_driver, I am afraid there would be regressions caused by this
> patch. The regression is because when system suspends, mhi_pci_suspend() is
> called and then qcom_mhi_qrtr_pm_suspend_late() called, however the latter
> would fail because MHI is moved to M3 state by call mhi_pm_suspend() in
> mhi_pci_suspend(). To address this, there might be two options: the first is
> to move mhi_pci_suspend() to a late suspend stage which would be called
> after qcom_mhi_qrtr_pm_suspend_late(). and the second is to avoid whole QRTR
> suspend operation in such cases. I prefer the second one, because if MHI is
> going to suspend, instead of power down, it is pointless to unprepare MHI
> channels and re-prepare them after resume back. We can achieve this purpose
> by adding a status_cb() to QRTR MHI driver which would be called when MHI
> goes to low power mode. And then QRTR MHI driver could decide not to
> suspend/resume if low power mode is notified.
>
> Your thoughts?
>
I'd rather query the MHI state of the device in suspend/resume callback using
mhi_get_mhi_state(mhi_dev->mhi_cntrl) and if the device is found to be in M3
(suspend) state, then I'd skip prepare/unprepare part.
Because this makes it clear that, if the device is in suspend state, then no
need for the client drivers to unprepare/prepare the channels.
- Mani
--
மணிவண்ணன் சதாசிவம்
On 2/27/2024 5:56 PM, Manivannan Sadhasivam wrote:
> On Tue, Feb 27, 2024 at 04:39:41PM +0800, Baochen Qiang wrote:
>>
>>
>> On 2/27/2024 3:15 PM, Manivannan Sadhasivam wrote:
>>> On Tue, Feb 27, 2024 at 02:36:12PM +0800, Baochen Qiang wrote:
>>>> MHI devices may not be destroyed during suspend/hibernation, so need
>>>> to unprepare/prepare MHI channels throughout the transition, this is
>>>> done by adding suspend/resume callbacks.
>>>>
>>>> The suspend callback is called in the late suspend stage, this means
>>>> MHI channels are still alive at suspend stage, and that makes it
>>>> possible for an MHI controller driver to communicate with others over
>>>> those channels at suspend stage. While the resume callback is called
>>>> in the early resume stage, for a similar reason.
>>>>
>>>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
>>>>
>>>> Signed-off-by: Baochen Qiang <[email protected]>
>>>
>>> Reviewed-by: Manivannan Sadhasivam <[email protected]>
>>>
>>> Could you please confirm if this patch can be applied separately and won't cause
>>> regression?
>> Just searched the kernel and found another two drivers using IPCR channels,
>> one is pci_epf_mhi_driver in drivers/pci/endpoint/functions/pci-epf-mhi.c
>> and the other is mhi_pci_driver in drivers/bus/mhi/host/pci_generic.c.
>>
>> For pci_epf_mhi_driver, this is not impacted because the devices for those
>> channels are attached to mhi_ep_bus_type while QRTR MHI driver attached to
>> mhi_bus_type.
>>
>> For mhi_pci_driver, I am afraid there would be regressions caused by this
>> patch. The regression is because when system suspends, mhi_pci_suspend() is
>> called and then qcom_mhi_qrtr_pm_suspend_late() called, however the latter
>> would fail because MHI is moved to M3 state by call mhi_pm_suspend() in
>> mhi_pci_suspend(). To address this, there might be two options: the first is
>> to move mhi_pci_suspend() to a late suspend stage which would be called
>> after qcom_mhi_qrtr_pm_suspend_late(). and the second is to avoid whole QRTR
>> suspend operation in such cases. I prefer the second one, because if MHI is
>> going to suspend, instead of power down, it is pointless to unprepare MHI
>> channels and re-prepare them after resume back. We can achieve this purpose
>> by adding a status_cb() to QRTR MHI driver which would be called when MHI
>> goes to low power mode. And then QRTR MHI driver could decide not to
>> suspend/resume if low power mode is notified.
>>
>> Your thoughts?
>>
>
> I'd rather query the MHI state of the device in suspend/resume callback using
> mhi_get_mhi_state(mhi_dev->mhi_cntrl) and if the device is found to be in M3
> (suspend) state, then I'd skip prepare/unprepare part.
>
> Because this makes it clear that, if the device is in suspend state, then no
> need for the client drivers to unprepare/prepare the channels.
>
Sounds reasonable, will do accordingly in v3.
> - Mani
>