| Message ID | 1405047349-15101-11-git-send-email-d-gerlach@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
+Ohad Dave Gerlach <d-gerlach@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-gerlach@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
On Tue, Sep 9, 2014 at 1:30 AM, Kevin Hilman <khilman@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.
Hi Ohad, On 09/09/2014 05:31 AM, Ohad Ben-Cohen wrote: > On Tue, Sep 9, 2014 at 1:30 AM, Kevin Hilman <khilman@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
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 <khilman@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 >
Dave Gerlach <d-gerlach@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 <khilman@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 platform PM code will use. IMO, the current "split" is very difficult to read/understand, which means it will even more difficult to maintain. Kevin
Kevin, On 09/09/2014 04:10 PM, Kevin Hilman wrote: > Dave Gerlach <d-gerlach@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 <khilman@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 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 > 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 dont think I entirely understand your vision for the API. I see it going in one of two directions: PM/Application agnostic: provide ability to write/read wkup_m3 (mailbox ping is handled automatically by the driver also) and then callbacks for rproc being ready and handling response from wkup_m3 for the PM code to use, and that's it. Well let the PM code sort out how it uses everything. This means that there is a payload (similar to the structure in place now that takes the register values) that gets configured and then the wkup_m3 driver just passes the info back and forth between the MPU and wkup_m3 without the driver ever knowing what's actually happening. OR Application specific: Provide ability to set use-case specific functionality, i.e. wkup_m3_set_low_power_mode, wkup_m3_set_resume_address, etc... which exposes the high level functions provided by the wkup_m3 firmware (which are limited to the functionality in the TI provided firmware, which is all that is intended anyway), and the pm code uses this to accomplish what it wants without any knowledge of the actual communication or configuration. I think the first version is more scalable and maintainable for future applications, but perhaps I am still not aligned with your vision. Thoughts? Regards, Dave > > Kevin >
On Wed, Sep 10, 2014 at 12:10 AM, Kevin Hilman <khilman@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.
Hi Ohad, On 09/16/2014 10:08 AM, Ohad Ben-Cohen wrote: > On Wed, Sep 10, 2014 at 12:10 AM, Kevin Hilman <khilman@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
Hi Suman, On Tue, Sep 16, 2014 at 7:14 PM, Suman Anna <s-anna@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.
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-anna@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 how the layers should be divided. Does remoteproc notify an IPC layer which notifies the PM code that the rproc is booted and the IPC data is ready? Or does the remoteproc notify PM code directly and then that uses the IPC layer to read the IPC registers? However the wkup_m3 does trigger the wkup_m3 interrupt after it is done booting, so perhaps that could be used as the trigger instead, which fits better into the IPC layer. There are two different hooks back to PM code. One that indicates when wkup_m3 has booted, and one that indicates when the wkup_m3 interrupt has been received. As I mentioned before, perhaps we can get away with just using the wkup_m3 interrupt to indicate when the rproc has booted because it won't be triggered unless the fw runs from the start. Perhaps we can live with one notifier hook (for the interrupt from the wkup_m3), but I am still unclear on how generic the IPC layer should be. A very generic layer that just reads the IPC registers and handlers the interrupts could be reused, leaving the PM code to interprets those values while a more PM specific IPC layer would have to evolve with the PM layer as functionality changes, so I would definitely lean towards the highly generic IPC layer. Regards, Dave > > Thanks, Ohad. >
diff --git a/arch/arm/mach-omap2/pm33xx.c b/arch/arm/mach-omap2/pm33xx.c new file mode 100644 index 0000000..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; +} + +static int am33xx_pm_suspend(void) +{ + int i, ret = 0; + int status = 0; + struct wkup_m3_wakeup_src wakeup_src = {.irq_nr = 0, + .src = "Unknown",}; + + /* Try to put GFX to sleep */ + omap_set_pwrdm_state(gfx_pwrdm, PWRDM_POWER_OFF); + + ret = cpu_suspend(0, am33xx_do_sram_idle); + + status = pwrdm_read_pwrst(gfx_pwrdm); + if (status != PWRDM_POWER_OFF) + pr_err("PM: GFX domain did not transition\n"); + + /* + * BUG: GFX_L4LS clock domain needs to be woken up to + * ensure thet L4LS clock domain does not get stuck in transition + * If that happens L3 module does not get disabled, thereby leading + * to PER power domain transition failing + */ + clkdm_wakeup(gfx_l4ls_clkdm); + clkdm_sleep(gfx_l4ls_clkdm); + + if (ret) { + pr_err("PM: Kernel suspend failure\n"); + } else { + i = wkup_m3_pm_status(); + + switch (i) { + case 0: + pr_info("PM: Successfully put all powerdomains to target state\n"); + + /* + * The PRCM registers on AM335x do not contain + * previous state information like those present on + * OMAP4 so we must manually indicate transition so + * state counters are properly incremented + */ + pwrdm_post_transition(mpu_pwrdm); + pwrdm_post_transition(per_pwrdm); + break; + case 1: + pr_err("PM: Could not transition all powerdomains to target state\n"); + ret = -1; + break; + default: + pr_err("PM: CM3 returned unknown result = %d\n", i); + ret = -1; + } + /* print the wakeup reason */ + wkup_m3_wake_src(&wakeup_src); + + pr_info("PM: Wakeup source %s\n", wakeup_src.src); + } + + return ret; +} + +static int am33xx_pm_enter(suspend_state_t suspend_state) +{ + int ret = 0; + + switch (suspend_state) { + case PM_SUSPEND_MEM: + ret = am33xx_pm_suspend(); + break; + default: + ret = -EINVAL; + } + + return ret; +} + + +static void am33xx_m3_state_machine_reset(void) +{ + int i; + + am33xx_pm->ipc.reg1 = IPC_CMD_RESET; + + wkup_m3_set_cmd(&am33xx_pm->ipc); + + am33xx_pm->state = M3_STATE_MSG_FOR_RESET; + + if (!wkup_m3_ping()) { + i = wait_for_completion_timeout(&am33xx_pm_sync, + msecs_to_jiffies(500)); + if (!i) { + WARN(1, "PM: MPU<->CM3 sync failure\n"); + am33xx_pm->state = M3_STATE_UNKNOWN; + } + } else { + pr_warn("PM: Unable to ping CM3\n"); + } +} + +static int am33xx_pm_begin(suspend_state_t state) +{ + int i; + + + switch (state) { + case PM_SUSPEND_MEM: + am33xx_pm->ipc.reg1 = IPC_CMD_DS0; + break; + } + + am33xx_pm->ipc.reg2 = DS_IPC_DEFAULT; + am33xx_pm->ipc.reg3 = DS_IPC_DEFAULT; + + wkup_m3_set_cmd(&am33xx_pm->ipc); + + am33xx_pm->state = M3_STATE_MSG_FOR_LP; + + if (!wkup_m3_ping()) { + i = wait_for_completion_timeout(&am33xx_pm_sync, + msecs_to_jiffies(500)); + if (!i) { + WARN(1, "PM: MPU<->CM3 sync failure\n"); + return -1; + } + } else { + pr_warn("PM: Unable to ping CM3\n"); + return -1; + } + + return 0; +} + +static void am33xx_pm_end(void) +{ + am33xx_m3_state_machine_reset(); +} + +static int am33xx_pm_valid(suspend_state_t state) +{ + switch (state) { + case PM_SUSPEND_MEM: + return 1; + default: + return 0; + } +} + +static const struct platform_suspend_ops am33xx_pm_ops = { + .begin = am33xx_pm_begin, + .end = am33xx_pm_end, + .enter = am33xx_pm_enter, + .valid = am33xx_pm_valid, +}; +#endif /* CONFIG_SUSPEND */ + +static void am33xx_txev_handler(void) +{ + switch (am33xx_pm->state) { + case M3_STATE_RESET: + am33xx_pm->state = M3_STATE_INITED; + complete(&am33xx_pm_sync); + break; + case M3_STATE_MSG_FOR_RESET: + am33xx_pm->state = M3_STATE_INITED; + complete(&am33xx_pm_sync); + break; + case M3_STATE_MSG_FOR_LP: + complete(&am33xx_pm_sync); + break; + case M3_STATE_UNKNOWN: + pr_warn("PM: Unknown CM3 State\n"); + } +} + +static void am33xx_m3_ready_cb(void) +{ + am33xx_pm->ver = wkup_m3_fw_version_read(); + + if (am33xx_pm->ver == M3_VERSION_UNKNOWN || + am33xx_pm->ver < M3_BASELINE_VERSION) { + pr_warn("PM: CM3 Firmware Version %x not supported\n", + am33xx_pm->ver); + return; + } else { + pr_info("PM: CM3 Firmware Version = 0x%x\n", + am33xx_pm->ver); + } + +#ifdef CONFIG_SUSPEND + suspend_set_ops(&am33xx_pm_ops); +#endif /* CONFIG_SUSPEND */ +} + +static struct wkup_m3_ops am33xx_wkup_m3_ops = { + .txev_handler = am33xx_txev_handler, + .rproc_ready = am33xx_m3_ready_cb, +}; + +/* + * Push the minimal suspend-resume code to SRAM + */ +void am33xx_push_sram_idle(void) +{ + am33xx_do_wfi_sram = (void *)omap_sram_push + (am33xx_do_wfi, am33xx_do_wfi_sz); +} + +static int __init am33xx_map_emif(void) +{ + am33xx_emif_base = ioremap(AM33XX_EMIF_BASE, SZ_32K); + + if (!am33xx_emif_base) + return -ENOMEM; + + return 0; +} + +int __init am33xx_pm_init(void) +{ + int ret; + u32 temp; + + if (!soc_is_am33xx()) + return -ENODEV; + + gfx_pwrdm = pwrdm_lookup("gfx_pwrdm"); + per_pwrdm = pwrdm_lookup("per_pwrdm"); + mpu_pwrdm = pwrdm_lookup("mpu_pwrdm"); + + gfx_l4ls_clkdm = clkdm_lookup("gfx_l4ls_gfx_clkdm"); + + if ((!gfx_pwrdm) || (!per_pwrdm) || (!mpu_pwrdm) || (!gfx_l4ls_clkdm)) { + ret = -ENODEV; + goto err; + } + + am33xx_pm = kzalloc(sizeof(*am33xx_pm), GFP_KERNEL); + if (!am33xx_pm) { + pr_err("Memory allocation failed\n"); + ret = -ENOMEM; + return ret; + } + + ret = am33xx_map_emif(); + if (ret) { + pr_err("PM: Could not ioremap EMIF\n"); + goto err; + } + + /* Determine Memory Type */ + temp = readl(am33xx_emif_base + EMIF_SDRAM_CONFIG); + temp = (temp & SDRAM_TYPE_MASK) >> SDRAM_TYPE_SHIFT; + /* Parameters to pass to aseembly code */ + susp_params.emif_addr_virt = am33xx_emif_base; + susp_params.dram_sync = am33xx_dram_sync; + susp_params.mem_type = temp; + am33xx_pm->ipc.reg4 = temp; + + (void) clkdm_for_each(omap_pm_clkdms_setup, NULL); + + /* CEFUSE domain can be turned off post bootup */ + cefuse_pwrdm = pwrdm_lookup("cefuse_pwrdm"); + if (cefuse_pwrdm) + omap_set_pwrdm_state(cefuse_pwrdm, PWRDM_POWER_OFF); + else + pr_err("PM: Failed to get cefuse_pwrdm\n"); + + am33xx_pm->state = M3_STATE_RESET; + + wkup_m3_set_ops(&am33xx_wkup_m3_ops); + + /* Physical resume address to be used by ROM code */ + am33xx_pm->ipc.reg0 = (AM33XX_OCMC_END - + am33xx_do_wfi_sz + am33xx_resume_offset + 0x4); + + return 0; + +err: + kfree(am33xx_pm); + return ret; +} diff --git a/arch/arm/mach-omap2/pm33xx.h b/arch/arm/mach-omap2/pm33xx.h new file mode 100644 index 0000000..309e65a --- /dev/null +++ b/arch/arm/mach-omap2/pm33xx.h @@ -0,0 +1,64 @@ +/* + * AM33XX Power Management Routines + * + * Copyright (C) 2012-2014 Texas Instruments Inc. + * 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. + */ +#ifndef __ARCH_ARM_MACH_OMAP2_PM33XX_H +#define __ARCH_ARM_MACH_OMAP2_PM33XX_H + +#ifndef __ASSEMBLER__ + +#include <linux/wkup_m3.h> + +struct am33xx_pm_context { + struct wkup_m3_ipc_regs ipc; + struct firmware *firmware; + struct omap_mbox *mbox; + u8 state; + u32 ver; +}; + +/* + * Params passed to suspend routine + * + * These are used to load into registers by suspend code, + * entries here must always be in sync with the suspend code + * in arm/mach-omap2/sleep33xx.S + */ +struct am33xx_suspend_params { + void __iomem *emif_addr_virt; + u32 mem_type; + void __iomem *dram_sync; +}; + +#endif /*__ASSEMBLER__ */ + +#define IPC_CMD_DS0 0x4 +#define IPC_CMD_STANDBY 0xc +#define IPC_CMD_RESET 0xe +#define DS_IPC_DEFAULT 0xffffffff +#define M3_VERSION_UNKNOWN 0x0000ffff +#define M3_BASELINE_VERSION 0x187 + +#define M3_STATE_UNKNOWN 0 +#define M3_STATE_RESET 1 +#define M3_STATE_INITED 2 +#define M3_STATE_MSG_FOR_LP 3 +#define M3_STATE_MSG_FOR_RESET 4 + +#define AM33XX_OCMC_END 0x40310000 +#define AM33XX_EMIF_BASE 0x4C000000 + +#define MEM_TYPE_DDR2 2 + +#endif /* __ARCH_ARM_MACH_OMAP2_PM33XX_H */
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-gerlach@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