[PATCH v2 0/5] Add support for GE SUNH hot-pluggable connector (was: "drm: add support for hot-pluggable bridges")

[PATCH v2 0/5] Add support for GE SUNH hot-pluggable connector (was: "drm: add support for hot-pluggable bridges")

[Luca Ceresoli]

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, using device tree overlays.

This is the v2 of "drm: add support for hot-pluggable bridges" [0] which
was however more limited in scope, covering only the DRM aspects. This new
series also takes a different approach to the DRM bridge instantiation.

[0] https://lore.kernel.org/all/20240326-hotplug-drm-bridge-v1-0-4b51b5eb75d5@bootlin.com/

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 allowign 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 not supported
 2. if enabled, overlay (un)loading exposes several bugs
 3. the DRM subsystem assumes video bridges are non-removable

This series targets items 1 and 3. Item 2 is 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 a 'ports' node to describe how the two
sides of the MIPI DSI bus connect to each other. So now, differently from
v1, there is no standalone representation of the DRM bridge because it is
not really a hardware component. 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>;
            powergood-gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;

            ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@0 {
                    reg = <0>;

                    hotplug_conn_dsi_in: endpoint {
                        remote-endpoint = <&previous_bridge_out>;
                    };
                };

                port@1 {
                    reg = <1>;

                    hotplug_conn_dsi_out: endpoint {
                        // remote-endpoint to be added by overlay
                    };
                };
            };
        };
    };

The connector has a specific compatible string, and this series adds a
driver supporting it. This driver uses the deivce tree overlay loading and
unloading facilities already implemented by the kernel but not currently
exposed. The driver detects the connection status from the GPIOs and reacts
to a connection event by loading a first overlay (the "base" overlay).

2: the "base" overlay

The "base" overlay describes the common components that are required to
read the model ID. These are identical for all add-on models, thus only one
"base" overlay is needed:

    &i2c1 {
        #address-cells = <1>;
        #size-cells = <0>;

        eeprom@50 {
            compatible = "atmel,24c64";
            reg = <0x50>;

            nvmem-layout {
                compatible = "fixed-layout";
                #address-cells = <1>;
                #size-cells = <1>;

                addon_model_id: addon-model-id@400 {
                    reg = <0x400 0x1>;
                };
            };
        };
    };

    &addon_connector {
        nvmem-cells = <&addon_model_id>;
        nvmem-cell-names = "id";
    };

Here "i2c1" is an I2C bus whose adapter is on the main part (possibly with
some clients) but whose lines extend through the connector to the add-on
where the EEPROM is (possibly with more clients). The EEPROM holds the
model ID of each add-on, using always the same I2C address and memory
offset.

The '&addon_connector' section provides the "glue" to describe how the
NVMEM cell can be accessed via the connector. This allows the connector
driver to read the model ID.

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.

    &hotplug_conn_dsi_out {
        remote-endpoint = <&addon_bridge_in>;
    };

    &i2c1 {
        #address-cells = <1>;
        #size-cells = <0>;

        dsi-lvds-bridge@42 {
            compatible = "some-video-bridge";
            reg = <0x42>;

            ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@0 {
                    reg = <0>;

                    addon_bridge_in: endpoint {
                        remote-endpoint = <&hotplug_conn_dsi_out>;
                        data-lanes = <1 2 3 4>;
                    };
                };

                port@1 {
                    reg = <1>;

                    addon_bridge_out: endpoint {
                        remote-endpoint = <&panel_in>;
                    };
                };
            };
        };
    };

    &{/}
    {
    panel {
        ports {
            #address-cells = <1>;
            #size-cells = <0>;

            port@0{
                reg = <0>;

                panel_in: endpoint {
                    remote-endpoint = <&addon_bridge_out>;
                };
            };
        };
    };

The '&hotplug_conn_dsi_out' section fills the port@1 section that was
initially missing, in order to point to the continuation of the MIPI DSI
line. The rest of this overlay just completes the video pipeline. In the
'&i2c1' section, another I2C client is added to a bus, just as done for the
EEPROM.

After these steps, the add-on is fully described and working on the
system. When the status GPIOs report a disconnection, the overlays are
unloaded in reverse order.

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 has nothing to do with the
"DRM connector" abstraction already present in the DRM subsystem (struct
drm_connector).

In order to support the above use case while being reasonably generic and
self-contained, the implementation is based on the introduction of the
"hotplug-bridge". The hotplug-bridge implementation is as transparent as
possible. In a nutshell, it decouples the preceding and the following
components: the upstream components will believe that the pipeline is
always there, whil the downstream components will undergo a normal
probe/remove process on insertion/removal as if the entire DRM pipeline
were being added/removed. This means all the other DRM components can be
implemented normally, without having to even be aware of hot-plugging.

The hotplug-bridge it is based on a few self-contained additions to
drm_bridge and drm_encoder, which are provided as individual patches in
this series, and does not require any modifications to other bridges.
However the implementation has some tricky aspects that make it more
complex than in an ideal design. See the patch adding the driver for the
details.

Outstanding bugs and issues
===========================

Unsurprisingly, enabling device tree overlay loading/unloading at runtime
is exposing a number of issues. While testing this work and another,
unrelated work also using overlay insertion/removal [1] we ancountered
several and we are working on solving them one by one. Here is a list of
the issues for which we have sent some patches:

 1. ERROR: remove_proc_entry: removing non-empty directory 'irq/233', leaking at least 'gpiomon'
    - https://lore.kernel.org/all/20240227113426.253232-1-herve.codina@bootlin.com/
 2. leds: gpio: Add devlink between the leds-gpio device and the gpio used
    - https://lore.kernel.org/all/20240220133950.138452-1-herve.codina@bootlin.com/
 3. kobject: 'gpiochip8' ((____ptrval____)): is not initialized, yet kobject_get() is being called.
    - https://lore.kernel.org/all/CAGETcx_YjRzA0joyESsgk=XJKBqqFD7YZeSwKu1a1deo-EyeKw@mail.gmail.com/

Overlay removal is also known for memory leaks of some properties (the
"deadprops" list). This is being examined for a proper solution.

An issue related to devlink appeared since commit 782bfd03c3ae ("of:
property: Improve finding the supplier of a remote-endpoint property"),
merged in v6.8-rc5, as reported in:

 https://lore.kernel.org/all/20240223171849.10f9901d@booty

This is on my todo list as well. The current workaround is reverting
782bfd03c3ae.

Finally, a known issue is related to NVMEM. The connector driver uses NVMEM
notifiers to be notified of cell addition. This works reliably assuming the
cell to be available when the notification fucntion is called, but this
does not happen. Two alternative patches have been sent that would fix
this:

 - https://lore.kernel.org/all/20240130160021.70ddef92@xps-13/
 - https://lore.kernel.org/all/20231229-nvmem-cell-add-notification-v1-1-8d8b426be9f9@bootlin.com/

There was no activity recently about this. Continuing this work in on my
todo list.

[1] https://lore.kernel.org/all/20240430083730.134918-1-herve.codina@bootlin.com

That's all
==========

Thanks for you patience in reading this!

Luca

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 (4):
      dt-bindings: connector: add GE SUNH hotplug addon connector
      drm/encoder: add drm_encoder_cleanup_from()
      drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges
      misc: add ge-addon-connector driver

Paul Kocialkowski (1):
      drm/bridge: add bridge notifier to be notified of bridge addition and removal

 .../connector/ge,sunh-addon-connector.yaml         | 197 +++++++
 MAINTAINERS                                        |  11 +
 drivers/gpu/drm/bridge/Kconfig                     |  15 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/hotplug-bridge.c            | 577 +++++++++++++++++++++
 drivers/gpu/drm/drm_bridge.c                       |  35 ++
 drivers/gpu/drm/drm_encoder.c                      |  21 +
 drivers/misc/Kconfig                               |  15 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/ge-sunh-connector.c                   | 464 +++++++++++++++++
 include/drm/drm_bridge.h                           |  19 +
 include/drm/drm_encoder.h                          |   1 +
 12 files changed, 1357 insertions(+)
---
base-commit: 5e3810587f43a24d2c568b7e08fcff7ce05d71a9
change-id: 20240319-hotplug-drm-bridge-16b86e67fe92

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH v2 1/5] dt-bindings: connector: add GE SUNH hotplug addon connector

[Luca Ceresoli]

Add bindings for the GE SUNH add-on connector. This is a physical,
hot-pluggable connector that allows to attach and detach at runtime an
add-on adding peripherals on non-discoverable busses.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

NOTE: the second and third examples fail 'make dt_binding_check' because
      they are example of DT overlay code -- I'm not aware of a way to
      validate overlay examples as of now

This patch is new in v2.
---
 .../connector/ge,sunh-addon-connector.yaml         | 197 +++++++++++++++++++++
 MAINTAINERS                                        |   5 +
 2 files changed, 202 insertions(+)

diff --git a/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml b/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
new file mode 100644
index 000000000000..c7ac62e5f2c9
--- /dev/null
+++ b/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
@@ -0,0 +1,197 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/connector/ge,sunh-addon-connector.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GE SUNH hotplug add-on connector
+
+maintainers:
+  - Luca Ceresoli <luca.ceresoli@bootlin.com>
+
+description:
+  Represent the physical connector present on GE SUNH devices that allows
+  to attach and detach at runtime an add-on adding peripherals on
+  non-discoverable busses.
+
+  This connector has status GPIOs to notify the connection status to the
+  CPU and a reset GPIO to allow the CPU to reset all the peripherals on the
+  add-on. It also has a 4-lane MIPI DSI bus.
+
+  Add-on removal can happen at any moment under user control and without
+  prior notice to the CPU, making all of its components not usable
+  anymore. Later on, the same or a different add-on model can be connected.
+
+properties:
+  compatible:
+    const: ge,sunh-addon-connector
+
+  reset-gpios:
+    description: An output GPIO to reset the peripherals on the add-on.
+    maxItems: 1
+
+  plugged-gpios:
+    description: An input GPIO that is asserted if and only if an add-on is
+      physically connected.
+    maxItems: 1
+
+  powergood-gpios:
+    description: An input GPIO that is asserted if and only if power rails
+      on the add-on are stable.
+    maxItems: 1
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    description: OF graph bindings modeling the MIPI DSI bus across the
+      connector. The connector splits the video pipeline in a fixed part
+      and a removable part.
+
+      The fixed part of the video pipeline includes all components up to
+      the display controller and 0 or more bridges. The removable part
+      includes any bridges and any other components up to the panel.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: The MIPI DSI bus line from the CPU to the connector.
+          The remote-endpoint sub-node must point to the last non-removable
+          component of the video pipeline.
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+
+        description: The MIPI DSI bus line from the connector to the
+          add-on. The remote-endpoint sub-node must point to the first
+          add-on component of the video pipeline. As it describes the
+          hot-pluggable hardware, the endpoint node cannot be filled until
+          an add-on is detected, so this needs to be done by a device tree
+          overlay at runtime.
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+
+unevaluatedProperties: false
+
+examples:
+  # Main DTS describing the "main" board up to the connector
+  - |
+    / {
+        #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>;
+            powergood-gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+
+                    hotplug_conn_dsi_in: endpoint {
+                        remote-endpoint = <&previous_bridge_out>;
+                    };
+                };
+
+                port@1 {
+                    reg = <1>;
+
+                    hotplug_conn_dsi_out: endpoint {
+                        // remote-endpoint to be added by overlay
+                    };
+                };
+            };
+        };
+    };
+
+  # "base" overlay describing the common components on every add-on that
+  # are required to read the model ID
+  - |
+    &i2c1 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        eeprom@50 {
+            compatible = "atmel,24c64";
+            reg = <0x50>;
+
+            nvmem-layout {
+                compatible = "fixed-layout";
+                #address-cells = <1>;
+                #size-cells = <1>;
+
+                addon_model_id: addon-model-id@400 {
+                    reg = <0x400 0x1>;
+                };
+            };
+        };
+    };
+
+    &addon_connector {
+        nvmem-cells = <&addon_model_id>;
+        nvmem-cell-names = "id";
+    };
+
+  # Add-on-specific overlay describing all add-on components not in the
+  # "base" overlay
+  - |
+    &hotplug_conn_dsi_out {
+        remote-endpoint = <&addon_bridge_in>;
+    };
+
+    &i2c1 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        dsi-lvds-bridge@42 {
+            compatible = "some-video-bridge";
+            reg = <0x42>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+
+                    addon_bridge_in: endpoint {
+                        remote-endpoint = <&hotplug_conn_dsi_out>;
+                        data-lanes = <1 2 3 4>;
+                    };
+                };
+
+                port@1 {
+                    reg = <1>;
+
+                    addon_bridge_out: endpoint {
+                        remote-endpoint = <&panel_in>;
+                    };
+                };
+            };
+        };
+    };
+
+    &{/}
+    {
+    panel {
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0{
+                reg = <0>;
+
+                panel_in: endpoint {
+                    remote-endpoint = <&addon_bridge_out>;
+                };
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index ec0284125e8f..4955501217eb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9897,6 +9897,11 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
 F:	drivers/iio/pressure/mprls0025pa*
 
+HOTPLUG CONNECTOR FOR GE SUNH ADDONS
+M:	Luca Ceresoli <luca.ceresoli@bootlin.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
+
 HP BIOSCFG DRIVER
 M:	Jorge Lopez <jorge.lopez2@hp.com>
 L:	platform-driver-x86@vger.kernel.org

-- 
2.34.1

[PATCH v2 2/5] drm/bridge: add bridge notifier to be notified of bridge addition and removal

[Luca Ceresoli]

From: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

In preparation for allowing bridges to be added to and removed from a DRM
card without destroying the whole card, add a DRM bridge notifier. Notified
events are addition and removal to/from the global bridge list.

Co-developed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpu/drm/drm_bridge.c | 35 +++++++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h     | 19 +++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 521a71c61b16..245f7fa4ea22 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -25,6 +25,7 @@
 #include <linux/media-bus-format.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/notifier.h>
 
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_bridge.h>
@@ -197,6 +198,36 @@
 
 static DEFINE_MUTEX(bridge_lock);
 static LIST_HEAD(bridge_list);
+static BLOCKING_NOTIFIER_HEAD(bridge_notifier);
+
+/**
+ * drm_bridge_notifier_register - add a DRM bridge notifier
+ * @nb: the notifier block to be registered
+ *
+ * The notifier block will be notified of events defined in
+ * &drm_bridge_notifier_event
+ */
+int drm_bridge_notifier_register(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&bridge_notifier, nb);
+}
+EXPORT_SYMBOL(drm_bridge_notifier_register);
+
+/**
+ * drm_bridge_notifier_unregister - remove a DRM bridge notifier
+ * @nb: the notifier block to be unregistered
+ */
+int drm_bridge_notifier_unregister(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&bridge_notifier, nb);
+}
+EXPORT_SYMBOL(drm_bridge_notifier_unregister);
+
+static void drm_bridge_notifier_notify(unsigned long event,
+				       struct drm_bridge *bridge)
+{
+	blocking_notifier_call_chain(&bridge_notifier, event, bridge);
+}
 
 /**
  * drm_bridge_add - add the given bridge to the global bridge list
@@ -210,6 +241,8 @@ void drm_bridge_add(struct drm_bridge *bridge)
 	mutex_lock(&bridge_lock);
 	list_add_tail(&bridge->list, &bridge_list);
 	mutex_unlock(&bridge_lock);
+
+	drm_bridge_notifier_notify(DRM_BRIDGE_NOTIFY_ADD, bridge);
 }
 EXPORT_SYMBOL(drm_bridge_add);
 
@@ -243,6 +276,8 @@ EXPORT_SYMBOL(devm_drm_bridge_add);
  */
 void drm_bridge_remove(struct drm_bridge *bridge)
 {
+	drm_bridge_notifier_notify(DRM_BRIDGE_NOTIFY_REMOVE, bridge);
+
 	mutex_lock(&bridge_lock);
 	list_del_init(&bridge->list);
 	mutex_unlock(&bridge_lock);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 4baca0d9107b..ee48c1eb76ae 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -43,6 +43,22 @@ struct drm_panel;
 struct edid;
 struct i2c_adapter;
 
+/**
+ * enum drm_bridge_notifier_event - DRM bridge events
+ */
+enum drm_bridge_notifier_event {
+	/**
+	 * @DRM_BRIDGE_NOTIFY_ADD: A bridge has just been added to the
+	 * global bridge list. See drm_bridge_add().
+	 */
+	DRM_BRIDGE_NOTIFY_ADD,
+	/**
+	 * @DRM_BRIDGE_NOTIFY_REMOVE: A bridge is about to be removed from
+	 * the global bridge list. See drm_bridge_remove().
+	 */
+	DRM_BRIDGE_NOTIFY_REMOVE,
+};
+
 /**
  * enum drm_bridge_attach_flags - Flags for &drm_bridge_funcs.attach
  */
@@ -781,6 +797,9 @@ drm_priv_to_bridge(struct drm_private_obj *priv)
 	return container_of(priv, struct drm_bridge, base);
 }
 
+int drm_bridge_notifier_register(struct notifier_block *nb);
+int drm_bridge_notifier_unregister(struct notifier_block *nb);
+
 void drm_bridge_add(struct drm_bridge *bridge);
 int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge);
 void drm_bridge_remove(struct drm_bridge *bridge);

-- 
2.34.1

[PATCH v2 3/5] drm/encoder: add drm_encoder_cleanup_from()

[Luca Ceresoli]

Supporting hardware whose final part of the DRM pipeline can be physically
removed requires the ability to detach all bridges from a given point to
the end of the pipeline.

Introduce a variant of drm_encoder_cleanup() for this.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changed in v2:
 - fix a typo in a comment
---
 drivers/gpu/drm/drm_encoder.c | 21 +++++++++++++++++++++
 include/drm/drm_encoder.h     |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 8f2bc6a28482..472dfbefe296 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -207,6 +207,27 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
 }
 EXPORT_SYMBOL(drm_encoder_cleanup);
 
+/**
+ * drm_encoder_cleanup_from - remove a given bridge and all the following
+ * @encoder: encoder whole list of bridges shall be pruned
+ * @bridge: first bridge to remove
+ *
+ * Removes from an encoder all the bridges starting with a given bridge
+ * and until the end of the chain.
+ *
+ * This should not be used in "normal" DRM pipelines. It is only useful for
+ * devices whose final part of the DRM chain can be physically removed and
+ * later reconnected (possibly with different hardware).
+ */
+void drm_encoder_cleanup_from(struct drm_encoder *encoder, struct drm_bridge *bridge)
+{
+	struct drm_bridge *next;
+
+	list_for_each_entry_safe_from(bridge, next, &encoder->bridge_chain, chain_node)
+		drm_bridge_detach(bridge);
+}
+EXPORT_SYMBOL(drm_encoder_cleanup_from);
+
 static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr)
 {
 	struct drm_encoder *encoder = ptr;
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 977a9381c8ba..bafcabb24267 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -320,6 +320,7 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
 }
 
 void drm_encoder_cleanup(struct drm_encoder *encoder);
