| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250417173810.4697b2b8@bootlin.com>
Date: Thu, 17 Apr 2025 17:38:10 +0200
From: Herve Codina <herve.codina@...tlin.com>
To: Rob Herring <robh@...nel.org>
Cc: Wolfram Sang <wsa+renesas@...g-engineering.com>, Andi Shyti
<andi.shyti@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
Dooley <conor+dt@...nel.org>, linux-i2c@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree-spec@...r.kernel.org, Luca Ceresoli <luca.ceresoli@...tlin.com>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 2/2] schemas: i2c: Introduce I2C bus extensions
Hi Rob, device-tree maintainers,
This discussion started a few weeks ago and hasn't reached any conclusion.
I answered questions and comments but didn't receive any feedback.
In order to move forward on that topic, can we revive this discussion?
As a reminder, topics started in the discussion were the following:
- i2c-bus-extension@0: usage of a subnode with unit address
- Presence of phandles in both direction (double linked list)
Best regards,
Hervé
On Wed, 2 Apr 2025 10:21:23 +0200
Herve Codina <herve.codina@...tlin.com> wrote:
> Hi Rob,
>
> On Tue, 1 Apr 2025 09:03:23 -0500
> Rob Herring <robh@...nel.org> wrote:
>
> > On Tue, Apr 1, 2025 at 3:11 AM Herve Codina <herve.codina@...tlin.com> wrote:
> > >
> > > An I2C bus can be wired to the connector and allows an add-on board to
> > > connect additional I2C devices to this bus.
> > >
> > > Those additional I2C devices could be described as sub-nodes of the I2C
> > > bus controller node however for hotplug connectors described via device
> > > tree overlays there is additional level of indirection, which is needed
> > > to decouple the overlay and the base tree:
> > >
> > > --- base device tree ---
> > >
> > > i2c1: i2c@...d0000 {
> > > compatible = "xyz,i2c-ctrl";
> > > i2c-bus-extension@0 {
> >
> > What does 0 represent? Some fake I2C address?
> >
> > Why do you even need a child node here?
>
> 0 represent the extension number. Multiple extensions can be connected
> to a single i2c controller:
> +-------------+
> +------------+ .----+ Connector 1 |
> | i2c | | +-------------+
> | controller +---- i2c bus ----+
> +------------+ | +-------------+
> '----+ Connector 2 |
> +-------------+
>
> I need a child node because extensions don't modify existing hardware (adding/removing
> a property) but add an entry point, the extension for a new set of devices.
> As it is not a existing hardware modification it is better represented as a node.
>
>
> Those extensions can be chained:
> +-----------------------------------+ +-------------------+
> | Base board | | addon board |
> | | | +------------+ |
> | +------------+ | | .--+ i2c device | |
> | | i2c | +-------------+ | | +------------+ |
> | | controller +-- i2c bus --+ connector A +----+ |
> | +------------+ +-------------+ | | +-------------+
> +-----------------------------------+ | '----------+ connector B |
> | +-------------+
> +-------------------+
> The addon board is described using an overlay.
>
> In that case, we have:
> - base-board.dts:
> i2c0: i2c@...e0000 {
> compatible = "xyz,i2c-ctrl";
> #address-cells = <1>;
> #size-cells = <0>;
>
> i2c-bus-extension@0 {
> reg = <0>;
> i2c-bus = <&i2c_connector_a>;
> };
> ...
> };
>
> connector-a {
> devices {
> /* Entry point for devices available on the addon
> * board that are not connected to a bus such as
> * fixed-clock, fixed-regulator, connectors, ...
> */
> };
> i2c_connector_a: i2c-connector-a {
> /* The i2c available at connector */
> #address-cells = <1>;
> #size-cells = <0>;
> i2c_parent <&i2c0>;
> };
> };
>
> - addon-board.dtso
> __overlay__ { /* This is applied at connector_a node */
> i2c_connector_a: i2c-connector-a {
> /* We do not modify the existing device i2c_connector_a
> * by changing, adding or removing its properties but
> * we add new devices (sub-nodes)
> */
>
> /* The i2c device available in the addon-board */
> i2c-device@...0 {
> compatible = "foo,bar";
> reg = 0x10;
> };
>
> /* The i2c extension forwarding the i2c bus */
> i2c-bus-extension@0 {
> reg = <0>;
> i2c-bus = <&i2c_connector_b>;
> };
> };
>
> devices {
> /* addon-board connector b */
> connector_b {
> i2c_connector_b: i2c_connector_b {
> /* The i2c available at connector */
> #address-cells = <1>;
> #size-cells = <0>;
> i2c_parent = <&i2c_connector_a>;
> };
> };
> };
>
> Without a child node for i2c-bus-extension, we need to add
> properties on already existing node (i2c-connector-a) to add
> the bus extension and adding/modifying/removing a property
> on a device-tree node correspond to modifying the device
> itself (description changed) whereas adding/removing sub-nodes
> correspond to adding/removing devices handled by the parent
> parent node of those sub-nodes.
>
> From the controller point of view, an extension is "a collection of
> devices described somewhere else in the device-tree and connected
> to the I2C SDA/SCL pins". Having that described as a sub-node seems
> correct.
>
> >
> > > i2c-bus = <&i2c_ctrl>;
> > > };
> > > ...
> > > };
> > >
> > > i2c5: i2c@...e0000 {
> > > compatible = "xyz,i2c-ctrl";
> > > i2c-bus-extension@0 {
> > > i2c-bus = <&i2c-sensors>;
> > > };
> > > ...
> > > };
> > >
> > > connector {
> > > i2c_ctrl: i2c-ctrl {
> > > i2c-parent = <&i2c1>;
> > > #address-cells = <1>;
> > > #size-cells = <0>;
> > > };
> > >
> > > i2c-sensors {
> > > i2c-parent = <&i2c5>;
> > > #address-cells = <1>;
> > > #size-cells = <0>;
> > > };
> > > };
> > >
> > > --- device tree overlay ---
> > >
> > > ...
> > > // This node will overlay on the i2c-ctrl node of the base tree
> > > i2c-ctrl {
> > > eeprom@50 { compatible = "atmel,24c64"; ... };
> > > };
> > > ...
> > >
> > > --- resulting device tree ---
> > >
> > > i2c1: i2c@...d0000 {
> > > compatible = "xyz,i2c-ctrl";
> > > i2c-bus-extension@0 {
> > > i2c-bus = <&i2c_ctrl>;
> > > };
> > > ...
> > > };
> > >
> > > i2c5: i2c@...e0000 {
> > > compatible = "xyz,i2c-ctrl";
> > > i2c-bus-extension@0 {
> > > i2c-bus = <&i2c-sensors>;
> > > };
> > > ...
> > > };
> > >
> > > connector {
> > > i2c-ctrl {
> > > i2c-parent = <&i2c1>;
> > > #address-cells = <1>;
> > > #size-cells = <0>;
> > >
> > > eeprom@50 { compatible = "atmel,24c64"; ... };
> > > };
> > >
> > > i2c-sensors {
> > > i2c-parent = <&i2c5>;
> > > #address-cells = <1>;
> > > #size-cells = <0>;
> > > };
> > > };
> > >
> > > Here i2c-ctrl (same goes for i2c-sensors) represent the part of I2C bus
> > > that is on the hot-pluggable add-on. On hot-plugging it will physically
> > > connect to the I2C adapter on the base board. Let's call the 'i2c-ctrl'
> > > node an "extension node".
> > >
> > > In order to decouple the overlay from the base tree, the I2C adapter
> > > (i2c@...d0000) and the extension node (i2c-ctrl) are separate nodes.
> > >
> > > The extension node is linked to the I2C bus controller in two ways. The
> > > first one with the i2c-bus-extension available in I2C controller
> > > sub-node and the second one with the i2c-parent property available in
> > > the extension node itself.
> > >
> > > The purpose of those two links is to provide the link in both direction
> > > from the I2C controller to the I2C extension and from the I2C extension
> > > to the I2C controller.
> >
> > Why do you need both directions? An i2c controller can search the tree
> > for i2c-parent and find the one's that belong to it. Or the connector
> > can register with the I2C controller somehow.
>
> Yes, but this is sub-optimal compare to the double-link references.
>
> I discarded any kind of registering from the connector which implies
> extra complexity compared to a simple double-link reference. In the I2C
> path, the connector is really a passive component and fully transparent.
> It should be transparent as well in the implementation.
>
> Using only i2c-parent (i.e. the reference from extension to i2c controller)
> works when the walk from extension to i2c controller but using it on the
> other direction is not as trivial as it could be.
>
> Indeed, starting from the i2c controller, we have to search for the entire
> tree any i2c-parent that references the i2c controller.
> Those i2c-parent can exist in node that are not at all related to i2c
> extensions.
>
> i2c-parent in all cases represents an i2c parent bus but not only for i2c
> extensions. I2C muxes and some other devices use this property.
>
> Here also, searching the entire tree for i2c-parent and being sure that
> the property found is related to an I2C extension adds extra complexity
> that is simply not present with the double-link references.
>
> Best regards,
> Hervé
Powered by blists - more mailing lists