[PATCH 0/5] clk: Fix simple video pipelines on i.MX8

[PATCH 0/5] clk: Fix simple video pipelines on i.MX8

[Miquel Raynal]

Recent changes in the clock tree have set CLK_SET_RATE_PARENT to the two
LCDIF pixel clocks. The idea is, instead of using assigned-clock
properties to set upstream PLL rates to high frequencies and hoping that
a single divisor (namely media_disp[12]_pix) will be close enough in
most cases, we should tell the clock core to use the PLL to properly
derive an accurate pixel clock rate in the first place. Here is the
situation.

[Before ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")]

Before setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the sequence of events was:
- PLL is assigned to a high rate,
- media_disp[12]_pix is set to approximately freq A by using a single divisor,
- media_ldb is set to approximately freq 7*A by using another single divisor.
=> The display was working, but the pixel clock was inaccurate.

[After ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")]

After setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the
sequence of events became:
- media_disp[12]_pix is set to freq A by using a divisor of 1 and
  setting video_pll1 to freq A.
- media_ldb is trying to compute its divisor to set freq 7*A, but the
  upstream PLL is to low, it does not recompute it, so it ends up
  setting a divisor of 1 and being at freq A instead of 7*A.
=> The display is sadly no longer working

[After applying PATCH "clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate"]

This is a commit from Marek, which is, I believe going in the right
direction, so I am including it. Just with this change, the situation is
slightly different, but the result is the same:
- media_disp[12]_pix is set to freq A by using a divisor of 1 and
  setting video_pll1 to freq A.
- media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
  to freq 7*A.
  /!\ This as the side effect of changing media_disp[12]_pix from freq A
  to freq 7*A.
=> The display is still not working

[After applying this series]

The goal of the following patches is to prevent clock subtree walks to
"just recalculate" the pixel clocks, ignoring the fact that they should
no longer change. They should adapt their divisors to the new upstream
rates instead. As a result, the display pipeline is working again.

Note: if more than one display is connected, we need the LDB driver to
act accordingly, thus the LDB driver must be adapted. Also, if accurate
pixel clocks are not possible with two different displays, we will still
need (at least for now) to make sure one of them is reparented to
another PLL, like the audio PLL (but audio PLL are of a different kind,
and are slightly less accurate).

So this series aims at fixing the i.MX8MP display pipeline for simple
setups. Said otherwise, returning to the same level of support as
before, but with (hopefully) more accurate frequencies. I believe this
approach manages to fix both Marek situation and all people using a
straightforward LCD based setup. For more complex setups, we need more
smartness from DRM and clk, but this is gonna take a bit of time.

---
Marek Vasut (1):
      clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate

Miquel Raynal (4):
      clk: Add a helper to determine a clock rate
      clk: Split clk_calc_subtree()
      clk: Add flag to prevent frequency changes when walking subtrees
      clk: imx: imx8mp: Prevent media clocks to be incompatibly changed

 drivers/clk/clk.c            | 39 ++++++++++++++++++++++++++++++++-------
 drivers/clk/imx/clk-imx8mp.c |  6 +++---
 include/linux/clk-provider.h |  2 ++
 3 files changed, 37 insertions(+), 10 deletions(-)
---
base-commit: 62facaf164585923d081eedcb6871f4ff3c2e953
change-id: 20241121-ge-ian-debug-imx8-clk-tree-bd325aa866f1

Best regards,
-- 
Miquel Raynal <miquel.raynal@bootlin.com>

[PATCH 1/5] clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate

[Miquel Raynal]

From: Marek Vasut <marex@denx.de>

The media_ldb_root_clk supply LDB serializer. These clock are usually
shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
results in accurate serializer clock.

Signed-off-by: Marek Vasut <marex@denx.de>
Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/imx/clk-imx8mp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index 516dbd170c8a356f293621b371b9ef9b9bec90a4..2e61d340b8ab7f626155563c46e0d4142caf3fa9 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
 	hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
 	hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
 	hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
-	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
+	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
 	hws[IMX8MP_CLK_MEMREPAIR] = imx8m_clk_hw_composite_critical("mem_repair", imx8mp_memrepair_sels, ccm_base + 0xbf80);
 	hws[IMX8MP_CLK_MEDIA_MIPI_TEST_BYTE] = imx8m_clk_hw_composite("media_mipi_test_byte", imx8mp_media_mipi_test_byte_sels, ccm_base + 0xc100);
 	hws[IMX8MP_CLK_ECSPI3] = imx8m_clk_hw_composite("ecspi3", imx8mp_ecspi3_sels, ccm_base + 0xc180);

-- 
2.47.0

[PATCH 2/5] clk: Add a helper to determine a clock rate

[Miquel Raynal]

In the context of the clock core, "determine" means we calculate a
possible clock rate based on its hardware capabilities and its current
upstream parent frequency. This is opposed to "round" which tries to
find the best parent and best rate and "recalc" which is about finding
the next output clock based on a parent frequency change.

The prototype is based on clk_recalc() which does exactly the same for
the "recalc" situation.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/clk.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d02451f951cf057d068f980d985c95deb861a2d9..f171539bbb842f57698249a475c62f3f5719ccd1 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1927,6 +1927,18 @@ long clk_get_accuracy(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_get_accuracy);
 
+__maybe_unused
+static unsigned long clk_determine(struct clk_core *core, unsigned long rate)
+{
+	struct clk_rate_request req = {};
+
+	clk_hw_init_rate_request(core->hw, &req, rate);
+	if (__clk_determine_rate(core->hw, &req))
+		return 0;
+
+	return req.rate;
+}
+
 static unsigned long clk_recalc(struct clk_core *core,
 				unsigned long parent_rate)
 {

-- 
2.47.0

[PATCH 3/5] clk: Split clk_calc_subtree()

[Miquel Raynal]

This helper does two different things:
- it calculates the new clock frequency
    - as part of this task, it also handles a possible parent change
- it walks the clock subtree to further update frequencies as well (but
  the parent changes are no longer relevant there).

In order to ease the understanding of the next step, let's split this
helper into:
- clk_calc_core_and_subtree() which performs the top clock update (with
  the parents handling) and then calls...
- clk_calc_subtree() (which calls itself recursively) in order to
  perform the subtree updates.

There is no functional change intended.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/clk.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f171539bbb842f57698249a475c62f3f5719ccd1..adfc5bfb93b5a65b6f58c52ca2c432d651f7dd7d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2268,8 +2268,18 @@ static int __clk_speculate_rates(struct clk_core *core,
 	return ret;
 }
 
-static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
-			     struct clk_core *new_parent, u8 p_index)
+static void clk_calc_subtree(struct clk_core *core)
+{
+	struct clk_core *child;
+
+	core->new_rate = clk_recalc(core, core->parent->new_rate);
+
+	hlist_for_each_entry(child, &core->children, child_node)
+		clk_calc_subtree(child);
+}
+
+static void clk_calc_core_and_subtree(struct clk_core *core, unsigned long new_rate,
+				      struct clk_core *new_parent, u8 p_index)
 {
 	struct clk_core *child;
 
@@ -2281,10 +2291,8 @@ static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
 	if (new_parent && new_parent != core->parent)
 		new_parent->new_child = core;
 
-	hlist_for_each_entry(child, &core->children, child_node) {
-		child->new_rate = clk_recalc(child, new_rate);
-		clk_calc_subtree(child, child->new_rate, NULL, 0);
-	}
+	hlist_for_each_entry(child, &core->children, child_node)
+		clk_calc_subtree(child);
 }
 
 /*
@@ -2368,7 +2376,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 		top = clk_calc_new_rates(parent, best_parent_rate);
 
 out:
-	clk_calc_subtree(core, new_rate, parent, p_index);
+	clk_calc_core_and_subtree(core, new_rate, parent, p_index);
 
 	return top;
 }

-- 
2.47.0

[PATCH 4/5] clk: Add flag to prevent frequency changes when walking subtrees

[Miquel Raynal]

There are mainly two ways to change a clock frequency. The active way
requires calling ->set_rate() in order to ask "on purpose" for a
frequency change. Otherwise, a clock can passively see its frequency
being updated depending on upstream clock frequency changes. In most
cases it is fine to just accept the new upstream frequency - which by
definition will have an impact on downstream frequencies if we do not
recalculate internal divisors. But there are cases where, upon an
upstream frequency change, we would like to maintain a specific rate.

As an example, on iMX8MP the video pipeline clocks are looking like this:

    video_pll1
       video_pll1_bypass
          video_pll1_out
             media_ldb
                media_ldb_root_clk
             media_disp2_pix
                media_disp2_pix_root_clk
             media_disp1_pix
                media_disp1_pix_root_clk

media_ldb, media_disp2_pix and media_disp1_pix are simple divisors from
which we might require 2 or 3 different rates, whereas video_pll1 is a
very configurable PLL which can achieve almost any frequency. There are
however relationships between them, typically the ldb clock must be 3.5
or 7 times higher than the media_disp* clocks.

Currently, if eg. media_disp2_pix is set to 71900000Hz, when media_ldb
is (later) set to 503300000Hz, media_disp2_pix is updated to 503300000Hz
as well, which clearly does not make sense. We want it to stay at
71900000Hz during the subtree walk.

Achieving this is the purpose of the new clock flag:
CLK_NO_RATE_CHANGE_DURING_PROPAGATION

Please note, if the kernel was setting the ldb clock before a pixel
clock, the result would be different, and this is also what this patch
is trying to solve.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
In all cases, the LDB must be aware of the device configuration, and ask
for a clever frequency, we will never cope with slightly aware drivers
when addressing this kind of subtle situation.
---
 drivers/clk/clk.c            | 9 +++++++--
 include/linux/clk-provider.h | 2 ++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index adfc5bfb93b5a65b6f58c52ca2c432d651f7dd7d..94d93470479e77769e63e97462b176261103b552 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1927,7 +1927,6 @@ long clk_get_accuracy(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_get_accuracy);
 
-__maybe_unused
 static unsigned long clk_determine(struct clk_core *core, unsigned long rate)
 {
 	struct clk_rate_request req = {};
@@ -2272,7 +2271,13 @@ static void clk_calc_subtree(struct clk_core *core)
 {
 	struct clk_core *child;
 
-	core->new_rate = clk_recalc(core, core->parent->new_rate);
+	if (core->flags & CLK_NO_RATE_CHANGE_DURING_PROPAGATION) {
+		core->new_rate = clk_determine(core, core->rate);
+		if (!core->new_rate)
+			core->new_rate = clk_recalc(core, core->parent->new_rate);
+	} else {
+		core->new_rate = clk_recalc(core, core->parent->new_rate);
+	}
 
 	hlist_for_each_entry(child, &core->children, child_node)
 		clk_calc_subtree(child);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 200135e0f6d00d48b10e843259333b9733c97f38..baef0b442ac1d36ee935cbcaaaa4e2d95fe7654c 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -38,6 +38,8 @@
 #define CLK_OPS_PARENT_ENABLE	BIT(12)
 /* duty cycle call may be forwarded to the parent clock */
 #define CLK_DUTY_CYCLE_PARENT	BIT(13)
+/* do not passively change this clock rate during subtree rate propagation */
+#define CLK_NO_RATE_CHANGE_DURING_PROPAGATION BIT(14)
 
 struct clk;
 struct clk_hw;

-- 
2.47.0

[PATCH 5/5] clk: imx: imx8mp: Prevent media clocks to be incompatibly changed

[Miquel Raynal]

Having set the CLK_SET_RATE_PARENT flag to gain accuracy to the i.MX8
media related clocks (media_ldb, media_disp1_pix, media_disp2_pix) broke
most simple setups using the LDB and one LCDIF. Indeed, pixel
frequencies being set first, the top level PLL (video_pll1) was tuned to
achieve the perfect frequency, and the media_disp*_pix divisor was set
to 1 (acting like a passthrough). But shortly later, when setting the
LDB clock to 7 times the pixel clock, the PLL machinery was recomputed,
leaving the pixel divisors untouched. As a result, the attempted factor
of 7 between the two clocks could never be observed.

Set the CLK_NO_RATE_CHANGE_DURING_PROPAGATION flag to the LDB and LCDIF
pixel clocks to force them to be kept as close as their initial target
rate as possible across subtree walks.

Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")
Cc: stable@vger.kernel.org
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
--
All patches in this series must be backported for this one to apply.
---
 drivers/clk/imx/clk-imx8mp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index 2e61d340b8ab7f626155563c46e0d4142caf3fa9..2b916a4df97141dce46cefeb22ff584178a3929b 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -547,7 +547,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
 	hws[IMX8MP_CLK_AHB] = imx8m_clk_hw_composite_bus_critical("ahb_root", imx8mp_ahb_sels, ccm_base + 0x9000);
 	hws[IMX8MP_CLK_AUDIO_AHB] = imx8m_clk_hw_composite_bus("audio_ahb", imx8mp_audio_ahb_sels, ccm_base + 0x9100);
 	hws[IMX8MP_CLK_MIPI_DSI_ESC_RX] = imx8m_clk_hw_composite_bus("mipi_dsi_esc_rx", imx8mp_mipi_dsi_esc_rx_sels, ccm_base + 0x9200);
-	hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT);
+	hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT | CLK_NO_RATE_CHANGE_DURING_PROPAGATION);
 
 	hws[IMX8MP_CLK_IPG_ROOT] = imx_clk_hw_divider2("ipg_root", "ahb_root", ccm_base + 0x9080, 0, 1);
 
@@ -609,9 +609,9 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
 	hws[IMX8MP_CLK_USDHC3] = imx8m_clk_hw_composite("usdhc3", imx8mp_usdhc3_sels, ccm_base + 0xbc80);
 	hws[IMX8MP_CLK_MEDIA_CAM1_PIX] = imx8m_clk_hw_composite("media_cam1_pix", imx8mp_media_cam1_pix_sels, ccm_base + 0xbd00);
 	hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
-	hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
+	hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT | CLK_NO_RATE_CHANGE_DURING_PROPAGATION);
 	hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
-	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
+	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT | CLK_NO_RATE_CHANGE_DURING_PROPAGATION);
 	hws[IMX8MP_CLK_MEMREPAIR] = imx8m_clk_hw_composite_critical("mem_repair", imx8mp_memrepair_sels, ccm_base + 0xbf80);
 	hws[IMX8MP_CLK_MEDIA_MIPI_TEST_BYTE] = imx8m_clk_hw_composite("media_mipi_test_byte", imx8mp_media_mipi_test_byte_sels, ccm_base + 0xc100);
 	hws[IMX8MP_CLK_ECSPI3] = imx8m_clk_hw_composite("ecspi3", imx8mp_ecspi3_sels, ccm_base + 0xc180);

-- 
2.47.0

Re: [PATCH 0/5] clk: Fix simple video pipelines on i.MX8

[Liu Ying]

Hi Miquel,

On 11/22/2024, Miquel Raynal wrote:
> Recent changes in the clock tree have set CLK_SET_RATE_PARENT to the two
> LCDIF pixel clocks. The idea is, instead of using assigned-clock
> properties to set upstream PLL rates to high frequencies and hoping that
> a single divisor (namely media_disp[12]_pix) will be close enough in
> most cases, we should tell the clock core to use the PLL to properly
> derive an accurate pixel clock rate in the first place. Here is the
> situation.
> 
> [Before ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")]
> 
> Before setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the sequence of events was:
> - PLL is assigned to a high rate,
> - media_disp[12]_pix is set to approximately freq A by using a single divisor,
> - media_ldb is set to approximately freq 7*A by using another single divisor.
> => The display was working, but the pixel clock was inaccurate.
> 
> [After ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")]
> 
> After setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the
> sequence of events became:
> - media_disp[12]_pix is set to freq A by using a divisor of 1 and
>   setting video_pll1 to freq A.
> - media_ldb is trying to compute its divisor to set freq 7*A, but the
>   upstream PLL is to low, it does not recompute it, so it ends up
>   setting a divisor of 1 and being at freq A instead of 7*A.
> => The display is sadly no longer working
> 
> [After applying PATCH "clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate"]
> 
> This is a commit from Marek, which is, I believe going in the right
> direction, so I am including it. Just with this change, the situation is
> slightly different, but the result is the same:
> - media_disp[12]_pix is set to freq A by using a divisor of 1 and
>   setting video_pll1 to freq A.
> - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
>   to freq 7*A.
>   /!\ This as the side effect of changing media_disp[12]_pix from freq A
>   to freq 7*A.

Although I'm not of a fan of setting CLK_SET_RATE_PARENT flag to the
LDB clock and pixel clocks, would it work if the pixel clock rate is
set after the LDB clock rate is set in fsl_ldb_atomic_enable()?  The
pixel clock can be got from LDB's remote input LCDIF DT node by
calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch
does. Similar to setting pixel clock rate, I think a chance is that
pixel clock enablement can be moved from LCDIF driver to
fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change.

https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/

Actually, one sibling patch of the above patch reverts ff06ea04e4cf
because I thought "fixed PLL rate" is the only solution, though I'm
discussing any alternative solution of "dynamically changeable PLL
rate" with Marek in the thread of the sibling patch.

BTW, as you know the LDB clock rate is 3.5x faster than the pixel
clock rate in dual-link LVDS use cases, the lowest PLL rate needs to
be explicitly set to 7x faster than the pixel clock rate *before*
LDB clock rate is set.  This way, the pixel clock would be derived
from the PLL with integer division ratio = 7, not the unsupported
3.5.

pixel    LDB         PLL
A        3.5*A       7*A      --> OK
A        3.5*A       3.5*A    --> not OK

> => The display is still not working
> 
> [After applying this series]
> 
> The goal of the following patches is to prevent clock subtree walks to
> "just recalculate" the pixel clocks, ignoring the fact that they should
> no longer change. They should adapt their divisors to the new upstream
> rates instead. As a result, the display pipeline is working again.
> 
> Note: if more than one display is connected, we need the LDB driver to
> act accordingly, thus the LDB driver must be adapted. Also, if accurate
> pixel clocks are not possible with two different displays, we will still
> need (at least for now) to make sure one of them is reparented to
> another PLL, like the audio PLL (but audio PLL are of a different kind,
> and are slightly less accurate).
> 
> So this series aims at fixing the i.MX8MP display pipeline for simple
> setups. Said otherwise, returning to the same level of support as
> before, but with (hopefully) more accurate frequencies. I believe this
> approach manages to fix both Marek situation and all people using a
> straightforward LCD based setup. For more complex setups, we need more
> smartness from DRM and clk, but this is gonna take a bit of time.
> 
> ---
> Marek Vasut (1):
>       clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate
> 
> Miquel Raynal (4):
>       clk: Add a helper to determine a clock rate
>       clk: Split clk_calc_subtree()
>       clk: Add flag to prevent frequency changes when walking subtrees
>       clk: imx: imx8mp: Prevent media clocks to be incompatibly changed
> 
>  drivers/clk/clk.c            | 39 ++++++++++++++++++++++++++++++++-------
>  drivers/clk/imx/clk-imx8mp.c |  6 +++---
>  include/linux/clk-provider.h |  2 ++
>  3 files changed, 37 insertions(+), 10 deletions(-)
> ---
> base-commit: 62facaf164585923d081eedcb6871f4ff3c2e953
> change-id: 20241121-ge-ian-debug-imx8-clk-tree-bd325aa866f1
> 
> Best regards,

-- 
Regards,
Liu Ying

Re: [PATCH 0/5] clk: Fix simple video pipelines on i.MX8

[Miquel Raynal]

Hello Liu,

Thanks for the feedback!

On 22/11/2024 at 14:01:49 +08, Liu Ying <victor.liu@nxp.com> wrote:

> Hi Miquel,
>
> On 11/22/2024, Miquel Raynal wrote:
>> Recent changes in the clock tree have set CLK_SET_RATE_PARENT to the two
>> LCDIF pixel clocks. The idea is, instead of using assigned-clock
>> properties to set upstream PLL rates to high frequencies and hoping that
>> a single divisor (namely media_disp[12]_pix) will be close enough in
>> most cases, we should tell the clock core to use the PLL to properly
>> derive an accurate pixel clock rate in the first place. Here is the
>> situation.
>> 
>> [Before ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")]
>> 
>> Before setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the sequence of events was:
>> - PLL is assigned to a high rate,
>> - media_disp[12]_pix is set to approximately freq A by using a single divisor,
>> - media_ldb is set to approximately freq 7*A by using another single divisor.
>> => The display was working, but the pixel clock was inaccurate.
>> 
>> [After ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")]
>> 
>> After setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the
>> sequence of events became:
>> - media_disp[12]_pix is set to freq A by using a divisor of 1 and
>>   setting video_pll1 to freq A.
>> - media_ldb is trying to compute its divisor to set freq 7*A, but the
>>   upstream PLL is to low, it does not recompute it, so it ends up
>>   setting a divisor of 1 and being at freq A instead of 7*A.
>> => The display is sadly no longer working
>> 
>> [After applying PATCH "clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate"]
>> 
>> This is a commit from Marek, which is, I believe going in the right
>> direction, so I am including it. Just with this change, the situation is
>> slightly different, but the result is the same:
>> - media_disp[12]_pix is set to freq A by using a divisor of 1 and
>>   setting video_pll1 to freq A.
>> - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
>>   to freq 7*A.
>>   /!\ This as the side effect of changing media_disp[12]_pix from freq A
>>   to freq 7*A.
>
> Although I'm not of a fan of setting CLK_SET_RATE_PARENT flag to the
> LDB clock and pixel clocks,

I haven't commented much on this. For me, inaccurate pixel clocks mostly
work fine (if not too inaccurate), but it is true that having very
powerful PLL like the PLL1443, it is a pity not to use them at their
highest capabilities. However, I consider "not breaking users" more
important than having "perfect clock rates".

This series has one unique goal: accepting more accurate frequencies
*and* not breaking users in the most simplest cases.

> would it work if the pixel clock rate is
> set after the LDB clock rate is set in fsl_ldb_atomic_enable()?

The situation would be:
- media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
  to freq 7*A.
- media_disp[12]_pix is set to freq A by using a divisor of 7.

So yes, and the explanation of why is there:
https://elixir.bootlin.com/linux/v6.11.8/source/drivers/clk/clk-divider.c#L322

> The
> pixel clock can be got from LDB's remote input LCDIF DT node by
> calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch
> does. Similar to setting pixel clock rate, I think a chance is that
> pixel clock enablement can be moved from LCDIF driver to
> fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change.

TBH, this sounds like a hack and is no longer required with this series.

You are just trying to circumvent the fact that until now, applying a
rate in an upper clock would unconfigure the downstream rates, and I
think this is our first real problem.

> https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/
>
> Actually, one sibling patch of the above patch reverts ff06ea04e4cf
> because I thought "fixed PLL rate" is the only solution, though I'm
> discussing any alternative solution of "dynamically changeable PLL
> rate" with Marek in the thread of the sibling patch.

I don't think we want fixed PLL rates. Especially if you start using
external (hot-pluggable) displays with different needs: it just does not
fly. There is one situation that cannot yet be handled and needs
manual reparenting: using 3 displays with a non-divisible pixel
frequency.

FYI we managed this specific "advanced" case with assigned-clock-parents
using an audio PLL as hinted by Marek. It mostly works, event though the
PLL1416 are less precise and offer less accurate pixel clocks.

> BTW, as you know the LDB clock rate is 3.5x faster than the pixel
> clock rate in dual-link LVDS use cases, the lowest PLL rate needs to
> be explicitly set to 7x faster than the pixel clock rate *before*
> LDB clock rate is set.  This way, the pixel clock would be derived
> from the PLL with integer division ratio = 7, not the unsupported
> 3.5.
>
> pixel    LDB         PLL
> A        3.5*A       7*A      --> OK
> A        3.5*A       3.5*A    --> not OK

This series was mostly solving the simpler case, with one display, but I
agree we should probably also consider the dual case.

The situation here is that you require the LDB to be aware of some
clocks constraints, like the fact that the downstream pixel clocks only
feature simple dividors which cannot achieve a 3.5 rate. That is all.

It is clearly the LDB driver duty to make this feasible. I cannot test
the dual case so I didn't brought any solution to it in this series, but
I already had a solution in mind. Please find a patch below, it is very
simple, and should, in conjunction with this series, fix the dual case
as well.

FYI here is the final clock tree with this trick "manually" enabled. You
can see video_pll1 is now twice media_ldb, and media ldb is still 7
times media_disp[12]_pix (video_pll1 is assigned in DT to 1039500000, so
it has effectively been reconfigured).

       video_pll1                            1   1   0   1006600000
          video_pll1_bypass                  1   1   0   1006600000
             video_pll1_out                  2   2   0   1006600000
                media_ldb                    1   1   0    503300000
                   media_ldb_root_clk        1   1   0    503300000
                media_disp2_pix              1   1   0     71900000
                   media_disp2_pix_root_clk  1   1   0     71900000
                media_disp1_pix              0   0   0     71900000
                   media_disp1_pix_root_clk  0   0   0     71900000

---8<---
Author: Miquel Raynal <miquel.raynal@bootlin.com>

    drm: bridge: ldb: Make sure the upper PLL is compatible with dual output
    
    The i.MX8 display pipeline has a number of clock constraints, among which:
    - The bridge clock must be 7 times faster than the pixel clock in single mode
    - The bridge clock must be 3.5 times faster than the pixel clocks in dual mode
    While a ratio of 7 is easy to build with simple divisors, 3.5 is not
    achievable. In order to make sure we keep these clock ratios correct is
    to configure the upper clock (usually video_pll1, but that does not
    matter really) to twice it's usual value. This way, the bridge clock is
    configured to divide the upstream rate by 2, and the pixel clocks are
    configured to divide the upstream rate by 7, achieving a final 3.5 ratio
    between the two.
    
    Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index 81ff4e5f52fa..069c960ee56b 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -177,6 +177,17 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
        mode = &crtc_state->adjusted_mode;
 
        requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
+       /*
+        * Dual cases require a 3.5 rate division on the pixel clocks, which
+        * cannot be achieved with regular single divisors. Instead, double the
+        * parent PLL rate in the first place. In order to do that, we first
+        * require twice the target clock rate, which will program the upper
+        * PLL. Then, we ask for the actual targeted rate, which can be achieved
+        * by dividing by 2 the already configured upper PLL rate, without
+        * making further changes to it.
+        */
+       if (fsl_ldb_is_dual(fsl_ldb))
+               clk_set_rate(fsl_ldb->clk, requested_link_freq * 2);
        clk_set_rate(fsl_ldb->clk, requested_link_freq);
 
        configured_link_freq = clk_get_rate(fsl_ldb->clk);

Re: [PATCH 0/5] clk: Fix simple video pipelines on i.MX8

[Liu Ying]

On 11/22/2024, Miquel Raynal wrote:
> Hello Liu,

Hello Miquel,

> 
> Thanks for the feedback!
> 
> On 22/11/2024 at 14:01:49 +08, Liu Ying <victor.liu@nxp.com> wrote:
> 
>> Hi Miquel,
>>
>> On 11/22/2024, Miquel Raynal wrote:
>>> Recent changes in the clock tree have set CLK_SET_RATE_PARENT to the two
>>> LCDIF pixel clocks. The idea is, instead of using assigned-clock
>>> properties to set upstream PLL rates to high frequencies and hoping that
>>> a single divisor (namely media_disp[12]_pix) will be close enough in
>>> most cases, we should tell the clock core to use the PLL to properly
>>> derive an accurate pixel clock rate in the first place. Here is the
>>> situation.
>>>
>>> [Before ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")]
>>>
>>> Before setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the sequence of events was:
>>> - PLL is assigned to a high rate,
>>> - media_disp[12]_pix is set to approximately freq A by using a single divisor,
>>> - media_ldb is set to approximately freq 7*A by using another single divisor.
>>> => The display was working, but the pixel clock was inaccurate.
>>>
>>> [After ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")]
>>>
>>> After setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the
>>> sequence of events became:
>>> - media_disp[12]_pix is set to freq A by using a divisor of 1 and
>>>   setting video_pll1 to freq A.
>>> - media_ldb is trying to compute its divisor to set freq 7*A, but the
>>>   upstream PLL is to low, it does not recompute it, so it ends up
>>>   setting a divisor of 1 and being at freq A instead of 7*A.
>>> => The display is sadly no longer working
>>>
>>> [After applying PATCH "clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate"]
>>>
>>> This is a commit from Marek, which is, I believe going in the right
>>> direction, so I am including it. Just with this change, the situation is
>>> slightly different, but the result is the same:
>>> - media_disp[12]_pix is set to freq A by using a divisor of 1 and
>>>   setting video_pll1 to freq A.
>>> - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
>>>   to freq 7*A.
>>>   /!\ This as the side effect of changing media_disp[12]_pix from freq A
>>>   to freq 7*A.
>>
>> Although I'm not of a fan of setting CLK_SET_RATE_PARENT flag to the
>> LDB clock and pixel clocks,
> 
> I haven't commented much on this. For me, inaccurate pixel clocks mostly
> work fine (if not too inaccurate), but it is true that having very
> powerful PLL like the PLL1443, it is a pity not to use them at their
> highest capabilities. However, I consider "not breaking users" more
> important than having "perfect clock rates".
> 
> This series has one unique goal: accepting more accurate frequencies
> *and* not breaking users in the most simplest cases.
> 
>> would it work if the pixel clock rate is
>> set after the LDB clock rate is set in fsl_ldb_atomic_enable()?
> 
> The situation would be:
> - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
>   to freq 7*A.
> - media_disp[12]_pix is set to freq A by using a divisor of 7.
> 
> So yes, and the explanation of why is there:
> https://elixir.bootlin.com/linux/v6.11.8/source/drivers/clk/clk-divider.c#L322

OK, I agree it would work(even without this patch set).

> 
>> The
>> pixel clock can be got from LDB's remote input LCDIF DT node by
>> calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch
>> does. Similar to setting pixel clock rate, I think a chance is that
>> pixel clock enablement can be moved from LCDIF driver to
>> fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change.
> 
> TBH, this sounds like a hack and is no longer required with this series.

Pixel clock is an input signal to LDB, which is reflected in LDB block
diagram in i.MX8MP TRM(see Figure 13-70) where "CLOCK" goes into LDB
along with "DATA", "HSYNC", "VSYNC", "DATA_EN" and "CONTROL".  So,
fsl,ldb.yaml should have documented the pixel clock in clocks and
clock-names properties, but unfortunately it doesn't and it's too late
to do so.  The workaround is to get the pixel clock from LCDIF DT node
in the LDB driver.  I would call it a workaround rather than a hack,
since fsl,ldb.yaml should have documented the pixel clock in the first
place.

> 
> You are just trying to circumvent the fact that until now, applying a
> rate in an upper clock would unconfigure the downstream rates, and I
> think this is our first real problem.

I'm still not a fan of setting CLK_SET_RATE_PARENT flag to the LDB clock
and pixel clocks.  If we look at the bigger picture, the first real
problem is probably how to support both separated video PLLs and shared
video PLL.

> 
>> https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/
>>
>> Actually, one sibling patch of the above patch reverts ff06ea04e4cf
>> because I thought "fixed PLL rate" is the only solution, though I'm
>> discussing any alternative solution of "dynamically changeable PLL
>> rate" with Marek in the thread of the sibling patch.
> 
> I don't think we want fixed PLL rates. Especially if you start using

I don't want fixed PLL rates, either...

> external (hot-pluggable) displays with different needs: it just does not

... but, considering the problem of how to support separated/shared video
PLLs, I think we have to accept the fixed PLL rates.  So, unfortunately
some video modes read from EDID cannot fly.  If there is an feasible
alternative solution, it will be good to implement it, but till now I
don't see any.

> fly. There is one situation that cannot yet be handled and needs
> manual reparenting: using 3 displays with a non-divisible pixel
> frequency.
> 
> FYI we managed this specific "advanced" case with assigned-clock-parents
> using an audio PLL as hinted by Marek. It mostly works, event though the
> PLL1416 are less precise and offer less accurate pixel clocks.

Good to know this.  I think if the audio PLL is free(e.g., it is not
used by audio subsystem) on your i.MX8MP platform, you may use it for
video output. However, i.MX8MP EVK made by NXP has to use shared video
PLL between LVDS and MIPI DSI display pipelines, because all audio PLLs
and PLL3 are supposed to be used by audio subsystem.

> 
>> BTW, as you know the LDB clock rate is 3.5x faster than the pixel
>> clock rate in dual-link LVDS use cases, the lowest PLL rate needs to
>> be explicitly set to 7x faster than the pixel clock rate *before*
>> LDB clock rate is set.  This way, the pixel clock would be derived
>> from the PLL with integer division ratio = 7, not the unsupported
>> 3.5.
>>
>> pixel    LDB         PLL
>> A        3.5*A       7*A      --> OK
>> A        3.5*A       3.5*A    --> not OK
> 
> This series was mostly solving the simpler case, with one display, but I
> agree we should probably also consider the dual case.
> 
> The situation here is that you require the LDB to be aware of some
> clocks constraints, like the fact that the downstream pixel clocks only
> feature simple dividors which cannot achieve a 3.5 rate. That is all.
> 
> It is clearly the LDB driver duty to make this feasible. I cannot test
> the dual case so I didn't brought any solution to it in this series, but
> I already had a solution in mind. Please find a patch below, it is very
> simple, and should, in conjunction with this series, fix the dual case
> as well.
> 
> FYI here is the final clock tree with this trick "manually" enabled. You
> can see video_pll1 is now twice media_ldb, and media ldb is still 7
> times media_disp[12]_pix (video_pll1 is assigned in DT to 1039500000, so
> it has effectively been reconfigured).
> 
>        video_pll1                            1   1   0   1006600000
>           video_pll1_bypass                  1   1   0   1006600000
>              video_pll1_out                  2   2   0   1006600000
>                 media_ldb                    1   1   0    503300000
>                    media_ldb_root_clk        1   1   0    503300000
>                 media_disp2_pix              1   1   0     71900000
>                    media_disp2_pix_root_clk  1   1   0     71900000
>                 media_disp1_pix              0   0   0     71900000
>                    media_disp1_pix_root_clk  0   0   0     71900000
> 
> ---8<---
> Author: Miquel Raynal <miquel.raynal@bootlin.com>
> 
>     drm: bridge: ldb: Make sure the upper PLL is compatible with dual output
>     
>     The i.MX8 display pipeline has a number of clock constraints, among which:
>     - The bridge clock must be 7 times faster than the pixel clock in single mode
>     - The bridge clock must be 3.5 times faster than the pixel clocks in dual mode
>     While a ratio of 7 is easy to build with simple divisors, 3.5 is not
>     achievable. In order to make sure we keep these clock ratios correct is
>     to configure the upper clock (usually video_pll1, but that does not
>     matter really) to twice it's usual value. This way, the bridge clock is
>     configured to divide the upstream rate by 2, and the pixel clocks are
>     configured to divide the upstream rate by 7, achieving a final 3.5 ratio
>     between the two.
>     
>     Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> index 81ff4e5f52fa..069c960ee56b 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -177,6 +177,17 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>         mode = &crtc_state->adjusted_mode;
>  
>         requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
> +       /*
> +        * Dual cases require a 3.5 rate division on the pixel clocks, which
> +        * cannot be achieved with regular single divisors. Instead, double the
> +        * parent PLL rate in the first place. In order to do that, we first
> +        * require twice the target clock rate, which will program the upper
> +        * PLL. Then, we ask for the actual targeted rate, which can be achieved
> +        * by dividing by 2 the already configured upper PLL rate, without
> +        * making further changes to it.
> +        */
> +       if (fsl_ldb_is_dual(fsl_ldb))
> +               clk_set_rate(fsl_ldb->clk, requested_link_freq * 2);

I don't think i.MX6SX LDB needs this, because the "ldb" clock's parent
is a mux clock with "ldb_di0_div_3_5" or "ldb_di0_div_7" parents.

>         clk_set_rate(fsl_ldb->clk, requested_link_freq);
>  
>         configured_link_freq = clk_get_rate(fsl_ldb->clk);

-- 
Regards,
Liu Ying

Re: [PATCH 0/5] clk: Fix simple video pipelines on i.MX8

[Miquel Raynal]

Hi Liu,

>>> The
>>> pixel clock can be got from LDB's remote input LCDIF DT node by
>>> calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch
>>> does. Similar to setting pixel clock rate, I think a chance is that
>>> pixel clock enablement can be moved from LCDIF driver to
>>> fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change.
>> 
>> TBH, this sounds like a hack and is no longer required with this series.
>
> Pixel clock is an input signal to LDB, which is reflected in LDB block
> diagram in i.MX8MP TRM(see Figure 13-70) where "CLOCK" goes into LDB
> along with "DATA", "HSYNC", "VSYNC", "DATA_EN" and "CONTROL".  So,
> fsl,ldb.yaml should have documented the pixel clock in clocks and
> clock-names properties, but unfortunately it doesn't and it's too late
> to do so.  The workaround is to get the pixel clock from LCDIF DT node
> in the LDB driver.  I would call it a workaround rather than a hack,
> since fsl,ldb.yaml should have documented the pixel clock in the first
> place.
>
>> 
>> You are just trying to circumvent the fact that until now, applying a
>> rate in an upper clock would unconfigure the downstream rates, and I
>> think this is our first real problem.
>
> I'm still not a fan of setting CLK_SET_RATE_PARENT flag to the LDB clock
> and pixel clocks.  If we look at the bigger picture, the first real
> problem is probably how to support both separated video PLLs and shared
> video PLL.
>
>> 
>>> https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/
>>>
>>> Actually, one sibling patch of the above patch reverts ff06ea04e4cf
>>> because I thought "fixed PLL rate" is the only solution, though I'm
>>> discussing any alternative solution of "dynamically changeable PLL
>>> rate" with Marek in the thread of the sibling patch.
>> 
>> I don't think we want fixed PLL rates. Especially if you start using
>
> I don't want fixed PLL rates, either...
>
>> external (hot-pluggable) displays with different needs: it just does not
>
> ... but, considering the problem of how to support separated/shared video
> PLLs, I think we have to accept the fixed PLL rates.  So, unfortunately
> some video modes read from EDID cannot fly.  If there is an feasible
> alternative solution, it will be good to implement it, but till now I
> don't see any.

Can you please remind me what your exact display setup (and their
required pixel clocks) is?

AFAIU, you don't want to use dynamic calculations for the PLL because it
breaks your use case with HDMI. Of course this is a very limited use
case, because using a static rate means almost a single type of display
can be plugged.

The clock subsystem will not recalculate the video_pll1 if you can
achieve a perfect rate change using the LDB/PIX divisors. So let me
propose the below addition to this series. With the below diff, your
setup should still work with assigned clock rates, while letting us
handle our calculations dynamically.

The addition I am now proposing is to remove CLK_SET_RATE_PARENT to both
media_disp[12]_pix clocks. This should actually fix your situation while
keeping pixel clocks accurate as far it is possible as the LDB clock
will change video_pll1 only if the PLL rate is not suitable for it in
the first place. And then, there will be no other clock messing with
this PLL. This is probably a safer approach, which should still allow
accurate dynamic rate calculations for "simple" setups *and* let the
static assignations work otherwise:

-       hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT | CLK_NO_RATE_CHANGE_DURING_PROPAGATION);
+       hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_NO_RATE_CHANGE_DURING_PROPAGATION);
[...]
-       hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT | CLK_NO_RATE_CHANGE_DURING_PROPAGATION);
+       hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_NO_RATE_CHANGE_DURING_PROPAGATION);

Can you please give this proposal a try?

[...]

>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
>> @@ -177,6 +177,17 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>>         mode = &crtc_state->adjusted_mode;
>>  
>>         requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
>> +       /*
>> +        * Dual cases require a 3.5 rate division on the pixel clocks, which
>> +        * cannot be achieved with regular single divisors. Instead, double the
>> +        * parent PLL rate in the first place. In order to do that, we first
>> +        * require twice the target clock rate, which will program the upper
>> +        * PLL. Then, we ask for the actual targeted rate, which can be achieved
>> +        * by dividing by 2 the already configured upper PLL rate, without
>> +        * making further changes to it.
>> +        */
>> +       if (fsl_ldb_is_dual(fsl_ldb))
>> +               clk_set_rate(fsl_ldb->clk, requested_link_freq * 2);
>
> I don't think i.MX6SX LDB needs this, because the "ldb" clock's parent
> is a mux clock with "ldb_di0_div_3_5" or "ldb_di0_div_7" parents.

Ah, you mean we should only do this in the imx8 case, right?

Thanks,
Miquèl

Re: [PATCH 0/5] clk: Fix simple video pipelines on i.MX8

[Liu Ying]

On 11/26/2024, Miquel Raynal wrote:
> Hi Liu,

Hi,

> 
>>>> The
>>>> pixel clock can be got from LDB's remote input LCDIF DT node by
>>>> calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch
>>>> does. Similar to setting pixel clock rate, I think a chance is that
>>>> pixel clock enablement can be moved from LCDIF driver to
>>>> fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change.
>>>
>>> TBH, this sounds like a hack and is no longer required with this series.
>>
>> Pixel clock is an input signal to LDB, which is reflected in LDB block
>> diagram in i.MX8MP TRM(see Figure 13-70) where "CLOCK" goes into LDB
>> along with "DATA", "HSYNC", "VSYNC", "DATA_EN" and "CONTROL".  So,
>> fsl,ldb.yaml should have documented the pixel clock in clocks and
>> clock-names properties, but unfortunately it doesn't and it's too late
>> to do so.  The workaround is to get the pixel clock from LCDIF DT node
>> in the LDB driver.  I would call it a workaround rather than a hack,
>> since fsl,ldb.yaml should have documented the pixel clock in the first
>> place.
>>
>>>
>>> You are just trying to circumvent the fact that until now, applying a
>>> rate in an upper clock would unconfigure the downstream rates, and I
>>> think this is our first real problem.
>>
>> I'm still not a fan of setting CLK_SET_RATE_PARENT flag to the LDB clock
>> and pixel clocks.  If we look at the bigger picture, the first real
>> problem is probably how to support both separated video PLLs and shared
>> video PLL.
>>
>>>
>>>> https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/
>>>>
>>>> Actually, one sibling patch of the above patch reverts ff06ea04e4cf
>>>> because I thought "fixed PLL rate" is the only solution, though I'm
>>>> discussing any alternative solution of "dynamically changeable PLL
>>>> rate" with Marek in the thread of the sibling patch.
>>>
>>> I don't think we want fixed PLL rates. Especially if you start using
>>
>> I don't want fixed PLL rates, either...
>>
>>> external (hot-pluggable) displays with different needs: it just does not
>>
>> ... but, considering the problem of how to support separated/shared video
>> PLLs, I think we have to accept the fixed PLL rates.  So, unfortunately
>> some video modes read from EDID cannot fly.  If there is an feasible
>> alternative solution, it will be good to implement it, but till now I
>> don't see any.
> 
> Can you please remind me what your exact display setup (and their
> required pixel clocks) is?

i.MX8MP EVK[1] supports MIPI DSI, LVDS and native HDMI outputs.
MIPI DSI and LVDS outputs use MINI-SAS connectors and HDMI output
uses a HDMI type A connector.
You may easily find the connectors in the EVK pictures in [1].
IMX-MIPI-HDMI(ADV7535), IMX-LVDS-HDMI(IT6263 single LVDS link) and
MX8-DSI-OLED1A(RAYDIUM RM67199 DSI panel) accessory boards[2] can
be connected/plugged to the EVK through the MINI-SAS connectors.
In addition, MX8-DSI-OLED1(RAYDIUM RM67191 DSI panel),
IMX-DLVDS-HDMI(IT6263 single DLVDS link) and
MX8-DLVDS-LCD1(koe,tx26d202vm0bwa dual-link LVDS panel) can also
be connected to the EVK through the MINI-SAS connectors, though
not listed in [2].

So, putting native HDMI output aside, there are quite a few MIPI
DSI + LVDS display device combinations to be supported with
a single video PLL.

For ADV7535 and IT6263, they support typical 1080p/720p video modes
read from EDID with 148.5MHz and 74.25MHz pixel clock rates.

For MX8-DLVDS-LCD1 LVDS panel, it is supported with in-tree
imx8mp-evk-mx8-dlvds-lcd1.dtso where nominal 156.72MHz pixel clock
rate is overridden to be 148.5MHz with a panel-timing DT node,
considering the fixed video PLL rate @1.0395GHz assigned in
imx8mp.dtsi.

For MX8-DSI-OLED1 and MX8-DSI-OLED1A MIPI DSI panels, both have
nominal 132MHz pixel clock rate(see panel-raydium-rm67191.c). NXP
downstream kernel uses 148.5MHz pixel clock rate for the two panel.

In short, I use 148.5MHz or 74.25MHz for ADV7535, IT6263 and the
LVDS panel.  I haven't tried the MIPI DSI panels with upstream
kernel yet.

[1] https://www.nxp.com/design/design-center/development-boards-and-designs/i-mx-evaluation-and-development-boards/evaluation-kit-for-the-i-mx-8m-plus-applications-processor:8MPLUSLPD4-EVK
[2] https://www.nxp.com/design/design-center/development-boards-and-designs/i-mx-evaluation-and-development-boards/i-mx-8-series-accessory-boards:i.MX8-ACCESSORY-BOARDS

> 
> AFAIU, you don't want to use dynamic calculations for the PLL because it
> breaks your use case with HDMI. Of course this is a very limited use
> case, because using a static rate means almost a single type of display
> can be plugged.
> 
> The clock subsystem will not recalculate the video_pll1 if you can
> achieve a perfect rate change using the LDB/PIX divisors. So let me
> propose the below addition to this series. With the below diff, your
> setup should still work with assigned clock rates, while letting us
> handle our calculations dynamically.
> 
> The addition I am now proposing is to remove CLK_SET_RATE_PARENT to both
> media_disp[12]_pix clocks. This should actually fix your situation while
> keeping pixel clocks accurate as far it is possible as the LDB clock
> will change video_pll1 only if the PLL rate is not suitable for it in
> the first place. And then, there will be no other clock messing with
> this PLL. This is probably a safer approach, which should still allow
> accurate dynamic rate calculations for "simple" setups *and* let the
> static assignations work otherwise:
> 
> -       hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT | CLK_NO_RATE_CHANGE_DURING_PROPAGATION);
> +       hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_NO_RATE_CHANGE_DURING_PROPAGATION);
> [...]
> -       hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT | CLK_NO_RATE_CHANGE_DURING_PROPAGATION);
> +       hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_NO_RATE_CHANGE_DURING_PROPAGATION);

Removing CLK_SET_RATE_PARENT from the two pixel clocks is also done
with my patch[3]. 

[3] https://patchwork.freedesktop.org/patch/624509/?series=139266&rev=7

> 
> Can you please give this proposal a try?

I tried this/your patch set + your additional patch(remove
CLK_SET_RATE_PARENT for pixel clocks) + your additional patch(set LDB
clock rate to 2x requested link rate for dual-link LVDS)[4] + my two
patches[5][6] with ADV7535 + IT6263 on i.MX8MP EVK.  I don't see
any particular issue for both single and dual link LVDS cases. Some
typical video modes(1080p60/50, 720p60) work ok for ADV7535, though
there is still no display for a few video modes(should be not caused
by your patch set because there is no mode validation logic for the
MIPI DSI display pipeline).  So, basically, this has the same test
result if simply only applying my patch set[7].

I think the key change for your patch set to work for my test case
is your additional patch(remove CLK_SET_RATE_PARENT for pixel clocks).
Together with [4][5][6], the video PLL rate is fixed to 1.0395GHz for
all video modes.

Back to the separated/shared PLLs problem, I still don't see any
feasible alternative solution till now, since you are proposing to
remove CLK_SET_RATE_PARENT for pixel clocks which defeats the idea
of dynamically changing PLL rate(at least for the MIPI DSI display
pipeline).

Also, I feel that CLK_NO_RATE_CHANGE_DURING_PROPAGATION is not
needed, because the pixel clock rate can be set in
fsl_ldb_atomic_enable() as I mentioned before.

[5] https://patchwork.freedesktop.org/patch/624511/?series=139266&rev=7
[6] https://patchwork.freedesktop.org/patch/624512/?series=139266&rev=7
[7] https://patchwork.freedesktop.org/series/139266/#rev7

> 
> [...]
> 
>>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
>>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
>>> @@ -177,6 +177,17 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>>>         mode = &crtc_state->adjusted_mode;
>>>  
>>>         requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
>>> +       /*
>>> +        * Dual cases require a 3.5 rate division on the pixel clocks, which
>>> +        * cannot be achieved with regular single divisors. Instead, double the
>>> +        * parent PLL rate in the first place. In order to do that, we first
>>> +        * require twice the target clock rate, which will program the upper
>>> +        * PLL. Then, we ask for the actual targeted rate, which can be achieved
>>> +        * by dividing by 2 the already configured upper PLL rate, without
>>> +        * making further changes to it.
>>> +        */
>>> +       if (fsl_ldb_is_dual(fsl_ldb))
>>> +               clk_set_rate(fsl_ldb->clk, requested_link_freq * 2);
>>
>> I don't think i.MX6SX LDB needs this, because the "ldb" clock's parent
>> is a mux clock with "ldb_di0_div_3_5" or "ldb_di0_div_7" parents.
> 
> Ah, you mean we should only do this in the imx8 case, right?

I think that doing this does no harm for both i.MX8MP LDB and i.MX93
LDB.

> 
> Thanks,
> Miquèl

-- 
Regards,
Liu Ying

Re: [PATCH 4/5] clk: Add flag to prevent frequency changes when walking subtrees

[Stephen Boyd]

Quoting Miquel Raynal (2024-11-21 09:41:14)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index adfc5bfb93b5a65b6f58c52ca2c432d651f7dd7d..94d93470479e77769e63e97462b176261103b552 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1927,7 +1927,6 @@ long clk_get_accuracy(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_get_accuracy);
>  
> -__maybe_unused
>  static unsigned long clk_determine(struct clk_core *core, unsigned long rate)
>  {
>         struct clk_rate_request req = {};

Please add functions in the same patch that uses them. It is hard to
review this patch when half the context is in another patch.

> @@ -2272,7 +2271,13 @@ static void clk_calc_subtree(struct clk_core *core)
>  {
>         struct clk_core *child;
>  
> -       core->new_rate = clk_recalc(core, core->parent->new_rate);
> +       if (core->flags & CLK_NO_RATE_CHANGE_DURING_PROPAGATION) {
> +               core->new_rate = clk_determine(core, core->rate);
> +               if (!core->new_rate)
> +                       core->new_rate = clk_recalc(core, core->parent->new_rate);
> +       } else {
> +               core->new_rate = clk_recalc(core, core->parent->new_rate);
> +       }
>  
>         hlist_for_each_entry(child, &core->children, child_node)
>                 clk_calc_subtree(child);
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 200135e0f6d00d48b10e843259333b9733c97f38..baef0b442ac1d36ee935cbcaaaa4e2d95fe7654c 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -38,6 +38,8 @@
>  #define CLK_OPS_PARENT_ENABLE  BIT(12)
>  /* duty cycle call may be forwarded to the parent clock */
>  #define CLK_DUTY_CYCLE_PARENT  BIT(13)
> +/* do not passively change this clock rate during subtree rate propagation */
> +#define CLK_NO_RATE_CHANGE_DURING_PROPAGATION BIT(14)

Why doesn't rate locking work?

Re: [PATCH 4/5] clk: Add flag to prevent frequency changes when walking subtrees

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3695 bytes --]

On Thu, Nov 21, 2024 at 06:41:14PM +0100, Miquel Raynal wrote:
> There are mainly two ways to change a clock frequency.

There's much more than that :)

Off the top of my head, setting/clearing a min/max rate and changing the
parent might also result in a rate change.

And then, the firmware might get involved too.

> The active way requires calling ->set_rate() in order to ask "on
> purpose" for a frequency change. Otherwise, a clock can passively see
> its frequency being updated depending on upstream clock frequency
> changes. In most cases it is fine to just accept the new upstream
> frequency - which by definition will have an impact on downstream
> frequencies if we do not recalculate internal divisors. But there are
> cases where, upon an upstream frequency change, we would like to
> maintain a specific rate.

Why is clk_set_rate_exclusive not enough?

> As an example, on iMX8MP the video pipeline clocks are looking like this:
> 
>     video_pll1
>        video_pll1_bypass
>           video_pll1_out
>              media_ldb
>                 media_ldb_root_clk
>              media_disp2_pix
>                 media_disp2_pix_root_clk
>              media_disp1_pix
>                 media_disp1_pix_root_clk
> 
> media_ldb, media_disp2_pix and media_disp1_pix are simple divisors from
> which we might require 2 or 3 different rates, whereas video_pll1 is a
> very configurable PLL which can achieve almost any frequency. There are
> however relationships between them, typically the ldb clock must be 3.5
> or 7 times higher than the media_disp* clocks.
> 
> Currently, if eg. media_disp2_pix is set to 71900000Hz, when media_ldb
> is (later) set to 503300000Hz, media_disp2_pix is updated to 503300000Hz
> as well, which clearly does not make sense. We want it to stay at
> 71900000Hz during the subtree walk.
> 
> Achieving this is the purpose of the new clock flag:
> CLK_NO_RATE_CHANGE_DURING_PROPAGATION
> 
> Please note, if the kernel was setting the ldb clock before a pixel
> clock, the result would be different, and this is also what this patch
> is trying to solve.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> In all cases, the LDB must be aware of the device configuration, and ask
> for a clever frequency, we will never cope with slightly aware drivers
> when addressing this kind of subtle situation.
> ---
>  drivers/clk/clk.c            | 9 +++++++--
>  include/linux/clk-provider.h | 2 ++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index adfc5bfb93b5a65b6f58c52ca2c432d651f7dd7d..94d93470479e77769e63e97462b176261103b552 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1927,7 +1927,6 @@ long clk_get_accuracy(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_get_accuracy);
>  
> -__maybe_unused
>  static unsigned long clk_determine(struct clk_core *core, unsigned long rate)
>  {
>  	struct clk_rate_request req = {};
> @@ -2272,7 +2271,13 @@ static void clk_calc_subtree(struct clk_core *core)
>  {
>  	struct clk_core *child;
>  
> -	core->new_rate = clk_recalc(core, core->parent->new_rate);
> +	if (core->flags & CLK_NO_RATE_CHANGE_DURING_PROPAGATION) {
> +		core->new_rate = clk_determine(core, core->rate);
> +		if (!core->new_rate)
> +			core->new_rate = clk_recalc(core, core->parent->new_rate);
> +	} else {
> +		core->new_rate = clk_recalc(core, core->parent->new_rate);
> +	}

Sorry, it's not clear to me how it works. How will the parent clocks
will get notified to adjust their dividers in that scenario? Also, what
if they can't?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 0/5] clk: Fix simple video pipelines on i.MX8

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 10072 bytes --]

On Fri, Nov 22, 2024 at 10:54:55AM +0100, Miquel Raynal wrote:
> Hello Liu,
> 
> Thanks for the feedback!
> 
> On 22/11/2024 at 14:01:49 +08, Liu Ying <victor.liu@nxp.com> wrote:
> 
> > Hi Miquel,
> >
> > On 11/22/2024, Miquel Raynal wrote:
> >> Recent changes in the clock tree have set CLK_SET_RATE_PARENT to the two
> >> LCDIF pixel clocks. The idea is, instead of using assigned-clock
> >> properties to set upstream PLL rates to high frequencies and hoping that
> >> a single divisor (namely media_disp[12]_pix) will be close enough in
> >> most cases, we should tell the clock core to use the PLL to properly
> >> derive an accurate pixel clock rate in the first place. Here is the
> >> situation.
> >> 
> >> [Before ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")]
> >> 
> >> Before setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the sequence of events was:
> >> - PLL is assigned to a high rate,
> >> - media_disp[12]_pix is set to approximately freq A by using a single divisor,
> >> - media_ldb is set to approximately freq 7*A by using another single divisor.
> >> => The display was working, but the pixel clock was inaccurate.
> >> 
> >> [After ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")]
> >> 
> >> After setting CLK_SET_RATE_PARENT to the media_disp[12]_pix clocks, the
> >> sequence of events became:
> >> - media_disp[12]_pix is set to freq A by using a divisor of 1 and
> >>   setting video_pll1 to freq A.
> >> - media_ldb is trying to compute its divisor to set freq 7*A, but the
> >>   upstream PLL is to low, it does not recompute it, so it ends up
> >>   setting a divisor of 1 and being at freq A instead of 7*A.
> >> => The display is sadly no longer working
> >> 
> >> [After applying PATCH "clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate"]
> >> 
> >> This is a commit from Marek, which is, I believe going in the right
> >> direction, so I am including it. Just with this change, the situation is
> >> slightly different, but the result is the same:
> >> - media_disp[12]_pix is set to freq A by using a divisor of 1 and
> >>   setting video_pll1 to freq A.
> >> - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
> >>   to freq 7*A.
> >>   /!\ This as the side effect of changing media_disp[12]_pix from freq A
> >>   to freq 7*A.
> >
> > Although I'm not of a fan of setting CLK_SET_RATE_PARENT flag to the
> > LDB clock and pixel clocks,
> 
> I haven't commented much on this. For me, inaccurate pixel clocks mostly
> work fine (if not too inaccurate), but it is true that having very
> powerful PLL like the PLL1443, it is a pity not to use them at their
> highest capabilities. However, I consider "not breaking users" more
> important than having "perfect clock rates".

Whether an inaccurate clock "works" depends on the context. A .5%
deviation will be out of spec for HDMI for example. An inaccurate VBLANK
frequency might also break some use cases.

So that your display still works is not enough to prove it works.

> This series has one unique goal: accepting more accurate frequencies
> *and* not breaking users in the most simplest cases.
> 
> > would it work if the pixel clock rate is
> > set after the LDB clock rate is set in fsl_ldb_atomic_enable()?
> 
> The situation would be:
> - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
>   to freq 7*A.
> - media_disp[12]_pix is set to freq A by using a divisor of 7.
> 
> So yes, and the explanation of why is there:
> https://elixir.bootlin.com/linux/v6.11.8/source/drivers/clk/clk-divider.c#L322
> 
> > The
> > pixel clock can be got from LDB's remote input LCDIF DT node by
> > calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch
> > does. Similar to setting pixel clock rate, I think a chance is that
> > pixel clock enablement can be moved from LCDIF driver to
> > fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change.
> 
> TBH, this sounds like a hack and is no longer required with this series.
> 
> You are just trying to circumvent the fact that until now, applying a
> rate in an upper clock would unconfigure the downstream rates, and I
> think this is our first real problem.
> 
> > https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/
> >
> > Actually, one sibling patch of the above patch reverts ff06ea04e4cf
> > because I thought "fixed PLL rate" is the only solution, though I'm
> > discussing any alternative solution of "dynamically changeable PLL
> > rate" with Marek in the thread of the sibling patch.
> 
> I don't think we want fixed PLL rates. Especially if you start using
> external (hot-pluggable) displays with different needs: it just does not
> fly. There is one situation that cannot yet be handled and needs
> manual reparenting: using 3 displays with a non-divisible pixel
> frequency.

Funnily, external interfaces (ie, HDMI, DP) tend to be easier to work
with than panels. HDMI for example works with roughly three frequencies:
148.5MHz, 297MHz and 594MHz. If you set the PLL to 594MHz and the
downstream clock has a basic divider, it works just fine.

> FYI we managed this specific "advanced" case with assigned-clock-parents
> using an audio PLL as hinted by Marek. It mostly works, event though the
> PLL1416 are less precise and offer less accurate pixel clocks.

Note that assigned-clock-parents doesn't provide any guarantee on
whether the parent will change in the future or not. It's strictly
equivalent to calling clk_set_parent in the driver's probe.

> > BTW, as you know the LDB clock rate is 3.5x faster than the pixel
> > clock rate in dual-link LVDS use cases, the lowest PLL rate needs to
> > be explicitly set to 7x faster than the pixel clock rate *before*
> > LDB clock rate is set.  This way, the pixel clock would be derived
> > from the PLL with integer division ratio = 7, not the unsupported
> > 3.5.
> >
> > pixel    LDB         PLL
> > A        3.5*A       7*A      --> OK
> > A        3.5*A       3.5*A    --> not OK
> 
> This series was mostly solving the simpler case, with one display, but I
> agree we should probably also consider the dual case.
> 
> The situation here is that you require the LDB to be aware of some
> clocks constraints, like the fact that the downstream pixel clocks only
> feature simple dividors which cannot achieve a 3.5 rate. That is all.
> 
> It is clearly the LDB driver duty to make this feasible. I cannot test
> the dual case so I didn't brought any solution to it in this series, but
> I already had a solution in mind. Please find a patch below, it is very
> simple, and should, in conjunction with this series, fix the dual case
> as well.
> 
> FYI here is the final clock tree with this trick "manually" enabled. You
> can see video_pll1 is now twice media_ldb, and media ldb is still 7
> times media_disp[12]_pix (video_pll1 is assigned in DT to 1039500000, so
> it has effectively been reconfigured).
> 
>        video_pll1                            1   1   0   1006600000
>           video_pll1_bypass                  1   1   0   1006600000
>              video_pll1_out                  2   2   0   1006600000
>                 media_ldb                    1   1   0    503300000
>                    media_ldb_root_clk        1   1   0    503300000
>                 media_disp2_pix              1   1   0     71900000
>                    media_disp2_pix_root_clk  1   1   0     71900000
>                 media_disp1_pix              0   0   0     71900000
>                    media_disp1_pix_root_clk  0   0   0     71900000
> 
> ---8<---
> Author: Miquel Raynal <miquel.raynal@bootlin.com>
> 
>     drm: bridge: ldb: Make sure the upper PLL is compatible with dual output
>     
>     The i.MX8 display pipeline has a number of clock constraints, among which:
>     - The bridge clock must be 7 times faster than the pixel clock in single mode
>     - The bridge clock must be 3.5 times faster than the pixel clocks in dual mode
>     While a ratio of 7 is easy to build with simple divisors, 3.5 is not
>     achievable. In order to make sure we keep these clock ratios correct is
>     to configure the upper clock (usually video_pll1, but that does not
>     matter really) to twice it's usual value. This way, the bridge clock is
>     configured to divide the upstream rate by 2, and the pixel clocks are
>     configured to divide the upstream rate by 7, achieving a final 3.5 ratio
>     between the two.
>     
>     Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> index 81ff4e5f52fa..069c960ee56b 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -177,6 +177,17 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>         mode = &crtc_state->adjusted_mode;
>  
>         requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
> +       /*
> +        * Dual cases require a 3.5 rate division on the pixel clocks, which
> +        * cannot be achieved with regular single divisors. Instead, double the
> +        * parent PLL rate in the first place. In order to do that, we first
> +        * require twice the target clock rate, which will program the upper
> +        * PLL. Then, we ask for the actual targeted rate, which can be achieved
> +        * by dividing by 2 the already configured upper PLL rate, without
> +        * making further changes to it.
> +        */
> +       if (fsl_ldb_is_dual(fsl_ldb))
> +               clk_set_rate(fsl_ldb->clk, requested_link_freq * 2);
>         clk_set_rate(fsl_ldb->clk, requested_link_freq);

This has nothing to do in a DRM driver. The clock driver logic needs to handle it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 4/5] clk: Add flag to prevent frequency changes when walking subtrees

[Miquel Raynal]

Hi Stephen,

>> +/* do not passively change this clock rate during subtree rate propagation */
>> +#define CLK_NO_RATE_CHANGE_DURING_PROPAGATION BIT(14)
>
> Why doesn't rate locking work?

Can you be more specific? What function from the API is supposed to do
what I need? AFAIU, none of them is properly locking the rate during a
subtree walk, but if I misread one of them, I'd be glad to drop all this.

Thanks,
Miquèl

Re: [PATCH 4/5] clk: Add flag to prevent frequency changes when walking subtrees

[Miquel Raynal]

Hi Maxime,

On 17/12/2024 at 13:47:53 +01, Maxime Ripard <mripard@redhat.com> wrote:

> On Thu, Nov 21, 2024 at 06:41:14PM +0100, Miquel Raynal wrote:
>> There are mainly two ways to change a clock frequency.
>
> There's much more than that :)

"mainly"

Or maybe I should have added "on purpose".

>
> Off the top of my head, setting/clearing a min/max rate and changing the
> parent might also result in a rate change.
>
> And then, the firmware might get involved too.
>
>> The active way requires calling ->set_rate() in order to ask "on
>> purpose" for a frequency change. Otherwise, a clock can passively see
>> its frequency being updated depending on upstream clock frequency
>> changes. In most cases it is fine to just accept the new upstream
>> frequency - which by definition will have an impact on downstream
>> frequencies if we do not recalculate internal divisors. But there are
>> cases where, upon an upstream frequency change, we would like to
>> maintain a specific rate.
>
> Why is clk_set_rate_exclusive not enough?

I am trying to protect these rate changes from subtree walks, I don't
see where setting an exclusive rate would have an effect? But I might be
overlooking something, definitely.

...

>> @@ -2272,7 +2271,13 @@ static void clk_calc_subtree(struct clk_core *core)
>>  {
>>  	struct clk_core *child;
>>  
>> -	core->new_rate = clk_recalc(core, core->parent->new_rate);
>> +	if (core->flags & CLK_NO_RATE_CHANGE_DURING_PROPAGATION) {
>> +		core->new_rate = clk_determine(core, core->rate);
>> +		if (!core->new_rate)
>> +			core->new_rate = clk_recalc(core, core->parent->new_rate);
>> +	} else {
>> +		core->new_rate = clk_recalc(core, core->parent->new_rate);
>> +	}
>
> Sorry, it's not clear to me how it works. How will the parent clocks
> will get notified to adjust their dividers in that scenario? Also, what
> if they can't?

The idea is: if the flag is set, instead of accepting the new upstream
rate and recalculate the downstream rate based on a previously set
divider value, we change our divider value to match the same frequency
as before. But if we cannot, then we just keep the old way.

Cheers,
Miquèl

Re: [PATCH 0/5] clk: Fix simple video pipelines on i.MX8

[Miquel Raynal]

Hello,

>> >> [After applying PATCH "clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate"]
>> >> 
>> >> This is a commit from Marek, which is, I believe going in the right
>> >> direction, so I am including it. Just with this change, the situation is
>> >> slightly different, but the result is the same:
>> >> - media_disp[12]_pix is set to freq A by using a divisor of 1 and
>> >>   setting video_pll1 to freq A.
>> >> - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
>> >>   to freq 7*A.
>> >>   /!\ This as the side effect of changing media_disp[12]_pix from freq A
>> >>   to freq 7*A.
>> >
>> > Although I'm not of a fan of setting CLK_SET_RATE_PARENT flag to the
>> > LDB clock and pixel clocks,
>> 
>> I haven't commented much on this. For me, inaccurate pixel clocks mostly
>> work fine (if not too inaccurate), but it is true that having very
>> powerful PLL like the PLL1443, it is a pity not to use them at their
>> highest capabilities. However, I consider "not breaking users" more
>> important than having "perfect clock rates".
>
> Whether an inaccurate clock "works" depends on the context. A .5%
> deviation will be out of spec for HDMI for example. An inaccurate VBLANK
> frequency might also break some use cases.
>
> So that your display still works is not enough to prove it works.

Well, my display used to work. And now it no longer does. The perfect
has become the enemy of the good.

>> This series has one unique goal: accepting more accurate frequencies
>> *and* not breaking users in the most simplest cases.
>> 
>> > would it work if the pixel clock rate is
>> > set after the LDB clock rate is set in fsl_ldb_atomic_enable()?
>> 
>> The situation would be:
>> - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
>>   to freq 7*A.
>> - media_disp[12]_pix is set to freq A by using a divisor of 7.
>> 
>> So yes, and the explanation of why is there:
>> https://elixir.bootlin.com/linux/v6.11.8/source/drivers/clk/clk-divider.c#L322
>> 
>> > The
>> > pixel clock can be got from LDB's remote input LCDIF DT node by
>> > calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch
>> > does. Similar to setting pixel clock rate, I think a chance is that
>> > pixel clock enablement can be moved from LCDIF driver to
>> > fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change.
>> 
>> TBH, this sounds like a hack and is no longer required with this series.
>> 
>> You are just trying to circumvent the fact that until now, applying a
>> rate in an upper clock would unconfigure the downstream rates, and I
>> think this is our first real problem.
>> 
>> > https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/
>> >
>> > Actually, one sibling patch of the above patch reverts ff06ea04e4cf
>> > because I thought "fixed PLL rate" is the only solution, though I'm
>> > discussing any alternative solution of "dynamically changeable PLL
>> > rate" with Marek in the thread of the sibling patch.
>> 
>> I don't think we want fixed PLL rates. Especially if you start using
>> external (hot-pluggable) displays with different needs: it just does not
>> fly. There is one situation that cannot yet be handled and needs
>> manual reparenting: using 3 displays with a non-divisible pixel
>> frequency.
>
> Funnily, external interfaces (ie, HDMI, DP) tend to be easier to work
> with than panels. HDMI for example works with roughly three frequencies:
> 148.5MHz, 297MHz and 594MHz. If you set the PLL to 594MHz and the
> downstream clock has a basic divider, it works just fine.
>
>> FYI we managed this specific "advanced" case with assigned-clock-parents
>> using an audio PLL as hinted by Marek. It mostly works, event though the
>> PLL1416 are less precise and offer less accurate pixel clocks.
>
> Note that assigned-clock-parents doesn't provide any guarantee on
> whether the parent will change in the future or not. It's strictly
> equivalent to calling clk_set_parent in the driver's probe.

Oh yeah, but here I'm mentioning en even more complex case where we
connect three panels with pixel clocks that cannot be all three derived
from the same parent. There has never been any upstream support for
that and I doubt we will have any anytime soon because we need a central
(drm) place to be aware of the clock limitations and make these
decisions.

[...]

>> +       /*
>> +        * Dual cases require a 3.5 rate division on the pixel clocks, which
>> +        * cannot be achieved with regular single divisors. Instead, double the
>> +        * parent PLL rate in the first place. In order to do that, we first
>> +        * require twice the target clock rate, which will program the upper
>> +        * PLL. Then, we ask for the actual targeted rate, which can be achieved
>> +        * by dividing by 2 the already configured upper PLL rate, without
>> +        * making further changes to it.
>> +        */
>> +       if (fsl_ldb_is_dual(fsl_ldb))
>> +               clk_set_rate(fsl_ldb->clk, requested_link_freq * 2);
>>         clk_set_rate(fsl_ldb->clk, requested_link_freq);
>
> This has nothing to do in a DRM driver. The clock driver logic needs
> to handle it.

The approach I am proposing in this series can sadly not work, because
it is not possible to slightly modify a clock after the crtc has been
set up without getting back into drm for further tuning. I observed
that my series was dependent on the probe order, because the exact same
clock tree would work and not work depending on the order.

To get back to your comment, unfortunately, there will be no other
choice than having drm drivers being aware of clock limitations, just
because the clock subsystem alone, even if it would do the right thing
behind the hood, would still sometimes break displays. This means drm
drivers will have to care about clock constraints.

As an example here, I am fine arguing about the way (calling
clk_set_rate twice on the same clock), but the fact that the parent
clock must be multiplied: this is drm business, because it is a drm
requirement.

Thanks,
Miquèl

Re: [PATCH 4/5] clk: Add flag to prevent frequency changes when walking subtrees

[Stephen Boyd]

Quoting Miquel Raynal (2024-12-23 10:38:20)
> Hi Stephen,
> 
> >> +/* do not passively change this clock rate during subtree rate propagation */
> >> +#define CLK_NO_RATE_CHANGE_DURING_PROPAGATION BIT(14)
> >
> > Why doesn't rate locking work?
> 
> Can you be more specific? What function from the API is supposed to do
> what I need? AFAIU, none of them is properly locking the rate during a
> subtree walk, but if I misread one of them, I'd be glad to drop all this.
> 

It's clk_set_rate_exclusive() as Maxime also asked about in the other
thread.

Re: [PATCH 4/5] clk: Add flag to prevent frequency changes when walking subtrees

[Stephen Boyd]

Quoting Miquel Raynal (2024-12-23 10:43:13)
> Hi Maxime,
> 
> On 17/12/2024 at 13:47:53 +01, Maxime Ripard <mripard@redhat.com> wrote:
> 
> > On Thu, Nov 21, 2024 at 06:41:14PM +0100, Miquel Raynal wrote:
> >> There are mainly two ways to change a clock frequency.
> >
> > There's much more than that :)
> 
> "mainly"
> 
> Or maybe I should have added "on purpose".
> 
> >
> > Off the top of my head, setting/clearing a min/max rate and changing the
> > parent might also result in a rate change.
> >
> > And then, the firmware might get involved too.
> >
> >> The active way requires calling ->set_rate() in order to ask "on
> >> purpose" for a frequency change. Otherwise, a clock can passively see
> >> its frequency being updated depending on upstream clock frequency
> >> changes. In most cases it is fine to just accept the new upstream
> >> frequency - which by definition will have an impact on downstream
> >> frequencies if we do not recalculate internal divisors. But there are
> >> cases where, upon an upstream frequency change, we would like to
> >> maintain a specific rate.
> >
> > Why is clk_set_rate_exclusive not enough?
> 
> I am trying to protect these rate changes from subtree walks, I don't
> see where setting an exclusive rate would have an effect? But I might be
> overlooking something, definitely.
> 
> ...
> 
> >> @@ -2272,7 +2271,13 @@ static void clk_calc_subtree(struct clk_core *core)
> >>  {
> >>      struct clk_core *child;
> >>  
> >> -    core->new_rate = clk_recalc(core, core->parent->new_rate);
> >> +    if (core->flags & CLK_NO_RATE_CHANGE_DURING_PROPAGATION) {
> >> +            core->new_rate = clk_determine(core, core->rate);
> >> +            if (!core->new_rate)
> >> +                    core->new_rate = clk_recalc(core, core->parent->new_rate);
> >> +    } else {
> >> +            core->new_rate = clk_recalc(core, core->parent->new_rate);
> >> +    }
> >
> > Sorry, it's not clear to me how it works. How will the parent clocks
> > will get notified to adjust their dividers in that scenario? Also, what
> > if they can't?
> 
> The idea is: if the flag is set, instead of accepting the new upstream
> rate and recalculate the downstream rate based on a previously set
> divider value, we change our divider value to match the same frequency
> as before. But if we cannot, then we just keep the old way.
> 

The exclusive rate code could support this if it doesn't already do so.
If you call clk_set_rate_exclusive(child, <constant rate>) followed by
clk_set_rate(parent, <new rate>) the core code should try to keep the
child at the constant rate, or fail the clk_set_rate() call on the
parent. It should be possible to confirm this with some KUnit tests for
clk_set_rate_exclusive(). Similarly, if another child, child_B, of the
parent changes the parent rate, we should speculate the new rate of the
child_A that's protected and fail if we can't maintain the rate. We need
to start generating a list of clks that we operate a rate change on to
support this though, because right now we rely on the stack to track the
clks that we change the rate of.

Initially we thought that we could do this with clk notifiers. That may
work here, but I suspect it will be clunky to get working because clk
notifiers operate on struct clk.

Re: [PATCH 4/5] clk: Add flag to prevent frequency changes when walking subtrees

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4125 bytes --]

On Mon, Dec 30, 2024 at 05:22:56PM -0800, Stephen Boyd wrote:
> Quoting Miquel Raynal (2024-12-23 10:43:13)
> > Hi Maxime,
> > 
> > On 17/12/2024 at 13:47:53 +01, Maxime Ripard <mripard@redhat.com> wrote:
> > 
> > > On Thu, Nov 21, 2024 at 06:41:14PM +0100, Miquel Raynal wrote:
> > >> There are mainly two ways to change a clock frequency.
> > >
> > > There's much more than that :)
> > 
> > "mainly"
> > 
> > Or maybe I should have added "on purpose".
> > 
> > >
> > > Off the top of my head, setting/clearing a min/max rate and changing the
> > > parent might also result in a rate change.
> > >
> > > And then, the firmware might get involved too.
> > >
> > >> The active way requires calling ->set_rate() in order to ask "on
> > >> purpose" for a frequency change. Otherwise, a clock can passively see
> > >> its frequency being updated depending on upstream clock frequency
> > >> changes. In most cases it is fine to just accept the new upstream
> > >> frequency - which by definition will have an impact on downstream
> > >> frequencies if we do not recalculate internal divisors. But there are
> > >> cases where, upon an upstream frequency change, we would like to
> > >> maintain a specific rate.
> > >
> > > Why is clk_set_rate_exclusive not enough?
> > 
> > I am trying to protect these rate changes from subtree walks, I don't
> > see where setting an exclusive rate would have an effect? But I might be
> > overlooking something, definitely.
> > 
> > ...
> > 
> > >> @@ -2272,7 +2271,13 @@ static void clk_calc_subtree(struct clk_core *core)
> > >>  {
> > >>      struct clk_core *child;
> > >>  
> > >> -    core->new_rate = clk_recalc(core, core->parent->new_rate);
> > >> +    if (core->flags & CLK_NO_RATE_CHANGE_DURING_PROPAGATION) {
> > >> +            core->new_rate = clk_determine(core, core->rate);
> > >> +            if (!core->new_rate)
> > >> +                    core->new_rate = clk_recalc(core, core->parent->new_rate);
> > >> +    } else {
> > >> +            core->new_rate = clk_recalc(core, core->parent->new_rate);
> > >> +    }
> > >
> > > Sorry, it's not clear to me how it works. How will the parent clocks
> > > will get notified to adjust their dividers in that scenario? Also, what
> > > if they can't?
> > 
> > The idea is: if the flag is set, instead of accepting the new upstream
> > rate and recalculate the downstream rate based on a previously set
> > divider value, we change our divider value to match the same frequency
> > as before. But if we cannot, then we just keep the old way.
> > 
> 
> The exclusive rate code could support this if it doesn't already do so.
> If you call clk_set_rate_exclusive(child, <constant rate>) followed by
> clk_set_rate(parent, <new rate>) the core code should try to keep the
> child at the constant rate, or fail the clk_set_rate() call on the
> parent. It should be possible to confirm this with some KUnit tests for
> clk_set_rate_exclusive(). Similarly, if another child, child_B, of the
> parent changes the parent rate, we should speculate the new rate of the
> child_A that's protected and fail if we can't maintain the rate. We need
> to start generating a list of clks that we operate a rate change on to
> support this though, because right now we rely on the stack to track the
> clks that we change the rate of.
> 
> Initially we thought that we could do this with clk notifiers. That may
> work here, but I suspect it will be clunky to get working because clk
> notifiers operate on struct clk.

I think notifiers are great for customers, but not really adequate for
the clock drivers tree. Indeed, you can only react to a (sub)tree
configuration using notifiers, but you can't affect it to try something
new that would be a better fit.

Like, if we have a PLL A, with two child clocks that are dividers. B is
initially (exclusively) set to freq X, and then you want to set C to 2X.

The best thing to do is to set A to 2X, and double B's divider. It's
simple enough, but we have no way to try to negociate that at the
moment.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 0/5] clk: Fix simple video pipelines on i.MX8

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 7614 bytes --]

On Mon, Dec 23, 2024 at 07:59:02PM +0100, Miquel Raynal wrote:
> Hello,
> 
> >> >> [After applying PATCH "clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate"]
> >> >> 
> >> >> This is a commit from Marek, which is, I believe going in the right
> >> >> direction, so I am including it. Just with this change, the situation is
> >> >> slightly different, but the result is the same:
> >> >> - media_disp[12]_pix is set to freq A by using a divisor of 1 and
> >> >>   setting video_pll1 to freq A.
> >> >> - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
> >> >>   to freq 7*A.
> >> >>   /!\ This as the side effect of changing media_disp[12]_pix from freq A
> >> >>   to freq 7*A.
> >> >
> >> > Although I'm not of a fan of setting CLK_SET_RATE_PARENT flag to the
> >> > LDB clock and pixel clocks,
> >> 
> >> I haven't commented much on this. For me, inaccurate pixel clocks mostly
> >> work fine (if not too inaccurate), but it is true that having very
> >> powerful PLL like the PLL1443, it is a pity not to use them at their
> >> highest capabilities. However, I consider "not breaking users" more
> >> important than having "perfect clock rates".
> >
> > Whether an inaccurate clock "works" depends on the context. A .5%
> > deviation will be out of spec for HDMI for example. An inaccurate VBLANK
> > frequency might also break some use cases.
> >
> > So that your display still works is not enough to prove it works.
> 
> Well, my display used to work. And now it no longer does.

I'm not disputing that.

> The perfect has become the enemy of the good.

What I'm disputing however is that your display might never have been
good or perfect in the first place.

> >> This series has one unique goal: accepting more accurate frequencies
> >> *and* not breaking users in the most simplest cases.
> >> 
> >> > would it work if the pixel clock rate is
> >> > set after the LDB clock rate is set in fsl_ldb_atomic_enable()?
> >> 
> >> The situation would be:
> >> - media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
> >>   to freq 7*A.
> >> - media_disp[12]_pix is set to freq A by using a divisor of 7.
> >> 
> >> So yes, and the explanation of why is there:
> >> https://elixir.bootlin.com/linux/v6.11.8/source/drivers/clk/clk-divider.c#L322
> >> 
> >> > The
> >> > pixel clock can be got from LDB's remote input LCDIF DT node by
> >> > calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch
> >> > does. Similar to setting pixel clock rate, I think a chance is that
> >> > pixel clock enablement can be moved from LCDIF driver to
> >> > fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change.
> >> 
> >> TBH, this sounds like a hack and is no longer required with this series.
> >> 
> >> You are just trying to circumvent the fact that until now, applying a
> >> rate in an upper clock would unconfigure the downstream rates, and I
> >> think this is our first real problem.
> >> 
> >> > https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/
> >> >
> >> > Actually, one sibling patch of the above patch reverts ff06ea04e4cf
> >> > because I thought "fixed PLL rate" is the only solution, though I'm
> >> > discussing any alternative solution of "dynamically changeable PLL
> >> > rate" with Marek in the thread of the sibling patch.
> >> 
> >> I don't think we want fixed PLL rates. Especially if you start using
> >> external (hot-pluggable) displays with different needs: it just does not
> >> fly. There is one situation that cannot yet be handled and needs
> >> manual reparenting: using 3 displays with a non-divisible pixel
> >> frequency.
> >
> > Funnily, external interfaces (ie, HDMI, DP) tend to be easier to work
> > with than panels. HDMI for example works with roughly three frequencies:
> > 148.5MHz, 297MHz and 594MHz. If you set the PLL to 594MHz and the
> > downstream clock has a basic divider, it works just fine.
> >
> >> FYI we managed this specific "advanced" case with assigned-clock-parents
> >> using an audio PLL as hinted by Marek. It mostly works, event though the
> >> PLL1416 are less precise and offer less accurate pixel clocks.
> >
> > Note that assigned-clock-parents doesn't provide any guarantee on
> > whether the parent will change in the future or not. It's strictly
> > equivalent to calling clk_set_parent in the driver's probe.
> 
> Oh yeah, but here I'm mentioning en even more complex case where we
> connect three panels with pixel clocks that cannot be all three derived
> from the same parent. There has never been any upstream support for
> that and I doubt we will have any anytime soon because we need a central
> (drm) place to be aware of the clock limitations and make these
> decisions.

I'm not sure why DRM would be involved here. It's internal clock
details, putting that in the consumer is an abstraction violation. Also,
this will happen with any clock that needs accurate frequency. If the
conflict happens between ALSA and MMC, how will you deal with it?

> >> +       /*
> >> +        * Dual cases require a 3.5 rate division on the pixel clocks, which
> >> +        * cannot be achieved with regular single divisors. Instead, double the
> >> +        * parent PLL rate in the first place. In order to do that, we first
> >> +        * require twice the target clock rate, which will program the upper
> >> +        * PLL. Then, we ask for the actual targeted rate, which can be achieved
> >> +        * by dividing by 2 the already configured upper PLL rate, without
> >> +        * making further changes to it.
> >> +        */
> >> +       if (fsl_ldb_is_dual(fsl_ldb))
> >> +               clk_set_rate(fsl_ldb->clk, requested_link_freq * 2);
> >>         clk_set_rate(fsl_ldb->clk, requested_link_freq);
> >
> > This has nothing to do in a DRM driver. The clock driver logic needs
> > to handle it.
> 
> The approach I am proposing in this series can sadly not work, because
> it is not possible to slightly modify a clock after the crtc has been
> set up without getting back into drm for further tuning.

I'm not sure what you mean or what you want here, sorry.

> I observed that my series was dependent on the probe order, because
> the exact same clock tree would work and not work depending on the
> order.
> 
> To get back to your comment, unfortunately, there will be no other
> choice than having drm drivers being aware of clock limitations, just
> because the clock subsystem alone, even if it would do the right thing
> behind the hood, would still sometimes break displays. This means drm
> drivers will have to care about clock constraints.

Clock constraints, sure. Like, if the clock cannot provide a given
frequency within a reasonable tolerance, DRM should indeed ignore that
mode. Trying to arbitrate between clocks and find a good configuration
for a given platform, absolutely not.

> As an example here, I am fine arguing about the way (calling
> clk_set_rate twice on the same clock), but the fact that the parent
> clock must be multiplied: this is drm business, because it is a drm
> requirement.

That's not what the abstraction is though. Any driver should call
clk_set_rate_exclusive on a clock once, and expect the clock to remain
the same going forward. That's what is documented, that's what the
driver will rely on. If or how the API implementation is able to perform
that task is none of the consumer's business.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 4/5] clk: Add flag to prevent frequency changes when walking subtrees

[Miquel Raynal]

Hi Stephen,

>> The idea is: if the flag is set, instead of accepting the new upstream
>> rate and recalculate the downstream rate based on a previously set
>> divider value, we change our divider value to match the same frequency
>> as before. But if we cannot, then we just keep the old way.
>> 
>
> The exclusive rate code could support this if it doesn't already do so.
> If you call clk_set_rate_exclusive(child, <constant rate>) followed by
> clk_set_rate(parent, <new rate>) the core code should try to keep the
> child at the constant rate, or fail the clk_set_rate() call on the
> parent. It should be possible to confirm this with some KUnit tests for
> clk_set_rate_exclusive(). Similarly, if another child, child_B, of the
> parent changes the parent rate, we should speculate the new rate of the
> child_A that's protected and fail if we can't maintain the rate. We need
> to start generating a list of clks that we operate a rate change on to
> support this though, because right now we rely on the stack to track the
> clks that we change the rate of.
>
> Initially we thought that we could do this with clk notifiers. That may
> work here, but I suspect it will be clunky to get working because clk
> notifiers operate on struct clk.

I see, thanks a lot for the feedback, I'll have a look.

Thanks,
Miquèl

Re: [PATCH 4/5] clk: Add flag to prevent frequency changes when walking subtrees

[Miquel Raynal]

Hi Maxime,

>> The exclusive rate code could support this if it doesn't already do so.
>> If you call clk_set_rate_exclusive(child, <constant rate>) followed by
>> clk_set_rate(parent, <new rate>) the core code should try to keep the
>> child at the constant rate, or fail the clk_set_rate() call on the
>> parent. It should be possible to confirm this with some KUnit tests for
>> clk_set_rate_exclusive(). Similarly, if another child, child_B, of the
>> parent changes the parent rate, we should speculate the new rate of the
>> child_A that's protected and fail if we can't maintain the rate. We need
>> to start generating a list of clks that we operate a rate change on to
>> support this though, because right now we rely on the stack to track the
>> clks that we change the rate of.
>> 
>> Initially we thought that we could do this with clk notifiers. That may
>> work here, but I suspect it will be clunky to get working because clk
>> notifiers operate on struct clk.
>
> I think notifiers are great for customers, but not really adequate for
> the clock drivers tree. Indeed, you can only react to a (sub)tree
> configuration using notifiers, but you can't affect it to try something
> new that would be a better fit.
>
> Like, if we have a PLL A, with two child clocks that are dividers. B is
> initially (exclusively) set to freq X, and then you want to set C to 2X.
>
> The best thing to do is to set A to 2X, and double B's divider. It's
> simple enough, but we have no way to try to negociate that at the
> moment.

Indeed. Do you have something in mind to address this situation (eg. a
new clk provider or core API)?

Thanks,
Miquèl