+void drm_encoder_cleanup_from(struct drm_encoder *encoder, struct drm_bridge *bridge);
 
 /**
  * drm_for_each_encoder_mask - iterate over encoders specified by bitmask

-- 
2.34.1

[PATCH v2 4/5] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges

[Luca Ceresoli]

This driver implements the point of a DRM pipeline where a connector allows
removal of all the following bridges up to the panel.

The DRM subsystem currently allows hotplug of the monitor but not preceding
components. However there are embedded devices where the "tail" of the DRM
pipeline, including one or more bridges, can be physically removed:

 .------------------------.
 |   DISPLAY CONTROLLER   |
 | .---------.   .------. |
 | | ENCODER |<--| CRTC | |
 | '---------'   '------' |
 '------|-----------------'
        |
        |               HOTPLUG
        V              CONNECTOR
   .---------.        .--.    .-.        .---------.         .-------.
   | 0 to N  |        | _|   _| |        | 1 to N  |         |       |
   | BRIDGES |--DSI-->||_   |_  |--DSI-->| BRIDGES |--LVDS-->| PANEL |
   |         |        |  |    | |        |         |         |       |
   '---------'        '--'    '-'        '---------'         '-------'

 [--- fixed components --]  [----------- removable add-on -----------]

This driver supports such devices, where the final segment of a MIPI DSI
bus, including one or more bridges, can be physically disconnected and
reconnected at runtime, possibly with a different model.

This implementation supports a MIPI DSI bus only, but it is designed to be
as far as possible generic and extendable to other busses that have no
native hotplug and model ID discovery.

This driver does not provide facilities to add and remove the hot-pluggable
components from the kernel: this needs to be done by other means
(e.g. device tree overlay runtime insertion and removal). The
hotplug-bridge gets notified of hot-plugging by the DRM bridge notifier
callbacks after they get added or before they get removed.

The hotplug-bridge role is to implement the "hot-pluggable connector" in
the bridge chain. In this position, what the hotplug-bridge should ideally
do is:

 * communicate with the previous component (bridge or encoder) so that it
   believes it always has a connected bridge following it and the DRM card
   is always present
 * be notified of the addition and removal of the following bridge and
   attach/detach to/from it
 * communicate with the following bridge so that it will attach and detach
   using the normal procedure (as if the entire pipeline were being created
   or destroyed, not only the tail)
 * expose the "add-on connected/disconnected" status via the DRM connector
   connected/disconnected status, so that users of the DRM pipeline know
   when they can render output on the display

However some aspects make it a bit more complex than that. Most notably:

 * the next bridge can be probed and removed at any moment and all probing
   sequences need to be handled
 * the DSI host/device registration process, which adds to the DRM bridge
   attach process, makes the initial card registration tricky
 * the need to register and deregister the following bridges at runtime
   without tearing down the whole DRM card prevents using the functions
   that are normally recommended
 * the automatic mechanism to call the appropriate .get_modes operation
   (typically provided by the panel bridge) cannot work as the panel can
   disappear and reappear as a different model, so an ad-hoc lookup is
   needed

The code handling these and other tricky aspects is accurately documented
by comments in the code.

Co-developed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changed in v2:
 - change to be a platform device instantiated from the connector driver
   instead of a self-standing OF driver
 - add missing error handling for devm_drm_bridge_add()
 - various cleanups and style improvements
---
 MAINTAINERS                             |   5 +
 drivers/gpu/drm/bridge/Kconfig          |  15 +
 drivers/gpu/drm/bridge/Makefile         |   1 +
 drivers/gpu/drm/bridge/hotplug-bridge.c | 577 ++++++++++++++++++++++++++++++++
 4 files changed, 598 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4955501217eb..672c26372c92 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6712,6 +6712,11 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
 F:	Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
 F:	drivers/gpu/drm/panel/panel-himax-hx8394.c
 
+DRM DRIVER FOR HOTPLUG VIDEO CONNECTOR BRIDGE
+M:	Luca Ceresoli <luca.ceresoli@bootlin.com>
+S:	Maintained
+F:	drivers/gpu/drm/bridge/hotplug-bridge.c
+
 DRM DRIVER FOR HX8357D PANELS
 S:	Orphan
 T:	git git://anongit.freedesktop.org/drm/drm-misc
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index efd996f6c138..409d090ee94d 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -90,6 +90,21 @@ config DRM_FSL_LDB
 	help
 	  Support for i.MX8MP DPI-to-LVDS on-SoC encoder.
 
+config DRM_HOTPLUG_BRIDGE
+	tristate "Hotplug DRM bridge support"
+	depends on OF
+	select DRM_PANEL_BRIDGE
+	select DRM_MIPI_DSI
+	select DRM_KMS_HELPER
+	help
+	  Driver for a DRM bridge representing a physical connector that
+	  splits a DRM pipeline into a fixed part and a physically
+	  removable part. The fixed part includes up to the encoder and
+	  zero or more bridges. The removable part includes any following
+	  bridges up to the connector and panel and can be physically
+	  removed and connected at runtime, possibly with different
+	  components.
+
 config DRM_ITE_IT6505
 	tristate "ITE IT6505 DisplayPort bridge"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 017b5832733b..278f20729c6c 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o
 obj-$(CONFIG_DRM_CROS_EC_ANX7688) += cros-ec-anx7688.o
 obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
 obj-$(CONFIG_DRM_FSL_LDB) += fsl-ldb.o
+obj-$(CONFIG_DRM_HOTPLUG_BRIDGE) += hotplug-bridge.o
 obj-$(CONFIG_DRM_ITE_IT6505) += ite-it6505.o
 obj-$(CONFIG_DRM_LONTIUM_LT8912B) += lontium-lt8912b.o
 obj-$(CONFIG_DRM_LONTIUM_LT9211) += lontium-lt9211.o
diff --git a/drivers/gpu/drm/bridge/hotplug-bridge.c b/drivers/gpu/drm/bridge/hotplug-bridge.c
new file mode 100644
index 000000000000..cd663b7fbdbd
--- /dev/null
+++ b/drivers/gpu/drm/bridge/hotplug-bridge.c
@@ -0,0 +1,577 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * A DRM bridge representing the split point between a fixed part of the
+ * DRM pipeline and a physically removable part. The fixed part includes up
+ * to the encoder and zero or more bridges. Insertion and removal of the
+ * "downstream" components happens via device driver probe/removal.
+ *
+ * Copyright (C) 2024, GE HealthCare
+ *
+ * Authors:
+ * Luca Ceresoli <luca.ceresoli@bootlin.com>
+ * Paul Kocialkowski <paul.kocialkowski@bootlin.com>
+ */
+
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_of.h>
+#include <drm/drm_probe_helper.h>
+
+struct hotplug_bridge {
+	struct device *dev;
+
+	/* Local bridge */
+	struct drm_bridge bridge;
+	/* Local connector for the bridge chain */
+	struct drm_connector *connector;
+	/* Downstream bridge (next in the chain) */
+	struct drm_bridge *next_bridge;
+	struct mutex next_bridge_mutex;
+
+	struct work_struct hpd_work;
+	struct notifier_block drm_bridge_nb;
+
+	/* Local DSI host, for the downstream DSI device to attach to */
+	struct mipi_dsi_host dsi_host;
+	/* Local DSI device, attached to the upstream DSI host */
+	struct mipi_dsi_device *dsi_dev;
+	/* Upstream DSI host (the actual DSI controller) */
+	struct mipi_dsi_host *prev_dsi_host;
+};
+
+static struct hotplug_bridge *hotplug_bridge_from_drm_bridge(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct hotplug_bridge, bridge);
+}
+
+/*
+ * Attach the remote bridge to the encoder and to the next bridge in the
+ * chain, if possible. For this to succeed, we need to know:
+ *
+ * - the encoder, which is set at the first drm_bridge_attach() time
+ * - the next bridge, which is obtained via a notifier whenever the next
+ *   bridge is (re)probed, or at probe time in case it was probed before us
+ *
+ * In order to handle different execution sequences, this function can be
+ * called from multiple places and needs to check all the prerequisites
+ * every time, and it will act only if both are met.
+ *
+ * Must be called with hpb->next_bridge_mutex held.
+ *
+ * Returns 0 if the encoder was attached successfully, -ENODEV if any of
+ * the two prerequisites above is not met (no encoder or no next bridge),
+ * the error returned by drm_bridge_attach() otherwise.
+ */
+static int hotplug_bridge_attach_to_encoder_chain(struct hotplug_bridge *hpb)
+{
+	int ret;
+
+	if (!hpb->next_bridge || !hpb->bridge.encoder)
+		return -ENODEV;
+
+	ret = drm_bridge_attach(hpb->bridge.encoder, hpb->next_bridge, &hpb->bridge,
+				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	if (ret)
+		return dev_err_probe(hpb->dev, ret, "drm_bridge_attach failed\n");
+
+	dev_dbg(hpb->dev, "attached to encoder chain\n");
+
+	return 0;
+}
+
+/*
+ * Stop the video pipeline and detach next_bridge.
+ *
+ * Must be called with hpb->next_bridge_mutex held.
+ */
+static void hotplug_bridge_detach_from_encoder_chain(struct hotplug_bridge *hpb)
+{
+	WARN_ON_ONCE(!hpb->next_bridge);
+
+	dev_dbg(hpb->dev, "detaching from encoder chain\n");
+
+	drm_atomic_helper_shutdown(hpb->bridge.dev);
+
+	drm_encoder_cleanup_from(hpb->bridge.encoder, hpb->next_bridge);
+}
+
+static void hotplug_bridge_grab(struct hotplug_bridge *hpb)
+{
+	struct device *dev = hpb->dev;
+	struct drm_bridge *bridge;
+	struct drm_panel *panel;
+	int err;
+
+	mutex_lock(&hpb->next_bridge_mutex);
+
+	if (hpb->next_bridge)
+		goto out;
+
+	/*
+	 * This is supposed to be replaced by devm_drm_of_get_bridge(), but
+	 * that is a devm_, and we need to remove the panel bridge also on
+	 * next_bridge disconnect.
+	 */
+	err = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, &bridge);
+	if (err)
+		goto out;
+
+	/* Convert the remote panel to a bridge */
+	if (panel)
+		bridge = drm_panel_bridge_add(panel);
+
+	hpb->next_bridge = bridge;
+
+	dev_dbg(dev, "grabbed next bridge (%pOFn)\n", hpb->next_bridge->of_node);
+
+	hpb->bridge.pre_enable_prev_first = hpb->next_bridge->pre_enable_prev_first;
+
+	hotplug_bridge_attach_to_encoder_chain(hpb);
+
+	queue_work(system_wq, &hpb->hpd_work);
+
+out:
+	mutex_unlock(&hpb->next_bridge_mutex);
+}
+
+/*
+ * Detach from the next bridge and remove the panel bridge, either on
+ * release or when the downstream bridge is being removed.
+ *
+ * Can be called in these ways:
+ *
+ * - bridge_being_removed is NULL: detach unconditionally
+ *   (this is useful on .remove() to teardown everything)
+ * - bridge_being_removed == hpb->next_bridge: detach
+ *   (the downstream bridge is being removed)
+ * - bridge_being_removed != hpb->next_bridge: do nothing
+ *   (the bridge being removed is not the downstream bridge)
+ *
+ * In all cases, does nothing when there is no downstream bridge.
+ */
+static void hotplug_bridge_release(struct hotplug_bridge *hpb,
+				   struct drm_bridge *bridge_being_removed)
+{
+	mutex_lock(&hpb->next_bridge_mutex);
+
+	if (!hpb->next_bridge)
+		goto out;
+
+	if (bridge_being_removed && bridge_being_removed != hpb->next_bridge)
+		goto out;
+
+	dev_dbg(hpb->dev, "releasing next bridge (%pOFn)\n", hpb->next_bridge->of_node);
+
+	hotplug_bridge_detach_from_encoder_chain(hpb);
+
+	/*
+	 * This will check that the bridge actually belongs to panel-bridge
+	 * before doing anything with it, so we can safely always call it.
+	 */
+	drm_panel_bridge_remove(hpb->next_bridge);
+	hpb->next_bridge = NULL;
+
+	queue_work(system_wq, &hpb->hpd_work);
+
+out:
+	mutex_unlock(&hpb->next_bridge_mutex);
+}
+
+static int hotplug_bridge_notifier_call(struct notifier_block *nb,
+					unsigned long event, void *private)
+{
+	struct hotplug_bridge *hpb = container_of(nb, struct hotplug_bridge, drm_bridge_nb);
+	struct drm_bridge *bridge = private;
+
+	switch (event) {
+	case DRM_BRIDGE_NOTIFY_ADD:
+		hotplug_bridge_grab(hpb);
+		break;
+	case DRM_BRIDGE_NOTIFY_REMOVE:
+		hotplug_bridge_release(hpb, bridge);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static int hotplug_bridge_attach(struct drm_bridge *bridge,
+				 enum drm_bridge_attach_flags flags)
+{
+	struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge);
+	struct device *dev = hpb->dev;
+	struct drm_connector *connector;
+	struct drm_encoder *encoder = hpb->bridge.encoder;
+	int err;
+
+	/* Encoder was not yet provided to our bridge */
+	if (!encoder)
+		return -ENODEV;
+
+	/* Connector was already created */
+	if (hpb->connector)
+		return dev_err_probe(dev, -EBUSY, "connector already created\n");
+
+	connector = drm_bridge_connector_init(bridge->dev, encoder);
+	if (IS_ERR(connector))
+		return dev_err_probe(dev, PTR_ERR(connector), "failed to initialize connector\n");
+
+	drm_connector_attach_encoder(connector, encoder);
+
+	hpb->connector = connector;
+
+	drm_connector_register(connector);
+
+	mutex_lock(&hpb->next_bridge_mutex);
+	err = hotplug_bridge_attach_to_encoder_chain(hpb);
+	mutex_unlock(&hpb->next_bridge_mutex);
+
+	/* -ENODEV is acceptable, in case next_bridge is not yet known */
+	if (err == -ENODEV)
+		err = 0;
+
+	return err;
+}
+
+static void hotplug_bridge_detach(struct drm_bridge *bridge)
+{
+	struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge);
+
+	mutex_lock(&hpb->next_bridge_mutex);
+	hotplug_bridge_detach_from_encoder_chain(hpb);
+	mutex_unlock(&hpb->next_bridge_mutex);
+
+	if (hpb->connector) {
+		drm_connector_unregister(hpb->connector);
+		drm_connector_cleanup(hpb->connector);
+		hpb->connector = NULL;
+	}
+}
+
+static enum drm_connector_status hotplug_bridge_detect(struct drm_bridge *bridge)
+{
+	struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge);
+
+	return hpb->next_bridge ? connector_status_connected : connector_status_disconnected;
+}
+
+static void hotplug_bridge_hpd_work_func(struct work_struct *work)
+{
+	struct hotplug_bridge *hpd = container_of(work, struct hotplug_bridge, hpd_work);
+
+	if (hpd->bridge.dev)
+		drm_helper_hpd_irq_event(hpd->bridge.dev);
+}
+
+static int hotplug_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector *connector)
+{
+	struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge);
+	struct drm_bridge *br;
+	int nmodes = 0;
+
+	mutex_lock(&hpb->next_bridge_mutex);
+
+	if (!hpb->next_bridge || !hpb->bridge.encoder)
+		goto out;
+
+	/*
+	 * In non-removable pipelines, drm_bridge_connector_init() stores
+	 * in the bridge_connector a pointer to the first bridge having
+	 * OP_MODES (typically the panel bridge), so the .get_modes op will
+	 * be automatically be called on that bridge. In our case the
+	 * connector is created once when attaching to the encoder, but the
+	 * panel bridge will appear later and can disappear and change at
+	 * runtime, so we need to look for it every time dynamically.
+	 */
+	br = hpb->next_bridge;
+	list_for_each_entry_from(br, &hpb->bridge.encoder->bridge_chain, chain_node) {
+		if (br->ops & DRM_BRIDGE_OP_MODES) {
+			nmodes = br->funcs->get_modes(br, connector);
+			break;
+		}
+	}
+
+out:
+	mutex_unlock(&hpb->next_bridge_mutex);
+
+	return nmodes;
+}
+
+static const struct drm_bridge_funcs hotplug_bridge_funcs = {
+	.attach		= hotplug_bridge_attach,
+	.detach		= hotplug_bridge_detach,
+	.detect		= hotplug_bridge_detect,
+	.get_modes	= hotplug_bridge_get_modes,
+};
+
+static int hotplug_bridge_dsi_detach(struct mipi_dsi_host *host,
+				     struct mipi_dsi_device *device_remote)
+{
+	struct hotplug_bridge *hpb = dev_get_drvdata(host->dev);
+
+	if (!hpb->dsi_dev)
+		return -ENODEV;
+
+	mipi_dsi_detach(hpb->dsi_dev);
+	mipi_dsi_device_unregister(hpb->dsi_dev);
+	hpb->dsi_dev = NULL;
+
+	return 0;
+}
+
+/*
+ * Attach the local DSI device to the upstream DSI host, possibly with a
+ * "null" format.
+ *
+ * In "normal" bridges this function should be _only_ used as the .attach
+ * callback of hotplug_bridge_dsi_ops. But "normal" bridges have their
+ * downstream DSI device always connected, which we don't. When booting
+ * without anything connected downstream, our upstream bridge could be not
+ * even calling drm_bridge_add() until we do attach ourselves as a DSI
+ * device, preventing the whole DRM card from being instantiated.
+ *
+ * In order to always have a DRM card after boot, we do call this same
+ * function while probing in order to attach as a DSI device to the DSI
+ * master. However during probe we don't know the bus format yet. It would
+ * be nice to be able to update the format afterwards when a downstream DSI
+ * device is attaching to our local host, but there is no callback for
+ * that. To overcome this limitation, this function can be called in two
+ * ways:
+ *
+ * - during probe, to make the upstream bridge happy, when there is no
+ *   next_dsi_dev yet and thus the lanes/format/etc are unknown
+ * - as the mipi_dsi_host_ops.attach callback proper, as soon as the
+ *   next_dsi_dev is known
+ *
+ * The resulting call sequence is:
+ *
+ * 1. hotplug_bridge_dsi_attach() called by hotplug_bridge_probe() with
+ *    next_dsi_dev == NULL: we attach to the host but with a fake format
+ *    so the DRM card can be populated. hpb->dsi_dev becomes non-NULL.
+ * 2. hotplug_bridge_dsi_attach() called as .attach callback from a
+ *    downstream device when it becomes available: we need to detach in
+ *    order to re-attach with the format of the device. hpb->dsi_dev
+ *    is found non-NULL, then reused so it will be non-NULL again.
+ * 3. hotplug_bridge_dsi_detach() called as the .detach callback by a
+ *    downstream device: cleans up everything normally. hpb->dsi_dev goes
+ *    from non-NULL to NULL.
+ * 4. hotplug_bridge_dsi_attach() called by a downstream device: attaches
+ *    normally to the upstream DSI host. hpb->dsi_dev goes from NULL to
+ *    non-NULL.
+ *
+ * Steps 3 and 4 are the "normal" attach/detach steps as on "normal"
+ * bridges.
+ *
+ * Steps 1 and 2 happen only the first time, steps 3 and 4 will happen
+ * every time the downstream bridge disconnects and reconnects.
+ */
+static int hotplug_bridge_dsi_attach(struct mipi_dsi_host *host,
+				     struct mipi_dsi_device *next_dsi_dev)
+{
+	struct device *dev = host->dev;
+	struct hotplug_bridge *hpb = dev_get_drvdata(dev);
+	struct mipi_dsi_device *dsi_dev;
+	const struct mipi_dsi_device_info dsi_info = {
+		.type = "hotplug-bridge",
+		.channel = 0,
+		.node = NULL,
+	};
+	int err;
+
+	/*
+	 * Step 2 only (first time we are called for an actual device
+	 * attaching): clean up the fake attach done at step 1
+	 */
+	if (hpb->dsi_dev)
+		hotplug_bridge_dsi_detach(&hpb->dsi_host, NULL);
+
+	/* Register a local DSI device with the remote DSI host */
+	dsi_dev = mipi_dsi_device_register_full(hpb->prev_dsi_host,
+						&dsi_info);
+	if (IS_ERR(dsi_dev))
+		return PTR_ERR(dsi_dev);
+
+	/* At step 1 we have no downstream device to get the format from */
+	if (next_dsi_dev) {
+		dsi_dev->channel    = next_dsi_dev->channel;
+		dsi_dev->lanes      = next_dsi_dev->lanes;
+		dsi_dev->format     = next_dsi_dev->format;
+		dsi_dev->mode_flags = next_dsi_dev->mode_flags;
+	}
+
+	/* Attach our local DSI device to the remote DSI host */
+	err = mipi_dsi_attach(dsi_dev);
+	if (err) {
+		mipi_dsi_device_unregister(dsi_dev);
+		return dev_err_probe(dev, err, "failed to attach hotplug dsi device to host\n");
+	}
+
+	hpb->dsi_dev = dsi_dev;
+
+	return 0;
+}
+
+/*
+ * Propagate mipi_dsi_device_transfer() to the upstream DSI host.
+ *
+ * Reimplements identically the minimal needed part of
+ * mipi_dsi_device_transfer(), including the -ENOSYS return value.
+ */
+static ssize_t hotplug_bridge_dsi_transfer(struct mipi_dsi_host *host,
+					   const struct mipi_dsi_msg *msg)
+{
+	struct hotplug_bridge *hpb = dev_get_drvdata(host->dev);
+	const struct mipi_dsi_host_ops *ops;
+
+	if (!hpb->dsi_dev)
+		return -ENODEV;
+
+	ops = hpb->dsi_dev->host->ops;
+
+	if (!ops || !ops->transfer)
+		return -ENOSYS;
+
+	return ops->transfer(hpb->dsi_dev->host, msg);
+}
+
+static const struct mipi_dsi_host_ops hotplug_bridge_dsi_ops = {
+	.attach		= hotplug_bridge_dsi_attach,
+	.detach		= hotplug_bridge_dsi_detach,
+	.transfer	= hotplug_bridge_dsi_transfer,
+};
+
+/*
+ * Find the upstream DSI host and register our downstream-facing DSI host.
+ */
+static int hotplug_bridge_dsi_setup(struct hotplug_bridge *hpb)
+{
+	struct device *dev = hpb->dev;
+	struct device_node *endpoint;
+	struct device_node *node;
+
+	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
+	node = of_graph_get_remote_port_parent(endpoint);
+
+	hpb->prev_dsi_host = of_find_mipi_dsi_host_by_node(node);
+
+	of_node_put(node);
+	of_node_put(endpoint);
+
+	if (!hpb->prev_dsi_host)
+		return -EPROBE_DEFER;
+
+	hpb->dsi_host.dev = dev;
+	hpb->dsi_host.ops = &hotplug_bridge_dsi_ops;
+
+	return mipi_dsi_host_register(&hpb->dsi_host);
+}
+
+static void hotplug_bridge_dsi_cleanup(struct hotplug_bridge *hpb)
+{
+	mipi_dsi_host_unregister(&hpb->dsi_host);
+}
+
+static int hotplug_bridge_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct hotplug_bridge *hpb;
+	struct drm_bridge *bridge;
+	int err;
+
+	hpb = devm_kzalloc(dev, sizeof(*hpb), GFP_KERNEL);
+	if (!hpb)
+		return -ENOMEM;
+
+	device_set_node(dev, dev_fwnode(dev->parent));
+
+	hpb->dev = dev;
+
+	mutex_init(&hpb->next_bridge_mutex);
+	INIT_WORK(&hpb->hpd_work, hotplug_bridge_hpd_work_func);
+
+	hpb->drm_bridge_nb.notifier_call = hotplug_bridge_notifier_call;
+
+	err = hotplug_bridge_dsi_setup(hpb);
+	if (err) {
+		dev_err_probe(dev, err, "failed to setup DSI\n");
+		goto err_unset_node;
+	}
+
+	bridge = &hpb->bridge;
+	bridge->of_node = dev->of_node;
+	bridge->funcs = &hotplug_bridge_funcs;
+	bridge->type = DRM_MODE_CONNECTOR_DSI;
+	bridge->ops |= DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD | DRM_BRIDGE_OP_MODES;
+
+	platform_set_drvdata(pdev, hpb);
+
+	err = devm_drm_bridge_add(dev, bridge);
+	if (err) {
+		dev_err_probe(dev, err, "failed adding bridge\n");
+		goto err_dsi_cleanup;
+	}
+
+	err = hotplug_bridge_dsi_attach(&hpb->dsi_host, NULL);
+	if (err) {
+		dev_err_probe(dev, err, "failed first attach to upstream DSI host\n");
+		goto err_dsi_cleanup;
+	}
+
+	/* To be notified when the next bridge appears... */
+	drm_bridge_notifier_register(&hpb->drm_bridge_nb);
+
+	/* ...but also check now, in case the next bridge was probed earlier */
+	hotplug_bridge_grab(hpb);
+
+	return 0;
+
+err_dsi_cleanup:
+	hotplug_bridge_dsi_cleanup(hpb);
+err_unset_node:
+	device_set_node(dev, NULL);
+	return err;
+}
+
+static void hotplug_bridge_remove(struct platform_device *pdev)
+{
+	struct hotplug_bridge *hpb = platform_get_drvdata(pdev);
+
+	cancel_work_sync(&hpb->hpd_work);
+
+	drm_bridge_notifier_unregister(&hpb->drm_bridge_nb);
+
+	hotplug_bridge_release(hpb, NULL);
+
+	hotplug_bridge_dsi_cleanup(hpb);
+
+	device_set_node(hpb->dev, NULL);
+}
+
+static const struct platform_device_id hotplug_bridge_platform_ids[] = {
+	{ .name = "hotplug-dsi-bridge" },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, hotplug_bridge_platform_ids);
+
+static struct platform_driver hotplug_bridge_driver = {
+	.probe		= hotplug_bridge_probe,
+	.remove_new	= hotplug_bridge_remove,
+	.id_table	= hotplug_bridge_platform_ids,
+	.driver		= {
+		.name		= "hotplug-drm-bridge",
+	},
+};
+
+module_platform_driver(hotplug_bridge_driver);
+
+MODULE_AUTHOR("Luca Ceresoli <luca.ceresoli@bootlin.com>");
+MODULE_AUTHOR("Paul Kocialkowski <paul.kocialkowski@bootlin.com>");
+MODULE_DESCRIPTION("Hotplug DRM Bridge");
+MODULE_LICENSE("GPL");

-- 
2.34.1

[PATCH v2 5/5] misc: add ge-addon-connector driver

[Luca Ceresoli]

Add a driver to support the runtime hot-pluggable add-on connector on the
GE SUNH device. This connector allows connecting and disconnecting an
add-on to/from the main device to augment its features. Connection and
disconnection can happen at runtime at any moment without notice.

Different add-on models can be connected, and each has an EEPROM with a
model identifier at a fixed address.

The add-on hardware is added and removed using device tree overlay loading
and unloading.

Co-developed-by: Herve Codina <herve.codina@bootlin.com>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

This commit is new in v2.
---
 MAINTAINERS                      |   1 +
 drivers/misc/Kconfig             |  15 ++
 drivers/misc/Makefile            |   1 +
 drivers/misc/ge-sunh-connector.c | 464 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 481 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 672c26372c92..0bdb4fc496b8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9905,6 +9905,7 @@ F:	drivers/iio/pressure/mprls0025pa*
 HOTPLUG CONNECTOR FOR GE SUNH ADDONS
 M:	Luca Ceresoli <luca.ceresoli@bootlin.com>
 S:	Maintained
+F:	drivers/misc/ge-sunh-connector.c
 F:	Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
 
 HP BIOSCFG DRIVER
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 4fb291f0bf7c..99ef2eccbbaa 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -574,6 +574,21 @@ config NSM
 	  To compile this driver as a module, choose M here.
 	  The module will be called nsm.
 
+config GE_SUNH_CONNECTOR
+	tristate "GE SUNH hotplug add-on connector"
+	depends on OF
+	select OF_OVERLAY
+	select FW_LOADER
+	select NVMEM
+	select DRM_HOTPLUG_BRIDGE
+	help
+	  Driver for the runtime hot-pluggable add-on connector on the GE SUNH
+	  device. This connector allows connecting and disconnecting an add-on
+	  to/from the main device to augment its features. Connection and
+	  disconnection can be done at runtime at any moment without
+	  notice. Different add-on models can be connected, and each has an EEPROM
+	  with a model identifier at a fixed address.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index ea6ea5bbbc9c..d973de89bd19 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -68,3 +68,4 @@ obj-$(CONFIG_TMR_INJECT)	+= xilinx_tmr_inject.o
 obj-$(CONFIG_TPS6594_ESM)	+= tps6594-esm.o
 obj-$(CONFIG_TPS6594_PFSM)	+= tps6594-pfsm.o
 obj-$(CONFIG_NSM)		+= nsm.o
+obj-$(CONFIG_GE_SUNH_CONNECTOR)	+= ge-sunh-connector.o
diff --git a/drivers/misc/ge-sunh-connector.c b/drivers/misc/ge-sunh-connector.c
new file mode 100644
index 000000000000..a40bf4bb56bf
--- /dev/null
+++ b/drivers/misc/ge-sunh-connector.c
@@ -0,0 +1,464 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GE SUNH hotplug add-on connector
+ *
+ * Driver for the runtime hot-pluggable add-on connector on the GE SUNH
+ * device. Add-on connection is detected via GPIOs (+ a debugfs
+ * trigger). On connection, a "base" DT overlay is added that describes
+ * enough to reach the NVMEM cell with the model ID. Based on the ID, an
+ * add-on-specific overlay is loaded on top to describe everything else.
+ *
+ * Copyright (C) 2024, GE HealthCare
+ *
+ * Authors:
+ * Luca Ceresoli <luca.ceresoli@bootlin.com>
+ * Herve Codina <herve.codina@bootlin.com>
+ */
+
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/workqueue.h>
+
+enum sunh_conn_overlay_level {
+	SUNH_CONN_OVERLAY_BASE,
+	SUNH_CONN_OVERLAY_ADDON,
+	SUNH_CONN_OVERLAY_N_LEVELS
+};
+
+#define SUNH_CONN_N_STATUS_GPIOS 2
+static const char * const sunh_conn_status_gpio_name[SUNH_CONN_N_STATUS_GPIOS] = {
+	"plugged", "powergood"
+};
+
+struct sunh_conn {
+	struct device *dev;
+	struct gpio_desc *reset_gpio;
+	struct gpio_desc *status_gpio[SUNH_CONN_N_STATUS_GPIOS];
+
+	bool plugged;
+	int ovcs_id[SUNH_CONN_OVERLAY_N_LEVELS];
+	struct mutex ovl_mutex; // serialize overlay code
+	struct notifier_block nvmem_nb;
+	struct work_struct nvmem_notifier_work;
+
+	struct platform_device *hpb_pdev;
+	struct dentry *debugfs_root;
+};
+
+static int sunh_conn_insert_overlay(struct sunh_conn *conn,
+				    enum sunh_conn_overlay_level level,
+				    const char *filename)
+{
+	const struct firmware *fw;
+	int err;
+
+	err = request_firmware(&fw, filename, conn->dev);
+	if (err)
+		return dev_err_probe(conn->dev, err, "Error requesting overlay %s", filename);
+
+	dev_dbg(conn->dev, "insert overlay %d: %s", level, filename);
+	err = of_overlay_fdt_apply(fw->data, fw->size, &conn->ovcs_id[level], NULL);
+	if (err) {
+		int err2;
+
+		dev_err_probe(conn->dev, err, "Failed to apply overlay %s\n", filename);
+
+		/* changeset may be partially applied */
+		err2 = of_overlay_remove(&conn->ovcs_id[level]);
+		if (err2 < 0)
+			dev_err_probe(conn->dev, err2,
+				      "Failed to remove failed overlay %s\n", filename);
+	}
+
+	release_firmware(fw);
+	return err;
+}
+
+static int sunh_conn_load_base_overlay(struct sunh_conn *conn)
+{
+	int err = 0;
+
+	mutex_lock(&conn->ovl_mutex);
+
+	if (conn->ovcs_id[0] != 0) {
+		dev_dbg(conn->dev, "base overlay already loaded\n");
+		goto out_unlock;
+	}
+
+	err = sunh_conn_insert_overlay(conn, 0, "imx8mp-sundv1-addon-base.dtbo");
+
+out_unlock:
+	mutex_unlock(&conn->ovl_mutex);
+	return err;
+}
+
+static int sunh_conn_load_addon_overlay(struct sunh_conn *conn)
+{
+	u8 addon_id;
+	const char *filename;
+	int err;
+
+	mutex_lock(&conn->ovl_mutex);
+
+	if (conn->ovcs_id[0] == 0) {
+		dev_dbg(conn->dev, "base overlay not loaded\n");
+		err = -EINVAL;
+		goto out_unlock;
+	}
+
+	if (conn->ovcs_id[1] != 0) {
+		dev_dbg(conn->dev, "addon overlay already loaded\n");
+		err = -EEXIST;
+		goto out_unlock;
+	}
+
+	err = nvmem_cell_read_u8(conn->dev, "id", &addon_id);
+	if (err)
+		goto out_unlock;
+
+	dev_dbg(conn->dev, "Found add-on ID %d\n", addon_id);
+
+	switch (addon_id) {
+	case 23:
+		filename = "imx8mp-sundv1-addon-13.dtbo";
+		break;
+	case 24:
+		filename = "imx8mp-sundv1-addon-15.dtbo";
+		break;
+	case 25:
+		filename = "imx8mp-sundv1-addon-18.dtbo";
+		break;
+	default:
+		dev_warn(conn->dev, "Unknown add-on ID %d\n", addon_id);
+		err = -ENODEV;
+		goto out_unlock;
+	}
+
+	err = sunh_conn_insert_overlay(conn, 1, filename);
+
+out_unlock:
+	mutex_unlock(&conn->ovl_mutex);
+	return err;
+}
+
+static void sunh_conn_unload_overlays(struct sunh_conn *conn)
+{
+	int level = SUNH_CONN_OVERLAY_N_LEVELS;
+	int err;
+
+	mutex_lock(&conn->ovl_mutex);
+	while (level) {
+		level--;
+
+		if (conn->ovcs_id[level] == 0)
+			continue;
+
+		dev_dbg(conn->dev, "remove overlay %d (ovcs id %d)",
+			level, conn->ovcs_id[level]);
+
+		err = of_overlay_remove(&conn->ovcs_id[level]);
+		if (err)
+			dev_err_probe(conn->dev, err, "Failed to remove overlay %d\n", level);
+	}
+	mutex_unlock(&conn->ovl_mutex);
+}
+
+static void sunh_conn_reset(struct sunh_conn *conn, bool keep_reset)
+{
+	dev_dbg(conn->dev, "reset\n");
+
+	gpiod_set_value_cansleep(conn->reset_gpio, 1);
+
+	if (keep_reset)
+		return;
+
+	mdelay(10);
+	gpiod_set_value_cansleep(conn->reset_gpio, 0);
+	mdelay(10);
+}
+
+static int sunh_conn_detach(struct sunh_conn *conn)
+{
+	/* Cancel any pending NVMEM notification jobs */
+	cancel_work_sync(&conn->nvmem_notifier_work);
+
+	/* Unload previouly loaded overlays */
+	sunh_conn_unload_overlays(conn);
+
+	/* Set reset signal to have it set on next plug */
+	sunh_conn_reset(conn, true);
+
+	dev_info(conn->dev, "detached\n");
+	return 0;
+}
+
+static int sunh_conn_attach(struct sunh_conn *conn)
+{
+	int err;
+
+	/* Reset the plugged board in order to start from a stable state */
+	sunh_conn_reset(conn, false);
+
+	err = sunh_conn_load_base_overlay(conn);
+	if (err)
+		goto err;
+
+	/*
+	 * -EPROBE_DEFER can be due to NVMEM cell not yet available, so
+	 * don't give up, an NVMEM event could arrive later
+	 */
+	err = sunh_conn_load_addon_overlay(conn);
+	if (err && err != -EPROBE_DEFER)
+		goto err;
+
+	dev_info(conn->dev, "attached\n");
+	return 0;
+
+err:
+	sunh_conn_detach(conn);
+	return err;
+}
+
+static int sunh_conn_handle_event(struct sunh_conn *conn, bool plugged)
+{
+	int err;
+
+	if (plugged == conn->plugged)
+		return 0;
+
+	dev_info(conn->dev, "%s\n", plugged ? "connected" : "disconnected");
+
+	err = (plugged ?
+	       sunh_conn_attach(conn) :
+	       sunh_conn_detach(conn));
+
+	conn->plugged = plugged;
+
+	return err;
+}
+
+/*
+ * Return the current status of the connector as reported by the hardware.
+ *
+ * Returns:
+ * - 0 if not connected (any of the existing status GPIOs not asserted) or
+ *   no status GPIOs exist
+ * - 1 if connected in a stable manner (all status GPIOs are asserted)
+ * - a negative error code in case reading the GPIOs fail
+ */
+static int sunh_conn_get_connector_status(struct sunh_conn *conn)
+{
+	int status = 0;
+	int i;
+
+	for (i = 0; i < SUNH_CONN_N_STATUS_GPIOS; i++) {
+		int val;
+
+		if (!conn->status_gpio[i])
+			continue;
+
+		val = gpiod_get_value_cansleep(conn->status_gpio[i]);
+
+		if (val < 0) {
+			dev_err(conn->dev, "Error reading %s GPIO (%d)\n",
+				sunh_conn_status_gpio_name[i], val);
+			return val;
+		}
+
+		if (val == 0) {
+			dev_dbg(conn->dev, "%s GPIO deasserted\n",
+				sunh_conn_status_gpio_name[i]);
+			return 0;
+		}
+
+		status = 1;
+	}
+
+	return status;
+}
+
+static irqreturn_t sunh_conn_gpio_irq(int irq, void *data)
+{
+	struct sunh_conn *conn = data;
+	int conn_status;
+
+	conn_status = sunh_conn_get_connector_status(conn);
+	if (conn_status >= 0)
+		sunh_conn_handle_event(conn, conn_status);
+
+	return IRQ_HANDLED;
+}
+
+static int plugged_read(void *dat, u64 *val)
+{
+	struct sunh_conn *conn = dat;
+
+	*val = conn->plugged;
+
+	return 0;
+}
+
+static int plugged_write(void *dat, u64 val)
+{
+	struct sunh_conn *conn = dat;
+
+	if (val > 1)
+		return -EINVAL;
+
+	return sunh_conn_handle_event(conn, val);
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(plugged_fops, plugged_read, plugged_write, "%lld\n");
+
+static void sunh_conn_nvmem_notifier_work(struct work_struct *work)
+{
+	struct sunh_conn *conn = container_of(work, struct sunh_conn, nvmem_notifier_work);
+
+	sunh_conn_load_addon_overlay(conn);
+}
+
+static int sunh_conn_nvmem_notifier(struct notifier_block *nb, unsigned long action, void *arg)
+{
+	struct sunh_conn *conn = container_of(nb, struct sunh_conn, nvmem_nb);
+
+	if (action == NVMEM_CELL_ADD)
+		queue_work(system_power_efficient_wq, &conn->nvmem_notifier_work);
+
+	return NOTIFY_OK;
+}
+
+static int sunh_conn_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sunh_conn *conn;
+	int conn_status;
+	int err;
+	int i;
+
+	const struct platform_device_info hpb_info = {
+		.parent = dev,
+		.fwnode = dev->fwnode,
+		.of_node_reused = true,
+		.name = "hotplug-dsi-bridge",
+		.id = PLATFORM_DEVID_NONE,
+	};
+
+	/* Cannot load overlay from filesystem before rootfs is mounted */
+	if (system_state < SYSTEM_RUNNING)
+		return -EPROBE_DEFER;
+
+	conn = devm_kzalloc(dev, sizeof(*conn), GFP_KERNEL);
+	if (!conn)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, conn);
+	conn->dev = dev;
+
+	mutex_init(&conn->ovl_mutex);
+	INIT_WORK(&conn->nvmem_notifier_work, sunh_conn_nvmem_notifier_work);
+
+	conn->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(conn->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(conn->reset_gpio),
+				     "Error getting reset GPIO\n");
+
+	for (i = 0; i < SUNH_CONN_N_STATUS_GPIOS; i++) {
+		conn->status_gpio[i] =
+			devm_gpiod_get_optional(dev, sunh_conn_status_gpio_name[i], GPIOD_IN);
+		if (IS_ERR(conn->status_gpio[i]))
+			return dev_err_probe(dev, PTR_ERR(conn->status_gpio[i]),
+					     "Error getting %s GPIO\n",
+					     sunh_conn_status_gpio_name[i]);
+	}
+
+	conn->hpb_pdev = platform_device_register_full(&hpb_info);
+	if (IS_ERR(conn->hpb_pdev)) {
+		err = PTR_ERR(conn->hpb_pdev);
+		return dev_err_probe(dev, err, "Error registering DRM bridge\n");
+	}
+
+	conn->nvmem_nb.notifier_call = sunh_conn_nvmem_notifier;
+	err = nvmem_register_notifier(&conn->nvmem_nb);
+	if (err) {
+		dev_err_probe(dev, err, "Error registering NVMEM notifier\n");
+		goto err_unregister_drm_bridge;
+	}
+
+	for (i = 0; i < SUNH_CONN_N_STATUS_GPIOS; i++) {
+		if (conn->status_gpio[i]) {
+			err = devm_request_threaded_irq(dev, gpiod_to_irq(conn->status_gpio[i]),
+							NULL, sunh_conn_gpio_irq,
+							IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
+							IRQF_ONESHOT,
+							dev_name(dev), conn);
+			if (err) {
+				dev_err_probe(dev, err, "Error getting %s GPIO IRQ\n",
+					sunh_conn_status_gpio_name[i]);
+				goto err_nvmem_unregister_notifier;
+			}
+		}
+	}
+
+	conn_status = sunh_conn_get_connector_status(conn);
+	if (conn_status < 0) {
+		err = conn_status;
+		goto err_nvmem_unregister_notifier;
+	}
+
+	/* Ensure initial state is known and overlay loaded if plugged */
+	sunh_conn_handle_event(conn, conn_status);
+
+	conn->debugfs_root = debugfs_create_dir(dev_name(dev), NULL);
+	debugfs_create_file("plugged", 0644, conn->debugfs_root, conn, &plugged_fops);
+
+	return 0;
+
+err_nvmem_unregister_notifier:
+	nvmem_unregister_notifier(&conn->nvmem_nb);
+	cancel_work_sync(&conn->nvmem_notifier_work);
+err_unregister_drm_bridge:
+	platform_device_unregister(conn->hpb_pdev);
+	return err;
+}
+
+static void sunh_conn_remove(struct platform_device *pdev)
+{
+	struct sunh_conn *conn = platform_get_drvdata(pdev);
+
+	debugfs_remove(conn->debugfs_root);
+	sunh_conn_detach(conn);
+
+	nvmem_unregister_notifier(&conn->nvmem_nb);
+	cancel_work_sync(&conn->nvmem_notifier_work);
+
+	platform_device_unregister(conn->hpb_pdev);
+}
+
+static const struct of_device_id sunh_conn_dt_ids[] = {
+	{ .compatible = "ge,sunh-addon-connector" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sunh_conn_dt_ids);
+
+static struct platform_driver sunh_conn_driver = {
+	.driver = {
+		.name = "sunh-addon-connector",
+		.of_match_table = sunh_conn_dt_ids,
+	},
+	.probe = sunh_conn_probe,
+	.remove_new = sunh_conn_remove,
+};
+module_platform_driver(sunh_conn_driver);
+
+MODULE_AUTHOR("Luca Ceresoli <luca.ceresoli@bootlin.com>");
+MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>");
+MODULE_DESCRIPTION("GE SUNH hotplug add-on connector");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH v2 5/5] misc: add ge-addon-connector driver

[Greg Kroah-Hartman]

On Fri, May 10, 2024 at 09:10:41AM +0200, Luca Ceresoli wrote:
> Add a driver to support the runtime hot-pluggable add-on connector on the
> GE SUNH device. This connector allows connecting and disconnecting an
> add-on to/from the main device to augment its features. Connection and
> disconnection can happen at runtime at any moment without notice.
> 
> Different add-on models can be connected, and each has an EEPROM with a
> model identifier at a fixed address.
> 
> The add-on hardware is added and removed using device tree overlay loading
> and unloading.
> 
> Co-developed-by: Herve Codina <herve.codina@bootlin.com>
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> This commit is new in v2.
> ---
>  MAINTAINERS                      |   1 +
>  drivers/misc/Kconfig             |  15 ++
>  drivers/misc/Makefile            |   1 +
>  drivers/misc/ge-sunh-connector.c | 464 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 481 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 672c26372c92..0bdb4fc496b8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9905,6 +9905,7 @@ F:	drivers/iio/pressure/mprls0025pa*
>  HOTPLUG CONNECTOR FOR GE SUNH ADDONS
>  M:	Luca Ceresoli <luca.ceresoli@bootlin.com>
>  S:	Maintained
> +F:	drivers/misc/ge-sunh-connector.c
>  F:	Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
>  
>  HP BIOSCFG DRIVER
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 4fb291f0bf7c..99ef2eccbbaa 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -574,6 +574,21 @@ config NSM
>  	  To compile this driver as a module, choose M here.
>  	  The module will be called nsm.
>  
> +config GE_SUNH_CONNECTOR
> +	tristate "GE SUNH hotplug add-on connector"
> +	depends on OF
> +	select OF_OVERLAY
> +	select FW_LOADER
> +	select NVMEM
> +	select DRM_HOTPLUG_BRIDGE

Can these be depends instead of select?  'select' causes dependencies
that are hard, if not almost impossible, to detect at times why
something is being enabled.

> +	help
> +	  Driver for the runtime hot-pluggable add-on connector on the GE SUNH
> +	  device. This connector allows connecting and disconnecting an add-on
> +	  to/from the main device to augment its features. Connection and
> +	  disconnection can be done at runtime at any moment without
> +	  notice. Different add-on models can be connected, and each has an EEPROM
> +	  with a model identifier at a fixed address.

Module name?


> +static void sunh_conn_reset(struct sunh_conn *conn, bool keep_reset)
> +{
> +	dev_dbg(conn->dev, "reset\n");

ftrace is your friend.

> +static int sunh_conn_handle_event(struct sunh_conn *conn, bool plugged)
> +{
> +	int err;
> +
> +	if (plugged == conn->plugged)
> +		return 0;
> +
> +	dev_info(conn->dev, "%s\n", plugged ? "connected" : "disconnected");

Please remove debugging code from stuff you want to see merged.

Same for all dev_info() calls here, when drivers work properly, they are
quiet.

thanks,

greg k-h

Re: [PATCH v2 1/5] dt-bindings: connector: add GE SUNH hotplug addon connector

[Rob Herring (Arm)]

On Fri, 10 May 2024 09:10:37 +0200, Luca Ceresoli wrote:
> Add bindings for the GE SUNH add-on connector. This is a physical,
> hot-pluggable connector that allows to attach and detach at runtime an
> add-on adding peripherals on non-discoverable busses.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> NOTE: the second and third examples fail 'make dt_binding_check' because
>       they are example of DT overlay code -- I'm not aware of a way to
>       validate overlay examples as of now
> 
> This patch is new in v2.
> ---
>  .../connector/ge,sunh-addon-connector.yaml         | 197 +++++++++++++++++++++
>  MAINTAINERS                                        |   5 +
>  2 files changed, 202 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.example.dts:49.9-14 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:427: Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240510-hotplug-drm-bridge-v2-1-ec32f2c66d56@bootlin.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 5/5] misc: add ge-addon-connector driver

[Arnd Bergmann]

On Fri, May 10, 2024, at 09:55, Greg Kroah-Hartman wrote:
> On Fri, May 10, 2024 at 09:10:41AM +0200, Luca Ceresoli wrote:
>>  
>> +config GE_SUNH_CONNECTOR
>> +	tristate "GE SUNH hotplug add-on connector"
>> +	depends on OF
>> +	select OF_OVERLAY
>> +	select FW_LOADER
>> +	select NVMEM
>> +	select DRM_HOTPLUG_BRIDGE
>
> Can these be depends instead of select?  'select' causes dependencies
> that are hard, if not almost impossible, to detect at times why
> something is being enabled.

I think FW_LOADER needs to be 'select' since it is normally
a hidden symbol and gets selected by its users, all the other
ones should be 'depends on'.

    Arnd

Re: [PATCH v2 1/5] dt-bindings: connector: add GE SUNH hotplug addon connector

[Luca Ceresoli]

Hello Rob,

On Fri, 10 May 2024 03:41:35 -0500
"Rob Herring (Arm)" <robh@kernel.org> wrote:

> On Fri, 10 May 2024 09:10:37 +0200, Luca Ceresoli wrote:
> > Add bindings for the GE SUNH add-on connector. This is a physical,
> > hot-pluggable connector that allows to attach and detach at runtime an
> > add-on adding peripherals on non-discoverable busses.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > 
> > ---
> > 
> > NOTE: the second and third examples fail 'make dt_binding_check' because
> >       they are example of DT overlay code -- I'm not aware of a way to
> >       validate overlay examples as of now

As mentioned here...

> > 
> > This patch is new in v2.
> > ---
> >  .../connector/ge,sunh-addon-connector.yaml         | 197 +++++++++++++++++++++
> >  MAINTAINERS                                        |   5 +
> >  2 files changed, 202 insertions(+)
> >   
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Error: Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.example.dts:49.9-14 syntax error
> FATAL ERROR: Unable to parse input tree

...this is expected.

Any hints on how this can be managed in bindings examples would be very
useful.

Best regards,

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 5/5] misc: add ge-addon-connector driver

[Luca Ceresoli]

Hello Greg,

thanks for reviewing.

On Fri, 10 May 2024 08:55:29 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Fri, May 10, 2024 at 09:10:41AM +0200, Luca Ceresoli wrote:
> > Add a driver to support the runtime hot-pluggable add-on connector on the
> > GE SUNH device. This connector allows connecting and disconnecting an
> > add-on to/from the main device to augment its features. Connection and
> > disconnection can happen at runtime at any moment without notice.
> > 
> > Different add-on models can be connected, and each has an EEPROM with a
> > model identifier at a fixed address.
> > 
> > The add-on hardware is added and removed using device tree overlay loading
> > and unloading.
> > 
> > Co-developed-by: Herve Codina <herve.codina@bootlin.com>
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > 
> > ---
> > 
> > This commit is new in v2.
> > ---
> >  MAINTAINERS                      |   1 +
> >  drivers/misc/Kconfig             |  15 ++
> >  drivers/misc/Makefile            |   1 +
> >  drivers/misc/ge-sunh-connector.c | 464 +++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 481 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 672c26372c92..0bdb4fc496b8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9905,6 +9905,7 @@ F:	drivers/iio/pressure/mprls0025pa*
> >  HOTPLUG CONNECTOR FOR GE SUNH ADDONS
> >  M:	Luca Ceresoli <luca.ceresoli@bootlin.com>
> >  S:	Maintained
> > +F:	drivers/misc/ge-sunh-connector.c
> >  F:	Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
> >  
> >  HP BIOSCFG DRIVER
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 4fb291f0bf7c..99ef2eccbbaa 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -574,6 +574,21 @@ config NSM
> >  	  To compile this driver as a module, choose M here.
> >  	  The module will be called nsm.
> >  
> > +config GE_SUNH_CONNECTOR
> > +	tristate "GE SUNH hotplug add-on connector"
> > +	depends on OF
> > +	select OF_OVERLAY
> > +	select FW_LOADER
> > +	select NVMEM
> > +	select DRM_HOTPLUG_BRIDGE  
> 
> Can these be depends instead of select?  'select' causes dependencies
> that are hard, if not almost impossible, to detect at times why
> something is being enabled.

(see reply to Arnd's follow-up e-mail for this)

> > +	help
> > +	  Driver for the runtime hot-pluggable add-on connector on the GE SUNH
> > +	  device. This connector allows connecting and disconnecting an add-on
> > +	  to/from the main device to augment its features. Connection and
> > +	  disconnection can be done at runtime at any moment without
> > +	  notice. Different add-on models can be connected, and each has an EEPROM
> > +	  with a model identifier at a fixed address.  
> 
> Module name?

OK, will add.

> > +static void sunh_conn_reset(struct sunh_conn *conn, bool keep_reset)
> > +{
> > +	dev_dbg(conn->dev, "reset\n");  
> 
> ftrace is your friend.

ACK.

> > +static int sunh_conn_handle_event(struct sunh_conn *conn, bool plugged)
> > +{
> > +	int err;
> > +
> > +	if (plugged == conn->plugged)
> > +		return 0;
> > +
> > +	dev_info(conn->dev, "%s\n", plugged ? "connected" : "disconnected");  
> 
> Please remove debugging code from stuff you want to see merged.
> 
> Same for all dev_info() calls here, when drivers work properly, they are
> quiet.

While agree for other dev_info() calls, this one seems quite similar in
principle to the link up/down messages that get logged by the MII code
at [0]:

  [347229.872315] asix 1-1.3.2:1.0 enx000cf616fecb: link up, 100Mbps,
  full-duplex, lpa 0xC5E1 [347229.920449] asix 1-1.3.2:1.0 enx000cf616fecb: link down

In my case it is logging that a removable part of the hardware has been
added or removed, which appears useful. Do you think it make sense in
this scenario?

Luca

[0] https://elixir.bootlin.com/linux/v6.8.9/source/drivers/net/mii.c#L557

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 5/5] misc: add ge-addon-connector driver

[Luca Ceresoli]

Hello Greg, Arnd,

On Fri, 10 May 2024 12:24:06 +0200
"Arnd Bergmann" <arnd@arndb.de> wrote:

> On Fri, May 10, 2024, at 09:55, Greg Kroah-Hartman wrote:
> > On Fri, May 10, 2024 at 09:10:41AM +0200, Luca Ceresoli wrote:  
> >>  
> >> +config GE_SUNH_CONNECTOR
> >> +	tristate "GE SUNH hotplug add-on connector"
> >> +	depends on OF
> >> +	select OF_OVERLAY
> >> +	select FW_LOADER
> >> +	select NVMEM
> >> +	select DRM_HOTPLUG_BRIDGE  
> >
> > Can these be depends instead of select?  'select' causes dependencies
> > that are hard, if not almost impossible, to detect at times why
> > something is being enabled.  
> 
> I think FW_LOADER needs to be 'select' since it is normally
> a hidden symbol and gets selected by its users, all the other
> ones should be 'depends on'.

I see, makes sense.

And as you pointed that out, I realize perhaps DRM_HOTPLUG_BRIDGE could
become a hidden symbol as it's not expected to be used alone.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 5/5] misc: add ge-addon-connector driver

[Arnd Bergmann]

On Fri, May 10, 2024, at 12:54, Luca Ceresoli wrote:
> On Fri, 10 May 2024 12:24:06 +0200 "Arnd Bergmann" <arnd@arndb.de> wrote:
>> On Fri, May 10, 2024, at 09:55, Greg Kroah-Hartman wrote:
>> > On Fri, May 10, 2024 at 09:10:41AM +0200, Luca Ceresoli wrote:  
>> >>  
>> >> +config GE_SUNH_CONNECTOR
>> >> +	tristate "GE SUNH hotplug add-on connector"
>> >> +	depends on OF
>> >> +	select OF_OVERLAY
>> >> +	select FW_LOADER
>> >> +	select NVMEM
>> >> +	select DRM_HOTPLUG_BRIDGE  
>> >
>> > Can these be depends instead of select?  'select' causes dependencies
>> > that are hard, if not almost impossible, to detect at times why
>> > something is being enabled.  
>> 
>> I think FW_LOADER needs to be 'select' since it is normally
>> a hidden symbol and gets selected by its users, all the other
>> ones should be 'depends on'.
>
> I see, makes sense.
>
> And as you pointed that out, I realize perhaps DRM_HOTPLUG_BRIDGE could
> become a hidden symbol as it's not expected to be used alone.

It's slightly easier to keep it as a visible symbol
with 'depends on' though, since otherwise you have to
add 'depends on' statments for anything that DRM_HOTPLUG_BRIDGE
in turn depends on, most notably DRM itself.

     Arnd

Re: [PATCH v2 5/5] misc: add ge-addon-connector driver

[Greg Kroah-Hartman]

On Fri, May 10, 2024 at 12:54:17PM +0200, Luca Ceresoli wrote:
> > > +static int sunh_conn_handle_event(struct sunh_conn *conn, bool plugged)
> > > +{
> > > +	int err;
> > > +
> > > +	if (plugged == conn->plugged)
> > > +		return 0;
> > > +
> > > +	dev_info(conn->dev, "%s\n", plugged ? "connected" : "disconnected");  
> > 
> > Please remove debugging code from stuff you want to see merged.
> > 
> > Same for all dev_info() calls here, when drivers work properly, they are
> > quiet.
> 
> While agree for other dev_info() calls, this one seems quite similar in
> principle to the link up/down messages that get logged by the MII code
> at [0]:
> 
>   [347229.872315] asix 1-1.3.2:1.0 enx000cf616fecb: link up, 100Mbps,
>   full-duplex, lpa 0xC5E1 [347229.920449] asix 1-1.3.2:1.0 enx000cf616fecb: link down
> 
> In my case it is logging that a removable part of the hardware has been
> added or removed, which appears useful. Do you think it make sense in
> this scenario?

Nope, sorry, again, when drivers are working properly, they should be
quiet otherwise they just fill up the log with unneeded messages.

thanks,

greg k-h

Re: [PATCH v2 1/5] dt-bindings: connector: add GE SUNH hotplug addon connector

[Rob Herring]

On Fri, May 10, 2024 at 5:37 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
>
> Hello Rob,
>
> On Fri, 10 May 2024 03:41:35 -0500
> "Rob Herring (Arm)" <robh@kernel.org> wrote:
>
> > On Fri, 10 May 2024 09:10:37 +0200, Luca Ceresoli wrote:
> > > Add bindings for the GE SUNH add-on connector. This is a physical,
> > > hot-pluggable connector that allows to attach and detach at runtime an
> > > add-on adding peripherals on non-discoverable busses.
> > >
> > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > >
> > > ---
> > >
> > > NOTE: the second and third examples fail 'make dt_binding_check' because
> > >       they are example of DT overlay code -- I'm not aware of a way to
> > >       validate overlay examples as of now
>
> As mentioned here...
>
> > >
> > > This patch is new in v2.
> > > ---
> > >  .../connector/ge,sunh-addon-connector.yaml         | 197 +++++++++++++++++++++
> > >  MAINTAINERS                                        |   5 +
> > >  2 files changed, 202 insertions(+)
> > >
> >
> > My bot found errors running 'make dt_binding_check' on your patch:
> >
> > yamllint warnings/errors:
> >
> > dtschema/dtc warnings/errors:
> > Error: Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.example.dts:49.9-14 syntax error
> > FATAL ERROR: Unable to parse input tree
>
> ...this is expected.
>
> Any hints on how this can be managed in bindings examples would be very
> useful.

Overlays in examples are not supported. Add actual .dtso files if you
want examples of overlays (maybe you did, shrug).

Overlays are somewhat orthogonal to bindings. Bindings define the ABI.
It only makes sense to validate applied overlays. Now maybe overlays
contain complete nodes and we could validate those, but that's a
problem for actual overlay files and not something we need to
complicate examples with.

Rob

Re: [PATCH v2 1/5] dt-bindings: connector: add GE SUNH hotplug addon connector

[Luca Ceresoli]

Hello Rob,

On Fri, 10 May 2024 08:22:53 -0500
Rob Herring <robh@kernel.org> wrote:

> On Fri, May 10, 2024 at 5:37 AM Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> >
> > Hello Rob,
> >
> > On Fri, 10 May 2024 03:41:35 -0500
> > "Rob Herring (Arm)" <robh@kernel.org> wrote:
> >  
> > > On Fri, 10 May 2024 09:10:37 +0200, Luca Ceresoli wrote:  
> > > > Add bindings for the GE SUNH add-on connector. This is a physical,
> > > > hot-pluggable connector that allows to attach and detach at runtime an
> > > > add-on adding peripherals on non-discoverable busses.
> > > >
> > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > > >
> > > > ---
> > > >
> > > > NOTE: the second and third examples fail 'make dt_binding_check' because
> > > >       they are example of DT overlay code -- I'm not aware of a way to
> > > >       validate overlay examples as of now  
> >
> > As mentioned here...
> >  
> > > >
> > > > This patch is new in v2.
> > > > ---
> > > >  .../connector/ge,sunh-addon-connector.yaml         | 197 +++++++++++++++++++++
> > > >  MAINTAINERS                                        |   5 +
> > > >  2 files changed, 202 insertions(+)
> > > >  
> > >
> > > My bot found errors running 'make dt_binding_check' on your patch:
> > >
> > > yamllint warnings/errors:
> > >
> > > dtschema/dtc warnings/errors:
> > > Error: Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.example.dts:49.9-14 syntax error
> > > FATAL ERROR: Unable to parse input tree  
> >
> > ...this is expected.
> >
> > Any hints on how this can be managed in bindings examples would be very
> > useful.  
> 
> Overlays in examples are not supported. Add actual .dtso files if you
> want examples of overlays (maybe you did, shrug).
> 
> Overlays are somewhat orthogonal to bindings. Bindings define the ABI.
> It only makes sense to validate applied overlays. Now maybe overlays
> contain complete nodes and we could validate those, but that's a
> problem for actual overlay files and not something we need to
> complicate examples with.

Many thanks for the insights.

The reason I added overlays in the bindings examples is that this
specific device calls for overlays by its very nature. And in fact the
implementation is based on overlays.

However I understand the reasons for not having overlays in examples. I
think I can just remove the two examples and mention the nvmem-cells
and nvmem-cell-names nodes as regular properties, and explain in their
descriptions that these are supposed to be loaded via overlays. Quick
draft:

properties:
  nvmem-cell-names:
    items:
      - const: speed-bin

  nvmem-cells:
    maxItems: 1
    description:
      NVMEM cell containing the add-on model ID for the add-on that is
      inserted. Multiple add-on models can be connected, and in order
      to find out the exact model connected all of them have an EEPROM
      at the same I2C bus and address with an ID at the same offset. By
      its nature, this and the nvmem-cell-names nodes are supposed to be
      added by an overlay once ad add-on is detected. So they must not
      be present in the initial device tree, and they must be added by
      an overlay before the add-on can be used.

Looks reasonable?

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 5/5] misc: add ge-addon-connector driver

[Luca Ceresoli]

Hi Greg, Arnd,

On Fri, 10 May 2024 12:57:24 +0200
"Arnd Bergmann" <arnd@arndb.de> wrote:

> On Fri, May 10, 2024, at 12:54, Luca Ceresoli wrote:
> > On Fri, 10 May 2024 12:24:06 +0200 "Arnd Bergmann" <arnd@arndb.de> wrote:  
> >> On Fri, May 10, 2024, at 09:55, Greg Kroah-Hartman wrote:  
> >> > On Fri, May 10, 2024 at 09:10:41AM +0200, Luca Ceresoli wrote:    
> >> >>  
> >> >> +config GE_SUNH_CONNECTOR
> >> >> +	tristate "GE SUNH hotplug add-on connector"
> >> >> +	depends on OF
> >> >> +	select OF_OVERLAY
> >> >> +	select FW_LOADER
> >> >> +	select NVMEM
> >> >> +	select DRM_HOTPLUG_BRIDGE    
> >> >
> >> > Can these be depends instead of select?  'select' causes dependencies
> >> > that are hard, if not almost impossible, to detect at times why
> >> > something is being enabled.    
> >> 
> >> I think FW_LOADER needs to be 'select' since it is normally
> >> a hidden symbol and gets selected by its users, all the other
> >> ones should be 'depends on'.  
> >
> > I see, makes sense.
> >
> > And as you pointed that out, I realize perhaps DRM_HOTPLUG_BRIDGE could
> > become a hidden symbol as it's not expected to be used alone.  
> 
> It's slightly easier to keep it as a visible symbol
> with 'depends on' though, since otherwise you have to
> add 'depends on' statments for anything that DRM_HOTPLUG_BRIDGE
> in turn depends on, most notably DRM itself.

I see, sure. Thanks both, changes applied locally.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 1/5] dt-bindings: connector: add GE SUNH hotplug addon connector

[Rob Herring]

On Fri, May 10, 2024 at 09:10:37AM +0200, Luca Ceresoli wrote:
> Add bindings for the GE SUNH add-on connector. This is a physical,
> hot-pluggable connector that allows to attach and detach at runtime an
> add-on adding peripherals on non-discoverable busses.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> NOTE: the second and third examples fail 'make dt_binding_check' because
>       they are example of DT overlay code -- I'm not aware of a way to
>       validate overlay examples as of now
> 
> This patch is new in v2.
> ---
>  .../connector/ge,sunh-addon-connector.yaml         | 197 +++++++++++++++++++++
>  MAINTAINERS                                        |   5 +
>  2 files changed, 202 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml b/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
> new file mode 100644
> index 000000000000..c7ac62e5f2c9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
> @@ -0,0 +1,197 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/connector/ge,sunh-addon-connector.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GE SUNH hotplug add-on connector
> +
> +maintainers:
> +  - Luca Ceresoli <luca.ceresoli@bootlin.com>
> +
> +description:
> +  Represent the physical connector present on GE SUNH devices that allows
> +  to attach and detach at runtime an add-on adding peripherals on
> +  non-discoverable busses.
> +
> +  This connector has status GPIOs to notify the connection status to the
> +  CPU and a reset GPIO to allow the CPU to reset all the peripherals on the
> +  add-on. It also has a 4-lane MIPI DSI bus.
> +
> +  Add-on removal can happen at any moment under user control and without
> +  prior notice to the CPU, making all of its components not usable
> +  anymore. Later on, the same or a different add-on model can be connected.

Is there any documentation for this connector?

Is the connector supposed to be generic in that any board with any SoC 
could have it? If so, the connector needs to be able to remap things so 
overlays aren't tied to the base dts, but only the connector. If not, 
then doing that isn't required, but still a good idea IMO.

> +
> +properties:
> +  compatible:
> +    const: ge,sunh-addon-connector
> +
> +  reset-gpios:
> +    description: An output GPIO to reset the peripherals on the add-on.
> +    maxItems: 1
> +
> +  plugged-gpios:
> +    description: An input GPIO that is asserted if and only if an add-on is
> +      physically connected.
> +    maxItems: 1
> +
> +  powergood-gpios:
> +    description: An input GPIO that is asserted if and only if power rails
> +      on the add-on are stable.
> +    maxItems: 1
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    description: OF graph bindings modeling the MIPI DSI bus across the
> +      connector. The connector splits the video pipeline in a fixed part
> +      and a removable part.
> +
> +      The fixed part of the video pipeline includes all components up to
> +      the display controller and 0 or more bridges. The removable part
> +      includes any bridges and any other components up to the panel.
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: The MIPI DSI bus line from the CPU to the connector.
> +          The remote-endpoint sub-node must point to the last non-removable
> +          component of the video pipeline.
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +
> +        description: The MIPI DSI bus line from the connector to the
> +          add-on. The remote-endpoint sub-node must point to the first
> +          add-on component of the video pipeline. As it describes the
> +          hot-pluggable hardware, the endpoint node cannot be filled until
> +          an add-on is detected, so this needs to be done by a device tree
> +          overlay at runtime.
> +
> +    required:
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  # Main DTS describing the "main" board up to the connector
> +  - |
> +    / {
> +        #include <dt-bindings/gpio/gpio.h>
> +
> +        addon_connector: addon-connector {

Just 'connector' for the node name.

> +            compatible = "ge,sunh-addon-connector";
> +            reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
> +            plugged-gpios = <&gpio1 2 GPIO_ACTIVE_LOW>;
> +            powergood-gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;
> +
> +            ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                    reg = <0>;
> +
> +                    hotplug_conn_dsi_in: endpoint {
> +                        remote-endpoint = <&previous_bridge_out>;
> +                    };
> +                };
> +
> +                port@1 {
> +                    reg = <1>;
> +
> +                    hotplug_conn_dsi_out: endpoint {
> +                        // remote-endpoint to be added by overlay
> +                    };
> +                };
> +            };
> +        };
> +    };
> +
> +  # "base" overlay describing the common components on every add-on that
> +  # are required to read the model ID

This is located on the add-on board, right?

Is it really any better to have this as a separate overlay rather than 
just making it an include? Better to have just 1 overlay per board 
applied atomically than splitting it up.

> +  - |
> +    &i2c1 {

Generally, I think everything on an add-on board should be underneath 
the connector node. For starters, that makes controlling probing and 
removal of devices easier. For example, you'll want to handle 
reset-gpios and powergood-gpios before any devices 'appear'. Otherwise, 
you add devices on i2c1, start probing them, and then reset them at some 
async time?

For i2c, it could look something like this:

connector {
  i2c {
	i2c-parent = <&i2c1>;

	eeprom@50 {...};
  };
};

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        eeprom@50 {
> +            compatible = "atmel,24c64";
> +            reg = <0x50>;
> +
> +            nvmem-layout {
> +                compatible = "fixed-layout";
> +                #address-cells = <1>;
> +                #size-cells = <1>;
> +
> +                addon_model_id: addon-model-id@400 {
> +                    reg = <0x400 0x1>;
> +                };
> +            };
> +        };
> +    };
> +
> +    &addon_connector {
> +        nvmem-cells = <&addon_model_id>;
> +        nvmem-cell-names = "id";
> +    };

It's kind of sad that an addon board has an eeprom to identify it, but 
it's not itself discoverable...

Do you load the first overlay and then from it decide which 
specific overlay to apply?

Rob

Re: [PATCH v2 0/5] Add support for GE SUNH hot-pluggable connector (was: "drm: add support for hot-pluggable bridges")

[Rob Herring]

On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:
> 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, using device tree overlays.
> 
> This is the v2 of "drm: add support for hot-pluggable bridges" [0] which
> was however more limited in scope, covering only the DRM aspects. This new
> series also takes a different approach to the DRM bridge instantiation.
> 
> [0] https://lore.kernel.org/all/20240326-hotplug-drm-bridge-v1-0-4b51b5eb75d5@bootlin.com/
> 
> 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 allowign 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 not supported

Not true. Device specific applying of overlays has been supported 
since we merged DT overlay support. What's not supported is a general 
purpose interface to userspace to change any part of the DT at any point 
in time.

>  2. if enabled, overlay (un)loading exposes several bugs

Hence why there is no general purpose interface.

>  3. the DRM subsystem assumes video bridges are non-removable

Rob

Re: [PATCH v2 1/5] dt-bindings: connector: add GE SUNH hotplug addon connector

[Luca Ceresoli]

Hello Rob,

+cc Srinivas and Miquèl for the NVMEM cell discussion below

On Fri, 10 May 2024 11:36:25 -0500
Rob Herring <robh@kernel.org> wrote:

> On Fri, May 10, 2024 at 09:10:37AM +0200, Luca Ceresoli wrote:
> > Add bindings for the GE SUNH add-on connector. This is a physical,
> > hot-pluggable connector that allows to attach and detach at runtime an
> > add-on adding peripherals on non-discoverable busses.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

[...]

> > +++ b/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
> > @@ -0,0 +1,197 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/connector/ge,sunh-addon-connector.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: GE SUNH hotplug add-on connector
> > +
> > +maintainers:
> > +  - Luca Ceresoli <luca.ceresoli@bootlin.com>
> > +
> > +description:
> > +  Represent the physical connector present on GE SUNH devices that allows
> > +  to attach and detach at runtime an add-on adding peripherals on
> > +  non-discoverable busses.
> > +
> > +  This connector has status GPIOs to notify the connection status to the
> > +  CPU and a reset GPIO to allow the CPU to reset all the peripherals on the
> > +  add-on. It also has a 4-lane MIPI DSI bus.
> > +
> > +  Add-on removal can happen at any moment under user control and without
> > +  prior notice to the CPU, making all of its components not usable
> > +  anymore. Later on, the same or a different add-on model can be connected.  
> 
> Is there any documentation for this connector?
> 
> Is the connector supposed to be generic in that any board with any SoC 
> could have it? If so, the connector needs to be able to remap things so 
> overlays aren't tied to the base dts, but only the connector. If not, 
> then doing that isn't required, but still a good idea IMO.

It is not generic. The connector pinout is very specific to this
product, and there is no public documentation.

> > +examples:
> > +  # Main DTS describing the "main" board up to the connector
> > +  - |
> > +    / {
> > +        #include <dt-bindings/gpio/gpio.h>
> > +
> > +        addon_connector: addon-connector {  
> 
> Just 'connector' for the node name.

OK

> > +            compatible = "ge,sunh-addon-connector";
> > +            reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
> > +            plugged-gpios = <&gpio1 2 GPIO_ACTIVE_LOW>;
> > +            powergood-gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;
> > +
> > +            ports {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +
> > +                port@0 {
> > +                    reg = <0>;
> > +
> > +                    hotplug_conn_dsi_in: endpoint {
> > +                        remote-endpoint = <&previous_bridge_out>;
> > +                    };
> > +                };
> > +
> > +                port@1 {
> > +                    reg = <1>;
> > +
> > +                    hotplug_conn_dsi_out: endpoint {
> > +                        // remote-endpoint to be added by overlay
> > +                    };
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +  # "base" overlay describing the common components on every add-on that
> > +  # are required to read the model ID  
> 
> This is located on the add-on board, right?

Exactly. Each add-on has an EEPROM with the add-on model ID stored
along with other data.

> Is it really any better to have this as a separate overlay rather than 
> just making it an include? Better to have just 1 overlay per board 
> applied atomically than splitting it up.

(see below)

> > +  - |
> > +    &i2c1 {  
> 
> Generally, I think everything on an add-on board should be underneath 
> the connector node. For starters, that makes controlling probing and 
> removal of devices easier. For example, you'll want to handle 
> reset-gpios and powergood-gpios before any devices 'appear'. Otherwise, 
> you add devices on i2c1, start probing them, and then reset them at some 
> async time?

This is not a problem because the code is asserting reset before
loading the first overlay. From patch 5/5:

    static int sunh_conn_attach(struct sunh_conn *conn)
    {
	int err;

	/* Reset the plugged board in order to start from a stable state */
	sunh_conn_reset(conn, false);

	err = sunh_conn_load_base_overlay(conn);
        ...
    }

> For i2c, it could look something like this:
> 
> connector {
>   i2c {
> 	i2c-parent = <&i2c1>;
> 
> 	eeprom@50 {...};
>   };
> };

I think this can be done, but I need to evaluate what is needed in the
driver code to support it.

> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        eeprom@50 {
> > +            compatible = "atmel,24c64";
> > +            reg = <0x50>;
> > +
> > +            nvmem-layout {
> > +                compatible = "fixed-layout";
> > +                #address-cells = <1>;
> > +                #size-cells = <1>;
> > +
> > +                addon_model_id: addon-model-id@400 {
> > +                    reg = <0x400 0x1>;
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +    &addon_connector {
> > +        nvmem-cells = <&addon_model_id>;
> > +        nvmem-cell-names = "id";
> > +    };  
> 
> It's kind of sad that an addon board has an eeprom to identify it, but 
> it's not itself discoverable...

Not sure I got what you mean exactly here, sorry.

The add-on board is discoverable in the sense that it has a GPIO
(actually two) to be notified of plug/unplug, and it has a way to
describe itself by reading a model ID. Conceptually this is what HDMI
monitors do: an HPD pin and an EEPROM at a fixed address with data at
fixed locations.

If you mean the addon_connector node might be avoided, then I kind of
agree, but this seems not what the NVMEM DT representation expects so
I'm not sure removing it would be correct in the first place.

Srinivas, do you have any insights to share about this? The topic is a
device tree overlay that describes a hotplug-removable add-on, and in
particular the EEPROM present on all add-ons to provide the add-on
model ID.

> Do you load the first overlay and then from it decide which 
> specific overlay to apply?

Exactly.

The first overlay (the example you quoted above) describes enough to
reach the model ID in the EEPROM, and this is identical for all add-on
models. The second add-on is model-specific, there is one for each
model, and the model ID allows to know which one to load.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 0/5] Add support for GE SUNH hot-pluggable connector (was: "drm: add support for hot-pluggable bridges")

[Luca Ceresoli]

Hello Rob,

On Fri, 10 May 2024 11:44:49 -0500
Rob Herring <robh@kernel.org> wrote:

> On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:

[...]

> > 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 not supported  
> 
> Not true. Device specific applying of overlays has been supported 
> since we merged DT overlay support. What's not supported is a general 
> purpose interface to userspace to change any part of the DT at any point 
> in time.

I think I should replace "supported" with "exposed [to user space]". In
other words, there is no user of of_overlay_fdt_apply() /
of_overlay_remove*() in mainline Linux, except in unittest. Would it be
a correct rewording?

> >  2. if enabled, overlay (un)loading exposes several bugs  
> 
> Hence why there is no general purpose interface.

Absolutely.

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 0/5] Add support for GE SUNH hot-pluggable connector (was: "drm: add support for hot-pluggable bridges")

[Daniel Vetter]

Apologies for missing v1 ...

On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:
> 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

So I think at a high level this design approach makes sense, but the
implementation needs some serious thought. One big thing upfront though,
we need to have a clear plan for the overlay hotunload issues, otherwise
trying to make drm bridges hotpluggable makes no sense to me. Hotunload is
very, very tricky, full of lifetime issues, and those need to be sorted
out first or we're just trying to build a castle on quicksand.

For bridges itself I don't think the current locking works. You're trying
to really cleverly hide it all behind a normal-looking bridge driver, but
there's many things beyond that which will blow up if bridges just
disappear. Most importantly the bridge states part of an atomic update.

Now in drm we have drm_connector as the only hotunpluggable thing, and it
took years to sort out all the issues. I think we should either model the
bridge hotunplug locking after that, or just outright reuse the connector
locking and lifetime rules. I much prefer the latter personally.

Anyway the big issues:

- We need to refcount the hotpluggable bridges, because software (like
  atomic state updates) might hang onto pointers for longer than the
  bridge physically exists. Assuming that you can all tear it down
  synchronously will not work.

  If we reuse connector locking/lifetime then we could put the
  hotpluggable part of the bridge chain into the drm_connector, since that
  already has refcounting as needed. It would mean that finding the next
  bridge in the chain becomes a lot more tricky though. With that model
  we'd create a new connector every time the bridge is hotplugged, which I
  think is also the cleaner model (because you might plug in a hdmi
  connector after a panel, so things like the connector type change).
  
- No notifiers please. The create a locking mess with inversions, and
  especially for hotunplug they create the illusion that you can
  synchronously keep up to date with hardware state. That's not possible.
  Fundamentally all bridge drivers which might be hotunplugged need to be
  able to cope with the hardware disappearing any momemnt.

  Most likely changes/fixes we need to make overlay hotunload work will
  impact how exactly this works all ...

  Also note that the entire dance around correctly stopping userspace from
  doing modesets on, see all the relevant changes in
  update_connector_routing(). Relying on hotplugging connectors will sort
  out a lot of these issues in a consistent way.

- Related to this: You're not allowed to shut down hardware behind the
  user's back with drm_atomic_helper_shutdown. We've tried that approach
  with dp mst, it really pisses off userspace when a page_flip that it
  expected to work doesn't work.

- There's also the design aspect that in atomic, only atomic_check is
  allowed to fail, atomic_commit must succeed, even when the hardware is
  gone. Using connectors and their refcounting should help with that.

- Somewhat aside, but I noticed that the bridge->atomic_reset is in
  drm_bridge_attach, and that's kinda the wrong place. It should be in
  drm_mode_config_reset, like all the other ->atomic_reset hooks. That
  would make it a lot clearer that we need to figure out who/when
  ->atomic_reset should be called for hotplugged bridges, maybe as part of
  connector registration when the entire bridge and it's new connector is
  assembled?

- Finally this very much means we need to rethink who/how the connector
  for a bridge is created. The new design is that the main driver creates
  this connector, once the entire bridge exists. But with hotplugging this
  gets a lot more complicated, so we might want to extract a pile of that
  encoder related code from drivers (same way dp mst helpers take care of
  connector creation too, it's just too much of a mess otherwise).

  The current bridge chaining infrastructure requires a lot of hand-rolled
  code in each bridge driver and the encoder, so that might be a good
  thing anyway.

- Finally I think the entire bridge hotplug infrastructure should be
  irrespective of the underlying bus. Which means for the mipi dsi case we
  might also want to look into what's missing to make mipi dsi
  hotunpluggable, at least for the case where it's a proper driver. I
  think we should ignore the old bridge model where driver's stitched it
  all toghether using the component framework, in my opinion that approach
  should be deprecated.

- Finally I think we should have a lot of safety checks, like only bridges
  which declare themselve to be hotunplug safe should be allowed as a part
  of the hotpluggable bridge chain part. All others must still be attached
  before the entire driver is registered with drm_dev_register.

  Or that we only allow bridges with the NO_CONNECTOR flag for
  drm_bridge_attach.

There's probably a pile more fundamental issues I've missed, but this
should get a good discussion started.
-Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 0/5] Add support for GE SUNH hot-pluggable connector (was: "drm: add support for hot-pluggable bridges")

[Luca Ceresoli]

Hello Daniel,

On Thu, 16 May 2024 15:22:01 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> Apologies for missing v1 ...
> 
> On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:
> > 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  
> 
> So I think at a high level this design approach makes sense,

Good starting point :)

> but the
> implementation needs some serious thought. One big thing upfront though,
> we need to have a clear plan for the overlay hotunload issues, otherwise
> trying to make drm bridges hotpluggable makes no sense to me. Hotunload is
> very, very tricky, full of lifetime issues, and those need to be sorted
> out first or we're just trying to build a castle on quicksand.
> 
> For bridges itself I don't think the current locking works. You're trying
> to really cleverly hide it all behind a normal-looking bridge driver, but
> there's many things beyond that which will blow up if bridges just
> disappear. Most importantly the bridge states part of an atomic update.

Surely possible as atomic updates are definitely not stimulated in my
use case. Can you recommend any testing tools to be able to trigger any
issues?

The main setups I used for my testing so far are 'modetest -s' for my
daily work and a simple weston setup to periodically test a complete
user space stack.

> Now in drm we have drm_connector as the only hotunpluggable thing, and it
> took years to sort out all the issues. I think we should either model the
> bridge hotunplug locking after that, or just outright reuse the connector
> locking and lifetime rules. I much prefer the latter personally.
> 
> Anyway the big issues:
> 
> - We need to refcount the hotpluggable bridges, because software (like
>   atomic state updates) might hang onto pointers for longer than the
>   bridge physically exists. Assuming that you can all tear it down
>   synchronously will not work.
> 
>   If we reuse connector locking/lifetime then we could put the
>   hotpluggable part of the bridge chain into the drm_connector, since that
>   already has refcounting as needed. It would mean that finding the next
>   bridge in the chain becomes a lot more tricky though. With that model
>   we'd create a new connector every time the bridge is hotplugged, which I
>   think is also the cleaner model (because you might plug in a hdmi
>   connector after a panel, so things like the connector type change).

I have been investigating the option of adding/removing a connector
based on hot-plug/unplug events initially, see my reply to Maxime after
v1 [0]:

> The first approach is based on removing the drm_connector. My laptop
> uses the i915 driver, and I have observed that attaching/removing a
> USB-C dock with an HDMI connector connected to a monitor, a new
> drm_connector appears/disappears for the card. User space gets notified
> and the external monitor is enabled/disabled, just the way a desktop
> user would expect, so this is possible. I had a look at the driver but
> how this magic happens was not clear to me honestly.

So if you could point to where/how this is done, I would certainly
reconsider that.

> - No notifiers please. The create a locking mess with inversions, and
>   especially for hotunplug they create the illusion that you can
>   synchronously keep up to date with hardware state. That's not possible.
>   Fundamentally all bridge drivers which might be hotunplugged need to be
>   able to cope with the hardware disappearing any momemnt.

Can you clarify this point? I'm sorry I fail to understand the
relationship between the use of notifiers and the ability of bridge
drivers to cope with hardware disappearing.

In this patch, the hotplug-bridge uses notifiers to be informed when
the following bridge is disappearing: which other way would you suggest
to make the hotplug-bridge aware of that? OTOH the hotplug-bridge is
not meant to disappear, and it has no actual hardware that can go away,
being just a mechanical connector.

On the opposite side, the following bridges are physically removable
and so their drivers will have to be fixed (if needed) to work when
removal happens, but I don't see how that relates to the DRM core
emitting a notification of such bridges being removed.

> - Related to this: You're not allowed to shut down hardware behind the
>   user's back with drm_atomic_helper_shutdown. We've tried that approach
>   with dp mst, it really pisses off userspace when a page_flip that it
>   expected to work doesn't work.
> 
> - There's also the design aspect that in atomic, only atomic_check is
>   allowed to fail, atomic_commit must succeed, even when the hardware is
>   gone. Using connectors and their refcounting should help with that.

IIUC here you are suggesting again to remove the connector instead of
marking it "disconnected". So, as above, pointers on how that is
achieved would be helpful.

> - Somewhat aside, but I noticed that the bridge->atomic_reset is in
>   drm_bridge_attach, and that's kinda the wrong place. It should be in
>   drm_mode_config_reset, like all the other ->atomic_reset hooks. That
>   would make it a lot clearer that we need to figure out who/when
>   ->atomic_reset should be called for hotplugged bridges, maybe as part of  
>   connector registration when the entire bridge and it's new connector is
>   assembled?
> 
> - Finally this very much means we need to rethink who/how the connector
>   for a bridge is created. The new design is that the main driver creates
>   this connector, once the entire bridge exists. But with hotplugging this
>   gets a lot more complicated, so we might want to extract a pile of that
>   encoder related code from drivers (same way dp mst helpers take care of
>   connector creation too, it's just too much of a mess otherwise).
> 
>   The current bridge chaining infrastructure requires a lot of hand-rolled
>   code in each bridge driver and the encoder, so that might be a good
>   thing anyway.
> 
> - Finally I think the entire bridge hotplug infrastructure should be
>   irrespective of the underlying bus. Which means for the mipi dsi case we
>   might also want to look into what's missing to make mipi dsi
>   hotunpluggable, at least for the case where it's a proper driver. I
>   think we should ignore the old bridge model where driver's stitched it
>   all toghether using the component framework, in my opinion that approach
>   should be deprecated.

The DSI side was one of my headaches on writing this driver, and I
must say I dislike the code in hotplug_bridge_dsi_attach(), with the
need for an initial "dummy" attach and detach the first time. At first
sight I think we need a .update_format callback in struct
mipi_dsi_host_ops so the DSI host is aware of a format change.

Right now there are DSI host drivers which keep a copy of the struct
mipi_dsi_device pointer and read the format from there when starting a
stream (e.g. the tc358768.c driver [1]). That somewhat provides a way
to react to format changes, but keeping a pointer is bad when the DSI
device hot-unplugs, and the new format won't be seen until a
disable/enable cycle. So a new callback looks more robust overall.

Any opinion about this?

> - Finally I think we should have a lot of safety checks, like only bridges
>   which declare themselve to be hotunplug safe should be allowed as a part
>   of the hotpluggable bridge chain part. All others must still be attached
>   before the entire driver is registered with drm_dev_register.

I'm fine with the principle of a "HOTPLUG" flag.

I wonder how that should be implemented, though. Based on my (surely
simplistic) understanding, right now bridges can be both added and
removed, but only in a specific sequence:

 1. add all bridges
 2. use the card
 3. remove all bridges
 4. EOF

We'd need to change to allow:

 1. add fixed bridges (including hotplug-bridge)
 2. add bridges on removable add-on
 3. use card
 4. remove bridges on removable add-on
 5. optionally goto 2
 6. remove fixed bridges (including hotplug-bridge)
 7. EOF

One naïve idea is that the DRM core keeps a flag whenever any fixed
bridge (no HOTLPUG flag) is removed and when that happens forbid adding
bridges as we are now at line 5.

Aside for tons of subtleties I'm surely missing, does this look a
proper approach? I'd not be surprised if there is something better and
more solid.

>   Or that we only allow bridges with the NO_CONNECTOR flag for
>   drm_bridge_attach.
> 
> There's probably a pile more fundamental issues I've missed, but this
> should get a good discussion started.

Sure, I think it has.

Bottom line, I'm clearly not seeing some issues that need to be
considered, and that do not show up for my use case. Based on my
limited DRM knowledge, this was expected, and I'm glad to work on those
issues with some practical indications about the path forward.

Luca

[0] https://lore.kernel.org/all/20240327170849.0c14728d@booty/
[1] https://elixir.bootlin.com/linux/v6.9.1/source/drivers/gpu/drm/bridge/tc358768.c#L435

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 0/5] Add support for GE SUNH hot-pluggable connector (was: "drm: add support for hot-pluggable bridges")

[Daniel Vetter]

On Mon, May 20, 2024 at 02:01:48PM +0200, Luca Ceresoli wrote:
> Hello Daniel,
> 
> On Thu, 16 May 2024 15:22:01 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > Apologies for missing v1 ...
> > 
> > On Fri, May 10, 2024 at 09:10:36AM +0200, Luca Ceresoli wrote:
> > > 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  
> > 
> > So I think at a high level this design approach makes sense,
> 
> Good starting point :)
> 
> > but the
> > implementation needs some serious thought. One big thing upfront though,
> > we need to have a clear plan for the overlay hotunload issues, otherwise
> > trying to make drm bridges hotpluggable makes no sense to me. Hotunload is
> > very, very tricky, full of lifetime issues, and those need to be sorted
> > out first or we're just trying to build a castle on quicksand.
> > 
> > For bridges itself I don't think the current locking works. You're trying
> > to really cleverly hide it all behind a normal-looking bridge driver, but
> > there's many things beyond that which will blow up if bridges just
> > disappear. Most importantly the bridge states part of an atomic update.
> 
> Surely possible as atomic updates are definitely not stimulated in my
> use case. Can you recommend any testing tools to be able to trigger any
> issues?

Uh really hard ... You'd need to create an atomic commit that's blocked on
a sync_file in-fence (so that you can extend the race window). And then
hotunplug the bridge chain _before_ you signal that fence.

That's not going to cover all possible races, but at least a large chunk
of the really big ones.

> The main setups I used for my testing so far are 'modetest -s' for my
> daily work and a simple weston setup to periodically test a complete
> user space stack.
> 
> > Now in drm we have drm_connector as the only hotunpluggable thing, and it
> > took years to sort out all the issues. I think we should either model the
> > bridge hotunplug locking after that, or just outright reuse the connector
> > locking and lifetime rules. I much prefer the latter personally.
> > 
> > Anyway the big issues:
> > 
> > - We need to refcount the hotpluggable bridges, because software (like
> >   atomic state updates) might hang onto pointers for longer than the
> >   bridge physically exists. Assuming that you can all tear it down
> >   synchronously will not work.
> > 
> >   If we reuse connector locking/lifetime then we could put the
> >   hotpluggable part of the bridge chain into the drm_connector, since that
> >   already has refcounting as needed. It would mean that finding the next
> >   bridge in the chain becomes a lot more tricky though. With that model
> >   we'd create a new connector every time the bridge is hotplugged, which I
> >   think is also the cleaner model (because you might plug in a hdmi
> >   connector after a panel, so things like the connector type change).
> 
> I have been investigating the option of adding/removing a connector
> based on hot-plug/unplug events initially, see my reply to Maxime after
> v1 [0]:
> 
> > The first approach is based on removing the drm_connector. My laptop
> > uses the i915 driver, and I have observed that attaching/removing a
> > USB-C dock with an HDMI connector connected to a monitor, a new
> > drm_connector appears/disappears for the card. User space gets notified
> > and the external monitor is enabled/disabled, just the way a desktop
> > user would expect, so this is possible. I had a look at the driver but
> > how this magic happens was not clear to me honestly.
> 
> So if you could point to where/how this is done, I would certainly
> reconsider that.

Right now only the dp mst code uses hotplug/unplug (like you've observed
in your testing with i915, usb-c docks are generally all dp mst). For code
reading it might be best to start with the i915 dp mst code and then go
through the helpers.

> > - No notifiers please. The create a locking mess with inversions, and
> >   especially for hotunplug they create the illusion that you can
> >   synchronously keep up to date with hardware state. That's not possible.
> >   Fundamentally all bridge drivers which might be hotunplugged need to be
> >   able to cope with the hardware disappearing any momemnt.
> 
> Can you clarify this point? I'm sorry I fail to understand the
> relationship between the use of notifiers and the ability of bridge
> drivers to cope with hardware disappearing.
> 
> In this patch, the hotplug-bridge uses notifiers to be informed when
> the following bridge is disappearing: which other way would you suggest
> to make the hotplug-bridge aware of that? OTOH the hotplug-bridge is
> not meant to disappear, and it has no actual hardware that can go away,
> being just a mechanical connector.

Yeah you need code to handle that. My point is that using a notifier is
the wrong design, because the notifier has it's own locking. Which tends
to get in the way.

Instead I think that code should be directly in core bridge code (I don't
see the benefit of having this entirely in a separate driver), using drm
locking to make sure there's no races.

> On the opposite side, the following bridges are physically removable
> and so their drivers will have to be fixed (if needed) to work when
> removal happens, but I don't see how that relates to the DRM core
> emitting a notification of such bridges being removed.

Yeah it's not directly related, just my experience that people assume
notifiers provide you more synchronization and race-preventation than they
really do. So it's better to hand-roll to make it all really explicit.

> > - Related to this: You're not allowed to shut down hardware behind the
> >   user's back with drm_atomic_helper_shutdown. We've tried that approach
> >   with dp mst, it really pisses off userspace when a page_flip that it
> >   expected to work doesn't work.
> > 
> > - There's also the design aspect that in atomic, only atomic_check is
> >   allowed to fail, atomic_commit must succeed, even when the hardware is
> >   gone. Using connectors and their refcounting should help with that.
> 
> IIUC here you are suggesting again to remove the connector instead of
> marking it "disconnected". So, as above, pointers on how that is
> achieved would be helpful.

See dp mst code. It's complex unfortunately, so there's some reading
involved :-/
>
> > - Somewhat aside, but I noticed that the bridge->atomic_reset is in
> >   drm_bridge_attach, and that's kinda the wrong place. It should be in
> >   drm_mode_config_reset, like all the other ->atomic_reset hooks. That
> >   would make it a lot clearer that we need to figure out who/when
> >   ->atomic_reset should be called for hotplugged bridges, maybe as part of  
> >   connector registration when the entire bridge and it's new connector is
> >   assembled?
> > 
> > - Finally this very much means we need to rethink who/how the connector
> >   for a bridge is created. The new design is that the main driver creates
> >   this connector, once the entire bridge exists. But with hotplugging this
> >   gets a lot more complicated, so we might want to extract a pile of that
> >   encoder related code from drivers (same way dp mst helpers take care of
> >   connector creation too, it's just too much of a mess otherwise).
> > 
> >   The current bridge chaining infrastructure requires a lot of hand-rolled
> >   code in each bridge driver and the encoder, so that might be a good
> >   thing anyway.
> > 
> > - Finally I think the entire bridge hotplug infrastructure should be
> >   irrespective of the underlying bus. Which means for the mipi dsi case we
> >   might also want to look into what's missing to make mipi dsi
> >   hotunpluggable, at least for the case where it's a proper driver. I
> >   think we should ignore the old bridge model where driver's stitched it
> >   all toghether using the component framework, in my opinion that approach
> >   should be deprecated.
> 
> The DSI side was one of my headaches on writing this driver, and I
> must say I dislike the code in hotplug_bridge_dsi_attach(), with the
> need for an initial "dummy" attach and detach the first time. At first
> sight I think we need a .update_format callback in struct
> mipi_dsi_host_ops so the DSI host is aware of a format change.
> 
> Right now there are DSI host drivers which keep a copy of the struct
> mipi_dsi_device pointer and read the format from there when starting a
> stream (e.g. the tc358768.c driver [1]). That somewhat provides a way
> to react to format changes, but keeping a pointer is bad when the DSI
> device hot-unplugs, and the new format won't be seen until a
> disable/enable cycle. So a new callback looks more robust overall.
> 
> Any opinion about this?

Yeah I don't like the code either.

What might help for prototyping is if you start with a hotpluggeable
bridge where the bridge is a i2c device. That way I think we should be
able to benefit from the driver bind/unbind code that exists already.
Although there's going to be issues with i2c hotunplug too in i2c core
code.

Then lift whatever we learn there to our dsi code. But essentially I think
we should model the driver core model a lot more, so I guess you could use
any hotunplug capable bus as a template. Usb might be closest, since
that's also a packet/message based interface, mostly at least?

> > - Finally I think we should have a lot of safety checks, like only bridges
> >   which declare themselve to be hotunplug safe should be allowed as a part
> >   of the hotpluggable bridge chain part. All others must still be attached
> >   before the entire driver is registered with drm_dev_register.
> 
> I'm fine with the principle of a "HOTPLUG" flag.
> 
> I wonder how that should be implemented, though. Based on my (surely
> simplistic) understanding, right now bridges can be both added and
> removed, but only in a specific sequence:
> 
>  1. add all bridges
>  2. use the card
>  3. remove all bridges
>  4. EOF
> 
> We'd need to change to allow:
> 
>  1. add fixed bridges (including hotplug-bridge)
>  2. add bridges on removable add-on
>  3. use card
>  4. remove bridges on removable add-on
>  5. optionally goto 2
>  6. remove fixed bridges (including hotplug-bridge)
>  7. EOF
> 
> One naïve idea is that the DRM core keeps a flag whenever any fixed
> bridge (no HOTLPUG flag) is removed and when that happens forbid adding
> bridges as we are now at line 5.

If a fixed bridge is removed while the driver is still in use (i.e. before
drm_dev_unregister is called), that's a driver bug. Would be good to catch
that, which is why I think a lot of all the bridge hotplug handling should
be in core bridge code and not the separate hotplug driver, so that we can
enforce all these constraints.

Also conceptually 3 can happen before 2 (but also before), and that's how
dp mst works too. It does add all kinds of complications though ...

> Aside for tons of subtleties I'm surely missing, does this look a
> proper approach? I'd not be surprised if there is something better and
> more solid.

Yeah roughly. If you look through dp mst code then that also adds all
kinds of structures (since dp mst is a routed network really), not just
the connectors. They also all come with refcounts (because the network is
a tree), but like I said I think for your case we can avoid the per-bridge
refcounts by relying on the connector refcount we have already.

But I might be overlooking something, and we need refcounting for each
bridge like dp mst also needs refcounting for all its internal structures
that map the entire hotpluggable display chain. If you want to read some
dp mst code, these internal structures are ports/branches with struct
drm_dp_mst_branch/port.

> >   Or that we only allow bridges with the NO_CONNECTOR flag for
> >   drm_bridge_attach.
> > 
> > There's probably a pile more fundamental issues I've missed, but this
> > should get a good discussion started.
> 
> Sure, I think it has.
> 
> Bottom line, I'm clearly not seeing some issues that need to be
> considered, and that do not show up for my use case. Based on my
> limited DRM knowledge, this was expected, and I'm glad to work on those
> issues with some practical indications about the path forward.

Yeah for learning I think dp mst is best. It's a bit complex, but since
you have a dock you should be able to play around and experiment with the
code with some real hardware.

That should help to get a bit a feel for the complexity, since lots of
opportunities for you to ask why/how locking/refcounting is used and
against which potential issue they protect.

Cheers, Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch