Re: [PATCH] ARM: tegra: fix ulpi regression on tegra20

[Marcel Ziswiler <marcel.ziswiler@toradex.com>]

Hi Dmitry

On Fri, 2018-04-20 at 13:50 +0300, Dmitry Osipenko wrote:
> ...
> I managed to find CDEV clocks in TRM this time.

And where exactly in which TRM? In all my attempts at finding anything
CDEV2 is just a pad group which includes the DAP_MCLK2 pad in question.

> Seems assigning CDEV2 clock to
> "ulpi-link" was correct

Sorry, but I do really not see how you can get to any such conclusion.

Whatever that CDEV2 clock exactly is. Its offset 93 points towards the
CLK_RST_CONTROLLER_CLK_OUT_ENB_U_0 register which has CLK_ENB_DEV2_OUT
at bit position 29 reading "Enable clock to DEV2 pad". While the TRM
misses further documenting what exactly that DEV2 pad should be I guess
it may point towards our suspected DAP_MCLK2. So for some reason
besides PLL_P_OUT4 which is what that pad is actually muxed to also
some additional DEV2 pad clock needs enabling. In addition there would
be a CLK_RST_CONTROLLER_MISC_CLK_ENB_0 register where one could also
specify a DEV2_OSC_DIV_SEL but I would assume that one only applies if
the pad in question being muxed to OSC which is not the case at least
in all device trees I have looked at.

> and both CDEV2 and PLL_P_OUT4 should be enabled,

Agreed, basically the DAP_MCLK2 pad seems gated by an additional enable
called CLK_ENB_DEV2_OUT.

> CDEV2
> should gate the PLL_P_OUT4 that feeds USB HW and PLL_P_OUT4 should be
> always-enabled because it is enabled by init_table, but apparently it
> is getting
> disabled erroneously.

At least that was the case back when I had trouble getting ULPI to work
on T20. Strangely latest -next right now does no longer seem to suffer
from that same issue even if my patch is reverted but as mentioned
before nobody stops the required PLL_P_OUT4 which is what is actually
driving DAP_MCLK2 to not be changed behind the scenes breaking the
whole thing again.

> Marcel, could you please revert your patch, add
> "trace_event=clk_enable,clk_disable,clk_set_parent tp_printk" to
> kernels cmdline
> and post the log?

Yes, the only difference in those traces is whether or not that non-
existent CDEV2 clock is enabled or not:

[    1.897521] clk_enable: cdev2_fixed
[    1.901008] clk_enable: cdev2

I also do have an explanation why it kept working in my case. Probably
the boot ROM or U-Boot is already setting that bit.

> It looks like there is some clk framework bug,

My conclusion is that there should be a DAP_MCLK2 clock which is gated
by that CLK_ENB_DEV2_OUT but really outputs a T20 internal clock
according to its pin muxing which if set to OSC may be further divided
down by DEV2_OSC_DIV_SEL.

> but just in case please also try
> to apply this patch:
> 
> diff --git a/drivers/clk/tegra/clk-tegra-periph.c
> b/drivers/clk/tegra/clk-tegra-periph.c
> index 2acba2986bc6..407bd0c0ac2f 100644
> --- a/drivers/clk/tegra/clk-tegra-periph.c
> +++ b/drivers/clk/tegra/clk-tegra-periph.c
> @@ -1024,7 +1024,7 @@ static void __init init_pllp(void __iomem
> *clk_base, void
> __iomem *pmc_base,
>                 if (dt_clk) {
>                         clk =
> tegra_clk_register_pll_out("pll_p_out4",
>                                         "pll_p_out4_div", clk_base +
> PLLP_OUTB,
> -                                       17, 16, CLK_IGNORE_UNUSED |
> +                                       17, 16, CLK_IS_CRITICAL |
>                                         CLK_SET_RATE_PARENT, 0,
>                                         &PLLP_OUTB_lock);
>                         *dt_clk = clk;

I did not have to do any such but maybe that would at least prevent any
further issues on PLL_P_OUT4. However I still believe this is kind of
wrong as PLL_P_OUT4 is what drives DAP_MCLK2 in its current muxing
which is connected to the ULPI transceivers REFCLK pin.

BTW: That pin is usually to be driven at 24 MHz and not 26 MHz which
CDEV2 claims to be at.

What do you think?

Cheers

Marcel