regmap: Fix regcache_sync generic implementation

PATCH Applied Dimitris Papastamos 2 participants 14 messages Sep 28, 2011

Subsystems (2)

Driver core Register map abstraction

lore

Share & tools

lore

Dimitris Papastamos

author

2011-09-28 10:43 UTC #0 lore raw

PATCH 1/7 regmap: Fix regcache_sync generic implementation

We want to use regmap_write() to actually write anything to the HW.

Signed-off-by Dimitris Papastamos <[email protected]>

---
 drivers/base/regmap/regcache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/base/regmap/regcache.c +1 -1

index 6b9efd9..5364dde 100644
@@ -242,7 +242,7 @@ int regcache_sync(struct regmap *map)
 			if (ret < 0)
 				goto out;
 			regcache_cache_bypass(map, true);
-			ret = regcache_write(map, i, val);
+			ret = regmap_write(map, i, val);
 			regcache_cache_bypass(map, false);
 			if (ret < 0)
 				goto out;

Signature · 1 line

1.7.6.4

Dimitris Papastamos

author

2011-09-28 10:43 UTC #1 lore raw

PATCH 2/7 regmap: Modify map->cache_bypass directly

In preperation for the upcoming patches, modify map->cache_bypass directly. The helper functions will grab an exclusive lock. Because we'll have acquired the same lock we need to avoid a deadlock.

Signed-off-by Dimitris Papastamos <[email protected]>

---
 drivers/base/regmap/regcache.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/base/regmap/regcache.c +2 -2

index 5364dde..f46e247 100644
@@ -241,9 +241,9 @@ int regcache_sync(struct regmap *map)
 			ret = regcache_read(map, i, &val);
 			if (ret < 0)
 				goto out;
-			regcache_cache_bypass(map, true);
+			map->cache_bypass = 1;
 			ret = regmap_write(map, i, val);
-			regcache_cache_bypass(map, false);
+			map->cache_bypass = 0;
 			if (ret < 0)
 				goto out;
 			dev_dbg(map->dev, "Synced register %#x, value %#x\n",

Signature · 1 line

1.7.6.4

Mark Brown

Applied

2011-09-28 12:57 UTC #2 lore raw

Re: [PATCH 2/7] regmap: Modify map->cache_bypass directly

Quoted 3 lines · Dimitris Papastamos

In preperation for the upcoming patches, modify map->cache_bypass directly. The helper functions will grab an exclusive lock. Because we'll have acquired the same lock we need to avoid a deadlock.

Applied, thanks.

Dimitris Papastamos

author

2011-09-28 10:44 UTC #3 lore raw

PATCH 3/7 regmap: Add a mutex to guard the sync operation

In reply to Dimitris Papastamos

Signed-off-by Dimitris Papastamos <[email protected]>

---
 drivers/base/regmap/internal.h |    1 +
 drivers/base/regmap/regcache.c |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

drivers/base/regmap/internal.h +1 -0

index 2d51b1b..a0b931d 100644
@@ -32,6 +32,7 @@ struct regmap_format {
 
 struct regmap {
 	struct mutex lock;
+	struct mutex sync_lock;
 
 	struct device *dev; /* Device we do I/O on */
 	void *work_buf;     /* Scratch buffer used to format I/O */

drivers/base/regmap/regcache.c +4 -0

index f46e247..b843b5f 100644
@@ -130,6 +130,8 @@ int regcache_init(struct regmap *map)
 	if (!map->max_register)
 		map->max_register = map->num_reg_defaults_raw;
 
+	mutex_init(&map->sync_lock);
+
 	if (map->cache_ops->init) {
 		dev_dbg(map->dev, "Initializing %s cache\n",
 			map->cache_ops->name);

@@ -230,6 +232,7 @@ int regcache_sync(struct regmap *map)
 
 	BUG_ON(!map->cache_ops);
 
+	mutex_lock(&map->sync_lock);
 	dev_dbg(map->dev, "Syncing %s cache\n",
 		map->cache_ops->name);
 	name = map->cache_ops->name;

@@ -254,6 +257,7 @@ int regcache_sync(struct regmap *map)
 	}
 out:
 	trace_regcache_sync(map->dev, name, "stop");
+	mutex_unlock(&map->sync_lock);
 
 	return ret;
 }

Signature · 1 line

1.7.6.4

Mark Brown

2011-09-28 12:52 UTC #4 lore raw

Re: [PATCH 3/7] regmap: Add a mutex to guard the sync operation

drivers/base/regmap/regcache.c int regcache_sync(struct regmap *map) · line 260

Quoted 9 lines · Dimitris Papastamos

drivers/base/regmap/regcache.c +2 -0

+	mutex_lock(&map->sync_lock);
 	dev_dbg(map->dev, "Syncing %s cache\n",
 		map->cache_ops->name);
 	name = map->cache_ops->name;

@@ -254,6 +257,7 @@ int regcache_sync(struct regmap *map)
 	}
 out:
 	trace_regcache_sync(map->dev, name, "stop");
+	mutex_unlock(&map->sync_lock);

Shouldn't we either be taking the regmap lock when doing the sync or otherwise guarding against something other than a cache sync?

Dimitris Papastamos

author

2011-09-28 14:24 UTC #5 lore raw

Re: [PATCH 3/7] regmap: Add a mutex to guard the sync operation

Quoted 14 lines · Mark Brown

drivers/base/regmap/regcache.c int regcache_sync(struct regmap *map) · line 260
Quoted 9 lines · Dimitris Papastamos
drivers/base/regmap/regcache.c +2 -0
+	mutex_lock(&map->sync_lock);
 	dev_dbg(map->dev, "Syncing %s cache\n",
 		map->cache_ops->name);
 	name = map->cache_ops->name;
@@ -254,6 +257,7 @@ int regcache_sync(struct regmap *map)
 	}
 out:
 	trace_regcache_sync(map->dev, name, "stop");
