diff mbox series

[v2,1/6] clk: ingenic: Make PLL clock "od" field optional

Message ID	20221026194345.243007-2-aidanmacdonald.0x0@gmail.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> To: paul@crapouillou.net, mturquette@baylibre.com, sboyd@kernel.org, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org Cc: zhouyu@wanyeetech.com, linux-mips@vger.kernel.org, linux-clk@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH v2 1/6] clk: ingenic: Make PLL clock "od" field optional Date: Wed, 26 Oct 2022 20:43:40 +0100 Message-Id: <20221026194345.243007-2-aidanmacdonald.0x0@gmail.com> In-Reply-To: <20221026194345.243007-1-aidanmacdonald.0x0@gmail.com> References: <20221026194345.243007-1-aidanmacdonald.0x0@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	Add support for X1000 audio clocks \| [v2,0/6] Add support for X1000 audio clocks [v2,1/6] clk: ingenic: Make PLL clock "od" field optional [v2,2/6] clk: ingenic: Make PLL clock enable_bit and stable_bit optional [v2,3/6] clk: ingenic: Add .set_rate_hook() for PLL clocks [v2,4/6] dt-bindings: ingenic,x1000-cgu: Add audio clocks [v2,5/6] clk: ingenic: Add X1000 audio clocks [v2,6/6] clk: ingenic: Minor cosmetic fixups for X1000

Commit Message

Aidan MacDonald Oct. 26, 2022, 7:43 p.m. UTC

  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 <aidanmacdonald.0x0@gmail.com>
---
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(-)

Comments

Paul Cercueil Oct. 27, 2022, 12:38 p.m. UTC | #1

Hi Aidan,

Le mer. 26 oct. 2022 à 20:43:40 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> 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 <aidanmacdonald.0x0@gmail.com>
> ---
> 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 <paul@crapouillou.net>

> -	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
>

Stephen Boyd Oct. 27, 2022, 6:59 p.m. UTC | #2

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 <aidanmacdonald.0x0@gmail.com>
> ---

Applied to clk-next

Aidan MacDonald Oct. 27, 2022, 9:40 p.m. UTC | #3

Paul Cercueil <paul@crapouillou.net> writes:

> Hi Aidan,
>
> Le mer. 26 oct. 2022 à 20:43:40 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> 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 <aidanmacdonald.0x0@gmail.com>
>> ---
>> 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 <paul@crapouillou.net>
>

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
>>

Stephen Boyd Oct. 27, 2022, 9:51 p.m. UTC | #4

Quoting Aidan MacDonald (2022-10-27 14:40:02)
> 
> Paul Cercueil <paul@crapouillou.net> writes:
> 
> > Hi Aidan,
> >
> > Le mer. 26 oct. 2022 à 20:43:40 +0100, Aidan MacDonald
> > <aidanmacdonald.0x0@gmail.com> 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 <aidanmacdonald.0x0@gmail.com>
> >> ---
> >> 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 <paul@crapouillou.net>
> >
> 
> 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.

diff mbox series

Patch

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