diff mbox series

[5/8] clk: qcom: support for duplicate freq in RCG2 freq table

Message ID TYZPR01MB5556DEA3D4740441EC561414C9762@TYZPR01MB5556.apcprd01.prod.exchangelabs.com (mailing list archive)
State New, archived
Headers show
Series ipq5018: enable ethernet support | expand

Commit Message

Ziyang Huang Jan. 21, 2024, 12:42 p.m. UTC
From: Praveenkumar I <ipkumar@codeaurora.org>

Currently RCG code looks up the frequency table during set
rate and return the first available frequency greater than
requested rate. If CLK_SET_RATE_PARENT flag is set then the
set_rate request will go to its parent otherwise the clock
framework will configure pre-div, m and n according to the
returned frequency table entry. In this case, it is assuming
that parent clock will run in the same frequency with which
pre-div, m and n has been derived. But it may be possible
that the parent clock supports multiple frequency and the
same frequency can be derived with different pre-div, m and
n values depending upon current frequency.  Also, the same
frequency can be derived from different parent sources and
currently there is no option for having duplicate
frequencies in frequency table and choosing the best one
according to current rate.

Now this patch adds the support for having duplicate
frequencies in frequency table. During set rate, it will
compare the actual rate for each entry with requested rate
and will select the best entry in which the difference will
be less.

The existing functionality won’t be affected with this code
change since this code change will hit only if frequency
table has duplicate values.

Change-Id: I97d9e1b55d8f3ee095f6f01729af527ba90e50e5
Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
(cherry picked from commit 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2)
Signed-off-by: Praveenkumar I <ipkumar@codeaurora.org>

Change-Id: If10193fc79a3c1375ab73597813745ff1f4df0ad

Pick from https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/6dfb368bae130bee58e00ddf8330b55066e1c8c5

Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
---
 drivers/clk/qcom/clk-rcg2.c | 86 ++++++++++++++++++++++++++++---------
 1 file changed, 66 insertions(+), 20 deletions(-)

Comments

Andrew Lunn Jan. 21, 2024, 4:57 p.m. UTC | #1
On Sun, Jan 21, 2024 at 08:42:34PM +0800, Ziyang Huang wrote:
> From: Praveenkumar I <ipkumar@codeaurora.org>
> 
> Currently RCG code looks up the frequency table during set
> rate and return the first available frequency greater than
> requested rate. If CLK_SET_RATE_PARENT flag is set then the
> set_rate request will go to its parent otherwise the clock
> framework will configure pre-div, m and n according to the
> returned frequency table entry. In this case, it is assuming
> that parent clock will run in the same frequency with which
> pre-div, m and n has been derived. But it may be possible
> that the parent clock supports multiple frequency and the
> same frequency can be derived with different pre-div, m and
> n values depending upon current frequency.  Also, the same
> frequency can be derived from different parent sources and
> currently there is no option for having duplicate
> frequencies in frequency table and choosing the best one
> according to current rate.
> 
> Now this patch adds the support for having duplicate
> frequencies in frequency table. During set rate, it will
> compare the actual rate for each entry with requested rate
> and will select the best entry in which the difference will
> be less.
> 
> The existing functionality won’t be affected with this code
> change since this code change will hit only if frequency
> table has duplicate values.

A good commit message for a change!

> 
> Change-Id: I97d9e1b55d8f3ee095f6f01729af527ba90e50e5
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> (cherry picked from commit 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2)
> Signed-off-by: Praveenkumar I <ipkumar@codeaurora.org>
> 
> Change-Id: If10193fc79a3c1375ab73597813745ff1f4df0ad
> 
> Pick from https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/6dfb368bae130bee58e00ddf8330b55066e1c8c5
> 
> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>

Please clean up these tags. These Change-ID tags are meaningless in
mainline. 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2 is not in mainline
either. The picked from might be interesting, but please put it into
the body of the commit message, not mixed in with the tags.

Who actually wrote this patch? The first Signed-off-by: is from
Abhishek Sahu. But you have a From of Praveenkumar I ?

    Andrew

---
pw-bot: cr
Krzysztof Kozlowski Jan. 22, 2024, 7:55 a.m. UTC | #2
On 21/01/2024 13:42, Ziyang Huang wrote:
> From: Praveenkumar I <ipkumar@codeaurora.org>
> 
> Currently RCG code looks up the frequency table during set
> rate and return the first available frequency greater than
> requested rate. If CLK_SET_RATE_PARENT flag is set then the
> set_rate request will go to its parent otherwise the clock
> framework will configure pre-div, m and n according to the
> returned frequency table entry. In this case, it is assuming
> that parent clock will run in the same frequency with which
> pre-div, m and n has been derived. But it may be possible
> that the parent clock supports multiple frequency and the
> same frequency can be derived with different pre-div, m and
> n values depending upon current frequency.  Also, the same
> frequency can be derived from different parent sources and
> currently there is no option for having duplicate
> frequencies in frequency table and choosing the best one
> according to current rate.
> 
> Now this patch adds the support for having duplicate
> frequencies in frequency table. During set rate, it will
> compare the actual rate for each entry with requested rate
> and will select the best entry in which the difference will
> be less.
> 
> The existing functionality won’t be affected with this code
> change since this code change will hit only if frequency
> table has duplicate values.
> 
> Change-Id: I97d9e1b55d8f3ee095f6f01729af527ba90e50e5
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> (cherry picked from commit 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2)
> Signed-off-by: Praveenkumar I <ipkumar@codeaurora.org>
> 
> Change-Id: If10193fc79a3c1375ab73597813745ff1f4df0ad

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

Best regards,
Krzysztof
Ziyang Huang Jan. 22, 2024, 2:48 p.m. UTC | #3
在 2024/1/22 15:55, Krzysztof Kozlowski 写道:
> On 21/01/2024 13:42, Ziyang Huang wrote:
> [...]
> 
> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
> 
> Best regards,
> Krzysztof
> 

Hi Krzysztof,

Sorrr for my mistake. For easily comparison, I added these patches to 
git index, which mess up ./scripts/checkpatch.pl and 
./scripts/get_maintainer.pl and led to wrong results. After restored 
them, I noticed these issues, will fix them in next patches.
Ziyang Huang Jan. 22, 2024, 4:35 p.m. UTC | #4
在 2024/1/22 0:57, Andrew Lunn 写道:
> On Sun, Jan 21, 2024 at 08:42:34PM +0800, Ziyang Huang wrote:
>> From: Praveenkumar I <ipkumar@codeaurora.org>
>>
>> Currently RCG code looks up the frequency table during set
>> rate and return the first available frequency greater than
>> requested rate. If CLK_SET_RATE_PARENT flag is set then the
>> set_rate request will go to its parent otherwise the clock
>> framework will configure pre-div, m and n according to the
>> returned frequency table entry. In this case, it is assuming
>> that parent clock will run in the same frequency with which
>> pre-div, m and n has been derived. But it may be possible
>> that the parent clock supports multiple frequency and the
>> same frequency can be derived with different pre-div, m and
>> n values depending upon current frequency.  Also, the same
>> frequency can be derived from different parent sources and
>> currently there is no option for having duplicate
>> frequencies in frequency table and choosing the best one
>> according to current rate.
>>
>> Now this patch adds the support for having duplicate
>> frequencies in frequency table. During set rate, it will
>> compare the actual rate for each entry with requested rate
>> and will select the best entry in which the difference will
>> be less.
>>
>> The existing functionality won’t be affected with this code
>> change since this code change will hit only if frequency
>> table has duplicate values.
> 
> A good commit message for a change!
> 
>>
>> Change-Id: I97d9e1b55d8f3ee095f6f01729af527ba90e50e5
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> (cherry picked from commit 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2)
>> Signed-off-by: Praveenkumar I <ipkumar@codeaurora.org>
>>
>> Change-Id: If10193fc79a3c1375ab73597813745ff1f4df0ad
>>
>> Pick from https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/6dfb368bae130bee58e00ddf8330b55066e1c8c5
>>
>> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
> 
> Please clean up these tags. These Change-ID tags are meaningless in
> mainline. 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2 is not in mainline
> either. The picked from might be interesting, but please put it into
> the body of the commit message, not mixed in with the tags.
> 
> Who actually wrote this patch? The first Signed-off-by: is from
> Abhishek Sahu. But you have a From of Praveenkumar I ?

