Messages in this thread | | | Subject | Re: [RFC PATCH 01/11] firmware: arm_scmi: Add receive buffer support for notifications | From | Cristian Marussi <> | Date | Fri, 14 Feb 2020 15:25:57 +0000 |
| |
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); > >
| |