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

[Sudeep Holla <sudeep.holla@kernel.org>]

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.

> 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