[RFC 0/2] iommu/arm-smmu-v3: Support software retention for pm_resume

[RFC 0/2] iommu/arm-smmu-v3: Support software retention for pm_resume

[Yisheng Xie]

- Backgroud:
 Hisilicon's implement of smmuv3 do not support hardware retention if system
 do power gating when system suspend, however for embed system, we do need
 to do power gating at trust zone for lower power comsume. So software
 retention is need.

- Implement:
 From the process of smmu probe, it will get the feature of smmu from IDR
 registers and keep the status in struct arm_smmu_device, then
 arm_smmu_device_reset() will initial(set) most of the registers according
 to it. So we can use arm_smmu_device_reset() to re-initial the smmuv3
 device to make it can works again.
 
 To achieve this, patch 1 move the bypass parameter from arm_smmu_device_reset()
 to struct arm_smmu_device to make arm_smmu_device_reset() re-usable. Patch 2
 introduces probed parameter for struct arm_smmu_device, so once smmuv3 is probed
 we can avoid request irqs once more. It also introduce a struct arm_smmu_msi_val
 to keep the value of smmuv3's msi register which can be restore when reset device
 after probed.

Yisheng Xie (2):
  iommu/arm-smmu-v3: Remove bypass in arm_smmu_reset_device
  iommu/arm-smmu-v3: Support software retention for pm_resume

 drivers/iommu/arm-smmu-v3.c | 80 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 72 insertions(+), 8 deletions(-)

-- 
1.7.12.4

[RFC 1/2] iommu/arm-smmu-v3: Remove bypass in arm_smmu_reset_device

[Yisheng Xie]

Add a bypass parameter in arm_smmu_device to keep whether smmu device
should pypass or not, so parameter bypass in arm_smmu_reset_device can
be removed.

This should not have any functional change, but prepare for later patch.

Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 1d64710..044df6e 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -587,6 +587,8 @@ struct arm_smmu_device {
 
 	u32				sync_count;
 
+	bool				bypass;
+
 	/* IOMMU core code handle */
 	struct iommu_device		iommu;
 };
@@ -2384,7 +2386,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;
@@ -2487,7 +2489,7 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
 
 
 	/* 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);
@@ -2778,7 +2780,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) {
@@ -2796,7 +2797,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);
@@ -2842,7 +2843,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);
+	ret = arm_smmu_device_reset(smmu);
 	if (ret)
 		return ret;
 
-- 
1.7.12.4

Re: [RFC 1/2] iommu/arm-smmu-v3: Remove bypass in arm_smmu_reset_device

[Robin Murphy]

On 23/04/18 12:45, Yisheng Xie wrote:
> Add a bypass parameter in arm_smmu_device to keep whether smmu device
> should pypass or not, so parameter bypass in arm_smmu_reset_device can
> be removed.

Given that the GBPA configuration implied by the bypass argument here is 
only there to avoid initialising a full stream table when the firmware 
is terminally broken, I wonder if it would make sense to simply skip 
allocating a stream table at all in that case. Then we could just base 
the subsequent SMMUEN/GPBA decision on whether strtab_cfg.strtab is 
valid or not.

Robin.

> This should not have any functional change, but prepare for later patch.
> 
> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
> ---
>   drivers/iommu/arm-smmu-v3.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 1d64710..044df6e 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -587,6 +587,8 @@ struct arm_smmu_device {
>   
>   	u32				sync_count;
>   
> +	bool				bypass;
> +
>   	/* IOMMU core code handle */
>   	struct iommu_device		iommu;
>   };
> @@ -2384,7 +2386,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;
> @@ -2487,7 +2489,7 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>   
>   
>   	/* 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);
> @@ -2778,7 +2780,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) {
> @@ -2796,7 +2797,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);
> @@ -2842,7 +2843,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);
> +	ret = arm_smmu_device_reset(smmu);
>   	if (ret)
>   		return ret;
>   
> 

Re: [RFC 1/2] iommu/arm-smmu-v3: Remove bypass in arm_smmu_reset_device

[Yisheng Xie]

Hi Robin,

Thanks for your comment!
On 2018/4/24 0:07, Robin Murphy wrote:
> On 23/04/18 12:45, Yisheng Xie wrote:
>> Add a bypass parameter in arm_smmu_device to keep whether smmu device
>> should pypass or not, so parameter bypass in arm_smmu_reset_device can
>> be removed.
> 
> Given that the GBPA configuration implied by the bypass argument here
> is only there to avoid initialising a full stream table when the firmware
> is terminally broken, I wonder if it would make sense to simply skip
> allocating a stream table at all in that case. Then we could just base
> the subsequent SMMUEN/GPBA decision on whether strtab_cfg.strtab is valid or not.
> 
Yes, it may save many memory, and should be care about not access strtab when attach
device or other similar scenario, anyway software can achieve this after move bypass
to struct arm_smmu_device.

Thanks
Yisheng
> Robin.
> 

[RFC 2/2] iommu/arm-smmu-v3: Support software retention for pm_resume

[Yisheng Xie]

When system suspend, hisilicon's smmu will do power gating for smmu,
this time smmu's reg will be set to default value for not having
hardware retention, which means need software do the retention instead.

The patch is to use arm_smmu_device_reset() to restore the register of
smmu. However, it need to save the msis setting at probe if smmu do not
support hardware retention.

Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 044df6e..6cb56d8 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -534,6 +534,11 @@ struct arm_smmu_strtab_cfg {
 	u32				strtab_base_cfg;
 };
 
+struct arm_smmu_msi_val {
+	u64				doorbell;
+	u32				data;
+};
+
 /* An SMMUv3 instance */
 struct arm_smmu_device {
 	struct device			*dev;
@@ -558,6 +563,7 @@ struct arm_smmu_device {
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
 #define ARM_SMMU_OPT_PAGE0_REGS_ONLY	(1 << 1)
+#define ARM_SMMU_OPT_SW_RETENTION	(1 << 2)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;
@@ -587,6 +593,8 @@ struct arm_smmu_device {
 
 	u32				sync_count;
 
+	struct arm_smmu_msi_val		*msi;
+	bool				probed;
 	bool				bypass;
 
 	/* IOMMU core code handle */
@@ -630,6 +638,7 @@ struct arm_smmu_option_prop {
 static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
 	{ ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
+	{ ARM_SMMU_OPT_SW_RETENTION, "hisilicon,broken-hardware-retention" },
 	{ 0, NULL},
 };
 
@@ -2228,7 +2237,8 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
 	phys_addr_t doorbell;
 	struct device *dev = msi_desc_to_dev(desc);
 	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
-	phys_addr_t *cfg = arm_smmu_msi_cfg[desc->platform.msi_index];
+	int msi_index = desc->platform.msi_index;
+	phys_addr_t *cfg = arm_smmu_msi_cfg[msi_index];
 
 	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
 	doorbell &= MSI_CFG0_ADDR_MASK;
@@ -2236,6 +2246,28 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
 	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]);
+
+	if (smmu->options & ARM_SMMU_OPT_SW_RETENTION) {
+		smmu->msi[msi_index].doorbell = doorbell;
+		smmu->msi[msi_index].data = msg->data;
+	}
+}
+
+static void arm_smmu_restore_msis(struct arm_smmu_device *smmu)
+{
+	int nevc = ARM_SMMU_MAX_MSIS - 1;
+
+	if (!(smmu->features & ARM_SMMU_FEAT_PRI))
+		nevc--;
+
+	for (; nevc >= 0; nevc--) {
+		phys_addr_t *cfg = arm_smmu_msi_cfg[nevc];
+		struct arm_smmu_msi_val msi_val = smmu->msi[nevc];
+
+		writeq_relaxed(msi_val.doorbell, smmu->base + cfg[0]);
+		writel_relaxed(msi_val.data, smmu->base + cfg[1]);
+		writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
+	}
 }
 
 static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
@@ -2261,6 +2293,16 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
 		return;
 	}
 
+	if (smmu->probed) {
+		BUG_ON(!(smmu->options & ARM_SMMU_OPT_SW_RETENTION));
+		arm_smmu_restore_msis(smmu);
+		return;
+	} else if (smmu->options & ARM_SMMU_OPT_SW_RETENTION) {
+		smmu->msi = devm_kmalloc_array(dev, nvec,
+					       sizeof(*(smmu->msi)),
+					       GFP_KERNEL);
+	}
+
 	/* Allocate MSIs for evtq, gerror and priq. Ignore cmdq */
 	ret = platform_msi_domain_alloc_irqs(dev, nvec, arm_smmu_write_msi_msg);
 	if (ret) {
@@ -2294,6 +2336,9 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
 
 	arm_smmu_setup_msis(smmu);
 
+	if (smmu->probed)
+		return;
+
 	/* Request interrupt lines */
 	irq = smmu->evtq.q.irq;
 	if (irq) {
@@ -2348,7 +2393,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 	}
 
 	irq = smmu->combined_irq;
-	if (irq) {
+	if (irq && !smmu->probed) {
 		/*
 		 * Cavium ThunderX2 implementation doesn't not support unique
 		 * irq lines. Use single irq line for all the SMMUv3 interrupts.
@@ -2360,7 +2405,7 @@ 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
+	} else if (!irq)
 		arm_smmu_setup_unique_irqs(smmu);
 
 	if (smmu->features & ARM_SMMU_FEAT_PRI)
@@ -2882,6 +2927,9 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		if (ret)
 			return ret;
 	}
+
+	smmu->probed = true;
+
 	return 0;
 }
 
@@ -2899,6 +2947,20 @@ static void arm_smmu_device_shutdown(struct platform_device *pdev)
 	arm_smmu_device_remove(pdev);
 }
 
+static int arm_smmu_pm_resume(struct device *dev)
+{
+	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+	if (smmu->options & ARM_SMMU_OPT_SW_RETENTION)
+		return arm_smmu_device_reset(smmu);
+
+	return 0;
+}
+
+static const struct dev_pm_ops arm_smmu_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume)
+};
+
 static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v3", },
 	{ },
@@ -2909,6 +2971,7 @@ static void arm_smmu_device_shutdown(struct platform_device *pdev)
 	.driver	= {
 		.name		= "arm-smmu-v3",
 		.of_match_table	= of_match_ptr(arm_smmu_of_match),
+		.pm		= &arm_smmu_pm_ops,
 	},
 	.probe	= arm_smmu_device_probe,
 	.remove	= arm_smmu_device_remove,
-- 
1.7.12.4

Re: [RFC 2/2] iommu/arm-smmu-v3: Support software retention for pm_resume

[Robin Murphy]

On 23/04/18 12:45, Yisheng Xie wrote:
> When system suspend, hisilicon's smmu will do power gating for smmu,
> this time smmu's reg will be set to default value for not having
> hardware retention, which means need software do the retention instead.
> 
> The patch is to use arm_smmu_device_reset() to restore the register of
> smmu. However, it need to save the msis setting at probe if smmu do not
> support hardware retention.
> 
> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
> ---
>   drivers/iommu/arm-smmu-v3.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 66 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 044df6e..6cb56d8 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -534,6 +534,11 @@ struct arm_smmu_strtab_cfg {
>   	u32				strtab_base_cfg;
>   };
>   
> +struct arm_smmu_msi_val {
> +	u64				doorbell;
> +	u32				data;
> +};

What does this do that struct msi_msg doesn't already (apart from take 
up more space in an array)?

> +
>   /* An SMMUv3 instance */
>   struct arm_smmu_device {
>   	struct device			*dev;
> @@ -558,6 +563,7 @@ struct arm_smmu_device {
>   
>   #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
>   #define ARM_SMMU_OPT_PAGE0_REGS_ONLY	(1 << 1)
> +#define ARM_SMMU_OPT_SW_RETENTION	(1 << 2)
>   	u32				options;
>   
>   	struct arm_smmu_cmdq		cmdq;
> @@ -587,6 +593,8 @@ struct arm_smmu_device {
>   
>   	u32				sync_count;
>   
> +	struct arm_smmu_msi_val		*msi;
> +	bool				probed;

This looks really hacky. I'm sure there's probably enough driver model 
information to be able to identify the probe state from just the struct 
device, but that's still not the right way to go. If you need to know 
this, then it can only mean we've got one-time software state 
initialisation mixed in with the actual hardware reset which programs 
the software state into the device. Thus there should be some 
refactoring to properly separate those concerns.

>   	bool				bypass;
>   
>   	/* IOMMU core code handle */
> @@ -630,6 +638,7 @@ struct arm_smmu_option_prop {
>   static struct arm_smmu_option_prop arm_smmu_options[] = {
>   	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
>   	{ ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
> +	{ ARM_SMMU_OPT_SW_RETENTION, "hisilicon,broken-hardware-retention" },

That seems a bit over-specific - there are going to be any number of 
SMMU implementations/integrations which may or may not implement 
hardware retention states. More crucially, it's also backwards. Making 
the driver assume that *every* SMMU implements hardware retention unless 
this new DT property is present is quite obviously completely wrong, 
especially for ACPI...

The sensible thing to do is to implement suspend/resume support which 
works in general, *then* consider optimising it for cases where 
explicitly restoring the hardware state may be skipped (if indeed it 
makes a significant difference). Are there not already generic DT/ACPI 
properties for describing the retention levels of different power 
states, which could be made use of here?

>   	{ 0, NULL},
>   };
>   
> @@ -2228,7 +2237,8 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
>   	phys_addr_t doorbell;
>   	struct device *dev = msi_desc_to_dev(desc);
>   	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> -	phys_addr_t *cfg = arm_smmu_msi_cfg[desc->platform.msi_index];
> +	int msi_index = desc->platform.msi_index;
> +	phys_addr_t *cfg = arm_smmu_msi_cfg[msi_index];
>   
>   	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
>   	doorbell &= MSI_CFG0_ADDR_MASK;
> @@ -2236,6 +2246,28 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
>   	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]);
> +
> +	if (smmu->options & ARM_SMMU_OPT_SW_RETENTION) {

The overhead of writing an extra 12 bytes per MSI to memory is entirely 
negligible; saving the message data just doesn't warrant the complexity 
of being conditional. In fact, given the need to untangle the IRQ 
requests from the hardware reset, I'd rather expect to end up *only* 
saving the message here, and writing the IRQ_CFG registers later along 
with everything else.

> +		smmu->msi[msi_index].doorbell = doorbell;
> +		smmu->msi[msi_index].data = msg->data;
> +	}
> +}
> +
> +static void arm_smmu_restore_msis(struct arm_smmu_device *smmu)
> +{
> +	int nevc = ARM_SMMU_MAX_MSIS - 1;
> +
> +	if (!(smmu->features & ARM_SMMU_FEAT_PRI))
> +		nevc--;
> +
> +	for (; nevc >= 0; nevc--) {
> +		phys_addr_t *cfg = arm_smmu_msi_cfg[nevc];
> +		struct arm_smmu_msi_val msi_val = smmu->msi[nevc];
> +
> +		writeq_relaxed(msi_val.doorbell, smmu->base + cfg[0]);
> +		writel_relaxed(msi_val.data, smmu->base + cfg[1]);
> +		writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
> +	}
>   }
>   
>   static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
> @@ -2261,6 +2293,16 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>   		return;
>   	}
>   
> +	if (smmu->probed) {
> +		BUG_ON(!(smmu->options & ARM_SMMU_OPT_SW_RETENTION));
> +		arm_smmu_restore_msis(smmu);
> +		return;
> +	} else if (smmu->options & ARM_SMMU_OPT_SW_RETENTION) {
> +		smmu->msi = devm_kmalloc_array(dev, nvec,
> +					       sizeof(*(smmu->msi)),
> +					       GFP_KERNEL);
> +	}

A single code path which either allocates an empty array *or* writes the 
contents of that array to hardware is a clear "you're doing it wrong" 
indicator. Yes, this definitely wants refactoring.

> +
>   	/* Allocate MSIs for evtq, gerror and priq. Ignore cmdq */
>   	ret = platform_msi_domain_alloc_irqs(dev, nvec, arm_smmu_write_msi_msg);
>   	if (ret) {
> @@ -2294,6 +2336,9 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
>   
>   	arm_smmu_setup_msis(smmu);
>   
> +	if (smmu->probed)
> +		return;
> +
>   	/* Request interrupt lines */
>   	irq = smmu->evtq.q.irq;
>   	if (irq) {
> @@ -2348,7 +2393,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>   	}
>   
>   	irq = smmu->combined_irq;
> -	if (irq) {
> +	if (irq && !smmu->probed) {
>   		/*
>   		 * Cavium ThunderX2 implementation doesn't not support unique
>   		 * irq lines. Use single irq line for all the SMMUv3 interrupts.
> @@ -2360,7 +2405,7 @@ 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
> +	} else if (!irq)
>   		arm_smmu_setup_unique_irqs(smmu);
>   
>   	if (smmu->features & ARM_SMMU_FEAT_PRI)
> @@ -2882,6 +2927,9 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   		if (ret)
>   			return ret;
>   	}
> +
> +	smmu->probed = true;
> +
>   	return 0;
>   }
>   
> @@ -2899,6 +2947,20 @@ static void arm_smmu_device_shutdown(struct platform_device *pdev)
>   	arm_smmu_device_remove(pdev);
>   }
>   
> +static int arm_smmu_pm_resume(struct device *dev)
> +{
> +	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +
> +	if (smmu->options & ARM_SMMU_OPT_SW_RETENTION)
> +		return arm_smmu_device_reset(smmu);

Given the SYSTEM_SLEEP_PM_OPS below, does your hardware really preserve 
state across hibernate/restore as well? That would be particularly 
impressive ;)

Robin.

> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops arm_smmu_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume)
> +};
> +
>   static const struct of_device_id arm_smmu_of_match[] = {
>   	{ .compatible = "arm,smmu-v3", },
>   	{ },
> @@ -2909,6 +2971,7 @@ static void arm_smmu_device_shutdown(struct platform_device *pdev)
>   	.driver	= {
>   		.name		= "arm-smmu-v3",
>   		.of_match_table	= of_match_ptr(arm_smmu_of_match),
> +		.pm		= &arm_smmu_pm_ops,
>   	},
>   	.probe	= arm_smmu_device_probe,
>   	.remove	= arm_smmu_device_remove,
> 

Re: [RFC 2/2] iommu/arm-smmu-v3: Support software retention for pm_resume

[Yisheng Xie]

Hi Robin,

Thanks for your comment.
On 2018/4/24 0:14, Robin Murphy wrote:
> On 23/04/18 12:45, Yisheng Xie wrote:
>> When system suspend, hisilicon's smmu will do power gating for smmu,
>> this time smmu's reg will be set to default value for not having
>> hardware retention, which means need software do the retention instead.
>>
>> The patch is to use arm_smmu_device_reset() to restore the register of
>> smmu. However, it need to save the msis setting at probe if smmu do not
>> support hardware retention.
>>
>> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
>> ---
>>   drivers/iommu/arm-smmu-v3.c | 69 +++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 66 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 044df6e..6cb56d8 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -534,6 +534,11 @@ struct arm_smmu_strtab_cfg {
>>       u32                strtab_base_cfg;
>>   };
>>   +struct arm_smmu_msi_val {
>> +    u64                doorbell;
>> +    u32                data;
>> +};
> 
> What does this do that struct msi_msg doesn't already (apart from take up more space in an array)?

Right, struct msi_msg can be used instead, and memcpy can be used when save the msi_msg.

> 
>> +
>>   /* An SMMUv3 instance */
>>   struct arm_smmu_device {
>>       struct device            *dev;
>> @@ -558,6 +563,7 @@ struct arm_smmu_device {
>>     #define ARM_SMMU_OPT_SKIP_PREFETCH    (1 << 0)
>>   #define ARM_SMMU_OPT_PAGE0_REGS_ONLY    (1 << 1)
>> +#define ARM_SMMU_OPT_SW_RETENTION    (1 << 2)
>>       u32                options;
>>         struct arm_smmu_cmdq        cmdq;
>> @@ -587,6 +593,8 @@ struct arm_smmu_device {
>>         u32                sync_count;
>>   +    struct arm_smmu_msi_val        *msi;
>> +    bool                probed;
> 
> This looks really hacky. I'm sure there's probably enough driver model information
> to be able to identify the probe state from just the struct device, but that's still
> not the right way to go. If you need to know this, then it can only mean we've got
> one-time software state initialisation mixed in with the actual hardware reset which
> programs the software state into the device. Thus there should be some refactoring to
> properly separate those concerns.

So you mean status enum should use instead? Maybe something like:
enum arm_smmu_status {
	ARM_SMMU_INACTIVE;
	ARM_SMMU_ACTIVE;
	ARM_SMMU_SUSPEND;
};
to indicate the status of device ?

> 
>>       bool                bypass;
>>         /* IOMMU core code handle */
>> @@ -630,6 +638,7 @@ struct arm_smmu_option_prop {
>>   static struct arm_smmu_option_prop arm_smmu_options[] = {
>>       { ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
>>       { ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
>> +    { ARM_SMMU_OPT_SW_RETENTION, "hisilicon,broken-hardware-retention" },
> 
> That seems a bit over-specific - there are going to be any number of SMMU implementations/integrations
> which may or may not implement hardware retention states. More crucially, it's also backwards. Making
> the driver assume that *every* SMMU implements hardware retention unless this new DT property is present
> is quite obviously completely wrong, especially for ACPI...
> 
I was thought about remove the word *hisilicon* for other implement may
also not have hardware retention. Anyway, this a wrong for ACPI cannot
works without hardware retention.

> The sensible thing to do is to implement suspend/resume support which works in general, *then* consider
> optimising it for cases where explicitly restoring the hardware state may be skipped (if indeed it makes
> a significant difference).

Yes, we should make it common, what I was doubt is that whether should enable it
by default or not, for someone who do not want do power gating at all when suspend,
it may suffer from device reset when resume ?

> Are there not already generic DT/ACPI properties for describing the retention
> levels of different power states, which could be made use of here?

AFAIK no, and I have just tried to find some clue from the document of DT/ACPI
but do not find any usefull information. Do you have any idea or clue about it?

>>       { 0, NULL},
>>   };
>>   @@ -2228,7 +2237,8 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
>>       phys_addr_t doorbell;
>>       struct device *dev = msi_desc_to_dev(desc);
>>       struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>> -    phys_addr_t *cfg = arm_smmu_msi_cfg[desc->platform.msi_index];
>> +    int msi_index = desc->platform.msi_index;
>> +    phys_addr_t *cfg = arm_smmu_msi_cfg[msi_index];
>>         doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
>>       doorbell &= MSI_CFG0_ADDR_MASK;
>> @@ -2236,6 +2246,28 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
>>       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]);
>> +
>> +    if (smmu->options & ARM_SMMU_OPT_SW_RETENTION) {
> 
> The overhead of writing an extra 12 bytes per MSI to memory is entirely negligible;
> saving the message data just doesn't warrant the complexity of being conditional. 
> In fact, given the need to untangle the IRQ requests from the hardware reset, I'd
> rather expect to end up *only* saving the message here, and writing the IRQ_CFG
> registers later along with everything else.

I will use msi_msg instead in next version.

> 
>> +        smmu->msi[msi_index].doorbell = doorbell;
>> +        smmu->msi[msi_index].data = msg->data;
>> +    }
>> +}
>> +
>> +static void arm_smmu_restore_msis(struct arm_smmu_device *smmu)
>> +{
>> +    int nevc = ARM_SMMU_MAX_MSIS - 1;
>> +
>> +    if (!(smmu->features & ARM_SMMU_FEAT_PRI))
>> +        nevc--;
>> +
>> +    for (; nevc >= 0; nevc--) {
>> +        phys_addr_t *cfg = arm_smmu_msi_cfg[nevc];
>> +        struct arm_smmu_msi_val msi_val = smmu->msi[nevc];
>> +
>> +        writeq_relaxed(msi_val.doorbell, smmu->base + cfg[0]);
>> +        writel_relaxed(msi_val.data, smmu->base + cfg[1]);
>> +        writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
>> +    }
>>   }
>>     static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>> @@ -2261,6 +2293,16 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>>           return;
>>       }
>>   +    if (smmu->probed) {
>> +        BUG_ON(!(smmu->options & ARM_SMMU_OPT_SW_RETENTION));
>> +        arm_smmu_restore_msis(smmu);
>> +        return;
>> +    } else if (smmu->options & ARM_SMMU_OPT_SW_RETENTION) {
>> +        smmu->msi = devm_kmalloc_array(dev, nvec,
>> +                           sizeof(*(smmu->msi)),
>> +                           GFP_KERNEL);
>> +    }
> 
> A single code path which either allocates an empty array *or* writes the contents of that array to hardware is a clear "you're doing it wrong" indicator. Yes, this definitely wants refactoring.

Right!

> 
>> +
>>       /* Allocate MSIs for evtq, gerror and priq. Ignore cmdq */
>>       ret = platform_msi_domain_alloc_irqs(dev, nvec, arm_smmu_write_msi_msg);
>>       if (ret) {
>> @@ -2294,6 +2336,9 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
>>         arm_smmu_setup_msis(smmu);
>>   +    if (smmu->probed)
>> +        return;
>> +
>>       /* Request interrupt lines */
>>       irq = smmu->evtq.q.irq;
>>       if (irq) {
>> @@ -2348,7 +2393,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>>       }
>>         irq = smmu->combined_irq;
>> -    if (irq) {
>> +    if (irq && !smmu->probed) {
>>           /*
>>            * Cavium ThunderX2 implementation doesn't not support unique
>>            * irq lines. Use single irq line for all the SMMUv3 interrupts.
>> @@ -2360,7 +2405,7 @@ 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
>> +    } else if (!irq)
>>           arm_smmu_setup_unique_irqs(smmu);
>>         if (smmu->features & ARM_SMMU_FEAT_PRI)
>> @@ -2882,6 +2927,9 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>           if (ret)
>>               return ret;
>>       }
>> +
>> +    smmu->probed = true;
>> +
>>       return 0;
>>   }
>>   @@ -2899,6 +2947,20 @@ static void arm_smmu_device_shutdown(struct platform_device *pdev)
>>       arm_smmu_device_remove(pdev);
>>   }
>>   +static int arm_smmu_pm_resume(struct device *dev)
>> +{
>> +    struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>> +
>> +    if (smmu->options & ARM_SMMU_OPT_SW_RETENTION)
>> +        return arm_smmu_device_reset(smmu);
> 
> Given the SYSTEM_SLEEP_PM_OPS below, does your hardware really preserve state across hibernate/restore as well?

Sorry, I not quite get this point, and maybe I was missing something.
What's state should also be preserved by software?

Thanks
Yisheng
> That would be particularly impressive ;)
> 
> Robin.
>