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@xxxxxxxxx> 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:
pgpkeUObeITAH.pgp
Description: PGP signature