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;
References: <20221023145653.177234-1-aidanmacdonald.0x0@gmail.com>
 <20221023145653.177234-2-aidanmacdonald.0x0@gmail.com>
 <TAP7KR.18SEZOBW3THM2@crapouillou.net>
From:   Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
To:     Paul Cercueil <paul@crapouillou.net>
Cc:     mturquette@baylibre.com, sboyd@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, zhouyu@wanyeetech.com,
        linux-mips@vger.kernel.org, linux-clk@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 1/5] clk: ingenic: Make PLL clock "od" field optional
In-reply-to: <TAP7KR.18SEZOBW3THM2@crapouillou.net>
Date:   Sun, 23 Oct 2022 23:45:28 +0100
Message-ID: <FdF2cloQKfcDjZeA3AGn61Yfkecl2tgV@localhost>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


Paul Cercueil <paul@crapouillou.net> writes:

> Hi Aidan,
>
> Le dim. 23 oct. 2022 =C3=A0 15:56:49 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> a =C3=A9crit :
>> Add support for defining PLL clocks with od_bits =3D 0, meaning that
>> OD is fixed to 1 and there is no OD field in the register.
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>> ---
>>  drivers/clk/ingenic/cgu.c | 28 +++++++++++++++++++---------
>>  drivers/clk/ingenic/cgu.h |  3 ++-
>>  2 files changed, 21 insertions(+), 10 deletions(-)
>> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
>> index 861c50d6cb24..7dc2e2567d53 100644
>> --- a/drivers/clk/ingenic/cgu.c
>> +++ b/drivers/clk/ingenic/cgu.c
>> @@ -96,8 +96,11 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned l=
ong
>> parent_rate)
>>  	m +=3D pll_info->m_offset;
>>  	n =3D (ctl >> pll_info->n_shift) & GENMASK(pll_info->n_bits - 1, 0);
>>  	n +=3D pll_info->n_offset;
>> -	od_enc =3D ctl >> pll_info->od_shift;
>> -	od_enc &=3D GENMASK(pll_info->od_bits - 1, 0);
>> +
>> +	if (pll_info->od_bits > 0) {
>> +		od_enc =3D ctl >> pll_info->od_shift;
>> +		od_enc &=3D GENMASK(pll_info->od_bits - 1, 0);
>> +	}
>>  	if (pll_info->bypass_bit >=3D 0) {
>>  		ctl =3D readl(cgu->base + pll_info->bypass_reg);
>> @@ -108,12 +111,17 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned
>> long parent_rate)
>>  			return parent_rate;
>>  	}
>> -	for (od =3D 0; od < pll_info->od_max; od++) {
>> -		if (pll_info->od_encoding[od] =3D=3D od_enc)
>> -			break;
>> +	if (pll_info->od_bits > 0) {
>> +		for (od =3D 0; od < pll_info->od_max; od++) {
>> +			if (pll_info->od_encoding[od] =3D=3D od_enc)
>> +				break;
>> +		}
>> +		BUG_ON(od =3D=3D pll_info->od_max);
>> +		od++;
>> +	} else {
>> +		/* OD is fixed to 1 if no OD field is present. */
>> +		od =3D 1;
>>  	}
>> -	BUG_ON(od =3D=3D pll_info->od_max);
>> -	od++;
>
> I think if pll_info->od_max is 0 you get the same result without modifyin=
g this
> code. You just need to modify the BUG_ON() to only trigger if pll_info->o=
d_max > 0.
>
> Cheers,
> -Paul
>

Yeah, you're right, that's simpler. Thanks,
Aidan

>>  	return div_u64((u64)parent_rate * m * pll_info->rate_multiplier,
>>  		n * od);
>> @@ -215,8 +223,10 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned lo=
ng
>> req_rate,
>>  	ctl &=3D ~(GENMASK(pll_info->n_bits - 1, 0) << pll_info->n_shift);
>>  	ctl |=3D (n - pll_info->n_offset) << pll_info->n_shift;
>> -	ctl &=3D ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
>> -	ctl |=3D pll_info->od_encoding[od - 1] << pll_info->od_shift;
>> +	if (pll_info->od_bits > 0) {
>> +		ctl &=3D ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
>> +		ctl |=3D 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
>>