[U-Boot] [PATCH v2 0/4] regmap: Add a managed API, custom read/write callbacks and support for regmap fields

[U-Boot] [PATCH v2 0/4] regmap: Add a managed API, custom read/write callbacks and support for regmap fields

[Jean-Jacques Hiblot]

This is the first of a few series, the goal of which is to facilitate
porting drivers from the linux kernel. Most of the series will be about
adding managed API to existing infrastructure (GPIO, reset, phy,...)

This particular series is about regmaps. It adds the managed API, using
the same API as linux. It also adds support for regmap fields and for
custom read/write callbacks.

Changes in v2:
- Fix comment for devm_regmap_init()
- Fix spelling in commit log
- Only use custom accessors if {,SPL,TPL}_REGMAP_ACCESSORS is enabled

Jean-Jacques Hiblot (4):
  regmap: Add devm_regmap_init()
  regmap: Allow providing read/write callbacks through struct
    regmap_config
  regmap: Add support for regmap fields
  test: dm: Add tests for regmap managed API and regmap fields

 arch/sandbox/dts/test.dts |  13 +++
 drivers/core/Kconfig      |  25 +++++
 drivers/core/regmap.c     | 123 ++++++++++++++++++++++++-
 include/regmap.h          | 148 +++++++++++++++++++++++++++++
 test/dm/regmap.c          | 189 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 497 insertions(+), 1 deletion(-)

-- 
2.17.1

[U-Boot] [PATCH v2 1/4] regmap: Add devm_regmap_init()

[Jean-Jacques Hiblot]

Most of new linux drivers are using managed-API to allocate resources. To
ease porting drivers from linux to U-Boot, introduce devm_regmap_init() as
a managed API to get a regmap from the device tree.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Reviewed-by: Simon Glass <sjg@chromium.org>

---

Changes in v2:
- Fix comment for devm_regmap_init()
- Fix spelling in commit log

 drivers/core/regmap.c | 26 ++++++++++++++++++++++++++
 include/regmap.h      | 18 ++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
index e9e55c9d16..f69ff6d12f 100644
--- a/drivers/core/regmap.c
+++ b/drivers/core/regmap.c
@@ -219,6 +219,32 @@ int regmap_init_mem(ofnode node, struct regmap **mapp)
 
 	return 0;
 }
+
+static void devm_regmap_release(struct udevice *dev, void *res)
+{
+	regmap_uninit(*(struct regmap **)res);
+}
+
+struct regmap *devm_regmap_init(struct udevice *dev,
+				const struct regmap_bus *bus,
+				void *bus_context,
+				const struct regmap_config *config)
+{
+	int rc;
+	struct regmap **mapp;
+
+	mapp = devres_alloc(devm_regmap_release, sizeof(struct regmap *),
+			    __GFP_ZERO);
+	if (unlikely(!mapp))
+		return ERR_PTR(-ENOMEM);
+
+	rc = regmap_init_mem(dev_ofnode(dev), mapp);
+	if (rc)
+		return ERR_PTR(rc);
+
+	devres_add(dev, mapp);
+	return *mapp;
+}
 #endif
 
 void *regmap_get_range(struct regmap *map, unsigned int range_num)
diff --git a/include/regmap.h b/include/regmap.h
index 9ada1af5ef..624cdd8c98 100644
--- a/include/regmap.h
+++ b/include/regmap.h
@@ -73,6 +73,9 @@ struct regmap_range {
 	ulong size;
 };
 
+struct regmap_bus;
+struct regmap_config;
+
 /**
  * struct regmap - a way of accessing hardware/bus registers
  *
@@ -333,6 +336,21 @@ int regmap_init_mem_platdata(struct udevice *dev, fdt_val_t *reg, int count,
 
 int regmap_init_mem_index(ofnode node, struct regmap **mapp, int index);
 
+/**
+ * devm_regmap_init() - Initialise register map (device managed)
+ *
+ * @dev: Device that will be interacted with
+ * @bus: Bus-specific callbacks to use with device (IGNORED)
+ * @bus_context: Data passed to bus-specific callbacks (IGNORED)
+ * @config: Configuration for register map (IGNORED)
+ *
+ * @Return a valid pointer to a struct regmap or a ERR_PTR() on error.
+ * The structure is automatically freed when the device is unbound
+ */
+struct regmap *devm_regmap_init(struct udevice *dev,
+				const struct regmap_bus *bus,
+				void *bus_context,
+				const struct regmap_config *config);
 /**
  * regmap_get_range() - Obtain the base memory address of a regmap range
  *
-- 
2.17.1

[U-Boot] [PATCH v2 2/4] regmap: Allow providing read/write callbacks through struct regmap_config

[Jean-Jacques Hiblot]

Some linux drivers provide their own read/write functions to access data
from/of the regmap. Adding support for it.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

---

Changes in v2:
- Only use custom accessors if {,SPL,TPL}_REGMAP_ACCESSORS is enabled

 drivers/core/Kconfig  | 25 +++++++++++++++++++++++++
 drivers/core/regmap.c | 22 ++++++++++++++++++++--
 include/regmap.h      | 28 +++++++++++++++++++++++++---
 3 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
index 3b95b5387b..3d836a63bf 100644
--- a/drivers/core/Kconfig
+++ b/drivers/core/Kconfig
@@ -129,6 +129,31 @@ config TPL_REGMAP
 	  support any bus type (I2C, SPI) but so far this only supports
 	  direct memory access.
 
+config REGMAP_ACCESSORS
+	bool
+	depends on REGMAP
+	default y
+
+config SPL_REGMAP_ACCESSORS
+	bool "Support custom regmap accessors in SPL"
+	depends on SPL_REGMAP
+	help
+	  Allow to use a regmap with custom accessors. The acessors are the
+	  low-level functions actually performing the read and write
+	  operations.
+	  This option can be used to access registers that are not
+	  memory-mapped.
+
+config TPL_REGMAP_ACCESSORS
+	bool "Support custom regmap accessors in TPL"
+	depends on TPL_REGMAP
+	help
+	  Allow to use a regmap with custom accessors. The acessors are the
+	  low-level functions actually performing the read and write
+	  operations.
+	  This option can be used to access registers that are not
+	  memory-mapped.
+
 config SYSCON
 	bool "Support system controllers"
 	depends on REGMAP
diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
index f69ff6d12f..10ae9f918a 100644
--- a/drivers/core/regmap.c
+++ b/drivers/core/regmap.c
@@ -31,7 +31,11 @@ static struct regmap *regmap_alloc(int count)
 	if (!map)
 		return NULL;
 	map->range_count = count;
-
+#if CONFIG_IS_ENABLED(REGMAP_ACCESSORS)
+	map->bus_context = NULL;
+	map->reg_read = NULL;
+	map->reg_write = NULL;
+#endif
 	return map;
 }
 
@@ -241,7 +245,11 @@ struct regmap *devm_regmap_init(struct udevice *dev,
 	rc = regmap_init_mem(dev_ofnode(dev), mapp);
 	if (rc)
 		return ERR_PTR(rc);
-
+#if CONFIG_IS_ENABLED(REGMAP_ACCESSORS)
+	(*mapp)->reg_read = config->reg_read;
+	(*mapp)->reg_write = config->reg_write;
+	(*mapp)->bus_context = bus_context;
+#endif
 	devres_add(dev, mapp);
 	return *mapp;
 }
@@ -320,6 +328,11 @@ int regmap_raw_read_range(struct regmap *map, uint range_num, uint offset,
 	struct regmap_range *range;
 	void *ptr;
 
+#if CONFIG_IS_ENABLED(REGMAP_ACCESSORS)
+	if (map->reg_read)
+		return map->reg_read(map->bus_context, offset, valp);
+#endif
+
 	if (range_num >= map->range_count) {
 		debug("%s: range index %d larger than range count\n",
 		      __func__, range_num);
@@ -429,6 +442,11 @@ int regmap_raw_write_range(struct regmap *map, uint range_num, uint offset,
 	struct regmap_range *range;
 	void *ptr;
 
+#if CONFIG_IS_ENABLED(REGMAP_ACCESSORS)
+	if (map->reg_write)
+		return map->reg_write(map->bus_context, offset,
+				      *(unsigned int *)val);
+#endif
 	if (range_num >= map->range_count) {
 		debug("%s: range index %d larger than range count\n",
 		      __func__, range_num);
diff --git a/include/regmap.h b/include/regmap.h
index 624cdd8c98..730e747d90 100644
--- a/include/regmap.h
+++ b/include/regmap.h
@@ -74,16 +74,38 @@ struct regmap_range {
 };
 
 struct regmap_bus;
-struct regmap_config;
+/**
+ * struct regmap_config - a way of accessing hardware/bus registers
+ *
+ * @reg_read:	  Optional callback that if filled will be used to perform
+ *		  all the reads from the registers. Should only be provided for
+ *		  devices whose read operation cannot be represented as a simple
+ *		  read operation on a bus such as SPI, I2C, etc. Most of the
+ *		  devices do not need this.
+ * @reg_write:	  Same as above for writing.
+ */
+struct regmap_config {
+	int (*reg_read)(void *context, unsigned int reg, unsigned int *val);
+	int (*reg_write)(void *context, unsigned int reg, unsigned int val);
+};
 
 /**
  * struct regmap - a way of accessing hardware/bus registers
  *
  * @range_count:	Number of ranges available within the map
  * @ranges:		Array of ranges
+ * @bus_context:	Data passed to bus-specific callbacks
+ * @reg_read:		Optional callback that if filled will be used to perform
+ *			all the reads from the registers.
+ * @reg_write:		Same as above for writing.
  */
 struct regmap {
 	enum regmap_endianness_t endianness;
+#if CONFIG_IS_ENABLED(REGMAP_ACCESSORS)
+	void *bus_context;
+	int (*reg_read)(void *context, unsigned int reg, unsigned int *val);
+	int (*reg_write)(void *context, unsigned int reg, unsigned int val);
+#endif
 	int range_count;
 	struct regmap_range ranges[0];
 };
@@ -341,8 +363,8 @@ int regmap_init_mem_index(ofnode node, struct regmap **mapp, int index);
  *
  * @dev: Device that will be interacted with
  * @bus: Bus-specific callbacks to use with device (IGNORED)
- * @bus_context: Data passed to bus-specific callbacks (IGNORED)
- * @config: Configuration for register map (IGNORED)
+ * @bus_context: Data passed to bus-specific callbacks
+ * @config: Configuration for register map
  *
  * @Return a valid pointer to a struct regmap or a ERR_PTR() on error.
  * The structure is automatically freed when the device is unbound
-- 
2.17.1

[U-Boot] [PATCH v2 3/4] regmap: Add support for regmap fields

[Jean-Jacques Hiblot]

A regmap field is an abstraction available in Linux. It provides to access
bitfields in a regmap without having to worry about shifts and masks.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v2: None

 drivers/core/regmap.c |  77 ++++++++++++++++++++++++++++++
 include/regmap.h      | 108 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 185 insertions(+)

diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
index 10ae9f918a..905bc0b63d 100644
--- a/drivers/core/regmap.c
+++ b/drivers/core/regmap.c
@@ -15,6 +15,14 @@
 #include <dm/of_addr.h>
 #include <linux/ioport.h>
 
+struct regmap_field {
+	struct regmap *regmap;
+	unsigned int mask;
+	/* lsb */
+	unsigned int shift;
+	unsigned int reg;
+};
+
 DECLARE_GLOBAL_DATA_PTR;
 
 /**
@@ -508,3 +516,72 @@ int regmap_update_bits(struct regmap *map, uint offset, uint mask, uint val)
 
 	return regmap_write(map, offset, reg | (val & mask));
 }
+
+int regmap_field_read(struct regmap_field *field, unsigned int *val)
+{
+	int ret;
+	unsigned int reg_val;
+
+	ret = regmap_read(field->regmap, field->reg, &reg_val);
+	if (ret != 0)
+		return ret;
+
+	reg_val &= field->mask;
+	reg_val >>= field->shift;
+	*val = reg_val;
+
+	return ret;
+}
+
+int regmap_field_write(struct regmap_field *field, unsigned int val)
+{
+	return regmap_update_bits(field->regmap, field->reg, field->mask,
+				  val << field->shift);
+}
+
+static void regmap_field_init(struct regmap_field *rm_field,
+			      struct regmap *regmap,
+			      struct reg_field reg_field)
+{
+	rm_field->regmap = regmap;
+	rm_field->reg = reg_field.reg;
+	rm_field->shift = reg_field.lsb;
+	rm_field->mask = GENMASK(reg_field.msb, reg_field.lsb);
+}
+
+struct regmap_field *devm_regmap_field_alloc(struct udevice *dev,
+					     struct regmap *regmap,
+					     struct reg_field reg_field)
+{
+	struct regmap_field *rm_field = devm_kzalloc(dev, sizeof(*rm_field),
+						     GFP_KERNEL);
+	if (!rm_field)
+		return ERR_PTR(-ENOMEM);
+
+	regmap_field_init(rm_field, regmap, reg_field);
+
+	return rm_field;
+}
+
+void devm_regmap_field_free(struct udevice *dev, struct regmap_field *field)
+{
+	devm_kfree(dev, field);
+}
+
+struct regmap_field *regmap_field_alloc(struct regmap *regmap,
+					struct reg_field reg_field)
+{
+	struct regmap_field *rm_field = kzalloc(sizeof(*rm_field), GFP_KERNEL);
+
+	if (!rm_field)
+		return ERR_PTR(-ENOMEM);
+
+	regmap_field_init(rm_field, regmap, reg_field);
+
+	return rm_field;
+}
+
+void regmap_field_free(struct regmap_field *field)
+{
+	kfree(field);
+}
diff --git a/include/regmap.h b/include/regmap.h
index 730e747d90..8619566a95 100644
--- a/include/regmap.h
+++ b/include/regmap.h
@@ -314,6 +314,43 @@ int regmap_raw_read_range(struct regmap *map, uint range_num, uint offset,
 	regmap_read_poll_timeout_test(map, addr, val, cond, sleep_us, \
 				      timeout_ms, 0) \
 
+/**
+ * regmap_field_read_poll_timeout - Poll until a condition is met or a timeout
+ *				    occurs
+ *
+ * @field:	Regmap field to read from
+ * @val:	Unsigned integer variable to read the value into
+ * @cond:	Break condition (usually involving @val)
+ * @sleep_us:	Maximum time to sleep between reads in us (0 tight-loops).
+ * @timeout_ms:	Timeout in ms, 0 means never timeout
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout or the regmap_field_read
+ * error return value in case of a error read. In the two former cases,
+ * the last read value at @addr is stored in @val.
+ *
+ * This is modelled after the regmap_read_poll_timeout macros in linux but
+ * with millisecond timeout.
+ */
+#define regmap_field_read_poll_timeout(field, val, cond, sleep_us, timeout_ms) \
+({ \
+	unsigned long __start = get_timer(0); \
+	int __ret; \
+	for (;;) { \
+		__ret = regmap_field_read((field), &(val)); \
+		if (__ret) \
+			break; \
+		if (cond) \
+			break; \
+		if ((timeout_ms) && get_timer(__start) > (timeout_ms)) { \
+			__ret = regmap_field_read((field), &(val)); \
+			break; \
+		} \
+		if ((sleep_us)) \
+			udelay((sleep_us)); \
+	} \
+	__ret ?: ((cond) ? 0 : -ETIMEDOUT); \
+})
+
 /**
  * regmap_update_bits() - Perform a read/modify/write using a mask
  *
@@ -390,4 +427,75 @@ void *regmap_get_range(struct regmap *map, unsigned int range_num);
  */
 int regmap_uninit(struct regmap *map);
 
+/**
+ * struct reg_field - Description of an register field
+ *
+ * @reg: Offset of the register within the regmap bank
+ * @lsb: lsb of the register field.
+ * @msb: msb of the register field.
+ * @id_size: port size if it has some ports
+ * @id_offset: address offset for each ports
+ */
+struct reg_field {
+	unsigned int reg;
+	unsigned int lsb;
+	unsigned int msb;
+};
+
+struct regmap_field;
+
+#define REG_FIELD(_reg, _lsb, _msb) {		\
+				.reg = _reg,	\
+				.lsb = _lsb,	\
+				.msb = _msb,	\
+				}
+
+/**
+ * devm_regmap_field_alloc() - Allocate and initialise a register field.
+ *
+ * @dev: Device that will be interacted with
+ * @regmap: regmap bank in which this register field is located.
+ * @reg_field: Register field with in the bank.
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap_field. The regmap_field will be automatically freed
+ * by the device management code.
+ */
+struct regmap_field *devm_regmap_field_alloc(struct udevice *dev,
+					     struct regmap *regmap,
+					     struct reg_field reg_field);
+/**
+ * devm_regmap_field_free() - Free a register field allocated using
+ *                            devm_regmap_field_alloc.
+ *
+ * @dev: Device that will be interacted with
+ * @field: regmap field which should be freed.
+ *
+ * Free register field allocated using devm_regmap_field_alloc(). Usually
+ * drivers need not call this function, as the memory allocated via devm
+ * will be freed as per device-driver life-cyle.
+ */
+void devm_regmap_field_free(struct udevice *dev, struct regmap_field *field);
+
+/**
+ * regmap_field_write() - Write a value to a regmap field
+ *
+ * @field:	Regmap field to write to
+ * @val:	Data to write to the regmap at the specified offset
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int regmap_field_write(struct regmap_field *field, unsigned int val);
+
+/**
+ * regmap_read() - Read a 32-bit value from a regmap
+ *
+ * @field:	Regmap field to write to
+ * @valp:	Pointer to the buffer to receive the data read from the regmap
+ *		field
+ *
+ * Return: 0 if OK, -ve on error
+ */
+int regmap_field_read(struct regmap_field *field, unsigned int *val);
+
 #endif
-- 
2.17.1

[U-Boot] [PATCH v2 4/4] test: dm: Add tests for regmap managed API and regmap fields

[Jean-Jacques Hiblot]

The tests rely on a dummy driver to allocate and initialize the regmap
and the regmap fields using the managed API.
The first test checks that the read/write callbacks are used.
The second test checks if regmap fields behave properly (mask and shift
are ok) by peeking into the regmap.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Reviewed-by: Simon Glass <sjg@chromium.org>

---

Changes in v2: None

 arch/sandbox/dts/test.dts |  13 +++
 test/dm/regmap.c          | 189 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 202 insertions(+)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index fdb08f2111..aa9eaec338 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -889,6 +889,19 @@
 	mdio: mdio-test {
 		compatible = "sandbox,mdio";
 	};
+
+	some_regmapped-bus {
+		#address-cells = <0x1>;
+		#size-cells = <0x1>;
+
+		ranges = <0x0 0x0 0x10>;
+		compatible = "simple-bus";
+
+		regmap-test_0 {
+			reg = <0 0x10>;
+			compatible = "sandbox,regmap_test";
+		};
+	};
 };
 
 #include "sandbox_pmic.dtsi"
diff --git a/test/dm/regmap.c b/test/dm/regmap.c
index 6fd1f20656..1a0dc78019 100644
--- a/test/dm/regmap.c
+++ b/test/dm/regmap.c
@@ -184,3 +184,192 @@ static int dm_test_regmap_poll(struct unit_test_state *uts)
 }
 
 DM_TEST(dm_test_regmap_poll, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+struct regmaptest_priv {
+	struct regmap *regmap;
+	struct regmap_field **fields;
+};
+
+#define REGMAP_TEST_BUF_SZ 12
+struct regmaptest_context {
+	unsigned short buffer[REGMAP_TEST_BUF_SZ];
+} ctx;
+
+static int regmaptest_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct regmaptest_context *ctx = context;
+
+	if (reg < ARRAY_SIZE(ctx->buffer)) {
+		ctx->buffer[reg] = val;
+		return 0;
+	}
+	return -ERANGE;
+}
+
+static int regmaptest_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct regmaptest_context *ctx = context;
+
+	if (reg < ARRAY_SIZE(ctx->buffer)) {
+		*val = ctx->buffer[reg];
+		return 0;
+	}
+
+	return -ERANGE;
+}
+
+static struct regmap_config cfg = {
+	.reg_write = regmaptest_write,
+	.reg_read = regmaptest_read,
+};
+
+static const struct reg_field field_cfgs[] = {
+	{
+		.reg = 0,
+		.lsb = 0,
+		.msb = 6,
+	},
+	{
+		.reg = 1,
+		.lsb = 4,
+		.msb = 12,
+	},
+	{
+		.reg = 1,
+		.lsb = 12,
+		.msb = 15,
+	}
+};
+
+static int remaptest_probe(struct udevice *dev)
+{
+	struct regmaptest_priv *priv = dev_get_priv(dev);
+	struct regmap *regmap;
+	struct regmap_field *field;
+	int i;
+	static const int n = ARRAY_SIZE(field_cfgs);
+
+	regmap = devm_regmap_init(dev, NULL, &ctx, &cfg);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+	priv->regmap = regmap;
+
+	priv->fields = devm_kzalloc(dev, sizeof(struct regmap_field *) * n,
+				    GFP_KERNEL);
+	if (!priv->fields)
+		return -ENOMEM;
+
+	for (i = 0 ; i < n; i++) {
+		field = devm_regmap_field_alloc(dev, regmap, field_cfgs[i]);
+		if (IS_ERR(field))
+			return PTR_ERR(field);
+		priv->fields[i] = field;
+	}
+	return 0;
+}
+
+static const struct udevice_id regmaptest_ids[] = {
+	{ .compatible = "sandbox,regmap_test" },
+	{ }
+};
+
+U_BOOT_DRIVER(regmap_test) = {
+	.name	= "regmaptest_drv",
+	.of_match	= regmaptest_ids,
+	.id	= UCLASS_NOP,
+	.probe = remaptest_probe,
+	.priv_auto_alloc_size = sizeof(struct regmaptest_priv),
+};
+
+static int dm_test_devm_regmap(struct unit_test_state *uts)
+{
+	int i = 0;
+	u32 val;
+	u16 pattern[REGMAP_TEST_BUF_SZ];
+	struct udevice *dev;
+	struct regmaptest_priv *priv;
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_NOP, "regmap-test_0",
+					      &dev));
+
+	priv = dev_get_priv(dev);
+
+	srand(get_ticks() + rand());
+	for (i = REGMAP_TEST_BUF_SZ - 1; i >= 0; i--) {
+		pattern[i] = rand() & 0xFFFF;
+		ut_assertok(regmap_write(priv->regmap, i, pattern[i]));
+	}
+	for (i = 0; i < REGMAP_TEST_BUF_SZ; i++) {
+		ut_assertok(regmap_read(priv->regmap, i, &val));
+		ut_asserteq(val, ctx.buffer[i]);
+		ut_asserteq(val, pattern[i]);
+	}
+
+	ut_asserteq(-ERANGE, regmap_write(priv->regmap, REGMAP_TEST_BUF_SZ,
+					  val));
+	ut_asserteq(-ERANGE, regmap_read(priv->regmap, REGMAP_TEST_BUF_SZ,
+					 &val));
+	ut_asserteq(-ERANGE, regmap_write(priv->regmap, -1, val));
+	ut_asserteq(-ERANGE, regmap_read(priv->regmap, -1, &val));
+
+	return 0;
+}
+DM_TEST(dm_test_devm_regmap, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+static int test_one_field(struct unit_test_state *uts,
+			  struct regmap *regmap,
+			  struct regmap_field *field,
+			  struct reg_field field_cfg)
+{
+	int j;
+	unsigned int val;
+	int mask = (1 << (field_cfg.msb - field_cfg.lsb + 1)) - 1;
+	int shift = field_cfg.lsb;
+
+	ut_assertok(regmap_write(regmap, field_cfg.reg, 0));
+	ut_assertok(regmap_read(regmap, field_cfg.reg, &val));
+	ut_asserteq(0, val);
+
+	for (j = 0; j <= mask; j++) {
+		ut_assertok(regmap_field_write(field, j));
+		ut_assertok(regmap_field_read(field, &val));
+		ut_asserteq(j, val);
+		ut_assertok(regmap_read(regmap, field_cfg.reg, &val));
+		ut_asserteq(j << shift, val);
+	}
+
+	ut_assertok(regmap_field_write(field, mask + 1));
+	ut_assertok(regmap_read(regmap, field_cfg.reg, &val));
+	ut_asserteq(0, val);
+
+	ut_assertok(regmap_field_write(field, 0xFFFF));
+	ut_assertok(regmap_read(regmap, field_cfg.reg, &val));
+	ut_asserteq(mask << shift, val);
+
+	ut_assertok(regmap_write(regmap, field_cfg.reg, 0xFFFF));
+	ut_assertok(regmap_field_write(field, 0));
+	ut_assertok(regmap_read(regmap, field_cfg.reg, &val));
+	ut_asserteq(0xFFFF & ~(mask << shift), val);
+	return 0;
+}
+
+static int dm_test_devm_regmap_field(struct unit_test_state *uts)
+{
+	int i, rc;
+	struct udevice *dev;
+	struct regmaptest_priv *priv;
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_NOP, "regmap-test_0",
+					      &dev));
+	priv = dev_get_priv(dev);
+
+	for (i = 0 ; i < ARRAY_SIZE(field_cfgs); i++) {
+		rc = test_one_field(uts, priv->regmap, priv->fields[i],
+				    field_cfgs[i]);
+		if (rc)
+			break;
+	}
+
+	return 0;
+}
+DM_TEST(dm_test_devm_regmap_field, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
-- 
2.17.1

[PATCH v2 2/4] regmap: Allow providing read/write callbacks through struct regmap_config

[Simon Glass]

Hi Jean-Jacques,

On Tue, 5 Nov 2019 at 04:47, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>
> Some linux drivers provide their own read/write functions to access data
> from/of the regmap. Adding support for it.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>
> ---
>
> Changes in v2:
> - Only use custom accessors if {,SPL,TPL}_REGMAP_ACCESSORS is enabled
>
>  drivers/core/Kconfig  | 25 +++++++++++++++++++++++++
>  drivers/core/regmap.c | 22 ++++++++++++++++++++--
>  include/regmap.h      | 28 +++++++++++++++++++++++++---
>  3 files changed, 70 insertions(+), 5 deletions(-)

Coming back to the discussion on driver model....

How do you specify the fields? I would expect that this would be done
in the driver tree? Perhaps in a subnode of the device?

Just to state what I see as the advantages of using a separate device
for access:

- Remove the #ifdef in the regmap struct
- Easy to specify the behaviour in a device-tree node
- Easy to extend as the child device can do what it likes with respect to access

Disadvantage is that it takes a bit more space.

Regards,
Simon

[PATCH v2 2/4] regmap: Allow providing read/write callbacks through struct regmap_config

[Jean-Jacques Hiblot]

Hi Simon,

On 10/12/2019 16:18, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On Tue, 5 Nov 2019 at 04:47, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>> Some linux drivers provide their own read/write functions to access data
>> from/of the regmap. Adding support for it.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>
>> ---
>>
>> Changes in v2:
>> - Only use custom accessors if {,SPL,TPL}_REGMAP_ACCESSORS is enabled
>>
>>   drivers/core/Kconfig  | 25 +++++++++++++++++++++++++
>>   drivers/core/regmap.c | 22 ++++++++++++++++++++--
>>   include/regmap.h      | 28 +++++++++++++++++++++++++---
>>   3 files changed, 70 insertions(+), 5 deletions(-)
> Coming back to the discussion on driver model....
>
> How do you specify the fields? I would expect that this would be done
> in the driver tree? Perhaps in a subnode of the device?
>
> Just to state what I see as the advantages of using a separate device
> for access:
>
> - Remove the #ifdef in the regmap struct
> - Easy to specify the behaviour in a device-tree node
> - Easy to extend as the child device can do what it likes with respect to access

That sure is a better abstraction. However the goal of this patch is 
only to use the same API as linux. It allows porting the drivers as-is 
and thus reduce the burden of maintenance.

JJ

>
> Disadvantage is that it takes a bit more space.
>
> Regards,
> Simon

[PATCH v2 2/4] regmap: Allow providing read/write callbacks through struct regmap_config

[Simon Glass]

Hi Jean-Jacques,

On Mon, 16 Dec 2019 at 03:10, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>
> Hi Simon,
>
> On 10/12/2019 16:18, Simon Glass wrote:
> > Hi Jean-Jacques,
> >
> > On Tue, 5 Nov 2019 at 04:47, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> >> Some linux drivers provide their own read/write functions to access data
> >> from/of the regmap. Adding support for it.
> >>
> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> >>
> >> ---
> >>
> >> Changes in v2:
> >> - Only use custom accessors if {,SPL,TPL}_REGMAP_ACCESSORS is enabled
> >>
> >>   drivers/core/Kconfig  | 25 +++++++++++++++++++++++++
> >>   drivers/core/regmap.c | 22 ++++++++++++++++++++--
> >>   include/regmap.h      | 28 +++++++++++++++++++++++++---
> >>   3 files changed, 70 insertions(+), 5 deletions(-)
> > Coming back to the discussion on driver model....
> >
> > How do you specify the fields? I would expect that this would be done
> > in the driver tree? Perhaps in a subnode of the device?
> >
> > Just to state what I see as the advantages of using a separate device
> > for access:
> >
> > - Remove the #ifdef in the regmap struct
> > - Easy to specify the behaviour in a device-tree node
> > - Easy to extend as the child device can do what it likes with respect to access
>
> That sure is a better abstraction. However the goal of this patch is
> only to use the same API as linux. It allows porting the drivers as-is
> and thus reduce the burden of maintenance.

So how do you specify the fields? See my question above.

It is not possible to use a similar API without importing the internal
implementation. Linux's driver model is less homogenous.

Regards,
Simon

[PATCH v2 2/4] regmap: Allow providing read/write callbacks through struct regmap_config

[Jean-Jacques Hiblot]

Simon,

On 24/12/2019 16:58, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On Mon, 16 Dec 2019 at 03:10, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>> Hi Simon,
>>
>> On 10/12/2019 16:18, Simon Glass wrote:
>>> Hi Jean-Jacques,
>>>
>>> On Tue, 5 Nov 2019 at 04:47, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>>>> Some linux drivers provide their own read/write functions to access data
>>>> from/of the regmap. Adding support for it.
>>>>
>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>>>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Only use custom accessors if {,SPL,TPL}_REGMAP_ACCESSORS is enabled
>>>>
>>>>    drivers/core/Kconfig  | 25 +++++++++++++++++++++++++
>>>>    drivers/core/regmap.c | 22 ++++++++++++++++++++--
>>>>    include/regmap.h      | 28 +++++++++++++++++++++++++---
>>>>    3 files changed, 70 insertions(+), 5 deletions(-)
>>> Coming back to the discussion on driver model....
>>>
>>> How do you specify the fields? I would expect that this would be done
>>> in the driver tree? Perhaps in a subnode of the device?
>>>
>>> Just to state what I see as the advantages of using a separate device
>>> for access:
>>>
>>> - Remove the #ifdef in the regmap struct
>>> - Easy to specify the behaviour in a device-tree node
>>> - Easy to extend as the child device can do what it likes with respect to access
>> That sure is a better abstraction. However the goal of this patch is
>> only to use the same API as linux. It allows porting the drivers as-is
>> and thus reduce the burden of maintenance.
> So how do you specify the fields? See my question above.

The fields are filled when creating the regmap.

ex:

static struct regmap_config cfg = {
	.reg_write = regmaptest_write,
	.reg_read = regmaptest_read,
};
and then
regmap = devm_regmap_init(dev, NULL, &ctx, &cfg);

You can have a look at the tests in the last patch of the series.

JJ

> It is not possible to use a similar API without importing the internal
> implementation. Linux's driver model is less homogenous.
>
> Regards,
> Simon

[PATCH v2 2/4] regmap: Allow providing read/write callbacks through struct regmap_config

[Simon Glass]

Hi Jean-Jacques,

On Wed, 8 Jan 2020 at 09:16, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>
> Simon,
>
> On 24/12/2019 16:58, Simon Glass wrote:
> > Hi Jean-Jacques,
> >
> > On Mon, 16 Dec 2019 at 03:10, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> >> Hi Simon,
> >>
> >> On 10/12/2019 16:18, Simon Glass wrote:
> >>> Hi Jean-Jacques,
> >>>
> >>> On Tue, 5 Nov 2019 at 04:47, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> >>>> Some linux drivers provide their own read/write functions to access data
> >>>> from/of the regmap. Adding support for it.
> >>>>
> >>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> >>>>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>> - Only use custom accessors if {,SPL,TPL}_REGMAP_ACCESSORS is enabled
> >>>>
> >>>>    drivers/core/Kconfig  | 25 +++++++++++++++++++++++++
> >>>>    drivers/core/regmap.c | 22 ++++++++++++++++++++--
> >>>>    include/regmap.h      | 28 +++++++++++++++++++++++++---
> >>>>    3 files changed, 70 insertions(+), 5 deletions(-)
> >>> Coming back to the discussion on driver model....
> >>>
> >>> How do you specify the fields? I would expect that this would be done
> >>> in the driver tree? Perhaps in a subnode of the device?
> >>>
> >>> Just to state what I see as the advantages of using a separate device
> >>> for access:
> >>>
> >>> - Remove the #ifdef in the regmap struct
> >>> - Easy to specify the behaviour in a device-tree node
> >>> - Easy to extend as the child device can do what it likes with respect to access
> >> That sure is a better abstraction. However the goal of this patch is
> >> only to use the same API as linux. It allows porting the drivers as-is
> >> and thus reduce the burden of maintenance.
> > So how do you specify the fields? See my question above.
>
> The fields are filled when creating the regmap.
>
> ex:
>
> static struct regmap_config cfg = {
>         .reg_write = regmaptest_write,
>         .reg_read = regmaptest_read,
> };
> and then
> regmap = devm_regmap_init(dev, NULL, &ctx, &cfg);
>
> You can have a look at the tests in the last patch of the series.
>
> JJ
>
> > It is not possible to use a similar API without importing the internal
> > implementation. Linux's driver model is less homogenous.

Then I really think we should try to use driver model for the accessors too.

IMO the access method could be described in the device tree. But if
not, we could bind the accessor driver as a child.

Regards,
Simon

[PATCH v2 2/4] regmap: Allow providing read/write callbacks through struct regmap_config

[Pratyush Yadav]

Hi Simon,

I would be taking up this series and address the comments.

On 10/12/19 03:18PM, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On Tue, 5 Nov 2019 at 04:47, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> >
> > Some linux drivers provide their own read/write functions to access data
> > from/of the regmap. Adding support for it.
> >
> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> >
> > ---
> >
> > Changes in v2:
> > - Only use custom accessors if {,SPL,TPL}_REGMAP_ACCESSORS is enabled
> >
> >  drivers/core/Kconfig  | 25 +++++++++++++++++++++++++
> >  drivers/core/regmap.c | 22 ++++++++++++++++++++--
> >  include/regmap.h      | 28 +++++++++++++++++++++++++---
> >  3 files changed, 70 insertions(+), 5 deletions(-)
>
> Coming back to the discussion on driver model....

IIUC, you are suggesting that we create a uclass for regmap, and fill in 
the ops with the driver's custom read/write functions, correct? This 
would mean we will have to create udevice for each regmap. A driver can 
have multiple regmaps (up to 18 for example in the Linux Cadence Sierra 
PHY driver in kernel). Creating a device for each regmap is very 
inefficient. Is the overhead really worth it? Does it solve any 
significant problem? Also, a regmap is not a device, it is an 
abstraction layer to simplify register access. Does it then make sense 
to model it as a device?

> How do you specify the fields? I would expect that this would be done
> in the driver tree? Perhaps in a subnode of the device?
>
> Just to state what I see as the advantages of using a separate device
> for access:
>
> - Remove the #ifdef in the regmap struct

If you really want to remove the #ifdefs, we can set reg_read to a 
default, and let drivers override if when creating a regmap. Then in 
regmap_read, just call reg_read. This gets rid of the #ifdefs with a 
small change. I suspect this will have a much smaller impact on memory 
usage than using a udevice.

> - Easy to specify the behaviour in a device-tree node

Quoting from the kernel's documentation of regmap_config:

  reg_read: Optional callback that if filled will be used to perform all 
  the reads from the registers. Should only be provided for devices 
  whose read operation cannot be represented as a simple read operation 
  on a bus such as SPI, I2C, etc. Most of the devices do not need this.
  reg_write: Same as above for writing.

These custom accessors are meant to be used when the read or write needs 
more work to be done than the standard regmap reads/writes. And so they 
are supposed to be driver-specific functions. I don't think it is at all 
possible to specify something like this in devicetree. Am I missing 
something?

> - Easy to extend as the child device can do what it likes with respect to access
>
> Disadvantage is that it takes a bit more space.

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH v2 2/4] regmap: Allow providing read/write callbacks through struct regmap_config

[Simon Glass]

Hi Pratyush,

On Tue, 5 May 2020 at 13:47, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Hi Simon,
>
> I would be taking up this series and address the comments.
>
> On 10/12/19 03:18PM, Simon Glass wrote:
> > Hi Jean-Jacques,
> >
> > On Tue, 5 Nov 2019 at 04:47, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> > >
> > > Some linux drivers provide their own read/write functions to access data
> > > from/of the regmap. Adding support for it.
> > >
> > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > >
> > > ---
> > >
> > > Changes in v2:
> > > - Only use custom accessors if {,SPL,TPL}_REGMAP_ACCESSORS is enabled
> > >
> > >  drivers/core/Kconfig  | 25 +++++++++++++++++++++++++
> > >  drivers/core/regmap.c | 22 ++++++++++++++++++++--
> > >  include/regmap.h      | 28 +++++++++++++++++++++++++---
> > >  3 files changed, 70 insertions(+), 5 deletions(-)
> >
> > Coming back to the discussion on driver model....
>
> IIUC, you are suggesting that we create a uclass for regmap, and fill in
> the ops with the driver's custom read/write functions, correct? This
> would mean we will have to create udevice for each regmap. A driver can
> have multiple regmaps (up to 18 for example in the Linux Cadence Sierra
> PHY driver in kernel). Creating a device for each regmap is very
> inefficient. Is the overhead really worth it? Does it solve any
> significant problem? Also, a regmap is not a device, it is an
> abstraction layer to simplify register access. Does it then make sense
> to model it as a device?

I was actually referring to the access method of the regmap but I
suppose the effect is the same. This question has been running for a
while.

The issue is that this is getting more and more complicated. We use
devices to model complicated things. It is an abstraction. It doesn't
have to be a physical device.

With regmap we are creating an ad-hoc structure and associated
infrastructure to deal with this one case. It is not possible to see
the regmaps with 'dm dump', for example.

>
> > How do you specify the fields? I would expect that this would be done
> > in the driver tree? Perhaps in a subnode of the device?
> >
> > Just to state what I see as the advantages of using a separate device
> > for access:
> >
> > - Remove the #ifdef in the regmap struct
>
> If you really want to remove the #ifdefs, we can set reg_read to a
> default, and let drivers override if when creating a regmap. Then in
> regmap_read, just call reg_read. This gets rid of the #ifdefs with a
> small change. I suspect this will have a much smaller impact on memory
> usage than using a udevice.

Yes that sounds good, but you are getting closer and closer to it
being a device.

>
> > - Easy to specify the behaviour in a device-tree node
>
> Quoting from the kernel's documentation of regmap_config:
>
>   reg_read: Optional callback that if filled will be used to perform all
>   the reads from the registers. Should only be provided for devices
>   whose read operation cannot be represented as a simple read operation
>   on a bus such as SPI, I2C, etc. Most of the devices do not need this.
>   reg_write: Same as above for writing.
>
> These custom accessors are meant to be used when the read or write needs
> more work to be done than the standard regmap reads/writes. And so they
> are supposed to be driver-specific functions. I don't think it is at all
> possible to specify something like this in devicetree. Am I missing
> something?

Can you point to an example of this extra read/write code?

The kernel is a rabbit-warren of custom interfaces. I am trying to
keep driver model uniform, so long as the cost is reasonable. I'm not
sure that it is worth having a regmap device for simple things, but as
things get more complex, I think we should look at it.

Of course you can specify this in the device tree: just use a
compatible string to select the appropriate regmap driver.

spi {
    compatible = "vendor,myspi";
    regmaps = <&regmap_simple 4
           &regmap_mailbox 0 FLAGS>;
}

regmap_mailbox: regmap-mailbox {
    compatible = "regmap,mailbox";
    other props
}

struct regmap *regmap = regmap_get_by_index(dev, 1)

regmap_read(regmap, ...)

>
> > - Easy to extend as the child device can do what it likes with respect to access
> >
> > Disadvantage is that it takes a bit more space.

Yes space is a concern. A device takes about 84 bytes I think. But as
we add more and more complexity we might as well take the hit.

Regards,
Simon

[PATCH v2 2/4] regmap: Allow providing read/write callbacks through struct regmap_config

[Vignesh Raghavendra]

On 06/05/20 8:17 pm, Simon Glass wrote:
> Hi Pratyush,
> 
> On Tue, 5 May 2020 at 13:47, Pratyush Yadav <p.yadav@ti.com> wrote:
>>
>> Hi Simon,
>>
>> I would be taking up this series and address the comments.
>>
>> On 10/12/19 03:18PM, Simon Glass wrote:
>>> Hi Jean-Jacques,
>>>
>>> On Tue, 5 Nov 2019 at 04:47, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>>>>
>>>> Some linux drivers provide their own read/write functions to access data
>>>> from/of the regmap. Adding support for it.
>>>>
>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>>>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Only use custom accessors if {,SPL,TPL}_REGMAP_ACCESSORS is enabled
>>>>
>>>>? drivers/core/Kconfig? | 25 +++++++++++++++++++++++++
>>>>? drivers/core/regmap.c | 22 ++++++++++++++++++++--
>>>>? include/regmap.h????? | 28 +++++++++++++++++++++++++---
>>>>? 3 files changed, 70 insertions(+), 5 deletions(-)
>>>
>>> Coming back to the discussion on driver model....
>>
>> IIUC, you are suggesting that we create a uclass for regmap, and fill in
>> the ops with the driver's custom read/write functions, correct? This
>> would mean we will have to create udevice for each regmap. A driver can
>> have multiple regmaps (up to 18 for example in the Linux Cadence Sierra
>> PHY driver in kernel). Creating a device for each regmap is very
>> inefficient. Is the overhead really worth it? Does it solve any
>> significant problem? Also, a regmap is not a device, it is an
>> abstraction layer to simplify register access. Does it then make sense
>> to model it as a device?
> 
> I was actually referring to the access method of the regmap but I
> suppose the effect is the same. This question has been running for a
> while.
> 
> The issue is that this is getting more and more complicated. We use
> devices to model complicated things. It is an abstraction. It doesn't
> have to be a physical device.
> 
> With regmap we are creating an ad-hoc structure and associated
> infrastructure to deal with this one case. It is not possible to see
> the regmaps with 'dm dump', for example.
> 
>>
>>> How do you specify the fields? I would expect that this would be done
>>> in the driver tree? Perhaps in a subnode of the device?
>>>
>>> Just to state what I see as the advantages of using a separate device
>>> for access:
>>>
>>> - Remove the #ifdef in the regmap struct
>>
>> If you really want to remove the #ifdefs, we can set reg_read to a
>> default, and let drivers override if when creating a regmap. Then in
>> regmap_read, just call reg_read. This gets rid of the #ifdefs with a
>> small change. I suspect this will have a much smaller impact on memory
>> usage than using a udevice.
> 
> Yes that sounds good, but you are getting closer and closer to it
> being a device.
> 
>>
>>> - Easy to specify the behaviour in a device-tree node
>>
>> Quoting from the kernel's documentation of regmap_config:
>>
>>?? reg_read: Optional callback that if filled will be used to perform all
>>?? the reads from the registers. Should only be provided for devices
>>?? whose read operation cannot be represented as a simple read operation
>>?? on a bus such as SPI, I2C, etc. Most of the devices do not need this.
>>?? reg_write: Same as above for writing.
>>
>> These custom accessors are meant to be used when the read or write needs
>> more work to be done than the standard regmap reads/writes. And so they
>> are supposed to be driver-specific functions. I don't think it is at all
>> possible to specify something like this in devicetree. Am I missing
>> something?
> 
> Can you point to an example of this extra read/write code?


Here is the cadence sierra PHY driver for which this series is a pre
requiste

https://elixir.bootlin.com/linux/latest/source/drivers/phy/cadence/phy-cadence-sierra.c

PHY Registers are 16 bit wide. Some implementations (SoCs) have arranged
these registers continuously (so they are aligned to 16 bit address
boundary) but some implementation place each register at 32 bit boundary
(wasting the upper 16 bits)

There are few other nits but above is the major one.

Custom regmap read()/write() hooks abstracts all these from actual
driver logic.

It is possible to re implement driver w/o regmap abstraction but this
would deviate the driver significantly from that of kernel. PHY drivers
often get updated with new register settings as and when HW
characterization progresses which need to be ported to kernel and
U-Boot. Having U-Boot driver same as kernel will ease porting effort and
also reduces the chances of errors.


> 
> The kernel is a rabbit-warren of custom interfaces. I am trying to
> keep driver model uniform, so long as the cost is reasonable. I'm not
> sure that it is worth having a regmap device for simple things, but as
> things get more complex, I think we should look at it.
> 
> Of course you can specify this in the device tree: just use a
> compatible string to select the appropriate regmap driver.
> 
> spi {
>???? compatible = "vendor,myspi";
>???? regmaps = <&regmap_simple 4
>??????????? &regmap_mailbox 0 FLAGS>;
> }
> 
> regmap_mailbox: regmap-mailbox {
>???? compatible = "regmap,mailbox";
>???? other props
> }

This would mean U-Boot and Kernel DT files would be significantly
different and would defeat the whole purpose of maintaining common DT
bindings b/w kernel and U-Boot.

IMHO, regmap is pure SW abstraction that hides complexities of accessing
underlying register map therefore I don't think representing such
abstraction in DT will be accepted by upstream DT maintainers.

If there a way to use DM w/o DT changes, we can definitely explore that...

> 
> struct regmap *regmap = regmap_get_by_index(dev, 1)
> 
> regmap_read(regmap, ...)
> 
>>
>>> - Easy to extend as the child device can do what it likes with respect to access
>>>
>>> Disadvantage is that it takes a bit more space.
> 
> Yes space is a concern. A device takes about 84 bytes I think. But as
> we add more and more complexity we might as well take the hit.
> 
> Regards,
> Simon
> 

[PATCH v2 2/4] regmap: Allow providing read/write callbacks through struct regmap_config

[Simon Glass]

Hi Vignesh,

On Thu, 7 May 2020 at 03:08, Vignesh Raghavendra <vigneshr@ti.com> wrote:
>
>
>
> On 06/05/20 8:17 pm, Simon Glass wrote:
> > Hi Pratyush,
> >
> > On Tue, 5 May 2020 at 13:47, Pratyush Yadav <p.yadav@ti.com> wrote:
> >>
> >> Hi Simon,
> >>
> >> I would be taking up this series and address the comments.
> >>
> >> On 10/12/19 03:18PM, Simon Glass wrote:
> >>> Hi Jean-Jacques,
> >>>
> >>> On Tue, 5 Nov 2019 at 04:47, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> >>>>
> >>>> Some linux drivers provide their own read/write functions to access data
> >>>> from/of the regmap. Adding support for it.
> >>>>
> >>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> >>>>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>> - Only use custom accessors if {,SPL,TPL}_REGMAP_ACCESSORS is enabled
> >>>>
> >>>>  drivers/core/Kconfig  | 25 +++++++++++++++++++++++++
> >>>>  drivers/core/regmap.c | 22 ++++++++++++++++++++--
> >>>>  include/regmap.h      | 28 +++++++++++++++++++++++++---
> >>>>  3 files changed, 70 insertions(+), 5 deletions(-)
> >>>
> >>> Coming back to the discussion on driver model....
> >>
> >> IIUC, you are suggesting that we create a uclass for regmap, and fill in
> >> the ops with the driver's custom read/write functions, correct? This
> >> would mean we will have to create udevice for each regmap. A driver can
> >> have multiple regmaps (up to 18 for example in the Linux Cadence Sierra
> >> PHY driver in kernel). Creating a device for each regmap is very
> >> inefficient. Is the overhead really worth it? Does it solve any
> >> significant problem? Also, a regmap is not a device, it is an
> >> abstraction layer to simplify register access. Does it then make sense
> >> to model it as a device?
> >
> > I was actually referring to the access method of the regmap but I
> > suppose the effect is the same. This question has been running for a
> > while.
> >
> > The issue is that this is getting more and more complicated. We use
> > devices to model complicated things. It is an abstraction. It doesn't
> > have to be a physical device.
> >
> > With regmap we are creating an ad-hoc structure and associated
> > infrastructure to deal with this one case. It is not possible to see
> > the regmaps with 'dm dump', for example.
> >
> >>
> >>> How do you specify the fields? I would expect that this would be done
> >>> in the driver tree? Perhaps in a subnode of the device?
> >>>
> >>> Just to state what I see as the advantages of using a separate device
> >>> for access:
> >>>
> >>> - Remove the #ifdef in the regmap struct
> >>
> >> If you really want to remove the #ifdefs, we can set reg_read to a
> >> default, and let drivers override if when creating a regmap. Then in
> >> regmap_read, just call reg_read. This gets rid of the #ifdefs with a
> >> small change. I suspect this will have a much smaller impact on memory
> >> usage than using a udevice.
> >
> > Yes that sounds good, but you are getting closer and closer to it
> > being a device.
> >
> >>
> >>> - Easy to specify the behaviour in a device-tree node
> >>
> >> Quoting from the kernel's documentation of regmap_config:
> >>
> >>   reg_read: Optional callback that if filled will be used to perform all
> >>   the reads from the registers. Should only be provided for devices
> >>   whose read operation cannot be represented as a simple read operation
> >>   on a bus such as SPI, I2C, etc. Most of the devices do not need this.
> >>   reg_write: Same as above for writing.
> >>
> >> These custom accessors are meant to be used when the read or write needs
> >> more work to be done than the standard regmap reads/writes. And so they
> >> are supposed to be driver-specific functions. I don't think it is at all
> >> possible to specify something like this in devicetree. Am I missing
> >> something?
> >
> > Can you point to an example of this extra read/write code?
>
>
> Here is the cadence sierra PHY driver for which this series is a pre
> requiste
>
> https://elixir.bootlin.com/linux/latest/source/drivers/phy/cadence/phy-cadence-sierra.c
>
> PHY Registers are 16 bit wide. Some implementations (SoCs) have arranged
> these registers continuously (so they are aligned to 16 bit address
> boundary) but some implementation place each register at 32 bit boundary
> (wasting the upper 16 bits)
>
> There are few other nits but above is the major one.
>

OK I see. So it's fairly simple, not accessing through a mailbox, etc.

> Custom regmap read()/write() hooks abstracts all these from actual
> driver logic.
>
> It is possible to re implement driver w/o regmap abstraction but this
> would deviate the driver significantly from that of kernel. PHY drivers
> often get updated with new register settings as and when HW
> characterization progresses which need to be ported to kernel and
> U-Boot. Having U-Boot driver same as kernel will ease porting effort and
> also reduces the chances of errors.

I actually don't think that argument holds much water. I am not
suggesting that the regmap interface be different, so the code should
remain the same in the driver. Perhaps the probe() code might change a
bit as it sets things up. But there are already differences there.

>
>
> >
> > The kernel is a rabbit-warren of custom interfaces. I am trying to
> > keep driver model uniform, so long as the cost is reasonable. I'm not
> > sure that it is worth having a regmap device for simple things, but as
> > things get more complex, I think we should look at it.
> >
> > Of course you can specify this in the device tree: just use a
> > compatible string to select the appropriate regmap driver.
> >
> > spi {
> >     compatible = "vendor,myspi";
> >     regmaps = <&regmap_simple 4
> >            &regmap_mailbox 0 FLAGS>;
> > }
> >
> > regmap_mailbox: regmap-mailbox {
> >     compatible = "regmap,mailbox";
> >     other props
> > }
>
> This would mean U-Boot and Kernel DT files would be significantly
> different and would defeat the whole purpose of maintaining common DT
> bindings b/w kernel and U-Boot.

Yes it means that U-Boot would have extra nodes that the kernel DT
does not have. We already have that problem, although I hope with the
DTE project U-Boot might finally get a say in what is allowed in the
bindings.

>
> IMHO, regmap is pure SW abstraction that hides complexities of accessing
> underlying register map therefore I don't think representing such
> abstraction in DT will be accepted by upstream DT maintainers.

That is a very strange argument. Above you told me that the access
mechanism is different in the hardware - 16 bits with different
positioning, for example. That is exactly the sort of thing the DT is
supposed to describe.

>
> If there a way to use DM w/o DT changes, we can definitely explore that...

You can certainly do that. Just device_bind() a driver that implements
your access mechanism. So long as the driver uses the same access
mechanism no matter what hardware it is running on, that makes sense.
But from what you say above, that might not be true, so you would end
up setting the platform data of the regmap driver from its parent.
That's not terrible, but not ideal.

Given the simple access you need above, I think you could just add
that as another option in the regmap, perhaps turning endianness into
some flags?


>
> >
> > struct regmap *regmap = regmap_get_by_index(dev, 1)
> >
> > regmap_read(regmap, ...)
> >
> >>
> >>> - Easy to extend as the child device can do what it likes with respect to access
> >>>
> >>> Disadvantage is that it takes a bit more space.
> >
> > Yes space is a concern. A device takes about 84 bytes I think. But as
> > we add more and more complexity we might as well take the hit.

Regards,
Simon

[PATCH v2 2/4] regmap: Allow providing read/write callbacks through struct regmap_config

[Yadav, Pratyush]

Hi Simon,

On 07/05/20 07:36PM, Simon Glass wrote:
> >
> > If there a way to use DM w/o DT changes, we can definitely explore that...
> 
> You can certainly do that. Just device_bind() a driver that implements
> your access mechanism. So long as the driver uses the same access
> mechanism no matter what hardware it is running on, that makes sense.
> But from what you say above, that might not be true, so you would end
> up setting the platform data of the regmap driver from its parent.
> That's not terrible, but not ideal.
> 
> Given the simple access you need above, I think you could just add
> that as another option in the regmap, perhaps turning endianness into
> some flags?

I tried doing it without custom regmap accessors. The rough patch below 
is what I came up with. Does it look any better than custom accessors?

PS: I can probably play around with regmap ranges instead of using
'base_offset', but it gets the job done minimally so I think it is good 
enough for a proof-of-concept.
 
 -- 8< --
Subject: [RFC PATCH] regmap: allow specifying base offset and register shift and
 access size

To accommodate more complex access patterns that are needed in the
Cadence Sierra PHY driver (which will be added as a follow-up),
introduce 'base_offset' and 'reg_offset_shift', two values that can be
used to adjust the final regmap read/write address.

'base_offset' is an extra offset on the regmap base discovered from the
device tree. This is useful when some regmaps of a device need to access
memory at one offset, and some at another.

'reg_offset_shift' will shift the register offset left before access.

'size' can be used to specify the width of reads (1/2/4/8 bytes). If 0,
defaults to 4 bytes to maintain backward compatibility.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/core/regmap.c | 16 ++++++++++++----
 include/regmap.h      | 27 ++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
index 41293e5af0..9bce719b45 100644
--- a/drivers/core/regmap.c
+++ b/drivers/core/regmap.c
@@ -38,6 +38,7 @@ static struct regmap *regmap_alloc(int count)
 	map = malloc(sizeof(*map) + sizeof(map->ranges[0]) * count);
 	if (!map)
 		return NULL;
+	memset(map, 0, sizeof(*map));
 	map->range_count = count;

 	return map;
@@ -258,6 +259,10 @@ struct regmap *devm_regmap_init(struct udevice *dev,
 	if (rc)
 		return ERR_PTR(rc);

+	(*mapp)->base_offset = config->base_offset;
+	(*mapp)->reg_offset_shift = config->reg_offset_shift;
+	(*mapp)->size = config->size;
+
 	devres_add(dev, mapp);
 	return *mapp;
 }
@@ -343,7 +348,9 @@ int regmap_raw_read_range(struct regmap *map, uint range_num, uint offset,
 	}
 	range = &map->ranges[range_num];

