atmel-hlcdc select pixel-clock outside of given range

[Peter Rosin]

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?

Cheers,
Peter