| Message ID | 20190708154730.16643-8-sudeep.holla@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | firmware: arm_scmi: Add support for Rx, async commands and delayed response | expand |
Hi Sudeep, Just a comment in general. The asynchronous commands you are implementing are not really asynchronous to the caller. Yes it is is "async" at the low level, but there is no way to use scmi_do_xfer() or scmi_do_xfer_with_response() and have the calling thread be able to continue on in parallel with the command being processed by the platform. This will limit the types of applications that can use SCMI (perhaps this is intentional). I was hoping that true async would be possible, and that the caller could also register a callback function to be invoked when the command was completed. Is this something that may be added in the future? It does overlap with notifications, because with those messages you will need some kind of callback or handler thread. BTW, if scmi_do_xfer_with_response() returns --ETIMEDOUT the caller has no way of knowing whether it was the command ack timeout or the command execution timeout. Regards, Jim On Mon, Jul 8, 2019 at 11:47 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > Messages that are sent to platform, also known as commands and can be: > > 1. Synchronous commands that block the channel until the requested work > has been completed. The platform responds to these commands over the > same channel and hence can't be used to send another command until the > previous command has completed. > > 2. Asynchronous commands on the other hand, the platform schedules the > requested work to complete later in time and returns almost immediately > freeing the channel for new commands. The response indicates the success > or failure in the ability to schedule the requested work. When the work > has completed, the platform sends an additional delayed response message. > > Using the same transmit buffer used for sending the asynchronous command > even for the delayed response corresponding to it simplifies handling of > the delayed response. It's the caller of asynchronous command that is > responsible for allocating the completion flag that scmi driver can > complete to indicate the arrival of delayed response. > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_scmi/common.h | 6 ++++- > drivers/firmware/arm_scmi/driver.c | 43 ++++++++++++++++++++++++++++-- > 2 files changed, 46 insertions(+), 3 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h > index 4349d836b392..f89fa3f74a6f 100644 > --- a/drivers/firmware/arm_scmi/common.h > +++ b/drivers/firmware/arm_scmi/common.h > @@ -84,17 +84,21 @@ struct scmi_msg { > * @rx: Receive message, the buffer should be pre-allocated to store > * message. If request-ACK protocol is used, we can reuse the same > * buffer for the rx path as we use for the tx path. > - * @done: completion event > + * @done: command message transmit completion event > + * @async: pointer to delayed response message received event completion > */ > struct scmi_xfer { > struct scmi_msg_hdr hdr; > struct scmi_msg tx; > struct scmi_msg rx; > struct completion done; > + struct completion *async_done; > }; > > void scmi_xfer_put(const struct scmi_handle *h, struct scmi_xfer *xfer); > int scmi_do_xfer(const struct scmi_handle *h, struct scmi_xfer *xfer); > +int scmi_do_xfer_with_response(const struct scmi_handle *h, > + struct scmi_xfer *xfer); > int scmi_xfer_get_init(const struct scmi_handle *h, u8 msg_id, u8 prot_id, > size_t tx_size, size_t rx_size, struct scmi_xfer **p); > int scmi_handle_put(const struct scmi_handle *handle); > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > index b384c818d8dd..049bb4af6b60 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -347,6 +347,8 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo, struct scmi_xfer *xfer) > */ > static void scmi_rx_callback(struct mbox_client *cl, void *m) > { > + u8 msg_type; > + u32 msg_hdr; > u16 xfer_id; > struct scmi_xfer *xfer; > struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl); > @@ -355,7 +357,12 @@ static void scmi_rx_callback(struct mbox_client *cl, void *m) > struct scmi_xfers_info *minfo = &info->tx_minfo; > struct scmi_shared_mem __iomem *mem = cinfo->payload; > > - xfer_id = MSG_XTRACT_TOKEN(ioread32(&mem->msg_header)); > + msg_hdr = ioread32(&mem->msg_header); > + msg_type = MSG_XTRACT_TYPE(msg_hdr); > + xfer_id = MSG_XTRACT_TOKEN(msg_hdr); > + > + if (msg_type == MSG_TYPE_NOTIFICATION) > + return; /* Notifications not yet supported */ > > /* Are we even expecting this? */ > if (!test_bit(xfer_id, minfo->xfer_alloc_table)) { > @@ -368,7 +375,11 @@ static void scmi_rx_callback(struct mbox_client *cl, void *m) > scmi_dump_header_dbg(dev, &xfer->hdr); > > scmi_fetch_response(xfer, mem); > - complete(&xfer->done); > + > + if (msg_type == MSG_TYPE_DELAYED_RESP) > + complete(xfer->async_done); > + else > + complete(&xfer->done); > } > > /** > @@ -472,6 +483,34 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer) > return ret; > } > > +#define SCMI_MAX_RESPONSE_TIMEOUT (2 * MSEC_PER_SEC) > + > +/** > + * scmi_do_xfer_with_response() - Do one transfer and wait until the delayed > + * response is received > + * > + * @handle: Pointer to SCMI entity handle > + * @xfer: Transfer to initiate and wait for response > + * > + * Return: -ETIMEDOUT in case of no delayed response, if transmit error, > + * return corresponding error, else if all goes well, return 0. > + */ > +int scmi_do_xfer_with_response(const struct scmi_handle *handle, > + struct scmi_xfer *xfer) > +{ > + int ret, timeout = msecs_to_jiffies(SCMI_MAX_RESPONSE_TIMEOUT); > + DECLARE_COMPLETION_ONSTACK(async_response); > + > + xfer->async_done = &async_response; > + > + ret = scmi_do_xfer(handle, xfer); > + if (!ret && !wait_for_completion_timeout(xfer->async_done, timeout)) > + ret = -ETIMEDOUT; > + > + xfer->async_done = NULL; > + return ret; > +} > + > /** > * scmi_xfer_get_init() - Allocate and initialise one message for transmit > * > -- > 2.17.1 >
On Thu, Jul 18, 2019 at 05:38:06PM -0400, Jim Quinlan wrote: > Hi Sudeep, > > Just a comment in general. The asynchronous commands you are > implementing are not really asynchronous to the caller. Yes, but as per specification, the idea is to release the channel for other communication. > Yes it is is "async" at the low level, but there is no way to use > scmi_do_xfer() or scmi_do_xfer_with_response() and have the calling > thread be able to continue on in parallel with the command being > processed by the platform. This will limit the types of applications > that can use SCMI (perhaps this is intentional). Yes indeed, it's intentional and async is applicable for channel usage. > I was hoping that true async would be possible, and that the caller > could also register a callback function to be invoked when the command > was completed. Is this something that may be added in the future? This is how notifications are designed and must work. I would suggest to use notifications instead. Do you see any issues with that approach ? > It does overlap with notifications, because with those messages you > will need some kind of callback or handler thread. > Ah you do mention about overlap. I am replying as I read, sorry for that. Anyways, let me know if we can just use notifications. Also the current sync users(mainly clocks and sensors), may need even change in Linux infrastructure if we need to make it work in notification style. It would be good to know the use case you are referring. > BTW, if scmi_do_xfer_with_response() returns --ETIMEDOUT the caller > has no way of knowing whether it was the command ack timeout or the > command execution timeout. > Yes, I did think about this but I left it as is thinking it may not be important. Do you think that makes a difference ? I am fine to change if there are users that needs to handle the difference. -- Regards, Sudeep
On Fri, Jul 19, 2019 at 7:03 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Thu, Jul 18, 2019 at 05:38:06PM -0400, Jim Quinlan wrote: > > Hi Sudeep, > > > > Just a comment in general. The asynchronous commands you are > > implementing are not really asynchronous to the caller. > > Yes, but as per specification, the idea is to release the channel for > other communication. > > > Yes it is is "async" at the low level, but there is no way to use > > scmi_do_xfer() or scmi_do_xfer_with_response() and have the calling > > thread be able to continue on in parallel with the command being > > processed by the platform. This will limit the types of applications > > that can use SCMI (perhaps this is intentional). > > Yes indeed, it's intentional and async is applicable for channel usage. > > > I was hoping that true async would be possible, and that the caller > > could also register a callback function to be invoked when the command > > was completed. Is this something that may be added in the future? > > This is how notifications are designed and must work. I would suggest > to use notifications instead. Do you see any issues with that approach ? > > > It does overlap with notifications, because with those messages you > > will need some kind of callback or handler thread. > > > > Ah you do mention about overlap. I am replying as I read, sorry for that. > Anyways, let me know if we can just use notifications. Also the current > sync users(mainly clocks and sensors), may need even change in Linux > infrastructure if we need to make it work in notification style. > > It would be good to know the use case you are referring. Hi Sudeep, Well, I'm just curious how you would implement notifications. Would you have a per-protorcol callback? The Spec says that multiple agents can receive them; in our usage we have only one agent and it is Linux. We have one use case where that this patchset will do wonderfully. We have another use case where we would like to go crazy on the asynchrony of the messages (caller's perspective, that is). This usage, which I don't think I can talk about, would like to use notifications and a per-protocol callback function. > > > BTW, if scmi_do_xfer_with_response() returns --ETIMEDOUT the caller > > has no way of knowing whether it was the command ack timeout or the > > command execution timeout. > > > Yes, I did think about this but I left it as is thinking it may not be > important. Do you think that makes a difference ? I am fine to change > if there are users that needs to handle the difference. I can't think of a case where it would matter. Just thought I'd mention it. Cheers, Jim > > -- > Regards, > Sudeep
On Fri, Jul 19, 2019 at 04:16:02PM -0400, Jim Quinlan wrote: > On Fri, Jul 19, 2019 at 7:03 AM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Thu, Jul 18, 2019 at 05:38:06PM -0400, Jim Quinlan wrote: > > > Hi Sudeep, > > > > > > Just a comment in general. The asynchronous commands you are > > > implementing are not really asynchronous to the caller. > > > > Yes, but as per specification, the idea is to release the channel for > > other communication. > > > > > Yes it is is "async" at the low level, but there is no way to use > > > scmi_do_xfer() or scmi_do_xfer_with_response() and have the calling > > > thread be able to continue on in parallel with the command being > > > processed by the platform. This will limit the types of applications > > > that can use SCMI (perhaps this is intentional). > > > > Yes indeed, it's intentional and async is applicable for channel usage. > > > > > I was hoping that true async would be possible, and that the caller > > > could also register a callback function to be invoked when the command > > > was completed. Is this something that may be added in the future? > > > > This is how notifications are designed and must work. I would suggest > > to use notifications instead. Do you see any issues with that approach ? > > > > > It does overlap with notifications, because with those messages you > > > will need some kind of callback or handler thread. > > > > > > > Ah you do mention about overlap. I am replying as I read, sorry for that. > > Anyways, let me know if we can just use notifications. Also the current > > sync users(mainly clocks and sensors), may need even change in Linux > > infrastructure if we need to make it work in notification style. > > > > It would be good to know the use case you are referring. > Hi Sudeep, > > Well, I'm just curious how you would implement notifications. Would > you have a per-protorcol callback? The Spec says that multiple agents > can receive them; in our usage we have only one agent and it is Linux. > Well that's something I don't have straight answer yet. I am undecided on per-protocol callback or just one callback. Yes, the platform can get the same notification to multiple agents if they have subscribe for the same. It doesn't matter if you have one or multiple agents on you system. You have 2 actually: Linux and the Platform(which runs this SCMI compliant firmware). It could be dedicated power controller or you secure side firmware. > We have one use case where that this patchset will do wonderfully. We > have another use case where we would like to go crazy on the > asynchrony of the messages (caller's perspective, that is). > This usage, which I don't think I can talk about, would like to use > notifications and a per-protocol callback function. That's fine. I am interested to know the user, but I understand if you can't talk about it. > > > > > BTW, if scmi_do_xfer_with_response() returns --ETIMEDOUT the caller > > > has no way of knowing whether it was the command ack timeout or the > > > command execution timeout. > > > > > Yes, I did think about this but I left it as is thinking it may not be > > important. Do you think that makes a difference ? I am fine to change > > if there are users that needs to handle the difference. > I can't think of a case where it would matter. Just thought I'd mention it. > Thanks, I can add a note to ensure it's not lost, you raised valid points in review. -- Regards, Sudeep
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 4349d836b392..f89fa3f74a6f 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -84,17 +84,21 @@ struct scmi_msg { * @rx: Receive message, the buffer should be pre-allocated to store * message. If request-ACK protocol is used, we can reuse the same * buffer for the rx path as we use for the tx path. - * @done: completion event + * @done: command message transmit completion event + * @async: pointer to delayed response message received event completion */ struct scmi_xfer { struct scmi_msg_hdr hdr; struct scmi_msg tx; struct scmi_msg rx; struct completion done; + struct completion *async_done; }; void scmi_xfer_put(const struct scmi_handle *h, struct scmi_xfer *xfer); int scmi_do_xfer(const struct scmi_handle *h, struct scmi_xfer *xfer); +int scmi_do_xfer_with_response(const struct scmi_handle *h, + struct scmi_xfer *xfer); int scmi_xfer_get_init(const struct scmi_handle *h, u8 msg_id, u8 prot_id, size_t tx_size, size_t rx_size, struct scmi_xfer **p); int scmi_handle_put(const struct scmi_handle *handle); diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index b384c818d8dd..049bb4af6b60 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -347,6 +347,8 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo, struct scmi_xfer *xfer) */ static void scmi_rx_callback(struct mbox_client *cl, void *m) { + u8 msg_type; + u32 msg_hdr; u16 xfer_id; struct scmi_xfer *xfer; struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl); @@ -355,7 +357,12 @@ static void scmi_rx_callback(struct mbox_client *cl, void *m) struct scmi_xfers_info *minfo = &info->tx_minfo; struct scmi_shared_mem __iomem *mem = cinfo->payload; - xfer_id = MSG_XTRACT_TOKEN(ioread32(&mem->msg_header)); + msg_hdr = ioread32(&mem->msg_header); + msg_type = MSG_XTRACT_TYPE(msg_hdr); + xfer_id = MSG_XTRACT_TOKEN(msg_hdr); + + if (msg_type == MSG_TYPE_NOTIFICATION) + return; /* Notifications not yet supported */ /* Are we even expecting this? */ if (!test_bit(xfer_id, minfo->xfer_alloc_table)) { @@ -368,7 +375,11 @@ static void scmi_rx_callback(struct mbox_client *cl, void *m) scmi_dump_header_dbg(dev, &xfer->hdr); scmi_fetch_response(xfer, mem); - complete(&xfer->done); + + if (msg_type == MSG_TYPE_DELAYED_RESP) + complete(xfer->async_done); + else + complete(&xfer->done); } /** @@ -472,6 +483,34 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer) return ret; } +#define SCMI_MAX_RESPONSE_TIMEOUT (2 * MSEC_PER_SEC) + +/** + * scmi_do_xfer_with_response() - Do one transfer and wait until the delayed + * response is received + * + * @handle: Pointer to SCMI entity handle + * @xfer: Transfer to initiate and wait for response + * + * Return: -ETIMEDOUT in case of no delayed response, if transmit error, + * return corresponding error, else if all goes well, return 0. + */ +int scmi_do_xfer_with_response(const struct scmi_handle *handle, + struct scmi_xfer *xfer) +{ + int ret, timeout = msecs_to_jiffies(SCMI_MAX_RESPONSE_TIMEOUT); + DECLARE_COMPLETION_ONSTACK(async_response); + + xfer->async_done = &async_response; + + ret = scmi_do_xfer(handle, xfer); + if (!ret && !wait_for_completion_timeout(xfer->async_done, timeout)) + ret = -ETIMEDOUT; + + xfer->async_done = NULL; + return ret; +} + /** * scmi_xfer_get_init() - Allocate and initialise one message for transmit *
Messages that are sent to platform, also known as commands and can be: 1. Synchronous commands that block the channel until the requested work has been completed. The platform responds to these commands over the same channel and hence can't be used to send another command until the previous command has completed. 2. Asynchronous commands on the other hand, the platform schedules the requested work to complete later in time and returns almost immediately freeing the channel for new commands. The response indicates the success or failure in the ability to schedule the requested work. When the work has completed, the platform sends an additional delayed response message. Using the same transmit buffer used for sending the asynchronous command even for the delayed response corresponding to it simplifies handling of the delayed response. It's the caller of asynchronous command that is responsible for allocating the completion flag that scmi driver can complete to indicate the arrival of delayed response. Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/firmware/arm_scmi/common.h | 6 ++++- drivers/firmware/arm_scmi/driver.c | 43 ++++++++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 3 deletions(-) -- 2.17.1