diff mbox series

[1/2] regmap: Add hardware spinlock support

Message ID bf9fd59f32f5c093598d5078d681967626ee9fd0.1509425993.git.baolin.wang@linaro.org
State New
Headers show
Series [1/2] regmap: Add hardware spinlock support | expand

Commit Message

(Exiting) Baolin Wang Oct. 31, 2017, 6:35 a.m. UTC 
On some platforms, when reading or writing some special registers through
regmap, we should acquire one hardware spinlock to synchronize between
the multiple subsystems. Thus this patch adds the hardware spinlock
support for regmap.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

---
 drivers/base/regmap/internal.h |    3 +
 drivers/base/regmap/regmap.c   |  118 ++++++++++++++++++++++++++++++++++------
 include/linux/regmap.h         |    9 +++
 3 files changed, 114 insertions(+), 16 deletions(-)

-- 
1.7.9.5

Comments

Mark Brown Oct. 31, 2017, 10:38 a.m. UTC | #1 
On Tue, Oct 31, 2017 at 02:35:55PM +0800, Baolin Wang wrote:

This looks mostly good, a few small things:

> +static void regmap_lock_hwlock(void *__map)

> +{

> +	struct regmap *map = __map;

> +	int ret;

> +

> +	if (map->hwlock_timeout)

> +		ret = hwspin_lock_timeout(map->hwlock, map->hwlock_timeout);

> +	else

> +		ret = hwspin_trylock(map->hwlock);

> +

> +	if (ret)

> +		dev_err(map->dev, "Failed to get hwlock %d\n", ret);

> +}


Given that we have no error handling path on the locks should we be
supporting timeout mode at all?  Otherwise we should probably add a
set of error handling paths whenever we take the lock...

> +		if (config->hwlock_mode == HWLOCK_IRQSTATE) {

> +			map->lock = regmap_lock_hwlock_irqsave;

> +			map->unlock = regmap_unlock_hwlock_irqrestore;

> +		} else if (config->hwlock_mode == HWLOCK_IRQ) {

> +			map->lock = regmap_lock_hwlock_irq;

> +			map->unlock = regmap_unlock_hwlock_irq;

> +		} else {

> +			map->lock = regmap_lock_hwlock;

> +			map->unlock = regmap_unlock_hwlock;

> +		}


This should be a switch statement.

> --- a/include/linux/regmap.h

> +++ b/include/linux/regmap.h

> @@ -19,6 +19,7 @@

>  #include <linux/err.h>

>  #include <linux/bug.h>

>  #include <linux/lockdep.h>

> +#include <linux/hwspinlock.h>


We don't actually use hwspinlock.h in the header (the config options are
all just unsigned ints) so this could be moved to regmap.c with a
forward declaration of the struct in internal.h.  That way we don't
force a rebuild of every regmap user when hwspinlock changes.
(Exiting) Baolin Wang Oct. 31, 2017, 11:31 a.m. UTC | #2 
Hi Mark,

On 31 October 2017 at 18:38, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Oct 31, 2017 at 02:35:55PM +0800, Baolin Wang wrote:

>

> This looks mostly good, a few small things:

>

>> +static void regmap_lock_hwlock(void *__map)

>> +{

>> +     struct regmap *map = __map;

>> +     int ret;

>> +

>> +     if (map->hwlock_timeout)

>> +             ret = hwspin_lock_timeout(map->hwlock, map->hwlock_timeout);

>> +     else

>> +             ret = hwspin_trylock(map->hwlock);

>> +

>> +     if (ret)

>> +             dev_err(map->dev, "Failed to get hwlock %d\n", ret);

>> +}

>

> Given that we have no error handling path on the locks should we be

> supporting timeout mode at all?  Otherwise we should probably add a

> set of error handling paths whenever we take the lock...


It will be more helpful to use the timeout to try more times to get
the hwlock, and we usually do not use hwspin_trylock_xxx(), so we can
remove hwspin_trylock_xxx() support and set timeout as MAX value as
default to avoid adding 'hwlock_timeout' config,
is this OK for you?

>

>> +             if (config->hwlock_mode == HWLOCK_IRQSTATE) {

>> +                     map->lock = regmap_lock_hwlock_irqsave;

>> +                     map->unlock = regmap_unlock_hwlock_irqrestore;

>> +             } else if (config->hwlock_mode == HWLOCK_IRQ) {

>> +                     map->lock = regmap_lock_hwlock_irq;

>> +                     map->unlock = regmap_unlock_hwlock_irq;

>> +             } else {

>> +                     map->lock = regmap_lock_hwlock;

>> +                     map->unlock = regmap_unlock_hwlock;

>> +             }

