[PATCH] iommu/arm-smmu: Report smmu type in dmesg

[PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robert Richter]

The ARM SMMU detection especially depends from system firmware. For
better diagnostic, log the detected type in dmesg.

The smmu type's name is now stored in struct arm_smmu_type and ACPI
code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
macro to ARM_SMMU_TYPE() for better readability.

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index abf6496843a6..5c793b3d3173 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -366,6 +366,7 @@ struct arm_smmu_device {
 	u32				options;
 	enum arm_smmu_arch_version	version;
 	enum arm_smmu_implementation	model;
+	const char			*name;
 
 	u32				num_context_banks;
 	u32				num_s2_context_banks;
@@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	return 0;
 }
 
-struct arm_smmu_match_data {
+struct arm_smmu_type {
 	enum arm_smmu_arch_version version;
 	enum arm_smmu_implementation model;
+	const char *name;
 };
 
-#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
-static struct arm_smmu_match_data name = { .version = ver, .model = imp }
+#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
+static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
 
-ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
-ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
-ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
-ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
-ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
+ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
+ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
+ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
+ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
+ARM_SMMU_TYPE(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2, "cavium-smmuv2");
 
 static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
@@ -1981,29 +1983,19 @@ static const struct of_device_id arm_smmu_of_match[] = {
 MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
 
 #ifdef CONFIG_ACPI
-static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
+static struct arm_smmu_type *acpi_smmu_get_type(u32 model)
 {
-	int ret = 0;
-
 	switch (model) {
 	case ACPI_IORT_SMMU_V1:
 	case ACPI_IORT_SMMU_CORELINK_MMU400:
-		smmu->version = ARM_SMMU_V1;
-		smmu->model = GENERIC_SMMU;
-		break;
+		return &smmu_generic_v1;
 	case ACPI_IORT_SMMU_V2:
-		smmu->version = ARM_SMMU_V2;
-		smmu->model = GENERIC_SMMU;
-		break;
+		return &smmu_generic_v2;
 	case ACPI_IORT_SMMU_CORELINK_MMU500:
-		smmu->version = ARM_SMMU_V2;
-		smmu->model = ARM_MMU500;
-		break;
-	default:
-		ret = -ENODEV;
+		return &arm_mmu500;
 	}
 
-	return ret;
+	return NULL;
 }
 
 static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
@@ -2013,14 +2005,18 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
 	struct acpi_iort_node *node =
 		*(struct acpi_iort_node **)dev_get_platdata(dev);
 	struct acpi_iort_smmu *iort_smmu;
-	int ret;
+	struct arm_smmu_type *type;
 
 	/* Retrieve SMMU1/2 specific data */
 	iort_smmu = (struct acpi_iort_smmu *)node->node_data;
 
-	ret = acpi_smmu_get_data(iort_smmu->model, smmu);
-	if (ret < 0)
-		return ret;
+	type = acpi_smmu_get_type(iort_smmu->model);
+	if (!type)
+		return -ENODEV;
+
+	smmu->version	= type->version;
+	smmu->model	= type->model;
+	smmu->name	= type->name;
 
 	/* Ignore the configuration access interrupt */
 	smmu->num_global_irqs = 1;
@@ -2041,8 +2037,8 @@ static inline int arm_smmu_device_acpi_probe(struct platform_device *pdev,
 static int arm_smmu_device_dt_probe(struct platform_device *pdev,
 				    struct arm_smmu_device *smmu)
 {
-	const struct arm_smmu_match_data *data;
 	struct device *dev = &pdev->dev;
+	const struct arm_smmu_type *type;
 	bool legacy_binding;
 
 	if (of_property_read_u32(dev->of_node, "#global-interrupts",
@@ -2051,9 +2047,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
 		return -ENODEV;
 	}
 
-	data = of_device_get_match_data(dev);
-	smmu->version = data->version;
-	smmu->model = data->model;
+	type = of_device_get_match_data(dev);
+	smmu->version	= type->version;
+	smmu->model	= type->model;
+	smmu->name	= type->name;
 
 	parse_driver_options(smmu);
 
@@ -2098,6 +2095,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	dev_notice(dev, "%s detected", smmu->name);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	ioaddr = res->start;
 	smmu->base = devm_ioremap_resource(dev, res);
-- 
2.11.0

[PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robert Richter]

The ARM SMMU detection especially depends from system firmware. For
better diagnostic, log the detected type in dmesg.

The smmu type's name is now stored in struct arm_smmu_type and ACPI
code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
macro to ARM_SMMU_TYPE() for better readability.

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index abf6496843a6..5c793b3d3173 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -366,6 +366,7 @@ struct arm_smmu_device {
 	u32				options;
 	enum arm_smmu_arch_version	version;
 	enum arm_smmu_implementation	model;
+	const char			*name;
 
 	u32				num_context_banks;
 	u32				num_s2_context_banks;
@@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	return 0;
 }
 
-struct arm_smmu_match_data {
+struct arm_smmu_type {
 	enum arm_smmu_arch_version version;
 	enum arm_smmu_implementation model;
+	const char *name;
 };
 
-#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
-static struct arm_smmu_match_data name = { .version = ver, .model = imp }
+#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
+static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
 
-ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
-ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
-ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
-ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
-ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
+ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
+ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
+ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
+ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
+ARM_SMMU_TYPE(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2, "cavium-smmuv2");
 
 static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
@@ -1981,29 +1983,19 @@ static const struct of_device_id arm_smmu_of_match[] = {
 MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
 
 #ifdef CONFIG_ACPI
-static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
+static struct arm_smmu_type *acpi_smmu_get_type(u32 model)
 {
-	int ret = 0;
-
 	switch (model) {
 	case ACPI_IORT_SMMU_V1:
 	case ACPI_IORT_SMMU_CORELINK_MMU400:
-		smmu->version = ARM_SMMU_V1;
-		smmu->model = GENERIC_SMMU;
-		break;
+		return &smmu_generic_v1;
 	case ACPI_IORT_SMMU_V2:
-		smmu->version = ARM_SMMU_V2;
-		smmu->model = GENERIC_SMMU;
-		break;
+		return &smmu_generic_v2;
 	case ACPI_IORT_SMMU_CORELINK_MMU500:
-		smmu->version = ARM_SMMU_V2;
-		smmu->model = ARM_MMU500;
-		break;
-	default:
-		ret = -ENODEV;
+		return &arm_mmu500;
 	}
 
-	return ret;
+	return NULL;
 }
 
 static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
@@ -2013,14 +2005,18 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
 	struct acpi_iort_node *node =
 		*(struct acpi_iort_node **)dev_get_platdata(dev);
 	struct acpi_iort_smmu *iort_smmu;
-	int ret;
+	struct arm_smmu_type *type;
 
 	/* Retrieve SMMU1/2 specific data */
 	iort_smmu = (struct acpi_iort_smmu *)node->node_data;
 
-	ret = acpi_smmu_get_data(iort_smmu->model, smmu);
-	if (ret < 0)
-		return ret;
+	type = acpi_smmu_get_type(iort_smmu->model);
+	if (!type)
+		return -ENODEV;
+
+	smmu->version	= type->version;
+	smmu->model	= type->model;
+	smmu->name	= type->name;
 
 	/* Ignore the configuration access interrupt */
 	smmu->num_global_irqs = 1;
@@ -2041,8 +2037,8 @@ static inline int arm_smmu_device_acpi_probe(struct platform_device *pdev,
 static int arm_smmu_device_dt_probe(struct platform_device *pdev,
 				    struct arm_smmu_device *smmu)
 {
-	const struct arm_smmu_match_data *data;
 	struct device *dev = &pdev->dev;
+	const struct arm_smmu_type *type;
 	bool legacy_binding;
 
 	if (of_property_read_u32(dev->of_node, "#global-interrupts",
@@ -2051,9 +2047,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
 		return -ENODEV;
 	}
 
-	data = of_device_get_match_data(dev);
-	smmu->version = data->version;
-	smmu->model = data->model;
+	type = of_device_get_match_data(dev);
+	smmu->version	= type->version;
+	smmu->model	= type->model;
+	smmu->name	= type->name;
 
 	parse_driver_options(smmu);
 
@@ -2098,6 +2095,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	dev_notice(dev, "%s detected", smmu->name);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	ioaddr = res->start;
 	smmu->base = devm_ioremap_resource(dev, res);
-- 
2.11.0

[PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robert Richter]

The ARM SMMU detection especially depends from system firmware. For
better diagnostic, log the detected type in dmesg.

The smmu type's name is now stored in struct arm_smmu_type and ACPI
code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
macro to ARM_SMMU_TYPE() for better readability.

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index abf6496843a6..5c793b3d3173 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -366,6 +366,7 @@ struct arm_smmu_device {
 	u32				options;
 	enum arm_smmu_arch_version	version;
 	enum arm_smmu_implementation	model;
+	const char			*name;
 
 	u32				num_context_banks;
 	u32				num_s2_context_banks;
@@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	return 0;
 }
 
-struct arm_smmu_match_data {
+struct arm_smmu_type {
 	enum arm_smmu_arch_version version;
 	enum arm_smmu_implementation model;
+	const char *name;
 };
 
-#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
-static struct arm_smmu_match_data name = { .version = ver, .model = imp }
+#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
+static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
 
-ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
-ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
-ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
-ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
-ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
+ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
+ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
+ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
+ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
+ARM_SMMU_TYPE(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2, "cavium-smmuv2");
 
 static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
@@ -1981,29 +1983,19 @@ static const struct of_device_id arm_smmu_of_match[] = {
 MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
 
 #ifdef CONFIG_ACPI
-static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
+static struct arm_smmu_type *acpi_smmu_get_type(u32 model)
 {
-	int ret = 0;
-
 	switch (model) {
 	case ACPI_IORT_SMMU_V1:
 	case ACPI_IORT_SMMU_CORELINK_MMU400:
-		smmu->version = ARM_SMMU_V1;
-		smmu->model = GENERIC_SMMU;
-		break;
+		return &smmu_generic_v1;
 	case ACPI_IORT_SMMU_V2:
-		smmu->version = ARM_SMMU_V2;
-		smmu->model = GENERIC_SMMU;
-		break;
+		return &smmu_generic_v2;
 	case ACPI_IORT_SMMU_CORELINK_MMU500:
-		smmu->version = ARM_SMMU_V2;
-		smmu->model = ARM_MMU500;
-		break;
-	default:
-		ret = -ENODEV;
+		return &arm_mmu500;
 	}
 
-	return ret;
+	return NULL;
 }
 
 static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
@@ -2013,14 +2005,18 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
 	struct acpi_iort_node *node =
 		*(struct acpi_iort_node **)dev_get_platdata(dev);
 	struct acpi_iort_smmu *iort_smmu;
-	int ret;
+	struct arm_smmu_type *type;
 
 	/* Retrieve SMMU1/2 specific data */
 	iort_smmu = (struct acpi_iort_smmu *)node->node_data;
 
-	ret = acpi_smmu_get_data(iort_smmu->model, smmu);
-	if (ret < 0)
-		return ret;
+	type = acpi_smmu_get_type(iort_smmu->model);
+	if (!type)
+		return -ENODEV;
+
+	smmu->version	= type->version;
+	smmu->model	= type->model;
+	smmu->name	= type->name;
 
 	/* Ignore the configuration access interrupt */
 	smmu->num_global_irqs = 1;
@@ -2041,8 +2037,8 @@ static inline int arm_smmu_device_acpi_probe(struct platform_device *pdev,
 static int arm_smmu_device_dt_probe(struct platform_device *pdev,
 				    struct arm_smmu_device *smmu)
 {
-	const struct arm_smmu_match_data *data;
 	struct device *dev = &pdev->dev;
+	const struct arm_smmu_type *type;
 	bool legacy_binding;
 
 	if (of_property_read_u32(dev->of_node, "#global-interrupts",
@@ -2051,9 +2047,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
 		return -ENODEV;
 	}
 
-	data = of_device_get_match_data(dev);
-	smmu->version = data->version;
-	smmu->model = data->model;
+	type = of_device_get_match_data(dev);
+	smmu->version	= type->version;
+	smmu->model	= type->model;
+	smmu->name	= type->name;
 
 	parse_driver_options(smmu);
 
@@ -2098,6 +2095,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	dev_notice(dev, "%s detected", smmu->name);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	ioaddr = res->start;
 	smmu->base = devm_ioremap_resource(dev, res);
-- 
2.11.0

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robin Murphy]

On 06/03/17 13:58, Robert Richter wrote:
> The ARM SMMU detection especially depends from system firmware. For
> better diagnostic, log the detected type in dmesg.

This paragraph especially depends from grammar. I think.

> The smmu type's name is now stored in struct arm_smmu_type and ACPI
> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
> macro to ARM_SMMU_TYPE() for better readability.
> 
> Signed-off-by: Robert Richter <rrichter@cavium.com>
> ---
>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 31 deletions(-)

That seems a relatively invasive diffstat for the sake of printing a
string once at boot time to what I can only assume is a small audience
of firmware developers who find "cat
/sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
equivalent) too hard ;)

Assuming there is a really good reason somewhere to justify this, I
still wonder if a simple self-contained "smmu->model to string" function
wouldn't do, if we really want to do this? Maybe it's not quite that
simple if the generic case needs to key off smmu->version as well, but
still. Arguably, just searching the of_match_table by model/version and
printing the corresponding DT compatible would do the job (given an
MMU-400 model to disambiguate those).

Either way it ought to be replacing the "SMMUv%d with:" message in
arm_smmu_device_cfg_probe() - this driver is noisy enough already
without starting to repeat itself.

> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index abf6496843a6..5c793b3d3173 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -366,6 +366,7 @@ struct arm_smmu_device {
>  	u32				options;
>  	enum arm_smmu_arch_version	version;
>  	enum arm_smmu_implementation	model;
> +	const char			*name;

If we are going to add a pointer to static implementation data, it may
as well be a "const arm_smmu_type *type" pointer to subsume version and
model as well.

>  
>  	u32				num_context_banks;
>  	u32				num_s2_context_banks;
> @@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  	return 0;
>  }
>  
> -struct arm_smmu_match_data {
> +struct arm_smmu_type {
>  	enum arm_smmu_arch_version version;
>  	enum arm_smmu_implementation model;
> +	const char *name;
>  };
>  
> -#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> +#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
> +static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
>  
> -ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
> -ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
> +ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
> +ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
> +ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");

Strictly, I think you mean "ARM® CoreLink™ MMU-401". Printing the name
of a driver-internal structure looks like someone left a debugging hack in.

> +ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");

And similarly. Of course, I'm not actually suggesting that using the
full marketing names of things is a great idea, but then again if we do
go naming specific IPs, and anyone gets sniffy about using the names
"properly", then guess what's going to get removed again? You'll always
find me firmly in the "easier not to go there" camp.

Robin.

> +ARM_SMMU_TYPE(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2, "cavium-smmuv2");
>  
>  static const struct of_device_id arm_smmu_of_match[] = {
>  	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
> @@ -1981,29 +1983,19 @@ static const struct of_device_id arm_smmu_of_match[] = {
>  MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>  
>  #ifdef CONFIG_ACPI
> -static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
> +static struct arm_smmu_type *acpi_smmu_get_type(u32 model)
>  {
> -	int ret = 0;
> -
>  	switch (model) {
>  	case ACPI_IORT_SMMU_V1:
>  	case ACPI_IORT_SMMU_CORELINK_MMU400:
> -		smmu->version = ARM_SMMU_V1;
> -		smmu->model = GENERIC_SMMU;
> -		break;
> +		return &smmu_generic_v1;
>  	case ACPI_IORT_SMMU_V2:
> -		smmu->version = ARM_SMMU_V2;
> -		smmu->model = GENERIC_SMMU;
> -		break;
> +		return &smmu_generic_v2;
>  	case ACPI_IORT_SMMU_CORELINK_MMU500:
> -		smmu->version = ARM_SMMU_V2;
> -		smmu->model = ARM_MMU500;
> -		break;
> -	default:
> -		ret = -ENODEV;
> +		return &arm_mmu500;
>  	}
>  
> -	return ret;
> +	return NULL;
>  }
>  
>  static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
> @@ -2013,14 +2005,18 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  	struct acpi_iort_node *node =
>  		*(struct acpi_iort_node **)dev_get_platdata(dev);
>  	struct acpi_iort_smmu *iort_smmu;
> -	int ret;
> +	struct arm_smmu_type *type;
>  
>  	/* Retrieve SMMU1/2 specific data */
>  	iort_smmu = (struct acpi_iort_smmu *)node->node_data;
>  
> -	ret = acpi_smmu_get_data(iort_smmu->model, smmu);
> -	if (ret < 0)
> -		return ret;
> +	type = acpi_smmu_get_type(iort_smmu->model);
> +	if (!type)
> +		return -ENODEV;
> +
> +	smmu->version	= type->version;
> +	smmu->model	= type->model;
> +	smmu->name	= type->name;
>  
>  	/* Ignore the configuration access interrupt */
>  	smmu->num_global_irqs = 1;
> @@ -2041,8 +2037,8 @@ static inline int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  				    struct arm_smmu_device *smmu)
>  {
> -	const struct arm_smmu_match_data *data;
>  	struct device *dev = &pdev->dev;
> +	const struct arm_smmu_type *type;
>  	bool legacy_binding;
>  
>  	if (of_property_read_u32(dev->of_node, "#global-interrupts",
> @@ -2051,9 +2047,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  		return -ENODEV;
>  	}
>  
> -	data = of_device_get_match_data(dev);
> -	smmu->version = data->version;
> -	smmu->model = data->model;
> +	type = of_device_get_match_data(dev);
> +	smmu->version	= type->version;
> +	smmu->model	= type->model;
> +	smmu->name	= type->name;
>  
>  	parse_driver_options(smmu);
>  
> @@ -2098,6 +2095,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> +	dev_notice(dev, "%s detected", smmu->name);
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	ioaddr = res->start;
>  	smmu->base = devm_ioremap_resource(dev, res);
> 

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robin Murphy]

On 06/03/17 13:58, Robert Richter wrote:
> The ARM SMMU detection especially depends from system firmware. For
> better diagnostic, log the detected type in dmesg.

This paragraph especially depends from grammar. I think.

> The smmu type's name is now stored in struct arm_smmu_type and ACPI
> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
> macro to ARM_SMMU_TYPE() for better readability.
> 
> Signed-off-by: Robert Richter <rrichter-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 31 deletions(-)

That seems a relatively invasive diffstat for the sake of printing a
string once at boot time to what I can only assume is a small audience
of firmware developers who find "cat
/sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
equivalent) too hard ;)

Assuming there is a really good reason somewhere to justify this, I
still wonder if a simple self-contained "smmu->model to string" function
wouldn't do, if we really want to do this? Maybe it's not quite that
simple if the generic case needs to key off smmu->version as well, but
still. Arguably, just searching the of_match_table by model/version and
printing the corresponding DT compatible would do the job (given an
MMU-400 model to disambiguate those).

Either way it ought to be replacing the "SMMUv%d with:" message in
arm_smmu_device_cfg_probe() - this driver is noisy enough already
without starting to repeat itself.

> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index abf6496843a6..5c793b3d3173 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -366,6 +366,7 @@ struct arm_smmu_device {
>  	u32				options;
>  	enum arm_smmu_arch_version	version;
>  	enum arm_smmu_implementation	model;
> +	const char			*name;

If we are going to add a pointer to static implementation data, it may
as well be a "const arm_smmu_type *type" pointer to subsume version and
model as well.

>  
>  	u32				num_context_banks;
>  	u32				num_s2_context_banks;
> @@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  	return 0;
>  }
>  
> -struct arm_smmu_match_data {
> +struct arm_smmu_type {
>  	enum arm_smmu_arch_version version;
>  	enum arm_smmu_implementation model;
> +	const char *name;
>  };
>  
> -#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> +#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
> +static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
>  
> -ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
> -ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
> +ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
> +ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
> +ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");

Strictly, I think you mean "ARM® CoreLink™ MMU-401". Printing the name
of a driver-internal structure looks like someone left a debugging hack in.

> +ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");

And similarly. Of course, I'm not actually suggesting that using the
full marketing names of things is a great idea, but then again if we do
go naming specific IPs, and anyone gets sniffy about using the names
"properly", then guess what's going to get removed again? You'll always
find me firmly in the "easier not to go there" camp.

Robin.

> +ARM_SMMU_TYPE(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2, "cavium-smmuv2");
>  
>  static const struct of_device_id arm_smmu_of_match[] = {
>  	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
> @@ -1981,29 +1983,19 @@ static const struct of_device_id arm_smmu_of_match[] = {
>  MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>  
>  #ifdef CONFIG_ACPI
> -static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
> +static struct arm_smmu_type *acpi_smmu_get_type(u32 model)
>  {
> -	int ret = 0;
> -
>  	switch (model) {
>  	case ACPI_IORT_SMMU_V1:
>  	case ACPI_IORT_SMMU_CORELINK_MMU400:
> -		smmu->version = ARM_SMMU_V1;
> -		smmu->model = GENERIC_SMMU;
> -		break;
> +		return &smmu_generic_v1;
>  	case ACPI_IORT_SMMU_V2:
> -		smmu->version = ARM_SMMU_V2;
> -		smmu->model = GENERIC_SMMU;
> -		break;
> +		return &smmu_generic_v2;
>  	case ACPI_IORT_SMMU_CORELINK_MMU500:
> -		smmu->version = ARM_SMMU_V2;
> -		smmu->model = ARM_MMU500;
> -		break;
> -	default:
> -		ret = -ENODEV;
> +		return &arm_mmu500;
>  	}
>  
> -	return ret;
> +	return NULL;
>  }
>  
>  static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
> @@ -2013,14 +2005,18 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  	struct acpi_iort_node *node =
>  		*(struct acpi_iort_node **)dev_get_platdata(dev);
>  	struct acpi_iort_smmu *iort_smmu;
> -	int ret;
> +	struct arm_smmu_type *type;
>  
>  	/* Retrieve SMMU1/2 specific data */
>  	iort_smmu = (struct acpi_iort_smmu *)node->node_data;
>  
> -	ret = acpi_smmu_get_data(iort_smmu->model, smmu);
> -	if (ret < 0)
> -		return ret;
> +	type = acpi_smmu_get_type(iort_smmu->model);
> +	if (!type)
> +		return -ENODEV;
> +
> +	smmu->version	= type->version;
> +	smmu->model	= type->model;
> +	smmu->name	= type->name;
>  
>  	/* Ignore the configuration access interrupt */
>  	smmu->num_global_irqs = 1;
> @@ -2041,8 +2037,8 @@ static inline int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  				    struct arm_smmu_device *smmu)
>  {
> -	const struct arm_smmu_match_data *data;
>  	struct device *dev = &pdev->dev;
> +	const struct arm_smmu_type *type;
>  	bool legacy_binding;
>  
>  	if (of_property_read_u32(dev->of_node, "#global-interrupts",
> @@ -2051,9 +2047,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  		return -ENODEV;
>  	}
>  
> -	data = of_device_get_match_data(dev);
> -	smmu->version = data->version;
> -	smmu->model = data->model;
> +	type = of_device_get_match_data(dev);
> +	smmu->version	= type->version;
> +	smmu->model	= type->model;
> +	smmu->name	= type->name;
>  
>  	parse_driver_options(smmu);
>  
> @@ -2098,6 +2095,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> +	dev_notice(dev, "%s detected", smmu->name);
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	ioaddr = res->start;
>  	smmu->base = devm_ioremap_resource(dev, res);
> 

[PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robin Murphy]

On 06/03/17 13:58, Robert Richter wrote:
> The ARM SMMU detection especially depends from system firmware. For
> better diagnostic, log the detected type in dmesg.

This paragraph especially depends from grammar. I think.

> The smmu type's name is now stored in struct arm_smmu_type and ACPI
> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
> macro to ARM_SMMU_TYPE() for better readability.
> 
> Signed-off-by: Robert Richter <rrichter@cavium.com>
> ---
>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 31 deletions(-)

That seems a relatively invasive diffstat for the sake of printing a
string once at boot time to what I can only assume is a small audience
of firmware developers who find "cat
/sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
equivalent) too hard ;)

Assuming there is a really good reason somewhere to justify this, I
still wonder if a simple self-contained "smmu->model to string" function
wouldn't do, if we really want to do this? Maybe it's not quite that
simple if the generic case needs to key off smmu->version as well, but
still. Arguably, just searching the of_match_table by model/version and
printing the corresponding DT compatible would do the job (given an
MMU-400 model to disambiguate those).

Either way it ought to be replacing the "SMMUv%d with:" message in
arm_smmu_device_cfg_probe() - this driver is noisy enough already
without starting to repeat itself.

> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index abf6496843a6..5c793b3d3173 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -366,6 +366,7 @@ struct arm_smmu_device {
>  	u32				options;
>  	enum arm_smmu_arch_version	version;
>  	enum arm_smmu_implementation	model;
> +	const char			*name;

If we are going to add a pointer to static implementation data, it may
as well be a "const arm_smmu_type *type" pointer to subsume version and
model as well.

>  
>  	u32				num_context_banks;
>  	u32				num_s2_context_banks;
> @@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  	return 0;
>  }
>  
> -struct arm_smmu_match_data {
> +struct arm_smmu_type {
>  	enum arm_smmu_arch_version version;
>  	enum arm_smmu_implementation model;
> +	const char *name;
>  };
>  
> -#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> +#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
> +static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
>  
> -ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
> -ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
> +ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
> +ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
> +ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");

Strictly, I think you mean "ARM? CoreLink? MMU-401". Printing the name
of a driver-internal structure looks like someone left a debugging hack in.

> +ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");

And similarly. Of course, I'm not actually suggesting that using the
full marketing names of things is a great idea, but then again if we do
go naming specific IPs, and anyone gets sniffy about using the names
"properly", then guess what's going to get removed again? You'll always
find me firmly in the "easier not to go there" camp.

Robin.

> +ARM_SMMU_TYPE(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2, "cavium-smmuv2");
>  
>  static const struct of_device_id arm_smmu_of_match[] = {
>  	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
> @@ -1981,29 +1983,19 @@ static const struct of_device_id arm_smmu_of_match[] = {
>  MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>  
>  #ifdef CONFIG_ACPI
> -static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
> +static struct arm_smmu_type *acpi_smmu_get_type(u32 model)
>  {
> -	int ret = 0;
> -
>  	switch (model) {
>  	case ACPI_IORT_SMMU_V1:
>  	case ACPI_IORT_SMMU_CORELINK_MMU400:
> -		smmu->version = ARM_SMMU_V1;
> -		smmu->model = GENERIC_SMMU;
> -		break;
> +		return &smmu_generic_v1;
>  	case ACPI_IORT_SMMU_V2:
> -		smmu->version = ARM_SMMU_V2;
> -		smmu->model = GENERIC_SMMU;
> -		break;
> +		return &smmu_generic_v2;
>  	case ACPI_IORT_SMMU_CORELINK_MMU500:
> -		smmu->version = ARM_SMMU_V2;
> -		smmu->model = ARM_MMU500;
> -		break;
> -	default:
> -		ret = -ENODEV;
> +		return &arm_mmu500;
>  	}
>  
> -	return ret;
> +	return NULL;
>  }
>  
>  static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
> @@ -2013,14 +2005,18 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  	struct acpi_iort_node *node =
>  		*(struct acpi_iort_node **)dev_get_platdata(dev);
>  	struct acpi_iort_smmu *iort_smmu;
> -	int ret;
> +	struct arm_smmu_type *type;
>  
>  	/* Retrieve SMMU1/2 specific data */
>  	iort_smmu = (struct acpi_iort_smmu *)node->node_data;
>  
> -	ret = acpi_smmu_get_data(iort_smmu->model, smmu);
> -	if (ret < 0)
> -		return ret;
> +	type = acpi_smmu_get_type(iort_smmu->model);
> +	if (!type)
> +		return -ENODEV;
> +
> +	smmu->version	= type->version;
> +	smmu->model	= type->model;
> +	smmu->name	= type->name;
>  
>  	/* Ignore the configuration access interrupt */
>  	smmu->num_global_irqs = 1;
> @@ -2041,8 +2037,8 @@ static inline int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  				    struct arm_smmu_device *smmu)
>  {
> -	const struct arm_smmu_match_data *data;
>  	struct device *dev = &pdev->dev;
> +	const struct arm_smmu_type *type;
>  	bool legacy_binding;
>  
>  	if (of_property_read_u32(dev->of_node, "#global-interrupts",
> @@ -2051,9 +2047,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  		return -ENODEV;
>  	}
>  
> -	data = of_device_get_match_data(dev);
> -	smmu->version = data->version;
> -	smmu->model = data->model;
> +	type = of_device_get_match_data(dev);
> +	smmu->version	= type->version;
> +	smmu->model	= type->model;
> +	smmu->name	= type->name;
>  
>  	parse_driver_options(smmu);
>  
> @@ -2098,6 +2095,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> +	dev_notice(dev, "%s detected", smmu->name);
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	ioaddr = res->start;
>  	smmu->base = devm_ioremap_resource(dev, res);
> 

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robert Richter]

On 06.03.17 18:22:08, Robin Murphy wrote:
> On 06/03/17 13:58, Robert Richter wrote:
> > The ARM SMMU detection especially depends from system firmware. For
> > better diagnostic, log the detected type in dmesg.
> 
> This paragraph especially depends from grammar. I think.

Thanks for the mail on you. :)

> 
> > The smmu type's name is now stored in struct arm_smmu_type and ACPI
> > code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
> > macro to ARM_SMMU_TYPE() for better readability.
> > 
> > Signed-off-by: Robert Richter <rrichter@cavium.com>
> > ---
> >  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
> >  1 file changed, 30 insertions(+), 31 deletions(-)
> 
> That seems a relatively invasive diffstat for the sake of printing a
> string once at boot time to what I can only assume is a small audience
> of firmware developers who find "cat
> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
> equivalent) too hard ;)

Reading firmware data is not really a solution as you don't know what
the driver is doing with it. The actual background of this patch is to
be sure a certain workaround was enabled in the kernel. ARM's cpu
errata framework does this nicely. In case of smmus we just have the
internal model implementation type which is not visible in the logs.
Right now, there is no way to figure that out without knowing fw
specifics and kernel sources.

The change is big but most of it is a reasonable rework anyway. I
didn't want to split that into a series of patches. But I could do
that.

> Assuming there is a really good reason somewhere to justify this, I
> still wonder if a simple self-contained "smmu->model to string" function
> wouldn't do, if we really want to do this? Maybe it's not quite that
> simple if the generic case needs to key off smmu->version as well, but
> still. Arguably, just searching the of_match_table by model/version and
> printing the corresponding DT compatible would do the job (given an
> MMU-400 model to disambiguate those).

Whatever you prefer. To me a dynamic search makes things more complex
for no benefit. And providing DT names in an ACPI context is confusing
either.

> Either way it ought to be replacing the "SMMUv%d with:" message in
> arm_smmu_device_cfg_probe() - this driver is noisy enough already
> without starting to repeat itself.
> 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index abf6496843a6..5c793b3d3173 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -366,6 +366,7 @@ struct arm_smmu_device {
> >  	u32				options;
> >  	enum arm_smmu_arch_version	version;
> >  	enum arm_smmu_implementation	model;
> > +	const char			*name;
> 
> If we are going to add a pointer to static implementation data, it may
> as well be a "const arm_smmu_type *type" pointer to subsume version and
> model as well.

The name still may change but not the particular string. Both work for
me.

> 
> >  
> >  	u32				num_context_banks;
> >  	u32				num_s2_context_banks;
> > @@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
> >  	return 0;
> >  }
> >  
> > -struct arm_smmu_match_data {
> > +struct arm_smmu_type {
> >  	enum arm_smmu_arch_version version;
> >  	enum arm_smmu_implementation model;
> > +	const char *name;
> >  };
> >  
> > -#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> > -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> > +#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
> > +static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
> >  
> > -ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> > -ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> > -ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
> > -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
> > -ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
> > +ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
> > +ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
> > +ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
> 
> Strictly, I think you mean "ARM® CoreLink™ MMU-401". Printing the name
> of a driver-internal structure looks like someone left a debugging hack in.
> 
> > +ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
> 
> And similarly. Of course, I'm not actually suggesting that using the
> full marketing names of things is a great idea, but then again if we do
> go naming specific IPs, and anyone gets sniffy about using the names
> "properly", then guess what's going to get removed again? You'll always
> find me firmly in the "easier not to go there" camp.

A change of naming is unrelated to this patch. I leave this up to you.

Thank you for review.

-Robert

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robert Richter]

On 06.03.17 18:22:08, Robin Murphy wrote:
> On 06/03/17 13:58, Robert Richter wrote:
> > The ARM SMMU detection especially depends from system firmware. For
> > better diagnostic, log the detected type in dmesg.
> 
> This paragraph especially depends from grammar. I think.

Thanks for the mail on you. :)

> 
> > The smmu type's name is now stored in struct arm_smmu_type and ACPI
> > code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
> > macro to ARM_SMMU_TYPE() for better readability.
> > 
> > Signed-off-by: Robert Richter <rrichter@cavium.com>
> > ---
> >  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
> >  1 file changed, 30 insertions(+), 31 deletions(-)
> 
> That seems a relatively invasive diffstat for the sake of printing a
> string once at boot time to what I can only assume is a small audience
> of firmware developers who find "cat
> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
> equivalent) too hard ;)

Reading firmware data is not really a solution as you don't know what
the driver is doing with it. The actual background of this patch is to
be sure a certain workaround was enabled in the kernel. ARM's cpu
errata framework does this nicely. In case of smmus we just have the
internal model implementation type which is not visible in the logs.
Right now, there is no way to figure that out without knowing fw
specifics and kernel sources.

The change is big but most of it is a reasonable rework anyway. I
didn't want to split that into a series of patches. But I could do
that.

> Assuming there is a really good reason somewhere to justify this, I
> still wonder if a simple self-contained "smmu->model to string" function
> wouldn't do, if we really want to do this? Maybe it's not quite that
> simple if the generic case needs to key off smmu->version as well, but
> still. Arguably, just searching the of_match_table by model/version and
> printing the corresponding DT compatible would do the job (given an
> MMU-400 model to disambiguate those).

Whatever you prefer. To me a dynamic search makes things more complex
for no benefit. And providing DT names in an ACPI context is confusing
either.

> Either way it ought to be replacing the "SMMUv%d with:" message in
> arm_smmu_device_cfg_probe() - this driver is noisy enough already
> without starting to repeat itself.
> 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index abf6496843a6..5c793b3d3173 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -366,6 +366,7 @@ struct arm_smmu_device {
> >  	u32				options;
> >  	enum arm_smmu_arch_version	version;
> >  	enum arm_smmu_implementation	model;
> > +	const char			*name;
> 
> If we are going to add a pointer to static implementation data, it may
> as well be a "const arm_smmu_type *type" pointer to subsume version and
> model as well.

The name still may change but not the particular string. Both work for
me.

> 
> >  
> >  	u32				num_context_banks;
> >  	u32				num_s2_context_banks;
> > @@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
> >  	return 0;
> >  }
> >  
> > -struct arm_smmu_match_data {
> > +struct arm_smmu_type {
> >  	enum arm_smmu_arch_version version;
> >  	enum arm_smmu_implementation model;
> > +	const char *name;
> >  };
> >  
> > -#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> > -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> > +#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
> > +static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
> >  
> > -ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> > -ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> > -ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
> > -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
> > -ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
> > +ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
> > +ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
> > +ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
> 
> Strictly, I think you mean "ARM® CoreLink™ MMU-401". Printing the name
> of a driver-internal structure looks like someone left a debugging hack in.
> 
> > +ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
> 
> And similarly. Of course, I'm not actually suggesting that using the
> full marketing names of things is a great idea, but then again if we do
> go naming specific IPs, and anyone gets sniffy about using the names
> "properly", then guess what's going to get removed again? You'll always
> find me firmly in the "easier not to go there" camp.

A change of naming is unrelated to this patch. I leave this up to you.

Thank you for review.

-Robert

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robert Richter]

On 06.03.17 18:22:08, Robin Murphy wrote:
> On 06/03/17 13:58, Robert Richter wrote:
> > The ARM SMMU detection especially depends from system firmware. For
> > better diagnostic, log the detected type in dmesg.
> 
> This paragraph especially depends from grammar. I think.

Thanks for the mail on you. :)

> 
> > The smmu type's name is now stored in struct arm_smmu_type and ACPI
> > code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
> > macro to ARM_SMMU_TYPE() for better readability.
> > 
> > Signed-off-by: Robert Richter <rrichter@cavium.com>
> > ---
> >  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
> >  1 file changed, 30 insertions(+), 31 deletions(-)
> 
> That seems a relatively invasive diffstat for the sake of printing a
> string once at boot time to what I can only assume is a small audience
> of firmware developers who find "cat
> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
> equivalent) too hard ;)

Reading firmware data is not really a solution as you don't know what
the driver is doing with it. The actual background of this patch is to
be sure a certain workaround was enabled in the kernel. ARM's cpu
errata framework does this nicely. In case of smmus we just have the
internal model implementation type which is not visible in the logs.
Right now, there is no way to figure that out without knowing fw
specifics and kernel sources.

The change is big but most of it is a reasonable rework anyway. I
didn't want to split that into a series of patches. But I could do
that.

> Assuming there is a really good reason somewhere to justify this, I
> still wonder if a simple self-contained "smmu->model to string" function
> wouldn't do, if we really want to do this? Maybe it's not quite that
> simple if the generic case needs to key off smmu->version as well, but
> still. Arguably, just searching the of_match_table by model/version and
> printing the corresponding DT compatible would do the job (given an
> MMU-400 model to disambiguate those).

Whatever you prefer. To me a dynamic search makes things more complex
for no benefit. And providing DT names in an ACPI context is confusing
either.

> Either way it ought to be replacing the "SMMUv%d with:" message in
> arm_smmu_device_cfg_probe() - this driver is noisy enough already
> without starting to repeat itself.
> 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index abf6496843a6..5c793b3d3173 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -366,6 +366,7 @@ struct arm_smmu_device {
> >  	u32				options;
> >  	enum arm_smmu_arch_version	version;
> >  	enum arm_smmu_implementation	model;
> > +	const char			*name;
> 
> If we are going to add a pointer to static implementation data, it may
> as well be a "const arm_smmu_type *type" pointer to subsume version and
> model as well.

The name still may change but not the particular string. Both work for
me.

> 
> >  
> >  	u32				num_context_banks;
> >  	u32				num_s2_context_banks;
> > @@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
> >  	return 0;
> >  }
> >  
> > -struct arm_smmu_match_data {
> > +struct arm_smmu_type {
> >  	enum arm_smmu_arch_version version;
> >  	enum arm_smmu_implementation model;
> > +	const char *name;
> >  };
> >  
> > -#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> > -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> > +#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
> > +static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
> >  
> > -ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> > -ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> > -ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
> > -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
> > -ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
> > +ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
> > +ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
> > +ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
> 
> Strictly, I think you mean "ARM? CoreLink? MMU-401". Printing the name
> of a driver-internal structure looks like someone left a debugging hack in.
> 
> > +ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
> 
> And similarly. Of course, I'm not actually suggesting that using the
> full marketing names of things is a great idea, but then again if we do
> go naming specific IPs, and anyone gets sniffy about using the names
> "properly", then guess what's going to get removed again? You'll always
> find me firmly in the "easier not to go there" camp.

A change of naming is unrelated to this patch. I leave this up to you.

Thank you for review.

-Robert

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robin Murphy]

On 07/03/17 14:06, Robert Richter wrote:
> On 06.03.17 18:22:08, Robin Murphy wrote:
>> On 06/03/17 13:58, Robert Richter wrote:
>>> The ARM SMMU detection especially depends from system firmware. For
>>> better diagnostic, log the detected type in dmesg.
>>
>> This paragraph especially depends from grammar. I think.
> 
> Thanks for the mail on you. :)
> 
>>
>>> The smmu type's name is now stored in struct arm_smmu_type and ACPI
>>> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
>>> macro to ARM_SMMU_TYPE() for better readability.
>>>
>>> Signed-off-by: Robert Richter <rrichter@cavium.com>
>>> ---
>>>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
>>>  1 file changed, 30 insertions(+), 31 deletions(-)
>>
>> That seems a relatively invasive diffstat for the sake of printing a
>> string once at boot time to what I can only assume is a small audience
>> of firmware developers who find "cat
>> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
>> equivalent) too hard ;)
> 
> Reading firmware data is not really a solution as you don't know what
> the driver is doing with it. The actual background of this patch is to
> be sure a certain workaround was enabled in the kernel. ARM's cpu
> errata framework does this nicely. In case of smmus we just have the
> internal model implementation type which is not visible in the logs.
> Right now, there is no way to figure that out without knowing fw
> specifics and kernel sources.

Ah, now it starts to become clear. In that case, if we want to confirm
the presence of specific workarounds, we should actually _confirm the
presence of specific workarounds_. I'd have no complaint with e.g. this:

-----8<-----
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index f7411109670f..9e50a092632c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1934,6 +1934,8 @@ static int arm_smmu_device_cfg_probe(struct
arm_smmu_device *smmu)
                        atomic_add_return(smmu->num_context_banks,
                                          &cavium_smmu_context_count);
                smmu->cavium_id_base -= smmu->num_context_banks;
+               dev_notice(smmu->dev, "\tusing ASID/VMID offset %u\n",
+                          smmu->cavium_id_base);
        }

        /* ID2 */
----->8-----

and the equivalent for other things, if need be. If you just print "hey,
this is SMMU-x", the user is in fact no better off, since they would
then still have to go and look at the source for whatever kernel they're
running to find out which particular workarounds for SMMU-x bugs that
particular kernel implements.

Robin.

> The change is big but most of it is a reasonable rework anyway. I
> didn't want to split that into a series of patches. But I could do
> that.
> 
>> Assuming there is a really good reason somewhere to justify this, I
>> still wonder if a simple self-contained "smmu->model to string" function
>> wouldn't do, if we really want to do this? Maybe it's not quite that
>> simple if the generic case needs to key off smmu->version as well, but
>> still. Arguably, just searching the of_match_table by model/version and
>> printing the corresponding DT compatible would do the job (given an
>> MMU-400 model to disambiguate those).
> 
> Whatever you prefer. To me a dynamic search makes things more complex
> for no benefit. And providing DT names in an ACPI context is confusing
> either.
> 
>> Either way it ought to be replacing the "SMMUv%d with:" message in
>> arm_smmu_device_cfg_probe() - this driver is noisy enough already
>> without starting to repeat itself.
>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index abf6496843a6..5c793b3d3173 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -366,6 +366,7 @@ struct arm_smmu_device {
>>>  	u32				options;
>>>  	enum arm_smmu_arch_version	version;
>>>  	enum arm_smmu_implementation	model;
>>> +	const char			*name;
>>
>> If we are going to add a pointer to static implementation data, it may
>> as well be a "const arm_smmu_type *type" pointer to subsume version and
>> model as well.
> 
> The name still may change but not the particular string. Both work for
> me.
> 
>>
>>>  
>>>  	u32				num_context_banks;
>>>  	u32				num_s2_context_banks;
>>> @@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>>>  	return 0;
>>>  }
>>>  
>>> -struct arm_smmu_match_data {
>>> +struct arm_smmu_type {
>>>  	enum arm_smmu_arch_version version;
>>>  	enum arm_smmu_implementation model;
>>> +	const char *name;
>>>  };
>>>  
>>> -#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
>>> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
>>> +#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
>>> +static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
>>>  
>>> -ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>>> -ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
>>> -ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
>>> -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
>>> -ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
>>> +ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
>>> +ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
>>> +ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
>>
>> Strictly, I think you mean "ARM® CoreLink™ MMU-401". Printing the name
>> of a driver-internal structure looks like someone left a debugging hack in.
>>
>>> +ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
>>
>> And similarly. Of course, I'm not actually suggesting that using the
>> full marketing names of things is a great idea, but then again if we do
>> go naming specific IPs, and anyone gets sniffy about using the names
>> "properly", then guess what's going to get removed again? You'll always
>> find me firmly in the "easier not to go there" camp.
> 
> A change of naming is unrelated to this patch. I leave this up to you.
> 
> Thank you for review.
> 
> -Robert
> 

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robin Murphy]

On 07/03/17 14:06, Robert Richter wrote:
> On 06.03.17 18:22:08, Robin Murphy wrote:
>> On 06/03/17 13:58, Robert Richter wrote:
>>> The ARM SMMU detection especially depends from system firmware. For
>>> better diagnostic, log the detected type in dmesg.
>>
>> This paragraph especially depends from grammar. I think.
> 
> Thanks for the mail on you. :)
> 
>>
>>> The smmu type's name is now stored in struct arm_smmu_type and ACPI
>>> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
>>> macro to ARM_SMMU_TYPE() for better readability.
>>>
>>> Signed-off-by: Robert Richter <rrichter@cavium.com>
>>> ---
>>>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
>>>  1 file changed, 30 insertions(+), 31 deletions(-)
>>
>> That seems a relatively invasive diffstat for the sake of printing a
>> string once at boot time to what I can only assume is a small audience
>> of firmware developers who find "cat
>> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
>> equivalent) too hard ;)
> 
> Reading firmware data is not really a solution as you don't know what
> the driver is doing with it. The actual background of this patch is to
> be sure a certain workaround was enabled in the kernel. ARM's cpu
> errata framework does this nicely. In case of smmus we just have the
> internal model implementation type which is not visible in the logs.
> Right now, there is no way to figure that out without knowing fw
> specifics and kernel sources.

Ah, now it starts to become clear. In that case, if we want to confirm
the presence of specific workarounds, we should actually _confirm the
presence of specific workarounds_. I'd have no complaint with e.g. this:

-----8<-----
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index f7411109670f..9e50a092632c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1934,6 +1934,8 @@ static int arm_smmu_device_cfg_probe(struct
arm_smmu_device *smmu)
                        atomic_add_return(smmu->num_context_banks,
                                          &cavium_smmu_context_count);
                smmu->cavium_id_base -= smmu->num_context_banks;
+               dev_notice(smmu->dev, "\tusing ASID/VMID offset %u\n",
+                          smmu->cavium_id_base);
        }

        /* ID2 */
----->8-----

and the equivalent for other things, if need be. If you just print "hey,
this is SMMU-x", the user is in fact no better off, since they would
then still have to go and look at the source for whatever kernel they're
running to find out which particular workarounds for SMMU-x bugs that
particular kernel implements.

Robin.

> The change is big but most of it is a reasonable rework anyway. I
> didn't want to split that into a series of patches. But I could do
> that.
> 
>> Assuming there is a really good reason somewhere to justify this, I
>> still wonder if a simple self-contained "smmu->model to string" function
>> wouldn't do, if we really want to do this? Maybe it's not quite that
>> simple if the generic case needs to key off smmu->version as well, but
>> still. Arguably, just searching the of_match_table by model/version and
>> printing the corresponding DT compatible would do the job (given an
>> MMU-400 model to disambiguate those).
> 
> Whatever you prefer. To me a dynamic search makes things more complex
> for no benefit. And providing DT names in an ACPI context is confusing
> either.
> 
>> Either way it ought to be replacing the "SMMUv%d with:" message in
>> arm_smmu_device_cfg_probe() - this driver is noisy enough already
>> without starting to repeat itself.
>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index abf6496843a6..5c793b3d3173 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -366,6 +366,7 @@ struct arm_smmu_device {
>>>  	u32				options;
>>>  	enum arm_smmu_arch_version	version;
>>>  	enum arm_smmu_implementation	model;
>>> +	const char			*name;
>>
>> If we are going to add a pointer to static implementation data, it may
>> as well be a "const arm_smmu_type *type" pointer to subsume version and
>> model as well.
> 
> The name still may change but not the particular string. Both work for
> me.
> 
>>
>>>  
>>>  	u32				num_context_banks;
>>>  	u32				num_s2_context_banks;
>>> @@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>>>  	return 0;
>>>  }
>>>  
>>> -struct arm_smmu_match_data {
>>> +struct arm_smmu_type {
>>>  	enum arm_smmu_arch_version version;
>>>  	enum arm_smmu_implementation model;
>>> +	const char *name;
>>>  };
>>>  
>>> -#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
>>> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
>>> +#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
>>> +static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
>>>  
>>> -ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>>> -ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
>>> -ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
>>> -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
>>> -ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
>>> +ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
>>> +ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
>>> +ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
>>
>> Strictly, I think you mean "ARM® CoreLink™ MMU-401". Printing the name
>> of a driver-internal structure looks like someone left a debugging hack in.
>>
>>> +ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
>>
>> And similarly. Of course, I'm not actually suggesting that using the
>> full marketing names of things is a great idea, but then again if we do
>> go naming specific IPs, and anyone gets sniffy about using the names
>> "properly", then guess what's going to get removed again? You'll always
>> find me firmly in the "easier not to go there" camp.
> 
> A change of naming is unrelated to this patch. I leave this up to you.
> 
> Thank you for review.
> 
> -Robert
> 

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robin Murphy]

On 07/03/17 14:06, Robert Richter wrote:
> On 06.03.17 18:22:08, Robin Murphy wrote:
>> On 06/03/17 13:58, Robert Richter wrote:
>>> The ARM SMMU detection especially depends from system firmware. For
>>> better diagnostic, log the detected type in dmesg.
>>
>> This paragraph especially depends from grammar. I think.
> 
> Thanks for the mail on you. :)
> 
>>
>>> The smmu type's name is now stored in struct arm_smmu_type and ACPI
>>> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
>>> macro to ARM_SMMU_TYPE() for better readability.
>>>
>>> Signed-off-by: Robert Richter <rrichter@cavium.com>
>>> ---
>>>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
>>>  1 file changed, 30 insertions(+), 31 deletions(-)
>>
>> That seems a relatively invasive diffstat for the sake of printing a
>> string once at boot time to what I can only assume is a small audience
>> of firmware developers who find "cat
>> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
>> equivalent) too hard ;)
> 
> Reading firmware data is not really a solution as you don't know what
> the driver is doing with it. The actual background of this patch is to
> be sure a certain workaround was enabled in the kernel. ARM's cpu
> errata framework does this nicely. In case of smmus we just have the
> internal model implementation type which is not visible in the logs.
> Right now, there is no way to figure that out without knowing fw
> specifics and kernel sources.

Ah, now it starts to become clear. In that case, if we want to confirm
the presence of specific workarounds, we should actually _confirm the
presence of specific workarounds_. I'd have no complaint with e.g. this:

-----8<-----
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index f7411109670f..9e50a092632c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1934,6 +1934,8 @@ static int arm_smmu_device_cfg_probe(struct
arm_smmu_device *smmu)
                        atomic_add_return(smmu->num_context_banks,
                                          &cavium_smmu_context_count);
                smmu->cavium_id_base -= smmu->num_context_banks;
+               dev_notice(smmu->dev, "\tusing ASID/VMID offset %u\n",
+                          smmu->cavium_id_base);
        }

        /* ID2 */
----->8-----

and the equivalent for other things, if need be. If you just print "hey,
this is SMMU-x", the user is in fact no better off, since they would
then still have to go and look at the source for whatever kernel they're
running to find out which particular workarounds for SMMU-x bugs that
particular kernel implements.

Robin.

> The change is big but most of it is a reasonable rework anyway. I
> didn't want to split that into a series of patches. But I could do
> that.
> 
>> Assuming there is a really good reason somewhere to justify this, I
>> still wonder if a simple self-contained "smmu->model to string" function
>> wouldn't do, if we really want to do this? Maybe it's not quite that
>> simple if the generic case needs to key off smmu->version as well, but
>> still. Arguably, just searching the of_match_table by model/version and
>> printing the corresponding DT compatible would do the job (given an
>> MMU-400 model to disambiguate those).
> 
> Whatever you prefer. To me a dynamic search makes things more complex
> for no benefit. And providing DT names in an ACPI context is confusing
> either.
> 
>> Either way it ought to be replacing the "SMMUv%d with:" message in
>> arm_smmu_device_cfg_probe() - this driver is noisy enough already
>> without starting to repeat itself.
>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index abf6496843a6..5c793b3d3173 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -366,6 +366,7 @@ struct arm_smmu_device {
>>>  	u32				options;
>>>  	enum arm_smmu_arch_version	version;
>>>  	enum arm_smmu_implementation	model;
>>> +	const char			*name;
>>
>> If we are going to add a pointer to static implementation data, it may
>> as well be a "const arm_smmu_type *type" pointer to subsume version and
>> model as well.
> 
> The name still may change but not the particular string. Both work for
> me.
> 
>>
>>>  
>>>  	u32				num_context_banks;
>>>  	u32				num_s2_context_banks;
>>> @@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>>>  	return 0;
>>>  }
>>>  
>>> -struct arm_smmu_match_data {
>>> +struct arm_smmu_type {
>>>  	enum arm_smmu_arch_version version;
>>>  	enum arm_smmu_implementation model;
>>> +	const char *name;
>>>  };
>>>  
>>> -#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
>>> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
>>> +#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
>>> +static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
>>>  
>>> -ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>>> -ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
>>> -ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
>>> -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
>>> -ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
>>> +ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
>>> +ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
>>> +ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
>>
>> Strictly, I think you mean "ARM? CoreLink? MMU-401". Printing the name
>> of a driver-internal structure looks like someone left a debugging hack in.
>>
>>> +ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
>>
>> And similarly. Of course, I'm not actually suggesting that using the
>> full marketing names of things is a great idea, but then again if we do
>> go naming specific IPs, and anyone gets sniffy about using the names
>> "properly", then guess what's going to get removed again? You'll always
>> find me firmly in the "easier not to go there" camp.
> 
> A change of naming is unrelated to this patch. I leave this up to you.
> 
> Thank you for review.
> 
> -Robert
> 

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Aleksey Makarov]

On 03/06/2017 02:58 PM, Robert Richter wrote:
> The ARM SMMU detection especially depends from system firmware. For
> better diagnostic, log the detected type in dmesg.
>
> The smmu type's name is now stored in struct arm_smmu_type and ACPI
> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
> macro to ARM_SMMU_TYPE() for better readability.
>
> Signed-off-by: Robert Richter <rrichter@cavium.com>
> ---
>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index abf6496843a6..5c793b3d3173 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -366,6 +366,7 @@ struct arm_smmu_device {
>  	u32				options;
>  	enum arm_smmu_arch_version	version;
>  	enum arm_smmu_implementation	model;
> +	const char			*name;
>
>  	u32				num_context_banks;
>  	u32				num_s2_context_banks;
> @@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  	return 0;
>  }
>
> -struct arm_smmu_match_data {
> +struct arm_smmu_type {
>  	enum arm_smmu_arch_version version;
>  	enum arm_smmu_implementation model;
> +	const char *name;
>  };
>
> -#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> +#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
> +static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }

Can we infer the _name from the name of var?

#define ARM_SMMU_TYPE(var, ver, imp)	\
static struct arm_smmu_type var = { .version = ver, .model = imp, .name = #var }

> -ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
> -ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
> +ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
> +ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
> +ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
> +ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
> +ARM_SMMU_TYPE(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2, "cavium-smmuv2");

So that this change will not be required?

Thank you
Aleksey Makarov

>  static const struct of_device_id arm_smmu_of_match[] = {
>  	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
> @@ -1981,29 +1983,19 @@ static const struct of_device_id arm_smmu_of_match[] = {
>  MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>
>  #ifdef CONFIG_ACPI
> -static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
> +static struct arm_smmu_type *acpi_smmu_get_type(u32 model)
>  {
> -	int ret = 0;
> -
>  	switch (model) {
>  	case ACPI_IORT_SMMU_V1:
>  	case ACPI_IORT_SMMU_CORELINK_MMU400:
> -		smmu->version = ARM_SMMU_V1;
> -		smmu->model = GENERIC_SMMU;
> -		break;
> +		return &smmu_generic_v1;
>  	case ACPI_IORT_SMMU_V2:
> -		smmu->version = ARM_SMMU_V2;
> -		smmu->model = GENERIC_SMMU;
> -		break;
> +		return &smmu_generic_v2;
>  	case ACPI_IORT_SMMU_CORELINK_MMU500:
> -		smmu->version = ARM_SMMU_V2;
> -		smmu->model = ARM_MMU500;
> -		break;
> -	default:
> -		ret = -ENODEV;
> +		return &arm_mmu500;
>  	}
>
> -	return ret;
> +	return NULL;
>  }
>
>  static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
> @@ -2013,14 +2005,18 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  	struct acpi_iort_node *node =
>  		*(struct acpi_iort_node **)dev_get_platdata(dev);
>  	struct acpi_iort_smmu *iort_smmu;
> -	int ret;
> +	struct arm_smmu_type *type;
>
>  	/* Retrieve SMMU1/2 specific data */
>  	iort_smmu = (struct acpi_iort_smmu *)node->node_data;
>
> -	ret = acpi_smmu_get_data(iort_smmu->model, smmu);
> -	if (ret < 0)
> -		return ret;
> +	type = acpi_smmu_get_type(iort_smmu->model);
> +	if (!type)
> +		return -ENODEV;
> +
> +	smmu->version	= type->version;
> +	smmu->model	= type->model;
> +	smmu->name	= type->name;
>
>  	/* Ignore the configuration access interrupt */
>  	smmu->num_global_irqs = 1;
> @@ -2041,8 +2037,8 @@ static inline int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  				    struct arm_smmu_device *smmu)
>  {
> -	const struct arm_smmu_match_data *data;
>  	struct device *dev = &pdev->dev;
> +	const struct arm_smmu_type *type;
>  	bool legacy_binding;
>
>  	if (of_property_read_u32(dev->of_node, "#global-interrupts",
> @@ -2051,9 +2047,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  		return -ENODEV;
>  	}
>
> -	data = of_device_get_match_data(dev);
> -	smmu->version = data->version;
> -	smmu->model = data->model;
> +	type = of_device_get_match_data(dev);
> +	smmu->version	= type->version;
> +	smmu->model	= type->model;
> +	smmu->name	= type->name;
>
>  	parse_driver_options(smmu);
>
> @@ -2098,6 +2095,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>
> +	dev_notice(dev, "%s detected", smmu->name);
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	ioaddr = res->start;
>  	smmu->base = devm_ioremap_resource(dev, res);
>

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Aleksey Makarov]

On 03/06/2017 02:58 PM, Robert Richter wrote:
> The ARM SMMU detection especially depends from system firmware. For
> better diagnostic, log the detected type in dmesg.
>
> The smmu type's name is now stored in struct arm_smmu_type and ACPI
> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
> macro to ARM_SMMU_TYPE() for better readability.
>
> Signed-off-by: Robert Richter <rrichter-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index abf6496843a6..5c793b3d3173 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -366,6 +366,7 @@ struct arm_smmu_device {
>  	u32				options;
>  	enum arm_smmu_arch_version	version;
>  	enum arm_smmu_implementation	model;
> +	const char			*name;
>
>  	u32				num_context_banks;
>  	u32				num_s2_context_banks;
> @@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  	return 0;
>  }
>
> -struct arm_smmu_match_data {
> +struct arm_smmu_type {
>  	enum arm_smmu_arch_version version;
>  	enum arm_smmu_implementation model;
> +	const char *name;
>  };
>
> -#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> +#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
> +static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }

Can we infer the _name from the name of var?

#define ARM_SMMU_TYPE(var, ver, imp)	\
static struct arm_smmu_type var = { .version = ver, .model = imp, .name = #var }

> -ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
> -ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
> +ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
> +ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
> +ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
> +ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
> +ARM_SMMU_TYPE(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2, "cavium-smmuv2");

So that this change will not be required?

Thank you
Aleksey Makarov

>  static const struct of_device_id arm_smmu_of_match[] = {
>  	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
> @@ -1981,29 +1983,19 @@ static const struct of_device_id arm_smmu_of_match[] = {
>  MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>
>  #ifdef CONFIG_ACPI
> -static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
> +static struct arm_smmu_type *acpi_smmu_get_type(u32 model)
>  {
> -	int ret = 0;
> -
>  	switch (model) {
>  	case ACPI_IORT_SMMU_V1:
>  	case ACPI_IORT_SMMU_CORELINK_MMU400:
> -		smmu->version = ARM_SMMU_V1;
> -		smmu->model = GENERIC_SMMU;
> -		break;
> +		return &smmu_generic_v1;
>  	case ACPI_IORT_SMMU_V2:
> -		smmu->version = ARM_SMMU_V2;
> -		smmu->model = GENERIC_SMMU;
> -		break;
> +		return &smmu_generic_v2;
>  	case ACPI_IORT_SMMU_CORELINK_MMU500:
> -		smmu->version = ARM_SMMU_V2;
> -		smmu->model = ARM_MMU500;
> -		break;
> -	default:
> -		ret = -ENODEV;
> +		return &arm_mmu500;
>  	}
>
> -	return ret;
> +	return NULL;
>  }
>
>  static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
> @@ -2013,14 +2005,18 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  	struct acpi_iort_node *node =
>  		*(struct acpi_iort_node **)dev_get_platdata(dev);
>  	struct acpi_iort_smmu *iort_smmu;
> -	int ret;
> +	struct arm_smmu_type *type;
>
>  	/* Retrieve SMMU1/2 specific data */
>  	iort_smmu = (struct acpi_iort_smmu *)node->node_data;
>
> -	ret = acpi_smmu_get_data(iort_smmu->model, smmu);
> -	if (ret < 0)
> -		return ret;
> +	type = acpi_smmu_get_type(iort_smmu->model);
> +	if (!type)
> +		return -ENODEV;
> +
> +	smmu->version	= type->version;
> +	smmu->model	= type->model;
> +	smmu->name	= type->name;
>
>  	/* Ignore the configuration access interrupt */
>  	smmu->num_global_irqs = 1;
> @@ -2041,8 +2037,8 @@ static inline int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  				    struct arm_smmu_device *smmu)
>  {
> -	const struct arm_smmu_match_data *data;
>  	struct device *dev = &pdev->dev;
> +	const struct arm_smmu_type *type;
>  	bool legacy_binding;
>
>  	if (of_property_read_u32(dev->of_node, "#global-interrupts",
> @@ -2051,9 +2047,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  		return -ENODEV;
>  	}
>
> -	data = of_device_get_match_data(dev);
> -	smmu->version = data->version;
> -	smmu->model = data->model;
> +	type = of_device_get_match_data(dev);
> +	smmu->version	= type->version;
> +	smmu->model	= type->model;
> +	smmu->name	= type->name;
>
>  	parse_driver_options(smmu);
>
> @@ -2098,6 +2095,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>
> +	dev_notice(dev, "%s detected", smmu->name);
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	ioaddr = res->start;
>  	smmu->base = devm_ioremap_resource(dev, res);
>

[PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Aleksey Makarov]

On 03/06/2017 02:58 PM, Robert Richter wrote:
> The ARM SMMU detection especially depends from system firmware. For
> better diagnostic, log the detected type in dmesg.
>
> The smmu type's name is now stored in struct arm_smmu_type and ACPI
> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
> macro to ARM_SMMU_TYPE() for better readability.
>
> Signed-off-by: Robert Richter <rrichter@cavium.com>
> ---
>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index abf6496843a6..5c793b3d3173 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -366,6 +366,7 @@ struct arm_smmu_device {
>  	u32				options;
>  	enum arm_smmu_arch_version	version;
>  	enum arm_smmu_implementation	model;
> +	const char			*name;
>
>  	u32				num_context_banks;
>  	u32				num_s2_context_banks;
> @@ -1955,19 +1956,20 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  	return 0;
>  }
>
> -struct arm_smmu_match_data {
> +struct arm_smmu_type {
>  	enum arm_smmu_arch_version version;
>  	enum arm_smmu_implementation model;
> +	const char *name;
>  };
>
> -#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> +#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
> +static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }

Can we infer the _name from the name of var?

#define ARM_SMMU_TYPE(var, ver, imp)	\
static struct arm_smmu_type var = { .version = ver, .model = imp, .name = #var }

> -ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
> -ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
> +ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
> +ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
> +ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
> +ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
> +ARM_SMMU_TYPE(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2, "cavium-smmuv2");

So that this change will not be required?

Thank you
Aleksey Makarov

>  static const struct of_device_id arm_smmu_of_match[] = {
>  	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
> @@ -1981,29 +1983,19 @@ static const struct of_device_id arm_smmu_of_match[] = {
>  MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>
>  #ifdef CONFIG_ACPI
> -static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
> +static struct arm_smmu_type *acpi_smmu_get_type(u32 model)
>  {
> -	int ret = 0;
> -
>  	switch (model) {
>  	case ACPI_IORT_SMMU_V1:
>  	case ACPI_IORT_SMMU_CORELINK_MMU400:
> -		smmu->version = ARM_SMMU_V1;
> -		smmu->model = GENERIC_SMMU;
> -		break;
> +		return &smmu_generic_v1;
>  	case ACPI_IORT_SMMU_V2:
> -		smmu->version = ARM_SMMU_V2;
> -		smmu->model = GENERIC_SMMU;
> -		break;
> +		return &smmu_generic_v2;
>  	case ACPI_IORT_SMMU_CORELINK_MMU500:
> -		smmu->version = ARM_SMMU_V2;
> -		smmu->model = ARM_MMU500;
> -		break;
> -	default:
> -		ret = -ENODEV;
> +		return &arm_mmu500;
>  	}
>
> -	return ret;
> +	return NULL;
>  }
>
>  static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
> @@ -2013,14 +2005,18 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  	struct acpi_iort_node *node =
>  		*(struct acpi_iort_node **)dev_get_platdata(dev);
>  	struct acpi_iort_smmu *iort_smmu;
> -	int ret;
> +	struct arm_smmu_type *type;
>
>  	/* Retrieve SMMU1/2 specific data */
>  	iort_smmu = (struct acpi_iort_smmu *)node->node_data;
>
> -	ret = acpi_smmu_get_data(iort_smmu->model, smmu);
> -	if (ret < 0)
> -		return ret;
> +	type = acpi_smmu_get_type(iort_smmu->model);
> +	if (!type)
> +		return -ENODEV;
> +
> +	smmu->version	= type->version;
> +	smmu->model	= type->model;
> +	smmu->name	= type->name;
>
>  	/* Ignore the configuration access interrupt */
>  	smmu->num_global_irqs = 1;
> @@ -2041,8 +2037,8 @@ static inline int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  				    struct arm_smmu_device *smmu)
>  {
> -	const struct arm_smmu_match_data *data;
>  	struct device *dev = &pdev->dev;
> +	const struct arm_smmu_type *type;
>  	bool legacy_binding;
>
>  	if (of_property_read_u32(dev->of_node, "#global-interrupts",
> @@ -2051,9 +2047,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  		return -ENODEV;
>  	}
>
> -	data = of_device_get_match_data(dev);
> -	smmu->version = data->version;
> -	smmu->model = data->model;
> +	type = of_device_get_match_data(dev);
> +	smmu->version	= type->version;
> +	smmu->model	= type->model;
> +	smmu->name	= type->name;
>
>  	parse_driver_options(smmu);
>
> @@ -2098,6 +2095,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>
> +	dev_notice(dev, "%s detected", smmu->name);
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	ioaddr = res->start;
>  	smmu->base = devm_ioremap_resource(dev, res);
>

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robert Richter]

On 07.03.17 18:41:33, Robin Murphy wrote:
> On 07/03/17 14:06, Robert Richter wrote:
> > On 06.03.17 18:22:08, Robin Murphy wrote:
> >> On 06/03/17 13:58, Robert Richter wrote:
> >>> The ARM SMMU detection especially depends from system firmware. For
> >>> better diagnostic, log the detected type in dmesg.
> >>
> >> This paragraph especially depends from grammar. I think.
> > 
> > Thanks for the mail on you. :)
> > 
> >>
> >>> The smmu type's name is now stored in struct arm_smmu_type and ACPI
> >>> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
> >>> macro to ARM_SMMU_TYPE() for better readability.
> >>>
> >>> Signed-off-by: Robert Richter <rrichter@cavium.com>
> >>> ---
> >>>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
> >>>  1 file changed, 30 insertions(+), 31 deletions(-)
> >>
> >> That seems a relatively invasive diffstat for the sake of printing a
> >> string once at boot time to what I can only assume is a small audience
> >> of firmware developers who find "cat
> >> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
> >> equivalent) too hard ;)
> > 
> > Reading firmware data is not really a solution as you don't know what
> > the driver is doing with it. The actual background of this patch is to
> > be sure a certain workaround was enabled in the kernel. ARM's cpu
> > errata framework does this nicely. In case of smmus we just have the
> > internal model implementation type which is not visible in the logs.
> > Right now, there is no way to figure that out without knowing fw
> > specifics and kernel sources.
> 
> Ah, now it starts to become clear. In that case, if we want to confirm
> the presence of specific workarounds, we should actually _confirm the
> presence of specific workarounds_. I'd have no complaint with e.g. this:
> 
> -----8<-----
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index f7411109670f..9e50a092632c 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1934,6 +1934,8 @@ static int arm_smmu_device_cfg_probe(struct
> arm_smmu_device *smmu)
>                         atomic_add_return(smmu->num_context_banks,
>                                           &cavium_smmu_context_count);
>                 smmu->cavium_id_base -= smmu->num_context_banks;
> +               dev_notice(smmu->dev, "\tusing ASID/VMID offset %u\n",
> +                          smmu->cavium_id_base);
>         }
> 
>         /* ID2 */
> ----->8-----
> 
> and the equivalent for other things, if need be. If you just print "hey,
> this is SMMU-x", the user is in fact no better off, since they would
> then still have to go and look at the source for whatever kernel they're
> running to find out which particular workarounds for SMMU-x bugs that
> particular kernel implements.

I don't understand why you don't want to expose in some way the smmu
type. There are a lot of things that can go wrong, esp. with firmware,
to detect the proper smmu type.

The above change is not a general solution too for reporting the
enablement of smmu errata workarounds. The check could be done
multiple times and in the fast path. For the particular problem the
above would work, but still some message on the type detected would be
fine. I could rework my patch in a way that .name is not permanently
stored in struct arm_smmu_device.

-Robert

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robert Richter]

On 07.03.17 18:41:33, Robin Murphy wrote:
> On 07/03/17 14:06, Robert Richter wrote:
> > On 06.03.17 18:22:08, Robin Murphy wrote:
> >> On 06/03/17 13:58, Robert Richter wrote:
> >>> The ARM SMMU detection especially depends from system firmware. For
> >>> better diagnostic, log the detected type in dmesg.
> >>
> >> This paragraph especially depends from grammar. I think.
> > 
> > Thanks for the mail on you. :)
> > 
> >>
> >>> The smmu type's name is now stored in struct arm_smmu_type and ACPI
> >>> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
> >>> macro to ARM_SMMU_TYPE() for better readability.
> >>>
> >>> Signed-off-by: Robert Richter <rrichter-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> >>> ---
> >>>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
> >>>  1 file changed, 30 insertions(+), 31 deletions(-)
> >>
> >> That seems a relatively invasive diffstat for the sake of printing a
> >> string once at boot time to what I can only assume is a small audience
> >> of firmware developers who find "cat
> >> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
> >> equivalent) too hard ;)
> > 
> > Reading firmware data is not really a solution as you don't know what
> > the driver is doing with it. The actual background of this patch is to
> > be sure a certain workaround was enabled in the kernel. ARM's cpu
> > errata framework does this nicely. In case of smmus we just have the
> > internal model implementation type which is not visible in the logs.
> > Right now, there is no way to figure that out without knowing fw
> > specifics and kernel sources.
> 
> Ah, now it starts to become clear. In that case, if we want to confirm
> the presence of specific workarounds, we should actually _confirm the
> presence of specific workarounds_. I'd have no complaint with e.g. this:
> 
> -----8<-----
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index f7411109670f..9e50a092632c 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1934,6 +1934,8 @@ static int arm_smmu_device_cfg_probe(struct
> arm_smmu_device *smmu)
>                         atomic_add_return(smmu->num_context_banks,
>                                           &cavium_smmu_context_count);
>                 smmu->cavium_id_base -= smmu->num_context_banks;
> +               dev_notice(smmu->dev, "\tusing ASID/VMID offset %u\n",
> +                          smmu->cavium_id_base);
>         }
> 
>         /* ID2 */
> ----->8-----
> 
> and the equivalent for other things, if need be. If you just print "hey,
> this is SMMU-x", the user is in fact no better off, since they would
> then still have to go and look at the source for whatever kernel they're
> running to find out which particular workarounds for SMMU-x bugs that
> particular kernel implements.

I don't understand why you don't want to expose in some way the smmu
type. There are a lot of things that can go wrong, esp. with firmware,
to detect the proper smmu type.

The above change is not a general solution too for reporting the
enablement of smmu errata workarounds. The check could be done
multiple times and in the fast path. For the particular problem the
above would work, but still some message on the type detected would be
fine. I could rework my patch in a way that .name is not permanently
stored in struct arm_smmu_device.

-Robert

[PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robert Richter]

On 07.03.17 18:41:33, Robin Murphy wrote:
> On 07/03/17 14:06, Robert Richter wrote:
> > On 06.03.17 18:22:08, Robin Murphy wrote:
> >> On 06/03/17 13:58, Robert Richter wrote:
> >>> The ARM SMMU detection especially depends from system firmware. For
> >>> better diagnostic, log the detected type in dmesg.
> >>
> >> This paragraph especially depends from grammar. I think.
> > 
> > Thanks for the mail on you. :)
> > 
> >>
> >>> The smmu type's name is now stored in struct arm_smmu_type and ACPI
> >>> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
> >>> macro to ARM_SMMU_TYPE() for better readability.
> >>>
> >>> Signed-off-by: Robert Richter <rrichter@cavium.com>
> >>> ---
> >>>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
> >>>  1 file changed, 30 insertions(+), 31 deletions(-)
> >>
> >> That seems a relatively invasive diffstat for the sake of printing a
> >> string once at boot time to what I can only assume is a small audience
> >> of firmware developers who find "cat
> >> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
> >> equivalent) too hard ;)
> > 
> > Reading firmware data is not really a solution as you don't know what
> > the driver is doing with it. The actual background of this patch is to
> > be sure a certain workaround was enabled in the kernel. ARM's cpu
> > errata framework does this nicely. In case of smmus we just have the
> > internal model implementation type which is not visible in the logs.
> > Right now, there is no way to figure that out without knowing fw
> > specifics and kernel sources.
> 
> Ah, now it starts to become clear. In that case, if we want to confirm
> the presence of specific workarounds, we should actually _confirm the
> presence of specific workarounds_. I'd have no complaint with e.g. this:
> 
> -----8<-----
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index f7411109670f..9e50a092632c 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1934,6 +1934,8 @@ static int arm_smmu_device_cfg_probe(struct
> arm_smmu_device *smmu)
>                         atomic_add_return(smmu->num_context_banks,
>                                           &cavium_smmu_context_count);
>                 smmu->cavium_id_base -= smmu->num_context_banks;
> +               dev_notice(smmu->dev, "\tusing ASID/VMID offset %u\n",
> +                          smmu->cavium_id_base);
>         }
> 
>         /* ID2 */
> ----->8-----
> 
> and the equivalent for other things, if need be. If you just print "hey,
> this is SMMU-x", the user is in fact no better off, since they would
> then still have to go and look at the source for whatever kernel they're
> running to find out which particular workarounds for SMMU-x bugs that
> particular kernel implements.

I don't understand why you don't want to expose in some way the smmu
type. There are a lot of things that can go wrong, esp. with firmware,
to detect the proper smmu type.

The above change is not a general solution too for reporting the
enablement of smmu errata workarounds. The check could be done
multiple times and in the fast path. For the particular problem the
above would work, but still some message on the type detected would be
fine. I could rework my patch in a way that .name is not permanently
stored in struct arm_smmu_device.

-Robert

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robert Richter]

On 08.03.17 16:25:24, Aleksey Makarov wrote:

> >-#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> >-static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> >+#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
> >+static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
> 
> Can we infer the _name from the name of var?
> 
> #define ARM_SMMU_TYPE(var, ver, imp)	\
> static struct arm_smmu_type var = { .version = ver, .model = imp, .name = #var }
> 
> >-ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> >-ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> >-ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
> >-ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
> >-ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
> >+ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
> >+ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
> >+ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
> >+ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
> >+ARM_SMMU_TYPE(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2, "cavium-smmuv2");

I thought about this but underscore names are not typical. Adding some
strings as names looks straighforward to me.

-Robert

> 
> So that this change will not be required?

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robert Richter]

On 08.03.17 16:25:24, Aleksey Makarov wrote:

> >-#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> >-static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> >+#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
> >+static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
> 
> Can we infer the _name from the name of var?
> 
> #define ARM_SMMU_TYPE(var, ver, imp)	\
> static struct arm_smmu_type var = { .version = ver, .model = imp, .name = #var }
> 
> >-ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> >-ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> >-ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
> >-ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
> >-ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
> >+ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
> >+ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
> >+ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
> >+ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
> >+ARM_SMMU_TYPE(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2, "cavium-smmuv2");

I thought about this but underscore names are not typical. Adding some
strings as names looks straighforward to me.

-Robert

> 
> So that this change will not be required?

[PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robert Richter]

On 08.03.17 16:25:24, Aleksey Makarov wrote:

> >-#define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> >-static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> >+#define ARM_SMMU_TYPE(var, ver, imp, _name)	\
> >+static struct arm_smmu_type var = { .version = ver, .model = imp, .name = _name }
> 
> Can we infer the _name from the name of var?
> 
> #define ARM_SMMU_TYPE(var, ver, imp)	\
> static struct arm_smmu_type var = { .version = ver, .model = imp, .name = #var }
> 
> >-ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> >-ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> >-ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
> >-ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
> >-ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
> >+ARM_SMMU_TYPE(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU, "smmu-generic-v1");
> >+ARM_SMMU_TYPE(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU, "smmu-generic-v2");
> >+ARM_SMMU_TYPE(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU, "arm-mmu401");
> >+ARM_SMMU_TYPE(arm_mmu500, ARM_SMMU_V2, ARM_MMU500, "arm-mmu500");
> >+ARM_SMMU_TYPE(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2, "cavium-smmuv2");

I thought about this but underscore names are not typical. Adding some
strings as names looks straighforward to me.

-Robert

> 
> So that this change will not be required?

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robin Murphy]

On 09/03/17 12:02, Robert Richter wrote:
> On 07.03.17 18:41:33, Robin Murphy wrote:
>> On 07/03/17 14:06, Robert Richter wrote:
>>> On 06.03.17 18:22:08, Robin Murphy wrote:
>>>> On 06/03/17 13:58, Robert Richter wrote:
>>>>> The ARM SMMU detection especially depends from system firmware. For
>>>>> better diagnostic, log the detected type in dmesg.
>>>>
>>>> This paragraph especially depends from grammar. I think.
>>>
>>> Thanks for the mail on you. :)
>>>
>>>>
>>>>> The smmu type's name is now stored in struct arm_smmu_type and ACPI
>>>>> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
>>>>> macro to ARM_SMMU_TYPE() for better readability.
>>>>>
>>>>> Signed-off-by: Robert Richter <rrichter@cavium.com>
>>>>> ---
>>>>>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
>>>>>  1 file changed, 30 insertions(+), 31 deletions(-)
>>>>
>>>> That seems a relatively invasive diffstat for the sake of printing a
>>>> string once at boot time to what I can only assume is a small audience
>>>> of firmware developers who find "cat
>>>> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
>>>> equivalent) too hard ;)
>>>
>>> Reading firmware data is not really a solution as you don't know what
>>> the driver is doing with it. The actual background of this patch is to
>>> be sure a certain workaround was enabled in the kernel. ARM's cpu
>>> errata framework does this nicely. In case of smmus we just have the
>>> internal model implementation type which is not visible in the logs.
>>> Right now, there is no way to figure that out without knowing fw
>>> specifics and kernel sources.
>>
>> Ah, now it starts to become clear. In that case, if we want to confirm
>> the presence of specific workarounds, we should actually _confirm the
>> presence of specific workarounds_. I'd have no complaint with e.g. this:
>>
>> -----8<-----
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index f7411109670f..9e50a092632c 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1934,6 +1934,8 @@ static int arm_smmu_device_cfg_probe(struct
>> arm_smmu_device *smmu)
>>                         atomic_add_return(smmu->num_context_banks,
>>                                           &cavium_smmu_context_count);
>>                 smmu->cavium_id_base -= smmu->num_context_banks;
>> +               dev_notice(smmu->dev, "\tusing ASID/VMID offset %u\n",
>> +                          smmu->cavium_id_base);
>>         }
>>
>>         /* ID2 */
>> ----->8-----
>>
>> and the equivalent for other things, if need be. If you just print "hey,
>> this is SMMU-x", the user is in fact no better off, since they would
>> then still have to go and look at the source for whatever kernel they're
>> running to find out which particular workarounds for SMMU-x bugs that
>> particular kernel implements.
> 
> I don't understand why you don't want to expose in some way the smmu
> type. There are a lot of things that can go wrong, esp. with firmware,
> to detect the proper smmu type.

Because there is only one reason for which detecting the "proper SMMU
type" matters - implementation-specific workarounds for areas in which a
given hardware implementation deviates from the architecture assumed by
the driver.

OK, let's print the model name. Now, if I give you this:

[    0.475009] arm-smmu 2b500000.iommu: probing hardware configuration...
[    0.481650] arm-smmu 2b500000.iommu: ARM MMU-401 r0p0 with:
[    0.486436] arm-smmu 2b500000.iommu: 	stage 2 translation
[    0.491925] arm-smmu 2b500000.iommu: 	coherent table walk
[    0.497420] arm-smmu 2b500000.iommu: 	stream matching with 32
register groups
[    0.504678] arm-smmu 2b500000.iommu: 	4 context banks (4 stage-2 only)
[    0.511312] arm-smmu 2b500000.iommu: 	Supported page sizes: 0x60211000
[    0.517943] arm-smmu 2b500000.iommu: 	Stage-2: 40-bit IPA -> 40-bit PA

please tell me which hardware problems this system has kernel
workarounds in place for, and which it doesn't. If you want another
hint, the version is 4.11.0-rc1+. Note the "+".

Robin.

> The above change is not a general solution too for reporting the
> enablement of smmu errata workarounds. The check could be done
> multiple times and in the fast path. For the particular problem the
> above would work, but still some message on the type detected would be
> fine. I could rework my patch in a way that .name is not permanently
> stored in struct arm_smmu_device.
> 
> -Robert
> 

