[PATCH 1/4] misc: sram: add support for remapping reserved regions only

[PATCH 1/4] misc: sram: add support for remapping reserved regions only

[Mian Yousaf Kaukab]

Some SRAM kernel users may be interested in SRAM’s reserved regions
only, though a bigger SRAM is available on the platform. Add an
optional flag 'reserved-only' which allows to ioremap reserved regions
only. Rest of SRAM is not remapped. ioremap type is selected depending
upon no-memory-wc as usual.

Signed-off-by: Mian Yousaf Kaukab <ykaukab-l3A5Bk7waGM@public.gmane.org>
---
*Tested only on Jetson TX2. Jetson AGX Xavier is untested.*

 drivers/misc/sram.c | 73 ++++++++++++++++++++++++++++++++++-----------
 drivers/misc/sram.h |  3 ++
 2 files changed, 58 insertions(+), 18 deletions(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 6c1a23cb3e8c..44e459f39e22 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -97,7 +97,7 @@ static int sram_add_partition(struct sram_dev *sram, struct sram_reserve *block,
 	struct sram_partition *part = &sram->partition[sram->partitions];
 
 	mutex_init(&part->lock);
-	part->base = sram->virt_base + block->start;
+	part->base = block->virt_start;
 
 	if (block->pool) {
 		ret = sram_add_pool(sram, block, start, part);
@@ -153,6 +153,25 @@ static int sram_reserve_cmp(void *priv, struct list_head *a,
 	return ra->start - rb->start;
 }
 
+static int sram_remap_resource(struct sram_dev *sram,
+		struct resource *res, void __iomem **virt)
+{
+	void __iomem *addr;
+
+	if (sram->no_memory_wc)
+		addr = devm_ioremap_resource(sram->dev, res);
+	else
+		addr = devm_ioremap_resource_wc(sram->dev, res);
+
+	if (IS_ERR(addr)) {
+		dev_err(sram->dev, "could not map SRAM region\n");
+		return PTR_ERR(addr);
+	}
+
+	*virt = addr;
+	return 0;
+}
+
 static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 {
 	struct device_node *np = sram->dev->of_node, *child;
@@ -239,6 +258,16 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 				block->start, block->start + block->size);
 		}
 
+		/* ioremap reserved block as whole sram is not remapped */
+		if (sram->reserved_only) {
+			ret = sram_remap_resource(sram, &child_res,
+					&block->virt_start);
+			if (ret)
+				goto err_chunks;
+		} else {
+			block->virt_start = sram->virt_base + block->start;
+		}
+
 		block++;
 	}
 	child = NULL;
@@ -282,8 +311,11 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 			}
 		}
 
-		/* current start is in a reserved block, so continue after it */
-		if (block->start == cur_start) {
+		/*
+		 * Current start is in a reserved block, so continue after it.
+		 * Or if only using reserved blocks
+		 */
+		if (block->start == cur_start || sram->reserved_only) {
 			cur_start = block->start + block->size;
 			continue;
 		}
@@ -342,6 +374,7 @@ static int sram_probe(struct platform_device *pdev)
 	struct sram_dev *sram;
 	int ret;
 	int (*init_func)(void);
+	struct resource *res;
 
 	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
 	if (!sram)
@@ -349,19 +382,22 @@ static int sram_probe(struct platform_device *pdev)
 
 	sram->dev = &pdev->dev;
 
-	if (of_property_read_bool(pdev->dev.of_node, "no-memory-wc"))
-		sram->virt_base = devm_platform_ioremap_resource(pdev, 0);
-	else
-		sram->virt_base = devm_platform_ioremap_resource_wc(pdev, 0);
-	if (IS_ERR(sram->virt_base)) {
-		dev_err(&pdev->dev, "could not map SRAM registers\n");
-		return PTR_ERR(sram->virt_base);
-	}
+	sram->no_memory_wc =
+		of_property_read_bool(pdev->dev.of_node, "no-memory-wc");
+	sram->reserved_only =
+		of_property_read_bool(pdev->dev.of_node, "reserved-only");
 
-	sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
-					  NUMA_NO_NODE, NULL);
-	if (IS_ERR(sram->pool))
-		return PTR_ERR(sram->pool);
+	if (!sram->reserved_only) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		ret = sram_remap_resource(sram, res, &sram->virt_base);
+		if (ret)
+			return ret;
+
+		sram->pool = devm_gen_pool_create(sram->dev,
+				ilog2(SRAM_GRANULARITY), NUMA_NO_NODE, NULL);
+		if (IS_ERR(sram->pool))
+			return PTR_ERR(sram->pool);
+	}
 
 	sram->clk = devm_clk_get(sram->dev, NULL);
 	if (IS_ERR(sram->clk))
@@ -383,8 +419,9 @@ static int sram_probe(struct platform_device *pdev)
 			goto err_free_partitions;
 	}
 
-	dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
-		gen_pool_size(sram->pool) / 1024, sram->virt_base);
+	if (sram->pool)
+		dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
+			gen_pool_size(sram->pool) / 1024, sram->virt_base);
 
 	return 0;
 
@@ -403,7 +440,7 @@ static int sram_remove(struct platform_device *pdev)
 
 	sram_free_partitions(sram);
 
-	if (gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
+	if (sram->pool && gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
 		dev_err(sram->dev, "removed while SRAM allocated\n");
 
 	if (sram->clk)
diff --git a/drivers/misc/sram.h b/drivers/misc/sram.h
index 9c1d21ff7347..a485fa29458b 100644
--- a/drivers/misc/sram.h
+++ b/drivers/misc/sram.h
@@ -23,6 +23,8 @@ struct sram_dev {
 
 	struct sram_partition *partition;
 	u32 partitions;
+	bool no_memory_wc;
+	bool reserved_only;
 };
 
 struct sram_reserve {
@@ -33,6 +35,7 @@ struct sram_reserve {
 	bool pool;
 	bool protect_exec;
 	const char *label;
+	void __iomem *virt_start;
 };
 
 #ifdef CONFIG_SRAM_EXEC
-- 
2.25.0

[PATCH 1/4] misc: sram: add support for remapping reserved regions only

[Mian Yousaf Kaukab]

Some SRAM kernel users may be interested in SRAM’s reserved regions
only, though a bigger SRAM is available on the platform. Add an
optional flag 'reserved-only' which allows to ioremap reserved regions
only. Rest of SRAM is not remapped. ioremap type is selected depending
upon no-memory-wc as usual.

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
*Tested only on Jetson TX2. Jetson AGX Xavier is untested.*

 drivers/misc/sram.c | 73 ++++++++++++++++++++++++++++++++++-----------
 drivers/misc/sram.h |  3 ++
 2 files changed, 58 insertions(+), 18 deletions(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 6c1a23cb3e8c..44e459f39e22 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -97,7 +97,7 @@ static int sram_add_partition(struct sram_dev *sram, struct sram_reserve *block,
 	struct sram_partition *part = &sram->partition[sram->partitions];
 
 	mutex_init(&part->lock);
-	part->base = sram->virt_base + block->start;
+	part->base = block->virt_start;
 
 	if (block->pool) {
 		ret = sram_add_pool(sram, block, start, part);
@@ -153,6 +153,25 @@ static int sram_reserve_cmp(void *priv, struct list_head *a,
 	return ra->start - rb->start;
 }
 
+static int sram_remap_resource(struct sram_dev *sram,
+		struct resource *res, void __iomem **virt)
+{
+	void __iomem *addr;
+
+	if (sram->no_memory_wc)
+		addr = devm_ioremap_resource(sram->dev, res);
+	else
+		addr = devm_ioremap_resource_wc(sram->dev, res);
+
+	if (IS_ERR(addr)) {
+		dev_err(sram->dev, "could not map SRAM region\n");
+		return PTR_ERR(addr);
+	}
+
+	*virt = addr;
+	return 0;
+}
+
 static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 {
 	struct device_node *np = sram->dev->of_node, *child;
@@ -239,6 +258,16 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 				block->start, block->start + block->size);
 		}
 
+		/* ioremap reserved block as whole sram is not remapped */
+		if (sram->reserved_only) {
+			ret = sram_remap_resource(sram, &child_res,
+					&block->virt_start);
+			if (ret)
+				goto err_chunks;
+		} else {
+			block->virt_start = sram->virt_base + block->start;
+		}
+
 		block++;
 	}
 	child = NULL;
@@ -282,8 +311,11 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 			}
 		}
 
-		/* current start is in a reserved block, so continue after it */
-		if (block->start == cur_start) {
+		/*
+		 * Current start is in a reserved block, so continue after it.
+		 * Or if only using reserved blocks
+		 */
+		if (block->start == cur_start || sram->reserved_only) {
 			cur_start = block->start + block->size;
 			continue;
 		}
@@ -342,6 +374,7 @@ static int sram_probe(struct platform_device *pdev)
 	struct sram_dev *sram;
 	int ret;
 	int (*init_func)(void);
+	struct resource *res;
 
 	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
 	if (!sram)
@@ -349,19 +382,22 @@ static int sram_probe(struct platform_device *pdev)
 
 	sram->dev = &pdev->dev;
 
-	if (of_property_read_bool(pdev->dev.of_node, "no-memory-wc"))
-		sram->virt_base = devm_platform_ioremap_resource(pdev, 0);
-	else
-		sram->virt_base = devm_platform_ioremap_resource_wc(pdev, 0);
-	if (IS_ERR(sram->virt_base)) {
-		dev_err(&pdev->dev, "could not map SRAM registers\n");
-		return PTR_ERR(sram->virt_base);
-	}
+	sram->no_memory_wc =
+		of_property_read_bool(pdev->dev.of_node, "no-memory-wc");
+	sram->reserved_only =
+		of_property_read_bool(pdev->dev.of_node, "reserved-only");
 
-	sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
-					  NUMA_NO_NODE, NULL);
-	if (IS_ERR(sram->pool))
-		return PTR_ERR(sram->pool);
+	if (!sram->reserved_only) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		ret = sram_remap_resource(sram, res, &sram->virt_base);
+		if (ret)
+			return ret;
+
+		sram->pool = devm_gen_pool_create(sram->dev,
+				ilog2(SRAM_GRANULARITY), NUMA_NO_NODE, NULL);
+		if (IS_ERR(sram->pool))
+			return PTR_ERR(sram->pool);
+	}
 
 	sram->clk = devm_clk_get(sram->dev, NULL);
 	if (IS_ERR(sram->clk))
@@ -383,8 +419,9 @@ static int sram_probe(struct platform_device *pdev)
 			goto err_free_partitions;
 	}
 
-	dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
-		gen_pool_size(sram->pool) / 1024, sram->virt_base);
+	if (sram->pool)
+		dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
+			gen_pool_size(sram->pool) / 1024, sram->virt_base);
 
 	return 0;
 
@@ -403,7 +440,7 @@ static int sram_remove(struct platform_device *pdev)
 
 	sram_free_partitions(sram);
 
-	if (gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
+	if (sram->pool && gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
 		dev_err(sram->dev, "removed while SRAM allocated\n");
 
 	if (sram->clk)
diff --git a/drivers/misc/sram.h b/drivers/misc/sram.h
index 9c1d21ff7347..a485fa29458b 100644
--- a/drivers/misc/sram.h
+++ b/drivers/misc/sram.h
@@ -23,6 +23,8 @@ struct sram_dev {
 
 	struct sram_partition *partition;
 	u32 partitions;
+	bool no_memory_wc;
+	bool reserved_only;
 };
 
 struct sram_reserve {
@@ -33,6 +35,7 @@ struct sram_reserve {
 	bool pool;
 	bool protect_exec;
 	const char *label;
+	void __iomem *virt_start;
 };
 
 #ifdef CONFIG_SRAM_EXEC
-- 
2.25.0


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

[PATCH 1/4] misc: sram: add support for remapping reserved regions only

[Mian Yousaf Kaukab]

Some SRAM kernel users may be interested in SRAM’s reserved regions
only, though a bigger SRAM is available on the platform. Add an
optional flag 'reserved-only' which allows to ioremap reserved regions
only. Rest of SRAM is not remapped. ioremap type is selected depending
upon no-memory-wc as usual.

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
*Tested only on Jetson TX2. Jetson AGX Xavier is untested.*

 drivers/misc/sram.c | 73 ++++++++++++++++++++++++++++++++++-----------
 drivers/misc/sram.h |  3 ++
 2 files changed, 58 insertions(+), 18 deletions(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 6c1a23cb3e8c..44e459f39e22 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -97,7 +97,7 @@ static int sram_add_partition(struct sram_dev *sram, struct sram_reserve *block,
 	struct sram_partition *part = &sram->partition[sram->partitions];
 
 	mutex_init(&part->lock);
-	part->base = sram->virt_base + block->start;
+	part->base = block->virt_start;
 
 	if (block->pool) {
 		ret = sram_add_pool(sram, block, start, part);
@@ -153,6 +153,25 @@ static int sram_reserve_cmp(void *priv, struct list_head *a,
 	return ra->start - rb->start;
 }
 
+static int sram_remap_resource(struct sram_dev *sram,
+		struct resource *res, void __iomem **virt)
+{
+	void __iomem *addr;
+
+	if (sram->no_memory_wc)
+		addr = devm_ioremap_resource(sram->dev, res);
+	else
+		addr = devm_ioremap_resource_wc(sram->dev, res);
+
+	if (IS_ERR(addr)) {
+		dev_err(sram->dev, "could not map SRAM region\n");
+		return PTR_ERR(addr);
+	}
+
+	*virt = addr;
+	return 0;
+}
+
 static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 {
 	struct device_node *np = sram->dev->of_node, *child;
@@ -239,6 +258,16 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 				block->start, block->start + block->size);
 		}
 
+		/* ioremap reserved block as whole sram is not remapped */
+		if (sram->reserved_only) {
+			ret = sram_remap_resource(sram, &child_res,
+					&block->virt_start);
+			if (ret)
+				goto err_chunks;
+		} else {
+			block->virt_start = sram->virt_base + block->start;
+		}
+
 		block++;
 	}
 	child = NULL;
@@ -282,8 +311,11 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 			}
 		}
 
-		/* current start is in a reserved block, so continue after it */
-		if (block->start == cur_start) {
+		/*
+		 * Current start is in a reserved block, so continue after it.
+		 * Or if only using reserved blocks
+		 */
+		if (block->start == cur_start || sram->reserved_only) {
 			cur_start = block->start + block->size;
 			continue;
 		}
@@ -342,6 +374,7 @@ static int sram_probe(struct platform_device *pdev)
 	struct sram_dev *sram;
 	int ret;
 	int (*init_func)(void);
+	struct resource *res;
 
 	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
 	if (!sram)
@@ -349,19 +382,22 @@ static int sram_probe(struct platform_device *pdev)
 
 	sram->dev = &pdev->dev;
 
-	if (of_property_read_bool(pdev->dev.of_node, "no-memory-wc"))
-		sram->virt_base = devm_platform_ioremap_resource(pdev, 0);
-	else
-		sram->virt_base = devm_platform_ioremap_resource_wc(pdev, 0);
-	if (IS_ERR(sram->virt_base)) {
-		dev_err(&pdev->dev, "could not map SRAM registers\n");
-		return PTR_ERR(sram->virt_base);
-	}
+	sram->no_memory_wc =
+		of_property_read_bool(pdev->dev.of_node, "no-memory-wc");
+	sram->reserved_only =
+		of_property_read_bool(pdev->dev.of_node, "reserved-only");
 
-	sram->pool = devm_gen_pool_create(sram->dev, ilog2(SRAM_GRANULARITY),
-					  NUMA_NO_NODE, NULL);
-	if (IS_ERR(sram->pool))
-		return PTR_ERR(sram->pool);
+	if (!sram->reserved_only) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		ret = sram_remap_resource(sram, res, &sram->virt_base);
+		if (ret)
+			return ret;
+
+		sram->pool = devm_gen_pool_create(sram->dev,
+				ilog2(SRAM_GRANULARITY), NUMA_NO_NODE, NULL);
+		if (IS_ERR(sram->pool))
+			return PTR_ERR(sram->pool);
+	}
 
 	sram->clk = devm_clk_get(sram->dev, NULL);
 	if (IS_ERR(sram->clk))
@@ -383,8 +419,9 @@ static int sram_probe(struct platform_device *pdev)
 			goto err_free_partitions;
 	}
 
-	dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
-		gen_pool_size(sram->pool) / 1024, sram->virt_base);
+	if (sram->pool)
+		dev_dbg(sram->dev, "SRAM pool: %zu KiB @ 0x%p\n",
+			gen_pool_size(sram->pool) / 1024, sram->virt_base);
 
 	return 0;
 
@@ -403,7 +440,7 @@ static int sram_remove(struct platform_device *pdev)
 
 	sram_free_partitions(sram);
 
-	if (gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
+	if (sram->pool && gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
 		dev_err(sram->dev, "removed while SRAM allocated\n");
 
 	if (sram->clk)
diff --git a/drivers/misc/sram.h b/drivers/misc/sram.h
index 9c1d21ff7347..a485fa29458b 100644
--- a/drivers/misc/sram.h
+++ b/drivers/misc/sram.h
@@ -23,6 +23,8 @@ struct sram_dev {
 
 	struct sram_partition *partition;
 	u32 partitions;
+	bool no_memory_wc;
+	bool reserved_only;
 };
 
 struct sram_reserve {
@@ -33,6 +35,7 @@ struct sram_reserve {
 	bool pool;
 	bool protect_exec;
 	const char *label;
+	void __iomem *virt_start;
 };
 
 #ifdef CONFIG_SRAM_EXEC
-- 
2.25.0

[PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Mian Yousaf Kaukab]

Add documentation for the new optional flag added for SRAM driver.

Signed-off-by: Mian Yousaf Kaukab <ykaukab-l3A5Bk7waGM@public.gmane.org>
---
 Documentation/devicetree/bindings/sram/sram.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
index 19d116ff9ddc..4bcc309fa841 100644
--- a/Documentation/devicetree/bindings/sram/sram.yaml
+++ b/Documentation/devicetree/bindings/sram/sram.yaml
@@ -55,6 +55,12 @@ properties:
       as write combining. WC is used by default.
     type: boolean
 
+  reserved-only:
+    description:
+      The flag indicating, that only SRAM reserved regions have to be remapped.
+      remapping type is selected depending upon no-memory-wc as usual.
+    type: boolean
+
 patternProperties:
   "^([a-z]*-)?sram(-section)?@[a-f0-9]+$":
     type: object
-- 
2.25.0

[PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Mian Yousaf Kaukab]

Add documentation for the new optional flag added for SRAM driver.

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
 Documentation/devicetree/bindings/sram/sram.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
index 19d116ff9ddc..4bcc309fa841 100644
--- a/Documentation/devicetree/bindings/sram/sram.yaml
+++ b/Documentation/devicetree/bindings/sram/sram.yaml
@@ -55,6 +55,12 @@ properties:
       as write combining. WC is used by default.
     type: boolean
 
+  reserved-only:
+    description:
+      The flag indicating, that only SRAM reserved regions have to be remapped.
+      remapping type is selected depending upon no-memory-wc as usual.
+    type: boolean
+
 patternProperties:
   "^([a-z]*-)?sram(-section)?@[a-f0-9]+$":
     type: object
-- 
2.25.0


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

[PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Mian Yousaf Kaukab]

Add documentation for the new optional flag added for SRAM driver.

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
 Documentation/devicetree/bindings/sram/sram.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
index 19d116ff9ddc..4bcc309fa841 100644
--- a/Documentation/devicetree/bindings/sram/sram.yaml
+++ b/Documentation/devicetree/bindings/sram/sram.yaml
@@ -55,6 +55,12 @@ properties:
       as write combining. WC is used by default.
     type: boolean
 
+  reserved-only:
+    description:
+      The flag indicating, that only SRAM reserved regions have to be remapped.
+      remapping type is selected depending upon no-memory-wc as usual.
+    type: boolean
+
 patternProperties:
   "^([a-z]*-)?sram(-section)?@[a-f0-9]+$":
     type: object
-- 
2.25.0

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Stephen Warren]

On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> Add documentation for the new optional flag added for SRAM driver.

> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml

> +  reserved-only:
> +    description:
> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> +      remapping type is selected depending upon no-memory-wc as usual.
> +    type: boolean

This feels a bit like a SW flag rather than a HW description, so I'm not
sure it's appropriate to put it into DT.

Are there any cases where the SW should map all of the SRAM, i.e. where
we wouldn't expect to set reserved-only? I'd expect reserved-only to be
the default, and perhaps only, mode of operation for the SRAM driver. If
we can't do that because some SW currently expects to be able to map
arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
SRAM driver which parts it's using, hence still allowing the driver to
only map in-use portions?

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Stephen Warren]

On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> Add documentation for the new optional flag added for SRAM driver.

> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml

> +  reserved-only:
> +    description:
> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> +      remapping type is selected depending upon no-memory-wc as usual.
> +    type: boolean

This feels a bit like a SW flag rather than a HW description, so I'm not
sure it's appropriate to put it into DT.

Are there any cases where the SW should map all of the SRAM, i.e. where
we wouldn't expect to set reserved-only? I'd expect reserved-only to be
the default, and perhaps only, mode of operation for the SRAM driver. If
we can't do that because some SW currently expects to be able to map
arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
SRAM driver which parts it's using, hence still allowing the driver to
only map in-use portions?

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

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Stephen Warren]

On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> Add documentation for the new optional flag added for SRAM driver.

> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml

> +  reserved-only:
> +    description:
> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> +      remapping type is selected depending upon no-memory-wc as usual.
> +    type: boolean

This feels a bit like a SW flag rather than a HW description, so I'm not
sure it's appropriate to put it into DT.

Are there any cases where the SW should map all of the SRAM, i.e. where
we wouldn't expect to set reserved-only? I'd expect reserved-only to be
the default, and perhaps only, mode of operation for the SRAM driver. If
we can't do that because some SW currently expects to be able to map
arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
SRAM driver which parts it's using, hence still allowing the driver to
only map in-use portions?

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Mian Yousaf Kaukab]

On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > Add documentation for the new optional flag added for SRAM driver.
> 
> > diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> 
> > +  reserved-only:
> > +    description:
> > +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > +      remapping type is selected depending upon no-memory-wc as usual.
> > +    type: boolean
> 
> This feels a bit like a SW flag rather than a HW description, so I'm not
> sure it's appropriate to put it into DT.

Reserved regions themselves are software descriptions, no? Then we have 'pool'
flag which is again a software flag and so on. This flag falls into same
category and nothing out of ordinary.
> 
> Are there any cases where the SW should map all of the SRAM, i.e. where
> we wouldn't expect to set reserved-only? [...]

Yes, here are a few examples:
arch/arm/boot/dts/aspeed-g*.dtsi
arch/arm/boot/dts/at91*.dtsi
arch/arm/boot/dts/bcm7445.dtsi
Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
except the reserved region.

> [...] I'd expect reserved-only to be
> the default, and perhaps only, mode of operation for the SRAM driver.

It will break compatibility with existing dtbs.

> If we can't do that because some SW currently expects to be able to map
> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> SRAM driver which parts it's using, hence still allowing the driver to
> only map in-use portions?

User doesn’t need sram driver in that case. It can use genalloc api directly.

BR,
Yousaf

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Mian Yousaf Kaukab]

On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > Add documentation for the new optional flag added for SRAM driver.
> 
> > diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> 
> > +  reserved-only:
> > +    description:
> > +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > +      remapping type is selected depending upon no-memory-wc as usual.
> > +    type: boolean
> 
> This feels a bit like a SW flag rather than a HW description, so I'm not
> sure it's appropriate to put it into DT.

Reserved regions themselves are software descriptions, no? Then we have 'pool'
flag which is again a software flag and so on. This flag falls into same
category and nothing out of ordinary.
> 
> Are there any cases where the SW should map all of the SRAM, i.e. where
> we wouldn't expect to set reserved-only? [...]

Yes, here are a few examples:
arch/arm/boot/dts/aspeed-g*.dtsi
arch/arm/boot/dts/at91*.dtsi
arch/arm/boot/dts/bcm7445.dtsi
Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
except the reserved region.

> [...] I'd expect reserved-only to be
> the default, and perhaps only, mode of operation for the SRAM driver.

It will break compatibility with existing dtbs.

> If we can't do that because some SW currently expects to be able to map
> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> SRAM driver which parts it's using, hence still allowing the driver to
> only map in-use portions?

User doesn’t need sram driver in that case. It can use genalloc api directly.

BR,
Yousaf

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

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Mian Yousaf Kaukab]

On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > Add documentation for the new optional flag added for SRAM driver.
> 
> > diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> 
> > +  reserved-only:
> > +    description:
> > +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > +      remapping type is selected depending upon no-memory-wc as usual.
> > +    type: boolean
> 
> This feels a bit like a SW flag rather than a HW description, so I'm not
> sure it's appropriate to put it into DT.

Reserved regions themselves are software descriptions, no? Then we have 'pool'
flag which is again a software flag and so on. This flag falls into same
category and nothing out of ordinary.
> 
> Are there any cases where the SW should map all of the SRAM, i.e. where
> we wouldn't expect to set reserved-only? [...]

Yes, here are a few examples:
arch/arm/boot/dts/aspeed-g*.dtsi
arch/arm/boot/dts/at91*.dtsi
arch/arm/boot/dts/bcm7445.dtsi
Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
except the reserved region.

> [...] I'd expect reserved-only to be
> the default, and perhaps only, mode of operation for the SRAM driver.

It will break compatibility with existing dtbs.

> If we can't do that because some SW currently expects to be able to map
> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> SRAM driver which parts it's using, hence still allowing the driver to
> only map in-use portions?

User doesn’t need sram driver in that case. It can use genalloc api directly.

BR,
Yousaf

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Stephen Warren]

On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
>> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
>>> Add documentation for the new optional flag added for SRAM driver.
>>
>>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
>>
>>> +  reserved-only:
>>> +    description:
>>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
>>> +      remapping type is selected depending upon no-memory-wc as usual.
>>> +    type: boolean
>>
>> This feels a bit like a SW flag rather than a HW description, so I'm not
>> sure it's appropriate to put it into DT.
> 
> Reserved regions themselves are software descriptions, no? Then we have 'pool'
> flag which is again a software flag and so on. This flag falls into same
> category and nothing out of ordinary.

I suppose that's true to some extent. This is indeed a description of
the system environment presented to the SW that consumes the DT, which
is a bit more than pure HW description but still a description of
something imposed externally rather than describing something that's up
to the discretion of the consuming SW. So, go ahead.

>> Are there any cases where the SW should map all of the SRAM, i.e. where
>> we wouldn't expect to set reserved-only? [...]
> 
> Yes, here are a few examples:
> arch/arm/boot/dts/aspeed-g*.dtsi
> arch/arm/boot/dts/at91*.dtsi
> arch/arm/boot/dts/bcm7445.dtsi
> Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> except the reserved region.
> 
>> [...] I'd expect reserved-only to be
>> the default, and perhaps only, mode of operation for the SRAM driver.
> 
> It will break compatibility with existing dtbs.
> 
>> If we can't do that because some SW currently expects to be able to map
>> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
>> SRAM driver which parts it's using, hence still allowing the driver to
>> only map in-use portions?
> 
> User doesn’t need sram driver in that case. It can use genalloc api directly.

This sounds a bit odd. Without a driver for the reserved region, nothing
should be touching it, since otherwise there's no code that owns an
manages the region. If any code needs to consume the region, it should
obtain info about the region from some form of provider code that can
handle both the allocation and mapping. Anything else sounds like some
consumer code directly making use of DT nodes it doesn't own. But since
I'm not familiar enough with the SRAM driver and genalloc code that you
mention to fully understand the allocation paths I guess I won't object
for now, although it does still sound fishy.

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Stephen Warren]

On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
>> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
>>> Add documentation for the new optional flag added for SRAM driver.
>>
>>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
>>
>>> +  reserved-only:
>>> +    description:
>>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
>>> +      remapping type is selected depending upon no-memory-wc as usual.
>>> +    type: boolean
>>
>> This feels a bit like a SW flag rather than a HW description, so I'm not
>> sure it's appropriate to put it into DT.
> 
> Reserved regions themselves are software descriptions, no? Then we have 'pool'
> flag which is again a software flag and so on. This flag falls into same
> category and nothing out of ordinary.

I suppose that's true to some extent. This is indeed a description of
the system environment presented to the SW that consumes the DT, which
is a bit more than pure HW description but still a description of
something imposed externally rather than describing something that's up
to the discretion of the consuming SW. So, go ahead.

>> Are there any cases where the SW should map all of the SRAM, i.e. where
>> we wouldn't expect to set reserved-only? [...]
> 
> Yes, here are a few examples:
> arch/arm/boot/dts/aspeed-g*.dtsi
> arch/arm/boot/dts/at91*.dtsi
> arch/arm/boot/dts/bcm7445.dtsi
> Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> except the reserved region.
> 
>> [...] I'd expect reserved-only to be
>> the default, and perhaps only, mode of operation for the SRAM driver.
> 
> It will break compatibility with existing dtbs.
> 
>> If we can't do that because some SW currently expects to be able to map
>> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
>> SRAM driver which parts it's using, hence still allowing the driver to
>> only map in-use portions?
> 
> User doesn’t need sram driver in that case. It can use genalloc api directly.

This sounds a bit odd. Without a driver for the reserved region, nothing
should be touching it, since otherwise there's no code that owns an
manages the region. If any code needs to consume the region, it should
obtain info about the region from some form of provider code that can
handle both the allocation and mapping. Anything else sounds like some
consumer code directly making use of DT nodes it doesn't own. But since
I'm not familiar enough with the SRAM driver and genalloc code that you
mention to fully understand the allocation paths I guess I won't object
for now, although it does still sound fishy.

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

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Stephen Warren]

On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
>> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
>>> Add documentation for the new optional flag added for SRAM driver.
>>
>>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
>>
>>> +  reserved-only:
>>> +    description:
>>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
>>> +      remapping type is selected depending upon no-memory-wc as usual.
>>> +    type: boolean
>>
>> This feels a bit like a SW flag rather than a HW description, so I'm not
>> sure it's appropriate to put it into DT.
> 
> Reserved regions themselves are software descriptions, no? Then we have 'pool'
> flag which is again a software flag and so on. This flag falls into same
> category and nothing out of ordinary.

I suppose that's true to some extent. This is indeed a description of
the system environment presented to the SW that consumes the DT, which
is a bit more than pure HW description but still a description of
something imposed externally rather than describing something that's up
to the discretion of the consuming SW. So, go ahead.

>> Are there any cases where the SW should map all of the SRAM, i.e. where
>> we wouldn't expect to set reserved-only? [...]
> 
> Yes, here are a few examples:
> arch/arm/boot/dts/aspeed-g*.dtsi
> arch/arm/boot/dts/at91*.dtsi
> arch/arm/boot/dts/bcm7445.dtsi
> Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> except the reserved region.
> 
>> [...] I'd expect reserved-only to be
>> the default, and perhaps only, mode of operation for the SRAM driver.
> 
> It will break compatibility with existing dtbs.
> 
>> If we can't do that because some SW currently expects to be able to map
>> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
>> SRAM driver which parts it's using, hence still allowing the driver to
>> only map in-use portions?
> 
> User doesn’t need sram driver in that case. It can use genalloc api directly.

This sounds a bit odd. Without a driver for the reserved region, nothing
should be touching it, since otherwise there's no code that owns an
manages the region. If any code needs to consume the region, it should
obtain info about the region from some form of provider code that can
handle both the allocation and mapping. Anything else sounds like some
consumer code directly making use of DT nodes it doesn't own. But since
I'm not familiar enough with the SRAM driver and genalloc code that you
mention to fully understand the allocation paths I guess I won't object
for now, although it does still sound fishy.

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Rob Herring]

On Tue, May 19, 2020 at 10:16:43AM -0600, Stephen Warren wrote:
> On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> >> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> >>> Add documentation for the new optional flag added for SRAM driver.
> >>
> >>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> >>
> >>> +  reserved-only:
> >>> +    description:
> >>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> >>> +      remapping type is selected depending upon no-memory-wc as usual.
> >>> +    type: boolean
> >>
> >> This feels a bit like a SW flag rather than a HW description, so I'm not
> >> sure it's appropriate to put it into DT.
> > 
> > Reserved regions themselves are software descriptions, no? Then we have 'pool'
> > flag which is again a software flag and so on. This flag falls into same
> > category and nothing out of ordinary.
> 
> I suppose that's true to some extent. This is indeed a description of
> the system environment presented to the SW that consumes the DT, which
> is a bit more than pure HW description but still a description of
> something imposed externally rather than describing something that's up
> to the discretion of the consuming SW. So, go ahead.
> 
> >> Are there any cases where the SW should map all of the SRAM, i.e. where
> >> we wouldn't expect to set reserved-only? [...]
> > 
> > Yes, here are a few examples:
> > arch/arm/boot/dts/aspeed-g*.dtsi
> > arch/arm/boot/dts/at91*.dtsi
> > arch/arm/boot/dts/bcm7445.dtsi
> > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> > except the reserved region.
> > 
> >> [...] I'd expect reserved-only to be
> >> the default, and perhaps only, mode of operation for the SRAM driver.
> > 
> > It will break compatibility with existing dtbs.
> > 
> >> If we can't do that because some SW currently expects to be able to map
> >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> >> SRAM driver which parts it's using, hence still allowing the driver to
> >> only map in-use portions?
> > 
> > User doesn’t need sram driver in that case. It can use genalloc api directly.
> 
> This sounds a bit odd. Without a driver for the reserved region, nothing
> should be touching it, since otherwise there's no code that owns an
> manages the region. If any code needs to consume the region, it should
> obtain info about the region from some form of provider code that can
> handle both the allocation and mapping. Anything else sounds like some
> consumer code directly making use of DT nodes it doesn't own. But since
> I'm not familiar enough with the SRAM driver and genalloc code that you
> mention to fully understand the allocation paths I guess I won't object
> for now, although it does still sound fishy.

I'm fine with the concept, but I don't think a single flag is adequate. 
If there are reserved regions within the SRAM, then define child nodes 
to mark those regions reserved. I don't think you need a new flag. Just 
a 'reg' property and nothing else.

Rob

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Rob Herring]

On Tue, May 19, 2020 at 10:16:43AM -0600, Stephen Warren wrote:
> On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> >> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> >>> Add documentation for the new optional flag added for SRAM driver.
> >>
> >>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> >>
> >>> +  reserved-only:
> >>> +    description:
> >>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> >>> +      remapping type is selected depending upon no-memory-wc as usual.
> >>> +    type: boolean
> >>
> >> This feels a bit like a SW flag rather than a HW description, so I'm not
> >> sure it's appropriate to put it into DT.
> > 
> > Reserved regions themselves are software descriptions, no? Then we have 'pool'
> > flag which is again a software flag and so on. This flag falls into same
> > category and nothing out of ordinary.
> 
> I suppose that's true to some extent. This is indeed a description of
> the system environment presented to the SW that consumes the DT, which
> is a bit more than pure HW description but still a description of
> something imposed externally rather than describing something that's up
> to the discretion of the consuming SW. So, go ahead.
> 
> >> Are there any cases where the SW should map all of the SRAM, i.e. where
> >> we wouldn't expect to set reserved-only? [...]
> > 
> > Yes, here are a few examples:
> > arch/arm/boot/dts/aspeed-g*.dtsi
> > arch/arm/boot/dts/at91*.dtsi
> > arch/arm/boot/dts/bcm7445.dtsi
> > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> > except the reserved region.
> > 
> >> [...] I'd expect reserved-only to be
> >> the default, and perhaps only, mode of operation for the SRAM driver.
> > 
> > It will break compatibility with existing dtbs.
> > 
> >> If we can't do that because some SW currently expects to be able to map
> >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> >> SRAM driver which parts it's using, hence still allowing the driver to
> >> only map in-use portions?
> > 
> > User doesn’t need sram driver in that case. It can use genalloc api directly.
> 
> This sounds a bit odd. Without a driver for the reserved region, nothing
> should be touching it, since otherwise there's no code that owns an
> manages the region. If any code needs to consume the region, it should
> obtain info about the region from some form of provider code that can
> handle both the allocation and mapping. Anything else sounds like some
> consumer code directly making use of DT nodes it doesn't own. But since
> I'm not familiar enough with the SRAM driver and genalloc code that you
> mention to fully understand the allocation paths I guess I won't object
> for now, although it does still sound fishy.

I'm fine with the concept, but I don't think a single flag is adequate. 
If there are reserved regions within the SRAM, then define child nodes 
to mark those regions reserved. I don't think you need a new flag. Just 
a 'reg' property and nothing else.

Rob

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

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Rob Herring]

On Tue, May 19, 2020 at 10:16:43AM -0600, Stephen Warren wrote:
> On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> >> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> >>> Add documentation for the new optional flag added for SRAM driver.
> >>
> >>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> >>
> >>> +  reserved-only:
> >>> +    description:
> >>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> >>> +      remapping type is selected depending upon no-memory-wc as usual.
> >>> +    type: boolean
> >>
> >> This feels a bit like a SW flag rather than a HW description, so I'm not
> >> sure it's appropriate to put it into DT.
> > 
> > Reserved regions themselves are software descriptions, no? Then we have 'pool'
> > flag which is again a software flag and so on. This flag falls into same
> > category and nothing out of ordinary.
> 
> I suppose that's true to some extent. This is indeed a description of
> the system environment presented to the SW that consumes the DT, which
> is a bit more than pure HW description but still a description of
> something imposed externally rather than describing something that's up
> to the discretion of the consuming SW. So, go ahead.
> 
> >> Are there any cases where the SW should map all of the SRAM, i.e. where
> >> we wouldn't expect to set reserved-only? [...]
> > 
> > Yes, here are a few examples:
> > arch/arm/boot/dts/aspeed-g*.dtsi
> > arch/arm/boot/dts/at91*.dtsi
> > arch/arm/boot/dts/bcm7445.dtsi
> > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> > except the reserved region.
> > 
> >> [...] I'd expect reserved-only to be
> >> the default, and perhaps only, mode of operation for the SRAM driver.
> > 
> > It will break compatibility with existing dtbs.
> > 
> >> If we can't do that because some SW currently expects to be able to map
> >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> >> SRAM driver which parts it's using, hence still allowing the driver to
> >> only map in-use portions?
> > 
> > User doesn’t need sram driver in that case. It can use genalloc api directly.
> 
> This sounds a bit odd. Without a driver for the reserved region, nothing
> should be touching it, since otherwise there's no code that owns an
> manages the region. If any code needs to consume the region, it should
> obtain info about the region from some form of provider code that can
> handle both the allocation and mapping. Anything else sounds like some
> consumer code directly making use of DT nodes it doesn't own. But since
> I'm not familiar enough with the SRAM driver and genalloc code that you
> mention to fully understand the allocation paths I guess I won't object
> for now, although it does still sound fishy.

I'm fine with the concept, but I don't think a single flag is adequate. 
If there are reserved regions within the SRAM, then define child nodes 
to mark those regions reserved. I don't think you need a new flag. Just 
a 'reg' property and nothing else.

Rob

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5789 bytes --]

On Tue, May 19, 2020 at 05:03:26PM -0600, Rob Herring wrote:
> On Tue, May 19, 2020 at 10:16:43AM -0600, Stephen Warren wrote:
> > On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> > > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> > >> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > >>> Add documentation for the new optional flag added for SRAM driver.
> > >>
> > >>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> > >>
> > >>> +  reserved-only:
> > >>> +    description:
> > >>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > >>> +      remapping type is selected depending upon no-memory-wc as usual.
> > >>> +    type: boolean
> > >>
> > >> This feels a bit like a SW flag rather than a HW description, so I'm not
> > >> sure it's appropriate to put it into DT.
> > > 
> > > Reserved regions themselves are software descriptions, no? Then we have 'pool'
> > > flag which is again a software flag and so on. This flag falls into same
> > > category and nothing out of ordinary.
> > 
> > I suppose that's true to some extent. This is indeed a description of
> > the system environment presented to the SW that consumes the DT, which
> > is a bit more than pure HW description but still a description of
> > something imposed externally rather than describing something that's up
> > to the discretion of the consuming SW. So, go ahead.
> > 
> > >> Are there any cases where the SW should map all of the SRAM, i.e. where
> > >> we wouldn't expect to set reserved-only? [...]
> > > 
> > > Yes, here are a few examples:
> > > arch/arm/boot/dts/aspeed-g*.dtsi
> > > arch/arm/boot/dts/at91*.dtsi
> > > arch/arm/boot/dts/bcm7445.dtsi
> > > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> > > except the reserved region.
> > > 
> > >> [...] I'd expect reserved-only to be
> > >> the default, and perhaps only, mode of operation for the SRAM driver.
> > > 
> > > It will break compatibility with existing dtbs.
> > > 
> > >> If we can't do that because some SW currently expects to be able to map
> > >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> > >> SRAM driver which parts it's using, hence still allowing the driver to
> > >> only map in-use portions?
> > > 
> > > User doesn’t need sram driver in that case. It can use genalloc api directly.
> > 
> > This sounds a bit odd. Without a driver for the reserved region, nothing
> > should be touching it, since otherwise there's no code that owns an
> > manages the region. If any code needs to consume the region, it should
> > obtain info about the region from some form of provider code that can
> > handle both the allocation and mapping. Anything else sounds like some
> > consumer code directly making use of DT nodes it doesn't own. But since
> > I'm not familiar enough with the SRAM driver and genalloc code that you
> > mention to fully understand the allocation paths I guess I won't object
> > for now, although it does still sound fishy.
> 
> I'm fine with the concept, but I don't think a single flag is adequate. 
> If there are reserved regions within the SRAM, then define child nodes 
> to mark those regions reserved. I don't think you need a new flag. Just 
> a 'reg' property and nothing else.

It sounds to me like there are two different interpretations of SRAM and
reserved regions. On one hand, as you suggest, we have one SRAM that's
made available as genalloc pool and then individual regions can be
marked as reserved so that they aren't added to that pool.

At the same time, each reserved region is also exposed as a separate
pool and that's in fact used by many consumers as a way of getting a
specific chunk of the SRAM for their own use (via phandle to the region
from the consumer's device tree node).

In addition to that, the reserved region code doesn't actually fully do
its job because while the reserved region isn't actually added to the
"top-level" SRAM pool, the memory is still mapped. At the same time this
is something that we actually want because, like I mentioned, some of
the consumers do want to get at their SRAM chunks via references to the
partitions.

The problem that this patch series is really trying to solve is another
still: the complete SRAM is always mapped to kernel memory, irrespective
of whether any regions are marked reserved or not and that can cause
speculative accesses to memory outside of the defined regions.

Stephen's suggestion is to default to only mapping memory for which a
partition has been defined in the SRAM and assuming that all SRAM
outside of those partitions is off limits. I think that's a sensible
default and it's unambiguous.

But as Yousaf points out that would break compatibility with existing
device trees. Depending on how you interpret the bindings one could
argue that those device trees are buggy and should have partitions
defined (in the cases I've looked at they end up using a fixed region
anyway, so that could've just been made explicit in the device tree).

However, it also looks like all of the users that rely on the original
behaviour where they can just access the full pool are those that don't
define any reserved regions, whereas all users that do reserve regions
will actually use those reserved regions.

So I think we can make use of this by differentiating in the driver
between SRAM nodes with or without children and change the behaviour
accordingly. I think that has the big advantage that it makes things
work as (I think) most people would expect and doesn't further
complicate the binding with extra flags.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 5789 bytes --]

On Tue, May 19, 2020 at 05:03:26PM -0600, Rob Herring wrote:
> On Tue, May 19, 2020 at 10:16:43AM -0600, Stephen Warren wrote:
> > On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> > > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> > >> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > >>> Add documentation for the new optional flag added for SRAM driver.
> > >>
> > >>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> > >>
> > >>> +  reserved-only:
> > >>> +    description:
> > >>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > >>> +      remapping type is selected depending upon no-memory-wc as usual.
> > >>> +    type: boolean
> > >>
> > >> This feels a bit like a SW flag rather than a HW description, so I'm not
> > >> sure it's appropriate to put it into DT.
> > > 
> > > Reserved regions themselves are software descriptions, no? Then we have 'pool'
> > > flag which is again a software flag and so on. This flag falls into same
> > > category and nothing out of ordinary.
> > 
> > I suppose that's true to some extent. This is indeed a description of
> > the system environment presented to the SW that consumes the DT, which
> > is a bit more than pure HW description but still a description of
> > something imposed externally rather than describing something that's up
> > to the discretion of the consuming SW. So, go ahead.
> > 
> > >> Are there any cases where the SW should map all of the SRAM, i.e. where
> > >> we wouldn't expect to set reserved-only? [...]
> > > 
> > > Yes, here are a few examples:
> > > arch/arm/boot/dts/aspeed-g*.dtsi
> > > arch/arm/boot/dts/at91*.dtsi
> > > arch/arm/boot/dts/bcm7445.dtsi
> > > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> > > except the reserved region.
> > > 
> > >> [...] I'd expect reserved-only to be
> > >> the default, and perhaps only, mode of operation for the SRAM driver.
> > > 
> > > It will break compatibility with existing dtbs.
> > > 
> > >> If we can't do that because some SW currently expects to be able to map
> > >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> > >> SRAM driver which parts it's using, hence still allowing the driver to
> > >> only map in-use portions?
> > > 
> > > User doesn’t need sram driver in that case. It can use genalloc api directly.
> > 
> > This sounds a bit odd. Without a driver for the reserved region, nothing
> > should be touching it, since otherwise there's no code that owns an
> > manages the region. If any code needs to consume the region, it should
> > obtain info about the region from some form of provider code that can
> > handle both the allocation and mapping. Anything else sounds like some
> > consumer code directly making use of DT nodes it doesn't own. But since
> > I'm not familiar enough with the SRAM driver and genalloc code that you
> > mention to fully understand the allocation paths I guess I won't object
> > for now, although it does still sound fishy.
> 
> I'm fine with the concept, but I don't think a single flag is adequate. 
> If there are reserved regions within the SRAM, then define child nodes 
> to mark those regions reserved. I don't think you need a new flag. Just 
> a 'reg' property and nothing else.

It sounds to me like there are two different interpretations of SRAM and
reserved regions. On one hand, as you suggest, we have one SRAM that's
made available as genalloc pool and then individual regions can be
marked as reserved so that they aren't added to that pool.

At the same time, each reserved region is also exposed as a separate
pool and that's in fact used by many consumers as a way of getting a
specific chunk of the SRAM for their own use (via phandle to the region
from the consumer's device tree node).

In addition to that, the reserved region code doesn't actually fully do
its job because while the reserved region isn't actually added to the
"top-level" SRAM pool, the memory is still mapped. At the same time this
is something that we actually want because, like I mentioned, some of
the consumers do want to get at their SRAM chunks via references to the
partitions.

The problem that this patch series is really trying to solve is another
still: the complete SRAM is always mapped to kernel memory, irrespective
of whether any regions are marked reserved or not and that can cause
speculative accesses to memory outside of the defined regions.

Stephen's suggestion is to default to only mapping memory for which a
partition has been defined in the SRAM and assuming that all SRAM
outside of those partitions is off limits. I think that's a sensible
default and it's unambiguous.

But as Yousaf points out that would break compatibility with existing
device trees. Depending on how you interpret the bindings one could
argue that those device trees are buggy and should have partitions
defined (in the cases I've looked at they end up using a fixed region
anyway, so that could've just been made explicit in the device tree).

However, it also looks like all of the users that rely on the original
behaviour where they can just access the full pool are those that don't
define any reserved regions, whereas all users that do reserve regions
will actually use those reserved regions.

So I think we can make use of this by differentiating in the driver
between SRAM nodes with or without children and change the behaviour
accordingly. I think that has the big advantage that it makes things
work as (I think) most people would expect and doesn't further
complicate the binding with extra flags.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Mian Yousaf Kaukab]

On Wed, May 20, 2020 at 10:55:58AM +0200, Thierry Reding wrote:
> On Tue, May 19, 2020 at 05:03:26PM -0600, Rob Herring wrote:
> > On Tue, May 19, 2020 at 10:16:43AM -0600, Stephen Warren wrote:
> > > On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> > > > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> > > >> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > > >>> Add documentation for the new optional flag added for SRAM driver.
> > > >>
> > > >>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> > > >>
> > > >>> +  reserved-only:
> > > >>> +    description:
> > > >>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > > >>> +      remapping type is selected depending upon no-memory-wc as usual.
> > > >>> +    type: boolean
> > > >>
> > > >> This feels a bit like a SW flag rather than a HW description, so I'm not
> > > >> sure it's appropriate to put it into DT.
> > > > 
> > > > Reserved regions themselves are software descriptions, no? Then we have 'pool'
> > > > flag which is again a software flag and so on. This flag falls into same
> > > > category and nothing out of ordinary.
> > > 
> > > I suppose that's true to some extent. This is indeed a description of
> > > the system environment presented to the SW that consumes the DT, which
> > > is a bit more than pure HW description but still a description of
> > > something imposed externally rather than describing something that's up
> > > to the discretion of the consuming SW. So, go ahead.
> > > 
> > > >> Are there any cases where the SW should map all of the SRAM, i.e. where
> > > >> we wouldn't expect to set reserved-only? [...]
> > > > 
> > > > Yes, here are a few examples:
> > > > arch/arm/boot/dts/aspeed-g*.dtsi
> > > > arch/arm/boot/dts/at91*.dtsi
> > > > arch/arm/boot/dts/bcm7445.dtsi
> > > > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> > > > except the reserved region.
> > > > 
> > > >> [...] I'd expect reserved-only to be
> > > >> the default, and perhaps only, mode of operation for the SRAM driver.
> > > > 
> > > > It will break compatibility with existing dtbs.
> > > > 
> > > >> If we can't do that because some SW currently expects to be able to map
> > > >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> > > >> SRAM driver which parts it's using, hence still allowing the driver to
> > > >> only map in-use portions?
> > > > 
> > > > User doesn’t need sram driver in that case. It can use genalloc api directly.
> > > 
> > > This sounds a bit odd. Without a driver for the reserved region, nothing
> > > should be touching it, since otherwise there's no code that owns an
> > > manages the region. If any code needs to consume the region, it should
> > > obtain info about the region from some form of provider code that can
> > > handle both the allocation and mapping. Anything else sounds like some
> > > consumer code directly making use of DT nodes it doesn't own. But since
> > > I'm not familiar enough with the SRAM driver and genalloc code that you
> > > mention to fully understand the allocation paths I guess I won't object
> > > for now, although it does still sound fishy.
> > 
> > I'm fine with the concept, but I don't think a single flag is adequate. 
> > If there are reserved regions within the SRAM, then define child nodes 
> > to mark those regions reserved. I don't think you need a new flag. Just 
> > a 'reg' property and nothing else.
> 
> It sounds to me like there are two different interpretations of SRAM and
> reserved regions. On one hand, as you suggest, we have one SRAM that's
> made available as genalloc pool and then individual regions can be
> marked as reserved so that they aren't added to that pool.
> 
> At the same time, each reserved region is also exposed as a separate
> pool and that's in fact used by many consumers as a way of getting a
> specific chunk of the SRAM for their own use (via phandle to the region
> from the consumer's device tree node).
> 
> In addition to that, the reserved region code doesn't actually fully do
> its job because while the reserved region isn't actually added to the
> "top-level" SRAM pool, the memory is still mapped. At the same time this
> is something that we actually want because, like I mentioned, some of
> the consumers do want to get at their SRAM chunks via references to the
> partitions.
> 
> The problem that this patch series is really trying to solve is another
> still: the complete SRAM is always mapped to kernel memory, irrespective
> of whether any regions are marked reserved or not and that can cause
> speculative accesses to memory outside of the defined regions.
> 
> Stephen's suggestion is to default to only mapping memory for which a
> partition has been defined in the SRAM and assuming that all SRAM
> outside of those partitions is off limits. I think that's a sensible
> default and it's unambiguous.
> 
> But as Yousaf points out that would break compatibility with existing
> device trees. Depending on how you interpret the bindings one could
> argue that those device trees are buggy and should have partitions
> defined (in the cases I've looked at they end up using a fixed region
> anyway, so that could've just been made explicit in the device tree).
> 
> However, it also looks like all of the users that rely on the original
> behaviour where they can just access the full pool are those that don't
> define any reserved regions, whereas all users that do reserve regions
> will actually use those reserved regions.
> 
> So I think we can make use of this by differentiating in the driver
> between SRAM nodes with or without children and change the behaviour
> accordingly. I think that has the big advantage that it makes things
> work as (I think) most people would expect and doesn't further
> complicate the binding with extra flags.

I tend to agree on mapping partitions only if they exist. So far I could
only find one exception. It is arch/arm/boot/dts/armada-370.dtsi which is
using the top level pool as well as a partition to reserve 32 bytes at the
bottom of sram. This can be fixed along with the sram driver change, by
adding another partition for the rest of the sram and using its handle in
the crypto@90000 instead of top-level sram node handle. Do you see anymore
exceptions where both top level pool and the partitions both are being used?

Then on the backward compatibility topic, another issue is that boot code
could add sram nodes dynamically. For example arch/arm/mach-k3/common.c in
u-boot does it. This particular case will not break after the suggested change
because it is not adding any partitions. However, there could be other
boot-loaders which are not this lucky.

> 
> Thierry

/Yousaf

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Mian Yousaf Kaukab]

On Wed, May 20, 2020 at 10:55:58AM +0200, Thierry Reding wrote:
> On Tue, May 19, 2020 at 05:03:26PM -0600, Rob Herring wrote:
> > On Tue, May 19, 2020 at 10:16:43AM -0600, Stephen Warren wrote:
> > > On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> > > > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> > > >> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > > >>> Add documentation for the new optional flag added for SRAM driver.
> > > >>
> > > >>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> > > >>
> > > >>> +  reserved-only:
> > > >>> +    description:
> > > >>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > > >>> +      remapping type is selected depending upon no-memory-wc as usual.
> > > >>> +    type: boolean
> > > >>
> > > >> This feels a bit like a SW flag rather than a HW description, so I'm not
> > > >> sure it's appropriate to put it into DT.
> > > > 
> > > > Reserved regions themselves are software descriptions, no? Then we have 'pool'
> > > > flag which is again a software flag and so on. This flag falls into same
> > > > category and nothing out of ordinary.
> > > 
> > > I suppose that's true to some extent. This is indeed a description of
> > > the system environment presented to the SW that consumes the DT, which
> > > is a bit more than pure HW description but still a description of
> > > something imposed externally rather than describing something that's up
> > > to the discretion of the consuming SW. So, go ahead.
> > > 
> > > >> Are there any cases where the SW should map all of the SRAM, i.e. where
> > > >> we wouldn't expect to set reserved-only? [...]
> > > > 
> > > > Yes, here are a few examples:
> > > > arch/arm/boot/dts/aspeed-g*.dtsi
> > > > arch/arm/boot/dts/at91*.dtsi
> > > > arch/arm/boot/dts/bcm7445.dtsi
> > > > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> > > > except the reserved region.
> > > > 
> > > >> [...] I'd expect reserved-only to be
> > > >> the default, and perhaps only, mode of operation for the SRAM driver.
> > > > 
> > > > It will break compatibility with existing dtbs.
> > > > 
> > > >> If we can't do that because some SW currently expects to be able to map
> > > >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> > > >> SRAM driver which parts it's using, hence still allowing the driver to
> > > >> only map in-use portions?
> > > > 
> > > > User doesn’t need sram driver in that case. It can use genalloc api directly.
> > > 
> > > This sounds a bit odd. Without a driver for the reserved region, nothing
> > > should be touching it, since otherwise there's no code that owns an
> > > manages the region. If any code needs to consume the region, it should
> > > obtain info about the region from some form of provider code that can
> > > handle both the allocation and mapping. Anything else sounds like some
> > > consumer code directly making use of DT nodes it doesn't own. But since
> > > I'm not familiar enough with the SRAM driver and genalloc code that you
> > > mention to fully understand the allocation paths I guess I won't object
> > > for now, although it does still sound fishy.
> > 
> > I'm fine with the concept, but I don't think a single flag is adequate. 
> > If there are reserved regions within the SRAM, then define child nodes 
> > to mark those regions reserved. I don't think you need a new flag. Just 
> > a 'reg' property and nothing else.
> 
> It sounds to me like there are two different interpretations of SRAM and
> reserved regions. On one hand, as you suggest, we have one SRAM that's
> made available as genalloc pool and then individual regions can be
> marked as reserved so that they aren't added to that pool.
> 
> At the same time, each reserved region is also exposed as a separate
> pool and that's in fact used by many consumers as a way of getting a
> specific chunk of the SRAM for their own use (via phandle to the region
> from the consumer's device tree node).
> 
> In addition to that, the reserved region code doesn't actually fully do
> its job because while the reserved region isn't actually added to the
> "top-level" SRAM pool, the memory is still mapped. At the same time this
> is something that we actually want because, like I mentioned, some of
> the consumers do want to get at their SRAM chunks via references to the
> partitions.
> 
> The problem that this patch series is really trying to solve is another
> still: the complete SRAM is always mapped to kernel memory, irrespective
> of whether any regions are marked reserved or not and that can cause
> speculative accesses to memory outside of the defined regions.
> 
> Stephen's suggestion is to default to only mapping memory for which a
> partition has been defined in the SRAM and assuming that all SRAM
> outside of those partitions is off limits. I think that's a sensible
> default and it's unambiguous.
> 
> But as Yousaf points out that would break compatibility with existing
> device trees. Depending on how you interpret the bindings one could
> argue that those device trees are buggy and should have partitions
> defined (in the cases I've looked at they end up using a fixed region
> anyway, so that could've just been made explicit in the device tree).
> 
> However, it also looks like all of the users that rely on the original
> behaviour where they can just access the full pool are those that don't
> define any reserved regions, whereas all users that do reserve regions
> will actually use those reserved regions.
> 
> So I think we can make use of this by differentiating in the driver
> between SRAM nodes with or without children and change the behaviour
> accordingly. I think that has the big advantage that it makes things
> work as (I think) most people would expect and doesn't further
> complicate the binding with extra flags.

I tend to agree on mapping partitions only if they exist. So far I could
only find one exception. It is arch/arm/boot/dts/armada-370.dtsi which is
using the top level pool as well as a partition to reserve 32 bytes at the
bottom of sram. This can be fixed along with the sram driver change, by
adding another partition for the rest of the sram and using its handle in
the crypto@90000 instead of top-level sram node handle. Do you see anymore
exceptions where both top level pool and the partitions both are being used?

Then on the backward compatibility topic, another issue is that boot code
could add sram nodes dynamically. For example arch/arm/mach-k3/common.c in
u-boot does it. This particular case will not break after the suggested change
because it is not adding any partitions. However, there could be other
boot-loaders which are not this lucky.

> 
> Thierry

/Yousaf


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

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Mian Yousaf Kaukab]

On Wed, May 20, 2020 at 10:55:58AM +0200, Thierry Reding wrote:
> On Tue, May 19, 2020 at 05:03:26PM -0600, Rob Herring wrote:
> > On Tue, May 19, 2020 at 10:16:43AM -0600, Stephen Warren wrote:
> > > On 5/13/20 4:41 AM, Mian Yousaf Kaukab wrote:
> > > > On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> > > >> On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > > >>> Add documentation for the new optional flag added for SRAM driver.
> > > >>
> > > >>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> > > >>
> > > >>> +  reserved-only:
> > > >>> +    description:
> > > >>> +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > > >>> +      remapping type is selected depending upon no-memory-wc as usual.
> > > >>> +    type: boolean
> > > >>
> > > >> This feels a bit like a SW flag rather than a HW description, so I'm not
> > > >> sure it's appropriate to put it into DT.
> > > > 
> > > > Reserved regions themselves are software descriptions, no? Then we have 'pool'
> > > > flag which is again a software flag and so on. This flag falls into same
> > > > category and nothing out of ordinary.
> > > 
> > > I suppose that's true to some extent. This is indeed a description of
> > > the system environment presented to the SW that consumes the DT, which
> > > is a bit more than pure HW description but still a description of
> > > something imposed externally rather than describing something that's up
> > > to the discretion of the consuming SW. So, go ahead.
> > > 
> > > >> Are there any cases where the SW should map all of the SRAM, i.e. where
> > > >> we wouldn't expect to set reserved-only? [...]
> > > > 
> > > > Yes, here are a few examples:
> > > > arch/arm/boot/dts/aspeed-g*.dtsi
> > > > arch/arm/boot/dts/at91*.dtsi
> > > > arch/arm/boot/dts/bcm7445.dtsi
> > > > Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> > > > except the reserved region.
> > > > 
> > > >> [...] I'd expect reserved-only to be
> > > >> the default, and perhaps only, mode of operation for the SRAM driver.
> > > > 
> > > > It will break compatibility with existing dtbs.
> > > > 
> > > >> If we can't do that because some SW currently expects to be able to map
> > > >> arbitrary portions of the SRAM, shouldn't that SW be fixed to tell the
> > > >> SRAM driver which parts it's using, hence still allowing the driver to
> > > >> only map in-use portions?
> > > > 
> > > > User doesn’t need sram driver in that case. It can use genalloc api directly.
> > > 
> > > This sounds a bit odd. Without a driver for the reserved region, nothing
> > > should be touching it, since otherwise there's no code that owns an
> > > manages the region. If any code needs to consume the region, it should
> > > obtain info about the region from some form of provider code that can
> > > handle both the allocation and mapping. Anything else sounds like some
> > > consumer code directly making use of DT nodes it doesn't own. But since
> > > I'm not familiar enough with the SRAM driver and genalloc code that you
> > > mention to fully understand the allocation paths I guess I won't object
> > > for now, although it does still sound fishy.
> > 
> > I'm fine with the concept, but I don't think a single flag is adequate. 
> > If there are reserved regions within the SRAM, then define child nodes 
> > to mark those regions reserved. I don't think you need a new flag. Just 
> > a 'reg' property and nothing else.
> 
> It sounds to me like there are two different interpretations of SRAM and
> reserved regions. On one hand, as you suggest, we have one SRAM that's
> made available as genalloc pool and then individual regions can be
> marked as reserved so that they aren't added to that pool.
> 
> At the same time, each reserved region is also exposed as a separate
> pool and that's in fact used by many consumers as a way of getting a
> specific chunk of the SRAM for their own use (via phandle to the region
> from the consumer's device tree node).
> 
> In addition to that, the reserved region code doesn't actually fully do
> its job because while the reserved region isn't actually added to the
> "top-level" SRAM pool, the memory is still mapped. At the same time this
> is something that we actually want because, like I mentioned, some of
> the consumers do want to get at their SRAM chunks via references to the
> partitions.
> 
> The problem that this patch series is really trying to solve is another
> still: the complete SRAM is always mapped to kernel memory, irrespective
> of whether any regions are marked reserved or not and that can cause
> speculative accesses to memory outside of the defined regions.
> 
> Stephen's suggestion is to default to only mapping memory for which a
> partition has been defined in the SRAM and assuming that all SRAM
> outside of those partitions is off limits. I think that's a sensible
> default and it's unambiguous.
> 
> But as Yousaf points out that would break compatibility with existing
> device trees. Depending on how you interpret the bindings one could
> argue that those device trees are buggy and should have partitions
> defined (in the cases I've looked at they end up using a fixed region
> anyway, so that could've just been made explicit in the device tree).
> 
> However, it also looks like all of the users that rely on the original
> behaviour where they can just access the full pool are those that don't
> define any reserved regions, whereas all users that do reserve regions
> will actually use those reserved regions.
> 
> So I think we can make use of this by differentiating in the driver
> between SRAM nodes with or without children and change the behaviour
> accordingly. I think that has the big advantage that it makes things
> work as (I think) most people would expect and doesn't further
> complicate the binding with extra flags.

I tend to agree on mapping partitions only if they exist. So far I could
only find one exception. It is arch/arm/boot/dts/armada-370.dtsi which is
using the top level pool as well as a partition to reserve 32 bytes at the
bottom of sram. This can be fixed along with the sram driver change, by
adding another partition for the rest of the sram and using its handle in
the crypto@90000 instead of top-level sram node handle. Do you see anymore
exceptions where both top level pool and the partitions both are being used?

Then on the backward compatibility topic, another issue is that boot code
could add sram nodes dynamically. For example arch/arm/mach-k3/common.c in
u-boot does it. This particular case will not break after the suggested change
because it is not adding any partitions. However, there could be other
boot-loaders which are not this lucky.

> 
> Thierry

/Yousaf

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3083 bytes --]

On Wed, May 13, 2020 at 12:41:27PM +0200, Mian Yousaf Kaukab wrote:
> On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> > On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > > Add documentation for the new optional flag added for SRAM driver.
> > 
> > > diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> > 
> > > +  reserved-only:
> > > +    description:
> > > +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > > +      remapping type is selected depending upon no-memory-wc as usual.
> > > +    type: boolean
> > 
> > This feels a bit like a SW flag rather than a HW description, so I'm not
> > sure it's appropriate to put it into DT.
> 
> Reserved regions themselves are software descriptions, no? Then we have 'pool'
> flag which is again a software flag and so on. This flag falls into same
> category and nothing out of ordinary.
> > 
> > Are there any cases where the SW should map all of the SRAM, i.e. where
> > we wouldn't expect to set reserved-only? [...]
> 
> Yes, here are a few examples:
> arch/arm/boot/dts/aspeed-g*.dtsi

Looking at the implementation of the sole user of this, which is in
drivers/fsi/fsi-master-ast-cf.c, it looks like this really should've
specified a partition because the driver basically goes on to allocate
a fixed 4 KiB region of memory anyway.

> arch/arm/boot/dts/at91*.dtsi

While these define SRAM nodes, I don't see them referenced anywhere.

> arch/arm/boot/dts/bcm7445.dtsi
> Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> except the reserved region.

The driver currently maps everything, so if this relies on this
particular reserved region not being mapped then that's already broken
anyway.

> > [...] I'd expect reserved-only to be
> > the default, and perhaps only, mode of operation for the SRAM driver.
> 
> It will break compatibility with existing dtbs.

Yes, that's a bit unfortunate. I think this driver may suffer from a
slightly ambiguous device tree binding and then people just trying to
fit it to their use-cases.

However, I think we could preserve DTB backwards-compatibility while at
the same time correcting course and establish some sort of consistency.

Looking at the examples that you've provided and others, there are two
classes of users: users that don't specify any partitions either use all
of the available SRAM exclusively or manually allocate some part of it,
whereas users that have specified partitions all seem to use only the
defined partitions.

Given that, I think what we could do is check if there are any child
nodes and if not, keep the existing behaviour of mapping the whole SRAM
area. For cases where child nodes exist we could decide to go with the
default that Stephen suggested and only map regions for which a child
node has been defined.

This should allow both categories of users to work the way that they
were probably expected to work.

Any thoughts?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3083 bytes --]

On Wed, May 13, 2020 at 12:41:27PM +0200, Mian Yousaf Kaukab wrote:
> On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> > On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > > Add documentation for the new optional flag added for SRAM driver.
> > 
> > > diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> > 
> > > +  reserved-only:
> > > +    description:
> > > +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > > +      remapping type is selected depending upon no-memory-wc as usual.
> > > +    type: boolean
> > 
> > This feels a bit like a SW flag rather than a HW description, so I'm not
> > sure it's appropriate to put it into DT.
> 
> Reserved regions themselves are software descriptions, no? Then we have 'pool'
> flag which is again a software flag and so on. This flag falls into same
> category and nothing out of ordinary.
> > 
> > Are there any cases where the SW should map all of the SRAM, i.e. where
> > we wouldn't expect to set reserved-only? [...]
> 
> Yes, here are a few examples:
> arch/arm/boot/dts/aspeed-g*.dtsi

Looking at the implementation of the sole user of this, which is in
drivers/fsi/fsi-master-ast-cf.c, it looks like this really should've
specified a partition because the driver basically goes on to allocate
a fixed 4 KiB region of memory anyway.

> arch/arm/boot/dts/at91*.dtsi

While these define SRAM nodes, I don't see them referenced anywhere.

> arch/arm/boot/dts/bcm7445.dtsi
> Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> except the reserved region.

The driver currently maps everything, so if this relies on this
particular reserved region not being mapped then that's already broken
anyway.

> > [...] I'd expect reserved-only to be
> > the default, and perhaps only, mode of operation for the SRAM driver.
> 
> It will break compatibility with existing dtbs.

Yes, that's a bit unfortunate. I think this driver may suffer from a
slightly ambiguous device tree binding and then people just trying to
fit it to their use-cases.

However, I think we could preserve DTB backwards-compatibility while at
the same time correcting course and establish some sort of consistency.

Looking at the examples that you've provided and others, there are two
classes of users: users that don't specify any partitions either use all
of the available SRAM exclusively or manually allocate some part of it,
whereas users that have specified partitions all seem to use only the
defined partitions.

Given that, I think what we could do is check if there are any child
nodes and if not, keep the existing behaviour of mapping the whole SRAM
area. For cases where child nodes exist we could decide to go with the
default that Stephen suggested and only map regions for which a child
node has been defined.

This should allow both categories of users to work the way that they
were probably expected to work.

Any thoughts?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 2/4] dt-bindings: sram: add documentation for reserved-only flag

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3083 bytes --]

On Wed, May 13, 2020 at 12:41:27PM +0200, Mian Yousaf Kaukab wrote:
> On Tue, May 12, 2020 at 01:45:28PM -0600, Stephen Warren wrote:
> > On 5/12/20 8:48 AM, Mian Yousaf Kaukab wrote:
> > > Add documentation for the new optional flag added for SRAM driver.
> > 
> > > diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> > 
> > > +  reserved-only:
> > > +    description:
> > > +      The flag indicating, that only SRAM reserved regions have to be remapped.
> > > +      remapping type is selected depending upon no-memory-wc as usual.
> > > +    type: boolean
> > 
> > This feels a bit like a SW flag rather than a HW description, so I'm not
> > sure it's appropriate to put it into DT.
> 
> Reserved regions themselves are software descriptions, no? Then we have 'pool'
> flag which is again a software flag and so on. This flag falls into same
> category and nothing out of ordinary.
> > 
> > Are there any cases where the SW should map all of the SRAM, i.e. where
> > we wouldn't expect to set reserved-only? [...]
> 
> Yes, here are a few examples:
> arch/arm/boot/dts/aspeed-g*.dtsi

Looking at the implementation of the sole user of this, which is in
drivers/fsi/fsi-master-ast-cf.c, it looks like this really should've
specified a partition because the driver basically goes on to allocate
a fixed 4 KiB region of memory anyway.

> arch/arm/boot/dts/at91*.dtsi

While these define SRAM nodes, I don't see them referenced anywhere.

> arch/arm/boot/dts/bcm7445.dtsi
> Then arch/arm/boot/dts/dra7.dtsi is an example where we should map everything
> except the reserved region.

The driver currently maps everything, so if this relies on this
particular reserved region not being mapped then that's already broken
anyway.

> > [...] I'd expect reserved-only to be
> > the default, and perhaps only, mode of operation for the SRAM driver.
> 
> It will break compatibility with existing dtbs.

Yes, that's a bit unfortunate. I think this driver may suffer from a
slightly ambiguous device tree binding and then people just trying to
fit it to their use-cases.

However, I think we could preserve DTB backwards-compatibility while at
the same time correcting course and establish some sort of consistency.

Looking at the examples that you've provided and others, there are two
classes of users: users that don't specify any partitions either use all
of the available SRAM exclusively or manually allocate some part of it,
whereas users that have specified partitions all seem to use only the
defined partitions.

Given that, I think what we could do is check if there are any child
nodes and if not, keep the existing behaviour of mapping the whole SRAM
area. For cases where child nodes exist we could decide to go with the
default that Stephen suggested and only map regions for which a child
node has been defined.

This should allow both categories of users to work the way that they
were probably expected to work.

Any thoughts?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/4] arm64: tegra186: add reserved-only flag in sysram node

[Mian Yousaf Kaukab]

bpmp driver has reserved regions in sysram. Since sysram is only used
by the bpmp driver, add ‘reserved-only’ flag to the sysram node so
that only regions used by the bpmp driver are remapped. Other regions
of the sysram may not be accessible to the kernel and a speculative
access to them can cause SError.

Signed-off-by: Mian Yousaf Kaukab <ykaukab-l3A5Bk7waGM@public.gmane.org>
---
 arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
index 58100fb9cd8b..07ce7e7b270c 100644
--- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
@@ -1219,6 +1219,7 @@ sysram@30000000 {
 		#address-cells = <2>;
 		#size-cells = <2>;
 		ranges = <0 0x0 0x0 0x30000000 0x0 0x50000>;
+		reserved-only;
 
 		cpu_bpmp_tx: shmem@4e000 {
 			compatible = "nvidia,tegra186-bpmp-shmem";
-- 
2.25.0

[PATCH 3/4] arm64: tegra186: add reserved-only flag in sysram node

[Mian Yousaf Kaukab]

bpmp driver has reserved regions in sysram. Since sysram is only used
by the bpmp driver, add ‘reserved-only’ flag to the sysram node so
that only regions used by the bpmp driver are remapped. Other regions
of the sysram may not be accessible to the kernel and a speculative
access to them can cause SError.

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
 arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
index 58100fb9cd8b..07ce7e7b270c 100644
--- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
@@ -1219,6 +1219,7 @@ sysram@30000000 {
 		#address-cells = <2>;
 		#size-cells = <2>;
 		ranges = <0 0x0 0x0 0x30000000 0x0 0x50000>;
+		reserved-only;
 
 		cpu_bpmp_tx: shmem@4e000 {
 			compatible = "nvidia,tegra186-bpmp-shmem";
-- 
2.25.0


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

[PATCH 3/4] arm64: tegra186: add reserved-only flag in sysram node

[Mian Yousaf Kaukab]

bpmp driver has reserved regions in sysram. Since sysram is only used
by the bpmp driver, add ‘reserved-only’ flag to the sysram node so
that only regions used by the bpmp driver are remapped. Other regions
of the sysram may not be accessible to the kernel and a speculative
access to them can cause SError.

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
 arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
index 58100fb9cd8b..07ce7e7b270c 100644
--- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
@@ -1219,6 +1219,7 @@ sysram@30000000 {
 		#address-cells = <2>;
 		#size-cells = <2>;
 		ranges = <0 0x0 0x0 0x30000000 0x0 0x50000>;
+		reserved-only;
 
 		cpu_bpmp_tx: shmem@4e000 {
 			compatible = "nvidia,tegra186-bpmp-shmem";
-- 
2.25.0

[PATCH 4/4] arm64: tegra194: add reserved-only flag in sysram node

[Mian Yousaf Kaukab]

bpmp driver has reserved regions in sysram. Since sysram is only used
by the bpmp driver, add ‘reserved-only’ flag to the sysram node so
that only regions used by the bpmp driver are remapped. Other regions
of the sysram may not be accessible to the kernel and a speculative
access to them can cause SError.

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
 arch/arm64/boot/dts/nvidia/tegra194.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index e1ae01c2d039..9a5276d9c2a6 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -1743,6 +1743,7 @@ sysram@40000000 {
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges = <0x0 0x0 0x40000000 0x50000>;
+		reserved-only;
 
 		cpu_bpmp_tx: shmem@4e000 {
 			compatible = "nvidia,tegra194-bpmp-shmem";
-- 
2.25.0

[PATCH 4/4] arm64: tegra194: add reserved-only flag in sysram node

[Mian Yousaf Kaukab]

bpmp driver has reserved regions in sysram. Since sysram is only used
by the bpmp driver, add ‘reserved-only’ flag to the sysram node so
that only regions used by the bpmp driver are remapped. Other regions
of the sysram may not be accessible to the kernel and a speculative
access to them can cause SError.

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
 arch/arm64/boot/dts/nvidia/tegra194.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index e1ae01c2d039..9a5276d9c2a6 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -1743,6 +1743,7 @@ sysram@40000000 {
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges = <0x0 0x0 0x40000000 0x50000>;
+		reserved-only;
 
 		cpu_bpmp_tx: shmem@4e000 {
 			compatible = "nvidia,tegra194-bpmp-shmem";
-- 
2.25.0


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