[RFC,08/13] smiapp-pll: Take existing divisor into account in minimum divisor check

Commit Message

Pavel Machek Feb. 14, 2017, 1:40 p.m. UTC

From: Sakari Ailus <[e-mail]>

Required added multiplier (and divisor) calculation did not take into
account the existing divisor when checking the values against the
minimum divisor. Do just that.

Signed-off-by: Sakari Ailus <[e-mail]>
Signed-off-by: Ivaylo Dimitrov <[e-mail]>
Signed-off-by: Pavel Machek <[e-mail]>
---
 drivers/media/i2c/smiapp-pll.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Sakari Ailus Feb. 14, 2017, 10:05 p.m. UTC | #1

Hi Pavel,

On Tue, Feb 14, 2017 at 02:40:04PM +0100, Pavel Machek wrote:
> From: Sakari Ailus <[e-mail]>
> 
> Required added multiplier (and divisor) calculation did not take into
> account the existing divisor when checking the values against the
> minimum divisor. Do just that.
> 
> Signed-off-by: Sakari Ailus <[e-mail]>
> Signed-off-by: Ivaylo Dimitrov <[e-mail]>
> Signed-off-by: Pavel Machek <[e-mail]>

I need to understand again why did I write this patch. :-)

Could you send me the smiapp driver output with debug level messages
enabled, please?

I think the problem was with the secondary sensor.

Laurent Pinchart Feb. 14, 2017, 10:14 p.m. UTC | #2

Hi Sakari,

On Wednesday 15 Feb 2017 00:05:03 Sakari Ailus wrote:
> On Tue, Feb 14, 2017 at 02:40:04PM +0100, Pavel Machek wrote:
> > From: Sakari Ailus <[e-mail]>
> > 
> > Required added multiplier (and divisor) calculation did not take into
> > account the existing divisor when checking the values against the
> > minimum divisor. Do just that.
> > 
> > Signed-off-by: Sakari Ailus <[e-mail]>
> > Signed-off-by: Ivaylo Dimitrov <[e-mail]>
> > Signed-off-by: Pavel Machek <[e-mail]>
> 
> I need to understand again why did I write this patch. :-)

I was about to mention that a more detailed commit message (or possibly event 
comments in the source code) would be good :-)

> Could you send me the smiapp driver output with debug level messages
> enabled, please?
> 
> I think the problem was with the secondary sensor.
>
> > diff --git a/drivers/media/i2c/smiapp-pll.c
> > b/drivers/media/i2c/smiapp-pll.c
> > index 771db56..166bbaf 100644
> > --- a/drivers/media/i2c/smiapp-pll.c
> > +++ b/drivers/media/i2c/smiapp-pll.c
> > @@ -16,6 +16,8 @@
> >   * General Public License for more details.
> >   */
> > 
> > +#define DEBUG
> > +

This should be removed.

> >  #include <linux/device.h>
> >  #include <linux/gcd.h>
> >  #include <linux/lcm.h>
> > @@ -227,7 +229,8 @@ static int __smiapp_pll_calculate(
> >  	more_mul_factor = lcm(div, pll->pre_pll_clk_div) / div;
> >  	dev_dbg(dev, "more_mul_factor: %u\n", more_mul_factor);
> > 
> > -	more_mul_factor = lcm(more_mul_factor, op_limits->min_sys_clk_div);
> > +	more_mul_factor = lcm(more_mul_factor,
> > +			      DIV_ROUND_UP(op_limits->min_sys_clk_div, div));
> >  	dev_dbg(dev, "more_mul_factor: min_op_sys_clk_div: %d\n",
> >  		more_mul_factor);
> >  	i = roundup(more_mul_min, more_mul_factor);
> > @@ -456,6 +459,10 @@ int smiapp_pll_calculate(struct device *dev,
> >  	i = gcd(pll->pll_op_clk_freq_hz, pll->ext_clk_freq_hz);
> >  	mul = div_u64(pll->pll_op_clk_freq_hz, i);
> >  	div = pll->ext_clk_freq_hz / i;
> > +	if (!mul) {
> > +		dev_err(dev, "forcing mul to 1\n");
> > +		mul = 1;
> > +	}
> >  	dev_dbg(dev, "mul %u / div %u\n", mul, div);
> >  	
> >  	min_pre_pll_clk_div =

Pavel Machek Feb. 15, 2017, 11:27 a.m. UTC | #3

On Wed 2017-02-15 00:05:03, Sakari Ailus wrote:
> Hi Pavel,
> 
> On Tue, Feb 14, 2017 at 02:40:04PM +0100, Pavel Machek wrote:
> > From: Sakari Ailus <[e-mail]>
> > 
> > Required added multiplier (and divisor) calculation did not take into
> > account the existing divisor when checking the values against the
> > minimum divisor. Do just that.
> > 
> > Signed-off-by: Sakari Ailus <[e-mail]>
> > Signed-off-by: Ivaylo Dimitrov <[e-mail]>
> > Signed-off-by: Pavel Machek <[e-mail]>
> 
> I need to understand again why did I write this patch. :-)

Can you just trust your former self?

> Could you send me the smiapp driver output with debug level messages
> enabled, please?

> I think the problem was with the secondary sensor.

I believe it was with the main sensor, actually. Anyway, here are the
messages.

[    0.791290] smiapp 2-0010: could not get clock (-517)
[    2.705352] smiapp 2-0010: GPIO lookup for consumer xshutdown
[    2.705352] smiapp 2-0010: using device tree for GPIO lookup
[    2.705413] smiapp 2-0010: using lookup tables for GPIO lookup
[    2.705413] smiapp 2-0010: lookup for GPIO xshutdown failed
[    2.875244] smiapp 2-0010: lane_op_clock_ratio: 1
[    2.875274] smiapp 2-0010: binning: 1x1
[    2.875274] smiapp 2-0010: min / max pre_pll_clk_div: 1 / 4
[    2.875305] smiapp 2-0010: pre-pll check: min / max
pre_pll_clk_div: 1 / 1
[    2.875305] smiapp 2-0010: mul 25 / div 2
[    2.875305] smiapp 2-0010: pll_op check: min / max pre_pll_clk_div:
1 / 1
[    2.875335] smiapp 2-0010: pre_pll_clk_div 1
[    2.875335] smiapp 2-0010: more_mul_max: max_pll_multiplier check:
1
[    2.875335] smiapp 2-0010: more_mul_max: max_pll_op_freq_hz check:
1
[    2.875335] smiapp 2-0010: more_mul_max: max_op_sys_clk_div check:
1
[    2.875366] smiapp 2-0010: more_mul_max: min_pll_multiplier check:
1
[    2.875366] smiapp 2-0010: more_mul_min: min_pll_op_freq_hz check:
1
[    2.875366] smiapp 2-0010: more_mul_min: min_pll_multiplier check:
1
[    2.875396] smiapp 2-0010: more_mul_factor: 1
[    2.875396] smiapp 2-0010: more_mul_factor: min_op_sys_clk_div: 1
[    2.875396] smiapp 2-0010: final more_mul: 1
[    2.875427] smiapp 2-0010: op_sys_clk_div: 2
[    2.875427] smiapp 2-0010: op_pix_clk_div: 10
[    2.875427] smiapp 2-0010: pre_pll_clk_div 1
[    2.875457] smiapp 2-0010: pll_multiplier  25
[    2.875457] smiapp 2-0010: vt_sys_clk_div  2
[    2.875457] smiapp 2-0010: vt_pix_clk_div  10
[    2.875457] smiapp 2-0010: ext_clk_freq_hz	9600000
[    2.875488] smiapp 2-0010: pll_ip_clk_freq_hz	9600000
[    2.875488] smiapp 2-0010: pll_op_clk_freq_hz 	240000000
[    2.875488] smiapp 2-0010: vt_sys_clk_freq_hz 	120000000
[    2.875518] smiapp 2-0010: vt_pix_clk_freq_hz 	12000000
[    2.876068] smiapp 2-0010: lane_op_clock_ratio: 1
[    2.876068] smiapp 2-0010: binning: 1x1
[    2.876098] smiapp 2-0010: min / max pre_pll_clk_div: 1 / 4
[    2.876098] smiapp 2-0010: pre-pll check: min / max
pre_pll_clk_div: 1 / 1
[    2.876098] smiapp 2-0010: mul 25 / div 2
[    2.876129] smiapp 2-0010: pll_op check: min / max pre_pll_clk_div:
1 / 1
[    2.876129] smiapp 2-0010: pre_pll_clk_div 1
[    2.876129] smiapp 2-0010: more_mul_max: max_pll_multiplier check:
1
[    2.876159] smiapp 2-0010: more_mul_max: max_pll_op_freq_hz check:
1
[    2.876159] smiapp 2-0010: more_mul_max: max_op_sys_clk_div check:
1
[    2.876159] smiapp 2-0010: more_mul_max: min_pll_multiplier check:
1
[    2.876190] smiapp 2-0010: more_mul_min: min_pll_op_freq_hz check:
1
[    2.876190] smiapp 2-0010: more_mul_min: min_pll_multiplier check:
1
[    2.876190] smiapp 2-0010: more_mul_factor: 1
[    2.876190] smiapp 2-0010: more_mul_factor: min_op_sys_clk_div: 1
[    2.876220] smiapp 2-0010: final more_mul: 1
[    2.876220] smiapp 2-0010: op_sys_clk_div: 2
[    2.876220] smiapp 2-0010: op_pix_clk_div: 10
[    2.876251] smiapp 2-0010: pre_pll_clk_div 1
[    2.876251] smiapp 2-0010: pll_multiplier  25
[    2.876251] smiapp 2-0010: vt_sys_clk_div  2
[    2.876251] smiapp 2-0010: vt_pix_clk_div  10
[    2.876281] smiapp 2-0010: ext_clk_freq_hz	9600000
[    2.876281] smiapp 2-0010: pll_ip_clk_freq_hz	9600000
[    2.876281] smiapp 2-0010: pll_op_clk_freq_hz 	240000000
[    2.876312] smiapp 2-0010: vt_sys_clk_freq_hz 	120000000
[    2.876312] smiapp 2-0010: vt_pix_clk_freq_hz 	12000000
...
[    4.728973] udevd[216]: starting version 175
[    8.031494] smiapp 2-0010: lane_op_clock_ratio: 1
[    8.031524] smiapp 2-0010: binning: 1x1
[    8.031524] smiapp 2-0010: min / max pre_pll_clk_div: 1 / 4
[    8.031524] smiapp 2-0010: pre-pll check: min / max
pre_pll_clk_div: 1 / 1
[    8.031555] smiapp 2-0010: mul 25 / div 2
[    8.031555] smiapp 2-0010: pll_op check: min / max pre_pll_clk_div:
1 / 1
[    8.031555] smiapp 2-0010: pre_pll_clk_div 1
[    8.031585] smiapp 2-0010: more_mul_max: max_pll_multiplier check:
1
[    8.031585] smiapp 2-0010: more_mul_max: max_pll_op_freq_hz check:
1
[    8.031585] smiapp 2-0010: more_mul_max: max_op_sys_clk_div check:
1
[    8.031616] smiapp 2-0010: more_mul_max: min_pll_multiplier check:
1
[    8.031616] smiapp 2-0010: more_mul_min: min_pll_op_freq_hz check:
1
[    8.031616] smiapp 2-0010: more_mul_min: min_pll_multiplier check:
1
[    8.031616] smiapp 2-0010: more_mul_factor: 1
[    8.031646] smiapp 2-0010: more_mul_factor: min_op_sys_clk_div: 1
[    8.031646] smiapp 2-0010: final more_mul: 1
[    8.031646] smiapp 2-0010: op_sys_clk_div: 2
[    8.031677] smiapp 2-0010: op_pix_clk_div: 10
[    8.031677] smiapp 2-0010: pre_pll_clk_div 1
[    8.031677] smiapp 2-0010: pll_multiplier  25
[    8.031707] smiapp 2-0010: vt_sys_clk_div  2
[    8.031707] smiapp 2-0010: vt_pix_clk_div  10
[    8.031707] smiapp 2-0010: ext_clk_freq_hz	9600000
[    8.031738] smiapp 2-0010: pll_ip_clk_freq_hz	9600000
[    8.031738] smiapp 2-0010: pll_op_clk_freq_hz 	240000000
[    8.031738] smiapp 2-0010: vt_sys_clk_freq_hz 	120000000
[    8.031768] smiapp 2-0010: vt_pix_clk_freq_hz 	12000000
[    8.064117] smiapp 2-0010: lane_op_clock_ratio: 1
[    8.064147] smiapp 2-0010: binning: 1x1
[    8.064147] smiapp 2-0010: min / max pre_pll_clk_div: 1 / 4
[    8.064178] smiapp 2-0010: pre-pll check: min / max
pre_pll_clk_div: 1 / 1
[    8.064178] smiapp 2-0010: mul 25 / div 2
[    8.064178] smiapp 2-0010: pll_op check: min / max pre_pll_clk_div:
1 / 1
[    8.064208] smiapp 2-0010: pre_pll_clk_div 1
[    8.064208] smiapp 2-0010: more_mul_max: max_pll_multiplier check:
1
[    8.064208] smiapp 2-0010: more_mul_max: max_pll_op_freq_hz check:
1
[    8.064239] smiapp 2-0010: more_mul_max: max_op_sys_clk_div check:
1
[    8.064239] smiapp 2-0010: more_mul_max: min_pll_multiplier check:
1
[    8.064239] smiapp 2-0010: more_mul_min: min_pll_op_freq_hz check:
1
[    8.064239] smiapp 2-0010: more_mul_min: min_pll_multiplier check:
1
[    8.064270] smiapp 2-0010: more_mul_factor: 1
[    8.064270] smiapp 2-0010: more_mul_factor: min_op_sys_clk_div: 1
[    8.064270] smiapp 2-0010: final more_mul: 1
[    8.064300] smiapp 2-0010: op_sys_clk_div: 2
[    8.064300] smiapp 2-0010: op_pix_clk_div: 10
[    8.064300] smiapp 2-0010: pre_pll_clk_div 1
[    8.064331] smiapp 2-0010: pll_multiplier  25
[    8.064331] smiapp 2-0010: vt_sys_clk_div  2
[    8.064331] smiapp 2-0010: vt_pix_clk_div  10
[    8.064361] smiapp 2-0010: ext_clk_freq_hz	9600000
[    8.064361] smiapp 2-0010: pll_ip_clk_freq_hz	9600000
[    8.064361] smiapp 2-0010: pll_op_clk_freq_hz 	240000000
[    8.064392] smiapp 2-0010: vt_sys_clk_freq_hz 	120000000
[    8.064392] smiapp 2-0010: vt_pix_clk_freq_hz 	12000000

Best regards,
								Pavel

Pali Rohár Feb. 15, 2017, 11:41 a.m. UTC | #4

On Wednesday 15 February 2017 00:05:03 Sakari Ailus wrote:
> Hi Pavel,
> 
> On Tue, Feb 14, 2017 at 02:40:04PM +0100, Pavel Machek wrote:
> > From: Sakari Ailus <[e-mail]>
> > 
> > Required added multiplier (and divisor) calculation did not take into
> > account the existing divisor when checking the values against the
> > minimum divisor. Do just that.
> > 
> > Signed-off-by: Sakari Ailus <[e-mail]>
> > Signed-off-by: Ivaylo Dimitrov <[e-mail]>
> > Signed-off-by: Pavel Machek <[e-mail]>
> 
> I need to understand again why did I write this patch. :-)
> 
> Could you send me the smiapp driver output with debug level messages
> enabled, please?
> 
> I think the problem was with the secondary sensor.
> 

