Nishanth Menon <nm@xxxxxx> writes: > Kevin Hilman had written, on 11/19/2010 02:39 PM, the following: > [...] >> In addtion, the patch from Santosh needs to better describe what other >> problems it is solving, since it is clearly not fixing this particular >> secure mode entry. Therefore, there must be others that are also doing >> WFI. That being said, instead of such a generic fix as is done by >> Santosh's patch, maybe we need a common secure-mode entry point which >> does the necessary ROM code prep. > Ideally speaking - save_secure_ram can hit latencies which are pretty > bad.. eventually this logic should be moved outside the current > boundaries in some manner - unfortunately, I cant at the moment think > of a sane mechanism to do that given various proprietary and > not-mainlined-but-public security drivers for OMAP3 out there > :(. IMHO, the responsibility of secure storage should be with secure > drivers, but, at the moment touching that topic is opening up a > pandora's box :( Hmm, so the complexity and mess is pushed into the OMAP PM core... /me no likey >> >>> This specific patch controls the clock domain from auto idling around >>> the secure ram save. Apologies on the confusion - but if the [1] patch >>> is fixing it, you can help me understand how it does it. >> >> Now that I understand the clockdomain part, I'm seeing the problem >> differently. (side note: A better written changelog could have avoided >> this confusion by being clear that it was *clockdomain* idle that was >> being added here and that it was in addition to the existing powerdomain >> settings.) >> >> Technically, $SUBJECT patch could have replaced the set_next_pwrst with >> the clkdm_deny_idle. IOW, setting the pwrdm next state to is redundant >> if you clkdm_deny_idle. >> >> I think this is the key to the confusion: >> >> 1) clkdm_deny_idle() implies the powerdomain stays on >> 2) setting powerdomain to on, does NOT imply clkdm_deny_idle() >> >> Another way of saying it is that setting a powerdomain to on does not >> prevent it from going inactive. It only prevents retention or off-mode. > Agreed and I apologize for the confusion caused by the commit message > - > will it be sufficient for the purpose of this series to change the > commit log to better describe the patch? - I will leave the power > domain control to Santosh's /Tero's series instead. > > Is this acceptable option? That is a minimum requirement, but... Based on the rest of this series, I am not at all comfortable with managing this directly in the idle path. The latencies you mentioned above are only part of the reason. I have been trying to remove this kind of device idle/PM management from the core idle path and I am not enthused about adding stuff back. I would much rather see a separate, secure-mode driver, which for starters only manages secure RAM. It doesn't have to manage all of security stuff, but it will make a clearer (and cleaner) separation between the idle path and secure RAM management. If implemented as a driver, it could be much more intelligent about its save/restore and can behave just like any other driver that has to manage context save/restore. If the concern is about trying to have a general purpose "secure driver", then just call it a secure RAM driver or something to be clear it has a small, targetted scope. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
