| Message ID | 20240917-hotplug-drm-bridge-v4-5-bc4dfee61be6@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for GE SUNH hot-pluggable connector | expand |
Hi Luca! On Tue, 2024-09-17 at 10:53 +0200, Luca Ceresoli wrote: > When device tree nodes are added, the I2C core tries to probe client > devices based on the classic DT structure: > > i2c@abcd0000 { > some-client@42 { compatible = "xyz,blah"; ... }; > }; > > However for hotplug connectors described via device tree overlays there is > additional level of indirection, which is needed to decouple the overlay > and the base tree: > > --- base device tree --- > > i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... }; > i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... }; > > connector { > i2c-ctrl { > i2c-parent = <&i2c1>; > #address-cells = <1>; > #size-cells = <0>; > }; > > i2c-sensors { > i2c-parent = <&i2c5>; > #address-cells = <1>; > #size-cells = <0>; > }; > }; > > --- device tree overlay --- > > ... > // This node will overlay on the i2c-ctrl node of the base tree Why don't you overlay it right over &i2c1? It should have worked since commit ea7513bbc041 ("i2c/of: Add OF_RECONFIG notifier handler"). Doesn't it work for your use-case? > i2c-ctrl { > eeprom@50 { compatible = "atmel,24c64"; ... }; > }; > ... > > --- resulting device tree --- > > i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... }; > i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... }; > > connector { > i2c-ctrl { > i2c-parent = <&i2c1>; > #address-cells = <1>; > #size-cells = <0>; > > eeprom@50 { compatible = "atmel,24c64"; ... }; > }; > > i2c-sensors { > i2c-parent = <&i2c5>; > #address-cells = <1>; > #size-cells = <0>; > }; > }; > > Here i2c-ctrl (same goes for i2c-sensors) represent the part of I2C bus > that is on the hot-pluggable add-on. On hot-plugging it will physically > connect to the I2C adapter on the base board. Let's call the 'i2c-ctrl' > node an "extension node". > > In order to decouple the overlay from the base tree, the I2C adapter > (i2c@abcd0000) and the extension node (i2c-ctrl) are separate > nodes. Rightfully, only the former will probe into an I2C adapter, and it > will do that perhaps during boot, long before overlay insertion. > > The extension node won't probe into an I2C adapter or any other device or > bus, so its subnodes ('eeprom@50') won't be interpreted as I2C clients by > current I2C core code. However it has an 'i2c-parent' phandle to point to > the corresponding I2C adapter node. This tells those nodes are I2C clients > of the adapter in that other node. > > Extend the i2c-core-of code to look for the adapter via the 'i2c-parent' > phandle when the regular adapter lookup does not find one. This allows all > clients to be probed: both those on the base board (described in the base > device tree) and those on the add-on and described by an overlay. > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > --- > > Note: while this patch works for normal hotplug and unplug, it has some > weaknesses too, due to the implementation being in a OF change > notifier. Two cases come to mind: > > 1. In the base device tree there must be _no_ nodes under the "extension > node" (i2c-ctrl), or they won't be picked up as they are not > dynamically added. > > 2. In case the I2C adapter is unbound and rebound, or it probes after > overlay insertion, it will miss the OF notifier events and so it won't > find the devices in the extension node. > > The first case is not a limiting factor: fixed I2C devices should just stay > under the good old I2C adapter node. > > The second case is a limiting factor, even though not happening in "normal" > use cases. I cannot see any solution without making the adapter aware of > the "bus extensions" it has, so on its probe it can always go look for any > devices there. Taking into account the case of multiple connectors each > having an extension of the same bus, this may look as follows in device > tree: > > --- base device tree --- > > i2c1: i2c@abcd0000 { > compatible = "xyz,i2c-ctrl"; ... > i2c-bus-extensions = <&i2c_ctrl_conn0, &i2c_ctrl_conn1>; > }; > > connector@0 { > i2c_ctrl_conn0: i2c-ctrl { > i2c-parent = <&i2c1>; > #address-cells = <1>; > #size-cells = <0>; > }; > }; > > connector@1 { > i2c_ctrl_conn1: i2c-ctrl { > i2c-parent = <&i2c1>; > #address-cells = <1>; > #size-cells = <0>; > }; > }; > > I'd love to have some feedback and opinions about the basic idea before > digging into the details of this additional step. > > --- > > Changes in v4: > - fix a typo in commit message > > This patch first appeared in v3. > --- > drivers/i2c/i2c-core-of.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c > index a6c407d36800..71c559539a13 100644 > --- a/drivers/i2c/i2c-core-of.c > +++ b/drivers/i2c/i2c-core-of.c > @@ -170,6 +170,15 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action, > switch (of_reconfig_get_state_change(action, rd)) { > case OF_RECONFIG_CHANGE_ADD: > adap = of_find_i2c_adapter_by_node(rd->dn->parent); > + if (adap == NULL) { > + struct device_node *i2c_bus; > + > + i2c_bus = of_parse_phandle(rd->dn->parent, "i2c-parent", 0); > + if (i2c_bus) { > + adap = of_find_i2c_adapter_by_node(i2c_bus); > + of_node_put(i2c_bus); > + } > + } > if (adap == NULL) > return NOTIFY_OK; /* not for us */ >
Hello Alexander, On Thu, 12 Dec 2024 19:12:02 +0000 "Sverdlin, Alexander" <alexander.sverdlin@siemens.com> wrote: > Hi Luca! > > On Tue, 2024-09-17 at 10:53 +0200, Luca Ceresoli wrote: > > When device tree nodes are added, the I2C core tries to probe client > > devices based on the classic DT structure: > > > > i2c@abcd0000 { > > some-client@42 { compatible = "xyz,blah"; ... }; > > }; > > > > However for hotplug connectors described via device tree overlays there is > > additional level of indirection, which is needed to decouple the overlay > > and the base tree: > > > > --- base device tree --- > > > > i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... }; > > i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... }; > > > > connector { > > i2c-ctrl { > > i2c-parent = <&i2c1>; > > #address-cells = <1>; > > #size-cells = <0>; > > }; > > > > i2c-sensors { > > i2c-parent = <&i2c5>; > > #address-cells = <1>; > > #size-cells = <0>; > > }; > > }; > > > > --- device tree overlay --- > > > > ... > > // This node will overlay on the i2c-ctrl node of the base tree > > Why don't you overlay it right over &i2c1? > It should have worked since commit ea7513bbc041 > ("i2c/of: Add OF_RECONFIG notifier handler"). > Doesn't it work for your use-case? One reason is decoupling the base board and addon. A different base board may wire the same connector pins to 'i2c4' instead of 'i2c1'. We want a single overlay to describe the addon, independently of the base board, so it has to mention only connector pins, not base board hardware. Another reason is that using phandles to labels in the base tree in the overlay (such as &i2c1) would need properties added by the __symbols__ node, and overlays adding properties to nodes in the live tree are not welcome. This is both for a conceptual reason (adding an overlay == adding hardware and not _changing_ hardware, so adding nodes should be enough) and an implementation one (properties added to nodes in the live tree become deadprops and thus leak memory. This topic was discussed at the latest Linux Plumbers Conference last September. Slides and video of the discussion are available here: https://lpc.events/event/18/contributions/1696/ More info are in the cover letter. Discussion leading to this implementation started after v2: https://lore.kernel.org/all/20240510163625.GA336987-robh@kernel.org/ Luca
Hi Luca! On Fri, 2024-12-13 at 12:28 +0100, Luca Ceresoli wrote: > > > However for hotplug connectors described via device tree overlays there is > > > additional level of indirection, which is needed to decouple the overlay > > > and the base tree: > > > > > > --- base device tree --- > > > > > > i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... }; > > > i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... }; > > > > > > connector { > > > i2c-ctrl { > > > i2c-parent = <&i2c1>; > > > #address-cells = <1>; > > > #size-cells = <0>; > > > }; > > > > > > i2c-sensors { > > > i2c-parent = <&i2c5>; > > > #address-cells = <1>; > > > #size-cells = <0>; > > > }; > > > }; > > > > > > --- device tree overlay --- > > > > > > ... > > > // This node will overlay on the i2c-ctrl node of the base tree > > > > Why don't you overlay it right over &i2c1? > > It should have worked since commit ea7513bbc041 > > ("i2c/of: Add OF_RECONFIG notifier handler"). > > Doesn't it work for your use-case? > > One reason is decoupling the base board and addon. A different base > board may wire the same connector pins to 'i2c4' instead of 'i2c1'. We > want a single overlay to describe the addon, independently of the base > board, so it has to mention only connector pins, not base board > hardware. > > Another reason is that using phandles to labels in the base tree in the > overlay (such as &i2c1) would need properties added by the __symbols__ > node, and overlays adding properties to nodes in the live tree are not > welcome. This is both for a conceptual reason (adding an overlay == > adding hardware and not _changing_ hardware, so adding nodes should be > enough) and an implementation one (properties added to nodes in the > live tree become deadprops and thus leak memory. > > This topic was discussed at the latest Linux Plumbers Conference last > September. Slides and video of the discussion are available here: > https://lpc.events/event/18/contributions/1696/ > > More info are in the cover letter. Discussion leading to this > implementation started after v2: > https://lore.kernel.org/all/20240510163625.GA336987-robh@kernel.org/ I see! Thank you for the explanation and for the references!
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c index a6c407d36800..71c559539a13 100644 --- a/drivers/i2c/i2c-core-of.c +++ b/drivers/i2c/i2c-core-of.c @@ -170,6 +170,15 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action, switch (of_reconfig_get_state_change(action, rd)) { case OF_RECONFIG_CHANGE_ADD: adap = of_find_i2c_adapter_by_node(rd->dn->parent); + if (adap == NULL) { + struct device_node *i2c_bus; + + i2c_bus = of_parse_phandle(rd->dn->parent, "i2c-parent", 0); + if (i2c_bus) { + adap = of_find_i2c_adapter_by_node(i2c_bus); + of_node_put(i2c_bus); + } + } if (adap == NULL) return NOTIFY_OK; /* not for us */
When device tree nodes are added, the I2C core tries to probe client devices based on the classic DT structure: i2c@abcd0000 { some-client@42 { compatible = "xyz,blah"; ... }; }; However for hotplug connectors described via device tree overlays there is additional level of indirection, which is needed to decouple the overlay and the base tree: --- base device tree --- i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... }; i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... }; connector { i2c-ctrl { i2c-parent = <&i2c1>; #address-cells = <1>; #size-cells = <0>; }; i2c-sensors { i2c-parent = <&i2c5>; #address-cells = <1>; #size-cells = <0>; }; }; --- device tree overlay --- ... // This node will overlay on the i2c-ctrl node of the base tree i2c-ctrl { eeprom@50 { compatible = "atmel,24c64"; ... }; }; ... --- resulting device tree --- i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... }; i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... }; connector { i2c-ctrl { i2c-parent = <&i2c1>; #address-cells = <1>; #size-cells = <0>; eeprom@50 { compatible = "atmel,24c64"; ... }; }; i2c-sensors { i2c-parent = <&i2c5>; #address-cells = <1>; #size-cells = <0>; }; }; Here i2c-ctrl (same goes for i2c-sensors) represent the part of I2C bus that is on the hot-pluggable add-on. On hot-plugging it will physically connect to the I2C adapter on the base board. Let's call the 'i2c-ctrl' node an "extension node". In order to decouple the overlay from the base tree, the I2C adapter (i2c@abcd0000) and the extension node (i2c-ctrl) are separate nodes. Rightfully, only the former will probe into an I2C adapter, and it will do that perhaps during boot, long before overlay insertion. The extension node won't probe into an I2C adapter or any other device or bus, so its subnodes ('eeprom@50') won't be interpreted as I2C clients by current I2C core code. However it has an 'i2c-parent' phandle to point to the corresponding I2C adapter node. This tells those nodes are I2C clients of the adapter in that other node. Extend the i2c-core-of code to look for the adapter via the 'i2c-parent' phandle when the regular adapter lookup does not find one. This allows all clients to be probed: both those on the base board (described in the base device tree) and those on the add-on and described by an overlay. Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- Note: while this patch works for normal hotplug and unplug, it has some weaknesses too, due to the implementation being in a OF change notifier. Two cases come to mind: 1. In the base device tree there must be _no_ nodes under the "extension node" (i2c-ctrl), or they won't be picked up as they are not dynamically added. 2. In case the I2C adapter is unbound and rebound, or it probes after overlay insertion, it will miss the OF notifier events and so it won't find the devices in the extension node. The first case is not a limiting factor: fixed I2C devices should just stay under the good old I2C adapter node. The second case is a limiting factor, even though not happening in "normal" use cases. I cannot see any solution without making the adapter aware of the "bus extensions" it has, so on its probe it can always go look for any devices there. Taking into account the case of multiple connectors each having an extension of the same bus, this may look as follows in device tree: --- base device tree --- i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... i2c-bus-extensions = <&i2c_ctrl_conn0, &i2c_ctrl_conn1>; }; connector@0 { i2c_ctrl_conn0: i2c-ctrl { i2c-parent = <&i2c1>; #address-cells = <1>; #size-cells = <0>; }; }; connector@1 { i2c_ctrl_conn1: i2c-ctrl { i2c-parent = <&i2c1>; #address-cells = <1>; #size-cells = <0>; }; }; I'd love to have some feedback and opinions about the basic idea before digging into the details of this additional step. --- Changes in v4: - fix a typo in commit message This patch first appeared in v3. --- drivers/i2c/i2c-core-of.c | 9 +++++++++ 1 file changed, 9 insertions(+)