Re: [PATCH v5 8/8] firmware: arm_scpi: Support unidirectional mailbox channels

[Sudeep Holla <sudeep.holla@arm.com>]

On Sat, Dec 14, 2019 at 10:24:55PM -0600, Samuel Holland wrote:
> Some mailbox controllers have only unidirectional channels, so we need a
> pair of them for each SCPI channel. If a mbox-names property is present,
> look for "rx" and "tx" mbox channels; otherwise, the existing behavior
> is preserved, and a single mbox channel is used for each SCPI channel.
>

I need to look at the bindings again, but I think you must update it.

> Note that since the mailbox framework only supports a single phandle
> with each name (mbox_request_channel_byname always returns the first
> one), this new mode only supports a single SCPI channel.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/firmware/arm_scpi.c | 58 +++++++++++++++++++++++++++++--------
>  1 file changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index a80c331c3a6e..36ff9dd8d0fa 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -231,7 +231,8 @@ struct scpi_xfer {
>
>  struct scpi_chan {
>  	struct mbox_client cl;
> -	struct mbox_chan *chan;
> +	struct mbox_chan *rx_chan;
> +	struct mbox_chan *tx_chan;
>  	void __iomem *tx_payload;
>  	void __iomem *rx_payload;
>  	struct list_head rx_pending;
> @@ -505,7 +506,7 @@ static int scpi_send_message(u8 idx, void *tx_buf, unsigned int tx_len,
>  	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;
>
> @@ -854,8 +855,13 @@ static void scpi_free_channels(void *data)
>  	struct scpi_drvinfo *info = data;
>  	int i;
>
> -	for (i = 0; i < info->num_chans; i++)
> -		mbox_free_channel(info->channels[i].chan);
> +	for (i = 0; i < info->num_chans; i++) {
> +		struct scpi_chan *pchan = &info->channels[i];
> +
> +		if (pchan->tx_chan != pchan->rx_chan)
> +			mbox_free_channel(pchan->tx_chan);
> +		mbox_free_channel(pchan->rx_chan);

I think mbox_free_channel handles !chan->cl, so just do unconditionally.

> +	}
>  }
>
>  static int scpi_remove(struct platform_device *pdev)
> @@ -903,6 +909,7 @@ static int scpi_probe(struct platform_device *pdev)
>  	struct resource res;
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
> +	bool use_mbox_names = false;
>
>  	scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
>  	if (!scpi_info)
> @@ -916,6 +923,14 @@ static int scpi_probe(struct platform_device *pdev)
>  		dev_err(dev, "no mboxes property in '%pOF'\n", np);
>  		return -ENODEV;
>  	}
> +	if (of_get_property(dev->of_node, "mbox-names", NULL)) {

So, for this platform, this is required and must fail if it is not found.
But instead your check here is optional and may end up populating 2
scpi channels instead of one. I would suggest to make it required and
fail for it based on some compatible, otherwise you are not checking
correctly.

Something like:
        if (of_match_device(blah_blah_of_match, &pdev->dev)) {
                use_mbox_names = true;
		count = 1;
	}


> +		use_mbox_names = true;
> +		if (count != 2) {
> +			dev_err(dev, "need exactly 2 mboxes with mbox-names\n");
> +			return -ENODEV;
> +		}
> +		count /= 2;
> +	}

Ah, OK then you must update the binding as it's different usage of mailbox. 

General query, not related to this patch: If you are in process of
implementing the firmware, I suggest to move to SCMI protocol than this
one if not too late. This specification is deprecated and no longer
updated while SCMI is actively developed and maintained. However it has
slightly different notion of tx and rx and the way the specification
commands which messages are synchronous and which can be async/delayed.

--
Regards,
Sudeep