[RFC 00/10] add support for fwnode in i2c mux system and sfp

[RFC 00/10] add support for fwnode in i2c mux system and sfp

[Clément Léger]

The purpose of this work is to allow i2c muxes and adapters to be
usable with devices that are described with software_node. A solution
for this is to use the fwnode API which works with both device-tree,
ACPI and software node. In this series, functions are added to retrieve
i2c_adapter from fwnode and to create new mux adapters from fwnode.

This series is part of a larger changeset that touches multiple
subsystems. series will be sent separately for each subsystems since
the amount of modified file is quite large. The following cover letter
gives an overview of this work:

---

The LAN966X SoC can either run it's own Linux system or be plugged in
a PCIe slot as a PCIe switch. When running with a Linux system, a
device-tree description is used to describe the system. However, when
plugged in a PCIe slot (on a x86), there is no device-tree support and
the peripherals that are present must be described in some other way.

Reusing the existing drivers is of course mandatory and they should
also be able to work without device-tree description. We decided to
describe this card using software nodes and a MFD device. Indeed, the
MFD subsystem allows to describe such systems using struct mfd_cells
and mfd_add_devices(). This support also allows to attach a
software_node description which might be used by fwnode API in drivers
and subsystems.

We thought about adding CONFIG_OF to x86 and potentially describe this
card using device-tree overlays but it introduce other problems that
also seems difficult to solve (overlay loading without base
device-tree, fixup of IRQs, adresses, and so on) and CONFIG_OF is not
often enabled on x86 to say the least.

TLDR: I know the series touches a lot of different files and has big
implications, but it turns out software_nodes looks the "best" way of
achieving this goal and has the advantage of converting some subsystems
to be node agnostics, also allowing some ACPI factorization. Criticism
is of course welcome as I might have overlooked something way simpler !

---

This series introduce a number of changes in multiple subsystems to
allow registering and using devices that are described with a
software_node description attached to a mfd_cell, making them usable
with the fwnode API. It was needed to modify many subsystem where
CONFIG_OF was tightly integrated through the use of of_xlate()
functions and other of_* calls. New calls have been added to use fwnode
API and thus be usable with a wider range of nodes. Functions that are
used to get the devices (pinctrl_get, clk_get and so on) also needed
to be changed to use the fwnode API internally.

For instance, the clk framework has been modified to add a
fwnode_xlate() callback and a new named fwnode_clk_add_hw_provider()
has been added. This function will register a clock using
fwnode_xlate() callback. Note that since the fwnode API is compatible
with devices that have a of_node member set, it will still be possible
to use the driver and get the clocks with CONFIG_OF enabled
configurations.

In some subsystems, it was possible to keep OF related function by
wrapping the fwnode ones. It is not yet sure if both support
(device-tree and fwnode) should still continue to coexists. For instance
if fwnode_xlate() and of_xlate() should remain since the fwnode version
also supports device-tree. Removing of_xlate() would of course require
to modify all drivers that uses it.

Here is an excerpt of the lan966x description when used as a PCIe card.
The complete description is visible at [2]. This part only describe the
flexcom controller and the fixed-clock that is used as an input clock.

static const struct property_entry ddr_clk_props[] = {
        PROPERTY_ENTRY_U32("clock-frequency", 30000000),
        PROPERTY_ENTRY_U32("#clock-cells", 0),
        {}
};

static const struct software_node ddr_clk_node = {
        .name = "ddr_clk",
        .properties = ddr_clk_props,
};

static const struct property_entry lan966x_flexcom_props[] = {
        PROPERTY_ENTRY_U32("atmel,flexcom-mode", ATMEL_FLEXCOM_MODE_TWI),
        PROPERTY_ENTRY_REF("clocks", &ddr_clk_node),
        {}
};

static const struct software_node lan966x_flexcom_node = {
        .name = "lan966x-flexcom",
        .properties = lan966x_flexcom_props,
};

...

static struct resource lan966x_flexcom_res[] = {
        [0] = {
                .flags = IORESOURCE_MEM,
                .start = LAN966X_DEV_ADDR(FLEXCOM_0_FLEXCOM_REG),
                .end = LAN966X_DEV_ADDR(FLEXCOM_0_FLEXCOM_REG),
        },
};

...

static struct mfd_cell lan966x_pci_mfd_cells[] = {
        ...
        [LAN966X_DEV_DDR_CLK] = {
                .name = "of_fixed_clk",
                .swnode = &ddr_clk_node,
        },
        [LAN966X_DEV_FLEXCOM] = {
                .name = "atmel_flexcom",
                .num_resources = ARRAY_SIZE(lan966x_flexcom_res),
                .resources = lan966x_flexcom_res,
                .swnode = &lan966x_flexcom_node,
        },
        ...
},

And finally registered using:

ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
                           lan966x_pci_mfd_cells,
                           ARRAY_SIZE(lan966x_pci_mfd_cells), pci_base, irq_base,
                           irq_domain);

With the modifications that have been made on this tree, it is now
possible to probe such description using existing platform drivers,
providing that they have been modified a bit to retrieve properties
using fwnode API and using the fwnode_xlate() callback instead of
of_xlate().

This series has been tested on a x86 kernel build without CONFIG_OF.
Another kernel was also built with COMPILE_TEST and CONFIG_OF support
to build as most drivers as possible. It was also tested on a sparx5
arm64 with CONFIG_OF. However, it was not tested with an ACPI
description evolved enough to validate all the changes.

A branch containing all theses modifications can be seen at [1] along
with a PCIe driver [2] which describes the card using software nodes.
Modifications that are on this branch are not completely finished (ie,
subsystems modifications for fwnode have not been factorized with OF
for all of them) in absence of feedback on the beginning of this work
from the community.

[1] https://github.com/clementleger/linux/tree/fwnode_support
[2] https://github.com/clementleger/linux/blob/fwnode_support/drivers/mfd/lan966x_pci_mfd.c

Clément Léger (10):
  property: add fwnode_match_node()
  property: add fwnode_get_match_data()
  base: swnode: use fwnode_get_match_data()
  property: add a callback parameter to fwnode_property_match_string()
  property: add fwnode_property_read_string_index()
  i2c: fwnode: add fwnode_find_i2c_adapter_by_node()
  i2c: of: use fwnode_get_i2c_adapter_by_node()
  i2c: mux: pinctrl: remove CONFIG_OF dependency and use fwnode API
  i2c: mux: add support for fwnode
  net: sfp: add support for fwnode

 drivers/base/property.c             | 133 ++++++++++++++++++++++++++--
 drivers/base/swnode.c               |   1 +
 drivers/i2c/Makefile                |   1 +
 drivers/i2c/i2c-core-fwnode.c       |  40 +++++++++
 drivers/i2c/i2c-core-of.c           |  30 -------
 drivers/i2c/i2c-mux.c               |  39 ++++----
 drivers/i2c/muxes/Kconfig           |   1 -
 drivers/i2c/muxes/i2c-mux-pinctrl.c |  21 ++---
 drivers/net/phy/sfp.c               |  44 +++------
 include/linux/i2c.h                 |   7 +-
 include/linux/property.h            |   9 ++
 11 files changed, 225 insertions(+), 101 deletions(-)
 create mode 100644 drivers/i2c/i2c-core-fwnode.c

-- 
2.34.1

[RFC 01/10] property: add fwnode_match_node()

[Clément Léger]

Add a function equivalent to of_match_node() which is usable for fwnode
support. Matching is based on the compatible property and it returns
the best matches for the node according to the compatible list
ordering.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/base/property.c  | 23 +++++++++++++++++++++++
 include/linux/property.h |  3 +++
 2 files changed, 26 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index e6497f6877ee..434c2713fd99 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1158,6 +1158,29 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
 
+const struct of_device_id *fwnode_match_node(const struct fwnode_handle *fwnode,
+					     const struct of_device_id *matches)
+{
+	int index;
+	const struct of_device_id *best_match = NULL;
+	int best_index = INT_MAX;
+
+	if (!matches)
+		return NULL;
+
+	for (; matches->name[0] || matches->type[0] || matches->compatible[0]; matches++) {
+		index = fwnode_property_match_string(fwnode, "compatible",
+						     matches->compatible);
+		if (index >= 0 && index < best_index) {
+			best_match = matches;
+			best_index = index;
+		}
+	}
+
+	return best_match;
+}
+EXPORT_SYMBOL(fwnode_match_node);
+
 const void *device_get_match_data(struct device *dev)
 {
 	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
diff --git a/include/linux/property.h b/include/linux/property.h
index 7399a0b45f98..978ecf6be34e 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -446,6 +446,9 @@ static inline void *device_connection_find_match(struct device *dev,
 	return fwnode_connection_find_match(dev_fwnode(dev), con_id, data, match);
 }
 
+const struct of_device_id *fwnode_match_node(const struct fwnode_handle *fwnode,
+					     const struct of_device_id *matches);
+
 /* -------------------------------------------------------------------------- */
 /* Software fwnode support - when HW description is incomplete or missing */
 
-- 
2.34.1

[RFC 02/10] property: add fwnode_get_match_data()

[Clément Léger]

Add fwnode_get_match_data() which is meant to be used as
device_get_match_data for fwnode_operations.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/base/property.c  | 12 ++++++++++++
 include/linux/property.h |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 434c2713fd99..6ffb3ac4509c 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1181,6 +1181,18 @@ const struct of_device_id *fwnode_match_node(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL(fwnode_match_node);
 
+const void *fwnode_get_match_data(const struct fwnode_handle *fwnode,
+				  const struct device *dev)
+{
+	const struct of_device_id *match;
+
+	match = fwnode_match_node(fwnode, dev->driver->of_match_table);
+	if (!match)
+		return NULL;
+
+	return match->data;
+}
+
 const void *device_get_match_data(struct device *dev)
 {
 	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
diff --git a/include/linux/property.h b/include/linux/property.h
index 978ecf6be34e..7f727c492602 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -449,6 +449,9 @@ static inline void *device_connection_find_match(struct device *dev,
 const struct of_device_id *fwnode_match_node(const struct fwnode_handle *fwnode,
 					     const struct of_device_id *matches);
 
+const void *fwnode_get_match_data(const struct fwnode_handle *fwnode,
+				  const struct device *dev);
+
 /* -------------------------------------------------------------------------- */
 /* Software fwnode support - when HW description is incomplete or missing */
 
-- 
2.34.1

[RFC 03/10] base: swnode: use fwnode_get_match_data()

[Clément Léger]

In order to allow matching devices with software node with
device_get_match_data(), use fwnode_get_match_data() for
.device_get_match_data operation.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/base/swnode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 0a482212c7e8..783ad18f49af 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -662,6 +662,7 @@ software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 static const struct fwnode_operations software_node_ops = {
 	.get = software_node_get,
 	.put = software_node_put,
+	.device_get_match_data = fwnode_get_match_data,
 	.property_present = software_node_property_present,
 	.property_read_int_array = software_node_read_int_array,
 	.property_read_string_array = software_node_read_string_array,
-- 
2.34.1

[RFC 04/10] property: add a callback parameter to fwnode_property_match_string()

[Clément Léger]

This function will be modified to be reused for
fwnode_property_read_string_index(). In order to avoid copy/paste of
existing code, split the existing function and pass a callback that
will be executed once the string array has been retrieved.

In order to reuse this function with other actions.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/base/property.c | 50 +++++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 7 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 6ffb3ac4509c..cd1c30999fd9 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -410,10 +410,11 @@ EXPORT_SYMBOL_GPL(fwnode_property_read_string);
  * fwnode_property_match_string - find a string in an array and return index
  * @fwnode: Firmware node to get the property of
  * @propname: Name of the property holding the array
- * @string: String to look for
+ * @cb: callback to execute on the string array
+ * @data: data to be passed to the callback
  *
- * Find a given string in a string array and if it is found return the
- * index back.
+ * Execute a given callback on a string array values and returns the callback
+ * return value.
  *
  * Return: %0 if the property was found (success),
  *	   %-EINVAL if given arguments are not valid,
@@ -421,8 +422,10 @@ EXPORT_SYMBOL_GPL(fwnode_property_read_string);
  *	   %-EPROTO if the property is not an array of strings,
  *	   %-ENXIO if no suitable firmware interface is present.
  */
-int fwnode_property_match_string(const struct fwnode_handle *fwnode,
-	const char *propname, const char *string)
+static int fwnode_property_string_match(const struct fwnode_handle *fwnode,
+					const char *propname,
+					int (*cb)(const char **, int, void *),
+					void *data)
 {
 	const char **values;
 	int nval, ret;
@@ -442,13 +445,46 @@ int fwnode_property_match_string(const struct fwnode_handle *fwnode,
 	if (ret < 0)
 		goto out;
 
+	ret = cb(values, nval, data);
+out:
+	kfree(values);
+	return ret;
+}
+
+static int match_string_callback(const char **values, int nval, void *data)
+{
+	int ret;
+	const char *string = data;
+
 	ret = match_string(values, nval, string);
 	if (ret < 0)
 		ret = -ENODATA;
-out:
-	kfree(values);
+
 	return ret;
 }
+
+/**
+ * fwnode_property_match_string - find a string in an array and return index
+ * @fwnode: Firmware node to get the property of
+ * @propname: Name of the property holding the array
+ * @string: String to look for
+ *
+ * Find a given string in a string array and if it is found return the
+ * index back.
+ *
+ * Return: %0 if the property was found (success),
+ *	   %-EINVAL if given arguments are not valid,
+ *	   %-ENODATA if the property does not have a value,
+ *	   %-EPROTO if the property is not an array of strings,
+ *	   %-ENXIO if no suitable firmware interface is present.
+ */
+int fwnode_property_match_string(const struct fwnode_handle *fwnode,
+				 const char *propname, const char *string)
+{
+	return fwnode_property_string_match(fwnode, propname,
+					    match_string_callback,
+					    (void *)string);
+}
 EXPORT_SYMBOL_GPL(fwnode_property_match_string);
 
 /**
-- 
2.34.1

[RFC 05/10] property: add fwnode_property_read_string_index()

[Clément Léger]

Add fwnode_property_read_string_index() function which allows to
retrieve a string from an array by its index. This function is the
equivalent of of_property_read_string_index() but for fwnode support.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/base/property.c  | 48 ++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  3 +++
 2 files changed, 51 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index cd1c30999fd9..00d9f171329c 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -487,6 +487,54 @@ int fwnode_property_match_string(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_property_match_string);
 
+struct read_index_data {
+	const char **string;
+	int index;
+};
+
+static int read_string_index(const char **values, int nval, void *data)
+{
+	struct read_index_data *cb_data = data;
+
+	if (cb_data->index >= nval)
+		return -EINVAL;
+
+	*cb_data->string = values[cb_data->index];
+
+	return 0;
+}
+
+/**
+ * fwnode_property_read_string_index - read a string in an array using an index
+ * and return a pointer to the string
+ * @fwnode: Firmware node to get the property of
+ * @propname: Name of the property holding the array
+ * @index: Index of the string to look for
+ * @string: Pointer to the string if found
+ *
+ * Find a string by a given index in a string array and if it is found return
+ * the string value in @string.
+ *
+ * Return: %0 if the property was found (success),
+ *	   %-EINVAL if given arguments are not valid,
+ *	   %-ENODATA if the property does not have a value,
+ *	   %-EPROTO if the property is not an array of strings,
+ *	   %-ENXIO if no suitable firmware interface is present.
+ */
+int fwnode_property_read_string_index(const struct fwnode_handle *fwnode,
+				      const char *propname, int index,
+				      const char **string)
+{
+	struct read_index_data cb_data;
+
+	cb_data.index = index;
+	cb_data.string = string;
+
+	return fwnode_property_string_match(fwnode, propname, read_string_index,
+					    &cb_data);
+}
+EXPORT_SYMBOL_GPL(fwnode_property_read_string_index);
+
 /**
  * fwnode_property_get_reference_args() - Find a reference with arguments
  * @fwnode:	Firmware node where to look for the reference
diff --git a/include/linux/property.h b/include/linux/property.h
index 7f727c492602..f6ede3840c40 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -70,6 +70,9 @@ int fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
 				      size_t nval);
 int fwnode_property_read_string(const struct fwnode_handle *fwnode,
 				const char *propname, const char **val);
+int fwnode_property_read_string_index(const struct fwnode_handle *fwnode,
+				      const char *propname, int index,
+				      const char **string);
 int fwnode_property_match_string(const struct fwnode_handle *fwnode,
 				 const char *propname, const char *string);
 int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
-- 
2.34.1

[RFC 06/10] i2c: fwnode: add fwnode_find_i2c_adapter_by_node()

[Clément Léger]

Add fwnode_find_i2c_adapter_by_node() which allows to retrieve a i2c
adapter using a fwnode. Since dev_fwnode() uses the fwnode provided by
the of_node member of the device, this will also work for devices were
the of_node has been set and not the fwnode field.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/i2c/Makefile          |  1 +
 drivers/i2c/i2c-core-fwnode.c | 40 +++++++++++++++++++++++++++++++++++
 include/linux/i2c.h           |  2 ++
 3 files changed, 43 insertions(+)
 create mode 100644 drivers/i2c/i2c-core-fwnode.c

diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index c1d493dc9bac..c9c97675163e 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -6,6 +6,7 @@
 obj-$(CONFIG_I2C_BOARDINFO)	+= i2c-boardinfo.o
 obj-$(CONFIG_I2C)		+= i2c-core.o
 i2c-core-objs 			:= i2c-core-base.o i2c-core-smbus.o
+i2c-core-objs			+= i2c-core-fwnode.o
 i2c-core-$(CONFIG_ACPI)		+= i2c-core-acpi.o
 i2c-core-$(CONFIG_I2C_SLAVE) 	+= i2c-core-slave.o
 i2c-core-$(CONFIG_OF) 		+= i2c-core-of.o
diff --git a/drivers/i2c/i2c-core-fwnode.c b/drivers/i2c/i2c-core-fwnode.c
new file mode 100644
index 000000000000..d116aca3e2ec
--- /dev/null
+++ b/drivers/i2c/i2c-core-fwnode.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Linux I2C core fwnode support code
+ *
+ * Copyright (C) 2022 Microchip
+ */
+
+#include <linux/device.h>
+#include <linux/i2c.h>
+
+#include "i2c-core.h"
+
+static int fwnode_dev_or_parent_node_match(struct device *dev, const void *data)
+{
+	if (dev_fwnode(dev) == data)
+		return 1;
+
+	if (dev->parent)
+		return dev_fwnode(dev->parent) == data;
+
+	return 0;
+}
+
+struct i2c_adapter *fwnode_find_i2c_adapter_by_node(struct fwnode_handle *node)
+{
+	struct device *dev;
+	struct i2c_adapter *adapter;
+
+	dev = bus_find_device(&i2c_bus_type, NULL, node,
+			      fwnode_dev_or_parent_node_match);
+	if (!dev)
+		return NULL;
+
+	adapter = i2c_verify_adapter(dev);
+	if (!adapter)
+		put_device(dev);
+
+	return adapter;
+}
+EXPORT_SYMBOL(fwnode_find_i2c_adapter_by_node);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 7d4f52ceb7b5..9b480a8b0a76 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -967,6 +967,8 @@ int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr);
 
 #endif /* I2C */
 
+struct i2c_adapter *fwnode_find_i2c_adapter_by_node(struct fwnode_handle *node);
+
 #if IS_ENABLED(CONFIG_OF)
 /* must call put_device() when done with returned i2c_client device */
 struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
-- 
2.34.1

[RFC 07/10] i2c: of: use fwnode_get_i2c_adapter_by_node()

[Clément Léger]

Since the new fwnode function does the same check that was done by
of_get_i2c_adapter_by_node(), call this one to avoid code duplication.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/i2c/i2c-core-of.c | 30 ------------------------------
 include/linux/i2c.h       |  5 ++++-
 2 files changed, 4 insertions(+), 31 deletions(-)

diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index 3ed74aa4b44b..be7d66aa0f49 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -113,17 +113,6 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
 	of_node_put(bus);
 }
 
-static int of_dev_or_parent_node_match(struct device *dev, const void *data)
-{
-	if (dev->of_node == data)
-		return 1;
-
-	if (dev->parent)
-		return dev->parent->of_node == data;
-
-	return 0;
-}
-
 /* must call put_device() when done with returned i2c_client device */
 struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
 {
@@ -142,25 +131,6 @@ struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
 }
 EXPORT_SYMBOL(of_find_i2c_device_by_node);
 
-/* must call put_device() when done with returned i2c_adapter device */
-struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
-{
-	struct device *dev;
-	struct i2c_adapter *adapter;
-
-	dev = bus_find_device(&i2c_bus_type, NULL, node,
-			      of_dev_or_parent_node_match);
-	if (!dev)
-		return NULL;
-
-	adapter = i2c_verify_adapter(dev);
-	if (!adapter)
-		put_device(dev);
-
-	return adapter;
-}
-EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
-
 /* must call i2c_put_adapter() when done with returned i2c_adapter device */
 struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node)
 {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 9b480a8b0a76..d1f384b805ad 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -974,7 +974,10 @@ struct i2c_adapter *fwnode_find_i2c_adapter_by_node(struct fwnode_handle *node);
 struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
 
 /* must call put_device() when done with returned i2c_adapter device */
-struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node);
+static inline struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
+{
+	return fwnode_find_i2c_adapter_by_node(of_fwnode_handle(node));
+}
 
 /* must call i2c_put_adapter() when done with returned i2c_adapter device */
 struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node);
-- 
2.34.1

[RFC 08/10] i2c: mux: pinctrl: remove CONFIG_OF dependency and use fwnode API

[Clément Léger]

In order to use i2c muxes with software_node when added with a struct
mfd_cell, switch to fwnode API. The fwnode layer will allow to use this
with both device_node and software_node.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/i2c/muxes/Kconfig           |  1 -
 drivers/i2c/muxes/i2c-mux-pinctrl.c | 21 +++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 1708b1a82da2..d9cb15cfba3e 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -77,7 +77,6 @@ config I2C_MUX_PCA954x
 config I2C_MUX_PINCTRL
 	tristate "pinctrl-based I2C multiplexer"
 	depends on PINCTRL
-	depends on OF || COMPILE_TEST
 	help
 	  If you say yes to this option, support will be included for an I2C
 	  multiplexer that uses the pinctrl subsystem, i.e. pin multiplexing.
diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
index f1bb00a11ad6..200890d7a625 100644
--- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
+++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
@@ -53,19 +53,20 @@ static struct i2c_adapter *i2c_mux_pinctrl_root_adapter(
 
 static struct i2c_adapter *i2c_mux_pinctrl_parent_adapter(struct device *dev)
 {
-	struct device_node *np = dev->of_node;
-	struct device_node *parent_np;
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+	struct fwnode_handle *parent_np;
 	struct i2c_adapter *parent;
 
-	parent_np = of_parse_phandle(np, "i2c-parent", 0);
+	parent_np = fwnode_find_reference(fwnode, "i2c-parent", 0);
 	if (!parent_np) {
 		dev_err(dev, "Cannot parse i2c-parent\n");
 		return ERR_PTR(-ENODEV);
 	}
-	parent = of_find_i2c_adapter_by_node(parent_np);
-	of_node_put(parent_np);
-	if (!parent)
+	parent = fwnode_find_i2c_adapter_by_node(parent_np);
+	if (!parent) {
+		dev_err(dev, "Cannot find i2c-parent\n");
 		return ERR_PTR(-EPROBE_DEFER);
+	}
 
 	return parent;
 }
@@ -73,7 +74,7 @@ static struct i2c_adapter *i2c_mux_pinctrl_parent_adapter(struct device *dev)
 static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
+	struct fwnode_handle *np = dev_fwnode(dev);
 	struct i2c_mux_core *muxc;
 	struct i2c_mux_pinctrl *mux;
 	struct i2c_adapter *parent;
@@ -81,7 +82,7 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
 	int num_names, i, ret;
 	const char *name;
 
-	num_names = of_property_count_strings(np, "pinctrl-names");
+	num_names = fwnode_property_string_array_count(np, "pinctrl-names");
 	if (num_names < 0) {
 		dev_err(dev, "Cannot parse pinctrl-names: %d\n",
 			num_names);
@@ -111,8 +112,8 @@ static int i2c_mux_pinctrl_probe(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < num_names; i++) {
-		ret = of_property_read_string_index(np, "pinctrl-names", i,
-						    &name);
+		ret = fwnode_property_read_string_index(np, "pinctrl-names", i,
+							&name);
 		if (ret < 0) {
 			dev_err(dev, "Cannot parse pinctrl-names: %d\n", ret);
 			goto err_put_parent;
-- 
2.34.1

[RFC 09/10] i2c: mux: add support for fwnode

[Clément Léger]

Modify i2c_mux_add_adapter() to use with fwnode API to allow creating
mux adapters with fwnode based devices. This allows to have a node
independent support for i2c muxes.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/i2c/i2c-mux.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 774507b54b57..93c916220da5 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -24,7 +24,7 @@
 #include <linux/i2c-mux.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/of.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
 
@@ -347,38 +347,35 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 	else
 		priv->adap.class = class;
 
-	/*
-	 * Try to populate the mux adapter's of_node, expands to
-	 * nothing if !CONFIG_OF.
-	 */
-	if (muxc->dev->of_node) {
-		struct device_node *dev_node = muxc->dev->of_node;
-		struct device_node *mux_node, *child = NULL;
+	/* Try to populate the mux adapter's device node */
+	if (dev_fwnode(muxc->dev) && !has_acpi_companion(muxc->dev)) {
+		struct fwnode_handle *dev_node = dev_fwnode(muxc->dev);
+		struct fwnode_handle *mux_node, *child = NULL;
 		u32 reg;
 
 		if (muxc->arbitrator)
-			mux_node = of_get_child_by_name(dev_node, "i2c-arb");
+			mux_node = fwnode_get_named_child_node(dev_node, "i2c-arb");
 		else if (muxc->gate)
-			mux_node = of_get_child_by_name(dev_node, "i2c-gate");
+			mux_node = fwnode_get_named_child_node(dev_node, "i2c-gate");
 		else
-			mux_node = of_get_child_by_name(dev_node, "i2c-mux");
+			mux_node = fwnode_get_named_child_node(dev_node, "i2c-mux");
 
 		if (mux_node) {
 			/* A "reg" property indicates an old-style DT entry */
-			if (!of_property_read_u32(mux_node, "reg", &reg)) {
-				of_node_put(mux_node);
+			if (!fwnode_property_read_u32(mux_node, "reg", &reg)) {
+				fwnode_handle_put(mux_node);
 				mux_node = NULL;
 			}
 		}
 
 		if (!mux_node)
-			mux_node = of_node_get(dev_node);
+			mux_node = fwnode_handle_get(dev_node);
 		else if (muxc->arbitrator || muxc->gate)
-			child = of_node_get(mux_node);
+			child = fwnode_handle_get(mux_node);
 
 		if (!child) {
-			for_each_child_of_node(mux_node, child) {
-				ret = of_property_read_u32(child, "reg", &reg);
+			fwnode_for_each_child_node(mux_node, child) {
+				ret = fwnode_property_read_u32(child, "reg", &reg);
 				if (ret)
 					continue;
 				if (chan_id == reg)
@@ -386,8 +383,8 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 			}
 		}
 
-		priv->adap.dev.of_node = child;
-		of_node_put(mux_node);
+		device_set_node(&priv->adap.dev, child);
+		fwnode_handle_put(mux_node);
 	}
 
 	/*
@@ -444,7 +441,7 @@ void i2c_mux_del_adapters(struct i2c_mux_core *muxc)
 	while (muxc->num_adapters) {
 		struct i2c_adapter *adap = muxc->adapter[--muxc->num_adapters];
 		struct i2c_mux_priv *priv = adap->algo_data;
-		struct device_node *np = adap->dev.of_node;
+		struct fwnode_handle *np = dev_fwnode(&adap->dev);
 
 		muxc->adapter[muxc->num_adapters] = NULL;
 
@@ -454,7 +451,7 @@ void i2c_mux_del_adapters(struct i2c_mux_core *muxc)
 
 		sysfs_remove_link(&priv->adap.dev.kobj, "mux_device");
 		i2c_del_adapter(adap);
-		of_node_put(np);
+		fwnode_handle_put(np);
 		kfree(priv);
 	}
 }
-- 
2.34.1

[RFC 10/10] net: sfp: add support for fwnode

[Clément Léger]

Add support to retrieve a i2c bus in sfp with a fwnode. This support
is using the fwnode API which also works with device-tree and ACPI.
For this purpose, the device-tree and ACPI code handling the i2c
adapter retrieval was factorized with the new code. This also allows
i2c devices using a software_node description to be used by sfp code.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/net/phy/sfp.c | 44 +++++++++++++------------------------------
 1 file changed, 13 insertions(+), 31 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 4720b24ca51b..9d9e3d209408 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -2499,43 +2499,25 @@ static int sfp_probe(struct platform_device *pdev)
 		return err;
 
 	sff = sfp->type = &sfp_data;
+	if (dev_fwnode(&pdev->dev)) {
+		struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
+		struct fwnode_handle *np;
 
-	if (pdev->dev.of_node) {
-		struct device_node *node = pdev->dev.of_node;
-		const struct of_device_id *id;
-		struct device_node *np;
+		if (!is_acpi_device_node(fwnode)) {
+			sff = device_get_match_data(&pdev->dev);
+			if (WARN_ON(!sff))
+				return -EINVAL;
 
-		id = of_match_node(sfp_of_match, node);
-		if (WARN_ON(!id))
-			return -EINVAL;
-
-		sff = sfp->type = id->data;
-
-		np = of_parse_phandle(node, "i2c-bus", 0);
-		if (!np) {
-			dev_err(sfp->dev, "missing 'i2c-bus' property\n");
-			return -ENODEV;
+			sfp->type = sff;
 		}
 
-		i2c = of_find_i2c_adapter_by_node(np);
-		of_node_put(np);
-	} else if (has_acpi_companion(&pdev->dev)) {
-		struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
-		struct fwnode_handle *fw = acpi_fwnode_handle(adev);
-		struct fwnode_reference_args args;
-		struct acpi_handle *acpi_handle;
-		int ret;
-
-		ret = acpi_node_get_property_reference(fw, "i2c-bus", 0, &args);
-		if (ret || !is_acpi_device_node(args.fwnode)) {
-			dev_err(&pdev->dev, "missing 'i2c-bus' property\n");
+		np = fwnode_find_reference(fwnode, "i2c-bus", 0);
+		if (!np) {
+			dev_err(&pdev->dev, "Cannot parse i2c-bus\n");
 			return -ENODEV;
 		}
-
-		acpi_handle = ACPI_HANDLE_FWNODE(args.fwnode);
-		i2c = i2c_acpi_find_adapter_by_handle(acpi_handle);
-	} else {
-		return -EINVAL;
+		i2c = fwnode_find_i2c_adapter_by_node(np);
+		fwnode_handle_put(np);
 	}
 
 	if (!i2c)
-- 
2.34.1

Re: [RFC 10/10] net: sfp: add support for fwnode

[Russell King (Oracle)]

On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
> Add support to retrieve a i2c bus in sfp with a fwnode. This support
> is using the fwnode API which also works with device-tree and ACPI.
> For this purpose, the device-tree and ACPI code handling the i2c
> adapter retrieval was factorized with the new code. This also allows
> i2c devices using a software_node description to be used by sfp code.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>

I think this looks fine.

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Andy Shevchenko]

On Mon, Feb 21, 2022 at 05:26:42PM +0100, Clément Léger wrote:
> The purpose of this work is to allow i2c muxes and adapters to be
> usable with devices that are described with software_node. A solution
> for this is to use the fwnode API which works with both device-tree,
> ACPI and software node. In this series, functions are added to retrieve
> i2c_adapter from fwnode and to create new mux adapters from fwnode.
> 
> This series is part of a larger changeset that touches multiple
> subsystems. series will be sent separately for each subsystems since
> the amount of modified file is quite large. The following cover letter
> gives an overview of this work:
> 
> ---
> 
> The LAN966X SoC can either run it's own Linux system or be plugged in
> a PCIe slot as a PCIe switch. When running with a Linux system, a
> device-tree description is used to describe the system. However, when
> plugged in a PCIe slot (on a x86), there is no device-tree support and
> the peripherals that are present must be described in some other way.
> 
> Reusing the existing drivers is of course mandatory and they should
> also be able to work without device-tree description. We decided to
> describe this card using software nodes and a MFD device. Indeed, the
> MFD subsystem allows to describe such systems using struct mfd_cells
> and mfd_add_devices(). This support also allows to attach a
> software_node description which might be used by fwnode API in drivers
> and subsystems.
> 
> We thought about adding CONFIG_OF to x86 and potentially describe this
> card using device-tree overlays but it introduce other problems that
> also seems difficult to solve (overlay loading without base
> device-tree, fixup of IRQs, adresses, and so on) and CONFIG_OF is not
> often enabled on x86 to say the least.

Why it can't be described by SSDT overlay (if the x86 platform in question is
ACPI based)?

> TLDR: I know the series touches a lot of different files and has big
> implications, but it turns out software_nodes looks the "best" way of
> achieving this goal and has the advantage of converting some subsystems
> to be node agnostics, also allowing some ACPI factorization. Criticism
> is of course welcome as I might have overlooked something way simpler !
> 
> ---
> 
> This series introduce a number of changes in multiple subsystems to
> allow registering and using devices that are described with a
> software_node description attached to a mfd_cell, making them usable
> with the fwnode API. It was needed to modify many subsystem where
> CONFIG_OF was tightly integrated through the use of of_xlate()
> functions and other of_* calls. New calls have been added to use fwnode
> API and thus be usable with a wider range of nodes. Functions that are
> used to get the devices (pinctrl_get, clk_get and so on) also needed
> to be changed to use the fwnode API internally.
> 
> For instance, the clk framework has been modified to add a
> fwnode_xlate() callback and a new named fwnode_clk_add_hw_provider()
> has been added. This function will register a clock using
> fwnode_xlate() callback. Note that since the fwnode API is compatible
> with devices that have a of_node member set, it will still be possible
> to use the driver and get the clocks with CONFIG_OF enabled
> configurations.

How does this all is compatible with ACPI approaches?
I mean we usually do not reintroduce 1:1 DT schemas in ACPI.

I think the CCF should be converted to use fwnode APIs and meanwhile
we may discuss how to deal with clocks on ACPI platforms, because
it may be a part of the power management methods.

> In some subsystems, it was possible to keep OF related function by
> wrapping the fwnode ones. It is not yet sure if both support
> (device-tree and fwnode) should still continue to coexists. For instance
> if fwnode_xlate() and of_xlate() should remain since the fwnode version
> also supports device-tree. Removing of_xlate() would of course require
> to modify all drivers that uses it.
> 
> Here is an excerpt of the lan966x description when used as a PCIe card.
> The complete description is visible at [2]. This part only describe the
> flexcom controller and the fixed-clock that is used as an input clock.
> 
> static const struct property_entry ddr_clk_props[] = {
>         PROPERTY_ENTRY_U32("clock-frequency", 30000000),

>         PROPERTY_ENTRY_U32("#clock-cells", 0),

Why this is used?

>         {}
> };
> 
> static const struct software_node ddr_clk_node = {
>         .name = "ddr_clk",
>         .properties = ddr_clk_props,
> };
> 
> static const struct property_entry lan966x_flexcom_props[] = {
>         PROPERTY_ENTRY_U32("atmel,flexcom-mode", ATMEL_FLEXCOM_MODE_TWI),
>         PROPERTY_ENTRY_REF("clocks", &ddr_clk_node),
>         {}
> };
> 
> static const struct software_node lan966x_flexcom_node = {
>         .name = "lan966x-flexcom",
>         .properties = lan966x_flexcom_props,
> };
> 
> ...
> 
> static struct resource lan966x_flexcom_res[] = {
>         [0] = {
>                 .flags = IORESOURCE_MEM,
>                 .start = LAN966X_DEV_ADDR(FLEXCOM_0_FLEXCOM_REG),
>                 .end = LAN966X_DEV_ADDR(FLEXCOM_0_FLEXCOM_REG),
>         },
> };
> 
> ...
> 
> static struct mfd_cell lan966x_pci_mfd_cells[] = {
>         ...
>         [LAN966X_DEV_DDR_CLK] = {
>                 .name = "of_fixed_clk",
>                 .swnode = &ddr_clk_node,
>         },
>         [LAN966X_DEV_FLEXCOM] = {
>                 .name = "atmel_flexcom",
>                 .num_resources = ARRAY_SIZE(lan966x_flexcom_res),
>                 .resources = lan966x_flexcom_res,
>                 .swnode = &lan966x_flexcom_node,
>         },
>         ...
> },
> 
> And finally registered using:
> 
> ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
>                            lan966x_pci_mfd_cells,
>                            ARRAY_SIZE(lan966x_pci_mfd_cells), pci_base, irq_base,
>                            irq_domain);
> 
> With the modifications that have been made on this tree, it is now
> possible to probe such description using existing platform drivers,
> providing that they have been modified a bit to retrieve properties
> using fwnode API and using the fwnode_xlate() callback instead of
> of_xlate().
> 
> This series has been tested on a x86 kernel build without CONFIG_OF.
> Another kernel was also built with COMPILE_TEST and CONFIG_OF support
> to build as most drivers as possible. It was also tested on a sparx5
> arm64 with CONFIG_OF. However, it was not tested with an ACPI
> description evolved enough to validate all the changes.
> 
> A branch containing all theses modifications can be seen at [1] along
> with a PCIe driver [2] which describes the card using software nodes.
> Modifications that are on this branch are not completely finished (ie,
> subsystems modifications for fwnode have not been factorized with OF
> for all of them) in absence of feedback on the beginning of this work
> from the community.
> 
> [1] https://github.com/clementleger/linux/tree/fwnode_support
> [2] https://github.com/clementleger/linux/blob/fwnode_support/drivers/mfd/lan966x_pci_mfd.c
> 
> Clément Léger (10):
>   property: add fwnode_match_node()
>   property: add fwnode_get_match_data()
>   base: swnode: use fwnode_get_match_data()
>   property: add a callback parameter to fwnode_property_match_string()
>   property: add fwnode_property_read_string_index()
>   i2c: fwnode: add fwnode_find_i2c_adapter_by_node()
>   i2c: of: use fwnode_get_i2c_adapter_by_node()
>   i2c: mux: pinctrl: remove CONFIG_OF dependency and use fwnode API
>   i2c: mux: add support for fwnode
>   net: sfp: add support for fwnode
> 
>  drivers/base/property.c             | 133 ++++++++++++++++++++++++++--
>  drivers/base/swnode.c               |   1 +
>  drivers/i2c/Makefile                |   1 +
>  drivers/i2c/i2c-core-fwnode.c       |  40 +++++++++
>  drivers/i2c/i2c-core-of.c           |  30 -------
>  drivers/i2c/i2c-mux.c               |  39 ++++----
>  drivers/i2c/muxes/Kconfig           |   1 -
>  drivers/i2c/muxes/i2c-mux-pinctrl.c |  21 ++---
>  drivers/net/phy/sfp.c               |  44 +++------
>  include/linux/i2c.h                 |   7 +-
>  include/linux/property.h            |   9 ++
>  11 files changed, 225 insertions(+), 101 deletions(-)
>  create mode 100644 drivers/i2c/i2c-core-fwnode.c
> 
> -- 
> 2.34.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 01/10] property: add fwnode_match_node()

[Andy Shevchenko]

On Mon, Feb 21, 2022 at 05:26:43PM +0100, Clément Léger wrote:
> Add a function equivalent to of_match_node() which is usable for fwnode
> support. Matching is based on the compatible property and it returns
> the best matches for the node according to the compatible list
> ordering.

Not sure I understand the purpose of this API.
We have device_get_match_data(), maybe you want similar for fwnode?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 02/10] property: add fwnode_get_match_data()

[Andy Shevchenko]

On Mon, Feb 21, 2022 at 05:26:44PM +0100, Clément Léger wrote:
> Add fwnode_get_match_data() which is meant to be used as
> device_get_match_data for fwnode_operations.

...

> +const void *fwnode_get_match_data(const struct fwnode_handle *fwnode,
> +				  const struct device *dev)
> +{
> +	const struct of_device_id *match;
> +
> +	match = fwnode_match_node(fwnode, dev->driver->of_match_table);
> +	if (!match)
> +		return NULL;
> +
> +	return match->data;
> +}

It's OF-centric API, why it has fwnode prefix? Can it leave in drivers/of instead?


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 03/10] base: swnode: use fwnode_get_match_data()

[Andy Shevchenko]

On Mon, Feb 21, 2022 at 05:26:45PM +0100, Clément Léger wrote:
> In order to allow matching devices with software node with
> device_get_match_data(), use fwnode_get_match_data() for
> .device_get_match_data operation.

...

> +	.device_get_match_data = fwnode_get_match_data,

Huh? It should be other way around, no?
I mean that each of the resource providers may (or may not) provide a method
for the specific fwnode abstraction.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 04/10] property: add a callback parameter to fwnode_property_match_string()

[Andy Shevchenko]

On Mon, Feb 21, 2022 at 05:26:46PM +0100, Clément Léger wrote:
> This function will be modified to be reused for
> fwnode_property_read_string_index(). In order to avoid copy/paste of
> existing code, split the existing function and pass a callback that
> will be executed once the string array has been retrieved.
> 
> In order to reuse this function with other actions.

...

> +int fwnode_property_match_string(const struct fwnode_handle *fwnode,
> +				 const char *propname, const char *string)
> +{
> +	return fwnode_property_string_match(fwnode, propname,
> +					    match_string_callback,

> +					    (void *)string);

We want to keep const qualifier.

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 09/10] i2c: mux: add support for fwnode

[Andy Shevchenko]

On Mon, Feb 21, 2022 at 05:26:51PM +0100, Clément Léger wrote:
> Modify i2c_mux_add_adapter() to use with fwnode API to allow creating
> mux adapters with fwnode based devices. This allows to have a node
> independent support for i2c muxes.

I^2C muxes have their own description for DT and ACPI platforms, I'm not sure
swnode should be used here at all. Just upload a corresponding SSDT overlay or
DT overlay depending on the platform. Can it be achieved?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 10/10] net: sfp: add support for fwnode

[Andy Shevchenko]

On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
> Add support to retrieve a i2c bus in sfp with a fwnode. This support
> is using the fwnode API which also works with device-tree and ACPI.
> For this purpose, the device-tree and ACPI code handling the i2c
> adapter retrieval was factorized with the new code. This also allows
> i2c devices using a software_node description to be used by sfp code.

If I'm not mistaken this patch can even go separately right now, since all used
APIs are already available.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 06/10] i2c: fwnode: add fwnode_find_i2c_adapter_by_node()

[Andy Shevchenko]

On Mon, Feb 21, 2022 at 05:26:48PM +0100, Clément Léger wrote:
> Add fwnode_find_i2c_adapter_by_node() which allows to retrieve a i2c
> adapter using a fwnode. Since dev_fwnode() uses the fwnode provided by
> the of_node member of the device, this will also work for devices were
> the of_node has been set and not the fwnode field.

...

> +static int fwnode_dev_or_parent_node_match(struct device *dev, const void *data)
> +{

> +	if (dev_fwnode(dev) == data)
> +		return 1;

This can use corresponding match function from bus.h.

> +	if (dev->parent)
> +		return dev_fwnode(dev->parent) == data;
> +
> +	return 0;

The same.

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Andrew Lunn]

> This series has been tested on a x86 kernel build without CONFIG_OF.
> Another kernel was also built with COMPILE_TEST and CONFIG_OF support
> to build as most drivers as possible. It was also tested on a sparx5
> arm64 with CONFIG_OF. However, it was not tested with an ACPI
> description evolved enough to validate all the changes.

By that, do you mean a DSD description?

In the DT world, we avoid snow flakes. Once you define a binding, it
is expected every following board will use it. So what i believe you
are doing here is defining how i2c muxes are described in APCI. How
SFP devices are described in ACPI. Until the ACPI standards committee
says otherwise, this is it. So you need to clearly document
this. Please add to Documentation/firmware-guide/acpi/dsd.

      Andrew

Re: [RFC 01/10] property: add fwnode_match_node()

[Clément Léger]

Le Mon, 21 Feb 2022 19:44:52 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> On Mon, Feb 21, 2022 at 05:26:43PM +0100, Clément Léger wrote:
> > Add a function equivalent to of_match_node() which is usable for
> > fwnode support. Matching is based on the compatible property and it
> > returns the best matches for the node according to the compatible
> > list ordering.  
> 
> Not sure I understand the purpose of this API.
> We have device_get_match_data(), maybe you want similar for fwnode?
> 

Hi Andy,

Actually device_get_match_data() is calling the .device_get_match_data
callback of the dev fwnode. This function is meant to be used by the
next patch (fwnode_get_match_data()) to be used as a generic fwnode
operation and thus be usable with software_node.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 02/10] property: add fwnode_get_match_data()

[Clément Léger]

Le Mon, 21 Feb 2022 19:46:12 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> On Mon, Feb 21, 2022 at 05:26:44PM +0100, Clément Léger wrote:
> > Add fwnode_get_match_data() which is meant to be used as
> > device_get_match_data for fwnode_operations.  
> 
> ...
> 
> > +const void *fwnode_get_match_data(const struct fwnode_handle *fwnode,
> > +				  const struct device *dev)
> > +{
> > +	const struct of_device_id *match;
> > +
> > +	match = fwnode_match_node(fwnode, dev->driver->of_match_table);
> > +	if (!match)
> > +		return NULL;
> > +
> > +	return match->data;
> > +}  
> 
> It's OF-centric API, why it has fwnode prefix? Can it leave in drivers/of instead?
> 
> 

The idea is to allow device with a software_node description to match
with the content of the of_match_table. Without this, we would need a
new type of match table that would probably duplicates part of the
of_match_table to be able to match software_node against a driver.
I did not found an other way to do it without modifying drivers
individually to support software_nodes.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Andy Shevchenko]

On Tue, Feb 22, 2022 at 5:57 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > This series has been tested on a x86 kernel build without CONFIG_OF.
> > Another kernel was also built with COMPILE_TEST and CONFIG_OF support
> > to build as most drivers as possible. It was also tested on a sparx5
> > arm64 with CONFIG_OF. However, it was not tested with an ACPI
> > description evolved enough to validate all the changes.
>
> By that, do you mean a DSD description?
>
> In the DT world, we avoid snow flakes. Once you define a binding, it
> is expected every following board will use it. So what i believe you
> are doing here is defining how i2c muxes are described in APCI.

Linux kernel has already established description of I2C muxes in ACPI:
https://www.kernel.org/doc/html/latest/firmware-guide/acpi/i2c-muxes.html

I'm not sure we want another one.

> How
> SFP devices are described in ACPI. Until the ACPI standards committee
> says otherwise, this is it. So you need to clearly document
> this. Please add to Documentation/firmware-guide/acpi/dsd.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 02/10] property: add fwnode_get_match_data()

[Andy Shevchenko]

On Tue, Feb 22, 2022 at 9:24 AM Clément Léger <clement.leger@bootlin.com> wrote:
> Le Mon, 21 Feb 2022 19:46:12 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> > On Mon, Feb 21, 2022 at 05:26:44PM +0100, Clément Léger wrote:

...

> > It's OF-centric API, why it has fwnode prefix? Can it leave in drivers/of instead?
>
> The idea is to allow device with a software_node description to match
> with the content of the of_match_table. Without this, we would need a
> new type of match table that would probably duplicates part of the
> of_match_table to be able to match software_node against a driver.
> I did not found an other way to do it without modifying drivers
> individually to support software_nodes.

software nodes should not be used as a replacement of the real
firmware nodes. The idea behind is to fill the gaps in the cases when
firmware doesn't provide enough information to the OS. I think Heikki
can confirm or correct me.

If you want to use the device on an ACPI based platform, you need to
describe it in ACPI as much as possible. The rest we may discuss.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 03/10] base: swnode: use fwnode_get_match_data()

[Clément Léger]

Le Mon, 21 Feb 2022 19:48:00 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> On Mon, Feb 21, 2022 at 05:26:45PM +0100, Clément Léger wrote:
> > In order to allow matching devices with software node with
> > device_get_match_data(), use fwnode_get_match_data() for
> > .device_get_match_data operation.  
> 
> ...
> 
> > +	.device_get_match_data = fwnode_get_match_data,  
> 
> Huh? It should be other way around, no?
> I mean that each of the resource providers may (or may not) provide a
> method for the specific fwnode abstraction.
> 

Indeed, it should be the other way. But since this function is generic
and uses only fwnode API I guessed it would be more convenient to
define it in the fwnode generic part and use it for specific
implementation. I could have modified device_get_match_data to call it
if there was no .device_get_match_data operation like this:

const void *device_get_match_data(struct device *dev)
{
	if (!fwnode_has_op(fwnode, device_get_match_data)
		return fwnode_get_match_data(dev);
	return fwnode_call_ptr_op(dev_fwnode(dev),device_get_match_data, dev);
}

But I thought it was more convenient to do it by setting the
.device_get_match_data field of software_node operations.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 02/10] property: add fwnode_get_match_data()

[Clément Léger]

Le Tue, 22 Feb 2022 09:33:32 +0100,
Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :

> On Tue, Feb 22, 2022 at 9:24 AM Clément Léger <clement.leger@bootlin.com> wrote:
> > Le Mon, 21 Feb 2022 19:46:12 +0200,
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :  
> > > On Mon, Feb 21, 2022 at 05:26:44PM +0100, Clément Léger wrote:  
> 
> ...
> 
> > > It's OF-centric API, why it has fwnode prefix? Can it leave in drivers/of instead?  
> >
> > The idea is to allow device with a software_node description to match
> > with the content of the of_match_table. Without this, we would need a
> > new type of match table that would probably duplicates part of the
> > of_match_table to be able to match software_node against a driver.
> > I did not found an other way to do it without modifying drivers
> > individually to support software_nodes.  
> 
> software nodes should not be used as a replacement of the real
> firmware nodes. The idea behind is to fill the gaps in the cases when
> firmware doesn't provide enough information to the OS. I think Heikki
> can confirm or correct me.

Yes, the documentation states that:

NOTE! The primary hardware description should always come from either
ACPI tables or DT. Describing an entire system with software nodes,
though possible, is not acceptable! The software nodes should only
complement the primary hardware description.

> 
> If you want to use the device on an ACPI based platform, you need to
> describe it in ACPI as much as possible. The rest we may discuss.
> 

Agreed but the PCIe card might also be plugged in a system using a
device-tree description (ARM for instance). I should I do that without
duplicating the description both in DT and ACPI ?

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 09/10] i2c: mux: add support for fwnode

[Clément Léger]

Le Mon, 21 Feb 2022 19:55:25 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> On Mon, Feb 21, 2022 at 05:26:51PM +0100, Clément Léger wrote:
> > Modify i2c_mux_add_adapter() to use with fwnode API to allow creating
> > mux adapters with fwnode based devices. This allows to have a node
> > independent support for i2c muxes.  
> 
> I^2C muxes have their own description for DT and ACPI platforms, I'm not sure
> swnode should be used here at all. Just upload a corresponding SSDT overlay or
> DT overlay depending on the platform. Can it be achieved?
> 

Problem is that this PCIe card can be plugged either in a X86 platform
using ACPI or on a ARM one with device-tree. So it means I should have
two "identical" descriptions for each platforms.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Andrew Lunn]

> > In the DT world, we avoid snow flakes. Once you define a binding, it
> > is expected every following board will use it. So what i believe you
> > are doing here is defining how i2c muxes are described in APCI.
> 
> Linux kernel has already established description of I2C muxes in ACPI:
> https://www.kernel.org/doc/html/latest/firmware-guide/acpi/i2c-muxes.html
> 
> I'm not sure we want another one.

Agreed. This implementation needs to make use of that. Thanks for
pointing it out. I don't know the ACPI world, are there any other
overlaps with existing ACPI bindings?

	Andrew

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Andy Shevchenko]

On Tue, Feb 22, 2022 at 9:57 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > In the DT world, we avoid snow flakes. Once you define a binding, it
> > > is expected every following board will use it. So what i believe you
> > > are doing here is defining how i2c muxes are described in APCI.
> >
> > Linux kernel has already established description of I2C muxes in ACPI:
> > https://www.kernel.org/doc/html/latest/firmware-guide/acpi/i2c-muxes.html
> >
> > I'm not sure we want another one.
>
> Agreed. This implementation needs to make use of that. Thanks for
> pointing it out. I don't know the ACPI world, are there any other
> overlaps with existing ACPI bindings?

Besides ACPI specification, which defines _CRS resources, such as I2C,
SPI, GPIO, and other peripheral connections, in the Linux kernel we
have already established these [1]. I hope it's all here, since in the
past not everything got documented and there were some documentation
patches in time.

On top of that there are some Microsoft documents on enumeration that
Linux follows, such as USB embedded devices [2]. There is also a PCI
FW specification that defines how PCI bus devices, bridges, etc have
to be represented in ACPI, including additional tables, such as MCFG.

[1]: https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html
[2]: https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-_adr-for-embedded-usb-devices

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 02/10] property: add fwnode_get_match_data()

[Andy Shevchenko]

On Tue, Feb 22, 2022 at 9:47 AM Clément Léger <clement.leger@bootlin.com> wrote:
> Le Tue, 22 Feb 2022 09:33:32 +0100,
> Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
> > On Tue, Feb 22, 2022 at 9:24 AM Clément Léger <clement.leger@bootlin.com> wrote:
> > > Le Mon, 21 Feb 2022 19:46:12 +0200,
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

...

> > > The idea is to allow device with a software_node description to match
> > > with the content of the of_match_table. Without this, we would need a
> > > new type of match table that would probably duplicates part of the
> > > of_match_table to be able to match software_node against a driver.
> > > I did not found an other way to do it without modifying drivers
> > > individually to support software_nodes.
> >
> > software nodes should not be used as a replacement of the real
> > firmware nodes. The idea behind is to fill the gaps in the cases when
> > firmware doesn't provide enough information to the OS. I think Heikki
> > can confirm or correct me.
>
> Yes, the documentation states that:
>
> NOTE! The primary hardware description should always come from either
> ACPI tables or DT. Describing an entire system with software nodes,
> though possible, is not acceptable! The software nodes should only
> complement the primary hardware description.
>
> > If you want to use the device on an ACPI based platform, you need to
> > describe it in ACPI as much as possible. The rest we may discuss.
>
> Agreed but the PCIe card might also be plugged in a system using a
> device-tree description (ARM for instance). I should I do that without
> duplicating the description both in DT and ACPI ?

Why is it (duplication) a problem?
Each platform has its own kind of description, so one needs to provide
it in the format the platform accepts.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 02/10] property: add fwnode_get_match_data()

[Clément Léger]

Le Tue, 22 Feb 2022 10:24:13 +0100,
Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :

> > > If you want to use the device on an ACPI based platform, you need to
> > > describe it in ACPI as much as possible. The rest we may discuss.  
> >
> > Agreed but the PCIe card might also be plugged in a system using a
> > device-tree description (ARM for instance). I should I do that without
> > duplicating the description both in DT and ACPI ?  
> 
> Why is it (duplication) a problem?
> Each platform has its own kind of description, so one needs to provide
> it in the format the platform accepts.
> 

The problem that I see is not only duplication but also that the PCIe
card won't work out of the box and will need a specific SSDT overlays
each time it is used. According to your document about SSDT overlays,
there is no way to load this from the driver. This means that the user
will have to compile a platform specific .aml file to match its
platform configuration. If the user wants to change the PCIe port, than
I guess it will have to load another .aml file. I do not think a user
expect to do so when plugging a PCIe card.

Moreover, the APCI documentation [1] says the following:

"PCI devices, which are below the host bridge, generally do not need to
be described via ACPI. The OS can discover them via the standard PCI
enumeration mechanism, using config accesses to discover and identify
devices and read and size their BARs. However, ACPI may describe PCI
devices if it provides power management or hotplug functionality for
them or if the device has INTx interrupts connected by platform
interrupt controllers and a _PRT is needed to describe those
connections."

The device I want to use (a PCIe switch) does not fall into these
categories so there should be no need to describe it using ACPI.
Regarding the use of software nodes, the documentation also says that:

"The software nodes can be used to complement fwnodes representing real
firmware nodes when they are incomplete, for example missing device
properties, *and to supply the primary fwnode when the firmware lacks
hardware description for a device completely.*"

I think my device falls into this last category but I might be wrong. I
understand that using software_node is probably not the best idea to do
so but I did not found any other convenient way to do it and SSDT
overlays do not really seems to be ideal. I would be glad if you
could provide me with an example of such usage to check if it's really
usable.

Thanks,

[1] https://www.kernel.org/doc/html/latest/PCI/acpi-info.html
-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 02/10] property: add fwnode_get_match_data()

[Greg Kroah-Hartman]

On Tue, Feb 22, 2022 at 10:47:05AM +0100, Clément Léger wrote:
> Le Tue, 22 Feb 2022 10:24:13 +0100,
> Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
> 
> > > > If you want to use the device on an ACPI based platform, you need to
> > > > describe it in ACPI as much as possible. The rest we may discuss.  
> > >
> > > Agreed but the PCIe card might also be plugged in a system using a
> > > device-tree description (ARM for instance). I should I do that without
> > > duplicating the description both in DT and ACPI ?  
> > 
> > Why is it (duplication) a problem?
> > Each platform has its own kind of description, so one needs to provide
> > it in the format the platform accepts.
> > 
> 
> The problem that I see is not only duplication but also that the PCIe
> card won't work out of the box and will need a specific SSDT overlays
> each time it is used. According to your document about SSDT overlays,
> there is no way to load this from the driver. This means that the user
> will have to compile a platform specific .aml file to match its
> platform configuration. If the user wants to change the PCIe port, than
> I guess it will have to load another .aml file. I do not think a user
> expect to do so when plugging a PCIe card.
> 
> Moreover, the APCI documentation [1] says the following:
> 
> "PCI devices, which are below the host bridge, generally do not need to
> be described via ACPI. The OS can discover them via the standard PCI
> enumeration mechanism, using config accesses to discover and identify
> devices and read and size their BARs. However, ACPI may describe PCI
> devices if it provides power management or hotplug functionality for
> them or if the device has INTx interrupts connected by platform
> interrupt controllers and a _PRT is needed to describe those
> connections."
> 
> The device I want to use (a PCIe switch) does not fall into these
> categories so there should be no need to describe it using ACPI.

There should not be any need to describe it in any way, the device
should provide all of the needed information.  PCIe devices do not need
a DT entry, as that does not make sense.

thanks,

greg k-h

Re: [RFC 09/10] i2c: mux: add support for fwnode

[Andrew Lunn]

On Tue, Feb 22, 2022 at 09:53:25AM +0100, Clément Léger wrote:
> Le Mon, 21 Feb 2022 19:55:25 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> 
> > On Mon, Feb 21, 2022 at 05:26:51PM +0100, Clément Léger wrote:
> > > Modify i2c_mux_add_adapter() to use with fwnode API to allow creating
> > > mux adapters with fwnode based devices. This allows to have a node
> > > independent support for i2c muxes.  
> > 
> > I^2C muxes have their own description for DT and ACPI platforms, I'm not sure
> > swnode should be used here at all. Just upload a corresponding SSDT overlay or
> > DT overlay depending on the platform. Can it be achieved?
> > 
> 
> Problem is that this PCIe card can be plugged either in a X86 platform
> using ACPI or on a ARM one with device-tree. So it means I should have
> two "identical" descriptions for each platforms.

ACPI != DT.

I know people like stuffing DT properties into ACPI tables, when ACPI
does not have a binding. But in this case, there is a well defined
ACPI mechanism for I2C muxes. You cannot ignore it because it is
different to DT. So you need to handle the muxes in both the ACPI way
and the DT way.

For other parts of what you are doing, you might be able to get away
by just stuffing DT properties into ACPI tables. But that is not for
me to decide, that is up to the ACPI maintainers.

	Andrew

Re: [RFC 10/10] net: sfp: add support for fwnode

[Clément Léger]

Le Mon, 21 Feb 2022 19:57:39 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
> > Add support to retrieve a i2c bus in sfp with a fwnode. This support
> > is using the fwnode API which also works with device-tree and ACPI.
> > For this purpose, the device-tree and ACPI code handling the i2c
> > adapter retrieval was factorized with the new code. This also allows
> > i2c devices using a software_node description to be used by sfp code.  
> 
> If I'm not mistaken this patch can even go separately right now, since all used
> APIs are already available.
> 

This patches uses fwnode_find_i2c_adapter_by_node() which is introduced
by "i2c: fwnode: add fwnode_find_i2c_adapter_by_node()" but they can
probably be contributed both in a separate series.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 02/10] property: add fwnode_get_match_data()

[Clément Léger]

Le Tue, 22 Feb 2022 11:20:54 +0100,
Greg Kroah-Hartman <gregkh@linuxfoundation.org> a écrit :

> > 
> > The problem that I see is not only duplication but also that the PCIe
> > card won't work out of the box and will need a specific SSDT overlays
> > each time it is used. According to your document about SSDT overlays,
> > there is no way to load this from the driver. This means that the user
> > will have to compile a platform specific .aml file to match its
> > platform configuration. If the user wants to change the PCIe port, than
> > I guess it will have to load another .aml file. I do not think a user
> > expect to do so when plugging a PCIe card.
> > 
> > Moreover, the APCI documentation [1] says the following:
> > 
> > "PCI devices, which are below the host bridge, generally do not need to
> > be described via ACPI. The OS can discover them via the standard PCI
> > enumeration mechanism, using config accesses to discover and identify
> > devices and read and size their BARs. However, ACPI may describe PCI
> > devices if it provides power management or hotplug functionality for
> > them or if the device has INTx interrupts connected by platform
> > interrupt controllers and a _PRT is needed to describe those
> > connections."
> > 
> > The device I want to use (a PCIe switch) does not fall into these
> > categories so there should be no need to describe it using ACPI.  
> 
> There should not be any need to describe it in any way, the device
> should provide all of the needed information.  PCIe devices do not need
> a DT entry, as that does not make sense.
> 
> thanks,
> 
> greg k-h

In my opinion, yes, there should be no need for an "external"
description. Loading any kind of description (either with ACPI or DT)
defeat the purpose of the PCI since the card won't be able to be
plug-and-play anymore on multiple platform.

The driver however should be self-contained and contain its own
"description" of the hardware, no matter the platform on which the card
is plugged. The PCIe card is independent of the platform, I do not
think describing it with a platform specific description language is
maintainable nor user acceptable for the user.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Clément Léger]

Le Mon, 21 Feb 2022 19:41:24 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> > 
> > We thought about adding CONFIG_OF to x86 and potentially describe this
> > card using device-tree overlays but it introduce other problems that
> > also seems difficult to solve (overlay loading without base
> > device-tree, fixup of IRQs, adresses, and so on) and CONFIG_OF is not
> > often enabled on x86 to say the least.  
> 
> Why it can't be described by SSDT overlay (if the x86 platform in question is
> ACPI based)?

This devices uses a SoC for which drivers are already available but are
meant to be used by a device-tree description. These drivers uses the
following subsystems:
- reset (no ACPI support ?)
- clk (no ACPI support ?)
- pinctrl (no ACPI support ?)
- syscon (no ACPI support ?)
- gpio
- phy
- mdio

Converting existing OF support to fwnode support and thus allowing
drivers and subsystems to be compatible with software nodes seemed like
the easiest way to do what I needed by keeping all existing drivers.
With this support, the driver is completely self-contained and does
allow the card to be plugged on whatever platform the user may have.

Again, the PCI card is independent of the platform, I do not really see
why it should be described using platform description language.

> > 
> > This series introduce a number of changes in multiple subsystems to
> > allow registering and using devices that are described with a
> > software_node description attached to a mfd_cell, making them usable
> > with the fwnode API. It was needed to modify many subsystem where
> > CONFIG_OF was tightly integrated through the use of of_xlate()
> > functions and other of_* calls. New calls have been added to use fwnode
> > API and thus be usable with a wider range of nodes. Functions that are
> > used to get the devices (pinctrl_get, clk_get and so on) also needed
> > to be changed to use the fwnode API internally.
> > 
> > For instance, the clk framework has been modified to add a
> > fwnode_xlate() callback and a new named fwnode_clk_add_hw_provider()
> > has been added. This function will register a clock using
> > fwnode_xlate() callback. Note that since the fwnode API is compatible
> > with devices that have a of_node member set, it will still be possible
> > to use the driver and get the clocks with CONFIG_OF enabled
> > configurations.  
> 
> How does this all is compatible with ACPI approaches?
> I mean we usually do not reintroduce 1:1 DT schemas in ACPI.

For the moment, I only added fwnode API support as an alternative to
support both OF and software nodes. ACPI is not meant to be handled by
this code "as-is". There is for sure some modifications to be made and
I do not know how clocks are handled when using ACPI. Based on some
thread dating back to 2018 [1], it seem it was even not supported at
all.

To be clear, I added the equivalent of the OF support but using
fwnode API because I was interested primarly in using it with software
nodes and still wanted OF support to work. I did not planned it to be
"ACPI compliant" right now since I do not have any knowledge in that
field.

> 
> I think the CCF should be converted to use fwnode APIs and meanwhile
> we may discuss how to deal with clocks on ACPI platforms, because
> it may be a part of the power management methods.

Ok, before going down that way, should the fwnode support be the "only"
one, ie remove of_clk_register and others and convert them to
fwnode_clk_register for instance or should it be left to avoid
modifying all clock drivers ?

> 
> > In some subsystems, it was possible to keep OF related function by
> > wrapping the fwnode ones. It is not yet sure if both support
> > (device-tree and fwnode) should still continue to coexists. For instance
> > if fwnode_xlate() and of_xlate() should remain since the fwnode version
> > also supports device-tree. Removing of_xlate() would of course require
> > to modify all drivers that uses it.
> > 
> > Here is an excerpt of the lan966x description when used as a PCIe card.
> > The complete description is visible at [2]. This part only describe the
> > flexcom controller and the fixed-clock that is used as an input clock.
> > 
> > static const struct property_entry ddr_clk_props[] = {
> >         PROPERTY_ENTRY_U32("clock-frequency", 30000000),  
> 
> >         PROPERTY_ENTRY_U32("#clock-cells", 0),  
> 
> Why this is used?
> 

These props actually describes a fixed-clock properties. When adding
fwnode support to clk framework, it was needed to add the
equivalent of of_xlate() for fwnode (fwnode_xlate()). The number of
cells used to describe a reference is still needed to do the
translation using fwnode_property_get_reference_args() and give the
correct arguments to fwnode_xlate().

[1]
https://lore.kernel.org/lkml/914341e7-ca94-054d-6127-522b745006b4@arm.com/T/
-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 09/10] i2c: mux: add support for fwnode

[Alexandre Belloni]

On 22/02/2022 11:57:39+0100, Andrew Lunn wrote:
> On Tue, Feb 22, 2022 at 09:53:25AM +0100, Clément Léger wrote:
> > Le Mon, 21 Feb 2022 19:55:25 +0200,
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> > 
> > > On Mon, Feb 21, 2022 at 05:26:51PM +0100, Clément Léger wrote:
> > > > Modify i2c_mux_add_adapter() to use with fwnode API to allow creating
> > > > mux adapters with fwnode based devices. This allows to have a node
> > > > independent support for i2c muxes.  
> > > 
> > > I^2C muxes have their own description for DT and ACPI platforms, I'm not sure
> > > swnode should be used here at all. Just upload a corresponding SSDT overlay or
> > > DT overlay depending on the platform. Can it be achieved?
> > > 
> > 
> > Problem is that this PCIe card can be plugged either in a X86 platform
> > using ACPI or on a ARM one with device-tree. So it means I should have
> > two "identical" descriptions for each platforms.
> 
> ACPI != DT.
> 
> I know people like stuffing DT properties into ACPI tables, when ACPI
> does not have a binding. But in this case, there is a well defined
> ACPI mechanism for I2C muxes. You cannot ignore it because it is
> different to DT. So you need to handle the muxes in both the ACPI way
> and the DT way.
> 
> For other parts of what you are doing, you might be able to get away
> by just stuffing DT properties into ACPI tables. But that is not for
> me to decide, that is up to the ACPI maintainers.
> 

What I'm wondering is why you would have to stuff anything in ACPI when
plugging any PCIe card in a PC. Wouldn't that be a first?

I don't have to do so when plugging an Intel network card, I don't
expect to have to do it when plugging any other network card...

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC 10/10] net: sfp: add support for fwnode

[Andy Shevchenko]

On Tue, Feb 22, 2022 at 02:25:13PM +0100, Clément Léger wrote:
> Le Mon, 21 Feb 2022 19:57:39 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> 
> > On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
> > > Add support to retrieve a i2c bus in sfp with a fwnode. This support
> > > is using the fwnode API which also works with device-tree and ACPI.
> > > For this purpose, the device-tree and ACPI code handling the i2c
> > > adapter retrieval was factorized with the new code. This also allows
> > > i2c devices using a software_node description to be used by sfp code.  
> > 
> > If I'm not mistaken this patch can even go separately right now, since all used
> > APIs are already available.
> 
> This patches uses fwnode_find_i2c_adapter_by_node() which is introduced
> by "i2c: fwnode: add fwnode_find_i2c_adapter_by_node()" but they can
> probably be contributed both in a separate series.

I summon Hans into the discussion since I remember he recently refactored
a bit I2C (ACPI/fwnode) APIs. Also he might have an idea about entire big
picture approach with this series based on his ACPI experience.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 10/10] net: sfp: add support for fwnode

[Hans de Goede]

Hi,

On 2/23/22 12:22, Andy Shevchenko wrote:
> On Tue, Feb 22, 2022 at 02:25:13PM +0100, Clément Léger wrote:
>> Le Mon, 21 Feb 2022 19:57:39 +0200,
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
>>
>>> On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
>>>> Add support to retrieve a i2c bus in sfp with a fwnode. This support
>>>> is using the fwnode API which also works with device-tree and ACPI.
>>>> For this purpose, the device-tree and ACPI code handling the i2c
>>>> adapter retrieval was factorized with the new code. This also allows
>>>> i2c devices using a software_node description to be used by sfp code.  
>>>
>>> If I'm not mistaken this patch can even go separately right now, since all used
>>> APIs are already available.
>>
>> This patches uses fwnode_find_i2c_adapter_by_node() which is introduced
>> by "i2c: fwnode: add fwnode_find_i2c_adapter_by_node()" but they can
>> probably be contributed both in a separate series.
> 
> I summon Hans into the discussion since I remember he recently refactored
> a bit I2C (ACPI/fwnode) APIs. Also he might have an idea about entire big
> picture approach with this series based on his ACPI experience.

If I understand this series correctly then this is about a PCI-E card
which has an I2C controller on the card and behind that I2C-controller
there are a couple if I2C muxes + I2C clients.

And the goal of the series is to describe those I2C muxes + I2C clients
with software nodes so that the existing I2C enumeration code can be
used (after porting the existing I2C enumeration code from OF functions
to generic fwnode functions).

Did I understand this bit correctly? I believe that a lot of the
discussion here is caused by the initial problem / hw-setup this
series tries to address / support is not described very well ?

Assuming I did understand the above correctly. One alternative would be
to simply manually instantiate the I2C muxes + clients using
i2c_new_client_device(). But I'm not sure if i2c_new_client_device()
will work for the muxes without adding some software_nodes which
brings us back to something like this patch-set.

In general I believe that porting things away from OF specific parsing
to the generic fwnode APIs is a good thing.

Making device_get_match_data() for devices with only a software fwnode
use of_device_id matching feels a bit weird. But it also makes sense
since that requires just adding a compatible string to the software
fwnode properties which is easy and it allows re-uses existing
matching code in the drivers.

I understand various people falling over this weirdness but AFAICT
the alternative would be adding some special swnode_id type + matching
code for devices where the primary fwnode is a software fwnode, which
would just be a whole bunch of extra code ending up with something
similar.

So re-using of_device_id-s for matching of devices where the primary
fwnode is a software fwnode seems like a good idea. *But* this all
needs to be explained in the commit message a lot better. It really
needs to be spelled out that this is:

a) Only for matching devices where the primary fwnode is a software fwnode 

b) Really has nothing to do with of/dt but of_device_id matching is
   used here to avoid having to introduce a new matching mechanism just
   for devices where the primary fwnode is a software fwnode

c) That introducing a new software fwnode matching mechanism would be
   a bad idea since this will require adding new swnode_match tables
   to many drivers, where as re-using of_device_id will make drivers
   which already have an of_match_table just work.

And this should be spelled out in both the commit message as well
as in some documentation / kdoc comments. Because although a useful
trick, reusing the of_match_id-s is also confusing which I believe
is what has led to a lot of the discussion on this patch-set so far.

Note the above all relies on my interpretation of this patch set,
which may be wrong, since as said the patch-set does seem to be
lacking when it comes to documentation / motivation of the patches.

Regards,

Hans

Re: [RFC 10/10] net: sfp: add support for fwnode

[Andrew Lunn]

> If I understand this series correctly then this is about a PCI-E card
> which has an I2C controller on the card and behind that I2C-controller
> there are a couple if I2C muxes + I2C clients.

They are not i2c clients in the normal sense. The i2c bus connects to
an SFP cage. You can hot plug different sort of network modules into
the cage. So for example fibre optic modules or copper modules. The
modules have an 'at24' like block of memory, which is a mix of EEPROM
and status values. For copper modules, there is an MDIO over I2C
protocol which allows access to the Copper Ethernet PHY inside the
module.

The current device tree binding is that you have a node for the SFP
cage, which includes a phandle to the i2c bus, plus a list of GPIOs
connected to pins on the SFP cage. The SFP driver will then directly
access the memory, maybe instantiate an mdio-over-i2c device if
needed, and control the GPIOs. The Ethernet driver then has an OF node
with a phandle pointing to the SFP device.

The whole design of this is based around the hardware being a
collection of standard parts, i2c bus, i2c mux, gpio controller,
ethernet controller, each with their own driver as part of a linux
subsystem, and then having some glue to put all the parts
together. And at the moment, that glue is DT.

> Note the above all relies on my interpretation of this patch set,
> which may be wrong, since as said the patch-set does seem to be
> lacking when it comes to documentation / motivation of the patches.

I think some examples from DT will help with this.

  Andrew

Re: [RFC 10/10] net: sfp: add support for fwnode

[Russell King (Oracle)]

On Wed, Feb 23, 2022 at 01:02:23PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 2/23/22 12:22, Andy Shevchenko wrote:
> > On Tue, Feb 22, 2022 at 02:25:13PM +0100, Clément Léger wrote:
> >> Le Mon, 21 Feb 2022 19:57:39 +0200,
> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> >>
> >>> On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
> >>>> Add support to retrieve a i2c bus in sfp with a fwnode. This support
> >>>> is using the fwnode API which also works with device-tree and ACPI.
> >>>> For this purpose, the device-tree and ACPI code handling the i2c
> >>>> adapter retrieval was factorized with the new code. This also allows
> >>>> i2c devices using a software_node description to be used by sfp code.  
> >>>
> >>> If I'm not mistaken this patch can even go separately right now, since all used
> >>> APIs are already available.
> >>
> >> This patches uses fwnode_find_i2c_adapter_by_node() which is introduced
> >> by "i2c: fwnode: add fwnode_find_i2c_adapter_by_node()" but they can
> >> probably be contributed both in a separate series.
> > 
> > I summon Hans into the discussion since I remember he recently refactored
> > a bit I2C (ACPI/fwnode) APIs. Also he might have an idea about entire big
> > picture approach with this series based on his ACPI experience.
> 
> If I understand this series correctly then this is about a PCI-E card
> which has an I2C controller on the card and behind that I2C-controller
> there are a couple if I2C muxes + I2C clients.

That is what I gathered as well.

> Assuming I did understand the above correctly. One alternative would be
> to simply manually instantiate the I2C muxes + clients using
> i2c_new_client_device(). But I'm not sure if i2c_new_client_device()
> will work for the muxes without adding some software_nodes which
> brings us back to something like this patch-set.

That assumes that an I2C device is always present, which is not always
the case - there are hot-pluggable devices on I2C buses.

Specifically, this series includes pluggable SFP modules, which fall
into this category of "hot-pluggable I2C devices" - spanning several
bus addresses (0x50, 0x51, 0x56). 0x50 is EEPROM like, but not quite
as the top 128 bytes is paged and sometimes buggy in terms of access
behaviour. 0x51 contains a bunch of monitoring and other controls
for the module which again can be paged. At 0x56, there may possibly
be some kind of device that translates I2C accesses to MDIO accesses
to access a PHY onboard.

Consequently, the SFP driver and MDIO translation layer wants access to
the I2C bus, rather than a device.

Now, before ARM was converted to DT, we had ways to cope with
non-firmware described setups like this by using platform devices and
platform data. Much of that ended up deprecated, because - hey - DT
is great and more modern and the old way is disgusting and we want to
get rid of it.

However, that approach locks us into describing stuff in firmware,
which is unsuitable when something like this comes along.

I think what we need is both approaches. We need a way for the SFP
driver (which is a platform_driver) to be used _without_ needing
descriptions in firmware. I think we have that for GPIOs, but for an
I2C bus, We have i2c_get_adapter() for I2C buses, but that needs the
bus number - we could either pass the i2c_adapter or the adapter
number through platform data to the SFP driver.

Or is there another solution to being able to reuse multi-driver
based infrastructure that we have developed based on DT descriptions
in situations such as an add-in PCI card?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC 10/10] net: sfp: add support for fwnode

[Hans de Goede]

Hi,

On 2/23/22 13:41, Russell King (Oracle) wrote:
> On Wed, Feb 23, 2022 at 01:02:23PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 2/23/22 12:22, Andy Shevchenko wrote:
>>> On Tue, Feb 22, 2022 at 02:25:13PM +0100, Clément Léger wrote:
>>>> Le Mon, 21 Feb 2022 19:57:39 +0200,
>>>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
>>>>
>>>>> On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
>>>>>> Add support to retrieve a i2c bus in sfp with a fwnode. This support
>>>>>> is using the fwnode API which also works with device-tree and ACPI.
>>>>>> For this purpose, the device-tree and ACPI code handling the i2c
>>>>>> adapter retrieval was factorized with the new code. This also allows
>>>>>> i2c devices using a software_node description to be used by sfp code.  
>>>>>
>>>>> If I'm not mistaken this patch can even go separately right now, since all used
>>>>> APIs are already available.
>>>>
>>>> This patches uses fwnode_find_i2c_adapter_by_node() which is introduced
>>>> by "i2c: fwnode: add fwnode_find_i2c_adapter_by_node()" but they can
>>>> probably be contributed both in a separate series.
>>>
>>> I summon Hans into the discussion since I remember he recently refactored
>>> a bit I2C (ACPI/fwnode) APIs. Also he might have an idea about entire big
>>> picture approach with this series based on his ACPI experience.
>>
>> If I understand this series correctly then this is about a PCI-E card
>> which has an I2C controller on the card and behind that I2C-controller
>> there are a couple if I2C muxes + I2C clients.
> 
> That is what I gathered as well.
> 
>> Assuming I did understand the above correctly. One alternative would be
>> to simply manually instantiate the I2C muxes + clients using
>> i2c_new_client_device(). But I'm not sure if i2c_new_client_device()
>> will work for the muxes without adding some software_nodes which
>> brings us back to something like this patch-set.
> 
> That assumes that an I2C device is always present, which is not always
> the case - there are hot-pluggable devices on I2C buses.
> 
> Specifically, this series includes pluggable SFP modules, which fall
> into this category of "hot-pluggable I2C devices" - spanning several
> bus addresses (0x50, 0x51, 0x56). 0x50 is EEPROM like, but not quite
> as the top 128 bytes is paged and sometimes buggy in terms of access
> behaviour. 0x51 contains a bunch of monitoring and other controls
> for the module which again can be paged. At 0x56, there may possibly
> be some kind of device that translates I2C accesses to MDIO accesses
> to access a PHY onboard.
> 
> Consequently, the SFP driver and MDIO translation layer wants access to
> the I2C bus, rather than a device.
> 
> Now, before ARM was converted to DT, we had ways to cope with
> non-firmware described setups like this by using platform devices and
> platform data. Much of that ended up deprecated, because - hey - DT
> is great and more modern and the old way is disgusting and we want to
> get rid of it.
> 
> However, that approach locks us into describing stuff in firmware,
> which is unsuitable when something like this comes along.
> 
> I think what we need is both approaches. We need a way for the SFP
> driver (which is a platform_driver) to be used _without_ needing
> descriptions in firmware. I think we have that for GPIOs, but for an
> I2C bus, We have i2c_get_adapter() for I2C buses, but that needs the
> bus number - we could either pass the i2c_adapter or the adapter
> number through platform data to the SFP driver.
> 
> Or is there another solution to being able to reuse multi-driver
> based infrastructure that we have developed based on DT descriptions
> in situations such as an add-in PCI card?

The use of software fwnode-s as proposed in this patch-set is another
way to deal with this. There has been work to abstract ACPI vs
of/dt firmware-nodes into a generic fwnode concept and software-nodes
are a third way to define fwnode-s for "struct device" devices.

Software nodes currently are mainly used as so called secondary
fwnodes which means they can e.g. add extra properties to cover
for the firmware description missing some info (which at least
on ACPI happens more often then we would like).

But a software-node can also be used as the primary fwnode for
a device. So what this patch-set does is move the i2c of/dt
enumeration code over to the fwnode abstraction (1). This allows
the driver for the SPF card to attach a software fwnode to the
device for the i2c-controller which describes the hotplug pins +
any other always present hw in the same way as it would be done
in a devicetree fwnode and then the existing of/dt based SPF
code can be re-used as is.

At least that is my understanding of this patch-set.

Regards,

Hans



1) This should result in no functional changes for existing
devicetree use cases.

Re: [RFC 10/10] net: sfp: add support for fwnode

[Clément Léger]

Le Wed, 23 Feb 2022 14:39:27 +0100,
Hans de Goede <hdegoede@redhat.com> a écrit :

> > I think what we need is both approaches. We need a way for the SFP
> > driver (which is a platform_driver) to be used _without_ needing
> > descriptions in firmware. I think we have that for GPIOs, but for an
> > I2C bus, We have i2c_get_adapter() for I2C buses, but that needs the
> > bus number - we could either pass the i2c_adapter or the adapter
> > number through platform data to the SFP driver.
> > 
> > Or is there another solution to being able to reuse multi-driver
> > based infrastructure that we have developed based on DT descriptions
> > in situations such as an add-in PCI card?  
> 
> The use of software fwnode-s as proposed in this patch-set is another
> way to deal with this. There has been work to abstract ACPI vs
> of/dt firmware-nodes into a generic fwnode concept and software-nodes
> are a third way to define fwnode-s for "struct device" devices.
> 
> Software nodes currently are mainly used as so called secondary
> fwnodes which means they can e.g. add extra properties to cover
> for the firmware description missing some info (which at least
> on ACPI happens more often then we would like).
> 
> But a software-node can also be used as the primary fwnode for
> a device. So what this patch-set does is move the i2c of/dt
> enumeration code over to the fwnode abstraction (1). This allows
> the driver for the SPF card to attach a software fwnode to the
> device for the i2c-controller which describes the hotplug pins +
> any other always present hw in the same way as it would be done
> in a devicetree fwnode and then the existing of/dt based SPF
> code can be re-used as is.
> 
> At least that is my understanding of this patch-set.
> 
> Regards,
> 
> Hans

Hello Hans, your understanding is totally correct.

This PCIe device actually embeds much more than just a I2C controller.
I should have made that clearer in the cover letter, sorry for the
confusion. The PCIe card is actually using a lan9662x SoC which is
meant to be used as an ethernet switch with 4 ports (2 RJ45 and two
SFPS). In order to use this switch, the following drivers can be reused:
 - lan966x-switch
 - reset-microchip-sparx5
 - lan966x_serdes
 - reset-microchip-lan966x-phy
 - mdio-mscc-miim
 - pinctrl-lan966x
 - atmel-flexcom
 - i2c-at91
 - i2c-mux
 - i2c-mux-pinctrl
 - sfp
 - clk-lan966x
 - lan966x-pci-mfd

All theses drivers are using of_* API and as such only works with a DT
description. One solution that did seems acceptable to me (although
not great)was to use mfd_cells and software_node description
as primary node.

Since I wanted to convert these to be software_node compatible, I had
to modify many subsystems (pinctrl, gpio, i2c, clocks, reset, etc).
This is why I stated in the cover letter that "This series is part of a
larger changeset that touches multiple subsystems". But clearly, it
lacks more context and namely the list of subsystems that needed to be
modify as well as the PCIe card type. I will modify this cover-letter to
add more informations.

So indeed, this series is targetting at using devices which uses a
software_node as a primary node and modifying subsystems to use the
fwnode API in order to make that compatible with these software nodes.
As you said, in order to avoid redefining the match tables and allow
device_get_match_data to work with software_node, the trick was to
reuse the of_table_id

However, I'm not totally happy with that as it seems we are doing what
was done with the "old" platform_data (a bit cleaner maybe since it
allows to reuse the driver with the fwnode API).

As Russell asked, I'm also really interested if someone has a solution
to reuse device-tree description (overlays ?) to describe such
hardware. However, the fact that CONFIG_OF isn't enabled on x86 config
seems a bit complicated on this side. This also requires to load a
device-tree overlay from the filesystem to describe the card, but that
might be something that could be made generic to allow other uses-cases.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 10/10] net: sfp: add support for fwnode

[Andy Shevchenko]

On Wed, Feb 23, 2022 at 12:41:47PM +0000, Russell King (Oracle) wrote:
> On Wed, Feb 23, 2022 at 01:02:23PM +0100, Hans de Goede wrote:
> > On 2/23/22 12:22, Andy Shevchenko wrote:
> > > On Tue, Feb 22, 2022 at 02:25:13PM +0100, Clément Léger wrote:
> > >> Le Mon, 21 Feb 2022 19:57:39 +0200,
> > >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> > >>> On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
> > >>>> Add support to retrieve a i2c bus in sfp with a fwnode. This support
> > >>>> is using the fwnode API which also works with device-tree and ACPI.
> > >>>> For this purpose, the device-tree and ACPI code handling the i2c
> > >>>> adapter retrieval was factorized with the new code. This also allows
> > >>>> i2c devices using a software_node description to be used by sfp code.  
> > >>>
> > >>> If I'm not mistaken this patch can even go separately right now, since all used
> > >>> APIs are already available.
> > >>
> > >> This patches uses fwnode_find_i2c_adapter_by_node() which is introduced
> > >> by "i2c: fwnode: add fwnode_find_i2c_adapter_by_node()" but they can
> > >> probably be contributed both in a separate series.
> > > 
> > > I summon Hans into the discussion since I remember he recently refactored
> > > a bit I2C (ACPI/fwnode) APIs. Also he might have an idea about entire big
> > > picture approach with this series based on his ACPI experience.
> > 
> > If I understand this series correctly then this is about a PCI-E card
> > which has an I2C controller on the card and behind that I2C-controller
> > there are a couple if I2C muxes + I2C clients.
> 
> That is what I gathered as well.
> 
> > Assuming I did understand the above correctly. One alternative would be
> > to simply manually instantiate the I2C muxes + clients using
> > i2c_new_client_device(). But I'm not sure if i2c_new_client_device()
> > will work for the muxes without adding some software_nodes which
> > brings us back to something like this patch-set.
> 
> That assumes that an I2C device is always present, which is not always
> the case - there are hot-pluggable devices on I2C buses.
> 
> Specifically, this series includes pluggable SFP modules, which fall
> into this category of "hot-pluggable I2C devices" - spanning several
> bus addresses (0x50, 0x51, 0x56). 0x50 is EEPROM like, but not quite
> as the top 128 bytes is paged and sometimes buggy in terms of access
> behaviour. 0x51 contains a bunch of monitoring and other controls
> for the module which again can be paged. At 0x56, there may possibly
> be some kind of device that translates I2C accesses to MDIO accesses
> to access a PHY onboard.
> 
> Consequently, the SFP driver and MDIO translation layer wants access to
> the I2C bus, rather than a device.
> 
> Now, before ARM was converted to DT, we had ways to cope with
> non-firmware described setups like this by using platform devices and
> platform data. Much of that ended up deprecated, because - hey - DT
> is great and more modern and the old way is disgusting and we want to
> get rid of it.
> 
> However, that approach locks us into describing stuff in firmware,
> which is unsuitable when something like this comes along.

Looks like this is a way to reinvent what FPGA should cope with already.
And if I remember correctly the discussions about PCIe FPGAs (from 2016,
though) the idea is that FPGA should have provided a firmware description
with itself. I.o.w. If we are talking about "run-time configurable"
devices they should provide a way to bring their description to the
system.

The currently available way to do it is to get this from EEPROM / ROM
specified on the hardware side in form of DT and ACPI blobs (representing
overlays). Then the only part that is missed (at least for ACPI case) is
to dynamically insert that based on the PCI BDF of the corresponding
PCI bridge.

TL;DR: In my opinion such hardware must bring the description with itself
in case it uses non-enumerable busses, such as SPI, I²C.

I dunno what was the last development in this area for FPGAs cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Andy Shevchenko]

On Tue, Feb 22, 2022 at 05:30:19PM +0100, Clément Léger wrote:
> Le Mon, 21 Feb 2022 19:41:24 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> > > We thought about adding CONFIG_OF to x86 and potentially describe this
> > > card using device-tree overlays but it introduce other problems that
> > > also seems difficult to solve (overlay loading without base
> > > device-tree, fixup of IRQs, adresses, and so on) and CONFIG_OF is not
> > > often enabled on x86 to say the least.  
> > 
> > Why it can't be described by SSDT overlay (if the x86 platform in question is
> > ACPI based)?
> 
> This devices uses a SoC for which drivers are already available but are
> meant to be used by a device-tree description. These drivers uses the
> following subsystems:
> - reset (no ACPI support ?)
> - clk (no ACPI support ?)
> - pinctrl (no ACPI support ?)
> - syscon (no ACPI support ?)
> - gpio
> - phy
> - mdio
> 
> Converting existing OF support to fwnode support and thus allowing
> drivers and subsystems to be compatible with software nodes seemed like
> the easiest way to do what I needed by keeping all existing drivers.
> With this support, the driver is completely self-contained and does
> allow the card to be plugged on whatever platform the user may have.

I agree with Hans on the point that converting to / supporting fwnode is
a good thing by its own.

> Again, the PCI card is independent of the platform, I do not really see
> why it should be described using platform description language.

Yep, and that why it should cope with the platforms it's designed to be used
with.

> > > This series introduce a number of changes in multiple subsystems to
> > > allow registering and using devices that are described with a
> > > software_node description attached to a mfd_cell, making them usable
> > > with the fwnode API. It was needed to modify many subsystem where
> > > CONFIG_OF was tightly integrated through the use of of_xlate()
> > > functions and other of_* calls. New calls have been added to use fwnode
> > > API and thus be usable with a wider range of nodes. Functions that are
> > > used to get the devices (pinctrl_get, clk_get and so on) also needed
> > > to be changed to use the fwnode API internally.
> > > 
> > > For instance, the clk framework has been modified to add a
> > > fwnode_xlate() callback and a new named fwnode_clk_add_hw_provider()
> > > has been added. This function will register a clock using
> > > fwnode_xlate() callback. Note that since the fwnode API is compatible
> > > with devices that have a of_node member set, it will still be possible
> > > to use the driver and get the clocks with CONFIG_OF enabled
> > > configurations.  
> > 
> > How does this all is compatible with ACPI approaches?
> > I mean we usually do not reintroduce 1:1 DT schemas in ACPI.
> 
> For the moment, I only added fwnode API support as an alternative to
> support both OF and software nodes. ACPI is not meant to be handled by
> this code "as-is". There is for sure some modifications to be made and
> I do not know how clocks are handled when using ACPI. Based on some
> thread dating back to 2018 [1], it seem it was even not supported at
> all.
> 
> To be clear, I added the equivalent of the OF support but using
> fwnode API because I was interested primarly in using it with software
> nodes and still wanted OF support to work. I did not planned it to be
> "ACPI compliant" right now since I do not have any knowledge in that
> field.

And here is the problem. We have a few different resource providers
(a.k.a. firmware interfaces) which we need to cope with.

What is going on in this series seems to me quite a violation of the
layers and technologies. But I guess you may find a supporter of your
ideas (I mean Enrico). However, I'm on the other side and do not like
this approach.

> > I think the CCF should be converted to use fwnode APIs and meanwhile
> > we may discuss how to deal with clocks on ACPI platforms, because
> > it may be a part of the power management methods.
> 
> Ok, before going down that way, should the fwnode support be the "only"
> one, ie remove of_clk_register and others and convert them to
> fwnode_clk_register for instance or should it be left to avoid
> modifying all clock drivers ?

IRQ domain framework decided to cohabit both, while deprecating the OF one.
(see "add" vs. "create" APIs there). I think it's a sane choice.

> > > In some subsystems, it was possible to keep OF related function by
> > > wrapping the fwnode ones. It is not yet sure if both support
> > > (device-tree and fwnode) should still continue to coexists. For instance
> > > if fwnode_xlate() and of_xlate() should remain since the fwnode version
> > > also supports device-tree. Removing of_xlate() would of course require
> > > to modify all drivers that uses it.
> > > 
> > > Here is an excerpt of the lan966x description when used as a PCIe card.
> > > The complete description is visible at [2]. This part only describe the
> > > flexcom controller and the fixed-clock that is used as an input clock.
> > > 
> > > static const struct property_entry ddr_clk_props[] = {
> > >         PROPERTY_ENTRY_U32("clock-frequency", 30000000),  
> > 
> > >         PROPERTY_ENTRY_U32("#clock-cells", 0),  
> > 
> > Why this is used?
> 
> These props actually describes a fixed-clock properties. When adding
> fwnode support to clk framework, it was needed to add the
> equivalent of of_xlate() for fwnode (fwnode_xlate()). The number of
> cells used to describe a reference is still needed to do the
> translation using fwnode_property_get_reference_args() and give the
> correct arguments to fwnode_xlate().

What you described is the programming (overkilled) point. But does hardware
needs this? I.o.w. does it make sense in the _hardware_ description?

> [1]
> https://lore.kernel.org/lkml/914341e7-ca94-054d-6127-522b745006b4@arm.com/T/

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 03/10] base: swnode: use fwnode_get_match_data()

[Sakari Ailus]

Hi Clément,

On Tue, Feb 22, 2022 at 09:39:21AM +0100, Clément Léger wrote:
> Le Mon, 21 Feb 2022 19:48:00 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> 
> > On Mon, Feb 21, 2022 at 05:26:45PM +0100, Clément Léger wrote:
> > > In order to allow matching devices with software node with
> > > device_get_match_data(), use fwnode_get_match_data() for
> > > .device_get_match_data operation.  
> > 
> > ...
> > 
> > > +	.device_get_match_data = fwnode_get_match_data,  
> > 
> > Huh? It should be other way around, no?
> > I mean that each of the resource providers may (or may not) provide a
> > method for the specific fwnode abstraction.
> > 
> 
> Indeed, it should be the other way. But since this function is generic
> and uses only fwnode API I guessed it would be more convenient to
> define it in the fwnode generic part and use it for specific
> implementation. I could have modified device_get_match_data to call it
> if there was no .device_get_match_data operation like this:
> 
> const void *device_get_match_data(struct device *dev)
> {
> 	if (!fwnode_has_op(fwnode, device_get_match_data)
> 		return fwnode_get_match_data(dev);
> 	return fwnode_call_ptr_op(dev_fwnode(dev),device_get_match_data, dev);
> }
> 
> But I thought it was more convenient to do it by setting the
> .device_get_match_data field of software_node operations.

Should this function be called e.g. software_node_get_match_data() instead,
as it seems to be specific to software nodes?

-- 
Regards,

Sakari Ailus

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Clément Léger]

Le Wed, 23 Feb 2022 16:46:45 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

[...]

> > 
> > Converting existing OF support to fwnode support and thus allowing
> > drivers and subsystems to be compatible with software nodes seemed like
> > the easiest way to do what I needed by keeping all existing drivers.
> > With this support, the driver is completely self-contained and does
> > allow the card to be plugged on whatever platform the user may have.  
> 
> I agree with Hans on the point that converting to / supporting fwnode is
> a good thing by its own.
> 
> > Again, the PCI card is independent of the platform, I do not really see
> > why it should be described using platform description language.  
> 
> Yep, and that why it should cope with the platforms it's designed to be used
> with.

I don't think PCIe card manufacturer expect them to be used solely on a
x86 platform with ACPI. So why should I used ACPI to describe it (or DT
by the way), that's my point.

[...]

> > 
> > For the moment, I only added fwnode API support as an alternative to
> > support both OF and software nodes. ACPI is not meant to be handled by
> > this code "as-is". There is for sure some modifications to be made and
> > I do not know how clocks are handled when using ACPI. Based on some
> > thread dating back to 2018 [1], it seem it was even not supported at
> > all.
> > 
> > To be clear, I added the equivalent of the OF support but using
> > fwnode API because I was interested primarly in using it with software
> > nodes and still wanted OF support to work. I did not planned it to be
> > "ACPI compliant" right now since I do not have any knowledge in that
> > field.  
> 
> And here is the problem. We have a few different resource providers
> (a.k.a. firmware interfaces) which we need to cope with.

Understood that but does adding fwnode support means it should work
as-is with both DT and ACPI ? ACPI code is still in place and only the
of part was converted. But maybe you expect the fwnode prot to be
conformant with ACPI.

> 
> What is going on in this series seems to me quite a violation of the
> layers and technologies. But I guess you may find a supporter of your
> ideas (I mean Enrico). However, I'm on the other side and do not like
> this approach.

As I said in the cover-letter, this approach is the only one that I did
found acceptable without being tied to some firmware description. If you
have another more portable approach, I'm ok with that. But this
solution should ideally work with pinctrl, gpio, clk, reset, phy, i2c,
i2c-mux without rewriting half of the code. And also allows to easily
swap the PCIe card to other slots/computer without having to modify the
description.

> 
> > 
> > Ok, before going down that way, should the fwnode support be the "only"
> > one, ie remove of_clk_register and others and convert them to
> > fwnode_clk_register for instance or should it be left to avoid
> > modifying all clock drivers ?  
> 
> IRQ domain framework decided to cohabit both, while deprecating the OF one.
> (see "add" vs. "create" APIs there). I think it's a sane choice.

Ok, thanks for the info.

[...]

> > > > static const struct property_entry ddr_clk_props[] = {
> > > >         PROPERTY_ENTRY_U32("clock-frequency", 30000000),    
> > >   
> > > >         PROPERTY_ENTRY_U32("#clock-cells", 0),    
> > > 
> > > Why this is used?  
> > 
> > These props actually describes a fixed-clock properties. When adding
> > fwnode support to clk framework, it was needed to add the
> > equivalent of of_xlate() for fwnode (fwnode_xlate()). The number of
> > cells used to describe a reference is still needed to do the
> > translation using fwnode_property_get_reference_args() and give the
> > correct arguments to fwnode_xlate().  
> 
> What you described is the programming (overkilled) point. But does hardware
> needs this? I.o.w. does it make sense in the _hardware_ description?

This does not makes sense for the hardware of course. It also does not
makes sense for the hardware to provide that in the device-tree though.
I actually think this should be only provided by the drivers but it
might be difficult to parse the descriptions then (either DT or
software_node), at least that's how it works right now.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 03/10] base: swnode: use fwnode_get_match_data()

[Clément Léger]

Le Wed, 23 Feb 2022 17:05:22 +0200,
Sakari Ailus <sakari.ailus@linux.intel.com> a écrit :

> > const void *device_get_match_data(struct device *dev)
> > {
> > 	if (!fwnode_has_op(fwnode, device_get_match_data)
> > 		return fwnode_get_match_data(dev);
> > 	return fwnode_call_ptr_op(dev_fwnode(dev),device_get_match_data, dev);
> > }
> > 
> > But I thought it was more convenient to do it by setting the
> > .device_get_match_data field of software_node operations.  
> 
> Should this function be called e.g. software_node_get_match_data() instead,
> as it seems to be specific to software nodes?

Hi Sakari,

You are right, since the only user of this function currently is the
software_node operations, then I should rename it and move it to
swnode.c maybe.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 03/10] base: swnode: use fwnode_get_match_data()

[Sakari Ailus]

On Wed, Feb 23, 2022 at 04:15:35PM +0100, Clément Léger wrote:
> Le Wed, 23 Feb 2022 17:05:22 +0200,
> Sakari Ailus <sakari.ailus@linux.intel.com> a écrit :
> 
> > > const void *device_get_match_data(struct device *dev)
> > > {
> > > 	if (!fwnode_has_op(fwnode, device_get_match_data)
> > > 		return fwnode_get_match_data(dev);
> > > 	return fwnode_call_ptr_op(dev_fwnode(dev),device_get_match_data, dev);
> > > }
> > > 
> > > But I thought it was more convenient to do it by setting the
> > > .device_get_match_data field of software_node operations.  
> > 
> > Should this function be called e.g. software_node_get_match_data() instead,
> > as it seems to be specific to software nodes?
> 
> Hi Sakari,
> 
> You are right, since the only user of this function currently is the
> software_node operations, then I should rename it and move it to
> swnode.c maybe.

It might be also fit to be used in OF, based on how it looks like.

But currently the original naming makes it seem an fwnode property API
function and that is misleading. I'd move this to swnode.c now with a new
software node specific name, and rethink the naming matter if there would
seem to be possibilities for code re-use.

-- 
Sakari Ailus

Re: [RFC 10/10] net: sfp: add support for fwnode

[Andrew Lunn]

> As Russell asked, I'm also really interested if someone has a solution
> to reuse device-tree description (overlays ?) to describe such
> hardware. However, the fact that CONFIG_OF isn't enabled on x86 config
> seems a bit complicated on this side.

It does work, intel even used it for one of there tiny x86 SoCs. Maybe
it was Newton? If you search around you can find maybe a Linux
Plumbers presentation about DT and x86.

You can probably use a udev rule, triggered by the PCIe device ID to
load the DT overlay.

Do you actually need anything from the host other than PCIe? It sounds
like this card is pretty self contained, so you won't need phandles
pointing to the host i2c bus, or the hosts GPIOs? You only need
phandles to your own i2c bus, your own GPIOs? That will make the
overlay much simpler.

	Andrew

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Andy Shevchenko]

On Wed, Feb 23, 2022 at 04:11:50PM +0100, Clément Léger wrote:
> Le Wed, 23 Feb 2022 16:46:45 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

...

> > > Again, the PCI card is independent of the platform, I do not really see
> > > why it should be described using platform description language.  
> > 
> > Yep, and that why it should cope with the platforms it's designed to be used
> > with.
> 
> I don't think PCIe card manufacturer expect them to be used solely on a
> x86 platform with ACPI. So why should I used ACPI to describe it (or DT
> by the way), that's my point.

Because you want it to be used on x86 platforms. On the rest it needs DT
or whatever is required by those platforms (I dunno about Zephyr RTOS, or
VxWorks, or *BSDs).

...

> > > For the moment, I only added fwnode API support as an alternative to
> > > support both OF and software nodes. ACPI is not meant to be handled by
> > > this code "as-is". There is for sure some modifications to be made and
> > > I do not know how clocks are handled when using ACPI. Based on some
> > > thread dating back to 2018 [1], it seem it was even not supported at
> > > all.
> > > 
> > > To be clear, I added the equivalent of the OF support but using
> > > fwnode API because I was interested primarly in using it with software
> > > nodes and still wanted OF support to work. I did not planned it to be
> > > "ACPI compliant" right now since I do not have any knowledge in that
> > > field.  
> > 
> > And here is the problem. We have a few different resource providers
> > (a.k.a. firmware interfaces) which we need to cope with.
> 
> Understood that but does adding fwnode support means it should work
> as-is with both DT and ACPI ? ACPI code is still in place and only the
> of part was converted. But maybe you expect the fwnode prot to be
> conformant with ACPI.

Not only me, I believe Mark also was against using pure DT approach on
ACPI enabled platforms.

...

> > What is going on in this series seems to me quite a violation of the
> > layers and technologies. But I guess you may find a supporter of your
> > ideas (I mean Enrico). However, I'm on the other side and do not like
> > this approach.
> 
> As I said in the cover-letter, this approach is the only one that I did
> found acceptable without being tied to some firmware description. If you
> have another more portable approach, I'm ok with that. But this
> solution should ideally work with pinctrl, gpio, clk, reset, phy, i2c,
> i2c-mux without rewriting half of the code. And also allows to easily
> swap the PCIe card to other slots/computer without having to modify the
> description.

My proposal is to use overlays that card provides with itself.
These are supported mechanisms by Linux kernel.

...

> > > > > static const struct property_entry ddr_clk_props[] = {
> > > > >         PROPERTY_ENTRY_U32("clock-frequency", 30000000),    
> > > >   
> > > > >         PROPERTY_ENTRY_U32("#clock-cells", 0),    
> > > > 
> > > > Why this is used?  
> > > 
> > > These props actually describes a fixed-clock properties. When adding
> > > fwnode support to clk framework, it was needed to add the
> > > equivalent of of_xlate() for fwnode (fwnode_xlate()). The number of
> > > cells used to describe a reference is still needed to do the
> > > translation using fwnode_property_get_reference_args() and give the
> > > correct arguments to fwnode_xlate().  
> > 
> > What you described is the programming (overkilled) point. But does hardware
> > needs this? I.o.w. does it make sense in the _hardware_ description?
> 
> This does not makes sense for the hardware of course. It also does not
> makes sense for the hardware to provide that in the device-tree though.

How it can be discovered and enumerated without a hint? And under hint
we may understand, in particular, the overlay blob.

> I actually think this should be only provided by the drivers but it
> might be difficult to parse the descriptions then (either DT or
> software_node), at least that's how it works right now.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 10/10] net: sfp: add support for fwnode

[Hans de Goede]

Hi,

On 2/23/22 16:23, Andrew Lunn wrote:
>> As Russell asked, I'm also really interested if someone has a solution
>> to reuse device-tree description (overlays ?) to describe such
>> hardware. However, the fact that CONFIG_OF isn't enabled on x86 config
>> seems a bit complicated on this side.
> 
> It does work, intel even used it for one of there tiny x86 SoCs. Maybe
> it was Newton?

IIRC those SoCs did not use standard EFI/ACPI though, but rather some
other special firmware, I think it was SFI ?  This is not so much about
the CPU architecture as it is about the firmware/bootloader <->
OS interface.

Note I'm not saying this can not be done with EFI/ACPI systems, but
I think it has never been tried.

Regards,

Hans

Re: [RFC 10/10] net: sfp: add support for fwnode

[Hans de Goede]

Hi,

On 2/23/22 16:23, Andrew Lunn wrote:
>> As Russell asked, I'm also really interested if someone has a solution
>> to reuse device-tree description (overlays ?) to describe such
>> hardware. However, the fact that CONFIG_OF isn't enabled on x86 config
>> seems a bit complicated on this side.
> 
> It does work, intel even used it for one of there tiny x86 SoCs. Maybe
> it was Newton?

IIRC those SoCs did not use standard EFI/ACPI though, but rather some
other special firmware, I think it was SFI ?  This is not so much about
the CPU architecture as it is about the firmware/bootloader <->
OS interface.

Note I'm not saying this can not be done with EFI/ACPI systems, but
I think it has never been tried.

Regards,

Hans

Re: [RFC 10/10] net: sfp: add support for fwnode

[Andy Shevchenko]

On Wed, Feb 23, 2022 at 04:27:33PM +0100, Hans de Goede wrote:
> On 2/23/22 16:23, Andrew Lunn wrote:
> >> As Russell asked, I'm also really interested if someone has a solution
> >> to reuse device-tree description (overlays ?) to describe such
> >> hardware. However, the fact that CONFIG_OF isn't enabled on x86 config
> >> seems a bit complicated on this side.
> > 
> > It does work, intel even used it for one of there tiny x86 SoCs. Maybe
> > it was Newton?
> 
> IIRC those SoCs did not use standard EFI/ACPI though, but rather some
> other special firmware, I think it was SFI ?  This is not so much about
> the CPU architecture as it is about the firmware/bootloader <->
> OS interface.

I think Andrew refers to Intel SoCs that are using OF. Those so far are
CE4xxx and SoFIA SoCs.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 10/10] net: sfp: add support for fwnode

[Clément Léger]

Le Wed, 23 Feb 2022 16:23:46 +0100,
Andrew Lunn <andrew@lunn.ch> a écrit :

> > As Russell asked, I'm also really interested if someone has a solution
> > to reuse device-tree description (overlays ?) to describe such
> > hardware. However, the fact that CONFIG_OF isn't enabled on x86 config
> > seems a bit complicated on this side.  
> 
> It does work, intel even used it for one of there tiny x86 SoCs. Maybe
> it was Newton? If you search around you can find maybe a Linux
> Plumbers presentation about DT and x86.

Oh yes, I know it works and I tried loading an overlay using CONFIG_OF
on a x86. Currently it does not works and generate a oops due to the
fact that the lack of "/" node is not handled and that the error path
has probably not been thoroughly tested. Adress remapping for PCI and
lack of PCI bus description might also be a bit cumbersome but maybe
this is the way to go.

I was saying a "bit complicated" as it is not often used and it would
require enabling CONFIG_OF to support this feature as well as allowing
loading overlays without any root node. But this is probably something
that is also achievable.

> 
> You can probably use a udev rule, triggered by the PCIe device ID to
> load the DT overlay.

Or maybe load the overlay from the PCIe driver ? This would also allow
to introduce some remapping of addresses (see below) inside the driver
maybe.

> 
> Do you actually need anything from the host other than PCIe? It sounds
> like this card is pretty self contained, so you won't need phandles
> pointing to the host i2c bus, or the hosts GPIOs? You only need
> phandles to your own i2c bus, your own GPIOs? That will make the
> overlay much simpler.

Yes, the device is almost self contained, only the IRQ needs to be
chained with the MSI.

> 
> 	Andrew



-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2871 bytes --]

On Wed, Feb 23, 2022 at 05:24:37PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 23, 2022 at 04:11:50PM +0100, Clément Léger wrote:
> > Le Wed, 23 Feb 2022 16:46:45 +0200,
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> > > And here is the problem. We have a few different resource providers
> > > (a.k.a. firmware interfaces) which we need to cope with.

> > Understood that but does adding fwnode support means it should work
> > as-is with both DT and ACPI ? ACPI code is still in place and only the
> > of part was converted. But maybe you expect the fwnode prot to be
> > conformant with ACPI.

> Not only me, I believe Mark also was against using pure DT approach on
> ACPI enabled platforms.

I'm not 100% clear on the context here (I did dig about a bit in the
thread on lore but it looks like there's some extra context here) but in
general I don't think there's any enthusiasm for trying to mix different
firmware interfaces on a single system.  Certainly in the case of ACPI
and DT they have substantial differences in system model and trying to
paper over those cracks and integrate the two is a route to trouble.
This doesn't look like it's trying to use a DT on an ACPI system though?

There's been some discussion on how to handle loadable descriptions for
things like FPGA but I don't recall it ever having got anywhere concrete
- I could have missed something.  Those are dynamic cases which are more
trouble though.  For something that's a PCI card it's not clear that we
can't just statically instanitate the devices from kernel code, that was
how the MFD subsystem started off although it's now primarily applied to
other applications.  That looks to be what's going on here?

There were separately some issues with people trying to create
completely swnode based enumeration mechanisms for things that required
totally independent code for handling swnodes which seemed very
concerning but it's not clear to me if that's what's going on here.

> > As I said in the cover-letter, this approach is the only one that I did
> > found acceptable without being tied to some firmware description. If you
> > have another more portable approach, I'm ok with that. But this
> > solution should ideally work with pinctrl, gpio, clk, reset, phy, i2c,
> > i2c-mux without rewriting half of the code. And also allows to easily
> > swap the PCIe card to other slots/computer without having to modify the
> > description.

> My proposal is to use overlays that card provides with itself.
> These are supported mechanisms by Linux kernel.

We have code for DT overlays in the kernel but it's not generically
available.  There's issues with binding onto the platform device tree,
though they're less of a problem with something like this where it seems
to be a separate card with no cross links.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Clément Léger]

Le Wed, 23 Feb 2022 17:41:30 +0000,
Mark Brown <broonie@kernel.org> a écrit :

> On Wed, Feb 23, 2022 at 05:24:37PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 23, 2022 at 04:11:50PM +0100, Clément Léger wrote:  
> > > Le Wed, 23 Feb 2022 16:46:45 +0200,
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :  
> 
> > > > And here is the problem. We have a few different resource providers
> > > > (a.k.a. firmware interfaces) which we need to cope with.  
> 
> > > Understood that but does adding fwnode support means it should work
> > > as-is with both DT and ACPI ? ACPI code is still in place and only the
> > > of part was converted. But maybe you expect the fwnode prot to be
> > > conformant with ACPI.  
> 
> > Not only me, I believe Mark also was against using pure DT approach on
> > ACPI enabled platforms.  
> 
> I'm not 100% clear on the context here (I did dig about a bit in the
> thread on lore but it looks like there's some extra context here) but in
> general I don't think there's any enthusiasm for trying to mix different
> firmware interfaces on a single system.  Certainly in the case of ACPI
> and DT they have substantial differences in system model and trying to
> paper over those cracks and integrate the two is a route to trouble.
> This doesn't look like it's trying to use a DT on an ACPI system though?

Ideally no, but it is a possibility mentionned by Andrew, use DT
overlays on an ACPI system. This series did not took this way (yet).
Andrew mentionned that it could potentially be done but judging by your
comment, i'm not sure you agree with that.

> 
> There's been some discussion on how to handle loadable descriptions for
> things like FPGA but I don't recall it ever having got anywhere concrete
> - I could have missed something.  Those are dynamic cases which are more
> trouble though.  For something that's a PCI card it's not clear that we
> can't just statically instanitate the devices from kernel code, that was
> how the MFD subsystem started off although it's now primarily applied to
> other applications.  That looks to be what's going on here?

Yes, in this series, I used the MFD susbsytems with mfd_cells. These
cells are attached with a swnode. Then, needed subsystems are
modified to use the fwnode API to be able to use them with
devices that have a swnode as a primary node.

> 
> There were separately some issues with people trying to create
> completely swnode based enumeration mechanisms for things that required
> totally independent code for handling swnodes which seemed very
> concerning but it's not clear to me if that's what's going on here.

The card is described entirely using swnode that in a MFD PCI
driver, everything is described statically. The "enumeration" is static
since all the devices are described in the driver and registered using
mfd_add_device() at probe time. Thus, I don't think it adds an
enumeration mechanism like you mention but I may be wrong.

> 
> > > As I said in the cover-letter, this approach is the only one that I did
> > > found acceptable without being tied to some firmware description. If you
> > > have another more portable approach, I'm ok with that. But this
> > > solution should ideally work with pinctrl, gpio, clk, reset, phy, i2c,
> > > i2c-mux without rewriting half of the code. And also allows to easily
> > > swap the PCIe card to other slots/computer without having to modify the
> > > description.  
> 
> > My proposal is to use overlays that card provides with itself.
> > These are supported mechanisms by Linux kernel.  
> 
> We have code for DT overlays in the kernel but it's not generically
> available.  There's issues with binding onto the platform device tree,
> though they're less of a problem with something like this where it seems
> to be a separate card with no cross links.

Indeed, the card does not have crosslinks with other devices and thus
it might be a solution to use a device-tree overlay (loaded from the
filesystem). But  I'm not sure if it's a good idea to do that on
a ACPI enabled platform.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2203 bytes --]

On Wed, Feb 23, 2022 at 06:59:27PM +0100, Clément Léger wrote:
> Mark Brown <broonie@kernel.org> a écrit :

> > This doesn't look like it's trying to use a DT on an ACPI system though?

> Ideally no, but it is a possibility mentionned by Andrew, use DT
> overlays on an ACPI system. This series did not took this way (yet).
> Andrew mentionned that it could potentially be done but judging by your
> comment, i'm not sure you agree with that.

That seems like it's opening a can of worms that might be best left
closed.

> > There's been some discussion on how to handle loadable descriptions for
> > things like FPGA but I don't recall it ever having got anywhere concrete
> > - I could have missed something.  Those are dynamic cases which are more
> > trouble though.  For something that's a PCI card it's not clear that we
> > can't just statically instanitate the devices from kernel code, that was
> > how the MFD subsystem started off although it's now primarily applied to
> > other applications.  That looks to be what's going on here?

> Yes, in this series, I used the MFD susbsytems with mfd_cells. These
> cells are attached with a swnode. Then, needed subsystems are
> modified to use the fwnode API to be able to use them with
> devices that have a swnode as a primary node.

Note that not all subsystems are going to be a good fit for fwnode, it's
concerning for the areas where ACPI and DT have substantially different
models like regulators.

> > There were separately some issues with people trying to create
> > completely swnode based enumeration mechanisms for things that required
> > totally independent code for handling swnodes which seemed very
> > concerning but it's not clear to me if that's what's going on here.

> The card is described entirely using swnode that in a MFD PCI
> driver, everything is described statically. The "enumeration" is static
> since all the devices are described in the driver and registered using
> mfd_add_device() at probe time. Thus, I don't think it adds an
> enumeration mechanism like you mention but I may be wrong.

This was all on the side parsing the swnodes rather than injecting the
data.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Andy Shevchenko]

On Wed, Feb 23, 2022 at 05:41:30PM +0000, Mark Brown wrote:
> On Wed, Feb 23, 2022 at 05:24:37PM +0200, Andy Shevchenko wrote:

...

> There were separately some issues with people trying to create
> completely swnode based enumeration mechanisms for things that required
> totally independent code for handling swnodes which seemed very
> concerning but it's not clear to me if that's what's going on here.

This is the case IIUC.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Clément Léger]

Hi,

As stated at the beginning of the cover letter, the PCIe card I'm
working on uses a lan9662 SoC. This card is meant to be used an
ethernet switch with 2 x RJ45 ports and 2 x 10G SFPs. The lan966x SoCs
can be used in two different ways:

 - It can run Linux by itself, on ARM64 cores included in the SoC. This
   use-case of the lan966x is currently being upstreamed, using a
   traditional Device Tree representation of the lan996x HW blocks [1]
   A number of drivers for the different IPs of the SoC have already
   been merged in upstream Linux.

 - It can be used as a PCIe endpoint, connected to a separate platform
   that acts as the PCIe root complex. In this case, all the devices
   that are embedded on this SoC are exposed through PCIe BARs and the
   ARM64 cores of the SoC are not used. Since this is a PCIe card, it
   can be plugged on any platform, of any architecture supporting PCIe.

The goal of this effort is to enable this second use-case, while
allowing the re-use of the existing drivers for the different devices
part of the SoC.

Following a first round of discussion, here are some clarifications on
what problem this series is trying to solve and what are the possible
choices to support this use-case.

Here is the list of devices that are exposed and needed to make this
card work as an ethernet switch:
 - lan966x-switch
 - reset-microchip-sparx5
 - lan966x_serdes
 - reset-microchip-lan966x-phy
 - mdio-mscc-miim
 - pinctrl-lan966x
 - atmel-flexcom
 - i2c-at91
 - i2c-mux
 - i2c-mux-pinctrl
 - sfp
 - clk-lan966x

All the devices on this card are "self-contained" and do not require
cross-links with devices that are on the host (except to demux IRQ but
this is something easy to do). These drivers already exists and are
using of_* API to register controllers, get properties and so on.

The challenge we're trying to solve is how can the PCI driver for this
card re-use the existing drivers, and using which hardware
representation to instantiate all those drivers.

Although this series only contained the modifications for the I2C
subsystem all the subsystems that are used or needed by the previously
listed driver have also been modified to have support for fwnode. This
includes the following subsystems:
- reset
- clk
- pinctrl
- syscon
- gpio
- pinctrl
- phy
- mdio
- i2c

The first feedback on this series does not seems to reach a consensus
(to say the least) on how to do it cleanly so here is a recap of the
possible solutions, either brought by this series or mentioned by
contributors:

1) Describe the card statically using swnode

This is the approach that was taken by this series. The devices are
described using the MFD subsystem with mfd_cells. These cells are
attached with a swnode which will be used as a primary node in place of
ACPI or OF description. This means that the device description
(properties and references) is conveyed entirely in the swnode. In order
to make these swnode usable with existing OF based subsystems, the
fwnode API can be used in needed subsystems.

Pros:
 - Self-contained in the driver.
 - Will work on all platforms no matter the firmware description.
 - Makes the subsystems less OF-centric.

Cons:
 - Modifications are required in subsystems to support fwnode
   (mitigated by the fact it makes to subsystems less OF-centric).
 - swnode are not meant to be used entirely as primary nodes.
 - Specifications for both ACPI and OF must be handled if using fwnode
   API.

2) Use SSDT overlays

Andy mentioned that SSDT overlays could be used. This overlay should
match the exact configuration that is used (ie correct PCIe bus/port
etc). It requires the user to write/modify/compile a .asl file and load
it using either EFI vars, custom initrd or via configfs. The existing
drivers would also need more modifications to work with ACPI. Some of
them might even be harder (if not possible) to use since there is no
ACPI support for the subsystems they are using .

Pros:
 - Can't really find any for this one

Cons:
 - Not all needed subsystems have appropriate ACPI bindings/support
   (reset, clk, pinctrl, syscon).
 - Difficult to setup for the user (modify/compile/load .aml file).
 - Not portable between machines, as the SSDT overlay need to be
   different depending on how the PCI device is connected to the
   platform.

3) Use device-tree overlays

This solution was proposed by Andrew and could potentially allows to
keep all the existing device-tree infrastructure and helpers. A
device-tree overlay could be loaded by the driver and applied using
of_overlay_fdt_apply(). There is some glue to make this work but it
could potentially be possible. Mark have raised some warnings about
using such device-tree overlays on an ACPI enabled platform.

Pros:
 - Reuse all the existing OF infrastructure, no modifications at all on
   drivers and subsystems.
 - Could potentially lead to designing a generic driver for PCI devices
   that uses a composition of other drivers.

Cons:
 - Might not the best idea to mix it with ACPI.
 - Needs CONFIG_OF, which typically isn't enabled today on most x86
   platforms.
 - Loading DT overlays on non-DT platforms is not currently working. It
   can be addressed, but it's not necessarily immediate.

My preferred solutions would be swnode or device-tree overlays but
since there to is no consensus on how to add this support, how
can we go on with this series ?

Thanks,

[1]
https://lore.kernel.org/linux-arm-kernel/20220210123704.477826-1-michael@walle.cc/

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Hans de Goede]

Hi Clément,

On 2/24/22 15:40, Clément Léger wrote:
> Hi,
> 
> As stated at the beginning of the cover letter, the PCIe card I'm
> working on uses a lan9662 SoC. This card is meant to be used an
> ethernet switch with 2 x RJ45 ports and 2 x 10G SFPs. The lan966x SoCs
> can be used in two different ways:
> 
>  - It can run Linux by itself, on ARM64 cores included in the SoC. This
>    use-case of the lan966x is currently being upstreamed, using a
>    traditional Device Tree representation of the lan996x HW blocks [1]
>    A number of drivers for the different IPs of the SoC have already
>    been merged in upstream Linux.
> 
>  - It can be used as a PCIe endpoint, connected to a separate platform
>    that acts as the PCIe root complex. In this case, all the devices
>    that are embedded on this SoC are exposed through PCIe BARs and the
>    ARM64 cores of the SoC are not used. Since this is a PCIe card, it
>    can be plugged on any platform, of any architecture supporting PCIe.
> 
> The goal of this effort is to enable this second use-case, while
> allowing the re-use of the existing drivers for the different devices
> part of the SoC.
> 
> Following a first round of discussion, here are some clarifications on
> what problem this series is trying to solve and what are the possible
> choices to support this use-case.
> 
> Here is the list of devices that are exposed and needed to make this
> card work as an ethernet switch:
>  - lan966x-switch
>  - reset-microchip-sparx5
>  - lan966x_serdes
>  - reset-microchip-lan966x-phy
>  - mdio-mscc-miim
>  - pinctrl-lan966x
>  - atmel-flexcom
>  - i2c-at91
>  - i2c-mux
>  - i2c-mux-pinctrl
>  - sfp
>  - clk-lan966x
> 
> All the devices on this card are "self-contained" and do not require
> cross-links with devices that are on the host (except to demux IRQ but
> this is something easy to do). These drivers already exists and are
> using of_* API to register controllers, get properties and so on.
> 
> The challenge we're trying to solve is how can the PCI driver for this
> card re-use the existing drivers, and using which hardware
> representation to instantiate all those drivers.
> 
> Although this series only contained the modifications for the I2C
> subsystem all the subsystems that are used or needed by the previously
> listed driver have also been modified to have support for fwnode. This
> includes the following subsystems:
> - reset
> - clk
> - pinctrl
> - syscon
> - gpio
> - pinctrl
> - phy
> - mdio
> - i2c
> 
> The first feedback on this series does not seems to reach a consensus
> (to say the least) on how to do it cleanly so here is a recap of the
> possible solutions, either brought by this series or mentioned by
> contributors:
> 
> 1) Describe the card statically using swnode
> 
> This is the approach that was taken by this series. The devices are
> described using the MFD subsystem with mfd_cells. These cells are
> attached with a swnode which will be used as a primary node in place of
> ACPI or OF description. This means that the device description
> (properties and references) is conveyed entirely in the swnode. In order
> to make these swnode usable with existing OF based subsystems, the
> fwnode API can be used in needed subsystems.
> 
> Pros:
>  - Self-contained in the driver.
>  - Will work on all platforms no matter the firmware description.
>  - Makes the subsystems less OF-centric.
> 
> Cons:
>  - Modifications are required in subsystems to support fwnode
>    (mitigated by the fact it makes to subsystems less OF-centric).
>  - swnode are not meant to be used entirely as primary nodes.
>  - Specifications for both ACPI and OF must be handled if using fwnode
>    API.
> 
> 2) Use SSDT overlays
> 
> Andy mentioned that SSDT overlays could be used. This overlay should
> match the exact configuration that is used (ie correct PCIe bus/port
> etc). It requires the user to write/modify/compile a .asl file and load
> it using either EFI vars, custom initrd or via configfs. The existing
> drivers would also need more modifications to work with ACPI. Some of
> them might even be harder (if not possible) to use since there is no
> ACPI support for the subsystems they are using .
> 
> Pros:
>  - Can't really find any for this one
> 
> Cons:
>  - Not all needed subsystems have appropriate ACPI bindings/support
>    (reset, clk, pinctrl, syscon).
>  - Difficult to setup for the user (modify/compile/load .aml file).
>  - Not portable between machines, as the SSDT overlay need to be
>    different depending on how the PCI device is connected to the
>    platform.
> 
> 3) Use device-tree overlays
> 
> This solution was proposed by Andrew and could potentially allows to
> keep all the existing device-tree infrastructure and helpers. A
> device-tree overlay could be loaded by the driver and applied using
> of_overlay_fdt_apply(). There is some glue to make this work but it
> could potentially be possible. Mark have raised some warnings about
> using such device-tree overlays on an ACPI enabled platform.
> 
> Pros:
>  - Reuse all the existing OF infrastructure, no modifications at all on
>    drivers and subsystems.
>  - Could potentially lead to designing a generic driver for PCI devices
>    that uses a composition of other drivers.
> 
> Cons:
>  - Might not the best idea to mix it with ACPI.
>  - Needs CONFIG_OF, which typically isn't enabled today on most x86
>    platforms.
>  - Loading DT overlays on non-DT platforms is not currently working. It
>    can be addressed, but it's not necessarily immediate.
> 
> My preferred solutions would be swnode or device-tree overlays but
> since there to is no consensus on how to add this support, how
> can we go on with this series ?

FWIW I think that the convert subsystems + drivers to use the fwnode
abstraction layer + use swnode-s approach makes sense. For a bunch of
x86/ACPI stuff like Type-C muxing/controllers/routing but also MIPI
cameras we have already been moving in that direction since sometimes
a bunch of info seems to be hardcoded in Windows drivers rather then
"spelled out" in the ACPI tables so from the x86 side we are seeing
a need to have platform glue code which replaces the hardcoding on
the Windows side and we have been using the fwnode abstraction +
swnodes for this, so that we can keep using the standard Linux
abstractions/subsystems for this.

As Mark already mentioned the regulator subsystem has shown to
be a bit problematic here, but you don't seem to need that?

Your i2c subsys patches looked reasonable to me. IMHO an important
thing missing to give you some advice whether to try 1. or 3. first
is how well / clean the move to the fwnode abstractions would work
for the other subsystems.

Have you already converted other subsystems and if yes, can you
give us a pointer to a branch somewhere with the conversion for
other subsystems ?

Regards,

Hans

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

On Thu, Feb 24, 2022 at 03:58:04PM +0100, Hans de Goede wrote:

> As Mark already mentioned the regulator subsystem has shown to
> be a bit problematic here, but you don't seem to need that?

I believe clocks are also potentially problematic for similar reasons
(ACPI wants to handle those as part of the device level power management
and/or should have native abstractions for them, and I think we also
have board file provisions that work well for them and are less error
prone than translating into an abstract data structure).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Clément Léger]

Le Thu, 24 Feb 2022 15:58:04 +0100,
Hans de Goede <hdegoede@redhat.com> a écrit :

[...]

> >    can be addressed, but it's not necessarily immediate.
> > 
> > My preferred solutions would be swnode or device-tree overlays but
> > since there to is no consensus on how to add this support, how
> > can we go on with this series ?  
> 
> FWIW I think that the convert subsystems + drivers to use the fwnode
> abstraction layer + use swnode-s approach makes sense. For a bunch of
> x86/ACPI stuff like Type-C muxing/controllers/routing but also MIPI
> cameras we have already been moving in that direction since sometimes
> a bunch of info seems to be hardcoded in Windows drivers rather then
> "spelled out" in the ACPI tables so from the x86 side we are seeing
> a need to have platform glue code which replaces the hardcoding on
> the Windows side and we have been using the fwnode abstraction +
> swnodes for this, so that we can keep using the standard Linux
> abstractions/subsystems for this.
> 
> As Mark already mentioned the regulator subsystem has shown to
> be a bit problematic here, but you don't seem to need that?

Hi Hans,

Indeed, I don't need this subsystem. However, I'm still not clear why
this subsystem in particular is problematic. Just so that I can
recognize the other subsystems with the same pattern, could you explain
me why it is problematic ? 

> 
> Your i2c subsys patches looked reasonable to me. IMHO an important
> thing missing to give you some advice whether to try 1. or 3. first
> is how well / clean the move to the fwnode abstractions would work
> for the other subsystems.

Actually, I did the conversion for pinctrl, gpios, i2c, reset, clk,
syscon, mdio but did not factorized all the of code on top of fwnode
adaptation. I did it completely for mdio and reset subsystems. Porting
them to fwnode was rather straightforward, and almost all the of_* API
now have a fwnode_* variant.

While porting them to fwnode, I mainly had to modify the "register" and
the "get" interface of these subsystems. I did not touched the
enumeration part if we can call it like this and thus all the
CLK_OF_DECLARE() related stuff is left untouched.

> 
> Have you already converted other subsystems and if yes, can you
> give us a pointer to a branch somewhere with the conversion for
> other subsystems ?

All the preliminary work I did is available at the link at [1]. But as I
said, I did not converted completely all the subsystems, only reset [2]
(for which I tried to convert all the drivers and fatorized OF on top
of fwnode functions) and mdio [3] which proved to be easily portable.

I also modified the clk framework [4] but did not went to the complete
factorization of it. I converted the fixed-clk driver to see how well
it could be done. Biggest difficulty is to keep of_xlate() and
fwnode_xlate() (if we want to do so) to avoid modifying all drivers
(even though not a lot of them implements custom of_xlate() functions).

If backward compatibility is really needed, it can potentially be done,
at the cost of keeping of_xlate() member and by converting the fwnode
stuff to OF world (which is easily doable).

Conversion to fwnode API actually proved to be rather straightforward
except for some specific subsystem (syscon) which I'm not quite happy
with the outcome, but again, I wanted the community feedback before
going further in this way so there is room for improvement.

Regards,

[1] https://github.com/clementleger/linux/tree/fwnode_support
[2] https://github.com/clementleger/linux/tree/fwnode_reset
[3] https://github.com/clementleger/linux/tree/fwnode_mdio
[4] https://github.com/clementleger/linux/tree/fwnode_clk

> 
> Regards,
> 
> Hans


-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2532 bytes --]

On Thu, Feb 24, 2022 at 05:42:05PM +0100, Clément Léger wrote:
> Hans de Goede <hdegoede@redhat.com> a écrit :

> > As Mark already mentioned the regulator subsystem has shown to
> > be a bit problematic here, but you don't seem to need that?

> Indeed, I don't need this subsystem. However, I'm still not clear why
> this subsystem in particular is problematic. Just so that I can
> recognize the other subsystems with the same pattern, could you explain
> me why it is problematic ? 

ACPI has a strong concept of how power supply (and general critical
resources) for devices should be described by firmware which is very
different to that which DT as it is used in Linux has, confusing that
model would make it much harder for generic OSs to work with generic
ACPI systems, and makes it much easier to create unfortunate interactions
between bits of software expecting ACPI models and bits of software
expecting DT models for dealing with a device.  Potentially we could
even run into issues with new versions of Linux if there's timing or
other changes.  If Linux starts parsing the core DT bindings for
regulators on ACPI systems then that makes it more likely that system
integrators who are primarily interested in Linux will produce firmwares
that run into these issues, perhaps unintentionally through a "this just
happens to work" process.

As a result of this we very much do not want to have the regulator code
parsing DT bindings using the fwnode APIs since that makes it much
easier for us to end up with a situation where we are interpreting _DSD
versions of regulator bindings and ending up with people making systems
that rely on that.  Instead the regulator API is intentional about which
platform description interfaces it is using.  We could potentially have
something that is specific to swnode and won't work with general fwnode
but it's hard to see any advantages for this over the board file based
mechanism we have already, swnode offers less error detection (typoing
field names is harder to spot) and the data marshalling takes more code.

fwnode is great for things like properties for leaf devices since those
are basically a free for all on ACPI systems, it allows us to quickly
and simply apply the work done defining bindings for DT to ACPI systems
in a way that's compatible with how APCI wants to work.  It's also good
for cross device bindings that are considered out of scope for ACPI,
though a bit of caution is needed determining when that's the case.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Sakari Ailus]

Hi Mark,

On Thu, Feb 24, 2022 at 03:33:12PM +0000, Mark Brown wrote:
> On Thu, Feb 24, 2022 at 03:58:04PM +0100, Hans de Goede wrote:
> 
> > As Mark already mentioned the regulator subsystem has shown to
> > be a bit problematic here, but you don't seem to need that?
> 
> I believe clocks are also potentially problematic for similar reasons
> (ACPI wants to handle those as part of the device level power management
> and/or should have native abstractions for them, and I think we also
> have board file provisions that work well for them and are less error
> prone than translating into an abstract data structure).

Per ACPI spec, what corresponds to clocks and regulators in DT is handled
through power resources. This is generally how things work in ACPI based
systems but there are cases out there where regulators and/or clocks are
exposed to software directly. This concerns e.g. camera sensors and lens
voice coils on some systems while rest of the devices in the system are
powered on and off the usual ACPI way.

So controlling regulators or clocks directly on an ACPI based system
wouldn't be exactly something new. All you need to do in that case is to
ensure that there's exactly one way regulators and clocks are controlled
for a given device. For software nodes this is a non-issue.

This does have the limitation that a clock or a regulator is either
controlled through power resources or relevant drivers, but that tends to
be the case in practice. But I presume it wouldn't be different with board
files.

-- 
Sakari Ailus

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Alexandre Belloni]

On 24/02/2022 20:14:51+0200, Sakari Ailus wrote:
> Hi Mark,
> 
> On Thu, Feb 24, 2022 at 03:33:12PM +0000, Mark Brown wrote:
> > On Thu, Feb 24, 2022 at 03:58:04PM +0100, Hans de Goede wrote:
> > 
> > > As Mark already mentioned the regulator subsystem has shown to
> > > be a bit problematic here, but you don't seem to need that?
> > 
> > I believe clocks are also potentially problematic for similar reasons
> > (ACPI wants to handle those as part of the device level power management
> > and/or should have native abstractions for them, and I think we also
> > have board file provisions that work well for them and are less error
> > prone than translating into an abstract data structure).
> 
> Per ACPI spec, what corresponds to clocks and regulators in DT is handled
> through power resources. This is generally how things work in ACPI based
> systems but there are cases out there where regulators and/or clocks are
> exposed to software directly. This concerns e.g. camera sensors and lens
> voice coils on some systems while rest of the devices in the system are
> powered on and off the usual ACPI way.
> 
> So controlling regulators or clocks directly on an ACPI based system
> wouldn't be exactly something new. All you need to do in that case is to
> ensure that there's exactly one way regulators and clocks are controlled
> for a given device. For software nodes this is a non-issue.
> 
> This does have the limitation that a clock or a regulator is either
> controlled through power resources or relevant drivers, but that tends to
> be the case in practice. But I presume it wouldn't be different with board
> files.
> 

In this use case, we don't need anything that is actually described in
ACPI as all the clocks we need to control are on the device itself so I
don't think this is relevant here.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

On Thu, Feb 24, 2022 at 08:14:51PM +0200, Sakari Ailus wrote:
> On Thu, Feb 24, 2022 at 03:33:12PM +0000, Mark Brown wrote:

> > I believe clocks are also potentially problematic for similar reasons
> > (ACPI wants to handle those as part of the device level power management
> > and/or should have native abstractions for them, and I think we also
> > have board file provisions that work well for them and are less error
> > prone than translating into an abstract data structure).

> Per ACPI spec, what corresponds to clocks and regulators in DT is handled
> through power resources. This is generally how things work in ACPI based
> systems but there are cases out there where regulators and/or clocks are
> exposed to software directly. This concerns e.g. camera sensors and lens
> voice coils on some systems while rest of the devices in the system are
> powered on and off the usual ACPI way.

But note crucially that when these things are controlled by the OS they
are enumerated via some custom mechanism that is *not* _DSD properties -
the issue is with the firmware interface, not with using the relevant
kernel APIs in the client or provider devices.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 406 bytes --]

On Thu, Feb 24, 2022 at 07:39:52PM +0100, Alexandre Belloni wrote:

> In this use case, we don't need anything that is actually described in
> ACPI as all the clocks we need to control are on the device itself so I
> don't think this is relevant here.

You should be able to support this case, the concern is if it's done in
a way that would allow things to be parsed out of the firmware by other
systems.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Clément Léger]

Le Thu, 24 Feb 2022 17:26:02 +0000,
Mark Brown <broonie@kernel.org> a écrit :

> On Thu, Feb 24, 2022 at 05:42:05PM +0100, Clément Léger wrote:
> > Hans de Goede <hdegoede@redhat.com> a écrit :  
> 
> > > As Mark already mentioned the regulator subsystem has shown to
> > > be a bit problematic here, but you don't seem to need that?  
> 
> > Indeed, I don't need this subsystem. However, I'm still not clear why
> > this subsystem in particular is problematic. Just so that I can
> > recognize the other subsystems with the same pattern, could you explain
> > me why it is problematic ?   
> 
> ACPI has a strong concept of how power supply (and general critical
> resources) for devices should be described by firmware which is very
> different to that which DT as it is used in Linux has, confusing that
> model would make it much harder for generic OSs to work with generic
> ACPI systems, and makes it much easier to create unfortunate interactions
> between bits of software expecting ACPI models and bits of software
> expecting DT models for dealing with a device.  Potentially we could
> even run into issues with new versions of Linux if there's timing or
> other changes.  If Linux starts parsing the core DT bindings for
> regulators on ACPI systems then that makes it more likely that system
> integrators who are primarily interested in Linux will produce firmwares
> that run into these issues, perhaps unintentionally through a "this just
> happens to work" process.

Ok that's way more clear.

> 
> As a result of this we very much do not want to have the regulator code
> parsing DT bindings using the fwnode APIs since that makes it much
> easier for us to end up with a situation where we are interpreting _DSD
> versions of regulator bindings and ending up with people making systems
> that rely on that.  Instead the regulator API is intentional about which
> platform description interfaces it is using.  We could potentially have
> something that is specific to swnode and won't work with general fwnode
> but it's hard to see any advantages for this over the board file based
> mechanism we have already, swnode offers less error detection (typoing
> field names is harder to spot) and the data marshalling takes more code.

Instead of making it specific for swnode, could we make it instead non
working for acpi nodes ? Thus, the parsing would work only for swnode
and device_node, not allowing to use the fwnode support with acpi for
such subsystems (not talking about regulators here).

If switching to board file based mechanism, this means that all drivers
that are used by the PCIe card will have to be modified to support this
mechanism.

> 
> fwnode is great for things like properties for leaf devices since those
> are basically a free for all on ACPI systems, it allows us to quickly
> and simply apply the work done defining bindings for DT to ACPI systems
> in a way that's compatible with how APCI wants to work.  It's also good
> for cross device bindings that are considered out of scope for ACPI,
> though a bit of caution is needed determining when that's the case.

Ok got it, thanks for the in-depth explanations.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

On Thu, Mar 03, 2022 at 09:48:40AM +0100, Clément Léger wrote:

> Instead of making it specific for swnode, could we make it instead non
> working for acpi nodes ? Thus, the parsing would work only for swnode
> and device_node, not allowing to use the fwnode support with acpi for
> such subsystems (not talking about regulators here).

*Potentially*.  I'd need to see the code, it doesn't fill me with great
joy and there are disadvantages in terms of input validation but that
does address the main issue and perhaps the implementation would be
nicer than I'm immediately imagining.

> If switching to board file based mechanism, this means that all drivers
> that are used by the PCIe card will have to be modified to support this
> mechanism.

You should be able to mix the use of board file regulator descriptions
with other descriptions for other bits of the hardware.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Clément Léger]

Le Thu, 24 Feb 2022 15:40:40 +0100,
Clément Léger <clement.leger@bootlin.com> a écrit :

> Hi,
> 
> As stated at the beginning of the cover letter, the PCIe card I'm
> working on uses a lan9662 SoC. This card is meant to be used an
> ethernet switch with 2 x RJ45 ports and 2 x 10G SFPs. The lan966x SoCs
> can be used in two different ways:
> 
>  - It can run Linux by itself, on ARM64 cores included in the SoC. This
>    use-case of the lan966x is currently being upstreamed, using a
>    traditional Device Tree representation of the lan996x HW blocks [1]
>    A number of drivers for the different IPs of the SoC have already
>    been merged in upstream Linux.
> 
>  - It can be used as a PCIe endpoint, connected to a separate platform
>    that acts as the PCIe root complex. In this case, all the devices
>    that are embedded on this SoC are exposed through PCIe BARs and the
>    ARM64 cores of the SoC are not used. Since this is a PCIe card, it
>    can be plugged on any platform, of any architecture supporting PCIe.
> 
> The goal of this effort is to enable this second use-case, while
> allowing the re-use of the existing drivers for the different devices
> part of the SoC.
> 
> Following a first round of discussion, here are some clarifications on
> what problem this series is trying to solve and what are the possible
> choices to support this use-case.
> 
> Here is the list of devices that are exposed and needed to make this
> card work as an ethernet switch:
>  - lan966x-switch
>  - reset-microchip-sparx5
>  - lan966x_serdes
>  - reset-microchip-lan966x-phy
>  - mdio-mscc-miim
>  - pinctrl-lan966x
>  - atmel-flexcom
>  - i2c-at91
>  - i2c-mux
>  - i2c-mux-pinctrl
>  - sfp
>  - clk-lan966x
> 
> All the devices on this card are "self-contained" and do not require
> cross-links with devices that are on the host (except to demux IRQ but
> this is something easy to do). These drivers already exists and are
> using of_* API to register controllers, get properties and so on.
> 
> The challenge we're trying to solve is how can the PCI driver for this
> card re-use the existing drivers, and using which hardware
> representation to instantiate all those drivers.
> 
> Although this series only contained the modifications for the I2C
> subsystem all the subsystems that are used or needed by the previously
> listed driver have also been modified to have support for fwnode. This
> includes the following subsystems:
> - reset
> - clk
> - pinctrl
> - syscon
> - gpio
> - pinctrl
> - phy
> - mdio
> - i2c
> 
> The first feedback on this series does not seems to reach a consensus
> (to say the least) on how to do it cleanly so here is a recap of the
> possible solutions, either brought by this series or mentioned by
> contributors:
> 
> 1) Describe the card statically using swnode
> 
> This is the approach that was taken by this series. The devices are
> described using the MFD subsystem with mfd_cells. These cells are
> attached with a swnode which will be used as a primary node in place of
> ACPI or OF description. This means that the device description
> (properties and references) is conveyed entirely in the swnode. In order
> to make these swnode usable with existing OF based subsystems, the
> fwnode API can be used in needed subsystems.
> 
> Pros:
>  - Self-contained in the driver.
>  - Will work on all platforms no matter the firmware description.
>  - Makes the subsystems less OF-centric.
> 
> Cons:
>  - Modifications are required in subsystems to support fwnode
>    (mitigated by the fact it makes to subsystems less OF-centric).
>  - swnode are not meant to be used entirely as primary nodes.
>  - Specifications for both ACPI and OF must be handled if using fwnode
>    API.
> 
> 2) Use SSDT overlays
> 
> Andy mentioned that SSDT overlays could be used. This overlay should
> match the exact configuration that is used (ie correct PCIe bus/port
> etc). It requires the user to write/modify/compile a .asl file and load
> it using either EFI vars, custom initrd or via configfs. The existing
> drivers would also need more modifications to work with ACPI. Some of
> them might even be harder (if not possible) to use since there is no
> ACPI support for the subsystems they are using .
> 
> Pros:
>  - Can't really find any for this one
> 
> Cons:
>  - Not all needed subsystems have appropriate ACPI bindings/support
>    (reset, clk, pinctrl, syscon).
>  - Difficult to setup for the user (modify/compile/load .aml file).
>  - Not portable between machines, as the SSDT overlay need to be
>    different depending on how the PCI device is connected to the
>    platform.
> 
> 3) Use device-tree overlays
> 
> This solution was proposed by Andrew and could potentially allows to
> keep all the existing device-tree infrastructure and helpers. A
> device-tree overlay could be loaded by the driver and applied using
> of_overlay_fdt_apply(). There is some glue to make this work but it
> could potentially be possible. Mark have raised some warnings about
> using such device-tree overlays on an ACPI enabled platform.
> 
> Pros:
>  - Reuse all the existing OF infrastructure, no modifications at all on
>    drivers and subsystems.
>  - Could potentially lead to designing a generic driver for PCI devices
>    that uses a composition of other drivers.
> 
> Cons:
>  - Might not the best idea to mix it with ACPI.
>  - Needs CONFIG_OF, which typically isn't enabled today on most x86
>    platforms.
>  - Loading DT overlays on non-DT platforms is not currently working. It
>    can be addressed, but it's not necessarily immediate.
> 
> My preferred solutions would be swnode or device-tree overlays but
> since there to is no consensus on how to add this support, how
> can we go on with this series ?
> 
> Thanks,
> 
> [1]
> https://lore.kernel.org/linux-arm-kernel/20220210123704.477826-1-michael@walle.cc/
> 

Does anybody have some other advices or recommendation regarding
this RFC ? It would be nice to have more feedback on the solution that
might e preferred to support this use-case.

Thanks,


-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com