Nishanth Menon <nm@xxxxxx> writes: > Santosh Shilimkar had written, on 11/19/2010 11:28 AM, the following: >>> -----Original Message----- >>> From: Nishanth Menon [mailto:nm@xxxxxx] >>> Sent: Friday, November 19, 2010 10:55 PM >>> To: Santosh Shilimkar >>> Cc: Kevin Hilman; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony >>> Subject: Re: [PATCH 08/13] OMAP3: PM: Deny MPU idle while saving secure >>> RAM >>> >>> Santosh Shilimkar had written, on 11/19/2010 11:18 AM, the following: >>> [..] >>>>> I guess we need some more details on which secure mode calls can >>> trigger >>>>> this problem. If this is an isolated case, I'm OK with this fix. If >>>>> it's more general, I'd like to see a more general fix. >>>>> >>>> On the related topic I posted a patch some time back. >>>> http://www.spinics.net/lists/linux-omap/msg37907.html >>>> I guess Kevin is referring to the above patch. >>> I believe the fix we are attempting here is for a specific scenario >>> which IMHO is different from the issue solved in the link above. >> >> It will also solve the above issue indirectly. > just my dumb brains :) agreed, good one that takes care of the power > domain, agreed that pwrdm_set_next_pwrst(which is currently present in > the omap3_save_secure_ram_context) would no longer be necessary Ah, you're right. I hadn't noticed that the current code is already setting he MPU powerdomain to on. > - so [1] should also have removed that - Agreed. 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. > 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. Tero had a series a while back[1] that addressed this in a more general way. IIRC, with his series, he generalized the powerdomain states so you could set a powerdomain to on, denying clkdm idle, or you could set the powerdomain to inactive, allowing clkdm idle. ISTR that I quite liked the inactive support from Tero in that series, but Paul and myself had some issues with how the IDLEST bits were managed in that series, which could probably be addressed with hwmod today. I think it's time to revisit this series from Tero. Kevin [1] http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/msg25268.html -- 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
