ARM: OMAP2+: fix gpmc_cs_remap: re-allocating chip-select address space based on DT

Each GPMC chip-select needs to be configured for (base-address,CS-size) so that GPMC understands the address-space allocated to device connected externally. These chip-select configurations (base-address, CS-size) follow some basic mapping rules like:
- The CS size is programmable from 256 MBytes to 16 MBytes (must be a power of 2) and is defined by the mask field. Attached memory smaller than the programmed CS region size is accessed through the entire CS region (aliasing).
- The programmed 'base-address' must be aligned to the 'CS-size' boundary and be a power of 2.
- Valid CS-size values are {256MB(max), 128MB, 64MB, 32MB and 16MB (min)} Any intermediate values creates holes in the chip-select memory-map.

This patch adds above checks in gpmc_cs_remap() so that any invalid value passed by DT <reg> property can be filtered before actually allocating the address space.

Signed-off-by Pekon Gupta <[email protected]>

---
 arch/arm/mach-omap2/gpmc.c | 42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

arch/arm/mach-omap2/gpmc.c +6 -12

index 8bc1338..4a4cc04 100644
@@ -521,26 +521,42 @@ static int gpmc_cs_delete_mem(int cs)
  * "base". Returns 0 on success and appropriate negative error code
  * on failure.
  */
