[PATCH 8/8] clk/qcom: fix rcg divider value

[Caleb Connolly]

On 27/10/2023 13:18, Sumit Garg wrote:
> On Wed, 25 Oct 2023 at 19:14, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>>
>>
>>
>> On 24/10/2023 21:23, Caleb Connolly wrote:
>>> The RCG divider field takes a value of (2*h - 1) where h is the divisor.
>>> This allows fractional dividers to be supported by calculating them at
>>> compile time using a macro.
>>>
>>> However, the clk_rcg_set_rate_mnd() function was also performing the
>>> calculation. Clean this all up and consistently use the F() macro to
>>> calculate these at compile time and properly support fractional divisors.
>>>
>>> Additionally, improve clk_bcr_update() to timeout with a warning rather
>>> than hanging the board, and make the freq_tbl struct and helpers common
>>> so that they can be reused by future platforms.
>>>
>>> Signed-off-by: Caleb Connolly <caleb.connolly at linaro.org>
>>
>> [...]
>>>
>>>  #define CFG_MODE_DUAL_EDGE (0x2 << 12) /* Counter mode */
>>>
>>> -#define CFG_MASK 0x3FFF
>>> +// Disable the HW_CLK_CONTROL bit
>>> +#define CFG_MASK (0x3FFF | (1 << 20))
>>
>> There seems to be a bug in this patch that causes the actual clock rate
>> to be wrong in some cases, most obvious with db845c UART. I had
>> initially thought it to be a board issue but upon further investigation
>> I think I'm wrong.
>>
>> The UART clock frequency table in clock-sdm845.c is taken from Linux,
>> the 115200 baud rate corresponds to the lowest frequency (7372800Hz).
>> However, with the implementation changes here, the RCG configuration
>> actually results in a measured clock rate of 9085208Hz.
>>
>> If the entry is changed as follows
>> -       F(7372800, CFG_CLK_SRC_GPLL0_EVEN, 1, 384, 15625)
>> +       F(7372800, CFG_CLK_SRC_GPLL0, 0.5, 192, 15625)
>>
> 
> I would suggest we keep the frequency table aligned to the Linux tree.
> We should try to fix the mismatch in the computation to support
> fractional divisors.

Please disregard, the original entry F(7372800, CFG_CLK_SRC_GPLL0_EVEN,
1, 384, 15625) is what Linux has. It turns out that setbits_le32()
doesn't use iowmb(), and as a result the writes weren't propagating
through to hardware correctly, resulting in the frequency being wrong.
This will be fixed in v2, and use the frequency table from Linux.
> 
> -Sumit
> 
>> Then the resultant clock rate is 7372604Hz (within the tolerance)...
>>
>> I will resolve this for v2.
>>>
>>>  #define CFG_DIVIDER_MASK 0x1F
>>>
>>> -/* root set rate for clocks with half integer and MND divider */
>>> +/*
>>> + * root set rate for clocks with half integer and MND divider
>>> + * div should be pre-calculated ((div * 2) - 1)
>>> + */
>>>  void clk_rcg_set_rate_mnd(phys_addr_t base, const struct bcr_regs *regs,
>>>                         int div, int m, int n, int source, u8 mnd_width)
>>>  {
>>> @@ -99,17 +125,15 @@ void clk_rcg_set_rate_mnd(phys_addr_t base, const struct bcr_regs *regs,
>>>       /* Program MND values */
>>>       setbits_le32(base + regs->M, m_val & mask);
>>>       setbits_le32(base + regs->N, n_val & mask);
>>> -     setbits_le32(base + regs->D, d_val & mask);
>>> +     setbits_le32(base + regs->D, (d_val & mask) == mask ? 0 : (d_val & mask));
>>>
>>>       /* setup src select and divider */
>>>       cfg  = readl(base + regs->cfg_rcgr);
>>>       cfg &= ~CFG_MASK;
>>>       cfg |= source & CFG_CLK_SRC_MASK; /* Select clock source */
>>>
>>> -     /* Set the divider; HW permits fraction dividers (+0.5), but
>>> -        for simplicity, we will support integers only */
>>>       if (div)
>>> -             cfg |= (2 * div - 1) & CFG_DIVIDER_MASK;
>>> +             cfg |= div & CFG_DIVIDER_MASK;
>>>
>>>       if (n_val)
>>>               cfg |= CFG_MODE_DUAL_EDGE;
>>
>> --
>> // Caleb (they/them)

-- 
// Caleb (they/them)