| Message ID | 20170504200539.27027-3-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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-remoteproc" 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-remoteproc" 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-remoteproc" 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 1:22 PM, Jassi Brar wrote: > 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'? Technically nothing. This is not a traditional register. Its an address that is modeled as a register that the hardware knows, when 1 is written to the specific bit, the hardware is to take a specific action (generate the interrupt). As Bjorn stated, its a write only address. The hardware documentation states not to read it. On most platforms, if you attempt to read the address, you will always get 0, although that is not guaranteed, and there have been platforms where attempting to read the address will result in a bus hang. Conceptually, the hardware automatically resets it to "0" in the next clock cycle. Software doesn't need to care. > > 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. That doesn't apply here. The doorbell in this case is a signal for the remote processor to go look at the FIFO in shared memory. Since interrupts can be lost (ie the local processor sends multiple interrupts before the remote processor can service them, so the GIC drops them since the interrupt is already pending), the remote processor is required to drain the FIFO when it processes the interrupt. There is no way to determine if the remote processor has observed a message, that does not involve pretty trivial race conditions. I invented this protocol. From what I've seen (granted I did not do a thorough review), Bjorn is doing the correct thing here.
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 :) -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" 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.
On Fri 05 May 13:22 PDT 2017, 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? 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. > How do you know when it's ok to overwrite the FIFO and send the next > command? As long as there's enough space between the write-index and the read-index we can write. And we don't need any locks, because the writer is the only one changing the write-index and the reader is the only one changing the read index. > 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 protocols implemented on top will guarantee this, as needed, so it's fine :) I'll update patch 1 and repost patch 1 through 3 for you to merge in the mailbox tree, patch 4 and 5 I'll pull into the rpmsg tree once I have received some additional feedback. (We don't have any build time dependencies between the two, so it's better to merge each driver through their respective tree) Thanks, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" 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-remoteproc" 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-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > 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. >> 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. 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. 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(). -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" 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-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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. >> 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. > > 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. > > 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? 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. thanks -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" 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-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 10, 2017 at 12:41 AM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Tue 09 May 09:41 PDT 2017, Jassi Brar wrote: >> >> >> >> 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. > I think I do understand the bigger picture... The client driver sets up data packet in SHM and submit a "doorbell" to be ringed. The controller driver simply sets some bit to trigger an irq on the remote side (doorbell). And before submitting a "doorbell" the client makes sure there is some space for data packet to be written. Right? You see, in the big picture you do have a state-machine. [Message to send] | | |-------------->| | No | | | |___[Space Available?] | |Yes | | [ Setup Data in SHM] | V [Ring Doorbell] Mailbox framework supports this whole picture. There is even a callback (tx_prepare) to setup data packet just before the doorbell is to be rung. >> 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. > Overhead remains the same if you move the check from your client drivers to last_tx_done. OR your client driver, rightfully, drive the state machine by calling mbox_client_txdone() like other platforms. [Message to send]<----------| | | | | |-------------->| | | No | | | | | |___[Space Available?] | | | |Yes | | | V | [Setup Data in SHM] | | | V | mbox_send_message() | | | V | mbox_client_txdone() | | | V______________| > 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? > This is a very common usecase. It would be unfair to other platforms to modify the API just because you find it awkward to call mbox_client_txdone() right after mbox_send_message(). For example, drivers/firmware/tegra/bpmp.c I'd much rather have mbox_send_message_and_tick() than implant a new api. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" 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 19:33 PDT 2017, Jassi Brar wrote: > On Wed, May 10, 2017 at 12:41 AM, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > On Tue 09 May 09:41 PDT 2017, Jassi Brar wrote: [..] > > 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? > > > This is a very common usecase. It would be unfair to other platforms > to modify the API just because you find it awkward to call > mbox_client_txdone() right after mbox_send_message(). For example, > drivers/firmware/tegra/bpmp.c > I'd much rather have mbox_send_message_and_tick() than implant a new api. > I wasn't proposing to implement a new API; for mailbox controllers with tx method set to POLL the client will only have call mbox_send_data() nothing else. So I proposed [1] yesterday, that will make the apcs controller behave just like this case. Looking at it again, making sure that tx method is TXDONE_BY_ACK should make mbox_client_txdone() essentially a nop, only locking the channel spinlock twice and then returning, as send_data() didn't leave any data in the queue. Is my understanding of this correct? 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. [1] https://patchwork.kernel.org/patch/9718931/ Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" 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 11, 2017 at 12:30 AM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Tue 09 May 19:33 PDT 2017, Jassi Brar wrote: > >> On Wed, May 10, 2017 at 12:41 AM, Bjorn Andersson >> <bjorn.andersson@linaro.org> wrote: >> > On Tue 09 May 09:41 PDT 2017, Jassi Brar wrote: > [..] >> > 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? >> > >> This is a very common usecase. It would be unfair to other platforms >> to modify the API just because you find it awkward to call >> mbox_client_txdone() right after mbox_send_message(). For example, >> drivers/firmware/tegra/bpmp.c >> I'd much rather have mbox_send_message_and_tick() than implant a new api. >> > > I wasn't proposing to implement a new API > Introducing mbox_controller.txdone_none for underlying mailbox controllers is indeed a new API. >; for mailbox controllers with > tx method set to POLL the client will only have call mbox_send_data() > nothing else. So I proposed [1] yesterday, that will make the apcs > controller behave just like this case. > mbox_send_data() for POLL usecase mbox_send_data()+mbox_client_txdone() for your usecase. > Looking at it again, making sure that tx method is TXDONE_BY_ACK should > make mbox_client_txdone() essentially a nop, only locking the channel > spinlock twice and then returning, as send_data() didn't leave any data > in the queue. Is my understanding of this correct? > Not exactly. tx_tick() frees the busy flag 'chan->active_req'. > 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. 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); } -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" 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-remoteproc" 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 13, 2017 at 4:18 AM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > 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. > OK cool. And please update the commit log discarding "The driver implements the "doorbell" binding and could be used as basis for a new Linux framework, if found useful outside Qualcomm." Cheers! -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" 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/Kconfig b/drivers/mailbox/Kconfig index ceff415f201c..fffc64da61f9 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -124,6 +124,14 @@ config MAILBOX_TEST Test client to help with testing new Controller driver implementations. +config QCOM_APCS_IPC + tristate "Qualcomm APCS IPC driver" + depends on ARCH_QCOM + help + Say y here to enable support for the APCS IPC doorbell driver, + providing an interface for invoking the inter-process communication + signals from the application processor to other masters. + config TEGRA_HSP_MBOX bool "Tegra HSP (Hardware Synchronization Primitives) Driver" depends on ARCH_TEGRA_186_SOC diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index 7dde4f609ae8..cc718c79669a 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -30,4 +30,6 @@ obj-$(CONFIG_HI6220_MBOX) += hi6220-mailbox.o obj-$(CONFIG_BCM_PDC_MBOX) += bcm-pdc-mailbox.o +obj-$(CONFIG_QCOM_APCS_IPC) += qcom-apcs-ipc-mailbox.o + obj-$(CONFIG_TEGRA_HSP_MBOX) += tegra-hsp.o diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c new file mode 100644 index 000000000000..41e31c66c7aa --- /dev/null +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c @@ -0,0 +1,128 @@ +/* + * Copyright (c) 2017, Linaro Ltd + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/mailbox_controller.h> + +#define QCOM_APCS_IPC_BITS 32 + +struct qcom_apcs_ipc { + struct device *dev; + + struct mbox_controller mbox; + struct mbox_chan mbox_chans[QCOM_APCS_IPC_BITS]; + + void __iomem *base; + unsigned long offset; +}; + +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); + + return 0; +} + +static const struct mbox_chan_ops qcom_apcs_ipc_ops = { + .send_data = qcom_apcs_ipc_send_data, +}; + +static int qcom_apcs_ipc_probe(struct platform_device *pdev) +{ + struct qcom_apcs_ipc *apcs; + struct resource *res; + unsigned long i; + int ret; + + apcs = devm_kzalloc(&pdev->dev, sizeof(*apcs), GFP_KERNEL); + if (!apcs) + return -ENOMEM; + + apcs->dev = &pdev->dev; + apcs->offset = (unsigned long)of_device_get_match_data(&pdev->dev); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + apcs->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(apcs->base)) + return PTR_ERR(apcs->base); + + /* Initialize channel identifiers */ + for (i = 0; i < ARRAY_SIZE(apcs->mbox_chans); i++) + apcs->mbox_chans[i].con_priv = (void *)i; + + apcs->mbox.dev = &pdev->dev; + apcs->mbox.ops = &qcom_apcs_ipc_ops; + apcs->mbox.chans = apcs->mbox_chans; + apcs->mbox.num_chans = ARRAY_SIZE(apcs->mbox_chans); + + ret = mbox_controller_register(&apcs->mbox); + if (ret) { + dev_err(&pdev->dev, "failed to register APCS IPC controller\n"); + return ret; + } + + platform_set_drvdata(pdev, apcs); + + return 0; +} + +static int qcom_apcs_ipc_remove(struct platform_device *pdev) +{ + struct qcom_apcs_ipc *apcs = platform_get_drvdata(pdev); + + mbox_controller_unregister(&apcs->mbox); + + return 0; +} + +/* .data is the offset of the ipc register within the global block */ +static const struct of_device_id qcom_apcs_ipc_of_match[] = { + { .compatible = "qcom,msm8916-apcs-kpss-global", .data = (void *)8 }, + { .compatible = "qcom,msm8996-apcs-hmss-global", .data = (void *)16 }, + {} +}; +MODULE_DEVICE_TABLE(of, qcom_apcs_ipc_of_match); + +static struct platform_driver qcom_apcs_ipc_driver = { + .probe = qcom_apcs_ipc_probe, + .remove = qcom_apcs_ipc_remove, + .driver = { + .name = "qcom_apcs_ipc", + .of_match_table = qcom_apcs_ipc_of_match, + }, +}; + +static int __init qcom_apcs_ipc_init(void) +{ + return platform_driver_register(&qcom_apcs_ipc_driver); +} +postcore_initcall(qcom_apcs_ipc_init); + +static void __exit qcom_apcs_ipc_exit(void) +{ + platform_driver_unregister(&qcom_apcs_ipc_driver); +} +module_exit(qcom_apcs_ipc_exit); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Qualcomm APCS IPC driver");
This implements a driver that exposes the IPC bits found in the APCS Global block in various Qualcomm platforms. The bits are used to signal inter-processor communication signals from the application CPU to other masters. The driver implements the "doorbell" binding and could be used as basis for a new Linux framework, if found useful outside Qualcomm. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v3: - Migrate to mailbox framework Changes since v2: - New driver drivers/mailbox/Kconfig | 8 ++ drivers/mailbox/Makefile | 2 + drivers/mailbox/qcom-apcs-ipc-mailbox.c | 128 ++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+) create mode 100644 drivers/mailbox/qcom-apcs-ipc-mailbox.c