2022-01-25 22:22:51

by Christian Eggers

[permalink] [raw]
Subject: [PATCH RESEND 0/6] clk: imx6*: avoid GPMI clock glitches on reparenting/divider change

RESEND because the series got corrupted on first tranmission

On the i.MX6 series (but not on i.MX7/8), most clock multiplexers and
switchable dividers are not "glitch safe", so switching them while a
consumer is connected can cause glitches with a higher frequency than
supported by the consuming peripheral.

Without fixes, peripherals can fail occasionally. One example is
that system boot fails due to lockup of the GPMI NAND controller:

f53d4c109a66 ("mtd: rawnand: gpmi: Add ERR007117 protection for nfc_apply_timings")

The conditions where this can appear also depend on the device
from which the system is booted and whether the bootloader has already
performed all reparenting of the clocks.

To avoid these problems, the clock subsystem must:
- Gate consumer clocks during reparenting
- Enforce that all consumer clocks are gated before client drivers can change the divider



2022-01-25 22:30:04

by Christian Eggers

[permalink] [raw]
Subject: [PATCH RESEND 4/6] clk: imx6q: enforce gating of gpmi_io clock

Clock parent and divider changes are both glitchy for enfc_clock_root.
Enforce that the child clock is gated.

Signed-off-by: Christian Eggers <[email protected]>
---
drivers/clk/imx/clk-imx6q.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
index fd5c37095ed0..390566fca054 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -674,7 +674,7 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
hws[IMX6QDL_CLK_USDHC2_SEL] = imx_clk_hw_mux("usdhc2_sel", base + 0x1c, 17, 1, usdhc_sels, ARRAY_SIZE(usdhc_sels));
hws[IMX6QDL_CLK_USDHC3_SEL] = imx_clk_hw_mux("usdhc3_sel", base + 0x1c, 18, 1, usdhc_sels, ARRAY_SIZE(usdhc_sels));
hws[IMX6QDL_CLK_USDHC4_SEL] = imx_clk_hw_mux("usdhc4_sel", base + 0x1c, 19, 1, usdhc_sels, ARRAY_SIZE(usdhc_sels));
- hws[IMX6QDL_CLK_ENFC_SEL] = imx_clk_hw_mux("enfc_sel", base + 0x2c, 15, 3, enfc_sels_2, ARRAY_SIZE(enfc_sels_2));
+ hws[IMX6QDL_CLK_ENFC_SEL] = imx_clk_hw_mux_flags("enfc_sel", base + 0x2c, 15, 3, enfc_sels_2, ARRAY_SIZE(enfc_sels_2), CLK_SET_PARENT_GATE);
hws[IMX6QDL_CLK_EIM_SEL] = imx_clk_hw_mux("eim_sel", base + 0x1c, 27, 2, eim_sels, ARRAY_SIZE(eim_sels));
hws[IMX6QDL_CLK_EIM_SLOW_SEL] = imx_clk_hw_mux("eim_slow_sel", base + 0x1c, 29, 2, eim_slow_sels, ARRAY_SIZE(eim_slow_sels));
hws[IMX6QDL_CLK_PRE_AXI] = imx_clk_hw_mux("pre_axi", base + 0x18, 1, 1, pre_axi_sels, ARRAY_SIZE(pre_axi_sels));
@@ -864,7 +864,7 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
hws[IMX6QDL_CLK_PWM4] = imx_clk_hw_gate2("pwm4", "ipg_per", base + 0x78, 22);
hws[IMX6QDL_CLK_GPMI_BCH_APB] = imx_clk_hw_gate2("gpmi_bch_apb", "usdhc3", base + 0x78, 24);
hws[IMX6QDL_CLK_GPMI_BCH] = imx_clk_hw_gate2("gpmi_bch", "usdhc4", base + 0x78, 26);
- hws[IMX6QDL_CLK_GPMI_IO] = imx_clk_hw_gate2("gpmi_io", "enfc", base + 0x78, 28);
+ hws[IMX6QDL_CLK_GPMI_IO] = imx_clk_hw_gate2_flags("gpmi_io", "enfc", base + 0x78, 28, CLK_SET_RATE_GATE);
hws[IMX6QDL_CLK_GPMI_APB] = imx_clk_hw_gate2("gpmi_apb", "usdhc3", base + 0x78, 30);
hws[IMX6QDL_CLK_ROM] = imx_clk_hw_gate2_flags("rom", "ahb", base + 0x7c, 0, CLK_IS_CRITICAL);
hws[IMX6QDL_CLK_SATA] = imx_clk_hw_gate2("sata", "ahb", base + 0x7c, 4);
--
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler

2022-01-25 22:31:24

by Christian Eggers

[permalink] [raw]
Subject: [PATCH RESEND 3/6] clk: imx6q: disable gpmi_io and ipt_io clocks before changing parent

gpmi_io and ipt_io clocks may have been enabled by the boot loader. All
children of enfc_clk_root must be gated in order to prevent glitches
during parent change.

Reparenting of enfc_clk_root may disable pll3_usb_otg. In order to avoid
immediately re-enabling it in imx_register_uart_clocks(), the whole
section has been moved to the bottom of imx6q_clocks_init().

Signed-off-by: Christian Eggers <[email protected]>
Co-developed-by: Stefan Riedmueller <[email protected]>
Signed-off-by: Stefan Riedmueller <[email protected]>
---
drivers/clk/imx/clk-imx6q.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
index de36f58d551c..fd5c37095ed0 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -927,13 +927,6 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
clk_set_parent(hws[IMX6QDL_CLK_IPU2_DI0_SEL]->clk, hws[IMX6QDL_CLK_IPU2_DI0_PRE]->clk);
clk_set_parent(hws[IMX6QDL_CLK_IPU2_DI1_SEL]->clk, hws[IMX6QDL_CLK_IPU2_DI1_PRE]->clk);

- /*
- * The gpmi needs 100MHz frequency in the EDO/Sync mode,
- * We can not get the 100MHz from the pll2_pfd0_352m.
- * So choose pll2_pfd2_396m as enfc_sel's parent.
- */
- clk_set_parent(hws[IMX6QDL_CLK_ENFC_SEL]->clk, hws[IMX6QDL_CLK_PLL2_PFD2_396M]->clk);
-
if (IS_ENABLED(CONFIG_USB_MXS_PHY)) {
clk_prepare_enable(hws[IMX6QDL_CLK_USBPHY1_GATE]->clk);
clk_prepare_enable(hws[IMX6QDL_CLK_USBPHY2_GATE]->clk);
@@ -975,5 +968,25 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
}

imx_register_uart_clocks(2);
+
+ /*
+ * The gpmi needs 100MHz frequency in the EDO/Sync mode. We can not get
+ * the 100MHz from the pll2_pfd0_352m. So choose pll2_pfd2_396m as
+ * enfc_sel's parent.
+ *
+ * gpmi_io and ipt_clk_io clocks may have been enabled by the boot
+ * loader. All children of enfc_clk_root must be gated in order to
+ * prevent glitches during parent change. The task of re-enabling
+ * gpio_io is left to the gpmi-nand driver.
+ */
+ if (clk_hw_is_enabled(hws[IMX6QDL_CLK_GPMI_IO])) {
+ clk_prepare_enable(hws[IMX6QDL_CLK_GPMI_IO]->clk);
+ clk_disable_unprepare(hws[IMX6QDL_CLK_GPMI_IO]->clk);
+ }
+ if (clk_hw_is_enabled(hws[IMX6QDL_CLK_ENFC])) {
+ clk_prepare_enable(hws[IMX6QDL_CLK_ENFC]->clk);
+ clk_disable_unprepare(hws[IMX6QDL_CLK_ENFC]->clk);
+ }
+ clk_set_parent(hws[IMX6QDL_CLK_ENFC_SEL]->clk, hws[IMX6QDL_CLK_PLL2_PFD2_396M]->clk);
}
CLK_OF_DECLARE(imx6q, "fsl,imx6q-ccm", imx6q_clocks_init);
--
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler

2022-01-25 22:32:41

by Christian Eggers

[permalink] [raw]
Subject: [PATCH RESEND 5/6] clk: imx6sx: disable gpmi_io clock before changing parent clock

gpmi_io clocks may have been enabled by the boot loader. All children of
qspi2_clk_root must be gated in order to prevent glitches during parent
change.

Signed-off-by: Christian Eggers <[email protected]>
---
drivers/clk/imx/clk-imx6sx.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx6sx.c b/drivers/clk/imx/clk-imx6sx.c
index fc1bd23d4583..cf1c1fad45f9 100644
--- a/drivers/clk/imx/clk-imx6sx.c
+++ b/drivers/clk/imx/clk-imx6sx.c
@@ -546,8 +546,19 @@ static void __init imx6sx_clocks_init(struct device_node *ccm_node)
clk_set_parent(hws[IMX6SX_CLK_GPU_AXI_SEL]->clk, hws[IMX6SX_CLK_PLL3_PFD0]->clk);

clk_set_parent(hws[IMX6SX_CLK_QSPI1_SEL]->clk, hws[IMX6SX_CLK_PLL2_BUS]->clk);
- clk_set_parent(hws[IMX6SX_CLK_QSPI2_SEL]->clk, hws[IMX6SX_CLK_PLL2_BUS]->clk);

imx_register_uart_clocks(2);
+
+ /*
+ * gpmi_io clock may have been enabled by the boot loader. All children
+ * of qspi2_clk_root must be gated in order to prevent glitches during
+ * parent change. The task of re-enabling is left to the gpmi-nand
+ * driver.
+ */
+ if (clk_hw_is_enabled(hws[IMX6SX_CLK_GPMI_IO])) {
+ clk_prepare_enable(hws[IMX6SX_CLK_GPMI_IO]->clk);
+ clk_disable_unprepare(hws[IMX6SX_CLK_GPMI_IO]->clk);
+ }
+ clk_set_parent(hws[IMX6SX_CLK_QSPI2_SEL]->clk, hws[IMX6SX_CLK_PLL2_BUS]->clk);
}
CLK_OF_DECLARE(imx6sx, "fsl,imx6sx-ccm", imx6sx_clocks_init);
--
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler

2022-01-25 22:43:03

by Christian Eggers

[permalink] [raw]
Subject: [PATCH RESEND 6/6] clk: imx6sx: enforce gating of gpmi_io clock

Clock parent and divider changes are both glitchy for qspi2_clock_root.
Enforce that the child clock is gated.

Signed-off-by: Christian Eggers <[email protected]>
---
drivers/clk/imx/clk-imx6sx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6sx.c b/drivers/clk/imx/clk-imx6sx.c
index cf1c1fad45f9..023a18594ebe 100644
--- a/drivers/clk/imx/clk-imx6sx.c
+++ b/drivers/clk/imx/clk-imx6sx.c
@@ -286,7 +286,7 @@ static void __init imx6sx_clocks_init(struct device_node *ccm_node)
hws[IMX6SX_CLK_ESAI_SEL] = imx_clk_hw_mux("esai_sel", base + 0x20, 19, 2, audio_sels, ARRAY_SIZE(audio_sels));
hws[IMX6SX_CLK_CAN_SEL] = imx_clk_hw_mux("can_sel", base + 0x20, 8, 2, can_sels, ARRAY_SIZE(can_sels));
hws[IMX6SX_CLK_UART_SEL] = imx_clk_hw_mux("uart_sel", base + 0x24, 6, 1, uart_sels, ARRAY_SIZE(uart_sels));
- hws[IMX6SX_CLK_QSPI2_SEL] = imx_clk_hw_mux_flags("qspi2_sel", base + 0x2c, 15, 3, qspi2_sels, ARRAY_SIZE(qspi2_sels), CLK_SET_RATE_PARENT);
+ hws[IMX6SX_CLK_QSPI2_SEL] = imx_clk_hw_mux_flags("qspi2_sel", base + 0x2c, 15, 3, qspi2_sels, ARRAY_SIZE(qspi2_sels), CLK_SET_RATE_PARENT | CLK_SET_PARENT_GATE);
hws[IMX6SX_CLK_SPDIF_SEL] = imx_clk_hw_mux("spdif_sel", base + 0x30, 20, 2, audio_sels, ARRAY_SIZE(audio_sels));
hws[IMX6SX_CLK_AUDIO_SEL] = imx_clk_hw_mux("audio_sel", base + 0x30, 7, 2, audio_sels, ARRAY_SIZE(audio_sels));
hws[IMX6SX_CLK_ENET_PRE_SEL] = imx_clk_hw_mux("enet_pre_sel", base + 0x34, 15, 3, enet_pre_sels, ARRAY_SIZE(enet_pre_sels));
@@ -441,7 +441,7 @@ static void __init imx6sx_clocks_init(struct device_node *ccm_node)
hws[IMX6SX_CLK_PWM4] = imx_clk_hw_gate2("pwm4", "perclk", base + 0x78, 22);
hws[IMX6SX_CLK_GPMI_BCH_APB] = imx_clk_hw_gate2("gpmi_bch_apb", "usdhc3", base + 0x78, 24);
hws[IMX6SX_CLK_GPMI_BCH] = imx_clk_hw_gate2("gpmi_bch", "usdhc4", base + 0x78, 26);
- hws[IMX6SX_CLK_GPMI_IO] = imx_clk_hw_gate2("gpmi_io", "qspi2_podf", base + 0x78, 28);
+ hws[IMX6SX_CLK_GPMI_IO] = imx_clk_hw_gate2_flags("gpmi_io", "qspi2_podf", base + 0x78, 28, CLK_SET_RATE_GATE);
hws[IMX6SX_CLK_GPMI_APB] = imx_clk_hw_gate2("gpmi_apb", "usdhc3", base + 0x78, 30);

/* CCGR5 */
--
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler

2022-01-26 11:22:42

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH RESEND 0/6] clk: imx6*: avoid GPMI clock glitches on reparenting/divider change

Quoting Christian Eggers (2022-01-25 06:44:35)
> RESEND because the series got corrupted on first tranmission

Still looks corrupted :(