Re: [PATCH] ARM: tegra: fix ulpi regression on tegra20
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.
Re: [PATCH] ARM: tegra: fix ulpi regression on tegra20
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 f
Re: [PATCH] ARM: tegra: fix ulpi regression on tegra20
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
Re: [PATCH] ARM: tegra: fix ulpi regression on tegra20
Hi Marc On Fri, 2018-04-20 at 10:52 +0200, Marc Dietrich wrote: > > ... > I booted 4.17-rc1 (which includes this fix) on an AC100 (T20 paz00 > board) and > the error above is still there. Surprisingly the error vanishes when > I revert > your patch. So this patch actually *causes* the problem above on my > board. That's really strange. I believe I do have one of them paz00 boards laying around somewhere as well. Just need to dig it out again and will give it a try. Looking at their schematics at least reveals the exact same circuit as found on all other T20 based boards using DAP_MCLK2 as REFCLK to the USB3315C which BTW is 24 MHz and not 26 MHz as CDEV2 claims! > Could it be, that we need all four clocks? Dimitry mentioned on IRC > that it > could also be a problem in the clock init table. I don't have the > technical > background myself to fix it, but I still wonder what could be so > different > between TrimSlice and AC100. I am wondering the same. However I still suspect that something is completely wrong in that area as that CDEV2 clock is completely bogus. It really does not exist! > Marc Cheers Marcel
Re: [PATCH] ARM: tegra: fix ulpi regression on tegra20
On 20.04.2018 11:52, Marc Dietrich wrote:
> Hi Marcel,
>
> Am Montag, 19. Februar 2018, 16:12:52 CEST schrieb Marcel Ziswiler:
>> From: Marcel Ziswiler
>>
>> Since commit f8f8f1d04494 ("clk: Don't touch hardware when reparenting
>> during registration") ULPI has been broken on Tegra20 leading to the
>> following error message during boot:
>>
>> [1.974698] ulpi_phy_power_on: ulpi write failed
>> [1.979384] tegra-ehci c5004000.usb: Failed to power on the phy
>> [1.985434] tegra-ehci: probe of c5004000.usb failed with error -110
>>
>> Debugging through the changes and finally also consulting the TRM
>> revealed that rather than the CDEV2 clock off OSC requiring such pin
>> muxing actually the PLL_P_OUT4 clock is in use. It looks like so far it
>> just worked by chance of that one having been enabled which Stephen's
>> commit now changed when reparenting sclk away from pll_p_out4 leaving
>> that one disabled. Fix this by properly assigning the PLL_P_OUT4 clock
>> as the ULPI PHY clock.
>
> I booted 4.17-rc1 (which includes this fix) on an AC100 (T20 paz00 board) and
> the error above is still there. Surprisingly the error vanishes when I revert
> your patch. So this patch actually *causes* the problem above on my board.
> Could it be, that we need all four clocks? Dimitry mentioned on IRC that it
> could also be a problem in the clock init table. I don't have the technical
> background myself to fix it, but I still wonder what could be so different
> between TrimSlice and AC100.
I managed to find CDEV clocks in TRM this time. Seems assigning CDEV2 clock to
"ulpi-link" was correct and both CDEV2 and PLL_P_OUT4 should be enabled, 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.
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?
It looks like there is some clk framework bug, 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;
Re: [PATCH] ARM: tegra: fix ulpi regression on tegra20
Hi Marcel,
Am Montag, 19. Februar 2018, 16:12:52 CEST schrieb Marcel Ziswiler:
> From: Marcel Ziswiler
>
> Since commit f8f8f1d04494 ("clk: Don't touch hardware when reparenting
> during registration") ULPI has been broken on Tegra20 leading to the
> following error message during boot:
>
> [1.974698] ulpi_phy_power_on: ulpi write failed
> [1.979384] tegra-ehci c5004000.usb: Failed to power on the phy
> [1.985434] tegra-ehci: probe of c5004000.usb failed with error -110
>
> Debugging through the changes and finally also consulting the TRM
> revealed that rather than the CDEV2 clock off OSC requiring such pin
> muxing actually the PLL_P_OUT4 clock is in use. It looks like so far it
> just worked by chance of that one having been enabled which Stephen's
> commit now changed when reparenting sclk away from pll_p_out4 leaving
> that one disabled. Fix this by properly assigning the PLL_P_OUT4 clock
> as the ULPI PHY clock.
I booted 4.17-rc1 (which includes this fix) on an AC100 (T20 paz00 board) and
the error above is still there. Surprisingly the error vanishes when I revert
your patch. So this patch actually *causes* the problem above on my board.
Could it be, that we need all four clocks? Dimitry mentioned on IRC that it
could also be a problem in the clock init table. I don't have the technical
background myself to fix it, but I still wonder what could be so different
between TrimSlice and AC100.
Marc
> Signed-off-by: Marcel Ziswiler
>
> ---
>
> arch/arm/boot/dts/tegra20.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 864a95872b8d..e05b6bb2599f 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -741,7 +741,7 @@
> phy_type = "ulpi";
> clocks = <&tegra_car TEGRA20_CLK_USB2>,
><&tegra_car TEGRA20_CLK_PLL_U>,
> - <&tegra_car TEGRA20_CLK_CDEV2>;
> + <&tegra_car TEGRA20_CLK_PLL_P_OUT4>;
> clock-names = "reg", "pll_u", "ulpi-link";
> resets = <&tegra_car 58>, <&tegra_car 22>;
> reset-names = "usb", "utmi-pads";
[PATCH] ARM: tegra: fix ulpi regression on tegra20
From: Marcel Ziswiler
Since commit f8f8f1d04494 ("clk: Don't touch hardware when reparenting
during registration") ULPI has been broken on Tegra20 leading to the
following error message during boot:
[1.974698] ulpi_phy_power_on: ulpi write failed
[1.979384] tegra-ehci c5004000.usb: Failed to power on the phy
[1.985434] tegra-ehci: probe of c5004000.usb failed with error -110
Debugging through the changes and finally also consulting the TRM
revealed that rather than the CDEV2 clock off OSC requiring such pin
muxing actually the PLL_P_OUT4 clock is in use. It looks like so far it
just worked by chance of that one having been enabled which Stephen's
commit now changed when reparenting sclk away from pll_p_out4 leaving
that one disabled. Fix this by properly assigning the PLL_P_OUT4 clock
as the ULPI PHY clock.
Signed-off-by: Marcel Ziswiler
---
arch/arm/boot/dts/tegra20.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 864a95872b8d..e05b6bb2599f 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -741,7 +741,7 @@
phy_type = "ulpi";
clocks = <&tegra_car TEGRA20_CLK_USB2>,
<&tegra_car TEGRA20_CLK_PLL_U>,
-<&tegra_car TEGRA20_CLK_CDEV2>;
+<&tegra_car TEGRA20_CLK_PLL_P_OUT4>;
clock-names = "reg", "pll_u", "ulpi-link";
resets = <&tegra_car 58>, <&tegra_car 22>;
reset-names = "usb", "utmi-pads";
--
2.14.3