Re: [PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robin Murphy]

On 09/03/17 12:02, Robert Richter wrote:
> On 07.03.17 18:41:33, Robin Murphy wrote:
>> On 07/03/17 14:06, Robert Richter wrote:
>>> On 06.03.17 18:22:08, Robin Murphy wrote:
>>>> On 06/03/17 13:58, Robert Richter wrote:
>>>>> The ARM SMMU detection especially depends from system firmware. For
>>>>> better diagnostic, log the detected type in dmesg.
>>>>
>>>> This paragraph especially depends from grammar. I think.
>>>
>>> Thanks for the mail on you. :)
>>>
>>>>
>>>>> The smmu type's name is now stored in struct arm_smmu_type and ACPI
>>>>> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
>>>>> macro to ARM_SMMU_TYPE() for better readability.
>>>>>
>>>>> Signed-off-by: Robert Richter <rrichter-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>>>> ---
>>>>>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
>>>>>  1 file changed, 30 insertions(+), 31 deletions(-)
>>>>
>>>> That seems a relatively invasive diffstat for the sake of printing a
>>>> string once at boot time to what I can only assume is a small audience
>>>> of firmware developers who find "cat
>>>> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
>>>> equivalent) too hard ;)
>>>
>>> Reading firmware data is not really a solution as you don't know what
>>> the driver is doing with it. The actual background of this patch is to
>>> be sure a certain workaround was enabled in the kernel. ARM's cpu
>>> errata framework does this nicely. In case of smmus we just have the
>>> internal model implementation type which is not visible in the logs.
>>> Right now, there is no way to figure that out without knowing fw
>>> specifics and kernel sources.
>>
>> Ah, now it starts to become clear. In that case, if we want to confirm
>> the presence of specific workarounds, we should actually _confirm the
>> presence of specific workarounds_. I'd have no complaint with e.g. this:
>>
>> -----8<-----
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index f7411109670f..9e50a092632c 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1934,6 +1934,8 @@ static int arm_smmu_device_cfg_probe(struct
>> arm_smmu_device *smmu)
>>                         atomic_add_return(smmu->num_context_banks,
>>                                           &cavium_smmu_context_count);
>>                 smmu->cavium_id_base -= smmu->num_context_banks;
>> +               dev_notice(smmu->dev, "\tusing ASID/VMID offset %u\n",
>> +                          smmu->cavium_id_base);
>>         }
>>
>>         /* ID2 */
>> ----->8-----
>>
>> and the equivalent for other things, if need be. If you just print "hey,
>> this is SMMU-x", the user is in fact no better off, since they would
>> then still have to go and look at the source for whatever kernel they're
>> running to find out which particular workarounds for SMMU-x bugs that
>> particular kernel implements.
> 
> I don't understand why you don't want to expose in some way the smmu
> type. There are a lot of things that can go wrong, esp. with firmware,
> to detect the proper smmu type.

Because there is only one reason for which detecting the "proper SMMU
type" matters - implementation-specific workarounds for areas in which a
given hardware implementation deviates from the architecture assumed by
the driver.

OK, let's print the model name. Now, if I give you this:

[    0.475009] arm-smmu 2b500000.iommu: probing hardware configuration...
[    0.481650] arm-smmu 2b500000.iommu: ARM MMU-401 r0p0 with:
[    0.486436] arm-smmu 2b500000.iommu: 	stage 2 translation
[    0.491925] arm-smmu 2b500000.iommu: 	coherent table walk
[    0.497420] arm-smmu 2b500000.iommu: 	stream matching with 32
register groups
[    0.504678] arm-smmu 2b500000.iommu: 	4 context banks (4 stage-2 only)
[    0.511312] arm-smmu 2b500000.iommu: 	Supported page sizes: 0x60211000
[    0.517943] arm-smmu 2b500000.iommu: 	Stage-2: 40-bit IPA -> 40-bit PA

please tell me which hardware problems this system has kernel
workarounds in place for, and which it doesn't. If you want another
hint, the version is 4.11.0-rc1+. Note the "+".

Robin.

> The above change is not a general solution too for reporting the
> enablement of smmu errata workarounds. The check could be done
> multiple times and in the fast path. For the particular problem the
> above would work, but still some message on the type detected would be
> fine. I could rework my patch in a way that .name is not permanently
> stored in struct arm_smmu_device.
> 
> -Robert
> 

[PATCH] iommu/arm-smmu: Report smmu type in dmesg

[Robin Murphy]

On 09/03/17 12:02, Robert Richter wrote:
> On 07.03.17 18:41:33, Robin Murphy wrote:
>> On 07/03/17 14:06, Robert Richter wrote:
>>> On 06.03.17 18:22:08, Robin Murphy wrote:
>>>> On 06/03/17 13:58, Robert Richter wrote:
>>>>> The ARM SMMU detection especially depends from system firmware. For
>>>>> better diagnostic, log the detected type in dmesg.
>>>>
>>>> This paragraph especially depends from grammar. I think.
>>>
>>> Thanks for the mail on you. :)
>>>
>>>>
>>>>> The smmu type's name is now stored in struct arm_smmu_type and ACPI
>>>>> code is modified to use that struct too. Rename ARM_SMMU_MATCH_DATA()
>>>>> macro to ARM_SMMU_TYPE() for better readability.
>>>>>
>>>>> Signed-off-by: Robert Richter <rrichter@cavium.com>
>>>>> ---
>>>>>  drivers/iommu/arm-smmu.c | 61 ++++++++++++++++++++++++------------------------
>>>>>  1 file changed, 30 insertions(+), 31 deletions(-)
>>>>
>>>> That seems a relatively invasive diffstat for the sake of printing a
>>>> string once at boot time to what I can only assume is a small audience
>>>> of firmware developers who find "cat
>>>> /sys/firmware/devicetree/base/iommu*/compatible" (or the ACPI
>>>> equivalent) too hard ;)
>>>
>>> Reading firmware data is not really a solution as you don't know what
>>> the driver is doing with it. The actual background of this patch is to
>>> be sure a certain workaround was enabled in the kernel. ARM's cpu
>>> errata framework does this nicely. In case of smmus we just have the
>>> internal model implementation type which is not visible in the logs.
>>> Right now, there is no way to figure that out without knowing fw
>>> specifics and kernel sources.
>>
>> Ah, now it starts to become clear. In that case, if we want to confirm
>> the presence of specific workarounds, we should actually _confirm the
>> presence of specific workarounds_. I'd have no complaint with e.g. this:
>>
>> -----8<-----
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index f7411109670f..9e50a092632c 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1934,6 +1934,8 @@ static int arm_smmu_device_cfg_probe(struct
>> arm_smmu_device *smmu)
>>                         atomic_add_return(smmu->num_context_banks,
>>                                           &cavium_smmu_context_count);
>>                 smmu->cavium_id_base -= smmu->num_context_banks;
>> +               dev_notice(smmu->dev, "\tusing ASID/VMID offset %u\n",
>> +                          smmu->cavium_id_base);
>>         }
>>
>>         /* ID2 */
>> ----->8-----
>>
>> and the equivalent for other things, if need be. If you just print "hey,
>> this is SMMU-x", the user is in fact no better off, since they would
>> then still have to go and look at the source for whatever kernel they're
>> running to find out which particular workarounds for SMMU-x bugs that
>> particular kernel implements.
> 
> I don't understand why you don't want to expose in some way the smmu
> type. There are a lot of things that can go wrong, esp. with firmware,
> to detect the proper smmu type.

Because there is only one reason for which detecting the "proper SMMU
type" matters - implementation-specific workarounds for areas in which a
given hardware implementation deviates from the architecture assumed by
the driver.

OK, let's print the model name. Now, if I give you this:

[    0.475009] arm-smmu 2b500000.iommu: probing hardware configuration...
[    0.481650] arm-smmu 2b500000.iommu: ARM MMU-401 r0p0 with:
[    0.486436] arm-smmu 2b500000.iommu: 	stage 2 translation
[    0.491925] arm-smmu 2b500000.iommu: 	coherent table walk
[    0.497420] arm-smmu 2b500000.iommu: 	stream matching with 32
register groups
[    0.504678] arm-smmu 2b500000.iommu: 	4 context banks (4 stage-2 only)
[    0.511312] arm-smmu 2b500000.iommu: 	Supported page sizes: 0x60211000
[    0.517943] arm-smmu 2b500000.iommu: 	Stage-2: 40-bit IPA -> 40-bit PA

please tell me which hardware problems this system has kernel
workarounds in place for, and which it doesn't. If you want another
hint, the version is 4.11.0-rc1+. Note the "+".

Robin.

> The above change is not a general solution too for reporting the
> enablement of smmu errata workarounds. The check could be done
> multiple times and in the fast path. For the particular problem the
> above would work, but still some message on the type detected would be
> fine. I could rework my patch in a way that .name is not permanently
> stored in struct arm_smmu_device.
> 
> -Robert
>