2009/10/4 Magnus Damm <magnus.damm@xxxxxxxxx>: > Hi Ian, Hi, >> That was an example. I suppose if your HCLK (the input or host clock, >> as named in the TMIO specs) never falls below double the maximum >> MMC/SD bus speed, then you will be ok. > > That should be the case. Thanks for explaining. Np. >> I doubt it'd be much use, the TMIO MMC core doesnt use much power to >> preserve its state. on my platforms, the chip containing it is simply >> put to sleep and can remain so for days without draining the battery, >> and when it wakes up again the registers are as expected. > > Well you measure days but I want to measure weeks. Well, days on a tiniy battery with the CPU and everything else in power-on-suspend. > The SDHI block can > most likely be found in SoCs aimed for portable handsets, and for such > devices "days" are just not good enough. Agreed. > The TMIO block itself may not use much power, but it's pretty common > to have power domains that cover a range of hardware blocks. Hm, that could get ugly. the MFD concept would tend to consider an MFD to control the power to its blocks. You want to use an MFD subdriver outside the power domain of the 'MFD' that contains it. >> Thus far only one TMIO block has register save / restore and thats >> because its in a braindamaged hardware design that shuts off power to >> the ASIC when it sleeps. > > Or maybe they have very coarse grained power domains. =) Some deeper > sleep modes on certain embedded SoCs turn off the power domains which > will give the same result as you are describing. No, its because that particular device is braindamaged. Normally, the tc6393xb MFD maintainns the state of all its blocks (even if the block itself is disabled in the MFD system config register) so you can suspend and resume the MFD without powering it off. One of sharps devices kills power to the ship too, which needed working around in the video driver. >> Its probably not worth it, unless the SH design is merely a copy of >> the tmio design rather than a reimplementation. it barely uses any >> power when the clock is off anyway. > > It's worth it, believe me. Do you have any figures (measure current draw) to back that up? Seriously, on e750 the power draw of the tc6393 in standby is so low my meter cant measure it. >> Given how low power the TMIO hardware is, and how fast it resumes, >> that seems counterproductive to me. > > Well, ask anyone technically competent doing runtime power management > for embedded systems and they'll probably say the same thing. I can > introduce you to some ARM hackers fairly high in the food chain that > most likely will agree with me. I wont consider anyone 'technically competant' unless they can back their claims with some facts. If its as big a win as all that *for tmio-mmc* then it wont be hard to back that with some measurements. Implement it as a hack, try it out, see if its worth it. >> I'd like to see that this would result in significant power saving >> first. simply halting the block on my boards drops it to damn near >> zero consumption, and its still ready to go then. > > Right, stopping the clock is great as a first step. This is the very > first step of power managment. As for the SDHI block - we're not even > fully there yet because we don't control what you call HCLK > dynamically. The next step after mucking around with more fine grained > clock stopping would be to implement save and restore. We dont control HCLK because the documentation for the tmio-mmc block says one should use the clock gating that is provided by it. Also stopping HCLK on platforms with tc63**xb would probably hang the system as soon as any of the other peripherals in the MFD got accessed. the SDHI block has the same clock gating available, so I expect its best to use it. Further, there is nothing in the docs detailing how one should start / stop HCLK for tmio-mmc - it is assumed that the MFD containing it always has HCLK enabled prior to starting the MMC block, and stopped after disabling it (or on suspend / resume) >> Thats easily solved - agree on what needs to be done first. > > Good. But I think we both knew what was needed earlier. From my > perspective, I needed to use the tmio-mmc driver on hardware that > wasn't supported yet. I sent a bunch of patches. Some got in, some > didn't which left my hardware unsupported. Thats pretty normal patch submission procedure. Somee make it, some get asked for a rework. > Then there was this clock framework discussion. I can understand your > pain with the lack of arch-independent clock framework support. But on > my hardware the clock framework is working perfectly fine. Your hardware is one of (currently) 5 users of the driver (with another one being developed, although I havent heard from the guy recently). I conceeded that expecting the clock API to be reworked wasnt realistic at the time, so I rewrote the clock/power handling for tmio-mmc. I dont intend to modify the clock / power handling further at this point because it works well enough now. Fully dynamic clocking / power control will require some sort of API to manage clocks and power. The dev pm stuff might do for power, but there is nothing suitable for use with clocks yet. Until there is, its going to be messy. Might I suggest that it should be now, now that you can use the in-kernel tmio-mmc driver, that you, I, we, and anyone else interested, should look into actually making the clk API useful? >> Well, if I had done that this time, we'd have a driver with two >> clocking methods in it now... > > I appreciate your work, but seriously, who cares if you have two > clocking methods? If it involves messing up the driver, I do. adding all sorts of conditionals gets hard to maintain very quickly. My time is limited so people who want new features can do the leg-work. > This is just one driver of an entire system. I'm > sure there are more important matters in the world. That zoomed out > perspective aside, I do think your patch removing the second area is > much nicer. So you agree that the code is better for the extra time it took - then there should be no problem. Throwing code into the kernel in a big heap isnt the way to go. its fine for private trees etc. but it rapidly gets out of control if stuff just getsb accepted without being thought out properly. I spent a night removing and coalescing in one place, about 20 cookie-cutter functions from the ASoC code a while back, which, if people had done things properly to begin with, would never have existed in the first place. Its all wasted time. That code should have been spotted in someones personal tree long ago and been fixed before it ever got to mainline. > So again, I really appreciate your help. But from my point of view you > could have just accepted my "make the second io area optional" patch > earlier and done this cleanup at any time later without stress. IOW, you want your code rushed into the kernel and me to 'just fix things later' for you. Since you want the feature in question, why should not _you_ be the one tasked with doing it properly? > That > would have disconnected the two activities and we would have had > SuperH support half a year ago. Just because you gave up for several months after your first attempt doesnt mean the process had to take that long. > I didn't force you to write the code. I came to you with a rather > light weight implementation that you rejected. Yes. Normally when that happens, you go away and rewrite the code, not expect the maintainer to do it for you. > So I do understand the internals of the clock framework, > believe me. When you do things like pass struct clk pointers around in violation of the API, that makes it a bit more tricky to believe. > And making it arch independent is not an easy task. It > involves changing code for a whole bunch of architectures. That's much > much much more complicated than any of this work so far. Perhaps you and I should collaborate on that and actually get the job done, rather tham complain about it? > I _am_ really grateful, believe me. Thank you very much to both you and Philipp. > > But from my point of view I didn't force you to do this work. True. Perhaps I should have kept NAKing until you did it? > You do > seem to think so. My original patch didn't require any MFD driver > changes. I understand and agree that breaking out the code for the > second io window results in cleaner code. I would have been happy to > do that work myself if I felt that the chance for a simple ACK would > have been higher. If you werent sure you could have asked if I'd be ok with it... > My memories is from last time just wild NAKing and > random clock framework rework requirements. I only recall one NAK but its in the past now. > I'm not here to break your driver, you know. And I don't invent > requirements just for fun. I dont doubt that. > I wanted to use the tmio-mmc driver because > I discovered that there is a lot of hardware out there that can make > use of it. The same thing with real requirements goes for Runtime PM - > I'm not pulling these requirements out of the dark - state save and > restore is really needed for proper power managment. Well, its possible it might give a fractional improvement. Prove me wrong with numbers... I think that tmio-mmc should be left alone in favour of improving the clk API at this point, which would mean you could use your struct clk directly in the driver and we could look at putting finer grained PM in the MFD drivers too. >> If the disable hook is not called then that may be a bug. Patches welcome. > > Cool. Where should it be called then? Both in the error case of > probe() and in remove()? I expect so (not looked at the code yet) > I plan on doing nothing at this point, but if > I make a patch I'd like to avoidto break all the other MFD drivers. Its the intended use of the hooks, so feel free to write a patch for it. > Yes, I agree it's available _when_the_module_is_loaded. But that's not > what I want. I'd like to see a more fine grained control. When the > system is idle it should have the option of being powered off. > Transparently. I think it should be found out if its worth it first. Its a lot of code for little gain probably. That said the mmc block is fairly simple, it can be simply reset to restore state. Of course there are other issues like how to suupport SD cards that remain poowered during suspend... > And I suspect that you feel far from motivated by > such a change since it doesn't benefit any of your platforms. Correct, however if you can back your claims with some current measurements with the block powered off / unclocked vs unclocked vs running, and if the gain is worthwhile, I will consider such a patch. > Ouch. Don't get me started on netbooks. =) I like my netbook. I just wishh it was a 4 core ARM rather than X86... -- Ian Molton Linux, Automotive, and other hacking: http://www.mnementh.co.uk/ -- 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
