RE: [PATCH 00/13] OMAP3: OFF mode fixes
(Sorry. You will see two replies from me on your email) -Original Message- From: Santosh Shilimkar [mailto:santosh.shilim...@ti.com] Sent: Wednesday, November 24, 2010 11:04 AM To: 'Kevin Hilman' Cc: Nishanth Menon; 'linux-omap'; 'Jean Pihet'; Vishwanath Sripathy; 'Tony' Subject: RE: [PATCH 00/13] OMAP3: OFF mode fixes -Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Kevin Hilman Sent: Wednesday, November 24, 2010 2:06 AM To: Santosh Shilimkar Cc: Nishanth Menon; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes [...] And we should treat ROM code as a hardware. Secure services don't give you garrulity of saving per each secure module. To get CPU OFF working on secure device, secure RAM must be saved. So I still think it is CPU specific code and pretty much relevant to CPU IDLE OFF state considering ROM code. Ofcourse this not related to GP device because we never enter Secure world again after the boot-up. So moving this code to a separate file is fine but it still related to CPU. Sure, it's still *related* to the CPU, but what I am arguing is that it should not be related to CPU *idle*. My undersanding is this (please correct me): Secure RAM context only needs to be saved/updated when something in it changes changes (e.g. secure driver usage.) Therefore, any driver/device usage that has a side effect of changing secure RAM should be responsible for updating secure RAM. This assumption holds true largely but not completely. There are more Secure APIs which are outside of any secure driver usage which can also alter the state of secure RAM. OMAP4, we have more APIs apart from secure RAM where the secure HW registers, firewalls, cache controllers, interrupt controllers are managed using secure APIs. All of this is must for correct CPU OFF functioning. The approach taken in $SUBJECT series is basically: since we don't know who is using/changing secure RAM, we better save it (or have the option to) during every off-mode transition. This approach is what I do not like. It's pushing work (and intellegence) that should be in the drivers into the PM core where it does not belong. The problem is because the secure RAM is not portioned per device basis but managed as a whole. If we had per secure device portioning then the respective device drivers saving it's context would have worked perfectly. And the fact is it's not the secure device driver context, but it's a Secure software context which runs in parallel with HLOS on HS devices. Rather, I want to follow the same approach we follow for every other device driver. Drivers must assume they can lose context. Therefore it's up to them to save it. IOW, the drivers that *change* secure RAM should be responsible for ensuring that any of the changes they make are saved. As I mentioned above its not just the driver context but the whole secure software context. I will check with ROM team if it can be made more granular for future OMAPs so that we can have the usual strategy of respective components taking care of there save/restore. This will also save huge latency we incure while saving whole RAM on every MPU OFF transition. I had a discussion with ROM team and they conformed that the secure context can get changed with protected applications (PA for FW, secure keys) as well as with secure drivers like crypto, aes etc. This can be centralized and some APIs can be provided by the secure middleware to know whether the context needs to be save or not. Secure middleware manages all the secure driver interfaces as well as PA's. This is the centralized place where all state knowledge is available. This component also can take care of doing a secure context save job based on needs. The only concern from them was that which security middleware to be used is not fixed and its upto customers to decide. Few of them develop this on there own. So with clarity now, I concur with your idea of moving out the secure context save from CPUIDLE code. The only problem I see is how do we support multiple security middleware's with a common infrastructure. Regards Santosh -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] OMAP3: OFF mode fixes
Nishant, On Wed, Nov 24, 2010 at 10:22 AM, Santosh Shilimkar santosh.shilim...@ti.com wrote: (Sorry. You will see two replies from me on your email) -Original Message- From: Santosh Shilimkar [mailto:santosh.shilim...@ti.com] Sent: Wednesday, November 24, 2010 11:04 AM To: 'Kevin Hilman' Cc: Nishanth Menon; 'linux-omap'; 'Jean Pihet'; Vishwanath Sripathy; 'Tony' Subject: RE: [PATCH 00/13] OMAP3: OFF mode fixes -Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Kevin Hilman Sent: Wednesday, November 24, 2010 2:06 AM To: Santosh Shilimkar Cc: Nishanth Menon; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes [...] And we should treat ROM code as a hardware. Secure services don't give you garrulity of saving per each secure module. To get CPU OFF working on secure device, secure RAM must be saved. So I still think it is CPU specific code and pretty much relevant to CPU IDLE OFF state considering ROM code. Ofcourse this not related to GP device because we never enter Secure world again after the boot-up. So moving this code to a separate file is fine but it still related to CPU. Sure, it's still *related* to the CPU, but what I am arguing is that it should not be related to CPU *idle*. My undersanding is this (please correct me): Secure RAM context only needs to be saved/updated when something in it changes changes (e.g. secure driver usage.) Therefore, any driver/device usage that has a side effect of changing secure RAM should be responsible for updating secure RAM. This assumption holds true largely but not completely. There are more Secure APIs which are outside of any secure driver usage which can also alter the state of secure RAM. OMAP4, we have more APIs apart from secure RAM where the secure HW registers, firewalls, cache controllers, interrupt controllers are managed using secure APIs. All of this is must for correct CPU OFF functioning. The approach taken in $SUBJECT series is basically: since we don't know who is using/changing secure RAM, we better save it (or have the option to) during every off-mode transition. This approach is what I do not like. It's pushing work (and intellegence) that should be in the drivers into the PM core where it does not belong. The problem is because the secure RAM is not portioned per device basis but managed as a whole. If we had per secure device portioning then the respective device drivers saving it's context would have worked perfectly. And the fact is it's not the secure device driver context, but it's a Secure software context which runs in parallel with HLOS on HS devices. Rather, I want to follow the same approach we follow for every other device driver. Drivers must assume they can lose context. Therefore it's up to them to save it. IOW, the drivers that *change* secure RAM should be responsible for ensuring that any of the changes they make are saved. As I mentioned above its not just the driver context but the whole secure software context. I will check with ROM team if it can be made more granular for future OMAPs so that we can have the usual strategy of respective components taking care of there save/restore. This will also save huge latency we incure while saving whole RAM on every MPU OFF transition. I had a discussion with ROM team and they conformed that the secure context can get changed with protected applications (PA for FW, secure keys) as well as with secure drivers like crypto, aes etc. This can be centralized and some APIs can be provided by the secure middleware to know whether the context needs to be save or not. Secure middleware manages all the secure driver interfaces as well as PA's. This is the centralized place where all state knowledge is available. This component also can take care of doing a secure context save job based on needs. The only concern from them was that which security middleware to be used is not fixed and its upto customers to decide. Few of them develop this on there own. So with clarity now, I concur with your idea of moving out the secure context save from CPUIDLE code. The only problem I see is how do we support multiple security middleware's with a common infrastructure. When the code is split into the pure sleep code and the secure code parts, I would like to have the sleep code part cleaned up. BTW I rebased my clean-up patch on top of the 13 patches you sent and performed some testing. It is working OK AFAICT (also stands for As far as I can test TM). Can this happen before the .38 merge window? Regards Santosh Thanks, Jean -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] OMAP3: OFF mode fixes
Jean Pihet had written, on 11/24/2010 11:11 AM, the following: [...] When the code is split into the pure sleep code and the secure code parts, I would like to have the sleep code part cleaned up. BTW I rebased my clean-up patch on top of the 13 patches you sent and performed some testing. It is working OK AFAICT (also stands for As far as I can test TM). I could help with testing on EMU zoom3 and SDP3430 If you could host a tree somewhere. Maybe some folks could pitch in for testing with N900 which has HS device as well. Can this happen before the .38 merge window? I plan to work on this set over the next week - hopefully we can do the testing and posting by then. will probably post offline my initial versions for sync up. -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 00/13] OMAP3: OFF mode fixes
-Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Kevin Hilman Sent: Tuesday, November 23, 2010 12:46 AM To: Nishanth Menon Cc: linux-omap; Jean Pihet; Vishwanath Sripathy; Tony Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes Nishanth Menon n...@ti.com writes: Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS devices for OFF mode logic. It is important to note - for proper functionality of HS OFF mode on OMAP3630, CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the correct service that the security PPA supports on the platform. Based on kernel.org 2.6.37-rc2 tag Smoke tested on: SDP3630 -GP Zoom3 (3630): GP EMU (Es1.1, ES1.2) SDP3430 - GP EMU (ES3.1) These are fixes for corner case bugs seen, so tests of off and ret done with wakeup timer - behavior between 2.6.37-rc2 before and after applying patch seen consistent. Request for testing this series for comparison between master and this series requested for additional platforms where available. After some more thought and review, here's what I think should be the approach moving this forward: This can be broken up into 3 independent series as follows: 1) fix for UART erratum (patch 10) 2) fixes for idle path errata (patches 1, 2, 11, 12, 13) 3) secure ram save path (the rest) For (3), I'd like to see the secure ram management moved out of the PM core, and into it's own library/driver. Strictly speaking, context save/restore for secure ram is not a function of the PM idle core. As with every other device, context save/restore is the responsibility of the driver(s) using that device. For secure RAM, the restore is handled by ROM code, but the save should be managed by the secure driver(s). IOW, any secure driver should be using runtime PM and when the secure driver is no longer in use, it should ensure secure RAM context is saved using its runtime_suspend method to save secure RAM. The code in this series should be moved out into a library/driver which can be called by secure drivers in their runtime PM hooks. I agree with you Kevin here except one point. The secure RAM contents are not just secure driver data but the ROM code infrastructure as well. And we should treat ROM code as a hardware. Secure services don't give you garrulity of saving per each secure module. To get CPU OFF working on secure device, secure RAM must be saved. So I still think it is CPU specific code and pretty much relevant to CPU IDLE OFF state considering ROM code. Ofcourse this not related to GP device because we never enter Secure world again after the boot-up. So moving this code to a separate file is fine but it still related to CPU. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] OMAP3: OFF mode fixes
Santosh Shilimkar santosh.shilim...@ti.com writes: -Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Kevin Hilman Sent: Tuesday, November 23, 2010 12:46 AM To: Nishanth Menon Cc: linux-omap; Jean Pihet; Vishwanath Sripathy; Tony Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes Nishanth Menon n...@ti.com writes: Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS devices for OFF mode logic. It is important to note - for proper functionality of HS OFF mode on OMAP3630, CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the correct service that the security PPA supports on the platform. Based on kernel.org 2.6.37-rc2 tag Smoke tested on: SDP3630 -GP Zoom3 (3630): GP EMU (Es1.1, ES1.2) SDP3430 - GP EMU (ES3.1) These are fixes for corner case bugs seen, so tests of off and ret done with wakeup timer - behavior between 2.6.37-rc2 before and after applying patch seen consistent. Request for testing this series for comparison between master and this series requested for additional platforms where available. After some more thought and review, here's what I think should be the approach moving this forward: This can be broken up into 3 independent series as follows: 1) fix for UART erratum (patch 10) 2) fixes for idle path errata (patches 1, 2, 11, 12, 13) 3) secure ram save path (the rest) For (3), I'd like to see the secure ram management moved out of the PM core, and into it's own library/driver. Strictly speaking, context save/restore for secure ram is not a function of the PM idle core. As with every other device, context save/restore is the responsibility of the driver(s) using that device. For secure RAM, the restore is handled by ROM code, but the save should be managed by the secure driver(s). IOW, any secure driver should be using runtime PM and when the secure driver is no longer in use, it should ensure secure RAM context is saved using its runtime_suspend method to save secure RAM. The code in this series should be moved out into a library/driver which can be called by secure drivers in their runtime PM hooks. I agree with you Kevin here except one point. The secure RAM contents are not just secure driver data but the ROM code infrastructure as well. And we should treat ROM code as a hardware. Secure services don't give you garrulity of saving per each secure module. To get CPU OFF working on secure device, secure RAM must be saved. So I still think it is CPU specific code and pretty much relevant to CPU IDLE OFF state considering ROM code. Ofcourse this not related to GP device because we never enter Secure world again after the boot-up. So moving this code to a separate file is fine but it still related to CPU. Sure, it's still *related* to the CPU, but what I am arguing is that it should not be related to CPU *idle*. My undersanding is this (please correct me): Secure RAM context only needs to be saved/updated when something in it changes changes (e.g. secure driver usage.) Therefore, any driver/device usage that has a side effect of changing secure RAM should be responsible for updating secure RAM. The approach taken in $SUBJECT series is basically: since we don't know who is using/changing secure RAM, we better save it (or have the option to) during every off-mode transition. This approach is what I do not like. It's pushing work (and intellegence) that should be in the drivers into the PM core where it does not belong. Rather, I want to follow the same approach we follow for every other device driver. Drivers must assume they can lose context. Therefore it's up to them to save it. IOW, the drivers that *change* secure RAM should be responsible for ensuring that any of the changes they make are saved. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 00/13] OMAP3: OFF mode fixes
-Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Kevin Hilman Sent: Wednesday, November 24, 2010 2:06 AM To: Santosh Shilimkar Cc: Nishanth Menon; linux-omap; Jean Pihet; Vishwanath Sripathy; Tony Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes Santosh Shilimkar santosh.shilim...@ti.com writes: -Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Kevin Hilman Sent: Tuesday, November 23, 2010 12:46 AM To: Nishanth Menon Cc: linux-omap; Jean Pihet; Vishwanath Sripathy; Tony Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes Nishanth Menon n...@ti.com writes: Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS devices for OFF mode logic. It is important to note - for proper functionality of HS OFF mode on OMAP3630, CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the correct service that the security PPA supports on the platform. Based on kernel.org 2.6.37-rc2 tag Smoke tested on: SDP3630 -GP Zoom3 (3630): GP EMU (Es1.1, ES1.2) SDP3430 - GP EMU (ES3.1) These are fixes for corner case bugs seen, so tests of off and ret done with wakeup timer - behavior between 2.6.37-rc2 before and after applying patch seen consistent. Request for testing this series for comparison between master and this series requested for additional platforms where available. After some more thought and review, here's what I think should be the approach moving this forward: This can be broken up into 3 independent series as follows: 1) fix for UART erratum (patch 10) 2) fixes for idle path errata (patches 1, 2, 11, 12, 13) 3) secure ram save path (the rest) For (3), I'd like to see the secure ram management moved out of the PM core, and into it's own library/driver. Strictly speaking, context save/restore for secure ram is not a function of the PM idle core. As with every other device, context save/restore is the responsibility of the driver(s) using that device. For secure RAM, the restore is handled by ROM code, but the save should be managed by the secure driver(s). IOW, any secure driver should be using runtime PM and when the secure driver is no longer in use, it should ensure secure RAM context is saved using its runtime_suspend method to save secure RAM. The code in this series should be moved out into a library/driver which can be called by secure drivers in their runtime PM hooks. I agree with you Kevin here except one point. The secure RAM contents are not just secure driver data but the ROM code infrastructure as well. And we should treat ROM code as a hardware. Secure services don't give you garrulity of saving per each secure module. To get CPU OFF working on secure device, secure RAM must be saved. So I still think it is CPU specific code and pretty much relevant to CPU IDLE OFF state considering ROM code. Ofcourse this not related to GP device because we never enter Secure world again after the boot-up. So moving this code to a separate file is fine but it still related to CPU. Sure, it's still *related* to the CPU, but what I am arguing is that it should not be related to CPU *idle*. My undersanding is this (please correct me): Secure RAM context only needs to be saved/updated when something in it changes changes (e.g. secure driver usage.) Therefore, any driver/device usage that has a side effect of changing secure RAM should be responsible for updating secure RAM. This assumption holds true largely but not completely. There are more Secure APIs which are outside of any secure driver usage which can also alter the state of secure RAM. OMAP4, we have more APIs apart from secure RAM where the secure HW registers, firewalls, cache controllers, interrupt controllers are managed using secure APIs. All of this is must for correct CPU OFF functioning. The approach taken in $SUBJECT series is basically: since we don't know who is using/changing secure RAM, we better save it (or have the option to) during every off-mode transition. This approach is what I do not like. It's pushing work (and intellegence) that should be in the drivers into the PM core where it does not belong. The problem is because the secure RAM is not portioned per device basis but managed as a whole. If we had per secure device portioning then the respective device drivers saving it's context would have worked perfectly. And the fact is it's not the secure device driver context, but it's a Secure software context which runs in parallel with HLOS on HS devices. Rather, I want to follow the same approach we follow for every other device driver. Drivers must assume they can lose context. Therefore it's up to them to save it. IOW, the drivers
Re: [PATCH 00/13] OMAP3: OFF mode fixes
Nishanth Menon n...@ti.com writes: Kevin Hilman had written, on 11/19/2010 03:20 PM, the following: Request for testing this series for comparison between master and this series requested for additional platforms where available. I haven't yet been through the entire series, but some general comments to share before the weekend: thanks for comments so far. I will wait for the complete series to be reviewed before reposting a v2. The secure mode code is growing in size and complexity, so I think it should be removed from pm34xx.c and moved into it's own file (secure3xxx.c ?) This will help keep pm34xx.c lean, and it can call into secure code as needed. IMHO - we should do that set of cleanups as part of Jean's patch series where we transition to sdram where possible - that will allow us to have C code replacement for sleep34xx.S and optimize better in conjunction with sram_idle function and helpers. No, I'd like to see the secure code in your patch all in a separate file. Jean's cleanups are independent of better organization and structure of your code. Even better (and already suggested in some comments on patch 8), now that there are board-configurable knobs, this should be set up as a very simple platform driver/device so boards can pass configuration in a standard way instead of having new APIs for use by board code to set configuration settings. in this specific context, having a platform data device is more of an overkill - 90% of the HS platforms (just a guess based on the limited devices I know of and is not a hard statistics) use the TI defaults - we do have exceptional customers who do their own PPA - and having a device-driver model IMHO is an overkill for that flexibility. The board-level configuration is only one (minor) reason to have a separate driver for the secure stuff. Discussions on the other patches suggested other reasons for this as well, including some reasons from you as well. :) Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] OMAP3: OFF mode fixes
Nishanth Menon n...@ti.com writes: Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS devices for OFF mode logic. It is important to note - for proper functionality of HS OFF mode on OMAP3630, CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the correct service that the security PPA supports on the platform. Based on kernel.org 2.6.37-rc2 tag Smoke tested on: SDP3630 -GP Zoom3 (3630): GP EMU (Es1.1, ES1.2) SDP3430 - GP EMU (ES3.1) These are fixes for corner case bugs seen, so tests of off and ret done with wakeup timer - behavior between 2.6.37-rc2 before and after applying patch seen consistent. Request for testing this series for comparison between master and this series requested for additional platforms where available. After some more thought and review, here's what I think should be the approach moving this forward: This can be broken up into 3 independent series as follows: 1) fix for UART erratum (patch 10) 2) fixes for idle path errata (patches 1, 2, 11, 12, 13) 3) secure ram save path (the rest) For (3), I'd like to see the secure ram management moved out of the PM core, and into it's own library/driver. Strictly speaking, context save/restore for secure ram is not a function of the PM idle core. As with every other device, context save/restore is the responsibility of the driver(s) using that device. For secure RAM, the restore is handled by ROM code, but the save should be managed by the secure driver(s). IOW, any secure driver should be using runtime PM and when the secure driver is no longer in use, it should ensure secure RAM context is saved using its runtime_suspend method to save secure RAM. The code in this series should be moved out into a library/driver which can be called by secure drivers in their runtime PM hooks. Stated otherwise, the PM core is doing the job that should be done by secure driver(s). Rather than continuing to extend that hack, it's time for that to be fixed correctly in a way that can be maintainted. Kevin Archana Sriram (1): OMAP3: PM: Errata i582: per domain reset issue: uart Eduardo Valentin (3): OMAP3: PM: Deny MPU idle while saving secure RAM OMAP3: PM: Apply errata i540 before save secure ram OMAP3630: PM: Errata i583: disable coreoff if ES1.2 Nishanth Menon (3): OMAP3: PM: make secure ram save size configurable OMAP3: PM: Fix secure save size for OMAP3 OMAP3630: PM: Errata i608: disable RTA Peter 'p2' De Schrijver (2): OMAP3: PM: Errata i581 suppport: dll kick strategy OMAP3630: PM: Disable L2 cache while invalidating L2 cache Richard Woodruff (1): OMAP3: PM: Update clean_l2 to use v7_flush_dcache_all Tero Kristo (3): OMAP3: PM: Save secure RAM context before entering WFI OMAP3: PM: optional save secure RAM context every core off cycle OMAP3: PM: allocate secure RAM context memory from low-mem arch/arm/mach-omap2/control.c|5 +- arch/arm/mach-omap2/control.h|5 + arch/arm/mach-omap2/io.c | 11 ++ arch/arm/mach-omap2/pm.h | 40 +++ arch/arm/mach-omap2/pm34xx.c | 184 - arch/arm/mach-omap2/serial.c | 80 + arch/arm/mach-omap2/sleep34xx.S | 187 ++--- arch/arm/plat-omap/include/plat/serial.h |4 + 8 files changed, 412 insertions(+), 104 deletions(-) Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 00/13] OMAP3: OFF mode fixes
-Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Nishanth Menon Sent: Saturday, November 20, 2010 3:07 AM To: Kevin Hilman Cc: linux-omap; Jean Pihet; Vishwanath Sripathy; Tony Subject: Re: [PATCH 00/13] OMAP3: OFF mode fixes Kevin Hilman had written, on 11/19/2010 03:20 PM, the following: Request for testing this series for comparison between master and this series requested for additional platforms where available. I haven't yet been through the entire series, but some general comments to share before the weekend: thanks for comments so far. I will wait for the complete series to be reviewed before reposting a v2. The secure mode code is growing in size and complexity, so I think it should be removed from pm34xx.c and moved into it's own file (secure3xxx.c ?) This will help keep pm34xx.c lean, and it can call into secure code as needed. We already have omap44xx-smc.S. I could make this generic. One problem with The secure APIs, they are consistent in all OMAP version. For example Secure SAR save has a different API and signature on OMAP3 and OMAP4. But this is something doable. Regards, Santosh -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] OMAP3: OFF mode fixes
Hi, On Fri, Nov 19, 2010 at 2:54 AM, Nishanth Menon n...@ti.com wrote: Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS devices for OFF mode logic. It is important to note - for proper functionality of HS OFF mode on OMAP3630, CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the correct service that the security PPA supports on the platform. Based on kernel.org 2.6.37-rc2 tag Smoke tested on: SDP3630 -GP Zoom3 (3630): GP EMU (Es1.1, ES1.2) SDP3430 - GP EMU (ES3.1) These are fixes for corner case bugs seen, so tests of off and ret done with wakeup timer - behavior between 2.6.37-rc2 before and after applying patch seen consistent. Request for testing this series for comparison between master and this series requested for additional platforms where available. Archana Sriram (1): OMAP3: PM: Errata i582: per domain reset issue: uart Eduardo Valentin (3): OMAP3: PM: Deny MPU idle while saving secure RAM OMAP3: PM: Apply errata i540 before save secure ram OMAP3630: PM: Errata i583: disable coreoff if ES1.2 Nishanth Menon (3): OMAP3: PM: make secure ram save size configurable OMAP3: PM: Fix secure save size for OMAP3 OMAP3630: PM: Errata i608: disable RTA Peter 'p2' De Schrijver (2): OMAP3: PM: Errata i581 suppport: dll kick strategy OMAP3630: PM: Disable L2 cache while invalidating L2 cache Richard Woodruff (1): OMAP3: PM: Update clean_l2 to use v7_flush_dcache_all Tero Kristo (3): OMAP3: PM: Save secure RAM context before entering WFI OMAP3: PM: optional save secure RAM context every core off cycle OMAP3: PM: allocate secure RAM context memory from low-mem arch/arm/mach-omap2/control.c | 5 +- arch/arm/mach-omap2/control.h | 5 + arch/arm/mach-omap2/io.c | 11 ++ arch/arm/mach-omap2/pm.h | 40 +++ arch/arm/mach-omap2/pm34xx.c | 184 - arch/arm/mach-omap2/serial.c | 80 + arch/arm/mach-omap2/sleep34xx.S | 187 ++--- arch/arm/plat-omap/include/plat/serial.h | 4 + 8 files changed, 412 insertions(+), 104 deletions(-) Regards, Nishanth Menon This is a nice set of changes, thanks! I sent my remarks. After having reviewed the patches I think the best is to merge those in, then apply the clean-up and possibly the move to DDR patches on top of that. What do you think? Regards, Jean -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] OMAP3: OFF mode fixes
Jean Pihet wrote, on 11/19/2010 04:18 AM: Hi, On Fri, Nov 19, 2010 at 2:54 AM, Nishanth Menonn...@ti.com wrote: Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS devices for OFF mode logic. It is important to note - for proper functionality of HS OFF mode on OMAP3630, CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the correct service that the security PPA supports on the platform. Based on kernel.org 2.6.37-rc2 tag Smoke tested on: SDP3630 -GP Zoom3 (3630): GP EMU (Es1.1, ES1.2) SDP3430 - GP EMU (ES3.1) These are fixes for corner case bugs seen, so tests of off and ret done with wakeup timer - behavior between 2.6.37-rc2 before and after applying patch seen consistent. Request for testing this series for comparison between master and this series requested for additional platforms where available. Archana Sriram (1): OMAP3: PM: Errata i582: per domain reset issue: uart Eduardo Valentin (3): OMAP3: PM: Deny MPU idle while saving secure RAM OMAP3: PM: Apply errata i540 before save secure ram OMAP3630: PM: Errata i583: disable coreoff if ES1.2 Nishanth Menon (3): OMAP3: PM: make secure ram save size configurable OMAP3: PM: Fix secure save size for OMAP3 OMAP3630: PM: Errata i608: disable RTA Peter 'p2' De Schrijver (2): OMAP3: PM: Errata i581 suppport: dll kick strategy OMAP3630: PM: Disable L2 cache while invalidating L2 cache Richard Woodruff (1): OMAP3: PM: Update clean_l2 to use v7_flush_dcache_all Tero Kristo (3): OMAP3: PM: Save secure RAM context before entering WFI OMAP3: PM: optional save secure RAM context every core off cycle OMAP3: PM: allocate secure RAM context memory from low-mem arch/arm/mach-omap2/control.c|5 +- arch/arm/mach-omap2/control.h|5 + arch/arm/mach-omap2/io.c | 11 ++ arch/arm/mach-omap2/pm.h | 40 +++ arch/arm/mach-omap2/pm34xx.c | 184 - arch/arm/mach-omap2/serial.c | 80 + arch/arm/mach-omap2/sleep34xx.S | 187 ++--- arch/arm/plat-omap/include/plat/serial.h |4 + 8 files changed, 412 insertions(+), 104 deletions(-) Regards, Nishanth Menon This is a nice set of changes, thanks! I sent my remarks. After having reviewed the patches I think the best is to merge those in, then apply the clean-up and possibly the move to DDR patches on top of that. What do you think? I have the same feel as well.. thanks for your review. I suggest we take some more test feedback as well to ensure that there are no more mess ups I might have done as the paths are critical. -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] OMAP3: OFF mode fixes
Nishanth Menon n...@ti.com writes: Bunch of fixes as part of phase 1 targetting mainly OMAP3630 HS devices for OFF mode logic. It is important to note - for proper functionality of HS OFF mode on OMAP3630, CONFIG_OMAP3_L2_AUX_SECURE_SAVE_RESTORE=y and CONFIG_OMAP3_L2_AUX_SECURE_SERVICE_SET_ID should be set to the correct service that the security PPA supports on the platform. Based on kernel.org 2.6.37-rc2 tag Smoke tested on: SDP3630 -GP Zoom3 (3630): GP EMU (Es1.1, ES1.2) SDP3430 - GP EMU (ES3.1) These are fixes for corner case bugs seen, so tests of off and ret done with wakeup timer - behavior between 2.6.37-rc2 before and after applying patch seen consistent. Request for testing this series for comparison between master and this series requested for additional platforms where available. I haven't yet been through the entire series, but some general comments to share before the weekend: The secure mode code is growing in size and complexity, so I think it should be removed from pm34xx.c and moved into it's own file (secure3xxx.c ?) This will help keep pm34xx.c lean, and it can call into secure code as needed. Even better (and already suggested in some comments on patch 8), now that there are board-configurable knobs, this should be set up as a very simple platform driver/device so boards can pass configuration in a standard way instead of having new APIs for use by board code to set configuration settings. Kevin Archana Sriram (1): OMAP3: PM: Errata i582: per domain reset issue: uart Eduardo Valentin (3): OMAP3: PM: Deny MPU idle while saving secure RAM OMAP3: PM: Apply errata i540 before save secure ram OMAP3630: PM: Errata i583: disable coreoff if ES1.2 Nishanth Menon (3): OMAP3: PM: make secure ram save size configurable OMAP3: PM: Fix secure save size for OMAP3 OMAP3630: PM: Errata i608: disable RTA Peter 'p2' De Schrijver (2): OMAP3: PM: Errata i581 suppport: dll kick strategy OMAP3630: PM: Disable L2 cache while invalidating L2 cache Richard Woodruff (1): OMAP3: PM: Update clean_l2 to use v7_flush_dcache_all Tero Kristo (3): OMAP3: PM: Save secure RAM context before entering WFI OMAP3: PM: optional save secure RAM context every core off cycle OMAP3: PM: allocate secure RAM context memory from low-mem arch/arm/mach-omap2/control.c|5 +- arch/arm/mach-omap2/control.h|5 + arch/arm/mach-omap2/io.c | 11 ++ arch/arm/mach-omap2/pm.h | 40 +++ arch/arm/mach-omap2/pm34xx.c | 184 - arch/arm/mach-omap2/serial.c | 80 + arch/arm/mach-omap2/sleep34xx.S | 187 ++--- arch/arm/plat-omap/include/plat/serial.h |4 + 8 files changed, 412 insertions(+), 104 deletions(-) Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] OMAP3: OFF mode fixes
Kevin Hilman had written, on 11/19/2010 03:20 PM, the following: Request for testing this series for comparison between master and this series requested for additional platforms where available. I haven't yet been through the entire series, but some general comments to share before the weekend: thanks for comments so far. I will wait for the complete series to be reviewed before reposting a v2. The secure mode code is growing in size and complexity, so I think it should be removed from pm34xx.c and moved into it's own file (secure3xxx.c ?) This will help keep pm34xx.c lean, and it can call into secure code as needed. IMHO - we should do that set of cleanups as part of Jean's patch series where we transition to sdram where possible - that will allow us to have C code replacement for sleep34xx.S and optimize better in conjunction with sram_idle function and helpers. Even better (and already suggested in some comments on patch 8), now that there are board-configurable knobs, this should be set up as a very simple platform driver/device so boards can pass configuration in a standard way instead of having new APIs for use by board code to set configuration settings. in this specific context, having a platform data device is more of an overkill - 90% of the HS platforms (just a guess based on the limited devices I know of and is not a hard statistics) use the TI defaults - we do have exceptional customers who do their own PPA - and having a device-driver model IMHO is an overkill for that flexibility. -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
