| Message ID | 20240510-hotplug-drm-bridge-v2-1-ec32f2c66d56@bootlin.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for GE SUNH hot-pluggable connector (was: "drm: add support for hot-pluggable bridges") | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
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.
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
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
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
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
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
On Tue, May 14, 2024 at 06:51:25PM +0200, Luca Ceresoli wrote: > 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: What if the bootloader happened to load the overlay already? Or you kexec into a new kernel? Keeping things underneath a connector node makes managing the dependencies easier. It also can allow us to have some control over what overlays can and can't modify. It also reflects reality that these devices sit behind the connector. > > 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. Only that to be discoverable, you shouldn't need DT. > 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. So you don't really need an overlay for this unless the EEPROM model changes or the model-id offset changes. I suppose nvmem needs something in DT to register a region. That's really nvmem's problem, but I guess what you have is fine. Rob
Hello Rob, thanks for the follow up. I still have a couple questions for you before I see a clear direction forward, see below. On Wed, 5 Jun 2024 08:45:31 -0600 Rob Herring <robh@kernel.org> wrote: [...] > > > > + # "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: > > What if the bootloader happened to load the overlay already? Or you > kexec into a new kernel? When an overlay is loaded by the bootloader, IIRC it becomes an integral part of the live device tree and is not removable anymore. This does not make sense for a physically removable add-on: as the add-on can be physically removed, its device tree representation must be removable as well. And the main board is able to work autonomously without the add-on, so I don't see any reason for loading the overlay in the bootloader. For the kexec case, the main use cases I can think of involves 'kexec --dtb=...' to loads its own copy of the base DT (without overlays). So everything will probe again, and the overlays will be loaded again by the connector driver if/whan the add-on is connected. And if there are use cases of kexec when the 2nd kernel finds the DT with the overlays already loaded, this is just as wrong as in the bootloader case. > Keeping things underneath a connector node makes managing the > dependencies easier. It also can allow us to have some control over what > overlays can and can't modify. It also reflects reality that these > devices sit behind the connector. From my limited point of view, these points appear all nice to have but not strictly needed. About the last one, referring to your example: > > > For i2c, it could look something like this: > > > > > > connector { > > > i2c { > > > i2c-parent = <&i2c1>; > > > > > > eeprom@50 {...}; > > > }; > > > }; I definitely understand the usefulness of such an additional level of indirection in the most general case, to decouple the add-on side of the I2C bus from the base board side of it, thus allowing multiple different base board models and even helping with having multiple connectors (multiple add-ons at the same time) on the same main board. But I also see drawbacks. The first one is added complexity. The second is that this representation seems to suggest that the 'i2c' node above is another bus w.r.t. '&i2c1', somewhat similarly to what happens with child busses of an i2c mux being a different node from the parent bus. But in this case they are really the same bus on the same electrical lines (when the add-on is connected). So I think both representations have pros and cons. Back to the specific product I'm working on: there is only one base board model, and also only one connector per main board, and this is by the very nature of the product, i.e. it would not make sense to have two connectors on the same board. So in the specific case of this product, do you think it would be OK to keep the representation I proposed initially? > > > 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. > > So you don't really need an overlay for this unless the EEPROM model > changes or the model-id offset changes. The EEPROM model is the same on all add-on models, or at least it's fully compatible. Otherwise finding out the model ID would become very annoying. However the EEPROM is on the add-on, so describing it in the main DT would be wrong. Not only conceptually, as hardware not present should not be in the live DT, but also practically, as when the add-on is removed and then a possibly different add-on is connected we need the EEPROM driver to probe again, in order to do any initialization that might be needed in the EEPROM driver probe function. So I see no option but adding the EEPROM in an overlay. But it cannot be the "full" overlay because before accessing the EEPROM we don't know which model is loaded. Do you have in mind a better approach that I just didn't think about? Best regards, Luca
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
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(+)