Hi!
Target of this series is to dynamically set the rate of video_pll1 to
the required LVDS clock rate(s), which are configured by the panel, and
the lvds-bridge respectively.
Some background:
The LVDS panel requires two clocks: the crtc clock and the lvds clock.
The lvds rate is always 7x the crtc rate. On the imx8mp, these are
assigned to media_disp2_pix and media_ldb, which are both
clk-composite-8m. The rates are set by drm_client_modeset_commit() (and
later by fsl_ldb_atomic_enable()), and the fsl-ldb driver, first crtc,
then lvds. The parent is typically assigned to video_pll1, which is a
clk-pll14xx (pll1443x).
The main problem:
As the clk-composite-8m currently doesn't support CLK_SET_RATE_PARENT,
the crtc rate is not propagated to video_pll1, and therefore must be
assigned in the device-tree manually.
The idea:
Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
When this is done, ensure that the pll1443x can be re-configured,
meaning it ensures that an already configured rate (crtc rate) is still
supported when a second child requires a different rate (lvds rate). As
the children have divider, the current approach is straight forward by
calculating the LCM of the required rates. During the rate change of the
PLL, it must ensure that all children still have the configured rate at
the end (and maybe also bypass the clock while doing so?). This is done
by implementing a notifier function for the clk-composite-8m. The tricky
part is now to find out if the rate change was intentional or not. This
is done by adding the "change trigger" to the notify data. In our case,
we now can infer if we aren't the change trigger, we need to keep the
existing rate after the PLL's rate change. We keep the existing rate by
modifying the new_rate of the clock's core, as we are quite late in an
already ongoing clock change process.
Future work:
The re-configuration of the PLL can definitely be improved for other use
cases where the children have more fancy inter-dependencies. That's one
of the main reasons I currently only touched the mentioned clocks.
Additionally, it might make sense to automatically re-parent if a
different possible parent suits better.
For the core part, I thought about extending my "unintentional change
check" so that the core ensures that the children keep the configured
rate, which might not be easy as the parent could be allowed to "round",
but it's not clear (at least to me yet) how much rounding is allowed. I
found a similar discussion posted here[1], therefore added Frank and
Maxime.
Thanks & regards,
Benjamin
[1] https://lore.kernel.org/lkml/[email protected]/
---
Benjamin Bara (13):
arm64: dts: imx8mp: lvds_bridge: use root instead of composite
arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF
clk: implement clk_hw_set_rate()
clk: print debug message if parent change is ignored
clk: keep track of the trigger of an ongoing clk_set_rate
clk: keep track if a clock is explicitly configured
clk: detect unintended rate changes
clk: divider: stop early if an optimal divider is found
clk: imx: pll14xx: consider active rate for re-config
clk: imx: composite-8m: convert compute_dividers to void
clk: imx: composite-8m: implement CLK_SET_RATE_PARENT
clk: imx: imx8mp: allow LVDS clocks to set parent rate
arm64: dts: imx8mp: remove assigned-clock-rate of IMX8MP_VIDEO_PLL1
arch/arm64/boot/dts/freescale/imx8mp.dtsi | 14 +--
drivers/clk/clk-divider.c | 9 ++
drivers/clk/clk.c | 146 +++++++++++++++++++++++++++++-
drivers/clk/imx/clk-composite-8m.c | 89 +++++++++++++++---
drivers/clk/imx/clk-imx8mp.c | 4 +-
drivers/clk/imx/clk-pll14xx.c | 20 ++++
drivers/clk/imx/clk.h | 4 +
include/linux/clk-provider.h | 2 +
include/linux/clk.h | 2 +
9 files changed, 261 insertions(+), 29 deletions(-)
---
base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56
change-id: 20230913-imx8mp-dtsi-7c6e25907e0e
Best regards,
--
Benjamin Bara <[email protected]>
On Sun, Sep 17, 2023 at 3:40 PM Benjamin Bara <[email protected]> wrote:
>
> Hi!
>
> Target of this series is to dynamically set the rate of video_pll1 to
> the required LVDS clock rate(s), which are configured by the panel, and
> the lvds-bridge respectively.
>
> Some background:
> The LVDS panel requires two clocks: the crtc clock and the lvds clock.
> The lvds rate is always 7x the crtc rate. On the imx8mp, these are
> assigned to media_disp2_pix and media_ldb, which are both
> clk-composite-8m. The rates are set by drm_client_modeset_commit() (and
> later by fsl_ldb_atomic_enable()), and the fsl-ldb driver, first crtc,
> then lvds. The parent is typically assigned to video_pll1, which is a
> clk-pll14xx (pll1443x).
>
> The main problem:
> As the clk-composite-8m currently doesn't support CLK_SET_RATE_PARENT,
> the crtc rate is not propagated to video_pll1, and therefore must be
> assigned in the device-tree manually.
>
> The idea:
> Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> When this is done, ensure that the pll1443x can be re-configured,
> meaning it ensures that an already configured rate (crtc rate) is still
> supported when a second child requires a different rate (lvds rate). As
Have you tested with the DSI as well? If memory servers, the DSI
clock and the LVDS clock are both clocked from the same video_pll. At
one time, I had done some experimentation with trying the DSI
connected to an HDMI bridge chip connected to a monitor and the LVDS
was connected to a display panel with a static resolution and refresh
rate. For my LVDS display, it needs 30MHz to display properly, but
various HDMI resolutions needed values that were not evenly divisible
by 30MHz which appeared to cause display sync issues when trying to
share a clock that was trying to dynamically adjust for two different
displays especially when trying to change the resoltuion of the HDMI
display to various values for different resolutions.
> the children have divider, the current approach is straight forward by
> calculating the LCM of the required rates. During the rate change of the
> PLL, it must ensure that all children still have the configured rate at
> the end (and maybe also bypass the clock while doing so?). This is done
> by implementing a notifier function for the clk-composite-8m. The tricky
> part is now to find out if the rate change was intentional or not. This
> is done by adding the "change trigger" to the notify data. In our case,
> we now can infer if we aren't the change trigger, we need to keep the
> existing rate after the PLL's rate change. We keep the existing rate by
> modifying the new_rate of the clock's core, as we are quite late in an
> already ongoing clock change process.
>
> Future work:
> The re-configuration of the PLL can definitely be improved for other use
> cases where the children have more fancy inter-dependencies. That's one
> of the main reasons I currently only touched the mentioned clocks.
> Additionally, it might make sense to automatically re-parent if a
> different possible parent suits better.
> For the core part, I thought about extending my "unintentional change
> check" so that the core ensures that the children keep the configured
> rate, which might not be easy as the parent could be allowed to "round",
> but it's not clear (at least to me yet) how much rounding is allowed. I
> found a similar discussion posted here[1], therefore added Frank and
> Maxime.
>
> Thanks & regards,
> Benjamin
>
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> ---
> Benjamin Bara (13):
> arm64: dts: imx8mp: lvds_bridge: use root instead of composite
> arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF
> clk: implement clk_hw_set_rate()
> clk: print debug message if parent change is ignored
> clk: keep track of the trigger of an ongoing clk_set_rate
> clk: keep track if a clock is explicitly configured
> clk: detect unintended rate changes
> clk: divider: stop early if an optimal divider is found
> clk: imx: pll14xx: consider active rate for re-config
> clk: imx: composite-8m: convert compute_dividers to void
> clk: imx: composite-8m: implement CLK_SET_RATE_PARENT
> clk: imx: imx8mp: allow LVDS clocks to set parent rate
> arm64: dts: imx8mp: remove assigned-clock-rate of IMX8MP_VIDEO_PLL1
>
> arch/arm64/boot/dts/freescale/imx8mp.dtsi | 14 +--
> drivers/clk/clk-divider.c | 9 ++
> drivers/clk/clk.c | 146 +++++++++++++++++++++++++++++-
> drivers/clk/imx/clk-composite-8m.c | 89 +++++++++++++++---
> drivers/clk/imx/clk-imx8mp.c | 4 +-
> drivers/clk/imx/clk-pll14xx.c | 20 ++++
> drivers/clk/imx/clk.h | 4 +
> include/linux/clk-provider.h | 2 +
> include/linux/clk.h | 2 +
> 9 files changed, 261 insertions(+), 29 deletions(-)
> ---
> base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56
> change-id: 20230913-imx8mp-dtsi-7c6e25907e0e
>
> Best regards,
> --
> Benjamin Bara <[email protected]>
>
From: Benjamin Bara <[email protected]>
Similar to commit 16c984524862 ("arm64: dts: imx8mp: don't initialize
audio clocks from CCM node").
With commit b09c68dc57c9 ("clk: imx: pll14xx: Support dynamic rates") in
place, the clock consumer (e.g. a panel) is able to set a more suitable
rate for the IMX8MP_VIDEO_PLL1. As composite-8m is now able to propagate
the rate through, avoid setting a rate in the dtsi.
Cc: Lucas Stach <[email protected]>
Cc: Sascha Hauer <[email protected]>
Signed-off-by: Benjamin Bara <[email protected]>
---
arch/arm64/boot/dts/freescale/imx8mp.dtsi | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 9539d747e28e..f40b40ee8f9e 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -1742,7 +1742,6 @@ media_blk_ctrl: blk-ctrl@32ec0000 {
<&clk IMX8MP_CLK_MEDIA_APB>,
<&clk IMX8MP_CLK_MEDIA_DISP1_PIX>,
<&clk IMX8MP_CLK_MEDIA_DISP2_PIX>,
- <&clk IMX8MP_VIDEO_PLL1>,
<&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF>;
assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
<&clk IMX8MP_SYS_PLL1_800M>,
@@ -1750,8 +1749,7 @@ media_blk_ctrl: blk-ctrl@32ec0000 {
<&clk IMX8MP_VIDEO_PLL1_OUT>,
<&clk IMX8MP_CLK_24M>;
assigned-clock-rates = <500000000>, <200000000>,
- <0>, <0>, <1039500000>,
- <24000000>;
+ <0>, <0>, <24000000>;
#power-domain-cells = <1>;
lvds_bridge: bridge@5c {
--
2.34.1
From: Benjamin Bara <[email protected]>
One of the key parts to enable dynamic clock propagation on the imx8m,
are the consumer-facing composites. They currently only divide,
therefore the parent must be already quite good in shape to provide a
close enough rate. Therefore, the parents are usually hard-assigned in
the dt. To workaround that, this commit enables propagation to the
parent of the composite.
If a rate cannot be reached exactly by only dividing, the parent is
asked (for now simply for the exact required rate - no dividers taken
into account). If the parent already has a configured rate, it's the
parent's job to ensure that all children are satisfied.
By using a notifier, the propagation-enabled clocks listen to clock
changes coming from the parent. If one is happening, it's the composites
job to verify if the rate is satisfying and if it is an intended change.
Otherwise, countermeasures have to be taken into account (e.g. setting
the rate back or aborting the change).
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/clk/imx/clk-composite-8m.c | 71 ++++++++++++++++++++++++++++++++++++--
1 file changed, 68 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
index a121f1285110..068f61df28b1 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -4,6 +4,7 @@
*/
#include <linux/clk-provider.h>
+#include <linux/clk.h>
#include <linux/errno.h>
#include <linux/export.h>
#include <linux/io.h>
@@ -119,8 +120,12 @@ static int imx8m_divider_determine_rate(struct clk_hw *hw,
struct clk_rate_request *req)
{
struct clk_divider *divider = to_clk_divider(hw);
+ struct clk_hw *parent = clk_hw_get_parent(hw);
int prediv_value;
int div_value;
+ unsigned long target_rate;
+ struct clk_rate_request req_parent;
+ int ret;
/* if read only, just return current value */
if (divider->flags & CLK_DIVIDER_READ_ONLY) {
@@ -140,9 +145,29 @@ static int imx8m_divider_determine_rate(struct clk_hw *hw,
divider->flags, prediv_value * div_value);
}
- return divider_determine_rate(hw, req, divider->table,
- PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
- divider->flags);
+ target_rate = req->rate;
+ ret = divider_determine_rate(hw, req, divider->table,
+ PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
+ divider->flags);
+ if (ret || req->rate == target_rate)
+ return ret;
+
+ /*
+ * If re-configuring the parent gives a better rate, do this instead.
+ * Avoid re-parenting for now, which could be done first if a possible
+ * parent already has a satisfying rate. The implementation does not
+ * consider the dividers between the parent and the current clock.
+ */
+ clk_hw_forward_rate_request(hw, req, parent, &req_parent, target_rate);
+ if (__clk_determine_rate(parent, &req_parent))
+ return 0;
+
+ if (abs(req_parent.rate - target_rate) < abs(req->rate - target_rate)) {
+ req->rate = req_parent.rate;
+ req->best_parent_rate = req_parent.rate;
+ }
+
+ return 0;
}
static const struct clk_ops imx8m_clk_composite_divider_ops = {
@@ -198,6 +223,33 @@ static const struct clk_ops imx8m_clk_composite_mux_ops = {
.determine_rate = imx8m_clk_composite_mux_determine_rate,
};
+static int imx8m_clk_composite_notifier_fn(struct notifier_block *notifier,
+ unsigned long code, void *data)
+{
+ struct clk_notifier_data *cnd = data;
+ struct clk_hw *hw = __clk_get_hw(cnd->clk);
+
+ if (code != PRE_RATE_CHANGE)
+ return NOTIFY_OK;
+
+ if (!__clk_is_rate_set(cnd->clk))
+ return NOTIFY_OK;
+
+ /*
+ * Consumer of a composite-m8 clock usually use the root clk, a gate
+ * connected to the composite (e.g. media_ldb and media_ldb_root).
+ * Therefore, evaluate the trigger's parent too.
+ */
+ if (cnd->clk != cnd->trigger && cnd->clk != clk_get_parent(cnd->trigger))
+ return notifier_from_errno(clk_hw_set_rate(hw, cnd->old_rate));
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block imx8m_clk_composite_notifier = {
+ .notifier_call = imx8m_clk_composite_notifier_fn,
+};
+
struct clk_hw *__imx8m_clk_hw_composite(const char *name,
const char * const *parent_names,
int num_parents, void __iomem *reg,
@@ -211,6 +263,7 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
struct clk_mux *mux = NULL;
const struct clk_ops *divider_ops;
const struct clk_ops *mux_ops;
+ int ret;
mux = kzalloc(sizeof(*mux), GFP_KERNEL);
if (!mux)
@@ -268,6 +321,18 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
if (IS_ERR(hw))
goto fail;
+ /*
+ * register a notifier which should switch back to the configured rate
+ * if the rate is going to be changed unintentionally.
+ */
+ if (flags & CLK_SET_RATE_PARENT) {
+ ret = clk_notifier_register(hw->clk, &imx8m_clk_composite_notifier);
+ if (ret) {
+ hw = ERR_PTR(ret);
+ goto fail;
+ }
+ }
+
return hw;
fail:
--
2.34.1
Hi Adam!
On Mon, 18 Sept 2023 at 07:00, Adam Ford <[email protected]> wrote:
> On Sun, Sep 17, 2023 at 3:40 PM Benjamin Bara <[email protected]> wrote:
> > The idea:
> > Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> > When this is done, ensure that the pll1443x can be re-configured,
> > meaning it ensures that an already configured rate (crtc rate) is still
> > supported when a second child requires a different rate (lvds rate). As
>
> Have you tested with the DSI as well? If memory servers, the DSI
> clock and the LVDS clock are both clocked from the same video_pll. At
> one time, I had done some experimentation with trying the DSI
> connected to an HDMI bridge chip connected to a monitor and the LVDS
> was connected to a display panel with a static resolution and refresh
> rate. For my LVDS display, it needs 30MHz to display properly, but
> various HDMI resolutions needed values that were not evenly divisible
> by 30MHz which appeared to cause display sync issues when trying to
> share a clock that was trying to dynamically adjust for two different
> displays especially when trying to change the resoltuion of the HDMI
> display to various values for different resolutions.
Unfortunately I haven't. I think if you have the use case to support
different "run-time-dynamic" (HDMI) rates in parallel with a static
(LVDS) rate, it probably makes sense (for now) to just use a LVDS panel
which can be feeded from one of the static PLLs directly and do a manual
re-parenting in the dt. The manual re-parenting could be replaced by an
automated re-parenting in the composite driver. When I think about it,
it might make sense to extend clk-divider's clk_divider_bestdiv()[1]
(which is currently used by the composite-8m) with a "find the best
parent" implementation, something like:
1. are we in range if we divide the active parent with all possible
dividers? (already existing)
2. are we in range if we switch to a different parent and divide it with
all possible dividers?
3. are we in range if we re-configure a possible parent (and switch to
it)?
Steps 2 & 3 are e.g. implemented by at91's clk-master[2]. There are
maybe also "smarter" solutions to the problem beside trying every
possibility. Anyways, we already have a CLK_SET_RATE_NO_REPARENT which
would indicate if we are allowed to do so.
For static use cases involving both, I would probably (for now) go with
a hard-assigned, tested clock rate in the dt. IMHO, this should always
work as fall-back.
Regards,
Benjamin
[1] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk-divider.c#L304
[2] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/at91/clk-master.c#L586
Hi Benjamin!
On 2023-09-18 at 00:39:56 +0200, Benjamin Bara <[email protected]> wrote:
> Hi!
>
> Target of this series is to dynamically set the rate of video_pll1 to
> the required LVDS clock rate(s), which are configured by the panel, and
> the lvds-bridge respectively.
>
> Some background:
> The LVDS panel requires two clocks: the crtc clock and the lvds clock.
> The lvds rate is always 7x the crtc rate. On the imx8mp, these are
> assigned to media_disp2_pix and media_ldb, which are both
> clk-composite-8m. The rates are set by drm_client_modeset_commit() (and
> later by fsl_ldb_atomic_enable()), and the fsl-ldb driver, first crtc,
> then lvds. The parent is typically assigned to video_pll1, which is a
> clk-pll14xx (pll1443x).
>
> The main problem:
> As the clk-composite-8m currently doesn't support CLK_SET_RATE_PARENT,
> the crtc rate is not propagated to video_pll1, and therefore must be
> assigned in the device-tree manually.
>
> The idea:
> Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> When this is done, ensure that the pll1443x can be re-configured,
> meaning it ensures that an already configured rate (crtc rate) is still
> supported when a second child requires a different rate (lvds rate). As
> the children have divider, the current approach is straight forward by
> calculating the LCM of the required rates. During the rate change of the
> PLL, it must ensure that all children still have the configured rate at
> the end (and maybe also bypass the clock while doing so?). This is done
> by implementing a notifier function for the clk-composite-8m. The tricky
> part is now to find out if the rate change was intentional or not. This
> is done by adding the "change trigger" to the notify data. In our case,
> we now can infer if we aren't the change trigger, we need to keep the
> existing rate after the PLL's rate change. We keep the existing rate by
> modifying the new_rate of the clock's core, as we are quite late in an
> already ongoing clock change process.
>
> Future work:
> The re-configuration of the PLL can definitely be improved for other use
> cases where the children have more fancy inter-dependencies. That's one
> of the main reasons I currently only touched the mentioned clocks.
> Additionally, it might make sense to automatically re-parent if a
> different possible parent suits better.
> For the core part, I thought about extending my "unintentional change
> check" so that the core ensures that the children keep the configured
> rate, which might not be easy as the parent could be allowed to "round",
> but it's not clear (at least to me yet) how much rounding is allowed. I
> found a similar discussion posted here[1], therefore added Frank and
> Maxime.
Thank you very much for including me in the discussion. If I understood
Maxime correctly, your proposal is close to what he was suggesting in
the discussion you referenced. Unfortunately, it doesn't cover the
rounding aspect (which you also mentioned in your cover letter and the
description for clk_detect_unintended_rate_changes in patch 7. I've been
pondering the last three weeks how to find a good solution to this
problem, but so far haven't found any.
Hopefully, your suggestion spurs some new thoughts.
Thanks,
Frank
>
> Thanks & regards,
> Benjamin
>
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> ---
> Benjamin Bara (13):
> arm64: dts: imx8mp: lvds_bridge: use root instead of composite
> arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF
> clk: implement clk_hw_set_rate()
> clk: print debug message if parent change is ignored
> clk: keep track of the trigger of an ongoing clk_set_rate
> clk: keep track if a clock is explicitly configured
> clk: detect unintended rate changes
> clk: divider: stop early if an optimal divider is found
> clk: imx: pll14xx: consider active rate for re-config
> clk: imx: composite-8m: convert compute_dividers to void
> clk: imx: composite-8m: implement CLK_SET_RATE_PARENT
> clk: imx: imx8mp: allow LVDS clocks to set parent rate
> arm64: dts: imx8mp: remove assigned-clock-rate of IMX8MP_VIDEO_PLL1
>
> arch/arm64/boot/dts/freescale/imx8mp.dtsi | 14 +--
> drivers/clk/clk-divider.c | 9 ++
> drivers/clk/clk.c | 146 +++++++++++++++++++++++++++++-
> drivers/clk/imx/clk-composite-8m.c | 89 +++++++++++++++---
> drivers/clk/imx/clk-imx8mp.c | 4 +-
> drivers/clk/imx/clk-pll14xx.c | 20 ++++
> drivers/clk/imx/clk.h | 4 +
> include/linux/clk-provider.h | 2 +
> include/linux/clk.h | 2 +
> 9 files changed, 261 insertions(+), 29 deletions(-)
> ---
> base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56
> change-id: 20230913-imx8mp-dtsi-7c6e25907e0e
>
> Best regards,
Hi Frank!
On Mon, 18 Sept 2023 at 19:24, Frank Oltmanns <[email protected]> wrote:
> On 2023-09-18 at 00:39:56 +0200, Benjamin Bara <[email protected]> wrote:
> Thank you very much for including me in the discussion. If I understood
> Maxime correctly, your proposal is close to what he was suggesting in
> the discussion you referenced. Unfortunately, it doesn't cover the
> rounding aspect (which you also mentioned in your cover letter and the
> description for clk_detect_unintended_rate_changes in patch 7. I've been
> pondering the last three weeks how to find a good solution to this
> problem, but so far haven't found any.
I think if we stick to the idea of always enforcing the exact "typical
rate", we cannot avoid physically impossible cases. IMHO, it might make
sense to add a set_rate() function with a "timing_entry" (e.g. used by
display_timing.h[1]) to the clock API, which gives a suggestion but also
defines the "real" boundaries. This would provide a shared parent PLL
more freedom to provide a satisfying rate for all its children.
Regards
Benjamin
[1] https://elixir.bootlin.com/linux/v6.5.3/source/include/video/display_timing.h#L64
Hi,
On Mon, Sep 18, 2023 at 12:39:56AM +0200, Benjamin Bara wrote:
> Target of this series is to dynamically set the rate of video_pll1 to
> the required LVDS clock rate(s), which are configured by the panel, and
> the lvds-bridge respectively.
>
> Some background:
> The LVDS panel requires two clocks: the crtc clock and the lvds clock.
> The lvds rate is always 7x the crtc rate. On the imx8mp, these are
> assigned to media_disp2_pix and media_ldb, which are both
> clk-composite-8m. The rates are set by drm_client_modeset_commit() (and
> later by fsl_ldb_atomic_enable()), and the fsl-ldb driver, first crtc,
> then lvds. The parent is typically assigned to video_pll1, which is a
> clk-pll14xx (pll1443x).
>
> The main problem:
> As the clk-composite-8m currently doesn't support CLK_SET_RATE_PARENT,
> the crtc rate is not propagated to video_pll1, and therefore must be
> assigned in the device-tree manually.
>
> The idea:
> Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> When this is done, ensure that the pll1443x can be re-configured,
> meaning it ensures that an already configured rate (crtc rate) is still
> supported when a second child requires a different rate (lvds rate). As
> the children have divider, the current approach is straight forward by
> calculating the LCM of the required rates. During the rate change of the
> PLL, it must ensure that all children still have the configured rate at
> the end (and maybe also bypass the clock while doing so?). This is done
> by implementing a notifier function for the clk-composite-8m. The tricky
> part is now to find out if the rate change was intentional or not. This
> is done by adding the "change trigger" to the notify data. In our case,
> we now can infer if we aren't the change trigger, we need to keep the
> existing rate after the PLL's rate change. We keep the existing rate by
> modifying the new_rate of the clock's core, as we are quite late in an
> already ongoing clock change process.
So just like the discussion we had on the Allwinner stuff, I don't think
you can cover it completely within the framework. If we take a step
backward, I guess what you want is that you have multiple clocks,
feeding multiple displays at varying clock rates depending on the
resolution, and the parent needs to accomodate all of them, right?
Could you share the clock tree and the capability of each clocks (range
of the multipliers / dividers mostly)?
I'm wondering if we couldn't set the parent clock to a fairly high rate
that would be high enough for each child to reach whatever rate it needs
to have without the need for CLK_SET_RATE_PARENT.
Maxime
On Mon, Sep 18, 2023 at 07:59:16PM +0200, Benjamin Bara wrote:
> Hi Adam!
>
> On Mon, 18 Sept 2023 at 07:00, Adam Ford <[email protected]> wrote:
> > On Sun, Sep 17, 2023 at 3:40 PM Benjamin Bara <[email protected]> wrote:
> > > The idea:
> > > Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> > > When this is done, ensure that the pll1443x can be re-configured,
> > > meaning it ensures that an already configured rate (crtc rate) is still
> > > supported when a second child requires a different rate (lvds rate). As
> >
> > Have you tested with the DSI as well? If memory servers, the DSI
> > clock and the LVDS clock are both clocked from the same video_pll. At
> > one time, I had done some experimentation with trying the DSI
> > connected to an HDMI bridge chip connected to a monitor and the LVDS
> > was connected to a display panel with a static resolution and refresh
> > rate. For my LVDS display, it needs 30MHz to display properly, but
> > various HDMI resolutions needed values that were not evenly divisible
> > by 30MHz which appeared to cause display sync issues when trying to
> > share a clock that was trying to dynamically adjust for two different
> > displays especially when trying to change the resoltuion of the HDMI
> > display to various values for different resolutions.
>
> Unfortunately I haven't. I think if you have the use case to support
> different "run-time-dynamic" (HDMI) rates in parallel with a static
> (LVDS) rate
If anything, LVDS is harder to deal with than HDMI. HDMI only has a
handful of clock rates (74.250, 148.5, 297 and 594MHz mostly) while LVDS
is more freeform.
We are more likely to change the rate on an HDMI device though, but a
rate change from 1080p to 720p would only require a divide by two (from
148.5 to 74.250) so fairly easy to do.
Maxime
On Mon, Sep 18, 2023 at 08:05:48PM +0200, Benjamin Bara wrote:
> Hi Frank!
>
> On Mon, 18 Sept 2023 at 19:24, Frank Oltmanns <[email protected]> wrote:
> > On 2023-09-18 at 00:39:56 +0200, Benjamin Bara <[email protected]> wrote:
> > Thank you very much for including me in the discussion. If I understood
> > Maxime correctly, your proposal is close to what he was suggesting in
> > the discussion you referenced. Unfortunately, it doesn't cover the
> > rounding aspect (which you also mentioned in your cover letter and the
> > description for clk_detect_unintended_rate_changes in patch 7. I've been
> > pondering the last three weeks how to find a good solution to this
> > problem, but so far haven't found any.
>
> I think if we stick to the idea of always enforcing the exact "typical
> rate", we cannot avoid physically impossible cases. IMHO, it might make
> sense to add a set_rate() function with a "timing_entry" (e.g. used by
> display_timing.h[1]) to the clock API, which gives a suggestion but also
> defines the "real" boundaries. This would provide a shared parent PLL
> more freedom to provide a satisfying rate for all its children.
It's definitely something we should do, and I've wanted to do that for a
while.
The clock rate is not the only thing we can change though. The usual
trick is to modify the blanking areas to come up with a rate that
matches what the hardware can provide without modifying the framerate.
It belongs more in a KMS helper
Maxime
On Sun, Sep 17, 2023 at 5:40 PM Benjamin Bara <[email protected]> wrote:
>
> Hi!
>
> Target of this series is to dynamically set the rate of video_pll1 to
> the required LVDS clock rate(s), which are configured by the panel, and
> the lvds-bridge respectively.
>
> Some background:
> The LVDS panel requires two clocks: the crtc clock and the lvds clock.
> The lvds rate is always 7x the crtc rate. On the imx8mp, these are
> assigned to media_disp2_pix and media_ldb, which are both
Could the LDB driver be updated to take in the crtc clock as a
parameter, then set the media_ldb to 7x crct clock. I wonder if that
might simplify the task a bit.
> clk-composite-8m. The rates are set by drm_client_modeset_commit() (and
> later by fsl_ldb_atomic_enable()), and the fsl-ldb driver, first crtc,
> then lvds. The parent is typically assigned to video_pll1, which is a
> clk-pll14xx (pll1443x).
>
> The main problem:
> As the clk-composite-8m currently doesn't support CLK_SET_RATE_PARENT,
> the crtc rate is not propagated to video_pll1, and therefore must be
> assigned in the device-tree manually.
>
> The idea:
> Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> When this is done, ensure that the pll1443x can be re-configured,
> meaning it ensures that an already configured rate (crtc rate) is still
> supported when a second child requires a different rate (lvds rate). As
I still have concerns that the CLK_SET_RATE_PARENT may break the
media_disp1_pix if media_disp2_pix is changing it.
I think we should consider adding some sort of configurable flag to
the CCM that lets people choose if CLK_SET_RATE_PARENT should be set
or not in the device tree instead of hard-coding it either on or off.
This would give people the flexibility of stating whether
media_disp1_pix, media_disp2_pix, both or neither could set
CLK_SET_RATE_PARENT.
I believe the imx8mp-evk can support both LVDS-> HDMI and DSI->HDMI
bridges, and I fear that if they are trying to both set different
clock rates, this may break something and the clocks need to be
selected in advance to give people a bunch of HDMI options as well as
being able to divide down to support the LVDS.
I think some of the displays could be tied to one of the Audio PLL's,
so I might experiment with splitting the media_disp1_pix and
media_disp2_pix from each other to see how well .
> the children have divider, the current approach is straight forward by
> calculating the LCM of the required rates. During the rate change of the
> PLL, it must ensure that all children still have the configured rate at
> the end (and maybe also bypass the clock while doing so?). This is done
> by implementing a notifier function for the clk-composite-8m. The tricky
> part is now to find out if the rate change was intentional or not. This
> is done by adding the "change trigger" to the notify data. In our case,
> we now can infer if we aren't the change trigger, we need to keep the
> existing rate after the PLL's rate change. We keep the existing rate by
> modifying the new_rate of the clock's core, as we are quite late in an
> already ongoing clock change process.
>
> Future work:
> The re-configuration of the PLL can definitely be improved for other use
> cases where the children have more fancy inter-dependencies. That's one
> of the main reasons I currently only touched the mentioned clocks.
> Additionally, it might make sense to automatically re-parent if a
> different possible parent suits better.
> For the core part, I thought about extending my "unintentional change
> check" so that the core ensures that the children keep the configured
> rate, which might not be easy as the parent could be allowed to "round",
> but it's not clear (at least to me yet) how much rounding is allowed. I
> found a similar discussion posted here[1], therefore added Frank and
> Maxime.
>
> Thanks & regards,
> Benjamin
>
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> ---
> Benjamin Bara (13):
> arm64: dts: imx8mp: lvds_bridge: use root instead of composite
> arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF
> clk: implement clk_hw_set_rate()
> clk: print debug message if parent change is ignored
> clk: keep track of the trigger of an ongoing clk_set_rate
> clk: keep track if a clock is explicitly configured
> clk: detect unintended rate changes
> clk: divider: stop early if an optimal divider is found
> clk: imx: pll14xx: consider active rate for re-config
> clk: imx: composite-8m: convert compute_dividers to void
> clk: imx: composite-8m: implement CLK_SET_RATE_PARENT
> clk: imx: imx8mp: allow LVDS clocks to set parent rate
> arm64: dts: imx8mp: remove assigned-clock-rate of IMX8MP_VIDEO_PLL1
>
> arch/arm64/boot/dts/freescale/imx8mp.dtsi | 14 +--
> drivers/clk/clk-divider.c | 9 ++
> drivers/clk/clk.c | 146 +++++++++++++++++++++++++++++-
> drivers/clk/imx/clk-composite-8m.c | 89 +++++++++++++++---
> drivers/clk/imx/clk-imx8mp.c | 4 +-
> drivers/clk/imx/clk-pll14xx.c | 20 ++++
> drivers/clk/imx/clk.h | 4 +
> include/linux/clk-provider.h | 2 +
> include/linux/clk.h | 2 +
> 9 files changed, 261 insertions(+), 29 deletions(-)
> ---
> base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56
> change-id: 20230913-imx8mp-dtsi-7c6e25907e0e
>
> Best regards,
> --
> Benjamin Bara <[email protected]>
>
Hi Adam,
Am Dienstag, 3. Oktober 2023, 15:28:05 CEST schrieb Adam Ford:
> On Sun, Sep 17, 2023 at 5:40 PM Benjamin Bara <[email protected]> wrote:
> > Hi!
> >
> > Target of this series is to dynamically set the rate of video_pll1 to
> > the required LVDS clock rate(s), which are configured by the panel, and
> > the lvds-bridge respectively.
> >
> > Some background:
> > The LVDS panel requires two clocks: the crtc clock and the lvds clock.
> > The lvds rate is always 7x the crtc rate. On the imx8mp, these are
> > assigned to media_disp2_pix and media_ldb, which are both
>
> Could the LDB driver be updated to take in the crtc clock as a
> parameter, then set the media_ldb to 7x crct clock. I wonder if that
> might simplify the task a bit.
I'm not sure if you had something different in mind, but I guess this happens
already in fsl_ldb_atomic_enable(), although using the mode clock.
As this might not always be possible, commit bd43a9844bc6f ("drm: bridge: ldb:
Warn if LDB clock does not match requested link frequency") was added to
indicate something might be wrong.
The main problem here is that both media_ldb and crct clock are not in a
parent<->child relationship, but are siblings, configurable individually.
Best regards,
Alexander
> > clk-composite-8m. The rates are set by drm_client_modeset_commit() (and
> > later by fsl_ldb_atomic_enable()), and the fsl-ldb driver, first crtc,
> > then lvds. The parent is typically assigned to video_pll1, which is a
> > clk-pll14xx (pll1443x).
> >
> > The main problem:
> > As the clk-composite-8m currently doesn't support CLK_SET_RATE_PARENT,
> > the crtc rate is not propagated to video_pll1, and therefore must be
> > assigned in the device-tree manually.
> >
> > The idea:
> > Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> > When this is done, ensure that the pll1443x can be re-configured,
> > meaning it ensures that an already configured rate (crtc rate) is still
> > supported when a second child requires a different rate (lvds rate). As
>
> I still have concerns that the CLK_SET_RATE_PARENT may break the
> media_disp1_pix if media_disp2_pix is changing it.
>
> I think we should consider adding some sort of configurable flag to
> the CCM that lets people choose if CLK_SET_RATE_PARENT should be set
> or not in the device tree instead of hard-coding it either on or off.
> This would give people the flexibility of stating whether
> media_disp1_pix, media_disp2_pix, both or neither could set
> CLK_SET_RATE_PARENT.
>
> I believe the imx8mp-evk can support both LVDS-> HDMI and DSI->HDMI
> bridges, and I fear that if they are trying to both set different
> clock rates, this may break something and the clocks need to be
> selected in advance to give people a bunch of HDMI options as well as
> being able to divide down to support the LVDS.
>
> I think some of the displays could be tied to one of the Audio PLL's,
> so I might experiment with splitting the media_disp1_pix and
> media_disp2_pix from each other to see how well .
>
> > the children have divider, the current approach is straight forward by
> > calculating the LCM of the required rates. During the rate change of the
> > PLL, it must ensure that all children still have the configured rate at
> > the end (and maybe also bypass the clock while doing so?). This is done
> > by implementing a notifier function for the clk-composite-8m. The tricky
> > part is now to find out if the rate change was intentional or not. This
> > is done by adding the "change trigger" to the notify data. In our case,
> > we now can infer if we aren't the change trigger, we need to keep the
> > existing rate after the PLL's rate change. We keep the existing rate by
> > modifying the new_rate of the clock's core, as we are quite late in an
> > already ongoing clock change process.
> >
> > Future work:
> > The re-configuration of the PLL can definitely be improved for other use
> > cases where the children have more fancy inter-dependencies. That's one
> > of the main reasons I currently only touched the mentioned clocks.
> > Additionally, it might make sense to automatically re-parent if a
> > different possible parent suits better.
> > For the core part, I thought about extending my "unintentional change
> > check" so that the core ensures that the children keep the configured
> > rate, which might not be easy as the parent could be allowed to "round",
> > but it's not clear (at least to me yet) how much rounding is allowed. I
> > found a similar discussion posted here[1], therefore added Frank and
> > Maxime.
> >
> > Thanks & regards,
> > Benjamin
> >
> > [1]
> > https://lore.kernel.org/lkml/20230825-pll-mipi_keep_rate-v1-0-35bc4357073
> > [email protected]/
> >
> > ---
> >
> > Benjamin Bara (13):
> > arm64: dts: imx8mp: lvds_bridge: use root instead of composite
> > arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF
> > clk: implement clk_hw_set_rate()
> > clk: print debug message if parent change is ignored
> > clk: keep track of the trigger of an ongoing clk_set_rate
> > clk: keep track if a clock is explicitly configured
> > clk: detect unintended rate changes
> > clk: divider: stop early if an optimal divider is found
> > clk: imx: pll14xx: consider active rate for re-config
> > clk: imx: composite-8m: convert compute_dividers to void
> > clk: imx: composite-8m: implement CLK_SET_RATE_PARENT
> > clk: imx: imx8mp: allow LVDS clocks to set parent rate
> > arm64: dts: imx8mp: remove assigned-clock-rate of IMX8MP_VIDEO_PLL1
> >
> > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 14 +--
> > drivers/clk/clk-divider.c | 9 ++
> > drivers/clk/clk.c | 146
> > +++++++++++++++++++++++++++++- drivers/clk/imx/clk-composite-8m.c
> > | 89 +++++++++++++++--- drivers/clk/imx/clk-imx8mp.c | 4
> > +-
> > drivers/clk/imx/clk-pll14xx.c | 20 ++++
> > drivers/clk/imx/clk.h | 4 +
> > include/linux/clk-provider.h | 2 +
> > include/linux/clk.h | 2 +
> > 9 files changed, 261 insertions(+), 29 deletions(-)
> >
> > ---
> > base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56
> > change-id: 20230913-imx8mp-dtsi-7c6e25907e0e
> >
> > Best regards,
> > --
> > Benjamin Bara <[email protected]>
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/
Hi Adam, Alexander!
On Wed, 4 Oct 2023 at 10:04, Alexander Stein <[email protected]> wrote:
> Am Dienstag, 3. Oktober 2023, 15:28:05 CEST schrieb Adam Ford:
> > On Sun, Sep 17, 2023 at 5:40 PM Benjamin Bara <[email protected]> wrote:
> > > Hi!
> > >
> > > Target of this series is to dynamically set the rate of video_pll1 to
> > > the required LVDS clock rate(s), which are configured by the panel, and
> > > the lvds-bridge respectively.
> > >
> > > Some background:
> > > The LVDS panel requires two clocks: the crtc clock and the lvds clock.
> > > The lvds rate is always 7x the crtc rate. On the imx8mp, these are
> > > assigned to media_disp2_pix and media_ldb, which are both
> >
> > Could the LDB driver be updated to take in the crtc clock as a
> > parameter, then set the media_ldb to 7x crct clock. I wonder if that
> > might simplify the task a bit.
>
> I'm not sure if you had something different in mind, but I guess this happens
> already in fsl_ldb_atomic_enable(), although using the mode clock.
> As this might not always be possible, commit bd43a9844bc6f ("drm: bridge: ldb:
> Warn if LDB clock does not match requested link frequency") was added to
> indicate something might be wrong.
> The main problem here is that both media_ldb and crct clock are not in a
> parent<->child relationship, but are siblings, configurable individually.
Yes, this already happens. First, the mode is set (which sets the CRTC
rate). Next, LDB sets the LVDS rate. Both do not have "access" to the
PLL, because the clocks haven't configured CLK_SET_RATE_PARENT. What
might be a working (but IMHO dirty) hack, is to give the LDB the PLL
clock as input too. Then it could set the PLL, LDB, CRTC rate (CRTC rate
must be set again after PLL is set!).
> > I still have concerns that the CLK_SET_RATE_PARENT may break the
> > media_disp1_pix if media_disp2_pix is changing it.
> > I think we should consider adding some sort of configurable flag to
> > the CCM that lets people choose if CLK_SET_RATE_PARENT should be set
> > or not in the device tree instead of hard-coding it either on or off.
> > This would give people the flexibility of stating whether
> > media_disp1_pix, media_disp2_pix, both or neither could set
> > CLK_SET_RATE_PARENT.
Probably we could do that (for now) by adding a second (optional) clock
to LDB. If it is set, the LDB driver should also set the LVDS rate on
this clock. This would then be set to the parent PLL.
> > I believe the imx8mp-evk can support both LVDS-> HDMI and DSI->HDMI
> > bridges, and I fear that if they are trying to both set different
> > clock rates, this may break something and the clocks need to be
> > selected in advance to give people a bunch of HDMI options as well as
> > being able to divide down to support the LVDS.
> >
> > I think some of the displays could be tied to one of the Audio PLL's,
> > so I might experiment with splitting the media_disp1_pix and
> > media_disp2_pix from each other to see how well .
Yes, you probably could also tie them to one of the other available
PLLs. We "could" also do that automatically, by not setting
CLK_SET_RATE_REPARENT and adapting the clk-divider driver to look for a
better suitable parent. However, I guess the outcome is currently quite
unpredictable, so this would require a lot of additional work. Just to
mention it here too: I created a small spin-off of this series[1] with
the changes of this series which affect the core.
Probably using the optional clock for LDB is a suitable short-term
solution?
Regards
Benjamin