[PATCH 00/11] firmware: arm_scmi: Add support for Rx, async commands and delayed response

[PATCH 00/11] firmware: arm_scmi: Add support for Rx, async commands and delayed response

[Sudeep Holla]

Hi,

This patch series adds SCMI infrastructure/core support for recieve(Rx)
channels, asynchronous commands and delayed response. It adds async
command support for clock rate setting and sensor reading based on the
attributes read from the firmware.

The code is rebased on the cleanup series[1] and is available @[2]

--
Regards,
Sudeep

[1] https://lore.kernel.org/lkml/20190708154358.16227-1-sudeep.holla@arm.com
[2] git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git scmi_updates

Sudeep Holla (11):
  firmware: arm_scmi: Reorder some functions to avoid forward declarations
  firmware: arm_scmi: Segregate tx channel handling and prepare to add rx
  firmware: arm_scmi: Add receive channel support for notifications
  firmware: arm_scmi: Separate out tx buffer handling and prepare to add rx
  firmware: arm_scmi: Add receive buffer support for notifications
  firmware: arm_scmi: Add mechanism to unpack message headers
  firmware: arm_scmi: Add support for asynchronous commands and delayed response
  firmware: arm_scmi: Drop async flag in sensor_ops->reading_get
  firmware: arm_scmi: Add asynchronous sensor read if it supports
  firmware: arm_scmi: Drop config flag in clk_ops->rate_set
  firmware: arm_scmi: Use asynchronous CLOCK_RATE_SET when possible

 drivers/clk/clk-scmi.c              |   2 +-
 drivers/firmware/arm_scmi/clock.c   |  23 +-
 drivers/firmware/arm_scmi/common.h  |   6 +-
 drivers/firmware/arm_scmi/driver.c  | 346 ++++++++++++++++++----------
 drivers/firmware/arm_scmi/sensors.c |  32 ++-
 drivers/hwmon/scmi-hwmon.c          |   2 +-
 include/linux/scmi_protocol.h       |   6 +-
 7 files changed, 280 insertions(+), 137 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 01/11] firmware: arm_scmi: Reorder some functions to avoid forward declarations

[Sudeep Holla]

Re-shuffling few functions to keep definitions and their usages close.
This is also needed to avoid too many unnecessary forward declarations
while adding new features(delayed response and notifications).

Keeping this separate to avoid mixing up of these trivial change that
doesn't affect functionality into the ones that does.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 180 ++++++++++++++---------------
 1 file changed, 90 insertions(+), 90 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 765573756987..0bd2af0a008f 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -197,44 +197,6 @@ static void scmi_fetch_response(struct scmi_xfer *xfer,
 	memcpy_fromio(xfer->rx.buf, mem->msg_payload + 4, xfer->rx.len);
 }
 
-/**
- * scmi_rx_callback() - mailbox client callback for receive messages
- *
- * @cl: client pointer
- * @m: mailbox message
- *
- * Processes one received message to appropriate transfer information and
- * signals completion of the transfer.
- *
- * NOTE: This function will be invoked in IRQ context, hence should be
- * as optimal as possible.
- */
-static void scmi_rx_callback(struct mbox_client *cl, void *m)
-{
-	u16 xfer_id;
-	struct scmi_xfer *xfer;
-	struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
-	struct device *dev = cinfo->dev;
-	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
-	struct scmi_xfers_info *minfo = &info->minfo;
-	struct scmi_shared_mem __iomem *mem = cinfo->payload;
-
-	xfer_id = MSG_XTRACT_TOKEN(ioread32(&mem->msg_header));
-
-	/* Are we even expecting this? */
-	if (!test_bit(xfer_id, minfo->xfer_alloc_table)) {
-		dev_err(dev, "message for %d is not expected!\n", xfer_id);
-		return;
-	}
-
-	xfer = &minfo->xfer_block[xfer_id];
-
-	scmi_dump_header_dbg(dev, &xfer->hdr);
-
-	scmi_fetch_response(xfer, mem);
-	complete(&xfer->done);
-}
-
 /**
  * pack_scmi_header() - packs and returns 32-bit header
  *
@@ -349,6 +311,44 @@ void scmi_xfer_put(const struct scmi_handle *handle, struct scmi_xfer *xfer)
 	spin_unlock_irqrestore(&minfo->xfer_lock, flags);
 }
 
+/**
+ * scmi_rx_callback() - mailbox client callback for receive messages
+ *
+ * @cl: client pointer
+ * @m: mailbox message
+ *
+ * Processes one received message to appropriate transfer information and
+ * signals completion of the transfer.
+ *
+ * NOTE: This function will be invoked in IRQ context, hence should be
+ * as optimal as possible.
+ */
+static void scmi_rx_callback(struct mbox_client *cl, void *m)
+{
+	u16 xfer_id;
+	struct scmi_xfer *xfer;
+	struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
+	struct device *dev = cinfo->dev;
+	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
+	struct scmi_xfers_info *minfo = &info->minfo;
+	struct scmi_shared_mem __iomem *mem = cinfo->payload;
+
+	xfer_id = MSG_XTRACT_TOKEN(ioread32(&mem->msg_header));
+
+	/* Are we even expecting this? */
+	if (!test_bit(xfer_id, minfo->xfer_alloc_table)) {
+		dev_err(dev, "message for %d is not expected!\n", xfer_id);
+		return;
+	}
+
+	xfer = &minfo->xfer_block[xfer_id];
+
+	scmi_dump_header_dbg(dev, &xfer->hdr);
+
+	scmi_fetch_response(xfer, mem);
+	complete(&xfer->done);
+}
+
 static bool
 scmi_xfer_poll_done(const struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
 {
@@ -599,20 +599,6 @@ int scmi_handle_put(const struct scmi_handle *handle)
 	return 0;
 }
 
-static const struct scmi_desc scmi_generic_desc = {
-	.max_rx_timeout_ms = 30,	/* We may increase this if required */
-	.max_msg = 20,		/* Limited by MBOX_TX_QUEUE_LEN */
-	.max_msg_size = 128,
-};
-
-/* Each compatible listed below must have descriptor associated with it */
-static const struct of_device_id scmi_of_match[] = {
-	{ .compatible = "arm,scmi", .data = &scmi_generic_desc },
-	{ /* Sentinel */ },
-};
-
-MODULE_DEVICE_TABLE(of, scmi_of_match);
-
 static int scmi_xfer_info_init(struct scmi_info *sinfo)
 {
 	int i;
@@ -659,44 +645,6 @@ static int scmi_mailbox_check(struct device_node *np)
 	return of_parse_phandle_with_args(np, "mboxes", "#mbox-cells", 0, NULL);
 }
 
-static int scmi_mbox_free_channel(int id, void *p, void *data)
-{
-	struct scmi_chan_info *cinfo = p;
-	struct idr *idr = data;
-
-	if (!IS_ERR_OR_NULL(cinfo->chan)) {
-		mbox_free_channel(cinfo->chan);
-		cinfo->chan = NULL;
-	}
-
-	idr_remove(idr, id);
-
-	return 0;
-}
-
-static int scmi_remove(struct platform_device *pdev)
-{
-	int ret = 0;
-	struct scmi_info *info = platform_get_drvdata(pdev);
-	struct idr *idr = &info->tx_idr;
-
-	mutex_lock(&scmi_list_mutex);
-	if (info->users)
-		ret = -EBUSY;
-	else
-		list_del(&info->node);
-	mutex_unlock(&scmi_list_mutex);
-
-	if (ret)
-		return ret;
-
-	/* Safe to free channels since no more users */
-	ret = idr_for_each(idr, scmi_mbox_free_channel, idr);
-	idr_destroy(&info->tx_idr);
-
-	return ret;
-}
-
 static inline int
 scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
 {
@@ -856,6 +804,58 @@ static int scmi_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int scmi_mbox_free_channel(int id, void *p, void *data)
+{
+	struct scmi_chan_info *cinfo = p;
+	struct idr *idr = data;
+
+	if (!IS_ERR_OR_NULL(cinfo->chan)) {
+		mbox_free_channel(cinfo->chan);
+		cinfo->chan = NULL;
+	}
+
+	idr_remove(idr, id);
+
+	return 0;
+}
+
+static int scmi_remove(struct platform_device *pdev)
+{
+	int ret = 0;
+	struct scmi_info *info = platform_get_drvdata(pdev);
+	struct idr *idr = &info->tx_idr;
+
+	mutex_lock(&scmi_list_mutex);
+	if (info->users)
+		ret = -EBUSY;
+	else
+		list_del(&info->node);
+	mutex_unlock(&scmi_list_mutex);
+
+	if (ret)
+		return ret;
+
+	/* Safe to free channels since no more users */
+	ret = idr_for_each(idr, scmi_mbox_free_channel, idr);
+	idr_destroy(&info->tx_idr);
+
+	return ret;
+}
+
+static const struct scmi_desc scmi_generic_desc = {
+	.max_rx_timeout_ms = 30,	/* We may increase this if required */
+	.max_msg = 20,		/* Limited by MBOX_TX_QUEUE_LEN */
+	.max_msg_size = 128,
+};
+
+/* Each compatible listed below must have descriptor associated with it */
+static const struct of_device_id scmi_of_match[] = {
+	{ .compatible = "arm,scmi", .data = &scmi_generic_desc },
+	{ /* Sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, scmi_of_match);
+
 static struct platform_driver scmi_driver = {
 	.driver = {
 		   .name = "arm-scmi",
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 02/11] firmware: arm_scmi: Segregate tx channel handling and prepare to add rx

[Sudeep Holla]

The transmit(Tx) channels are specified as the first entry and the
receive(Rx) channels are the second entry as per the device tree
bindings. Since we currently just support Tx, index 0 is hardcoded at
all required callsites.

In order to prepare for adding Rx support, let's remove those hardcoded
index and add boolean parameter to identify Tx/Rx channels when setting
them up.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 33 ++++++++++++++++--------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 0bd2af0a008f..f7fb6d5bfc64 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -112,7 +112,7 @@ struct scmi_chan_info {
  * @version: SCMI revision information containing protocol version,
  *	implementation version and (sub-)vendor identification.
  * @minfo: Message info
- * @tx_idr: IDR object to map protocol id to channel info pointer
+ * @tx_idr: IDR object to map protocol id to Tx channel info pointer
  * @protocols_imp: List of protocols implemented, currently maximum of
  *	MAX_PROTOCOLS_IMP elements allocated by the base protocol
  * @node: List head
@@ -640,22 +640,26 @@ static int scmi_xfer_info_init(struct scmi_info *sinfo)
 	return 0;
 }
 
-static int scmi_mailbox_check(struct device_node *np)
+static int scmi_mailbox_check(struct device_node *np, int idx)
 {
-	return of_parse_phandle_with_args(np, "mboxes", "#mbox-cells", 0, NULL);
+	return of_parse_phandle_with_args(np, "mboxes", "#mbox-cells",
+					  idx, NULL);
 }
 
-static inline int
-scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
+static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
+				int prot_id, bool tx)
 {
-	int ret;
+	int ret, idx;
 	struct resource res;
 	resource_size_t size;
 	struct device_node *shmem, *np = dev->of_node;
 	struct scmi_chan_info *cinfo;
 	struct mbox_client *cl;
 
-	if (scmi_mailbox_check(np)) {
+	/* Transmit channel is first entry i.e. index 0 */
+	idx = tx ? 0 : 1;
+
+	if (scmi_mailbox_check(np, idx)) {
 		cinfo = idr_find(&info->tx_idr, SCMI_PROTOCOL_BASE);
 		goto idr_alloc;
 	}
@@ -669,11 +673,11 @@ scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
 	cl = &cinfo->cl;
 	cl->dev = dev;
 	cl->rx_callback = scmi_rx_callback;
-	cl->tx_prepare = scmi_tx_prepare;
+	cl->tx_prepare = tx ? scmi_tx_prepare : NULL;
 	cl->tx_block = false;
-	cl->knows_txdone = true;
+	cl->knows_txdone = tx;
 
-	shmem = of_parse_phandle(np, "shmem", 0);
+	shmem = of_parse_phandle(np, "shmem", idx);
 	ret = of_address_to_resource(shmem, 0, &res);
 	of_node_put(shmem);
 	if (ret) {
@@ -688,8 +692,7 @@ scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
 		return -EADDRNOTAVAIL;
 	}
 
-	/* Transmit channel is first entry i.e. index 0 */
-	cinfo->chan = mbox_request_channel(cl, 0);
+	cinfo->chan = mbox_request_channel(cl, idx);
 	if (IS_ERR(cinfo->chan)) {
 		ret = PTR_ERR(cinfo->chan);
 		if (ret != -EPROBE_DEFER)
@@ -721,7 +724,7 @@ scmi_create_protocol_device(struct device_node *np, struct scmi_info *info,
 		return;
 	}
 
-	if (scmi_mbox_chan_setup(info, &sdev->dev, prot_id)) {
+	if (scmi_mbox_chan_setup(info, &sdev->dev, prot_id, true)) {
 		dev_err(&sdev->dev, "failed to setup transport\n");
 		scmi_device_destroy(sdev);
 		return;
@@ -741,7 +744,7 @@ static int scmi_probe(struct platform_device *pdev)
 	struct device_node *child, *np = dev->of_node;
 
 	/* Only mailbox method supported, check for the presence of one */
-	if (scmi_mailbox_check(np)) {
+	if (scmi_mailbox_check(np, 0)) {
 		dev_err(dev, "no mailbox found in %pOF\n", np);
 		return -EINVAL;
 	}
@@ -769,7 +772,7 @@ static int scmi_probe(struct platform_device *pdev)
 	handle->dev = info->dev;
 	handle->version = &info->version;
 
-	ret = scmi_mbox_chan_setup(info, dev, SCMI_PROTOCOL_BASE);
+	ret = scmi_mbox_chan_setup(info, dev, SCMI_PROTOCOL_BASE, true);
 	if (ret)
 		return ret;
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/11] firmware: arm_scmi: Segregate tx channel handling and prepare to add rx

[Jim Quinlan]

On Mon, Jul 8, 2019 at 11:47 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> The transmit(Tx) channels are specified as the first entry and the
> receive(Rx) channels are the second entry as per the device tree
> bindings. Since we currently just support Tx, index 0 is hardcoded at
> all required callsites.
>
> In order to prepare for adding Rx support, let's remove those hardcoded
> index and add boolean parameter to identify Tx/Rx channels when setting
> them up.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/driver.c | 33 ++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 0bd2af0a008f..f7fb6d5bfc64 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -112,7 +112,7 @@ struct scmi_chan_info {
>   * @version: SCMI revision information containing protocol version,
>   *     implementation version and (sub-)vendor identification.
>   * @minfo: Message info
> - * @tx_idr: IDR object to map protocol id to channel info pointer
> + * @tx_idr: IDR object to map protocol id to Tx channel info pointer
>   * @protocols_imp: List of protocols implemented, currently maximum of
>   *     MAX_PROTOCOLS_IMP elements allocated by the base protocol
>   * @node: List head
> @@ -640,22 +640,26 @@ static int scmi_xfer_info_init(struct scmi_info *sinfo)
>         return 0;
>  }
>
> -static int scmi_mailbox_check(struct device_node *np)
> +static int scmi_mailbox_check(struct device_node *np, int idx)
>  {
> -       return of_parse_phandle_with_args(np, "mboxes", "#mbox-cells", 0, NULL);
> +       return of_parse_phandle_with_args(np, "mboxes", "#mbox-cells",
> +                                         idx, NULL);
>  }
>
> -static inline int
> -scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
> +static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
> +                               int prot_id, bool tx)
>  {
> -       int ret;
> +       int ret, idx;
>         struct resource res;
>         resource_size_t size;
>         struct device_node *shmem, *np = dev->of_node;
>         struct scmi_chan_info *cinfo;
>         struct mbox_client *cl;
>
> -       if (scmi_mailbox_check(np)) {
> +       /* Transmit channel is first entry i.e. index 0 */
> +       idx = tx ? 0 : 1;
> +
> +       if (scmi_mailbox_check(np, idx)) {
>                 cinfo = idr_find(&info->tx_idr, SCMI_PROTOCOL_BASE);
>                 goto idr_alloc;
>         }
> @@ -669,11 +673,11 @@ scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
>         cl = &cinfo->cl;
>         cl->dev = dev;
>         cl->rx_callback = scmi_rx_callback;
> -       cl->tx_prepare = scmi_tx_prepare;
> +       cl->tx_prepare = tx ? scmi_tx_prepare : NULL;
>         cl->tx_block = false;
> -       cl->knows_txdone = true;
> +       cl->knows_txdone = tx;
>
> -       shmem = of_parse_phandle(np, "shmem", 0);
> +       shmem = of_parse_phandle(np, "shmem", idx);
Hi Sudeep,

You can't see it in the diff but you have two error messages that use
"Tx"; should this be changed to "Tx/Rx"?

Jim
>         ret = of_address_to_resource(shmem, 0, &res);
>         of_node_put(shmem);
>         if (ret) {
> @@ -688,8 +692,7 @@ scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
>                 return -EADDRNOTAVAIL;
>         }
>
> -       /* Transmit channel is first entry i.e. index 0 */
> -       cinfo->chan = mbox_request_channel(cl, 0);
> +       cinfo->chan = mbox_request_channel(cl, idx);
>         if (IS_ERR(cinfo->chan)) {
>                 ret = PTR_ERR(cinfo->chan);
>                 if (ret != -EPROBE_DEFER)
> @@ -721,7 +724,7 @@ scmi_create_protocol_device(struct device_node *np, struct scmi_info *info,
>                 return;
>         }
>
> -       if (scmi_mbox_chan_setup(info, &sdev->dev, prot_id)) {
> +       if (scmi_mbox_chan_setup(info, &sdev->dev, prot_id, true)) {
>                 dev_err(&sdev->dev, "failed to setup transport\n");
>                 scmi_device_destroy(sdev);
>                 return;
> @@ -741,7 +744,7 @@ static int scmi_probe(struct platform_device *pdev)
>         struct device_node *child, *np = dev->of_node;
>
>         /* Only mailbox method supported, check for the presence of one */
> -       if (scmi_mailbox_check(np)) {
> +       if (scmi_mailbox_check(np, 0)) {
>                 dev_err(dev, "no mailbox found in %pOF\n", np);
>                 return -EINVAL;
>         }
> @@ -769,7 +772,7 @@ static int scmi_probe(struct platform_device *pdev)
>         handle->dev = info->dev;
>         handle->version = &info->version;
>
> -       ret = scmi_mbox_chan_setup(info, dev, SCMI_PROTOCOL_BASE);
> +       ret = scmi_mbox_chan_setup(info, dev, SCMI_PROTOCOL_BASE, true);
>         if (ret)
>                 return ret;
>
> --
> 2.17.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/11] firmware: arm_scmi: Segregate tx channel handling and prepare to add rx

[Sudeep Holla]

On Thu, Jul 18, 2019 at 05:23:10PM -0400, Jim Quinlan wrote:
> On Mon, Jul 8, 2019 at 11:47 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > The transmit(Tx) channels are specified as the first entry and the
> > receive(Rx) channels are the second entry as per the device tree
> > bindings. Since we currently just support Tx, index 0 is hardcoded at
> > all required callsites.
> >
> > In order to prepare for adding Rx support, let's remove those hardcoded
> > index and add boolean parameter to identify Tx/Rx channels when setting
> > them up.
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/driver.c | 33 ++++++++++++++++--------------
> >  1 file changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 0bd2af0a008f..f7fb6d5bfc64 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -112,7 +112,7 @@ struct scmi_chan_info {
> >   * @version: SCMI revision information containing protocol version,
> >   *     implementation version and (sub-)vendor identification.
> >   * @minfo: Message info
> > - * @tx_idr: IDR object to map protocol id to channel info pointer
> > + * @tx_idr: IDR object to map protocol id to Tx channel info pointer
> >   * @protocols_imp: List of protocols implemented, currently maximum of
> >   *     MAX_PROTOCOLS_IMP elements allocated by the base protocol
> >   * @node: List head
> > @@ -640,22 +640,26 @@ static int scmi_xfer_info_init(struct scmi_info *sinfo)
> >         return 0;
> >  }
> >
> > -static int scmi_mailbox_check(struct device_node *np)
> > +static int scmi_mailbox_check(struct device_node *np, int idx)
> >  {
> > -       return of_parse_phandle_with_args(np, "mboxes", "#mbox-cells", 0, NULL);
> > +       return of_parse_phandle_with_args(np, "mboxes", "#mbox-cells",
> > +                                         idx, NULL);
> >  }
> >
> > -static inline int
> > -scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
> > +static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
> > +                               int prot_id, bool tx)
> >  {
> > -       int ret;
> > +       int ret, idx;
> >         struct resource res;
> >         resource_size_t size;
> >         struct device_node *shmem, *np = dev->of_node;
> >         struct scmi_chan_info *cinfo;
> >         struct mbox_client *cl;
> >
> > -       if (scmi_mailbox_check(np)) {
> > +       /* Transmit channel is first entry i.e. index 0 */
> > +       idx = tx ? 0 : 1;
> > +
> > +       if (scmi_mailbox_check(np, idx)) {
> >                 cinfo = idr_find(&info->tx_idr, SCMI_PROTOCOL_BASE);
> >                 goto idr_alloc;
> >         }
> > @@ -669,11 +673,11 @@ scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev, int prot_id)
> >         cl = &cinfo->cl;
> >         cl->dev = dev;
> >         cl->rx_callback = scmi_rx_callback;
> > -       cl->tx_prepare = scmi_tx_prepare;
> > +       cl->tx_prepare = tx ? scmi_tx_prepare : NULL;
> >         cl->tx_block = false;
> > -       cl->knows_txdone = true;
> > +       cl->knows_txdone = tx;
> >
> > -       shmem = of_parse_phandle(np, "shmem", 0);
> > +       shmem = of_parse_phandle(np, "shmem", idx);
> Hi Sudeep,
> 
> You can't see it in the diff but you have two error messages that use
> "Tx"; should this be changed to "Tx/Rx"?
> 

Thanks for spotting, will fix it.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 03/11] firmware: arm_scmi: Add receive channel support for notifications

[Sudeep Holla]

With scmi_mbox_chan_setup enabled to identify and setup both Tx and Rx,
let's consolidate setting up of both the channels under the function
scmi_mbox_txrx_setup.

Since some platforms may opt not to support notifications or delayed
response, they may not need support for Rx. Hence Rx is optional and
failure of setting one up is not considered fatal.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index f7fb6d5bfc64..e9b762348eff 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -113,6 +113,7 @@ struct scmi_chan_info {
  *	implementation version and (sub-)vendor identification.
  * @minfo: Message 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
  *	MAX_PROTOCOLS_IMP elements allocated by the base protocol
  * @node: List head
@@ -125,6 +126,7 @@ struct scmi_info {
 	struct scmi_handle handle;
 	struct scmi_xfers_info minfo;
 	struct idr tx_idr;
+	struct idr rx_idr;
 	u8 *protocols_imp;
 	struct list_head node;
 	int users;
@@ -655,12 +657,16 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
 	struct device_node *shmem, *np = dev->of_node;
 	struct scmi_chan_info *cinfo;
 	struct mbox_client *cl;
+	struct idr *idr;
 
 	/* Transmit channel is first entry i.e. index 0 */
 	idx = tx ? 0 : 1;
+	idr = tx ? &info->tx_idr : &info->rx_idr;
 
 	if (scmi_mailbox_check(np, idx)) {
-		cinfo = idr_find(&info->tx_idr, SCMI_PROTOCOL_BASE);
+		cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
+		if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
+			return -EINVAL;
 		goto idr_alloc;
 	}
 
@@ -701,7 +707,7 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
 	}
 
 idr_alloc:
-	ret = idr_alloc(&info->tx_idr, cinfo, prot_id, prot_id + 1, GFP_KERNEL);
+	ret = idr_alloc(idr, cinfo, prot_id, prot_id + 1, GFP_KERNEL);
 	if (ret != prot_id) {
 		dev_err(dev, "unable to allocate SCMI idr slot err %d\n", ret);
 		return ret;
@@ -711,6 +717,17 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
 	return 0;
 }
 
+static inline int
+scmi_mbox_txrx_setup(struct scmi_info *info, struct device *dev, int prot_id)
+{
+	int ret = scmi_mbox_chan_setup(info, dev, prot_id, true);
+
+	if (!ret) /* Rx is optional, hence no error check */
+		scmi_mbox_chan_setup(info, dev, prot_id, false);
+
+	return ret;
+}
+
 static inline void
 scmi_create_protocol_device(struct device_node *np, struct scmi_info *info,
 			    int prot_id)
@@ -724,7 +741,7 @@ scmi_create_protocol_device(struct device_node *np, struct scmi_info *info,
 		return;
 	}
 
-	if (scmi_mbox_chan_setup(info, &sdev->dev, prot_id, true)) {
+	if (scmi_mbox_txrx_setup(info, &sdev->dev, prot_id)) {
 		dev_err(&sdev->dev, "failed to setup transport\n");
 		scmi_device_destroy(sdev);
 		return;
@@ -767,12 +784,13 @@ static int scmi_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, info);
 	idr_init(&info->tx_idr);
+	idr_init(&info->rx_idr);
 
 	handle = &info->handle;
 	handle->dev = info->dev;
 	handle->version = &info->version;
 
-	ret = scmi_mbox_chan_setup(info, dev, SCMI_PROTOCOL_BASE, true);
+	ret = scmi_mbox_txrx_setup(info, dev, SCMI_PROTOCOL_BASE);
 	if (ret)
 		return ret;
 
@@ -842,6 +860,10 @@ static int scmi_remove(struct platform_device *pdev)
 	ret = idr_for_each(idr, scmi_mbox_free_channel, idr);
 	idr_destroy(&info->tx_idr);
 
+	idr = &info->rx_idr;
+	ret = idr_for_each(idr, scmi_mbox_free_channel, idr);
+	idr_destroy(&info->rx_idr);
+
 	return ret;
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 04/11] firmware: arm_scmi: Separate out tx buffer handling and prepare to add rx

[Sudeep Holla]

Currently we pre-allocate transmit buffers only and use the first free
slot in that pre-allocated buffer for transmitting any new message that
are generally originated from OS to the platform firmware.

Notifications or the delayed responses on the other hand are originated
from the platform firmware and consumes by the OS. It's better to have
separate and dedicated pre-allocated buffers to handle the notifications.
We can still use the transmit buffers for the delayed responses.

In addition, let's prepare existing scmi_xfer_{get,put} for acquiring
and releasing a slot to identify the right(tx/rx) buffers.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 40 ++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index e9b762348eff..1a7ffd3f8534 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -111,7 +111,7 @@ struct scmi_chan_info {
  * @handle: Instance of SCMI handle to send to clients
  * @version: SCMI revision information containing protocol version,
  *	implementation version and (sub-)vendor identification.
- * @minfo: Message info
+ * @tx_minfo: Universal Transmit 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
@@ -124,7 +124,7 @@ struct scmi_info {
 	const struct scmi_desc *desc;
 	struct scmi_revision_info version;
 	struct scmi_handle handle;
-	struct scmi_xfers_info minfo;
+	struct scmi_xfers_info tx_minfo;
 	struct idr tx_idr;
 	struct idr rx_idr;
 	u8 *protocols_imp;
@@ -251,6 +251,7 @@ static void scmi_tx_prepare(struct mbox_client *cl, void *m)
  * scmi_xfer_get() - Allocate one message
  *
  * @handle: Pointer to SCMI entity handle
+ * @minfo: Pointer to Tx/Rx Message management info based on channel type
  *
  * Helper function which is used by various message functions that are
  * exposed to clients of this driver for allocating a message traffic event.
@@ -261,13 +262,13 @@ static void scmi_tx_prepare(struct mbox_client *cl, void *m)
  *
  * Return: 0 if all went fine, else corresponding error.
  */
-static struct scmi_xfer *scmi_xfer_get(const struct scmi_handle *handle)
+static struct scmi_xfer *scmi_xfer_get(const struct scmi_handle *handle,
+				       struct scmi_xfers_info *minfo)
 {
 	u16 xfer_id;
 	struct scmi_xfer *xfer;
 	unsigned long flags, bit_pos;
 	struct scmi_info *info = handle_to_scmi_info(handle);
-	struct scmi_xfers_info *minfo = &info->minfo;
 
 	/* Keep the locked section as small as possible */
 	spin_lock_irqsave(&minfo->xfer_lock, flags);
@@ -290,18 +291,17 @@ static struct scmi_xfer *scmi_xfer_get(const struct scmi_handle *handle)
 }
 
 /**
- * scmi_xfer_put() - Release a message
+ * __scmi_xfer_put() - Release a message
  *
- * @handle: Pointer to SCMI entity handle
+ * @minfo: Pointer to Tx/Rx Message management info based on channel type
  * @xfer: message that was reserved by scmi_xfer_get
  *
  * This holds a spinlock to maintain integrity of internal data structures.
  */
-void scmi_xfer_put(const struct scmi_handle *handle, struct scmi_xfer *xfer)
+static void
+__scmi_xfer_put(struct scmi_xfers_info *minfo, struct scmi_xfer *xfer)
 {
 	unsigned long flags;
-	struct scmi_info *info = handle_to_scmi_info(handle);
-	struct scmi_xfers_info *minfo = &info->minfo;
 
 	/*
 	 * Keep the locked section as small as possible
@@ -332,7 +332,7 @@ static void scmi_rx_callback(struct mbox_client *cl, void *m)
 	struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
 	struct device *dev = cinfo->dev;
 	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
-	struct scmi_xfers_info *minfo = &info->minfo;
+	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));
@@ -351,6 +351,19 @@ static void scmi_rx_callback(struct mbox_client *cl, void *m)
 	complete(&xfer->done);
 }
 
+/**
+ * scmi_xfer_put() - Release a transmit message
+ *
+ * @handle: Pointer to SCMI entity handle
+ * @xfer: message that was reserved by scmi_xfer_get
+ */
+void scmi_xfer_put(const struct scmi_handle *handle, struct scmi_xfer *xfer)
+{
+	struct scmi_info *info = handle_to_scmi_info(handle);
+
+	__scmi_xfer_put(&info->tx_minfo, xfer);
+}
+
 static bool
 scmi_xfer_poll_done(const struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
 {
@@ -440,7 +453,7 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
 }
 
 /**
- * scmi_xfer_get_init() - Allocate and initialise one message
+ * scmi_xfer_get_init() - Allocate and initialise one message for transmit
  *
  * @handle: Pointer to SCMI entity handle
  * @msg_id: Message identifier
@@ -461,6 +474,7 @@ int scmi_xfer_get_init(const struct scmi_handle *handle, u8 msg_id, u8 prot_id,
 	int ret;
 	struct scmi_xfer *xfer;
 	struct scmi_info *info = handle_to_scmi_info(handle);
+	struct scmi_xfers_info *minfo = &info->tx_minfo;
 	struct device *dev = info->dev;
 
 	/* Ensure we have sane transfer sizes */
@@ -468,7 +482,7 @@ int scmi_xfer_get_init(const struct scmi_handle *handle, u8 msg_id, u8 prot_id,
 	    tx_size > info->desc->max_msg_size)
 		return -ERANGE;
 
-	xfer = scmi_xfer_get(handle);
+	xfer = scmi_xfer_get(handle, minfo);
 	if (IS_ERR(xfer)) {
 		ret = PTR_ERR(xfer);
 		dev_err(dev, "failed to get free message slot(%d)\n", ret);
@@ -607,7 +621,7 @@ static int scmi_xfer_info_init(struct scmi_info *sinfo)
 	struct scmi_xfer *xfer;
 	struct device *dev = sinfo->dev;
 	const struct scmi_desc *desc = sinfo->desc;
-	struct scmi_xfers_info *info = &sinfo->minfo;
+	struct scmi_xfers_info *info = &sinfo->tx_minfo;
 
 	/* Pre-allocated messages, no more than what hdr.seq can support */
 	if (WARN_ON(desc->max_msg >= MSG_TOKEN_MAX)) {
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 05/11] firmware: arm_scmi: Add receive buffer support for notifications

[Sudeep Holla]

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.

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 1a7ffd3f8534..eb5a2f271806 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -112,6 +112,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
@@ -125,6 +126,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;
@@ -615,13 +617,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)) {
@@ -656,6 +658,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",
@@ -792,10 +804,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);
@@ -808,6 +816,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);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jim Quinlan]

Hi Sudeep,

Would it make sense to save commits that support notifications for
when you actually support them (correct me if I wrong, but this
commit-set does not implement notifications.

Jim

On Mon, Jul 8, 2019 at 11:47 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> 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.
>
> 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 1a7ffd3f8534..eb5a2f271806 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -112,6 +112,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
> @@ -125,6 +126,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;
> @@ -615,13 +617,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)) {
> @@ -656,6 +658,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",
> @@ -792,10 +804,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);
> @@ -808,6 +816,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);
> --
> 2.17.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Thu, Jul 18, 2019 at 05:14:55PM -0400, Jim Quinlan wrote:
> Hi Sudeep,
>
> I'm not sure why you are adding code for notifications when this commit-set
> does not support notifications.  Why not save this commit for when you add
> notifications?
>

Right, I missed this patch and sent. Yes we just need Rx channels and not
Rx buffers to implement asynchronous commands. I will drop it from this set.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 06/11] firmware: arm_scmi: Add mechanism to unpack message headers

[Sudeep Holla]

In order to identify the message type when a response arrives, we need
a mechanism to unpack the message header similar to packing. Let's
add one.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index eb5a2f271806..b384c818d8dd 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -30,8 +30,14 @@
 #include "common.h"
 
 #define MSG_ID_MASK		GENMASK(7, 0)
+#define MSG_XTRACT_ID(hdr)	FIELD_GET(MSG_ID_MASK, (hdr))
 #define MSG_TYPE_MASK		GENMASK(9, 8)
+#define MSG_XTRACT_TYPE(hdr)	FIELD_GET(MSG_TYPE_MASK, (hdr))
+#define MSG_TYPE_COMMAND	0
+#define MSG_TYPE_DELAYED_RESP	2
+#define MSG_TYPE_NOTIFICATION	3
 #define MSG_PROTOCOL_ID_MASK	GENMASK(17, 10)
+#define MSG_XTRACT_PROT_ID(hdr)	FIELD_GET(MSG_PROTOCOL_ID_MASK, (hdr))
 #define MSG_TOKEN_ID_MASK	GENMASK(27, 18)
 #define MSG_XTRACT_TOKEN(hdr)	FIELD_GET(MSG_TOKEN_ID_MASK, (hdr))
 #define MSG_TOKEN_MAX		(MSG_XTRACT_TOKEN(MSG_TOKEN_ID_MASK) + 1)
@@ -216,6 +222,18 @@ static inline u32 pack_scmi_header(struct scmi_msg_hdr *hdr)
 		FIELD_PREP(MSG_PROTOCOL_ID_MASK, hdr->protocol_id);
 }
 
+/**
+ * unpack_scmi_header() - unpacks and records message and protocol id
+ *
+ * @msg_hdr: 32-bit packed message header sent from the platform
+ * @hdr: pointer to header to fetch message and protocol id.
+ */
+static inline void unpack_scmi_header(u32 msg_hdr, struct scmi_msg_hdr *hdr)
+{
+	hdr->id = MSG_XTRACT_ID(msg_hdr);
+	hdr->protocol_id = MSG_XTRACT_PROT_ID(msg_hdr);
+}
+
 /**
  * scmi_tx_prepare() - mailbox client callback to prepare for the transfer
  *
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 07/11] firmware: arm_scmi: Add support for asynchronous commands and delayed response

[Sudeep Holla]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/11] firmware: arm_scmi: Add support for asynchronous commands and delayed response

[Jim Quinlan]

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
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/11] firmware: arm_scmi: Add support for asynchronous commands and delayed response

[Sudeep Holla]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/11] firmware: arm_scmi: Add support for asynchronous commands and delayed response

[Jim Quinlan]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/11] firmware: arm_scmi: Add support for asynchronous commands and delayed response

[Sudeep Holla]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 08/11] firmware: arm_scmi: Drop async flag in sensor_ops->reading_get

[Sudeep Holla]

SENSOR_DESCRIPTION_GET provides attributes to indicate if the sensor
supports asynchronous read. Ideally we should be able to read that flag
and use asynchronous reads for any sensors with that attribute set.

In order to add that support, let's drop the async flag passed to
sensor_ops->reading_get and dynamically switch between sync and async
flags based on the attributes as provided by the firmware.

Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/sensors.c | 4 ++--
 drivers/hwmon/scmi-hwmon.c          | 2 +-
 include/linux/scmi_protocol.h       | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 17dbabd8a94a..1b5757c77a35 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -211,7 +211,7 @@ scmi_sensor_trip_point_config(const struct scmi_handle *handle, u32 sensor_id,
 }
 
 static int scmi_sensor_reading_get(const struct scmi_handle *handle,
-				   u32 sensor_id, bool async, u64 *value)
+				   u32 sensor_id, u64 *value)
 {
 	int ret;
 	struct scmi_xfer *t;
@@ -225,7 +225,7 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle,
 
 	sensor = t->tx.buf;
 	sensor->id = cpu_to_le32(sensor_id);
-	sensor->flags = cpu_to_le32(async ? SENSOR_READ_ASYNC : 0);
+	sensor->flags = cpu_to_le32(0);
 
 	ret = scmi_do_xfer(handle, t);
 	if (!ret) {
diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
index 0c93fc5ca762..8a7732c0bef3 100644
--- a/drivers/hwmon/scmi-hwmon.c
+++ b/drivers/hwmon/scmi-hwmon.c
@@ -72,7 +72,7 @@ static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 	const struct scmi_handle *h = scmi_sensors->handle;
 
 	sensor = *(scmi_sensors->info[type] + channel);
-	ret = h->sensor_ops->reading_get(h, sensor->id, false, &value);
+	ret = h->sensor_ops->reading_get(h, sensor->id, &value);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index ea6b72018752..697e30fb9004 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -182,7 +182,7 @@ struct scmi_sensor_ops {
 	int (*trip_point_config)(const struct scmi_handle *handle,
 				 u32 sensor_id, u8 trip_id, u64 trip_value);
 	int (*reading_get)(const struct scmi_handle *handle, u32 sensor_id,
-			   bool async, u64 *value);
+			   u64 *value);
 };
 
 /**
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/11] firmware: arm_scmi: Drop async flag in sensor_ops->reading_get

[Guenter Roeck]

On Mon, Jul 08, 2019 at 04:47:27PM +0100, Sudeep Holla wrote:
> SENSOR_DESCRIPTION_GET provides attributes to indicate if the sensor
> supports asynchronous read. Ideally we should be able to read that flag
> and use asynchronous reads for any sensors with that attribute set.
> 
> In order to add that support, let's drop the async flag passed to
> sensor_ops->reading_get and dynamically switch between sync and async
> flags based on the attributes as provided by the firmware.
> 
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

For hwmon:

Acked-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
>  drivers/firmware/arm_scmi/sensors.c | 4 ++--
>  drivers/hwmon/scmi-hwmon.c          | 2 +-
>  include/linux/scmi_protocol.h       | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index 17dbabd8a94a..1b5757c77a35 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -211,7 +211,7 @@ scmi_sensor_trip_point_config(const struct scmi_handle *handle, u32 sensor_id,
>  }
>  
>  static int scmi_sensor_reading_get(const struct scmi_handle *handle,
> -				   u32 sensor_id, bool async, u64 *value)
> +				   u32 sensor_id, u64 *value)
>  {
>  	int ret;
>  	struct scmi_xfer *t;
> @@ -225,7 +225,7 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle,
>  
>  	sensor = t->tx.buf;
>  	sensor->id = cpu_to_le32(sensor_id);
> -	sensor->flags = cpu_to_le32(async ? SENSOR_READ_ASYNC : 0);
> +	sensor->flags = cpu_to_le32(0);
>  
>  	ret = scmi_do_xfer(handle, t);
>  	if (!ret) {
> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> index 0c93fc5ca762..8a7732c0bef3 100644
> --- a/drivers/hwmon/scmi-hwmon.c
> +++ b/drivers/hwmon/scmi-hwmon.c
> @@ -72,7 +72,7 @@ static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  	const struct scmi_handle *h = scmi_sensors->handle;
>  
>  	sensor = *(scmi_sensors->info[type] + channel);
> -	ret = h->sensor_ops->reading_get(h, sensor->id, false, &value);
> +	ret = h->sensor_ops->reading_get(h, sensor->id, &value);
>  	if (ret)
>  		return ret;
>  
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index ea6b72018752..697e30fb9004 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -182,7 +182,7 @@ struct scmi_sensor_ops {
>  	int (*trip_point_config)(const struct scmi_handle *handle,
>  				 u32 sensor_id, u8 trip_id, u64 trip_value);
>  	int (*reading_get)(const struct scmi_handle *handle, u32 sensor_id,
> -			   bool async, u64 *value);
> +			   u64 *value);
>  };
>  
>  /**
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 09/11] firmware: arm_scmi: Add asynchronous sensor read if it supports

[Sudeep Holla]

SENSOR_DESCRIPTION_GET provides attributes to indicate if the sensor
supports asynchronous read. We can read that flag and use asynchronous
reads for any sensors with that attribute set.

Let's use the new scmi_do_xfer_with_response to support asynchronous
sensor reads.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/sensors.c | 30 +++++++++++++++++++++--------
 include/linux/scmi_protocol.h       |  2 ++
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 1b5757c77a35..b8f226a365de 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -136,9 +136,10 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
 		}
 
 		for (cnt = 0; cnt < num_returned; cnt++) {
-			u32 attrh;
+			u32 attrh, attrl;
 			struct scmi_sensor_info *s;
 
+			attrl = le32_to_cpu(buf->desc[cnt].attributes_low);
 			attrh = le32_to_cpu(buf->desc[cnt].attributes_high);
 			s = &si->sensors[desc_index + cnt];
 			s->id = le32_to_cpu(buf->desc[cnt].id);
@@ -147,6 +148,8 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
 			/* Sign extend to a full s8 */
 			if (s->scale & SENSOR_SCALE_SIGN)
 				s->scale |= SENSOR_SCALE_EXTEND;
+			s->async = SUPPORTS_ASYNC_READ(attrl);
+			s->num_trip_points = NUM_TRIP_POINTS(attrl);
 			strlcpy(s->name, buf->desc[cnt].name, SCMI_MAX_STR_SIZE);
 		}
 
@@ -214,8 +217,11 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle,
 				   u32 sensor_id, u64 *value)
 {
 	int ret;
+	__le32 *pval;
 	struct scmi_xfer *t;
 	struct scmi_msg_sensor_reading_get *sensor;
+	struct sensors_info *si = handle->sensor_priv;
+	struct scmi_sensor_info *s = si->sensors + sensor_id;
 
 	ret = scmi_xfer_get_init(handle, SENSOR_READING_GET,
 				 SCMI_PROTOCOL_SENSOR, sizeof(*sensor),
@@ -223,16 +229,24 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle,
 	if (ret)
 		return ret;
 
+	pval = t->rx.buf;
 	sensor = t->tx.buf;
 	sensor->id = cpu_to_le32(sensor_id);
-	sensor->flags = cpu_to_le32(0);
-
-	ret = scmi_do_xfer(handle, t);
-	if (!ret) {
-		__le32 *pval = t->rx.buf;
 
-		*value = le32_to_cpu(*pval);
-		*value |= (u64)le32_to_cpu(*(pval + 1)) << 32;
+	if (s->async) {
+		sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC);
+		ret = scmi_do_xfer_with_response(handle, t);
+		if (!ret) {
+			*value = le32_to_cpu(*pval + 1);
+			*value |= (u64)le32_to_cpu(*(pval + 2)) << 32;
+		}
+	} else {
+		sensor->flags = cpu_to_le32(0);
+		ret = scmi_do_xfer(handle, t);
+		if (!ret) {
+			*value = le32_to_cpu(*pval);
+			*value |= (u64)le32_to_cpu(*(pval + 1)) << 32;
+		}
 	}
 
 	scmi_xfer_put(handle, t);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 697e30fb9004..1be16d7730e2 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -145,6 +145,8 @@ struct scmi_sensor_info {
 	u32 id;
 	u8 type;
 	s8 scale;
+	u8 num_trip_points;
+	bool async;
 	char name[SCMI_MAX_STR_SIZE];
 };
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 10/11] firmware: arm_scmi: Drop config flag in clk_ops->rate_set

[Sudeep Holla]

CLOCK_PROTOCOL_ATTRIBUTES provides attributes to indicate the maximum
number of pending asynchronous clock rate changes supported by the
platform. If it's non-zero, then we should be able to use asynchronous
clock rate set for any clocks until the maximum limit is reached.

In order to add that support, let's drop the config flag passed to
clk_ops->rate_set and handle the asynchronous requests dynamically.

Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/clk/clk-scmi.c            | 2 +-
 drivers/firmware/arm_scmi/clock.c | 4 ++--
 include/linux/scmi_protocol.h     | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index a2287c770d5c..886f7c5df51a 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -69,7 +69,7 @@ static int scmi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct scmi_clk *clk = to_scmi_clk(hw);
 
-	return clk->handle->clk_ops->rate_set(clk->handle, clk->id, 0, rate);
+	return clk->handle->clk_ops->rate_set(clk->handle, clk->id, rate);
 }
 
 static int scmi_clk_enable(struct clk_hw *hw)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 0a194af92438..dd215bd11a58 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -218,7 +218,7 @@ scmi_clock_rate_get(const struct scmi_handle *handle, u32 clk_id, u64 *value)
 }
 
 static int scmi_clock_rate_set(const struct scmi_handle *handle, u32 clk_id,
-			       u32 config, u64 rate)
+			       u64 rate)
 {
 	int ret;
 	struct scmi_xfer *t;
@@ -230,7 +230,7 @@ static int scmi_clock_rate_set(const struct scmi_handle *handle, u32 clk_id,
 		return ret;
 
 	cfg = t->tx.buf;
-	cfg->flags = cpu_to_le32(config);
+	cfg->flags = cpu_to_le32(0);
 	cfg->id = cpu_to_le32(clk_id);
 	cfg->value_low = cpu_to_le32(rate & 0xffffffff);
 	cfg->value_high = cpu_to_le32(rate >> 32);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 1be16d7730e2..1694ee1b410e 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -71,7 +71,7 @@ struct scmi_clk_ops {
 	int (*rate_get)(const struct scmi_handle *handle, u32 clk_id,
 			u64 *rate);
 	int (*rate_set)(const struct scmi_handle *handle, u32 clk_id,
-			u32 config, u64 rate);
+			u64 rate);
 	int (*enable)(const struct scmi_handle *handle, u32 clk_id);
 	int (*disable)(const struct scmi_handle *handle, u32 clk_id);
 };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 10/11] firmware: arm_scmi: Drop config flag in clk_ops->rate_set

[Stephen Boyd]

Quoting Sudeep Holla (2019-07-08 08:47:29)
> CLOCK_PROTOCOL_ATTRIBUTES provides attributes to indicate the maximum
> number of pending asynchronous clock rate changes supported by the
> platform. If it's non-zero, then we should be able to use asynchronous
> clock rate set for any clocks until the maximum limit is reached.
> 
> In order to add that support, let's drop the config flag passed to
> clk_ops->rate_set and handle the asynchronous requests dynamically.
> 
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 11/11] firmware: arm_scmi: Use asynchronous CLOCK_RATE_SET when possible

[Sudeep Holla]

CLOCK_PROTOCOL_ATTRIBUTES provides attributes to indicate the maximum
number of pending asynchronous clock rate changes supported by the
platform. If it's non-zero, then we should be able to use asynchronous
clock rate set for any clocks until the maximum limit is reached.

Keeping the current count of pending asynchronous clock set rate
requests, we can decide if we can you asynchronous request for the
incoming/new request.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index dd215bd11a58..70044b7c812e 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -56,7 +56,7 @@ struct scmi_msg_resp_clock_describe_rates {
 struct scmi_clock_set_rate {
 	__le32 flags;
 #define CLOCK_SET_ASYNC		BIT(0)
-#define CLOCK_SET_DELAYED	BIT(1)
+#define CLOCK_SET_IGNORE_RESP	BIT(1)
 #define CLOCK_SET_ROUND_UP	BIT(2)
 #define CLOCK_SET_ROUND_AUTO	BIT(3)
 	__le32 id;
@@ -67,6 +67,7 @@ struct scmi_clock_set_rate {
 struct clock_info {
 	int num_clocks;
 	int max_async_req;
+	atomic_t cur_async_req;
 	struct scmi_clock_info *clk;
 };
 
@@ -221,21 +222,35 @@ static int scmi_clock_rate_set(const struct scmi_handle *handle, u32 clk_id,
 			       u64 rate)
 {
 	int ret;
+	u32 flags = 0;
 	struct scmi_xfer *t;
 	struct scmi_clock_set_rate *cfg;
+	struct clock_info *ci = handle->clk_priv;
 
 	ret = scmi_xfer_get_init(handle, CLOCK_RATE_SET, SCMI_PROTOCOL_CLOCK,
 				 sizeof(*cfg), 0, &t);
 	if (ret)
 		return ret;
 
+	if (ci->max_async_req) {
+		if (atomic_inc_return(&ci->cur_async_req) < ci->max_async_req)
+			flags |= CLOCK_SET_ASYNC;
+		else
+			atomic_dec(&ci->cur_async_req);
+	}
+
 	cfg = t->tx.buf;
-	cfg->flags = cpu_to_le32(0);
+	cfg->flags = cpu_to_le32(flags);
 	cfg->id = cpu_to_le32(clk_id);
 	cfg->value_low = cpu_to_le32(rate & 0xffffffff);
 	cfg->value_high = cpu_to_le32(rate >> 32);
 
-	ret = scmi_do_xfer(handle, t);
+	if (flags & CLOCK_SET_ASYNC) {
+		ret = scmi_do_xfer_with_response(handle, t);
+		atomic_dec(&ci->cur_async_req);
+	} else {
+		ret = scmi_do_xfer(handle, t);
+	}
 
 	scmi_xfer_put(handle, t);
 	return ret;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 11/11] firmware: arm_scmi: Use asynchronous CLOCK_RATE_SET when possible

[Stephen Boyd]

Quoting Sudeep Holla (2019-07-08 08:47:30)
> CLOCK_PROTOCOL_ATTRIBUTES provides attributes to indicate the maximum
> number of pending asynchronous clock rate changes supported by the
> platform. If it's non-zero, then we should be able to use asynchronous
> clock rate set for any clocks until the maximum limit is reached.
> 
> Keeping the current count of pending asynchronous clock set rate
> requests, we can decide if we can you asynchronous request for the

This last part of the sentence doesn't read properly. Please rewrite.

> incoming/new request.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/clock.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index dd215bd11a58..70044b7c812e 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -221,21 +222,35 @@ static int scmi_clock_rate_set(const struct scmi_handle *handle, u32 clk_id,
>                                u64 rate)
>  {
>         int ret;
> +       u32 flags = 0;
>         struct scmi_xfer *t;
>         struct scmi_clock_set_rate *cfg;
> +       struct clock_info *ci = handle->clk_priv;
>  
>         ret = scmi_xfer_get_init(handle, CLOCK_RATE_SET, SCMI_PROTOCOL_CLOCK,
>                                  sizeof(*cfg), 0, &t);
>         if (ret)
>                 return ret;
>  
> +       if (ci->max_async_req) {
> +               if (atomic_inc_return(&ci->cur_async_req) < ci->max_async_req)
> +                       flags |= CLOCK_SET_ASYNC;
> +               else
> +                       atomic_dec(&ci->cur_async_req);

Can this be combined with the atomic_dec() below and done after either
transfer?

> +       }
> +
>         cfg = t->tx.buf;
> -       cfg->flags = cpu_to_le32(0);
> +       cfg->flags = cpu_to_le32(flags);
>         cfg->id = cpu_to_le32(clk_id);
>         cfg->value_low = cpu_to_le32(rate & 0xffffffff);
>         cfg->value_high = cpu_to_le32(rate >> 32);
>  
> -       ret = scmi_do_xfer(handle, t);
> +       if (flags & CLOCK_SET_ASYNC) {
> +               ret = scmi_do_xfer_with_response(handle, t);
> +               atomic_dec(&ci->cur_async_req);
> +       } else {
> +               ret = scmi_do_xfer(handle, t);
> +       }

I mean putting the atomic_dec() here.

>  
>         scmi_xfer_put(handle, t);
>         return ret;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 11/11] firmware: arm_scmi: Use asynchronous CLOCK_RATE_SET when possible

[Sudeep Holla]

On Mon, Jul 22, 2019 at 02:29:53PM -0700, Stephen Boyd wrote:
> Quoting Sudeep Holla (2019-07-08 08:47:30)
> > CLOCK_PROTOCOL_ATTRIBUTES provides attributes to indicate the maximum
> > number of pending asynchronous clock rate changes supported by the
> > platform. If it's non-zero, then we should be able to use asynchronous
> > clock rate set for any clocks until the maximum limit is reached.
> >
> > Keeping the current count of pending asynchronous clock set rate
> > requests, we can decide if we can you asynchronous request for the
>
> This last part of the sentence doesn't read properly. Please rewrite.
>

Will fix.

> > incoming/new request.
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/clock.c | 21 ++++++++++++++++++---
> >  1 file changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> > index dd215bd11a58..70044b7c812e 100644
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -221,21 +222,35 @@ static int scmi_clock_rate_set(const struct scmi_handle *handle, u32 clk_id,
> >                                u64 rate)
> >  {
> >         int ret;
> > +       u32 flags = 0;
> >         struct scmi_xfer *t;
> >         struct scmi_clock_set_rate *cfg;
> > +       struct clock_info *ci = handle->clk_priv;
> >
> >         ret = scmi_xfer_get_init(handle, CLOCK_RATE_SET, SCMI_PROTOCOL_CLOCK,
> >                                  sizeof(*cfg), 0, &t);
> >         if (ret)
> >                 return ret;
> >
> > +       if (ci->max_async_req) {
> > +               if (atomic_inc_return(&ci->cur_async_req) < ci->max_async_req)
> > +                       flags |= CLOCK_SET_ASYNC;
> > +               else
> > +                       atomic_dec(&ci->cur_async_req);
>
> Can this be combined with the atomic_dec() below and done after either
> transfer?
>

Yes but cleaner.

> > +       }
> > +
> >         cfg = t->tx.buf;
> > -       cfg->flags = cpu_to_le32(0);
> > +       cfg->flags = cpu_to_le32(flags);
> >         cfg->id = cpu_to_le32(clk_id);
> >         cfg->value_low = cpu_to_le32(rate & 0xffffffff);
> >         cfg->value_high = cpu_to_le32(rate >> 32);
> >
> > -       ret = scmi_do_xfer(handle, t);
> > +       if (flags & CLOCK_SET_ASYNC) {
> > +               ret = scmi_do_xfer_with_response(handle, t);
> > +               atomic_dec(&ci->cur_async_req);
> > +       } else {
> > +               ret = scmi_do_xfer(handle, t);
> > +       }
>
> I mean putting the atomic_dec() here.
>

Understood and done locally, will post as v2.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel