RFC: oftree based setup of composite board devices

RFC: oftree based setup of composite board devices

[Enrico Weigelt, metux IT consult]

Hello folks,

here's an RFC for using compiled-in dtb's for initializing board devices
that can't be probed via bus'es or firmware.

Use cases are boards with non-oftree firmware (ACPI, etc) where certain
platform devices can't be directly enumerated via firmware. Traditionally
we had to write board specific drivers that check for board identification
(DMI strings, etc), then initialize the actual devices and their links
(eg. gpio<->leds/buttons, ...). Often this can be expressed just by DT.

This patch queue does a bunch of preparations in oftree code, so we can
support multiple fully independent DT's (not using DT overlays). And then
adds a generic driver parses compiled-in fdt blobs, checks for mathing
DMI strings and initializes the devices. As an example, the last patch
adds an alternative implementation for the PC engines APU2/3/4 board
family based on device tree.

The approach can be easily be extended to other kinds of composite devices,
eg. PCI cards or USB dongles.


Yet some drawbacks of the current implementation:

 * individual FDT's can't be modularized yet (IMHO, we don't have DMI-based
   modprobing anyways)
 * can't reconfigure or attach to devices outside the individual DT's
   (eg. probed by PCI, etc)


have fun,

--mtx

[RFC PATCH 01/12] of: base: improve error message in of_phandle_iterator_next()

[Enrico Weigelt, metux IT consult]

Also print out the phandle ID on error message, as a debug aid.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/of/base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 161a23631472..8a348f0d3c5e 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1297,8 +1297,8 @@ int of_phandle_iterator_next(struct of_phandle_iterator *it)
 
 		if (it->cells_name) {
 			if (!it->node) {
-				pr_err("%pOF: could not find phandle\n",
-				       it->parent);
+				pr_err("%pOF: could not find phandle %d\n",
+				       it->parent, it->phandle);
 				goto err;
 			}
 
-- 
2.11.0

[RFC PATCH 02/12] of: base: introduce of_find_node_by_phandle_from()

[Enrico Weigelt, metux IT consult]

Introducing a variant of of_find_node_by_phandle() that starts off a
different root node. This root can also point to an detached oftree,
which has no relations to primary oftree, even on platforms that natively
don't have oftree at all (eg. ACPI platforms).

Note that this has nothing to do with oftree overlays.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/of/base.c  | 14 +++++++++-----
 include/linux/of.h |  7 ++++++-
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 8a348f0d3c5e..6b3d1e817808 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1187,13 +1187,15 @@ int of_modalias_node(struct device_node *node, char *modalias, int len)
 EXPORT_SYMBOL_GPL(of_modalias_node);
 
 /**
- * of_find_node_by_phandle - Find a node given a phandle
+ * of_find_node_by_phandle_from - Find a node given a phandle
+ * @root:	root of the tree (on NULL default to of_root)
  * @handle:	phandle of the node to find
  *
  * Returns a node pointer with refcount incremented, use
  * of_node_put() on it when done.
  */
-struct device_node *of_find_node_by_phandle(phandle handle)
+struct device_node *of_find_node_by_phandle_from(struct device_node *root,
+						 phandle handle)
 {
 	struct device_node *np = NULL;
 	unsigned long flags;
@@ -1211,19 +1213,21 @@ struct device_node *of_find_node_by_phandle(phandle handle)
 		np = phandle_cache[handle_hash];
 
 	if (!np) {
-		for_each_of_allnodes(np)
+		for_each_of_allnodes_from(root, np) {
 			if (np->phandle == handle &&
 			    !of_node_check_flag(np, OF_DETACHED)) {
-				phandle_cache[handle_hash] = np;
+				if (!root)
+					phandle_cache[handle_hash] = np;
 				break;
 			}
+		}
 	}
 
 	of_node_get(np);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
 }
-EXPORT_SYMBOL(of_find_node_by_phandle);
+EXPORT_SYMBOL(of_find_node_by_phandle_from);
 
 void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
 {
diff --git a/include/linux/of.h b/include/linux/of.h
index 4b27c9a27df3..c285141653e5 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -282,7 +282,12 @@ static inline struct device_node *of_find_node_by_path(const char *path)
 	return of_find_node_opts_by_path(path, NULL);
 }
 
-extern struct device_node *of_find_node_by_phandle(phandle handle);
+extern struct device_node *of_find_node_by_phandle_from(
+	struct device_node* root, phandle handle);
+static inline struct device_node *of_find_node_by_phandle(phandle handle)
+{
+	return of_find_node_by_phandle_from(NULL, handle);
+}
 extern struct device_node *of_get_parent(const struct device_node *node);
 extern struct device_node *of_get_next_parent(struct device_node *node);
 extern struct device_node *of_get_next_child(const struct device_node *node,
-- 
2.11.0

[RFC PATCH 03/12] of: base: record root node in interator and use it for phandle lookup

[Enrico Weigelt, metux IT consult]

For detached oftree support, find the root node and record it, on iterator
creation. If we find the root of the global oftree, record NULL, in order to
have a clear distinction between detached and non-detached cases. The recorded
root node is then used for resolving phandles.

Note that in the detached case, phandle cache can't be used, so we have a
little performance penalty on repeated phandle lookups.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/of/base.c  | 13 ++++++++++++-
 include/linux/of.h |  1 +
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 6b3d1e817808..e5ef611ed233 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1249,6 +1249,7 @@ int of_phandle_iterator_init(struct of_phandle_iterator *it,
 {
 	const __be32 *list;
 	int size;
+	struct device_node *walk;
 
 	memset(it, 0, sizeof(*it));
 
@@ -1270,6 +1271,16 @@ int of_phandle_iterator_init(struct of_phandle_iterator *it,
 	it->phandle_end = list;
 	it->cur = list;
 
+	/*
+	 * find the root of our tree and record it, if we're dealing with an
+	 * detached oftree - in non-detached case, we record NULL, for clear
+	 * distinction between these two cases.
+	 */
+	for (walk=(struct device_node*)np;
+	     walk->parent;
+	     walk=(struct device_node*)walk->parent);
+	it->root = ((walk == of_root) ? NULL : walk);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_phandle_iterator_init);
@@ -1297,7 +1308,7 @@ int of_phandle_iterator_next(struct of_phandle_iterator *it)
 		 * Find the provider node and parse the #*-cells property to
 		 * determine the argument length.
 		 */
-		it->node = of_find_node_by_phandle(it->phandle);
+		it->node = of_find_node_by_phandle_from(it->root, it->phandle);
 
 		if (it->cells_name) {
 			if (!it->node) {
diff --git a/include/linux/of.h b/include/linux/of.h
index c285141653e5..dbf2c7442389 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -82,6 +82,7 @@ struct of_phandle_iterator {
 	const char *cells_name;
 	int cell_count;
 	const struct device_node *parent;
+	struct device_node *root;
 
 	/* List size information */
 	const __be32 *list_end;
-- 
2.11.0

[RFC PATCH 04/12] of: base: introduce of_match_string()

[Enrico Weigelt, metux IT consult]

Introduce a new helper function that looks up a given propery and
matches all string elements against a given string. This is useful
if we want to check wether one string element of some property
matches a given string.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/of/base.c  | 32 ++++++++++++++++++++++++++++++++
 include/linux/of.h |  2 ++
 2 files changed, 34 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index e5ef611ed233..649c2a32bb48 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -287,6 +287,38 @@ const void *of_get_property(const struct device_node *np, const char *name,
 EXPORT_SYMBOL(of_get_property);
 
 /*
+ * of_match_string - match a propery against given string
+ * @node: device_node to look up at
+ * @name: name of the property
+ * @value: value to match against
+ *
+ * Look for property by name and match all string elements against value.
+ * Returns true if the property exists and any one of the string elements
+ * matches the given value.
+ */
+bool of_match_string(const struct device_node *node, const char* name,
+		     const char* value)
+{
+	struct property *prop;
+	const char *walk;
+
+	if (!name || !value)
+		return false;
+
+	prop = of_find_property(node, name, NULL);
+	if (!prop)
+		return false;
+
+	for (walk=of_prop_next_string(prop, NULL); walk;
+	     walk=of_prop_next_string(prop, walk)) {
+		if (strcmp(walk, value)==0)
+			return true;
+	}
+	return true;
+}
+EXPORT_SYMBOL_GPL(of_match_string);
+
+/*
  * arch_match_cpu_phys_id - Match the given logical CPU and physical id
  *
  * @cpu: logical cpu index of a core/thread
diff --git a/include/linux/of.h b/include/linux/of.h
index dbf2c7442389..3612429632f4 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -355,6 +355,8 @@ extern bool of_device_is_big_endian(const struct device_node *device);
 extern const void *of_get_property(const struct device_node *node,
 				const char *name,
 				int *lenp);
+extern bool of_match_string(const struct device_node *node, const char* name,
+			    const char* value);
 extern struct device_node *of_get_cpu_node(int cpu, unsigned int *thread);
 extern struct device_node *of_get_next_cpu_node(struct device_node *prev);
 extern struct device_node *of_get_cpu_state_node(struct device_node *cpu_node,
-- 
2.11.0

Re: [RFC PATCH 04/12] of: base: introduce of_match_string()

[Rob Herring]

On Mon, Feb 8, 2021 at 4:24 PM Enrico Weigelt, metux IT consult
<info@metux.net> wrote:
>
> Introduce a new helper function that looks up a given propery and

typo

> matches all string elements against a given string. This is useful
> if we want to check wether one string element of some property

typo


> matches a given string.

Is there a user? Didn't see one in the series, but may have missed it.
I'd think we have some existing cases that could be converted.

> Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
> ---
>  drivers/of/base.c  | 32 ++++++++++++++++++++++++++++++++
>  include/linux/of.h |  2 ++
>  2 files changed, 34 insertions(+)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index e5ef611ed233..649c2a32bb48 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -287,6 +287,38 @@ const void *of_get_property(const struct device_node *np, const char *name,
>  EXPORT_SYMBOL(of_get_property);
>
>  /*
> + * of_match_string - match a propery against given string
> + * @node: device_node to look up at
> + * @name: name of the property
> + * @value: value to match against
> + *
> + * Look for property by name and match all string elements against value.
> + * Returns true if the property exists and any one of the string elements
> + * matches the given value.
> + */
> +bool of_match_string(const struct device_node *node, const char* name,
> +                    const char* value)
> +{
> +       struct property *prop;
> +       const char *walk;
> +
> +       if (!name || !value)
> +               return false;
> +
> +       prop = of_find_property(node, name, NULL);
> +       if (!prop)
> +               return false;
> +
> +       for (walk=of_prop_next_string(prop, NULL); walk;
> +            walk=of_prop_next_string(prop, walk)) {
> +               if (strcmp(walk, value)==0)
> +                       return true;
> +       }
> +       return true;
> +}
> +EXPORT_SYMBOL_GPL(of_match_string);
> +
> +/*
>   * arch_match_cpu_phys_id - Match the given logical CPU and physical id
>   *
>   * @cpu: logical cpu index of a core/thread
> diff --git a/include/linux/of.h b/include/linux/of.h
> index dbf2c7442389..3612429632f4 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -355,6 +355,8 @@ extern bool of_device_is_big_endian(const struct device_node *device);
>  extern const void *of_get_property(const struct device_node *node,
>                                 const char *name,
>                                 int *lenp);
> +extern bool of_match_string(const struct device_node *node, const char* name,
> +                           const char* value);
>  extern struct device_node *of_get_cpu_node(int cpu, unsigned int *thread);
>  extern struct device_node *of_get_next_cpu_node(struct device_node *prev);
>  extern struct device_node *of_get_cpu_state_node(struct device_node *cpu_node,
> --
> 2.11.0
>

[RFC PATCH 05/12] of: kobj: __of_attach_node_sysfs(): add optional basename parameter

[Enrico Weigelt, metux IT consult]

Introduce additional parameter for specifying the name of the base directory
underneath /sys/firmware/devicetree. This is for scenarios where we want
entirely separate oftree instances. Passing NULL falls back to the existing
base name 'base'.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/of/base.c       | 2 +-
 drivers/of/dynamic.c    | 4 ++--
 drivers/of/kobj.c       | 7 +++++--
 drivers/of/of_private.h | 6 +++---
 drivers/of/unittest.c   | 6 +++---
 5 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 649c2a32bb48..be63493bd232 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -177,7 +177,7 @@ void __init of_core_init(void)
 		return;
 	}
 	for_each_of_allnodes(np) {
-		__of_attach_node_sysfs(np);
+		__of_attach_node_sysfs(np, NULL);
 		if (np->phandle && !phandle_cache[of_phandle_cache_hash(np->phandle)])
 			phandle_cache[of_phandle_cache_hash(np->phandle)] = np;
 	}
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 9a824decf61f..63768f0dc60e 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -243,7 +243,7 @@ int of_attach_node(struct device_node *np)
 	__of_attach_node(np);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-	__of_attach_node_sysfs(np);
+	__of_attach_node_sysfs(np, NULL);
 	mutex_unlock(&of_mutex);
 
 	of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, &rd);
@@ -635,7 +635,7 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 
 	switch (ce->action) {
 	case OF_RECONFIG_ATTACH_NODE:
-		__of_attach_node_sysfs(ce->np);
+		__of_attach_node_sysfs(ce->np, NULL);
 		break;
 	case OF_RECONFIG_DETACH_NODE:
 		__of_detach_node_sysfs(ce->np);
diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
index a32e60b024b8..511d7e8b9068 100644
--- a/drivers/of/kobj.c
+++ b/drivers/of/kobj.c
@@ -112,20 +112,23 @@ void __of_update_property_sysfs(struct device_node *np, struct property *newprop
 	__of_add_property_sysfs(np, newprop);
 }
 
-int __of_attach_node_sysfs(struct device_node *np)
+int __of_attach_node_sysfs(struct device_node *np, const char *basename)
 {
 	const char *name;
 	struct kobject *parent;
 	struct property *pp;
 	int rc;
 
+	if (!basename)
+		basename = "base";
+
 	if (!of_kset)
 		return 0;
 
 	np->kobj.kset = of_kset;
 	if (!np->parent) {
 		/* Nodes without parents are new top level trees */
-		name = safe_name(&of_kset->kobj, "base");
+		name = safe_name(&of_kset->kobj, basename);
 		parent = NULL;
 	} else {
 		name = safe_name(&np->parent->kobj, kbasename(np->full_name));
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index d9e6a324de0a..371f4da77161 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -63,7 +63,7 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp);
 void __of_remove_property_sysfs(struct device_node *np, struct property *prop);
 void __of_update_property_sysfs(struct device_node *np, struct property *newprop,
 		struct property *oldprop);
-int __of_attach_node_sysfs(struct device_node *np);
+int __of_attach_node_sysfs(struct device_node *np, const char *basename);
 void __of_detach_node_sysfs(struct device_node *np);
 #else
 static inline int __of_add_property_sysfs(struct device_node *np, struct property *pp)
@@ -73,7 +73,7 @@ static inline int __of_add_property_sysfs(struct device_node *np, struct propert
 static inline void __of_remove_property_sysfs(struct device_node *np, struct property *prop) {}
 static inline void __of_update_property_sysfs(struct device_node *np,
 		struct property *newprop, struct property *oldprop) {}
-static inline int __of_attach_node_sysfs(struct device_node *np)
+static inline int __of_attach_node_sysfs(struct device_node *np, const char *basename)
 {
 	return 0;
 }
@@ -135,7 +135,7 @@ extern int __of_update_property(struct device_node *np,
 extern void __of_update_property_sysfs(struct device_node *np,
 		struct property *newprop, struct property *oldprop);
 
-extern int __of_attach_node_sysfs(struct device_node *np);
+extern int __of_attach_node_sysfs(struct device_node *np, const char *basename);
 extern void __of_detach_node(struct device_node *np);
 extern void __of_detach_node_sysfs(struct device_node *np);
 
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index eb51bc147440..caf4ade8b141 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1391,7 +1391,7 @@ static void attach_node_and_children(struct device_node *np)
 	of_node_clear_flag(np, OF_DETACHED);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-	__of_attach_node_sysfs(np);
+	__of_attach_node_sysfs(np, NULL);
 	mutex_unlock(&of_mutex);
 
 	while (child) {
@@ -1451,7 +1451,7 @@ static int __init unittest_data_add(void)
 	if (!of_root) {
 		of_root = unittest_data_node;
 		for_each_of_allnodes(np)
-			__of_attach_node_sysfs(np);
+			__of_attach_node_sysfs(np, NULL);
 		of_aliases = of_find_node_by_path("/aliases");
 		of_chosen = of_find_node_by_path("/chosen");
 		of_overlay_mutex_unlock();
@@ -3115,7 +3115,7 @@ static __init void of_unittest_overlay_high_level(void)
 		of_root->child = overlay_base_root->child;
 
 	for_each_of_allnodes_from(overlay_base_root, np)
-		__of_attach_node_sysfs(np);
+		__of_attach_node_sysfs(np, NULL);
 
 	if (of_symbols) {
 		struct property *new_prop;
-- 
2.11.0

[RFC PATCH 06/12] of: kobj: introduce of_attach_tree_sysfs()

[Enrico Weigelt, metux IT consult]

Introduce helper for attaching an (separate) oftree into sysfs.
This is useful, when drivers use their own internal device trees,
separate from the platform's global one, and wanna make it visible
to userspace via sysfs.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/of/kobj.c  | 17 +++++++++++++++++
 include/linux/of.h |  7 +++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
index 511d7e8b9068..96dc5a2753f4 100644
--- a/drivers/of/kobj.c
+++ b/drivers/of/kobj.c
@@ -166,3 +166,20 @@ void __of_detach_node_sysfs(struct device_node *np)
 
 	of_node_put(np);
 }
+
+void of_attach_tree_sysfs(struct device_node *root, const char* base)
+{
+	struct device_node *np;
+
+	if (!root)
+		return;
+
+	/* need to from our parent, so we don't traverse above our root,
+	 * if it's actually a subtree */
+	root->parent = NULL;
+
+	__of_attach_node_sysfs(root, base);
+	for_each_of_allnodes_from(root, np)
+		__of_attach_node_sysfs(np, base);
+}
+EXPORT_SYMBOL_GPL(of_attach_tree_sysfs);
diff --git a/include/linux/of.h b/include/linux/of.h
index 3612429632f4..c2fb12ce07f9 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -421,6 +421,8 @@ extern int of_update_property(struct device_node *np, struct property *newprop);
 extern int of_attach_node(struct device_node *);
 extern int of_detach_node(struct device_node *);
 
+extern void of_attach_tree_sysfs(struct device_node *root, const char* base);
+
 #define of_match_ptr(_ptr)	(_ptr)
 
 /**
@@ -1010,6 +1012,11 @@ static inline phys_addr_t of_dma_get_max_cpu_address(struct device_node *np)
 	return PHYS_ADDR_MAX;
 }
 
+static inline void of_attach_tree_sysfs(struct device_node *root,
+					const char* base)
+{
+}
+
 #define of_match_ptr(_ptr)	NULL
 #define of_match_node(_matches, _node)	NULL
 #endif /* CONFIG_OF */
-- 
2.11.0

[RFC PATCH 07/12] gpio: amd-fch: add oftree probing support

[Enrico Weigelt, metux IT consult]

Add support for probing via device tree.
---
 drivers/gpio/gpio-amd-fch.c                     | 58 +++++++++++++++++++++++++
 include/dt-bindings/gpio/amd-fch-gpio.h         | 36 +++++++++++++++
 include/linux/platform_data/gpio/gpio-amd-fch.h | 24 ++--------
 3 files changed, 98 insertions(+), 20 deletions(-)
 create mode 100644 include/dt-bindings/gpio/amd-fch-gpio.h

diff --git a/drivers/gpio/gpio-amd-fch.c b/drivers/gpio/gpio-amd-fch.c
index 2a21354ed6a0..32024f99dae5 100644
--- a/drivers/gpio/gpio-amd-fch.c
+++ b/drivers/gpio/gpio-amd-fch.c
@@ -136,12 +136,61 @@ static int amd_fch_gpio_request(struct gpio_chip *chip,
 	return 0;
 }
 
+static struct amd_fch_gpio_pdata *load_pdata(struct device *dev)
+{
+	struct amd_fch_gpio_pdata *pdata;
+	int ret;
+
+	pdata = devm_kzalloc(dev, sizeof(struct amd_fch_gpio_pdata),
+			     GFP_KERNEL);
+	if (!pdata)
+		goto nomem;
+
+	pdata->gpio_num = of_property_count_elems_of_size(dev->of_node,
+							  "gpio-regs",
+							  sizeof(u32));
+	pdata->gpio_reg = devm_kzalloc(dev, sizeof(int)*pdata->gpio_num,
+				       GFP_KERNEL);
+	if (!pdata->gpio_reg)
+		goto nomem;
+
+	pdata->gpio_names = devm_kzalloc(dev, sizeof(char*)*pdata->gpio_num,
+					 GFP_KERNEL);
+	if (!pdata->gpio_names)
+		goto nomem;
+
+	ret = of_property_read_variable_u32_array(dev->of_node, "gpio-regs",
+						  pdata->gpio_reg,
+						  pdata->gpio_num,
+						  pdata->gpio_num);
+	if (ret != pdata->gpio_num) {
+		dev_err(dev, "failed reading gpio-regs from DT: %d\n", ret);
+		return NULL;
+	}
+
+	ret = of_property_read_string_array(dev->of_node, "gpio-line-names",
+					    pdata->gpio_names, pdata->gpio_num);
+	if (ret != pdata->gpio_num) {
+		dev_err(dev, "failed reading gpio-names from DT: %d\n", ret);
+		return NULL;
+	}
+
+	return pdata;
+
+nomem:
+	dev_err(dev, "load_pdata: failed allocating memory\n");
+	return NULL;
+}
+
 static int amd_fch_gpio_probe(struct platform_device *pdev)
 {
 	struct amd_fch_gpio_priv *priv;
 	struct amd_fch_gpio_pdata *pdata;
 
 	pdata = dev_get_platdata(&pdev->dev);
+	if (!pdata)
+		pdata = load_pdata(&pdev->dev);
+
 	if (!pdata) {
 		dev_err(&pdev->dev, "no platform_data\n");
 		return -ENOENT;
@@ -165,6 +214,9 @@ static int amd_fch_gpio_probe(struct platform_device *pdev)
 	priv->gc.get_direction		= amd_fch_gpio_get_direction;
 	priv->gc.get			= amd_fch_gpio_get;
 	priv->gc.set			= amd_fch_gpio_set;
+#ifdef CONFIG_OF_GPIO
+	priv->gc.of_node		= pdev->dev.of_node;
+#endif
 
 	spin_lock_init(&priv->lock);
 
@@ -177,9 +229,15 @@ static int amd_fch_gpio_probe(struct platform_device *pdev)
 	return devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv);
 }
 
+static const struct of_device_id amd_fch_gpio_of_match[] = {
+	{ .compatible = "amd,fch-gpio" },
+	{}
+};
+
 static struct platform_driver amd_fch_gpio_driver = {
 	.driver = {
 		.name = AMD_FCH_GPIO_DRIVER_NAME,
+		.of_match_table = amd_fch_gpio_of_match,
 	},
 	.probe = amd_fch_gpio_probe,
 };
diff --git a/include/dt-bindings/gpio/amd-fch-gpio.h b/include/dt-bindings/gpio/amd-fch-gpio.h
new file mode 100644
index 000000000000..7a47e6debcdb
--- /dev/null
+++ b/include/dt-bindings/gpio/amd-fch-gpio.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+/*
+ * AMD FCH gpio platform data definitions
+ *
+ * Copyright (C) 2020 metux IT consult
+ * Author: Enrico Weigelt <info@metux.net>
+ *
+ */
+
+#ifndef __DT_BINDINGS_GPIO_AMD_FCH_REGS_H
+#define __DT_BINDINGS_GPIO_AMD_FCH_REGS_H
+
+/*
+ * gpio registers addresses
+ *
+ * these regs need to be assigned by board setup, since they're wired
+ * in very board specifici was, rarely documented, this should not be
+ * available to users.
+ */
+#define AMD_FCH_GPIO_REG_GPIO49		0x40
+#define AMD_FCH_GPIO_REG_GPIO50		0x41
+#define AMD_FCH_GPIO_REG_GPIO51		0x42
+#define AMD_FCH_GPIO_REG_GPIO55_DEVSLP0	0x43
+#define AMD_FCH_GPIO_REG_GPIO57		0x44
+#define AMD_FCH_GPIO_REG_GPIO58		0x45
+#define AMD_FCH_GPIO_REG_GPIO59_DEVSLP1	0x46
+#define AMD_FCH_GPIO_REG_GPIO64		0x47
+#define AMD_FCH_GPIO_REG_GPIO68		0x48
+#define AMD_FCH_GPIO_REG_GPIO66_SPKR	0x5B
+#define AMD_FCH_GPIO_REG_GPIO71		0x4D
+#define AMD_FCH_GPIO_REG_GPIO32_GE1	0x59
+#define AMD_FCH_GPIO_REG_GPIO33_GE2	0x5A
+#define AMT_FCH_GPIO_REG_GEVT22		0x09
+
+#endif /* __DT_BINDINGS_GPIO_AMD_FCH_REGS_H */
diff --git a/include/linux/platform_data/gpio/gpio-amd-fch.h b/include/linux/platform_data/gpio/gpio-amd-fch.h
index 255d51c9d36d..336f7387e82c 100644
--- a/include/linux/platform_data/gpio/gpio-amd-fch.h
+++ b/include/linux/platform_data/gpio/gpio-amd-fch.h
@@ -11,25 +11,9 @@
 #ifndef __LINUX_PLATFORM_DATA_GPIO_AMD_FCH_H
 #define __LINUX_PLATFORM_DATA_GPIO_AMD_FCH_H
 
-#define AMD_FCH_GPIO_DRIVER_NAME "gpio_amd_fch"
+#include <dt-bindings/gpio/amd-fch-gpio.h>
 
-/*
- * gpio register index definitions
- */
-#define AMD_FCH_GPIO_REG_GPIO49		0x40
-#define AMD_FCH_GPIO_REG_GPIO50		0x41
-#define AMD_FCH_GPIO_REG_GPIO51		0x42
-#define AMD_FCH_GPIO_REG_GPIO55_DEVSLP0	0x43
-#define AMD_FCH_GPIO_REG_GPIO57		0x44
-#define AMD_FCH_GPIO_REG_GPIO58		0x45
-#define AMD_FCH_GPIO_REG_GPIO59_DEVSLP1	0x46
-#define AMD_FCH_GPIO_REG_GPIO64		0x47
-#define AMD_FCH_GPIO_REG_GPIO68		0x48
-#define AMD_FCH_GPIO_REG_GPIO66_SPKR	0x5B
-#define AMD_FCH_GPIO_REG_GPIO71		0x4D
-#define AMD_FCH_GPIO_REG_GPIO32_GE1	0x59
-#define AMD_FCH_GPIO_REG_GPIO33_GE2	0x5A
-#define AMT_FCH_GPIO_REG_GEVT22		0x09
+#define AMD_FCH_GPIO_DRIVER_NAME "gpio_amd_fch"
 
 /*
  * struct amd_fch_gpio_pdata - GPIO chip platform data
@@ -39,8 +23,8 @@
  */
 struct amd_fch_gpio_pdata {
 	int			gpio_num;
-	int			*gpio_reg;
-	const char * const	*gpio_names;
+	u32			*gpio_reg;
+	const char * 		*gpio_names;
 };
 
 #endif /* __LINUX_PLATFORM_DATA_GPIO_AMD_FCH_H */
-- 
2.11.0

Re: [RFC PATCH 07/12] gpio: amd-fch: add oftree probing support

[Bartosz Golaszewski]

On Mon, Feb 8, 2021 at 11:22 PM Enrico Weigelt, metux IT consult
<info@metux.net> wrote:
>
> Add support for probing via device tree.
> ---
>  drivers/gpio/gpio-amd-fch.c                     | 58 +++++++++++++++++++++++++
>  include/dt-bindings/gpio/amd-fch-gpio.h         | 36 +++++++++++++++
>  include/linux/platform_data/gpio/gpio-amd-fch.h | 24 ++--------
>  3 files changed, 98 insertions(+), 20 deletions(-)
>  create mode 100644 include/dt-bindings/gpio/amd-fch-gpio.h
>
> diff --git a/drivers/gpio/gpio-amd-fch.c b/drivers/gpio/gpio-amd-fch.c
> index 2a21354ed6a0..32024f99dae5 100644
> --- a/drivers/gpio/gpio-amd-fch.c
> +++ b/drivers/gpio/gpio-amd-fch.c
> @@ -136,12 +136,61 @@ static int amd_fch_gpio_request(struct gpio_chip *chip,
>         return 0;
>  }
>
> +static struct amd_fch_gpio_pdata *load_pdata(struct device *dev)

Please stick to the amd_fch_ prefix to all symbols for consistency.

> +{
> +       struct amd_fch_gpio_pdata *pdata;
> +       int ret;
> +
> +       pdata = devm_kzalloc(dev, sizeof(struct amd_fch_gpio_pdata),
> +                            GFP_KERNEL);
> +       if (!pdata)
> +               goto nomem;
> +
> +       pdata->gpio_num = of_property_count_elems_of_size(dev->of_node,
> +                                                         "gpio-regs",
> +                                                         sizeof(u32));
> +       pdata->gpio_reg = devm_kzalloc(dev, sizeof(int)*pdata->gpio_num,
> +                                      GFP_KERNEL);
> +       if (!pdata->gpio_reg)
> +               goto nomem;
> +
> +       pdata->gpio_names = devm_kzalloc(dev, sizeof(char*)*pdata->gpio_num,
> +                                        GFP_KERNEL);
> +       if (!pdata->gpio_names)
> +               goto nomem;
> +
> +       ret = of_property_read_variable_u32_array(dev->of_node, "gpio-regs",
> +                                                 pdata->gpio_reg,
> +                                                 pdata->gpio_num,
> +                                                 pdata->gpio_num);
> +       if (ret != pdata->gpio_num) {
> +               dev_err(dev, "failed reading gpio-regs from DT: %d\n", ret);
> +               return NULL;
> +       }
> +
> +       ret = of_property_read_string_array(dev->of_node, "gpio-line-names",
> +                                           pdata->gpio_names, pdata->gpio_num);
> +       if (ret != pdata->gpio_num) {
> +               dev_err(dev, "failed reading gpio-names from DT: %d\n", ret);
> +               return NULL;
> +       }

Since you're not iterating over DT nodes nor use any interfaces
specific to OF - I suggest you use the generic device properties to
load the fill the platform data.

> +
> +       return pdata;
> +
> +nomem:
> +       dev_err(dev, "load_pdata: failed allocating memory\n");
> +       return NULL;
> +}
> +
>  static int amd_fch_gpio_probe(struct platform_device *pdev)
>  {
>         struct amd_fch_gpio_priv *priv;
>         struct amd_fch_gpio_pdata *pdata;
>
>         pdata = dev_get_platdata(&pdev->dev);
> +       if (!pdata)
> +               pdata = load_pdata(&pdev->dev);
> +
>         if (!pdata) {
>                 dev_err(&pdev->dev, "no platform_data\n");
>                 return -ENOENT;
> @@ -165,6 +214,9 @@ static int amd_fch_gpio_probe(struct platform_device *pdev)
>         priv->gc.get_direction          = amd_fch_gpio_get_direction;
>         priv->gc.get                    = amd_fch_gpio_get;
>         priv->gc.set                    = amd_fch_gpio_set;
> +#ifdef CONFIG_OF_GPIO
> +       priv->gc.of_node                = pdev->dev.of_node;
> +#endif
>
>         spin_lock_init(&priv->lock);
>
> @@ -177,9 +229,15 @@ static int amd_fch_gpio_probe(struct platform_device *pdev)
>         return devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv);
>  }
>
> +static const struct of_device_id amd_fch_gpio_of_match[] = {
> +       { .compatible = "amd,fch-gpio" },
> +       {}
> +};

Don't you need the module table?

> +
>  static struct platform_driver amd_fch_gpio_driver = {
>         .driver = {
>                 .name = AMD_FCH_GPIO_DRIVER_NAME,
> +               .of_match_table = amd_fch_gpio_of_match,
>         },
>         .probe = amd_fch_gpio_probe,
>  };
> diff --git a/include/dt-bindings/gpio/amd-fch-gpio.h b/include/dt-bindings/gpio/amd-fch-gpio.h
> new file mode 100644
> index 000000000000..7a47e6debcdb
> --- /dev/null
> +++ b/include/dt-bindings/gpio/amd-fch-gpio.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +/*
> + * AMD FCH gpio platform data definitions
> + *
> + * Copyright (C) 2020 metux IT consult
> + * Author: Enrico Weigelt <info@metux.net>
> + *
> + */
> +
> +#ifndef __DT_BINDINGS_GPIO_AMD_FCH_REGS_H
> +#define __DT_BINDINGS_GPIO_AMD_FCH_REGS_H
> +
> +/*
> + * gpio registers addresses
> + *
> + * these regs need to be assigned by board setup, since they're wired
> + * in very board specifici was, rarely documented, this should not be
> + * available to users.
> + */
> +#define AMD_FCH_GPIO_REG_GPIO49                0x40
> +#define AMD_FCH_GPIO_REG_GPIO50                0x41
> +#define AMD_FCH_GPIO_REG_GPIO51                0x42
> +#define AMD_FCH_GPIO_REG_GPIO55_DEVSLP0        0x43
> +#define AMD_FCH_GPIO_REG_GPIO57                0x44
> +#define AMD_FCH_GPIO_REG_GPIO58                0x45
> +#define AMD_FCH_GPIO_REG_GPIO59_DEVSLP1        0x46
> +#define AMD_FCH_GPIO_REG_GPIO64                0x47
> +#define AMD_FCH_GPIO_REG_GPIO68                0x48
> +#define AMD_FCH_GPIO_REG_GPIO66_SPKR   0x5B
> +#define AMD_FCH_GPIO_REG_GPIO71                0x4D
> +#define AMD_FCH_GPIO_REG_GPIO32_GE1    0x59
> +#define AMD_FCH_GPIO_REG_GPIO33_GE2    0x5A
> +#define AMT_FCH_GPIO_REG_GEVT22                0x09
> +
> +#endif /* __DT_BINDINGS_GPIO_AMD_FCH_REGS_H */
> diff --git a/include/linux/platform_data/gpio/gpio-amd-fch.h b/include/linux/platform_data/gpio/gpio-amd-fch.h
> index 255d51c9d36d..336f7387e82c 100644
> --- a/include/linux/platform_data/gpio/gpio-amd-fch.h
> +++ b/include/linux/platform_data/gpio/gpio-amd-fch.h
> @@ -11,25 +11,9 @@
>  #ifndef __LINUX_PLATFORM_DATA_GPIO_AMD_FCH_H
>  #define __LINUX_PLATFORM_DATA_GPIO_AMD_FCH_H
>
> -#define AMD_FCH_GPIO_DRIVER_NAME "gpio_amd_fch"
> +#include <dt-bindings/gpio/amd-fch-gpio.h>
>
> -/*
> - * gpio register index definitions
> - */
> -#define AMD_FCH_GPIO_REG_GPIO49                0x40
> -#define AMD_FCH_GPIO_REG_GPIO50                0x41
> -#define AMD_FCH_GPIO_REG_GPIO51                0x42
> -#define AMD_FCH_GPIO_REG_GPIO55_DEVSLP0        0x43
> -#define AMD_FCH_GPIO_REG_GPIO57                0x44
> -#define AMD_FCH_GPIO_REG_GPIO58                0x45
> -#define AMD_FCH_GPIO_REG_GPIO59_DEVSLP1        0x46
> -#define AMD_FCH_GPIO_REG_GPIO64                0x47
> -#define AMD_FCH_GPIO_REG_GPIO68                0x48
> -#define AMD_FCH_GPIO_REG_GPIO66_SPKR   0x5B
> -#define AMD_FCH_GPIO_REG_GPIO71                0x4D
> -#define AMD_FCH_GPIO_REG_GPIO32_GE1    0x59
> -#define AMD_FCH_GPIO_REG_GPIO33_GE2    0x5A
> -#define AMT_FCH_GPIO_REG_GEVT22                0x09
> +#define AMD_FCH_GPIO_DRIVER_NAME "gpio_amd_fch"
>
>  /*
>   * struct amd_fch_gpio_pdata - GPIO chip platform data
> @@ -39,8 +23,8 @@
>   */
>  struct amd_fch_gpio_pdata {
>         int                     gpio_num;
> -       int                     *gpio_reg;
> -       const char * const      *gpio_names;
> +       u32                     *gpio_reg;
> +       const char *            *gpio_names;
>  };
>
>  #endif /* __LINUX_PLATFORM_DATA_GPIO_AMD_FCH_H */
> --
> 2.11.0
>

Bartosz

Re: [RFC PATCH 07/12] gpio: amd-fch: add oftree probing support

[Linus Walleij]

On Mon, Feb 8, 2021 at 11:24 PM Enrico Weigelt, metux IT consult
<info@metux.net> wrote:

> Add support for probing via device tree.
(...)
> +       pdata->gpio_num = of_property_count_elems_of_size(dev->of_node,
> +                                                         "gpio-regs",
> +                                                         sizeof(u32));
> +       pdata->gpio_reg = devm_kzalloc(dev, sizeof(int)*pdata->gpio_num,
> +                                      GFP_KERNEL);
> +       if (!pdata->gpio_reg)
> +               goto nomem;

I don't know what the idea is with this but register are not normally defined
in the DTS files. The registers are determined from the compatible value.

> +       pdata->gpio_names = devm_kzalloc(dev, sizeof(char*)*pdata->gpio_num,
> +                                        GFP_KERNEL);
> +       if (!pdata->gpio_names)
> +               goto nomem;
(...)
> +       ret = of_property_read_string_array(dev->of_node, "gpio-line-names",
> +                                           pdata->gpio_names, pdata->gpio_num);

And this is already handled by the core.

Yours,
Linus Walleij

Re: [RFC PATCH 07/12] gpio: amd-fch: add oftree probing support

[Enrico Weigelt, metux IT consult]

On 01.03.21 15:51, Linus Walleij wrote:

Hi,

> I don't know what the idea is with this but register are not normally defined
> in the DTS files. The registers are determined from the compatible value.

The idea is basically replacing the pdata struct by oftree node.
(subsequent patches in this queue use this by doing the board setup via
compiled-in dtb, instead of the currently hardcoded tables).

On these SoCs, the gpio setup is a little bit more complex than just
having a fixed range of registers (one per pin): the actual meaning
depends und Soc model and board type - some regs aren't even gpios.
(I'm still in progress of RE'ing the bios blob, to find out more,
eg. pinmux setups, etc). Writing to the wrong regs can cause weird
effects (actually not even sure whether it could lead to damage)

In essence: only a specific subset of the register range can be used
for GPIOs - the others shouldn't ever be touched. And this specific
subset is soc/board specific.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [RFC PATCH 07/12] gpio: amd-fch: add oftree probing support

[Andy Shevchenko]

On Thu, Mar 11, 2021 at 12:20 PM Enrico Weigelt, metux IT consult
<info@metux.net> wrote:
> On 01.03.21 15:51, Linus Walleij wrote:

> > I don't know what the idea is with this but register are not normally defined
> > in the DTS files. The registers are determined from the compatible value.
>
> The idea is basically replacing the pdata struct by oftree node.
> (subsequent patches in this queue use this by doing the board setup via
> compiled-in dtb, instead of the currently hardcoded tables).

You are a bit late. We have built-in device properties (and
corresponding API, which recently becomes swnode) which aims exactly
this.


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 07/12] gpio: amd-fch: add oftree probing support

[Enrico Weigelt, metux IT consult]

On 11.03.21 11:42, Andy Shevchenko wrote:

Hi,

> You are a bit late. We have built-in device properties (and
> corresponding API, which recently becomes swnode) which aims exactly
> this.

Is there some compact notation for swnode's that's as small and simple
as some piece of DTS ?

My reasons for choosing built-in dtb have been:

* it's a very small and compact notation for describing devices
* no more open-coded registrations, etc
* no more need for board drivers (except for the little piece of DT)


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [RFC PATCH 07/12] gpio: amd-fch: add oftree probing support

[Linus Walleij]

On Thu, Mar 18, 2021 at 9:00 AM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:

> Is there some compact notation for swnode's that's as small and simple
> as some piece of DTS ?

Yes it's really neat. It's all in <linux/property.h> and examples
in e.g. the testsuite:
drivers/base/test/property-entry-test.c

You can just grep for PROPERTY_ENTRY and you find some
examples of how we use it.

Yours,
Linus Walleij

[RFC PATCH 08/12] drivers: base: introduce bus_remove_device_by_name()

[Enrico Weigelt, metux IT consult]

Introduce a helper for detaching a named device from bus and
unregistering it. This is helpful eg. if some board specific driver
needs to remove an unwanted device that had been probed via firmware,
but should be handled differently.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/base/bus.c         | 16 ++++++++++++++++
 include/linux/device/bus.h |  9 +++++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index a9c23ecebc7c..450d3ed6cf1f 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -178,6 +178,22 @@ static const struct kset_uevent_ops bus_uevent_ops = {
 
 static struct kset *bus_kset;
 
+int bus_unregister_device_by_name(struct bus_type *bus, const char *name)
+{
+	struct device *dev;
+
+	dev = bus_find_device_by_name(bus, NULL, name);
+	if (!dev)
+		return -ENOENT;
+
+	device_driver_detach(dev);
+	device_unregister(dev);
+	put_device(dev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(bus_unregister_device_by_name);
+
 /* Manually detach a device from its associated driver. */
 static ssize_t unbind_store(struct device_driver *drv, const char *buf,
 			    size_t count)
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index 1ea5e1d1545b..36a1dae26c95 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -253,6 +253,15 @@ int bus_for_each_drv(struct bus_type *bus, struct device_driver *start,
 void bus_sort_breadthfirst(struct bus_type *bus,
 			   int (*compare)(const struct device *a,
 					  const struct device *b));
+
+/**
+ * bus_unregister_device_by_name - remove device by bus id from specific bus
+ *                                 and unregister it from device core
+ * @bus: bus type
+ * @name: name of the device to remove
+ */
+int bus_unregister_device_by_name(struct bus_type *bus, const char *name);
+
 /*
  * Bus notifiers: Get notified of addition/removal of devices
  * and binding/unbinding of drivers to devices.
-- 
2.11.0

[RFC PATCH 09/12] drivers: base: reintroduce find_bus()

[Enrico Weigelt, metux IT consult]

---
 drivers/base/bus.c         | 14 ++++++++++----
 include/linux/device/bus.h |  2 ++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 450d3ed6cf1f..a06ae2786092 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -754,13 +754,19 @@ EXPORT_SYMBOL_GPL(device_reprobe);
  *
  * Note that kset_find_obj increments bus' reference count.
  */
-#if 0
-struct bus_type *find_bus(char *name)
+struct bus_type *find_bus(const char *name)
 {
 	struct kobject *k = kset_find_obj(bus_kset, name);
-	return k ? to_bus(k) : NULL;
+	struct subsys_private *subsys_priv;
+
+	if (!k)
+		return NULL;
+
+	subsys_priv = container_of(to_kset(k), struct subsys_private, subsys);
+
+	return subsys_priv->bus;
 }
-#endif  /*  0  */
+EXPORT_SYMBOL_GPL(find_bus);
 
 static int bus_add_groups(struct bus_type *bus,
 			  const struct attribute_group **groups)
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index 36a1dae26c95..b4cbcfe176c5 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -254,6 +254,8 @@ void bus_sort_breadthfirst(struct bus_type *bus,
 			   int (*compare)(const struct device *a,
 					  const struct device *b));
 
+struct bus_type *find_bus(const char *name);
+
 /**
  * bus_unregister_device_by_name - remove device by bus id from specific bus
  *                                 and unregister it from device core
-- 
2.11.0

Re: [RFC PATCH 09/12] drivers: base: reintroduce find_bus()

[Greg KH]

On Mon, Feb 08, 2021 at 11:22:00PM +0100, Enrico Weigelt, metux IT consult wrote:
> ---
>  drivers/base/bus.c         | 14 ++++++++++----
>  include/linux/device/bus.h |  2 ++
>  2 files changed, 12 insertions(+), 4 deletions(-)

Um, no.

Re: [RFC PATCH 09/12] drivers: base: reintroduce find_bus()

[Enrico Weigelt, metux IT consult]

On 13.02.21 11:20, Greg KH wrote:
> On Mon, Feb 08, 2021 at 11:22:00PM +0100, Enrico Weigelt, metux IT consult wrote:
>> ---
>>   drivers/base/bus.c         | 14 ++++++++++----
>>   include/linux/device/bus.h |  2 ++
>>   2 files changed, 12 insertions(+), 4 deletions(-)
> 
> Um, no.

Why not ? Do you have a better idea ?

What I actually need is a way to unbdind a specific device, identified
by bus name and device name. The problem to be solved here is dropping
devices that have been enumerated in a bad way by firmware (ACPI in this
case), and then recreating it in a clean, consistent way.

If there was a variant of bus_find_device_by_name() which takes the name
instead of ptr to the bus, that would also be okay for me.


--mtx

-- 
-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [RFC PATCH 09/12] drivers: base: reintroduce find_bus()

[Greg KH]

On Tue, Feb 23, 2021 at 09:13:26PM +0100, Enrico Weigelt, metux IT consult wrote:
> On 13.02.21 11:20, Greg KH wrote:
> > On Mon, Feb 08, 2021 at 11:22:00PM +0100, Enrico Weigelt, metux IT consult wrote:
> > > ---
> > >   drivers/base/bus.c         | 14 ++++++++++----
> > >   include/linux/device/bus.h |  2 ++
> > >   2 files changed, 12 insertions(+), 4 deletions(-)
> > 
> > Um, no.
> 
> Why not ? Do you have a better idea ?
> 
> What I actually need is a way to unbdind a specific device, identified
> by bus name and device name. The problem to be solved here is dropping
> devices that have been enumerated in a bad way by firmware (ACPI in this
> case), and then recreating it in a clean, consistent way.

Have the firmware code do it itself, do nto try to "reach across" like
this.

And what problem are you really trying to solve here by doing this?

thanks,

greg k-h

Re: [RFC PATCH 09/12] drivers: base: reintroduce find_bus()

[Enrico Weigelt, metux IT consult]

On 24.02.21 09:00, Greg KH wrote:

> Have the firmware code do it itself, do nto try to "reach across" like
> this.

By "firmware code" you mean Linux acpi core or the board's bios ?

a) Fixing BIOS would be the cleanest solution, but we cant expect all
    users to do field upgrades. Many of the devices (eg. the customer,
    I've originally wrote the apu board driver for, deployed them in
    really remote locations, sometimes even just reachable by ship,
    heli or horse, litterally)

b) Explicit blacklisting somewhere in apci enumeration code could work,
    but I really hate the idea of such board and bios version specific
    quirks in a place, completely unrelated to the actual board driver.

Actually, I'm also hoping to find a proper way for having those things
in one file per board, in the future. (probably not applicable for
early stuff, or _OSI(Linux), etc)

> And what problem are you really trying to solve here by doing this?

The problem is that *some* bios versions (that came much later, after
pcengines-apuv2 driver went into production) added a few things that
the driver is already doing - different versions doing it differently
(eg. even enumerating gpio connected leds with completely different
names, etc), and still some gpio connected devices missing. Some
versions (just forgot, which one it's been exactly) even enumerate
*some* gpios (and LEDs behind them) as a different device, whose Linux
driver just happens to work. Meanwhile I can't find any reference of
that in the coreboot source, anymore.

As you can see: bios is anything but reliable on that platform.

What I'm trying to achieve: the kernel should behave exactly the
same, no matter what board revision, bios version, kernel version,
etc. (there should be especially no need to have special per-board
quirks in userland, depending on board rev, bios version, kernel
version).

If you've got a better solution, I'll be glad to hear it.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [RFC PATCH 09/12] drivers: base: reintroduce find_bus()

[Greg KH]

On Wed, Feb 24, 2021 at 04:30:42PM +0100, Enrico Weigelt, metux IT consult wrote:
> On 24.02.21 09:00, Greg KH wrote:
> 
> > Have the firmware code do it itself, do nto try to "reach across" like
> > this.
> 
> By "firmware code" you mean Linux acpi core or the board's bios ?

either.

> a) Fixing BIOS would be the cleanest solution, but we cant expect all
>    users to do field upgrades. Many of the devices (eg. the customer,
>    I've originally wrote the apu board driver for, deployed them in
>    really remote locations, sometimes even just reachable by ship,
>    heli or horse, litterally)
> 
> b) Explicit blacklisting somewhere in apci enumeration code could work,
>    but I really hate the idea of such board and bios version specific
>    quirks in a place, completely unrelated to the actual board driver.

We have quirks all over the place, that's normal and how we handle
broken hardware/bios al the time.

> Actually, I'm also hoping to find a proper way for having those things
> in one file per board, in the future. (probably not applicable for
> early stuff, or _OSI(Linux), etc)

I don't know what "things" you are referring to here at all.

> > And what problem are you really trying to solve here by doing this?
> 
> The problem is that *some* bios versions (that came much later, after
> pcengines-apuv2 driver went into production) added a few things that
> the driver is already doing - different versions doing it differently
> (eg. even enumerating gpio connected leds with completely different
> names, etc), and still some gpio connected devices missing. Some
> versions (just forgot, which one it's been exactly) even enumerate
> *some* gpios (and LEDs behind them) as a different device, whose Linux
> driver just happens to work. Meanwhile I can't find any reference of
> that in the coreboot source, anymore.

I have no idea what you are talking about here, you did not describe a
problem :(

> As you can see: bios is anything but reliable on that platform.

I do not understand.

> What I'm trying to achieve: the kernel should behave exactly the
> same, no matter what board revision, bios version, kernel version,
> etc. (there should be especially no need to have special per-board
> quirks in userland, depending on board rev, bios version, kernel
> version).
> 
> If you've got a better solution, I'll be glad to hear it.

I really do not understand the problem, sorry.

greg k-h

[RFC PATCH 10/12] export bus_get() / bus_put()

[Enrico Weigelt, metux IT consult]

---
 drivers/base/bus.c         | 6 ++++--
 include/linux/device/bus.h | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index a06ae2786092..2ef92a3c5d7b 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -39,7 +39,7 @@ static struct kset *system_kset;
 static int __must_check bus_rescan_devices_helper(struct device *dev,
 						void *data);
 
-static struct bus_type *bus_get(struct bus_type *bus)
+struct bus_type *bus_get(struct bus_type *bus)
 {
 	if (bus) {
 		kset_get(&bus->p->subsys);
@@ -47,12 +47,14 @@ static struct bus_type *bus_get(struct bus_type *bus)
 	}
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(bus_get);
 
-static void bus_put(struct bus_type *bus)
+void bus_put(struct bus_type *bus)
 {
 	if (bus)
 		kset_put(&bus->p->subsys);
 }
+EXPORT_SYMBOL_GPL(bus_put);
 
 static ssize_t drv_attr_show(struct kobject *kobj, struct attribute *attr,
 			     char *buf)
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index b4cbcfe176c5..8d6b45df0a82 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -120,6 +120,9 @@ extern void bus_unregister(struct bus_type *bus);
 
 extern int __must_check bus_rescan_devices(struct bus_type *bus);
 
+struct bus_type *bus_get(struct bus_type *bus);
+void bus_put(struct bus_type *bus);
+
 struct bus_attribute {
 	struct attribute	attr;
 	ssize_t (*show)(struct bus_type *bus, char *buf);
-- 
2.11.0

[RFC PATCH 11/12] platform/x86: skeleton for oftree based board device initialization

[Enrico Weigelt, metux IT consult]

Lots of boards have extra devices that can't be fully probed via bus'es
(like PCI) or generic firmware mechanisms like ACPI. Often those capabilities
are just partial or even highly depend on firmware version.

Instead of hand-writing board specific drivers merely for the correct
initialization / parameterization of generic drivers, hereby introducing
a generic mechanism, using the already well supported oftree.

These oftrees are compiled into the driver, which first tries to match
machine identifications (eg. DMI strings) against rules defined in the
individual oftrees, and on success, probes the devices that are defined
by them.

For the time being, we just support matching on DMI_BOARD_NAME and
DMI_SYS_VENDOR - other criteria, even bus- or ACPI-id's can be added later,
when needed.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/platform/Kconfig      |   2 +
 drivers/platform/Makefile     |   1 +
 drivers/platform/of/Kconfig   |  41 ++++++++++++++
 drivers/platform/of/Makefile  |   5 ++
 drivers/platform/of/drv.c     | 123 +++++++++++++++++++++++++++++++++++++++++
 drivers/platform/of/init.c    | 126 ++++++++++++++++++++++++++++++++++++++++++
 drivers/platform/of/ofboard.h |   8 +++
 7 files changed, 306 insertions(+)
 create mode 100644 drivers/platform/of/Kconfig
 create mode 100644 drivers/platform/of/Makefile
 create mode 100644 drivers/platform/of/drv.c
 create mode 100644 drivers/platform/of/init.c
 create mode 100644 drivers/platform/of/ofboard.h

diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
index 18fc6a08569e..9ac6d4e2a762 100644
--- a/drivers/platform/Kconfig
+++ b/drivers/platform/Kconfig
@@ -15,3 +15,5 @@ source "drivers/platform/mellanox/Kconfig"
 source "drivers/platform/olpc/Kconfig"
 
 source "drivers/platform/surface/Kconfig"
+
+source "drivers/platform/of/Kconfig"
diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile
index 4de08ef4ec9d..ca4d74701fd7 100644
--- a/drivers/platform/Makefile
+++ b/drivers/platform/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_OLPC_EC)		+= olpc/
 obj-$(CONFIG_GOLDFISH)		+= goldfish/
 obj-$(CONFIG_CHROME_PLATFORMS)	+= chrome/
 obj-$(CONFIG_SURFACE_PLATFORMS)	+= surface/
+obj-$(CONFIG_PLATFORM_OF_DRV)	+= of/
diff --git a/drivers/platform/of/Kconfig b/drivers/platform/of/Kconfig
new file mode 100644
index 000000000000..a0b5a641a7c6
--- /dev/null
+++ b/drivers/platform/of/Kconfig
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# X86 Platform Specific Drivers
+#
+
+menuconfig PLATFORM_OF_DRV
+	tristate "Platform support via device tree"
+	select OF
+	select OF_FLATTREE
+	help
+	  Say Y here to get to see options for board support that's initialized
+	  via compiled-in flattened device trees.
+
+	  This is entirely independent from traditional DT-based booting (or DT
+	  overlays) and meant for additional devices on non-OF-based (eg. APCI)
+	  boards or composite devices behind probing-capable busses (eg. PCI).
+
+	  Instead of writing individual drivers for just the initialization of
+	  subdevices, this option provides a generic mechanism for describing
+	  these devices via device tree.
+
+	  If you say N, all options in this submenu will be skipped and disabled.
+
+if PLATFORM_OF_DRV
+
+config PLATFORM_OF_DRV_SYSFS_DTB
+	bool "Expose device tree binaries in sysfs"
+	default y
+	depends on SYSFS
+	help
+	  Say Y here to enable exposing device tree binaries at /sys/firmware.
+
+config PLATFORM_OF_DRV_SYSFS_DT
+	bool "Expose parsed device tree in sysfs"
+	default y
+	depends on SYSFS
+	help
+	  Say Y here to enable exposing device tree nodes at
+	  /sys/firmware/devicetree.
+
+endif # PLATFORM_OF_DRV
diff --git a/drivers/platform/of/Makefile b/drivers/platform/of/Makefile
new file mode 100644
index 000000000000..84cf3003c500
--- /dev/null
+++ b/drivers/platform/of/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+ofboard-y := init.o drv.o
+
+obj-$(CONFIG_PLATFORM_OF_DRV) += ofboard.o
diff --git a/drivers/platform/of/drv.c b/drivers/platform/of/drv.c
new file mode 100644
index 000000000000..ff7006c24cf7
--- /dev/null
+++ b/drivers/platform/of/drv.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/*
+ * Copyright (C) 2021 metux IT consult
+ * Author: Enrico Weigelt <info@metux.net>
+ */
+
+#include <linux/dmi.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+
+#include "ofboard.h"
+
+static bool __init ofboard_match_dmi(struct device *dev)
+{
+#ifdef CONFIG_DMI
+	const char *board = dmi_get_system_info(DMI_BOARD_NAME);
+	const char *vendor = dmi_get_system_info(DMI_SYS_VENDOR);
+	const struct device_node *node = dev->of_node;
+
+	if (!of_match_string(node, "dmi-sys-vendor", vendor))
+		return false;
+
+	if (!of_match_string(node, "dmi-board-name", board))
+		return false;
+
+	dev_info(dev, "matched dmi: vendor=\"%s\" board=\"%s\"\n", vendor,
+		 board);
+
+	return true;
+#else
+	return false;
+#endif
+}
+
+static void __init ofboard_kick_devs(struct device *dev,
+				     struct device_node *np,
+				     const char *bus_name)
+{
+	struct property *prop;
+	const char *walk;
+	struct bus_type *bus;
+	int ret;
+
+	if (strcmp(bus_name, "name")==0)
+		return;
+
+	bus = find_bus(bus_name);
+	if (!bus) {
+		dev_warn(dev, "cant find bus \"%s\"\n", bus_name);
+		return;
+	}
+
+	of_property_for_each_string(np, bus_name, prop, walk) {
+		ret = bus_unregister_device_by_name(bus, walk);
+		if (ret)
+			dev_warn(dev, "failed removing device \"%s\" on bus "
+				 "\"%s\": %d\n", walk, bus_name, ret);
+		else
+			dev_info(dev, "removed device \"%s\" from bus "
+				 "\"%s\"\n", walk, bus_name);
+	}
+
+	bus_put(bus);
+}
+
+static void __init ofboard_unbind(struct device *dev)
+{
+	struct property *pr;
+	struct device_node *np = of_get_child_by_name(dev->of_node, "unbind");
+
+	if (!IS_ERR_OR_NULL(np))
+		for_each_property_of_node(np, pr)
+			ofboard_kick_devs(dev, np, pr->name);
+}
+
+static int ofboard_populate(struct device *dev)
+{
+	int ret;
+	struct device_node *of_node = dev->of_node;
+	struct device_node *np = of_get_child_by_name(of_node, "devices");
+
+	if (IS_ERR_OR_NULL(np)) {
+		dev_info(dev, "board oftree has no devices\n");
+		return -ENOENT;
+	}
+
+	ret = of_platform_populate(np, NULL, NULL, dev);
+	if (ret) {
+		dev_err(dev, "failed probing of childs: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ofboard_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	if (!ofboard_match_dmi(dev))
+		return -EINVAL;
+
+	ofboard_unbind(&pdev->dev);
+
+	return ofboard_populate(dev);
+}
+
+static const struct of_device_id ofboard_of_match[] = {
+	{ .compatible = "virtual,dmi-board" },
+	{}
+};
+
+struct platform_driver ofboard_driver = {
+	.driver = {
+		.name = "ofboard",
+		.of_match_table = ofboard_of_match,
+	},
+	.probe = ofboard_probe,
+};
diff --git a/drivers/platform/of/init.c b/drivers/platform/of/init.c
new file mode 100644
index 000000000000..3b8373cda77a
--- /dev/null
+++ b/drivers/platform/of/init.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/*
+ * Copyright (C) 2021 metux IT consult
+ * Author: Enrico Weigelt <info@metux.net>
+ */
+
+#define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
+
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_fdt.h>
+#include <linux/libfdt.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+#include "ofboard.h"
+
+#define DECLARE_FDT_EXTERN(n) \
+	extern char __dtb_##n##_begin[]; \
+	static struct bin_attribute __dtb_##n##_attr = { \
+		.attr = { .name = "fdt-" #n, .mode = S_IRUSR }, \
+		.private = __dtb_##n##_begin, \
+		.read = fdt_image_raw_read, \
+	};
+
+struct fdt_image {
+	char *begin;
+	size_t size;
+	char *basename;
+	struct bin_attribute *bin_attr;
+	struct device_node *root;
+};
+
+#define FDT_IMAGE_ENT(n)			\
+	{					\
+		.begin = __dtb_##n##_begin,	\
+		.bin_attr = &__dtb_##n##_attr,	\
+		.basename = "ofboard-" #n	\
+	}
+
+static ssize_t fdt_image_raw_read(struct file *filep, struct kobject *kobj,
+				  struct bin_attribute *bin_attr, char *buf,
+				  loff_t off, size_t count)
+{
+	memcpy(buf, bin_attr->private + off, count);
+	return count;
+}
+
+static struct fdt_image fdt[] = {
+};
+
+static int __init ofdrv_init_sysfs(struct fdt_image *image)
+{
+	image->bin_attr->size = image->size;
+	image->bin_attr->private = image->begin;
+
+	if (sysfs_create_bin_file(firmware_kobj, image->bin_attr))
+		pr_warn("failed creating sysfs bin_file\n");
+
+	of_attach_tree_sysfs(image->root, image->basename);
+
+	return 0;
+}
+
+static int __init ofdrv_parse_image(struct fdt_image *image)
+{
+	struct device_node* root;
+	void *new_fdt;
+
+	image->size = fdt_totalsize(image->begin);
+	new_fdt = kmemdup(image->begin, image->size, GFP_KERNEL);
+	if (!new_fdt)
+		return -ENOMEM;
+
+	image->begin = new_fdt;
+	of_fdt_unflatten_tree(new_fdt, NULL, &root);
+
+	if (IS_ERR_OR_NULL(root))
+		return PTR_ERR(root);
+
+	image->root = root;
+
+	return 0;
+}
+
+static int __init ofdrv_init_image(struct fdt_image *image)
+{
+	struct device_node *np;
+	int ret;
+
+	ret = ofdrv_parse_image(image);
+	if (ret)
+		return ret;
+
+	ofdrv_init_sysfs(image);
+
+	for_each_child_of_node(image->root, np) {
+		struct platform_device_info pdevinfo = {
+			.name = np->name,
+			.fwnode = &np->fwnode,
+			.id = PLATFORM_DEVID_NONE,
+		};
+		platform_device_register_full(&pdevinfo);
+	}
+
+	return 0;
+}
+
+static int __init ofdrv_init(void)
+{
+	int x;
+
+	platform_driver_register(&ofboard_driver);
+
+	for (x=0; x<ARRAY_SIZE(fdt); x++)
+		ofdrv_init_image(&fdt[x]);
+
+	return 0;
+}
+
+module_init(ofdrv_init);
+
+MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@metux.net>");
+MODULE_DESCRIPTION("Generic oftree based initialization of custom board devices");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/of/ofboard.h b/drivers/platform/of/ofboard.h
new file mode 100644
index 000000000000..7516e5df4f18
--- /dev/null
+++ b/drivers/platform/of/ofboard.h
@@ -0,0 +1,8 @@
+#ifndef __DRIVERS_PLATFORM_OFBOARD_H
+#define __DRIVERS_PLATFORM_OFBOARD_H
+
+#include <linux/platform_device.h>
+
+extern struct platform_driver ofboard_driver;
+
+#endif
-- 
2.11.0

Re: [RFC PATCH 11/12] platform/x86: skeleton for oftree based board device initialization

[Andy Shevchenko]

On Tue, Feb 9, 2021 at 12:27 AM Enrico Weigelt, metux IT consult
<info@metux.net> wrote:
>
> Lots of boards have extra devices that can't be fully probed via bus'es
> (like PCI) or generic firmware mechanisms like ACPI. Often those capabilities
> are just partial or even highly depend on firmware version.
>
> Instead of hand-writing board specific drivers merely for the correct
> initialization / parameterization of generic drivers, hereby introducing
> a generic mechanism, using the already well supported oftree.
>
> These oftrees are compiled into the driver, which first tries to match
> machine identifications (eg. DMI strings) against rules defined in the
> individual oftrees, and on success, probes the devices that are defined
> by them.
>
> For the time being, we just support matching on DMI_BOARD_NAME and
> DMI_SYS_VENDOR - other criteria, even bus- or ACPI-id's can be added later,
> when needed.

This is weird and seems it missed the fact of the DMI based modprobe
mechanism that the kernel has for ages.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 11/12] platform/x86: skeleton for oftree based board device initialization

[Linus Walleij]

On Mon, Feb 8, 2021 at 11:22 PM Enrico Weigelt, metux IT consult
<info@metux.net> wrote:

> Lots of boards have extra devices that can't be fully probed via bus'es
> (like PCI) or generic firmware mechanisms like ACPI. Often those capabilities
> are just partial or even highly depend on firmware version.

I think Intel people often take the stance that the ACPI DSDT (or whatever)
needs to be fixed.

If the usecase is to explicitly work around deployed firmware that cannot
and will not be upgraded/fixed by describing the hardware using DT
instead, based on just the DMI ID then we should spell that out
explicitly.

It feels a bit like fixing a problem using a different hardware description
just because we can. Look in drivers/gpio/gpiolib-acpi.c
table gpiolib_acpi_quirks[]. It's just an example how this is fixed using
fine granular ACPI-specific mechanisms at several places in the kernel
instead of just tossing out the whole description and redoing it in
device tree.

I haven't worked with this in practice so I suppose it is a bit up to
the people who end up having to fix this kind of stuff, Hans de Goede
and Andy are fixing this kind of stuff all the time so their buy-in is
important, we need to see that this is a real, useful tool for people
like them, not just nice to have.

Yours,
Linus Walleij

Re: [RFC PATCH 11/12] platform/x86: skeleton for oftree based board device initialization

[Enrico Weigelt, metux IT consult]

On 12.02.21 10:58, Linus Walleij wrote:

Hi,

> I think Intel people often take the stance that the ACPI DSDT (or whatever)
> needs to be fixed.

It should, actually board/firmware vendors should think more carefully
and do it right in the first place. But reality is different. And
firmware upgrade often is anything but easy (as soon as we leave the
field of average Joh Doe's home PC)

> If the usecase is to explicitly work around deployed firmware that cannot
> and will not be upgraded/fixed by describing the hardware using DT
> instead, based on just the DMI ID then we should spell that out
> explicitly.

Okay, maybe I should have stated this more clearly.

OTOH, the scope is also a little bit greater: certain external cards
that don't need much special handling for the card itself, just
enumerate devices (and connections between them) using existing drivers.

That's a pretty common scenario in industrial backplane systems, where
we have lots of different (even application specific) cards, usually
composed of standard chips, that can be identified by some ID, but
cannot describe themselves. We have to write lots of specific drivers
for them, usually just for instantiating existing drivers. (we rarely
see such code going towards mainline).

A similar case (mainlined) seems to be the RCAR display unit - they're
using dt overlays that are built into the driver and applied by it
based on the detected DU at runtime. RCAR seems to be a pure DT
platform, so that's an obvious move. APU2/3/4 is ACPI based, so I went
in a different direction - but I'm now investigating how to make DT
overlays work on an ACPI platform (eg. needs some initial nodes, ...)
In case that's successful, I'll rework my RFC to use overlays, and
it will become much smaller (my oftree core changes then won't be
necessary anymore).

> It feels a bit like fixing a problem using a different hardware description
> just because we can. Look in drivers/gpio/gpiolib-acpi.c
> table gpiolib_acpi_quirks[]. It's just an example how this is fixed using
> fine granular ACPI-specific mechanisms at several places in the kernel
> instead of just tossing out the whole description and redoing it in
> device tree.

I'm quite reluctant to put everything in there. Theoretically, for apu
case, I could prevent enumerating the incomplete gpios there, but the
actual driver setup still remains (certainly don't wanna put that into
such a global place). But the original problem of having to write so
much code for just instantiating generic drivers remains. And
distributing knowledge of certain devices over several places doesn't
feel like a good idea to me.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [RFC PATCH 11/12] platform/x86: skeleton for oftree based board device initialization

[Frank Rowand]

On 2/12/21 5:54 AM, Enrico Weigelt, metux IT consult wrote:
> On 12.02.21 10:58, Linus Walleij wrote:
> 
> Hi,
> 
>> I think Intel people often take the stance that the ACPI DSDT (or whatever)
>> needs to be fixed.
> 
> It should, actually board/firmware vendors should think more carefully
> and do it right in the first place. But reality is different. And
> firmware upgrade often is anything but easy (as soon as we leave the
> field of average Joh Doe's home PC)
> 
>> If the usecase is to explicitly work around deployed firmware that cannot
>> and will not be upgraded/fixed by describing the hardware using DT
>> instead, based on just the DMI ID then we should spell that out
>> explicitly.
> 
> Okay, maybe I should have stated this more clearly.
> 
> OTOH, the scope is also a little bit greater: certain external cards
> that don't need much special handling for the card itself, just
> enumerate devices (and connections between them) using existing drivers.
> 
> That's a pretty common scenario in industrial backplane systems, where
> we have lots of different (even application specific) cards, usually
> composed of standard chips, that can be identified by some ID, but
> cannot describe themselves. We have to write lots of specific drivers
> for them, usually just for instantiating existing drivers. (we rarely
> see such code going towards mainline).
> 
> A similar case (mainlined) seems to be the RCAR display unit - they're
> using dt overlays that are built into the driver and applied by it
> based on the detected DU at runtime. RCAR seems to be a pure DT

The RCAR use of overlays that are built into the driver are a known
pattern that is explicitly not to be repeated.  The driver has been
granted a grandfathered in status, thus an exception as long as
needed.

-Frank

> platform, so that's an obvious move. APU2/3/4 is ACPI based, so I went
> in a different direction - but I'm now investigating how to make DT
> overlays work on an ACPI platform (eg. needs some initial nodes, ...)
> In case that's successful, I'll rework my RFC to use overlays, and
> it will become much smaller (my oftree core changes then won't be
> necessary anymore).
> 
>> It feels a bit like fixing a problem using a different hardware description
>> just because we can. Look in drivers/gpio/gpiolib-acpi.c
>> table gpiolib_acpi_quirks[]. It's just an example how this is fixed using
>> fine granular ACPI-specific mechanisms at several places in the kernel
>> instead of just tossing out the whole description and redoing it in
>> device tree.
> 
> I'm quite reluctant to put everything in there. Theoretically, for apu
> case, I could prevent enumerating the incomplete gpios there, but the
> actual driver setup still remains (certainly don't wanna put that into
> such a global place). But the original problem of having to write so
> much code for just instantiating generic drivers remains. And
> distributing knowledge of certain devices over several places doesn't
> feel like a good idea to me.
> 
> 
> --mtx
> 

Re: [RFC PATCH 11/12] platform/x86: skeleton for oftree based board device initialization

[Enrico Weigelt, metux IT consult]

On 15.02.21 02:18, Frank Rowand wrote:

> The RCAR use of overlays that are built into the driver are a known
> pattern that is explicitly not to be repeated. 

Well, that driver indeed looks quite complex - if belive unnecessarily.
But can't judge on these devices, don't have one of them.

In my case, I believe it's a simple and straightforward approach,
instead of writing a whole driver, that just consists of a bunch
of tables and some trivial setup calls. DT seems to be a perfect
choice for that, since it's a very short and precise language for
describing hw layout, w/o any piece of imperative code.

The only point where I'm still not satisfied with: module auto-loading
requires the match data in the kernel module. But i'd like to have
everything in one source file and not having to write individual
modules for invididual boards anymore. Finally, there should be one
dts per board and really minimal effort adding another dts.
(hmm, maybe I should try generating glue code from dt ?)

BTW: I've already rewritten much of it, using overlay instead of an
completely own detached tree (so, some of the prev patches will fall
off the queue).


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [RFC PATCH 11/12] platform/x86: skeleton for oftree based board device initialization

[Linus Walleij]

On Fri, Feb 12, 2021 at 12:54 PM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
> On 12.02.21 10:58, Linus Walleij wrote:

> > If the usecase is to explicitly work around deployed firmware that cannot
> > and will not be upgraded/fixed by describing the hardware using DT
> > instead, based on just the DMI ID then we should spell that out
> > explicitly.
>
> Okay, maybe I should have stated this more clearly.
>
> OTOH, the scope is also a little bit greater: certain external cards
> that don't need much special handling for the card itself, just
> enumerate devices (and connections between them) using existing drivers.
>
> That's a pretty common scenario in industrial backplane systems, where
> we have lots of different (even application specific) cards, usually
> composed of standard chips, that can be identified by some ID, but
> cannot describe themselves. We have to write lots of specific drivers
> for them, usually just for instantiating existing drivers. (we rarely
> see such code going towards mainline).
>
> A similar case (mainlined) seems to be the RCAR display unit - they're
> using dt overlays that are built into the driver and applied by it
> based on the detected DU at runtime. RCAR seems to be a pure DT
> platform, so that's an obvious move. APU2/3/4 is ACPI based, so I went
> in a different direction - but I'm now investigating how to make DT
> overlays work on an ACPI platform (eg. needs some initial nodes, ...)
> In case that's successful, I'll rework my RFC to use overlays, and
> it will become much smaller (my oftree core changes then won't be
> necessary anymore).

I understand. I have had the same problem with trying to fix 96boards
mezzanines.

I also tried to sidestep the DT overlays, and it was generally disliked.
The DT people have made up their mind that overlays is what they
want to use for this type of stuff.

Yours,
Linus Walleij

[RFC PATCH 12/12] platform/x86/of: add support for PC Engines APU v2/3/4 boards

[Enrico Weigelt, metux IT consult]

Add oftree based support for PC Engines APUv2/3/4 board family.
This is entirely separate from the existing pcengines-apuv2 driver.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 drivers/platform/of/Kconfig   | 15 ++++++++
 drivers/platform/of/Makefile  |  2 +
 drivers/platform/of/apu2x.dts | 86 +++++++++++++++++++++++++++++++++++++++++++
 drivers/platform/of/init.c    |  7 ++++
 4 files changed, 110 insertions(+)
 create mode 100644 drivers/platform/of/apu2x.dts

diff --git a/drivers/platform/of/Kconfig b/drivers/platform/of/Kconfig
index a0b5a641a7c6..e13b6ee8c316 100644
--- a/drivers/platform/of/Kconfig
+++ b/drivers/platform/of/Kconfig
@@ -38,4 +38,19 @@ config PLATFORM_OF_DRV_SYSFS_DT
 	  Say Y here to enable exposing device tree nodes at
 	  /sys/firmware/devicetree.
 
+config PLATFORM_OF_DRV_PCENGINES_APU2
+	bool "Support PC Engines APU2/3/4 mainboards"
+	depends on INPUT
+	depends on GPIOLIB
+	depends on X86
+	select GPIO_AMD_FCH
+	select KEYBOARD_GPIO_POLLED
+	select LEDS_GPIO
+	select INPUT_KEYBOARD
+	help
+	  Say Y to enable devicetree based support for PC Engines APU2/3/4
+	  mainboards. This supersedes the older pcengines-apu2 driver.
+
+	  Supports Gpios, front panel LEDs and front button.
+
 endif # PLATFORM_OF_DRV
diff --git a/drivers/platform/of/Makefile b/drivers/platform/of/Makefile
index 84cf3003c500..dd4a13c18f16 100644
--- a/drivers/platform/of/Makefile
+++ b/drivers/platform/of/Makefile
@@ -2,4 +2,6 @@
 
 ofboard-y := init.o drv.o
 
+ofboard-$(CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2) += apu2x.dtb.o
+
 obj-$(CONFIG_PLATFORM_OF_DRV) += ofboard.o
diff --git a/drivers/platform/of/apu2x.dts b/drivers/platform/of/apu2x.dts
new file mode 100644
index 000000000000..c16a59eb2a0e
--- /dev/null
+++ b/drivers/platform/of/apu2x.dts
@@ -0,0 +1,86 @@
+/dts-v1/;
+
+#include <dt-bindings/leds/common.h>
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/gpio/amd-fch-gpio.h>
+
+/ {
+    apu2x {
+        compatible = "virtual,dmi-board";
+        dmi-sys-vendor = "PC engines";
+        dmi-board-name =
+          "APU2",
+          "apu2",
+          "PC engines apu2",
+          "APU3",
+          "apu3",
+          "PC engines apu3",
+          "APU4",
+          "apu4",
+          "PC engines apu4";
+        unbind {
+            acpi = "PNP0076:00", "PNP0B00:00";
+            platform = "platform-framebuffer.0", "PNP0103:00";
+        };
+        devices {
+            gpio1: gpio1 {
+                compatible = "amd,fch-gpio";
+                gpio-controller;
+                status = "okay";
+                ngpios=<7>;
+                #gpio-cells=<2>;
+                gpio-regs = <
+                    AMD_FCH_GPIO_REG_GPIO57 // led1
+                    AMD_FCH_GPIO_REG_GPIO58 // led2
+                    AMD_FCH_GPIO_REG_GPIO59_DEVSLP1 // led3
+                    AMD_FCH_GPIO_REG_GPIO32_GE1 // modesw
+                    AMD_FCH_GPIO_REG_GPIO33_GE2 // simawap
+                    AMD_FCH_GPIO_REG_GPIO55_DEVSLP0 // mpcie2
+                    AMD_FCH_GPIO_REG_GPIO51 // mpcie3
+                >;
+                gpio-line-names =
+                    "front-led1",
+                    "front-led2",
+                    "front-led3",
+                    "front-button",
+                    "simswap",
+                    "mpcie2_reset",
+                    "mpcie3_reset";
+            };
+            front-leds {
+                compatible = "gpio-leds";
+                led@0 {
+                    gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>;
+                    color = <LED_COLOR_ID_GREEN>;
+                    default-state = "keep";
+                    label = "apu:green:1";
+                };
+                led@1 {
+                    gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>;
+                    color = <LED_COLOR_ID_GREEN>;
+                    default-state = "keep";
+                    label = "apu:green:2";
+                };
+                led@2 {
+                    gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
+                    color = <LED_COLOR_ID_GREEN>;
+                    default-state = "keep";
+                    label = "apu:green:3";
+                };
+            };
+            front-keys {
+                compatible = "gpio-keys-polled";
+                address-cells = <1>;
+                size-cells = <0>;
+                poll-interval = <100>;
+                button@1 {
+                    linux,code = <KEY_RESTART>;
+                    label = "front button";
+                    debounce-interval = <10>;
+                    gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;
+                };
+            };
+        };
+    };
+};
diff --git a/drivers/platform/of/init.c b/drivers/platform/of/init.c
index 3b8373cda77a..195120dad26d 100644
--- a/drivers/platform/of/init.c
+++ b/drivers/platform/of/init.c
@@ -47,7 +47,14 @@ static ssize_t fdt_image_raw_read(struct file *filep, struct kobject *kobj,
 	return count;
 }
 
+#ifdef CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2
+DECLARE_FDT_EXTERN(apu2x);
+#endif
+
 static struct fdt_image fdt[] = {
+#ifdef CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2
+	FDT_IMAGE_ENT(apu2x)
+#endif
 };
 
 static int __init ofdrv_init_sysfs(struct fdt_image *image)
-- 
2.11.0

Re: [RFC PATCH 12/12] platform/x86/of: add support for PC Engines APU v2/3/4 boards

[Rob Herring]

On Mon, Feb 8, 2021 at 4:22 PM Enrico Weigelt, metux IT consult
<info@metux.net> wrote:
>
> Add oftree based support for PC Engines APUv2/3/4 board family.
> This is entirely separate from the existing pcengines-apuv2 driver.
>
> Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
> ---
>  drivers/platform/of/Kconfig   | 15 ++++++++
>  drivers/platform/of/Makefile  |  2 +
>  drivers/platform/of/apu2x.dts | 86 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/platform/of/init.c    |  7 ++++
>  4 files changed, 110 insertions(+)
>  create mode 100644 drivers/platform/of/apu2x.dts
>
> diff --git a/drivers/platform/of/Kconfig b/drivers/platform/of/Kconfig
> index a0b5a641a7c6..e13b6ee8c316 100644
> --- a/drivers/platform/of/Kconfig
> +++ b/drivers/platform/of/Kconfig
> @@ -38,4 +38,19 @@ config PLATFORM_OF_DRV_SYSFS_DT
>           Say Y here to enable exposing device tree nodes at
>           /sys/firmware/devicetree.
>
> +config PLATFORM_OF_DRV_PCENGINES_APU2
> +       bool "Support PC Engines APU2/3/4 mainboards"
> +       depends on INPUT
> +       depends on GPIOLIB
> +       depends on X86
> +       select GPIO_AMD_FCH
> +       select KEYBOARD_GPIO_POLLED
> +       select LEDS_GPIO
> +       select INPUT_KEYBOARD
> +       help
> +         Say Y to enable devicetree based support for PC Engines APU2/3/4
> +         mainboards. This supersedes the older pcengines-apu2 driver.
> +
> +         Supports Gpios, front panel LEDs and front button.
> +
>  endif # PLATFORM_OF_DRV
> diff --git a/drivers/platform/of/Makefile b/drivers/platform/of/Makefile
> index 84cf3003c500..dd4a13c18f16 100644
> --- a/drivers/platform/of/Makefile
> +++ b/drivers/platform/of/Makefile
> @@ -2,4 +2,6 @@
>
>  ofboard-y := init.o drv.o
>
> +ofboard-$(CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2) += apu2x.dtb.o
> +
>  obj-$(CONFIG_PLATFORM_OF_DRV) += ofboard.o
> diff --git a/drivers/platform/of/apu2x.dts b/drivers/platform/of/apu2x.dts
> new file mode 100644
> index 000000000000..c16a59eb2a0e
> --- /dev/null
> +++ b/drivers/platform/of/apu2x.dts
> @@ -0,0 +1,86 @@
> +/dts-v1/;
> +
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/gpio/amd-fch-gpio.h>
> +
> +/ {
> +    apu2x {
> +        compatible = "virtual,dmi-board";
> +        dmi-sys-vendor = "PC engines";
> +        dmi-board-name =
> +          "APU2",
> +          "apu2",
> +          "PC engines apu2",
> +          "APU3",
> +          "apu3",
> +          "PC engines apu3",
> +          "APU4",
> +          "apu4",
> +          "PC engines apu4";

I think these DMI properties just need to be the compatible string(s).
We already have a way to do matching with DT and don't need a
secondary way. If you can

> +        unbind {
> +            acpi = "PNP0076:00", "PNP0B00:00";
> +            platform = "platform-framebuffer.0", "PNP0103:00";

This node really needs to go. It's clearly Linuxisms. It either has to
go in the kernel or userspace.

> +        };
> +        devices {
> +            gpio1: gpio1 {
> +                compatible = "amd,fch-gpio";

This of course will need to be documented.

> +                gpio-controller;
> +                status = "okay";

nit: That's the default.

> +                ngpios=<7>;
> +                #gpio-cells=<2>;
> +                gpio-regs = <
> +                    AMD_FCH_GPIO_REG_GPIO57 // led1
> +                    AMD_FCH_GPIO_REG_GPIO58 // led2
> +                    AMD_FCH_GPIO_REG_GPIO59_DEVSLP1 // led3
> +                    AMD_FCH_GPIO_REG_GPIO32_GE1 // modesw
> +                    AMD_FCH_GPIO_REG_GPIO33_GE2 // simawap
> +                    AMD_FCH_GPIO_REG_GPIO55_DEVSLP0 // mpcie2
> +                    AMD_FCH_GPIO_REG_GPIO51 // mpcie3
> +                >;
> +                gpio-line-names =
> +                    "front-led1",
> +                    "front-led2",
> +                    "front-led3",
> +                    "front-button",
> +                    "simswap",
> +                    "mpcie2_reset",
> +                    "mpcie3_reset";
> +            };
> +            front-leds {
> +                compatible = "gpio-leds";
> +                led@0 {
> +                    gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>;
> +                    color = <LED_COLOR_ID_GREEN>;
> +                    default-state = "keep";
> +                    label = "apu:green:1";
> +                };
> +                led@1 {
> +                    gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>;
> +                    color = <LED_COLOR_ID_GREEN>;
> +                    default-state = "keep";
> +                    label = "apu:green:2";
> +                };
> +                led@2 {
> +                    gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
> +                    color = <LED_COLOR_ID_GREEN>;
> +                    default-state = "keep";
> +                    label = "apu:green:3";
> +                };
> +            };
> +            front-keys {
> +                compatible = "gpio-keys-polled";
> +                address-cells = <1>;
> +                size-cells = <0>;
> +                poll-interval = <100>;
> +                button@1 {
> +                    linux,code = <KEY_RESTART>;
> +                    label = "front button";
> +                    debounce-interval = <10>;
> +                    gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;
> +                };
> +            };
> +        };
> +    };
> +};
> diff --git a/drivers/platform/of/init.c b/drivers/platform/of/init.c
> index 3b8373cda77a..195120dad26d 100644
> --- a/drivers/platform/of/init.c
> +++ b/drivers/platform/of/init.c
> @@ -47,7 +47,14 @@ static ssize_t fdt_image_raw_read(struct file *filep, struct kobject *kobj,
>         return count;
>  }
>
> +#ifdef CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2
> +DECLARE_FDT_EXTERN(apu2x);
> +#endif
> +
>  static struct fdt_image fdt[] = {
> +#ifdef CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2
> +       FDT_IMAGE_ENT(apu2x)
> +#endif
>  };
>
>  static int __init ofdrv_init_sysfs(struct fdt_image *image)
> --
> 2.11.0
>

Re: [RFC PATCH 12/12] platform/x86/of: add support for PC Engines APU v2/3/4 boards

[Enrico Weigelt, metux IT consult]

On 09.02.21 01:06, Rob Herring wrote:

Hi,

>> +/ {
>> +    apu2x {
>> +        compatible = "virtual,dmi-board";
>> +        dmi-sys-vendor = "PC engines";
>> +        dmi-board-name =
>> +          "APU2",
>> +          "apu2",
>> +          "PC engines apu2",
>> +          "APU3",
>> +          "apu3",
>> +          "PC engines apu3",
>> +          "APU4",
>> +          "apu4",
>> +          "PC engines apu4";
> 
> I think these DMI properties just need to be the compatible string(s).
> We already have a way to do matching with DT and don't need a
> secondary way. If you can

It's not easy fitting that into one string, because we've got lots of
combinations that need to be matched. In this specific case, I haven't
seen any board where the vendor name isn't an exact match of the given
string (that's why it's only one entry), but in the past seen several
boards where even this changes between bios versions. The board names,
more varying.

Something that's not reflected in this example yet: there're even more
subtle differences between production series (eg. certain pins not
wired, etc). Supporting such things would need adding more matching
rules and possibly runtime DT manipulations.

>> +        unbind {
>> +            acpi = "PNP0076:00", "PNP0B00:00";
>> +            platform = "platform-framebuffer.0", "PNP0103:00";
> 
> This node really needs to go. It's clearly Linuxisms. It either has to
> go in the kernel or userspace.

Note that the whole thing here *is* a Linuxism. This kind of DTs is
built into the kernel, not in firmware or anywhere else. This stuff is
only for cases where firmware is not giving, or giving broken
information. And it's for replacing hand-written C code by a machine
readable description.

I had to put that in, since in some cases firmware (-versions) already
enumerates some devices, but does it in a wrong or incomplete way.
So, these devices need to be removed first, before the correct ones
can be initialized. (note that this patch, for now, is just an hacking
example - some details are still broken).

If anybody has a better idea how to do that, let me know.

In general, I'd like to have everything for one board (family) in one
declarative file.

>> +        };
>> +        devices {
>> +            gpio1: gpio1 {
>> +                compatible = "amd,fch-gpio";
> 
> This of course will need to be documented.

Yes, but that's a different issue. It's still in RFC stage.
The gpio-amd-fch changes are in this patch queue for a complete example,
but probably will be upstreamed separately.

>> +                gpio-controller;
>> +                status = "okay";
> 
> nit: That's the default.

Okay, dropping it.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [RFC PATCH 12/12] platform/x86/of: add support for PC Engines APU v2/3/4 boards

[Linus Walleij]

On Mon, Feb 8, 2021 at 11:22 PM Enrico Weigelt, metux IT consult
<info@metux.net> wrote:

> +                gpio-regs = <
> +                    AMD_FCH_GPIO_REG_GPIO57 // led1
> +                    AMD_FCH_GPIO_REG_GPIO58 // led2
> +                    AMD_FCH_GPIO_REG_GPIO59_DEVSLP1 // led3
> +                    AMD_FCH_GPIO_REG_GPIO32_GE1 // modesw
> +                    AMD_FCH_GPIO_REG_GPIO33_GE2 // simawap
> +                    AMD_FCH_GPIO_REG_GPIO55_DEVSLP0 // mpcie2
> +                    AMD_FCH_GPIO_REG_GPIO51 // mpcie3
> +                >;

Please don't define registers in the DTS files. Determine the set of registers
from the compatible string and put them in the driver. If that is not possible,
the compatible string is not precise enough and needs to indicate properly
which hardware this is.

Yours,
Linus Walleij

Re: RFC: oftree based setup of composite board devices

[Rob Herring]

+Johan H

On Mon, Feb 8, 2021 at 4:22 PM Enrico Weigelt, metux IT consult
<info@metux.net> wrote:
>
> Hello folks,
>
> here's an RFC for using compiled-in dtb's for initializing board devices
> that can't be probed via bus'es or firmware.

I'm not convinced compiled in is the mechanism we want.

> Use cases are boards with non-oftree firmware (ACPI, etc) where certain
> platform devices can't be directly enumerated via firmware. Traditionally
> we had to write board specific drivers that check for board identification
> (DMI strings, etc), then initialize the actual devices and their links
> (eg. gpio<->leds/buttons, ...). Often this can be expressed just by DT.

This is something I've wanted to see for a while. There's use cases
for DT based systems too. The example I'd like to see supported are
USB serial adapters with downstream serdev, GPIO, I2C, SPI, etc. Then
plug more than one of those in.

> This patch queue does a bunch of preparations in oftree code, so we can
> support multiple fully independent DT's (not using DT overlays). And then
> adds a generic driver parses compiled-in fdt blobs, checks for mathing
> DMI strings and initializes the devices. As an example, the last patch
> adds an alternative implementation for the PC engines APU2/3/4 board
> family based on device tree.

I think there's a couple of approaches we could take. Either support
multiple root nodes as you have done or keep a single root and add
child nodes to them. I think the latter would be less invasive. In the
non-DT cases, we'd just always create an empty skeleton DT. A 3rd
variation on a DT system is we could want to create parent nodes if
they don't exist to attach this DT to so we have a full hierarchy.

I'm not saying which one we should do, just laying out some of the options.

> The approach can be easily be extended to other kinds of composite devices,
> eg. PCI cards or USB dongles.
>
>
> Yet some drawbacks of the current implementation:
>
>  * individual FDT's can't be modularized yet (IMHO, we don't have DMI-based
>    modprobing anyways)

I think we need to use either firmware loading or udev mechanisms to
load the FDTs.

>  * can't reconfigure or attach to devices outside the individual DT's
>    (eg. probed by PCI, etc)

Not sure I follow.

Rob

Re: RFC: oftree based setup of composite board devices

[Enrico Weigelt, metux IT consult]

On 09.02.21 00:48, Rob Herring wrote:

Hi,

>> here's an RFC for using compiled-in dtb's for initializing board devices
>> that can't be probed via bus'es or firmware.
> 
> I'm not convinced compiled in is the mechanism we want.

To make it clear, I'm talking of DTBs compiled into the ofboard driver
(which itself can be a module). And yes, that's pretty much what I want.
It's meant for drop-in replacement of composite board drivers, in cases
where this driver doesn't do more than initializing other drivers.

Therefore, it should work without any special userland actions, and it
should put all board specific stuff into dtb.

>> Use cases are boards with non-oftree firmware (ACPI, etc) where certain
>> platform devices can't be directly enumerated via firmware. Traditionally
>> we had to write board specific drivers that check for board identification
>> (DMI strings, etc), then initialize the actual devices and their links
>> (eg. gpio<->leds/buttons, ...). Often this can be expressed just by DT.
> 
> This is something I've wanted to see for a while. There's use cases
> for DT based systems too. The example I'd like to see supported are
> USB serial adapters with downstream serdev, GPIO, I2C, SPI, etc. Then
> plug more than one of those in.

Yes, that's also on my 2do list (eg. adcs behind some usb-i2c dongle)

> I think there's a couple of approaches we could take. Either support
> multiple root nodes as you have done or keep a single root and add
> child nodes to them. I think the latter would be less invasive. In the
> non-DT cases, we'd just always create an empty skeleton DT. A 3rd
> variation on a DT system is we could want to create parent nodes if
> they don't exist to attach this DT to so we have a full hierarchy.

I'm already investigating this idea.

Actually, I'm also thinking a bit further, whether for the future it
could make sense converting the acpi tables into oftree at runtime.
Not sure whether it's good idea, but maybe we could consolidate the
platform driver probing into one, more generic mechanism.

>> Yet some drawbacks of the current implementation:
>>
>>   * individual FDT's can't be modularized yet (IMHO, we don't have DMI-based
>>     modprobing anyways)
> 
> I think we need to use either firmware loading or udev mechanisms to
> load the FDTs.

In my usecase neither would not be a good idea, because:

a) on common machines (eg. pc's) we can't touch firmware easily
    (if we could, we wouldn't need those board drivers in the first
     place - we'd just fix the firmware :p)
b) I'd like to have my new mechanism as a drop-in replacement for
    existing drivers, reduce the init boilerplace to just a piece of dt.
    Don't wanna force users to do userland changes on a kernel upgrade.

Userland-driven approach IMHO makes sense for extra devices behind some
interfaces, that itself is probed otherwise, and we don't know hat the
user has attached to it (eg. USB->SPI adapter).

>>   * can't reconfigure or attach to devices outside the individual DT's
>>     (eg. probed by PCI, etc)
> 
> Not sure I follow.

Let's take an example:

I've got a PCI card with a bunch of generic chips, where we already have
drivers for. A traditional driver would be probed the usual pci way, and
then instantiate sub-devices directly.

That's lots of boilerplace code, whose semantics could be described
entirely via DT. In order to make that work, I need two things:

1. create a pci device instance (when the card is found)
2. instantiate all sub-devices with the card device as parent

Another problem:

I've got extra devices behind an interface that itself already is
enumerated by firmware or some bus, but the extra devices aren't.
(eg. acpi already enumerates some gpio's, but not what's connected
to them, eg. leds, keys, ...). In this case, I somehow need to get
these parent devices into my DT's scope, so the additional devices
can refer to them.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: RFC: oftree based setup of composite board devices

[Frank Rowand]

On 2/8/21 5:48 PM, Rob Herring wrote:
> +Johan H
> 
> On Mon, Feb 8, 2021 at 4:22 PM Enrico Weigelt, metux IT consult
> <info@metux.net> wrote:
>>
>> Hello folks,
>>
>> here's an RFC for using compiled-in dtb's for initializing board devices
>> that can't be probed via bus'es or firmware.

I've just been monitoring this thread for several days, hoping that the
discussion would make things more clear for me.

Disclaimer: I know essentially nothing about ACPI, please excuse improper
naming and misunderstandings on my part.

Why not compile in ACPI data (tables?) instead of devicetree description?

> 
> I'm not convinced compiled in is the mechanism we want.
> 
>> Use cases are boards with non-oftree firmware (ACPI, etc) where certain
>> platform devices can't be directly enumerated via firmware. Traditionally
>> we had to write board specific drivers that check for board identification
>> (DMI strings, etc), then initialize the actual devices and their links
>> (eg. gpio<->leds/buttons, ...). Often this can be expressed just by DT.
> 
> This is something I've wanted to see for a while. There's use cases
> for DT based systems too. The example I'd like to see supported are
> USB serial adapters with downstream serdev, GPIO, I2C, SPI, etc. Then
> plug more than one of those in.

My understanding from the past is that the experts (those who understand both
devicetree and ACPI) regard trying to mix devicetree and ACPI in a single
running Linux kernel image is insanity, or at least likely to be confusing,
difficult, and problematic.

From the devicetree side, I expect nightmares for me if devicetree and ACPI
are mixed in a single running kernel image.

> 
>> This patch queue does a bunch of preparations in oftree code, so we can
>> support multiple fully independent DT's (not using DT overlays). And then
>> adds a generic driver parses compiled-in fdt blobs, checks for mathing
>> DMI strings and initializes the devices. As an example, the last patch
>> adds an alternative implementation for the PC engines APU2/3/4 board
>> family based on device tree.
> 
> I think there's a couple of approaches we could take. Either support
> multiple root nodes as you have done or keep a single root and add
> child nodes to them. I think the latter would be less invasive. In the
> non-DT cases, we'd just always create an empty skeleton DT. A 3rd
> variation on a DT system is we could want to create parent nodes if
> they don't exist to attach this DT to so we have a full hierarchy.
> 
> I'm not saying which one we should do, just laying out some of the options.
> 

Multiple root nodes and disjoint trees both seem problematic.  Existing
subsystems and drivers expect a single cohesive tree.  Changing that
architecture looks to me to be a painful exercise.

>> The approach can be easily be extended to other kinds of composite devices,
>> eg. PCI cards or USB dongles.
>>
>>
>> Yet some drawbacks of the current implementation:
>>
>>  * individual FDT's can't be modularized yet (IMHO, we don't have DMI-based
>>    modprobing anyways)
> 
> I think we need to use either firmware loading or udev mechanisms to
> load the FDTs.
> 
>>  * can't reconfigure or attach to devices outside the individual DT's
>>    (eg. probed by PCI, etc)
> 
> Not sure I follow.
> 
> Rob
> 

Re: RFC: oftree based setup of composite board devices

[Andy Shevchenko]

On Mon, Feb 15, 2021 at 3:15 AM Frank Rowand <frowand.list@gmail.com> wrote:
> On 2/8/21 5:48 PM, Rob Herring wrote:
> > On Mon, Feb 8, 2021 at 4:22 PM Enrico Weigelt, metux IT consult
> > <info@metux.net> wrote:
> >>
> >> Hello folks,
> >>
> >> here's an RFC for using compiled-in dtb's for initializing board devices
> >> that can't be probed via bus'es or firmware.
>
> I've just been monitoring this thread for several days, hoping that the
> discussion would make things more clear for me.

You beat me up to it. I support your comments.

I have to comment to Enrico and others that under overlays for
ACPI-based platforms I meant SSDT overlays, no DT.

Also I have to point out that we have swnode API for the cases where
we need quirks for either ACPI or DT or whatever (not yet present)
firmware quirks.

> Disclaimer: I know essentially nothing about ACPI, please excuse improper
> naming and misunderstandings on my part.
>
> Why not compile in ACPI data (tables?) instead of devicetree description?
>
> >
> > I'm not convinced compiled in is the mechanism we want.
> >
> >> Use cases are boards with non-oftree firmware (ACPI, etc) where certain
> >> platform devices can't be directly enumerated via firmware. Traditionally
> >> we had to write board specific drivers that check for board identification
> >> (DMI strings, etc), then initialize the actual devices and their links
> >> (eg. gpio<->leds/buttons, ...). Often this can be expressed just by DT.
> >
> > This is something I've wanted to see for a while. There's use cases
> > for DT based systems too. The example I'd like to see supported are
> > USB serial adapters with downstream serdev, GPIO, I2C, SPI, etc. Then
> > plug more than one of those in.
>
> My understanding from the past is that the experts (those who understand both
> devicetree and ACPI) regard trying to mix devicetree and ACPI in a single
> running Linux kernel image is insanity, or at least likely to be confusing,
> difficult, and problematic.
>
> From the devicetree side, I expect nightmares for me if devicetree and ACPI
> are mixed in a single running kernel image.
>
> >
> >> This patch queue does a bunch of preparations in oftree code, so we can
> >> support multiple fully independent DT's (not using DT overlays). And then
> >> adds a generic driver parses compiled-in fdt blobs, checks for mathing
> >> DMI strings and initializes the devices. As an example, the last patch
> >> adds an alternative implementation for the PC engines APU2/3/4 boa> Disclaimer: I know essentially nothing about ACPI, please excuse improper
rd
> >> family based on device tree.
> >
> > I think there's a couple of approaches we could take. Either support
> > multiple root nodes as you have done or keep a single root and add
> > child nodes to them. I think the latter would be less invasive. In the
> > non-DT cases, we'd just always create an empty skeleton DT. A 3rd
> > variation on a DT system is we could want to create parent nodes if
> > they don't exist to attach this DT to so we have a full hierarchy.
> >
> > I'm not saying which one we should do, just laying out some of the options.
> >
>
> Multiple root nodes and disjoint trees both seem problematic.  Existing
> subsystems and drivers expect a single cohesive tree.  Changing that
> architecture looks to me to be a painful exercise.
>
> >> The approach can be easily be extended to other kinds of composite devices,
> >> eg. PCI cards or USB dongles.
> >>
> >>
> >> Yet some drawbacks of the current implementation:
> >>
> >>  * individual FDT's can't be modularized yet (IMHO, we don't have DMI-based
> >>    modprobing anyways)
> >
> > I think we need to use either firmware loading or udev mechanisms to
> > load the FDTs.
> >
> >>  * can't reconfigure or attach to devices outside the individual DT's
> >>    (eg. probed by PCI, etc)
> >
> > Not sure I follow.
> >
> > Rob
> >
>


-- 
With Best Regards,
Andy Shevchenko

Re: RFC: oftree based setup of composite board devices

[Enrico Weigelt, metux IT consult]

On 15.02.21 02:12, Frank Rowand wrote:

> Why not compile in ACPI data (tables?) instead of devicetree description?

The problem is a bit more complex than it might seem.

Let's take the APU2/37/4 boards as an example. They've got some aux
devices, eg. some gpio controller, and some things (leds, keys, reset
lines, etc) attached to it.

Now we've got lots of different bios versions in the field,
enumerating only some of the devices. For example, older ones didn't
even contain the gpio, later ones added just gpio, other ones just
added LEDs (with different names than the Linux driver already mainlined
and field-deployed at that time), but still other lines unhandled, etc, 
etc. etc.

A big mess :( And I can't ask everybody to do bios uprade on devices far
out in the field (litterally open field, sometimes offshore, ...). So, I
need a usable solution, that's also maintainable, w/o testing each
single combination of board, bios, etc. IOW: without relying on bios
(except for board identification)

OTOH, I'm also looking for a solution get rid writing those kind of
relatively huge board drivers, that pretty are much like old fashioned
board files from pre-DT times - just made up of lots of tables and
a few trivial register-something calls. Sounds pretty much like the
original use case of oftree.

The primary difference between classic oftree and this scanario:
* this is additional to existing platform information, which is
   incomplete or even incorrect (and that can't be fixed)
* extra carrier boards that are detected by other means, but no
   enumeration of the devices on it.

>> This is something I've wanted to see for a while. There's use cases
>> for DT based systems too. The example I'd like to see supported are
>> USB serial adapters with downstream serdev, GPIO, I2C, SPI, etc. Then
>> plug more than one of those in.
> 
> My understanding from the past is that the experts (those who understand both
> devicetree and ACPI) regard trying to mix devicetree and ACPI in a single
> running Linux kernel image is insanity, or at least likely to be confusing,
> difficult, and problematic.

Well, mixing different, overlapping data sources tends to be tricky. The
same problem exists with the classic approach of hand-written board
drivers. So there have to be clear border lines.

In my case (eg. apu2+ boards), the overlap is only that some bios
versions enumerate the gpio chip, others even some of the gpio-based
devices. I'm attempting to solve this by just kicking out those
duplicate devices, if they exist. The alternative could be leaving them
in an trying to bind the missing ones to them. But that would be really
complicatd and needs to be well crafted for lots of different board and
bios versions - a kind of complexity we wanna avoid.

My use cases are actually a bit easier than the average dt overlay
cases, as I have almost no interactions with already existing devices
(except that some specific devices have to be moved out of the way)

The original DT overlay use case, arbitrary expansion boards (eg. on
raspi), are trickier, if the overlays shall be generic over a wider
range of base boards (eg. same overlay for any raspi or odroid).
This is something calling for an own (pseudo-)bus type that handles
the correct probing ... I've hacked up something similar for the APU2+'s
combined msata/usb/mpcie ports.

BTW: I've already been thinking of ways for internally transforming ACPI
tables into DT data structures (struct device_node) at an early point,
before probing. But that would be another research project with unknown
outcome, and most likely a HUGE change. Not what I'm talking about now.

> From the devicetree side, I expect nightmares for me if devicetree and ACPI
> are mixed in a single running kernel image.

Note that I'm not talking about arbitrary configurations. Just re-using
existing device tree code to express things that are currently open
coded C into DT.

It's NOT trying to boot an ACPI-based machine with DT. (which would be
yet another research project)

> Multiple root nodes and disjoint trees both seem problematic.  Existing
> subsystems and drivers expect a single cohesive tree.  Changing that
> architecture looks to me to be a painful exercise.

Yes, it's not entirely trivial, but managable. My experiments seemed to
work so far, and I couldn't see general blockers yet. Drivers usually
expect certain sub-nodes, but haven't found any that expect their node
being embedded in some other one. (maybe there really are some, but the
likehood that they're applicable in these use cases looks pretty low).


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: RFC: oftree based setup of composite board devices

[Frank Rowand]

On 2/24/21 7:00 AM, Enrico Weigelt, metux IT consult wrote:
> On 15.02.21 02:12, Frank Rowand wrote:
> 
>> Why not compile in ACPI data (tables?) instead of devicetree description?
> 
> The problem is a bit more complex than it might seem.
> 
> Let's take the APU2/37/4 boards as an example. They've got some aux
> devices, eg. some gpio controller, and some things (leds, keys, reset
> lines, etc) attached to it.
> 
> Now we've got lots of different bios versions in the field,
> enumerating only some of the devices. For example, older ones didn't
> even contain the gpio, later ones added just gpio, other ones just
> added LEDs (with different names than the Linux driver already mainlined
> and field-deployed at that time), but still other lines unhandled, etc, etc. etc.
> 
> A big mess :( And I can't ask everybody to do bios uprade on devices far
> out in the field (litterally open field, sometimes offshore, ...). So, I
> need a usable solution, that's also maintainable, w/o testing each
> single combination of board, bios, etc. IOW: without relying on bios
> (except for board identification)
> 
> OTOH, I'm also looking for a solution get rid writing those kind of
> relatively huge board drivers, that pretty are much like old fashioned
> board files from pre-DT times - just made up of lots of tables and
> a few trivial register-something calls. Sounds pretty much like the
> original use case of oftree.
> 
> The primary difference between classic oftree and this scanario:
> * this is additional to existing platform information, which is
>   incomplete or even incorrect (and that can't be fixed)
> * extra carrier boards that are detected by other means, but no
>   enumeration of the devices on it.
> 
>>> This is something I've wanted to see for a while. There's use cases
>>> for DT based systems too. The example I'd like to see supported are
>>> USB serial adapters with downstream serdev, GPIO, I2C, SPI, etc. Then
>>> plug more than one of those in.
>>
>> My understanding from the past is that the experts (those who understand both
>> devicetree and ACPI) regard trying to mix devicetree and ACPI in a single
>> running Linux kernel image is insanity, or at least likely to be confusing,
>> difficult, and problematic.

Since you have persisted, a more referenced and emphatic "no" to mixing ACPI
and devicetree:

  https://elinux.org/Device_Tree_Linux#mixing_devicetree_and_ACPI


> 
> Well, mixing different, overlapping data sources tends to be tricky. The
> same problem exists with the classic approach of hand-written board
> drivers. So there have to be clear border lines.
> 
> In my case (eg. apu2+ boards), the overlap is only that some bios
> versions enumerate the gpio chip, others even some of the gpio-based
> devices. I'm attempting to solve this by just kicking out those
> duplicate devices, if they exist. The alternative could be leaving them
> in an trying to bind the missing ones to them. But that would be really
> complicatd and needs to be well crafted for lots of different board and
> bios versions - a kind of complexity we wanna avoid.

So you want to use devicetree data to fix broken ACPI data.

Again, why don't you use data in an ACPI format to fix broken ACPI
data?

-Frank

> 
> My use cases are actually a bit easier than the average dt overlay
> cases, as I have almost no interactions with already existing devices
> (except that some specific devices have to be moved out of the way)
> 
> The original DT overlay use case, arbitrary expansion boards (eg. on
> raspi), are trickier, if the overlays shall be generic over a wider
> range of base boards (eg. same overlay for any raspi or odroid).
> This is something calling for an own (pseudo-)bus type that handles
> the correct probing ... I've hacked up something similar for the APU2+'s
> combined msata/usb/mpcie ports.
> 
> BTW: I've already been thinking of ways for internally transforming ACPI
> tables into DT data structures (struct device_node) at an early point,
> before probing. But that would be another research project with unknown
> outcome, and most likely a HUGE change. Not what I'm talking about now.
> 
>> From the devicetree side, I expect nightmares for me if devicetree and ACPI
>> are mixed in a single running kernel image.
> 
> Note that I'm not talking about arbitrary configurations. Just re-using
> existing device tree code to express things that are currently open
> coded C into DT.
> 
> It's NOT trying to boot an ACPI-based machine with DT. (which would be
> yet another research project)
> 
>> Multiple root nodes and disjoint trees both seem problematic.  Existing
>> subsystems and drivers expect a single cohesive tree.  Changing that
>> architecture looks to me to be a painful exercise.
> 
> Yes, it's not entirely trivial, but managable. My experiments seemed to
> work so far, and I couldn't see general blockers yet. Drivers usually
> expect certain sub-nodes, but haven't found any that expect their node
> being embedded in some other one. (maybe there really are some, but the
> likehood that they're applicable in these use cases looks pretty low).
> 
> 
> --mtx
> 

Re: RFC: oftree based setup of composite board devices

[Rob Herring]

On Wed, Feb 24, 2021 at 05:14:10PM -0600, Frank Rowand wrote:
> On 2/24/21 7:00 AM, Enrico Weigelt, metux IT consult wrote:
> > On 15.02.21 02:12, Frank Rowand wrote:
> > 
> >> Why not compile in ACPI data (tables?) instead of devicetree description?
> > 
> > The problem is a bit more complex than it might seem.
> > 
> > Let's take the APU2/37/4 boards as an example. They've got some aux
> > devices, eg. some gpio controller, and some things (leds, keys, reset
> > lines, etc) attached to it.
> > 
> > Now we've got lots of different bios versions in the field,
> > enumerating only some of the devices. For example, older ones didn't
> > even contain the gpio, later ones added just gpio, other ones just
> > added LEDs (with different names than the Linux driver already mainlined
> > and field-deployed at that time), but still other lines unhandled, etc, etc. etc.
> > 
> > A big mess :( And I can't ask everybody to do bios uprade on devices far
> > out in the field (litterally open field, sometimes offshore, ...). So, I
> > need a usable solution, that's also maintainable, w/o testing each
> > single combination of board, bios, etc. IOW: without relying on bios
> > (except for board identification)
> > 
> > OTOH, I'm also looking for a solution get rid writing those kind of
> > relatively huge board drivers, that pretty are much like old fashioned
> > board files from pre-DT times - just made up of lots of tables and
> > a few trivial register-something calls. Sounds pretty much like the
> > original use case of oftree.
> > 
> > The primary difference between classic oftree and this scanario:
> > * this is additional to existing platform information, which is
> >   incomplete or even incorrect (and that can't be fixed)
> > * extra carrier boards that are detected by other means, but no
> >   enumeration of the devices on it.
> > 
> >>> This is something I've wanted to see for a while. There's use cases
> >>> for DT based systems too. The example I'd like to see supported are
> >>> USB serial adapters with downstream serdev, GPIO, I2C, SPI, etc. Then
> >>> plug more than one of those in.
> >>
> >> My understanding from the past is that the experts (those who understand both
> >> devicetree and ACPI) regard trying to mix devicetree and ACPI in a single
> >> running Linux kernel image is insanity, or at least likely to be confusing,
> >> difficult, and problematic.
> 
> Since you have persisted, a more referenced and emphatic "no" to mixing ACPI
> and devicetree:
> 
>   https://elinux.org/Device_Tree_Linux#mixing_devicetree_and_ACPI

The 'no' is for mixing the 2. Despite the no, it's been happening 
anyways. That's mostly been the reusing DT bindings in ACPI tables. I 
won't review such bindings if I realize that's what they are, but 
usually that's not evident.

This is a bit different I think where both ACPI and DT are used, but 
they are totally disjoint. I don't have any issue with that. Why would 
we want to force 2 different firmware descriptions for something 
otherwise independent of the base system's firmware. Maybe that's not 
exactly the case here, but the same changes to the DT code are needed 
(unless you have a better solution for my example than multiple roots). 
If someone wants to implement the changes that align with what I want, 
then I don't really care what their motivation is.

Rob

Re: RFC: oftree based setup of composite board devices

[Andy Shevchenko]

On Tue, Feb 9, 2021 at 12:25 AM Enrico Weigelt, metux IT consult
<info@metux.net> wrote:
>
> Hello folks,
>
> here's an RFC for using compiled-in dtb's for initializing board devices
> that can't be probed via bus'es or firmware.
>
> Use cases are boards with non-oftree firmware (ACPI, etc) where certain
> platform devices can't be directly enumerated via firmware. Traditionally
> we had to write board specific drivers that check for board identification
> (DMI strings, etc), then initialize the actual devices and their links
> (eg. gpio<->leds/buttons, ...). Often this can be expressed just by DT.

In ACPI we support DT compatible strings, and we support overlays for
a long time. Would it work for you?

> This patch queue does a bunch of preparations in oftree code, so we can
> support multiple fully independent DT's (not using DT overlays). And then
> adds a generic driver parses compiled-in fdt blobs, checks for mathing
> DMI strings and initializes the devices. As an example, the last patch
> adds an alternative implementation for the PC engines APU2/3/4 board
> family based on device tree.

Sounds weird, but let's see...

> The approach can be easily be extended to other kinds of composite devices,
> eg. PCI cards or USB dongles.

What do you mean? PCI and USB are self-enumerated. What's wrong with them?

> Yet some drawbacks of the current implementation:
>
>  * individual FDT's can't be modularized yet (IMHO, we don't have DMI-based
>    modprobing anyways)

What?! https://lwn.net/Articles/233385/
`git grep -n 'MODULE_DEVICE_TABLE(dmi'`

>  * can't reconfigure or attach to devices outside the individual DT's
>    (eg. probed by PCI, etc)


-- 
With Best Regards,
Andy Shevchenko

Re: RFC: oftree based setup of composite board devices

[Enrico Weigelt, metux IT consult]

On 10.02.21 11:30, Andy Shevchenko wrote:

Hi,

>> Use cases are boards with non-oftree firmware (ACPI, etc) where certain
>> platform devices can't be directly enumerated via firmware. Traditionally
>> we had to write board specific drivers that check for board identification
>> (DMI strings, etc), then initialize the actual devices and their links
>> (eg. gpio<->leds/buttons, ...). Often this can be expressed just by DT.
> 
> In ACPI we support DT compatible strings, and we support overlays for
> a long time. Would it work for you?

please tell me more, how ACPI and DT can already work together ?

You already know my apu board driver - that's my first example usecase.

There're few things I don't know how to solve w/ overlays:

* match rules shall be inside the DTS
* future match rules shall also check for bios versions etc
* adding new boards shall be possible by just adding another DTS to
   the tree (not a whole module)
* supporting several board variants (w/ small differences) by one DTS
* sometimes existing devices (eg. enumerated by acpi) need to be kicked
   out (buggy firmware, ...)
* can't rely on any special userland tweaks

>> The approach can be easily be extended to other kinds of composite devices,
>> eg. PCI cards or USB dongles.
> 
> What do you mean? PCI and USB are self-enumerated. What's wrong with them?

In general yes, but of course you need drivers for them. Sometimes those
devices are composites of other devices, wired up in some special way.
Traditionally, we'd need to write a special driver that just don't do
much more than instantiating other drivers.

Those things could be expressed via DTS, so we don't need to write
individual drivers anymore.

>> Yet some drawbacks of the current implementation:
>>
>>   * individual FDT's can't be modularized yet (IMHO, we don't have DMI-based
>>     modprobing anyways)
> 
> What?! https://lwn.net/Articles/233385/
> `git grep -n 'MODULE_DEVICE_TABLE(dmi'`

Shame on me, I really must have missed that all the time, thanks for the
hint.

But that has some drawbacks in my case:

* need to split the information into several places (instead of having
   all in one DTS)
* need to have one separate module board, or merge the dmi tables.

My goal is having everything that describes a board into one DTS 
(source) file.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: RFC: oftree based setup of composite board devices

[Andy Shevchenko]

On Thu, Feb 11, 2021 at 1:15 PM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
> On 10.02.21 11:30, Andy Shevchenko wrote:

> >> Use cases are boards with non-oftree firmware (ACPI, etc) where certain
> >> platform devices can't be directly enumerated via firmware. Traditionally
> >> we had to write board specific drivers that check for board identification
> >> (DMI strings, etc), then initialize the actual devices and their links
> >> (eg. gpio<->leds/buttons, ...). Often this can be expressed just by DT.
> >
> > In ACPI we support DT compatible strings, and we support overlays for
> > a long time. Would it work for you?
>
> please tell me more, how ACPI and DT can already work together ?

It's all in documentation.

https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html#device-tree-namespace-link-device-id
https://www.kernel.org/doc/html/latest/admin-guide/acpi/ssdt-overlays.html

Please, please, read documentation beforehand!

> You already know my apu board driver - that's my first example usecase.

Sorry, but I forgot about it. Can you summarize what is your use case
that really needs so intrusive and hard work?

> There're few things I don't know how to solve w/ overlays:
>
> * match rules shall be inside the DTS
> * future match rules shall also check for bios versions etc
> * adding new boards shall be possible by just adding another DTS to
>    the tree (not a whole module)
> * supporting several board variants (w/ small differences) by one DTS
> * sometimes existing devices (eg. enumerated by acpi) need to be kicked
>    out (buggy firmware, ...)
> * can't rely on any special userland tweaks

Show an example why either of the above is needed in your case and
tell what is the exact issue.

> >> The approach can be easily be extended to other kinds of composite devices,
> >> eg. PCI cards or USB dongles.
> >
> > What do you mean? PCI and USB are self-enumerated. What's wrong with them?
>
> In general yes, but of course you need drivers for them. Sometimes those
> devices are composites of other devices, wired up in some special way.
> Traditionally, we'd need to write a special driver that just don't do
> much more than instantiating other drivers.

Yes, that driver represents hardware. MFD already has some support for
composite devices. We have the auxiliary bus for some other
interesting cases, etc. Depending on the hardware in question you have
to choose a proper bus and locking (access synchronisation) schema.

> Those things could be expressed via DTS, so we don't need to write
> individual drivers anymore.

It seems you are trying to create something like "universal quirk".
Brave idea, but from my experience a fiasco is what will be out of it.
The hardware has a lot of different issues and levels of issues and it
is close to impossible to describe everything possible and predict the
future... Good luck!


...

> * need to split the information into several places (instead of having
>    all in one DTS)
> * need to have one separate module board, or merge the dmi tables.

Have no idea what you are talking about here, sorry.

> My goal is having everything that describes a board into one DTS
> (source) file.

I'm confused, you are talking about non-DT platforms in the
cover-letter and now you are talking about DTS. AFAIK DTS allows you
to put everything in one source.


-- 
With Best Regards,
Andy Shevchenko

Re: RFC: oftree based setup of composite board devices

[Enrico Weigelt, metux IT consult]

On 11.02.21 12:41, Andy Shevchenko wrote:

Hi,

> On Thu, Feb 11, 2021 at 1:15 PM Enrico Weigelt, metux IT consult
> <lkml@metux.net> wrote:
>> On 10.02.21 11:30, Andy Shevchenko wrote:
> 
>>>> Use cases are boards with non-oftree firmware (ACPI, etc) where certain
>>>> platform devices can't be directly enumerated via firmware. Traditionally
>>>> we had to write board specific drivers that check for board identification
>>>> (DMI strings, etc), then initialize the actual devices and their links
>>>> (eg. gpio<->leds/buttons, ...). Often this can be expressed just by DT.
>>>
>>> In ACPI we support DT compatible strings, and we support overlays for
>>> a long time. Would it work for you?
>>
>> please tell me more, how ACPI and DT can already work together ?
> 
> It's all in documentation.
> 
> https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html#device-tree-namespace-link-device-id

Thanks, but I'm still unsure how this helps me. I'm not intending to
touch any firmware (and expect people in the field flashing new fw).
I have to live with what we find in the field (otherwise I'd just write
omplete DTs for the corresponding boards and throw out ACPI :p)

> https://www.kernel.org/doc/html/latest/admin-guide/acpi/ssdt-overlays.html

Are you suggesting I should load SSDT overlays at runtime (from
userland ?) instead of using DT ?

> Please, please, read documentation beforehand!

I actually did read that documentation, but unfortunately it doesn't
tell me how to use additional DTs on ACPI systems.

>> You already know my apu board driver - that's my first example usecase.
> 
> Sorry, but I forgot about it. Can you summarize what is your use case
> that really needs so intrusive and hard work?

The current APU2/3/4 driver is pretty much fine (except that possibly
some more bios-version specific quirks might be needed). It basically
just instantiates a bunch of other devices (gpio, led, input, ...)
and connects them.

All of that (except for the DMI match table) already can be easily
expressed in DT, so this calls for a generalization. At that point I
tried to achieve the following goals:

* a generic mechanism for detecting boards (and later pci cards, usb
   dongles, etc) and probing the devices from the corresponding DT
* have everything that makes up an individual board in one DT(S)
* ability to blacklist (kick out) specific devices already probed via
   ACPI, so they don't conflict. (*1)

>> * match rules shall be inside the DTS
>> * future match rules shall also check for bios versions etc
>> * adding new boards shall be possible by just adding another DTS to
>>     the tree (not a whole module)
>> * supporting several board variants (w/ small differences) by one DTS
>> * sometimes existing devices (eg. enumerated by acpi) need to be kicked
>>     out (buggy firmware, ...)
>> * can't rely on any special userland tweaks
> 
> Show an example why either of the above is needed in your case and
> tell what is the exact issue.

In the specific APU case (note that my proposal is a generic mechanism
for a whole class of similar usecases), *some* bios versions already
enumerate *some* gpios, later versions already enumerate some LEDs but
different naming than the driver's, etc., etc. (at time of writing the
driver, apu bios didn't support any of that). For preventing conflicts
and consistency between all bios versions, it's IMHO better to just
remove the conflicting devices if they're enumerated by bios.

> Yes, that driver represents hardware. MFD already has some support for
> composite devices. We have the auxiliary bus for some other
> interesting cases, etc. Depending on the hardware in question you have
> to choose a proper bus and locking (access synchronisation) schema.

Yes, but similar to the apu case, I'd like to be able describe those
devices just by a DT (instead of lots of C code).

>> Those things could be expressed via DTS, so we don't need to write
>> individual drivers anymore.
> 
> It seems you are trying to create something like "universal quirk".
> Brave idea, but from my experience a fiasco is what will be out of it.
> The hardware has a lot of different issues and levels of issues and it
> is close to impossible to describe everything possible and predict the
> future... Good luck!

Dont worry, I don't try create some one-fits-all-solution. It's just for
a specific class of use cases, where we need additional devices that
can't be (reliably) enumerated via firmware or buses.

>> * need to split the information into several places (instead of having
>>     all in one DTS)
>> * need to have one separate module board, or merge the dmi tables.
> 
> Have no idea what you are talking about here, sorry.

Well, for now I have the matching criteria (DMI strings) within the DT -
don't need any additional code per board. For using the existing
mechanisms, I would need to move that out into a separate .c file,
something I'd like to avoid.

>> My goal is having everything that describes a board into one DTS
>> (source) file.
> 
> I'm confused, you are talking about non-DT platforms in the
> cover-letter and now you are talking about DTS. AFAIK DTS allows you
> to put everything in one source.

Nope, I'm using DT *in addition* to firmware data (ACPI), for things
that arent't properly described by firmware. The idea goes like this:

* walk through all available board descriptions (builtin dtbs)
   * check whether board matches critiera given in the DT
   * if match:
     * kick out blacklisted devices
     * populate devices from the DT

The idea is just not having to write lots of C code for cases like the
apu boards anymore, but reuse existing DT infrastructure for that and
also heavily reduce code size this way (for apu case, the dtb is around
2kb, while the existing driver is around 17kb)


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287