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 ?
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.
> > > > +
> > > > /* 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
