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

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

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

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

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

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

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

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

Why this is used?

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

-- 
With Best Regards,
Andy Shevchenko

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[...]

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

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

[...]

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

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

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

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

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

Ok, thanks for the info.

[...]

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

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

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

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

...

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

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

...

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

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

...

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

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

...

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

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

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

-- 
With Best Regards,
Andy Shevchenko

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> 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

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

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

I'm not sure we want another one.

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

-- 
With Best Regards,
Andy Shevchenko

> > 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

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

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

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

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

-- 
With Best Regards,
Andy Shevchenko

Hi,

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

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

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

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

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

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

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

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

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

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

1) Describe the card statically using swnode

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

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

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

2) Use SSDT overlays

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

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

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

3) Use device-tree overlays

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

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

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

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

Thanks,

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

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

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).

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 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.

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

[...]

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

Hi Hans,

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

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

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

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

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

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

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

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

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

Regards,

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

> 
> Regards,
> 
> Hans


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

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

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

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

Thanks,


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