Add support for defining PLL clocks with od_bits = 0, meaning that
OD is fixed to 1 and there is no OD field in the register. In this
case od_max must also be 0, which is enforced with BUG_ON().
Signed-off-by: Aidan MacDonald <[email protected]>
---
v1 -> v2: Simplify od lookup in ingenic_pll_recalc_rate() and
enforce od_max == 0 when od_bits is zero.
drivers/clk/ingenic/cgu.c | 21 +++++++++++++++------
drivers/clk/ingenic/cgu.h | 3 ++-
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
index 861c50d6cb24..3481129114b1 100644
--- a/drivers/clk/ingenic/cgu.c
+++ b/drivers/clk/ingenic/cgu.c
@@ -83,7 +83,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
const struct ingenic_cgu_clk_info *clk_info = to_clk_info(ingenic_clk);
struct ingenic_cgu *cgu = ingenic_clk->cgu;
const struct ingenic_cgu_pll_info *pll_info;
- unsigned m, n, od_enc, od;
+ unsigned m, n, od, od_enc = 0;
bool bypass;
u32 ctl;
@@ -96,8 +96,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
m += pll_info->m_offset;
n = (ctl >> pll_info->n_shift) & GENMASK(pll_info->n_bits - 1, 0);
n += pll_info->n_offset;
- od_enc = ctl >> pll_info->od_shift;
- od_enc &= GENMASK(pll_info->od_bits - 1, 0);
+
+ if (pll_info->od_bits > 0) {
+ od_enc = ctl >> pll_info->od_shift;
+ od_enc &= GENMASK(pll_info->od_bits - 1, 0);
+ }
if (pll_info->bypass_bit >= 0) {
ctl = readl(cgu->base + pll_info->bypass_reg);
@@ -112,7 +115,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
if (pll_info->od_encoding[od] == od_enc)
break;
}
- BUG_ON(od == pll_info->od_max);
+ /* if od_max = 0, od_bits should be 0 and od is fixed to 1. */
+ if (pll_info->od_max == 0)
+ BUG_ON(pll_info->od_bits != 0);
+ else
+ BUG_ON(od == pll_info->od_max);
od++;
return div_u64((u64)parent_rate * m * pll_info->rate_multiplier,
@@ -215,8 +222,10 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned long req_rate,
ctl &= ~(GENMASK(pll_info->n_bits - 1, 0) << pll_info->n_shift);
ctl |= (n - pll_info->n_offset) << pll_info->n_shift;
- ctl &= ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
- ctl |= pll_info->od_encoding[od - 1] << pll_info->od_shift;
+ if (pll_info->od_bits > 0) {
+ ctl &= ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
+ ctl |= pll_info->od_encoding[od - 1] << pll_info->od_shift;
+ }
writel(ctl, cgu->base + pll_info->reg);
diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
index 147b7df0d657..567142b584bb 100644
--- a/drivers/clk/ingenic/cgu.h
+++ b/drivers/clk/ingenic/cgu.h
@@ -33,7 +33,8 @@
* @od_shift: the number of bits to shift the post-VCO divider value by (ie.
* the index of the lowest bit of the post-VCO divider value in
* the PLL's control register)
- * @od_bits: the size of the post-VCO divider field in bits
+ * @od_bits: the size of the post-VCO divider field in bits, or 0 if no
+ * OD field exists (then the OD is fixed to 1)
* @od_max: the maximum post-VCO divider value
* @od_encoding: a pointer to an array mapping post-VCO divider values to
* their encoded values in the PLL control register, or -1 for
--
2.38.1
Hi Aidan,
Le mer. 26 oct. 2022 ? 20:43:40 +0100, Aidan MacDonald
<[email protected]> a ?crit :
> Add support for defining PLL clocks with od_bits = 0, meaning that
> OD is fixed to 1 and there is no OD field in the register. In this
> case od_max must also be 0, which is enforced with BUG_ON().
>
> Signed-off-by: Aidan MacDonald <[email protected]>
> ---
> v1 -> v2: Simplify od lookup in ingenic_pll_recalc_rate() and
> enforce od_max == 0 when od_bits is zero.
>
> drivers/clk/ingenic/cgu.c | 21 +++++++++++++++------
> drivers/clk/ingenic/cgu.h | 3 ++-
> 2 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
> index 861c50d6cb24..3481129114b1 100644
> --- a/drivers/clk/ingenic/cgu.c
> +++ b/drivers/clk/ingenic/cgu.c
> @@ -83,7 +83,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned
> long parent_rate)
> const struct ingenic_cgu_clk_info *clk_info =
> to_clk_info(ingenic_clk);
> struct ingenic_cgu *cgu = ingenic_clk->cgu;
> const struct ingenic_cgu_pll_info *pll_info;
> - unsigned m, n, od_enc, od;
> + unsigned m, n, od, od_enc = 0;
> bool bypass;
> u32 ctl;
>
> @@ -96,8 +96,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw,
> unsigned long parent_rate)
> m += pll_info->m_offset;
> n = (ctl >> pll_info->n_shift) & GENMASK(pll_info->n_bits - 1, 0);
> n += pll_info->n_offset;
> - od_enc = ctl >> pll_info->od_shift;
> - od_enc &= GENMASK(pll_info->od_bits - 1, 0);
> +
> + if (pll_info->od_bits > 0) {
> + od_enc = ctl >> pll_info->od_shift;
> + od_enc &= GENMASK(pll_info->od_bits - 1, 0);
> + }
>
> if (pll_info->bypass_bit >= 0) {
> ctl = readl(cgu->base + pll_info->bypass_reg);
> @@ -112,7 +115,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw,
> unsigned long parent_rate)
> if (pll_info->od_encoding[od] == od_enc)
> break;
> }
I'd add a space there.
With that:
Reviewed-by: Paul Cercueil <[email protected]>
> - BUG_ON(od == pll_info->od_max);
> + /* if od_max = 0, od_bits should be 0 and od is fixed to 1. */
> + if (pll_info->od_max == 0)
> + BUG_ON(pll_info->od_bits != 0);
I don't think this first BUG_ON() is needed, if we do a good job
reviewing patches. But I don't care enough to ask you to remove it.
Cheers,
-Paul
> + else
> + BUG_ON(od == pll_info->od_max);
> od++;
>
> return div_u64((u64)parent_rate * m * pll_info->rate_multiplier,
> @@ -215,8 +222,10 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned
> long req_rate,
> ctl &= ~(GENMASK(pll_info->n_bits - 1, 0) << pll_info->n_shift);
> ctl |= (n - pll_info->n_offset) << pll_info->n_shift;
>
> - ctl &= ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
> - ctl |= pll_info->od_encoding[od - 1] << pll_info->od_shift;
> + if (pll_info->od_bits > 0) {
> + ctl &= ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
> + ctl |= pll_info->od_encoding[od - 1] << pll_info->od_shift;
> + }
>
> writel(ctl, cgu->base + pll_info->reg);
>
> diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
> index 147b7df0d657..567142b584bb 100644
> --- a/drivers/clk/ingenic/cgu.h
> +++ b/drivers/clk/ingenic/cgu.h
> @@ -33,7 +33,8 @@
> * @od_shift: the number of bits to shift the post-VCO divider value
> by (ie.
> * the index of the lowest bit of the post-VCO divider
> value in
> * the PLL's control register)
> - * @od_bits: the size of the post-VCO divider field in bits
> + * @od_bits: the size of the post-VCO divider field in bits, or 0 if
> no
> + * OD field exists (then the OD is fixed to 1)
> * @od_max: the maximum post-VCO divider value
> * @od_encoding: a pointer to an array mapping post-VCO divider
> values to
> * their encoded values in the PLL control register,
> or -1 for
> --
> 2.38.1
>
Quoting Aidan MacDonald (2022-10-26 12:43:40)
> Add support for defining PLL clocks with od_bits = 0, meaning that
> OD is fixed to 1 and there is no OD field in the register. In this
> case od_max must also be 0, which is enforced with BUG_ON().
>
> Signed-off-by: Aidan MacDonald <[email protected]>
> ---
Applied to clk-next
Paul Cercueil <[email protected]> writes:
> Hi Aidan,
>
> Le mer. 26 oct. 2022 à 20:43:40 +0100, Aidan MacDonald
> <[email protected]> a écrit :
>> Add support for defining PLL clocks with od_bits = 0, meaning that
>> OD is fixed to 1 and there is no OD field in the register. In this
>> case od_max must also be 0, which is enforced with BUG_ON().
>> Signed-off-by: Aidan MacDonald <[email protected]>
>> ---
>> v1 -> v2: Simplify od lookup in ingenic_pll_recalc_rate() and
>> enforce od_max == 0 when od_bits is zero.
>> drivers/clk/ingenic/cgu.c | 21 +++++++++++++++------
>> drivers/clk/ingenic/cgu.h | 3 ++-
>> 2 files changed, 17 insertions(+), 7 deletions(-)
>> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
>> index 861c50d6cb24..3481129114b1 100644
>> --- a/drivers/clk/ingenic/cgu.c
>> +++ b/drivers/clk/ingenic/cgu.c
>> @@ -83,7 +83,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long
>> parent_rate)
>> const struct ingenic_cgu_clk_info *clk_info = to_clk_info(ingenic_clk);
>> struct ingenic_cgu *cgu = ingenic_clk->cgu;
>> const struct ingenic_cgu_pll_info *pll_info;
>> - unsigned m, n, od_enc, od;
>> + unsigned m, n, od, od_enc = 0;
>> bool bypass;
>> u32 ctl;
>> @@ -96,8 +96,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long
>> parent_rate)
>> m += pll_info->m_offset;
>> n = (ctl >> pll_info->n_shift) & GENMASK(pll_info->n_bits - 1, 0);
>> n += pll_info->n_offset;
>> - od_enc = ctl >> pll_info->od_shift;
>> - od_enc &= GENMASK(pll_info->od_bits - 1, 0);
>> +
>> + if (pll_info->od_bits > 0) {
>> + od_enc = ctl >> pll_info->od_shift;
>> + od_enc &= GENMASK(pll_info->od_bits - 1, 0);
>> + }
>> if (pll_info->bypass_bit >= 0) {
>> ctl = readl(cgu->base + pll_info->bypass_reg);
>> @@ -112,7 +115,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long
>> parent_rate)
>> if (pll_info->od_encoding[od] == od_enc)
>> break;
>> }
>
> I'd add a space there.
>
> With that:
> Reviewed-by: Paul Cercueil <[email protected]>
>
Already done; the space is there in my outbox and on lore.kernel.org.
I think you might've accidentally removed it. Stephen's already
applied the series anyway, so...
>> - BUG_ON(od == pll_info->od_max);
>> + /* if od_max = 0, od_bits should be 0 and od is fixed to 1. */
>> + if (pll_info->od_max == 0)
>> + BUG_ON(pll_info->od_bits != 0);
>
> I don't think this first BUG_ON() is needed, if we do a good job reviewing
> patches. But I don't care enough to ask you to remove it.
>
> Cheers,
> -Paul
>
>> + else
>> + BUG_ON(od == pll_info->od_max);
>> od++;
>> return div_u64((u64)parent_rate * m * pll_info->rate_multiplier,
>> @@ -215,8 +222,10 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned long
>> req_rate,
>> ctl &= ~(GENMASK(pll_info->n_bits - 1, 0) << pll_info->n_shift);
>> ctl |= (n - pll_info->n_offset) << pll_info->n_shift;
>> - ctl &= ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
>> - ctl |= pll_info->od_encoding[od - 1] << pll_info->od_shift;
>> + if (pll_info->od_bits > 0) {
>> + ctl &= ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
>> + ctl |= pll_info->od_encoding[od - 1] << pll_info->od_shift;
>> + }
>> writel(ctl, cgu->base + pll_info->reg);
>> diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
>> index 147b7df0d657..567142b584bb 100644
>> --- a/drivers/clk/ingenic/cgu.h
>> +++ b/drivers/clk/ingenic/cgu.h
>> @@ -33,7 +33,8 @@
>> * @od_shift: the number of bits to shift the post-VCO divider value by (ie.
>> * the index of the lowest bit of the post-VCO divider value in
>> * the PLL's control register)
>> - * @od_bits: the size of the post-VCO divider field in bits
>> + * @od_bits: the size of the post-VCO divider field in bits, or 0 if no
>> + * OD field exists (then the OD is fixed to 1)
>> * @od_max: the maximum post-VCO divider value
>> * @od_encoding: a pointer to an array mapping post-VCO divider values to
>> * their encoded values in the PLL control register, or -1 for
>> --
>> 2.38.1
>>
Quoting Aidan MacDonald (2022-10-27 14:40:02)
>
> Paul Cercueil <[email protected]> writes:
>
> > Hi Aidan,
> >
> > Le mer. 26 oct. 2022 à 20:43:40 +0100, Aidan MacDonald
> > <[email protected]> a écrit :
> >> Add support for defining PLL clocks with od_bits = 0, meaning that
> >> OD is fixed to 1 and there is no OD field in the register. In this
> >> case od_max must also be 0, which is enforced with BUG_ON().
> >> Signed-off-by: Aidan MacDonald <[email protected]>
> >> ---
> >> v1 -> v2: Simplify od lookup in ingenic_pll_recalc_rate() and
> >> enforce od_max == 0 when od_bits is zero.
> >> drivers/clk/ingenic/cgu.c | 21 +++++++++++++++------
> >> drivers/clk/ingenic/cgu.h | 3 ++-
> >> 2 files changed, 17 insertions(+), 7 deletions(-)
> >> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
> >> index 861c50d6cb24..3481129114b1 100644
> >> --- a/drivers/clk/ingenic/cgu.c
> >> +++ b/drivers/clk/ingenic/cgu.c
> >> @@ -83,7 +83,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long
> >> parent_rate)
> >> const struct ingenic_cgu_clk_info *clk_info = to_clk_info(ingenic_clk);
> >> struct ingenic_cgu *cgu = ingenic_clk->cgu;
> >> const struct ingenic_cgu_pll_info *pll_info;
> >> - unsigned m, n, od_enc, od;
> >> + unsigned m, n, od, od_enc = 0;
> >> bool bypass;
> >> u32 ctl;
> >> @@ -96,8 +96,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long
> >> parent_rate)
> >> m += pll_info->m_offset;
> >> n = (ctl >> pll_info->n_shift) & GENMASK(pll_info->n_bits - 1, 0);
> >> n += pll_info->n_offset;
> >> - od_enc = ctl >> pll_info->od_shift;
> >> - od_enc &= GENMASK(pll_info->od_bits - 1, 0);
> >> +
> >> + if (pll_info->od_bits > 0) {
> >> + od_enc = ctl >> pll_info->od_shift;
> >> + od_enc &= GENMASK(pll_info->od_bits - 1, 0);
> >> + }
> >> if (pll_info->bypass_bit >= 0) {
> >> ctl = readl(cgu->base + pll_info->bypass_reg);
> >> @@ -112,7 +115,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long
> >> parent_rate)
> >> if (pll_info->od_encoding[od] == od_enc)
> >> break;
> >> }
> >
> > I'd add a space there.
> >
> > With that:
> > Reviewed-by: Paul Cercueil <[email protected]>
> >
>
> Already done; the space is there in my outbox and on lore.kernel.org.
> I think you might've accidentally removed it. Stephen's already
> applied the series anyway, so...
>
I fixed the whitespace.