atmel-hlcdc select pixel-clock outside of given range

[Peter Rosin]

On 2018-08-22 13:50, Peter Rosin 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.
> 
> 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.
> 
> Sure, I can work around it by saying
> 		clock-frequency = <53350000 66000000 80000000>;
> but that is ... just an inappropriate workaround.
> 
> 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?
> 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 Thierry Reding since I'm dragging panel-lvds into this]

I dug a little deeper and this is apparently in no way the fault of the
atmel-hlcdc driver. It has no knowledge of the upper and lower bounds of
the given display-timings, and it simply can't be responsible for digging
them up.

Those constraints are thrown away in the panel-lvds driver just after they
are parsed in panel_lvds_parse_dt() with the call to videomode_from_timing().
Apparently, the panel-lvds driver is only ever going to report one supported
mode, with the typical timings, and has no way of forwarding the given
constraints that I see. So, what should it do?

IMHO, it is a serious design limitation that the constraints on the mode
cannot be propagated to the driver responsible for generating it.

So, given this, maybe it is not so obvious that you should always round
the pixel-clock divider up in the atmel-hlcdc driver? In my case it would
certainly have been better if the pixel-clock was rounded to the closes
possible divider even if that would have generated a pixel-clock frequency
1.5% higher than requested by the drm machinery...

Cheers,
Peter