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

[Kevin Hilman]

Vaibhav Bedia <vaibhav.bedia at gmail.com> writes:

> On Tue, Aug 27, 2013 at 5:45 PM, Kevin Hilman <khilman at 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.

IMO, there should actually be 3 levels. the SoC PM implementation
(pm33xx.c), the M3 driver where the protocol, state-machine, etc. are
handled, and the messaging layer.  In the current proposal, the last 2
are combined, but I'd really like to see a generalized messaging layer
that everyone else using an M3 coprocessor might use as well.  As
mentioned already, I think that should be rpmsg, but that still needs
some exploration.

> 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 ;)

Sounds like AM43xx is better.  If you have a control module bit to
trigger an interrupt, why do you need the mailbox at all?

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

Thanks,

Kevin