-	ptr = map_physmem(range->start + offset, val_len, MAP_NOCACHE);
+	offset <<= map->reg_offset_shift;
+
+	ptr = map_physmem(range->start + map->base_offset + offset, val_len, MAP_NOCACHE);

 	if (offset + val_len > range->size) {
 		debug("%s: offset/size combination invalid\n", __func__);
@@ -380,7 +387,7 @@ int regmap_raw_read(struct regmap *map, uint offset, void *valp, size_t val_len)

 int regmap_read(struct regmap *map, uint offset, uint *valp)
 {
-	return regmap_raw_read(map, offset, valp, REGMAP_SIZE_32);
+	return regmap_raw_read(map, offset, valp, map->size ? map->size : REGMAP_SIZE_32);
 }

 static inline void __write_8(u8 *addr, const u8 *val,
@@ -452,7 +459,8 @@ int regmap_raw_write_range(struct regmap *map, uint range_num, uint offset,
 	}
 	range = &map->ranges[range_num];

-	ptr = map_physmem(range->start + offset, val_len, MAP_NOCACHE);
+	offset <<= map->reg_offset_shift;
+	ptr = map_physmem(range->start + map->base_offset + offset, val_len, MAP_NOCACHE);

 	if (offset + val_len > range->size) {
 		debug("%s: offset/size combination invalid\n", __func__);
@@ -490,7 +498,7 @@ int regmap_raw_write(struct regmap *map, uint offset, const void *val,

 int regmap_write(struct regmap *map, uint offset, uint val)
 {
-	return regmap_raw_write(map, offset, &val, REGMAP_SIZE_32);
+	return regmap_raw_write(map, offset, &val, map->size ? map->size : REGMAP_SIZE_32);
 }

 int regmap_update_bits(struct regmap *map, uint offset, uint mask, uint val)
diff --git a/include/regmap.h b/include/regmap.h
index 2edbf9359a..4adb04f7e3 100644
--- a/include/regmap.h
+++ b/include/regmap.h
@@ -74,16 +74,41 @@ struct regmap_range {
 };

 struct regmap_bus;
-struct regmap_config;
+/**
+ * struct regmap_config - a way of accessing hardware/bus registers
+ *
+ * @base_offset: Offset from the base of the regmap for reads and writes.
+ *		 (OPTIONAL)
+ * @reg_offset_shift: Left shift register offset by this value before
+ *		      performing reads or writes. (OPTIONAL)
+ * @size: Size/width of the read or write operation. Defaults to
+ *	  REGMAP_SIZE_32 if set to zero.
+ */
+struct regmap_config {
+	u32 base_offset;
+	u32 reg_offset_shift;
+	enum regmap_size_t size;
+};

 /**
  * struct regmap - a way of accessing hardware/bus registers
  *
+ * @base_offset: Offset from the base of the regmap for reads and writes.
+ *		 (OPTIONAL)
+ * @reg_offset_shift: Left shift register offset by this value before
+ *		      performing reads or writes. (OPTIONAL)
+ * @size: Size/width of the read or write operation. Defaults to
+ *	  REGMAP_SIZE_32 if set to zero.
  * @range_count:	Number of ranges available within the map
  * @ranges:		Array of ranges
  */
 struct regmap {
 	enum regmap_endianness_t endianness;
+
+	u32 base_offset;
+	u32 reg_offset_shift;
+	enum regmap_size_t size;
+
 	int range_count;
 	struct regmap_range ranges[0];
 };

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH v2 2/4] regmap: Allow providing read/write callbacks through struct regmap_config

[Simon Glass]

Hi Pratyush,

On Mon, 11 May 2020 at 06:00, Yadav, Pratyush <p.yadav@ti.com> wrote:
>
> Hi Simon,
>
> On 07/05/20 07:36PM, Simon Glass wrote:
> > >
> > > If there a way to use DM w/o DT changes, we can definitely explore that...
> >
> > You can certainly do that. Just device_bind() a driver that implements
> > your access mechanism. So long as the driver uses the same access
> > mechanism no matter what hardware it is running on, that makes sense.
> > But from what you say above, that might not be true, so you would end
> > up setting the platform data of the regmap driver from its parent.
> > That's not terrible, but not ideal.
> >
> > Given the simple access you need above, I think you could just add
> > that as another option in the regmap, perhaps turning endianness into
> > some flags?
>
> I tried doing it without custom regmap accessors. The rough patch below
> is what I came up with. Does it look any better than custom accessors?

This looks OK. Should use a local variable instead of (*mapp)->
though. Also please add docs.


- SImon

>
> PS: I can probably play around with regmap ranges instead of using
> 'base_offset', but it gets the job done minimally so I think it is good
> enough for a proof-of-concept.
>
>  -- 8< --
> Subject: [RFC PATCH] regmap: allow specifying base offset and register shift and
>  access size
>
> To accommodate more complex access patterns that are needed in the
> Cadence Sierra PHY driver (which will be added as a follow-up),
> introduce 'base_offset' and 'reg_offset_shift', two values that can be
> used to adjust the final regmap read/write address.
>
> 'base_offset' is an extra offset on the regmap base discovered from the
> device tree. This is useful when some regmaps of a device need to access
> memory at one offset, and some at another.
>
> 'reg_offset_shift' will shift the register offset left before access.
>
> 'size' can be used to specify the width of reads (1/2/4/8 bytes). If 0,
> defaults to 4 bytes to maintain backward compatibility.
>
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/core/regmap.c | 16 ++++++++++++----
>  include/regmap.h      | 27 ++++++++++++++++++++++++++-
>  2 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> index 41293e5af0..9bce719b45 100644
> --- a/drivers/core/regmap.c
> +++ b/drivers/core/regmap.c
> @@ -38,6 +38,7 @@ static struct regmap *regmap_alloc(int count)
>         map = malloc(sizeof(*map) + sizeof(map->ranges[0]) * count);
>         if (!map)
>                 return NULL;
> +       memset(map, 0, sizeof(*map));
>         map->range_count = count;
>
>         return map;
> @@ -258,6 +259,10 @@ struct regmap *devm_regmap_init(struct udevice *dev,
>         if (rc)
>                 return ERR_PTR(rc);
>
> +       (*mapp)->base_offset = config->base_offset;
> +       (*mapp)->reg_offset_shift = config->reg_offset_shift;
> +       (*mapp)->size = config->size;
> +
>         devres_add(dev, mapp);
>         return *mapp;
>  }
> @@ -343,7 +348,9 @@ int regmap_raw_read_range(struct regmap *map, uint range_num, uint offset,
>         }
>         range = &map->ranges[range_num];
>
> -       ptr = map_physmem(range->start + offset, val_len, MAP_NOCACHE);
> +       offset <<= map->reg_offset_shift;
> +
> +       ptr = map_physmem(range->start + map->base_offset + offset, val_len, MAP_NOCACHE);
>
>         if (offset + val_len > range->size) {
>                 debug("%s: offset/size combination invalid\n", __func__);
> @@ -380,7 +387,7 @@ int regmap_raw_read(struct regmap *map, uint offset, void *valp, size_t val_len)
>
>  int regmap_read(struct regmap *map, uint offset, uint *valp)
>  {
> -       return regmap_raw_read(map, offset, valp, REGMAP_SIZE_32);
> +       return regmap_raw_read(map, offset, valp, map->size ? map->size : REGMAP_SIZE_32);
>  }
>
>  static inline void __write_8(u8 *addr, const u8 *val,
> @@ -452,7 +459,8 @@ int regmap_raw_write_range(struct regmap *map, uint range_num, uint offset,
>         }
>         range = &map->ranges[range_num];
>
> -       ptr = map_physmem(range->start + offset, val_len, MAP_NOCACHE);
> +       offset <<= map->reg_offset_shift;
> +       ptr = map_physmem(range->start + map->base_offset + offset, val_len, MAP_NOCACHE);
>
>         if (offset + val_len > range->size) {
>                 debug("%s: offset/size combination invalid\n", __func__);
> @@ -490,7 +498,7 @@ int regmap_raw_write(struct regmap *map, uint offset, const void *val,
>
>  int regmap_write(struct regmap *map, uint offset, uint val)
>  {
> -       return regmap_raw_write(map, offset, &val, REGMAP_SIZE_32);
> +       return regmap_raw_write(map, offset, &val, map->size ? map->size : REGMAP_SIZE_32);
>  }
>
>  int regmap_update_bits(struct regmap *map, uint offset, uint mask, uint val)
> diff --git a/include/regmap.h b/include/regmap.h
> index 2edbf9359a..4adb04f7e3 100644
> --- a/include/regmap.h
> +++ b/include/regmap.h
> @@ -74,16 +74,41 @@ struct regmap_range {
>  };
>
>  struct regmap_bus;
> -struct regmap_config;
> +/**
> + * struct regmap_config - a way of accessing hardware/bus registers
> + *
> + * @base_offset: Offset from the base of the regmap for reads and writes.
> + *              (OPTIONAL)
> + * @reg_offset_shift: Left shift register offset by this value before
> + *                   performing reads or writes. (OPTIONAL)
> + * @size: Size/width of the read or write operation. Defaults to
> + *       REGMAP_SIZE_32 if set to zero.
> + */
> +struct regmap_config {
> +       u32 base_offset;
> +       u32 reg_offset_shift;
> +       enum regmap_size_t size;
> +};
>
>  /**
>   * struct regmap - a way of accessing hardware/bus registers
>   *
> + * @base_offset: Offset from the base of the regmap for reads and writes.
> + *              (OPTIONAL)
> + * @reg_offset_shift: Left shift register offset by this value before
> + *                   performing reads or writes. (OPTIONAL)
> + * @size: Size/width of the read or write operation. Defaults to
> + *       REGMAP_SIZE_32 if set to zero.
>   * @range_count:       Number of ranges available within the map
>   * @ranges:            Array of ranges
>   */
>  struct regmap {
>         enum regmap_endianness_t endianness;
> +
> +       u32 base_offset;
> +       u32 reg_offset_shift;
> +       enum regmap_size_t size;
> +
>         int range_count;
>         struct regmap_range ranges[0];
>  };
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments India