[RFC] clk: imx6: Mark imx_clk_mux as glitchy by default
With some exceptions clk muxes on imx6 are "glitchy": If the output
is not gated before reparenting then high-frequency pulses can occur and
cause unpredictable behavior. Only a very small subset of muxes are
listed as "glitchless" in the reference manual.
Adapt to this by adding the CLK_SET_PARENT_GATE flag by default to
imx_clk_mux. Add imx_clk_mux_glitchless for the muxes listed as
glitchless and which don't already have have custom behavior.
The CLK_SET_PARENT_GATE flags enables a small check inside the clk core
which make clk_set_parent return -EBUSY if the clock is active. Ensuring
outputs are gated prior to calling set_parent is up to the clk
consumers.
The effect of this patch is that drivers which perform unsafe operations
which mostly work will receive errors instead, possibly breaking
functionality.
Signed-off-by: Leonard Crestez
---
Unfortunately one of the misbehaving drivers is ldb for LVDS display and
it seems to be a real issue: ldb enable/disable does reparenting on an
active clk used by IPU.
The imx_ldb_encoder_disable function will try to change the parent of
clk_sel to what it was before enabling but at this point the crtc
(inside ipu) is still active. With this patch it gets -EBUSY instead.
This happens because the DRM core disables encoders before crtc (and
backwards on enable). This assumption is reasonable, having an "encoder"
feed a clk back into the "crtc" is odd and needs special handling by the
driver.
Perhaps ipu_di_enable/disable should be handled in atomic_commit_tail
instead of at the crtc level?
More concretely on 6qp-sdb blanking the display happens like this:
* imx_ldb_encoder_disable switches ipu1_di0_sel to ipu1_di0_pre from
ldb_di1_podf
* reparenting to ipu1_di0_pre enables it and its parents up to pll5
* possibly glitchy muxing
* ipu_di_disable disables ipu1_di0 (and parents, up to pll5)
---
drivers/clk/imx/clk-imx6q.c | 4 ++--
drivers/clk/imx/clk-imx6sl.c | 2 +-
drivers/clk/imx/clk-imx6sll.c | 2 +-
drivers/clk/imx/clk-imx6sx.c | 2 +-
drivers/clk/imx/clk-imx6ul.c | 2 +-
drivers/clk/imx/clk.h | 16
6 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
index 8c7c2fcb8d94..0434d943b1bc 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -547,16 +547,16 @@ static void __init imx6q_clocks_init(struct device_node
*ccm_node)
base = of_iomap(np, 0);
WARN_ON(!base);
/* namereg
shift width parent_names num_parents */
clk[IMX6QDL_CLK_STEP] = imx_clk_mux("step",
base + 0xc, 8, 1, step_sels, ARRAY_SIZE(step_sels));
- clk[IMX6QDL_CLK_PLL1_SW] = imx_clk_mux("pll1_sw",
base + 0xc, 2, 1, pll1_sw_sels, ARRAY_SIZE(pll1_sw_sels));
+ clk[IMX6QDL_CLK_PLL1_SW] = imx_clk_mux_glitchless("pll1_sw",
base + 0xc, 2, 1, pll1_sw_sels, ARRAY_SIZE(pll1_sw_sels));
clk[IMX6QDL_CLK_PERIPH_PRE] = imx_clk_mux("periph_pre",
base + 0x18, 18, 2, periph_pre_sels, ARRAY_SIZE(periph_pre_sels));
clk[IMX6QDL_CLK_PERIPH2_PRE] = imx_clk_mux("periph2_pre",
base + 0x18, 21, 2, periph_pre_sels, ARRAY_SIZE(periph_pre_sels));
clk[IMX6QDL_CLK_PERIPH_CLK2_SEL] = imx_clk_mux("periph_clk2_sel",
base + 0x18, 12, 2, periph_clk2_sels, ARRAY_SIZE(periph_clk2_sels));
clk[IMX6QDL_CLK_PERIPH2_CLK2_SEL] = imx_clk_mux("periph2_clk2_sel",
base + 0x18, 20, 1, periph2_clk2_sels, ARRAY_SIZE(periph2_clk2_sels));
- clk[IMX6QDL_CLK_AXI_SEL] = imx_clk_mux("axi_sel",
base + 0x14, 6, 2, axi_sels, ARRAY_SIZE(axi_sels));
+ clk[IMX6QDL_CLK_AXI_SEL] = imx_clk_mux_glitchless("axi_sel",
base + 0x14, 6, 2, axi_sels, ARRAY_SIZE(axi_sels));
clk[IMX6QDL_CLK_ESAI_SEL] = imx_clk_mux("esai_sel",
base + 0x20, 19, 2, audio_sels,ARRAY_SIZE(audio_sels));
clk[IMX6QDL_CLK_ASRC_SEL] = imx_clk_mux("asrc_sel",
base + 0x30, 7, 2, audio_sels,ARRAY_SIZE(audio_sels));
clk[IMX6QDL_CLK_SPDIF_SEL]= imx_clk_mux("spdif_sel",
base + 0x30, 20, 2, audio_sels,ARRAY_SIZE(audio_sels));
if (clk_on_imx6q()) {
clk[IMX6QDL_CLK_GPU2D_AXI]= imx_clk_mux("gpu2d_axi",
base + 0x18, 0, 1, gpu_axi_sels, ARRAY_SIZE(gpu_axi_sels));
diff --git a/drivers/clk/imx/clk-imx6sl.c b/drivers/clk/imx/clk-imx6sl.c
index eb6bcbf345a3..0d1fd6f4 100644
--- a/drivers/clk/imx/clk-imx6sl.c
+++ b/drivers/clk/imx/clk-imx6sl.c
@@ -289,11 +289,11 @@ static void __init imx6sl_clocks_init(struct device_node
*ccm_node)
WARN_ON(!base);
ccm_base = base;
/* namereg
shift width parent_names
Re: [RFC] clk: imx6: Mark imx_clk_mux as glitchy by default
On Tue, 2018-08-21 at 19:42 -0300, Fabio Estevam wrote:
> Hi Leonard,
>
> On Tue, Aug 21, 2018 at 4:34 PM, Leonard Crestez
> wrote:
>
> > More concretely on 6qp-sdb blanking the display happens like this:
> > * imx_ldb_encoder_disable switches ipu1_di0_sel to ipu1_di0_pre from
> > ldb_di1_podf
> > * reparenting to ipu1_di0_pre enables it and its parents up to pll5
> > * possibly glitchy muxing
> > * ipu_di_disable disables ipu1_di0 (and parents, up to pll5)
> We have already taken care of it in these commits:
> clk: imx6: Fix procedure to switch the parent of LDB_DI_CLK
> clk: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only
That's for ldb_di{0,1}_sel, this is for ipu{1,2}_di{0,1}_sel.
On imx6q the ldb_di{0,1}_sel mux has an improperly placed gate and a
special switch procedure needs to be performed at boot time. This
design was fixed on 6qp by moving the ldb_di{0,1} gate immediately
after the mux. My tests are on 6qp.
For ipu{1,2}_di{0,1}_sel the ipu{1,2}_di{0,1} gate can be used on all
chips, it's just that the drm driver doesn't do this. Adding the
CLK_SET_RATE_PARENT flag exposes the issue by returning errors.
> I think we should also take care of the other glitchy muxes as
> you propose here.
>
> Have you seen such glitch issue in practice with the LDB clocks?
If I run "while true; do rtcwake -m mem -s 5; done" it will hang in ~30
minutes because the "suspend devices" takes too long and RTC alarm
expires. I tracked this down to the clk_set_parent call inside
imx_ldb_encoder_disable: sometimes this takes many seconds.
As far as I can tell this live reparenting is unsafe (RM claims
glitches are possible) and not useful (we're deactivating the display
anyway). However I'm not sure this can be blamed on a "glitch".
What seems to be happening is that this triggers the enabling of pll5
(yes, on disable) and the usleep from clk_pllv3_wait_lock takes far too
much time to return. This might be an unrelated timekeeping issue, I'll
look into this further.
According to the RM this reparenting is unsafe anyway so we should
avoid it, probably by disabling ipu_di sooner.
--
Regards,
Leonard
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] clk: imx6: Mark imx_clk_mux as glitchy by default
Hi Leonard,
On Tue, Aug 21, 2018 at 4:34 PM, Leonard Crestez
wrote:
> More concretely on 6qp-sdb blanking the display happens like this:
> * imx_ldb_encoder_disable switches ipu1_di0_sel to ipu1_di0_pre from
> ldb_di1_podf
> * reparenting to ipu1_di0_pre enables it and its parents up to pll5
> * possibly glitchy muxing
> * ipu_di_disable disables ipu1_di0 (and parents, up to pll5)
Have you seen such glitch issue in practice with the LDB clocks?
We have already taken care of it in these commits:
commit 5d283b083800867dc329e6433576664bf0fc18d5
Author: Fabio Estevam
Date: Mon Oct 17 22:29:14 2016 -0200
clk: imx6: Fix procedure to switch the parent of LDB_DI_CLK
Due to incorrect placement of the clock gate cell in the ldb_di[x]_clk
tree, the glitchy parent mux of ldb_di[x]_clk can cause a glitch to
enter the ldb_di_ipu_div divider. If the divider gets locked up, no
ldb_di[x]_clk is generated, and the LVDS display will hang when the
ipu_di_clk is sourced from ldb_di_clk.
To fix the problem, both the new and current parent of the ldb_di_clk
should be disabled before the switch. This patch ensures that correct
steps are followed when ldb_di_clk parent is switched in the beginning
of boot. The glitchy muxes are then registered as read-only. The clock
parent can be selected using the assigned-clocks and
assigned-clock-parents properties of the ccm device tree node:
&clks {
assigned-clocks = <&clks IMX6QDL_CLK_LDB_DI0_SEL>,
<&clks IMX6QDL_CLK_LDB_DI1_SEL>;
assigned-clock-parents = <&clks IMX6QDL_CLK_MMDC_CH1_AXI>,
<&clks IMX6QDL_CLK_PLL5_VIDEO_DIV>;
};
The issue is explained in detail in EB821 ("LDB Clock Switch Procedure &
i.MX6 Asynchronous Clock Switching Guidelines") [1].
[1] http://www.nxp.com/files/32bit/doc/eng_bulletin/EB821.pdf
Signed-off-by: Ranjani Vaidyanathan
Signed-off-by: Fabio Estevam
Signed-off-by: Philipp Zabel
Reviewed-by: Akshay Bhat
Tested-by Joshua Clayton
Tested-by: Charles Kang
Signed-off-by: Shawn Guo
commit 03d576f202e8cd40d500aa4f7594ad702d861096
Author: Philipp Zabel
Date: Mon Oct 17 22:29:13 2016 -0200
clk: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only
Due to incorrect placement of the clock gate cell in the ldb_di[x]_clk
tree, the glitchy parent mux of ldb_di[x]_clk can cause a glitch to
enter the ldb_di_ipu_div divider. If the divider gets locked up, no
ldb_di[x]_clk is generated, and the LVDS display will hang when the
ipu_di_clk is sourced from ldb_di_clk.
To fix the problem, both the new and current parent of the ldb_di_clk
should be disabled before the switch. As this can not be guaranteed by
the clock framework during runtime, make the ldb_di[x]_sel muxes read-only.
A workaround to set the muxes once during boot could be added to the
kernel or bootloader.
Signed-off-by: Philipp Zabel
Signed-off-by: Fabio Estevam
Signed-off-by: Shawn Guo
,but I think we should also take care of the other glitchy muxes as
you propose here.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