-static int gpmc_cs_remap(int cs, u32 base)
+static int gpmc_cs_remap(int cs, u32 base, u32 size)
 {
 	int ret;
-	u32 old_base, size;
 
 	if (cs > gpmc_cs_num) {
 		pr_err("%s: requested chip-select is disabled\n", __func__);
 		return -ENODEV;
 	}
 
-	/*
-	 * Make sure we ignore any device offsets from the GPMC partition
-	 * allocated for the chip select and that the new base confirms
-	 * to the GPMC 16MB minimum granularity.
-	 */ 
-	base &= ~(SZ_16M - 1);
-
-	gpmc_cs_get_memconf(cs, &old_base, &size);
-	if (base == old_base)
-		return 0;
+	/* allocate enough address-space under GPMC chip-select to device */
+	if (size > SZ_256M) {
+		pr_err("%s: memory device > 256MB not supported\n", __func__);
+		return -ENODEV;
+	} else if (size > SZ_128M) {

arch/arm/mach-omap2/gpmc.c +9 -0

+		size = SZ_16M;
+	}
+
+	/* base address should be aligned with address-space size */
+	if (base & (size - 1)) {
+		pr_err("base-addr=%x should be aligned to size=%x", base, size);
+		return -EINVAL;
+	}
+
 	gpmc_cs_disable_mem(cs);
 	ret = gpmc_cs_delete_mem(cs);
 	if (ret < 0)

Quoted 5 lines · Pekon Gupta

arch/arm/mach-omap2/gpmc.c +5 -0

+     /* allocate enough address-space under GPMC chip-select to device */
+     if (size > SZ_256M) {
+             pr_err("%s: memory device > 256MB not supported\n", __func__);
+             return -ENODEV;
+     } else if (size > SZ_128M) {

arch/arm/mach-omap2/gpmc.c static int gpmc_cs_delete_mem(int cs) · line 547

Quoted 10 lines

arch/arm/mach-omap2/gpmc.c +10 -0

+             WARN((size != SZ_256M), "cs=%d: allocating 256MB\n", cs);
+             size = SZ_256M;
+     } else if (size > SZ_64M) {
+             WARN((size != SZ_128M), "cs=%d: allocating 128MB\n", cs);
+             size = SZ_128M;
+     } else if (size > SZ_32M) {
+             WARN((size != SZ_64M), "cs=%d: allocating 64MB\n", cs);
+             size = SZ_64M;
+     } else if (size > SZ_16M) {
+             WARN((size != SZ_32M), "cs=%d: allocating 64MB\n", cs);

Print message should be "allocating 32MB"

Otherwise it is fine. I can make the changes and resend.

cheers,
-roger

Each GPMC chip-select needs to be configured for (base-address,CS-size) so that GPMC understands the address-space allocated to device connected externally. These chip-select configurations (base-address, CS-size) follow some basic mapping rules like:
- The CS size is programmable from 256 MBytes to 16 MBytes (must be a power of 2) and is defined by the mask field. Attached memory smaller than the programmed CS region size is accessed through the entire CS region (aliasing).
- The programmed 'base-address' must be aligned to the 'CS-size' boundary and be a power of 2.
- Valid CS-size values are {256MB(max), 128MB, 64MB, 32MB and 16MB (min)} Any intermediate values creates holes in the chip-select memory-map.

This patch adds above checks in gpmc_cs_remap() so that any invalid value passed by DT <reg> property can be filtered before actually allocating the address space.

Signed-off-by Pekon Gupta <[email protected]>

---
 arch/arm/mach-omap2/gpmc.c | 42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

arch/arm/mach-omap2/gpmc.c +29 -13

index 8bc1338..4a4cc04 100644
@@ -521,26 +521,42 @@ static int gpmc_cs_delete_mem(int cs)
  * "base". Returns 0 on success and appropriate negative error code
  * on failure.
  */
-static int gpmc_cs_remap(int cs, u32 base)
+static int gpmc_cs_remap(int cs, u32 base, u32 size)
 {
 	int ret;
-	u32 old_base, size;
 
 	if (cs > gpmc_cs_num) {
 		pr_err("%s: requested chip-select is disabled\n", __func__);
 		return -ENODEV;
 	}
 
-	/*
-	 * Make sure we ignore any device offsets from the GPMC partition
-	 * allocated for the chip select and that the new base confirms
-	 * to the GPMC 16MB minimum granularity.
-	 */ 
-	base &= ~(SZ_16M - 1);
-
-	gpmc_cs_get_memconf(cs, &old_base, &size);
-	if (base == old_base)
-		return 0;
+	/* allocate enough address-space under GPMC chip-select to device */
+	if (size > SZ_256M) {
+		pr_err("%s: memory device > 256MB not supported\n", __func__);
+		return -ENODEV;
+	} else if (size > SZ_128M) {
+		WARN((size != SZ_256M), "cs=%d: allocating 256MB\n", cs);
+		size = SZ_256M;
+	} else if (size > SZ_64M) {
+		WARN((size != SZ_128M), "cs=%d: allocating 128MB\n", cs);
+		size = SZ_128M;
+	} else if (size > SZ_32M) {
+		WARN((size != SZ_64M), "cs=%d: allocating 64MB\n", cs);
+		size = SZ_64M;
+	} else if (size > SZ_16M) {
+		WARN((size != SZ_32M), "cs=%d: allocating 64MB\n", cs);
+		size = SZ_32M;
+	} else {
+		WARN((size != SZ_16M), "cs=%d: allocating 64MB\n", cs);
+		size = SZ_16M;
+	}
+
+	/* base address should be aligned with address-space size */
+	if (base & (size - 1)) {
+		pr_err("base-addr=%x should be aligned to size=%x", base, size);
+		return -EINVAL;
+	}
+
 	gpmc_cs_disable_mem(cs);
 	ret = gpmc_cs_delete_mem(cs);
 	if (ret < 0)

@@ -1551,7 +1567,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 	 * CS to this location. Once DT migration is complete should
 	 * just make gpmc_cs_request() map a specific address.
 	 */
-	ret = gpmc_cs_remap(cs, res.start);
+	ret = gpmc_cs_remap(cs, res.start, resource_size(&res));
 	if (ret < 0) {
 		dev_err(&pdev->dev, "cannot remap GPMC CS %d to %pa\n",
 			cs, &res.start);

Signature · 1 line

1.8.5.1.163.gd7aced9

Quoted 12 lines · Pekon Gupta

Each GPMC chip-select needs to be configured for (base-address,CS-size) so that GPMC understands the address-space allocated to device connected externally. These chip-select configurations (base-address, CS-size) follow some basic mapping rules like:
- The CS size is programmable from 256 MBytes to 16 MBytes (must be a power of 2) and is defined by the mask field. Attached memory smaller than the programmed CS region size is accessed through the entire CS region (aliasing).
- The programmed 'base-address' must be aligned to the 'CS-size' boundary and be a power of 2.
- Valid CS-size values are {256MB(max), 128MB, 64MB, 32MB and 16MB (min)} Any intermediate values creates holes in the chip-select memory-map.

Quoted 5 lines

This patch adds above checks in gpmc_cs_remap() so that any invalid value passed by DT <reg> property can be filtered before actually allocating the address space.

Signed-off-by Pekon Gupta <[email protected]>

Looks like size typos Roger mentioned are fixed in this one, so applying into omap-for-v3.17/fixes thanks.

Quoted 76 lines

---
 arch/arm/mach-omap2/gpmc.c | 42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

arch/arm/mach-omap2/gpmc.c +29 -13

index 8bc1338..4a4cc04 100644
@@ -521,26 +521,42 @@ static int gpmc_cs_delete_mem(int cs)
  * "base". Returns 0 on success and appropriate negative error code
  * on failure.
  */
-static int gpmc_cs_remap(int cs, u32 base)
+static int gpmc_cs_remap(int cs, u32 base, u32 size)
 {
 	int ret;
-	u32 old_base, size;
 
 	if (cs > gpmc_cs_num) {
 		pr_err("%s: requested chip-select is disabled\n", __func__);
 		return -ENODEV;
 	}
 
-	/*
-	 * Make sure we ignore any device offsets from the GPMC partition
-	 * allocated for the chip select and that the new base confirms
-	 * to the GPMC 16MB minimum granularity.
-	 */ 
-	base &= ~(SZ_16M - 1);
-
-	gpmc_cs_get_memconf(cs, &old_base, &size);
-	if (base == old_base)
-		return 0;
+	/* allocate enough address-space under GPMC chip-select to device */
+	if (size > SZ_256M) {
+		pr_err("%s: memory device > 256MB not supported\n", __func__);
+		return -ENODEV;
+	} else if (size > SZ_128M) {
+		WARN((size != SZ_256M), "cs=%d: allocating 256MB\n", cs);
+		size = SZ_256M;
+	} else if (size > SZ_64M) {
+		WARN((size != SZ_128M), "cs=%d: allocating 128MB\n", cs);
+		size = SZ_128M;
+	} else if (size > SZ_32M) {
+		WARN((size != SZ_64M), "cs=%d: allocating 64MB\n", cs);
+		size = SZ_64M;
+	} else if (size > SZ_16M) {
+		WARN((size != SZ_32M), "cs=%d: allocating 64MB\n", cs);
+		size = SZ_32M;
+	} else {
+		WARN((size != SZ_16M), "cs=%d: allocating 64MB\n", cs);
+		size = SZ_16M;
+	}
+
+	/* base address should be aligned with address-space size */
+	if (base & (size - 1)) {
+		pr_err("base-addr=%x should be aligned to size=%x", base, size);
+		return -EINVAL;
+	}
+
 	gpmc_cs_disable_mem(cs);
 	ret = gpmc_cs_delete_mem(cs);
 	if (ret < 0)

@@ -1551,7 +1567,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 	 * CS to this location. Once DT migration is complete should
 	 * just make gpmc_cs_request() map a specific address.
 	 */
-	ret = gpmc_cs_remap(cs, res.start);
+	ret = gpmc_cs_remap(cs, res.start, resource_size(&res));
 	if (ret < 0) {
 		dev_err(&pdev->dev, "cannot remap GPMC CS %d to %pa\n",
 			cs, &res.start);

Signature · 1 line

1.8.5.1.163.gd7aced9

Hi Tony,

Quoted 17 lines · Tony Lindgren

Quoted 16 lines · Pekon Gupta

Each GPMC chip-select needs to be configured for (base-address,CS-size) so that GPMC understands the address-space allocated to device connected externally. These chip-select configurations (base-address, CS-size) follow some basic mapping rules like:
- The CS size is programmable from 256 MBytes to 16 MBytes (must be a power of 2) and is defined by the mask field. Attached memory smaller than the programmed CS region size is accessed through the entire CS region (aliasing).
- The programmed 'base-address' must be aligned to the 'CS-size' boundary and be a power of 2.
- Valid CS-size values are {256MB(max), 128MB, 64MB, 32MB and 16MB (min)} Any intermediate values creates holes in the chip-select memory-map.

This patch adds above checks in gpmc_cs_remap() so that any invalid value passed by DT <reg> property can be filtered before actually allocating the address space.

Quoted 4 lines

Quoted 1 line

Signed-off-by Pekon Gupta <[email protected]>

Looks like size typos Roger mentioned are fixed in this one, so applying into omap-for-v3.17/fixes thanks.

I don't see the typos fixed here. I'll reply with a v2 patch.

Quoted 22 lines · Roger Quadros

Hi Tony,

Quoted 19 lines · Tony Lindgren

Quoted 17 lines · Pekon Gupta

Each GPMC chip-select needs to be configured for (base-address,CS-size) so that GPMC understands the address-space allocated to device connected externally. These chip-select configurations (base-address, CS-size) follow some basic mapping rules like:
- The CS size is programmable from 256 MBytes to 16 MBytes (must be a power of 2) and is defined by the mask field. Attached memory smaller than the programmed CS region size is accessed through the entire CS region (aliasing).
- The programmed 'base-address' must be aligned to the 'CS-size' boundary and be a power of 2.
- Valid CS-size values are {256MB(max), 128MB, 64MB, 32MB and 16MB (min)} Any intermediate values creates holes in the chip-select memory-map.

This patch adds above checks in gpmc_cs_remap() so that any invalid value passed by DT <reg> property can be filtered before actually allocating the address space.

Signed-off-by Pekon Gupta <[email protected]>

Quoted 4 lines

Quoted 2 lines

Looks like size typos Roger mentioned are fixed in this one, so applying into omap-for-v3.17/fixes thanks.

I don't see the typos fixed here. I'll reply with a v2 patch.

Oops indeed. I'll redo the fixes branch to avoid extra churn and apply your updated version into omap-for-v3.17/fixes-v2.

From: Pekon Gupta <[email protected]>

Each GPMC chip-select needs to be configured for (base-address,CS-size) so that GPMC understands the address-space allocated to device connected externally. These chip-select configurations (base-address, CS-size) follow some basic mapping rules like:
- The CS size is programmable from 256 MBytes to 16 MBytes (must be a power of 2) and is defined by the mask field. Attached memory smaller than the programmed CS region size is accessed through the entire CS region (aliasing).
- The programmed 'base-address' must be aligned to the 'CS-size' boundary and be a power of 2.
- Valid CS-size values are {256MB(max), 128MB, 64MB, 32MB and 16MB (min)} Any intermediate values creates holes in the chip-select memory-map.

This patch adds above checks in gpmc_cs_remap() so that any invalid value passed by DT <reg> property can be filtered before actually allocating the address space.

[[email protected]] typo and print message fixes.

Signed-off-by Pekon Gupta <[email protected]>

Signed-off-by Roger Quadros <[email protected]>

---
 arch/arm/mach-omap2/gpmc.c | 51 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 14 deletions(-)

arch/arm/mach-omap2/gpmc.c +37 -14

index 8bc1338..793f3a9 100644
@@ -516,31 +516,54 @@ static int gpmc_cs_delete_mem(int cs)
  * gpmc_cs_remap - remaps a chip-select physical base address
  * @cs:		chip-select to remap
  * @base:	physical base address to re-map chip-select to
+ * @size:	size of the chip select map
  *
  * Re-maps a chip-select to a new physical base address specified by
  * "base". Returns 0 on success and appropriate negative error code
- * on failure.
+ * on failure. "size" of the map must be either 16M, 32M, 64M or 128M.
  */
-static int gpmc_cs_remap(int cs, u32 base)
+static int gpmc_cs_remap(int cs, u32 base, u32 size)
 {
 	int ret;
-	u32 old_base, size;
 
 	if (cs > gpmc_cs_num) {
 		pr_err("%s: requested chip-select is disabled\n", __func__);
 		return -ENODEV;
 	}
 
-	/*
-	 * Make sure we ignore any device offsets from the GPMC partition
-	 * allocated for the chip select and that the new base confirms
-	 * to the GPMC 16MB minimum granularity.
-	 */ 
-	base &= ~(SZ_16M - 1);
-
-	gpmc_cs_get_memconf(cs, &old_base, &size);
-	if (base == old_base)
-		return 0;
+	/* Align size to meet SoC limitations */
+	if (size > SZ_256M) {
+		pr_err("%s: CS memory map > 256MB not supported\n", __func__);
+		return -ENODEV;
+	} else if (size > SZ_128M) {
+		WARN((size != SZ_256M), "%s: cs=%d: allocating 256MB\n",
+		     __func__, cs);
+		size = SZ_256M;
+	} else if (size > SZ_64M) {
+		WARN((size != SZ_128M), "%s: cs=%d: allocating 128MB\n",
+		     __func__, cs);
+		size = SZ_128M;
+	} else if (size > SZ_32M) {
+		WARN((size != SZ_64M), "%s: cs=%d: allocating 64MB\n",
+		     __func__, cs);
+		size = SZ_64M;
+	} else if (size > SZ_16M) {
+		WARN((size != SZ_32M), "%s: cs=%d: allocating 32MB\n",
+		     __func__, cs);
+		size = SZ_32M;
+	} else {
+		WARN((size != SZ_16M), "%s: cs=%d: allocating 16MB\n",
+		     __func__, cs);
+		size = SZ_16M;
+	}
+
+	/* base address should be aligned with address-space size */
+	if (base & (size - 1)) {
+		pr_err("%s: cs base-addr: %x should be aligned to cs size: %x",
+		       __func__, base, size);
+		return -EINVAL;
+	}
+
 	gpmc_cs_disable_mem(cs);
 	ret = gpmc_cs_delete_mem(cs);
 	if (ret < 0)

@@ -1551,7 +1574,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 	 * CS to this location. Once DT migration is complete should
 	 * just make gpmc_cs_request() map a specific address.
 	 */
-	ret = gpmc_cs_remap(cs, res.start);
+	ret = gpmc_cs_remap(cs, res.start, resource_size(&res));
 	if (ret < 0) {
 		dev_err(&pdev->dev, "cannot remap GPMC CS %d to %pa\n",
 			cs, &res.start);

Signature · 1 line

1.8.3.2

Quoted 14 lines · Roger Quadros

From: Pekon Gupta <[email protected]>

Each GPMC chip-select needs to be configured for (base-address,CS-size) so that GPMC understands the address-space allocated to device connected externally. These chip-select configurations (base-address, CS-size) follow some basic mapping rules like:
- The CS size is programmable from 256 MBytes to 16 MBytes (must be a power of 2) and is defined by the mask field. Attached memory smaller than the programmed CS region size is accessed through the entire CS region (aliasing).
- The programmed 'base-address' must be aligned to the 'CS-size' boundary and be a power of 2.
- Valid CS-size values are {256MB(max), 128MB, 64MB, 32MB and 16MB (min)} Any intermediate values creates holes in the chip-select memory-map.

Quoted 3 lines

This patch adds above checks in gpmc_cs_remap() so that any invalid value passed by DT <reg> property can be filtered before actually allocating the address space.

There's now an issue here where it mixes up the configured CS size and the configured device IO size. With this patch GPMC based Ethernet devices trigger warning at arch/arm/mach-omap2/gpmc.c:556 as we have the minimal GPMC CS range of 16MB with the smsc IO size being either 0xf or 0xff depending on the model.

So that check should be done on the CS size, not the device IO size.

Regards,

Tony

Quoted 92 lines

[[email protected]] typo and print message fixes.

Signed-off-by Pekon Gupta <[email protected]>

Signed-off-by Roger Quadros <[email protected]>

---
 arch/arm/mach-omap2/gpmc.c | 51 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 14 deletions(-)

arch/arm/mach-omap2/gpmc.c +37 -14

index 8bc1338..793f3a9 100644
@@ -516,31 +516,54 @@ static int gpmc_cs_delete_mem(int cs)
  * gpmc_cs_remap - remaps a chip-select physical base address
  * @cs:		chip-select to remap
  * @base:	physical base address to re-map chip-select to
+ * @size:	size of the chip select map
  *
  * Re-maps a chip-select to a new physical base address specified by
  * "base". Returns 0 on success and appropriate negative error code
- * on failure.
+ * on failure. "size" of the map must be either 16M, 32M, 64M or 128M.
  */
-static int gpmc_cs_remap(int cs, u32 base)
+static int gpmc_cs_remap(int cs, u32 base, u32 size)
 {
 	int ret;
-	u32 old_base, size;
 
 	if (cs > gpmc_cs_num) {
 		pr_err("%s: requested chip-select is disabled\n", __func__);
 		return -ENODEV;
 	}
 
-	/*
-	 * Make sure we ignore any device offsets from the GPMC partition
-	 * allocated for the chip select and that the new base confirms
-	 * to the GPMC 16MB minimum granularity.
-	 */ 
-	base &= ~(SZ_16M - 1);
-
-	gpmc_cs_get_memconf(cs, &old_base, &size);
-	if (base == old_base)
-		return 0;
+	/* Align size to meet SoC limitations */
+	if (size > SZ_256M) {
+		pr_err("%s: CS memory map > 256MB not supported\n", __func__);
+		return -ENODEV;
+	} else if (size > SZ_128M) {
+		WARN((size != SZ_256M), "%s: cs=%d: allocating 256MB\n",
+		     __func__, cs);
+		size = SZ_256M;
+	} else if (size > SZ_64M) {
+		WARN((size != SZ_128M), "%s: cs=%d: allocating 128MB\n",
+		     __func__, cs);
+		size = SZ_128M;
+	} else if (size > SZ_32M) {
+		WARN((size != SZ_64M), "%s: cs=%d: allocating 64MB\n",
+		     __func__, cs);
+		size = SZ_64M;
+	} else if (size > SZ_16M) {
+		WARN((size != SZ_32M), "%s: cs=%d: allocating 32MB\n",
+		     __func__, cs);
+		size = SZ_32M;
+	} else {
+		WARN((size != SZ_16M), "%s: cs=%d: allocating 16MB\n",
+		     __func__, cs);
+		size = SZ_16M;
+	}
+
+	/* base address should be aligned with address-space size */
+	if (base & (size - 1)) {
+		pr_err("%s: cs base-addr: %x should be aligned to cs size: %x",
+		       __func__, base, size);
+		return -EINVAL;
+	}
+
 	gpmc_cs_disable_mem(cs);
 	ret = gpmc_cs_delete_mem(cs);
 	if (ret < 0)

@@ -1551,7 +1574,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 	 * CS to this location. Once DT migration is complete should
 	 * just make gpmc_cs_request() map a specific address.
 	 */
-	ret = gpmc_cs_remap(cs, res.start);
+	ret = gpmc_cs_remap(cs, res.start, resource_size(&res));
 	if (ret < 0) {
 		dev_err(&pdev->dev, "cannot remap GPMC CS %d to %pa\n",
 			cs, &res.start);

Signature · 1 line

1.8.3.2