Re: [PATCH 1/4] host1x: mipi: Add new parent clock for mipi calibration

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3674 bytes --]

On Thu, Aug 07, 2014 at 10:15:42AM -0400, Sean Paul wrote:
> On Thu, Aug 7, 2014 at 4:11 AM, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Thu, Aug 07, 2014 at 02:11:44AM -0400, Sean Paul wrote:
> >> This patch adds a new parent clock to enable/disable the 72MHz
> >> clock required for mipi calibration.
> >
> > s/mipi/MIPI/ please. Also this doesn't explain why this change is
> > necessary. Doesn't MIPI D-PHY calibration work without this patch? It
> > sure does for me.
> 
> Hi Thierry,
> Thanks for the prompt reviews.
> 
> It doesn't work for me on T132 without this additional clock. It seems
> the source for mipi-cal has changed between T124 & T132 from PLL_OUT3
> to CLK72MHZ, so that could be why it's working for you and not for me.

I don't have Tegra124 hardware with DSI (I don't have Tegra132 hardware
with DSI either, for that matter) so I only tested on Tegra114. So I
guess it's possible that Tegra124 already uses clk72mhz as parent for
mipi_cal. However I can't find any authoritative source as to what
exactly is the parent on Tegra124 and later.

Peter, can you help find out what the right thing to do is here? The
clock driver currently always registers the mipi_cal clock as child of
clk_m, but that's clearly not correct. Should this be split up per SoC
so that it's a child of pll_p_out3 on Tegra114 and clk72mhz on Tegra124
and later?

> > Furthermore you say 72 MHz clock, but the below uses PLL_P_OUT3 as the
> > parent in the example, yet PLL_P_OUT3 runs at 102 MHz on all of my
> > systems. What 72 MHz clock are you referring to?
> >
> 
> This was just a bogus assumption on my part that PLL_P_OUT3 was to be
> programmed to 72MHz on pre-T132 setups.
> 
> > Also can this parent clock ever be anything other than PLL_P_OUT3? If
> > not it would probably be better to set that statically in the clock
> > initialization tables.
> 
> I'm not entirely certain how I'd set CLK72MHZ statically in the init
> tables, could you elaborate? I can get it to work by re-parenting
> mipi-cal to clk72mhz, however I'm not sure if that would break other
> platforms.

Given the above that probably won't work. The reason is that the clock
is a simple gate and uses clk_m as a parent (see the gate_clks array in
drivers/clk/tegra/clk-tegra-periph.c).

What I think should work is if we rip out the mipi_cal entry and move
registration of the clock into clk-tegra114.c and clk-tegra124.c
respectively.

> My initial thought was to add a compatible = "nvidia,tegra132-mipi",
> which then would require the parent to be present in the dt.

We need to add a new compatible anyway since there are register
differences between the two. But I'd assume that the compatible should
be "nvidia,tegra124-mipi" since it most likely remained unmodified from
Tegra124 to Tegra132.

> >> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
> >> index 9882ea1..4dd91fd 100644
> >> --- a/drivers/gpu/host1x/mipi.c
> >> +++ b/drivers/gpu/host1x/mipi.c
> >> @@ -80,7 +80,8 @@ static const struct module {
> >>  struct tegra_mipi {
> >>       void __iomem *regs;
> >>       struct mutex lock;
> >> -     struct clk *clk;
> >> +     struct clk *clk_parent;
> >> +     struct clk *clk_mipi_cal;
> >
> > I don't think the clk -> clk_mipi_cal rename is warranted here.
> 
> Will do.

Given the above discussion I think this patch may simply become obsolete
if the mipi_cal clock is properly registered with pll_p_out3 or clk72mhz
as parent, since enabling the clock will then end up propagating the
enable up the whole clock tree.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]