From: Luca Ceresoli <luca.ceresoli@bootlin.com>
To: Rob Herring <robh@kernel.org>
Cc: "Krzysztof Kozlowski" <krzk+dt@kernel.org>,
"Conor Dooley" <conor+dt@kernel.org>,
"Andrzej Hajda" <andrzej.hajda@intel.com>,
"Neil Armstrong" <neil.armstrong@linaro.org>,
"Robert Foss" <rfoss@kernel.org>,
"Laurent Pinchart" <Laurent.pinchart@ideasonboard.com>,
"Jonas Karlman" <jonas@kwiboo.se>,
"Jernej Skrabec" <jernej.skrabec@gmail.com>,
"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
"Maxime Ripard" <mripard@kernel.org>,
"Thomas Zimmermann" <tzimmermann@suse.de>,
"David Airlie" <airlied@gmail.com>,
"Daniel Vetter" <daniel@ffwll.ch>,
"Derek Kiernan" <derek.kiernan@amd.com>,
"Dragan Cvetic" <dragan.cvetic@amd.com>,
"Arnd Bergmann" <arnd@arndb.de>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Saravana Kannan" <saravanak@google.com>,
"Paul Kocialkowski" <contact@paulk.fr>,
"Hervé Codina" <herve.codina@bootlin.com>,
"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org,
"Paul Kocialkowski" <paul.kocialkowski@bootlin.com>,
"Srinivas Kandagatla" <srinivas.kandagatla@linaro.org>,
"Miquel Raynal" <miquel.raynal@bootlin.com>
Subject: Re: [PATCH v2 1/5] dt-bindings: connector: add GE SUNH hotplug addon connector
Date: Tue, 14 May 2024 18:51:25 +0200 [thread overview]
Message-ID: <20240514185125.58225238@booty> (raw)
In-Reply-To: <20240510163625.GA336987-robh@kernel.org>
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
next prev parent reply other threads:[~2024-05-14 16:51 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-10 7:10 [PATCH v2 0/5] Add support for GE SUNH hot-pluggable connector (was: "drm: add support for hot-pluggable bridges") Luca Ceresoli
2024-05-10 7:10 ` [PATCH v2 1/5] dt-bindings: connector: add GE SUNH hotplug addon connector Luca Ceresoli
2024-05-10 8:41 ` Rob Herring (Arm)
2024-05-10 10:37 ` Luca Ceresoli
2024-05-10 13:22 ` Rob Herring
2024-05-10 14:39 ` Luca Ceresoli
2024-05-10 16:36 ` Rob Herring
2024-05-14 16:51 ` Luca Ceresoli [this message]
2024-06-05 14:45 ` Rob Herring
2024-06-11 14:43 ` Luca Ceresoli
2024-05-10 7:10 ` [PATCH v2 2/5] drm/bridge: add bridge notifier to be notified of bridge addition and removal Luca Ceresoli
2024-05-10 7:10 ` [PATCH v2 3/5] drm/encoder: add drm_encoder_cleanup_from() Luca Ceresoli
2024-05-10 7:10 ` [PATCH v2 4/5] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges Luca Ceresoli
2024-05-10 7:10 ` [PATCH v2 5/5] misc: add ge-addon-connector driver Luca Ceresoli
2024-05-10 7:55 ` Greg Kroah-Hartman
2024-05-10 10:24 ` Arnd Bergmann
2024-05-10 10:54 ` Luca Ceresoli
2024-05-10 10:57 ` Arnd Bergmann
2024-05-10 15:32 ` Luca Ceresoli
2024-05-10 10:54 ` Luca Ceresoli
2024-05-10 11:03 ` Greg Kroah-Hartman
2024-05-10 16:44 ` [PATCH v2 0/5] Add support for GE SUNH hot-pluggable connector (was: "drm: add support for hot-pluggable bridges") Rob Herring
2024-05-14 17:11 ` Luca Ceresoli
2024-05-16 13:22 ` Daniel Vetter
2024-05-20 12:01 ` Luca Ceresoli
2024-05-21 12:01 ` Daniel Vetter
2024-06-05 14:47 ` Luca Ceresoli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240514185125.58225238@booty \
--to=luca.ceresoli@bootlin.com \
--cc=Laurent.pinchart@ideasonboard.com \
--cc=airlied@gmail.com \
--cc=andrzej.hajda@intel.com \
--cc=arnd@arndb.de \
--cc=conor+dt@kernel.org \
--cc=contact@paulk.fr \
--cc=daniel@ffwll.ch \
--cc=derek.kiernan@amd.com \
--cc=devicetree@vger.kernel.org \
--cc=dragan.cvetic@amd.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=herve.codina@bootlin.com \
--cc=jernej.skrabec@gmail.com \
--cc=jonas@kwiboo.se \
--cc=krzk+dt@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=miquel.raynal@bootlin.com \
--cc=mripard@kernel.org \
--cc=neil.armstrong@linaro.org \
--cc=paul.kocialkowski@bootlin.com \
--cc=rfoss@kernel.org \
--cc=robh@kernel.org \
--cc=saravanak@google.com \
--cc=srinivas.kandagatla@linaro.org \
--cc=thomas.petazzoni@bootlin.com \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).