On Wed, 25 Oct 2023 at 19:14, Caleb Connolly <caleb.conno...@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.conno...@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)