[PATCH 1/3] clk: imx6q: reset exclusive gates on init

[PATCH 1/3] clk: imx6q: reset exclusive gates on init

[Lucas Stach]

The exclusive gates may be set up in the wrong way by software running
before the clock driver comes up. In that case the exclusive setup is
locked in its initial state, as the complementary function can't be
activated without disabling the initial setup first.

To avoid this lock situation, reset the exclusive gates to the off
state and allow the kernel to provide the proper setup.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/clk/imx/clk-imx6q.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
index b9ea7037e193..9059a369ae95 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -515,8 +515,12 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	 * lvds1_gate and lvds2_gate are pseudo-gates.  Both can be
 	 * independently configured as clock inputs or outputs.  We treat
 	 * the "output_enable" bit as a gate, even though it's really just
-	 * enabling clock output.
+	 * enabling clock output. Initially the gate bits are cleared, as
+	 * otherwise the exclusive configuration gets locked in the setup done
+	 * by software running before the clock driver, with no way to change
+	 * it.
 	 */
+	writel(readl(base + 0x160) & ~0x3c00, base + 0x160);
 	clk[IMX6QDL_CLK_LVDS1_GATE] = imx_clk_gate_exclusive("lvds1_gate", "lvds1_sel", base + 0x160, 10, BIT(12));
 	clk[IMX6QDL_CLK_LVDS2_GATE] = imx_clk_gate_exclusive("lvds2_gate", "lvds2_sel", base + 0x160, 11, BIT(13));
 
-- 
2.18.0

[PATCH 2/3] clk: imx6q: optionally get CCM inputs via standard clock handles

[Lucas Stach]

When specifying external clock inputs to the CCM the current code
requires the clocks to be in a "clocks" child node of the DT root.
This is not really conformant with DT best practices.

To avoid the need to deviate from those best practices, allow the
clock inputs to be specifies via standard clock handles. This is
in line with how drivers of the later CCM driver revisions on
newer i.MX SoCs handle this.

As we can't retroactively change the DT binding, allow this as an
option with a fallback to the old way of how this has been handled.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 .../devicetree/bindings/clock/imx6q-clock.txt |  5 +++++
 drivers/clk/imx/clk-imx6q.c                   | 22 ++++++++++++++-----
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/imx6q-clock.txt b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
index a45ca67a9d5f..ce6aa9920c05 100644
--- a/Documentation/devicetree/bindings/clock/imx6q-clock.txt
+++ b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
@@ -6,6 +6,11 @@ Required properties:
 - interrupts: Should contain CCM interrupt
 - #clock-cells: Should be <1>
 
+Optional properties:
+- clocks: list of clock specifiers, must contain an entry for each entry
+          in clock-names
+- clock-names: valid names are "osc", "ckil", "ckil", "anaclk1" and "anaclk2"
+
 The clock consumer should specify the desired clock by having the clock
 ID in its "clocks" phandle cell.  See include/dt-bindings/clock/imx6qdl-clock.h
 for the full list of i.MX6 Quad and DualLite clock IDs.
diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
index 9059a369ae95..f64f0fe76658 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -421,12 +421,24 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	int ret;
 
 	clk[IMX6QDL_CLK_DUMMY] = imx_clk_fixed("dummy", 0);
-	clk[IMX6QDL_CLK_CKIL] = imx_obtain_fixed_clock("ckil", 0);
-	clk[IMX6QDL_CLK_CKIH] = imx_obtain_fixed_clock("ckih1", 0);
-	clk[IMX6QDL_CLK_OSC] = imx_obtain_fixed_clock("osc", 0);
+	clk[IMX6QDL_CLK_CKIL] = of_clk_get_by_name(ccm_node, "ckil");
+	if (IS_ERR(clk[IMX6QDL_CLK_CKIL]))
+		clk[IMX6QDL_CLK_CKIL] = imx_obtain_fixed_clock("ckil", 0);
+	clk[IMX6QDL_CLK_CKIH] = of_clk_get_by_name(ccm_node, "ckih1");
+	if (IS_ERR(clk[IMX6QDL_CLK_CKIH]))
+		clk[IMX6QDL_CLK_CKIH] = imx_obtain_fixed_clock("ckih1", 0);
+	clk[IMX6QDL_CLK_OSC] = of_clk_get_by_name(ccm_node, "osc");
+	if (IS_ERR(clk[IMX6QDL_CLK_OSC]))
+		clk[IMX6QDL_CLK_OSC] = imx_obtain_fixed_clock("osc", 0);
+
 	/* Clock source from external clock via CLK1/2 PADs */
