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

[Aidan MacDonald]

Paul Cercueil <paul@xxxxxxxxxxxxxxx> writes:

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

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