Add managed ioremap function for shared resources

PATCH In review Ladislav Michl 2 participants 9 messages Jan 24, 2018 – Jan 26, 2018

Subsystems (4)

Library code Memory technology devices (MTD) Driver core NAND flash

lore

Share & tools

lore

Ladislav Michl

author

2018-01-24 10:06 UTC #0 lore raw

v2 cover Add managed ioremap function for shared resources

Many drivers can simplified by using devm_ioremap_resource() instead of open coding its functionality. However, as pointed by Wei Yongjun, that function cannot be used when memory region is already taken. See previous discussion here:
https://www.spinics.net/lists/linux-pci/msg68495.html

To ease job of driver developers, new function for that purpose is implemented and its usage shown on davinci mtd driver.

Changes from previous version:
- moved function prototype in headers other way around (PATCH 1/3), the rest of patches was dropped.

Ladislav Michl (3):
devres: Move devm_ioremap_resource() out of device.h
devres: Add devm_ioremap_shared_resource()
mtd: nand: davinci: Use devm_ioremap_shared_resource()

 drivers/mtd/nand/davinci_nand.c | 24 +++++++-----------------
 include/linux/device.h          |  2 --
 include/linux/io.h              |  7 +++++++
 lib/devres.c                    | 22 ++++++++++++++--------
 4 files changed, 28 insertions(+), 27 deletions(-)

Signature · 1 line

2.15.1

Ladislav Michl

author

2018-01-24 10:06 UTC #1 lore raw

PATCH v2 1/3 devres: Move devm_ioremap_resource() out of device.h

Move devm_ioremap_resource() out of device.h into io.h to be consistent with similar APIs.

Signed-off-by Ladislav Michl <[email protected]>

---
Changes:
- v2: new patch

 include/linux/device.h | 2 --
 include/linux/io.h     | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

include/linux/device.h +0 -2

index 4d88b6b9cda9..c9fcee2f5b91 100644
@@ -683,8 +683,6 @@ extern unsigned long devm_get_free_pages(struct device *dev,
 					 gfp_t gfp_mask, unsigned int order);
 extern void devm_free_pages(struct device *dev, unsigned long addr);
 
-void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
-
 /* allows to add/remove a custom action to devres stack */
 int devm_add_action(struct device *dev, void (*action)(void *), void *data);
 void devm_remove_action(struct device *dev, void (*action)(void *), void *data);

include/linux/io.h +1 -0

index 32e30e8fb9db..2aea3363bfb2 100644
@@ -79,6 +79,7 @@ void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
 				   resource_size_t size);
 void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
 				   resource_size_t size);
+void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
 void devm_iounmap(struct device *dev, void __iomem *addr);
 int check_signature(const volatile void __iomem *io_addr,
 			const unsigned char *signature, int length);

Signature · 1 line

2.15.1

Ladislav Michl

author

2018-01-24 10:07 UTC #2 lore raw

PATCH v2 2/3 devres: Add devm_ioremap_shared_resource()

In reply to Ladislav Michl

Implement managed ioremap function for shared resources.

Signed-off-by Ladislav Michl <[email protected]>

---
Changes:
- v2: Rebased on top of PATCH v2 1/3

 include/linux/io.h |  8 +++++++-
 lib/devres.c       | 22 ++++++++++++++--------
 2 files changed, 21 insertions(+), 9 deletions(-)

include/linux/io.h +7 -1

index 2aea3363bfb2..2b9eb48e0f49 100644
@@ -79,10 +79,16 @@ void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
 				   resource_size_t size);
 void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
 				   resource_size_t size);
-void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
+#define devm_ioremap_resource(dev, res) \
+	__devm_ioremap_resource(dev, res, false)
+#define devm_ioremap_shared_resource(dev, res) \
+	__devm_ioremap_resource(dev, res, true)
+void __iomem *__devm_ioremap_resource(struct device *dev, struct resource *res,
+				      bool shared);
 void devm_iounmap(struct device *dev, void __iomem *addr);
 int check_signature(const volatile void __iomem *io_addr,
 			const unsigned char *signature, int length);
