Message-ID: <YhPOxL++yhNHh+xH@smile.fi.intel.com>
Date:   Mon, 21 Feb 2022 19:41:24 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Clément Léger <clement.leger@...tlin.com>
Cc:     Daniel Scally <djrscally@...il.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Wolfram Sang <wsa@...nel.org>, Peter Rosin <peda@...ntia.se>,
        Russell King <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
        linux-acpi@...r.kernel.org, linux-i2c@...r.kernel.org,
        netdev@...r.kernel.org,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>
Subject: Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

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

Powered by blists - more mailing lists