RE: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent Pinchart,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Tuesday, January 23, 2024 10:10 PM
> Subject: Re: [PATCH 4/4] media: platform: rzg2l-cru: rzg2l-video:
> Restructure clk handling
> 
> On Tue, Jan 23, 2024 at 06:42:19PM +0000, Biju Das wrote:
> > > On Tue, Jan 23, 2024 at 12:20:05PM +0000, Sakari Ailus wrote:
> > > > 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().
> 
> 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?

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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux