| Message ID | 20231024-b4-qcom-clk-v1-8-9d96359b9a82@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm: mach-snapdragon: Qualcomm clock driver cleanup | expand |
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@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) 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;
On Wed, 25 Oct 2023 at 19:14, Caleb Connolly <caleb.connolly@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@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)
On 27/10/2023 13:18, Sumit Garg wrote: > On Wed, 25 Oct 2023 at 19:14, Caleb Connolly <caleb.connolly@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@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)
diff --git a/drivers/clk/qcom/clock-apq8016.c b/drivers/clk/qcom/clock-apq8016.c index 5eba18739cfb..a1481cd5177b 100644 --- a/drivers/clk/qcom/clock-apq8016.c +++ b/drivers/clk/qcom/clock-apq8016.c @@ -53,7 +53,7 @@ static struct vote_clk gcc_blsp1_ahb_clk = { /* SDHCI */ static int clk_init_sdc(struct qcom_cc_priv *priv, int slot, uint rate) { - int div = 8; /* 100MHz default */ + int div = 15; /* 100MHz default */ if (rate == 200000000) div = 4; diff --git a/drivers/clk/qcom/clock-apq8096.c b/drivers/clk/qcom/clock-apq8096.c index 48cac08eed67..ef81cd16223c 100644 --- a/drivers/clk/qcom/clock-apq8096.c +++ b/drivers/clk/qcom/clock-apq8096.c @@ -44,7 +44,7 @@ static struct vote_clk gcc_blsp2_ahb_clk = { static int clk_init_sdc(struct qcom_cc_priv *priv, uint rate) { - int div = 3; + int div = 5; clk_enable_cbc(priv->base + SDCC2_AHB_CBCR); clk_rcg_set_rate_mnd(priv->base, &sdc_regs, div, 0, 0, diff --git a/drivers/clk/qcom/clock-qcom.c b/drivers/clk/qcom/clock-qcom.c index 7a6157bf123f..a83c74cd20ba 100644 --- a/drivers/clk/qcom/clock-qcom.c +++ b/drivers/clk/qcom/clock-qcom.c @@ -68,20 +68,46 @@ void clk_enable_vote_clk(phys_addr_t base, const struct vote_clk *vclk) /* Update clock command via CMD_RCGR */ void clk_bcr_update(phys_addr_t apps_cmd_rcgr) { + u32 count; setbits_le32(apps_cmd_rcgr, APPS_CMD_RCGR_UPDATE); /* Wait for frequency to be updated. */ - while (readl(apps_cmd_rcgr) & APPS_CMD_RCGR_UPDATE) - ; + for (count = 0; count < 50000; count++) { + if (!(readl(apps_cmd_rcgr) & APPS_CMD_RCGR_UPDATE)) + break; + udelay(1); + } + WARN(count == 50000, "WARNING: RCG @ %#llx [%#010x] stuck at off\n", + apps_cmd_rcgr, readl(apps_cmd_rcgr)); +} + +const struct freq_tbl *qcom_find_freq(const struct freq_tbl *f, uint rate) +{ + if (!f) + return NULL; + + if (!f->freq) + return f; + + for (; f->freq; f++) + if (rate <= f->freq) + return f; + + /* Default to our fastest rate */ + return f - 1; } #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)) #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; diff --git a/drivers/clk/qcom/clock-qcom.h b/drivers/clk/qcom/clock-qcom.h index 6fa88fb40af8..f91e9d47dd22 100644 --- a/drivers/clk/qcom/clock-qcom.h +++ b/drivers/clk/qcom/clock-qcom.h @@ -30,6 +30,16 @@ struct bcr_regs { uintptr_t D; }; +struct freq_tbl { + uint freq; + uint src; + u8 pre_div; + u16 m; + u16 n; +}; + +#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) } + struct gate_clk { uintptr_t reg; u32 en_val; @@ -69,6 +79,7 @@ void clk_enable_gpll0(phys_addr_t base, const struct pll_vote_clk *gpll0); void clk_bcr_update(phys_addr_t apps_cmd_rgcr); void clk_enable_cbc(phys_addr_t cbcr); void clk_enable_vote_clk(phys_addr_t base, const struct vote_clk *vclk); +const struct freq_tbl *qcom_find_freq(const struct freq_tbl *f, uint rate); 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); void clk_rcg_set_rate(phys_addr_t base, const struct bcr_regs *regs, int div, diff --git a/drivers/clk/qcom/clock-qcs404.c b/drivers/clk/qcom/clock-qcs404.c index d10992ee58bf..b30d5c388d81 100644 --- a/drivers/clk/qcom/clock-qcs404.c +++ b/drivers/clk/qcom/clock-qcs404.c @@ -129,7 +129,7 @@ static ulong qcs404_set_rate(struct clk *clk, ulong rate) break; case GCC_SDCC1_APPS_CLK: /* SDCC1: 200MHz */ - clk_rcg_set_rate_mnd(priv->base, &sdc_regs, 4, 0, 0, + clk_rcg_set_rate_mnd(priv->base, &sdc_regs, 7, 0, 0, CFG_CLK_SRC_GPLL0, 8); clk_enable_gpll0(priv->base, &gpll0_vote_clk); clk_enable_cbc(priv->base + SDCC_APPS_CBCR(1)); @@ -139,16 +139,16 @@ static ulong qcs404_set_rate(struct clk *clk, ulong rate) break; case GCC_ETH_RGMII_CLK: if (rate == 250000000) - clk_rcg_set_rate_mnd(priv->base, &emac_regs, 2, 0, 0, + clk_rcg_set_rate_mnd(priv->base, &emac_regs, 3, 0, 0, CFG_CLK_SRC_GPLL1, 8); else if (rate == 125000000) - clk_rcg_set_rate_mnd(priv->base, &emac_regs, 4, 0, 0, + clk_rcg_set_rate_mnd(priv->base, &emac_regs, 7, 0, 0, CFG_CLK_SRC_GPLL1, 8); else if (rate == 50000000) - clk_rcg_set_rate_mnd(priv->base, &emac_regs, 10, 0, 0, + clk_rcg_set_rate_mnd(priv->base, &emac_regs, 19, 0, 0, CFG_CLK_SRC_GPLL1, 8); else if (rate == 5000000) - clk_rcg_set_rate_mnd(priv->base, &emac_regs, 2, 1, 50, + clk_rcg_set_rate_mnd(priv->base, &emac_regs, 3, 1, 50, CFG_CLK_SRC_GPLL1, 8); break; default: @@ -165,7 +165,7 @@ static int qcs404_enable(struct clk *clk) switch (clk->id) { case GCC_USB30_MASTER_CLK: clk_enable_cbc(priv->base + USB30_MASTER_CBCR); - clk_rcg_set_rate_mnd(priv->base, &usb30_master_regs, 4, 0, 0, + clk_rcg_set_rate_mnd(priv->base, &usb30_master_regs, 7, 0, 0, CFG_CLK_SRC_GPLL0, 8); break; case GCC_SYS_NOC_USB3_CLK: @@ -187,14 +187,14 @@ static int qcs404_enable(struct clk *clk) /* SPEED_1000: freq -> 250MHz */ clk_enable_cbc(priv->base + ETH_PTP_CBCR); clk_enable_gpll0(priv->base, &gpll1_vote_clk); - clk_rcg_set_rate_mnd(priv->base, &emac_ptp_regs, 2, 0, 0, + clk_rcg_set_rate_mnd(priv->base, &emac_ptp_regs, 3, 0, 0, CFG_CLK_SRC_GPLL1, 8); break; case GCC_ETH_RGMII_CLK: /* SPEED_1000: freq -> 250MHz */ clk_enable_cbc(priv->base + ETH_RGMII_CBCR); clk_enable_gpll0(priv->base, &gpll1_vote_clk); - clk_rcg_set_rate_mnd(priv->base, &emac_regs, 2, 0, 0, + clk_rcg_set_rate_mnd(priv->base, &emac_regs, 3, 0, 0, CFG_CLK_SRC_GPLL1, 8); break; case GCC_ETH_SLAVE_AHB_CLK: diff --git a/drivers/clk/qcom/clock-sdm845.c b/drivers/clk/qcom/clock-sdm845.c index 9345d5293f1e..efe8495b7fb0 100644 --- a/drivers/clk/qcom/clock-sdm845.c +++ b/drivers/clk/qcom/clock-sdm845.c @@ -20,16 +20,6 @@ #include "clock-qcom.h" -#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) } - -struct freq_tbl { - uint freq; - uint src; - u8 pre_div; - u16 m; - u16 n; -}; - static const struct freq_tbl ftbl_gcc_qupv3_wrap0_s0_clk_src[] = { F(7372800, CFG_CLK_SRC_GPLL0_EVEN, 1, 384, 15625), F(14745600, CFG_CLK_SRC_GPLL0_EVEN, 1, 768, 15625), @@ -57,22 +47,6 @@ static const struct bcr_regs uart2_regs = { .D = SE9_UART_APPS_D, }; -const struct freq_tbl *qcom_find_freq(const struct freq_tbl *f, uint rate) -{ - if (!f) - return NULL; - - if (!f->freq) - return f; - - for (; f->freq; f++) - if (rate <= f->freq) - return f; - - /* Default to our fastest rate */ - return f - 1; -} - static ulong sdm845_set_rate(struct clk *clk, ulong rate) { struct qcom_cc_priv *priv = dev_get_priv(clk->dev);
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@linaro.org> --- drivers/clk/qcom/clock-apq8016.c | 2 +- drivers/clk/qcom/clock-apq8096.c | 2 +- drivers/clk/qcom/clock-qcom.c | 40 ++++++++++++++++++++++++++++++++-------- drivers/clk/qcom/clock-qcom.h | 11 +++++++++++ drivers/clk/qcom/clock-qcs404.c | 16 ++++++++-------- drivers/clk/qcom/clock-sdm845.c | 26 -------------------------- 6 files changed, 53 insertions(+), 44 deletions(-)