>

> This should be a switch statement.


Yes, will fix in next version.

>

>> --- a/include/linux/regmap.h

>> +++ b/include/linux/regmap.h

>> @@ -19,6 +19,7 @@

>>  #include <linux/err.h>

>>  #include <linux/bug.h>

>>  #include <linux/lockdep.h>

>> +#include <linux/hwspinlock.h>

>

> We don't actually use hwspinlock.h in the header (the config options are

> all just unsigned ints) so this could be moved to regmap.c with a

> forward declaration of the struct in internal.h.  That way we don't

> force a rebuild of every regmap user when hwspinlock changes.


You are right. I will more the hwspinlock.h to regmap.c file. Very
appreciated for your comments.

-- 
Baolin.wang
Best Regards
Mark Brown Oct. 31, 2017, 11:37 a.m. UTC | #3 
On Tue, Oct 31, 2017 at 07:31:57PM +0800, Baolin Wang wrote:
> On 31 October 2017 at 18:38, Mark Brown <broonie@kernel.org> wrote:


> > Given that we have no error handling path on the locks should we be

> > supporting timeout mode at all?  Otherwise we should probably add a

> > set of error handling paths whenever we take the lock...


> It will be more helpful to use the timeout to try more times to get

> the hwlock, and we usually do not use hwspin_trylock_xxx(), so we can

> remove hwspin_trylock_xxx() support and set timeout as MAX value as

> default to avoid adding 'hwlock_timeout' config,

> is this OK for you?


I *think* so - but let's see the code.  It might make sense to do two
patches, one with the base hwspinlock support then another adding the
timeout functionality.  That way if there's any problem we can still
merge the non-timeout code and there's less to review next time.
(Exiting) Baolin Wang Oct. 31, 2017, 11:47 a.m. UTC | #4 
On 31 October 2017 at 19:37, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Oct 31, 2017 at 07:31:57PM +0800, Baolin Wang wrote:

>> On 31 October 2017 at 18:38, Mark Brown <broonie@kernel.org> wrote:

>

>> > Given that we have no error handling path on the locks should we be

>> > supporting timeout mode at all?  Otherwise we should probably add a

>> > set of error handling paths whenever we take the lock...

>

>> It will be more helpful to use the timeout to try more times to get

>> the hwlock, and we usually do not use hwspin_trylock_xxx(), so we can

>> remove hwspin_trylock_xxx() support and set timeout as MAX value as

>> default to avoid adding 'hwlock_timeout' config,

>> is this OK for you?

>

> I *think* so - but let's see the code.  It might make sense to do two

> patches, one with the base hwspinlock support then another adding the

> timeout functionality.  That way if there's any problem we can still

> merge the non-timeout code and there's less to review next time.


What I mean is we only introduce the timeout functions, since we do
not want to get the hwlocks failed to avoid error handling path:

static void regmap_lock_hwlock(void *__map)
{
    struct regmap *map = __map;

    hwspin_lock_timeout(map->hwlock, ~0U);
}

static void regmap_lock_hwlock_irq(void *__map)
{
    struct regmap *map = __map;

    hwspin_lock_timeout_irq(map->hwlock, ~0U);
}

static void regmap_lock_hwlock_irqsave(void *__map)
{
    struct regmap *map = __map;

    hwspin_lock_timeout_irqsave(map->hwlock, ~0U, &map->spinlock_flags);
}

-- 
Baolin.wang
Best Regards
Mark Brown Oct. 31, 2017, 7:43 p.m. UTC | #5 
On Tue, Oct 31, 2017 at 07:47:57PM +0800, Baolin Wang wrote:

> What I mean is we only introduce the timeout functions, since we do

> not want to get the hwlocks failed to avoid error handling path:


OK.
diff mbox series

Patch

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 2a4435d..bb8b663 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -157,6 +157,9 @@  struct regmap {
 
 	struct rb_root range_tree;
 	void *selector_work_buf;	/* Scratch buffer used for selector */
+
+	struct hwspinlock *hwlock;
+	unsigned int hwlock_timeout;
 };
 
 struct regcache_ops {
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index b9a779a..615e8d0 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -413,6 +413,71 @@  static unsigned int regmap_parse_64_native(const void *buf)
 }
 #endif
 