+
 void devm_ioremap_release(struct device *dev, void *res);
 
 void *devm_memremap(struct device *dev, resource_size_t offset,

lib/devres.c +14 -8

index 5f2aedd58bc5..7711ff40a572 100644
@@ -22,6 +22,9 @@ static int devm_ioremap_match(struct device *dev, void *res, void *match_data)
  * @size: Size of map
  *
  * Managed ioremap().  Map is automatically unmapped on driver detach.
+ *
+ * When possible, use devm_ioremap_resource() or
+ * devm_ioremap_shared_resource() instead.
  */
 void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
 			   resource_size_t size)

@@ -116,13 +119,14 @@ void devm_iounmap(struct device *dev, void __iomem *addr)
 EXPORT_SYMBOL(devm_iounmap);
 
 /**
- * devm_ioremap_resource() - check, request region, and ioremap resource
+ * __devm_ioremap_resource() - check, request region, and ioremap resource
  * @dev: generic device to handle the resource for
  * @res: resource to be handled
+ * @shared: region is not requested when true
  *
- * Checks that a resource is a valid memory region, requests the memory
- * region and ioremaps it. All operations are managed and will be undone
- * on driver detach.
+ * Checks that a resource is a valid memory region, eventually requests the
+ * memory region and ioremaps it. All operations are managed and will be
+ * undone on driver detach.
  *
  * Returns a pointer to the remapped memory or an ERR_PTR() encoded error code
  * on failure. Usage example:

@@ -132,7 +136,8 @@ EXPORT_SYMBOL(devm_iounmap);
  *	if (IS_ERR(base))
  *		return PTR_ERR(base);
  */
-void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res)
+void __iomem *__devm_ioremap_resource(struct device *dev, struct resource *res,
+				      bool shared)
 {
 	resource_size_t size;
 	const char *name;

@@ -148,7 +153,7 @@ void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res)
 	size = resource_size(res);
 	name = res->name ?: dev_name(dev);
 
-	if (!devm_request_mem_region(dev, res->start, size, name)) {
+	if (!shared && !devm_request_mem_region(dev, res->start, size, name)) {
 		dev_err(dev, "can't request region for resource %pR\n", res);
 		return IOMEM_ERR_PTR(-EBUSY);
 	}

@@ -156,13 +161,14 @@ void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res)
 	dest_ptr = devm_ioremap(dev, res->start, size);
 	if (!dest_ptr) {
 		dev_err(dev, "ioremap failed for resource %pR\n", res);
-		devm_release_mem_region(dev, res->start, size);
+		if (!shared)
+			devm_release_mem_region(dev, res->start, size);
 		dest_ptr = IOMEM_ERR_PTR(-ENOMEM);
 	}
 
 	return dest_ptr;
 }
