[PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver
From: samuel@sholland.org (Samuel Holland)
Date: 2018-02-28 18:56:46
Also in:
linux-devicetree, linux-mediatek, lkml
On 02/28/18 12:14, Jassi Brar wrote:
On Wed, Feb 28, 2018 at 11:21 PM, Samuel Holland [off-list ref] wrote:quoted
Hi, On 02/28/18 03:16, Jassi Brar wrote:quoted
On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland [off-list ref] wrote: ....quoted
+/* + * The message box hardware provides 8 unidirectional channels. As the mailbox + * framework expects them to be bidirectionalThat is incorrect. Mailbox framework does not require a channel to be TX and RX capable.Sorry, it would be more accurate to say that the intended mailbox _client_ expects the channels to be bidirectional.Mailbox clients are very mailbox provider specific. Your client driver is unlikely to be reuseable over another controller+remote combo. Your client has to know already what a physical channel can do (RX, TX or RXTX). There is no api to provide that info.
There's a public specification for SCPI available[1]. I'm writing the remote endpoint to follow that protocol specification. (There's even an explicitly platform-agnostic update to the protocol called SCMI[2]). Ideally, I should be able to reuse the existing Linux client drivers for that protocol. Are you suggesting that I need to write a copy of the arm_scpi driver, completely identical except that it requests two mailbox channels per client (sending on one and receiving on the other) instead of one? In the >1000 line file, this is all that I would need to change:
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c@@ -257,7 +257,8 @@ struct scpi_xfer { struct scpi_chan { struct mbox_client cl; - struct mbox_chan *chan; + struct mbox_chan *tx_chan; + struct mbox_chan *rx_chan; void __iomem *tx_payload; void __iomem *rx_payload; struct list_head rx_pending;
@@ -541,7 +542,7 @@ msg->rx_len = rx_len; reinit_completion(&msg->done); - ret = mbox_send_message(scpi_chan->chan, msg); + ret = mbox_send_message(scpi_chan->tx_chan, msg); if (ret < 0 || !rx_buf) goto out;
@@ -894,8 +895,10 @@ { int i; - for (i = 0; i < count && pchan->chan; i++, pchan++) { - mbox_free_channel(pchan->chan); + for (i = 0; i < count && pchan->tx_chan; i++, pchan++) { + if (pchan->rx_chan && pchan->rx_chan != pchan->tx_chan) + mbox_free_channel(pchan->rx_chan); + mbox_free_channel(pchan->tx_chan); devm_kfree(dev, pchan->xfers); devm_iounmap(dev, pchan->rx_payload); }
@@ -968,6 +971,7 @@ dev_err(dev, "no mboxes property in '%pOF'\n", np); return -ENODEV; } + count /= 2; scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL); if (!scpi_chan)
@@ -1009,13 +1013,19 @@ ret = scpi_alloc_xfer_list(dev, pchan); if (!ret) { - pchan->chan = mbox_request_channel(cl, idx); - if (!IS_ERR(pchan->chan)) + pchan->tx_chan = mbox_request_channel(cl, idx * 2); + pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1); + /* This isn't quite right, but the idea stands. */ + if (!IS_ERR(pchan->tx_chan) && !IS_ERR(pchan->rx_chan)) continue; - ret = PTR_ERR(pchan->chan); + ret = PTR_ERR(pchan->tx_chan); if (ret != -EPROBE_DEFER) dev_err(dev, "failed to get channel%d err %d\n", - idx, ret); + idx * 2, ret); + ret = PTR_ERR(pchan->rx_chan); + if (ret != -EPROBE_DEFER) + dev_err(dev, "failed to get channel%d err %d\n", + idx * 2 + 1, ret); } err: scpi_free_channels(dev, scpi_chan, idx);
But then there are two copies of almost exactly the same driver! If there was an API for determining if a channel was unidirectional or bidirectional, than it would be trivial to support both models in one driver:
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c@@ -953,7 +955,7 @@ static int scpi_probe(struct platform_device *pdev) { - int count, idx, ret; + int count, idx, mbox_idx, ret; struct resource res; struct scpi_chan *scpi_chan; struct device *dev = &pdev->dev;
@@ -971,13 +973,12 @@ dev_err(dev, "no mboxes property in '%pOF'\n", np); return -ENODEV; } - count /= 2; scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL); if (!scpi_chan) return -ENOMEM; - for (idx = 0; idx < count; idx++) { + for (idx = 0, mbox_idx = 0; mbox_idx < count; idx++) { resource_size_t size; struct scpi_chan *pchan = scpi_chan + idx; struct mbox_client *cl = &pchan->cl;
@@ -1014,7 +1015,13 @@ ret = scpi_alloc_xfer_list(dev, pchan); if (!ret) { pchan->tx_chan = mbox_request_channel(cl, idx * 2); - pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1); + if (mbox_chan_is_bidirectional(pchan->tx_chan)) { + pchan->rx_chan = pchan->tx_chan; + mbox_idx += 1; + } else { + pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1); + mbox_idx += 2; + } /* This isn't quite right, but the idea stands. */ if (!IS_ERR(pchan->tx_chan) && !IS_ERR(pchan->rx_chan)) continue;
@@ -1034,7 +1041,7 @@ } scpi_info->channels = scpi_chan; - scpi_info->num_chans = count; + scpi_info->num_chans = idx; scpi_info->commands = scpi_std_commands; platform_set_drvdata(pdev, scpi_info);
Obviously this is just proof-of-concept code and would need to be cleaned up a bit.
The are platform-agnostic protocols using mailbox hardware. There's no reason
that the drivers for them need to be platform-specific. I'd really like to avoid
duplicating large amounts of code unnecessarily. I'm willing to prepare a patch
series adding this functionality to the mailbox API, if it would be accepted.
Something like:
bool mbox_chan_is_bidirectional(struct mbox_chan *chan);
Implemented in drivers like:
struct mbox_controller {
...
bool bidirectional_chans;
};
or:
struct mbox_chan_ops {
...
bool (*is_bidirectional)(struct mbox_chan *chan);
};
thanks
Regards, Samuel [1]: http://infocenter.arm.com/help/topic/com.arm.doc.dui0922g/scp_message_interface_v1_2_DUI0922G_en.pdf [2]: http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/DEN0056A_System_Control_and_Management_Interface.pdf