by Rob Herring (Arm)

[permalink] [raw]

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

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 <[email protected]> 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 <[email protected]>
>
> [...]
>
> > > +++ 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 <[email protected]>
> > > +
> > > +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

2024-06-11 14:43:33

by Luca Ceresoli

[permalink] [raw]

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

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 <[email protected]> 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

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