[PATCH v4 1/4] clk: fractional-divider: Export approximation algorithm to the CCF users

At least one user currently duplicates some functions that are provided
by fractional divider module. Let's export approximation algorithm and
replace the open-coded variant.

As a bonus the exported function will get better documentation in place.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Heiko Stuebner <heiko@sntech.de>
---
v4: rebased on top of latest CLK codebase
 drivers/clk/clk-fractional-divider.c | 11 +++++++----
 drivers/clk/clk-fractional-divider.h |  9 +++++++++
 drivers/clk/rockchip/clk.c           | 17 +++--------------
 3 files changed, 19 insertions(+), 18 deletions(-)
 create mode 100644 drivers/clk/clk-fractional-divider.h

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index b1e556f20911..535d299af646 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c

@@ -14,6 +14,8 @@
 #include <linux/slab.h>
 #include <linux/rational.h>
 
+#include "clk-fractional-divider.h"
+
 static inline u32 clk_fd_readl(struct clk_fractional_divider *fd)
 {
 	if (fd->flags & CLK_FRAC_DIVIDER_BIG_ENDIAN)

@@ -68,9 +70,10 @@ static unsigned long clk_fd_recalc_rate(struct clk_hw *hw,
 	return ret;
 }
 
-static void clk_fd_general_approximation(struct clk_hw *hw, unsigned long rate,
-					 unsigned long *parent_rate,
-					 unsigned long *m, unsigned long *n)
+void clk_fractional_divider_general_approximation(struct clk_hw *hw,
+						  unsigned long rate,
+						  unsigned long *parent_rate,
+						  unsigned long *m, unsigned long *n)
 {
 	struct clk_fractional_divider *fd = to_clk_fd(hw);
 	unsigned long scale;

@@ -102,7 +105,7 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
 	if (fd->approximation)
 		fd->approximation(hw, rate, parent_rate, &m, &n);
 	else
-		clk_fd_general_approximation(hw, rate, parent_rate, &m, &n);
+		clk_fractional_divider_general_approximation(hw, rate, parent_rate, &m, &n);
 
 	ret = (u64)*parent_rate * m;
 	do_div(ret, n);

diff --git a/drivers/clk/clk-fractional-divider.h b/drivers/clk/clk-fractional-divider.h
new file mode 100644
index 000000000000..4fa359a12ef4
--- /dev/null
+++ b/drivers/clk/clk-fractional-divider.h

@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+struct clk_hw;
+
+void clk_fractional_divider_general_approximation(struct clk_hw *hw,
+						  unsigned long rate,
+						  unsigned long *parent_rate,
+						  unsigned long *m,
+						  unsigned long *n);

diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 049e5e0b64f6..b7be7e11b0df 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c

@@ -22,6 +22,8 @@
 #include <linux/regmap.h>
 #include <linux/reboot.h>
 #include <linux/rational.h>
+
+#include "../clk-fractional-divider.h"
 #include "clk.h"
 
 /*

@@ -178,10 +180,8 @@ static void rockchip_fractional_approximation(struct clk_hw *hw,
 		unsigned long rate, unsigned long *parent_rate,
 		unsigned long *m, unsigned long *n)
 {
-	struct clk_fractional_divider *fd = to_clk_fd(hw);
 	unsigned long p_rate, p_parent_rate;
 	struct clk_hw *p_parent;
-	unsigned long scale;
 
 	p_rate = clk_hw_get_rate(clk_hw_get_parent(hw));
 	if ((rate * 20 > p_rate) && (p_rate % rate != 0)) {

@@ -190,18 +190,7 @@ static void rockchip_fractional_approximation(struct clk_hw *hw,
 		*parent_rate = p_parent_rate;
 	}
 
-	/*
-	 * Get rate closer to *parent_rate to guarantee there is no overflow
-	 * for m and n. In the result it will be the nearest rate left shifted
-	 * by (scale - fd->nwidth) bits.
-	 */
-	scale = fls_long(*parent_rate / rate - 1);
-	if (scale > fd->nwidth)
-		rate <<= scale - fd->nwidth;
-
-	rational_best_approximation(rate, *parent_rate,
-			GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),
-			m, n);
+	clk_fractional_divider_general_approximation(hw, rate, parent_rate, m, n);
 }
 
 static struct clk *rockchip_clk_register_frac_branch(

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

The newly introduced POWER_OF_TWO_PS flag, when set, makes the flow
to skip the assumption that the caller will use an additional 2^scale
prescaler to get the desired clock rate.

Reported-by: Liu Ying <victor.liu@nxp.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v4: rebased on top of latest CLK codebase

 drivers/acpi/acpi_lpss.c             |  4 ++--
 drivers/clk/clk-fractional-divider.c | 10 ++++++----
 drivers/mfd/intel-lpss.c             |  3 ++-
 include/linux/clk-provider.h         | 11 +++++++++--
 4 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 7f163074e4e4..30b1f511c2af 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c

@@ -436,8 +436,8 @@ static int register_device_clock(struct acpi_device *adev,
 		if (!clk_name)
 			return -ENOMEM;
 		clk = clk_register_fractional_divider(NULL, clk_name, parent,
-						      0, prv_base,
-						      1, 15, 16, 15, 0, NULL);
+						      CLK_FRAC_DIVIDER_POWER_OF_TWO_PS,
+						      prv_base, 1, 15, 16, 15, 0, NULL);
 		parent = clk_name;
 
 		clk_name = kasprintf(GFP_KERNEL, "%s-update", devname);

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 535d299af646..6a3ed82fdae9 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c

@@ -76,16 +76,18 @@ void clk_fractional_divider_general_approximation(struct clk_hw *hw,
 						  unsigned long *m, unsigned long *n)
 {
 	struct clk_fractional_divider *fd = to_clk_fd(hw);
-	unsigned long scale;
 
 	/*
 	 * Get rate closer to *parent_rate to guarantee there is no overflow
 	 * for m and n. In the result it will be the nearest rate left shifted
 	 * by (scale - fd->nwidth) bits.
 	 */
-	scale = fls_long(*parent_rate / rate - 1);
-	if (scale > fd->nwidth)
-		rate <<= scale - fd->nwidth;
+	if (fd->flags & CLK_FRAC_DIVIDER_POWER_OF_TWO_PS) {
+		unsigned long scale = fls_long(*parent_rate / rate - 1);
+
+		if (scale > fd->nwidth)
+			rate <<= scale - fd->nwidth;
+	}
 
 	rational_best_approximation(rate, *parent_rate,
 			GENMASK(fd->mwidth - 1, 0), GENMASK(fd->nwidth - 1, 0),

diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
index a9bf10bee796..0e15afc39f54 100644
--- a/drivers/mfd/intel-lpss.c
+++ b/drivers/mfd/intel-lpss.c

@@ -301,7 +301,8 @@ static int intel_lpss_register_clock_divider(struct intel_lpss *lpss,
 
 	snprintf(name, sizeof(name), "%s-div", devname);
 	tmp = clk_register_fractional_divider(NULL, name, __clk_get_name(tmp),
-					      0, lpss->priv, 1, 15, 16, 15, 0,
+					      CLK_FRAC_DIVIDER_POWER_OF_TWO_PS,
+					      lpss->priv, 1, 15, 16, 15, 0,
 					      NULL);
 	if (IS_ERR(tmp))
 		return PTR_ERR(tmp);

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index d8609aef2394..f59c875271a0 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h

@@ -1001,6 +1001,12 @@ struct clk_hw *devm_clk_hw_register_fixed_factor(struct device *dev,
  * CLK_FRAC_DIVIDER_BIG_ENDIAN - By default little endian register accesses are
  *	used for the divider register.  Setting this flag makes the register
  *	accesses big endian.
+ * CLK_FRAC_DIVIDER_POWER_OF_TWO_PS - By default the resulting fraction might
+ *	be saturated and the caller will get quite far from the good enough
+ *	approximation. Instead the caller may require, by setting this flag,
+ *	to shift left by a few bits in case, when the asked one is quite small
+ *	to satisfy the desired range of denominator. It assumes that on the
+ *	caller's side the power-of-two capable prescaler exists.
  */
 struct clk_fractional_divider {
 	struct clk_hw	hw;

@@ -1020,8 +1026,9 @@ struct clk_fractional_divider {
 
 #define to_clk_fd(_hw) container_of(_hw, struct clk_fractional_divider, hw)
 
-#define CLK_FRAC_DIVIDER_ZERO_BASED	BIT(0)
-#define CLK_FRAC_DIVIDER_BIG_ENDIAN	BIT(1)
+#define CLK_FRAC_DIVIDER_ZERO_BASED		BIT(0)
+#define CLK_FRAC_DIVIDER_BIG_ENDIAN		BIT(1)
+#define CLK_FRAC_DIVIDER_POWER_OF_TWO_PS	BIT(2)
 
 struct clk *clk_register_fractional_divider(struct device *dev,
 		const char *name, const char *parent_name, unsigned long flags,

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

The providers are all located in drivers/clk/ and hence no need
to export the clock operations to wider audience. Hide them by
moving to drivers/clk/clk-fractional-divider.h.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v4: rebased on top of latest CLK codebase, fixed compilation error

 drivers/clk/clk-fractional-divider.h | 2 ++
 drivers/clk/imx/clk-composite-7ulp.c | 1 +
 include/linux/clk-provider.h         | 1 -
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-fractional-divider.h b/drivers/clk/clk-fractional-divider.h
index 4fa359a12ef4..aaaef90b2777 100644
--- a/drivers/clk/clk-fractional-divider.h
+++ b/drivers/clk/clk-fractional-divider.h

@@ -2,6 +2,8 @@
 
 struct clk_hw;
 
+extern const struct clk_ops clk_fractional_divider_ops;
+
 void clk_fractional_divider_general_approximation(struct clk_hw *hw,
 						  unsigned long rate,
 						  unsigned long *parent_rate,

diff --git a/drivers/clk/imx/clk-composite-7ulp.c b/drivers/clk/imx/clk-composite-7ulp.c
index 7c4f31b31eb0..d85ba78abbb1 100644
--- a/drivers/clk/imx/clk-composite-7ulp.c
+++ b/drivers/clk/imx/clk-composite-7ulp.c

@@ -10,6 +10,7 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 
+#include "../clk-fractional-divider.h"
 #include "clk.h"
 
 #define PCG_PCS_SHIFT	24

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 7be81d5fcf8c..d8609aef2394 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h

@@ -1023,7 +1023,6 @@ struct clk_fractional_divider {
 #define CLK_FRAC_DIVIDER_ZERO_BASED	BIT(0)
 #define CLK_FRAC_DIVIDER_BIG_ENDIAN	BIT(1)
 
-extern const struct clk_ops clk_fractional_divider_ops;
 struct clk *clk_register_fractional_divider(struct device *dev,
 		const char *name, const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

It appears that some code lines raise the question why they are needed
and how they are participated in the calculus of the resulting values.

Document this in a form of the top comment in the module file.

Reported-by: Liu Ying <victor.liu@nxp.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v4: rebased on top of latest CLK codebase

 drivers/clk/clk-fractional-divider.c | 35 +++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 6a3ed82fdae9..b4f365d97bd0 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c

@@ -3,8 +3,39 @@
  * Copyright (C) 2014 Intel Corporation
  *
  * Adjustable fractional divider clock implementation.
- * Output rate = (m / n) * parent_rate.
  * Uses rational best approximation algorithm.
+ *
+ * Output is calculated as
+ *
+ *	rate = (m / n) * parent_rate				(1)
+ *
+ * This is useful when on die we have a prescaler block which asks for
+ * m (numerator) and n (denominator) values to be provided to satisfy
+ * the (1) as much as possible.
+ *
+ * Since m and n have the limitation by a range, e.g.
+ *
+ *	n >= 1, n < N_width, where N_width = 2^nwidth		(2)
+ *
+ * for some cases the output may be saturated. Hence, from (1) and (2),
+ * assuming the worst case when m = 1, the inequality
+ *
+ *	floor(log2(parent_rate / rate)) <= nwidth		(3)
+ *
+ * may be derived. Thus, in cases when
+ *
+ *	(parent_rate / rate) >> N_width				(4)
+ *
+ * we might scale up the rate by 2^scale (see the description of
+ * CLK_FRAC_DIVIDER_POWER_OF_TWO_PS for additional information), where
+ *
+ *	scale = floor(log2(parent_rate / rate)) - nwidth	(5)
+ *
+ * and assume that the IP, that needs m and n, has also its own
+ * prescaler, which is capable to divide by 2^scale. In this way
+ * we get the denominator to satisfy the desired range (2) and
+ * at the same time much much better result of m and n than simple
+ * saturated values.
  */
 
 #include <linux/clk-provider.h>

@@ -81,6 +112,8 @@ void clk_fractional_divider_general_approximation(struct clk_hw *hw,
 	 * Get rate closer to *parent_rate to guarantee there is no overflow
 	 * for m and n. In the result it will be the nearest rate left shifted
 	 * by (scale - fd->nwidth) bits.
+	 *
+	 * For the detailed explanation see the top comment in this file.
 	 */
 	if (fd->flags & CLK_FRAC_DIVIDER_POWER_OF_TWO_PS) {
 		unsigned long scale = fls_long(*parent_rate / rate - 1);

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoting Andy Shevchenko (2021-08-12 10:00:23)

The providers are all located in drivers/clk/ and hence no need
to export the clock operations to wider audience. Hide them by
moving to drivers/clk/clk-fractional-divider.h.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

Applied to clk-next

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoting Andy Shevchenko (2021-08-12 10:00:22)

At least one user currently duplicates some functions that are provided
by fractional divider module. Let's export approximation algorithm and
replace the open-coded variant.

As a bonus the exported function will get better documentation in place.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Heiko Stuebner <heiko@sntech.de>
---

Applied to clk-next

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoting Andy Shevchenko (2021-08-12 10:00:24)

The newly introduced POWER_OF_TWO_PS flag, when set, makes the flow
to skip the assumption that the caller will use an additional 2^scale
prescaler to get the desired clock rate.

Reported-by: Liu Ying <victor.liu@nxp.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

Applied to clk-next

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoting Andy Shevchenko (2021-08-12 10:00:25)

It appears that some code lines raise the question why they are needed
and how they are participated in the calculus of the resulting values.

Document this in a form of the top comment in the module file.

Reported-by: Liu Ying <victor.liu@nxp.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

Applied to clk-next

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Thu, Aug 12, 2021 at 12:56:35PM -0700, Stephen Boyd wrote:

Quoting Andy Shevchenko (2021-08-12 10:00:22)

quoted

At least one user currently duplicates some functions that are provided
by fractional divider module. Let's export approximation algorithm and
replace the open-coded variant.

As a bonus the exported function will get better documentation in place.

Applied to clk-next

Thank you, Stephen!

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Aug 13, 2021 at 12:43:22PM +0300, Andy Shevchenko wrote:

On Thu, Aug 12, 2021 at 12:56:35PM -0700, Stephen Boyd wrote:

quoted

Quoting Andy Shevchenko (2021-08-12 10:00:22)

quoted

At least one user currently duplicates some functions that are provided
by fractional divider module. Let's export approximation algorithm and
replace the open-coded variant.

As a bonus the exported function will get better documentation in place.

Applied to clk-next

Thank you, Stephen!

When they are expected to be visible in Linux Next?

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: Chris Morgan <redacted>

Unfortunately, I can confirm this breaks the DSI panel on the Rockchip
PX30 (and possibly other SoCs). Tested on my Odroid Go Advance. When
I revert 4e7cf74fa3b2 "clk: fractional-divider: Export approximation
algorithm to the CCF users" and 928f9e268611 "clk: fractional-divider:
Hide clk_fractional_divider_ops from wide audience" the panel begins
working again as expected on the master branch.

It looks like an assumption is made in the vop_crtc_mode_fixup()
function in the rockchip_drm_vop.c that gets broken with this change.
Specifically, the function says in the comments "When DRM gives us a
mode, we should add 999 Hz to it.". I believe this is no longer true
after this clk change, and when I remove the + 999 from the function
the DSI panel works again. Note that I do not know the implications
of removing this 999 aside from that it fixes the DSI panel on my
PX30 after this change, so I don't know if it's a positive change
or not.

Thank you.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Sep 07, 2021 at 10:44:00AM -0500, Chris Morgan wrote:

From: Chris Morgan <redacted>

Unfortunately, I can confirm this breaks the DSI panel on the Rockchip
PX30 (and possibly other SoCs). Tested on my Odroid Go Advance. When
I revert 4e7cf74fa3b2 "clk: fractional-divider: Export approximation
algorithm to the CCF users" and 928f9e268611 "clk: fractional-divider:
Hide clk_fractional_divider_ops from wide audience" the panel begins
working again as expected on the master branch.

It looks like an assumption is made in the vop_crtc_mode_fixup()
function in the rockchip_drm_vop.c that gets broken with this change.
Specifically, the function says in the comments "When DRM gives us a
mode, we should add 999 Hz to it.". I believe this is no longer true
after this clk change, and when I remove the + 999 from the function
the DSI panel works again. Note that I do not know the implications
of removing this 999 aside from that it fixes the DSI panel on my
PX30 after this change, so I don't know if it's a positive change
or not.

Thank you.

Thank you for the report!

I'll check this. Perhaps Heiko can help with testing as well on his side.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Sep 07, 2021 at 08:54:04PM +0300, Andy Shevchenko wrote:

On Tue, Sep 07, 2021 at 10:44:00AM -0500, Chris Morgan wrote:

quoted

From: Chris Morgan <redacted>

Unfortunately, I can confirm this breaks the DSI panel on the Rockchip
PX30 (and possibly other SoCs). Tested on my Odroid Go Advance. When
I revert 4e7cf74fa3b2 "clk: fractional-divider: Export approximation
algorithm to the CCF users" and 928f9e268611 "clk: fractional-divider:
Hide clk_fractional_divider_ops from wide audience" the panel begins
working again as expected on the master branch.

It looks like an assumption is made in the vop_crtc_mode_fixup()
function in the rockchip_drm_vop.c that gets broken with this change.
Specifically, the function says in the comments "When DRM gives us a
mode, we should add 999 Hz to it.". I believe this is no longer true
after this clk change, and when I remove the + 999 from the function
the DSI panel works again. Note that I do not know the implications
of removing this 999 aside from that it fixes the DSI panel on my
PX30 after this change, so I don't know if it's a positive change
or not.

Thank you.

Thank you for the report!

I'll check this. Perhaps Heiko can help with testing as well on his side.

On the first glance the mentioned patch may not be the culprit because it does
not change the functional behaviour (if I'm not mistaken). What really changes
it is the additional flag that removes the left-shift of the rate in the
calculations.

To me sounds like you found a proper solution to the issue and that +999 is
a hack against the (blindly?) copied part of the algorithm used in fractional
divider. Have you read the top comment in clk-fractional-divider.c? It should
explain how it works after my series.

In any case I'm not going to come to any conclusions right now and also want
to hear from people who have better understanding of this hardware.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Sep 07, 2021 at 09:06:10PM +0300, Andy Shevchenko wrote:

On Tue, Sep 07, 2021 at 08:54:04PM +0300, Andy Shevchenko wrote:

quoted

On Tue, Sep 07, 2021 at 10:44:00AM -0500, Chris Morgan wrote:

quoted

From: Chris Morgan <redacted>

Unfortunately, I can confirm this breaks the DSI panel on the Rockchip
PX30 (and possibly other SoCs). Tested on my Odroid Go Advance. When
I revert 4e7cf74fa3b2 "clk: fractional-divider: Export approximation
algorithm to the CCF users" and 928f9e268611 "clk: fractional-divider:
Hide clk_fractional_divider_ops from wide audience" the panel begins
working again as expected on the master branch.

It looks like an assumption is made in the vop_crtc_mode_fixup()
function in the rockchip_drm_vop.c that gets broken with this change.
Specifically, the function says in the comments "When DRM gives us a
mode, we should add 999 Hz to it.". I believe this is no longer true
after this clk change, and when I remove the + 999 from the function
the DSI panel works again. Note that I do not know the implications
of removing this 999 aside from that it fixes the DSI panel on my
PX30 after this change, so I don't know if it's a positive change
or not.

Thank you.

Thank you for the report!

I'll check this. Perhaps Heiko can help with testing as well on his side.

On the first glance the mentioned patch may not be the culprit because it does
not change the functional behaviour (if I'm not mistaken). What really changes
it is the additional flag that removes the left-shift of the rate in the
calculations.

I noticed the behavior on the 5.14 kernel was to set the numerator at
an ungodly 7649082492112076800 and the denominator at 1 (no, that's not
a typo). I think it tried to write 65535 to the register though, but it
would go through this a few times and eventually settle on 1:1 as the
fractional ratio (which I assume is all good, because that would work).

Contrast this to the 5.15 behavior where it would try to set the ratio
to 17001:17000, which would cause the DSI screen to fail to initalize.

After tracing through the code I figured out that the VOP was trying to
add 999 to the clock and set it at 17000999. 17000000/17000999 gives us
0, and subtracting 1 from that gives us a -1. The fls_long function 
would then return 64, and if we subtract 16 (the value of fd->mwidth
for my board) it would tell us to shift the 17000999 48 bits to the
left, which matches the ungodly large number.

With the changes in 5.15 if I remove the + 999 from the VOP driver the
clock then gets set at 17000000, since the parent is at 17000000 that
gives us a 1:1 where everything works and everything is fine.

Long story short I think this is a bug that's existed all along, and
this change simply exposed it in a manner where it stopped working
despite the bug being present. Unfortunately I neither know enough
about the hardware to be confident in this fix beyond my specific
board, nor do I have enough hardware to test it on anything except
a Rockchip rk3326 with a DSI panel.

To me sounds like you found a proper solution to the issue and that +999 is
a hack against the (blindly?) copied part of the algorithm used in fractional
divider. Have you read the top comment in clk-fractional-divider.c? It should
explain how it works after my series.

No, but I probably should read the docs more. I just stumbled on this
series doing a bisect when the DSI panel stopped working.

In any case I'm not going to come to any conclusions right now and also want
to hear from people who have better understanding of this hardware.

Yeah, I want to see what Heiko says after some more research, or anyone
who has more familiarity with clocks/DRM than I do or who has more
hardware to test on than I do.

I intended to send a message informing you that "hey, this breaks
upstream", but I think it turns out it's more a matter of "hey,
this makes a broken upstream break instead of limp along".

Thank you.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Sep 07, 2021 at 09:17:47PM -0500, Chris Morgan wrote:

On Tue, Sep 07, 2021 at 09:06:10PM +0300, Andy Shevchenko wrote:

quoted

On Tue, Sep 07, 2021 at 08:54:04PM +0300, Andy Shevchenko wrote:

quoted

On Tue, Sep 07, 2021 at 10:44:00AM -0500, Chris Morgan wrote:

quoted

quoted

quoted

Unfortunately, I can confirm this breaks the DSI panel on the Rockchip
PX30 (and possibly other SoCs). Tested on my Odroid Go Advance. When
I revert 4e7cf74fa3b2 "clk: fractional-divider: Export approximation
algorithm to the CCF users" and 928f9e268611 "clk: fractional-divider:
Hide clk_fractional_divider_ops from wide audience" the panel begins
working again as expected on the master branch.

It looks like an assumption is made in the vop_crtc_mode_fixup()
function in the rockchip_drm_vop.c that gets broken with this change.
Specifically, the function says in the comments "When DRM gives us a
mode, we should add 999 Hz to it.". I believe this is no longer true
after this clk change, and when I remove the + 999 from the function
the DSI panel works again. Note that I do not know the implications
of removing this 999 aside from that it fixes the DSI panel on my
PX30 after this change, so I don't know if it's a positive change
or not.

Thank you for the report!

I'll check this. Perhaps Heiko can help with testing as well on his side.

On the first glance the mentioned patch may not be the culprit because it does
not change the functional behaviour (if I'm not mistaken). What really changes
it is the additional flag that removes the left-shift of the rate in the
calculations.

I noticed the behavior on the 5.14 kernel was to set the numerator at
an ungodly 7649082492112076800 and the denominator at 1 (no, that's not
a typo). I think it tried to write 65535 to the register though, but it
would go through this a few times and eventually settle on 1:1 as the
fractional ratio (which I assume is all good, because that would work).

Contrast this to the 5.15 behavior where it would try to set the ratio
to 17001:17000, which would cause the DSI screen to fail to initalize.

After tracing through the code I figured out that the VOP was trying to
add 999 to the clock and set it at 17000999. 17000000/17000999 gives us
0, and subtracting 1 from that gives us a -1. The fls_long function
would then return 64, and if we subtract 16 (the value of fd->mwidth
for my board) it would tell us to shift the 17000999 48 bits to the
left, which matches the ungodly large number.

With the changes in 5.15 if I remove the + 999 from the VOP driver the
clock then gets set at 17000000, since the parent is at 17000000 that
gives us a 1:1 where everything works and everything is fine.

Long story short I think this is a bug that's existed all along, and
this change simply exposed it in a manner where it stopped working
despite the bug being present. Unfortunately I neither know enough
about the hardware to be confident in this fix beyond my specific
board, nor do I have enough hardware to test it on anything except
a Rockchip rk3326 with a DSI panel.

This is a very good analysis!

quoted

To me sounds like you found a proper solution to the issue and that +999 is
a hack against the (blindly?) copied part of the algorithm used in fractional
divider. Have you read the top comment in clk-fractional-divider.c? It should
explain how it works after my series.

No, but I probably should read the docs more. I just stumbled on this
series doing a bisect when the DSI panel stopped working.

quoted

In any case I'm not going to come to any conclusions right now and also want
to hear from people who have better understanding of this hardware.

Yeah, I want to see what Heiko says after some more research, or anyone
who has more familiarity with clocks/DRM than I do or who has more
hardware to test on than I do.

After what I read above I can't add anything and what I think is the best
course of actions is to submit a patch with removal of +999 part and above
explanation. It would be nice to find the real commit ID that may be used
for a Fixes tag.

Then we  at least will have a patch ready in case it's considered correct
by people from Rockchip side.

I intended to send a message informing you that "hey, this breaks
upstream", but I think it turns out it's more a matter of "hey,
this makes a broken upstream break instead of limp along".

Understand.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel