| Message ID | 1442967294-23837-2-git-send-email-d-gerlach@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 23, 2015 at 5:44 AM, Dave Gerlach <d-gerlach@ti.com> wrote: > The mailbox framework controls the transmission queue and requires > either its controller implementations or clients to run the state > machine for the Tx queue. The OMAP mailbox controller uses a Tx-ready > interrupt as the equivalent of a Tx-done interrupt to run this Tx > queue state-machine. > > The WkupM3 processor on AM33xx and AM43xx SoCs is used to offload > certain PM tasks, like doing the necessary operations for Device > PM suspend/resume or for entering lower c-states during cpuidle. > > The CPUIdle on AM33xx requires the messages to be sent without > having to trigger the Tx-ready interrupts, as the interrupt > would immediately terminate the CPUIdle operation. Support for > this has been added by introducing a DT quirk, "ti,mbox-send-noirq" > and using it to modify the normal OMAP mailbox controller behavior > on the sub-mailboxes used to communicate with the WkupM3 remote > processor. This also requires the wkup_m3_ipc driver to adjust > its mailbox usage logic to run the Tx state machine. > Probably Suman already updated you on what was discussed about this patch at Connect-SFO. My suggestion was to drive TX poll-based (I know, I know...) because the "interrupt-driven" is just an impression, it is not really. You get the interrupt as soon as you enable it because it is the "FIFO not-full" interrupt which is always because there is always space left after writing to the fifo. The cons are that it buffers messages hidden from the client while abusing the irq. If you guys could break away from the "interrupt-driven" illusion and use polling which it actually is, everything falls into place and you avoid the "ti,mbox-send-noirq" quirk. Anyways I am OK too, if you guys want to fix it with a platform specific quirk. Let me know I'll pick this patch. Cheers!
Hi Jassi, On 10/20/2015 11:44 PM, Jassi Brar wrote: > On Wed, Sep 23, 2015 at 5:44 AM, Dave Gerlach <d-gerlach@ti.com> wrote: >> The mailbox framework controls the transmission queue and requires >> either its controller implementations or clients to run the state >> machine for the Tx queue. The OMAP mailbox controller uses a Tx-ready >> interrupt as the equivalent of a Tx-done interrupt to run this Tx >> queue state-machine. >> >> The WkupM3 processor on AM33xx and AM43xx SoCs is used to offload >> certain PM tasks, like doing the necessary operations for Device >> PM suspend/resume or for entering lower c-states during cpuidle. >> >> The CPUIdle on AM33xx requires the messages to be sent without >> having to trigger the Tx-ready interrupts, as the interrupt >> would immediately terminate the CPUIdle operation. Support for >> this has been added by introducing a DT quirk, "ti,mbox-send-noirq" >> and using it to modify the normal OMAP mailbox controller behavior >> on the sub-mailboxes used to communicate with the WkupM3 remote >> processor. This also requires the wkup_m3_ipc driver to adjust >> its mailbox usage logic to run the Tx state machine. >> > Probably Suman already updated you on what was discussed about this > patch at Connect-SFO. My suggestion was to drive TX poll-based (I > know, I know...) because the "interrupt-driven" is just an impression, > it is not really. You get the interrupt as soon as you enable it > because it is the "FIFO not-full" interrupt which is always because > there is always space left after writing to the fifo. The cons are > that it buffers messages hidden from the client while abusing the irq. > If you guys could break away from the "interrupt-driven" illusion and > use polling which it actually is, everything falls into place and you > avoid the "ti,mbox-send-noirq" quirk. Looking at this closely, even with that approach, we would still need a quirk to deal with the weird behavior of our wakeup m3. Essentially we have two independent things: 1. Tx state machine (currently interrupt driven) for ticking the mailbox core tx state machine. 2. A quirk that we need for dealing with Wakeup M3 behavior, MPU needs to take out the message on behalf of WkupM3 processor after transmission and clearing the IP-level interrupt intended for WkupM3. This will be irrespective of the Tx state machine choice, and this needs to be different only on the specific mbox channel used to talk with WkupM3 processor. Previously, we were doing #2 in the OMAP mailbox regular interrupt handler (handles both Rx and Tx interrupts) as part of the Rx interrupt handler with the rx mbox variables reflecting those of the WkupM3 processor. But when we remove the interrupt processing, we now actually need to add code to deal with this. > Anyways I am OK too, if you guys want to fix it with a platform > specific quirk. Let me know I'll pick this patch. I haven't gotten a chance to try #1, and I won't be able to look at it atleast for another month. I suggest that you go ahead and pick this patch up, as a quirk is needed in one form or the other for #2, and #1 is anyways a bigger change that will affect all our IPC stacks across all pairs of MPU - remote processors on different SoCs. regards Suman
On 22 October 2015 at 05:35, Suman Anna <s-anna@ti.com> wrote: >> Anyways I am OK too, if you guys want to fix it with a platform >> specific quirk. Let me know I'll pick this patch. > > I haven't gotten a chance to try #1, and I won't be able to look at it > atleast for another month. I suggest that you go ahead and pick this > patch up, as a quirk is needed in one form or the other for #2, and #1 > is anyways a bigger change that will affect all our IPC stacks across > all pairs of MPU - remote processors on different SoCs. > Yeah that is the reason I offered to pick this patch as such. OK I'll take this patch.
On 10/21/2015 11:42 PM, Jassi Brar wrote: > On 22 October 2015 at 05:35, Suman Anna <s-anna@ti.com> wrote: > >>> Anyways I am OK too, if you guys want to fix it with a platform >>> specific quirk. Let me know I'll pick this patch. >> >> I haven't gotten a chance to try #1, and I won't be able to look at it >> atleast for another month. I suggest that you go ahead and pick this >> patch up, as a quirk is needed in one form or the other for #2, and #1 >> is anyways a bigger change that will affect all our IPC stacks across >> all pairs of MPU - remote processors on different SoCs. >> > Yeah that is the reason I offered to pick this patch as such. > OK I'll take this patch. Thanks a lot. We will be a lot closer to get PM working on AM335/AM437x SoCs. regards Suman
diff --git a/Documentation/devicetree/bindings/mailbox/omap-mailbox.txt b/Documentation/devicetree/bindings/mailbox/omap-mailbox.txt index d1a0433..9b40c49 100644 --- a/Documentation/devicetree/bindings/mailbox/omap-mailbox.txt +++ b/Documentation/devicetree/bindings/mailbox/omap-mailbox.txt @@ -75,6 +75,14 @@ data that represent the following: Cell #3 (usr_id) - mailbox user id for identifying the interrupt line associated with generating a tx/rx fifo interrupt. +Optional Properties: +-------------------- +- ti,mbox-send-noirq: Quirk flag to allow the client user of this sub-mailbox + to send messages without triggering a Tx ready interrupt, + and to control the Tx ticker. Should be used only on + sub-mailboxes used to communicate with WkupM3 remote + processor on AM33xx/AM43xx SoCs. + Mailbox Users: ============== A device needing to communicate with a target processor device should specify diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c index a3dbfd9..b7f636f 100644 --- a/drivers/mailbox/omap-mailbox.c +++ b/drivers/mailbox/omap-mailbox.c @@ -38,6 +38,8 @@ #include <linux/mailbox_controller.h> #include <linux/mailbox_client.h> +#include "mailbox.h" + #define MAILBOX_REVISION 0x000 #define MAILBOX_MESSAGE(m) (0x040 + 4 * (m)) #define MAILBOX_FIFOSTATUS(m) (0x080 + 4 * (m)) @@ -106,6 +108,7 @@ struct omap_mbox_fifo_info { int rx_irq; const char *name; + bool send_no_irq; }; struct omap_mbox { @@ -119,6 +122,7 @@ struct omap_mbox { u32 ctx[OMAP4_MBOX_NR_REGS]; u32 intr_type; struct mbox_chan *chan; + bool send_no_irq; }; /* global variables for the mailbox devices */ @@ -418,6 +422,9 @@ static int omap_mbox_startup(struct omap_mbox *mbox) goto fail_request_irq; } + if (mbox->send_no_irq) + mbox->chan->txdone_method = TXDONE_BY_ACK; + _omap_mbox_enable_irq(mbox, IRQ_RX); return 0; @@ -586,13 +593,27 @@ static void omap_mbox_chan_shutdown(struct mbox_chan *chan) mutex_unlock(&mdev->cfg_lock); } -static int omap_mbox_chan_send_data(struct mbox_chan *chan, void *data) +static int omap_mbox_chan_send_noirq(struct omap_mbox *mbox, void *data) { - struct omap_mbox *mbox = mbox_chan_to_omap_mbox(chan); int ret = -EBUSY; - if (!mbox) - return -EINVAL; + if (!mbox_fifo_full(mbox)) { + _omap_mbox_enable_irq(mbox, IRQ_RX); + mbox_fifo_write(mbox, (mbox_msg_t)data); + ret = 0; + _omap_mbox_disable_irq(mbox, IRQ_RX); + + /* we must read and ack the interrupt directly from here */ + mbox_fifo_read(mbox); + ack_mbox_irq(mbox, IRQ_RX); + } + + return ret; +} + +static int omap_mbox_chan_send(struct omap_mbox *mbox, void *data) +{ + int ret = -EBUSY; if (!mbox_fifo_full(mbox)) { mbox_fifo_write(mbox, (mbox_msg_t)data); @@ -604,6 +625,22 @@ static int omap_mbox_chan_send_data(struct mbox_chan *chan, void *data) return ret; } +static int omap_mbox_chan_send_data(struct mbox_chan *chan, void *data) +{ + struct omap_mbox *mbox = mbox_chan_to_omap_mbox(chan); + int ret; + + if (!mbox) + return -EINVAL; + + if (mbox->send_no_irq) + ret = omap_mbox_chan_send_noirq(mbox, data); + else + ret = omap_mbox_chan_send(mbox, data); + + return ret; +} + static const struct mbox_chan_ops omap_mbox_chan_ops = { .startup = omap_mbox_chan_startup, .send_data = omap_mbox_chan_send_data, @@ -732,6 +769,9 @@ static int omap_mbox_probe(struct platform_device *pdev) finfo->rx_usr = tmp[2]; finfo->name = child->name; + + if (of_find_property(child, "ti,mbox-send-noirq", NULL)) + finfo->send_no_irq = true; } else { finfo->tx_id = info->tx_id; finfo->rx_id = info->rx_id; @@ -791,6 +831,7 @@ static int omap_mbox_probe(struct platform_device *pdev) fifo->irqstatus = MAILBOX_IRQSTATUS(intr_type, finfo->rx_usr); fifo->irqdisable = MAILBOX_IRQDISABLE(intr_type, finfo->rx_usr); + mbox->send_no_irq = finfo->send_no_irq; mbox->intr_type = intr_type; mbox->parent = mdev;