Thread (7 messages) flat view 7 messages, 3 authors, 2021-07-22

Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support

From: Robin Murphy <robin.murphy@arm.com>
Date: 2021-07-21 11:56:08
Also in: linux-iommu, lkml

[ +Marc for MSI bits ]

On 2021-07-21 02:33, Bixuan Cui wrote:
quoted hunk ↗ jump to hunk
Add suspend and resume support for arm-smmu-v3 by low-power mode.

When the smmu is suspended, it is powered off and the registers are
cleared. So saves the msi_msg context during msi interrupt initialization
of smmu. When resume happens it calls arm_smmu_device_reset() to restore
the registers.

Signed-off-by: Bixuan Cui <redacted>
Reviewed-by: Wei Yongjun <redacted>
Reviewed-by: Zhen Lei <redacted>
Reviewed-by: Ding Tianhong <redacted>
Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
---

  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 72 ++++++++++++++++++---
  1 file changed, 64 insertions(+), 8 deletions(-)
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 235f9bdaeaf2..bf1163acbcb1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -40,6 +40,7 @@ MODULE_PARM_DESC(disable_bypass,
  
  static bool disable_msipolling;
  module_param(disable_msipolling, bool, 0444);
+static bool bypass;
  MODULE_PARM_DESC(disable_msipolling,
  	"Disable MSI-based polling for CMD_SYNC completion.");
  
@@ -3129,11 +3130,37 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
  	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
  	doorbell &= MSI_CFG0_ADDR_MASK;
  
+	/* Saves the msg context for resume if desc->msg is empty */
+	if (desc->msg.address_lo == 0 && desc->msg.address_hi == 0) {
+		desc->msg.address_lo = msg->address_lo;
+		desc->msg.address_hi = msg->address_hi;
+		desc->msg.data = msg->data;
+	}
My gut feeling is that this is something a device driver maybe shouldn't 
be poking into, but I'm not entirely familiar with the area :/
quoted hunk ↗ jump to hunk
+
  	writeq_relaxed(doorbell, smmu->base + cfg[0]);
  	writel_relaxed(msg->data, smmu->base + cfg[1]);
  	writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
  }
  
+static void arm_smmu_resume_msis(struct arm_smmu_device *smmu)
+{
+	struct msi_desc *desc;
+	struct device *dev = smmu->dev;
+
+	for_each_msi_entry(desc, dev) {
+		switch (desc->platform.msi_index) {
+		case EVTQ_MSI_INDEX:
+		case GERROR_MSI_INDEX:
+		case PRIQ_MSI_INDEX:
+			arm_smmu_write_msi_msg(desc, &(desc->msg));
+			break;
+		default:
+			continue;
+
+		}
+	}
+}
+
  static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
  {
  	struct msi_desc *desc;
@@ -3184,11 +3211,17 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
  	devm_add_action(dev, arm_smmu_free_msis, dev);
  }
  
-static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
+static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu, bool resume_mode)
  {
  	int irq, ret;
  
-	arm_smmu_setup_msis(smmu);
+	if (!resume_mode)
+		arm_smmu_setup_msis(smmu);
+	else {
+		/* The irq doesn't need to be re-requested during resume */
+		arm_smmu_resume_msis(smmu);
+		return;
What about wired IRQs?
quoted hunk ↗ jump to hunk
+	}
  
  	/* Request interrupt lines */
  	irq = smmu->evtq.q.irq;
@@ -3230,7 +3263,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_mode)
  {
  	int ret, irq;
  	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
@@ -3257,7 +3290,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
  		if (ret < 0)
  			dev_warn(smmu->dev, "failed to enable combined irq\n");
  	} else
-		arm_smmu_setup_unique_irqs(smmu);
+		arm_smmu_setup_unique_irqs(smmu, resume_mode);
  
  	if (smmu->features & ARM_SMMU_FEAT_PRI)
  		irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
@@ -3282,7 +3315,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, bool bypass)
+static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool resume_mode)
Er, what about the use of "bypass" towards the end of the function. Have 
you even compiled this?
quoted hunk ↗ jump to hunk
  {
  	int ret;
  	u32 reg, enables;
@@ -3392,7 +3425,7 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
  		}
  	}
  
-	ret = arm_smmu_setup_irqs(smmu);
+	ret = arm_smmu_setup_irqs(smmu, resume_mode);
  	if (ret) {
  		dev_err(smmu->dev, "failed to setup irqs\n");
  		return ret;
@@ -3749,6 +3782,24 @@ static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
  	return devm_ioremap_resource(dev, &res);
  }
  
+static int __maybe_unused arm_smmu_suspend(struct device *dev)
+{
+	/*
+	 * The smmu is powered off and related registers are automatically
+	 * cleared when suspend. No need to do anything.
+	 */
Is that guaranteed? What if suspend is only implemented by external 
clock-gating?
quoted hunk ↗ jump to hunk
+	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 int arm_smmu_device_probe(struct platform_device *pdev)
  {
  	int irq, ret;
@@ -3756,7 +3807,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
  	resource_size_t ioaddr;
  	struct arm_smmu_device *smmu;
  	struct device *dev = &pdev->dev;
-	bool bypass;
Once again...
quoted hunk ↗ jump to hunk
  	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
  	if (!smmu)
@@ -3831,7 +3881,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
  	platform_set_drvdata(pdev, smmu);
  
  	/* Reset the device */
-	ret = arm_smmu_device_reset(smmu, bypass);
...either this is based on some out-of-tree hack which introduced its 
own uninitialised-usage bug here, or it doesn't even compile.
quoted hunk ↗ jump to hunk
+	ret = arm_smmu_device_reset(smmu, false);
  	if (ret)
  		return ret;
  
@@ -3884,6 +3934,11 @@ static const struct of_device_id arm_smmu_of_match[] = {
  };
  MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
  
+static const struct dev_pm_ops arm_smmu_pm_ops = {
+	.suspend = arm_smmu_suspend,
+	.resume = arm_smmu_resume,
Either use SET_SYSTEM_SLEEP_PM_OPS() here or drop the __maybe_unused 
annmotations above - they're pointless if the callbacks are referenced 
unconditionally.

Robin.
quoted hunk ↗ jump to hunk
+};
+
  static void arm_smmu_driver_unregister(struct platform_driver *drv)
  {
  	arm_smmu_sva_notifier_synchronize();
@@ -3895,6 +3950,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	= arm_smmu_device_remove,
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help