Re: [PATCH] clk: qcom: rcg2: compute 2d using duty fraction directly

[Taniya Das <taniya.das@oss.qualcomm.com>]

On 12/25/2025 9:21 AM, Bjorn Andersson wrote:
> On Tue, Dec 23, 2025 at 04:18:20PM +0530, Taniya Das wrote:
>>
>>
>> On 12/23/2025 12:39 AM, Bjorn Andersson wrote:
>>> On Mon, Dec 22, 2025 at 10:38:14PM +0530, Taniya Das wrote:
>>>> From: Taniya Das <quic_tdas@quicinc.com>
>>>
>>> Please use oss.qualcomm.com.
>>>
>>
>> My bad, will update it.
>>
>>>>
>>>> The duty-cycle calculation in clk_rcg2_set_duty_cycle() currently
>>>> derives an intermediate percentage `duty_per = (num * 100) / den` and
>>>> then computes:
>>>>
>>>>     d = DIV_ROUND_CLOSEST(n * duty_per * 2, 100);
>>>>
>>>> This introduces integer truncation at the percentage step (division by
>>>> `den`) and a redundant scaling by 100, which can reduce precision for
>>>> large `den` and skew the final rounding.
>>>>
>>>> Compute `2d` directly from the duty fraction to preserve precision and
>>>> avoid the unnecessary scaling:
>>>>
>>>>     d = DIV_ROUND_CLOSEST(n * duty->num * 2, duty->den);
>>>>
>>>> This keeps the intended formula `d ≈ n * 2 * (num/den)` while performing
>>>> a single, final rounded division, improving accuracy especially for small
>>>> duty cycles or large denominators. It also removes the unused `duty_per`
>>>> variable, simplifying the code.
>>>>
>>>> There is no functional changes beyond improved numerical accuracy.
>>>>
>>>> Fixes: 7f891faf596ed ("clk: qcom: clk-rcg2: Add support for duty-cycle for RCG")
>>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>>> ---
>>>> Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>
>>>> ---
>>>>  drivers/clk/qcom/clk-rcg2.c | 6 ++----
>>>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
>>>> index e18cb8807d73534c6437c08aeb524353a2eab06f..2838d4cb2d58ea1e351d6a5599045c72f4dc3801 100644
>>>> --- a/drivers/clk/qcom/clk-rcg2.c
>>>> +++ b/drivers/clk/qcom/clk-rcg2.c
>>>> @@ -755,7 +755,7 @@ static int clk_rcg2_get_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
>>>>  static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
>>>>  {
>>>>  	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
>>>> -	u32 notn_m, n, m, d, not2d, mask, duty_per, cfg;
>>>> +	u32 notn_m, n, m, d, not2d, mask, cfg;
>>>>  	int ret;
>>>>  
>>>>  	/* Duty-cycle cannot be modified for non-MND RCGs */
>>>> @@ -774,10 +774,8 @@ static int clk_rcg2_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
>>>>  
>>>>  	n = (~(notn_m) + m) & mask;
>>>>  
>>>> -	duty_per = (duty->num * 100) / duty->den;
>>>> -
>>>>  	/* Calculate 2d value */
>>>> -	d = DIV_ROUND_CLOSEST(n * duty_per * 2, 100);
>>>> +	d = DIV_ROUND_CLOSEST(n * duty->num * 2, duty->den);
>>>
>>> This looks better/cleaner. But for my understanding, can you share some
>>> example numbers that shows the problem?
>>>
>>
>> Sure Bjorn, will share the examples.
>>
> 
> I don't think these examples need to necessarily be added in the git
> history - in particular since the proposed new style looks more
> reasonable than what's currently is in the code.
> 
> So, providing them here would suffice, for me at least.

Frequency requirement from customers as below.

F(10000, P_BI_TCXO, 2, 1, 960),

For example, with N = 960.

Duty cycle(%)| num/den | d_old |NOT_2D(old)| d_new |NOT_2D(new)|Match
--------------------------------------------------------------------
0.05         | 1/2000  | 0     |0x0000FFFF |  1    |0x0000FFFE |No
0.10         | 1/1000  | 0     |0x0000FFFF |  2    |0x0000FFFD |No
0.3125       | 1/320   | 0     |0x0000FFFF |  6    |0x0000FFF9 |No
0.50         | 1/200   | 0     |0x0000FFFF |  10   |0x0000FFF5 |No
0.78125      | 1/128   | 0     |0x0000FFFF |  15   |0x0000FFF0 |No
2.00         | 1/50    | 38    |0x0000FFD9 |  38   |0x0000FFD9 |Yes
2.10         | 7/333   | 38    |0x0000FFD9 |  40   |0x0000FFD7 |No
2.50         | 1/40    | 38    |0x0000FFD9 |  48   |0x0000FFCF |No
3.00         | 3/100   | 58    |0x0000FFC5 |  58   |0x0000FFC5 |Yes



> 
> 
> Adding kunit tests certainly sounds useful though.
> 

Sure, will take a look.


-- 
Thanks,
Taniya Das