+	mutex_unlock(&map->sync_lock);
Shouldn't we either be taking the regmap lock when doing the sync or otherwise guarding against something other than a cache sync?

Em the main issue is that the sync() implementation will use regmap_write() which will grab map->lock. To avoid this we could have gone and used directly the lockless _regmap_write() but that's static. To be honest, it feels cleaner to have only 1 lock to guard the map so maybe we should get rid of map->sync_lock.

Mark Brown

2011-09-28 15:19 UTC #6 lore raw

Re: [PATCH 3/7] regmap: Add a mutex to guard the sync operation

Quoted 10 lines · Dimitris Papastamos

Quoted 2 lines · Mark Brown
Shouldn't we either be taking the regmap lock when doing the sync or otherwise guarding against something other than a cache sync?
Em the main issue is that the sync() implementation will use regmap_write() which will grab map->lock. To avoid this we could have gone and used directly the lockless _regmap_write() but that's static. To be honest, it feels cleaner to have only 1 lock to guard the map so maybe we should get rid of map->sync_lock.

Making _regmap_write() global seems fine, it's not an externally visible API but the cache code is part of the infrastructure.

Dimitris Papastamos

author

2011-09-28 15:52 UTC #7 lore raw

Re: [PATCH 3/7] regmap: Add a mutex to guard the sync operation

Quoted 6 lines · Mark Brown

Quoted 4 lines · Dimitris Papastamos
Quoted 2 lines · Mark Brown
Shouldn't we either be taking the regmap lock when doing the sync or otherwise guarding against something other than a cache sync?

Quoted 8 lines

Quoted 5 lines
Em the main issue is that the sync() implementation will use regmap_write() which will grab map->lock. To avoid this we could have gone and used directly the lockless _regmap_write() but that's static. To be honest, it feels cleaner to have only 1 lock to guard the map so maybe we should get rid of map->sync_lock.
Making _regmap_write() global seems fine, it's not an externally visible API but the cache code is part of the infrastructure.

Okay, so I'll go forth and revert the use of map->sync_lock and just grab map->lock.

Thanks,
Dimitris

Dimitris Papastamos

author

2011-09-28 10:44 UTC #8 lore raw

PATCH 4/7 regmap: Save/restore the bypass state upon syncing

In reply to Dimitris Papastamos

Signed-off-by Dimitris Papastamos <[email protected]>

---
 drivers/base/regmap/regcache.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

drivers/base/regmap/regcache.c +5 -0

index b843b5f..695802c 100644
@@ -229,10 +229,13 @@ int regcache_sync(struct regmap *map)
 	unsigned int val;
 	unsigned int i;
 	const char *name;
+	unsigned int bypass;
 
 	BUG_ON(!map->cache_ops);
 
 	mutex_lock(&map->sync_lock);
+	/* Remember the initial bypass state */
+	bypass = map->cache_bypass;
 	dev_dbg(map->dev, "Syncing %s cache\n",
 		map->cache_ops->name);
 	name = map->cache_ops->name;

@@ -257,6 +260,8 @@ int regcache_sync(struct regmap *map)
 	}
 out:
 	trace_regcache_sync(map->dev, name, "stop");
+	/* Restore the bypass state */
+	map->cache_bypass = bypass;
 	mutex_unlock(&map->sync_lock);
 
 	return ret;

Signature · 1 line

