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

[Cristian Marussi]

Hi

On 27/01/2020 17:07, Jonathan Cameron wrote:
> On Mon, 20 Jan 2020 12:23:23 +0000
> Cristian Marussi <cristian.marussi@xxxxxxx> wrote:
> 
>> From: Sudeep Holla <sudeep.holla@xxxxxxx>
>>
>> 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@xxxxxxx>
>> ---
>>  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);
> 
>