On Tue, Jul 07, 2009 at 00:34:23, David Brownell wrote: > On Monday 06 July 2009, Sekhar Nori wrote: > > The patch implements cpufreq driver, support to change PLL > > output rate and recalculation of the rates of PLL derived clocks > > It seems to me this is missing some sanity checks. > > First, that the DRAM isn't being clocked through this PLL... > since changing the PLL would imply recalculating all of its > timings, and might require running the re-clock from SRAM. > > Second, similar issues crop up with every clock derived > from that same PLL. If you change the clock going into > the MMC controller, that can require recalculating the > dividers used to clock the MMC/SD card it's talking to. > > Now, those are issues the clock framework handles poorly. > So I don't think there are likely to be easy fixes for > those PLL recalc problems ... unless I missed something.
All of these are valid side effects of changing the PLL frequency runtime. But, this patch just implements the functionality of changing the rate of a given PLL without worrying too much about the side effects. I think the responsibility of taking care of the side effects should lie with the particular <soc>.c file or other caller which is attempting to change the rate. The side affects are different for different SoCs. For OMAP-L138, changing the PLL0 rate doesn't affect the mDDR as that clock is derived from PLL1. But there are a bunch of other peripherals which will get affected. I think peripheral rate changes are taken care of by the CPUFreq pre- and post- rate change notification mechanism (which I must admit I am not on top of right now). > > It might be simpler to just restrict a first pass of > this to changing dividers for the ARM's clock. > This is not straightforward either; most of the SYSCLKs have fixed ratios with other SYSCLKs - so changing one affects others. Example on OMAP-L138, SYSCLK6 (ARM clock) and SYSCLK2 (MMC/SD0 etc) are tied 1:2. > > Also, this patch is doing two separate things. One is > adding clk_set_rate() support for PLLs. The other is > matching $SUBJECT. Better to split those two. Okay sure. > > (And notice how your patch [2/2] hit that second issue > already, with the UART2 clock getting goofed ...) > Yes, and da850.c takes care of it in its own specific way, by moving UART2 to a separate clock domain. I agree code to take care of all frequency change side-effects is not there yet. I submitted the clock rate change code early to get some feedback (and hopefully acceptance :) going. As we add more peripheral capability, we will also need support to get it working correctly with CPUFreq. Thanks, Sekhar _______________________________________________ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