1.7.6.4

Dimitris Papastamos

author

2011-09-28 10:44 UTC #9 lore raw

PATCH 5/7 regmap: Grab the lock in regcache_cache_only()

In reply to Dimitris Papastamos

Signed-off-by Dimitris Papastamos <[email protected]>

---
 drivers/base/regmap/regcache.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/base/regmap/regcache.c +2 -0

index 695802c..8900897 100644
@@ -282,7 +282,9 @@ EXPORT_SYMBOL_GPL(regcache_sync);
  */
 void regcache_cache_only(struct regmap *map, bool enable)
 {
+	mutex_lock(&map->sync_lock);
 	map->cache_only = enable;
+	mutex_unlock(&map->sync_lock);
 }
 EXPORT_SYMBOL_GPL(regcache_cache_only);

Signature · 1 line

1.7.6.4

Mark Brown

Applied

2011-09-28 12:57 UTC #10 lore raw

Re: [PATCH 5/7] regmap: Grab the lock in regcache_cache_only()

Quoted 1 line · Dimitris Papastamos

Signed-off-by Dimitris Papastamos <[email protected]>

Applied, thanks.

Dimitris Papastamos

author

2011-09-28 10:45 UTC #11 lore raw

PATCH 6/7 regmap: Implement regcache_cache_bypass helper function

In reply to Dimitris Papastamos

Ensure we've got a function so users can enable/disable the cache bypass option.

Signed-off-by Dimitris Papastamos <[email protected]>

---
 drivers/base/regmap/regcache.c |   19 +++++++++++++++++++
 include/linux/regmap.h         |    1 +
 2 files changed, 20 insertions(+), 0 deletions(-)

drivers/base/regmap/regcache.c +19 -0

index 8900897..a3f466c 100644
@@ -288,6 +288,25 @@ void regcache_cache_only(struct regmap *map, bool enable)
 }
 EXPORT_SYMBOL_GPL(regcache_cache_only);
 
+/**
+ * regcache_cache_bypass: Put a register map into cache bypass mode
+ *
+ * @map: map to configure
+ * @cache_only: flag if changes should not be written to the hardware
+ *
+ * When a register map is marked with the cache bypass option, writes
+ * to the register map API will only update the hardware and not the
+ * the cache directly.  This is useful when syncing the cache back to
+ * the hardware.
+ */
+void regcache_cache_bypass(struct regmap *map, bool enable)
+{
+	mutex_lock(&map->sync_lock);
+	map->cache_bypass = enable;
+	mutex_unlock(&map->sync_lock);
+}
+EXPORT_SYMBOL_GPL(regcache_cache_bypass);
+
 bool regcache_set_val(void *base, unsigned int idx,
 		      unsigned int val, unsigned int word_size)
 {

include/linux/regmap.h +1 -0

index 76ac255..3daac2d 100644
@@ -142,5 +142,6 @@ int regmap_update_bits(struct regmap *map, unsigned int reg,
 
 int regcache_sync(struct regmap *map);
 void regcache_cache_only(struct regmap *map, bool enable);
+void regcache_cache_bypass(struct regmap *map, bool enable);
 
 #endif

Signature · 1 line

1.7.6.4

Dimitris Papastamos

author

2011-09-28 10:44 UTC #12 lore raw

PATCH 7/7 regmap: Ensure we scream if we enable cache bypass/only at the same time

In reply to Dimitris Papastamos

Signed-off-by Dimitris Papastamos <[email protected]>

---
 drivers/base/regmap/regcache.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

drivers/base/regmap/regcache.c +4 -0

index a3f466c..3d59f14 100644
@@ -282,6 +282,8 @@ EXPORT_SYMBOL_GPL(regcache_sync);
  */
 void regcache_cache_only(struct regmap *map, bool enable)
 {
+	BUG_ON(map->cache_bypass && enable);
+
 	mutex_lock(&map->sync_lock);
 	map->cache_only = enable;
 	mutex_unlock(&map->sync_lock);

@@ -301,6 +303,8 @@ EXPORT_SYMBOL_GPL(regcache_cache_only);
  */
 void regcache_cache_bypass(struct regmap *map, bool enable)
 {
+	BUG_ON(map->cache_only && enable);
+
 	mutex_lock(&map->sync_lock);
 	map->cache_bypass = enable;
 	mutex_unlock(&map->sync_lock);

Signature · 1 line

1.7.6.4

Mark Brown

Applied

2011-09-28 12:56 UTC #13 lore raw

In reply to Dimitris Papastamos

Quoted 2 lines · Dimitris Papastamos

We want to use regmap_write() to actually write anything to the HW.

Applied, thanks.