Re: [PATCH] i2c: jz4780: Cache clock rate at probe to prevent CCF prepare_lock deadlock

[Paul Cercueil]

Hi Andi, Nikolaus,

Le jeudi 16 juillet 2026 à 13:41 +0200, Andi Shyti a écrit :
> On Thu, Jul 16, 2026 at 10:33:56AM +0200, H. Nikolaus Schaller wrote:
> > > Am 16.07.2026 um 09:43 schrieb Andi Shyti
> > > <andi.shyti@xxxxxxxxxx>:
> > > On Thu, Jul 16, 2026 at 07:44:20AM +0200, H. Nikolaus Schaller
> > > wrote:
> > > > > Am 16.07.2026 um 00:07 schrieb Andi Shyti
> > > > > <andi.shyti@xxxxxxxxxx>:
> > > > > On Fri, Jul 10, 2026 at 08:58:35AM +0200, H. Nikolaus
> > > > > Schaller wrote:
> > > > > > Fix a severe AB/BA deadlock between the Common Clock
> > > > > > Framework (CCF)
> > > > > > and the I2C adapter lock, which triggers when an I2C-
> > > > > > controlled clock
> > > > > > generator (like the Si5351) is registered or modified under
> > > > > > the CCF.
> > > > > > 
> > > > > > During a clock frequency change, the CCF acquires its
> > > > > > global 'prepare_lock'
> > > > > > mutex and calls i2c_transfer() to update the chip
> > > > > > registers, stalling
> > > > > > for the adapter's I2C bus lock.
> > > > > 
> > > > > I don't think caching the clock rate once at probe is safe.
> > > > 
> > > > Ok, valid point to discuss.
> > > > 
> > > > 
> > > > > If the controller clock rate changes afterwards,
> > > > > jz4780_i2c_set_speed() will keep using the stale cached value
> > > > > and
> > > > > calculate incorrect bus timings.
> > > > 
> > > > But: is clock rate ever changed during operation? Usually it is
> > > > defined
> > > > by the device tree constant.
> > > 
> > > That's what you are saying in your commit message.
> > 
> > The Si5351 clock generator driver is connected through this i2c
> > bus. And that
> > is referred to by "when an I2C-controlled clock generator (like the
> > Si5351) is 
> > registered or modified under the CCF".
> > 
> > Only then, this bug faces surface. Because that driver changes it's
> > clock through
> > sending i2c commands which also ask for the lock.
> 
> If you make a claim in the commit message, I expect the code to
> support it.
> 
> You say that the clock rate can change, but then you store it
> only once at probe time. That immediately suggests that later
> rate changes will be ignored by the transfer path.

I think there's a misunderstanding.

The JZ4780 I2C host's parent clock rate does not change here, so
caching its rate should be fine.

The clock rate that can change in this setup is the one of the Si5351
clock *generator* that is a I2C device.

> 
> > But again: how would you solve this?

I believe you have this problem because your Si5351's clocks are
parented to the I2C host's clock, am I right?

So when you try to change a Si5351 clock rate, it will get the clock's
lock and the clock parent's lock, aka. the I2C host clock's lock. Then
trying to send a I2C command will indirectly trigger clk_get_rate() on
the I2C host's clock to be called, causing a lockup.

I think the solution would be, and CLK people can correct me here, to
just drop the relation to the I2C host lock. Your Si5351 clocks are not
derived in any way from the clock used by the I2C hardware in the JZ
SoC (unless they are?), so I think they should just not have any
parent.

> It's not my role to give you a solution. I'm just commenting on
> what I see.
> 
> Besides that, is that an issue you have encountered yourself? Is
> it a frequent issue?
> 
> Thanks,
> Andi

Cheers,
-Paul