Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1214429 > unrolled thread
| Started by | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| First post | 2015-08-27 11:40 +0200 |
| Last post | 2015-08-28 02:00 +0200 |
| Articles | 3 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] clk: at91: add audio pll clock driver Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-08-27 11:40 +0200
Re: [PATCH] clk: at91: add audio pll clock driver Stephen Boyd <sboyd@codeaurora.org> - 2015-08-27 21:00 +0200
Re: [PATCH] clk: at91: add audio pll clock driver Michael Turquette <mturquette@baylibre.com> - 2015-08-28 02:00 +0200
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2015-08-27 11:40 +0200 |
| Subject | Re: [PATCH] clk: at91: add audio pll clock driver |
| Message-ID | <q21P3-v3-7@gated-at.bofh.it> |
Hi Nicolas,
On Fri, 31 Jul 2015 12:17:44 +0200
Nicolas Ferre <nicolas.ferre@atmel.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.
>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2015-08-27 21:00 +0200 |
| Message-ID | <q2az0-4zD-19@gated-at.bofh.it> |
| In reply to | #1214429 |
On 08/27, Boris Brezillon wrote: > Nicolas Ferre <nicolas.ferre@atmel.com> wrote: > > + > > +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? I'm not opposed to people using ->round_rate() if they want to use it and it serves their purpose. Moving everyone to ->determine_rate() will be a long journey that has little to no benefit for most drivers, so it's not like we need to force everyone to use the determine rate op for new submissions so that we can delete the round rate op one day. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Michael Turquette <mturquette@baylibre.com> |
|---|---|
| Date | 2015-08-28 02:00 +0200 |
| Message-ID | <q2ffk-2SO-11@gated-at.bofh.it> |
| In reply to | #1214429 |
Quoting Boris Brezillon (2015-08-27 02:30:35)
> Hi Nicolas,
>
> On Fri, 31 Jul 2015 12:17:44 +0200
> Nicolas Ferre <nicolas.ferre@atmel.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@atmel.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@vger.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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web