+static void regmap_lock_hwlock(void *__map)
+{
+	struct regmap *map = __map;
+	int ret;
+
+	if (map->hwlock_timeout)
+		ret = hwspin_lock_timeout(map->hwlock, map->hwlock_timeout);
+	else
+		ret = hwspin_trylock(map->hwlock);
+
+	if (ret)
+		dev_err(map->dev, "Failed to get hwlock %d\n", ret);
+}
+
+static void regmap_lock_hwlock_irq(void *__map)
+{
+	struct regmap *map = __map;
+	int ret;
+
+	if (map->hwlock_timeout)
+		ret = hwspin_lock_timeout_irq(map->hwlock, map->hwlock_timeout);
+	else
+		ret = hwspin_trylock_irq(map->hwlock);
+
+	if (ret)
+		dev_err(map->dev, "Failed to get hwlock %d\n", ret);
+}
+
+static void regmap_lock_hwlock_irqsave(void *__map)
+{
+	struct regmap *map = __map;
+	int ret;
+
+	if (map->hwlock_timeout)
+		ret = hwspin_lock_timeout_irqsave(map->hwlock,
+						  map->hwlock_timeout,
+						  &map->spinlock_flags);
+	else
+		ret = hwspin_trylock_irqsave(map->hwlock, &map->spinlock_flags);
+
+	if (ret)
+		dev_err(map->dev, "Failed to get hwlock %d\n", ret);
+}
+
+static void regmap_unlock_hwlock(void *__map)
+{
+	struct regmap *map = __map;
+
+	hwspin_unlock(map->hwlock);
+}
+
+static void regmap_unlock_hwlock_irq(void *__map)
+{
+	struct regmap *map = __map;
+
+	hwspin_unlock_irq(map->hwlock);
+}
+
+static void regmap_unlock_hwlock_irqrestore(void *__map)
+{
+	struct regmap *map = __map;
+
+	hwspin_unlock_irqrestore(map->hwlock, &map->spinlock_flags);
+}
+
 static void regmap_lock_mutex(void *__map)
 {
 	struct regmap *map = __map;
@@ -627,6 +692,25 @@  struct regmap *__regmap_init(struct device *dev,
 		map->lock = config->lock;
 		map->unlock = config->unlock;
 		map->lock_arg = config->lock_arg;
+	} else if (config->hwlock_id) {
+		map->hwlock = hwspin_lock_request_specific(config->hwlock_id);
+		if (!map->hwlock) {
+			ret = -ENXIO;
+			goto err_map;
+		}
+
+		map->hwlock_timeout = config->hwlock_timeout;
+		if (config->hwlock_mode == HWLOCK_IRQSTATE) {
+			map->lock = regmap_lock_hwlock_irqsave;
+			map->unlock = regmap_unlock_hwlock_irqrestore;
+		} else if (config->hwlock_mode == HWLOCK_IRQ) {
+			map->lock = regmap_lock_hwlock_irq;
+			map->unlock = regmap_unlock_hwlock_irq;
+		} else {
+			map->lock = regmap_lock_hwlock;
+			map->unlock = regmap_unlock_hwlock;
+		}
+		map->lock_arg = map;
 	} else {
 		if ((bus && bus->fast_io) ||
 		    config->fast_io) {
@@ -729,7 +813,7 @@  struct regmap *__regmap_init(struct device *dev,
 			map->format.format_write = regmap_format_2_6_write;
 			break;
 		default:
-			goto err_map;
+			goto err_hwlock;
 		}
 		break;
 
@@ -739,7 +823,7 @@  struct regmap *__regmap_init(struct device *dev,
 			map->format.format_write = regmap_format_4_12_write;
 			break;
 		default:
-			goto err_map;
+			goto err_hwlock;
 		}
 		break;
 
@@ -749,7 +833,7 @@  struct regmap *__regmap_init(struct device *dev,
 			map->format.format_write = regmap_format_7_9_write;
 			break;
 		default:
-			goto err_map;
+			goto err_hwlock;
 		}
 		break;
 
@@ -759,7 +843,7 @@  struct regmap *__regmap_init(struct device *dev,
 			map->format.format_write = regmap_format_10_14_write;
 			break;
 		default:
-			goto err_map;
+			goto err_hwlock;
 		}
 		break;
 
@@ -779,13 +863,13 @@  struct regmap *__regmap_init(struct device *dev,
 			map->format.format_reg = regmap_format_16_native;
 			break;
 		default:
-			goto err_map;
+			goto err_hwlock;
 		}
 		break;
 
 	case 24:
 		if (reg_endian != REGMAP_ENDIAN_BIG)
-			goto err_map;
+			goto err_hwlock;
 		map->format.format_reg = regmap_format_24;
 		break;
 
@@ -801,7 +885,7 @@  struct regmap *__regmap_init(struct device *dev,
 			map->format.format_reg = regmap_format_32_native;
 			break;
 		default:
-			goto err_map;
+			goto err_hwlock;
 		}
 		break;
 
@@ -818,13 +902,13 @@  struct regmap *__regmap_init(struct device *dev,
 			map->format.format_reg = regmap_format_64_native;
 			break;
 		default:
-			goto err_map;
+			goto err_hwlock;
 		}
 		break;
 #endif
 
 	default:
-		goto err_map;
+		goto err_hwlock;
 	}
 
 	if (val_endian == REGMAP_ENDIAN_NATIVE)
@@ -853,12 +937,12 @@  struct regmap *__regmap_init(struct device *dev,
 			map->format.parse_val = regmap_parse_16_native;
 			break;
 		default:
-			goto err_map;
+			goto err_hwlock;
 		}
 		break;
 	case 24:
 		if (val_endian != REGMAP_ENDIAN_BIG)
-			goto err_map;
+			goto err_hwlock;
 		map->format.format_val = regmap_format_24;
 		map->format.parse_val = regmap_parse_24;
 		break;
@@ -879,7 +963,7 @@  struct regmap *__regmap_init(struct device *dev,
 			map->format.parse_val = regmap_parse_32_native;
 			break;
 		default:
-			goto err_map;
+			goto err_hwlock;
 		}
 		break;
 #ifdef CONFIG_64BIT
@@ -900,7 +984,7 @@  struct regmap *__regmap_init(struct device *dev,
 			map->format.parse_val = regmap_parse_64_native;
 			break;
 		default:
-			goto err_map;
+			goto err_hwlock;
 		}
 		break;
 #endif
@@ -909,18 +993,18 @@  struct regmap *__regmap_init(struct device *dev,
 	if (map->format.format_write) {
 		if ((reg_endian != REGMAP_ENDIAN_BIG) ||
 		    (val_endian != REGMAP_ENDIAN_BIG))
-			goto err_map;
+			goto err_hwlock;
 		map->use_single_write = true;
 	}
 
 	if (!map->format.format_write &&
 	    !(map->format.format_reg && map->format.format_val))
-		goto err_map;
+		goto err_hwlock;
 
 	map->work_buf = kzalloc(map->format.buf_size, GFP_KERNEL);
 	if (map->work_buf == NULL) {
 		ret = -ENOMEM;
-		goto err_map;
+		goto err_hwlock;
 	}
 
 	if (map->format.format_write) {
@@ -1041,6 +1125,8 @@  struct regmap *__regmap_init(struct device *dev,
 err_range:
 	regmap_range_exit(map);
 	kfree(map->work_buf);
+err_hwlock:
+	hwspin_lock_free(map->hwlock);
 err_map:
 	kfree(map);
 err:
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 978abfb..fe958e2 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -19,6 +19,7 @@ 
 #include <linux/err.h>
 #include <linux/bug.h>
 #include <linux/lockdep.h>
+#include <linux/hwspinlock.h>
 
 struct module;
 struct device;
@@ -273,6 +274,10 @@  struct regmap_access_table {
  *
  * @ranges: Array of configuration entries for virtual address ranges.
  * @num_ranges: Number of range configuration entries.
+ * @hwlock_id: Specify the hardware spinlock id.
+ * @hwlock_mode: The hardware spinlock mode, should be HWLOCK_IRQSTATE,
+ *		 HWLOCK_IRQ or 0.
+ * @hwlock_timeout: Timeout value in msecs.
  */
 struct regmap_config {
 	const char *name;
@@ -317,6 +322,10 @@  struct regmap_config {
 
 	const struct regmap_range_cfg *ranges;
 	unsigned int num_ranges;
+
+	unsigned int hwlock_id;
+	unsigned int hwlock_mode;
+	unsigned int hwlock_timeout;
 };
 
 /**