Re: [PATCH RFC 2/2] clk: scmi: Add support for two #clock-cells to pass rate rounding mode

[Peng Fan <peng.fan@oss.nxp.com>]

On Wed, Apr 22, 2026 at 07:51:03PM +0100, Sudeep Holla wrote:
>On Wed, Apr 22, 2026 at 10:00:23PM +0800, Peng Fan wrote:
>> Hi Sudeep,
>> 
>> Thanks for giving a look.
>> 
>> On Wed, Apr 22, 2026 at 02:14:56PM +0100, Sudeep Holla wrote:
>> >On Fri, Mar 06, 2026 at 02:20:13PM +0800, Peng Fan (OSS) wrote:
>> >> From: Peng Fan <peng.fan@nxp.com>
>> >> 
>> >> SCMI CLOCK_RATE_SET allows the caller to specify the rounding behaviour
>> >> when setting a clock rate. The previously added dt-bindings header
>> >> defines three modes:
>> >> 
>> >>   ROUND_DOWN / ROUND_UP / ROUND_AUTO
>> >> 
>> >> To enable device tree clients to select a rounding mode, extend the
>> >> SCMI clock provider to support "#clock-cells = <2>", where the second
>> >> cell encodes the desired rounding mode. The default remains
>> >> ROUND_DOWN for backwards compatibility with existing device trees.
>> >> 
>> >
>> >Where is the binding update documented ? It's not in 1/2.
>> 
>> This was missed in this patchset, I will fix in new version, if this
>> patchset does not have big design flaw.
>> 
>> >
>> >Also if it can be static in the device tree, why can't it be
>> >autonomously handled in the platform firmware ? I think I know the
>> 
>> Linux passes ROUND_DOWN, SCMI firmware uses round down for clk calculation.
>> 
>> >answer for this but I want to make sure it is a valid use-case and
>> >gets documented here as part of binding updates.
>> 
>> Per info from our video software team.
>> We have some video modes where the best pixel clock rate is slightly above the
>> nominal rate, and the default round down rule (CLOCK_ROUND_RULE_CEILING in SM
>> firmware) can cause the resulting clock rate to be much lower than expected.
>> 
>> disp1pix = 96200000 Hz (desired pixel clock rate)
>> 
>> The MIPI DPHY cannot hit the exact frequency of 288600000 Hz needed for this
>> pixel clock rate, so the next best DPHY PLL frequency is 289000000 Hz. This
>> corresponds to a pixel clock frequency of 96333333 Hz, which is slightly higher
>> than the nominal rate of 96200000 Hz the video mode specifies.
>> 
>> Setting the VIDEOPLL (disp1pix parent) to 289000000 Hz should divide down to
>> the adjusted disp1pix frequency of 96333333 Hz, but here is what happens in the
>> SM firmware:
>> 
>> quotient = 289000000 / (96200000 + 1) = 3.004 => 3 (notice that the SM always
>> receives the nominal clock rate, not the adjusted rate)
>> 
>> If the rounding rule is round down (CLOCK_ROUND_RULE_CEILING),
>> quotient = quotient + 1. Therefore, quotient becomes 4.
>> 
>> disp1pix = 289000000 / 4 = 72250000, which is nowhere close to the target of
>> 96333333.
>> 
>
>I do not think this is the correct interpretation of `CLOCK_ROUND_DOWN/UP`.
>
>`CLOCK_ROUND_DOWN/UP` should apply to the requested `disp1pix` rate itself,
>not to the divider choice in a way that forces selection of the next integer
>divisor and produces a much lower output clock.
>
>Here, the requested `disp1pix` is `96,200,000 Hz`, and the parent rate is
>`289,000,000 Hz`. The achievable child rates nearby are:
>
>`289,000,000 / 3 = 96,333,333 Hz`
>`289,000,000 / 4 = 72,250,000 Hz`
>
>Given those options, the firmware should be able to round the request
>autonomously to the nearest supported `disp1pix` rate, which is `96,333,333
>Hz` (`289,000,000 / 3`).
>
>Under that interpretation:
>
>`CLOCK_ROUND_UP` would permit choosing `96,333,333`
>`CLOCK_ROUND_AUTO` would also likely choose `96,333,333`
>Choosing `/4` and ending up at `72,250,000` does not look like a meaningful
>rounding of `96,200,000`
>
>So the issue appears to be that the firmware is applying the rounding rule to
>divider selection rather than to the resulting `disp1pix` frequency.

User requests 96.2 MHz with ROUND_DOWN semantics, expecting the closest
achievable frequency that does not meaningfully deviate from the request.

Firmware evaluates the parent rate of 289,000,000 Hz and computes:

    289,000,000 / 3 = 96,333,333 Hz

Since this resulting frequency is slightly higher than the requested
96.2 MHz, the firmware, applying a strict `output <= requested` rule,
rejects divider 3 and selects divider 4 instead, producing:

    289,000,000 / 4 = 72,250,000 Hz

This leads to an output frequency that is much farther from the requested
value.

My question is: if the firmware were to select divider 3 and produce
96,333,333 Hz (only ~0.13% higher than the request), would that be
considered a violation of ROUND_DOWN semantics, or is ROUND_DOWN intended
to select the closest achievable output frequency rather than enforcing
a strict inequality against the requested rate?

Thanks,
Peng

>
>> However, if we can use `ROUND_AUTO` the SM firmware would select a quotient of 3
>> in this case, and `disp1pix` would match our target: `289000000 / 3 = 96333333`.
>
>Given the explanation above, I would not support this approach. `ROUND_AUTO`
>should be sufficient for this case if the firmware is making a sensible
>selection.
>
>-- 
>Regards,
>Sudeep
>