On 2021/7/21 21:59, Robin Murphy wrote:
>> On Wed, 21 Jul 2021 12:42:14 +0100,
>> Robin Murphy <[email protected]> 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 <[email protected]>
>>>> Reviewed-by: Wei Yongjun <[email protected]>
>>>> Reviewed-by: Zhen Lei <[email protected]>
>>>> Reviewed-by: Ding Tianhong <[email protected]>
>>>> Reviewed-by: Hanjun Guo <[email protected]>
>>>> ---
>>>>
>>>> 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
> appropriate. Personally I have a particular dislike of this general
> anti-patten
>
> int do_a_thing(bool but_only_do_part_of_it)
I have verified this patch on the board in arm64, and the MSI interrupt can
work properly after resume.
The important work is to restore
ARM_SMMU_EVTQ_IRQ_CFGn/ARM_SMMU_GERROR_IRQ_CFGn/ARM_SMMU_PRIQ_IRQ_CFG0 register
after the resume.
When adding the suspend/resume, I try to change the original code as little as
possible.
Thanks
Bixuan Cui
>
>>>> + }
>>>> /* 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?
>>
>> The author of the patch has conveniently made it a global value (see
>> line 3 of the patch). I'm sure it doesn't break anything... :-(
>
> Oh blimey, I hadn't even turned on my "things that make no flippin' sense"
> radar to notice that, thanks for the correction. Indeed that's pretty busted
> for SMMU instances probing concurrently.
>
> As with interrupts, reset probably wants a bit of refactoring to separate out
> the probe-time concerns of cleaning up any left-over configuration from
> firmware/kdump/whatever, and the act of putting the hardware into the
> operational state we want. For instance, if it happens that state is *not*
> lost on suspend, subsequently warning about CR0_SMMUEN being enabled on
> resume would be bad.
>
>>>> {
>>>> 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?
>>
>> +1. This seems awfully implementation/integration specific. I'd be
>> more in favour of a controlled teardown.
Indeed, The current code applies to the scenario where the SMMU is directly
powered off.
I'm not sure what other implementation/integration are. :(
>>
>>>
>>>> + 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...
>>>
>>>> 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.
>
> As above, apologies for the oversight there. It's not as badly wrong as I
> thought, but it's still not right.
>
> Thanks,
> Robin.
>
>>>
>>>> + 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.
Thank you for the advice here.
Thanks,
Bixuan Cui
>>
>> Thanks,
>>
>> M.
>>
> .
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
