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
> 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
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 <[email protected]>
---
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
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 <[email protected]>
---
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", ®)) {
- of_node_put(mux_node);
+ if (!fwnode_property_read_u32(mux_node, "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", ®);
+ fwnode_for_each_child_node(mux_node, child) {
+ ret = fwnode_property_read_u32(child, "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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
On Tue, Feb 22, 2022 at 5:57 AM Andrew Lunn <[email protected]> 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
> > 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
Le Mon, 21 Feb 2022 19:55:25 +0200,
Andy Shevchenko <[email protected]> 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
On Tue, Feb 22, 2022 at 9:57 AM Andrew Lunn <[email protected]> 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
On Tue, Feb 22, 2022 at 9:47 AM Clément Léger <[email protected]> wrote:
> Le Tue, 22 Feb 2022 09:33:32 +0100,
> Andy Shevchenko <[email protected]> a écrit :
> > On Tue, Feb 22, 2022 at 9:24 AM Clément Léger <[email protected]> wrote:
> > > Le Mon, 21 Feb 2022 19:46:12 +0200,
> > > Andy Shevchenko <[email protected]> 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
Le Tue, 22 Feb 2022 10:24:13 +0100,
Andy Shevchenko <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]> 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
Le Tue, 22 Feb 2022 11:20:54 +0100,
Greg Kroah-Hartman <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]> 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!
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 <[email protected]> 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.
Le Wed, 23 Feb 2022 16:46:45 +0200,
Andy Shevchenko <[email protected]> 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
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
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 <[email protected]> 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
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 <[email protected]> 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.
On Wed, Feb 23, 2022 at 06:59:27PM +0100, Cl?ment L?ger wrote:
> Mark Brown <[email protected]> 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.
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 <[email protected]> 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
Le Wed, 23 Feb 2022 14:39:27 +0100,
Hans de Goede <[email protected]> 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
> 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
Le Wed, 23 Feb 2022 16:23:46 +0100,
Andrew Lunn <[email protected]> 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
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
> 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
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
Le Wed, 23 Feb 2022 17:41:30 +0000,
Mark Brown <[email protected]> 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 <[email protected]> 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
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
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/[email protected]/
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
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
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).
Le Thu, 24 Feb 2022 15:58:04 +0100,
Hans de Goede <[email protected]> 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
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
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.
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
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.
On Thu, Feb 24, 2022 at 05:42:05PM +0100, Cl?ment L?ger wrote:
> Hans de Goede <[email protected]> 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.
Le Thu, 24 Feb 2022 17:26:02 +0000,
Mark Brown <[email protected]> a écrit :
> On Thu, Feb 24, 2022 at 05:42:05PM +0100, Clément Léger wrote:
> > Hans de Goede <[email protected]> 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
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.
Le Thu, 24 Feb 2022 15:40:40 +0100,
Clément Léger <[email protected]> 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/[email protected]/
>
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