On Wed, May 27, 2015 at 12:50:21PM -0700, Stephen Boyd wrote: > On 05/21, Mikko Perttunen wrote: > > On 05/20/2015 10:54 PM, Stephen Boyd wrote: > > > On 05/13, Thierry Reding wrote: > > >> Hi Mike, Stephen, > > >> > > >> The following changes since commit b787f68c36d49bb1d9236f403813641efa74a031: > > >> > > >> Linux 4.1-rc1 (2015-04-26 17:59:10 -0700) > > >> > > >> are available in the git repository at: > > >> > > >> git://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git tags/tegra-for-4.2-clk > > >> > > >> for you to fetch changes up to 36b7be6d3ea8f434f1e0723f3fb0e85c3e00ebc2: > > >> > > >> clk: tegra: Fix hda2codec_2x clock name for Tegra30 (2015-05-13 15:17:14 +0200) > > >> > > >> I've based this pull request on top of the tegra-for-4.2-ramcode pull > > >> request, so pulling only this one should be sufficient to resolve the > > >> dependency. > > >> > > >> Thanks, > > >> Thierry > > >> > > >> ---------------------------------------------------------------- > > >> clk: tegra: Changes for v4.2-rc1 > > >> > > >> This contains the EMC clock driver that's been exhaustively reviewed and > > >> tested. It also includes a change to the clock core that allows a clock > > >> provider to perform low-level reparenting of clocks. This is required by > > >> the EMC clock driver because the reparenting needs to be done at a very > > >> specific point in time during the EMC frequency switch. > > > > > > Can someone please describe why we need to do software > > > reparenting at a specific point in time during a frequency > > > switch? I must have missed out on the conversation somewhere and > > > looking at the commit that introduces the function, the argument > > > for why the API is exposed: > > > > > > To be used by clock implementations for switching to a new > > > parent during rate change. > > > > > > is lacking in any details about *why* we need it. > > > > > > > Hi Stephen, > > > > the way the EMC clock rate is set in hardware is similar to other > > clocks, i.e. there's a register and you write the new divider and parent > > id to it. However, with EMC, you cannot just do this any time you want; > > writing to the register initiates a state machine in the clock hardware > > that changes a large number of other parameters regarding DRAM timings > > as well. These parameters need to be programmed into shadow registers > > before the rate change write is done. This means that both the new > > divisor and the parent must be written in the same register write. > > > > The CCF has a callback, set_rate_and_parent, which allows for both to be > > passed to the driver at the same time. However, it also requires > > set_rate and set_parent to be implemented, which the EMC driver cannot do. > > Ok so we should change the framework to allow drivers to only > implement set_rate_and_parent and use that in set_rate and > set_parent implementations if it's the only option available. Or > is there a problem if CCF allows clk_set_parent() on the EMC > clock by calling set_rate_and_parent() with the new parent and > whatever recalc_rate returns for the new parent's rate going into > the clock? > > > > > Furthermore, the CCF cannot help with parent selection for the EMC clock > > at all. The parent for each rate is selected by the board designer. > > I'm not following this part. The CCF mostly asks clock providers > what parent should be used when clk_set_rate() is called. > There are a fixed number of allowed EMC OPPs per board. For each OPP there is a set of EMC parameters, the parent clock and the rate for the parent clock defined. Due to jitter requirements these settings have to be used. It's not allowed to choose a different parent even when that would result in the same clock rate. > > > > There is also the issue that sometimes, the EMC driver cannot directly > > switch to the target (rate, parent) pair; instead it is necessary to > > switch first to another pair we call a backup timing. In this situation, > > we temporarily have a parent that is neither the one we had before the > > set_rate call or the one it will be after. Especially, if the switch to > > the backup timing succeeds but the following switch to the target timing > > fails, then without the reparent call the parent in hardware would be > > left inconsistent with the software state. > > Yes, I've sent a patch a while ago to support an idea of a "safe > parent" or a backup timing that can be used while a clock is > reprogrammed. Mike has also proposed the concept of "coordinated > clock rates" which would do something similar and allow clock > providers to control complicated rate transitions themselves. It > seems that we may be able to use the "safe parent" concept here, > where first we switch to some other parent, call the set_rate* > op, and then switch the parent back if we're not already using > the parent that we should be using. > Due to the interlock between the CAR block and the EMC controller, there is a precise sequence of register accesses to be followed when doing a EMC frequency switch. Changing the EMC clock parent or divider in CAR triggers a state machine in the EMC controller which causes a bunch of shadow registers to take effect and a FIFO of commands to be send to the SDRAM chips. Because of this precise sequence, I think a single entity should be responsible for handling this. Splitting the sequence in multiple parts maintained by different people, will greatly increase the risk of difficult to trace stability regressions. Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
