| Message ID | 20240917-hotplug-drm-bridge-v4-0-bc4dfee61be6@bootlin.com |
|---|---|
| Headers | show |
| Series | Add support for GE SUNH hot-pluggable connector | expand |
Hi, On Tue, 17 Sep 2024 05:29:51 -0500 "Rob Herring (Arm)" <robh@kernel.org> wrote: > dtschema/dtc warnings/errors: > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.example.dtb: addon-connector: Unevaluated properties are not allowed ('powergood-gpios' was unexpected) Ouch, a leftover from v3. Fixed queued for v5. Luca
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!
Hello, this series aims at supporting a Linux device with a connector to physically add and remove an add-on to/from the main device to augment its features at runtime, adding devices on non-discoverable busses, using device tree overlays. Changes since v4 are limited, but I'm sending this to have the state of the art public before the discussion at the Lonux Plumbers Conference tomorrow (https://lpc.events/event/18/contributions/1750/). See the changelog at the end for the details. The high-level description below is an updated version of to the one in v3. Use case ======== This series targets a professional product (GE SUNH) that is composed of a "main" part running on battery, with the main SoC and able to work autonomously with limited features, and an optional "add-on" that enables more features by adding more hardware peripherals, some of which are on non-discoverable busses such as I2C and MIPI DSI. The add-on can be connected and disconnected at runtime at any moment by the end user, and add-on features need to be enabled and disabled automatically at runtime. The add-on has status pins that are connected to GPIOs on the main board, allowing the CPU to detect add-on insertion and removal. It also has a reset GPIO allowing to reset all peripherals on the add-on at once. The features provided by the add-on include a display and a battery charger to recharge the battery of the main part. The display on the add-on has an LVDS input but the connector between the base and the add-on has a MIPI DSI bus, so a DSI-to-LVDS bridge is present on the add-on. Different add-on models can be connected to the main part, and for this a model ID is stored in the add-on itself so the software running on the CPU on the main part knows which non-discoverable hardware to probe. Overall approach ================ Device tree overlays appear as the most natural solution to support the addition and removal of devices from a running system. Several features are missing from the mainline Linux kernel in order to support this use case: 1. runtime (un)loading of device tree overlays is currently not exposed 2. if enabled, overlay (un)loading exposes several issues and limitations 3. the DRM subsystem assumes video bridges are non-removable This series targets items 1 and 3 and some of the issues mentioned in item 2. Other issues are being handled separately (see "Device tree overlay issues" below). Device tree representation and connector driver =============================================== The device tree description we propose involves 3 main parts. 1: the main (fixed) device tree The main device tree describes the connector itself along with the status and reset GPIOs. It also provides specific nodes for the various interfaces (I2C and DSI). Here is how the connector is represented in the fixed part of the device tree: / { #include <dt-bindings/gpio/gpio.h> addon_connector: addon-connector { compatible = "ge,sunh-addon-connector"; reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>; plugged-gpios = <&gpio1 2 GPIO_ACTIVE_LOW>; i2c-dbat { i2c-parent = <&i2c2_ch1>; #address-cells = <1>; #size-cells = <0>; }; i2c-gp { i2c-parent = <&i2c5>; #address-cells = <1>; #size-cells = <0>; }; i2c-btp { i2c-parent = <&i2c3>; #address-cells = <1>; #size-cells = <0>; }; dsi { ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; hotplug_bridge_sink: endpoint { remote-endpoint = <&dsi_to_hotplug_bridge>; }; }; }; }; }; }; The connector has a specific compatible string, and this series adds a driver supporting it. This driver uses the device tree overlay loading and unloading facilities already implemented by the kernel but not currently exposed. The driver detects the connection status from the "plugged" GPIO and reacts to a connection event by loading a first overlay (the "base" overlay, see below). The 'i2c-*' nodes represent the hot-pluggable section of I2C busses crossing the connector, whose controller is on the main board but which have devices on the add-on. There is one node per each such bus: this allows full decoupling between the base board and the overlay, but requires an additional info to associate the 'i2c-*' node to the physical bus controller. Similarly, the 'dsi' node describes how the two sides of the MIPI DSI bus connect to each other. The base device tree describes the fixed part of the video pipeline as 'port@0', i.e. the physical bus that terminates on the connector. 'port@1', representing the continuation of the video bus on the add-on, will be added by an overlay. The 'dsi' node would also allow to describe a similar connector having multiple video busses: these would have one node each, such as 'dsi-foo', 'dsi-bar', 'lvds-foo', 'lvds-bar' etc while keeping the ports for each connector appropriately separated. 2: the "base" overlay The "base" overlay describes the common components that are required to read the model ID. These components are identical for all add-on models, thus only one "base" overlay is needed: /dts-v1/; /plugin/; / { fragment@0 { target-path = ""; __overlay__ { nvmem-cells = <&addon_id>; nvmem-cell-names = "id"; i2c-dbat { addon_eeprom: eeprom@51 { compatible = "atmel,24c64"; reg = <0x51>; pagesize = <32>; nvmem-layout { compatible = "fixed-layout"; #address-cells = <1>; #size-cells = <1>; /* Data cells */ addon_id: addon-id@400 { reg = <0x400 0x1>; }; }; }; }; }; }; }; Note the overlay does not have a target node. This allows the overlay to be fully decoupled from the base tree, and to allow multiple compatible connectors on the same base board. It also avoids the need to add properties to nodes in the base tree by avoiding phandle references across the overlay boundaries. With an exception. Indeed the 'nvmem-cells' and 'nvmem-cell-names' are the only two properties added to a node that is in the base tree. This is still waiting for a different representation to avoid adding such properties and all the deadprops and leaks thereof. Here an I2C device is added by a subnode of 'i2c-dbat' for an EEPROM. The i2c-dbat node itself is already present in the base tree, carrying the link to the actual I2C adapter node. The EEPROM holds the model ID of each add-on, using always the same I2C address and memory offset. 3: the "add-on-specific" overlay Based on the model ID, the connector driver loads the second overlay, which describes all the add-on hardware not yet described by the base overlay. This overlay is model-specific. Excerpt: / { fragment@0 { target-path = ""; __overlay__ { dsi { ports { port@1 { reg = <1>; hotplug_bridge_source: endpoint { remote-endpoint = <&sn65dsi84_from_bridge>; }; }; }; }; i2c-gp { #address-cells = <1>; #size-cells = <0>; dsi-lvds-bridge@2c { compatible = "ti,sn65dsi84"; reg = <0x2c>; ports { port@0 { reg = <0>; sn65dsi84_from_bridge: endpoint { remote-endpoint = <&hotplug_bridge_source>; data-lanes = <1 2 3 4>; }; }; port@2 { reg = <2>; sn65dsi84_out0: endpoint { remote-endpoint = <&panel_dsi_lvds_in0>; }; }; port@3 { reg = <3>; sn65dsi84_out1: endpoint { remote-endpoint = <&panel_dsi_lvds_in1>; }; }; }; }; }; devices { reg_addon_3v3_lcd: regulator-addon-3v3-lcd { compatible = "regulator-fixed"; regulator-name = "3V3_LCD_ADDON"; ... }; backlight_addon: backlight-addon { compatible = "led-backlight"; ... }; addon_panel_dsi_lvds: panel-dsi-lvds { compatible = "..."; power-supply = <®_addon_3v3_lcd>; backlight = <&backlight_addon>; ports { #address-cells = <1>; #size-cells = <0>; port@0{ reg = <0>; dual-lvds-odd-pixels; panel_dsi_lvds_in0: endpoint { remote-endpoint = <&sn65dsi84_out0>; }; }; port@1{ reg = <1>; dual-lvds-even-pixels; panel_dsi_lvds_in1: endpoint { remote-endpoint = <&sn65dsi84_out1>; }; }; }; }; }; }; }; }; Here the 'dsi/ports/port@1' node is completing the 'dsi' section already present in the base tree, thus describing that this add-on is connecting those DSI lines to something, in this case the SN65DSI84 DSI-to-LVDS bridge and from to an LVDS panel. The 'devices' node, containts one subnode for each device that is not on any CPU-reachable bus (I2C, DSI, etc): fixed/GPIO regulators, backlight, the panel etc. In normal (no overlay) device tree systems nodes for these devices are children of the root node, and are probed as platform devices by kernel code. With the connector we need to have them under the connector node, and the choice was to let them be children of the connector node or to group them into a new subnode. We chose the latter because that provides a more explicit representation of reality, and is coherent with the 'dsi' and 'i2c-*' nodes. As a good side effect for the implementation, this means other nodes under the connector node (dsi, i2c-*) are not considered when populating platform devices. After these steps, the add-on is fully described and working on the system. When the "plugged" GPIO reports a disconnection, the overlays are unloaded in reverse order and devices removed. DRM hotplug bridge driver ========================= DRM natively supports pipelines whose display can be removed, but all the components preceding it (all the display controller and any bridges) are assumed to be fixed and cannot be plugged, removed or modified at runtime. This series adds support for DRM pipelines having a removable part after the encoder, thus also allowing bridges to be removed and reconnected at runtime, possibly with different components. This picture summarizes the DRM structure implemented by this series: .------------------------. | DISPLAY CONTROLLER | | .---------. .------. | | | ENCODER |<--| CRTC | | | '---------' '------' | '------|-----------------' | |DSI HOTPLUG V CONNECTOR .---------. .--. .-. .---------. .-------. | 0 to N | | _| _| | | 1 to N | | | | BRIDGES |--DSI-->||_ |_ |--DSI-->| BRIDGES |--LVDS-->| PANEL | | | | | | | | | | | '---------' '--' '-' '---------' '-------' [--- fixed components --] [----------- removable add-on -----------] Fixed components include: * all components up to the DRM encoder, usually part of the SoC * optionally some bridges, in the SoC and/or as external chips Components on the removable add-on include: * one or more bridges * a fixed connector (not one natively supporting hotplug such as HDMI) * the panel The video bus is MIPI DSI in the example and in the implementation provided by this series, but the implementation is meant to allow generalization to other video busses without native hotplug support, such as parallel video and LVDS. Note that the term "connector" in this context is different from the "DRM connector" abstraction already present in the DRM subsystem (struct drm_connector). More details in the commit message of patch 4. That's all ========== Thanks for you patience in reading this! Luca Changes in v4: - Replaced DRM bridge notifier with a new callback in struct drm_bridge_funcs - Added patch for missing devlink (LEDs used by backlight) - Various cleanups - Rebased on v6.11 - Link to v3: https://lore.kernel.org/r/20240809-hotplug-drm-bridge-v3-0-b4c178380bc9@bootlin.com Changes in v3 (too many changes in v3 to mention them all, but here are the big ones): - Rewrote the DT format to allow fully decoupled overlays and to avoid adding properties (with the NVMEM exception still to be solved) - Implemented device instantiation based on the new DT format: i2c in i2c-core-of.c nobus-devices in the connector driver - DRM: insert/remove an LVDS DRM connector on hot(un)plug events - Added patch for a devlink issue on overlay removal (mostly to start discussion) - Link to v2: https://lore.kernel.org/r/20240510-hotplug-drm-bridge-v2-0-ec32f2c66d56@bootlin.com Changes in v2: - Added bindings and driver for ge,sunh-addon-connector - Removed bindings for the hotplug-video-connector, this is now represented in DT as part of the ge,sunh-addon-connector - Various monior improvements to the DRM hotplug-bridge driver - Link to v1: https://lore.kernel.org/r/20240326-hotplug-drm-bridge-v1-0-4b51b5eb75d5@bootlin.com Co-developed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- Luca Ceresoli (8): dt-bindings: connector: add GE SUNH hotplug addon connector drm/bridge: allow bridges to be informed about added and removed bridges drm/encoder: add drm_encoder_cleanup_from() drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges i2c: i2c-core-of: follow i2c-parent phandle to probe devices from added nodes backlight: led-backlight: add devlink to supplier LEDs [RFC] driver core: devlink: do not unblock consumers without any drivers found misc: add ge-addon-connector driver .../connector/ge,sunh-addon-connector.yaml | 177 ++++++ MAINTAINERS | 11 + drivers/base/core.c | 21 - drivers/gpu/drm/bridge/Kconfig | 17 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/hotplug-bridge.c | 661 +++++++++++++++++++++ drivers/gpu/drm/drm_bridge.c | 12 + drivers/gpu/drm/drm_encoder.c | 21 + drivers/i2c/i2c-core-of.c | 9 + drivers/misc/Kconfig | 18 + drivers/misc/Makefile | 1 + drivers/misc/ge-sunh-connector.c | 481 +++++++++++++++ drivers/video/backlight/led_bl.c | 13 + include/drm/drm_bridge.h | 23 + include/drm/drm_encoder.h | 1 + 15 files changed, 1446 insertions(+), 21 deletions(-) --- base-commit: c377ce116b054708c8106e58a89123f1eb43e426 change-id: 20240319-hotplug-drm-bridge-16b86e67fe92 Best regards,