I have no idea about this. This patch is from Qualcomm vendor linux 
code. And it's necessary for choosing parent and calculating clock rate 
correctly.

What's more, I don't known how to deal with these commit message since 
I'm not the author and I'm not sure do I have right to edit them even 
though this is GPL code.

> 
>      Andrew
> 
> ---
> pw-bot: cr
Andrew Lunn Jan. 22, 2024, 5:34 p.m. UTC | #5
> > > Change-Id: I97d9e1b55d8f3ee095f6f01729af527ba90e50e5
> > > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> > > (cherry picked from commit 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2)
> > > Signed-off-by: Praveenkumar I <ipkumar@codeaurora.org>
> > > 
> > > Change-Id: If10193fc79a3c1375ab73597813745ff1f4df0ad
> > > 
> > > Pick from https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/6dfb368bae130bee58e00ddf8330b55066e1c8c5
> > > 
> > > Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
> > 
> > Please clean up these tags. These Change-ID tags are meaningless in
> > mainline. 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2 is not in mainline
> > either. The picked from might be interesting, but please put it into
> > the body of the commit message, not mixed in with the tags.
> > 
> > Who actually wrote this patch? The first Signed-off-by: is from
> > Abhishek Sahu. But you have a From of Praveenkumar I ?
> 
> I have no idea about this. This patch is from Qualcomm vendor linux code.

O.K. Since this is direct from the vendor, who probably does not track
code authorship correctly, i would say the author in git is probably
wrong. I would set the author: to Abhishek Sahu <absahu@codeaurora.org>.

> What's more, I don't known how to deal with these commit message since I'm
> not the author and I'm not sure do I have right to edit them even though
> this is GPL code.

You should keep all the Signed-off-by, in the order they are. But the
Change-Id is meaningless, so there is no problem removing them.

	  Andrew
Ziyang Huang Jan. 23, 2024, 3:43 p.m. UTC | #6
在 2024/1/23 1:34, Andrew Lunn 写道:
>>>> Change-Id: I97d9e1b55d8f3ee095f6f01729af527ba90e50e5
>>>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>>>> (cherry picked from commit 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2)
>>>> Signed-off-by: Praveenkumar I <ipkumar@codeaurora.org>
>>>>
>>>> Change-Id: If10193fc79a3c1375ab73597813745ff1f4df0ad
>>>>
>>>> Pick from https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/6dfb368bae130bee58e00ddf8330b55066e1c8c5
>>>>
>>>> Signed-off-by: Ziyang Huang <hzyitc@outlook.com>
>>>
>>> Please clean up these tags. These Change-ID tags are meaningless in
>>> mainline. 775e7d3b69ffc97afb5bd5a6c9c423f2f4d8a0b2 is not in mainline
>>> either. The picked from might be interesting, but please put it into
>>> the body of the commit message, not mixed in with the tags.
>>>
>>> Who actually wrote this patch? The first Signed-off-by: is from
>>> Abhishek Sahu. But you have a From of Praveenkumar I ?
>>
>> I have no idea about this. This patch is from Qualcomm vendor linux code.
> 
> O.K. Since this is direct from the vendor, who probably does not track
> code authorship correctly, i would say the author in git is probably
> wrong. I would set the author: to Abhishek Sahu <absahu@codeaurora.org>.
> 
>> What's more, I don't known how to deal with these commit message since I'm
>> not the author and I'm not sure do I have right to edit them even though
>> this is GPL code.
> 
> You should keep all the Signed-off-by, in the order they are. But the
> Change-Id is meaningless, so there is no problem removing them.
> 
> 	  Andrew

These days, I known a new knowledge: SGMII+ only support 2.5G. 1000M 
can't work with SGMII+ mode. So the 1000M frequency divided from 3.125G 
can be dropped. Then we don't have duplicated frequency and this patch 
is unnecessary. Will test my guess.
diff mbox series

Patch

diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index e22baf3a7112..6141e4991fbc 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -209,26 +209,82 @@  clk_rcg2_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 	return __clk_rcg2_recalc_rate(hw, parent_rate, cfg);
 }
 
-static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f,
-				    struct clk_rate_request *req,
-				    enum freq_policy policy)
+static const struct freq_tbl *
+clk_rcg2_find_best_freq(struct clk_hw *hw, const struct freq_tbl *f,
+			unsigned long rate, enum freq_policy policy)
 {
-	unsigned long clk_flags, rate = req->rate;
-	struct clk_hw *p;
+	unsigned long req_rate = rate, best = 0, freq;
 	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
 	int index;
+	u64 tmp;
+	const struct freq_tbl *best_ftable = NULL;
 
 	switch (policy) {
 	case FLOOR:
-		f = qcom_find_freq_floor(f, rate);
+		f = qcom_find_freq_floor(rcg->freq_tbl, rate);
 		break;
 	case CEIL:
-		f = qcom_find_freq(f, rate);
+		f = qcom_find_freq(rcg->freq_tbl, rate);
 		break;
 	default:
-		return -EINVAL;
+		return best_ftable;
 	}
 
+	/*
+	 * Check for duplicate frequencies in frequency table if
+	 * CLK_SET_RATE_PARENT flag is not set
+	 */
+	if (!f || (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) ||
+	    ((f->freq && (f + 1)->freq != f->freq)))
+		return f;
+
+	/*
+	 * Check for all the duplicate entries in frequency table and
+	 * calculate the actual rate from current parent rate with each
+	 * entries pre_div, m and n values. The entry, which gives the
+	 * minimum difference in requested rate and actual rate, will be
+	 * selected as the best one.
+	 */
+	for (freq = f->freq; freq == f->freq; f++) {
+		index = qcom_find_src_index(hw, rcg->parent_map, f->src);
+		if (index < 0)
+			continue;
+
+		rate =  clk_hw_get_rate(clk_hw_get_parent_by_index(hw, index));
+		if (rcg->hid_width && f->pre_div) {
+			rate *= 2;
+			rate /= f->pre_div + 1;
+		}
+
+		if (rcg->mnd_width && f->n) {
+			tmp = rate;
+			tmp = tmp * f->n;
+			do_div(tmp, f->m);
+			rate = tmp;
+		}
+
+		if (abs(req_rate - rate) < abs(best - rate)) {
+			best_ftable = f;
+			best = rate;
+
+			if (req_rate == rate)
+				break;
+		}
+	}
+
+	return best_ftable;
+}
+
+static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f,
+				    struct clk_rate_request *req,
+				    enum freq_policy policy)
+{
+	unsigned long clk_flags, rate = req->rate;
+	struct clk_hw *p;
+	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
+	int index;
+
+	f = clk_rcg2_find_best_freq(hw, f, rate, policy);
 	if (!f)
 		return -EINVAL;
 
@@ -360,17 +416,7 @@  static int __clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate,
 	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
 	const struct freq_tbl *f;
 
-	switch (policy) {
-	case FLOOR:
-		f = qcom_find_freq_floor(rcg->freq_tbl, rate);
-		break;
-	case CEIL:
-		f = qcom_find_freq(rcg->freq_tbl, rate);
-		break;
-	default:
-		return -EINVAL;
-	}
-
+	f = clk_rcg2_find_best_freq(hw, rcg->freq_tbl, rate, policy);
 	if (!f)
 		return -EINVAL;
 
@@ -1032,7 +1078,7 @@  static int clk_rcg2_shared_set_rate(struct clk_hw *hw, unsigned long rate,
 	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
 	const struct freq_tbl *f;
 
-	f = qcom_find_freq(rcg->freq_tbl, rate);
+	f = clk_rcg2_find_best_freq(hw, rcg->freq_tbl, rate, CEIL);
 	if (!f)
 		return -EINVAL;