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

[Paul Cercueil]

Hi Nikolaus,

Le dimanche 19 juillet 2026 à 15:23 +0200, H. Nikolaus Schaller a
écrit :
> Hi Paul,
> 
> > Am 19.07.2026 um 14:53 schrieb Paul Cercueil
> > <paul@xxxxxxxxxxxxxxx>:
> > 
> > Hi Andi, Nikolaus,
> > 
> > Le jeudi 16 juillet 2026 à 13:41 +0200, Andi Shyti a écrit :
> > 
> > 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.
> 
> Exactly.
> 
> > 
> > > 
> > > > 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?
> 
> No. There is no direct reference to i2c clocks:
> 
> / {
> 	ref25: xtal {
> 		compatible = "fixed-clock";
> 		clock-output-names ="ref";
> 		#clock-cells = <0>;
> 		clock-frequency = <25000000>;
> 	};
> };
> 
> &i2c1 {
> 	clkgen: clock-generator@60 {
> 		status = "okay";
> 
> 		compatible = "silabs,si5351a-msop";	// 10-MSOP
> package has just 3 clock outputs
> 		reg = <0x60>;	// 61?
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		#clock-cells = <1>;
> 
> 		/* Connect XTAL input to 25MHz reference */
> 		clocks = <&ref25>;
> 		clock-names = "xtal";
> 
> 		/* Use XTAL input as source of PLL0 and PLL1 */
> 		silabs,pll-source = <0 0>, <1 0>;
> 
> 		/* Don't reset PLL1 on rate adjustment */
> 		silabs,pll-reset-mode = <1 1>;
> 
> 		/*
> 		* Overwrite CLK0 configuration with:
> 		* - 8 mA output drive strength
> 		* - initialize output as floating
> 		* - PLL0 as clock source of multisynth 0
> 		* - Multisynth 0 as clock source of output divider
> 		* - Multisynth 0 can change PLL0
> 		*/
> 		master: clkout@0 {
> 			reg = <0>;
> 			silabs,drive-strength = <8>;
> 			silabs,multisynth-source = <0>;
> 			silabs,clock-source = <0>;
> 			silabs,pll-master;
> 			silabs,disable-state = <2>;
> 			clock-frequency = <8000000>;
> 		};
> ...
> 
> > 
> > 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.
> 
> The problem appears to be deeper, that an already locked clock
> manipulation
> requested from the Si5351 drivers issues an jz4780 xfer. Which asks
> for clock
> information. This then tries to acquire the same lock which blocks.
> 
> And the next i2c transfer from a different i2c client tries another
> jz4780 xfer,
> which is locked because the previous one with the si5351 never
> completes.

Indeed, clk_get_rate() gets a static lock.

> IMHO the standard solution is never to ask for clock rates during i2c
> xfer
> but only once during jz4780 i2c probe and cache this value. Drivers
> for other
> SoC are doing it the same way. And this is what this patch is fixing.

Yeah, agreed.

> 
> BR and thanks,
> Nikolaus

Cheers,
-Paul