| Message ID | 20200120122333.46217-2-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SCMI Notifications Support | expand |
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 ;) 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. > > /* 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);
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); > >
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; /* 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);