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

[Rob Herring <robh@kernel.org>]

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