Don't bother reviewing this patch. It needs to be tested and possibly
refactored first.
Geert and Herve,
This patch serious should hopefully fix both of your use cases
[1][2][3]. Can you please check to make sure the device links created
to/from the overlay devices are to/from the right ones?
I've only compile tested it. If I made some obvious mistake, feel free
to fix it and give it a shot.
Cc: Rob Herring <[email protected]>
[1] - https://lore.kernel.org/lkml/CAMuHMdXEnSD4rRJ-o90x4OprUacN_rJgyo8x6=9F9rZ+-KzjOg@mail.gmail.com/
[2] - https://lore.kernel.org/all/[email protected]/
[3] - https://lore.kernel.org/lkml/[email protected]/
Thanks,
Saravana
Saravana Kannan (2):
Revert "treewide: Fix probing of devices in DT overlays"
of: dynamic: Fix probing of overlay devices
drivers/base/core.c | 23 +++++++++++++++++++++++
drivers/bus/imx-weim.c | 6 ------
drivers/i2c/i2c-core-of.c | 5 -----
drivers/of/dynamic.c | 2 +-
drivers/of/platform.c | 5 -----
drivers/spi/spi.c | 5 -----
include/linux/fwnode.h | 1 +
7 files changed, 25 insertions(+), 22 deletions(-)
--
2.44.0.478.gd926399ef9-goog
This reverts commit 1a50d9403fb90cbe4dea0ec9fd0351d2ecbd8924.
---
drivers/bus/imx-weim.c | 6 ------
drivers/i2c/i2c-core-of.c | 5 -----
drivers/of/dynamic.c | 1 -
drivers/of/platform.c | 5 -----
drivers/spi/spi.c | 5 -----
5 files changed, 22 deletions(-)
diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 837bf9d51c6e..caaf887e0ccc 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -331,12 +331,6 @@ static int of_weim_notify(struct notifier_block *nb, unsigned long action,
"Failed to setup timing for '%pOF'\n", rd->dn);
if (!of_node_check_flag(rd->dn, OF_POPULATED)) {
- /*
- * Clear the flag before adding the device so that
- * fw_devlink doesn't skip adding consumers to this
- * device.
- */
- rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
if (!of_platform_device_create(rd->dn, NULL, &pdev->dev)) {
dev_err(&pdev->dev,
"Failed to create child device '%pOF'\n",
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index a6c407d36800..a250921bbce0 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -178,11 +178,6 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
return NOTIFY_OK;
}
- /*
- * Clear the flag before adding the device so that fw_devlink
- * doesn't skip adding consumers to this device.
- */
- rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
client = of_i2c_register_device(adap, rd->dn);
if (IS_ERR(client)) {
dev_err(&adap->dev, "failed to create client for '%pOF'\n",
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 4d57a4e34105..19a1a38554f2 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -224,7 +224,6 @@ static void __of_attach_node(struct device_node *np)
np->sibling = np->parent->child;
np->parent->child = np;
of_node_clear_flag(np, OF_DETACHED);
- np->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
raw_spin_unlock_irqrestore(&devtree_lock, flags);
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 389d4ea6bfc1..efd861fa254f 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -743,11 +743,6 @@ static int of_platform_notify(struct notifier_block *nb,
if (of_node_check_flag(rd->dn, OF_POPULATED))
return NOTIFY_OK;
- /*
- * Clear the flag before adding the device so that fw_devlink
- * doesn't skip adding consumers to this device.
- */
- rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
/* pdev_parent may be NULL when no bus platform device */
pdev_parent = of_find_device_by_node(rd->dn->parent);
pdev = of_platform_device_create(rd->dn, NULL,
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index ff75838c1b5d..17cd417f7681 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -4761,11 +4761,6 @@ static int of_spi_notify(struct notifier_block *nb, unsigned long action,
return NOTIFY_OK;
}
- /*
- * Clear the flag before adding the device so that fw_devlink
- * doesn't skip adding consumers to this device.
- */
- rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
spi = of_register_spi_device(ctlr, rd->dn);
put_device(&ctlr->dev);
--
2.44.0.478.gd926399ef9-goog
Get fw_devlink to work well with overlay devices.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/base/core.c | 23 +++++++++++++++++++++++
drivers/of/dynamic.c | 1 +
include/linux/fwnode.h | 1 +
3 files changed, 25 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5f4e03336e68..5ce33dbd7560 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -237,6 +237,29 @@ static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode,
__fw_devlink_pickup_dangling_consumers(child, new_sup);
}
+
+void fw_devlink_fixup_new_fwnode(struct fwnode_handle *new_fwnode)
+{
+ struct fwnode_handle *parent;
+
+ if (new_fwnode->dev && new_fwnode->dev->bus)
+ return;
+
+ fwnode_for_each_parent_node(new_fwnode, parent)
+ if (parent->dev && parent->dev->bus)
+ break;
+
+ if (!parent)
+ return;
+
+ mutex_lock(&fwnode_link_lock);
+ __fw_devlink_pickup_dangling_consumers(new_fwnode, parent);
+ __fw_devlink_link_to_consumers(parent->dev);
+ mutex_unlock(&fwnode_link_lock);
+
+ fwnode_handle_put(parent);
+}
+
static DEFINE_MUTEX(device_links_lock);
DEFINE_STATIC_SRCU(device_links_srcu);
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 19a1a38554f2..a2ad79ea9a06 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -581,6 +581,7 @@ static int __of_changeset_entry_notify(struct of_changeset_entry *ce,
memset(&rd, 0, sizeof(rd));
rd.dn = ce->np;
ret = of_reconfig_notify(ce->action, &rd);
+ fw_devlink_fixup_new_fwnode(of_fwnode_handle(ce->np));
break;
case OF_RECONFIG_ADD_PROPERTY:
case OF_RECONFIG_REMOVE_PROPERTY:
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 0d79070c5a70..230aeccef6f3 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -220,6 +220,7 @@ int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup,
u8 flags);
void fwnode_links_purge(struct fwnode_handle *fwnode);
void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode);
+void fw_devlink_fixup_new_fwnode(struct fwnode_handle *new_fwnode);
bool fw_devlink_is_strict(void);
#endif
--
2.44.0.478.gd926399ef9-goog
On Tue, Apr 9, 2024 at 12:37 AM Saravana Kannan <[email protected]> wrote:
>
> This reverts commit 1a50d9403fb90cbe4dea0ec9fd0351d2ecbd8924.
Please say *why* we are reverting. And you still need a S-o-b.
And you are missing some maintainers.
Rob
Hi Saravana,
+CC Luca and Thomas
On Mon, 8 Apr 2024 22:37:01 -0700
Saravana Kannan <[email protected]> wrote:
> Don't bother reviewing this patch. It needs to be tested and possibly
> refactored first.
>
> Geert and Herve,
>
> This patch serious should hopefully fix both of your use cases
> [1][2][3]. Can you please check to make sure the device links created
> to/from the overlay devices are to/from the right ones?
>
> I've only compile tested it. If I made some obvious mistake, feel free
> to fix it and give it a shot.
>
> Cc: Rob Herring <[email protected]>
>
> [1] - https://lore.kernel.org/lkml/CAMuHMdXEnSD4rRJ-o90x4OprUacN_rJgyo8x6=9F9rZ+-KzjOg@mail.gmail.com/
> [2] - https://lore.kernel.org/all/[email protected]/
> [3] - https://lore.kernel.org/lkml/[email protected]/
>
I tested your patches.
Concerning my use cases, they fix the issue described in
https://lore.kernel.org/all/[email protected]/
But not the one described in
https://lore.kernel.org/lkml/[email protected]/
A link is still present between the i2c@600 and the PCI device.
instead of the i2c@600 and the pci-ep-bus.
Adding the patch clearing the FWNODE_FLAG_NOT_DEVICE in device_add() available
at [1] on top of your patches fixes the link issue.
With this additional patch applied, the link is present between the i2c@600
and the pci-ep-bus.
[1] https://lore.kernel.org/lkml/[email protected]/
Best regards,
Hervé
Hi Saravana,
On Tue, Apr 9, 2024 at 7:37 AM Saravana Kannan <[email protected]> wrote:
> Don't bother reviewing this patch. It needs to be tested and possibly
> refactored first.
>
> Geert and Herve,
>
> This patch serious should hopefully fix both of your use cases
> [1][2][3]. Can you please check to make sure the device links created
> to/from the overlay devices are to/from the right ones?
Thanks for your series!
After applying the first patch (the revert), the issue reported in
[1] is back, as expected.
After applying both patches, applying[A]/unapplying[B]/reapplying[C]
overlay [4] works as without this series, so
Tested-by: Geert Uytterhoeven <[email protected]>
Note that the state of /sys/class/devlink/ after [C] is still not the
same as after [A], as reported before in [5]:
- platform:e6060000.pinctrl--platform:keys link is not recreated in [B],
- nothing changes in /sys/class/devlink in [C].
But that issue is not introduced in this series.
> [1] - https://lore.kernel.org/lkml/CAMuHMdXEnSD4rRJ-o90x4OprUacN_rJgyo8x6=9F9rZ+-KzjOg@mail.gmail.com/
[4] "arm64: dts: renesas: ebisu: cn41: Add overlay for MSIOF0 and 25LC040"
https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/commit/?h=topic/renesas-overlays&id=222a4936b0d3dabd43bdffb3a578423bff97b02d
[5] https://lore.kernel.org/lkml/CAMuHMdXNoYH8PJE1xb4PK-vzjXtOzrxNJoZhsHT-H4Ucm=7_ig@mail.gmail.com/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, Apr 9, 2024 at 6:02 AM Herve Codina <[email protected]> wrote:
>
> Hi Saravana,
>
> +CC Luca and Thomas
>
> On Mon, 8 Apr 2024 22:37:01 -0700
> Saravana Kannan <[email protected]> wrote:
>
> > Don't bother reviewing this patch. It needs to be tested and possibly
> > refactored first.
> >
> > Geert and Herve,
> >
> > This patch serious should hopefully fix both of your use cases
> > [1][2][3]. Can you please check to make sure the device links created
> > to/from the overlay devices are to/from the right ones?
> >
> > I've only compile tested it. If I made some obvious mistake, feel free
> > to fix it and give it a shot.
> >
> > Cc: Rob Herring <[email protected]>
> >
> > [1] - https://lore.kernel.org/lkml/CAMuHMdXEnSD4rRJ-o90x4OprUacN_rJgyo8x6=9F9rZ+-KzjOg@mail.gmail.com/
> > [2] - https://lore.kernel.org/all/[email protected]/
> > [3] - https://lore.kernel.org/lkml/[email protected]/
> >
>
> I tested your patches.
>
> Concerning my use cases, they fix the issue described in
> https://lore.kernel.org/all/[email protected]/
I went back and looked at the example. I'm not even sure I understand
that example. In that example at the link above, are you saying
without any changes to upstream reg_dock_sys_3v3 was listing it's
supplier as i2c5 instead of tca6424_dock_1? Why wasn't Geert's
existing changes in of_i2c_notify not sufficient? Looking at it, it
does:
rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
Which should clear the flag for tca6424_dock_1. Can you help me
understand why it's not getting cleared?
> But not the one described in
> https://lore.kernel.org/lkml/[email protected]/
> A link is still present between the i2c@600 and the PCI device.
> instead of the i2c@600 and the pci-ep-bus.
What do you mean by PCI device here? You say the same thing in the
link, but I don't understand what you mean. Can you clarify what
exactly gets added by the overlay? Please use the fwnode name in all
the descriptions, even when talking about device links. That should
help avoid the confusion.
Also, if you can show what the target node of the overlay looks like,
that'd help too.
> Adding the patch clearing the FWNODE_FLAG_NOT_DEVICE in device_add() available
> at [1] on top of your patches fixes the link issue.
> With this additional patch applied, the link is present between the i2c@600
> and the pci-ep-bus.
I know the problem with this patch series. But to fix it properly, I
need to understand the root of the overlay node in your examples and
the target it's applied to.
-Saravana
>
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> Best regards,
> Hervé
Hi Saravana,
On Tue, 9 Apr 2024 18:06:33 -0700
Saravana Kannan <[email protected]> wrote:
> On Tue, Apr 9, 2024 at 6:02 AM Herve Codina <[email protected]> wrote:
> >
> > Hi Saravana,
> >
> > +CC Luca and Thomas
> >
> > On Mon, 8 Apr 2024 22:37:01 -0700
> > Saravana Kannan <[email protected]> wrote:
> >
> > > Don't bother reviewing this patch. It needs to be tested and possibly
> > > refactored first.
> > >
> > > Geert and Herve,
> > >
> > > This patch serious should hopefully fix both of your use cases
> > > [1][2][3]. Can you please check to make sure the device links created
> > > to/from the overlay devices are to/from the right ones?
> > >
> > > I've only compile tested it. If I made some obvious mistake, feel free
> > > to fix it and give it a shot.
> > >
> > > Cc: Rob Herring <[email protected]>
> > >
> > > [1] - https://lore.kernel.org/lkml/CAMuHMdXEnSD4rRJ-o90x4OprUacN_rJgyo8x6=9F9rZ+-KzjOg@mail.gmail.com/
> > > [2] - https://lore.kernel.org/all/[email protected]/
> > > [3] - https://lore.kernel.org/lkml/[email protected]/
> > >
> >
> > I tested your patches.
> >
> > Concerning my use cases, they fix the issue described in
> > https://lore.kernel.org/all/[email protected]/
>
> I went back and looked at the example. I'm not even sure I understand
> that example. In that example at the link above, are you saying
> without any changes to upstream reg_dock_sys_3v3 was listing it's
> supplier as i2c5 instead of tca6424_dock_1? Why wasn't Geert's
> existing changes in of_i2c_notify not sufficient? Looking at it, it
> does:
> rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
>
> Which should clear the flag for tca6424_dock_1. Can you help me
> understand why it's not getting cleared?
I don't really know but I can mke some asumptions.
Maybe the link involved in the issue is created quite early in the overlay
applying process, before the calls to the of notifier of_i2c_notify() that
can clear the FWNODE_FLAG_NOT_DEVICE.
The link is created by of_link_to_phandle(). This functions is triggered
from of_fwnode_add_links() which is the .add_link() operation of the OF
fwnode_operations.
The operation is called by fw_devlink_parse_fwnode(), itself triggered by
fw_devlink_link_device() each time device_add() is called.
If the device_add() related to the reg_dock_sys_3v3 is called before the
of_i2c_notify() related to tca6424_dock_1, the FWNODE_FLAG_NOT_DEVICE flag
in tca6424_dock_1 is not cleared when of_link_to_phandle() is called.
Does this scenario make sense ?
>
> > But not the one described in
> > https://lore.kernel.org/lkml/[email protected]/
> > A link is still present between the i2c@600 and the PCI device.
> > instead of the i2c@600 and the pci-ep-bus.
>
> What do you mean by PCI device here? You say the same thing in the
> link, but I don't understand what you mean. Can you clarify what
> exactly gets added by the overlay? Please use the fwnode name in all
> the descriptions, even when talking about device links. That should
> help avoid the confusion.
>
> Also, if you can show what the target node of the overlay looks like,
> that'd help too.
>
> > Adding the patch clearing the FWNODE_FLAG_NOT_DEVICE in device_add() available
> > at [1] on top of your patches fixes the link issue.
> > With this additional patch applied, the link is present between the i2c@600
> > and the pci-ep-bus.
>
> I know the problem with this patch series. But to fix it properly, I
> need to understand the root of the overlay node in your examples and
> the target it's applied to.
This is the Microchip Lan966x PCI device use case.
The Lan966x is a component that can be a "standard" SoC (i.e. core CPUs
and some peripherals) or as a PCI device.
When in PCI device mode, the core CPUs are not available and are replaced
by an endpoint PCI. The PCI host in PCI device mode has to act the code CPUs
in SoC mode. In other word the PCI host has access to peripherals (PCI MMIO
accesses) and has to handle each peripherals.
Drivers for these peripherals are already available in the kernel (reset
controller, i2c controller, network switch, clock controllers, ...) and are
functional when the Lan966x is in SoC mode.
In order to re-use all of these drivers, the solution was to have a Lan966x
PCI driver that is probe() matching the Lan966x PCI Vendor/Device ID.
This driver then load a DT overlay to described the Lan966x internal components.
Basically, the base DT (the DT describing the PCI host board) has the PCI nodes
between the root PCI node and the Lan966x PCI device built at runtime during the
PCI bus scan. At the end of the scan, the Lan966x driver load the overlay and
the final DT looks like the following:
--- 8< ---
soc {
...
pcie@d0070000 {
compatible = "marvell,armada-3700-pcie";
...
/* A bridge, created at runtime during PCI scan */
pci@0,0 {
compatible = "pci11ab,100\0pciclass,060400\0pciclass,0604";
...
/* The Lan966x PCI device, created at runtime during PCI scan */
dev@0,0 {
compatible = "pci1055,9660\0pciclass,020000\0pciclass,0200";
...
/*
* This node is added by the overlay
* during lan966x PCI driver probe()
*/
pci-ep-bus@0 {
compatible = "simple-bus";
...
flx0: flexcom@e0040000 {
compatible = "atmel,sama5d2-flexcom";
reg = <0xe0040000 0x100>;
ranges = <0x0 0xe0040000 0x800>;
...
i2c_lan966x: i2c@600 {
compatible = "microchip,lan966x-i2c";
reg = <0x600 0x200>;
...
};
};
...
};
};
};
};
};
--- 8< ---
Without clearing FWNODE_FLAG_NOT_DEVICE, a link is created between
i2c@600 and dev@0,0.
With the FWNODE_FLAG_NOT_DEVICE cleared in device_add(), the link is
created between i2c@600 and pci-ep-bus@0.
flexcom@e0040000 is a MFD device.
In the lan966x PCI driver, the overlay is applied using the following:
--- 8< ---
static int lan966x_pci_load_overlay(struct lan966x_pci *data)
{
u32 dtbo_size = __dtbo_lan966x_pci_end - __dtbo_lan966x_pci_begin;
void *dtbo_start = __dtbo_lan966x_pci_begin;
int ret;
ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id,
data->dev->of_node);
if (ret)
return ret;
return 0;
}
static int lan966x_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
struct device *dev = &pdev->dev;
struct lan966x_pci *data;
...
data->dev = dev;
...
ret = lan966x_pci_load_overlay(data);
if (ret)
return ret;
...
ret = of_platform_default_populate(dev->of_node, NULL, dev);
if (ret)
goto err_unload_overlay;
return 0;
...
}
--- 8< ---
Hope my explanations were clear enough and answered your question.
Feel free to ask some more details if needed.
I provided only extractions of the full dtso file. If you need some more
information (all nodes, all properties) feel free to ask.
Best regards,
Hervé
On Tue, Apr 9, 2024 at 8:10 AM Geert Uytterhoeven <geert@linux-m68korg> wrote:
>
> Hi Saravana,
>
> On Tue, Apr 9, 2024 at 7:37 AM Saravana Kannan <[email protected]> wrote:
> > Don't bother reviewing this patch. It needs to be tested and possibly
> > refactored first.
> >
> > Geert and Herve,
> >
> > This patch serious should hopefully fix both of your use cases
> > [1][2][3]. Can you please check to make sure the device links created
> > to/from the overlay devices are to/from the right ones?
>
> Thanks for your series!
>
> After applying the first patch (the revert), the issue reported in
> [1] is back, as expected.
> After applying both patches, applying[A]/unapplying[B]/reapplying[C]
> overlay [4] works as without this series, so
> Tested-by: Geert Uytterhoeven <[email protected]>
>
> Note that the state of /sys/class/devlink/ after [C] is still not the
> same as after [A], as reported before in [5]:
> - platform:e6060000.pinctrl--platform:keys link is not recreated in [B],
> - nothing changes in /sys/class/devlink in [C].
> But that issue is not introduced in this series.
Thanks for the testing and additional info! Looks like I'll need to
make more changes to accommodate more cases. I'll send out v3 once I
figure it out, but it should continue working for you.
-Saravana
>
> > [1] - https://lore.kernel.org/lkml/CAMuHMdXEnSD4rRJ-o90x4OprUacN_rJgyo8x6=9F9rZ+-KzjOg@mail.gmail.com/
>
> [4] "arm64: dts: renesas: ebisu: cn41: Add overlay for MSIOF0 and 25LC040"
> https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-driversgit/commit/?h=topic/renesas-overlays&id=222a4936b0d3dabd43bdffb3a578423bff97b02d
> [5] https://lore.kernel.org/lkml/CAMuHMdXNoYH8PJE1xb4PK-vzjXtOzrxNJoZhsHT-H4Ucm=7_ig@mail.gmail.com/
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds