Hi Laurent and Sakari,
> -----Original Message-----
> From: Biju Das
> Sent: Wednesday, January 24, 2024 9:50 PM
> Subject: RE: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video:
> Restructure clk handling
>
> > -----Original Message-----
> > From: Stephen Boyd <sboyd@xxxxxxxxxx>
> > Sent: Wednesday, January 24, 2024 8:49 PM
> > Subject: RE: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video:
> > Restructure clk handling
> >
> > Quoting Biju Das (2024-01-24 06:01:30)
> > > > > > > > CSI2nMCT0_VDLN(csi2->lanes)); @@
> > > > > > > > -386,11 +389,40 @@ static void
> > > > > > > > rzg2l_csi2_mipi_link_enable(struct
> > > > > > rzg2l_csi2 *csi2)
> > > > > > > > rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
> > > > > > > > rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
> > > > > > > >
> > > > > > > > + clk_disable_unprepare(csi2->vclk);
> > > > > > > > + for (count = 0; count < 5; count++) {
> > > > > > > > + if (!(__clk_is_enabled(csi2->vclk)))
> >
> > __clk_is_enabled() is a clk provider API. You shouldn't be using it in
> > consumer drivers.
>
> OK, Basically before enabling the link reception, vclk must be disabled.
>
> Currently rzg2l clk driver is turning off the clock but it is not checking
> the status to make sure it is actually turned off.
>
> I guess checking the status is not mandatory for clock OFF(disable) for
> performance reasons.
>
> So shall I introduce clk_disable_unprepare_sync() and .disable_sync() for
> disabling and checking the status to make sure clock is turned off So that
> I can enable link reception reliably??
>
> .disable(), just turn off the clock, but it doesn't guarantee that it is
> actually turned off.
>
> whereas .disable_sync() guarantees that the clk is actually turned off.
>
> >
> > > > > > > > + break;
> > > > > > > > + usleep_range(10, 20);
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + if (count == 5) {
> > > > > > > > + dev_err(csi2->dev, "Timeout, not able to
> > > > > > > > + turn OFF
> > > > vclk\n");
> > > > > > > > + return -ETIMEDOUT;
> > > > > > > > + }
> > > > > > >
> > > > > > > It'd be nice to have a CCF function to do this. Either way,
> > > > > > > you can use read_poll_timeout().
> > > > > >
> > > > > > I would have sworn that clk_disable_unprepare() is
> > > > > > synchronous, and would have sworn even stronger for
> clk_prepare_enable().
> > > > > > What's going on here, is there really a delay ? It sounds like
> > > > > > a bug in the
> > > > clock driver.
> > > > >
> > > > > At the moment we are not checking clock status when we turn off
> > > > > a clock However, for clock ON we are checking the status while
> > > > > turning it
> > > > ON.
> > > > > We need to fix the driver for clk_disable_unprepare().
> > > >
> > > > Does that mean that the check below clk_prepare_enable() could be
> > > > removed already ?
> > >
> > > Yes, that is correct I will remove in the next version as
> > > clk_prepare_enable() is synchronous as it checks the status to make
> > > sure
> > it is turned ON.
> > >
> > > >
> > > > Regarding clock disable, it isn't clear if the API guarantees
> > > > synchronous calls. I'd recommend asking the clock maintainers. If
> > > > it doesn't, then the clock driver isn't wrong (and may lead to
> > > > faster operation in most cases), but I think synchronizing the
> > > > clock disable by waiting for the clock to be actually disabled
> > > > should be
> > implemented as a helper in CCF.
> > >
> > > +clk.
> > >
> > > Hi Stephen and all,
> > >
> > > Can you please shed some light on this?
> > >
> >
> > clk_disable() is reference counted. The call to clk_disable() may do
> > nothing besides decrement a count and return. Per the documentation in
> > clk.h it means that the consumer is no longer using the clk. The clk
> > could be turned off later, or not at all.
> >
> > It seems like the clk driver has a bug. Make the clk driver
> > synchronize the disable with enable please.
>
> You mean like enable(), we should check the clock status for disable() to
> make sure it is turned off??
>
> Or
>
> Introduce a new API .disable_sync() for this special synchronization
> operation??
I have tested RZ/G2{L,LC,UL} and RZ/V2L SMARC EVK platforms with
just clk_disable_unprepare() and the clk_prepare_enable() the capture works fine
on these platforms.
So, I will send V2 with these changes. Please let me know if you think otherwise.
Also, I am planning to send a RFC for the API clk_disable_unprepare_sync()
in CCF for separate discussion which guarantees synchronous operation
for Clock OFF.
Cheers,
Biju
