Re: [PATCH v4 2/5] i2c: mux: add support for per channel bus frequency

[Marcus Folkesson <marcus.folkesson@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5998 bytes --]

Hi Peter!

On Fri, Feb 13, 2026 at 12:37:29PM +0100, Peter Rosin wrote:
> Hi!
> 
> 2026-02-12 at 22:47, Marcus Folkesson wrote:
> >>> +static int i2c_mux_select_chan(struct i2c_adapter *adap, u32 chan_id)
> > 

[...]

> > Consider the following chain:
> > Root - P1 - M1 - M2 - P2 - D1
> > 
> > P - Parent locked
> > M - Mux locked
> > D - Device
> > 
> > In this case we need to lock both M1 and M2, not just M2 ?
> > I'm not completely sure though, I need to refresh myself on the code
> > base.
> 
> No, that should not be needed. The reason is that when you initiate a
> xfer for D1 the following happens (xfer is a locked transfer, __xfer
> is unlocked):
> 
> - xfer using P2
>  - lock(P2, I2C_LOCK_SEGMENT)
>   + take mux_lock for P2->parent == M2
>   + P2 is parent-locked -> recurse to P2->parent == M2
>    + lock(M2, I2C_LOCK_SEGMENT)
>     + take mux_lock for M2->parent == M1
>     + M2 is mux-locked -> no recursion
>  - ***** (see below)
>  - P2->select (commonly __xfer using M2, elided here)
>  - __xfer using M2 (unlocked since P2 is parent-locked)
>   - §§§§§ (see below)
>   - M2->select (commonly xfer using M1, elided here)
>   - locked xfer using M1 (locked since M2 is mux-locked)
>    - lock(M1, I2C_LOCK_SEGMENT)
>     + take mux_lock for M1->parent == P1
>     + M1 is mux-locked -> no recursion
>    - M1->select (commonly xfer using P1, elided here)
>    - xfer using P1 (locked since M1 is mux-locked)
>     - lock(P1, I2C_LOCK_SEGMENT)
>      + take mux_lock for P1->parent == Root
>      + P1 is parent-locked -> recurse to P1->parent == Root
>       + lock(Root, I2C_LOCK_SEGMENT)
>        + take bus_lock for Root
>        + Root is Root -> no recursion
>     - P1->select (commonly __xfer using Root, elided here)
> 
>     - __xfer using Root (unlocked since P1 is parent-locked)
> 
>     - P1->deselect (commonly __xfer using Root, elided here)
>     - unlock(P1, I2C_LOCK_SEGMENT)
>      + P1 is parent-locked -> recurse to P1->parent == Root
>       + unlock(Root, I2C_LOCK_SEGMENT)
>        + Root is Root -> no recursion
>        + release bus_lock for Root
>      + release mux_lock for P1->parent == Root
>    - M1->deselect (commonly xfer using P1, elided here)
>    - unlock(M1, I2C_LOCK_SEGMENT)
>     + M1 is mux-locked -> no recursion
>     + release mux_unlock for M1->parent == P1
>   - M2->deselect (commonly xfer using M1, elided here)
>  - P2->deselect (commonly __xfer using M2, elided here)
>  - unlock(P2, I2C_LOCK_SEGMENT)
>   + P2 is parent-locked -> recurse to P2->parent == M2
>    + unlock(M2, I2C_LOCK_SEGMENT)
>     + M2 is mux-locked -> no recursion
>     + release mux_lock for M2->parent == P2
>   + release mux_lock for P2->parent == M2
> 
> (Phhew, I wonder how many typos are in there...)
> 
> So, between the steps lock(P2,...) and P2->select (at the ***** mark,
> which is where you add set_clk_freq), what you need to lock is M1,
> i.e. the parent of the first ancestor that is not mux-locked. When
> you lock with I2C_LOCK_ROOT_ADAPTER, this happens:
> 
>  - lock(M1, I2C_LOCK_ROOT_ADAPTER)
>   + take mux_lock for M1->parent == P1
>   + recures to M1->parent == P1
>    + lock(P1, I2C_LOCK_ROOT_ADAPTER)
>     + take mux_lock for P1->parent == Root
>     + recurse to P1->parent == Root
>      + lock(Root, I2C_LOCK_ROOT_ADAPTER)
>       + take bus_lock for Root
>       + Root is Root -> no recursion
>  - Root->set_clk_freq                  <<<< the new thing
>  - unlock(M1, I2C_LOCK_ROOT_ADAPTER)
>   + recurse to M1->parent == P1
>    + unlock(P1, I2C_LOCK_ROOT_ADAPTER)
>     + recurse to P1->parent == Root
>      + unlock(Root, I2C_LOCK_ROOT_ADAPTER)
>       + Root is Root, no recursion
>       + release bus_lock for Root
>     + release mux_lock for P1->parent == Root
>   + release mux_lock for M1->parent == P1

Thanks for the explaination, that makes it much clearer!
> 
> However, spelling that out makes it clearer that Root->set_clk_freq
> will be inserted in more places in the "call tree". It will be added
> before every ->select call, e.g. at §§§§§. Since any of these
> additional set_clk_freq calls happen after the one at *****, they will
> take precedence and ruin the whole thing if any of them should request
> an intermediate frequency (1MHz at Root, 400kHz for any intermediate
> mux and 100kHz for D1, for example).
> 
> I don't immediately see how to reverse that such that the set_clk_freq
> for the top-most level happens closest to the xfer on the root
> adapter.

I've now experimented a bit and think I've landed on this solution:

- i2c_mux_select_chan() will only lower the root bus frequency, this to
  ensure that no intermediate mux will be able to change the frequency
  (all muxes in the middle must have a higher frequency).
- i2c_mux select_chan() store the original bus frequency
- i2c_mux_deselect_chan() will restore the original bus frequency


Something like this:

static int i2c_mux_select_chan(struct i2c_adapter *adap, u32 chan_id, u32 *oldclock)
{
	if (priv->adap.clock_hz && priv->adap.clock_hz < parent->clock_hz) {
		*oldclock = root->clock_hz;
		i2c_adapter_set_clk_freq(root, priv->adap.clock_hz);
    }
}

static void i2c_mux_deselect_chan(struct i2c_adapter *adap, u32 chan_id, u32 oldclock)
{
	if (oldclock && oldclock != priv->adap.clock_hz) {
		i2c_adapter_set_clk_freq(root, oldclock);
    }
}

static int __i2c_mux_master_xfer(struct i2c_adapter *adap,
				 struct i2c_msg msgs[], int num)
{
	u32 oldclock = 0;

	ret = i2c_mux_select_chan(adap, priv->chan_id, &oldclock);
    __i2c_transfer(parent, msgs, num);
	i2c_mux_deselect_chan(adap, priv->chan_id, oldclock);
}

I will do more testing during the weekend.
I now have a virtual i2c bus with virtual mux drivers and virtual
devices so that I can setup different combinations to test with. 

> 
> Cheers,
> Peter

Best regards,
Marcus Folkesson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]