Re: [PATCH v9 5/5] docs: i2c: i2c-topology: add section about bus speed

[Peter Rosin]

Hi all,

First off, sorry for being so incredibly slow in getting to this
series.

On 2026-05-26 21:48, Wolfram Sang wrote:
> > +Multiple muxes in series
> > +--------------------------
> > +
> > +When multiple muxes are used in series the same rules applies.
> > +
> > +Transfers to D3 may interleave between select-transfer-deselect to D1, which
> > +results that the bus speed to D2 or D3 will be at 100KHz.
> > +
> > +Transfers to D2 may interleave between select-transfer-deselect to D1, which
> > +results in that the bus speed to D1 may be at 400kHz as the transfer to D2
> > +will set the bus speed to before the transfer to D1 starts.
> > +
> > +This is probably a bad topology ::
>
> In documentation, we probably shouldn't say "probably" ;)

What makes the topology bad is D2. Without that device, it should
be OK. So, the question is which is more important?
1. protect ignorant system designers from creating a topology
   that includes problematic devices like D2
2. allow multi-level mux-locked muxes with variable bus-speed at all

Checking the rules at run-time feels complicated to me, as devices
may come and go. Also, naming people "ignorant" over a mistake such
as the above in 1 is perhaps not all that fair. But, it feels sad
to disallow things that in fact do work.

> > +
> > +                     .----------. 400kHz .----------. 100kHz .--------.
> > +    .--------.400kHz |   mux-   |--------|   mux-   |--------| dev D1 |
> > +    |  root  |--+----|  locked  | 400kHz |  locked  |        '--------'
> > +    '--------'  |    |  mux M1  |--.     |  mux M2  |
> > +                |    '----------'  |     '----------'
> > +                |  .--------.      |  .--------.
> > +                '--| dev D3 |      '--| dev D2 |
> > +                   '--------'         '--------'
>
> 400kHz leaking to a 100kHz device is bad. I guess we cannot do much
> about it here because the bus speed is a board specific parameter...
>
> > +Idle state
> > +-----------
> > +
> > +Muxes have an idle state, which is the state the channels are put into when no channel
> > +is active. The state is typically one of the following:
> > +
> > +- All channels are disconnected
> > +- The last selected channel is left as-is
> > +- A predefined channel is selected
> > +
> > +Muxes that support an idle state where all channels are disconnected are preferred when using
> > +different bus speeds. Otherwise high bus speeds may "leak" through to devices that
> > +may not support that higher speed.
> > +
> > +Consider the following example: ::
> > +
> > +                          .----------. 100kHz .--------.
> > +        .--------. 400kHz |   mux-   |--------| dev D1 |
> > +        |  root  |--+-----|  locked  |        '--------'
> > +        '--------'  |     |  mux M1  |--. 400kHz .--------.
> > +                    |     '----------'  '--------| dev D2 |
> > +                    |  .--------.                '--------'
> > +                    '--| dev D3 |
> > +                       '--------'
> > +
> > +If the idle state of M1 is:
> > +
> > +- All channels disconnected: No problem, D1 and D2 are not affected by communication
> > +  to D3.
> > +- Last selected channel: Problem if D1 was the last selected channel. High speed
> > +  communication to D3 will be "leaked" to D1.
> > +- Predefined channel: Problem if the predefined channel D1. Set predefined channel
> > +  to D2 as D2 may handle 400kHz.
>
> ... unlike here. We have MUX_IDLE_AS_IS and MUX_IDLE_DISCONNECT defined
> already. And I'd think we should only allow bus speed switching for
> MUX_IDLE_DISCONNECT to avoid out-of-spec scenarios. Opinions?

It is probably not unwise to disallow MUX_IDLE_AS_IS in this context,
at least until someone actually needs it. Which does not seem all that
likely?

However, it seems like it should be fairly OK to allow a predefined
idle channel, as long as that channel is not lowering the bus speed
compared to the parent. But maybe supporting that can also wait for
an actual user?

> > +Supported controllers
> > +-----------------------
> > +
> > +Not all I2C controllers support setting the bus speed dynamically.
> > +At the time of writing, the following controllers have support:
> > +
> > +============================   =============================================
> > +i2c-davinci                    Supports dynamic bus speed
> > +============================   =============================================
>
> This paragaph is easy to get outdated. We can document that only
> controller drivers with the callback function implemented will work.
> People then can find out if that applies for their driver...

There are other such hard-to-maintain lists elsewhere in the text.
Not saying that this makes neither those nor this list a good idea,
but it is an explanation for adding one more.

Cheers,
Peter

> There are some grammar correction in the Sashiko-review[1]. Most of them
> look correct to me. Can you have a look at these, too, please?
>
> [1] https://sashiko.dev/#/patchset/20260324-i2c-mux-v9-0-5292b0608243%40gmail.com