On Thu, 21 Oct 2010, Philip Rakity wrote: > > On Oct 21, 2010, at 9:57 AM, Nicolas Pitre wrote: > > > I don't like this. Again we should strive to make the controller driver > > as simple as possible and let the core code handle this kind of clocking > > logic as much as possible. The OMAP driver is again a terribly bad > > example to follow. > > Not looking at OMAP I know... I'm just dropping a hint in the passing to those who may be concerned. > > > > If there are cases where the clock still has to remain active with some > > cards, it is far better to take care of such exceptions in only one > > place instead of having to add a card quirk in every controller drivers. > > The controller driver should remain card agnostic as much as possible > > and not take decisions on its own based on card type. > > Was not going to add a quirk. The low level platform specific code for > sdhci.c can handle this if it knows the card type. It can program the > appropriate hardware registers. NO!!! That's EXACTLY what I'm against! > > Stopping the clock would also be a good thing even for those controllers > > without dynamic clock support. The core could certainly stop the clock > > manually after a period of inactivity for example. > > > agree except that stopping the clock for SDIO cards is not a good thing to do in our experience. > if the core can signal this via set_ios this works for me. This is why I suggested the code telling > set_ios the card type which allowed the platform specific code to enable or not dynamic > clocks. NO! The core should tell the platform specific code via set_ios() whether or not to enable dynamic clock, period. The _decision_ to do so (is it a SDIO card, is it a broken SDHC card which requires the clock all the time, etc.) should remain in the core code. The controller drivers have no business duplicating the code for making such decisions on their own. > > So if your controller hardware has the ability to dynamically stop the > > clock on its own, then the driver should simply expose that knowledge to > > the core with a capability bit, and the core should tell the driver to > > enable that mode through mmc_set_ios() when applicable. > > sd 3.0 spec (not marvell specific) suppports dynamic clocking as well as marvell v2.0 > controller. > > I can add a CAP bit -- but for SDIO cards we will certainly set this to not disable the clocks. Again, the "for SDIO cards" has to be decided in the *core* code, not in your driver. > 100,000 foot summary > a) add new mmc->caps bit indicating dynamic clocks can be supported in the controller > b) add new field in ios that indicates if clocks should be dynamic > c) pass this to set_ios > d) add platform specific hook to set_ios to control the dynamic clocking option. > > Is my understanding correct ? Yes! And then the core code may either set the dynamic clock bit in the ios structure if the host controller supports it, or manually turn off the clock after a delay of inactivity, but only when this makes sense i.e. not with SDIO cards (and even for SDIO that might be possible if no IRQ is claimed). Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
