Hi Bjorn,
On 11/16/2017 12:38 PM, Bjorn Andersson wrote:
> Attempt to acquire the APCS IPC through the mailbox framework and fall
> back to the old syscon based approach, to allow us to move away from
> using the syscon.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/rpmsg/qcom_smd.c | 62 +++++++++++++++++++++++++++++++++---------------
> 1 file changed, 43 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
> index b01774e9fac0..ef2a526ebc8f 100644
> --- a/drivers/rpmsg/qcom_smd.c
> +++ b/drivers/rpmsg/qcom_smd.c
> @@ -14,6 +14,7 @@
>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> +#include <linux/mailbox_client.h>
> #include <linux/mfd/syscon.h>
> #include <linux/module.h>
> #include <linux/of_irq.h>
> @@ -107,6 +108,8 @@ static const struct {
> * @ipc_regmap: regmap handle holding the outgoing ipc register
> * @ipc_offset: offset within @ipc_regmap of the register for ipc
> * @ipc_bit: bit in the register at @ipc_offset of @ipc_regmap
> + * @mbox_client: mailbox client handle
> + * @mbox_chan: apcs ipc mailbox channel handle
> * @channels: list of all channels detected on this edge
> * @channels_lock: guard for modifications of @channels
> * @allocated: array of bitmaps representing already allocated channels
> @@ -129,6 +132,9 @@ struct qcom_smd_edge {
> int ipc_offset;
> int ipc_bit;
>
> + struct mbox_client mbox_client;
> + struct mbox_chan *mbox_chan;
> +
> struct list_head channels;
> spinlock_t channels_lock;
>
> @@ -365,7 +371,12 @@ static void qcom_smd_signal_channel(struct qcom_smd_channel *channel)
> {
> struct qcom_smd_edge *edge = channel->edge;
>
> - regmap_write(edge->ipc_regmap, edge->ipc_offset, BIT(edge->ipc_bit));
> + if (edge->mbox_chan) {
> + mbox_send_message(edge->mbox_chan, NULL);
mbox_send_message could fail. So return value should be checked
> + mbox_client_txdone(edge->mbox_chan, 0);
> + } else {
> + regmap_write(edge->ipc_regmap, edge->ipc_offset, BIT(edge->ipc_bit));
> + }
> }
>
> /*
> @@ -1268,27 +1279,37 @@ static int qcom_smd_parse_edge(struct device *dev,
> key = "qcom,remote-pid";
> of_property_read_u32(node, key, &edge->remote_pid);
>
> - syscon_np = of_parse_phandle(node, "qcom,ipc", 0);
> - if (!syscon_np) {
> - dev_err(dev, "no qcom,ipc node\n");
> - return -ENODEV;
> - }
> + edge->mbox_client.dev = dev;
> + edge->mbox_client.knows_txdone = true;
> + edge->mbox_chan = mbox_request_channel(&edge->mbox_client, 0);
> + if (IS_ERR(edge->mbox_chan)) {
> + if (PTR_ERR(edge->mbox_chan) != -ENODEV)
> + return PTR_ERR(edge->mbox_chan);
>
> - edge->ipc_regmap = syscon_node_to_regmap(syscon_np);
> - if (IS_ERR(edge->ipc_regmap))
> - return PTR_ERR(edge->ipc_regmap);
> + edge->mbox_chan = NULL;
>
> - key = "qcom,ipc";
> - ret = of_property_read_u32_index(node, key, 1, &edge->ipc_offset);
> - if (ret < 0) {
> - dev_err(dev, "no offset in %s\n", key);
> - return -EINVAL;
> - }
> + syscon_np = of_parse_phandle(node, "qcom,ipc", 0);
> + if (!syscon_np) {
> + dev_err(dev, "no qcom,ipc node\n");
> + return -ENODEV;
> + }
>
> - ret = of_property_read_u32_index(node, key, 2, &edge->ipc_bit);
> - if (ret < 0) {
> - dev_err(dev, "no bit in %s\n", key);
> - return -EINVAL;
> + edge->ipc_regmap = syscon_node_to_regmap(syscon_np);
> + if (IS_ERR(edge->ipc_regmap))
> + return PTR_ERR(edge->ipc_regmap);
> +
> + key = "qcom,ipc";
> + ret = of_property_read_u32_index(node, key, 1, &edge->ipc_offset);
> + if (ret < 0) {
> + dev_err(dev, "no offset in %s\n", key);
> + return -EINVAL;
> + }
> +
> + ret = of_property_read_u32_index(node, key, 2, &edge->ipc_bit);
> + if (ret < 0) {
> + dev_err(dev, "no bit in %s\n", key);
> + return -EINVAL;
> + }
> }
>
> ret = of_property_read_string(node, "label", &edge->name);
> @@ -1394,6 +1415,8 @@ struct qcom_smd_edge *qcom_smd_register_edge(struct device *parent,
> return edge;
>
> unregister_dev:
> + if (!IS_ERR_OR_NULL(edge->mbox_chan))
> + mbox_free_channel(edge->mbox_chan);
> put_device(&edge->dev);
> return ERR_PTR(ret);
> }
> @@ -1422,6 +1445,7 @@ int qcom_smd_unregister_edge(struct qcom_smd_edge *edge)
> if (ret)
> dev_warn(&edge->dev, "can't remove smd device: %d\n", ret);
>
> + mbox_free_channel(edge->mbox_chan);
> device_unregister(&edge->dev);
>
> return 0;
On Wed 06 Dec 04:08 PST 2017, Jitendra Sharma wrote:
> Hi Bjorn,
>
Hi Jitendra,
> On 11/16/2017 12:38 PM, Bjorn Andersson wrote:
[..]
> > @@ -365,7 +371,12 @@ static void qcom_smd_signal_channel(struct qcom_smd_channel *channel)
> > {
> > struct qcom_smd_edge *edge = channel->edge;
> > - regmap_write(edge->ipc_regmap, edge->ipc_offset, BIT(edge->ipc_bit));
> > + if (edge->mbox_chan) {
> > + mbox_send_message(edge->mbox_chan, NULL);
> mbox_send_message could fail. So return value should be checked
qcom_apcs_ipc_send_data() can't fail, so the case when
mbox_send_message() would fail is if more than MBOX_TX_QUEUE_LEN (20)
callers that has managed to put their data in the queue but not yet
execute msg_submit().
As each bit in the APCS IPC register is modelled as it's own mailbox
channel this error case would mean that as mbox_send_message() returns
with an error there will soon be 20 callers entering
qcom_apcs_ipc_send_data() and trigger this very bit.
When this happens mbox_send_message() will print an error in the log, so
there's no point in having the caller also print an error.
When it comes to dealing with a failing call to mbox_send_message() we
have already posted the message in the FIFO, so we have no way to abort
the transmission, as such the only way to deal with this is to either
retry or ignore the problem; and the mailbox queue will ensure that we
retry 20 times.
Regards,
Bjorn
On 12/06/2017 11:02 AM, Bjorn Andersson wrote:
> On Wed 06 Dec 04:08 PST 2017, Jitendra Sharma wrote:
>
>> Hi Bjorn,
>>
> Hi Jitendra,
>
>> On 11/16/2017 12:38 PM, Bjorn Andersson wrote:
> [..]
>>> @@ -365,7 +371,12 @@ static void qcom_smd_signal_channel(struct qcom_smd_channel *channel)
>>> {
>>> struct qcom_smd_edge *edge = channel->edge;
>>> - regmap_write(edge->ipc_regmap, edge->ipc_offset, BIT(edge->ipc_bit));
>>> + if (edge->mbox_chan) {
>>> + mbox_send_message(edge->mbox_chan, NULL);
>> mbox_send_message could fail. So return value should be checked
> qcom_apcs_ipc_send_data() can't fail, so the case when
> mbox_send_message() would fail is if more than MBOX_TX_QUEUE_LEN (20)
> callers that has managed to put their data in the queue but not yet
> execute msg_submit().
>
> As each bit in the APCS IPC register is modelled as it's own mailbox
> channel this error case would mean that as mbox_send_message() returns
> with an error there will soon be 20 callers entering
> qcom_apcs_ipc_send_data() and trigger this very bit.
>
>
> When this happens mbox_send_message() will print an error in the log, so
> there's no point in having the caller also print an error.
>
> When it comes to dealing with a failing call to mbox_send_message() we
> have already posted the message in the FIFO, so we have no way to abort
> the transmission, as such the only way to deal with this is to either
> retry or ignore the problem; and the mailbox queue will ensure that we
> retry 20 times.
>
Maybe you should wrap this up into a comment in the code? Then we don't
have to dig this out of the mail list archives to figure out why we
aren't checking for an error.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Wed 06 Dec 11:54 PST 2017, Stephen Boyd wrote:
> On 12/06/2017 11:02 AM, Bjorn Andersson wrote:
> > On Wed 06 Dec 04:08 PST 2017, Jitendra Sharma wrote:
> >
> >> Hi Bjorn,
> >>
> > Hi Jitendra,
> >
> >> On 11/16/2017 12:38 PM, Bjorn Andersson wrote:
> > [..]
> >>> @@ -365,7 +371,12 @@ static void qcom_smd_signal_channel(struct qcom_smd_channel *channel)
> >>> {
> >>> struct qcom_smd_edge *edge = channel->edge;
> >>> - regmap_write(edge->ipc_regmap, edge->ipc_offset, BIT(edge->ipc_bit));
> >>> + if (edge->mbox_chan) {
> >>> + mbox_send_message(edge->mbox_chan, NULL);
> >> mbox_send_message could fail. So return value should be checked
> > qcom_apcs_ipc_send_data() can't fail, so the case when
> > mbox_send_message() would fail is if more than MBOX_TX_QUEUE_LEN (20)
> > callers that has managed to put their data in the queue but not yet
> > execute msg_submit().
> >
> > As each bit in the APCS IPC register is modelled as it's own mailbox
> > channel this error case would mean that as mbox_send_message() returns
> > with an error there will soon be 20 callers entering
> > qcom_apcs_ipc_send_data() and trigger this very bit.
> >
> >
> > When this happens mbox_send_message() will print an error in the log, so
> > there's no point in having the caller also print an error.
> >
> > When it comes to dealing with a failing call to mbox_send_message() we
> > have already posted the message in the FIFO, so we have no way to abort
> > the transmission, as such the only way to deal with this is to either
> > retry or ignore the problem; and the mailbox queue will ensure that we
> > retry 20 times.
> >
>
> Maybe you should wrap this up into a comment in the code? Then we don't
> have to dig this out of the mail list archives to figure out why we
> aren't checking for an error.
>
Sounds very reasonable, I will do so.
Regards,
Bjorn