ARM: tegra: fix ulpi regression on tegra20

Commit Message

Dmitry Osipenko April 24, 2018, 2:38 p.m. UTC

Hi Marcel,

On 24.04.2018 01:05, Marcel Ziswiler wrote:
> 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. 

That's the DEV2 clock you're talking about below.

>> 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.

Could be that DEV2_OSC_DIV_SEL is only relevant when DAP_MCLK2 is mux'ed to OSC.
Maybe Peter could clarify everything.

>> 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.

Pin is driven by the PLL_P_OUT4, which is set to 24 MHz via the init_table. The
clock tree is invalid in that regards because Tegra's clock driver defines
non-existent "cdev2_fixed" 26 MHz clock source and sets it as a parent for the
"cdev2" clock, while "cdev2" should be a gate (and maybe divider?) for the real
parent clock (PLL_P_OUT4 in our case).

> What do you think?

It looks to me that a clock signal coming from the mux'ed CDEV2 pin is routed to
the DEV2 of CLK controller (which can gate it) and then it goes out to DAP_MCLK2.

PLL_P_OUT4 ---> PINMUX_CDEV2 ---> CLK_DEV2 ---> DAP_MCLK2

But it also could be that CLK_ENB_DEV2_OUT is indeed some internal
pinmux-related clock-gate. I think Peter should have a clue.

Anyway the implementation details do not really matter for us, what matters is
that PLL_P_OUT4 clk and DEV2 clk-gate must be enabled.

Seems indeed ideally would be to have DAP_MCLK2 for the USB controller's
"ulpi-link" clock and then DEV2 will be enable bit for the DAP_MCLK2. But also
information about parent clock of the DAP_MCLK2 needs to be conducted to the clk
driver via DT, that probably would require backward-incompatible change of the
binding which is undesirable.

One tolerable solution could be to remove fake "cdev2_fixed" clock in the driver
and hardcode PLL_P_OUT4 for the parent of "cdev2". (Note that cdev1 also has a
fake parent)

index 0ee56dd04cec..4708653dedeb 100644

Now the real problem is that PLL_P_OUT4 was getting disabled. We had CDEV2 clock
in the DT for "ulpi-link" and PLL_P_OUT4 was permanently enabled (supposedly) by
the Tegra's clock driver using init_table. Apparently PLL_P_OUT4 was disabled on
SCLK re-parenting, as you mentioned in the commit description. This should be
considered as a bug because one of two purposes (permanently enable and setup
default clock rate) of the init_table is defeated.

Could you please try to bisect to the point when erroneous PLL_P_OUT4 disable
issue is fixed->broken? It is quite important to know what commit changed the
behaviour. If it was some common clk issue that got fixed - that should be a
good case for us, if it was some unrelated change that obscured the issue -
that's not good and we should go back and fix the real bug.

Comments

Peter De Schrijver April 26, 2018, 11:39 a.m. UTC | #1

On Tue, Apr 24, 2018 at 05:38:41PM +0300, Dmitry Osipenko wrote:
> Hi Marcel,
> 
> On 24.04.2018 01:05, Marcel Ziswiler wrote:
> > 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. 
> 
> That's the DEV2 clock you're talking about below.
> 
> >> 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.
> 
> Could be that DEV2_OSC_DIV_SEL is only relevant when DAP_MCLK2 is mux'ed to OSC.
> Maybe Peter could clarify everything.
> 

Yes. This looks likely. Problem here is that the source selection for cdev1
and cdev2 is done via pinmuxing rather than via a regular mux control in CAR.
So probably the "OSC" input is not really OSC but the output of the divider
configured in CLK_RST_CONTROLLER_MISC_CLK_ENB_0.

Possible solutions I see are:

1) make the tegra20 pinctrl driver a clock provider for the cdev1 and cdev2
   clocks. We would then need to introduce 2 new clocks cdev1_mux and
   cdev2_mux which represent pinmuxes. cdev1 and cdev2 would use those as
   a parent. cdev1_fixed and cdev2_fixed would need to be renamed to refelect
   the settings in CLK_RST_CONTROLLER_MISC_CLK_ENB_0 and can be inputs for
   cdev1_mux and cdev2_mux

2) read the pinmux configuration for cdev1 and cdev2 at boot. this would
   require mapping the apbmisc aperture in the clock driver, but we already
   map the PMC aperture, so there's a precedent for mappings outside CAR.
   Then define the cdev1 and cdev2 parent accordingly. Also cdev1_fixed and
   cdev2_fixed could be defined as the divided version of OSC according to
   the settings in CLK_RST_CONTROLLER_MISC_CLK_ENB_0. Limitation of this
   approach is that we assume these settings are never changed at runtime.
   This seems acceptable to me though. Or do we know of a use case where the
   setting is changed at runtime?

Solution 1 is nicer, but would require backwards-incompatible changes to the
DT bindings.

Peter.

Patch

--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -838,8 +838,7 @@  static void __init tegra20_periph_clk_init(void)
        clks[TEGRA20_CLK_CDEV1] = clk;

        /* cdev2 */
-       clk = clk_register_fixed_rate(NULL, "cdev2_fixed", NULL, 0, 26000000);
-       clk = tegra_clk_register_periph_gate("cdev2", "cdev2_fixed", 0,
+       clk = tegra_clk_register_periph_gate("cdev2", "pll_p_out4", 0,
                                    clk_base, 0, 93, periph_clk_enb_refcnt);
        clks[TEGRA20_CLK_CDEV2] = clk;