[PATCH v4 0/5] Pinephone video out fixes (flipping between two frames)

[PATCH v4 0/5] Pinephone video out fixes (flipping between two frames)

[Frank Oltmanns]

On some pinephones the video output sometimes freezes (flips between two
frames) [1]. It seems to be that the reason for this behaviour is that
PLL-MIPI is outside its limits, and the GPU is not running at a fixed
rate.

In this patch series I propose the following changes:
  1. sunxi-ng: Adhere to the following constraints given in the
     Allwinner A64 Manual regarding PLL-MIPI:
      * M/N <= 3
      * (PLL_VIDEO0)/M >= 24MHz
      * 500MHz <= clockrate <= 1400MHz

  2. Remove two operating points from the A64 DTS OPPs, so that the GPU
     runs at a fixed rate of 432 MHz.

Note, that when pinning the GPU to 432 MHz the issue [1] completely
disappears for me. I've searched the BSP and could not find any
indication that supports the idea of having the three OPPs. The only
frequency I found in the BPSs for A64 is 432 MHz, which has also proven
stable for me.

I very much appreciate your feedback!

[1] https://gitlab.com/postmarketOS/pmaports/-/issues/805

Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
---
Changes in v4:
- sunxi-ng: common: Address review comments.
- Link to v3: https://lore.kernel.org/r/20240304-pinephone-pll-fixes-v3-0-94ab828f269a@oltmanns.dev

Changes in v3:
- dts: Pin GPU to 432 MHz.
- nkm and a64: Move minimum and maximum rate handling to the common part
  of the sunxi-ng driver.
- Removed st7703 patch from series.
- Link to v2: https://lore.kernel.org/r/20240205-pinephone-pll-fixes-v2-0-96a46a2d8c9b@oltmanns.dev

Changes in v2:
- dts: Increase minimum GPU frequency to 192 MHz.
- nkm and a64: Add minimum and maximum rate for PLL-MIPI.
- nkm: Use the same approach for skipping invalid rates in
  ccu_nkm_find_best() as in ccu_nkm_find_best_with_parent_adj().
- nkm: Improve names for ratio struct members and hence get rid of
  describing comments.
- nkm and a64: Correct description in the commit messages: M/N <= 3
- Remove patches for nm as they were not needed.
- st7703: Rework the commit message to cover more background for the
  change.
- Link to v1: https://lore.kernel.org/r/20231218-pinephone-pll-fixes-v1-0-e238b6ed6dc1@oltmanns.dev

---
Frank Oltmanns (5):
      clk: sunxi-ng: common: Support minimum and maximum rate
      clk: sunxi-ng: a64: Set minimum and maximum rate for PLL-MIPI
      clk: sunxi-ng: nkm: Support constraints on m/n ratio and parent rate
      clk: sunxi-ng: a64: Add constraints on PLL-MIPI's n/m ratio and parent rate
      arm64: dts: allwinner: a64: Run GPU at 432 MHz

 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |  8 --------
 drivers/clk/sunxi-ng/ccu-sun50i-a64.c         | 14 +++++++++-----
 drivers/clk/sunxi-ng/ccu_common.c             | 19 +++++++++++++++++++
 drivers/clk/sunxi-ng/ccu_common.h             |  3 +++
 drivers/clk/sunxi-ng/ccu_nkm.c                | 21 +++++++++++++++++++++
 drivers/clk/sunxi-ng/ccu_nkm.h                |  2 ++
 6 files changed, 54 insertions(+), 13 deletions(-)
---
base-commit: dcb6c8ee6acc6c347caec1e73fb900c0f4ff9806
change-id: 20231218-pinephone-pll-fixes-0ccdfde273e4

Best regards,
-- 
Frank Oltmanns <frank@oltmanns.dev>

[PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Frank Oltmanns]

The Allwinner SoC's typically have an upper and lower limit for their
clocks' rates. Up until now, support for that has been implemented
separately for each clock type.

Implement that functionality in the sunxi-ng's common part making use of
the CCF rate liming capabilities, so that it is available for all clock
types.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
Cc: stable@vger.kernel.org
---
 drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
 drivers/clk/sunxi-ng/ccu_common.h |  3 +++
 2 files changed, 22 insertions(+)

