| Message ID | 20170504200539.27027-1-bjorn.andersson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/5] mailbox: Make startup and shutdown ops optional | expand |
On 04/05/17 21:05, Bjorn Andersson wrote: > Some mailbox hardware doesn't have to perform any additional operations > on startup of shutdown, so make these optional. > Quite handy. I can you this to get rid of empty shutdown that I added in[1] Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> -- Regards, Sudeep [1] https://lkml.org/lkml/2017/5/2/315 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 5, 2017 at 1:35 AM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > + > +static int qcom_apcs_ipc_send_data(struct mbox_chan *chan, void *data) > +{ > + struct qcom_apcs_ipc *apcs = container_of(chan->mbox, > + struct qcom_apcs_ipc, mbox); > + unsigned long idx = (unsigned long)chan->con_priv; > + > + writel(BIT(idx), apcs->base + apcs->offset); > + When/how does this bit get ever cleared again? You may want to add last_tx_done() callback to check if this bit is cleared before you can send the next interrupt. And set txdone_poll/irq accordingly. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 05 May 03:26 PDT 2017, Jassi Brar wrote: > On Fri, May 5, 2017 at 1:35 AM, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > + > > +static int qcom_apcs_ipc_send_data(struct mbox_chan *chan, void *data) > > +{ > > + struct qcom_apcs_ipc *apcs = container_of(chan->mbox, > > + struct qcom_apcs_ipc, mbox); > > + unsigned long idx = (unsigned long)chan->con_priv; > > + > > + writel(BIT(idx), apcs->base + apcs->offset); > > + > When/how does this bit get ever cleared again? > You may want to add last_tx_done() callback to check if this bit is > cleared before you can send the next interrupt. And set > txdone_poll/irq accordingly. > It's a write-only register, writing a bit fires off an edge triggered interrupt on the specific remote processor, which will ack the associated IRQ status and handle the interrupt. As the "message" is just a notification to the other side that it needs to act on "something", there's no harm in notifying it multiple times before it has a chance to ack the IRQ and a write after that will be seen as a separate interrupt. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, May 6, 2017 at 12:07 AM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Fri 05 May 03:26 PDT 2017, Jassi Brar wrote: > >> On Fri, May 5, 2017 at 1:35 AM, Bjorn Andersson >> <bjorn.andersson@linaro.org> wrote: >> >> > + >> > +static int qcom_apcs_ipc_send_data(struct mbox_chan *chan, void *data) >> > +{ >> > + struct qcom_apcs_ipc *apcs = container_of(chan->mbox, >> > + struct qcom_apcs_ipc, mbox); >> > + unsigned long idx = (unsigned long)chan->con_priv; >> > + >> > + writel(BIT(idx), apcs->base + apcs->offset); >> > + >> When/how does this bit get ever cleared again? >> You may want to add last_tx_done() callback to check if this bit is >> cleared before you can send the next interrupt. And set >> txdone_poll/irq accordingly. >> > > It's a write-only register, writing a bit fires off an edge triggered > interrupt on the specific remote processor, which will ack the > associated IRQ status and handle the interrupt. > > As the "message" is just a notification to the other side that it needs > to act on "something", there's no harm in notifying it multiple times > before it has a chance to ack the IRQ and a write after that will be > seen as a separate interrupt. > What causes it to return to '0'? I think the driver should wait for it to become 0 before writing 1. For example, the protocol has a command that says to remote cpu to increase the voltage supply by 0.1v. This command is filled in a structure and laid out in the shared memory before you ring the 'doorbell'. In this situation you don't want the remote cpu to act twice on the same command. Also for a new command, you don't want to overwrite the last command packet before remote cpu has consumed it. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/5/2017 2:22 PM, Jassi Brar wrote: > On Sat, May 6, 2017 at 1:23 AM, Jeffrey Hugo <jhugo@codeaurora.org> wrote: >> On 5/5/2017 1:22 PM, Jassi Brar wrote: >>> >>> On Sat, May 6, 2017 at 12:07 AM, Bjorn Andersson >>> <bjorn.andersson@linaro.org> wrote: >>>> >> >> There is no way to determine if the remote processor has observed a message, >> that does not involve pretty trivial race conditions. >> > Thanks for chiming in. > How is it supposed to work if a client queues more than one request? > How do you know when it's ok to overwrite the FIFO and send the next > command? > Usually if h/w doesn't indicate, we cook up some ack packet for each > command. Otherwise the protocol seems badly broken. > > If there is really nothing that can be done to check delivery of a > message, I'll pick the driver as such. Best of luck :) The protocol is designed so that we don't need an ack, or confirmation of delivery. Such details are left to the higher level protocol, if needed. The transmitter owns the "head" pointer in the fifo, and the receiver owns the "tail" pointer. The fifo is empty if those pointers are the same value. When the receiver has copied data out of the fifo, it advances the tail pointer. The transmitter must ensure that the head pointer never meets the tail pointer through wrap around. You can kind of check delivery based on the position of the tail pointer, but I can tell you from experience, you never really want to do that as it never tells you what you really want to know. -- Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, May 6, 2017 at 6:49 AM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Fri 05 May 13:22 PDT 2017, Jassi Brar wrote: >> How is it supposed to work if a client queues more than one request? > > One such example is found in patch 5 in this series. There are two FIFOs > in shared memory, one in each direction. Each fifo has a index-pair > associated; a write-index is used by the writer to inform the reader > where the valid data in the ring buffer ends and a read-index indicates > to the writer how far behind the read is. > > The writer will just push data into the FIFO, each time firing off an > APCS IPC interrupt and when the remote interrupt handler runs it will > consume all the messages from the read-index to the write-index. All > without the need for the reader to signal the writer that it has > received the interrupts. > > In the event that the write-index catches up with the read-index a > dedicated flag is set which will cause the reader to signal that the > read-index is updated - allowing the writer to sleep waiting for room in > the FIFO. > Interesting.Just for my enlightenment... Where does the writer sleep in the driver? I see it simply sets the bit and leave. Such a flag (or head and tail pointers matching) should be checked in last_tx_done() If you think RPM will _always_ be ready to accept new messages (though we have seen that doesn't hold in some situations), then you don't need last_tx_done. The client should call mbox_client_txdone() after mbox_send_message(). thnx -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 05 May 21:48 PDT 2017, Jassi Brar wrote: > On Sat, May 6, 2017 at 6:49 AM, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > On Fri 05 May 13:22 PDT 2017, Jassi Brar wrote: > > >> How is it supposed to work if a client queues more than one request? > > > > One such example is found in patch 5 in this series. There are two FIFOs > > in shared memory, one in each direction. Each fifo has a index-pair > > associated; a write-index is used by the writer to inform the reader > > where the valid data in the ring buffer ends and a read-index indicates > > to the writer how far behind the read is. > > > > The writer will just push data into the FIFO, each time firing off an > > APCS IPC interrupt and when the remote interrupt handler runs it will > > consume all the messages from the read-index to the write-index. All > > without the need for the reader to signal the writer that it has > > received the interrupts. > > > > In the event that the write-index catches up with the read-index a > > dedicated flag is set which will cause the reader to signal that the > > read-index is updated - allowing the writer to sleep waiting for room in > > the FIFO. > > > Interesting.Just for my enlightenment... > > Where does the writer sleep in the driver? I see it simply sets the > bit and leave. > Such a flag (or head and tail pointers matching) should be checked in > last_tx_done() > In the case of the FIFO based communication the flow control is implemented one layer up. You can see this in glink_rpm_tx() (in patch 5/5), where we check to see if there's enough room between the writer and the reader to store the new message. The remote side will process messages and increment the read index, which will break us out of the loop. Note that its possible to write any number of messages before invoking the APCS IPC and there's no harm (beyond wasting a little bit of power) in invoking it when there's no data written. > If you think RPM will _always_ be ready to accept new messages (though > we have seen that doesn't hold in some situations), then you don't > need last_tx_done. Whether the remote processor is ready to accept a new message or not is irrelevant in the sense of APCS IPC. When a client sends the signal over the APCS IPC it has already determined that there was space, filled in the shared memory buffers and is now simply invoking the remote handler. The APCS IPC register serves the basis for all inter-processor communication in a Qualcomm platform, so it's not only the RPM driver discussed earlier that uses this. It's also used for other non-FIFO based communication channels, where the signalled information either isn't acked at all or acked on a system-level. But regardless of the protocol implemented ontop, the purpose of the APCS IPC bit is _only_ to invoke some remote handler to consume some data, somewhere - the event in itself does not carry any information. > The client should call mbox_client_txdone() after > mbox_send_message(). So every time we call mbox_send_message() from any of the client drivers we also needs to call mbox_client_txdone()? This seems like an awkward side effect of using the mailbox framework - which has to be spread out in at least 6 different client drivers :( Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 04, 2017 at 01:05:38PM -0700, Bjorn Andersson wrote: > Add device tree binding documentation for the Qualcomm GLINK RPM, used > for communication with the Resource Power Management subsystem in > various Qualcomm SoCs. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > Changes since v3: > - Transition doorbell to mailbox framework > > Changes since v2: > - Replace qcom,ipc syscon with a "doorbell" > > Changes since v1: > - None > > .../devicetree/bindings/soc/qcom/qcom,glink.txt | 73 ++++++++++++++++++++++ > 1 file changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt > new file mode 100644 > index 000000000000..e32198df0e9c > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt > @@ -0,0 +1,73 @@ > +Qualcomm RPM GLINK binding > + > +This binding describes the Qualcomm RPM GLINK, a fifo based mechanism for > +communication with the Resource Power Management system on various Qualcomm > +platforms. > + > +- compatible: > + Usage: required > + Value type: <stringlist> > + Definition: must be "qcom,glink-rpm" > + > +- interrupts: > + Usage: required > + Value type: <prop-encoded-array> > + Definition: should specify the IRQ used by the remote processor to > + signal this processor about communication related events > + > +- qcom,rpm-msg-ram: > + Usage: required > + Value type: <prop-encoded-array> > + Definition: handle to RPM message memory resource > + > +- mboxes: > + Usage: required > + Value type: <prop-encoded-array> > + Definition: reference to the "rpm_hlos" mailbox in APCS, as described > + in mailbox/mailbox.txt > + > += GLINK DEVICES > +Each subnode of the GLINK node represent function tied to a virtual > +communication channel. The name of the nodes are not important. The properties > +of these nodes are defined by the individual bindings for the specific function > +- but must contain the following property: > + > +- qcom,glink-channels: > + Usage: required > + Value type: <stringlist> > + Definition: a list of channels tied to this function, used for matching > + the function to a set of virtual channels > + > += EXAMPLE > +The following example represents the GLINK RPM node on a MSM8996 device, with > +the function for the "rpm_request" channel defined, which is used for > +regualtors and root clocks. > + > + apcs_glb: apcs-glb@9820000 { mailbox@... Or does this block do other things? Otherwise, Acked-by: Rob Herring <robh@kernel.org> > + compatible = "qcom,msm8996-apcs-hmss-global"; > + reg = <0x9820000 0x1000>; > + > + #mbox-cells = <1>; > + }; > + > + rpm_msg_ram: memory@68000 { > + compatible = "qcom,rpm-msg-ram"; > + reg = <0x68000 0x6000>; > + }; > + > + rpm-glink { > + compatible = "qcom,glink-rpm"; > + > + interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>; > + > + qcom,rpm-msg-ram = <&rpm_msg_ram>; > + > + mboxes = <&apcs_glb 0>; > + > + rpm-requests { > + compatible = "qcom,rpm-msm8996"; > + qcom,glink-channels = "rpm_requests"; > + > + ... > + }; > + }; > -- > 2.12.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon 08 May 10:06 PDT 2017, Rob Herring wrote: > On Thu, May 04, 2017 at 01:05:38PM -0700, Bjorn Andersson wrote: > > Add device tree binding documentation for the Qualcomm GLINK RPM, used > > for communication with the Resource Power Management subsystem in > > various Qualcomm SoCs. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > > > Changes since v3: > > - Transition doorbell to mailbox framework > > > > Changes since v2: > > - Replace qcom,ipc syscon with a "doorbell" > > > > Changes since v1: > > - None > > > > .../devicetree/bindings/soc/qcom/qcom,glink.txt | 73 ++++++++++++++++++++++ > > 1 file changed, 73 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt > > new file mode 100644 > > index 000000000000..e32198df0e9c > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt > > @@ -0,0 +1,73 @@ > > +Qualcomm RPM GLINK binding > > + > > +This binding describes the Qualcomm RPM GLINK, a fifo based mechanism for > > +communication with the Resource Power Management system on various Qualcomm > > +platforms. > > + > > +- compatible: > > + Usage: required > > + Value type: <stringlist> > > + Definition: must be "qcom,glink-rpm" > > + > > +- interrupts: > > + Usage: required > > + Value type: <prop-encoded-array> > > + Definition: should specify the IRQ used by the remote processor to > > + signal this processor about communication related events > > + > > +- qcom,rpm-msg-ram: > > + Usage: required > > + Value type: <prop-encoded-array> > > + Definition: handle to RPM message memory resource > > + > > +- mboxes: > > + Usage: required > > + Value type: <prop-encoded-array> > > + Definition: reference to the "rpm_hlos" mailbox in APCS, as described > > + in mailbox/mailbox.txt > > + > > += GLINK DEVICES > > +Each subnode of the GLINK node represent function tied to a virtual > > +communication channel. The name of the nodes are not important. The properties > > +of these nodes are defined by the individual bindings for the specific function > > +- but must contain the following property: > > + > > +- qcom,glink-channels: > > + Usage: required > > + Value type: <stringlist> > > + Definition: a list of channels tied to this function, used for matching > > + the function to a set of virtual channels > > + > > += EXAMPLE > > +The following example represents the GLINK RPM node on a MSM8996 device, with > > +the function for the "rpm_request" channel defined, which is used for > > +regualtors and root clocks. > > + > > + apcs_glb: apcs-glb@9820000 { > > mailbox@... Or does this block do other things? > So far it's just a mailbox, so I'll update this. > Otherwise, > > Acked-by: Rob Herring <robh@kernel.org> > Thanks, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun 07 May 23:47 PDT 2017, Jassi Brar wrote: > On Mon, May 8, 2017 at 11:24 AM, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > On Fri 05 May 21:48 PDT 2017, Jassi Brar wrote: > > > > The APCS IPC register serves the basis for all inter-processor > > communication in a Qualcomm platform, so it's not only the RPM driver > > discussed earlier that uses this. It's also used for other non-FIFO > > based communication channels, where the signalled information either > > isn't acked at all or acked on a system-level. > > > Something has to indicate consumption of data or "requested action > taken". Otherwise the protocol is design-wise broken. > The SMD and GLINK protocols work by providing two independent one-way pipes that higher levels can use to send and receive messages. When some driver pushes a message into the transmit-pipe we check if there's space, then write the message, signal the remote (APCS IPC) and then return. It's then up to the higher level driver and protocol what to do next. In some cases it will expect that there will appear a packet on the incoming pipe indicating that the action was taken, but there's nothing in the communication path enforcing this. By this you can have strictly one-way notifications, two-way stop-and-wait style communication or multiple-messages-in-flight communication over the same transport mechanism. The remote will update the read index (in shared memory) as it consumes the data from the FIFO, but under normal circumstances there are no reason for it to actively notify the sender or for the sender to wait for it to be consumed. > > But regardless of the protocol implemented ontop, the purpose of the > > APCS IPC bit is _only_ to invoke some remote handler to consume some > > data, somewhere - the event in itself does not carry any information. > > > Yes, every platform that uses shared-memory works like that. However > there is always something that tells if the command has been acted > upon by the remote. In your case that is the read-pointer movement. > In a straight forward stop-and-wait flow control based setup like the version of RPM previously discussed this makes a lot of sense. But there are a multitude of different protocols using this mechanism to signal that something has happened. > >> The client should call mbox_client_txdone() after > >> mbox_send_message(). > > > > So every time we call mbox_send_message() from any of the client drivers > > we also needs to call mbox_client_txdone()? > > > Yes. > > > This seems like an awkward side effect of using the mailbox framework - > > which has to be spread out in at least 6 different client drivers :( > > > No. Mailbox or whatever you implement - you must (and do) tick the > state machine to keep the messages moving. But the state you have in the other mailbox drivers is not a concern of the APCS IPC. > Best designs have some interrupt occurring when the message has been > consumed by the remote. Some designs have a flag set which needs to be > polled to detect completion. Very few (like yours) that support > neither irq nor polling, have to be driven by the upper protocol layer > by some ack packet (or tracking read/write pointers like you do). > These three cases are denoted by TXDONE_BY_IRQ, TXDONE_BY_POLL and > TXDONE_BY_ACK respectively. > You're confusing the APCS IPC with the larger communication mechanism, flow control is taken care of in some higher layer - if it's needed at all. This is why I suggested that this is a doorbell, rather than a mailbox. Your argumentation of how a mailbox should work makes perfect sense, but it's not how the Qualcomm IPC works. > If no client driver will ever submit a message if there is no space > in FIFO, then you can specify TXDONE_BY_POLL and have last_tx_done() > always return true. That way you don't need to call > mbox_client_txdone(). Clients of the APCS IPC will never post a message to the mailbox, it's non-blocking and the "transaction" is done when the operation returns. All the other parts of a "non-broken protocol" is a concern of some other part of the software stack. Setting TXDONE_BY_POLL and specifying a dummy last_tx_done() comes with a crazy overhead. To set a single bit in a register we will take the channel spinlock 4 times, start a timer, iterate over all registered channels and the client must be marked as blocking so we will get at least 2 additional context switches. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 09 May 09:41 PDT 2017, Jassi Brar wrote: > On Tue, May 9, 2017 at 12:41 AM, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > On Sun 07 May 23:47 PDT 2017, Jassi Brar wrote: > > > >> On Mon, May 8, 2017 at 11:24 AM, Bjorn Andersson > >> <bjorn.andersson@linaro.org> wrote: > >> > On Fri 05 May 21:48 PDT 2017, Jassi Brar wrote: > >> > > >> > The APCS IPC register serves the basis for all inter-processor > >> > communication in a Qualcomm platform, so it's not only the RPM driver > >> > discussed earlier that uses this. It's also used for other non-FIFO > >> > based communication channels, where the signalled information either > >> > isn't acked at all or acked on a system-level. > >> > > >> Something has to indicate consumption of data or "requested action > >> taken". Otherwise the protocol is design-wise broken. > >> > > > > The SMD and GLINK protocols work by providing two independent one-way > > pipes that higher levels can use to send and receive messages. When some > > driver pushes a message into the transmit-pipe we check if there's > > space, then write the message, signal the remote (APCS IPC) and then > > return. > > > "we check if there's space" -> this is what mailbox api tries to do > with last_tx_done before starting the next message. > The space we're looking for is in a higher level in the protocol stack, the APCS IPC doesn't have a space concern. > > >> >> The client should call mbox_client_txdone() after > >> >> mbox_send_message(). > >> > > >> > So every time we call mbox_send_message() from any of the client drivers > >> > we also needs to call mbox_client_txdone()? > >> > > >> Yes. > >> > >> > This seems like an awkward side effect of using the mailbox framework - > >> > which has to be spread out in at least 6 different client drivers :( > >> > > >> No. Mailbox or whatever you implement - you must (and do) tick the > >> state machine to keep the messages moving. > > > > But the state you have in the other mailbox drivers is not a concern of > > the APCS IPC. > > > No, as you say above you check for space before writing the next > message, this is what I call ticking the state machine. > Sure, but you're talking about the mailbox state machine. The APCS IPC doesn't have states. The _only_ thing that the APCS IPC provides is a mechanism for informing the other side that "hey there's something to do". So it doesn't matter if there's already a pending "hey there's something to do", because adding another will still only be "hey there's something to do". I'm just trying to describe the big picture, but you keep confusing the mailbox/doorbell responsibilities with the client's responsibilities. > > >> Best designs have some interrupt occurring when the message has been > >> consumed by the remote. Some designs have a flag set which needs to be > >> polled to detect completion. Very few (like yours) that support > >> neither irq nor polling, have to be driven by the upper protocol layer > >> by some ack packet (or tracking read/write pointers like you do). > >> These three cases are denoted by TXDONE_BY_IRQ, TXDONE_BY_POLL and > >> TXDONE_BY_ACK respectively. > >> > > > > You're confusing the APCS IPC with the larger communication mechanism, > > > Maybe. I am not versed with QCom technologies like RPM, SMD, GLINK, APCS etc. > Controller driver is what physically transmits a signal to remote. > Users above the mailbox api are client drivers. > Correct, and the signal that we're trying to transmit is "hey there's _something_ to do", nothing else. Relate this to when Fedex drops a packet at your door; first he checks there's space on the porch, then he puts the packet there, he rings the doorbell and then he walks away. You are free to "answer" the doorbell now or at any point in the future. He doesn't have to stand there are wait and it doesn't matter if he rings the bell multiple times - you will still only check for packages once (regardless of how many he left). In the case of him wanting your signature, then that's an implementation detail of the Fedex guy, it has nothing to do with how you wire your doorbell! > > > > This is why I suggested that this is a doorbell, rather than a mailbox. > > Your argumentation of how a mailbox should work makes perfect sense, but > > it's not how the Qualcomm IPC works. > > > Mailbox framework is designed to support, what you call doorbell type > of communication, just fine. There is no need to define another class. > Okay good. > > > > Setting TXDONE_BY_POLL and specifying a dummy last_tx_done() comes with > > a crazy overhead. To set a single bit in a register we will take the > > channel spinlock 4 times, start a timer, iterate over all registered > > channels and the client must be marked as blocking so we will get at > > least 2 additional context switches. > > > How often does the platform send messages for it to be a considerable load? This is the basis for all inter-processor communication in the Qualcomm platforms, so the above list of extra hoops to jump through is not acceptable. > BTW, this is an option only if your client driver doesn't want to > explicitly tick the state machine by calling mbox_client_txdone()... > which I think should be done in the first place. > There is no state of the APCS IPC, so the overhead is created by the mailbox framework. The part where this piece of hardware differs from the other mailboxes is that TX is done as send_data() returns and in the realm of the mailbox there is no such thing as "tx done". So how about we extend the framework to handle stateless and message-less doorbells? Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 10 May 19:07 PDT 2017, Jassi Brar wrote: > On Thu, May 11, 2017 at 12:30 AM, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > On Tue 09 May 19:33 PDT 2017, Jassi Brar wrote: [..] > > So please let me know what you think about [1], if you don't like it > > I'll fix the things pointed to by Stephen and we'll have to live with > > the two calls. > > > My last reply was about [1]. Other platforms call > mbox_send_data()+mbox_client_txdone() see > drivers/firmware/tegra/bpmp.c, but you want to introduce another API > in the innards of the framework. Okay, lets go with that then. I will incorporate the changes requested by Stephen and post a final version and then add the mbox_client_txdone() in the clients. > If we must do it, it should be done > above the framework by introducing > > void mbox_send_message_and_tick(struct mbox_chan *chan, void *mssg) > OR > void mbox_ring_doorbell(struct mbox_chan *chan, void *mssg) > { > (void)mbox_send_message(chan, mssg); > mbox_client_txdone(chan, 0); > } This sounds reasonable, but I would prefer that we get the two drivers merged - so I suggest that we deal with that later, when we see if its worth the effort. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c index 4671f8a12872..c88de953394a 100644 --- a/drivers/mailbox/mailbox.c +++ b/drivers/mailbox/mailbox.c @@ -137,6 +137,20 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer) return HRTIMER_NORESTART; } +static int mbox_startup(struct mbox_chan *chan) +{ + if (chan->mbox->ops->startup) + return chan->mbox->ops->startup(chan); + + return 0; +} + +static void mbox_shutdown(struct mbox_chan *chan) +{ + if (chan->mbox->ops->shutdown) + chan->mbox->ops->shutdown(chan); +} + /** * mbox_chan_received_data - A way for controller driver to push data * received from remote to the upper layer. @@ -352,7 +366,7 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index) spin_unlock_irqrestore(&chan->lock, flags); - ret = chan->mbox->ops->startup(chan); + ret = mbox_startup(chan); if (ret) { dev_err(dev, "Unable to startup the chan (%d)\n", ret); mbox_free_channel(chan); @@ -405,7 +419,7 @@ void mbox_free_channel(struct mbox_chan *chan) if (!chan || !chan->cl) return; - chan->mbox->ops->shutdown(chan); + mbox_shutdown(chan); /* The queued TX requests are simply aborted, no callbacks are made */ spin_lock_irqsave(&chan->lock, flags);
Some mailbox hardware doesn't have to perform any additional operations on startup of shutdown, so make these optional. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v3: - New patch drivers/mailbox/mailbox.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) -- 2.12.0 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html