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