On 11/18/2013 11:39 AM, Mark Brown wrote: > On Mon, Nov 18, 2013 at 10:25:46AM -0700, Stephen Warren wrote: >> On 11/16/2013 03:02 AM, Mark Brown wrote: > >>> Or alternatively should the driver be making the device cache >>> only when runtime PM is disabled? > >> The regmap is already cache-only when runtime-suspended. However >> the registers don't get flushed during resume. I suppose that >> would require only adding one extra call to the PM resume >> function? > > Yup, should probably do that anyway since that's going to bite > someone otherwise. We probably want a single operation to flush > and enable writes now I think about it, it's the common case. > >> For some reason, my gut prefers this current solution, but I >> could be persuaded. > > I'm not dead set against doing it this way, it's just that it feels > like if it's required something's not working as well as it > should. The problems with the following approach: static int tegra30_ahub_runtime_resume(struct device *dev) ... regcache_cache_only(ahub->regmap_apbif, false); regcache_cache_only(ahub->regmap_ahub, false); + regcache_sync(ahub->regmap_apbif, false); + regcache_sync(ahub->regmap_ahub, false); (or an automated variant of it, whereby regmap automatically does the sync inside cache_only(false)) are: 1) regcache_sync() doesn't mean "if the cache is dirty, flush the dirty registers to HW", but rather, "if the cache is dirty, write any registers that don't match HW defaults to HW". If the HW was in cache-only mode because simply because clocks were turned off but not power, then the register values were retained, and the current HW register values may not be "HW defaults", and you may in fact /need/ to write a value to HW that matches the HW default, yet is different from the current retained register content. 2) tegra30_ahub_allocate_tx_fifo() does a read-modify-write of a control register, and may execute when cache_only(true). If that register was never touched while cache_only(false), which is the case the first time the r-m-w happens, then there is no cached value, so the regmap_read() for it will fail. This will either cause tegra30_ahub_allocate_tx_fifo() to use uninitialized return data from regmap_read() (if error-checking is missing, which it is), or fail (if the error-checking were present). Neither is a good choice. Possible solutions are: a) Make regmap creation read the initial HW state to use as HW defaults when the regmap is created. IIRC, this is done for some regmap configurations but not others. That said, this doesn't seem correct, since there's no guarantee that the HW state when the regmap is created /is/ the default HW state. b) Add a table of HW register defaults. Aside from this issue, there's no need for this, so I'm not really motivated to do this. Besides, the number of registers is rather large. c) Simply put pm_runtime_get()/put() around the body of tegra30_ahub_allocate_tx_fifo(), which works fine, and was my original patch. d) Have regmap_read/write do a pm_runtime_get()/put() themselves. I know you mentioned this earlier in the thread, but you'd rejected this approach when I first upstreamed this AHUB driver, and said to rely on cache_only instead. I think I still prefer option (c). -- 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
