Re: [PATCH v2 03/12] media: mt9m114: Use aptina-PLL helper to get PLL values

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Sat, Jun 28, 2025 at 11:27:23AM +0200, Hans de Goede wrote:
> On 27-Jun-25 8:06 PM, Laurent Pinchart wrote:
> > On Fri, Jun 27, 2025 at 04:33:46PM +0200, Hans de Goede wrote:
> >> On 3-Jun-25 4:05 PM, Laurent Pinchart wrote:
> >>> On Tue, Jun 03, 2025 at 03:29:42PM +0200, Hans de Goede wrote:
> >>>> On 3-Jun-25 12:57 PM, Laurent Pinchart wrote:
> >>>>> On Sat, May 31, 2025 at 06:31:38PM +0200, Hans de Goede wrote:
> >>>>>> Before this change the driver used hardcoded PLL m, n and p values to
> >>>>>> achieve a 48MHz pixclock when used with an external clock with a frequency
> >>>>>> of 24 MHz.
> >>>>>>
> >>>>>> Use aptina_pll_calculate() to allow the driver to work with different
> >>>>>> external clock frequencies. The m, n, and p values will be unchanged
> >>>>>> with a 24 MHz extclk and this has also been tested with a 19.2 MHz
> >>>>>> clock where m gets increased from 32 to 40.
> >>>>>>
> >>>>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>>> Signed-off-by: Hans de Goede <hansg@kernel.org>
> >>>>>> ---
> >>>>>> Changes in v2:
> >>>>>> - Add select VIDEO_APTINA_PLL to Kconfig
> >>>>>> - Use correct aptina_pll_limits
> >>>>>> ---
> >>>>>>  drivers/media/i2c/Kconfig   |  1 +
> >>>>>>  drivers/media/i2c/mt9m114.c | 54 ++++++++++++++++++++++++++-----------
> >>>>>>  2 files changed, 40 insertions(+), 15 deletions(-)
> 
> ...
> 
> >>>>>> @@ -2262,12 +2262,29 @@ static int mt9m114_verify_link_frequency(struct mt9m114 *sensor,
> >>>>>>  
> >>>>>>  static int mt9m114_clk_init(struct mt9m114 *sensor)
> >>>>>>  {
> >>>>>> +	static const struct aptina_pll_limits limits = {
> >>>>>> +		.ext_clock_min = 6000000,
> >>>>>> +		.ext_clock_max = 54000000,
> >>>>>> +		/* int_clock_* limits are not documented taken from mt9p031.c */
> >>>>>> +		.int_clock_min = 2000000,
> >>>>>> +		.int_clock_max = 13500000,
> >>>>>> +		/*
> >>>>>> +		 * out_clock_min is not documented, taken from mt9p031.c.
> >>>>>> +		 * out_clock_max is documented as 768MHz, but this leads to
> >>>>>> +		 * different PLL settings then used by the vendor's drivers.
> >>>>>
> >>>>> s/then/than/
> >>>>>
> >>>>> Is that an issue though ? Does it prevent the sensor from working ?
> >>>>
> >>>> I did not try. It seems safer to just stick with the tested / proven
> >>>> values from the older register-list based drivers?
> >>>
> >>> Sometimes there are multiple options without any practical differences.
> >>> I wouldn't necessarily assume we have to follow the hardcoded register
> >>> values. Can you share the two PLL configurations ?
> >>>
> >>>> Even if it does work on my single mt9m114 sensor, that hardly
> >>>> constitutes testing on a representative sample.
> >>>
> >>> This sensor is rather old and not widely used. If using the correct
> >>> limits doesn't cause issues on platforms we can test, that's good enough
> >>> for me. We can always address problems later if any arise.
> >>
> >> Ok, so I've tried setting out_clock_max to 768MHz, but that results
> >> in PLL setting which cause the sensor to fail to stream.
> > 
> > Thank you for testing.
> > 
> > Could you share the PLL configuration you obtain with the 400 MHz and
> > 768 MHz limits ?
> 
> 400 MHz out_clock_max, working setup:
> 
> [   40.136435] mt9m114 i2c-INT33F0:00: PLL: ext clock 19200000 pix clock 48000000
> [   40.136453] mt9m114 i2c-INT33F0:00: pll: mf min 4 max 38
> [   40.136463] mt9m114 i2c-INT33F0:00: pll: p1 min 4 max 8
> [   40.136473] mt9m114 i2c-INT33F0:00: PLL: N 2 M 40 P1 8
> 
> 768 MHz non working setup:
> 
> [   28.388940] mt9m114 i2c-INT33F0:00: PLL: ext clock 19200000 pix clock 48000000
> [   28.388955] mt9m114 i2c-INT33F0:00: pll: mf min 4 max 38
> [   28.388966] mt9m114 i2c-INT33F0:00: pll: p1 min 4 max 16
> [   28.388976] mt9m114 i2c-INT33F0:00: PLL: N 2 M 80 P1 16

Hmmm...

Re-reading the documentation, I wonder if I had misunderstood the PLL.

There is no single clear description of the PLL (it would be too easy),
but there's informatio scattered in multiple places. One of them states
that

Fout = (Fin*2*m)/((n+1)*(p+1)).

Note the *2 multiplier. In another location, multiple frequencies are
defined:

fSensor = (M x fin) / ((N + 1) x (P + 1)) (max 48 MHz)
fWord = fSensor x 2 (max 96 MHz)
fBit = fWord * 8 (max 763 MHz)

The '+1' for N and P are understood, they relate to the values
programmed in the registers. From the point of view of the PLL
calculator, we should reason with that offset, with

Fout = (Fin*2*m)/(n*p).

and

fSensor = (M x fin) / (N x P)

This leads me to believe that the PLL is organized as follows:

        +-----+     +-----+     +-----+     +-----+     +-----+
Fin --> | / N | --> | x M | --> | x 2 | --> | / P | --> | / 2 | -->
        +-----+     +-----+     +-----+     +-----+     +-----+
	                                fBit       fWord        fSensor
ext_clock    int_clock   out_clock                             pix_clock

Seen this way, the maximum limit for out_clock should be 384 MHz, and
the P factor should be hardcoded to 8 for CSI-2 output.

Does that make sense to you ?

-- 
Regards,

Laurent Pinchart