diff --git a/drivers/clk/sunxi-ng/ccu_common.c b/drivers/clk/sunxi-ng/ccu_common.c
index 8babce55302f..ac0091b4ce24 100644
--- a/drivers/clk/sunxi-ng/ccu_common.c
+++ b/drivers/clk/sunxi-ng/ccu_common.c
@@ -44,6 +44,16 @@ bool ccu_is_better_rate(struct ccu_common *common,
 			unsigned long current_rate,
 			unsigned long best_rate)
 {
+	unsigned long min_rate, max_rate;
+
+	clk_hw_get_rate_range(&common->hw, &min_rate, &max_rate);
+
+	if (current_rate > max_rate)
+		return false;
+
+	if (current_rate < min_rate)
+		return false;
+
 	if (common->features & CCU_FEATURE_CLOSEST_RATE)
 		return abs(current_rate - target_rate) < abs(best_rate - target_rate);
 
@@ -122,6 +132,7 @@ static int sunxi_ccu_probe(struct sunxi_ccu *ccu, struct device *dev,
 
 	for (i = 0; i < desc->hw_clks->num ; i++) {
 		struct clk_hw *hw = desc->hw_clks->hws[i];
+		struct ccu_common *common = hw_to_ccu_common(hw);
 		const char *name;
 
 		if (!hw)
@@ -136,6 +147,14 @@ static int sunxi_ccu_probe(struct sunxi_ccu *ccu, struct device *dev,
 			pr_err("Couldn't register clock %d - %s\n", i, name);
 			goto err_clk_unreg;
 		}
+
+		if (common->max_rate)
+			clk_hw_set_rate_range(hw, common->min_rate,
+					      common->max_rate);
+		else
+			WARN(common->min_rate,
+			     "No max_rate, ignoring min_rate of clock %d - %s\n",
+			     i, name);
 	}
 
 	ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
diff --git a/drivers/clk/sunxi-ng/ccu_common.h b/drivers/clk/sunxi-ng/ccu_common.h
index 942a72c09437..329734f8cf42 100644
--- a/drivers/clk/sunxi-ng/ccu_common.h
+++ b/drivers/clk/sunxi-ng/ccu_common.h
@@ -31,6 +31,9 @@ struct ccu_common {
 	u16		lock_reg;
 	u32		prediv;
 
+	unsigned long	min_rate;
+	unsigned long	max_rate;
+
 	unsigned long	features;
 	spinlock_t	*lock;
 	struct clk_hw	hw;

-- 
2.44.0

[PATCH v4 2/5] clk: sunxi-ng: a64: Set minimum and maximum rate for PLL-MIPI

[Frank Oltmanns]

When the Allwinner A64's TCON0 searches the ideal rate for the connected
panel, it may happen that it requests a rate from its parent PLL-MIPI
which PLL-MIPI does not support.

This happens for example on the Olimex TERES-I laptop where TCON0
requests PLL-MIPI to change to a rate of several GHz which causes the
panel to stay blank. It also happens on the pinephone where a rate of
less than 500 MHz is requested which causes instabilities on some
phones.

Set the minimum and maximum rate of Allwinner A64's PLL-MIPI according
to the Allwinner User Manual.

Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux")
Reported-by: Diego Roversi <diegor@tiscali.it>
Closes: https://groups.google.com/g/linux-sunxi/c/Rh-Uqqa66bw
Tested-by: Diego Roversi <diegor@tiscali.it>
Cc: stable@vger.kernel.org
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
---
 drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
index 8951ffc14ff5..6a4b2b9ef30a 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -182,6 +182,8 @@ static struct ccu_nkm pll_mipi_clk = {
 					      &ccu_nkm_ops,
 					      CLK_SET_RATE_UNGATE | CLK_SET_RATE_PARENT),
 		.features	= CCU_FEATURE_CLOSEST_RATE,
+		.min_rate	= 500000000,
+		.max_rate	= 1400000000,
 	},
 };
 

-- 
2.44.0

[PATCH v4 3/5] clk: sunxi-ng: nkm: Support constraints on m/n ratio and parent rate

[Frank Oltmanns]

The Allwinner A64 manual lists the following constraints for the
PLL-MIPI clock:
 - M/N <= 3
 - (PLL_VIDEO0)/M >= 24MHz

The PLL-MIPI clock is implemented as ccu_nkm. Therefore, add support for
these constraints.

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
---
 drivers/clk/sunxi-ng/ccu_nkm.c | 21 +++++++++++++++++++++
 drivers/clk/sunxi-ng/ccu_nkm.h |  2 ++
 2 files changed, 23 insertions(+)

diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
index 853f84398e2b..1168d894d636 100644
--- a/drivers/clk/sunxi-ng/ccu_nkm.c
+++ b/drivers/clk/sunxi-ng/ccu_nkm.c
@@ -16,6 +16,20 @@ struct _ccu_nkm {
 	unsigned long	m, min_m, max_m;
 };
 
+static bool ccu_nkm_is_valid_rate(struct ccu_common *common, unsigned long parent,
+				  unsigned long n, unsigned long m)
+{
+	struct ccu_nkm *nkm = container_of(common, struct ccu_nkm, common);
+
+	if (nkm->max_m_n_ratio && (m > nkm->max_m_n_ratio * n))
+		return false;
+
+	if (nkm->min_parent_m_ratio && (parent < nkm->min_parent_m_ratio * m))
+		return false;
+
+	return true;
+}
+
 static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common,
 						       struct clk_hw *parent_hw,
 						       unsigned long *parent, unsigned long rate,
@@ -31,6 +45,10 @@ static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common
 				unsigned long tmp_rate, tmp_parent;
 
 				tmp_parent = clk_hw_round_rate(parent_hw, rate * _m / (_n * _k));
+
+				if (!ccu_nkm_is_valid_rate(common, tmp_parent, _n, _m))
+					continue;
+
 				tmp_rate = tmp_parent * _n * _k / _m;
 
 				if (ccu_is_better_rate(common, rate, tmp_rate, best_rate) ||
@@ -64,6 +82,9 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate,
 	for (_k = nkm->min_k; _k <= nkm->max_k; _k++) {
 		for (_n = nkm->min_n; _n <= nkm->max_n; _n++) {
 			for (_m = nkm->min_m; _m <= nkm->max_m; _m++) {
+				if (!ccu_nkm_is_valid_rate(common, parent, _n, _m))
+					continue;
+
 				unsigned long tmp_rate;
 
 				tmp_rate = parent * _n * _k / _m;
diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h
index 6601defb3f38..c409212ee40e 100644
--- a/drivers/clk/sunxi-ng/ccu_nkm.h
+++ b/drivers/clk/sunxi-ng/ccu_nkm.h
@@ -27,6 +27,8 @@ struct ccu_nkm {
 	struct ccu_mux_internal	mux;
 
 	unsigned int		fixed_post_div;
+	unsigned long		max_m_n_ratio;
+	unsigned long		min_parent_m_ratio;
 
 	struct ccu_common	common;
 };

-- 
2.44.0

[PATCH v4 4/5] clk: sunxi-ng: a64: Add constraints on PLL-MIPI's n/m ratio and parent rate

[Frank Oltmanns]

The Allwinner A64 manual lists the following constraints for the
PLL-MIPI clock:
 - M/N <= 3
 - (PLL_VIDEO0)/M >= 24MHz

Use these constraints.

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
---
 drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
index 6a4b2b9ef30a..07796c79a23e 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -171,11 +171,13 @@ static struct ccu_nkm pll_mipi_clk = {
 	 * user manual, and by experiments the PLL doesn't work without
 	 * these bits toggled.
 	 */
-	.enable		= BIT(31) | BIT(23) | BIT(22),
-	.lock		= BIT(28),
-	.n		= _SUNXI_CCU_MULT(8, 4),
-	.k		= _SUNXI_CCU_MULT_MIN(4, 2, 2),
-	.m		= _SUNXI_CCU_DIV(0, 4),
+	.enable			= BIT(31) | BIT(23) | BIT(22),
+	.lock			= BIT(28),
+	.n			= _SUNXI_CCU_MULT(8, 4),
+	.k			= _SUNXI_CCU_MULT_MIN(4, 2, 2),
+	.m			= _SUNXI_CCU_DIV(0, 4),
+	.max_m_n_ratio		= 3,
+	.min_parent_m_ratio	= 24000000,
 	.common		= {
 		.reg		= 0x040,
 		.hw.init	= CLK_HW_INIT("pll-mipi", "pll-video0",

-- 
2.44.0

[PATCH v4 5/5] arm64: dts: allwinner: a64: Run GPU at 432 MHz

[Frank Oltmanns]

The Allwinner A64's GPU has currently three operating points. However,
the BSP runs the GPU fixed at 432 MHz. In addition, at least one of the
devices using that SoC - the pinephone - shows unstabilities (see link)
that can be circumvented by running the GPU at a fixed rate.

Therefore, remove the other two operating points from the GPU OPP table,
so that the GPU runs at a fixed rate of 432 MHz.

Link: https://gitlab.com/postmarketOS/pmaports/-/issues/805
Acked-by: Erico Nunes <nunes.erico@gmail.com>
Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 57ac18738c99..c810380aab6d 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -107,14 +107,6 @@ de: display-engine {
 	gpu_opp_table: opp-table-gpu {
 		compatible = "operating-points-v2";
 
-		opp-120000000 {
-			opp-hz = /bits/ 64 <120000000>;
-		};
-
-		opp-312000000 {
-			opp-hz = /bits/ 64 <312000000>;
-		};
-
 		opp-432000000 {
 			opp-hz = /bits/ 64 <432000000>;
 		};

-- 
2.44.0

Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Jernej Škrabec]

Dne nedelja, 10. marec 2024 ob 14:21:11 CET je Frank Oltmanns napisal(a):
> The Allwinner SoC's typically have an upper and lower limit for their
> clocks' rates. Up until now, support for that has been implemented
> separately for each clock type.
> 
> Implement that functionality in the sunxi-ng's common part making use of
> the CCF rate liming capabilities, so that it is available for all clock
> types.
> 
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
> Cc: stable@vger.kernel.org

This looks pretty nice now.

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> ---
>  drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
>  drivers/clk/sunxi-ng/ccu_common.h |  3 +++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/clk/sunxi-ng/ccu_common.c b/drivers/clk/sunxi-ng/ccu_common.c
> index 8babce55302f..ac0091b4ce24 100644
> --- a/drivers/clk/sunxi-ng/ccu_common.c
> +++ b/drivers/clk/sunxi-ng/ccu_common.c
> @@ -44,6 +44,16 @@ bool ccu_is_better_rate(struct ccu_common *common,
>  			unsigned long current_rate,
>  			unsigned long best_rate)
>  {
> +	unsigned long min_rate, max_rate;
> +
> +	clk_hw_get_rate_range(&common->hw, &min_rate, &max_rate);
> +
> +	if (current_rate > max_rate)
> +		return false;
> +
> +	if (current_rate < min_rate)
> +		return false;
> +
>  	if (common->features & CCU_FEATURE_CLOSEST_RATE)
>  		return abs(current_rate - target_rate) < abs(best_rate - target_rate);
>  
> @@ -122,6 +132,7 @@ static int sunxi_ccu_probe(struct sunxi_ccu *ccu, struct device *dev,
>  
>  	for (i = 0; i < desc->hw_clks->num ; i++) {
>  		struct clk_hw *hw = desc->hw_clks->hws[i];
> +		struct ccu_common *common = hw_to_ccu_common(hw);
>  		const char *name;
>  
>  		if (!hw)
> @@ -136,6 +147,14 @@ static int sunxi_ccu_probe(struct sunxi_ccu *ccu, struct device *dev,
>  			pr_err("Couldn't register clock %d - %s\n", i, name);
>  			goto err_clk_unreg;
>  		}
> +
> +		if (common->max_rate)
> +			clk_hw_set_rate_range(hw, common->min_rate,
> +					      common->max_rate);
> +		else
> +			WARN(common->min_rate,
> +			     "No max_rate, ignoring min_rate of clock %d - %s\n",
> +			     i, name);
>  	}
>  
>  	ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
> diff --git a/drivers/clk/sunxi-ng/ccu_common.h b/drivers/clk/sunxi-ng/ccu_common.h
> index 942a72c09437..329734f8cf42 100644
> --- a/drivers/clk/sunxi-ng/ccu_common.h
> +++ b/drivers/clk/sunxi-ng/ccu_common.h
> @@ -31,6 +31,9 @@ struct ccu_common {
>  	u16		lock_reg;
>  	u32		prediv;
>  
> +	unsigned long	min_rate;
> +	unsigned long	max_rate;
> +
>  	unsigned long	features;
>  	spinlock_t	*lock;
>  	struct clk_hw	hw;
> 
> 

Re: [PATCH v4 2/5] clk: sunxi-ng: a64: Set minimum and maximum rate for PLL-MIPI

[Jernej Škrabec]

Dne nedelja, 10. marec 2024 ob 14:21:12 CET je Frank Oltmanns napisal(a):
> When the Allwinner A64's TCON0 searches the ideal rate for the connected
> panel, it may happen that it requests a rate from its parent PLL-MIPI
> which PLL-MIPI does not support.
> 
> This happens for example on the Olimex TERES-I laptop where TCON0
> requests PLL-MIPI to change to a rate of several GHz which causes the
> panel to stay blank. It also happens on the pinephone where a rate of
> less than 500 MHz is requested which causes instabilities on some
> phones.
> 
> Set the minimum and maximum rate of Allwinner A64's PLL-MIPI according
> to the Allwinner User Manual.
> 
> Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux")
> Reported-by: Diego Roversi <diegor@tiscali.it>
> Closes: https://groups.google.com/g/linux-sunxi/c/Rh-Uqqa66bw
> Tested-by: Diego Roversi <diegor@tiscali.it>
> Cc: stable@vger.kernel.org
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> ---
>  drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> index 8951ffc14ff5..6a4b2b9ef30a 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> @@ -182,6 +182,8 @@ static struct ccu_nkm pll_mipi_clk = {
>  					      &ccu_nkm_ops,
>  					      CLK_SET_RATE_UNGATE | CLK_SET_RATE_PARENT),
>  		.features	= CCU_FEATURE_CLOSEST_RATE,
> +		.min_rate	= 500000000,
> +		.max_rate	= 1400000000,
>  	},
>  };
>  
> 
> 

Re: [PATCH v4 5/5] arm64: dts: allwinner: a64: Run GPU at 432 MHz

[Jernej Škrabec]

Dne nedelja, 10. marec 2024 ob 14:21:15 CET je Frank Oltmanns napisal(a):
> The Allwinner A64's GPU has currently three operating points. However,
> the BSP runs the GPU fixed at 432 MHz. In addition, at least one of the
> devices using that SoC - the pinephone - shows unstabilities (see link)
> that can be circumvented by running the GPU at a fixed rate.
> 
> Therefore, remove the other two operating points from the GPU OPP table,
> so that the GPU runs at a fixed rate of 432 MHz.
> 
> Link: https://gitlab.com/postmarketOS/pmaports/-/issues/805
> Acked-by: Erico Nunes <nunes.erico@gmail.com>
> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>

Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> ---
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 57ac18738c99..c810380aab6d 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -107,14 +107,6 @@ de: display-engine {
>  	gpu_opp_table: opp-table-gpu {
>  		compatible = "operating-points-v2";
>  
> -		opp-120000000 {
> -			opp-hz = /bits/ 64 <120000000>;
> -		};
> -
> -		opp-312000000 {
> -			opp-hz = /bits/ 64 <312000000>;
> -		};
> -
>  		opp-432000000 {
>  			opp-hz = /bits/ 64 <432000000>;
>  		};
> 
> 

Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Maxime Ripard]

On Sun, 10 Mar 2024 14:21:11 +0100, Frank Oltmanns wrote:
> The Allwinner SoC's typically have an upper and lower limit for their
> clocks' rates. Up until now, support for that has been implemented
> separately for each clock type.
> 
> Implement that functionality in the sunxi-ng's common part making use of
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [PATCH v4 0/5] Pinephone video out fixes (flipping between two frames)

[Frank Oltmanns]

Dear clk and sunxi-ng maintainers,

Patches 1-4 have been reviewed and there are no pending issues. If there
is something else you need me to do to get this applied, please let me
know.

Thanks,
  Frank

On 2024-03-10 at 14:21:10 +0100, Frank Oltmanns <frank@oltmanns.dev> wrote:
> On some pinephones the video output sometimes freezes (flips between two
> frames) [1]. It seems to be that the reason for this behaviour is that
> PLL-MIPI is outside its limits, and the GPU is not running at a fixed
> rate.
>
> In this patch series I propose the following changes:
>   1. sunxi-ng: Adhere to the following constraints given in the
>      Allwinner A64 Manual regarding PLL-MIPI:
>       * M/N <= 3
>       * (PLL_VIDEO0)/M >= 24MHz
>       * 500MHz <= clockrate <= 1400MHz
>
>   2. Remove two operating points from the A64 DTS OPPs, so that the GPU
>      runs at a fixed rate of 432 MHz.
>
> Note, that when pinning the GPU to 432 MHz the issue [1] completely
> disappears for me. I've searched the BSP and could not find any
> indication that supports the idea of having the three OPPs. The only
> frequency I found in the BPSs for A64 is 432 MHz, which has also proven
> stable for me.
>
> I very much appreciate your feedback!
>
> [1] https://gitlab.com/postmarketOS/pmaports/-/issues/805
>
> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
> ---
> Changes in v4:
> - sunxi-ng: common: Address review comments.
> - Link to v3: https://lore.kernel.org/r/20240304-pinephone-pll-fixes-v3-0-94ab828f269a@oltmanns.dev
>
> Changes in v3:
> - dts: Pin GPU to 432 MHz.
> - nkm and a64: Move minimum and maximum rate handling to the common part
>   of the sunxi-ng driver.
> - Removed st7703 patch from series.
> - Link to v2: https://lore.kernel.org/r/20240205-pinephone-pll-fixes-v2-0-96a46a2d8c9b@oltmanns.dev
>
> Changes in v2:
> - dts: Increase minimum GPU frequency to 192 MHz.
> - nkm and a64: Add minimum and maximum rate for PLL-MIPI.
> - nkm: Use the same approach for skipping invalid rates in
>   ccu_nkm_find_best() as in ccu_nkm_find_best_with_parent_adj().
> - nkm: Improve names for ratio struct members and hence get rid of
>   describing comments.
> - nkm and a64: Correct description in the commit messages: M/N <= 3
> - Remove patches for nm as they were not needed.
> - st7703: Rework the commit message to cover more background for the
>   change.
> - Link to v1: https://lore.kernel.org/r/20231218-pinephone-pll-fixes-v1-0-e238b6ed6dc1@oltmanns.dev
>
> ---
> Frank Oltmanns (5):
>       clk: sunxi-ng: common: Support minimum and maximum rate
>       clk: sunxi-ng: a64: Set minimum and maximum rate for PLL-MIPI
>       clk: sunxi-ng: nkm: Support constraints on m/n ratio and parent rate
>       clk: sunxi-ng: a64: Add constraints on PLL-MIPI's n/m ratio and parent rate
>       arm64: dts: allwinner: a64: Run GPU at 432 MHz
>
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |  8 --------
>  drivers/clk/sunxi-ng/ccu-sun50i-a64.c         | 14 +++++++++-----
>  drivers/clk/sunxi-ng/ccu_common.c             | 19 +++++++++++++++++++
>  drivers/clk/sunxi-ng/ccu_common.h             |  3 +++
>  drivers/clk/sunxi-ng/ccu_nkm.c                | 21 +++++++++++++++++++++
>  drivers/clk/sunxi-ng/ccu_nkm.h                |  2 ++
>  6 files changed, 54 insertions(+), 13 deletions(-)
> ---
> base-commit: dcb6c8ee6acc6c347caec1e73fb900c0f4ff9806
> change-id: 20231218-pinephone-pll-fixes-0ccdfde273e4
>
> Best regards,

Re: [PATCH v4 0/5] Pinephone video out fixes (flipping between two frames)

[Stephen Boyd]

Quoting Frank Oltmanns (2024-04-03 08:31:47)
> Dear clk and sunxi-ng maintainers,
> 
> Patches 1-4 have been reviewed and there are no pending issues. If there
> is something else you need me to do to get this applied, please let me
> know.
> 

I'm assuming sunxi maintainers will pick up the clk patches and send
them to clk tree in a PR.

Re: [PATCH v4 0/5] Pinephone video out fixes (flipping between two frames)

[Jernej Škrabec]

Dne sreda, 3. april 2024 ob 17:31:47 GMT +2 je Frank Oltmanns napisal(a):
> Dear clk and sunxi-ng maintainers,
> 
> Patches 1-4 have been reviewed and there are no pending issues. If there
> is something else you need me to do to get this applied, please let me
> know.

Sorry for late patch merge. Patch 1-2 are applied as a fix to 6.9, the rest
will go to 6.10.

Best regards,
Jernej

> 
> Thanks,
>   Frank
> 
> On 2024-03-10 at 14:21:10 +0100, Frank Oltmanns <frank@oltmanns.dev> wrote:
> > On some pinephones the video output sometimes freezes (flips between two
> > frames) [1]. It seems to be that the reason for this behaviour is that
> > PLL-MIPI is outside its limits, and the GPU is not running at a fixed
> > rate.
> >
> > In this patch series I propose the following changes:
> >   1. sunxi-ng: Adhere to the following constraints given in the
> >      Allwinner A64 Manual regarding PLL-MIPI:
> >       * M/N <= 3
> >       * (PLL_VIDEO0)/M >= 24MHz
> >       * 500MHz <= clockrate <= 1400MHz
> >
> >   2. Remove two operating points from the A64 DTS OPPs, so that the GPU
> >      runs at a fixed rate of 432 MHz.
> >
> > Note, that when pinning the GPU to 432 MHz the issue [1] completely
> > disappears for me. I've searched the BSP and could not find any
> > indication that supports the idea of having the three OPPs. The only
> > frequency I found in the BPSs for A64 is 432 MHz, which has also proven
> > stable for me.
> >
> > I very much appreciate your feedback!
> >
> > [1] https://gitlab.com/postmarketOS/pmaports/-/issues/805
> >
> > Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
> > ---
> > Changes in v4:
> > - sunxi-ng: common: Address review comments.
> > - Link to v3: https://lore.kernel.org/r/20240304-pinephone-pll-fixes-v3-0-94ab828f269a@oltmanns.dev
> >
> > Changes in v3:
> > - dts: Pin GPU to 432 MHz.
> > - nkm and a64: Move minimum and maximum rate handling to the common part
> >   of the sunxi-ng driver.
> > - Removed st7703 patch from series.
> > - Link to v2: https://lore.kernel.org/r/20240205-pinephone-pll-fixes-v2-0-96a46a2d8c9b@oltmanns.dev
> >
> > Changes in v2:
> > - dts: Increase minimum GPU frequency to 192 MHz.
> > - nkm and a64: Add minimum and maximum rate for PLL-MIPI.
> > - nkm: Use the same approach for skipping invalid rates in
> >   ccu_nkm_find_best() as in ccu_nkm_find_best_with_parent_adj().
> > - nkm: Improve names for ratio struct members and hence get rid of
> >   describing comments.
> > - nkm and a64: Correct description in the commit messages: M/N <= 3
> > - Remove patches for nm as they were not needed.
> > - st7703: Rework the commit message to cover more background for the
> >   change.
> > - Link to v1: https://lore.kernel.org/r/20231218-pinephone-pll-fixes-v1-0-e238b6ed6dc1@oltmanns.dev
> >
> > ---
> > Frank Oltmanns (5):
> >       clk: sunxi-ng: common: Support minimum and maximum rate
> >       clk: sunxi-ng: a64: Set minimum and maximum rate for PLL-MIPI
> >       clk: sunxi-ng: nkm: Support constraints on m/n ratio and parent rate
> >       clk: sunxi-ng: a64: Add constraints on PLL-MIPI's n/m ratio and parent rate
> >       arm64: dts: allwinner: a64: Run GPU at 432 MHz
> >
> >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |  8 --------
> >  drivers/clk/sunxi-ng/ccu-sun50i-a64.c         | 14 +++++++++-----
> >  drivers/clk/sunxi-ng/ccu_common.c             | 19 +++++++++++++++++++
> >  drivers/clk/sunxi-ng/ccu_common.h             |  3 +++
> >  drivers/clk/sunxi-ng/ccu_nkm.c                | 21 +++++++++++++++++++++
> >  drivers/clk/sunxi-ng/ccu_nkm.h                |  2 ++
> >  6 files changed, 54 insertions(+), 13 deletions(-)
> > ---
> > base-commit: dcb6c8ee6acc6c347caec1e73fb900c0f4ff9806
> > change-id: 20231218-pinephone-pll-fixes-0ccdfde273e4
> >
> > Best regards,
> 

Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Måns Rullgård]

Frank Oltmanns <frank@oltmanns.dev> writes:

> The Allwinner SoC's typically have an upper and lower limit for their
> clocks' rates. Up until now, support for that has been implemented
> separately for each clock type.
>
> Implement that functionality in the sunxi-ng's common part making use of
> the CCF rate liming capabilities, so that it is available for all clock
> types.
>
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
> Cc: stable@vger.kernel.org
> ---
>  drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
>  drivers/clk/sunxi-ng/ccu_common.h |  3 +++
>  2 files changed, 22 insertions(+)

This just landed in 6.6 stable, and it broke HDMI output on an A20 based
device, the clocks ending up all wrong as seen in this diff of
/sys/kernel/debug/clk/clk_summary:

@@ -70,16 +71,14 @@
           apb1-i2c0                  0       0        0        24000000    0   
                                                                                
        pll-gpu                       0       0        0        1200000000  0   
-       pll-video1                    3       3        1        159000000   0   
+       pll-video1                    2       2        1        159000000   0   
           hdmi                       1       1        0        39750000    0   
                                                                                
           tcon0-ch1-sclk2            1       1        1        39750000    0   
              tcon0-ch1-sclk1         1       1        1        39750000    0   
                                                                                
-          pll-video1-2x              1       1        0        318000000   0   
+          pll-video1-2x              0       0        0        318000000   0   
                                                                                
-             hdmi-tmds               2       2        0        39750000    0   
-                hdmi-ddc             1       1        0        1987500     0   
        pll-periph-base               2       2        0        1200000000  0   
           mbus                       1       1        0        300000000   0   
           pll-periph-sata            0       0        0        100000000   0   
@@ -199,7 +198,7 @@
                                                                                
           ace                        0       0        0        384000000   0   
           ve                         0       0        0        384000000   0   
-       pll-video0                    4       4        2        297000000   0   
+       pll-video0                    5       5        2        297000000   0   
           hdmi1                      0       0        0        297000000   0   
           tcon1-ch1-sclk2            0       0        0        297000000   0   
              tcon1-ch1-sclk1         0       0        0        297000000   0   
@@ -222,8 +221,10 @@
                                                                                
           de-be0                     1       1        1        297000000   0   
                                                                                
-          pll-video0-2x              0       0        0        594000000   0   
+          pll-video0-2x              1       1        0        594000000   0   
                                                                                
+             hdmi-tmds               2       2        0        594000000   0   
+                hdmi-ddc             1       1        0        29700000    0   
        pll-audio-base                0       0        0        1500000     0   
           pll-audio-8x               0       0        0        3000000     0   
              i2s2                    0       0        0        3000000     0   

Reverting this commit makes it work again.

> diff --git a/drivers/clk/sunxi-ng/ccu_common.c b/drivers/clk/sunxi-ng/ccu_common.c
> index 8babce55302f..ac0091b4ce24 100644
> --- a/drivers/clk/sunxi-ng/ccu_common.c
> +++ b/drivers/clk/sunxi-ng/ccu_common.c
> @@ -44,6 +44,16 @@ bool ccu_is_better_rate(struct ccu_common *common,
>  			unsigned long current_rate,
>  			unsigned long best_rate)
>  {
> +	unsigned long min_rate, max_rate;
> +
> +	clk_hw_get_rate_range(&common->hw, &min_rate, &max_rate);
> +
> +	if (current_rate > max_rate)
> +		return false;
> +
> +	if (current_rate < min_rate)
> +		return false;
> +
>  	if (common->features & CCU_FEATURE_CLOSEST_RATE)
>  		return abs(current_rate - target_rate) < abs(best_rate - target_rate);
>
> @@ -122,6 +132,7 @@ static int sunxi_ccu_probe(struct sunxi_ccu *ccu, struct device *dev,
>
>  	for (i = 0; i < desc->hw_clks->num ; i++) {
>  		struct clk_hw *hw = desc->hw_clks->hws[i];
> +		struct ccu_common *common = hw_to_ccu_common(hw);
>  		const char *name;
>
>  		if (!hw)
> @@ -136,6 +147,14 @@ static int sunxi_ccu_probe(struct sunxi_ccu *ccu, struct device *dev,
>  			pr_err("Couldn't register clock %d - %s\n", i, name);
>  			goto err_clk_unreg;
>  		}
> +
> +		if (common->max_rate)
> +			clk_hw_set_rate_range(hw, common->min_rate,
> +					      common->max_rate);
> +		else
> +			WARN(common->min_rate,
> +			     "No max_rate, ignoring min_rate of clock %d - %s\n",
> +			     i, name);
>  	}
>
>  	ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
> diff --git a/drivers/clk/sunxi-ng/ccu_common.h b/drivers/clk/sunxi-ng/ccu_common.h
> index 942a72c09437..329734f8cf42 100644
> --- a/drivers/clk/sunxi-ng/ccu_common.h
> +++ b/drivers/clk/sunxi-ng/ccu_common.h
> @@ -31,6 +31,9 @@ struct ccu_common {
>  	u16		lock_reg;
>  	u32		prediv;
>
> +	unsigned long	min_rate;
> +	unsigned long	max_rate;
> +
>  	unsigned long	features;
>  	spinlock_t	*lock;
>  	struct clk_hw	hw;
>
> -- 
>
> 2.44.0
>

-- 
Måns Rullgård

Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Frank Oltmanns]

Hi Måns,

21.05.2024 15:43:10 Måns Rullgård <mans@mansr.com>:

> Frank Oltmanns <frank@oltmanns.dev> writes:
>
>> The Allwinner SoC's typically have an upper and lower limit for their
>> clocks' rates. Up until now, support for that has been implemented
>> separately for each clock type.
>>
>> Implement that functionality in the sunxi-ng's common part making use of
>> the CCF rate liming capabilities, so that it is available for all clock
>> types.
>>
>> Suggested-by: Maxime Ripard <mripard@kernel.org>
>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>> Cc: stable@vger.kernel.org
>> ---
>> drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
>> drivers/clk/sunxi-ng/ccu_common.h |  3 +++
>> 2 files changed, 22 insertions(+)
>
> This just landed in 6.6 stable, and it broke HDMI output on an A20 based
> device, the clocks ending up all wrong as seen in this diff of
> /sys/kernel/debug/clk/clk_summary:
>
> @@ -70,16 +71,14 @@
>            apb1-i2c0                  0       0        0        24000000    0  
>                                                                                
>         pll-gpu                       0       0        0        1200000000  0  
> -       pll-video1                    3       3        1        159000000   0  
> +       pll-video1                    2       2        1        159000000   0  
>            hdmi                       1       1        0        39750000    0  
>                                                                                
>            tcon0-ch1-sclk2            1       1        1        39750000    0  
>               tcon0-ch1-sclk1         1       1        1        39750000    0  
>                                                                                
> -          pll-video1-2x              1       1        0        318000000   0  
> +          pll-video1-2x              0       0        0        318000000   0  
>                                                                                
> -             hdmi-tmds               2       2        0        39750000    0  
> -                hdmi-ddc             1       1        0        1987500     0  
>         pll-periph-base               2       2        0        1200000000  0  
>            mbus                       1       1        0        300000000   0  
>            pll-periph-sata            0       0        0        100000000   0  
> @@ -199,7 +198,7 @@
>                                                                                
>            ace                        0       0        0        384000000   0  
>            ve                         0       0        0        384000000   0  
> -       pll-video0                    4       4        2        297000000   0  
> +       pll-video0                    5       5        2        297000000   0  
>            hdmi1                      0       0        0        297000000   0  
>            tcon1-ch1-sclk2            0       0        0        297000000   0  
>               tcon1-ch1-sclk1         0       0        0        297000000   0  
> @@ -222,8 +221,10 @@
>                                                                                
>            de-be0                     1       1        1        297000000   0  
>                                                                                
> -          pll-video0-2x              0       0        0        594000000   0  
> +          pll-video0-2x              1       1        0        594000000   0  
>                                                                                
> +             hdmi-tmds               2       2        0        594000000   0  
> +                hdmi-ddc             1       1        0        29700000    0  
>         pll-audio-base                0       0        0        1500000     0  
>            pll-audio-8x               0       0        0        3000000     0  
>               i2s2                    0       0        0        3000000     0  
>
> Reverting this commit makes it work again.

Thank you for your detailed report!

I've had a first look at hdmi-tmds and hdmi-ddc, and neither seems to be calling ccu_is_better_rate() in their determine_rate() functions. Their parents have the exact same rates in your diff, so, my current working assumption is that they can't be the cause either.

I'll have a more detailed look over the weekend. Until then, if anyone has some ideas where I should have a look next, please share your thoughts.

Best regards,
  Frank

>
>> diff --git a/drivers/clk/sunxi-ng/ccu_common.c b/drivers/clk/sunxi-ng/ccu_common.c
>> index 8babce55302f..ac0091b4ce24 100644
>> --- a/drivers/clk/sunxi-ng/ccu_common.c
>> +++ b/drivers/clk/sunxi-ng/ccu_common.c
>> @@ -44,6 +44,16 @@ bool ccu_is_better_rate(struct ccu_common *common,
>>             unsigned long current_rate,
>>             unsigned long best_rate)
>> {
>> +   unsigned long min_rate, max_rate;
>> +
>> +   clk_hw_get_rate_range(&common->hw, &min_rate, &max_rate);
>> +
>> +   if (current_rate > max_rate)
>> +       return false;
>> +
>> +   if (current_rate < min_rate)
>> +       return false;
>> +
>>     if (common->features & CCU_FEATURE_CLOSEST_RATE)
>>         return abs(current_rate - target_rate) < abs(best_rate - target_rate);
>>
>> @@ -122,6 +132,7 @@ static int sunxi_ccu_probe(struct sunxi_ccu *ccu, struct device *dev,
>>
>>     for (i = 0; i < desc->hw_clks->num ; i++) {
>>         struct clk_hw *hw = desc->hw_clks->hws[i];
>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>>         const char *name;
>>
>>         if (!hw)
>> @@ -136,6 +147,14 @@ static int sunxi_ccu_probe(struct sunxi_ccu *ccu, struct device *dev,
>>             pr_err("Couldn't register clock %d - %s\n", i, name);
>>             goto err_clk_unreg;
>>         }
>> +
>> +       if (common->max_rate)
>> +           clk_hw_set_rate_range(hw, common->min_rate,
>> +                         common->max_rate);
>> +       else
>> +           WARN(common->min_rate,
>> +                "No max_rate, ignoring min_rate of clock %d - %s\n",
>> +                i, name);
>>     }
>>
>>     ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
>> diff --git a/drivers/clk/sunxi-ng/ccu_common.h b/drivers/clk/sunxi-ng/ccu_common.h
>> index 942a72c09437..329734f8cf42 100644
>> --- a/drivers/clk/sunxi-ng/ccu_common.h
>> +++ b/drivers/clk/sunxi-ng/ccu_common.h
>> @@ -31,6 +31,9 @@ struct ccu_common {
>>     u16     lock_reg;
>>     u32     prediv;
>>
>> +   unsigned long   min_rate;
>> +   unsigned long   max_rate;
>> +
>>     unsigned long   features;
>>     spinlock_t  *lock;
>>     struct clk_hw   hw;
>>
>> --
>>
>> 2.44.0
>>
>
> --
> Måns Rullgård

Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Måns Rullgård]

Frank Oltmanns <frank@oltmanns.dev> writes:

> Hi Måns,
>
> 21.05.2024 15:43:10 Måns Rullgård <mans@mansr.com>:
>
>> Frank Oltmanns <frank@oltmanns.dev> writes:
>>
>>> The Allwinner SoC's typically have an upper and lower limit for their
>>> clocks' rates. Up until now, support for that has been implemented
>>> separately for each clock type.
>>>
>>> Implement that functionality in the sunxi-ng's common part making use of
>>> the CCF rate liming capabilities, so that it is available for all clock
>>> types.
>>>
>>> Suggested-by: Maxime Ripard <mripard@kernel.org>
>>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>>> Cc: stable@vger.kernel.org
>>> ---
>>> drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
>>> drivers/clk/sunxi-ng/ccu_common.h |  3 +++
>>> 2 files changed, 22 insertions(+)
>>
>> This just landed in 6.6 stable, and it broke HDMI output on an A20 based
>> device, the clocks ending up all wrong as seen in this diff of
>> /sys/kernel/debug/clk/clk_summary:
>>
>> @@ -70,16 +71,14 @@
>>            apb1-i2c0                  0       0        0        24000000    0
>>
>>         pll-gpu                       0       0        0        1200000000  0
>> -       pll-video1                    3       3        1        159000000   0
>> +       pll-video1                    2       2        1        159000000   0
>>            hdmi                       1       1        0        39750000    0
>>
>>            tcon0-ch1-sclk2            1       1        1        39750000    0
>>               tcon0-ch1-sclk1         1       1        1        39750000    0
>>
>> -          pll-video1-2x              1       1        0        318000000   0
>> +          pll-video1-2x              0       0        0        318000000   0
>>
>> -             hdmi-tmds               2       2        0        39750000    0
>> -                hdmi-ddc             1       1        0        1987500     0
>>         pll-periph-base               2       2        0        1200000000  0
>>            mbus                       1       1        0        300000000   0
>>            pll-periph-sata            0       0        0        100000000   0
>> @@ -199,7 +198,7 @@
>>
>>            ace                        0       0        0        384000000   0
>>            ve                         0       0        0        384000000   0
>> -       pll-video0                    4       4        2        297000000   0
>> +       pll-video0                    5       5        2        297000000   0
>>            hdmi1                      0       0        0        297000000   0
>>            tcon1-ch1-sclk2            0       0        0        297000000   0
>>               tcon1-ch1-sclk1         0       0        0        297000000   0
>> @@ -222,8 +221,10 @@
>>
>>            de-be0                     1       1        1        297000000   0
>>
>> -          pll-video0-2x              0       0        0        594000000   0
>> +          pll-video0-2x              1       1        0        594000000   0
>>
>> +             hdmi-tmds               2       2        0        594000000   0
>> +                hdmi-ddc             1       1        0        29700000    0
>>         pll-audio-base                0       0        0        1500000     0
>>            pll-audio-8x               0       0        0        3000000     0
>>               i2s2                    0       0        0        3000000     0
>>
>> Reverting this commit makes it work again.
>
> Thank you for your detailed report!
>
> I've had a first look at hdmi-tmds and hdmi-ddc, and neither seems to
> be calling ccu_is_better_rate() in their determine_rate()
> functions. Their parents have the exact same rates in your diff, so,
> my current working assumption is that they can't be the cause either.
>
> I'll have a more detailed look over the weekend. Until then, if anyone
> has some ideas where I should have a look next, please share your
> thoughts.

In case it's relevant, this system doesn't use the HDMI DDC, the
physical DDC pins being connected to a different I2C adapter for
various reasons.

From the clk_summary diff, I see a few things:

1. hdmi-tmds has changed parent from pll-video1-2x to pll-video0-2x.
2. The ratio of hdmi-tmds to its parent has changed from 1/8 to 1.
3. The resulting rate bears no relation to the pixel clock from EDID.

I tried kernel 6.9.1 as well, and that doesn't work either.  I'll keep
digging and try to narrow it down.

>>> diff --git a/drivers/clk/sunxi-ng/ccu_common.c b/drivers/clk/sunxi-ng/ccu_common.c
>>> index 8babce55302f..ac0091b4ce24 100644
>>> --- a/drivers/clk/sunxi-ng/ccu_common.c
>>> +++ b/drivers/clk/sunxi-ng/ccu_common.c
>>> @@ -44,6 +44,16 @@ bool ccu_is_better_rate(struct ccu_common *common,
>>>             unsigned long current_rate,
>>>             unsigned long best_rate)
>>> {
>>> +   unsigned long min_rate, max_rate;
>>> +
>>> +   clk_hw_get_rate_range(&common->hw, &min_rate, &max_rate);
>>> +
>>> +   if (current_rate > max_rate)
>>> +       return false;
>>> +
>>> +   if (current_rate < min_rate)
>>> +       return false;
>>> +
>>>     if (common->features & CCU_FEATURE_CLOSEST_RATE)
>>>         return abs(current_rate - target_rate) < abs(best_rate - target_rate);
>>>
>>> @@ -122,6 +132,7 @@ static int sunxi_ccu_probe(struct sunxi_ccu *ccu, struct device *dev,
>>>
>>>     for (i = 0; i < desc->hw_clks->num ; i++) {
>>>         struct clk_hw *hw = desc->hw_clks->hws[i];
>>> +       struct ccu_common *common = hw_to_ccu_common(hw);
>>>         const char *name;
>>>
>>>         if (!hw)
>>> @@ -136,6 +147,14 @@ static int sunxi_ccu_probe(struct sunxi_ccu *ccu, struct device *dev,
>>>             pr_err("Couldn't register clock %d - %s\n", i, name);
>>>             goto err_clk_unreg;
>>>         }
>>> +
>>> +       if (common->max_rate)
>>> +           clk_hw_set_rate_range(hw, common->min_rate,
>>> +                         common->max_rate);
>>> +       else
>>> +           WARN(common->min_rate,
>>> +                "No max_rate, ignoring min_rate of clock %d - %s\n",
>>> +                i, name);
>>>     }
>>>
>>>     ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
>>> diff --git a/drivers/clk/sunxi-ng/ccu_common.h b/drivers/clk/sunxi-ng/ccu_common.h
>>> index 942a72c09437..329734f8cf42 100644
>>> --- a/drivers/clk/sunxi-ng/ccu_common.h
>>> +++ b/drivers/clk/sunxi-ng/ccu_common.h
>>> @@ -31,6 +31,9 @@ struct ccu_common {
>>>     u16     lock_reg;
>>>     u32     prediv;
>>>
>>> +   unsigned long   min_rate;
>>> +   unsigned long   max_rate;
>>> +
>>>     unsigned long   features;
>>>     spinlock_t  *lock;
>>>     struct clk_hw   hw;
>>>
>>> --
>>>
>>> 2.44.0
>>>
>>
>> --
>> Måns Rullgård
>

-- 
Måns Rullgård

Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Måns Rullgård]

Måns Rullgård <mans@mansr.com> writes:

> Frank Oltmanns <frank@oltmanns.dev> writes:
>
>> Hi Måns,
>>
>> 21.05.2024 15:43:10 Måns Rullgård <mans@mansr.com>:
>>
>>> Frank Oltmanns <frank@oltmanns.dev> writes:
>>>
>>>> The Allwinner SoC's typically have an upper and lower limit for their
>>>> clocks' rates. Up until now, support for that has been implemented
>>>> separately for each clock type.
>>>>
>>>> Implement that functionality in the sunxi-ng's common part making use of
>>>> the CCF rate liming capabilities, so that it is available for all clock
>>>> types.
>>>>
>>>> Suggested-by: Maxime Ripard <mripard@kernel.org>
>>>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>> drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
>>>> drivers/clk/sunxi-ng/ccu_common.h |  3 +++
>>>> 2 files changed, 22 insertions(+)
>>>
>>> This just landed in 6.6 stable, and it broke HDMI output on an A20 based
>>> device, the clocks ending up all wrong as seen in this diff of
>>> /sys/kernel/debug/clk/clk_summary:

[...]

>>> Reverting this commit makes it work again.
>>
>> Thank you for your detailed report!
>>
>> I've had a first look at hdmi-tmds and hdmi-ddc, and neither seems to
>> be calling ccu_is_better_rate() in their determine_rate()
>> functions. Their parents have the exact same rates in your diff, so,
>> my current working assumption is that they can't be the cause either.
>>
>> I'll have a more detailed look over the weekend. Until then, if anyone
>> has some ideas where I should have a look next, please share your
>> thoughts.
>
> In case it's relevant, this system doesn't use the HDMI DDC, the
> physical DDC pins being connected to a different I2C adapter for
> various reasons.
>
> From the clk_summary diff, I see a few things:
>
> 1. hdmi-tmds has changed parent from pll-video1-2x to pll-video0-2x.
> 2. The ratio of hdmi-tmds to its parent has changed from 1/8 to 1.
> 3. The resulting rate bears no relation to the pixel clock from EDID.
>
> I tried kernel 6.9.1 as well, and that doesn't work either.  I'll keep
> digging and try to narrow it down.

It turns out HDMI output is broken in v6.9 for a different reason.
However, this commit (b914ec33b391 clk: sunxi-ng: common: Support
minimum and maximum rate) requires two others as well in order not
to break things on the A20:

cedb7dd193f6 drm/sun4i: hdmi: Convert encoder to atomic
9ca6bc246035 drm/sun4i: hdmi: Move mode_set into enable

With those two (the second depends on the first) cherry-picked on top of
v6.6.31, the HDMI output is working again.  Likewise on v6.8.10.

-- 
Måns Rullgård

Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Linux regression tracking (Thorsten Leemhuis)]

On 23.05.24 20:58, Måns Rullgård wrote:
> Måns Rullgård <mans@mansr.com> writes:
>> Frank Oltmanns <frank@oltmanns.dev> writes:
>>> 21.05.2024 15:43:10 Måns Rullgård <mans@mansr.com>:
>>>> Frank Oltmanns <frank@oltmanns.dev> writes:
>>>>
>>>>> The Allwinner SoC's typically have an upper and lower limit for their
>>>>> clocks' rates. Up until now, support for that has been implemented
>>>>> separately for each clock type.
>>>>>
>>>>> Implement that functionality in the sunxi-ng's common part making use of
>>>>> the CCF rate liming capabilities, so that it is available for all clock
>>>>> types.
>>>>>
>>>>> Suggested-by: Maxime Ripard <mripard@kernel.org>
>>>>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>>>>> Cc: stable@vger.kernel.org
>>>>> ---
>>>>> drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
>>>>> drivers/clk/sunxi-ng/ccu_common.h |  3 +++
>>>>> 2 files changed, 22 insertions(+)
>>>>
>>>> This just landed in 6.6 stable, and it broke HDMI output on an A20 based
>>>> device, the clocks ending up all wrong as seen in this diff of
>>>> /sys/kernel/debug/clk/clk_summary:
> [...]
> 
>>>> Reverting this commit makes it work again.
>>> Thank you for your detailed report!
> [...]
> It turns out HDMI output is broken in v6.9 for a different reason.
> However, this commit (b914ec33b391 clk: sunxi-ng: common: Support
> minimum and maximum rate) requires two others as well in order not
> to break things on the A20:
> 
> cedb7dd193f6 drm/sun4i: hdmi: Convert encoder to atomic
> 9ca6bc246035 drm/sun4i: hdmi: Move mode_set into enable
> 
> With those two (the second depends on the first) cherry-picked on top of
> v6.6.31, the HDMI output is working again.  Likewise on v6.8.10.

They from what I can see are not yet in 6.6.y or on their way there (6.8
is EOL now). Did anyone ask Greg to pick this up? If not: Månsm could
you maybe do that? CCing him on a reply and asking is likely enough if
both changes apply cleanly.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot introduced: 547263745e15a0
#regzbot fix: drm/sun4i: hdmi: Move mode_set into enable
#regzbot poke

Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Greg KH]

On Wed, Jun 12, 2024 at 03:28:01PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 23.05.24 20:58, Måns Rullgård wrote:
> > Måns Rullgård <mans@mansr.com> writes:
> >> Frank Oltmanns <frank@oltmanns.dev> writes:
> >>> 21.05.2024 15:43:10 Måns Rullgård <mans@mansr.com>:
> >>>> Frank Oltmanns <frank@oltmanns.dev> writes:
> >>>>
> >>>>> The Allwinner SoC's typically have an upper and lower limit for their
> >>>>> clocks' rates. Up until now, support for that has been implemented
> >>>>> separately for each clock type.
> >>>>>
> >>>>> Implement that functionality in the sunxi-ng's common part making use of
> >>>>> the CCF rate liming capabilities, so that it is available for all clock
> >>>>> types.
> >>>>>
> >>>>> Suggested-by: Maxime Ripard <mripard@kernel.org>
> >>>>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
> >>>>> Cc: stable@vger.kernel.org
> >>>>> ---
> >>>>> drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
> >>>>> drivers/clk/sunxi-ng/ccu_common.h |  3 +++
> >>>>> 2 files changed, 22 insertions(+)
> >>>>
> >>>> This just landed in 6.6 stable, and it broke HDMI output on an A20 based
> >>>> device, the clocks ending up all wrong as seen in this diff of
> >>>> /sys/kernel/debug/clk/clk_summary:
> > [...]
> > 
> >>>> Reverting this commit makes it work again.
> >>> Thank you for your detailed report!
> > [...]
> > It turns out HDMI output is broken in v6.9 for a different reason.
> > However, this commit (b914ec33b391 clk: sunxi-ng: common: Support
> > minimum and maximum rate) requires two others as well in order not
> > to break things on the A20:
> > 
> > cedb7dd193f6 drm/sun4i: hdmi: Convert encoder to atomic
> > 9ca6bc246035 drm/sun4i: hdmi: Move mode_set into enable
> > 
> > With those two (the second depends on the first) cherry-picked on top of
> > v6.6.31, the HDMI output is working again.  Likewise on v6.8.10.
> 
> They from what I can see are not yet in 6.6.y or on their way there (6.8
> is EOL now). Did anyone ask Greg to pick this up? If not: Månsm could
> you maybe do that? CCing him on a reply and asking is likely enough if
> both changes apply cleanly.

Both now queued up, thanks.

greg k-h

RE: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Pafford, Robert J.]

> The Allwinner SoC's typically have an upper and lower limit for their
> clocks' rates. Up until now, support for that has been implemented
> separately for each clock type.
>
> Implement that functionality in the sunxi-ng's common part making use of
> the CCF rate liming capabilities, so that it is available for all clock
> types.
>
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
> Cc: stable@vger.kernel.org
> ---
>  drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
>  drivers/clk/sunxi-ng/ccu_common.h |  3 +++
>  2 files changed, 22 insertions(+)

This patch appears to cause a buffer under-read bug due to the call to 'hw_to_ccu_common', which assumes all entries
in the desc->hw_clocks->hws array are contained in ccu_common structs.

However, not all clocks in the array are contained in ccu_common structs. For example, as part
of the "sun20i-d1-ccu" driver, the "pll-video0" clock holds the 'clk_hw' struct inside of a 'clk_fixed_factor' struct,
as it is a fixed factor clock based on the "pll-video0-4x" clock, created with the CLK_FIXED_FACTOR_HWS macro.
This results in undefined behavior as the hw_to_ccu_common returns an invalid pointer referencing memory before the
'clk_fixed_factor' struct.

I have attached kernel warnings from a system based on the "sun8i-t113s.dtsi" device tree, where the memory contains
a non-zero value for the min-rate but a zero value for the max-rate, triggering the "No max_rate, ignoring min_rate"
warning in the 'sunxi_ccu_probe' function.


[    0.549013] ------------[ cut here ]------------
[    0.553727] WARNING: CPU: 0 PID: 1 at drivers/clk/sunxi-ng/ccu_common.c:155 sunxi_ccu_probe+0x105/0x164
[    0.563153] No max_rate, ignoring min_rate of clock 6 - pll-periph0-div3
[    0.569846] Modules linked in:
[    0.572913] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.6.32-winglet #7
[    0.579540] Hardware name: Generic DT based system
[    0.584350]  unwind_backtrace from show_stack+0xb/0xc
[    0.589445]  show_stack from dump_stack_lvl+0x2b/0x34
[    0.594531]  dump_stack_lvl from __warn+0x5d/0x92
[    0.599275]  __warn from warn_slowpath_fmt+0xd7/0x12c
[    0.604354]  warn_slowpath_fmt from sunxi_ccu_probe+0x105/0x164
[    0.610299]  sunxi_ccu_probe from devm_sunxi_ccu_probe+0x3d/0x60
[    0.616317]  devm_sunxi_ccu_probe from sun20i_d1_ccu_probe+0xbf/0xec
[    0.622681]  sun20i_d1_ccu_probe from platform_probe+0x3d/0x78
[    0.628542]  platform_probe from really_probe+0x81/0x1d0
[    0.633862]  really_probe from __driver_probe_device+0x59/0x130
[    0.639813]  __driver_probe_device from driver_probe_device+0x2d/0xc8
[    0.646283]  driver_probe_device from __driver_attach+0x4d/0xf0
[    0.652216]  __driver_attach from bus_for_each_dev+0x49/0x84
[    0.657888]  bus_for_each_dev from bus_add_driver+0x91/0x13c
[    0.663567]  bus_add_driver from driver_register+0x37/0xa4
[    0.669066]  driver_register from do_one_initcall+0x41/0x1c4
[    0.674740]  do_one_initcall from kernel_init_freeable+0x13d/0x180
[    0.680937]  kernel_init_freeable from kernel_init+0x15/0xec
[    0.686607]  kernel_init from ret_from_fork+0x11/0x1c
[    0.691674] Exception stack(0xc8815fb0 to 0xc8815ff8)
[    0.696739] 5fa0:                                     00000000 00000000 00000000 00000000
[    0.704926] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    0.713111] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    0.719765] ---[ end trace 0000000000000000 ]---
[    0.724452] ------------[ cut here ]------------
[    0.729082] WARNING: CPU: 0 PID: 1 at drivers/clk/sunxi-ng/ccu_common.c:155 sunxi_ccu_probe+0x105/0x164
[    0.738518] No max_rate, ignoring min_rate of clock 9 - pll-video0
[    0.744730] Modules linked in:
[    0.747801] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.6.32-winglet #7
[    0.755911] Hardware name: Generic DT based system
[    0.760696]  unwind_backtrace from show_stack+0xb/0xc
[    0.765768]  show_stack from dump_stack_lvl+0x2b/0x34
[    0.770859]  dump_stack_lvl from __warn+0x5d/0x92
[    0.775600]  __warn from warn_slowpath_fmt+0xd7/0x12c
[    0.780668]  warn_slowpath_fmt from sunxi_ccu_probe+0x105/0x164
[    0.786620]  sunxi_ccu_probe from devm_sunxi_ccu_probe+0x3d/0x60
[    0.792664]  devm_sunxi_ccu_probe from sun20i_d1_ccu_probe+0xbf/0xec
[    0.799035]  sun20i_d1_ccu_probe from platform_probe+0x3d/0x78
[    0.804901]  platform_probe from really_probe+0x81/0x1d0
[    0.810229]  really_probe from __driver_probe_device+0x59/0x130
[    0.816171]  __driver_probe_device from driver_probe_device+0x2d/0xc8
[    0.822624]  driver_probe_device from __driver_attach+0x4d/0xf0
[    0.828566]  __driver_attach from bus_for_each_dev+0x49/0x84
[    0.834237]  bus_for_each_dev from bus_add_driver+0x91/0x13c
[    0.839925]  bus_add_driver from driver_register+0x37/0xa4
[    0.845441]  driver_register from do_one_initcall+0x41/0x1c4
[    0.851123]  do_one_initcall from kernel_init_freeable+0x13d/0x180
[    0.857335]  kernel_init_freeable from kernel_init+0x15/0xec
[    0.863022]  kernel_init from ret_from_fork+0x11/0x1c
[    0.868096] Exception stack(0xc8815fb0 to 0xc8815ff8)
[    0.873145] 5fa0:                                     00000000 00000000 00000000 00000000
[    0.881332] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    0.889525] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    0.896165] ---[ end trace 0000000000000000 ]---
[    0.900821] ------------[ cut here ]------------
[    0.905471] WARNING: CPU: 0 PID: 1 at drivers/clk/sunxi-ng/ccu_common.c:155 sunxi_ccu_probe+0x105/0x164
[    0.914885] No max_rate, ignoring min_rate of clock 12 - pll-video1
[    0.921143] Modules linked in:
[    0.924208] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.6.32-winglet #7
[    0.932308] Hardware name: Generic DT based system
[    0.937102]  unwind_backtrace from show_stack+0xb/0xc
[    0.942173]  show_stack from dump_stack_lvl+0x2b/0x34
[    0.947254]  dump_stack_lvl from __warn+0x5d/0x92
[    0.952004]  __warn from warn_slowpath_fmt+0xd7/0x12c
[    0.957081]  warn_slowpath_fmt from sunxi_ccu_probe+0x105/0x164
[    0.963034]  sunxi_ccu_probe from devm_sunxi_ccu_probe+0x3d/0x60
[    0.969052]  devm_sunxi_ccu_probe from sun20i_d1_ccu_probe+0xbf/0xec
[    0.975422]  sun20i_d1_ccu_probe from platform_probe+0x3d/0x78
[    0.981288]  platform_probe from really_probe+0x81/0x1d0
[    0.986607]  really_probe from __driver_probe_device+0x59/0x130
[    0.992540]  __driver_probe_device from driver_probe_device+0x2d/0xc8
[    0.999002]  driver_probe_device from __driver_attach+0x4d/0xf0
[    1.004944]  __driver_attach from bus_for_each_dev+0x49/0x84
[    1.010606]  bus_for_each_dev from bus_add_driver+0x91/0x13c
[    1.016286]  bus_add_driver from driver_register+0x37/0xa4
[    1.021785]  driver_register from do_one_initcall+0x41/0x1c4
[    1.027467]  do_one_initcall from kernel_init_freeable+0x13d/0x180
[    1.033679]  kernel_init_freeable from kernel_init+0x15/0xec
[    1.039356]  kernel_init from ret_from_fork+0x11/0x1c
[    1.044440] Exception stack(0xc8815fb0 to 0xc8815ff8)
[    1.049496] 5fa0:                                     00000000 00000000 00000000 00000000
[    1.057674] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    1.065850] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    1.072471] ---[ end trace 0000000000000000 ]---
[    1.077106] ------------[ cut here ]------------
[    1.081734] WARNING: CPU: 0 PID: 1 at drivers/clk/sunxi-ng/ccu_common.c:155 sunxi_ccu_probe+0x105/0x164
[    1.091165] No max_rate, ignoring min_rate of clock 16 - pll-audio0
[    1.097441] Modules linked in:
[    1.100503] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.6.32-winglet #7
[    1.108602] Hardware name: Generic DT based system
[    1.113404]  unwind_backtrace from show_stack+0xb/0xc
[    1.118474]  show_stack from dump_stack_lvl+0x2b/0x34
[    1.123564]  dump_stack_lvl from __warn+0x5d/0x92
[    1.128288]  __warn from warn_slowpath_fmt+0xd7/0x12c
[    1.133356]  warn_slowpath_fmt from sunxi_ccu_probe+0x105/0x164
[    1.139283]  sunxi_ccu_probe from devm_sunxi_ccu_probe+0x3d/0x60
[    1.145318]  devm_sunxi_ccu_probe from sun20i_d1_ccu_probe+0xbf/0xec
[    1.151680]  sun20i_d1_ccu_probe from platform_probe+0x3d/0x78
[    1.157537]  platform_probe from really_probe+0x81/0x1d0
[    1.162857]  really_probe from __driver_probe_device+0x59/0x130
[    1.168816]  __driver_probe_device from driver_probe_device+0x2d/0xc8
[    1.175278]  driver_probe_device from __driver_attach+0x4d/0xf0
[    1.181219]  __driver_attach from bus_for_each_dev+0x49/0x84
[    1.186908]  bus_for_each_dev from bus_add_driver+0x91/0x13c
[    1.192595]  bus_add_driver from driver_register+0x37/0xa4
[    1.198103]  driver_register from do_one_initcall+0x41/0x1c4
[    1.203803]  do_one_initcall from kernel_init_freeable+0x13d/0x180
[    1.210006]  kernel_init_freeable from kernel_init+0x15/0xec
[    1.215684]  kernel_init from ret_from_fork+0x11/0x1c
[    1.220759] Exception stack(0xc8815fb0 to 0xc8815ff8)
[    1.225806] 5fa0:                                     00000000 00000000 00000000 00000000
[    1.233984] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    1.242169] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    1.248818] ---[ end trace 0000000000000000 ]---

Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Frank Oltmanns]

Hi Robert,

I'm truly sorry for the trouble the patch has caused you and for my late
reply!

On 2024-06-14 at 23:52:08 +0000, "Pafford, Robert J." <pafford.9@buckeyemail.osu.edu> wrote:
>> The Allwinner SoC's typically have an upper and lower limit for their
>> clocks' rates. Up until now, support for that has been implemented
>> separately for each clock type.
>>
>> Implement that functionality in the sunxi-ng's common part making use of
>> the CCF rate liming capabilities, so that it is available for all clock
>> types.
>>
>> Suggested-by: Maxime Ripard <mripard@kernel.org>
>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>> Cc: stable@vger.kernel.org
>> ---
>>  drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
>>  drivers/clk/sunxi-ng/ccu_common.h |  3 +++
>>  2 files changed, 22 insertions(+)
>
> This patch appears to cause a buffer under-read bug due to the call to 'hw_to_ccu_common', which assumes all entries
> in the desc->hw_clocks->hws array are contained in ccu_common structs.
>
> However, not all clocks in the array are contained in ccu_common structs. For example, as part
> of the "sun20i-d1-ccu" driver, the "pll-video0" clock holds the 'clk_hw' struct inside of a 'clk_fixed_factor' struct,
> as it is a fixed factor clock based on the "pll-video0-4x" clock, created with the CLK_FIXED_FACTOR_HWS macro.
> This results in undefined behavior as the hw_to_ccu_common returns an invalid pointer referencing memory before the
> 'clk_fixed_factor' struct.
>

Great catch! At first glance, it seems to me that calling
clk_hw_set_rate_range() in sunxi_ccu_probe() should not have happenend
in the loop that iterates over the hw_clks.

Instead we should add one more loop that iterates over the ccu_clks.
Note, that there is already one such loop but, unfortunately, we can't
use that as it happens before the hw_clks loop and we can only call
clk_hw_set_rate_range() after the hw_clk has been registered.

Hence, I propose to move the offending code to a new loop:
	for (i = 0; i < desc->num_ccu_clks; i++) {
		struct ccu_common *cclk = desc->ccu_clks[i];

		if (!cclk)
			continue;

		if (cclk->max_rate)
			clk_hw_set_rate_range(&cclk->hw, common->min_rate,
					      common->max_rate);
		else
			WARN(cclk->min_rate,
			     "No max_rate, ignoring min_rate of clock %d - %s\n",
			     i, cclk->hw.init->name);
	}

I haven't tested (or even compiled) the above, but I'll test and send a
patch within the next few days for you to test.

Thanks again,
  Frank

>
> I have attached kernel warnings from a system based on the "sun8i-t113s.dtsi" device tree, where the memory contains
> a non-zero value for the min-rate but a zero value for the max-rate, triggering the "No max_rate, ignoring min_rate"
> warning in the 'sunxi_ccu_probe' function.
>
>
> [    0.549013] ------------[ cut here ]------------
> [    0.553727] WARNING: CPU: 0 PID: 1 at drivers/clk/sunxi-ng/ccu_common.c:155 sunxi_ccu_probe+0x105/0x164
> [    0.563153] No max_rate, ignoring min_rate of clock 6 - pll-periph0-div3
> [    0.569846] Modules linked in:
> [    0.572913] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.6.32-winglet #7
> [    0.579540] Hardware name: Generic DT based system
> [    0.584350]  unwind_backtrace from show_stack+0xb/0xc
> [    0.589445]  show_stack from dump_stack_lvl+0x2b/0x34
> [    0.594531]  dump_stack_lvl from __warn+0x5d/0x92
> [    0.599275]  __warn from warn_slowpath_fmt+0xd7/0x12c
> [    0.604354]  warn_slowpath_fmt from sunxi_ccu_probe+0x105/0x164
> [    0.610299]  sunxi_ccu_probe from devm_sunxi_ccu_probe+0x3d/0x60
> [    0.616317]  devm_sunxi_ccu_probe from sun20i_d1_ccu_probe+0xbf/0xec
> [    0.622681]  sun20i_d1_ccu_probe from platform_probe+0x3d/0x78
> [    0.628542]  platform_probe from really_probe+0x81/0x1d0
> [    0.633862]  really_probe from __driver_probe_device+0x59/0x130
> [    0.639813]  __driver_probe_device from driver_probe_device+0x2d/0xc8
> [    0.646283]  driver_probe_device from __driver_attach+0x4d/0xf0
> [    0.652216]  __driver_attach from bus_for_each_dev+0x49/0x84
> [    0.657888]  bus_for_each_dev from bus_add_driver+0x91/0x13c
> [    0.663567]  bus_add_driver from driver_register+0x37/0xa4
> [    0.669066]  driver_register from do_one_initcall+0x41/0x1c4
> [    0.674740]  do_one_initcall from kernel_init_freeable+0x13d/0x180
> [    0.680937]  kernel_init_freeable from kernel_init+0x15/0xec
> [    0.686607]  kernel_init from ret_from_fork+0x11/0x1c
> [    0.691674] Exception stack(0xc8815fb0 to 0xc8815ff8)
> [    0.696739] 5fa0:                                     00000000 00000000 00000000 00000000
> [    0.704926] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    0.713111] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    0.719765] ---[ end trace 0000000000000000 ]---
> [    0.724452] ------------[ cut here ]------------
> [    0.729082] WARNING: CPU: 0 PID: 1 at drivers/clk/sunxi-ng/ccu_common.c:155 sunxi_ccu_probe+0x105/0x164
> [    0.738518] No max_rate, ignoring min_rate of clock 9 - pll-video0
> [    0.744730] Modules linked in:
> [    0.747801] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.6.32-winglet #7
> [    0.755911] Hardware name: Generic DT based system
> [    0.760696]  unwind_backtrace from show_stack+0xb/0xc
> [    0.765768]  show_stack from dump_stack_lvl+0x2b/0x34
> [    0.770859]  dump_stack_lvl from __warn+0x5d/0x92
> [    0.775600]  __warn from warn_slowpath_fmt+0xd7/0x12c
> [    0.780668]  warn_slowpath_fmt from sunxi_ccu_probe+0x105/0x164
> [    0.786620]  sunxi_ccu_probe from devm_sunxi_ccu_probe+0x3d/0x60
> [    0.792664]  devm_sunxi_ccu_probe from sun20i_d1_ccu_probe+0xbf/0xec
> [    0.799035]  sun20i_d1_ccu_probe from platform_probe+0x3d/0x78
> [    0.804901]  platform_probe from really_probe+0x81/0x1d0
> [    0.810229]  really_probe from __driver_probe_device+0x59/0x130
> [    0.816171]  __driver_probe_device from driver_probe_device+0x2d/0xc8
> [    0.822624]  driver_probe_device from __driver_attach+0x4d/0xf0
> [    0.828566]  __driver_attach from bus_for_each_dev+0x49/0x84
> [    0.834237]  bus_for_each_dev from bus_add_driver+0x91/0x13c
> [    0.839925]  bus_add_driver from driver_register+0x37/0xa4
> [    0.845441]  driver_register from do_one_initcall+0x41/0x1c4
> [    0.851123]  do_one_initcall from kernel_init_freeable+0x13d/0x180
> [    0.857335]  kernel_init_freeable from kernel_init+0x15/0xec
> [    0.863022]  kernel_init from ret_from_fork+0x11/0x1c
> [    0.868096] Exception stack(0xc8815fb0 to 0xc8815ff8)
> [    0.873145] 5fa0:                                     00000000 00000000 00000000 00000000
> [    0.881332] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    0.889525] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    0.896165] ---[ end trace 0000000000000000 ]---
> [    0.900821] ------------[ cut here ]------------
> [    0.905471] WARNING: CPU: 0 PID: 1 at drivers/clk/sunxi-ng/ccu_common.c:155 sunxi_ccu_probe+0x105/0x164
> [    0.914885] No max_rate, ignoring min_rate of clock 12 - pll-video1
> [    0.921143] Modules linked in:
> [    0.924208] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.6.32-winglet #7
> [    0.932308] Hardware name: Generic DT based system
> [    0.937102]  unwind_backtrace from show_stack+0xb/0xc
> [    0.942173]  show_stack from dump_stack_lvl+0x2b/0x34
> [    0.947254]  dump_stack_lvl from __warn+0x5d/0x92
> [    0.952004]  __warn from warn_slowpath_fmt+0xd7/0x12c
> [    0.957081]  warn_slowpath_fmt from sunxi_ccu_probe+0x105/0x164
> [    0.963034]  sunxi_ccu_probe from devm_sunxi_ccu_probe+0x3d/0x60
> [    0.969052]  devm_sunxi_ccu_probe from sun20i_d1_ccu_probe+0xbf/0xec
> [    0.975422]  sun20i_d1_ccu_probe from platform_probe+0x3d/0x78
> [    0.981288]  platform_probe from really_probe+0x81/0x1d0
> [    0.986607]  really_probe from __driver_probe_device+0x59/0x130
> [    0.992540]  __driver_probe_device from driver_probe_device+0x2d/0xc8
> [    0.999002]  driver_probe_device from __driver_attach+0x4d/0xf0
> [    1.004944]  __driver_attach from bus_for_each_dev+0x49/0x84
> [    1.010606]  bus_for_each_dev from bus_add_driver+0x91/0x13c
> [    1.016286]  bus_add_driver from driver_register+0x37/0xa4
> [    1.021785]  driver_register from do_one_initcall+0x41/0x1c4
> [    1.027467]  do_one_initcall from kernel_init_freeable+0x13d/0x180
> [    1.033679]  kernel_init_freeable from kernel_init+0x15/0xec
> [    1.039356]  kernel_init from ret_from_fork+0x11/0x1c
> [    1.044440] Exception stack(0xc8815fb0 to 0xc8815ff8)
> [    1.049496] 5fa0:                                     00000000 00000000 00000000 00000000
> [    1.057674] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    1.065850] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    1.072471] ---[ end trace 0000000000000000 ]---
> [    1.077106] ------------[ cut here ]------------
> [    1.081734] WARNING: CPU: 0 PID: 1 at drivers/clk/sunxi-ng/ccu_common.c:155 sunxi_ccu_probe+0x105/0x164
> [    1.091165] No max_rate, ignoring min_rate of clock 16 - pll-audio0
> [    1.097441] Modules linked in:
> [    1.100503] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.6.32-winglet #7
> [    1.108602] Hardware name: Generic DT based system
> [    1.113404]  unwind_backtrace from show_stack+0xb/0xc
> [    1.118474]  show_stack from dump_stack_lvl+0x2b/0x34
> [    1.123564]  dump_stack_lvl from __warn+0x5d/0x92
> [    1.128288]  __warn from warn_slowpath_fmt+0xd7/0x12c
> [    1.133356]  warn_slowpath_fmt from sunxi_ccu_probe+0x105/0x164
> [    1.139283]  sunxi_ccu_probe from devm_sunxi_ccu_probe+0x3d/0x60
> [    1.145318]  devm_sunxi_ccu_probe from sun20i_d1_ccu_probe+0xbf/0xec
> [    1.151680]  sun20i_d1_ccu_probe from platform_probe+0x3d/0x78
> [    1.157537]  platform_probe from really_probe+0x81/0x1d0
> [    1.162857]  really_probe from __driver_probe_device+0x59/0x130
> [    1.168816]  __driver_probe_device from driver_probe_device+0x2d/0xc8
> [    1.175278]  driver_probe_device from __driver_attach+0x4d/0xf0
> [    1.181219]  __driver_attach from bus_for_each_dev+0x49/0x84
> [    1.186908]  bus_for_each_dev from bus_add_driver+0x91/0x13c
> [    1.192595]  bus_add_driver from driver_register+0x37/0xa4
> [    1.198103]  driver_register from do_one_initcall+0x41/0x1c4
> [    1.203803]  do_one_initcall from kernel_init_freeable+0x13d/0x180
> [    1.210006]  kernel_init_freeable from kernel_init+0x15/0xec
> [    1.215684]  kernel_init from ret_from_fork+0x11/0x1c
> [    1.220759] Exception stack(0xc8815fb0 to 0xc8815ff8)
> [    1.225806] 5fa0:                                     00000000 00000000 00000000 00000000
> [    1.233984] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    1.242169] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    1.248818] ---[ end trace 0000000000000000 ]---

Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Pafford, Robert J.]

Hi Frank,

Moving to a new for loop makes sense. Let me know when you have a patch
and I'll be glad to test it on my board. I do also wonder if this may
have contributed to some of the HDMI issues seen in the other thread.

Best,
Robert

> Hi Robert,
>
> I'm truly sorry for the trouble the patch has caused you and for my late
> reply!
>
> On 2024-06-14 at 23:52:08 +0000, "Pafford, Robert J." <pafford.9@buckeyemail.osu.edu> wrote:
>>> The Allwinner SoC's typically have an upper and lower limit for their
>>> clocks' rates. Up until now, support for that has been implemented
>>> separately for each clock type.
>>>
>>> Implement that functionality in the sunxi-ng's common part making use of
>>> the CCF rate liming capabilities, so that it is available for all clock
>>> types.
>>>
>>> Suggested-by: Maxime Ripard <mripard@kernel.org>
>>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>  drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
>>>  drivers/clk/sunxi-ng/ccu_common.h |  3 +++
>>>  2 files changed, 22 insertions(+)
>>
>> This patch appears to cause a buffer under-read bug due to the call to 'hw_to_ccu_common', which assumes all entries
>> in the desc->hw_clocks->hws array are contained in ccu_common structs.
>>
>> However, not all clocks in the array are contained in ccu_common structs. For example, as part
>> of the "sun20i-d1-ccu" driver, the "pll-video0" clock holds the 'clk_hw' struct inside of a 'clk_fixed_factor' struct,
>> as it is a fixed factor clock based on the "pll-video0-4x" clock, created with the CLK_FIXED_FACTOR_HWS macro.
>> This results in undefined behavior as the hw_to_ccu_common returns an invalid pointer referencing memory before the
>> 'clk_fixed_factor' struct.
>>
>
> Great catch! At first glance, it seems to me that calling
> clk_hw_set_rate_range() in sunxi_ccu_probe() should not have happenend
> in the loop that iterates over the hw_clks.
> 
> Instead we should add one more loop that iterates over the ccu_clks.
> Note, that there is already one such loop but, unfortunately, we can't
> use that as it happens before the hw_clks loop and we can only call
> clk_hw_set_rate_range() after the hw_clk has been registered.
> 
> Hence, I propose to move the offending code to a new loop:
>         for (i = 0; i < desc->num_ccu_clks; i++) {
>                 struct ccu_common *cclk = desc->ccu_clks[i];
> 
>                 if (!cclk)
>                         continue;
> 
>                 if (cclk->max_rate)
>                         clk_hw_set_rate_range(&cclk->hw, common->min_rate,
>                                               common->max_rate);
>                 else
>                         WARN(cclk->min_rate,
>                              "No max_rate, ignoring min_rate of clock %d - %s\n",
>                              i, cclk->hw.init->name);
>         }
> 
> I haven't tested (or even compiled) the above, but I'll test and send a
> patch within the next few days for you to test.
> 
> Thanks again,
>   Frank
> 
>>
>> I have attached kernel warnings from a system based on the "sun8i-t113s.dtsi" device tree, where the memory contains
>> a non-zero value for the min-rate but a zero value for the max-rate, triggering the "No max_rate, ignoring min_rate"
>> warning in the 'sunxi_ccu_probe' function.
>>
>> [...]

Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Frank Oltmanns]

Hi Robert,

26.06.2024 18:03:24 Pafford, Robert J. <pafford.9@buckeyemail.osu.edu>:

> Hi Frank,
>
> Moving to a new for loop makes sense. Let me know when you have a patch

The patch is here, strange you didn't receive it:
https://lore.kernel.org/all/20240623-sunxi-ng_fix_common_probe-v1-1-7c97e32824a1@oltmanns.dev/


> and I'll be glad to test it on my board. I do also wonder if this may
> have contributed to some of the HDMI issues seen in the other thread.

My thought's exactly!

Best regards,
  Frank

>
> Best,
> Robert
>
>> Hi Robert,
>>
>> I'm truly sorry for the trouble the patch has caused you and for my late
>> reply!
>>
>> On 2024-06-14 at 23:52:08 +0000, "Pafford, Robert J." <pafford.9@buckeyemail.osu.edu> wrote:
>>>> The Allwinner SoC's typically have an upper and lower limit for their
>>>> clocks' rates. Up until now, support for that has been implemented
>>>> separately for each clock type.
>>>>
>>>> Implement that functionality in the sunxi-ng's common part making use of
>>>> the CCF rate liming capabilities, so that it is available for all clock
>>>> types.
>>>>
>>>> Suggested-by: Maxime Ripard <mripard@kernel.org>
>>>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>>   drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
>>>>   drivers/clk/sunxi-ng/ccu_common.h |  3 +++
>>>>   2 files changed, 22 insertions(+)
>>>
>>> This patch appears to cause a buffer under-read bug due to the call to 'hw_to_ccu_common', which assumes all entries
>>> in the desc->hw_clocks->hws array are contained in ccu_common structs.
>>>
>>> However, not all clocks in the array are contained in ccu_common structs. For example, as part
>>> of the "sun20i-d1-ccu" driver, the "pll-video0" clock holds the 'clk_hw' struct inside of a 'clk_fixed_factor' struct,
>>> as it is a fixed factor clock based on the "pll-video0-4x" clock, created with the CLK_FIXED_FACTOR_HWS macro.
>>> This results in undefined behavior as the hw_to_ccu_common returns an invalid pointer referencing memory before the
>>> 'clk_fixed_factor' struct.
>>>
>>
>> Great catch! At first glance, it seems to me that calling
>> clk_hw_set_rate_range() in sunxi_ccu_probe() should not have happenend
>> in the loop that iterates over the hw_clks.
>>
>> Instead we should add one more loop that iterates over the ccu_clks.
>> Note, that there is already one such loop but, unfortunately, we can't
>> use that as it happens before the hw_clks loop and we can only call
>> clk_hw_set_rate_range() after the hw_clk has been registered.
>>
>> Hence, I propose to move the offending code to a new loop:
>>         for (i = 0; i < desc->num_ccu_clks; i++) {
>>                 struct ccu_common *cclk = desc->ccu_clks[i];
>>
>>                 if (!cclk)
>>                         continue;
>>
>>                 if (cclk->max_rate)
>>                         clk_hw_set_rate_range(&cclk->hw, common->min_rate,
>>                                               common->max_rate);
>>                 else
>>                         WARN(cclk->min_rate,
>>                              "No max_rate, ignoring min_rate of clock %d - %s\n",
>>                              i, cclk->hw.init->name);
>>         }
>>
>> I haven't tested (or even compiled) the above, but I'll test and send a
>> patch within the next few days for you to test.
>>
>> Thanks again,
>>   Frank
>>
>>>
>>> I have attached kernel warnings from a system based on the "sun8i-t113s.dtsi" device tree, where the memory contains
>>> a non-zero value for the min-rate but a zero value for the max-rate, triggering the "No max_rate, ignoring min_rate"
>>> warning in the 'sunxi_ccu_probe' function.
>>>
>>> [...]

Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Pafford, Robert J.]

Frank Oltmanns <frank@oltmanns.dev> writes:

> Hi Robert,
>
> 26.06.2024 18:03:24 Pafford, Robert J. <pafford.9@buckeyemail.osu.edu>:
>
>> Hi Frank,
>>
>> Moving to a new for loop makes sense. Let me know when you have a patch
>
> The patch is here, strange you didn't receive it:
> https://lore.kernel.org/all/20240623-sunxi-ng_fix_common_probe-v1-1-7c97e32824a1@oltmanns.dev/

Ah, this must have slipped through my inbox. I just applied it on my board and it is
now cooperating with the min/max clock rates!

>
>> and I'll be glad to test it on my board. I do also wonder if this may
>> have contributed to some of the HDMI issues seen in the other thread.
>
> My thought's exactly!
>
> Best regards,
>   Frank
>
>>
>> Best,
>> Robert
>>
>>> Hi Robert,
>>>
>>> I'm truly sorry for the trouble the patch has caused you and for my late
>>> reply!
>>>
>>> On 2024-06-14 at 23:52:08 +0000, "Pafford, Robert J." <pafford.9@buckeyemail.osu.edu> wrote:
>>>>> The Allwinner SoC's typically have an upper and lower limit for their
>>>>> clocks' rates. Up until now, support for that has been implemented
>>>>> separately for each clock type.
>>>>>
>>>>> Implement that functionality in the sunxi-ng's common part making use of
>>>>> the CCF rate liming capabilities, so that it is available for all clock
>>>>> types.
>>>>>
>>>>> Suggested-by: Maxime Ripard <mripard@kernel.org>
>>>>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>>>>> Cc: stable@vger.kernel.org
>>>>> ---
>>>>>   drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
>>>>>   drivers/clk/sunxi-ng/ccu_common.h |  3 +++
>>>>>   2 files changed, 22 insertions(+)
>>>>
>>>> This patch appears to cause a buffer under-read bug due to the call to 'hw_to_ccu_common', which assumes all entries
>>>> in the desc->hw_clocks->hws array are contained in ccu_common structs.
>>>>
>>>> However, not all clocks in the array are contained in ccu_common structs. For example, as part
>>>> of the "sun20i-d1-ccu" driver, the "pll-video0" clock holds the 'clk_hw' struct inside of a 'clk_fixed_factor' struct,
>>>> as it is a fixed factor clock based on the "pll-video0-4x" clock, created with the CLK_FIXED_FACTOR_HWS macro.
>>>> This results in undefined behavior as the hw_to_ccu_common returns an invalid pointer referencing memory before the
>>>> 'clk_fixed_factor' struct.
>>>>
>>>
>>> Great catch! At first glance, it seems to me that calling
>>> clk_hw_set_rate_range() in sunxi_ccu_probe() should not have happenend
>>> in the loop that iterates over the hw_clks.
>>>
>>> Instead we should add one more loop that iterates over the ccu_clks.
>>> Note, that there is already one such loop but, unfortunately, we can't
>>> use that as it happens before the hw_clks loop and we can only call
>>> clk_hw_set_rate_range() after the hw_clk has been registered.
>>>
>>> Hence, I propose to move the offending code to a new loop:
>>>         for (i = 0; i < desc->num_ccu_clks; i++) {
>>>                 struct ccu_common *cclk = desc->ccu_clks[i];
>>>
>>>                 if (!cclk)
>>>                         continue;
>>>
>>>                 if (cclk->max_rate)
>>>                         clk_hw_set_rate_range(&cclk->hw, common->min_rate,
>>>                                               common->max_rate);
>>>                 else
>>>                         WARN(cclk->min_rate,
>>>                              "No max_rate, ignoring min_rate of clock %d - %s\n",
>>>                              i, cclk->hw.init->name);
>>>         }
>>>
>>> I haven't tested (or even compiled) the above, but I'll test and send a
>>> patch within the next few days for you to test.
>>>
>>> Thanks again,
>>>   Frank
>>>
>>>>
>>>> I have attached kernel warnings from a system based on the "sun8i-t113s.dtsi" device tree, where the memory contains
>>>> a non-zero value for the min-rate but a zero value for the max-rate, triggering the "No max_rate, ignoring min_rate"
>>>> warning in the 'sunxi_ccu_probe' function.
>>>>
>>>> [...]

Thanks,
Robert

Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Chen-Yu Tsai]

On Thu, Jun 27, 2024 at 9:23 AM Pafford, Robert J.
<pafford.9@buckeyemail.osu.edu> wrote:
>
> Frank Oltmanns <frank@oltmanns.dev> writes:
>
> > Hi Robert,
> >
> > 26.06.2024 18:03:24 Pafford, Robert J. <pafford.9@buckeyemail.osu.edu>:
> >
> >> Hi Frank,
> >>
> >> Moving to a new for loop makes sense. Let me know when you have a patch
> >
> > The patch is here, strange you didn't receive it:
> > https://lore.kernel.org/all/20240623-sunxi-ng_fix_common_probe-v1-1-7c97e32824a1@oltmanns.dev/
>
> Ah, this must have slipped through my inbox. I just applied it on my board and it is
> now cooperating with the min/max clock rates!

Please reply to the thread and give a Tested-by.

ChenYu

> >
> >> and I'll be glad to test it on my board. I do also wonder if this may
> >> have contributed to some of the HDMI issues seen in the other thread.
> >
> > My thought's exactly!
> >
> > Best regards,
> >   Frank
> >
> >>
> >> Best,
> >> Robert
> >>
> >>> Hi Robert,
> >>>
> >>> I'm truly sorry for the trouble the patch has caused you and for my late
> >>> reply!
> >>>
> >>> On 2024-06-14 at 23:52:08 +0000, "Pafford, Robert J." <pafford.9@buckeyemail.osu.edu> wrote:
> >>>>> The Allwinner SoC's typically have an upper and lower limit for their
> >>>>> clocks' rates. Up until now, support for that has been implemented
> >>>>> separately for each clock type.
> >>>>>
> >>>>> Implement that functionality in the sunxi-ng's common part making use of
> >>>>> the CCF rate liming capabilities, so that it is available for all clock
> >>>>> types.
> >>>>>
> >>>>> Suggested-by: Maxime Ripard <mripard@kernel.org>
> >>>>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
> >>>>> Cc: stable@vger.kernel.org
> >>>>> ---
> >>>>>   drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
> >>>>>   drivers/clk/sunxi-ng/ccu_common.h |  3 +++
> >>>>>   2 files changed, 22 insertions(+)
> >>>>
> >>>> This patch appears to cause a buffer under-read bug due to the call to 'hw_to_ccu_common', which assumes all entries
> >>>> in the desc->hw_clocks->hws array are contained in ccu_common structs.
> >>>>
> >>>> However, not all clocks in the array are contained in ccu_common structs. For example, as part
> >>>> of the "sun20i-d1-ccu" driver, the "pll-video0" clock holds the 'clk_hw' struct inside of a 'clk_fixed_factor' struct,
> >>>> as it is a fixed factor clock based on the "pll-video0-4x" clock, created with the CLK_FIXED_FACTOR_HWS macro.
> >>>> This results in undefined behavior as the hw_to_ccu_common returns an invalid pointer referencing memory before the
> >>>> 'clk_fixed_factor' struct.
> >>>>
> >>>
> >>> Great catch! At first glance, it seems to me that calling
> >>> clk_hw_set_rate_range() in sunxi_ccu_probe() should not have happenend
> >>> in the loop that iterates over the hw_clks.
> >>>
> >>> Instead we should add one more loop that iterates over the ccu_clks.
> >>> Note, that there is already one such loop but, unfortunately, we can't
> >>> use that as it happens before the hw_clks loop and we can only call
> >>> clk_hw_set_rate_range() after the hw_clk has been registered.
> >>>
> >>> Hence, I propose to move the offending code to a new loop:
> >>>         for (i = 0; i < desc->num_ccu_clks; i++) {
> >>>                 struct ccu_common *cclk = desc->ccu_clks[i];
> >>>
> >>>                 if (!cclk)
> >>>                         continue;
> >>>
> >>>                 if (cclk->max_rate)
> >>>                         clk_hw_set_rate_range(&cclk->hw, common->min_rate,
> >>>                                               common->max_rate);
> >>>                 else
> >>>                         WARN(cclk->min_rate,
> >>>                              "No max_rate, ignoring min_rate of clock %d - %s\n",
> >>>                              i, cclk->hw.init->name);
> >>>         }
> >>>
> >>> I haven't tested (or even compiled) the above, but I'll test and send a
> >>> patch within the next few days for you to test.
> >>>
> >>> Thanks again,
> >>>   Frank
> >>>
> >>>>
> >>>> I have attached kernel warnings from a system based on the "sun8i-t113s.dtsi" device tree, where the memory contains
> >>>> a non-zero value for the min-rate but a zero value for the max-rate, triggering the "No max_rate, ignoring min_rate"
> >>>> warning in the 'sunxi_ccu_probe' function.
> >>>>
> >>>> [...]
>
> Thanks,
> Robert