Re: [RFC PATCH 01/11] firmware: arm_scmi: Add receive buffer support for notifications

Cristian Marussi
2020-02-14T15:26:01Z

Hi

On 27/01/2020 17:07, Jonathan Cameron wrote:
> On Mon, 20 Jan 2020 12:23:23 +0000
> Cristian Marussi <cristian.marussi@arm.com> wrote:
>
>> From: Sudeep Holla <sudeep.holla@arm.com>
>>
>> With all the plumbing in place, let's just add the separate dedicated
>> receive buffers to handle notifications that can arrive asynchronously
>> from the platform firmware to OS.
>>
>> Also add check to see if the platform supports any receive channels
>> before allocating the receive buffers.
>
> Perhaps hand hold the reader a tiny bit more by saying that we need
> to move the initialization later so that we can know *if* the receive
> channels are supported. Took me a moment to figure out why you did that ;)
>

Addressed in v2.

> One minor suggestion inline.
>
>>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>> drivers/firmware/arm_scmi/driver.c | 24 ++++++++++++++++++------
>> 1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
>> index 2c96f6b5a7d8..9611e8037d77 100644
>> --- a/drivers/firmware/arm_scmi/driver.c
>> +++ b/drivers/firmware/arm_scmi/driver.c
>> @@ -123,6 +123,7 @@ struct scmi_chan_info {
>> * @version: SCMI revision information containing protocol version,
>> * implementation version and (sub-)vendor identification.
>> * @tx_minfo: Universal Transmit Message management info
>> + * @rx_minfo: Universal Receive Message management info
>> * @tx_idr: IDR object to map protocol id to Tx channel info pointer
>> * @rx_idr: IDR object to map protocol id to Rx channel info pointer
>> * @protocols_imp: List of protocols implemented, currently maximum of
>> @@ -136,6 +137,7 @@ struct scmi_info {
>> struct scmi_revision_info version;
>> struct scmi_handle handle;
>> struct scmi_xfers_info tx_minfo;
>> + struct scmi_xfers_info rx_minfo;
>> struct idr tx_idr;
>> struct idr rx_idr;
>> u8 *protocols_imp;
>> @@ -690,13 +692,13 @@ int scmi_handle_put(const struct scmi_handle *handle)
>> return 0;
>> }
>>
>> -static int scmi_xfer_info_init(struct scmi_info *sinfo)
>> +static int __scmi_xfer_info_init(struct scmi_info *sinfo, bool tx)
>> {
>> int i;
>> struct scmi_xfer *xfer;
>> struct device *dev = sinfo->dev;
>> const struct scmi_desc *desc = sinfo->desc;
>> - struct scmi_xfers_info *info = &sinfo->tx_minfo;
>> + struct scmi_xfers_info *info = tx ? &sinfo->tx_minfo : &sinfo->rx_minfo;
>
> Perhaps cleaner to just pass in the relevant info structure rather than a boolean
> to pick it. Saves people having to check if the boolean is saying it's
> tx or rx when reading the call sites.
>

Done in the upcoming v2.

Regards

Cristian

>>
>> /* Pre-allocated messages, no more than what hdr.seq can support */
>> if (WARN_ON(desc->max_msg >= MSG_TOKEN_MAX)) {
>> @@ -731,6 +733,16 @@ static int scmi_xfer_info_init(struct scmi_info *sinfo)
>> return 0;
>> }
>>
>> +static int scmi_xfer_info_init(struct scmi_info *sinfo)
>> +{
>> + int ret = __scmi_xfer_info_init(sinfo, true);
>> +
>> + if (!ret && idr_find(&sinfo->rx_idr, SCMI_PROTOCOL_BASE))
>> + ret = __scmi_xfer_info_init(sinfo, false);
>> +
>> + return ret;
>> +}
>> +
>> static int scmi_mailbox_check(struct device_node *np, int idx)
>> {
>> return of_parse_phandle_with_args(np, "mboxes", "#mbox-cells",
>> @@ -908,10 +920,6 @@ static int scmi_probe(struct platform_device *pdev)
>> info->desc = desc;
>> INIT_LIST_HEAD(&info->node);
>>
>> - ret = scmi_xfer_info_init(info);
>> - if (ret)
>> - return ret;
>> -
>> platform_set_drvdata(pdev, info);
>> idr_init(&info->tx_idr);
>> idr_init(&info->rx_idr);
>> @@ -924,6 +932,10 @@ static int scmi_probe(struct platform_device *pdev)
>> if (ret)
>> return ret;
>>
>> + ret = scmi_xfer_info_init(info);
>> + if (ret)
>> + return ret;
>> +
>> ret = scmi_base_protocol_init(handle);
>> if (ret) {
>> dev_err(dev, "unable to communicate with SCMI(%d)\n", ret);
>
>