| Message ID | 1375825238-18299-1-git-send-email-s-anna@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Suman Anna <s-anna@ti.com> writes: > The WkupM3 mailbox used for triggering PM operations such as suspend > and resume on AM33x/AM43x is special in that the M3 processor cannot > access the mailbox registers. However, an interrupt is needed to be > sent to request the M3 to perform a desired PM operation. This patch > adds the support for this special mailbox through separate ops for > this mailbox. These ops are designed to have the WkupM3's Rx interrupt > programmed within the driver, during transmission of a message. The > message is immediately read back and the internal mailbox interrupt > acknowledged as well to avoid triggering any spurious interrupts to > the M3. > > Signed-off-by: Suman Anna <s-anna@ti.com> Dumb Q: why does all this extra logic belong in the mailbox driver and not in the wkup_m3 driver? To me, this seems like part of the IPC protocol between the MPU and M3 firmware, and not an inherent part of the AM33xx mbox. Kevin
Kevin, On 08/26/2013 10:50 PM, Kevin Hilman wrote: > Suman Anna <s-anna@ti.com> writes: > >> The WkupM3 mailbox used for triggering PM operations such as suspend >> and resume on AM33x/AM43x is special in that the M3 processor cannot >> access the mailbox registers. However, an interrupt is needed to be >> sent to request the M3 to perform a desired PM operation. This patch >> adds the support for this special mailbox through separate ops for >> this mailbox. These ops are designed to have the WkupM3's Rx interrupt >> programmed within the driver, during transmission of a message. The >> message is immediately read back and the internal mailbox interrupt >> acknowledged as well to avoid triggering any spurious interrupts to >> the M3. >> >> Signed-off-by: Suman Anna <s-anna@ti.com> > > Dumb Q: why does all this extra logic belong in the mailbox driver and > not in the wkup_m3 driver? To me, this seems like part of the IPC > protocol between the MPU and M3 firmware, and not an inherent part of > the AM33xx mbox. The IPC protocol state machine for the PM operations is still very much handled in the WkupM3 driver. The IPC registers in Control module provide the messaging payloads, but unfortunately it is not associated with any direct interrupts to make it a separate driver. As far as the WkupM3 driver is concerned, it is just using the mailbox for signalling - the internals of which would involve accessing the various mailbox registers including interrupt configuration and clearing. It is preferable to have the various operations on mailbox registers handled by the mailbox driver with the API supporting the necessary operations. The previous version from Vaibhav added a new API to mailbox driver for clearing out the Tx fifo, which is non-standard. The mailboxes on AM335 will also be used for IPC with the Programmable Real-time Unit (PRU) subsystem, which will use separate mbox devices. This version handles the wkup_m3 mbox device using its device-specific ops without the need for any new API, and not having to expose the mailbox registers outside. regards Suman
Suman Anna <s-anna@ti.com> writes: > Kevin, > > On 08/26/2013 10:50 PM, Kevin Hilman wrote: >> Suman Anna <s-anna@ti.com> writes: >> >>> The WkupM3 mailbox used for triggering PM operations such as suspend >>> and resume on AM33x/AM43x is special in that the M3 processor cannot >>> access the mailbox registers. However, an interrupt is needed to be >>> sent to request the M3 to perform a desired PM operation. This patch >>> adds the support for this special mailbox through separate ops for >>> this mailbox. These ops are designed to have the WkupM3's Rx interrupt >>> programmed within the driver, during transmission of a message. The >>> message is immediately read back and the internal mailbox interrupt >>> acknowledged as well to avoid triggering any spurious interrupts to >>> the M3. After reading this multiple times, I had to go back to the TRM to try and remember what's going on here because this changelog wasn't helping me understand. IIUC, the basic idea is that the mailbox is only used to trigger an IRQ to the M3, the messages themselves are dummy. You explained some more of this in subsequent replies, but all of that detail should be in the changelogs. Remember that changelogs should be written for those who don't have (or don't remember) all the internal details that you know off the top of your head. Think of writing a changelog that you'll have to understand in a year when you haven't been staring at the hardware and code. I've never seen a changelog with too much detail, so please err on the verbose side. >>> >>> Signed-off-by: Suman Anna <s-anna@ti.com> >> >> Dumb Q: why does all this extra logic belong in the mailbox driver and >> not in the wkup_m3 driver? To me, this seems like part of the IPC >> protocol between the MPU and M3 firmware, and not an inherent part of >> the AM33xx mbox. > > The IPC protocol state machine for the PM operations is still very > much handled in the WkupM3 driver. The IPC registers in Control > module provide the messaging payloads, but unfortunately it is not > associated with any direct interrupts to make it a separate driver. As > far as the WkupM3 driver is concerned, it is just using the mailbox > for signalling - the internals of which would involve accessing the > various mailbox registers including interrupt configuration and > clearing. It is preferable to have the various operations on mailbox > registers handled by the mailbox driver with the API supporting the > necessary operations. > > The previous version from Vaibhav added a new API to mailbox driver for > clearing out the Tx fifo, which is non-standard. The mailboxes on AM335 > will also be used for IPC with the Programmable Real-time Unit (PRU) > subsystem, which will use separate mbox devices. This version handles > the wkup_m3 mbox device using its device-specific ops without the need > for any new API, and not having to expose the mailbox registers outside. Perhaps I'm misunderstanding all of this (admittedly, I'm not up on the details of OMAP mailbox internals), but the changelog and code are not helping me understand so I have to ask dumb questions. The more I look at this, the more confused I get. The wkupm3 mbox is defined in the DTS to use usr_id=0 yet internally is overridden/hard-coded internally to use usr_id=3 in (at least for managing interrupts.) Since only the interrupt management matters anyways (message payload is dummy), is there any reason not to define the mbox in DTS using usr_id=3 so you don't have to do this clumsy override? IOW, since the M3 (usr_id=3) can't ever be a real user of the mbox registers, just have the MPU drive the mbox as usr_id=3? Even that is a bit hacky IMO, but with proper documentation, is at least better than the current approach IMO. That should at least get rid of the need to customize the IRQ management functions of mailbox-omap2.c. After that, the M3 driver could then just do: omap_mbox_enable_irq() omap_mbox_msg_send() omap_mbox_disable_irq() and you could get rid of the rest of the custom functions in mailbox-omap2.c also. Kevin
Kevin, On 08/27/2013 04:25 PM, Kevin Hilman wrote: > Suman Anna <s-anna@ti.com> writes: > >> Kevin, >> >> On 08/26/2013 10:50 PM, Kevin Hilman wrote: >>> Suman Anna <s-anna@ti.com> writes: >>> >>>> The WkupM3 mailbox used for triggering PM operations such as suspend >>>> and resume on AM33x/AM43x is special in that the M3 processor cannot >>>> access the mailbox registers. However, an interrupt is needed to be >>>> sent to request the M3 to perform a desired PM operation. This patch >>>> adds the support for this special mailbox through separate ops for >>>> this mailbox. These ops are designed to have the WkupM3's Rx interrupt >>>> programmed within the driver, during transmission of a message. The >>>> message is immediately read back and the internal mailbox interrupt >>>> acknowledged as well to avoid triggering any spurious interrupts to >>>> the M3. > > After reading this multiple times, I had to go back to the TRM to try > and remember what's going on here because this changelog wasn't helping > me understand. IIUC, the basic idea is that the mailbox is only used to > trigger an IRQ to the M3, the messages themselves are dummy. You > explained some more of this in subsequent replies, but all of that > detail should be in the changelogs. > > Remember that changelogs should be written for those who don't have (or > don't remember) all the internal details that you know off the top of > your head. Think of writing a changelog that you'll have to understand > in a year when you haven't been staring at the hardware and code. I've > never seen a changelog with too much detail, so please err on the > verbose side. > >>>> >>>> Signed-off-by: Suman Anna <s-anna@ti.com> >>> >>> Dumb Q: why does all this extra logic belong in the mailbox driver and >>> not in the wkup_m3 driver? To me, this seems like part of the IPC >>> protocol between the MPU and M3 firmware, and not an inherent part of >>> the AM33xx mbox. >> >> The IPC protocol state machine for the PM operations is still very >> much handled in the WkupM3 driver. The IPC registers in Control >> module provide the messaging payloads, but unfortunately it is not >> associated with any direct interrupts to make it a separate driver. As >> far as the WkupM3 driver is concerned, it is just using the mailbox >> for signalling - the internals of which would involve accessing the >> various mailbox registers including interrupt configuration and >> clearing. It is preferable to have the various operations on mailbox >> registers handled by the mailbox driver with the API supporting the >> necessary operations. >> >> The previous version from Vaibhav added a new API to mailbox driver for >> clearing out the Tx fifo, which is non-standard. The mailboxes on AM335 >> will also be used for IPC with the Programmable Real-time Unit (PRU) >> subsystem, which will use separate mbox devices. This version handles >> the wkup_m3 mbox device using its device-specific ops without the need >> for any new API, and not having to expose the mailbox registers outside. > > Perhaps I'm misunderstanding all of this (admittedly, I'm not up on the > details of OMAP mailbox internals), but the changelog and code are not > helping me understand so I have to ask dumb questions. > > The more I look at this, the more confused I get. The wkupm3 mbox is > defined in the DTS to use usr_id=0 yet internally is > overridden/hard-coded internally to use usr_id=3 in (at least for > managing interrupts.) > > Since only the interrupt management matters anyways (message payload is > dummy), is there any reason not to define the mbox in DTS using usr_id=3 > so you don't have to do this clumsy override? IOW, since the M3 > (usr_id=3) can't ever be a real user of the mbox registers, just have > the MPU drive the mbox as usr_id=3? Yeah, the wkupm3 mbox usage is actually odd (an unfortunate result of the way the hardware is designed) and doesn't quite fit into the way a regular mailbox is used. I will summarize the following into the changelog in the next version to better explain the odd usage and avoid any confusion. A typical mbox device is used for communicating both to and fro with a slave processor using two fifos, and has two internal interrupts on the MPU that needs to be handled - a rx interrupt for processing in-bound messages and a tx-ready interrupt for handling mbox tx readiness. Since the WkupM3 cannot access the mailbox registers, the burden of handling the M3's Rx interrupt as well as those associated with the MPU lies with the wkupm3 mbox device code. As you can see, this usage is non-standard, and warrants that the code deal with both usr_ids since they would be different (0 for MPU and 3 for M3). The OMAP mailbox core does use the internal per-device ops, so it is actually better to plug-in custom ops for WkupM3, and deal with both usr_ids within the ops. I had actually started out with usr_id=3 but then I changed it to usr_id=0 so that it aligns with the DT documentation and the expected behavior that usr_id corresponds to the MPU, like for all mboxes on all other SoCs. > > Even that is a bit hacky IMO, but with proper documentation, is at least > better than the current approach IMO. Even with approach you are suggesting, you still need to have specific ops since the MPU interrupt handling code needs to deal with two usr_ids for handling MPU Tx and WkupM3 Rx interrupt. > > That should at least get rid of the need to customize the IRQ management > functions of mailbox-omap2.c. After that, the M3 driver could then > just do: > > omap_mbox_enable_irq() > omap_mbox_msg_send() > omap_mbox_disable_irq() This is infact how it was done in the previous AM335 PM suspend version [1] along with another API to read-back the message put in the Tx queue [2]. We do not want to add any new users of omap_mbox_enable_irq() or omap_mbox_disable_irq() API, since these will not fit into the generic mailbox framework. The actual IRQ raised in the M3's NVIC would be handled by M3, so the entire functionality of enabling, disabling the M3's mailbox Rx interrupt and reading back the message is handled in the omap_mbox_msg_send() without having to expose any new additional API. regards Suman > > and you could get rid of the rest of the custom functions in > mailbox-omap2.c also. > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/129519.html [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/129504.html
Suman Anna <s-anna@ti.com> writes: [...] > Yeah, the wkupm3 mbox usage is actually odd (an unfortunate result of > the way the hardware is designed) heh, "design" is a generous term for this hardware. ;) > and doesn't quite fit into the way a regular mailbox is used. I will > summarize the following into the changelog in the next version to > better explain the odd usage and avoid any confusion. > > A typical mbox device is used for communicating both to and fro with a > slave processor using two fifos, and has two internal interrupts on the > MPU that needs to be handled - a rx interrupt for processing in-bound > messages but there are no inbound message, so we don't care about that interrupt. > and a tx-ready interrupt for handling mbox tx readiness. Yes, this is where it gets tricky. However, in the case of AM33xx, the MPU doesn't care about RX interrupts and the M3 doesn't care about TX interrupts. hmm > Since the WkupM3 cannot access the mailbox registers, the burden of > handling the M3's Rx interrupt as well as those associated with the > MPU lies with the wkupm3 mbox device code. As you can see, this usage > is non-standard, and warrants that the code deal with both usr_ids > since they would be different (0 for MPU and 3 for M3). Another possibility would be to allow configurability over which usr_id is used for RX and which usr_id is used for TX. IOW, have the DT binding have a usr_id_tx and and optional usr_id_rx. If usr_id_rx isn't present (for most normal users), it uses usr_id_tx. For AM33xx, we'd use usr_id_tx=0, usr_id_rx=3. That would allow you to get rid of the overrides for the IRQ functions, no? But as I think about it, this id a bit yucky too. Yet another possiblity would be to use the DT to define 2 mailboxes. One "normal" one for control of the MPU's view (usr_id=0) and one dummy one (usr_id=3) for control of the M3's view of the world. Since Linux needs to control both, just define them both in the DT for linux control, and drive them from the M3 driver: mbox_mpu = omap_mbox_get(<normal one>) mbox_m3 = omap_mbox_get(<M3 one>) omap_mbox_enable_irq(mbox_m3, IRQ_RX) omap_mbox_msg_send(mbox_mpu, msg) omap_mbox_disable_irq(mbox_m3, IRQ_RX) omap_mbox_fifo_readback(mbox_mpu) /* API from earlier patch */ omap_mbox_ack_irq(mbox_m3) /* this would need to be added/exported */ AFAICS, other than the new APIs to export, this wouldn't require any additional M3 quirks in the mailbox driver. To me, what's important here is that the quirkiness of the M3 setup remains contained in the M3 driver (where it should be), not hidden in the mailbox driver, when it has nothing to do with the mailbox hardware. > The OMAP mailbox core does use the internal per-device ops, so it is > actually better to plug-in custom ops for WkupM3, and deal with both > usr_ids within the ops. This is where I disagree. The M3 brokenness should be contained in the M3 driver, not the mailbox driver. > I had actually started out with usr_id=3 but > then I changed it to usr_id=0 so that it aligns with the DT > documentation Which DT documentation? I don't see how the DT documentation forces you into any usr_id choice. > and the expected behavior that usr_id corresponds to the MPU, like for > all mboxes on all other SoCs. For AM33xx, I think you can throw out any ideas of "expected behavior". ;) Using usr_id=3 as described above along with some documentation in the .dts would go a long ways. >> >> Even that is a bit hacky IMO, but with proper documentation, is at least >> better than the current approach IMO. > > Even with approach you are suggesting, you still need to have specific > ops since the MPU interrupt handling code needs to deal with two usr_ids > for handling MPU Tx and WkupM3 Rx interrupt. Not if you actually define 2 mailboxes in the DT and use them both from the driver as I describe above. >> >> That should at least get rid of the need to customize the IRQ management >> functions of mailbox-omap2.c. After that, the M3 driver could then >> just do: >> >> omap_mbox_enable_irq() >> omap_mbox_msg_send() >> omap_mbox_disable_irq() > > This is infact how it was done in the previous AM335 PM suspend version > [1] along with another API to read-back the message put in the Tx queue > [2]. Except that the previous version was doing the readback/flush in the txev_handler of the M3 driver instead of in the message send like the current version (and my proposed one above.) > We do not want to add any new users of omap_mbox_enable_irq() or > omap_mbox_disable_irq() API, since these will not fit into the generic > mailbox framework. What generic mailbox framework? The only mailbox API I see currently is omap_mbox_*. Assuming there's a generic one coming, how will a generic mailbox framework work without the enable/disable IRQ? What other method would there be for determining the receiver of the message? Whatever that new generic API is for that is, it would have to use the omap_mbox_[enable|disable]_irq() APIs internally, so they are those not going away, so I don't see why you can't add more users. IOW, any users of omap_mbox_[enable|disable]_irq() would be converted to the new API for configuring the message receiver. > The actual IRQ raised in the M3's NVIC would be > handled by M3, so the entire functionality of enabling, disabling the > M3's mailbox Rx interrupt and reading back the message is handled in the > omap_mbox_msg_send() without having to expose any new additional API. So the fundamental difference we have here is that you believe exposing a new API is wrong and I believe hiding non-mailbox related quirks of the M3 inside the mailbox driver is wrong. I've tried to argue above why I think adding the API is not a big deal. At least, it's not as big a deal (to me) as hiding M3 quirks inside the mailbox driver. Kevin
Kevin, Sorry, I couldn't get back earlier due to long weekend. On 08/29/2013 11:57 AM, Kevin Hilman wrote: > Suman Anna <s-anna@ti.com> writes: > > [...] > >> Yeah, the wkupm3 mbox usage is actually odd (an unfortunate result of >> the way the hardware is designed) > > heh, "design" is a generous term for this hardware. ;) > >> and doesn't quite fit into the way a regular mailbox is used. I will >> summarize the following into the changelog in the next version to >> better explain the odd usage and avoid any confusion. >> >> A typical mbox device is used for communicating both to and fro with a >> slave processor using two fifos, and has two internal interrupts on the >> MPU that needs to be handled - a rx interrupt for processing in-bound >> messages > > but there are no inbound message, so we don't care about that > interrupt. > >> and a tx-ready interrupt for handling mbox tx readiness. > > Yes, this is where it gets tricky. However, in the case of AM33xx, the > MPU doesn't care about RX interrupts and the M3 doesn't care about TX > interrupts. hmm Correct, MPU still needs to care about the Tx interrupt though. > >> Since the WkupM3 cannot access the mailbox registers, the burden of >> handling the M3's Rx interrupt as well as those associated with the >> MPU lies with the wkupm3 mbox device code. As you can see, this usage >> is non-standard, and warrants that the code deal with both usr_ids >> since they would be different (0 for MPU and 3 for M3). > > Another possibility would be to allow configurability over which usr_id > is used for RX and which usr_id is used for TX. IOW, have the DT > binding have a usr_id_tx and and optional usr_id_rx. If usr_id_rx isn't > present (for most normal users), it uses usr_id_tx. For AM33xx, we'd > use usr_id_tx=0, usr_id_rx=3. > > That would allow you to get rid of the overrides for the IRQ functions, > no? But as I think about it, this id a bit yucky too. > > Yet another possiblity would be to use the DT to define 2 mailboxes. > One "normal" one for control of the MPU's view (usr_id=0) and one dummy > one (usr_id=3) for control of the M3's view of the world. Since Linux > needs to control both, just define them both in the DT for linux > control, and drive them from the M3 driver: I like the idea of having the ability to define a omap mailbox as rx-only or tx-only or both, rather than defining two usr_ids. So far, for all the IPC communication between processors, we always needed a pair since we only had a single instance and the fifos are unidirectional. For DRA7xx, where we have multiple instances, this should allow flexibility to pick different fifos from different instances. I will redo the patches to support this feature in general. > > mbox_mpu = omap_mbox_get(<normal one>) > mbox_m3 = omap_mbox_get(<M3 one>) > > omap_mbox_enable_irq(mbox_m3, IRQ_RX) > omap_mbox_msg_send(mbox_mpu, msg) > omap_mbox_disable_irq(mbox_m3, IRQ_RX) > omap_mbox_fifo_readback(mbox_mpu) /* API from earlier patch */ > omap_mbox_ack_irq(mbox_m3) /* this would need to be added/exported */ If you look at this sequence, there's an inherent understanding of the mailbox behavior. The enable and disable are used to suppress additional interrupts, since the mailbox IP would always trigger an interrupt as long as there is a message within the FIFO. The fifo_readback is clearing the message, but it need not be done in the response message as was done in the previous PM series. The WkupM3 driver has to primarily just trigger an interrupt. > > AFAICS, other than the new APIs to export, this wouldn't require any > additional M3 quirks in the mailbox driver. The omap_mbox_fifo_readback is not a generic mailbox API and actually makes the mailbox driver complex since the driver does support tx buffering as well. This is the case irrespective of the framework. The omap_mbox_ack_irq is also not generic, since the irq management usually lies with the mailbox driver, and the clients need not be bothered. > > To me, what's important here is that the quirkiness of the M3 setup > remains contained in the M3 driver (where it should be), not hidden in > the mailbox driver, when it has nothing to do with the mailbox hardware. > >> The OMAP mailbox core does use the internal per-device ops, so it is >> actually better to plug-in custom ops for WkupM3, and deal with both >> usr_ids within the ops. > > This is where I disagree. The M3 brokenness should be contained in the > M3 driver, not the mailbox driver. > >> I had actually started out with usr_id=3 but >> then I changed it to usr_id=0 so that it aligns with the DT >> documentation > > Which DT documentation? Documentation in Patch 2 [3] of this series, where I add the DT binding documentation and the guidelines on what each field means. > I don't see how the DT documentation forces you into any usr_id choice. > >> and the expected behavior that usr_id corresponds to the MPU, like for >> all mboxes on all other SoCs. > > For AM33xx, I think you can throw out any ideas of "expected > behavior". ;) Yep, seems like a common theme indeed ;). > Using usr_id=3 as described above along with some > documentation in the .dts would go a long ways. > >>> >>> Even that is a bit hacky IMO, but with proper documentation, is at least >>> better than the current approach IMO. >> >> Even with approach you are suggesting, you still need to have specific >> ops since the MPU interrupt handling code needs to deal with two usr_ids >> for handling MPU Tx and WkupM3 Rx interrupt. > > Not if you actually define 2 mailboxes in the DT and use them both from > the driver as I describe above. > >>> >>> That should at least get rid of the need to customize the IRQ management >>> functions of mailbox-omap2.c. After that, the M3 driver could then >>> just do: >>> >>> omap_mbox_enable_irq() >>> omap_mbox_msg_send() >>> omap_mbox_disable_irq() >> >> This is infact how it was done in the previous AM335 PM suspend version >> [1] along with another API to read-back the message put in the Tx queue >> [2]. > > Except that the previous version was doing the readback/flush in the > txev_handler of the M3 driver instead of in the message send like the > current version (and my proposed one above.) > >> We do not want to add any new users of omap_mbox_enable_irq() or >> omap_mbox_disable_irq() API, since these will not fit into the generic >> mailbox framework. > > What generic mailbox framework? The only mailbox API I see currently is > omap_mbox_*. Yeah, there's a generic one still under development [4]. You can look through the mailbox_client.h for the generic API that would be used by a client. I started out the support for AM335 mailbox on top of it originally, the preliminary dev work including the OMAP adaptation is hosted at [5] (using usr_id 3) & [6] (using usr_id 0), but decided to post this to break the dependency and unblock the PM series. I will adapt all existing OMAP mbox clients at once the framework shows up. > Assuming there's a generic one coming, how will a generic > mailbox framework work without the enable/disable IRQ? The IRQ handling would be internal to the driver, the clients would not be required to manage them. The framework provides API for the clients to send messages, and receive callbacks for rx messages. > What other method would there be for determining the receiver of the message? A client would operate on a mbox device, so the knowledge of receiver is automatic. > Whatever that new generic API is for that is, it would have to use the > omap_mbox_[enable|disable]_irq() APIs internally, so they are those not > going away, so I don't see why you can't add more users. At present, DSP/Bridge is the only user of this API. The remoteproc infrastructure doesn't use the omap_mbox_[enable|disable]_irq(), and we want all clients to use the generic API in the long term. > IOW, any users of > omap_mbox_[enable|disable]_irq() would be converted to the new API for > configuring the message receiver. For now, these would be exposed until I rework the DSP/Bridge to remove these. > >> The actual IRQ raised in the M3's NVIC would be >> handled by M3, so the entire functionality of enabling, disabling the >> M3's mailbox Rx interrupt and reading back the message is handled in the >> omap_mbox_msg_send() without having to expose any new additional API. > > So the fundamental difference we have here is that you believe exposing > a new API is wrong and I believe hiding non-mailbox related quirks of > the M3 inside the mailbox driver is wrong. Yeah, the mailbox IP is external to both the MPU and the WkupM3 IPs, with a dedicated mailbox driver dealing with the IP. It is definitely debatable as to who should be responsible for the AM335 PM usecase. The WkupM3 driver is currently using mailbox to raise the interrupt, and if it were using a different interrupt trigger source, then it would have to use that respective driver's API. The WkupM3 usage of mailbox is odd, no matter how we look at it. My primary reasoning is that we want all clients to migrate to the generic API, so we do not want to be adding/exposing (at this point) driver-specific API for dealing with quirks. My approach was to deal with the wkupm3 interaction/quirks as a device-specific ops within the driver for this reason. regards Suman [3] http://marc.info/?l=linux-omap&m=137582562717980&w=2 [4] http://marc.info/?l=linux-kernel&m=136782510209473&w=2 [5] https://github.com/sumananna/mailbox/commits/jassiv3-3.11rc1-am335-mbox-suspend [6] https://github.com/sumananna/mailbox/commits/jassiv3-3.11rc3-am335-mbox-suspend-v1
Suman Anna <s-anna@ti.com> writes: > Kevin, > > Sorry, I couldn't get back earlier due to long weekend. > > On 08/29/2013 11:57 AM, Kevin Hilman wrote: >> Suman Anna <s-anna@ti.com> writes: >> >> [...] >> >>> Yeah, the wkupm3 mbox usage is actually odd (an unfortunate result of >>> the way the hardware is designed) >> >> heh, "design" is a generous term for this hardware. ;) >> >>> and doesn't quite fit into the way a regular mailbox is used. I will >>> summarize the following into the changelog in the next version to >>> better explain the odd usage and avoid any confusion. >>> >>> A typical mbox device is used for communicating both to and fro with a >>> slave processor using two fifos, and has two internal interrupts on the >>> MPU that needs to be handled - a rx interrupt for processing in-bound >>> messages >> >> but there are no inbound message, so we don't care about that >> interrupt. >> >>> and a tx-ready interrupt for handling mbox tx readiness. >> >> Yes, this is where it gets tricky. However, in the case of AM33xx, the >> MPU doesn't care about RX interrupts and the M3 doesn't care about TX >> interrupts. hmm > > Correct, MPU still needs to care about the Tx interrupt though. > >> >>> Since the WkupM3 cannot access the mailbox registers, the burden of >>> handling the M3's Rx interrupt as well as those associated with the >>> MPU lies with the wkupm3 mbox device code. As you can see, this usage >>> is non-standard, and warrants that the code deal with both usr_ids >>> since they would be different (0 for MPU and 3 for M3). >> >> Another possibility would be to allow configurability over which usr_id >> is used for RX and which usr_id is used for TX. IOW, have the DT >> binding have a usr_id_tx and and optional usr_id_rx. If usr_id_rx isn't >> present (for most normal users), it uses usr_id_tx. For AM33xx, we'd >> use usr_id_tx=0, usr_id_rx=3. >> >> That would allow you to get rid of the overrides for the IRQ functions, >> no? But as I think about it, this id a bit yucky too. >> >> Yet another possiblity would be to use the DT to define 2 mailboxes. >> One "normal" one for control of the MPU's view (usr_id=0) and one dummy >> one (usr_id=3) for control of the M3's view of the world. Since Linux >> needs to control both, just define them both in the DT for linux >> control, and drive them from the M3 driver: > > I like the idea of having the ability to define a omap mailbox as > rx-only or tx-only or both, rather than defining two usr_ids. So far, > for all the IPC communication between processors, we always needed a > pair since we only had a single instance and the fifos are > unidirectional. For DRA7xx, where we have multiple instances, this > should allow flexibility to pick different fifos from different > instances. I will redo the patches to support this feature in general. Sounds good. Does that mean you plan to define a TX only mailbox for the MPU view (usr_id=0) and an RX only mailbox for the M3 view (usr_id=3)? >> >> mbox_mpu = omap_mbox_get(<normal one>) >> mbox_m3 = omap_mbox_get(<M3 one>) >> >> omap_mbox_enable_irq(mbox_m3, IRQ_RX) >> omap_mbox_msg_send(mbox_mpu, msg) >> omap_mbox_disable_irq(mbox_m3, IRQ_RX) >> omap_mbox_fifo_readback(mbox_mpu) /* API from earlier patch */ >> omap_mbox_ack_irq(mbox_m3) /* this would need to be added/exported */ > > If you look at this sequence, there's an inherent understanding of the > mailbox behavior. The enable and disable are used to suppress additional > interrupts, since the mailbox IP would always trigger an interrupt as > long as there is a message within the FIFO. The fifo_readback is > clearing the message, but it need not be done in the response message as > was done in the previous PM series. The WkupM3 driver has to primarily > just trigger an interrupt. OK, I didn't fully understand the need for the readback everytime either, but was just following what was proposed in this series. >> >> AFAICS, other than the new APIs to export, this wouldn't require any >> additional M3 quirks in the mailbox driver. > > The omap_mbox_fifo_readback is not a generic mailbox API and actually > makes the mailbox driver complex since the driver does support tx > buffering as well. I think this could be solved by just having some quirks flags or similar for each mbox device. e.g. a quirk that says "read back message immediately after sending" or something like that. Or maybe an "IRQ only" mode that implies the readback to keep the fifo empty but still triggers the IRQ. > This is the case irrespective of the framework. The > omap_mbox_ack_irq is also not generic, since the irq management usually > lies with the mailbox driver, and the clients need not be bothered. Similarily, you could have another "quirk" flag such that the disable_irq also does an ack. Or maybe it should already do this? >> >> To me, what's important here is that the quirkiness of the M3 setup >> remains contained in the M3 driver (where it should be), not hidden in >> the mailbox driver, when it has nothing to do with the mailbox hardware. >> >>> The OMAP mailbox core does use the internal per-device ops, so it is >>> actually better to plug-in custom ops for WkupM3, and deal with both >>> usr_ids within the ops. >> >> This is where I disagree. The M3 brokenness should be contained in the >> M3 driver, not the mailbox driver. >> >>> I had actually started out with usr_id=3 but >>> then I changed it to usr_id=0 so that it aligns with the DT >>> documentation >> >> Which DT documentation? > > Documentation in Patch 2 [3] of this series, where I add the DT binding > documentation and the guidelines on what each field means. > >> I don't see how the DT documentation forces you into any usr_id choice. >> >>> and the expected behavior that usr_id corresponds to the MPU, like for >>> all mboxes on all other SoCs. >> >> For AM33xx, I think you can throw out any ideas of "expected >> behavior". ;) > > Yep, seems like a common theme indeed ;). > >> Using usr_id=3 as described above along with some >> documentation in the .dts would go a long ways. >> >>>> >>>> Even that is a bit hacky IMO, but with proper documentation, is at least >>>> better than the current approach IMO. >>> >>> Even with approach you are suggesting, you still need to have specific >>> ops since the MPU interrupt handling code needs to deal with two usr_ids >>> for handling MPU Tx and WkupM3 Rx interrupt. >> >> Not if you actually define 2 mailboxes in the DT and use them both from >> the driver as I describe above. >> >>>> >>>> That should at least get rid of the need to customize the IRQ management >>>> functions of mailbox-omap2.c. After that, the M3 driver could then >>>> just do: >>>> >>>> omap_mbox_enable_irq() >>>> omap_mbox_msg_send() >>>> omap_mbox_disable_irq() >>> >>> This is infact how it was done in the previous AM335 PM suspend version >>> [1] along with another API to read-back the message put in the Tx queue >>> [2]. >> >> Except that the previous version was doing the readback/flush in the >> txev_handler of the M3 driver instead of in the message send like the >> current version (and my proposed one above.) >> >>> We do not want to add any new users of omap_mbox_enable_irq() or >>> omap_mbox_disable_irq() API, since these will not fit into the generic >>> mailbox framework. >> >> What generic mailbox framework? The only mailbox API I see currently is >> omap_mbox_*. > > Yeah, there's a generic one still under development [4]. You can look > through the mailbox_client.h for the generic API that would be used by a > client. I started out the support for AM335 mailbox on top of it > originally, the preliminary dev work including the OMAP adaptation is > hosted at [5] (using usr_id 3) & [6] (using usr_id 0), but decided to > post this to break the dependency and unblock the PM series. I will > adapt all existing OMAP mbox clients at once the framework shows up. > >> Assuming there's a generic one coming, how will a generic >> mailbox framework work without the enable/disable IRQ? > > The IRQ handling would be internal to the driver, the clients would > not be required to manage them. The framework provides API for the > clients to send messages, and receive callbacks for rx messages. > >> What other method would there be for determining the receiver of the message? > > A client would operate on a mbox device, so the knowledge of receiver is > automatic. > >> Whatever that new generic API is for that is, it would have to use the >> omap_mbox_[enable|disable]_irq() APIs internally, so they are those not >> going away, so I don't see why you can't add more users. > > At present, DSP/Bridge is the only user of this API. The remoteproc > infrastructure doesn't use the omap_mbox_[enable|disable]_irq(), and we > want all clients to use the generic API in the long term. OK, then I think the only solution is to add some quirks/modes to the generic code so it can handle the case where client has to manage both the sender and the receiver. >> IOW, any users of >> omap_mbox_[enable|disable]_irq() would be converted to the new API for >> configuring the message receiver. > > For now, these would be exposed until I rework the DSP/Bridge to remove > these. > >> >>> The actual IRQ raised in the M3's NVIC would be >>> handled by M3, so the entire functionality of enabling, disabling the >>> M3's mailbox Rx interrupt and reading back the message is handled in the >>> omap_mbox_msg_send() without having to expose any new additional API. >> >> So the fundamental difference we have here is that you believe exposing >> a new API is wrong and I believe hiding non-mailbox related quirks of >> the M3 inside the mailbox driver is wrong. > > Yeah, the mailbox IP is external to both the MPU and the WkupM3 IPs, > with a dedicated mailbox driver dealing with the IP. It is definitely > debatable as to who should be responsible for the AM335 PM usecase. The > WkupM3 driver is currently using mailbox to raise the interrupt, and if > it were using a different interrupt trigger source, then it would have > to use that respective driver's API. The WkupM3 usage of mailbox is odd, > no matter how we look at it. Precisely. IIUC, AM43xx has the ability to trigger an interrupt by a write to the control module, which eliminates the need for using the mailbox hardware all together. Is that correct? > My primary reasoning is that we want all clients to migrate to the > generic API, so we do not want to be adding/exposing (at this point) > driver-specific API for dealing with quirks. My approach was to deal > with the wkupm3 interaction/quirks as a device-specific ops within the > driver for this reason. I agree that's the right motiviation. However if a "generic" driver doesn't handle existing uses on existing hardware, then it's probably not generic enough. Could you explore adding some more general quirks/modes to the generic driver such that it could handle the AM335x brokenness^Wpeculiarity. Kevin
Kevin, >> >> Sorry, I couldn't get back earlier due to long weekend. >> >> On 08/29/2013 11:57 AM, Kevin Hilman wrote: >>> Suman Anna <s-anna@ti.com> writes: >>> >>> [...] >>> >>>> Yeah, the wkupm3 mbox usage is actually odd (an unfortunate result of >>>> the way the hardware is designed) >>> >>> heh, "design" is a generous term for this hardware. ;) >>> >>>> and doesn't quite fit into the way a regular mailbox is used. I will >>>> summarize the following into the changelog in the next version to >>>> better explain the odd usage and avoid any confusion. >>>> >>>> A typical mbox device is used for communicating both to and fro with a >>>> slave processor using two fifos, and has two internal interrupts on the >>>> MPU that needs to be handled - a rx interrupt for processing in-bound >>>> messages >>> >>> but there are no inbound message, so we don't care about that >>> interrupt. >>> >>>> and a tx-ready interrupt for handling mbox tx readiness. >>> >>> Yes, this is where it gets tricky. However, in the case of AM33xx, the >>> MPU doesn't care about RX interrupts and the M3 doesn't care about TX >>> interrupts. hmm >> >> Correct, MPU still needs to care about the Tx interrupt though. >> >>> >>>> Since the WkupM3 cannot access the mailbox registers, the burden of >>>> handling the M3's Rx interrupt as well as those associated with the >>>> MPU lies with the wkupm3 mbox device code. As you can see, this usage >>>> is non-standard, and warrants that the code deal with both usr_ids >>>> since they would be different (0 for MPU and 3 for M3). >>> >>> Another possibility would be to allow configurability over which usr_id >>> is used for RX and which usr_id is used for TX. IOW, have the DT >>> binding have a usr_id_tx and and optional usr_id_rx. If usr_id_rx isn't >>> present (for most normal users), it uses usr_id_tx. For AM33xx, we'd >>> use usr_id_tx=0, usr_id_rx=3. >>> >>> That would allow you to get rid of the overrides for the IRQ functions, >>> no? But as I think about it, this id a bit yucky too. >>> >>> Yet another possiblity would be to use the DT to define 2 mailboxes. >>> One "normal" one for control of the MPU's view (usr_id=0) and one dummy >>> one (usr_id=3) for control of the M3's view of the world. Since Linux >>> needs to control both, just define them both in the DT for linux >>> control, and drive them from the M3 driver: >> >> I like the idea of having the ability to define a omap mailbox as >> rx-only or tx-only or both, rather than defining two usr_ids. So far, >> for all the IPC communication between processors, we always needed a >> pair since we only had a single instance and the fifos are >> unidirectional. For DRA7xx, where we have multiple instances, this >> should allow flexibility to pick different fifos from different >> instances. I will redo the patches to support this feature in general. > > Sounds good. > > Does that mean you plan to define a TX only mailbox for the MPU view > (usr_id=0) and an RX only mailbox for the M3 view (usr_id=3)? I will see how this pans out for the M3 view, because mainly from the API point of view, we will have one to send, and the receive is usually a callback. > >>> >>> mbox_mpu = omap_mbox_get(<normal one>) >>> mbox_m3 = omap_mbox_get(<M3 one>) >>> >>> omap_mbox_enable_irq(mbox_m3, IRQ_RX) >>> omap_mbox_msg_send(mbox_mpu, msg) >>> omap_mbox_disable_irq(mbox_m3, IRQ_RX) >>> omap_mbox_fifo_readback(mbox_mpu) /* API from earlier patch */ >>> omap_mbox_ack_irq(mbox_m3) /* this would need to be added/exported */ >> >> If you look at this sequence, there's an inherent understanding of the >> mailbox behavior. The enable and disable are used to suppress additional >> interrupts, since the mailbox IP would always trigger an interrupt as >> long as there is a message within the FIFO. The fifo_readback is >> clearing the message, but it need not be done in the response message as >> was done in the previous PM series. The WkupM3 driver has to primarily >> just trigger an interrupt. > > OK, I didn't fully understand the need for the readback everytime > either, but was just following what was proposed in this series. Yeah, the reads are needed to take out the message, since the h/w has a fifo, and as long as messages are present (number doesn't matter) and mbox irq enabled, the interrupt keeps getting triggered. Reading the message, and acking the interrupt within the mailbox IP are the typical actions performed on an mbox interrupt. This patch essentially was doing all the above steps with the send message API, so at the end of it, an interrupt is raised within the M3's NVIC, and the firmware deals with performing the necessary steps based on the message in IPC Control registers. > >>> >>> AFAICS, other than the new APIs to export, this wouldn't require any >>> additional M3 quirks in the mailbox driver. >> >> The omap_mbox_fifo_readback is not a generic mailbox API and actually >> makes the mailbox driver complex since the driver does support tx >> buffering as well. > > I think this could be solved by just having some quirks flags or similar > for each mbox device. e.g. a quirk that says "read back message > immediately after sending" or something like that. Or maybe an "IRQ > only" mode that implies the readback to keep the fifo empty but still > triggers the IRQ. > >> This is the case irrespective of the framework. The >> omap_mbox_ack_irq is also not generic, since the irq management usually >> lies with the mailbox driver, and the clients need not be bothered. > > Similarily, you could have another "quirk" flag such that the > disable_irq also does an ack. Or maybe it should already do this? Yeah, I will look into if a quirk flag can be plumbed into regular function ops. My initial development was deciding the quirk based on an string compare, and it was much simpler to do this decision at the time of plugging in the ops, so that other SoCs are not at all affected by having to check for the quirk. > >>> >>> To me, what's important here is that the quirkiness of the M3 setup >>> remains contained in the M3 driver (where it should be), not hidden in >>> the mailbox driver, when it has nothing to do with the mailbox hardware. >>> >>>> The OMAP mailbox core does use the internal per-device ops, so it is >>>> actually better to plug-in custom ops for WkupM3, and deal with both >>>> usr_ids within the ops. >>> >>> This is where I disagree. The M3 brokenness should be contained in the >>> M3 driver, not the mailbox driver. >>> >>>> I had actually started out with usr_id=3 but >>>> then I changed it to usr_id=0 so that it aligns with the DT >>>> documentation >>> >>> Which DT documentation? >> >> Documentation in Patch 2 [3] of this series, where I add the DT binding >> documentation and the guidelines on what each field means. >> >>> I don't see how the DT documentation forces you into any usr_id choice. >>> >>>> and the expected behavior that usr_id corresponds to the MPU, like for >>>> all mboxes on all other SoCs. >>> >>> For AM33xx, I think you can throw out any ideas of "expected >>> behavior". ;) >> >> Yep, seems like a common theme indeed ;). >> >>> Using usr_id=3 as described above along with some >>> documentation in the .dts would go a long ways. >>> >>>>> >>>>> Even that is a bit hacky IMO, but with proper documentation, is at least >>>>> better than the current approach IMO. >>>> >>>> Even with approach you are suggesting, you still need to have specific >>>> ops since the MPU interrupt handling code needs to deal with two usr_ids >>>> for handling MPU Tx and WkupM3 Rx interrupt. >>> >>> Not if you actually define 2 mailboxes in the DT and use them both from >>> the driver as I describe above. >>> >>>>> >>>>> That should at least get rid of the need to customize the IRQ management >>>>> functions of mailbox-omap2.c. After that, the M3 driver could then >>>>> just do: >>>>> >>>>> omap_mbox_enable_irq() >>>>> omap_mbox_msg_send() >>>>> omap_mbox_disable_irq() >>>> >>>> This is infact how it was done in the previous AM335 PM suspend version >>>> [1] along with another API to read-back the message put in the Tx queue >>>> [2]. >>> >>> Except that the previous version was doing the readback/flush in the >>> txev_handler of the M3 driver instead of in the message send like the >>> current version (and my proposed one above.) >>> >>>> We do not want to add any new users of omap_mbox_enable_irq() or >>>> omap_mbox_disable_irq() API, since these will not fit into the generic >>>> mailbox framework. >>> >>> What generic mailbox framework? The only mailbox API I see currently is >>> omap_mbox_*. >> >> Yeah, there's a generic one still under development [4]. You can look >> through the mailbox_client.h for the generic API that would be used by a >> client. I started out the support for AM335 mailbox on top of it >> originally, the preliminary dev work including the OMAP adaptation is >> hosted at [5] (using usr_id 3) & [6] (using usr_id 0), but decided to >> post this to break the dependency and unblock the PM series. I will >> adapt all existing OMAP mbox clients at once the framework shows up. >> >>> Assuming there's a generic one coming, how will a generic >>> mailbox framework work without the enable/disable IRQ? >> >> The IRQ handling would be internal to the driver, the clients would >> not be required to manage them. The framework provides API for the >> clients to send messages, and receive callbacks for rx messages. >> >>> What other method would there be for determining the receiver of the message? >> >> A client would operate on a mbox device, so the knowledge of receiver is >> automatic. >> >>> Whatever that new generic API is for that is, it would have to use the >>> omap_mbox_[enable|disable]_irq() APIs internally, so they are those not >>> going away, so I don't see why you can't add more users. >> >> At present, DSP/Bridge is the only user of this API. The remoteproc >> infrastructure doesn't use the omap_mbox_[enable|disable]_irq(), and we >> want all clients to use the generic API in the long term. > > OK, then I think the only solution is to add some quirks/modes to the > generic code so it can handle the case where client has to manage both > the sender and the receiver. > >>> IOW, any users of >>> omap_mbox_[enable|disable]_irq() would be converted to the new API for >>> configuring the message receiver. >> >> For now, these would be exposed until I rework the DSP/Bridge to remove >> these. >> >>> >>>> The actual IRQ raised in the M3's NVIC would be >>>> handled by M3, so the entire functionality of enabling, disabling the >>>> M3's mailbox Rx interrupt and reading back the message is handled in the >>>> omap_mbox_msg_send() without having to expose any new additional API. >>> >>> So the fundamental difference we have here is that you believe exposing >>> a new API is wrong and I believe hiding non-mailbox related quirks of >>> the M3 inside the mailbox driver is wrong. >> >> Yeah, the mailbox IP is external to both the MPU and the WkupM3 IPs, >> with a dedicated mailbox driver dealing with the IP. It is definitely >> debatable as to who should be responsible for the AM335 PM usecase. The >> WkupM3 driver is currently using mailbox to raise the interrupt, and if >> it were using a different interrupt trigger source, then it would have >> to use that respective driver's API. The WkupM3 usage of mailbox is odd, >> no matter how we look at it. > > Precisely. > > IIUC, AM43xx has the ability to trigger an interrupt by a write to the > control module, which eliminates the need for using the mailbox hardware > all together. Is that correct? I am not sure, can't make out clearly from the early TRM drafts. But it would be great, if there is one. If the way the M3 interrupts the MPU is also a generic interrupt, then the IPC registers along with these two interrupts could be made into its own mailbox driver (would expect it to look something similar to the current pl320-ipc driver). > >> My primary reasoning is that we want all clients to migrate to the >> generic API, so we do not want to be adding/exposing (at this point) >> driver-specific API for dealing with quirks. My approach was to deal >> with the wkupm3 interaction/quirks as a device-specific ops within the >> driver for this reason. > > I agree that's the right motiviation. However if a "generic" driver > doesn't handle existing uses on existing hardware, then it's probably > not generic enough. > > Could you explore adding some more general quirks/modes to the generic > driver such that it could handle the AM335x brokenness^Wpeculiarity. The quirks would have to be handled within the OMAP mailbox driver, I doubt the generic core needs to know about these, but definitely I will explore this as I rework my patches. regards Suman
diff --git a/drivers/mailbox/mailbox-omap2.c b/drivers/mailbox/mailbox-omap2.c index fef18f4..00ac2a2 100644 --- a/drivers/mailbox/mailbox-omap2.c +++ b/drivers/mailbox/mailbox-omap2.c @@ -36,6 +36,8 @@ #define MAILBOX_IRQ_NEWMSG(m) (1 << (2 * (m))) #define MAILBOX_IRQ_NOTFULL(m) (1 << (2 * (m) + 1)) +#define AM33X_MBOX_WKUPM3_USR 3 + #define MBOX_REG_SIZE 0x120 #define OMAP4_MBOX_REG_SIZE 0x130 @@ -179,6 +181,57 @@ static int omap2_mbox_is_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) return (int)(enable & status & bit); } +static void wkupm3_mbox_enable_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) +{ + struct omap_mbox2_priv *p = mbox->priv; + u32 l, bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit; + unsigned long irqenable = ((irq == IRQ_RX) ? + OMAP4_MAILBOX_IRQENABLE(AM33X_MBOX_WKUPM3_USR) : p->irqenable); + + l = mbox_read_reg(mbox->parent, irqenable); + l |= bit; + mbox_write_reg(mbox->parent, l, irqenable); +} + +static void wkupm3_mbox_disable_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) +{ + struct omap_mbox2_priv *p = mbox->priv; + u32 bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit; + unsigned long irqdisable = ((irq == IRQ_RX) ? + OMAP4_MAILBOX_IRQENABLE_CLR(AM33X_MBOX_WKUPM3_USR) : p->irqdisable); + + mbox_write_reg(mbox->parent, bit, irqdisable); +} + +static void wkupm3_mbox_ack_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) +{ + struct omap_mbox2_priv *p = mbox->priv; + u32 bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit; + unsigned long irqstatus = ((irq == IRQ_RX) ? + OMAP4_MAILBOX_IRQSTATUS(AM33X_MBOX_WKUPM3_USR) : p->irqstatus); + + mbox_write_reg(mbox->parent, bit, irqstatus); + + /* Flush posted write for irq status to avoid spurious interrupts */ + mbox_read_reg(mbox->parent, irqstatus); +} + +static int wkupm3_mbox_is_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) +{ + struct omap_mbox2_priv *p = mbox->priv; + u32 bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit; + u32 enable, status; + + /* WkupM3 mailbox does not use a receive queue */ + if (irq == IRQ_RX) + return 0; + + enable = mbox_read_reg(mbox->parent, p->irqenable); + status = mbox_read_reg(mbox->parent, p->irqstatus); + + return (int)(enable & status & bit); +} + static void omap2_mbox_save_ctx(struct omap_mbox *mbox) { int i; @@ -215,6 +268,20 @@ static void omap2_mbox_restore_ctx(struct omap_mbox *mbox) } } +static void wkupm3_mbox_send_data(struct omap_mbox *mbox, mbox_msg_t msg) +{ + mbox_msg_t rmsg; + + /* enable the mbox Rx interrupt for WkupM3 only briefly */ + wkupm3_mbox_enable_irq(mbox, IRQ_RX); + omap2_mbox_fifo_write(mbox, msg); + wkupm3_mbox_disable_irq(mbox, IRQ_RX); + + /* read back the message and ack the interrupt on behalf of WkupM3 */ + rmsg = omap2_mbox_fifo_read(mbox); + wkupm3_mbox_ack_irq(mbox, IRQ_RX); +} + static struct omap_mbox_ops omap2_mbox_ops = { .startup = omap2_mbox_startup, .shutdown = omap2_mbox_shutdown, @@ -230,6 +297,21 @@ static struct omap_mbox_ops omap2_mbox_ops = { .restore_ctx = omap2_mbox_restore_ctx, }; +static struct omap_mbox_ops wkupm3_mbox_ops = { + .startup = omap2_mbox_startup, + .shutdown = omap2_mbox_shutdown, + .fifo_read = omap2_mbox_fifo_read, + .fifo_write = wkupm3_mbox_send_data, + .fifo_empty = omap2_mbox_fifo_empty, + .poll_for_space = omap2_mbox_poll_for_space, + .enable_irq = wkupm3_mbox_enable_irq, + .disable_irq = wkupm3_mbox_disable_irq, + .ack_irq = wkupm3_mbox_ack_irq, + .is_irq = wkupm3_mbox_is_irq, + .save_ctx = omap2_mbox_save_ctx, + .restore_ctx = omap2_mbox_restore_ctx, +}; + static const struct of_device_id omap_mailbox_of_match[] = { { .compatible = "ti,omap2-mailbox", @@ -387,7 +469,10 @@ static int omap2_mbox_probe(struct platform_device *pdev) mbox->priv = priv; mbox->parent = mdev; mbox->name = info->name; - mbox->ops = &omap2_mbox_ops; + if (!strcmp(mbox->name, "wkup_m3")) + mbox->ops = &wkupm3_mbox_ops; + else + mbox->ops = &omap2_mbox_ops; mbox->irq = platform_get_irq(pdev, info->irq_id); if (mbox->irq < 0) { ret = mbox->irq;
The WkupM3 mailbox used for triggering PM operations such as suspend and resume on AM33x/AM43x is special in that the M3 processor cannot access the mailbox registers. However, an interrupt is needed to be sent to request the M3 to perform a desired PM operation. This patch adds the support for this special mailbox through separate ops for this mailbox. These ops are designed to have the WkupM3's Rx interrupt programmed within the driver, during transmission of a message. The message is immediately read back and the internal mailbox interrupt acknowledged as well to avoid triggering any spurious interrupts to the M3. Signed-off-by: Suman Anna <s-anna@ti.com> --- drivers/mailbox/mailbox-omap2.c | 87 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-)