-EXPORT_SYMBOL(devm_ioremap_resource);
+EXPORT_SYMBOL(__devm_ioremap_resource);
 
 #ifdef CONFIG_HAS_IOPORT_MAP
 /*

Signature · 1 line

2.15.1

Andy Shevchenko

2018-01-24 16:21 UTC #3 lore raw

Re: [PATCH v2 2/3] devres: Add devm_ioremap_shared_resource()

include/linux/io.h void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset, · line 87

Quoted 8 lines · Ladislav Michl

Implement managed ioremap function for shared resources.

include/linux/io.h +6 -0

+#define devm_ioremap_resource(dev, res) \
+       __devm_ioremap_resource(dev, res, false)
+#define devm_ioremap_shared_resource(dev, res) \
+       __devm_ioremap_resource(dev, res, true)
+void __iomem *__devm_ioremap_resource(struct device *dev, struct resource *res,
+                                     bool shared);

I would rather do the following:

_resource() -> _resource_exclusive()

#define _resource() _resource_exclusive()

Add _resource_shared()

And use long names below in this file whenever refer to exclusive or shared variant.

Quoted 5 lines

void devm_iounmap(struct device *dev, void __iomem *addr); int check_signature(const volatile void __iomem *io_addr,
                        const unsigned char *signature, int length);
+
void devm_ioremap_release(struct device *dev, void *res);

This part doesn't belong to the change.

Quoted 8 lines

+ * When possible, use devm_ioremap_resource() or
+ * devm_ioremap_shared_resource() instead.
- * Checks that a resource is a valid memory region, requests the memory
- * region and ioremaps it. All operations are managed and will be undone
- * on driver detach.
+ * Checks that a resource is a valid memory region, eventually requests the
+ * memory region and ioremaps it. All operations are managed and will be
+ * undone on driver detach.

Wording is changed and no clue in commit message why.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Ladislav Michl

author

2018-01-24 17:15 UTC #4 lore raw

Re: [PATCH v2 2/3] devres: Add devm_ioremap_shared_resource()

Quoted 12 lines · Andy Shevchenko

include/linux/io.h void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset, · line 87
Quoted 8 lines · Ladislav Michl
Implement managed ioremap function for shared resources.
include/linux/io.h +6 -0
+#define devm_ioremap_resource(dev, res) \
+       __devm_ioremap_resource(dev, res, false)
+#define devm_ioremap_shared_resource(dev, res) \
+       __devm_ioremap_resource(dev, res, true)
+void __iomem *__devm_ioremap_resource(struct device *dev, struct resource *res,
+                                     bool shared);
I would rather do the following:

Quoted 8 lines

_resource() -> _resource_exclusive()
#define _resource() _resource_exclusive()
Add _resource_shared()
And use long names below in this file whenever refer to exclusive or shared variant.

Two separate functions were also considered, but I was unable to find small common implementation. If code size does not matter or you can provide a hint to make this solution small and nice, I'll go for it.

Quoted 17 lines

Quoted 5 lines
void devm_iounmap(struct device *dev, void __iomem *addr); int check_signature(const volatile void __iomem *io_addr,
                        const unsigned char *signature, int length);
+
void devm_ioremap_release(struct device *dev, void *res);
This part doesn't belong to the change.
Quoted 8 lines
+ * When possible, use devm_ioremap_resource() or
+ * devm_ioremap_shared_resource() instead.
- * Checks that a resource is a valid memory region, requests the memory
- * region and ioremaps it. All operations are managed and will be undone
- * on driver detach.
+ * Checks that a resource is a valid memory region, eventually requests the
+ * memory region and ioremaps it. All operations are managed and will be
+ * undone on driver detach.

Quoted 1 line

Wording is changed and no clue in commit message why.

Above will be moved to separate patch.

ladis

Andy Shevchenko

2018-01-26 18:52 UTC #5 lore raw

Re: [PATCH v2 2/3] devres: Add devm_ioremap_shared_resource()

Quoted 15 lines · Ladislav Michl

Quoted 13 lines · Andy Shevchenko
include/linux/io.h void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset, · line 87
Quoted 6 lines
include/linux/io.h +6 -0
+#define devm_ioremap_resource(dev, res) \
+       __devm_ioremap_resource(dev, res, false)
+#define devm_ioremap_shared_resource(dev, res) \
+       __devm_ioremap_resource(dev, res, true)
+void __iomem *__devm_ioremap_resource(struct device *dev, struct resource *res,
+                                     bool shared);
I would rather do the following:
_resource() -> _resource_exclusive()
#define _resource() _resource_exclusive()

Quoted 7 lines

Quoted 4 lines
Add _resource_shared()
And use long names below in this file whenever refer to exclusive or shared variant.
Two separate functions were also considered, but I was unable to find small common implementation.

Common part is still like you have done.

I'm talking about _naming_ scheme here.

Quoted 2 lines

If code size does not matter or you can provide a hint to make this solution small and nice, I'll go for it.

It would be the same as in your initial proposal, since I'm not talking about duplicating anything.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Ladislav Michl

author

2018-01-24 10:08 UTC #6 lore raw

PATCH v2 3/3 mtd: nand: davinci: Use devm_ioremap_shared_resource()

In reply to Ladislav Michl

Simplify error handling by using devm_ioremap_shared_resource().

Signed-off-by Ladislav Michl <[email protected]>

---
Changes:
- v2: None

 drivers/mtd/nand/davinci_nand.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

drivers/mtd/nand/davinci_nand.c +7 ~~-17~~

index ccc8c43abcff..9b6f06b177b9 100644
@@ -612,10 +612,8 @@ static int nand_davinci_probe(struct platform_device *pdev)
 {
 	struct davinci_nand_pdata	*pdata;
 	struct davinci_nand_info	*info;
-	struct resource			*res1;
-	struct resource			*res2;
+	struct resource			*res;
 	void __iomem			*vaddr;
-	void __iomem			*base;
 	int				ret;
 	uint32_t			val;
 	struct mtd_info			*mtd;

@@ -638,14 +636,8 @@ static int nand_davinci_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, info);
 
-	res1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	res2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	if (!res1 || !res2) {
-		dev_err(&pdev->dev, "resource missing\n");
-		return -EINVAL;
-	}
-
-	vaddr = devm_ioremap_resource(&pdev->dev, res1);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	vaddr = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(vaddr))
 		return PTR_ERR(vaddr);

@@ -655,14 +647,12 @@ static int nand_davinci_probe(struct platform_device *pdev)
 	 * by AEMIF, so we cannot request it twice, just ioremap.
 	 * The AEMIF and NAND drivers not use the same registers in this range.
 	 */