-	clk[IMX6QDL_CLK_ANACLK1] = imx_obtain_fixed_clock("anaclk1", 0);
-	clk[IMX6QDL_CLK_ANACLK2] = imx_obtain_fixed_clock("anaclk2", 0);
+	clk[IMX6QDL_CLK_ANACLK1] = of_clk_get_by_name(ccm_node, "anaclk1");
+	if (IS_ERR(clk[IMX6QDL_CLK_ANACLK1]))
+		clk[IMX6QDL_CLK_ANACLK1] = imx_obtain_fixed_clock("anaclk1", 0);
+
+	clk[IMX6QDL_CLK_ANACLK2] = of_clk_get_by_name(ccm_node, "anaclk2");
+	if (IS_ERR(clk[IMX6QDL_CLK_ANACLK2]))
+		clk[IMX6QDL_CLK_ANACLK2] = imx_obtain_fixed_clock("anaclk2", 0);
 
 	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-anatop");
 	anatop_base = base = of_iomap(np, 0);
-- 
2.18.0

Re: [PATCH 2/3] clk: imx6q: optionally get CCM inputs via standard clock handles

[Rob Herring]

On Tue, Jul 31, 2018 at 12:20:08PM +0200, Lucas Stach wrote:
> When specifying external clock inputs to the CCM the current code
> requires the clocks to be in a "clocks" child node of the DT root.
> This is not really conformant with DT best practices.
> 
> To avoid the need to deviate from those best practices, allow the
> clock inputs to be specifies via standard clock handles. This is
> in line with how drivers of the later CCM driver revisions on
> newer i.MX SoCs handle this.
> 
> As we can't retroactively change the DT binding, allow this as an
> option with a fallback to the old way of how this has been handled.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../devicetree/bindings/clock/imx6q-clock.txt |  5 +++++

Ignoring what my bot said if there's no other changes needed,

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/clk/imx/clk-imx6q.c                   | 22 ++++++++++++++-----
>  2 files changed, 22 insertions(+), 5 deletions(-)

RE: [PATCH 2/3] clk: imx6q: optionally get CCM inputs via standard clock handles

[A.s. Dong]

Hi Lucas,

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Tuesday, July 31, 2018 6:20 PM

[...]

> When specifying external clock inputs to the CCM the current code require=
s
> the clocks to be in a "clocks" child node of the DT root.
> This is not really conformant with DT best practices.
>=20
> To avoid the need to deviate from those best practices, allow the clock i=
nputs
> to be specifies via standard clock handles. This is in line with how driv=
ers of
> the later CCM driver revisions on newer i.MX SoCs handle this.
>=20
> As we can't retroactively change the DT binding, allow this as an option =
with a
> fallback to the old way of how this has been handled.
>=20
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../devicetree/bindings/clock/imx6q-clock.txt |  5 +++++
>  drivers/clk/imx/clk-imx6q.c                   | 22 ++++++++++++++-----
>  2 files changed, 22 insertions(+), 5 deletions(-)
>=20
> diff --git a/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> index a45ca67a9d5f..ce6aa9920c05 100644
> --- a/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> +++ b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> @@ -6,6 +6,11 @@ Required properties:
>  - interrupts: Should contain CCM interrupt
>  - #clock-cells: Should be <1>
>=20
> +Optional properties:
> +- clocks: list of clock specifiers, must contain an entry for each entry
> +          in clock-names
> +- clock-names: valid names are "osc", "ckil", "ckil", "anaclk1" and "ana=
clk2"

A minor issue:=20
The third name should be "ckih1".

Otherwise:
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

Regards
Dong Aisheng

RE: [PATCH 2/3] clk: imx6q: optionally get CCM inputs via standard clock handles

[Stephen Boyd]

Quoting A.s. Dong (2018-08-21 01:56:09)
> Hi Lucas,
> 
> > -----Original Message-----
> > From: Lucas Stach <l.stach@pengutronix.de>
> > Sent: Tuesday, July 31, 2018 6:20 PM
> 
> [...]
> 
> > When specifying external clock inputs to the CCM the current code requires
> > the clocks to be in a "clocks" child node of the DT root.
> > This is not really conformant with DT best practices.
> > 
> > To avoid the need to deviate from those best practices, allow the clock inputs
> > to be specifies via standard clock handles. This is in line with how drivers of
> > the later CCM driver revisions on newer i.MX SoCs handle this.
> > 
> > As we can't retroactively change the DT binding, allow this as an option with a
> > fallback to the old way of how this has been handled.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  .../devicetree/bindings/clock/imx6q-clock.txt |  5 +++++
> >  drivers/clk/imx/clk-imx6q.c                   | 22 ++++++++++++++-----
> >  2 files changed, 22 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> > b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> > index a45ca67a9d5f..ce6aa9920c05 100644
> > --- a/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> > +++ b/Documentation/devicetree/bindings/clock/imx6q-clock.txt
> > @@ -6,6 +6,11 @@ Required properties:
> >  - interrupts: Should contain CCM interrupt
> >  - #clock-cells: Should be <1>
> > 
> > +Optional properties:
> > +- clocks: list of clock specifiers, must contain an entry for each entry
> > +          in clock-names
> > +- clock-names: valid names are "osc", "ckil", "ckil", "anaclk1" and "anaclk2"
> 
> A minor issue: 
> The third name should be "ckih1".
> 
> Otherwise:
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> 

Is this going to be resent? I don't see anything new and there are
review comments on this patch series so I'm dropping it from the clk
review queue. Please resend.

[PATCH 3/3] clk: imx6q: handle ENET PLL bypass

[Lucas Stach]

The ENET PLL is different from the other i.MX6 PLLs, as it has
multiple outputs with different post-dividers, which are all
bypassed if the single bypass bit is activated. The hardware setup
looks something like this:
                                _
refclk-o---PLL---o----DIV1-----| \
       |         |             |M |----OUT1
       o-----------------------|_/
       |         |              _
       |         o----DIV2-----| \
       |         |             |M |----OUT2
       o-----------------------|_/
       |         |              _
       |         `----DIV3-----| \
       |                       |M |----OUT3
       `-----------------------|_/

The bypass bit not only bypasses the PLL, but also the attached
post-dividers. This would be reasonbly straight forward to model
with a single output, or with different bypass bits for each output,
but sadly the HW guys decided that it would be good to actuate all
3 muxes with a single bit.

So the need to have the PLL bypassed for one of the outputs always
affects 2 other (in our model) independent branches of the clock
tree.

This means the decision to bypass this PLL is a system wide design
choice and should not be changed on-the-fly, so we can treat any
bapass configuration as static. As such we can just register the
post-dividiers with a ratio that reflects the bypass status, which
allows us to bypass the PLL without breaking our abstraction model
and with it DT stability.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/clk/imx/clk-imx6q.c | 63 +++++++++++++++++++++++++++++++++----
 1 file changed, 57 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
index f64f0fe76658..c52294694273 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -231,6 +231,41 @@ static void of_assigned_ldb_sels(struct device_node *node,
 	}
 }
 
+static bool pll6_bypassed(struct device_node *node)
+{
+	int index, ret, num_clocks;
+	struct of_phandle_args clkspec;
+
+	num_clocks = of_count_phandle_with_args(node, "assigned-clocks",
+						"#clock-cells");
+	if (num_clocks < 0)
+		return false;
+
+	for (index = 0; index < num_clocks; index++) {
+		ret = of_parse_phandle_with_args(node, "assigned-clocks",
+						 "#clock-cells", index,
+						 &clkspec);
+		if (ret < 0)
+			return false;
+
+		if (clkspec.np == node &&
+		    clkspec.args[0] == IMX6QDL_PLL6_BYPASS)
+			break;
+	}
+
+	/* PLL6 bypass is not part of the assigned clock list */
+	if (index == num_clocks)
+		return false;
+
+	ret = of_parse_phandle_with_args(node, "assigned-clock-parents",
+					 "#clock-cells", index, &clkspec);
+
+	if (clkspec.args[0] != IMX6QDL_CLK_PLL6)
+		return true;
+
+	return false;
+}
+
 #define CCM_CCDR		0x04
 #define CCM_CCSR		0x0c
 #define CCM_CS2CDR		0x2c
@@ -510,16 +545,32 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	clk[IMX6QDL_CLK_USBPHY1_GATE] = imx_clk_gate("usbphy1_gate", "dummy", base + 0x10, 6);
 	clk[IMX6QDL_CLK_USBPHY2_GATE] = imx_clk_gate("usbphy2_gate", "dummy", base + 0x20, 6);
 
-	clk[IMX6QDL_CLK_SATA_REF] = imx_clk_fixed_factor("sata_ref", "pll6_enet", 1, 5);
-	clk[IMX6QDL_CLK_PCIE_REF] = imx_clk_fixed_factor("pcie_ref", "pll6_enet", 1, 4);
+	/*
+	 * The ENET PLL is special in that is has multiple outputs with
+	 * different post-dividers that are all affected by the single bypass
+	 * bit, so a single mux bit affects 3 independent branches of the clock
+	 * tree. There is no good way to model this in the clock framework and
+	 * dynamically changing the bypass bit, will yield unexpected results.
+	 * So we treat any configuration that bypasses the ENET PLL as
+	 * essentially static with the divider ratios refelcting the bypass
+	 * status.
+	 *
+	 */
+	if (!pll6_bypassed(ccm_node)) {
+		clk[IMX6QDL_CLK_SATA_REF] = imx_clk_fixed_factor("sata_ref", "pll6_enet", 1, 5);
+		clk[IMX6QDL_CLK_PCIE_REF] = imx_clk_fixed_factor("pcie_ref", "pll6_enet", 1, 4);
+		clk[IMX6QDL_CLK_ENET_REF] = clk_register_divider_table(NULL, "enet_ref", "pll6_enet", 0,
+						base + 0xe0, 0, 2, 0, clk_enet_ref_table,
+						&imx_ccm_lock);
+	} else {
+		clk[IMX6QDL_CLK_SATA_REF] = imx_clk_fixed_factor("sata_ref", "pll6_enet", 1, 1);
+		clk[IMX6QDL_CLK_PCIE_REF] = imx_clk_fixed_factor("pcie_ref", "pll6_enet", 1, 1);
+		clk[IMX6QDL_CLK_ENET_REF] = imx_clk_fixed_factor("enet_ref", "pll6_enet", 1, 1);
+	}
 
 	clk[IMX6QDL_CLK_SATA_REF_100M] = imx_clk_gate("sata_ref_100m", "sata_ref", base + 0xe0, 20);
 	clk[IMX6QDL_CLK_PCIE_REF_125M] = imx_clk_gate("pcie_ref_125m", "pcie_ref", base + 0xe0, 19);
 
-	clk[IMX6QDL_CLK_ENET_REF] = clk_register_divider_table(NULL, "enet_ref", "pll6_enet", 0,
-			base + 0xe0, 0, 2, 0, clk_enet_ref_table,
-			&imx_ccm_lock);
-
 	clk[IMX6QDL_CLK_LVDS1_SEL] = imx_clk_mux("lvds1_sel", base + 0x160, 0, 5, lvds_sels, ARRAY_SIZE(lvds_sels));
 	clk[IMX6QDL_CLK_LVDS2_SEL] = imx_clk_mux("lvds2_sel", base + 0x160, 5, 5, lvds_sels, ARRAY_SIZE(lvds_sels));
 
-- 
2.18.0

Re: [PATCH 3/3] clk: imx6q: handle ENET PLL bypass

[Stefan Agner]

On 31.07.2018 12:20, Lucas Stach wrote:
> The ENET PLL is different from the other i.MX6 PLLs, as it has
> multiple outputs with different post-dividers, which are all
> bypassed if the single bypass bit is activated. The hardware setup
> looks something like this:
>                                 _
> refclk-o---PLL---o----DIV1-----| \
>        |         |             |M |----OUT1
>        o-----------------------|_/
>        |         |              _
>        |         o----DIV2-----| \
>        |         |             |M |----OUT2
>        o-----------------------|_/
>        |         |              _
>        |         `----DIV3-----| \
>        |                       |M |----OUT3
>        `-----------------------|_/
> 
> The bypass bit not only bypasses the PLL, but also the attached
> post-dividers. This would be reasonbly straight forward to model
> with a single output, or with different bypass bits for each output,
> but sadly the HW guys decided that it would be good to actuate all
> 3 muxes with a single bit.

So that bypass bit is set when using IMX6QDL_PLL6_BYPASS_SRC correct?

So what happens today when one is doing that? Clocks such as
IMX6QDL_CLK_ENET_REF get the clock rate wrong?

> 
> So the need to have the PLL bypassed for one of the outputs always
> affects 2 other (in our model) independent branches of the clock
> tree.
> 
> This means the decision to bypass this PLL is a system wide design
> choice and should not be changed on-the-fly, so we can treat any
> bapass configuration as static. As such we can just register the

s/bapass/bypass

> post-dividiers with a ratio that reflects the bypass status, which
> allows us to bypass the PLL without breaking our abstraction model
> and with it DT stability.

I am assuming that the bypass bit is set depending on device tree parent
setting.

So you basically parse the device tree again to infer what the code did
a bit further up?

Can we not just read the bit from hardware? We already access clock
registers directly why not this one...

--
Stefan

> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/clk/imx/clk-imx6q.c | 63 +++++++++++++++++++++++++++++++++----
>  1 file changed, 57 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
> index f64f0fe76658..c52294694273 100644
> --- a/drivers/clk/imx/clk-imx6q.c
> +++ b/drivers/clk/imx/clk-imx6q.c
> @@ -231,6 +231,41 @@ static void of_assigned_ldb_sels(struct device_node *node,
>  	}
>  }
>  
> +static bool pll6_bypassed(struct device_node *node)
> +{
> +	int index, ret, num_clocks;
> +	struct of_phandle_args clkspec;
> +
> +	num_clocks = of_count_phandle_with_args(node, "assigned-clocks",
> +						"#clock-cells");
> +	if (num_clocks < 0)
> +		return false;
> +
> +	for (index = 0; index < num_clocks; index++) {
> +		ret = of_parse_phandle_with_args(node, "assigned-clocks",
> +						 "#clock-cells", index,
> +						 &clkspec);
> +		if (ret < 0)
> +			return false;
> +
> +		if (clkspec.np == node &&
> +		    clkspec.args[0] == IMX6QDL_PLL6_BYPASS)
> +			break;
> +	}
> +
> +	/* PLL6 bypass is not part of the assigned clock list */
> +	if (index == num_clocks)
> +		return false;
> +
> +	ret = of_parse_phandle_with_args(node, "assigned-clock-parents",
> +					 "#clock-cells", index, &clkspec);
> +
> +	if (clkspec.args[0] != IMX6QDL_CLK_PLL6)
> +		return true;
> +
> +	return false;
> +}
> +
>  #define CCM_CCDR		0x04
>  #define CCM_CCSR		0x0c
>  #define CCM_CS2CDR		0x2c
> @@ -510,16 +545,32 @@ static void __init imx6q_clocks_init(struct
> device_node *ccm_node)
>  	clk[IMX6QDL_CLK_USBPHY1_GATE] = imx_clk_gate("usbphy1_gate",
> "dummy", base + 0x10, 6);
>  	clk[IMX6QDL_CLK_USBPHY2_GATE] = imx_clk_gate("usbphy2_gate",
> "dummy", base + 0x20, 6);
>  
> -	clk[IMX6QDL_CLK_SATA_REF] = imx_clk_fixed_factor("sata_ref",
> "pll6_enet", 1, 5);
> -	clk[IMX6QDL_CLK_PCIE_REF] = imx_clk_fixed_factor("pcie_ref",
> "pll6_enet", 1, 4);
> +	/*
> +	 * The ENET PLL is special in that is has multiple outputs with
> +	 * different post-dividers that are all affected by the single bypass
> +	 * bit, so a single mux bit affects 3 independent branches of the clock
> +	 * tree. There is no good way to model this in the clock framework and
> +	 * dynamically changing the bypass bit, will yield unexpected results.
> +	 * So we treat any configuration that bypasses the ENET PLL as
> +	 * essentially static with the divider ratios refelcting the bypass
> +	 * status.
> +	 *
> +	 */
> +	if (!pll6_bypassed(ccm_node)) {
> +		clk[IMX6QDL_CLK_SATA_REF] = imx_clk_fixed_factor("sata_ref",
> "pll6_enet", 1, 5);
> +		clk[IMX6QDL_CLK_PCIE_REF] = imx_clk_fixed_factor("pcie_ref",
> "pll6_enet", 1, 4);
> +		clk[IMX6QDL_CLK_ENET_REF] = clk_register_divider_table(NULL,
> "enet_ref", "pll6_enet", 0,
> +						base + 0xe0, 0, 2, 0, clk_enet_ref_table,
> +						&imx_ccm_lock);
> +	} else {
> +		clk[IMX6QDL_CLK_SATA_REF] = imx_clk_fixed_factor("sata_ref",
> "pll6_enet", 1, 1);
> +		clk[IMX6QDL_CLK_PCIE_REF] = imx_clk_fixed_factor("pcie_ref",
> "pll6_enet", 1, 1);
> +		clk[IMX6QDL_CLK_ENET_REF] = imx_clk_fixed_factor("enet_ref",
> "pll6_enet", 1, 1);
> +	}
>  
>  	clk[IMX6QDL_CLK_SATA_REF_100M] = imx_clk_gate("sata_ref_100m",
> "sata_ref", base + 0xe0, 20);
>  	clk[IMX6QDL_CLK_PCIE_REF_125M] = imx_clk_gate("pcie_ref_125m",
> "pcie_ref", base + 0xe0, 19);
>  
> -	clk[IMX6QDL_CLK_ENET_REF] = clk_register_divider_table(NULL,
> "enet_ref", "pll6_enet", 0,
> -			base + 0xe0, 0, 2, 0, clk_enet_ref_table,
> -			&imx_ccm_lock);
> -
>  	clk[IMX6QDL_CLK_LVDS1_SEL] = imx_clk_mux("lvds1_sel", base + 0x160,
> 0, 5, lvds_sels, ARRAY_SIZE(lvds_sels));
>  	clk[IMX6QDL_CLK_LVDS2_SEL] = imx_clk_mux("lvds2_sel", base + 0x160,
> 5, 5, lvds_sels, ARRAY_SIZE(lvds_sels));

Re: [PATCH 3/3] clk: imx6q: handle ENET PLL bypass

[Lucas Stach]

Am Freitag, den 10.08.2018, 09:45 +0200 schrieb Stefan Agner:
> On 31.07.2018 12:20, Lucas Stach wrote:
> > The ENET PLL is different from the other i.MX6 PLLs, as it has
> > multiple outputs with different post-dividers, which are all
> > bypassed if the single bypass bit is activated. The hardware setup
> > looks something like this:
> >                                 _
> > refclk-o---PLL---o----DIV1-----| \
> >        |         |             |M |----OUT1
> >        o-----------------------|_/
> >        |         |              _
> >        |         o----DIV2-----| \
> >        |         |             |M |----OUT2
> >        o-----------------------|_/
> >        |         |              _
> >        |         `----DIV3-----| \
> >        |                       |M |----OUT3
> >        `-----------------------|_/
> > 
> > The bypass bit not only bypasses the PLL, but also the attached
> > post-dividers. This would be reasonbly straight forward to model
> > with a single output, or with different bypass bits for each output,
> > but sadly the HW guys decided that it would be good to actuate all
> > 3 muxes with a single bit.
> 
> So that bypass bit is set when using IMX6QDL_PLL6_BYPASS_SRC correct?
> 
> So what happens today when one is doing that? Clocks such as
> IMX6QDL_CLK_ENET_REF get the clock rate wrong?

Yep, exactly.
It's not a big deal for most systems, as most of them never want the
ENET PLL bypassed, but for the case where we want to feed an external
clock into the PCIe controller the only way to do this is through the
PLL bypass (breaking SATA in the process if the external clock is not
100MHz or SS and breaking ENET if the ref clock isn't generated by the
PHY).

> > 
> > So the need to have the PLL bypassed for one of the outputs always
> > affects 2 other (in our model) independent branches of the clock
> > tree.
> > 
> > This means the decision to bypass this PLL is a system wide design
> > choice and should not be changed on-the-fly, so we can treat any
> > bapass configuration as static. As such we can just register the
> 
> s/bapass/bypass
> 
> > post-dividiers with a ratio that reflects the bypass status, which
> > allows us to bypass the PLL without breaking our abstraction model
> > and with it DT stability.
> 
> I am assuming that the bypass bit is set depending on device tree parent
> setting.
> 
> So you basically parse the device tree again to infer what the code did
> a bit further up?
> 
> Can we not just read the bit from hardware? We already access clock
> registers directly why not this one...

The DT assigned-clock stuff gets applied by the clk core _after_ the
clk controller is registered. What we are doing here is changing the
clock tree setup before registering the controller.

Regards,
Lucas

> --
> Stefan
> 
> > 
> > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/clk/imx/clk-imx6q.c | 63 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 57 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
> > index f64f0fe76658..c52294694273 100644
> > --- a/drivers/clk/imx/clk-imx6q.c
> > +++ b/drivers/clk/imx/clk-imx6q.c
> > @@ -231,6 +231,41 @@ static void of_assigned_ldb_sels(struct device_node *node,
> > > >  	}
> >  }
> >  
> > +static bool pll6_bypassed(struct device_node *node)
> > +{
> > > > +	int index, ret, num_clocks;
> > > > +	struct of_phandle_args clkspec;
> > +
> > > > +	num_clocks = of_count_phandle_with_args(node, "assigned-clocks",
> > > > +						"#clock-cells");
> > > > +	if (num_clocks < 0)
> > > > +		return false;
> > +
> > > > +	for (index = 0; index < num_clocks; index++) {
> > > > +		ret = of_parse_phandle_with_args(node, "assigned-clocks",
> > > > +						 "#clock-cells", index,
> > > > +						 &clkspec);
> > > > +		if (ret < 0)
> > > > +			return false;
> > +
> > > > +		if (clkspec.np == node &&
> > > > +		    clkspec.args[0] == IMX6QDL_PLL6_BYPASS)
> > > > +			break;
> > > > +	}
> > +
> > > > +	/* PLL6 bypass is not part of the assigned clock list */
> > > > +	if (index == num_clocks)
> > > > +		return false;
> > +
> > > > +	ret = of_parse_phandle_with_args(node, "assigned-clock-parents",
> > > > +					 "#clock-cells", index, &clkspec);
> > +
> > > > +	if (clkspec.args[0] != IMX6QDL_CLK_PLL6)
> > > > +		return true;
> > +
> > > > +	return false;
> > +}
> > +
> > > >  #define CCM_CCDR		0x04
> > > >  #define CCM_CCSR		0x0c
> > > >  #define CCM_CS2CDR		0x2c
> > @@ -510,16 +545,32 @@ static void __init imx6q_clocks_init(struct
> > device_node *ccm_node)
> > > >  	clk[IMX6QDL_CLK_USBPHY1_GATE] = imx_clk_gate("usbphy1_gate",
> > "dummy", base + 0x10, 6);
> > > >  	clk[IMX6QDL_CLK_USBPHY2_GATE] = imx_clk_gate("usbphy2_gate",
> > "dummy", base + 0x20, 6);
> >  
> > > > -	clk[IMX6QDL_CLK_SATA_REF] = imx_clk_fixed_factor("sata_ref",
> > "pll6_enet", 1, 5);
> > > > -	clk[IMX6QDL_CLK_PCIE_REF] = imx_clk_fixed_factor("pcie_ref",
> > "pll6_enet", 1, 4);
> > > > +	/*
> > > > +	 * The ENET PLL is special in that is has multiple outputs with
> > > > +	 * different post-dividers that are all affected by the single bypass
> > > > +	 * bit, so a single mux bit affects 3 independent branches of the clock
> > > > +	 * tree. There is no good way to model this in the clock framework and
> > > > +	 * dynamically changing the bypass bit, will yield unexpected results.
> > > > +	 * So we treat any configuration that bypasses the ENET PLL as
> > > > +	 * essentially static with the divider ratios refelcting the bypass
> > > > +	 * status.
> > > > +	 *
> > > > +	 */
> > > > +	if (!pll6_bypassed(ccm_node)) {
> > > > +		clk[IMX6QDL_CLK_SATA_REF] = imx_clk_fixed_factor("sata_ref",
> > "pll6_enet", 1, 5);
> > > > +		clk[IMX6QDL_CLK_PCIE_REF] = imx_clk_fixed_factor("pcie_ref",
> > "pll6_enet", 1, 4);
> > > > +		clk[IMX6QDL_CLK_ENET_REF] = clk_register_divider_table(NULL,
> > "enet_ref", "pll6_enet", 0,
> > > > +						base + 0xe0, 0, 2, 0, clk_enet_ref_table,
> > > > +						&imx_ccm_lock);
> > > > +	} else {
> > > > +		clk[IMX6QDL_CLK_SATA_REF] = imx_clk_fixed_factor("sata_ref",
> > "pll6_enet", 1, 1);
> > > > +		clk[IMX6QDL_CLK_PCIE_REF] = imx_clk_fixed_factor("pcie_ref",
> > "pll6_enet", 1, 1);
> > > > +		clk[IMX6QDL_CLK_ENET_REF] = imx_clk_fixed_factor("enet_ref",
> > "pll6_enet", 1, 1);
> > > > +	}
> >  
> > > >  	clk[IMX6QDL_CLK_SATA_REF_100M] = imx_clk_gate("sata_ref_100m",
> > "sata_ref", base + 0xe0, 20);
> > > >  	clk[IMX6QDL_CLK_PCIE_REF_125M] = imx_clk_gate("pcie_ref_125m",
> > "pcie_ref", base + 0xe0, 19);
> >  
> > > > -	clk[IMX6QDL_CLK_ENET_REF] = clk_register_divider_table(NULL,
> > "enet_ref", "pll6_enet", 0,
> > > > -			base + 0xe0, 0, 2, 0, clk_enet_ref_table,
> > > > -			&imx_ccm_lock);
> > -
> > > >  	clk[IMX6QDL_CLK_LVDS1_SEL] = imx_clk_mux("lvds1_sel", base + 0x160,
> > 0, 5, lvds_sels, ARRAY_SIZE(lvds_sels));
> > > >  	clk[IMX6QDL_CLK_LVDS2_SEL] = imx_clk_mux("lvds2_sel", base + 0x160,
> > 5, 5, lvds_sels, ARRAY_SIZE(lvds_sels));

RE: [PATCH 1/3] clk: imx6q: reset exclusive gates on init

[A.s. Dong]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Tuesday, July 31, 2018 6:20 PM

[...]

>=20
> The exclusive gates may be set up in the wrong way by software running
> before the clock driver comes up. In that case the exclusive setup is loc=
ked in
> its initial state, as the complementary function can't be activated witho=
ut
> disabling the initial setup first.
>=20
> To avoid this lock situation, reset the exclusive gates to the off state =
and
> allow the kernel to provide the proper setup.
>=20
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

Reviewed-by: Dong Aisheng <Aisheng.dong@nxp.com>

BTW, I'm just a bit curious what software may do a wrong setting before?

Regards
Dong Aisheng

> ---
>  drivers/clk/imx/clk-imx6q.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>=20
> diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c in=
dex
> b9ea7037e193..9059a369ae95 100644
> --- a/drivers/clk/imx/clk-imx6q.c
> +++ b/drivers/clk/imx/clk-imx6q.c
> @@ -515,8 +515,12 @@ static void __init imx6q_clocks_init(struct
> device_node *ccm_node)
>  	 * lvds1_gate and lvds2_gate are pseudo-gates.  Both can be
>  	 * independently configured as clock inputs or outputs.  We treat
>  	 * the "output_enable" bit as a gate, even though it's really just
> -	 * enabling clock output.
> +	 * enabling clock output. Initially the gate bits are cleared, as
> +	 * otherwise the exclusive configuration gets locked in the setup
> done
> +	 * by software running before the clock driver, with no way to change
> +	 * it.
>  	 */
> +	writel(readl(base + 0x160) & ~0x3c00, base + 0x160);
>  	clk[IMX6QDL_CLK_LVDS1_GATE] =3D
> imx_clk_gate_exclusive("lvds1_gate", "lvds1_sel", base + 0x160, 10, BIT(1=
2));
>  	clk[IMX6QDL_CLK_LVDS2_GATE] =3D
> imx_clk_gate_exclusive("lvds2_gate", "lvds2_sel", base + 0x160, 11, BIT(1=
3));
>=20
> --
> 2.18.0