Hi Laurent Pinchart, thanks for the feedback. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > Sent: Tuesday, January 23, 2024 3:36 PM > Subject: Re: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: > Restructure clk handling > > On Tue, Jan 23, 2024 at 12:20:05PM +0000, Sakari Ailus wrote: > > Hi Biju, > > > > Thanks for the patch. > > > > On Tue, Jan 23, 2024 at 11:58:21AM +0000, Biju Das wrote: > > > As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on > > > the latest hardware manual(R01UH0914EJ0140 Rev.1.40) it is mentioned > > > that we need to supply all CRU clks and we need to disable the vclk > > > before enabling the LINK reception and enable the vclk after > > > enabling the link Reception. So restructure clk handling as per the HW > manual. > > > > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> > > > --- > > > .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 3 - > > > .../platform/renesas/rzg2l-cru/rzg2l-csi2.c | 55 ++++++++++++--- > > > .../platform/renesas/rzg2l-cru/rzg2l-ip.c | 15 +--- > > > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 69 > > > ++++++++----------- > > > 4 files changed, 74 insertions(+), 68 deletions(-) > > > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > > index 811603f18af0..a5a99b004322 100644 > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h > > > @@ -133,9 +133,6 @@ struct rzg2l_cru_dev { > > > struct v4l2_pix_format format; > > > }; > > > > > > -void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru); -int > > > rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru); > > > - > > > int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru); > > > void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru); > > > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c > > > index c4609da9bf1a..f4c5cbb30bc9 100644 > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c > > > @@ -6,6 +6,7 @@ > > > */ > > > > > > #include <linux/clk.h> > > > +#include <linux/clk-provider.h> > > > #include <linux/delay.h> > > > #include <linux/interrupt.h> > > > #include <linux/io.h> > > > @@ -108,6 +109,7 @@ struct rzg2l_csi2 { > > > struct reset_control *presetn; > > > struct reset_control *cmn_rstb; > > > struct clk *sysclk; > > > + struct clk *vclk; > > > unsigned long vclk_rate; > > > > > > struct v4l2_subdev subdev; > > > @@ -361,10 +363,11 @@ static int rzg2l_csi2_dphy_setting(struct > v4l2_subdev *sd, bool on) > > > return rzg2l_csi2_dphy_disable(csi2); } > > > > > > -static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2) > > > +static int rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2) > > > { > > > unsigned long vclk_rate = csi2->vclk_rate / HZ_PER_MHZ; > > > u32 frrskw, frrclk, frrskw_coeff, frrclk_coeff; > > > + int ret, count; > > > > > > /* Select data lanes */ > > > rzg2l_csi2_write(csi2, CSI2nMCT0, 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))) > > > + 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(). Cheers, Biju > > > > + > > > /* Enable LINK reception */ > > > rzg2l_csi2_write(csi2, CSI2nMCT3, CSI2nMCT3_RXEN); > > > + > > > + ret = clk_prepare_enable(csi2->vclk); > > > + if (ret) > > > + return ret; > > > + > > > + for (count = 0; count < 5; count++) { > > > + if (__clk_is_enabled(csi2->vclk)) > > > + break; > > > + usleep_range(10, 20); > > > + } > > > + > > > + if (count == 5) { > > > + dev_err(csi2->dev, "Timeout, not able to turn ON vclk\n"); > > > + return -ETIMEDOUT; > > > + } > > > + > > > + return 0; > > > } > > > > > -- > Regards, > > Laurent Pinchart