Re: [PATCH v9 09/13] xen/arm: smmu-v3: add suspend/resume handlers

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Mykola,

> 
> +static int arm_smmu_enable_irqs(struct arm_smmu_device *smmu)
> +{
> + int ret;
> + u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> +
> + if ( smmu->features & ARM_SMMU_FEAT_PRI )
> + irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
> +
> + /* Enable interrupt generation on the SMMU */
> + ret = arm_smmu_write_reg_sync(smmu, irqen_flags,
> +      ARM_SMMU_IRQ_CTRL, ARM_SMMU_IRQ_CTRLACK);
> + if ( ret )
> + {
> + dev_warn(smmu->dev, "failed to enable irqs\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Probe-time only: request host IRQs and, when available, program the SMMU's
> + * MSI doorbells. Resume does not restore the SMMU *_IRQ_CFGn MSI registers,
> + * so any host suspend support must treat the active MSI IRQ path as
> + * unsupported until that restore path exists.
> + */
> static int __init arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> {
> int ret, irq;
> - u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> 
> /* Disable IRQs first */
> ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
> @@ -2028,22 +2052,7 @@ static int __init arm_smmu_setup_irqs(struct 
> arm_smmu_device *smmu)
> }
> }
> 
> - if (smmu->features & ARM_SMMU_FEAT_PRI)
> - irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
> -
> - /* Enable interrupt generation on the SMMU */
> - ret = arm_smmu_write_reg_sync(smmu, irqen_flags,
> -      ARM_SMMU_IRQ_CTRL, ARM_SMMU_IRQ_CTRLACK);
> - if (ret) {
> - dev_warn(smmu->dev, "failed to enable irqs\n");
> - goto err_free_irqs;
> - }
> -
> return 0;
> -
> -err_free_irqs:
> - arm_smmu_free_irqs(smmu);
> - return ret;
> }
> 
> static int arm_smmu_device_disable(struct arm_smmu_device *smmu)
> @@ -2057,7 +2066,7 @@ static int arm_smmu_device_disable(struct 
> arm_smmu_device *smmu)
> return ret;
> }
> 
> -static int __init arm_smmu_device_reset(struct arm_smmu_device *smmu)
> +static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
> {
> int ret;
> u32 reg, enables;
> @@ -2163,17 +2172,9 @@ static int __init arm_smmu_device_reset(struct 
> arm_smmu_device *smmu)
> }
> }
> 
> - ret = arm_smmu_setup_irqs(smmu);
> - if (ret) {
> - dev_err(smmu->dev, "failed to setup irqs\n");
> + ret = arm_smmu_enable_irqs(smmu);
> + if ( ret )
> return ret;
> - }
> -
> - /* Initialize tasklets for threaded IRQs*/
> - tasklet_init(&smmu->evtq_irq_tasklet, arm_smmu_evtq_tasklet, smmu);
> - tasklet_init(&smmu->priq_irq_tasklet, arm_smmu_priq_tasklet, smmu);
> - tasklet_init(&smmu->combined_irq_tasklet, arm_smmu_combined_irq_tasklet,
> - smmu);
> 
> /* Enable the SMMU interface, or ensure bypass */
> if (disable_bypass) {
> @@ -2181,20 +2182,16 @@ static int __init arm_smmu_device_reset(struct 
> arm_smmu_device *smmu)
> } else {
> ret = arm_smmu_update_gbpa(smmu, 0, GBPA_ABORT);
> if (ret)
> - goto err_free_irqs;
> + return ret;
> }
> ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
>      ARM_SMMU_CR0ACK);
> if (ret) {
> dev_err(smmu->dev, "failed to enable SMMU interface\n");
> - goto err_free_irqs;
> + return ret;
> }
> 
> return 0;
> -
> -err_free_irqs:
> - arm_smmu_free_irqs(smmu);
> - return ret;
> }
> 
> static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
> @@ -2558,10 +2555,23 @@ static int __init arm_smmu_device_probe(struct 
> platform_device *pdev)
> if (ret)
> goto out_free;
> 
> + ret = arm_smmu_setup_irqs(smmu);
> + if ( ret )
> + {
> + dev_err(smmu->dev, "failed to setup irqs\n");
> + goto out_free;
> + }
> +
> + /* Initialize tasklets for threaded IRQs*/
> + tasklet_init(&smmu->evtq_irq_tasklet, arm_smmu_evtq_tasklet, smmu);
> + tasklet_init(&smmu->priq_irq_tasklet, arm_smmu_priq_tasklet, smmu);
> + tasklet_init(&smmu->combined_irq_tasklet, arm_smmu_combined_irq_tasklet,
> + smmu);
> +
> /* Reset the device */
> ret = arm_smmu_device_reset(smmu);
> if (ret)
> - goto out_free;
> + goto out_free_irqs;
> 
> /*
> * Keep a list of all probed devices. This will be used to query
> @@ -2575,6 +2585,8 @@ static int __init arm_smmu_device_probe(struct 
> platform_device *pdev)
> 
> return 0;
> 
> +out_free_irqs:
> + arm_smmu_free_irqs(smmu);
> 
> out_free:
> arm_smmu_free_structures(smmu);
> @@ -2855,6 +2867,96 @@ static void arm_smmu_iommu_xen_domain_teardown(struct 
> domain *d)
> xfree(xen_domain);
> }
> 
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +
> +static void arm_smmu_reset_for_suspend_rollback(struct arm_smmu_device *smmu)
> +{
> + int ret = arm_smmu_device_reset(smmu);
> +
> + if ( ret )
> + dev_err(smmu->dev, "Failed to reset during suspend rollback: %d\n",
> + ret);
> +}
> +
> +static int arm_smmu_suspend(void)
> +{
> + struct arm_smmu_device *smmu;
> + int ret = 0;
> +
> + list_for_each_entry(smmu, &arm_smmu_devices, devices)
> + {
> + /* Abort all transactions before disable to avoid spurious bypass */
> + ret = arm_smmu_update_gbpa(smmu, GBPA_ABORT, 0);
> + if ( ret )
> + goto fail;

Should we have this here as the restore path calls arm_smmu_enable_irqs()?

ret = arm_smmu_write_reg_sync(smmu, 0,
                              ARM_SMMU_IRQ_CTRL,
                              ARM_SMMU_IRQ_CTRLACK);
if ( ret )
    goto fail;

Cheers,
Luca