Re: [PATCHv3 8/9] ARM: OMAP2+: AM33XX: Basic suspend resume support

[Vaibhav Bedia <vaibhav.bedia@gmail.com>]

On Tue, Aug 27, 2013 at 5:45 PM, Kevin Hilman <khilman@linaro.org> wrote:
[...]
>
> Looking closer at this code as I'm trying to fully get my head around
> all the IPC, I have some more comments.
>
> I think the split between pm33xx.c and the M3 driver is still confusing
> here.  For example, am33xx_ping_wkup_m3(),
> am33xx_m3_state_machine_reset() and the guts of am33xx_pm_begin() all
> belong inside the M3 driver, along with all the wakeup_src stuff, which
> is info coming from the M3.
>
> IOW, the communication with M3 should be abstracted from pm33xx by the
> M3 driver (or possibly an eventual remoteproc/rpmsg implementation) with
> a well defined API.  In this implementation, the interface is pretty
> fuzzy and mixed between pm33xx.c and wkup_m3.c.
>
>

The reason for the current split was to have the M3 driver just do the minimal
that's needed to talk to get M3 and MPU talking. What made me do it this way
was to attempt to address a previous comment on keeping options open for folks
to use M3 for things other than PM stuff. The IPC stuff is how
implementors of the
firmware (anything other than the PM one that TI provides) want it to be.

The top level idea was to have the users of the firmware (PM in this case)
decide what functionality they want when talking to M3. They are also free to
decide the register bitfield layout and other IPC details.

This was also a feeble attempt to keep things extensible for AM437x where
in addition to the broken mailbox usage there's now a control module bit
to trigger the interrupt to M3 (what's worse? pick one that you hate more ;)
The AM437x PM routines could then just register different callbacks that
are triggered when the M3 interrupts the MPU.

Hope this clears up some of the confusion.

Regards,
Vaibhav