> -----Original Message----- > From: Nishanth Menon [mailto:nm@xxxxxx] > Sent: Saturday, November 20, 2010 2:45 AM > To: Kevin Hilman > Cc: Santosh Shilimkar; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony > Subject: Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure > RAM > > Kevin Hilman had written, on 11/19/2010 03:06 PM, the following: > > 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 > /me neither :( > > > > >>>> 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 > Keep in mind that the latency is incurred by the default settings in > this series *only* for the very first off mode currently. > > > 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. > There are few other issues with this approach. secure ram save by itself > is just a function. it's trigger should ideally be not just one security > driver IMHO - there is AES, SHAM, and other ones that will need to > implement runtime pm, context save and restore hooks -> E.g. Dimitry's > series[1] is trying to introduce an opensource security driver solution > for OMAP - this is just a start - it will be some time before these > drivers are ready and merged to mainline followed by power management > enablement - do we want to keep omap3 broken while a fix is available > till then? > I read the thread again. Indeed the change log confused me as well. The original patch as such is ok then. I thought you want to prevent MPU PD not to transition and that you are achieving by not allowing MPU clock domain idle. Then your patch seems to be right fix. Regards Santosh -- 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
