Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
On 2021/7/21 21:59, Robin Murphy wrote:
>> On Wed, 21 Jul 2021 12:42:14 +0100,
>> Robin Murphy wrote:
>>>
>>> [ +Marc for MSI bits ]
>>>
>>> On 2021-07-21 02:33, Bixuan Cui wrote:
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
Reviewed-by: Wei Yongjun
Reviewed-by: Zhen Lei
Reviewed-by: Ding Tianhong
Reviewed-by: Hanjun Guo
---
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
>>> :/
>>
>> Certainly not. If you rely on the message being stored into the
>> descriptors, then implement this in the core code, like we do for PCI.
>
> Ah, so it would be an acceptable compromise to *read* desc->msg (and thus
> avoid having to store our own copy of the message) if the core was guaranteed
> to cache it? That's good to know, thanks.
>
+
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));
>>
>> Consider using get_cached_msi_msg() instead of using the internals of
>> the descriptor.
>
> Oh, there's even a proper API for it, marvellous! I hadn't managed to dig
> that far myself :)
Thanks for your suggestion, I'll use get_cached_msi_msg() instead.
>
+ 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?
>>
>> Yeah. I assume the SMMU needs to be told which event gets signalled
>> using MSIs or IRQs? Or is that implied by the MSI being configured or
>> not (I used to know the answer to that, but I have long paged it out).
>
> My mistake there - I misread the rather convoluted existing flow to think
> that bailing here would fail to enable wired IRQs, but of course the
> signalling decision is based on whether the MSI address is non-zero, and the
> enables in ARM_SMMU_IRQ_CTRL still apply either way>
> Given that, I think this is that point at which to refactor this whole part
> so that logically requesting and physically programming the interrupts are
> split into distinct stages, which can then be called seperately as
>
Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
On 2021/7/21 23:01, Marc Zyngier wrote:
> On Wed, 21 Jul 2021 14:59:47 +0100,
> Robin Murphy wrote:
>>
>> On 2021-07-21 14:12, Marc Zyngier wrote:
>>> On Wed, 21 Jul 2021 12:42:14 +0100,
>>> Robin Murphy wrote:
[ +Marc for MSI bits ]
On 2021-07-21 02:33, Bixuan Cui wrote:
> 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
> Reviewed-by: Wei Yongjun
> Reviewed-by: Zhen Lei
> Reviewed-by: Ding Tianhong
> Reviewed-by: Hanjun Guo
> ---
>
>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
:/
>>>
>>> Certainly not. If you rely on the message being stored into the
>>> descriptors, then implement this in the core code, like we do for PCI.
>>
>> Ah, so it would be an acceptable compromise to *read* desc->msg (and
>> thus avoid having to store our own copy of the message) if the core
>> was guaranteed to cache it? That's good to know, thanks.
>
> Yeah, vfio, a couple of other weird drivers and (*surprise!*) ia64 are
> using this kind of trick. I don't see a reason not to implement that
> for platform-MSI (although level signalling may be interesting...), or
> even to move it into the core MSI code.
Agree. If msg is saved to desc->msg in MSI core, the code here will not need.
During the initialization of the MSI interrupt of the SMMU, the desc->msg
is never used. So I save msg to desc->msg for resume use.
>>
> +
> 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));
>>>
>>> Consider using get_cached_msi_msg() instead of using the internals of
>>> the descriptor.
>>
>> Oh, there's even a proper API for it, marvellous! I hadn't managed to
>> dig that far myself :)
>
> It is a bit odd in the sense that it takes a copy of the message
> instead of returning a pointer, but at least this solves lifetime
> issues.
The code of arm_smmu_write_msi_msg() is multiplexed to restore the register.
Therefore,
the parameter must be supplemented. Generally, desc is sufficient as an input
parameter..
:)
Thanks,
Bixuan Cui
>
> Thanks,
>
> M.
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
On Wed, 21 Jul 2021 14:59:47 +0100,
Robin Murphy wrote:
>
> On 2021-07-21 14:12, Marc Zyngier wrote:
> > On Wed, 21 Jul 2021 12:42:14 +0100,
> > Robin Murphy wrote:
> >>
> >> [ +Marc for MSI bits ]
> >>
> >> On 2021-07-21 02:33, Bixuan Cui wrote:
> >>> 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
> >>> Reviewed-by: Wei Yongjun
> >>> Reviewed-by: Zhen Lei
> >>> Reviewed-by: Ding Tianhong
> >>> Reviewed-by: Hanjun Guo
> >>> ---
> >>>
> >>>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
> >> :/
> >
> > Certainly not. If you rely on the message being stored into the
> > descriptors, then implement this in the core code, like we do for PCI.
>
> Ah, so it would be an acceptable compromise to *read* desc->msg (and
> thus avoid having to store our own copy of the message) if the core
> was guaranteed to cache it? That's good to know, thanks.
Yeah, vfio, a couple of other weird drivers and (*surprise!*) ia64 are
using this kind of trick. I don't see a reason not to implement that
for platform-MSI (although level signalling may be interesting...), or
even to move it into the core MSI code.
>
> >>> +
> >>> 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));
> >
> > Consider using get_cached_msi_msg() instead of using the internals of
> > the descriptor.
>
> Oh, there's even a proper API for it, marvellous! I hadn't managed to
> dig that far myself :)
It is a bit odd in the sense that it takes a copy of the message
instead of returning a pointer, but at least this solves lifetime
issues.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
On 2021-07-21 14:12, Marc Zyngier wrote:
On Wed, 21 Jul 2021 12:42:14 +0100,
Robin Murphy wrote:
[ +Marc for MSI bits ]
On 2021-07-21 02:33, Bixuan Cui wrote:
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
Reviewed-by: Wei Yongjun
Reviewed-by: Zhen Lei
Reviewed-by: Ding Tianhong
Reviewed-by: Hanjun Guo
---
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
:/
Certainly not. If you rely on the message being stored into the
descriptors, then implement this in the core code, like we do for PCI.
Ah, so it would be an acceptable compromise to *read* desc->msg (and
thus avoid having to store our own copy of the message) if the core was
guaranteed to cache it? That's good to know, thanks.
+
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));
Consider using get_cached_msi_msg() instead of using the internals of
the descriptor.
Oh, there's even a proper API for it, marvellous! I hadn't managed to
dig that far myself :)
+ 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?
Yeah. I assume the SMMU needs to be told which event gets signalled
using MSIs or IRQs? Or is that implied by the MSI being configured or
not (I used to know the answer to that, but I have long paged it out).
My mistake there - I misread the rather convoluted existing flow to
think that bailing here would fail to enable wired IRQs, but of course
the signalling decision is based on whether the MSI address is non-zero,
and the enables in ARM_SMMU_IRQ_CTRL still apply either way.
Given that, I think this is that point at which to refactor this whole
part so that logically requesting and physically programming the
interrupts are split into distinct stages, which can then be called
seperately as appropriate. Personally I have a particular dislike of
this general anti-pattern:
int do_a_thing(bool but_only_do_part_of_it)
+ }
/* 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
Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
On Wed, 21 Jul 2021 12:42:14 +0100,
Robin Murphy wrote:
>
> [ +Marc for MSI bits ]
>
> On 2021-07-21 02:33, Bixuan Cui wrote:
> > 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
> > Reviewed-by: Wei Yongjun
> > Reviewed-by: Zhen Lei
> > Reviewed-by: Ding Tianhong
> > Reviewed-by: Hanjun Guo
> > ---
> >
> > 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
> :/
Certainly not. If you rely on the message being stored into the
descriptors, then implement this in the core code, like we do for PCI.
>
> > +
> > 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));
Consider using get_cached_msi_msg() instead of using the internals of
the descriptor.
> > + 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?
Yeah. I assume the SMMU needs to be told which event gets signalled
using MSIs or IRQs? Or is that implied by the MSI being configured or
not (I used to know the answer to that, but I have long paged it out).
>
> > + }
> > /* 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
Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
[ +Marc for MSI bits ]
On 2021-07-21 02:33, Bixuan Cui wrote:
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
Reviewed-by: Wei Yongjun
Reviewed-by: Zhen Lei
Reviewed-by: Ding Tianhong
Reviewed-by: Hanjun Guo
---
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 :/
+
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?
+ }
/* 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?
{
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, );
[PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
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
Reviewed-by: Wei Yongjun
Reviewed-by: Zhen Lei
Reviewed-by: Ding Tianhong
Reviewed-by: Hanjun Guo
---
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;
+ }
+
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;
+ }
/* 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)
{
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, );
}
+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.
+*/
+ return 0;
+}
+
+static int __maybe_unused arm_smmu_resume(struct device *dev)
+{
+ struct