-	base = devm_ioremap(&pdev->dev, res2->start, resource_size(res2));
-	if (!base) {
-		dev_err(&pdev->dev, "ioremap failed for resource %pR\n", res2);
-		return -EADDRNOTAVAIL;
-	}
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	info->base = devm_ioremap_shared_resource(&pdev->dev, res);
+	if (IS_ERR(info->base))
+		return PTR_ERR(info->base);
 
 	info->dev		= &pdev->dev;
-	info->base		= base;
 	info->vaddr		= vaddr;
 
 	mtd			= nand_to_mtd(&info->chip);

Signature · 1 line

2.15.1

Andy Shevchenko

2018-01-24 16:22 UTC #7 lore raw

Re: [PATCH v2 3/3] mtd: nand: davinci: Use devm_ioremap_shared_resource()

drivers/mtd/nand/davinci_nand.c static int nand_davinci_probe(struct platform_device *pdev) · line 640

Quoted 2 lines · Ladislav Michl

drivers/mtd/nand/davinci_nand.c +2 -0

+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       vaddr = devm_ioremap_resource(&pdev->dev, res);

_resource_exclusive()

Quoted 1 line

+ info->base = devm_ioremap_shared_resource(&pdev->dev, res);

_resource_shared()

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

Reviewed-by on the v2 series

2018-01-24 16:23 UTC #8 lore raw

In reply to Ladislav Michl

Quoted 10 lines · Ladislav Michl

Many drivers can simplified by using devm_ioremap_resource() instead of open coding its functionality. However, as pointed by Wei Yongjun, that function cannot be used when memory region is already taken. See previous discussion here:
https://www.spinics.net/lists/linux-pci/msg68495.html
To ease job of driver developers, new function for that purpose is implemented and its usage shown on davinci mtd driver.

Sometimes we also need that, so, after addressing my comments FWIW

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 18 lines

Changes from previous version:
- moved function prototype in headers other way around (PATCH 1/3), the rest of patches was dropped.
Ladislav Michl (3):
devres: Move devm_ioremap_resource() out of device.h
devres: Add devm_ioremap_shared_resource()
mtd: nand: davinci: Use devm_ioremap_shared_resource()
 drivers/mtd/nand/davinci_nand.c | 24 +++++++-----------------
 include/linux/device.h          |  2 --
 include/linux/io.h              |  7 +++++++
 lib/devres.c                    | 22 ++++++++++++++--------
 4 files changed, 28 insertions(+), 27 deletions(-)
--
2.15.1

Signature · 2 lines

With Best Regards,
Andy Shevchenko