[PATCH v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support

[Dave Gerlach]

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 at 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
>