Hi, search for emails and threads:
Message-Id: <[e-mail]>
Message-ID: <[e-mail]>

I think I already resent those information again :-)

Pavel Machek Feb. 28, 2017, 2:09 p.m. UTC | #5

Hi!

> > On Tue, Feb 14, 2017 at 02:40:04PM +0100, Pavel Machek wrote:
> > > From: Sakari Ailus <[e-mail]>
> > > 
> > > Required added multiplier (and divisor) calculation did not take into
> > > account the existing divisor when checking the values against the
> > > minimum divisor. Do just that.
> > > 
> > > Signed-off-by: Sakari Ailus <[e-mail]>
> > > Signed-off-by: Ivaylo Dimitrov <[e-mail]>
> > > Signed-off-by: Pavel Machek <[e-mail]>
> > 
> > I need to understand again why did I write this patch. :-)
> 
> Can you just trust your former self?
> 
> > Could you send me the smiapp driver output with debug level messages
> > enabled, please?
> 
> > I think the problem was with the secondary sensor.
> 
> I believe it was with the main sensor, actually. Anyway, here are the
> messages.

Can I get you to apply this one? :-).

Thanks,
								Pavel

> [    0.791290] smiapp 2-0010: could not get clock (-517)
> [    2.705352] smiapp 2-0010: GPIO lookup for consumer xshutdown
> [    2.705352] smiapp 2-0010: using device tree for GPIO lookup
> [    2.705413] smiapp 2-0010: using lookup tables for GPIO lookup
> [    2.705413] smiapp 2-0010: lookup for GPIO xshutdown failed
> [    2.875244] smiapp 2-0010: lane_op_clock_ratio: 1
> [    2.875274] smiapp 2-0010: binning: 1x1
> [    2.875274] smiapp 2-0010: min / max pre_pll_clk_div: 1 / 4
> [    2.875305] smiapp 2-0010: pre-pll check: min / max
> pre_pll_clk_div: 1 / 1
> [    2.875305] smiapp 2-0010: mul 25 / div 2
> [    2.875305] smiapp 2-0010: pll_op check: min / max pre_pll_clk_div:
> 1 / 1
> [    2.875335] smiapp 2-0010: pre_pll_clk_div 1
> [    2.875335] smiapp 2-0010: more_mul_max: max_pll_multiplier check:
> 1
> [    2.875335] smiapp 2-0010: more_mul_max: max_pll_op_freq_hz check:
> 1
> [    2.875335] smiapp 2-0010: more_mul_max: max_op_sys_clk_div check:
> 1
> [    2.875366] smiapp 2-0010: more_mul_max: min_pll_multiplier check:
> 1
> [    2.875366] smiapp 2-0010: more_mul_min: min_pll_op_freq_hz check:
> 1
> [    2.875366] smiapp 2-0010: more_mul_min: min_pll_multiplier check:
> 1
> [    2.875396] smiapp 2-0010: more_mul_factor: 1
> [    2.875396] smiapp 2-0010: more_mul_factor: min_op_sys_clk_div: 1
> [    2.875396] smiapp 2-0010: final more_mul: 1
> [    2.875427] smiapp 2-0010: op_sys_clk_div: 2
> [    2.875427] smiapp 2-0010: op_pix_clk_div: 10
> [    2.875427] smiapp 2-0010: pre_pll_clk_div 1
> [    2.875457] smiapp 2-0010: pll_multiplier  25
> [    2.875457] smiapp 2-0010: vt_sys_clk_div  2
> [    2.875457] smiapp 2-0010: vt_pix_clk_div  10
> [    2.875457] smiapp 2-0010: ext_clk_freq_hz	9600000
> [    2.875488] smiapp 2-0010: pll_ip_clk_freq_hz	9600000
> [    2.875488] smiapp 2-0010: pll_op_clk_freq_hz 	240000000
> [    2.875488] smiapp 2-0010: vt_sys_clk_freq_hz 	120000000
> [    2.875518] smiapp 2-0010: vt_pix_clk_freq_hz 	12000000
> [    2.876068] smiapp 2-0010: lane_op_clock_ratio: 1
> [    2.876068] smiapp 2-0010: binning: 1x1
> [    2.876098] smiapp 2-0010: min / max pre_pll_clk_div: 1 / 4
> [    2.876098] smiapp 2-0010: pre-pll check: min / max
> pre_pll_clk_div: 1 / 1
> [    2.876098] smiapp 2-0010: mul 25 / div 2
> [    2.876129] smiapp 2-0010: pll_op check: min / max pre_pll_clk_div:
> 1 / 1
> [    2.876129] smiapp 2-0010: pre_pll_clk_div 1
> [    2.876129] smiapp 2-0010: more_mul_max: max_pll_multiplier check:
> 1
> [    2.876159] smiapp 2-0010: more_mul_max: max_pll_op_freq_hz check:
> 1
> [    2.876159] smiapp 2-0010: more_mul_max: max_op_sys_clk_div check:
> 1
> [    2.876159] smiapp 2-0010: more_mul_max: min_pll_multiplier check:
> 1
> [    2.876190] smiapp 2-0010: more_mul_min: min_pll_op_freq_hz check:
> 1
> [    2.876190] smiapp 2-0010: more_mul_min: min_pll_multiplier check:
> 1
> [    2.876190] smiapp 2-0010: more_mul_factor: 1
> [    2.876190] smiapp 2-0010: more_mul_factor: min_op_sys_clk_div: 1
> [    2.876220] smiapp 2-0010: final more_mul: 1
> [    2.876220] smiapp 2-0010: op_sys_clk_div: 2
> [    2.876220] smiapp 2-0010: op_pix_clk_div: 10
> [    2.876251] smiapp 2-0010: pre_pll_clk_div 1
> [    2.876251] smiapp 2-0010: pll_multiplier  25
> [    2.876251] smiapp 2-0010: vt_sys_clk_div  2
> [    2.876251] smiapp 2-0010: vt_pix_clk_div  10
> [    2.876281] smiapp 2-0010: ext_clk_freq_hz	9600000
> [    2.876281] smiapp 2-0010: pll_ip_clk_freq_hz	9600000
> [    2.876281] smiapp 2-0010: pll_op_clk_freq_hz 	240000000
> [    2.876312] smiapp 2-0010: vt_sys_clk_freq_hz 	120000000
> [    2.876312] smiapp 2-0010: vt_pix_clk_freq_hz 	12000000
> ...
> [    4.728973] udevd[216]: starting version 175
> [    8.031494] smiapp 2-0010: lane_op_clock_ratio: 1
> [    8.031524] smiapp 2-0010: binning: 1x1
> [    8.031524] smiapp 2-0010: min / max pre_pll_clk_div: 1 / 4
> [    8.031524] smiapp 2-0010: pre-pll check: min / max
> pre_pll_clk_div: 1 / 1
> [    8.031555] smiapp 2-0010: mul 25 / div 2
> [    8.031555] smiapp 2-0010: pll_op check: min / max pre_pll_clk_div:
> 1 / 1
> [    8.031555] smiapp 2-0010: pre_pll_clk_div 1
> [    8.031585] smiapp 2-0010: more_mul_max: max_pll_multiplier check:
> 1
> [    8.031585] smiapp 2-0010: more_mul_max: max_pll_op_freq_hz check:
> 1
> [    8.031585] smiapp 2-0010: more_mul_max: max_op_sys_clk_div check:
> 1
> [    8.031616] smiapp 2-0010: more_mul_max: min_pll_multiplier check:
> 1
> [    8.031616] smiapp 2-0010: more_mul_min: min_pll_op_freq_hz check:
> 1
> [    8.031616] smiapp 2-0010: more_mul_min: min_pll_multiplier check:
> 1
> [    8.031616] smiapp 2-0010: more_mul_factor: 1
> [    8.031646] smiapp 2-0010: more_mul_factor: min_op_sys_clk_div: 1
> [    8.031646] smiapp 2-0010: final more_mul: 1
> [    8.031646] smiapp 2-0010: op_sys_clk_div: 2
> [    8.031677] smiapp 2-0010: op_pix_clk_div: 10
> [    8.031677] smiapp 2-0010: pre_pll_clk_div 1
> [    8.031677] smiapp 2-0010: pll_multiplier  25
> [    8.031707] smiapp 2-0010: vt_sys_clk_div  2
> [    8.031707] smiapp 2-0010: vt_pix_clk_div  10
> [    8.031707] smiapp 2-0010: ext_clk_freq_hz	9600000
> [    8.031738] smiapp 2-0010: pll_ip_clk_freq_hz	9600000
> [    8.031738] smiapp 2-0010: pll_op_clk_freq_hz 	240000000
> [    8.031738] smiapp 2-0010: vt_sys_clk_freq_hz 	120000000
> [    8.031768] smiapp 2-0010: vt_pix_clk_freq_hz 	12000000
> [    8.064117] smiapp 2-0010: lane_op_clock_ratio: 1
> [    8.064147] smiapp 2-0010: binning: 1x1
> [    8.064147] smiapp 2-0010: min / max pre_pll_clk_div: 1 / 4
> [    8.064178] smiapp 2-0010: pre-pll check: min / max
> pre_pll_clk_div: 1 / 1
> [    8.064178] smiapp 2-0010: mul 25 / div 2
> [    8.064178] smiapp 2-0010: pll_op check: min / max pre_pll_clk_div:
> 1 / 1
> [    8.064208] smiapp 2-0010: pre_pll_clk_div 1
> [    8.064208] smiapp 2-0010: more_mul_max: max_pll_multiplier check:
> 1
> [    8.064208] smiapp 2-0010: more_mul_max: max_pll_op_freq_hz check:
> 1
> [    8.064239] smiapp 2-0010: more_mul_max: max_op_sys_clk_div check:
> 1
> [    8.064239] smiapp 2-0010: more_mul_max: min_pll_multiplier check:
> 1
> [    8.064239] smiapp 2-0010: more_mul_min: min_pll_op_freq_hz check:
> 1
> [    8.064239] smiapp 2-0010: more_mul_min: min_pll_multiplier check:
> 1
> [    8.064270] smiapp 2-0010: more_mul_factor: 1
> [    8.064270] smiapp 2-0010: more_mul_factor: min_op_sys_clk_div: 1
> [    8.064270] smiapp 2-0010: final more_mul: 1
> [    8.064300] smiapp 2-0010: op_sys_clk_div: 2
> [    8.064300] smiapp 2-0010: op_pix_clk_div: 10
> [    8.064300] smiapp 2-0010: pre_pll_clk_div 1
> [    8.064331] smiapp 2-0010: pll_multiplier  25
> [    8.064331] smiapp 2-0010: vt_sys_clk_div  2
> [    8.064331] smiapp 2-0010: vt_pix_clk_div  10
> [    8.064361] smiapp 2-0010: ext_clk_freq_hz	9600000
> [    8.064361] smiapp 2-0010: pll_ip_clk_freq_hz	9600000
> [    8.064361] smiapp 2-0010: pll_op_clk_freq_hz 	240000000
> [    8.064392] smiapp 2-0010: vt_sys_clk_freq_hz 	120000000
> [    8.064392] smiapp 2-0010: vt_pix_clk_freq_hz 	12000000
> 
> Best regards,
> 								Pavel
> 
>

Sakari Ailus Feb. 28, 2017, 2:16 p.m. UTC | #6

On Tue, Feb 28, 2017 at 03:09:21PM +0100, Pavel Machek wrote:
> Can I get you to apply this one? :-).

Let me try to understand again what does that change actually do. I'll find
the time during the rest of this week.

I'm starting to think we need a test suite for the PLL calculator...

Pavel Machek March 22, 2017, 11:46 p.m. UTC | #7

On Tue 2017-02-28 16:16:21, Sakari Ailus wrote:
> On Tue, Feb 28, 2017 at 03:09:21PM +0100, Pavel Machek wrote:
> > Can I get you to apply this one? :-).
> 
> Let me try to understand again what does that change actually do. I'll find
> the time during the rest of this week.
> 
> I'm starting to think we need a test suite for the PLL calculator...

Any update here or on the other patch? We are quite close to working
camera now...

Plus I have played with v4l-utils, and managed to implement autofocus
and autoexposure -- it was easier than expected. I believe you
mentioned you had some patches to automatically initialize the
pipeline. Do you and can I have them?

Last thing.. Is someone able to compute new modes for et8ek8? I
believe smaller than 640x480 mode would be useful for video streaming,
and I still can't get 5MPix mode to work; understanding what goes on
there would be useful.

Thanks,
									Pavel

Pavel Machek March 23, 2017, 12:09 a.m. UTC | #8

On Thu 2017-03-23 00:46:51, Pavel Machek wrote:
> On Tue 2017-02-28 16:16:21, Sakari Ailus wrote:
> > On Tue, Feb 28, 2017 at 03:09:21PM +0100, Pavel Machek wrote:
> > > Can I get you to apply this one? :-).
> > 
> > Let me try to understand again what does that change actually do. I'll find
> > the time during the rest of this week.
> > 
> > I'm starting to think we need a test suite for the PLL calculator...
> 
> Any update here or on the other patch? We are quite close to working
> camera now...
> 
> Plus I have played with v4l-utils, and managed to implement autofocus
> and autoexposure -- it was easier than expected. I believe you
> mentioned you had some patches to automatically initialize the
> pipeline. Do you and can I have them?
> 
> Last thing.. Is someone able to compute new modes for et8ek8? I
> believe smaller than 640x480 mode would be useful for video streaming,
> and I still can't get 5MPix mode to work; understanding what goes on
> there would be useful.

Oh and.. is it possible to generate modes with more than 30fps? I
guess that would be useful for initial autofocus/autogain....
								Pavel

Sakari Ailus March 23, 2017, 7:32 a.m. UTC | #9

Hi Pavel,

On Thu, Mar 23, 2017 at 12:46:51AM +0100, Pavel Machek wrote:
> On Tue 2017-02-28 16:16:21, Sakari Ailus wrote:
> > On Tue, Feb 28, 2017 at 03:09:21PM +0100, Pavel Machek wrote:
> > > Can I get you to apply this one? :-).
> > 
> > Let me try to understand again what does that change actually do. I'll find
> > the time during the rest of this week.
> > 
> > I'm starting to think we need a test suite for the PLL calculator...
> 
> Any update here or on the other patch? We are quite close to working
> camera now...

I've been working on PLL test cases. The PLL calculator really requires that
to be able to test any changes made to it.

> 
> Plus I have played with v4l-utils, and managed to implement autofocus
> and autoexposure -- it was easier than expected. I believe you
> mentioned you had some patches to automatically initialize the
> pipeline. Do you and can I have them?

It was an early prototype and it wasn't really functional yet.

Given a video node, it can find possible pipelines to the image sources with
common formats. I.e. the ccdc -> rsz path is not available for raw cameras.

C (especially without helper libraries) wasn't particularly suitable for the
task, the data structures I had didn't end up too nice. What would also be
necessary is to associate library or application specific data to entities,
this could be as simple as key-value pairs with both key and value being
pointers.

> 
> Last thing.. Is someone able to compute new modes for et8ek8? I
> believe smaller than 640x480 mode would be useful for video streaming,
> and I still can't get 5MPix mode to work; understanding what goes on
> there would be useful.

Unfortunately the et8ek8 does not conform to a standard such as SMIA. :-(
I'm not sure the datasheet provides enough information to come up with new
mode definitions. Perhaps with some experimentation it could be possible.
There are a few additional embedded documents in it, those are worth
checking out. LibreOffice can open them AFAIR.

Pavel Machek March 23, 2017, 1:22 p.m. UTC | #10

Hi!

> > Plus I have played with v4l-utils, and managed to implement autofocus
> > and autoexposure -- it was easier than expected. I believe you
> > mentioned you had some patches to automatically initialize the
> > pipeline. Do you and can I have them?
> 
> It was an early prototype and it wasn't really functional yet.
> 
> Given a video node, it can find possible pipelines to the image sources with
> common formats. I.e. the ccdc -> rsz path is not available for raw
> cameras.

> C (especially without helper libraries) wasn't particularly suitable for the
> task, the data structures I had didn't end up too nice. What would also be
> necessary is to associate library or application specific data to entities,
> this could be as simple as key-value pairs with both key and value being
> pointers.

Could I get a copy, anyway? Need not be perfect, but starting point
would be welcome.

Thanks,
									Pavel

Patch

diff --git a/drivers/media/i2c/smiapp-pll.c b/drivers/media/i2c/smiapp-pll.c
index 771db56..166bbaf 100644
--- a/drivers/media/i2c/smiapp-pll.c
+++ b/drivers/media/i2c/smiapp-pll.c
@@ -16,6 +16,8 @@ 
  * General Public License for more details.
  */
 
+#define DEBUG
+
 #include <linux/device.h>
 #include <linux/gcd.h>
 #include <linux/lcm.h>
@@ -227,7 +229,8 @@  static int __smiapp_pll_calculate(
 
 	more_mul_factor = lcm(div, pll->pre_pll_clk_div) / div;
 	dev_dbg(dev, "more_mul_factor: %u\n", more_mul_factor);
-	more_mul_factor = lcm(more_mul_factor, op_limits->min_sys_clk_div);
+	more_mul_factor = lcm(more_mul_factor,
+			      DIV_ROUND_UP(op_limits->min_sys_clk_div, div));
 	dev_dbg(dev, "more_mul_factor: min_op_sys_clk_div: %d\n",
 		more_mul_factor);
 	i = roundup(more_mul_min, more_mul_factor);
@@ -456,6 +459,10 @@  int smiapp_pll_calculate(struct device *dev,
 	i = gcd(pll->pll_op_clk_freq_hz, pll->ext_clk_freq_hz);
 	mul = div_u64(pll->pll_op_clk_freq_hz, i);
 	div = pll->ext_clk_freq_hz / i;
+	if (!mul) {
+		dev_err(dev, "forcing mul to 1\n");
+		mul = 1;
+	}
 	dev_dbg(dev, "mul %u / div %u\n", mul, div);
 
 	min_pre_pll_clk_div =