| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <19848593-4879-46fa-8969-d34c2fc511ed@emfend.at>
Date: Mon, 22 Dec 2025 16:58:28 +0100
From: Matthias Fend <matthias.fend@...end.at>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Hans Verkuil <hverkuil@...nel.org>,
Hans de Goede <hansg@...nel.org>, Ricardo Ribalda <ribalda@...omium.org>,
André Apitzsch <git@...tzsch.eu>,
Tarang Raval <tarang.raval@...iconsignals.io>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
Sylvain Petinot <sylvain.petinot@...s.st.com>,
Dongcheng Yan <dongcheng.yan@...el.com>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Alan Stern <stern@...land.harvard.edu>,
Jingjing Xiong <jingjing.xiong@...el.com>,
Heimir Thor Sverrisson <heimir.sverrisson@...il.com>,
Mehdi Djait <mehdi.djait@...ux.intel.com>,
Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>,
Svyatoslav Ryhel <clamor95@...il.com>, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Hao Yao <hao.yao@...el.com>
Subject: Re: [PATCH v6 2/2] media: i2c: add Himax HM1246 image sensor driver
Hi Laurent,
Am 22.12.2025 um 14:30 schrieb Laurent Pinchart:
> On Mon, Dec 22, 2025 at 12:17:56PM +0100, Matthias Fend wrote:
>> Am 19.12.2025 um 23:29 schrieb Sakari Ailus:
>>> Hi Matthias,
>>>
>>> Thanks for the update.
>>>
>>> On Tue, Dec 02, 2025 at 04:26:06PM +0100, Matthias Fend wrote:
>>>
>>> ...
>>>
>>>> +static int hm1246_calc_pll(struct hm1246 *hm1246, u32 xclk, u32 link_freq,
>>>> + u32 clocks_per_pixel, u8 *pll1, u8 *pll2, u8 *pll3)
>>>> +{
>>>> + const u8 pclk_div_table[] = { 4, 5, 6, 7, 8, 12, 14, 16 };
>>>> + const u8 sysclk_div_table[] = { 1, 2, 3, 4 };
>>>> + const u8 post_div_table[] = { 1, 2, 4, 8 };
>>>> + const int sysclk_pclk_ratio = 3; /* Recommended value */
>>>> + u32 pclk, vco_out, best_vco_diff;
>>>> + int pclk_div_index, sysclk_div_index, post_div_index;
>>>> + u8 pre_div = 0, multiplier_h = 0, multiplier_l = 0;
>>>> + bool sysclk_pclk_ratio_found = false;
>>>> +
>>>> + if (link_freq < HM1246_PCLK_MIN || link_freq > HM1246_PCLK_MAX)
>>>> + return -EINVAL;
>>>> +
>>>> + /*
>>>> + * In raw mode (1 pixel per clock) the pixel clock is internally
>>>> + * divided by two.
>>>> + */
>>>> + pclk = 2 * link_freq / clocks_per_pixel;
>>>> +
>>>> + /* Find suitable PCLK and SYSCLK dividers. */
>>>> + for (pclk_div_index = 0; pclk_div_index < ARRAY_SIZE(pclk_div_table);
>>>> + pclk_div_index++) {
>>>> + for (sysclk_div_index = 0;
>>>> + sysclk_div_index < ARRAY_SIZE(sysclk_div_table);
>>>> + sysclk_div_index++) {
>>>> + if (sysclk_div_table[sysclk_div_index] *
>>>> + sysclk_pclk_ratio ==
>>>> + pclk_div_table[pclk_div_index]) {
>>>> + sysclk_pclk_ratio_found = true;
>>>> + break;
>>>> + }
>>>> + }
>>>> + if (sysclk_pclk_ratio_found)
>>>> + break;
>>>> + }
>>>> +
>>>> + if (!sysclk_pclk_ratio_found)
>>>> + return -EINVAL;
>>>> +
>>>> + /* Determine an appropriate post divider. */
>>>> + for (post_div_index = 0; post_div_index < ARRAY_SIZE(post_div_table);
>>>> + post_div_index++) {
>>>> + vco_out = pclk * pclk_div_table[pclk_div_index] *
>>>> + post_div_table[post_div_index];
>>>> +
>>>> + if (vco_out >= HM1246_PLL_VCO_MIN &&
>>>> + vco_out <= HM1246_PLL_VCO_MAX)
>>>> + break;
>>>> + }
>>>> + if (post_div_index >= ARRAY_SIZE(post_div_table))
>>>> + return -EINVAL;
>>>> +
>>>> + /* Find best pre-divider and multiplier values. */
>>>> + best_vco_diff = U32_MAX;
>>>> + for (u32 div = DIV_ROUND_UP(xclk, HM1246_PLL_INCLK_MAX);
>>>> + div <= xclk / HM1246_PLL_INCLK_MIN; div++) {
>>>> + u32 multi, multi_h, multi_l, vco, diff;
>>>> +
>>>> + multi = DIV_ROUND_CLOSEST_ULL((u64)vco_out * div, xclk);
>>>> + if (multi < HM1246_PLL_MULTI_MIN ||
>>>> + multi > HM1246_PLL_MULTI_MAX)
>>>> + continue;
>>>> +
>>>> + multi_h = multi / (HM1246_PLL_MULTI_H_MIN *
>>>> + HM1246_PLL_MULTI_L_MAX) +
>>>> + 2;
>>>> + multi_l = multi / multi_h;
>>>> + vco = div_u64((u64)xclk * multi_h * multi_l, div);
>>>> +
>>>> + diff = abs_diff(vco_out, vco);
>>>> +
>>>> + if (diff < best_vco_diff) {
>>>> + best_vco_diff = diff;
>>>> + pre_div = div;
>>>> + multiplier_h = multi_h;
>>>> + multiplier_l = multi_l;
>>>> + }
>>>> +
>>>> + if (!diff)
>>>> + break;
>>>> + }
>>>> +
>>>> + if (best_vco_diff == U32_MAX)
>>>> + return -EINVAL;
>>>
>>> How much difference is acceptable? Isn't any difference a bug either in DT
>>> or the code above? In other words, I'd return an error in that case.
>>
>> Hard to tell, but almost every input clock will result in a slight
>> difference. Even the recommended reference register configuration
>> doesn't create a perfect match. Therefore, I don't think it's a good
>> idea to treat every deviation as an error.
>
> I understand that the PLL won't be able to produce the exact nominal
> expected frequency, but can't we require the link-frequencies property
> in DT to match the PLL output exactly ? That's what we do with other
> sensors.
You mean that any link frequency (within the allowed range) can be
specified in the device tree, which the PLL can generate exactly? The
values for the v4l2 controls V4L2_CID_PIXEL_RATE and
V4L2_CID_LINK_FREQ are then also based on the DT link freuqency.
The recently added IMX111 does something similar. For many other
sensors, the link frequencies are rather fixed.
Thanks
~Matthias
>
>> However, every supported input frequency (6-27MHz) will result in a
>> pixel clock deviation of less than 0.5%. Since the sensor uses a
>> parallel interface, these frequencies will all work without any
>> problems. The frame timings may, of course, be slightly different.
>>
>> To change this and prevent any deviation, one would probably have to
>> replace the PLL calculation with one or more dedicated frequency-setup
>> pairs (with adjusted pixelclocks). Which wouldn't be ideal, as the
>> solution isn't very flexible - and I've invested quite a bit of effort
>> in the PLL calculation ;)
>>
>>>> +
>>>> + *pll1 = HM1246_PLL1CFG_MULTIPLIER(multiplier_l - 1);
>>>> + *pll2 = HM1246_PLL2CFG_PRE_DIV(pre_div - 1) |
>>>> + HM1246_PLL2CFG_MULTIPLIER(multiplier_h - 2);
>>>> + *pll3 = HM1246_PLL3CFG_POST_DIV(post_div_index) |
>>>> + HM1246_PLL3CFG_SYSCLK_DIV(sysclk_div_index) |
>>>> + HM1246_PLL3CFG_PCLK_DIV(pclk_div_index);
>>>> +
>>>> + return 0;
>>>> +}
>
Powered by blists - more mailing lists