Re: [PATCH v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
Ohad, On 09/17/2014 08:37 AM, Ohad Ben-Cohen wrote: Hi Suman, On Tue, Sep 16, 2014 at 7:14 PM, Suman Anna s-a...@ti.com wrote: The current remoteproc infrastructure automatically calls rproc_boot only as part of the rpmsg/virtio stack (in remoteproc_virtio.c), but since the WkupM3 does not use rpmsg, there is no automatic booting of the WkupM3 processor. This is the reason why rproc_boot is called as part of the WkupM3 driver probe sequence. What are your concerns here, and if you think this is not the right place do invoke rproc_boot, where do you expect it to be called? The remoteproc layer is meant to hide hardware-specific details, and allow high-level hardware-agnostic code to boot a remote processor, in order to achieve some task, without even caring what kind of hardware it is booting. So generally we have some consumer driver asking remoteproc to boot a remote processor, and in turn, remoteproc asking a hardware-specific vendor driver to take care of the hardware details like actually taking the remote processor out of reset. The consumer driver is some code that deals with some hardware agnostic task. Today the only consumer we have is rpmsg, so it's perfectly reasonable if remoteproc needs to be adapted a bit to accommodate other consumers as they show up. Can you think of any component in your code that is not necessarily hardware specific, and that one day might be useful for other vendors? Can you describe the task you're trying to achieve, the entities involved and the flow between them? Also do note that, there is no way at present to retrieve the struct rproc for a given remote processor, to be able to invoke the rproc_boot from a different layer. It's perfectly ok to make struct rproc public if we have a consumer that requires it. Splitting this would still require some kind of notifier from remoteproc for the other layers for them to know that the WkupM3 remote processor has been loaded and booted successfully. This is also done as part of the WkupM3 driver at the moment. Are there entities, other than the one that calls rproc_boot, that needs to know that the WkupM3 is up? if so, let's discuss who should notify them - remoteproc or the actual invoker of rproc_boot. It probably depends on who those entities are and what's their relation, if any, to the WkupM3 driver. There are three layers at play here. The pm layer, the ipc layer, and the rproc layer. As we know, currently the problem is that the ipc and rproc layer are combined. PM on am33xx is ENTIRELY dependent on the wkup_m3. It can't enable any PM OPs until the FW is ready. So that is one place where the PM layer must be notified. The other instance is for IPC, the wkup_m3 triggers an interrupt back to the MPU when it it is done with it's work after an IPC event (explained more later), so the PM code must be notified of this as well. Here is the exact flow between PM, IPC, and wkup_m3 during a suspend cycle: Boot: *Firmware gets loaded and wkup_m3 has hard reset deasserted and starts executing (Definite wkup_m3 rproc responsibility). *PM code is notified that wkup_m3 is awake (currently by wkup_m3_rproc start hook) and calls into pm code with rproc_ready hook and uses IPC registers (which were filled by wkup_m3) to check version number and make sure it's compatible. This sounds like IPC layer responsibility, but does the IPC layer just handle reading back the registers and give these values for the PM layer to interpret? Or does it do the interpreting and gives back a specific PM value? (In this case fw version.) Suspend: *PM code tells wkup_m3_rproc driver to place the command for the desired PM state in the IPC registers along with any other info needed for suspend (determined by PM code) and then ping the wkup_m3 using the mailbox, which is just a dummy write, the mailbox is not actually used just the interrupt. Once this happens the wkup_m3 itself runs, prepares for the desired PM state, and triggers it's own wkup_m3 interrupt, unrelated to the mailbox interrupt. *Once this interrupt is sent the wkup_m3_rproc has the irq handler for the interrupt and calls into pm code using txev_handler hook I defined, and with this, the PM code proceeds, and the wkup_m3 just waits for MPU clock gate (unrelated to IPC or wkup_m3, triggered by other SoC functionality). *On resume, no interrupt is generated from wkup_m3 but the PM code, in the standard resume path, reads from the IPC registers to check a status value (without receiving an interrupt like before, just assumes there is data because a resume is happening, interrupts are disabled still at this point) and then the PM code interprets the status value and a wake source value and does whatever it wants with it. This also sounds like IPC layer responsibility but again the question comes up do we provide generic IPC reg reading functionality or let the IPC layer do more, like with the version number read? I am not sure exactly
Re: [PATCH v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
Hi Suman, On Tue, Sep 16, 2014 at 7:14 PM, Suman Anna s-a...@ti.com wrote: The current remoteproc infrastructure automatically calls rproc_boot only as part of the rpmsg/virtio stack (in remoteproc_virtio.c), but since the WkupM3 does not use rpmsg, there is no automatic booting of the WkupM3 processor. This is the reason why rproc_boot is called as part of the WkupM3 driver probe sequence. What are your concerns here, and if you think this is not the right place do invoke rproc_boot, where do you expect it to be called? The remoteproc layer is meant to hide hardware-specific details, and allow high-level hardware-agnostic code to boot a remote processor, in order to achieve some task, without even caring what kind of hardware it is booting. So generally we have some consumer driver asking remoteproc to boot a remote processor, and in turn, remoteproc asking a hardware-specific vendor driver to take care of the hardware details like actually taking the remote processor out of reset. The consumer driver is some code that deals with some hardware agnostic task. Today the only consumer we have is rpmsg, so it's perfectly reasonable if remoteproc needs to be adapted a bit to accommodate other consumers as they show up. Can you think of any component in your code that is not necessarily hardware specific, and that one day might be useful for other vendors? Can you describe the task you're trying to achieve, the entities involved and the flow between them? Also do note that, there is no way at present to retrieve the struct rproc for a given remote processor, to be able to invoke the rproc_boot from a different layer. It's perfectly ok to make struct rproc public if we have a consumer that requires it. Splitting this would still require some kind of notifier from remoteproc for the other layers for them to know that the WkupM3 remote processor has been loaded and booted successfully. This is also done as part of the WkupM3 driver at the moment. Are there entities, other than the one that calls rproc_boot, that needs to know that the WkupM3 is up? if so, let's discuss who should notify them - remoteproc or the actual invoker of rproc_boot. It probably depends on who those entities are and what's their relation, if any, to the WkupM3 driver. Thanks, Ohad. -- 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 v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
On Wed, Sep 10, 2014 at 12:10 AM, Kevin Hilman khil...@kernel.org wrote: What I think you need to do (and what I've recommended at least once in earlier reviews) put all the (non-rproc) wkup_m3 IPC into into one driver and create a well-described, well-documented API that the platform PM code will use. IMO, the current split is very difficult to read/understand, which means it will even more difficult to maintain. I strongly agree. A remoteproc driver should generally only register its hardware-specific implementation of the rproc_ops via the remoteproc framework. It is not expected to expose public API of its own - that's why we have the generic remoteproc layer for. It makes perfect sense not to use rpmsg for communications if it's not lightweight enough, but exposing a new set of IPC API should take place in a separate well-documented driver. In addition, the suggested remoteproc driver seems to act both as a low-level hardware-specific driver that plugs into remoteproc (by registering rproc_ops via the remoteproc framework), and also as a high-level driver that utilizes the remoteproc public API (by calling rproc_boot). This alone calls for scrutinizing the design as this is not very intuitive. At least for the remoteproc part: if you could provide a simple and straight-forward remoteproc driver that just implements the rproc_ops, this could be merged very quickly. The rest of the code most probably belongs in a different entity, just like Kevin suggested. Thanks, Ohad. -- 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 v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
Hi Ohad, On 09/16/2014 10:08 AM, Ohad Ben-Cohen wrote: On Wed, Sep 10, 2014 at 12:10 AM, Kevin Hilman khil...@kernel.org wrote: What I think you need to do (and what I've recommended at least once in earlier reviews) put all the (non-rproc) wkup_m3 IPC into into one driver and create a well-described, well-documented API that the platform PM code will use. IMO, the current split is very difficult to read/understand, which means it will even more difficult to maintain. I strongly agree. A remoteproc driver should generally only register its hardware-specific implementation of the rproc_ops via the remoteproc framework. It is not expected to expose public API of its own - that's why we have the generic remoteproc layer for. It makes perfect sense not to use rpmsg for communications if it's not lightweight enough, but exposing a new set of IPC API should take place in a separate well-documented driver. In addition, the suggested remoteproc driver seems to act both as a low-level hardware-specific driver that plugs into remoteproc (by registering rproc_ops via the remoteproc framework), and also as a high-level driver that utilizes the remoteproc public API (by calling rproc_boot). This alone calls for scrutinizing the design as this is not very intuitive. The current remoteproc infrastructure automatically calls rproc_boot only as part of the rpmsg/virtio stack (in remoteproc_virtio.c), but since the WkupM3 does not use rpmsg, there is no automatic booting of the WkupM3 processor. This is the reason why rproc_boot is called as part of the WkupM3 driver probe sequence. What are your concerns here, and if you think this is not the right place do invoke rproc_boot, where do you expect it to be called? Also do note that, there is no way at present to retrieve the struct rproc for a given remote processor, to be able to invoke the rproc_boot from a different layer. At least for the remoteproc part: if you could provide a simple and straight-forward remoteproc driver that just implements the rproc_ops, this could be merged very quickly. The rest of the code most probably belongs in a different entity, just like Kevin suggested. Splitting this would still require some kind of notifier from remoteproc for the other layers for them to know that the WkupM3 remote processor has been loaded and booted successfully. This is also done as part of the WkupM3 driver at the moment. regards Suman -- 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 v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
Kevin, On 09/09/2014 04:10 PM, Kevin Hilman wrote: Dave Gerlach d-gerl...@ti.com writes: Kevin/Ohad, On 09/09/2014 02:59 PM, Suman Anna wrote: Hi Ohad, On 09/09/2014 05:31 AM, Ohad Ben-Cohen wrote: On Tue, Sep 9, 2014 at 1:30 AM, Kevin Hilman khil...@linaro.org wrote: To me, it's not terribly clear how you made the split between this PM core code an the remoteproc code. In the changelog for the remoteproc patch, it states it's to load the firmware for and boot the wkup_m3. But, while parts of the IPC are here in pm33xx.c, parts of the IPC are also inside the remoteproc driver, so I'm quite curious if that's OK with the remoteproc maintainers. Either way, please make it clearer how and why you made the split, and please isolate the wkup_m3 IPC/protocol from this code. Think of people wanting to rework/extend the wkup_m3 firmware. They shouldn't be messing around in here, but rather inside a driver specificaly for the wkup_m3. I haven't looked at the code very thoroughly yet, but generally a remoteproc driver should only implement the three start/stop/kick rproc_ops, and then register them via the remoteproc framework. Exposing additional API directly from that driver isn't something we immediately want to accept. If relevant, we would generally prefer to extend remoteproc instead, so other platform-specific drivers could utilize that functionality as well. Or rpmsg - if we're missing some IPC functionality. The WkupM3 cannot access DDR, and so we don't intend to use rpmsg. The IPC with wkup_m3 is usually one of the last steps for putting the SoC into a desired low-power state either during suspend or cpuidle, and the communication uses a bank of fixed registers. The .kick is specific to virtio-based communication, and so this is not gonna be used. If you can take a closer look at the wkup_m3 remoteproc driver and give your comments, then we can plan on the next steps. Especially as there are also pieces pertaining to the PM layer knowing the WkupM3 has been loaded and booted. There are already some pending comments on code fragments from Santosh and myself, but let us know your inputs on the integration aspects on PM, remoteproc and IPC with WkupM3. The split was defined by putting all the application specific (to the firmware in use) code in the platform pm code while trying to keep all the IPC code within the wkup_m3_rproc driver. I don't even see that split. I see the platform PM code directly setting IPC register values, but then rproc driver actually sends the mailbox command. Well, really the pm code is setting a structure which gets passed to the wkup_m3_rproc API and that's what does the write. I suppose the naming of the structure is misleading though. However, the wkup_m3 driver isn't aware of the protocol or what is going into these registers for the writes, the PM code defines the usage. The exposed API is definitely heavily biased towards the intended use-case, Maybe if the API was actually documented, it would be easier for us to review it. but the CM3 was designed with this exact purpose in mind and not much else, and due to the limited IPC registers we have to work with there isn't a whole lot of flexibility. Only IPC reg 0 is always used as the resume address, the usage of the other registers is defined by the firmware and pm code. Just as a refresher for those not familiar with it, the IPC mechanism works like this: we load the ipc registers (8 for am33xx, 16 for am43xx) with any information we want to communicate to the CM3, OK, and this happens currently in the platform PM code, right? then we make a dummy write to the Mailbox which triggers an interrupt on the CM3, the CM3 does what it needs to with the info passed in the IPC regs and writes anything it wants to communicate back to these registers, and then triggers a different interrupt (not related to mailbox) to let the MPU know it is done. And this part happens in the rproc driver, right? It's kind of a mess so I figured one driver was the best way to encapsulate it all, So where is this one driver that encapsulates it all? Sp my thinking was that I put the IPC writing in the wkup_m3_rproc driver, but the actual configuration of what gets written in the PM platform code, to at least try to keep things generic. Still, I do agree now that the split is not that clear. and I still had to introduce callbacks within the wkup_m3_rproc driver so it could let the pm code know when the FW loaded (to actually enable pm) and when an interrupt was received from the wkup_m3 (so the pm code can process the response). As Suman stated, this sequence is part of the suspend path and also will be part of the lower c-states for cpuidle, so we need something fast and lightweight. RPMsg is way more than we need and it doesn't really fit the use case, so I'm not sure what makes the most sense, extending remoteproc in some way to
Re: [PATCH v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
On Tue, Sep 9, 2014 at 1:30 AM, Kevin Hilman khil...@linaro.org wrote: To me, it's not terribly clear how you made the split between this PM core code an the remoteproc code. In the changelog for the remoteproc patch, it states it's to load the firmware for and boot the wkup_m3. But, while parts of the IPC are here in pm33xx.c, parts of the IPC are also inside the remoteproc driver, so I'm quite curious if that's OK with the remoteproc maintainers. Either way, please make it clearer how and why you made the split, and please isolate the wkup_m3 IPC/protocol from this code. Think of people wanting to rework/extend the wkup_m3 firmware. They shouldn't be messing around in here, but rather inside a driver specificaly for the wkup_m3. I haven't looked at the code very thoroughly yet, but generally a remoteproc driver should only implement the three start/stop/kick rproc_ops, and then register them via the remoteproc framework. Exposing additional API directly from that driver isn't something we immediately want to accept. If relevant, we would generally prefer to extend remoteproc instead, so other platform-specific drivers could utilize that functionality as well. Or rpmsg - if we're missing some IPC functionality. Thanks, Ohad. -- 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 v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
Hi Ohad, On 09/09/2014 05:31 AM, Ohad Ben-Cohen wrote: On Tue, Sep 9, 2014 at 1:30 AM, Kevin Hilman khil...@linaro.org wrote: To me, it's not terribly clear how you made the split between this PM core code an the remoteproc code. In the changelog for the remoteproc patch, it states it's to load the firmware for and boot the wkup_m3. But, while parts of the IPC are here in pm33xx.c, parts of the IPC are also inside the remoteproc driver, so I'm quite curious if that's OK with the remoteproc maintainers. Either way, please make it clearer how and why you made the split, and please isolate the wkup_m3 IPC/protocol from this code. Think of people wanting to rework/extend the wkup_m3 firmware. They shouldn't be messing around in here, but rather inside a driver specificaly for the wkup_m3. I haven't looked at the code very thoroughly yet, but generally a remoteproc driver should only implement the three start/stop/kick rproc_ops, and then register them via the remoteproc framework. Exposing additional API directly from that driver isn't something we immediately want to accept. If relevant, we would generally prefer to extend remoteproc instead, so other platform-specific drivers could utilize that functionality as well. Or rpmsg - if we're missing some IPC functionality. The WkupM3 cannot access DDR, and so we don't intend to use rpmsg. The IPC with wkup_m3 is usually one of the last steps for putting the SoC into a desired low-power state either during suspend or cpuidle, and the communication uses a bank of fixed registers. The .kick is specific to virtio-based communication, and so this is not gonna be used. If you can take a closer look at the wkup_m3 remoteproc driver and give your comments, then we can plan on the next steps. Especially as there are also pieces pertaining to the PM layer knowing the WkupM3 has been loaded and booted. There are already some pending comments on code fragments from Santosh and myself, but let us know your inputs on the integration aspects on PM, remoteproc and IPC with WkupM3. regards Suman -- 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 v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
Kevin/Ohad, On 09/09/2014 02:59 PM, Suman Anna wrote: Hi Ohad, On 09/09/2014 05:31 AM, Ohad Ben-Cohen wrote: On Tue, Sep 9, 2014 at 1:30 AM, Kevin Hilman khil...@linaro.org wrote: To me, it's not terribly clear how you made the split between this PM core code an the remoteproc code. In the changelog for the remoteproc patch, it states it's to load the firmware for and boot the wkup_m3. But, while parts of the IPC are here in pm33xx.c, parts of the IPC are also inside the remoteproc driver, so I'm quite curious if that's OK with the remoteproc maintainers. Either way, please make it clearer how and why you made the split, and please isolate the wkup_m3 IPC/protocol from this code. Think of people wanting to rework/extend the wkup_m3 firmware. They shouldn't be messing around in here, but rather inside a driver specificaly for the wkup_m3. I haven't looked at the code very thoroughly yet, but generally a remoteproc driver should only implement the three start/stop/kick rproc_ops, and then register them via the remoteproc framework. Exposing additional API directly from that driver isn't something we immediately want to accept. If relevant, we would generally prefer to extend remoteproc instead, so other platform-specific drivers could utilize that functionality as well. Or rpmsg - if we're missing some IPC functionality. The WkupM3 cannot access DDR, and so we don't intend to use rpmsg. The IPC with wkup_m3 is usually one of the last steps for putting the SoC into a desired low-power state either during suspend or cpuidle, and the communication uses a bank of fixed registers. The .kick is specific to virtio-based communication, and so this is not gonna be used. If you can take a closer look at the wkup_m3 remoteproc driver and give your comments, then we can plan on the next steps. Especially as there are also pieces pertaining to the PM layer knowing the WkupM3 has been loaded and booted. There are already some pending comments on code fragments from Santosh and myself, but let us know your inputs on the integration aspects on PM, remoteproc and IPC with WkupM3. The split was defined by putting all the application specific (to the firmware in use) code in the platform pm code while trying to keep all the IPC code within the wkup_m3_rproc driver. The exposed API is definitely heavily biased towards the intended use-case, but the CM3 was designed with this exact purpose in mind and not much else, and due to the limited IPC registers we have to work with there isn't a whole lot of flexibility. Only IPC reg 0 is always used as the resume address, the usage of the other registers is defined by the firmware and pm code. Just as a refresher for those not familiar with it, the IPC mechanism works like this: we load the ipc registers (8 for am33xx, 16 for am43xx) with any information we want to communicate to the CM3, then we make a dummy write to the Mailbox which triggers an interrupt on the CM3, the CM3 does what it needs to with the info passed in the IPC regs and writes anything it wants to communicate back to these registers, and then triggers a different interrupt (not related to mailbox) to let the MPU know it is done. It's kind of a mess so I figured one driver was the best way to encapsulate it all, and I still had to introduce callbacks within the wkup_m3_rproc driver so it could let the pm code know when the FW loaded (to actually enable pm) and when an interrupt was received from the wkup_m3 (so the pm code can process the response). As Suman stated, this sequence is part of the suspend path and also will be part of the lower c-states for cpuidle, so we need something fast and lightweight. RPMsg is way more than we need and it doesn't really fit the use case, so I'm not sure what makes the most sense, extending remoteproc in some way to support IPC communication like described above or leaving the basic FW loading functionality in place in remoteproc but moving the IPC and wkup_m3 functionality back into the platform pm33xx code as it's so specific to that use-case anyway. Regards, Dave regards Suman -- 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 v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
Dave Gerlach d-gerl...@ti.com writes: Kevin/Ohad, On 09/09/2014 02:59 PM, Suman Anna wrote: Hi Ohad, On 09/09/2014 05:31 AM, Ohad Ben-Cohen wrote: On Tue, Sep 9, 2014 at 1:30 AM, Kevin Hilman khil...@linaro.org wrote: To me, it's not terribly clear how you made the split between this PM core code an the remoteproc code. In the changelog for the remoteproc patch, it states it's to load the firmware for and boot the wkup_m3. But, while parts of the IPC are here in pm33xx.c, parts of the IPC are also inside the remoteproc driver, so I'm quite curious if that's OK with the remoteproc maintainers. Either way, please make it clearer how and why you made the split, and please isolate the wkup_m3 IPC/protocol from this code. Think of people wanting to rework/extend the wkup_m3 firmware. They shouldn't be messing around in here, but rather inside a driver specificaly for the wkup_m3. I haven't looked at the code very thoroughly yet, but generally a remoteproc driver should only implement the three start/stop/kick rproc_ops, and then register them via the remoteproc framework. Exposing additional API directly from that driver isn't something we immediately want to accept. If relevant, we would generally prefer to extend remoteproc instead, so other platform-specific drivers could utilize that functionality as well. Or rpmsg - if we're missing some IPC functionality. The WkupM3 cannot access DDR, and so we don't intend to use rpmsg. The IPC with wkup_m3 is usually one of the last steps for putting the SoC into a desired low-power state either during suspend or cpuidle, and the communication uses a bank of fixed registers. The .kick is specific to virtio-based communication, and so this is not gonna be used. If you can take a closer look at the wkup_m3 remoteproc driver and give your comments, then we can plan on the next steps. Especially as there are also pieces pertaining to the PM layer knowing the WkupM3 has been loaded and booted. There are already some pending comments on code fragments from Santosh and myself, but let us know your inputs on the integration aspects on PM, remoteproc and IPC with WkupM3. The split was defined by putting all the application specific (to the firmware in use) code in the platform pm code while trying to keep all the IPC code within the wkup_m3_rproc driver. I don't even see that split. I see the platform PM code directly setting IPC register values, but then rproc driver actually sends the mailbox command. The exposed API is definitely heavily biased towards the intended use-case, Maybe if the API was actually documented, it would be easier for us to review it. but the CM3 was designed with this exact purpose in mind and not much else, and due to the limited IPC registers we have to work with there isn't a whole lot of flexibility. Only IPC reg 0 is always used as the resume address, the usage of the other registers is defined by the firmware and pm code. Just as a refresher for those not familiar with it, the IPC mechanism works like this: we load the ipc registers (8 for am33xx, 16 for am43xx) with any information we want to communicate to the CM3, OK, and this happens currently in the platform PM code, right? then we make a dummy write to the Mailbox which triggers an interrupt on the CM3, the CM3 does what it needs to with the info passed in the IPC regs and writes anything it wants to communicate back to these registers, and then triggers a different interrupt (not related to mailbox) to let the MPU know it is done. And this part happens in the rproc driver, right? It's kind of a mess so I figured one driver was the best way to encapsulate it all, So where is this one driver that encapsulates it all? and I still had to introduce callbacks within the wkup_m3_rproc driver so it could let the pm code know when the FW loaded (to actually enable pm) and when an interrupt was received from the wkup_m3 (so the pm code can process the response). As Suman stated, this sequence is part of the suspend path and also will be part of the lower c-states for cpuidle, so we need something fast and lightweight. RPMsg is way more than we need and it doesn't really fit the use case, so I'm not sure what makes the most sense, extending remoteproc in some way to support IPC communication like described above or leaving the basic FW loading functionality in place in remoteproc but moving the IPC and wkup_m3 functionality back into the platform pm33xx code as it's so specific to that use-case anyway. I'm not advocating for using rpmsg (anymore). But I dont' think shoving your rpmsg-lite IPC into your rproc driver is the right answer either (and Ohad's repsonse confirmed my suspicion.) What I think you need to do (and what I've recommended at least once in earlier reviews) put all the (non-rproc) wkup_m3 IPC into into one driver and create a well-described, well-documented API that the
Re: [PATCH v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
+Ohad Dave Gerlach d-gerl...@ti.com writes: AM335x supports various low power modes as documented in section 8.1.4.3 of the AM335x Technical Reference Manual. DeepSleep0 mode offers the lowest power mode with limited wakeup sources without a system reboot and is mapped as the suspend state in the kernel. In this state, MPU and PER domains are turned off with the internal RAM held in retention to facilitate the resume process. As part of the boot process, the assembly code is copied over to OCMCRAM using the OMAP SRAM code so it can be executed to turn of the EMIF. AM335x has a Cortex-M3 (WKUP_M3) which assists the MPU in DeepSleep0 and Standby entry and exit. WKUP_M3 takes care of the clockdomain and powerdomain transitions based on the intended low power state. MPU needs to load the appropriate WKUP_M3 binary onto the WKUP_M3 memory space before it can leverage any of the PM features like DeepSleep. The WKUP_M3 is managed by a remoteproc driver. The PM code hooks into the remoteproc driver through an rproc_ready callback to allow PM features to become available once the firmware for the wkup_m3 has been loaded. This code maintains its own state machine for the wkup_m3 so that it can be used in the manner intended for low power modes. In the current implementation when the suspend process is initiated, MPU interrupts the WKUP_M3 to let it know about the intent of entering DeepSleep0 and waits for an ACK. When the ACK is received MPU continues with its suspend process to suspend all the drivers and then jumps to assembly in OCMC RAM. The assembly code puts the external RAM in self-refresh mode, gates the MPU clock, and then finally executes the WFI instruction. Execution of the WFI instruction with MPU clock gated triggers another interrupt to the WKUP_M3 which then continues with the power down sequence wherein the clockdomain and powerdomain transition takes place. As part of the sleep sequence, WKUP_M3 unmasks the interrupt lines for the wakeup sources. WFI execution on WKUP_M3 causes the hardware to disable the main oscillator of the SoC and from here system remains in sleep state until a wake source brings the system into resume path. When a wakeup event occurs, WKUP_M3 starts the power-up sequence by switching on the power domains and finally enabling the clock to MPU. Since the MPU gets powered down as part of the sleep sequence in the resume path ROM code starts executing. The ROM code detects a wakeup from sleep and then jumps to the resume location in OCMC which was populated in one of the IPC registers as part of the suspend sequence. Code is based on work by Vaibhav Bedia. Signed-off-by: Dave Gerlach d-gerl...@ti.com --- v3-v4: Remove all direct wkup_m3 usage and moved to rproc driver introduced in previous patch. This statement is rather confusing as there's still quite a bit of direct wkup_m3 usage, including the guts of the protocal for message passing. I thought you had agreed based on earilier reviews to split out the wkup_m3 into it's on little driver with a clear/clean API which could be called from here? To me, it's not terribly clear how you made the split between this PM core code an the remoteproc code. In the changelog for the remoteproc patch, it states it's to load the firmware for and boot the wkup_m3. But, while parts of the IPC are here in pm33xx.c, parts of the IPC are also inside the remoteproc driver, so I'm quite curious if that's OK with the remoteproc maintainers. Either way, please make it clearer how and why you made the split, and please isolate the wkup_m3 IPC/protocol from this code. Think of people wanting to rework/extend the wkup_m3 firmware. They shouldn't be messing around in here, but rather inside a driver specificaly for the wkup_m3. Also, I'll beat this drum again even though nobody is listening: it's still very fuzzy to me how this approach is going to be used to manage low-power idle? Is low-power idle just being completely ignored for this SoC? 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
[PATCH v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support
AM335x supports various low power modes as documented
in section 8.1.4.3 of the AM335x Technical Reference Manual.
DeepSleep0 mode offers the lowest power mode with limited
wakeup sources without a system reboot and is mapped as
the suspend state in the kernel. In this state, MPU and
PER domains are turned off with the internal RAM held in
retention to facilitate the resume process. As part of
the boot process, the assembly code is copied over to OCMCRAM
using the OMAP SRAM code so it can be executed to turn of the
EMIF.
AM335x has a Cortex-M3 (WKUP_M3) which assists the MPU
in DeepSleep0 and Standby entry and exit. WKUP_M3 takes care
of the clockdomain and powerdomain transitions based on the
intended low power state. MPU needs to load the appropriate
WKUP_M3 binary onto the WKUP_M3 memory space before it can
leverage any of the PM features like DeepSleep.
The WKUP_M3 is managed by a remoteproc driver. The PM code hooks
into the remoteproc driver through an rproc_ready callback to
allow PM features to become available once the firmware for the
wkup_m3 has been loaded. This code maintains its own state machine
for the wkup_m3 so that it can be used in the manner intended for
low power modes.
In the current implementation when the suspend process
is initiated, MPU interrupts the WKUP_M3 to let it know about
the intent of entering DeepSleep0 and waits for an ACK. When
the ACK is received MPU continues with its suspend process
to suspend all the drivers and then jumps to assembly in
OCMC RAM. The assembly code puts the external RAM in self-refresh
mode, gates the MPU clock, and then finally executes the WFI
instruction. Execution of the WFI instruction with MPU clock gated
triggers another interrupt to the WKUP_M3 which then continues
with the power down sequence wherein the clockdomain and
powerdomain transition takes place. As part of the sleep sequence,
WKUP_M3 unmasks the interrupt lines for the wakeup sources. WFI
execution on WKUP_M3 causes the hardware to disable the main
oscillator of the SoC and from here system remains in sleep state
until a wake source brings the system into resume path.
When a wakeup event occurs, WKUP_M3 starts the power-up
sequence by switching on the power domains and finally
enabling the clock to MPU. Since the MPU gets powered down
as part of the sleep sequence in the resume path ROM code
starts executing. The ROM code detects a wakeup from sleep
and then jumps to the resume location in OCMC which was
populated in one of the IPC registers as part of the suspend
sequence.
Code is based on work by Vaibhav Bedia.
Signed-off-by: Dave Gerlach d-gerl...@ti.com
---
v3-v4:
Remove all direct wkup_m3 usage and moved to rproc driver introduced
in previous patch.
arch/arm/mach-omap2/pm33xx.c | 346 +++
arch/arm/mach-omap2/pm33xx.h | 64
2 files changed, 410 insertions(+)
create mode 100644 arch/arm/mach-omap2/pm33xx.c
create mode 100644 arch/arm/mach-omap2/pm33xx.h
diff --git a/arch/arm/mach-omap2/pm33xx.c b/arch/arm/mach-omap2/pm33xx.c
new file mode 100644
index 000..30d0f7d
--- /dev/null
+++ b/arch/arm/mach-omap2/pm33xx.c
@@ -0,0 +1,346 @@
+/*
+ * AM33XX Power Management Routines
+ *
+ * Copyright (C) 2012-2014 Texas Instruments Incorporated - http://www.ti.com/
+ * Vaibhav Bedia, Dave Gerlach
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed as is WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/kernel.h
+#include linux/init.h
+#include linux/cpu.h
+#include linux/err.h
+#include linux/firmware.h
+#include linux/io.h
+#include linux/platform_device.h
+#include linux/sched.h
+#include linux/suspend.h
+#include linux/completion.h
+#include linux/module.h
+#include linux/interrupt.h
+#include linux/ti_emif.h
+#include linux/omap-mailbox.h
+#include linux/sizes.h
+
+#include asm/suspend.h
+#include asm/proc-fns.h
+#include asm/fncpy.h
+#include asm/system_misc.h
+
+#include pm.h
+#include cm33xx.h
+#include pm33xx.h
+#include common.h
+#include clockdomain.h
+#include powerdomain.h
+#include soc.h
+#include sram.h
+
+static void __iomem *am33xx_emif_base;
+static struct powerdomain *cefuse_pwrdm, *gfx_pwrdm, *per_pwrdm, *mpu_pwrdm;
+static struct clockdomain *gfx_l4ls_clkdm;
+
+static struct am33xx_pm_context *am33xx_pm;
+
+static DECLARE_COMPLETION(am33xx_pm_sync);
+
+static void (*am33xx_do_wfi_sram)(struct am33xx_suspend_params *);
+
+static struct am33xx_suspend_params susp_params;
+
+#ifdef CONFIG_SUSPEND
+
+static int am33xx_do_sram_idle(long unsigned int unused)
+{
+ am33xx_do_wfi_sram(susp_params);
+ return 0;
+}
+
