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

[Sumit Garg]

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.

-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)