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@xxxxxxxxxxxxxx>
> > ---
> > 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));
