| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 27 Aug 2015 16:58:49 -0700
From: Michael Turquette <mturquette@...libre.com>
To: Boris Brezillon <boris.brezillon@...e-electrons.com>,
"Nicolas Ferre" <nicolas.ferre@...el.com>
Cc: "Alexandre Belloni" <alexandre.belloni@...e-electrons.com>,
"Ludovic Desroches" <ludovic.desroches@...el.com>,
"Josh Wu" <josh.wu@...el.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
"Stephen Boyd" <sboyd@...eaurora.org>
Subject: Re: [PATCH] clk: at91: add audio pll clock driver
Quoting Boris Brezillon (2015-08-27 02:30:35)
> Hi Nicolas,
>
> On Fri, 31 Jul 2015 12:17:44 +0200
> Nicolas Ferre <nicolas.ferre@...el.com> wrote:
>
> > This new clock driver set allows to have a fractional divided clock
> > that would generate a precise clock particularly suitable for
> > audio applications.
> >
> > The main audio pll clock has two children clocks: one that is connected
> > to the PMC, the other that can directly drive a pad. As these two routes
> > have different enable bits and different dividers and divider formula,
> > they are handled by two different drivers.
> > Each of them would modify the rate of the main audio pll parent.
>
> Hm, not sure allowing both children to modify the parent clk rate is
> a good idea, but let's see how it works.
Might be a good idea to use clk_set_rate_range? Of course most audio use
cases need exact rates...
Regards,
Mike
>
> >
> > Signed-off-by: Nicolas Ferre <nicolas.ferre@...el.com>
> > ---
> > .../devicetree/bindings/clock/at91-clock.txt | 38 +++
> > drivers/clk/at91/Makefile | 2 +
> > drivers/clk/at91/clk-audio-pll-pad.c | 220 +++++++++++++++++
> > drivers/clk/at91/clk-audio-pll-pmc.c | 171 ++++++++++++++
> > drivers/clk/at91/clk-audio-pll.c | 261 +++++++++++++++++++++
> > drivers/clk/at91/pmc.c | 14 ++
> > drivers/clk/at91/pmc.h | 7 +
> > include/linux/clk/at91_pmc.h | 25 ++
> > 8 files changed, 738 insertions(+)
> > create mode 100644 drivers/clk/at91/clk-audio-pll-pad.c
> > create mode 100644 drivers/clk/at91/clk-audio-pll-pmc.c
> > create mode 100644 drivers/clk/at91/clk-audio-pll.c
> >
>
> [...]
>
> > +
> > +static int clk_audio_pll_compute_qdpad(unsigned long q_rate, unsigned long rate,
> > + unsigned long *qd, u8 *div, u8 *ext_div)
> > +{
> > + unsigned long tmp_qd;
> > + unsigned long rem2, rem3;
> > + unsigned long ldiv, lext_div;
> > +
> > + if (!rate)
> > + return -EINVAL;
> > +
> > + tmp_qd = q_rate / rate;
> > + if (!tmp_qd || tmp_qd > AUDIO_PLL_QDPAD_MAX)
> > + return -EINVAL;
>
> Do you really want to return an error in this case?
> I mean, you're calling this function from ->round_rate(), and
> ->round_rate() is supposed to find the closest rate, not to return an
> error if the rate is too low or to high.
> ITOH, you're calling the same function from ->set_rate(), which is
> supposed to complain if the requested rate is not exactly met.
>
> Maybe you can deal with that by passing an additional argument to this
> function. Something like:
>
> tmp_qd = q_rate / rate;
>
> if (!strict) {
> if (!tmp_qd)
> tmp_qd = 1;
> else if (tmp_qd > AUDIO_PLL_QDPAD_MAX)
> tmp_qd = AUDIO_PLL_QDPAD_MAX;
> }
>
> if (!tmp_qd || tmp_qd > AUDIO_PLL_QDPAD_MAX)
> return -EINVAL;
>
> > +
> > + if (tmp_qd <= AT91_PMC_AUDIO_PLL_QDPAD_EXTDIV_MAX) {
> > + ldiv = 1;
> > + lext_div = tmp_qd;
> > + } else {
> > + rem2 = tmp_qd % 2;
> > + rem3 = tmp_qd % 3;
> > +
> > + if (rem3 == 0 ||
> > + tmp_qd > AT91_PMC_AUDIO_PLL_QDPAD_EXTDIV_MAX * 2 ||
> > + rem3 < rem2) {
> > + ldiv = 3;
> > + lext_div = tmp_qd / 3;
> > + } else {
> > + ldiv = 2;
> > + lext_div = tmp_qd >> 1;
> > + }
> > + }
> > +
> > + pr_debug("A PLL/PAD: %s, qd = %lu (div = %lu, ext_div = %lu)\n",
> > + __func__, ldiv * lext_div, ldiv, lext_div);
> > +
> > + /* if we were given variable to store, we can provide them */
> > + if (qd)
> > + *qd = ldiv * lext_div;
> > + if (div && ext_div) {
> > + /* we can cast here as we verified the bounds just above */
> > + *div = (u8)ldiv;
> > + *ext_div = (u8)lext_div;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static long clk_audio_pll_pad_round_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long *parent_rate)
>
> I thought we were trying to get rid of the ->round_rate() function in
> favor of the ->determine_rate() one (which is more flexible), but maybe
> I'm wrong. Stephen, Mike, what's your opinion?
>
> > +{
> > + struct clk *pclk = __clk_get_parent(hw->clk);
> > + long best_rate = -EINVAL;
> > + unsigned long best_parent_rate = 0;
> > + unsigned long tmp_qd;
> > +
> > + pr_debug("A PLL/PAD: %s, rate = %lu (parent_rate = %lu)\n",
> > + __func__, rate, *parent_rate);
> > +
> > + if (clk_audio_pll_compute_qdpad(AUDIO_PLL_REFERENCE_FOUT, rate,
> > + &tmp_qd, NULL, NULL))
> > + return -EINVAL;
>
> You're calculating your divisor based on the AUDIO_PLL_REFERENCE_FOUT
> value...
>
> > +
> > + best_parent_rate = __clk_round_rate(pclk, rate * tmp_qd);
>
> ... and then asking your parent to adapt its rate based on the returned
> divisor. Wouldn't it be preferable to search for the best parent rate
> when choosing the divisor?
>
> > + best_rate = best_parent_rate / tmp_qd;
> > +
> > + pr_debug("A PLL/PAD: %s, best_rate = %ld, best_parent_rate = %lu\n",
> > + __func__, best_rate, best_parent_rate);
> > +
> > + *parent_rate = best_parent_rate;
> > + return best_rate;
> > +}
>
> [...]
>
> > +
> > +static long clk_audio_pll_pmc_round_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long *parent_rate)
> > +{
> > + struct clk *pclk = __clk_get_parent(hw->clk);
> > + long best_rate = -EINVAL;
> > + unsigned long best_parent_rate = 0;
> > + unsigned long tmp_qd;
> > +
> > + pr_debug("A PLL/PMC: %s, rate = %lu (parent_rate = %lu)\n",
> > + __func__, rate, *parent_rate);
> > +
> > + if (clk_audio_pll_compute_qdpmc(AUDIO_PLL_REFERENCE_FOUT, rate, &tmp_qd))
> > + return -EINVAL;
> > +
> > + best_parent_rate = __clk_round_rate(pclk, rate * tmp_qd);
>
> Ditto.
>
> BTW, I'm not sure this is safe to allow both pll-audio children to
> change their parent rate. What will happen if one of them change the
> pll-audio rate while the other is still using it?
>
> > + best_rate = best_parent_rate / tmp_qd;
> > +
> > + pr_debug("A PLL/PMC: %s, best_rate = %ld, best_parent_rate = %lu (qd = %lu)\n",
> > + __func__, best_rate, best_parent_rate, tmp_qd - 1);
> > +
> > + *parent_rate = best_parent_rate;
> > + return best_rate;
> > +}
>
> [...]
>
> > +
> > +#define to_clk_audio_frac(hw) container_of(hw, struct clk_audio_frac, hw)
> > +
> > +/* make sure that pll is in reset state beforehand */
> > +static int clk_audio_pll_prepare(struct clk_hw *hw)
> > +{
> > + struct clk_audio_frac *fck = to_clk_audio_frac(hw);
> > + struct at91_pmc *pmc = fck->pmc;
> > + u32 tmp;
> > +
> > + pmc_lock(pmc);
> > + tmp = pmc_read(pmc, AT91_PMC_AUDIO_PLL0) & ~AT91_PMC_AUDIO_PLL_RESETN;
> > + pmc_write(pmc, AT91_PMC_AUDIO_PLL0, tmp);
> > + pmc_unlock(pmc);
>
> Nothing sleeps in this code, so you could move everything in your
> ->enable() function.
>
> > + return 0;
> > +}
> > +
> > +static void clk_audio_pll_unprepare(struct clk_hw *hw)
> > +{
> > + clk_audio_pll_prepare(hw);
>
> Ditto.
>
> Best Regards,
>
> Boris
>
> --
> Boris Brezillon, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists