On 02/28/18 12:14, Jassi Brar wrote:
> On Wed, Feb 28, 2018 at 11:21 PM, Samuel Holland <[email protected]> wrote:
>> Hi,
>>
>> On 02/28/18 03:16, Jassi Brar wrote:
>>> On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland <[email protected]> wrote:
>>> ....
>>>
>>>> +/*
>>>> + * The message box hardware provides 8 unidirectional channels. As the
>>>> mailbox
>>>> + * framework expects them to be bidirectional
>>>>
>>> That 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
