| Message ID | 20220221162652.103834-3-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | add support for fwnode in i2c mux system and sfp | expand |
On Mon, Feb 21, 2022 at 05:26:44PM +0100, Clément Léger wrote: > Add fwnode_get_match_data() which is meant to be used as > device_get_match_data for fwnode_operations. ... > +const void *fwnode_get_match_data(const struct fwnode_handle *fwnode, > + const struct device *dev) > +{ > + const struct of_device_id *match; > + > + match = fwnode_match_node(fwnode, dev->driver->of_match_table); > + if (!match) > + return NULL; > + > + return match->data; > +} It's OF-centric API, why it has fwnode prefix? Can it leave in drivers/of instead?
Le Mon, 21 Feb 2022 19:46:12 +0200, Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > On Mon, Feb 21, 2022 at 05:26:44PM +0100, Clément Léger wrote: > > Add fwnode_get_match_data() which is meant to be used as > > device_get_match_data for fwnode_operations. > > ... > > > +const void *fwnode_get_match_data(const struct fwnode_handle *fwnode, > > + const struct device *dev) > > +{ > > + const struct of_device_id *match; > > + > > + match = fwnode_match_node(fwnode, dev->driver->of_match_table); > > + if (!match) > > + return NULL; > > + > > + return match->data; > > +} > > It's OF-centric API, why it has fwnode prefix? Can it leave in drivers/of instead? > > The idea is to allow device with a software_node description to match with the content of the of_match_table. Without this, we would need a new type of match table that would probably duplicates part of the of_match_table to be able to match software_node against a driver. I did not found an other way to do it without modifying drivers individually to support software_nodes.
On Tue, Feb 22, 2022 at 9:24 AM Clément Léger <clement.leger@bootlin.com> wrote: > Le Mon, 21 Feb 2022 19:46:12 +0200, > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > > On Mon, Feb 21, 2022 at 05:26:44PM +0100, Clément Léger wrote: ... > > It's OF-centric API, why it has fwnode prefix? Can it leave in drivers/of instead? > > The idea is to allow device with a software_node description to match > with the content of the of_match_table. Without this, we would need a > new type of match table that would probably duplicates part of the > of_match_table to be able to match software_node against a driver. > I did not found an other way to do it without modifying drivers > individually to support software_nodes. software nodes should not be used as a replacement of the real firmware nodes. The idea behind is to fill the gaps in the cases when firmware doesn't provide enough information to the OS. I think Heikki can confirm or correct me. If you want to use the device on an ACPI based platform, you need to describe it in ACPI as much as possible. The rest we may discuss.
Le Tue, 22 Feb 2022 09:33:32 +0100, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > On Tue, Feb 22, 2022 at 9:24 AM Clément Léger <clement.leger@bootlin.com> wrote: > > Le Mon, 21 Feb 2022 19:46:12 +0200, > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > > > On Mon, Feb 21, 2022 at 05:26:44PM +0100, Clément Léger wrote: > > ... > > > > It's OF-centric API, why it has fwnode prefix? Can it leave in drivers/of instead? > > > > The idea is to allow device with a software_node description to match > > with the content of the of_match_table. Without this, we would need a > > new type of match table that would probably duplicates part of the > > of_match_table to be able to match software_node against a driver. > > I did not found an other way to do it without modifying drivers > > individually to support software_nodes. > > software nodes should not be used as a replacement of the real > firmware nodes. The idea behind is to fill the gaps in the cases when > firmware doesn't provide enough information to the OS. I think Heikki > can confirm or correct me. Yes, the documentation states that: NOTE! The primary hardware description should always come from either ACPI tables or DT. Describing an entire system with software nodes, though possible, is not acceptable! The software nodes should only complement the primary hardware description. > > If you want to use the device on an ACPI based platform, you need to > describe it in ACPI as much as possible. The rest we may discuss. > Agreed but the PCIe card might also be plugged in a system using a device-tree description (ARM for instance). I should I do that without duplicating the description both in DT and ACPI ?
On Tue, Feb 22, 2022 at 9:47 AM Clément Léger <clement.leger@bootlin.com> wrote: > Le Tue, 22 Feb 2022 09:33:32 +0100, > Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > > On Tue, Feb 22, 2022 at 9:24 AM Clément Léger <clement.leger@bootlin.com> wrote: > > > Le Mon, 21 Feb 2022 19:46:12 +0200, > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : ... > > > The idea is to allow device with a software_node description to match > > > with the content of the of_match_table. Without this, we would need a > > > new type of match table that would probably duplicates part of the > > > of_match_table to be able to match software_node against a driver. > > > I did not found an other way to do it without modifying drivers > > > individually to support software_nodes. > > > > software nodes should not be used as a replacement of the real > > firmware nodes. The idea behind is to fill the gaps in the cases when > > firmware doesn't provide enough information to the OS. I think Heikki > > can confirm or correct me. > > Yes, the documentation states that: > > NOTE! The primary hardware description should always come from either > ACPI tables or DT. Describing an entire system with software nodes, > though possible, is not acceptable! The software nodes should only > complement the primary hardware description. > > > If you want to use the device on an ACPI based platform, you need to > > describe it in ACPI as much as possible. The rest we may discuss. > > Agreed but the PCIe card might also be plugged in a system using a > device-tree description (ARM for instance). I should I do that without > duplicating the description both in DT and ACPI ? Why is it (duplication) a problem? Each platform has its own kind of description, so one needs to provide it in the format the platform accepts.
Le Tue, 22 Feb 2022 10:24:13 +0100, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > > > If you want to use the device on an ACPI based platform, you need to > > > describe it in ACPI as much as possible. The rest we may discuss. > > > > Agreed but the PCIe card might also be plugged in a system using a > > device-tree description (ARM for instance). I should I do that without > > duplicating the description both in DT and ACPI ? > > Why is it (duplication) a problem? > Each platform has its own kind of description, so one needs to provide > it in the format the platform accepts. > The problem that I see is not only duplication but also that the PCIe card won't work out of the box and will need a specific SSDT overlays each time it is used. According to your document about SSDT overlays, there is no way to load this from the driver. This means that the user will have to compile a platform specific .aml file to match its platform configuration. If the user wants to change the PCIe port, than I guess it will have to load another .aml file. I do not think a user expect to do so when plugging a PCIe card. Moreover, the APCI documentation [1] says the following: "PCI devices, which are below the host bridge, generally do not need to be described via ACPI. The OS can discover them via the standard PCI enumeration mechanism, using config accesses to discover and identify devices and read and size their BARs. However, ACPI may describe PCI devices if it provides power management or hotplug functionality for them or if the device has INTx interrupts connected by platform interrupt controllers and a _PRT is needed to describe those connections." The device I want to use (a PCIe switch) does not fall into these categories so there should be no need to describe it using ACPI. Regarding the use of software nodes, the documentation also says that: "The software nodes can be used to complement fwnodes representing real firmware nodes when they are incomplete, for example missing device properties, *and to supply the primary fwnode when the firmware lacks hardware description for a device completely.*" I think my device falls into this last category but I might be wrong. I understand that using software_node is probably not the best idea to do so but I did not found any other convenient way to do it and SSDT overlays do not really seems to be ideal. I would be glad if you could provide me with an example of such usage to check if it's really usable. Thanks, [1] https://www.kernel.org/doc/html/latest/PCI/acpi-info.html
On Tue, Feb 22, 2022 at 10:47:05AM +0100, Clément Léger wrote: > Le Tue, 22 Feb 2022 10:24:13 +0100, > Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > > > > > If you want to use the device on an ACPI based platform, you need to > > > > describe it in ACPI as much as possible. The rest we may discuss. > > > > > > Agreed but the PCIe card might also be plugged in a system using a > > > device-tree description (ARM for instance). I should I do that without > > > duplicating the description both in DT and ACPI ? > > > > Why is it (duplication) a problem? > > Each platform has its own kind of description, so one needs to provide > > it in the format the platform accepts. > > > > The problem that I see is not only duplication but also that the PCIe > card won't work out of the box and will need a specific SSDT overlays > each time it is used. According to your document about SSDT overlays, > there is no way to load this from the driver. This means that the user > will have to compile a platform specific .aml file to match its > platform configuration. If the user wants to change the PCIe port, than > I guess it will have to load another .aml file. I do not think a user > expect to do so when plugging a PCIe card. > > Moreover, the APCI documentation [1] says the following: > > "PCI devices, which are below the host bridge, generally do not need to > be described via ACPI. The OS can discover them via the standard PCI > enumeration mechanism, using config accesses to discover and identify > devices and read and size their BARs. However, ACPI may describe PCI > devices if it provides power management or hotplug functionality for > them or if the device has INTx interrupts connected by platform > interrupt controllers and a _PRT is needed to describe those > connections." > > The device I want to use (a PCIe switch) does not fall into these > categories so there should be no need to describe it using ACPI. There should not be any need to describe it in any way, the device should provide all of the needed information. PCIe devices do not need a DT entry, as that does not make sense. thanks, greg k-h
Le Tue, 22 Feb 2022 11:20:54 +0100, Greg Kroah-Hartman <gregkh@linuxfoundation.org> a écrit : > > > > The problem that I see is not only duplication but also that the PCIe > > card won't work out of the box and will need a specific SSDT overlays > > each time it is used. According to your document about SSDT overlays, > > there is no way to load this from the driver. This means that the user > > will have to compile a platform specific .aml file to match its > > platform configuration. If the user wants to change the PCIe port, than > > I guess it will have to load another .aml file. I do not think a user > > expect to do so when plugging a PCIe card. > > > > Moreover, the APCI documentation [1] says the following: > > > > "PCI devices, which are below the host bridge, generally do not need to > > be described via ACPI. The OS can discover them via the standard PCI > > enumeration mechanism, using config accesses to discover and identify > > devices and read and size their BARs. However, ACPI may describe PCI > > devices if it provides power management or hotplug functionality for > > them or if the device has INTx interrupts connected by platform > > interrupt controllers and a _PRT is needed to describe those > > connections." > > > > The device I want to use (a PCIe switch) does not fall into these > > categories so there should be no need to describe it using ACPI. > > There should not be any need to describe it in any way, the device > should provide all of the needed information. PCIe devices do not need > a DT entry, as that does not make sense. > > thanks, > > greg k-h In my opinion, yes, there should be no need for an "external" description. Loading any kind of description (either with ACPI or DT) defeat the purpose of the PCI since the card won't be able to be plug-and-play anymore on multiple platform. The driver however should be self-contained and contain its own "description" of the hardware, no matter the platform on which the card is plugged. The PCIe card is independent of the platform, I do not think describing it with a platform specific description language is maintainable nor user acceptable for the user.
diff --git a/drivers/base/property.c b/drivers/base/property.c index 434c2713fd99..6ffb3ac4509c 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -1181,6 +1181,18 @@ const struct of_device_id *fwnode_match_node(const struct fwnode_handle *fwnode, } EXPORT_SYMBOL(fwnode_match_node); +const void *fwnode_get_match_data(const struct fwnode_handle *fwnode, + const struct device *dev) +{ + const struct of_device_id *match; + + match = fwnode_match_node(fwnode, dev->driver->of_match_table); + if (!match) + return NULL; + + return match->data; +} + const void *device_get_match_data(struct device *dev) { return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev); diff --git a/include/linux/property.h b/include/linux/property.h index 978ecf6be34e..7f727c492602 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -449,6 +449,9 @@ static inline void *device_connection_find_match(struct device *dev, const struct of_device_id *fwnode_match_node(const struct fwnode_handle *fwnode, const struct of_device_id *matches); +const void *fwnode_get_match_data(const struct fwnode_handle *fwnode, + const struct device *dev); + /* -------------------------------------------------------------------------- */ /* Software fwnode support - when HW description is incomplete or missing */
Add fwnode_get_match_data() which is meant to be used as device_get_match_data for fwnode_operations. Signed-off-by: Clément Léger <clement.leger@bootlin.com> --- drivers/base/property.c | 12 ++++++++++++ include/linux/property.h | 3 +++ 2 files changed, 15 insertions(+)