atmel-hlcdc select pixel-clock outside of given range

[Boris Brezillon]

Hi Peter,

On Wed, 22 Aug 2018 13:50:47 +0200
Peter Rosin <peda at axentia.se> wrote:

> Hi!
> 
> I just discovered that the atmel-hlcdc driver picks a pixel-clock
> way outside the given range when used with a panel with these
> timings from the device tree.
> 
> 	panel-timing {
> 		// 1024x768 @ 60Hz (typical)
> 		clock-frequency = <53350000 65000000 80000000>;
> 		hactive = <1024>;
> 		vactive = <768>;
> 		hfront-porch = <48 88 88>;
> 		hback-porch = <96 168 168>;
> 		hsync-len = <32 64 64>;
> 		vfront-porch = <8 13 14>;
> 		vback-porch = <8 13 14>;
> 		vsync-len = <8 12 14>;
> 	};
> 
> IIUC, the atmel-hlcdc driver uses this code to set the pixel-clock
> 
> 	cfg = 0;
> 
> 	prate = clk_get_rate(crtc->dc->hlcdc->sys_clk);
> 	mode_rate = adj->crtc_clock * 1000;
> 	if ((prate / 2) < mode_rate) {
> 		prate *= 2;
> 		cfg |= ATMEL_HLCDC_CLKSEL;
> 	}
> 
> 	div = DIV_ROUND_UP(prate, mode_rate);
> 	if (div < 2)
> 		div = 2;
> 
> 	cfg |= ATMEL_HLCDC_CLKDIV(div);
> 
> 	regmap_update_bits(regmap, ATMEL_HLCDC_CFG(0),
> 			   ATMEL_HLCDC_CLKSEL | ATMEL_HLCDC_CLKDIV_MASK |
> 			   ATMEL_HLCDC_CLKPOL, cfg);
> 
> I.e., the driver sets the divider so that the output frequency is as high
> as possible, but not above the target frequency, which seems sane enough.
> Until you realize that the target frequency is the typical frequency
> from the above device tree (i.e. 65MHz) without regard to the range of
> allowed frequencies.

Yes. I don't know what's the status now, but when I developed this
driver, the accepted range was not exposed. Now there's a way to define
that in the panel desc, but I'm not sure this information is propagated
to the drm_display_mode that is passed to the CRTC.

> 
> In my case, which happens to be really unfortunate, sys_clk is running
> at 132MHz so the above results in div set to 3 which gives an output
> frequency of 44MHz. This is way outside the given 53.35-80Mhz range.
> Why was the mode allowed at all when the constraint was not met?
> Further, selecting a div of 2 gets 66MHz, which is really close to the
> target frequency and well inside the given range.
> 
> Given the overall picture, selecting div = 3 seems totally buggy.

Absolutely.

> 
> Sure, I can work around it by saying
> 		clock-frequency = <53350000 66000000 80000000>;
> but that is ... just an inappropriate workaround.

I agree. The proper solution would probably involve exposing
accepted timing ranges in drm_display_mode.

> 
> Side note, while looking at the above code, I wonder why ATMEL_HLCDC_CLKSEL
> is not set most of the time, instead of only when it absolutely has to?

That sounds like a good idea. Feel free to propose a patch changing
that.

> Dividing the doubled frequency (which seems to be what the flag is doing)
> increases the accuracy of the output frequency. E.g. I would have gotten
> 52.8Mhz instead of 44MHz. So, still not inside the range, but much closer...
> 
> Side note, there is no sanity check in the code for too large div. That
> will cause bits to be written outside the divider field in the register
> and an output frequency that is not what the driver intended. But it is
> probably not that important since div will probably be small most of the
> time?

Adding a check on div makes sense too.

Thanks for reporting the bug and proposing options to fix it.

Boris