Message-ID: <176347245796.268162.1039078457352251254@freya>
Date: Tue, 18 Nov 2025 18:57:37 +0530
From: Jai Luthra <jai.luthra@...asonboard.com>
To: Dave Stevenson <dave.stevenson@...pberrypi.com>
Cc: Jacopo Mondi <jacopo.mondi@...asonboard.com>, Sakari Ailus <sakari.ailus@...ux.intel.com>, Jacopo Mondi <jacopo@...ndi.org>, Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, Mauro Carvalho Chehab <mchehab+huawei@...nel.org>, Kieran Bingham <kieran.bingham@...asonboard.com>, David Plowman <david.plowman@...pberrypi.com>, Laurent Pinchart <laurent.pinchart@...asonboard.com>, Peter Robinson <pbrobinson@...il.com>, Stefan Wahren <wahrenst@....net>, Ivan T. Ivanov <iivanov@...e.de>
Subject: Re: [PATCH 13/13] media: i2c: ov5647: Add V4L2_CID_LINK_FREQUENCY control

Hi Dave,

Quoting Dave Stevenson (2025-11-18 18:17:25)
> Hi Jacopo & Jai
> 
> On Tue, 18 Nov 2025 at 11:28, Jai Luthra <jai.luthra@...asonboard.com> wrote:
> >
> > Hi Jacopo,
> >
> > Quoting Jacopo Mondi (2025-11-02 16:59:02)
> > > Hi Jai
> > >
> > > On Tue, Oct 28, 2025 at 12:57:24PM +0530, Jai Luthra wrote:
> > > > From: Dave Stevenson <dave.stevenson@...pberrypi.com>
> > > >
> > > > The link frequency can vary between modes, so add it as a
> > > > control.
> > > >
> > > > Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> > > > Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
> > > > ---
> > > >  drivers/media/i2c/ov5647.c | 25 ++++++++++++++++++++++++-
> > > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > > index be0b96c4372ae0c6d8fc57280b195d6069dd7019..dea978305c3c868819780f7f631b225f4c1e7756 100644
> > > > --- a/drivers/media/i2c/ov5647.c
> > > > +++ b/drivers/media/i2c/ov5647.c
> > > > @@ -97,6 +97,13 @@ static const char * const ov5647_supply_names[] = {
> > > >
> > > >  #define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
> > > >
> > > > +#define FREQ_INDEX_FULL              0
> > > > +#define FREQ_INDEX_VGA               1
> > > > +static const s64 ov5647_link_freqs[] = {
> > > > +     [FREQ_INDEX_FULL]       = 218500000,
> > >
> > > The full mode pixel rate is set to 87500000, which considering CSI-2
> > > DDR mode and the 2 lanes in use give me a link freq of 21875000.
> >
> > Indeed, I get the same value, will update.
> 
> Agreed. I obviously lost a digit.
> 
> > >
> > > Do you know where 218500000 comes from ? (it might be perfectly legit,
> > > I'm not questioning that).
> > >
> >
> > > > +     [FREQ_INDEX_VGA]        = 208333000,
> >
> > This value should be 137500000 if we do the same calculation using the
> > pixel rate for the VGA mode. But for the VGA mode, the sensor does 2x2
> > binning + 2x2 subsampling, which is quite a bit different than other modes.
> >
> > https://docs.kernel.org/driver-api/media/tx-rx.html#pixel-rate mentions
> > that the pixel rate value calculated from the bus link frequency does not
> > necessarily have to match the PIXEL_RATE control value (which is for the
> > sensor's internal readout of pixels including blanking).
> 
> Indeed you should never assume that pixel rate and link frequency are
> directly linked. So many sensors have separate PLLs for the pixel
> array vs the MIPI block.
> 
> Having said that, OV5647 appears to use the same PLL for pixel clock
> and MIPI, although it does have a separate PLLADCLK which is
> presumably for the ADC.

Ah that's really helpful information, thanks.

> 
> > Ultimately, these values are coming from the BSP where the CFE driver is
> > using the link frequency control to configure the DPHY-RX rate, so I think
> > it would be wiser to not reduce the VGA link frequency value, which may
> > cause issues with DPHY-RX latching. We can always fix it later if needed.
> 
> It's been a long time since I looked at these settings, but I do have
> a spreadsheet from Omnivision that gives clock frequencies based on
> register values.
> In my experience the link frequency isn't critical to be exactly right
> as it typically only sets up timeout ranges in the PHY. Even if the
> value is significantly out it will generally work just fine.
> 
> VGA and full modes differ in register 0x3036 (SC_CMMN_PLL_MULTIPLIER)
> which alters all the timings.
> 
> Running the numbers again, I get the VGA link frequency to be
> 145.8333MHz, but also the pixel rate to be 58.333MPix/s vs 55 in the
> driver. I don't recall the VGA mode being 6% out on frame rate and
> exposure setup, so I can't quite square that with reality. I'll try to
> find 10 minutes to confirm, unless either of you happen to have one
> set up and could validate the frame times.
> 

Running 640x480 capture with default hblank/vblank I get frames at
62.49fps.

58333000/(1852*504) = 62.4946

So 58.333MPix/s seems to be the correct value instead of 55MPix/s.

I had already sent a v2, so I'll wait till next -rc1 is tagged for any
more comments, and will update these values in v3.

Thanks,
Jai

>   Dave
> 

[snip]

Powered by blists - more mailing lists