diff mbox series

[3/3] iommu/arm-smmu-v3: support suspend/resume

Message ID 20240324-smmu-v3-v1-3-11bc96e156a5@nxp.com (mailing list archive)
State New, archived
Headers show
Series iommu/smmu-v3: support suspend/resume | expand

Commit Message

Peng Fan (OSS) March 24, 2024, 12:29 p.m. UTC 
From: Peng Fan <peng.fan@nxp.com>

smmu maybe power-gated, and the registers are cleared. So
need to restore its registers with arm_smmu_device_reset and
resume the msi interrupt settings in resume callback.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 71 ++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 7 deletions(-)

Comments

Jason Gunthorpe March 25, 2024, 1:48 p.m. UTC | #1 
On Sun, Mar 24, 2024 at 08:29:00PM +0800, Peng Fan (OSS) wrote:

> +static void arm_smmu_resume_unique_irqs(struct arm_smmu_device *smmu)
> +{
> +	struct device *dev = smmu->dev;
> +	struct msi_desc *desc;
> +	struct msi_msg msg;
> +
> +	if (!dev->msi.domain)
> +		return;
> +
> +	desc = irq_get_msi_desc(smmu->evtq.q.irq);
> +	if (desc) {
> +		get_cached_msi_msg(smmu->evtq.q.irq, &msg);
> +		arm_smmu_write_msi_msg(desc, &msg);
> +	}
> +
> +	desc = irq_get_msi_desc(smmu->gerr_irq);
> +	if (desc) {
> +		get_cached_msi_msg(smmu->gerr_irq, &msg);
> +		arm_smmu_write_msi_msg(desc, &msg);
> +	}
> +
> +	if (smmu->features & ARM_SMMU_FEAT_PRI) {
> +		desc = irq_get_msi_desc(smmu->priq.q.irq);
> +		if (desc) {
> +			get_cached_msi_msg(smmu->priq.q.irq, &msg);
> +			arm_smmu_write_msi_msg(desc, &msg);
> +		}
> +	}
> +}

I wonder if this should be done instead by converting the driver away
from platform MSI to the new MSI mechanism?

Jason
Thomas Gleixner March 25, 2024, 5:29 p.m. UTC | #2 
On Mon, Mar 25 2024 at 10:48, Jason Gunthorpe wrote:
> On Sun, Mar 24, 2024 at 08:29:00PM +0800, Peng Fan (OSS) wrote:
>> +static void arm_smmu_resume_unique_irqs(struct arm_smmu_device *smmu)
>> +{
>> +	struct device *dev = smmu->dev;
>> +	struct msi_desc *desc;
>> +	struct msi_msg msg;
>> +
>> +	if (!dev->msi.domain)
>> +		return;
>> +
>> +	desc = irq_get_msi_desc(smmu->evtq.q.irq);
>> +	if (desc) {
>> +		get_cached_msi_msg(smmu->evtq.q.irq, &msg);
>> +		arm_smmu_write_msi_msg(desc, &msg);
>> +	}
>> +
>> +	desc = irq_get_msi_desc(smmu->gerr_irq);
>> +	if (desc) {
>> +		get_cached_msi_msg(smmu->gerr_irq, &msg);
>> +		arm_smmu_write_msi_msg(desc, &msg);
>> +	}
>> +
>> +	if (smmu->features & ARM_SMMU_FEAT_PRI) {
>> +		desc = irq_get_msi_desc(smmu->priq.q.irq);
>> +		if (desc) {
>> +			get_cached_msi_msg(smmu->priq.q.irq, &msg);
>> +			arm_smmu_write_msi_msg(desc, &msg);
>> +		}
>> +	}
>> +}
>
> I wonder if this should be done instead by converting the driver away
> from platform MSI to the new MSI mechanism?

There is work in progress for that. Should come around in the next
weeks.
Peng Fan March 26, 2024, 1:26 a.m. UTC | #3 
> Subject: Re: [PATCH 3/3] iommu/arm-smmu-v3: support suspend/resume
> 
> On Mon, Mar 25 2024 at 10:48, Jason Gunthorpe wrote:
> > On Sun, Mar 24, 2024 at 08:29:00PM +0800, Peng Fan (OSS) wrote:
> >> +static void arm_smmu_resume_unique_irqs(struct arm_smmu_device
> >> +*smmu) {
> >> +	struct device *dev = smmu->dev;
> >> +	struct msi_desc *desc;
> >> +	struct msi_msg msg;
> >> +
> >> +	if (!dev->msi.domain)
> >> +		return;
> >> +
> >> +	desc = irq_get_msi_desc(smmu->evtq.q.irq);
> >> +	if (desc) {
> >> +		get_cached_msi_msg(smmu->evtq.q.irq, &msg);
> >> +		arm_smmu_write_msi_msg(desc, &msg);
> >> +	}
> >> +
> >> +	desc = irq_get_msi_desc(smmu->gerr_irq);
> >> +	if (desc) {
> >> +		get_cached_msi_msg(smmu->gerr_irq, &msg);
> >> +		arm_smmu_write_msi_msg(desc, &msg);
> >> +	}
> >> +
> >> +	if (smmu->features & ARM_SMMU_FEAT_PRI) {
> >> +		desc = irq_get_msi_desc(smmu->priq.q.irq);
> >> +		if (desc) {
> >> +			get_cached_msi_msg(smmu->priq.q.irq, &msg);
> >> +			arm_smmu_write_msi_msg(desc, &msg);
> >> +		}
> >> +	}
> >> +}
> >
> > I wonder if this should be done instead by converting the driver away
> > from platform MSI to the new MSI mechanism?
> 
> There is work in progress for that. Should come around in the next weeks.

Then I need to wait for your patches, and rebase this patchset, or could
the non-msi part be reviewed first?

Thanks,
Peng.
Thomas Gleixner March 26, 2024, 8:11 p.m. UTC | #4 
On Mon, Mar 25 2024 at 18:29, Thomas Gleixner wrote:
> On Mon, Mar 25 2024 at 10:48, Jason Gunthorpe wrote:
>> On Sun, Mar 24, 2024 at 08:29:00PM +0800, Peng Fan (OSS) wrote:
>>> +	if (smmu->features & ARM_SMMU_FEAT_PRI) {
>>> +		desc = irq_get_msi_desc(smmu->priq.q.irq);
>>> +		if (desc) {
>>> +			get_cached_msi_msg(smmu->priq.q.irq, &msg);
>>> +			arm_smmu_write_msi_msg(desc, &msg);
>>> +		}
>>> +	}
>>> +}
>>
>> I wonder if this should be done instead by converting the driver away
>> from platform MSI to the new MSI mechanism?
>
> There is work in progress for that. Should come around in the next
> weeks.

But that won't solve it. The above is a horrible hack and I think this
should be solved completely differently.

On suspend the core interrupt code disables all interrupts which are not
marked as wakeup interrupts. On resume it reenables them including
restoring the affinity setting.

So this just should work, but it does not because the MSI message is
only written once, when the interrupt is activated. Further affinity
changes affect only the ITS table and do not result in a message write.

The most trivial way w/o changing any core code for it, would be to free
the interrupts on suspend and request them on resume again, because that
would deactivate and reactivate it.

Though that does not work if the IOMMU is resumed in the early resume
path. But we can enforce writing the message in the resume path.
See the untested below.

Thanks,

        tglx
---
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -223,6 +223,7 @@ struct irq_data {
  *				  irqchip have flag IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND set.
  * IRQD_RESEND_WHEN_IN_PROGRESS	- Interrupt may fire when already in progress in which
  *				  case it must be resent at the next available opportunity.
+ * IRQD_RESUMING		- Interrupt is resumed after suspend
  */
 enum {
 	IRQD_TRIGGER_MASK		= 0xf,
@@ -249,6 +250,7 @@ enum {
 	IRQD_AFFINITY_ON_ACTIVATE	= BIT(28),
 	IRQD_IRQ_ENABLED_ON_SUSPEND	= BIT(29),
 	IRQD_RESEND_WHEN_IN_PROGRESS    = BIT(30),
+	IRQD_RESUMING			= BIT(31),
 };
 
 #define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors)
@@ -443,6 +445,11 @@ static inline bool irqd_needs_resend_whe
 	return __irqd_to_state(d) & IRQD_RESEND_WHEN_IN_PROGRESS;
 }
 
+static inline bool irqd_resuming(struct irq_data *d)
+{
+	return __irqd_to_state(d) & IRQD_RESUMING;
+}
+
 #undef __irqd_to_state
 
 static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -617,6 +617,7 @@ static unsigned int msi_domain_get_hwsiz
 static inline void irq_chip_write_msi_msg(struct irq_data *data,
 					  struct msi_msg *msg)
 {
+	irq_data_get_msi_desc(data)->msg = *msg;
 	data->chip->irq_write_msi_msg(data, msg);
 }
 
@@ -652,7 +653,10 @@ int msi_domain_set_affinity(struct irq_d
 	int ret;
 
 	ret = parent->chip->irq_set_affinity(parent, mask, force);
-	if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
+	if (ret < 0)
+		return ret;
+
+	if (ret != IRQ_SET_MASK_OK_DONE || irqd_resuming(irq_data)) {
 		BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
 		msi_check_level(irq_data->domain, msg);
 		irq_chip_write_msi_msg(irq_data, msg);
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -177,7 +177,13 @@ static void resume_irq(struct irq_desc *
 	irq_state_set_masked(desc);
 resume:
 	desc->istate &= ~IRQS_SUSPENDED;
+	/*
+	 * Ensure that MSI messages get rewritten on resume. The device
+	 * might have lost it due to power disabling.
+	 */
+	irqd_set(&desc->irq_data, IRQD_RESUMING);
 	__enable_irq(desc);
+	irqd_clear(&desc->irq_data, IRQD_RESUMING);
 }
 
 static void resume_irqs(bool want_early)
diff mbox series

Patch

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index a8a569573c2f..2bfe4b3d0ba1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3396,6 +3396,36 @@  static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
 	devm_add_action(dev, arm_smmu_free_msis, dev);
 }
 
+static void arm_smmu_resume_unique_irqs(struct arm_smmu_device *smmu)
+{
+	struct device *dev = smmu->dev;
+	struct msi_desc *desc;
+	struct msi_msg msg;
+
+	if (!dev->msi.domain)
+		return;
+
+	desc = irq_get_msi_desc(smmu->evtq.q.irq);
+	if (desc) {
+		get_cached_msi_msg(smmu->evtq.q.irq, &msg);
+		arm_smmu_write_msi_msg(desc, &msg);
+	}
+
+	desc = irq_get_msi_desc(smmu->gerr_irq);
+	if (desc) {
+		get_cached_msi_msg(smmu->gerr_irq, &msg);
+		arm_smmu_write_msi_msg(desc, &msg);
+	}
+
+	if (smmu->features & ARM_SMMU_FEAT_PRI) {
+		desc = irq_get_msi_desc(smmu->priq.q.irq);
+		if (desc) {
+			get_cached_msi_msg(smmu->priq.q.irq, &msg);
+			arm_smmu_write_msi_msg(desc, &msg);
+		}
+	}
+}
+
 static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
 {
 	int irq, ret;
@@ -3442,7 +3472,7 @@  static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
 	}
 }
 
-static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
+static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu, bool resume)
 {
 	int ret, irq;
 	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
@@ -3456,7 +3486,7 @@  static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 	}
 
 	irq = smmu->combined_irq;
-	if (irq) {
+	if (irq && !resume) {
 		/*
 		 * Cavium ThunderX2 implementation doesn't support unique irq
 		 * lines. Use a single irq line for all the SMMUv3 interrupts.
@@ -3468,8 +3498,12 @@  static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 					"arm-smmu-v3-combined-irq", smmu);
 		if (ret < 0)
 			dev_warn(smmu->dev, "failed to enable combined irq\n");
-	} else
-		arm_smmu_setup_unique_irqs(smmu);
+	} else {
+		if (resume)
+			arm_smmu_resume_unique_irqs(smmu);
+		else
+			arm_smmu_setup_unique_irqs(smmu);
+	}
 
 	if (smmu->features & ARM_SMMU_FEAT_PRI)
 		irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
@@ -3494,7 +3528,7 @@  static int arm_smmu_device_disable(struct arm_smmu_device *smmu)
 	return ret;
 }
 
-static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
+static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool resume)
 {
 	int ret;
 	u32 reg, enables;
@@ -3602,7 +3636,7 @@  static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
 		}
 	}
 
-	ret = arm_smmu_setup_irqs(smmu);
+	ret = arm_smmu_setup_irqs(smmu, resume);
 	if (ret) {
 		dev_err(smmu->dev, "failed to setup irqs\n");
 		return ret;
@@ -4086,7 +4120,7 @@  static int arm_smmu_device_probe(struct platform_device *pdev)
 	arm_smmu_rmr_install_bypass_ste(smmu);
 
 	/* Reset the device */
-	ret = arm_smmu_device_reset(smmu);
+	ret = arm_smmu_device_reset(smmu, false);
 	if (ret)
 		return ret;
 
@@ -4124,12 +4158,34 @@  static void arm_smmu_device_shutdown(struct platform_device *pdev)
 	arm_smmu_device_disable(smmu);
 }
 
+static int __maybe_unused arm_smmu_suspend(struct device *dev)
+{
+	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+	arm_smmu_device_disable(smmu);
+
+	return 0;
+}
+
+static int __maybe_unused arm_smmu_resume(struct device *dev)
+{
+	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+	arm_smmu_device_reset(smmu, true);
+
+	return 0;
+}
+
 static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v3", },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
 
+static const struct dev_pm_ops arm_smmu_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(arm_smmu_suspend, arm_smmu_resume)
+};
+
 static void arm_smmu_driver_unregister(struct platform_driver *drv)
 {
 	arm_smmu_sva_notifier_synchronize();
@@ -4141,6 +4197,7 @@  static struct platform_driver arm_smmu_driver = {
 		.name			= "arm-smmu-v3",
 		.of_match_table		= arm_smmu_of_match,
 		.suppress_bind_attrs	= true,
+		.pm			= &arm_smmu_pm_ops,
 	},
 	.probe	= arm_smmu_device_probe,
 	.remove_new = arm_smmu_device_remove,