This patchset takes [1] for reference.
Patchset was tested on NXP i.MX95, but since i.MX95 not has SMMU MSI,
so not able to test MSI part. If anyone could help test, that would be
great.
[1]:
https://lore.kernel.org/lkml/[email protected]/T/#m06bf92e2306ba52c106f2d4d24765921d4e9781e
Signed-off-by: Peng Fan <[email protected]>
---
Peng Fan (3):
iommu/arm-smmu-v3: save bypass in smmu device structure
genirq/msi: cache the last msi msg
iommu/arm-smmu-v3: support suspend/resume
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 76 +++++++++++++++++++++++++----
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 +
kernel/irq/msi.c | 3 ++
3 files changed, 70 insertions(+), 10 deletions(-)
---
base-commit: 13ee4a7161b6fd938aef6688ff43b163f6d83e37
change-id: 20240324-smmu-v3-7fd1a43ed735
Best regards,
--
Peng Fan <[email protected]>
From: Peng Fan <[email protected]>
'bypass' will be used in smmu resume function, so need to save
its value for future usage.
Signed-off-by: Peng Fan <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++++-----
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 +
2 files changed, 5 insertions(+), 5 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 5ed036225e69..a8a569573c2f 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3494,7 +3494,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)
{
int ret;
u32 reg, enables;
@@ -3612,7 +3612,7 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
enables &= ~(CR0_EVTQEN | CR0_PRIQEN);
/* Enable the SMMU interface, or ensure bypass */
- if (!bypass || disable_bypass) {
+ if (!smmu->bypass || disable_bypass) {
enables |= CR0_SMMUEN;
} else {
ret = arm_smmu_update_gbpa(smmu, 0, GBPA_ABORT);
@@ -4007,7 +4007,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;
smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
if (!smmu)
@@ -4023,7 +4022,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
}
/* Set bypass mode according to firmware probing result */
- bypass = !!ret;
+ smmu->bypass = !!ret;
/* Base address */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -4087,7 +4086,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, bypass);
+ ret = arm_smmu_device_reset(smmu);
if (ret)
return ret;
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 23baf117e7e4..97064a6aa87c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -683,6 +683,7 @@ struct arm_smmu_device {
struct rb_root streams;
struct mutex streams_mutex;
+ bool bypass;
};
struct arm_smmu_stream {
--
2.37.1
From: Peng Fan <[email protected]>
Cache the last msi msg which will be used for ARM SMMU V3 resume
Signed-off-by: Peng Fan <[email protected]>
---
kernel/irq/msi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index f90952ebc494..9d7e0a675089 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -617,6 +617,9 @@ static unsigned int msi_domain_get_hwsize(struct device *dev, unsigned int domid
static inline void irq_chip_write_msi_msg(struct irq_data *data,
struct msi_msg *msg)
{
+ struct msi_desc *desc = irq_data_get_msi_desc(data);
+
+ desc->msg = *msg;
data->chip->irq_write_msi_msg(data, msg);
}
--
2.37.1
From: Peng Fan <[email protected]>
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 <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 71 ++++++++++++++++++++++++++---
1 file changed, 64 insertions(+), 7 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 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,
--
2.37.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
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.
On Sun, Mar 24 2024 at 20:28, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> Cache the last msi msg which will be used for ARM SMMU V3 resume
>
> Signed-off-by: Peng Fan <[email protected]>
> ---
> kernel/irq/msi.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index f90952ebc494..9d7e0a675089 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -617,6 +617,9 @@ static unsigned int msi_domain_get_hwsize(struct device *dev, unsigned int domid
> static inline void irq_chip_write_msi_msg(struct irq_data *data,
> struct msi_msg *msg)
> {
> + struct msi_desc *desc = irq_data_get_msi_desc(data);
> +
> + desc->msg = *msg;
> data->chip->irq_write_msi_msg(data, msg);
> }
This should remove the very same line in the PCI/MSI code, no?
Thanks,
tglx
> Subject: Re: [PATCH 2/3] genirq/msi: cache the last msi msg
>
> On Sun, Mar 24 2024 at 20:28, Peng Fan (OSS) wrote:
> > From: Peng Fan <[email protected]>
> >
> > Cache the last msi msg which will be used for ARM SMMU V3 resume
> >
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> > kernel/irq/msi.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c index
> > f90952ebc494..9d7e0a675089 100644
> > --- a/kernel/irq/msi.c
> > +++ b/kernel/irq/msi.c
> > @@ -617,6 +617,9 @@ static unsigned int msi_domain_get_hwsize(struct
> > device *dev, unsigned int domid static inline void
> irq_chip_write_msi_msg(struct irq_data *data,
> > struct msi_msg *msg)
> > {
> > + struct msi_desc *desc = irq_data_get_msi_desc(data);
> > +
> > + desc->msg = *msg;
> > data->chip->irq_write_msi_msg(data, msg); }
>
> This should remove the very same line in the PCI/MSI code, no?
I think no, but you have more knowledge than me, please correct if I am
wrong.
To zpci_irq_chip.irq_set_affinity
zpci_set_irq_affinity->pci_write_msi_msg->cache the msg
In kernel/irq/msi.c
msi_domain_update_chip_ops
|-> if (!chip->irq_set_affinity)
chip->irq_set_affinity = msi_domain_set_affinity
From above code, the pci/msi code should keep the caching code.
Thanks,
Peng.
>
> Thanks,
>
> tglx
> 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.
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)