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