All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 0/4] Add Linux-compatible syscon_to_regmap API
@ 2018-04-18  2:38 Masahiro Yamada
  2018-04-18  2:38 ` [U-Boot] [PATCH 1/4] regmap: clean up regmap allocation Masahiro Yamada
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Masahiro Yamada @ 2018-04-18  2:38 UTC (permalink / raw)
  To: u-boot

The current syscon in U-Boot works differently from Linux.
Therefore, DT files imported from Linux do not work for U-Boot.

The current usage of syscon in U-Boot should be discouraged because
using different DT-binding across projects is a significant problem.



Masahiro Yamada (4):
  regmap: clean up regmap allocation
  dm: ofnode: add ofnode_device_is_compatible() helper
  regmap: change regmap_init_mem() to take ofnode instead udevice
  syscon: add Linux-compatible syscon API

 arch/arm/mach-aspeed/ast2500/sdram_ast2500.c |  2 +-
 drivers/core/device.c                        |  8 +---
 drivers/core/ofnode.c                        | 11 +++++
 drivers/core/regmap.c                        | 42 ++++++------------
 drivers/core/syscon-uclass.c                 | 65 +++++++++++++++++++++++++++-
 drivers/phy/meson-gxl-usb2.c                 |  2 +-
 drivers/phy/meson-gxl-usb3.c                 |  2 +-
 drivers/ram/rockchip/dmc-rk3368.c            |  2 +-
 drivers/ram/rockchip/sdram_rk3188.c          |  2 +-
 drivers/ram/rockchip/sdram_rk322x.c          |  2 +-
 drivers/ram/rockchip/sdram_rk3288.c          |  2 +-
 drivers/ram/rockchip/sdram_rk3399.c          |  2 +-
 drivers/ram/stm32mp1/stm32mp1_ram.c          |  2 +-
 drivers/reset/reset-meson.c                  |  2 +-
 include/dm/ofnode.h                          | 11 +++++
 include/regmap.h                             | 11 ++---
 include/syscon.h                             |  8 ++++
 17 files changed, 123 insertions(+), 53 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 1/4] regmap: clean up regmap allocation
  2018-04-18  2:38 [U-Boot] [PATCH 0/4] Add Linux-compatible syscon_to_regmap API Masahiro Yamada
@ 2018-04-18  2:38 ` Masahiro Yamada
  2018-04-22 20:11   ` Simon Glass
  2018-04-18  2:38 ` [U-Boot] [PATCH 2/4] dm: ofnode: add ofnode_device_is_compatible() helper Masahiro Yamada
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ messages in thread
From: Masahiro Yamada @ 2018-04-18  2:38 UTC (permalink / raw)
  To: u-boot

Putting zero length array at the end of struct is a common technique
to embed arbitrary length of members.  There is no good reason to let
regmap_alloc_count() branch by "if (count <= 1)".

As far as I understood the code, regmap->base is an alias of
regmap->ranges[0].start, but it is not helpful but make the code
just ugly.

Rename regmap_alloc_count() to regmap_alloc() because the _count
suffix seems pointless.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/core/regmap.c | 31 +++++++++----------------------
 include/regmap.h      |  7 ++-----
 2 files changed, 11 insertions(+), 27 deletions(-)

diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
index 8a0e00f..6c3dbe9 100644
--- a/drivers/core/regmap.c
+++ b/drivers/core/regmap.c
@@ -18,22 +18,13 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-static struct regmap *regmap_alloc_count(int count)
+static struct regmap *regmap_alloc(int count)
 {
 	struct regmap *map;
 
-	map = malloc(sizeof(struct regmap));
+	map = malloc(sizeof(*map) + sizeof(map->ranges[0]) * count);
 	if (!map)
 		return NULL;
-	if (count <= 1) {
-		map->range = &map->base_range;
-	} else {
-		map->range = malloc(count * sizeof(struct regmap_range));
-		if (!map->range) {
-			free(map);
-			return NULL;
-		}
-	}
 	map->range_count = count;
 
 	return map;
@@ -46,12 +37,11 @@ int regmap_init_mem_platdata(struct udevice *dev, fdt_val_t *reg, int count,
 	struct regmap_range *range;
 	struct regmap *map;
 
-	map = regmap_alloc_count(count);
+	map = regmap_alloc(count);
 	if (!map)
 		return -ENOMEM;
 
-	map->base = *reg;
-	for (range = map->range; count > 0; reg += 2, range++, count--) {
+	for (range = map->ranges; count > 0; reg += 2, range++, count--) {
 		range->start = *reg;
 		range->size = reg[1];
 	}
@@ -84,11 +74,11 @@ int regmap_init_mem(struct udevice *dev, struct regmap **mapp)
 	if (!count)
 		return -EINVAL;
 
-	map = regmap_alloc_count(count);
+	map = regmap_alloc(count);
 	if (!map)
 		return -ENOMEM;
 
-	for (range = map->range, index = 0; count > 0;
+	for (range = map->ranges, index = 0; count > 0;
 	     count--, range++, index++) {
 		fdt_size_t sz;
 		if (of_live_active()) {
@@ -102,7 +92,6 @@ int regmap_init_mem(struct udevice *dev, struct regmap **mapp)
 			range->size = sz;
 		}
 	}
-	map->base = map->range[0].start;
 
 	*mapp = map;
 
@@ -116,15 +105,13 @@ void *regmap_get_range(struct regmap *map, unsigned int range_num)
 
 	if (range_num >= map->range_count)
 		return NULL;
-	range = &map->range[range_num];
+	range = &map->ranges[range_num];
 
 	return map_sysmem(range->start, range->size);
 }
 
 int regmap_uninit(struct regmap *map)
 {
-	if (map->range_count > 1)
-		free(map->range);
 	free(map);
 
 	return 0;
@@ -132,7 +119,7 @@ int regmap_uninit(struct regmap *map)
 
 int regmap_read(struct regmap *map, uint offset, uint *valp)
 {
-	uint32_t *ptr = map_physmem(map->base + offset, 4, MAP_NOCACHE);
+	u32 *ptr = map_physmem(map->ranges[0].start + offset, 4, MAP_NOCACHE);
 
 	*valp = le32_to_cpu(readl(ptr));
 
@@ -141,7 +128,7 @@ int regmap_read(struct regmap *map, uint offset, uint *valp)
 
 int regmap_write(struct regmap *map, uint offset, uint val)
 {
-	uint32_t *ptr = map_physmem(map->base + offset, 4, MAP_NOCACHE);
+	u32 *ptr = map_physmem(map->ranges[0].start + offset, 4, MAP_NOCACHE);
 
 	writel(cpu_to_le32(val), ptr);
 
diff --git a/include/regmap.h b/include/regmap.h
index 493a5d8..858aa7e 100644
--- a/include/regmap.h
+++ b/include/regmap.h
@@ -22,15 +22,12 @@ struct regmap_range {
 /**
  * struct regmap - a way of accessing hardware/bus registers
  *
- * @base:	Base address of register map
  * @range_count: Number of ranges available within the map
- * @range:	Pointer to the list of ranges, allocated if @range_count > 1
- * @base_range:	If @range_count is <= 1, @range points here
+ * @ranges:	Array of ranges
  */
 struct regmap {
-	phys_addr_t base;
 	int range_count;
-	struct regmap_range *range, base_range;
+	struct regmap_range ranges[];
 };
 
 /*
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 2/4] dm: ofnode: add ofnode_device_is_compatible() helper
  2018-04-18  2:38 [U-Boot] [PATCH 0/4] Add Linux-compatible syscon_to_regmap API Masahiro Yamada
  2018-04-18  2:38 ` [U-Boot] [PATCH 1/4] regmap: clean up regmap allocation Masahiro Yamada
@ 2018-04-18  2:38 ` Masahiro Yamada
  2018-04-22 20:11   ` Simon Glass
  2018-04-18  2:38 ` [U-Boot] [PATCH 3/4] regmap: change regmap_init_mem() to take ofnode instead udevice Masahiro Yamada
  2018-04-18  2:38 ` [U-Boot] [PATCH 4/4] syscon: add Linux-compatible syscon API Masahiro Yamada
  3 siblings, 1 reply; 16+ messages in thread
From: Masahiro Yamada @ 2018-04-18  2:38 UTC (permalink / raw)
  To: u-boot

device_is_compatible() takes udevice, but there is no such a helper
that takes ofnode.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/core/device.c |  8 +-------
 drivers/core/ofnode.c | 11 +++++++++++
 include/dm/ofnode.h   | 11 +++++++++++
 3 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/core/device.c b/drivers/core/device.c
index 940a153..8d95787 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -709,13 +709,7 @@ int device_set_name(struct udevice *dev, const char *name)
 
 bool device_is_compatible(struct udevice *dev, const char *compat)
 {
-	const void *fdt = gd->fdt_blob;
-	ofnode node = dev_ofnode(dev);
-
-	if (ofnode_is_np(node))
-		return of_device_is_compatible(ofnode_to_np(node), compat, NULL, NULL);
-	else
-		return !fdt_node_check_compatible(fdt, ofnode_to_offset(node), compat);
+	return ofnode_device_is_compatible(dev_ofnode(dev), compat);
 }
 
 bool of_machine_is_compatible(const char *compat)
diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 5909a25..ee2109b 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -687,3 +687,14 @@ u64 ofnode_translate_address(ofnode node, const fdt32_t *in_addr)
 	else
 		return fdt_translate_address(gd->fdt_blob, ofnode_to_offset(node), in_addr);
 }
+
+int ofnode_device_is_compatible(ofnode node, const char *compat)
+{
+	if (ofnode_is_np(node))
+		return of_device_is_compatible(ofnode_to_np(node), compat,
+					       NULL, NULL);
+	else
+		return !fdt_node_check_compatible(gd->fdt_blob,
+						  ofnode_to_offset(node),
+						  compat);
+}
diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
index 0d00840..a2c6a50 100644
--- a/include/dm/ofnode.h
+++ b/include/dm/ofnode.h
@@ -681,4 +681,15 @@ int ofnode_read_resource_byname(ofnode node, const char *name,
  * @return the translated address; OF_BAD_ADDR on error
  */
 u64 ofnode_translate_address(ofnode node, const fdt32_t *in_addr);
+
+/**
+ * ofnode_device_is_compatible() - check if the node is compatible with compat
+ *
+ * This allows to check whether the node is comaptible with the compat.
+ *
+ * @node:	Device tree node for which compatible needs to be verified.
+ * @compat:	Compatible string which needs to verified in the given node.
+ * @return true if OK, false if the compatible is not found
+ */
+int ofnode_device_is_compatible(ofnode node, const char *compat);
 #endif
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 3/4] regmap: change regmap_init_mem() to take ofnode instead udevice
  2018-04-18  2:38 [U-Boot] [PATCH 0/4] Add Linux-compatible syscon_to_regmap API Masahiro Yamada
  2018-04-18  2:38 ` [U-Boot] [PATCH 1/4] regmap: clean up regmap allocation Masahiro Yamada
  2018-04-18  2:38 ` [U-Boot] [PATCH 2/4] dm: ofnode: add ofnode_device_is_compatible() helper Masahiro Yamada
@ 2018-04-18  2:38 ` Masahiro Yamada
  2018-04-18 15:34   ` Neil Armstrong
  2018-04-22 20:10   ` Simon Glass
  2018-04-18  2:38 ` [U-Boot] [PATCH 4/4] syscon: add Linux-compatible syscon API Masahiro Yamada
  3 siblings, 2 replies; 16+ messages in thread
From: Masahiro Yamada @ 2018-04-18  2:38 UTC (permalink / raw)
  To: u-boot

Currently, regmap_init_mem() takes udevice. This requires the node
has already been associated with a device. It prevents syscon/regmap
from behaving like those in Linux.

Change the first argumenet to take the device node.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 arch/arm/mach-aspeed/ast2500/sdram_ast2500.c |  2 +-
 drivers/core/regmap.c                        | 11 +++++------
 drivers/core/syscon-uclass.c                 |  2 +-
 drivers/phy/meson-gxl-usb2.c                 |  2 +-
 drivers/phy/meson-gxl-usb3.c                 |  2 +-
 drivers/ram/rockchip/dmc-rk3368.c            |  2 +-
 drivers/ram/rockchip/sdram_rk3188.c          |  2 +-
 drivers/ram/rockchip/sdram_rk322x.c          |  2 +-
 drivers/ram/rockchip/sdram_rk3288.c          |  2 +-
 drivers/ram/rockchip/sdram_rk3399.c          |  2 +-
 drivers/ram/stm32mp1/stm32mp1_ram.c          |  2 +-
 drivers/reset/reset-meson.c                  |  2 +-
 include/regmap.h                             |  4 ++--
 13 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-aspeed/ast2500/sdram_ast2500.c b/arch/arm/mach-aspeed/ast2500/sdram_ast2500.c
index 6383f72..f267c58 100644
--- a/arch/arm/mach-aspeed/ast2500/sdram_ast2500.c
+++ b/arch/arm/mach-aspeed/ast2500/sdram_ast2500.c
@@ -392,7 +392,7 @@ static int ast2500_sdrammc_ofdata_to_platdata(struct udevice *dev)
 	struct regmap *map;
 	int ret;
 
-	ret = regmap_init_mem(dev, &map);
+	ret = regmap_init_mem(dev_ofnode(dev), &map);
 	if (ret)
 		return ret;
 
diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
index 6c3dbe9..1185a44 100644
--- a/drivers/core/regmap.c
+++ b/drivers/core/regmap.c
@@ -51,7 +51,7 @@ int regmap_init_mem_platdata(struct udevice *dev, fdt_val_t *reg, int count,
 	return 0;
 }
 #else
-int regmap_init_mem(struct udevice *dev, struct regmap **mapp)
+int regmap_init_mem(ofnode node, struct regmap **mapp)
 {
 	struct regmap_range *range;
 	struct regmap *map;
@@ -59,14 +59,13 @@ int regmap_init_mem(struct udevice *dev, struct regmap **mapp)
 	int addr_len, size_len, both_len;
 	int len;
 	int index;
-	ofnode node = dev_ofnode(dev);
 	struct resource r;
 
-	addr_len = dev_read_simple_addr_cells(dev->parent);
-	size_len = dev_read_simple_size_cells(dev->parent);
+	addr_len = ofnode_read_simple_addr_cells(ofnode_get_parent(node));
+	size_len = ofnode_read_simple_size_cells(ofnode_get_parent(node));
 	both_len = addr_len + size_len;
 
-	len = dev_read_size(dev, "reg");
+	len = ofnode_read_size(node, "reg");
 	if (len < 0)
 		return len;
 	len /= sizeof(fdt32_t);
@@ -87,7 +86,7 @@ int regmap_init_mem(struct udevice *dev, struct regmap **mapp)
 			range->size = r.end - r.start + 1;
 		} else {
 			range->start = fdtdec_get_addr_size_fixed(gd->fdt_blob,
-					dev_of_offset(dev), "reg", index,
+					ofnode_to_offset(node), "reg", index,
 					addr_len, size_len, &sz, true);
 			range->size = sz;
 		}
diff --git a/drivers/core/syscon-uclass.c b/drivers/core/syscon-uclass.c
index a69937e..c99409b 100644
--- a/drivers/core/syscon-uclass.c
+++ b/drivers/core/syscon-uclass.c
@@ -41,7 +41,7 @@ static int syscon_pre_probe(struct udevice *dev)
 	return regmap_init_mem_platdata(dev, plat->reg, ARRAY_SIZE(plat->reg),
 					&priv->regmap);
 #else
-	return regmap_init_mem(dev, &priv->regmap);
+	return regmap_init_mem(dev_ofnode(dev), &priv->regmap);
 #endif
 }
 
diff --git a/drivers/phy/meson-gxl-usb2.c b/drivers/phy/meson-gxl-usb2.c
index 15c9c89..7242bf6 100644
--- a/drivers/phy/meson-gxl-usb2.c
+++ b/drivers/phy/meson-gxl-usb2.c
@@ -195,7 +195,7 @@ int meson_gxl_usb2_phy_probe(struct udevice *dev)
 	struct phy_meson_gxl_usb2_priv *priv = dev_get_priv(dev);
 	int ret;
 
-	ret = regmap_init_mem(dev, &priv->regmap);
+	ret = regmap_init_mem(dev_ofnode(dev), &priv->regmap);
 	if (ret)
 		return ret;
 
diff --git a/drivers/phy/meson-gxl-usb3.c b/drivers/phy/meson-gxl-usb3.c
index a385fbd..47a41fd 100644
--- a/drivers/phy/meson-gxl-usb3.c
+++ b/drivers/phy/meson-gxl-usb3.c
@@ -166,7 +166,7 @@ int meson_gxl_usb3_phy_probe(struct udevice *dev)
 	struct phy_meson_gxl_usb3_priv *priv = dev_get_priv(dev);
 	int ret;
 
-	ret = regmap_init_mem(dev, &priv->regmap);
+	ret = regmap_init_mem(dev_ofnode(dev), &priv->regmap);
 	if (ret)
 		return ret;
 	
diff --git a/drivers/ram/rockchip/dmc-rk3368.c b/drivers/ram/rockchip/dmc-rk3368.c
index bfcb1dd..9bf64bf 100644
--- a/drivers/ram/rockchip/dmc-rk3368.c
+++ b/drivers/ram/rockchip/dmc-rk3368.c
@@ -880,7 +880,7 @@ static int rk3368_dmc_ofdata_to_platdata(struct udevice *dev)
 #if !CONFIG_IS_ENABLED(OF_PLATDATA)
 	struct rk3368_sdram_params *plat = dev_get_platdata(dev);
 
-	ret = regmap_init_mem(dev, &plat->map);
+	ret = regmap_init_mem(dev_ofnode(dev), &plat->map);
 	if (ret)
 		return ret;
 #endif
diff --git a/drivers/ram/rockchip/sdram_rk3188.c b/drivers/ram/rockchip/sdram_rk3188.c
index 365d00e..c0a2618 100644
--- a/drivers/ram/rockchip/sdram_rk3188.c
+++ b/drivers/ram/rockchip/sdram_rk3188.c
@@ -842,7 +842,7 @@ static int rk3188_dmc_ofdata_to_platdata(struct udevice *dev)
 		printf("%s: Cannot read rockchip,sdram-params\n", __func__);
 		return -EINVAL;
 	}
-	ret = regmap_init_mem(dev, &params->map);
+	ret = regmap_init_mem(dev_ofnode(dev), &params->map);
 	if (ret)
 		return ret;
 #endif
diff --git a/drivers/ram/rockchip/sdram_rk322x.c b/drivers/ram/rockchip/sdram_rk322x.c
index cc3138b..dca07db 100644
--- a/drivers/ram/rockchip/sdram_rk322x.c
+++ b/drivers/ram/rockchip/sdram_rk322x.c
@@ -744,7 +744,7 @@ static int rk322x_dmc_ofdata_to_platdata(struct udevice *dev)
 		printf("%s: Cannot read rockchip,sdram-params\n", __func__);
 		return -EINVAL;
 	}
-	ret = regmap_init_mem(dev, &params->map);
+	ret = regmap_init_mem(dev_ofnode(dev), &params->map);
 	if (ret)
 		return ret;
 #endif
diff --git a/drivers/ram/rockchip/sdram_rk3288.c b/drivers/ram/rockchip/sdram_rk3288.c
index 95efb11..ffb663c 100644
--- a/drivers/ram/rockchip/sdram_rk3288.c
+++ b/drivers/ram/rockchip/sdram_rk3288.c
@@ -1003,7 +1003,7 @@ static int rk3288_dmc_ofdata_to_platdata(struct udevice *dev)
 
 	priv->is_veyron = !fdt_node_check_compatible(blob, 0, "google,veyron");
 #endif
-	ret = regmap_init_mem(dev, &params->map);
+	ret = regmap_init_mem(dev_ofnode(dev), &params->map);
 	if (ret)
 		return ret;
 #endif
diff --git a/drivers/ram/rockchip/sdram_rk3399.c b/drivers/ram/rockchip/sdram_rk3399.c
index 5cb470c..6e2a39e 100644
--- a/drivers/ram/rockchip/sdram_rk3399.c
+++ b/drivers/ram/rockchip/sdram_rk3399.c
@@ -1100,7 +1100,7 @@ static int rk3399_dmc_ofdata_to_platdata(struct udevice *dev)
 		       __func__, ret);
 		return ret;
 	}
-	ret = regmap_init_mem(dev, &plat->map);
+	ret = regmap_init_mem(dev_ofnode(dev), &plat->map);
 	if (ret)
 		printf("%s: regmap failed %d\n", __func__, ret);
 
diff --git a/drivers/ram/stm32mp1/stm32mp1_ram.c b/drivers/ram/stm32mp1/stm32mp1_ram.c
index 9599444..a4d5906 100644
--- a/drivers/ram/stm32mp1/stm32mp1_ram.c
+++ b/drivers/ram/stm32mp1/stm32mp1_ram.c
@@ -149,7 +149,7 @@ static int stm32mp1_ddr_probe(struct udevice *dev)
 	debug("STM32MP1 DDR probe\n");
 	priv->dev = dev;
 
-	ret = regmap_init_mem(dev, &map);
+	ret = regmap_init_mem(dev_ofnode(dev), &map);
 	if (ret)
 		return ret;
 
diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
index 5324f86..c41d176 100644
--- a/drivers/reset/reset-meson.c
+++ b/drivers/reset/reset-meson.c
@@ -77,7 +77,7 @@ static int meson_reset_probe(struct udevice *dev)
 {
 	struct meson_reset_priv *priv = dev_get_priv(dev);
 	
-	return regmap_init_mem(dev, &priv->regmap);
+	return regmap_init_mem(dev_ofnode(dev), &priv->regmap);
 }
 
 U_BOOT_DRIVER(meson_reset) = {
diff --git a/include/regmap.h b/include/regmap.h
index 858aa7e..c8a1df0 100644
--- a/include/regmap.h
+++ b/include/regmap.h
@@ -48,10 +48,10 @@ int regmap_read(struct regmap *map, uint offset, uint *valp);
  *
  * Use regmap_uninit() to free it.
  *
- * @dev:	Device that uses this map
+ * @node:	Device node that uses this map
  * @mapp:	Returns allocated map
  */
-int regmap_init_mem(struct udevice *dev, struct regmap **mapp);
+int regmap_init_mem(ofnode node, struct regmap **mapp);
 
 /**
  * regmap_init_mem_platdata() - Set up a new memory register map for of-platdata
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 4/4] syscon: add Linux-compatible syscon API
  2018-04-18  2:38 [U-Boot] [PATCH 0/4] Add Linux-compatible syscon_to_regmap API Masahiro Yamada
                   ` (2 preceding siblings ...)
  2018-04-18  2:38 ` [U-Boot] [PATCH 3/4] regmap: change regmap_init_mem() to take ofnode instead udevice Masahiro Yamada
@ 2018-04-18  2:38 ` Masahiro Yamada
  2018-04-22 20:11   ` Simon Glass
  3 siblings, 1 reply; 16+ messages in thread
From: Masahiro Yamada @ 2018-04-18  2:38 UTC (permalink / raw)
  To: u-boot

The syscon implementation in U-Boot is different from that in Linux.
Thus, DT files imported from Linux do not work for U-Boot.

In U-Boot driver model, each node is bound to a dedicated driver
that is the most compatible to it.  This design gets along with the
concept of DT, and the syscon in Linux originally worked like that.

However, Linux commit bdb0066df96e ("mfd: syscon: Decouple syscon
interface from platform devices") changed the behavior because it is
useful to let a device bind to another driver, but still works as a
syscon provider.

That change had happened before U-Boot initially supported the syscon
driver by commit 6f98b7504f70 ("dm: Add support for generic system
controllers (syscon)").  So, the U-Boot's syscon works differently
from the beginning.  I'd say this is mis-implementation given that
DT is not oriented to a particular project, but Linux is the canon
of DT in practice.

The problem typically arises in the combination of "syscon" and
"simple-mfd" compatibles.

In Linux, they are orthogonal, i.e., the order between "syscon" and
"simple-mfd" does not matter at all.

Assume the following compatible.

   compatible = "foo,bar-syscon", "syscon", "simple-mfd";

In U-Boot, this device node is bound to the syscon driver
(driver/core/syscon-uclass.c) since the "syscon" is found to be the
most compatible.  Then, syscon_get_regmap() succeeds.

However,

   compatible = "foo,bar-syscon", "simple-mfd", "syscon";

does not work because this node is bound to the simple-bus driver
(drivers/core/simple-bus.c) in the favor of "simple-mfd" compatible.
The compatible string "syscon" is just dismissed.

Moreover,

   compatible = "foo,bar-syscon", "syscon";

works like the first case because the syscon driver populates the
child devices.  This is wrong because populating children is the job
of "simple-mfd" (or "simple-bus").

This commit ports syscon_node_to_regmap() from Linux.  This API
does not require the given node to be bound to a driver in any way.

Reported-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/core/syscon-uclass.c | 63 ++++++++++++++++++++++++++++++++++++++++++++
 include/syscon.h             |  8 ++++++
 2 files changed, 71 insertions(+)

diff --git a/drivers/core/syscon-uclass.c b/drivers/core/syscon-uclass.c
index c99409b..bd2f681 100644
--- a/drivers/core/syscon-uclass.c
+++ b/drivers/core/syscon-uclass.c
@@ -15,6 +15,14 @@
 #include <dm/root.h>
 #include <linux/err.h>
 
+/*
+ * Caution:
+ * This API requires the given node has already been bound to "syscon".
+ *    compatible = "syscon", "simple-mfd";
+ * works, but
+ *    compatible = "simple-mfd", "syscon";
+ * does not.  This behavior is different from Linux.
+ */
 struct regmap *syscon_get_regmap(struct udevice *dev)
 {
 	struct syscon_uc_info *priv;
@@ -109,3 +117,58 @@ U_BOOT_DRIVER(generic_syscon) = {
 #endif
 	.of_match = generic_syscon_ids,
 };
+
+/*
+ * Linux-compatible syscon-to-regmap
+ * The syscon node can be bound to another driver, but still works
+ * as a syscon provider.
+ */
+static LIST_HEAD(syscon_list);
+
+struct syscon {
+	ofnode node;
+	struct regmap *regmap;
+	struct list_head list;
+};
+
+static struct syscon *of_syscon_register(ofnode node)
+{
+	struct syscon *syscon;
+	int ret;
+
+	if (!ofnode_device_is_compatible(node, "syscon"))
+		return ERR_PTR(-EINVAL);
+
+	syscon = malloc(sizeof(*syscon));
+	if (!syscon)
+		return ERR_PTR(-ENOMEM);
+
+	ret = regmap_init_mem(node, &syscon->regmap);
+	if (ret) {
+		free(syscon);
+		return ERR_PTR(ret);
+	}
+
+	list_add_tail(&syscon->list, &syscon_list);
+
+	return syscon;
+}
+
+struct regmap *syscon_node_to_regmap(ofnode node)
+{
+	struct syscon *entry, *syscon = NULL;
+
+	list_for_each_entry(entry, &syscon_list, list)
+		if (ofnode_equal(entry->node, node)) {
+			syscon = entry;
+			break;
+		}
+
+	if (!syscon)
+		syscon = of_syscon_register(node);
+
+	if (IS_ERR(syscon))
+		return ERR_CAST(syscon);
+
+	return syscon->regmap;
+}
diff --git a/include/syscon.h b/include/syscon.h
index 5d52b1c..a019345 100644
--- a/include/syscon.h
+++ b/include/syscon.h
@@ -8,6 +8,7 @@
 #ifndef __SYSCON_H
 #define __SYSCON_H
 
+#include <dm/ofnode.h>
 #include <fdtdec.h>
 
 /**
@@ -82,4 +83,11 @@ struct regmap *syscon_get_regmap_by_driver_data(ulong driver_data);
  */
 void *syscon_get_first_range(ulong driver_data);
 
+/**
+ * syscon_node_to_regmap - Linux-compat syscon lookup
+ *
+ * @node:		Device node of syscon
+ */
+struct regmap *syscon_node_to_regmap(ofnode node);
+
 #endif
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 3/4] regmap: change regmap_init_mem() to take ofnode instead udevice
  2018-04-18  2:38 ` [U-Boot] [PATCH 3/4] regmap: change regmap_init_mem() to take ofnode instead udevice Masahiro Yamada
@ 2018-04-18 15:34   ` Neil Armstrong
  2018-04-22 20:10   ` Simon Glass
  1 sibling, 0 replies; 16+ messages in thread
From: Neil Armstrong @ 2018-04-18 15:34 UTC (permalink / raw)
  To: u-boot

On 18/04/2018 04:38, Masahiro Yamada wrote:
> Currently, regmap_init_mem() takes udevice. This requires the node
> has already been associated with a device. It prevents syscon/regmap
> from behaving like those in Linux.
> 
> Change the first argumenet to take the device node.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
>  arch/arm/mach-aspeed/ast2500/sdram_ast2500.c |  2 +-
>  drivers/core/regmap.c                        | 11 +++++------
>  drivers/core/syscon-uclass.c                 |  2 +-
>  drivers/phy/meson-gxl-usb2.c                 |  2 +-
>  drivers/phy/meson-gxl-usb3.c                 |  2 +-
>  drivers/ram/rockchip/dmc-rk3368.c            |  2 +-
>  drivers/ram/rockchip/sdram_rk3188.c          |  2 +-
>  drivers/ram/rockchip/sdram_rk322x.c          |  2 +-
>  drivers/ram/rockchip/sdram_rk3288.c          |  2 +-
>  drivers/ram/rockchip/sdram_rk3399.c          |  2 +-
>  drivers/ram/stm32mp1/stm32mp1_ram.c          |  2 +-
>  drivers/reset/reset-meson.c                  |  2 +-
>  include/regmap.h                             |  4 ++--
>  13 files changed, 18 insertions(+), 19 deletions(-)
> 
[..]
> diff --git a/drivers/phy/meson-gxl-usb2.c b/drivers/phy/meson-gxl-usb2.c
> index 15c9c89..7242bf6 100644
> --- a/drivers/phy/meson-gxl-usb2.c
> +++ b/drivers/phy/meson-gxl-usb2.c
> @@ -195,7 +195,7 @@ int meson_gxl_usb2_phy_probe(struct udevice *dev)
>  	struct phy_meson_gxl_usb2_priv *priv = dev_get_priv(dev);
>  	int ret;
>  
> -	ret = regmap_init_mem(dev, &priv->regmap);
> +	ret = regmap_init_mem(dev_ofnode(dev), &priv->regmap);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/phy/meson-gxl-usb3.c b/drivers/phy/meson-gxl-usb3.c
> index a385fbd..47a41fd 100644
> --- a/drivers/phy/meson-gxl-usb3.c
> +++ b/drivers/phy/meson-gxl-usb3.c
> @@ -166,7 +166,7 @@ int meson_gxl_usb3_phy_probe(struct udevice *dev)
>  	struct phy_meson_gxl_usb3_priv *priv = dev_get_priv(dev);
>  	int ret;
>  
> -	ret = regmap_init_mem(dev, &priv->regmap);
> +	ret = regmap_init_mem(dev_ofnode(dev), &priv->regmap);
>  	if (ret)
>  		return ret;
>  	
[..]
> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
> index 5324f86..c41d176 100644
> --- a/drivers/reset/reset-meson.c
> +++ b/drivers/reset/reset-meson.c
> @@ -77,7 +77,7 @@ static int meson_reset_probe(struct udevice *dev)
>  {
>  	struct meson_reset_priv *priv = dev_get_priv(dev);
>  	
> -	return regmap_init_mem(dev, &priv->regmap);
> +	return regmap_init_mem(dev_ofnode(dev), &priv->regmap);
>  }
>  
>  U_BOOT_DRIVER(meson_reset) = {

For reset-meson, meson-gxl-usb*

Acked-by: Neil Armstrong <narmstrong@baylibre.com>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 3/4] regmap: change regmap_init_mem() to take ofnode instead udevice
  2018-04-18  2:38 ` [U-Boot] [PATCH 3/4] regmap: change regmap_init_mem() to take ofnode instead udevice Masahiro Yamada
  2018-04-18 15:34   ` Neil Armstrong
@ 2018-04-22 20:10   ` Simon Glass
  2018-04-23  4:56     ` Masahiro Yamada
  1 sibling, 1 reply; 16+ messages in thread
From: Simon Glass @ 2018-04-22 20:10 UTC (permalink / raw)
  To: u-boot

Hi Masahiro,

On 17 April 2018 at 20:38, Masahiro Yamada <yamada.masahiro@socionext.com>
wrote:
> Currently, regmap_init_mem() takes udevice. This requires the node
> has already been associated with a device. It prevents syscon/regmap
> from behaving like those in Linux.
>
> Change the first argumenet to take the device node.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  arch/arm/mach-aspeed/ast2500/sdram_ast2500.c |  2 +-
>  drivers/core/regmap.c                        | 11 +++++------
>  drivers/core/syscon-uclass.c                 |  2 +-
>  drivers/phy/meson-gxl-usb2.c                 |  2 +-
>  drivers/phy/meson-gxl-usb3.c                 |  2 +-
>  drivers/ram/rockchip/dmc-rk3368.c            |  2 +-
>  drivers/ram/rockchip/sdram_rk3188.c          |  2 +-
>  drivers/ram/rockchip/sdram_rk322x.c          |  2 +-
>  drivers/ram/rockchip/sdram_rk3288.c          |  2 +-
>  drivers/ram/rockchip/sdram_rk3399.c          |  2 +-
>  drivers/ram/stm32mp1/stm32mp1_ram.c          |  2 +-
>  drivers/reset/reset-meson.c                  |  2 +-
>  include/regmap.h                             |  4 ++--
>  13 files changed, 18 insertions(+), 19 deletions(-)

Can you please add a simple test for regmap on sandbox?

Regards,
Simon

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 2/4] dm: ofnode: add ofnode_device_is_compatible() helper
  2018-04-18  2:38 ` [U-Boot] [PATCH 2/4] dm: ofnode: add ofnode_device_is_compatible() helper Masahiro Yamada
@ 2018-04-22 20:11   ` Simon Glass
  2018-04-23  4:50     ` Masahiro Yamada
  0 siblings, 1 reply; 16+ messages in thread
From: Simon Glass @ 2018-04-22 20:11 UTC (permalink / raw)
  To: u-boot

Hi Masahiro,

On 17 April 2018 at 20:38, Masahiro Yamada <yamada.masahiro@socionext.com>
wrote:
> device_is_compatible() takes udevice, but there is no such a helper
> that takes ofnode.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  drivers/core/device.c |  8 +-------
>  drivers/core/ofnode.c | 11 +++++++++++
>  include/dm/ofnode.h   | 11 +++++++++++
>  3 files changed, 23 insertions(+), 7 deletions(-)

Please can you add a call to this to a test?

Regards,
Simon

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 1/4] regmap: clean up regmap allocation
  2018-04-18  2:38 ` [U-Boot] [PATCH 1/4] regmap: clean up regmap allocation Masahiro Yamada
@ 2018-04-22 20:11   ` Simon Glass
  0 siblings, 0 replies; 16+ messages in thread
From: Simon Glass @ 2018-04-22 20:11 UTC (permalink / raw)
  To: u-boot

Hi Masahiro,

On 17 April 2018 at 20:38, Masahiro Yamada <yamada.masahiro@socionext.com>
wrote:
> Putting zero length array at the end of struct is a common technique
> to embed arbitrary length of members.  There is no good reason to let
> regmap_alloc_count() branch by "if (count <= 1)".
>
> As far as I understood the code, regmap->base is an alias of
> regmap->ranges[0].start, but it is not helpful but make the code
> just ugly.
>
> Rename regmap_alloc_count() to regmap_alloc() because the _count
> suffix seems pointless.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  drivers/core/regmap.c | 31 +++++++++----------------------
>  include/regmap.h      |  7 ++-----
>  2 files changed, 11 insertions(+), 27 deletions(-)

This seems fine to me and does not increase the number of allocations.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 4/4] syscon: add Linux-compatible syscon API
  2018-04-18  2:38 ` [U-Boot] [PATCH 4/4] syscon: add Linux-compatible syscon API Masahiro Yamada
@ 2018-04-22 20:11   ` Simon Glass
  2018-04-23  4:58     ` Masahiro Yamada
  0 siblings, 1 reply; 16+ messages in thread
From: Simon Glass @ 2018-04-22 20:11 UTC (permalink / raw)
  To: u-boot

Hi Masahiro,

On 17 April 2018 at 20:38, Masahiro Yamada <yamada.masahiro@socionext.com>
wrote:
> The syscon implementation in U-Boot is different from that in Linux.
> Thus, DT files imported from Linux do not work for U-Boot.
>
> In U-Boot driver model, each node is bound to a dedicated driver
> that is the most compatible to it.  This design gets along with the
> concept of DT, and the syscon in Linux originally worked like that.
>
> However, Linux commit bdb0066df96e ("mfd: syscon: Decouple syscon
> interface from platform devices") changed the behavior because it is
> useful to let a device bind to another driver, but still works as a
> syscon provider.
>
> That change had happened before U-Boot initially supported the syscon
> driver by commit 6f98b7504f70 ("dm: Add support for generic system
> controllers (syscon)").  So, the U-Boot's syscon works differently
> from the beginning.  I'd say this is mis-implementation given that
> DT is not oriented to a particular project, but Linux is the canon
> of DT in practice.
>
> The problem typically arises in the combination of "syscon" and
> "simple-mfd" compatibles.
>
> In Linux, they are orthogonal, i.e., the order between "syscon" and
> "simple-mfd" does not matter at all.
>
> Assume the following compatible.
>
>    compatible = "foo,bar-syscon", "syscon", "simple-mfd";
>
> In U-Boot, this device node is bound to the syscon driver
> (driver/core/syscon-uclass.c) since the "syscon" is found to be the
> most compatible.  Then, syscon_get_regmap() succeeds.
>
> However,
>
>    compatible = "foo,bar-syscon", "simple-mfd", "syscon";
>
> does not work because this node is bound to the simple-bus driver
> (drivers/core/simple-bus.c) in the favor of "simple-mfd" compatible.
> The compatible string "syscon" is just dismissed.
>
> Moreover,
>
>    compatible = "foo,bar-syscon", "syscon";
>
> works like the first case because the syscon driver populates the
> child devices.  This is wrong because populating children is the job
> of "simple-mfd" (or "simple-bus").
>
> This commit ports syscon_node_to_regmap() from Linux.  This API
> does not require the given node to be bound to a driver in any way.
>
> Reported-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  drivers/core/syscon-uclass.c | 63
++++++++++++++++++++++++++++++++++++++++++++
>  include/syscon.h             |  8 ++++++
>  2 files changed, 71 insertions(+)

I was not aware of this change in how Linux worked, but it makes sense.

Please can you add a test for this?

Regards,
Simon

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 2/4] dm: ofnode: add ofnode_device_is_compatible() helper
  2018-04-22 20:11   ` Simon Glass
@ 2018-04-23  4:50     ` Masahiro Yamada
  2018-04-25  5:01       ` Simon Glass
  0 siblings, 1 reply; 16+ messages in thread
From: Masahiro Yamada @ 2018-04-23  4:50 UTC (permalink / raw)
  To: u-boot

2018-04-23 5:11 GMT+09:00 Simon Glass <sjg@chromium.org>:
> Hi Masahiro,
>
> On 17 April 2018 at 20:38, Masahiro Yamada <yamada.masahiro@socionext.com>
> wrote:
>> device_is_compatible() takes udevice, but there is no such a helper
>> that takes ofnode.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  drivers/core/device.c |  8 +-------
>>  drivers/core/ofnode.c | 11 +++++++++++
>>  include/dm/ofnode.h   | 11 +++++++++++
>>  3 files changed, 23 insertions(+), 7 deletions(-)
>
> Please can you add a call to this to a test?
>

No.

I do not see any ofnode helper test in test/dm/.

You are requesting additional work beyond this patch.
It is unfair.


This helper is tested indirectly by other tests.

Of course, you (and anybody) are free to add per-helper grained tests,
but this is not a good reason to block this patch.




-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 3/4] regmap: change regmap_init_mem() to take ofnode instead udevice
  2018-04-22 20:10   ` Simon Glass
@ 2018-04-23  4:56     ` Masahiro Yamada
  2018-04-25  5:01       ` Simon Glass
  0 siblings, 1 reply; 16+ messages in thread
From: Masahiro Yamada @ 2018-04-23  4:56 UTC (permalink / raw)
  To: u-boot

Hi Simon,


2018-04-23 5:10 GMT+09:00 Simon Glass <sjg@chromium.org>:
> Hi Masahiro,
>
> On 17 April 2018 at 20:38, Masahiro Yamada <yamada.masahiro@socionext.com>
> wrote:
>> Currently, regmap_init_mem() takes udevice. This requires the node
>> has already been associated with a device. It prevents syscon/regmap
>> from behaving like those in Linux.
>>
>> Change the first argumenet to take the device node.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  arch/arm/mach-aspeed/ast2500/sdram_ast2500.c |  2 +-
>>  drivers/core/regmap.c                        | 11 +++++------
>>  drivers/core/syscon-uclass.c                 |  2 +-
>>  drivers/phy/meson-gxl-usb2.c                 |  2 +-
>>  drivers/phy/meson-gxl-usb3.c                 |  2 +-
>>  drivers/ram/rockchip/dmc-rk3368.c            |  2 +-
>>  drivers/ram/rockchip/sdram_rk3188.c          |  2 +-
>>  drivers/ram/rockchip/sdram_rk322x.c          |  2 +-
>>  drivers/ram/rockchip/sdram_rk3288.c          |  2 +-
>>  drivers/ram/rockchip/sdram_rk3399.c          |  2 +-
>>  drivers/ram/stm32mp1/stm32mp1_ram.c          |  2 +-
>>  drivers/reset/reset-meson.c                  |  2 +-
>>  include/regmap.h                             |  4 ++--
>>  13 files changed, 18 insertions(+), 19 deletions(-)
>
> Can you please add a simple test for regmap on sandbox?
>

No.

Please stop this boiler-plate response.

Simple tests for regmap already exist in test/dm/regmap.c

regmap_init_mem() is not a new function, and it is tested
through other function tests.

You block patches several times before
by making the contributors say "Sorry, I am busy".


I am really busy, but I need to fix the misimplementation
of syscon for Socionext drivers.

Why should I be annoyed by additional work
to fix the problem?


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 4/4] syscon: add Linux-compatible syscon API
  2018-04-22 20:11   ` Simon Glass
@ 2018-04-23  4:58     ` Masahiro Yamada
  2018-04-25  5:01       ` Simon Glass
  0 siblings, 1 reply; 16+ messages in thread
From: Masahiro Yamada @ 2018-04-23  4:58 UTC (permalink / raw)
  To: u-boot

Hi Simon,


2018-04-23 5:11 GMT+09:00 Simon Glass <sjg@chromium.org>:
> Hi Masahiro,
>
> On 17 April 2018 at 20:38, Masahiro Yamada <yamada.masahiro@socionext.com>
> wrote:
>> The syscon implementation in U-Boot is different from that in Linux.
>> Thus, DT files imported from Linux do not work for U-Boot.
>>
>> In U-Boot driver model, each node is bound to a dedicated driver
>> that is the most compatible to it.  This design gets along with the
>> concept of DT, and the syscon in Linux originally worked like that.
>>
>> However, Linux commit bdb0066df96e ("mfd: syscon: Decouple syscon
>> interface from platform devices") changed the behavior because it is
>> useful to let a device bind to another driver, but still works as a
>> syscon provider.
>>
>> That change had happened before U-Boot initially supported the syscon
>> driver by commit 6f98b7504f70 ("dm: Add support for generic system
>> controllers (syscon)").  So, the U-Boot's syscon works differently
>> from the beginning.  I'd say this is mis-implementation given that
>> DT is not oriented to a particular project, but Linux is the canon
>> of DT in practice.
>>
>> The problem typically arises in the combination of "syscon" and
>> "simple-mfd" compatibles.
>>
>> In Linux, they are orthogonal, i.e., the order between "syscon" and
>> "simple-mfd" does not matter at all.
>>
>> Assume the following compatible.
>>
>>    compatible = "foo,bar-syscon", "syscon", "simple-mfd";
>>
>> In U-Boot, this device node is bound to the syscon driver
>> (driver/core/syscon-uclass.c) since the "syscon" is found to be the
>> most compatible.  Then, syscon_get_regmap() succeeds.
>>
>> However,
>>
>>    compatible = "foo,bar-syscon", "simple-mfd", "syscon";
>>
>> does not work because this node is bound to the simple-bus driver
>> (drivers/core/simple-bus.c) in the favor of "simple-mfd" compatible.
>> The compatible string "syscon" is just dismissed.
>>
>> Moreover,
>>
>>    compatible = "foo,bar-syscon", "syscon";
>>
>> works like the first case because the syscon driver populates the
>> child devices.  This is wrong because populating children is the job
>> of "simple-mfd" (or "simple-bus").
>>
>> This commit ports syscon_node_to_regmap() from Linux.  This API
>> does not require the given node to be bound to a driver in any way.
>>
>> Reported-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  drivers/core/syscon-uclass.c | 63
> ++++++++++++++++++++++++++++++++++++++++++++
>>  include/syscon.h             |  8 ++++++
>>  2 files changed, 71 insertions(+)
>
> I was not aware of this change in how Linux worked, but it makes sense.
>
> Please can you add a test for this?


I do not believe a missing test would block this patch,
but I volunteered to add it (as a separate patch).

http://patchwork.ozlabs.org/patch/902733/




-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 2/4] dm: ofnode: add ofnode_device_is_compatible() helper
  2018-04-23  4:50     ` Masahiro Yamada
@ 2018-04-25  5:01       ` Simon Glass
  0 siblings, 0 replies; 16+ messages in thread
From: Simon Glass @ 2018-04-25  5:01 UTC (permalink / raw)
  To: u-boot

Hi Masahiro,

On 22 April 2018 at 22:50, Masahiro Yamada <yamada.masahiro@socionext.com>
wrote:
> 2018-04-23 5:11 GMT+09:00 Simon Glass <sjg@chromium.org>:
>> Hi Masahiro,
>>
>> On 17 April 2018 at 20:38, Masahiro Yamada <yamada.masahiro@socionext.com
>
>> wrote:
>>> device_is_compatible() takes udevice, but there is no such a helper
>>> that takes ofnode.
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>>
>>>  drivers/core/device.c |  8 +-------
>>>  drivers/core/ofnode.c | 11 +++++++++++
>>>  include/dm/ofnode.h   | 11 +++++++++++
>>>  3 files changed, 23 insertions(+), 7 deletions(-)
>>
>> Please can you add a call to this to a test?
>>
>
> No.
>
> I do not see any ofnode helper test in test/dm/.
>
> You are requesting additional work beyond this patch.
> It is unfair.
>
>
> This helper is tested indirectly by other tests.
>
> Of course, you (and anybody) are free to add per-helper grained tests,
> but this is not a good reason to block this patch.

I understand what you are saying, but you don't need to do any plumbing to
make this work. A simple call is enough, perhaps in test-fdt.c? If we get
enough tests we can refactor them into a separate ofnode test suite.

We should not add code without tests. I plan to go back and add tests for
specific calls, but don't want the work to become any harder for me.

Regards,
Simon

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 3/4] regmap: change regmap_init_mem() to take ofnode instead udevice
  2018-04-23  4:56     ` Masahiro Yamada
@ 2018-04-25  5:01       ` Simon Glass
  0 siblings, 0 replies; 16+ messages in thread
From: Simon Glass @ 2018-04-25  5:01 UTC (permalink / raw)
  To: u-boot

Hi Masahiro,

On 22 April 2018 at 22:56, Masahiro Yamada <yamada.masahiro@socionext.com>
wrote:
> Hi Simon,
>
>
> 2018-04-23 5:10 GMT+09:00 Simon Glass <sjg@chromium.org>:
>> Hi Masahiro,
>>
>> On 17 April 2018 at 20:38, Masahiro Yamada <yamada.masahiro@socionext.com
>
>> wrote:
>>> Currently, regmap_init_mem() takes udevice. This requires the node
>>> has already been associated with a device. It prevents syscon/regmap
>>> from behaving like those in Linux.
>>>
>>> Change the first argumenet to take the device node.
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>>
>>>  arch/arm/mach-aspeed/ast2500/sdram_ast2500.c |  2 +-
>>>  drivers/core/regmap.c                        | 11 +++++------
>>>  drivers/core/syscon-uclass.c                 |  2 +-
>>>  drivers/phy/meson-gxl-usb2.c                 |  2 +-
>>>  drivers/phy/meson-gxl-usb3.c                 |  2 +-
>>>  drivers/ram/rockchip/dmc-rk3368.c            |  2 +-
>>>  drivers/ram/rockchip/sdram_rk3188.c          |  2 +-
>>>  drivers/ram/rockchip/sdram_rk322x.c          |  2 +-
>>>  drivers/ram/rockchip/sdram_rk3288.c          |  2 +-
>>>  drivers/ram/rockchip/sdram_rk3399.c          |  2 +-
>>>  drivers/ram/stm32mp1/stm32mp1_ram.c          |  2 +-
>>>  drivers/reset/reset-meson.c                  |  2 +-
>>>  include/regmap.h                             |  4 ++--
>>>  13 files changed, 18 insertions(+), 19 deletions(-)
>>
>> Can you please add a simple test for regmap on sandbox?
>>
>
> No.
>
> Please stop this boiler-plate response.
>
> Simple tests for regmap already exist in test/dm/regmap.c
>
> regmap_init_mem() is not a new function, and it is tested
> through other function tests.

Can you please point to that? I don't see anything for sandbox.

>
> You block patches several times before
> by making the contributors say "Sorry, I am busy".
>
>
> I am really busy, but I need to fix the misimplementation
> of syscon for Socionext drivers.
>
> Why should I be annoyed by additional work
> to fix the problem?

Because otherwise I have no idea if the code works, no one will be able to
change it later without potentially breaking it, etc. If people get into
the habit of writing a little test at the same time, it takes very little
extra effort.

I have pour an ENORMOUS amount of time into making testing in U-Boot
better, as has Stephen Warren. We need to continue the effort. I'm sorry
that this adds a little more time to the patch submission process, but we
gain a lot of benefits. Look at all the times we have tried to fix address
translation in U-Boot, or FIT behaviour, when we don't have tests or even a
clear definition of the correct behaviour.

So please try to understand my point of view here. This is not just about
your patch, it is about the future of U-Boot for future users and
contributors.

Regards,
Simon

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [U-Boot] [PATCH 4/4] syscon: add Linux-compatible syscon API
  2018-04-23  4:58     ` Masahiro Yamada
@ 2018-04-25  5:01       ` Simon Glass
  0 siblings, 0 replies; 16+ messages in thread
From: Simon Glass @ 2018-04-25  5:01 UTC (permalink / raw)
  To: u-boot

On 22 April 2018 at 22:58, Masahiro Yamada <yamada.masahiro@socionext.com>
wrote:
> Hi Simon,
>
>
> 2018-04-23 5:11 GMT+09:00 Simon Glass <sjg@chromium.org>:
>> Hi Masahiro,
>>
>> On 17 April 2018 at 20:38, Masahiro Yamada <yamada.masahiro@socionext.com
>
>> wrote:
>>> The syscon implementation in U-Boot is different from that in Linux.
>>> Thus, DT files imported from Linux do not work for U-Boot.
>>>
>>> In U-Boot driver model, each node is bound to a dedicated driver
>>> that is the most compatible to it.  This design gets along with the
>>> concept of DT, and the syscon in Linux originally worked like that.
>>>
>>> However, Linux commit bdb0066df96e ("mfd: syscon: Decouple syscon
>>> interface from platform devices") changed the behavior because it is
>>> useful to let a device bind to another driver, but still works as a
>>> syscon provider.
>>>
>>> That change had happened before U-Boot initially supported the syscon
>>> driver by commit 6f98b7504f70 ("dm: Add support for generic system
>>> controllers (syscon)").  So, the U-Boot's syscon works differently
>>> from the beginning.  I'd say this is mis-implementation given that
>>> DT is not oriented to a particular project, but Linux is the canon
>>> of DT in practice.
>>>
>>> The problem typically arises in the combination of "syscon" and
>>> "simple-mfd" compatibles.
>>>
>>> In Linux, they are orthogonal, i.e., the order between "syscon" and
>>> "simple-mfd" does not matter at all.
>>>
>>> Assume the following compatible.
>>>
>>>    compatible = "foo,bar-syscon", "syscon", "simple-mfd";
>>>
>>> In U-Boot, this device node is bound to the syscon driver
>>> (driver/core/syscon-uclass.c) since the "syscon" is found to be the
>>> most compatible.  Then, syscon_get_regmap() succeeds.
>>>
>>> However,
>>>
>>>    compatible = "foo,bar-syscon", "simple-mfd", "syscon";
>>>
>>> does not work because this node is bound to the simple-bus driver
>>> (drivers/core/simple-bus.c) in the favor of "simple-mfd" compatible.
>>> The compatible string "syscon" is just dismissed.
>>>
>>> Moreover,
>>>
>>>    compatible = "foo,bar-syscon", "syscon";
>>>
>>> works like the first case because the syscon driver populates the
>>> child devices.  This is wrong because populating children is the job
>>> of "simple-mfd" (or "simple-bus").
>>>
>>> This commit ports syscon_node_to_regmap() from Linux.  This API
>>> does not require the given node to be bound to a driver in any way.
>>>
>>> Reported-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>>
>>>  drivers/core/syscon-uclass.c | 63
>> ++++++++++++++++++++++++++++++++++++++++++++
>>>  include/syscon.h             |  8 ++++++
>>>  2 files changed, 71 insertions(+)
>>
>> I was not aware of this change in how Linux worked, but it makes sense.
>>
>> Please can you add a test for this?
>
>
> I do not believe a missing test would block this patch,
> but I volunteered to add it (as a separate patch).
>
> http://patchwork.ozlabs.org/patch/902733/

Thanks!

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-04-25  5:01 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-18  2:38 [U-Boot] [PATCH 0/4] Add Linux-compatible syscon_to_regmap API Masahiro Yamada
2018-04-18  2:38 ` [U-Boot] [PATCH 1/4] regmap: clean up regmap allocation Masahiro Yamada
2018-04-22 20:11   ` Simon Glass
2018-04-18  2:38 ` [U-Boot] [PATCH 2/4] dm: ofnode: add ofnode_device_is_compatible() helper Masahiro Yamada
2018-04-22 20:11   ` Simon Glass
2018-04-23  4:50     ` Masahiro Yamada
2018-04-25  5:01       ` Simon Glass
2018-04-18  2:38 ` [U-Boot] [PATCH 3/4] regmap: change regmap_init_mem() to take ofnode instead udevice Masahiro Yamada
2018-04-18 15:34   ` Neil Armstrong
2018-04-22 20:10   ` Simon Glass
2018-04-23  4:56     ` Masahiro Yamada
2018-04-25  5:01       ` Simon Glass
2018-04-18  2:38 ` [U-Boot] [PATCH 4/4] syscon: add Linux-compatible syscon API Masahiro Yamada
2018-04-22 20:11   ` Simon Glass
2018-04-23  4:58     ` Masahiro Yamada
2018-04-25  5:01       ` Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.