| Message ID | 20220221162652.103834-1-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | add support for fwnode in i2c mux system and sfp | expand |
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 >
> 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.
> > 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
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/
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/
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.
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.
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.
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.
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/
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 <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
On Thu, Feb 24, 2022 at 05:42:05PM +0100, Clément Léger wrote: > Hans de Goede <hdegoede@redhat.com> a écrit : > > As Mark already mentioned the regulator subsystem has shown to > > be a bit problematic here, but you don't seem to need that? > Indeed, I don't need this subsystem. However, I'm still not clear why > this subsystem in particular is problematic. Just so that I can > recognize the other subsystems with the same pattern, could you explain > me why it is problematic ? ACPI has a strong concept of how power supply (and general critical resources) for devices should be described by firmware which is very different to that which DT as it is used in Linux has, confusing that model would make it much harder for generic OSs to work with generic ACPI systems, and makes it much easier to create unfortunate interactions between bits of software expecting ACPI models and bits of software expecting DT models for dealing with a device. Potentially we could even run into issues with new versions of Linux if there's timing or other changes. If Linux starts parsing the core DT bindings for regulators on ACPI systems then that makes it more likely that system integrators who are primarily interested in Linux will produce firmwares that run into these issues, perhaps unintentionally through a "this just happens to work" process. As a result of this we very much do not want to have the regulator code parsing DT bindings using the fwnode APIs since that makes it much easier for us to end up with a situation where we are interpreting _DSD versions of regulator bindings and ending up with people making systems that rely on that. Instead the regulator API is intentional about which platform description interfaces it is using. We could potentially have something that is specific to swnode and won't work with general fwnode but it's hard to see any advantages for this over the board file based mechanism we have already, swnode offers less error detection (typoing field names is harder to spot) and the data marshalling takes more code. fwnode is great for things like properties for leaf devices since those are basically a free for all on ACPI systems, it allows us to quickly and simply apply the work done defining bindings for DT to ACPI systems in a way that's compatible with how APCI wants to work. It's also good for cross device bindings that are considered out of scope for ACPI, though a bit of caution is needed determining when that's the case.
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.
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.
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 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 17:26:02 +0000, Mark Brown <broonie@kernel.org> a écrit : > On Thu, Feb 24, 2022 at 05:42:05PM +0100, Clément Léger wrote: > > Hans de Goede <hdegoede@redhat.com> a écrit : > > > > As Mark already mentioned the regulator subsystem has shown to > > > be a bit problematic here, but you don't seem to need that? > > > Indeed, I don't need this subsystem. However, I'm still not clear why > > this subsystem in particular is problematic. Just so that I can > > recognize the other subsystems with the same pattern, could you explain > > me why it is problematic ? > > ACPI has a strong concept of how power supply (and general critical > resources) for devices should be described by firmware which is very > different to that which DT as it is used in Linux has, confusing that > model would make it much harder for generic OSs to work with generic > ACPI systems, and makes it much easier to create unfortunate interactions > between bits of software expecting ACPI models and bits of software > expecting DT models for dealing with a device. Potentially we could > even run into issues with new versions of Linux if there's timing or > other changes. If Linux starts parsing the core DT bindings for > regulators on ACPI systems then that makes it more likely that system > integrators who are primarily interested in Linux will produce firmwares > that run into these issues, perhaps unintentionally through a "this just > happens to work" process. Ok that's way more clear. > > As a result of this we very much do not want to have the regulator code > parsing DT bindings using the fwnode APIs since that makes it much > easier for us to end up with a situation where we are interpreting _DSD > versions of regulator bindings and ending up with people making systems > that rely on that. Instead the regulator API is intentional about which > platform description interfaces it is using. We could potentially have > something that is specific to swnode and won't work with general fwnode > but it's hard to see any advantages for this over the board file based > mechanism we have already, swnode offers less error detection (typoing > field names is harder to spot) and the data marshalling takes more code. Instead of making it specific for swnode, could we make it instead non working for acpi nodes ? Thus, the parsing would work only for swnode and device_node, not allowing to use the fwnode support with acpi for such subsystems (not talking about regulators here). If switching to board file based mechanism, this means that all drivers that are used by the PCIe card will have to be modified to support this mechanism. > > fwnode is great for things like properties for leaf devices since those > are basically a free for all on ACPI systems, it allows us to quickly > and simply apply the work done defining bindings for DT to ACPI systems > in a way that's compatible with how APCI wants to work. It's also good > for cross device bindings that are considered out of scope for ACPI, > though a bit of caution is needed determining when that's the case. Ok got it, thanks for the in-depth explanations.
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 <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,
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