[PATCH v4 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[PATCH v4 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

v4 -- s/message-serviced/a2p/ in the bindings commit message.
   -- Changed author/s-o-b/committer email address as my company is now
      appending boilerplate text to all outgoing emails.

v3 -- Changed interrupt name from "message-serviced" to "a2p" (RobH)

v2 -- Correct commit message, s/msg/message/, and remove extra WS on
      "dt-bindings" commit (Sudeep)
   -- Change interrupt name to "message-serviced", move irq assignent to end
      of function. (Sudeep)

v1 -- original.

Jim Quinlan (2):
  dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport
  firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

 .../devicetree/bindings/arm/arm,scmi.txt      |  8 ++++
 drivers/firmware/arm_scmi/smc.c               | 38 ++++++++++++++++++-
 2 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v4 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

v4 -- s/message-serviced/a2p/ in the bindings commit message.
   -- Changed author/s-o-b/committer email address as my company is now
      appending boilerplate text to all outgoing emails.

v3 -- Changed interrupt name from "message-serviced" to "a2p" (RobH)

v2 -- Correct commit message, s/msg/message/, and remove extra WS on
      "dt-bindings" commit (Sudeep)
   -- Change interrupt name to "message-serviced", move irq assignent to end
      of function. (Sudeep)

v1 -- original.

Jim Quinlan (2):
  dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport
  firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

 .../devicetree/bindings/arm/arm,scmi.txt      |  8 ++++
 drivers/firmware/arm_scmi/smc.c               | 38 ++++++++++++++++++-
 2 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.17.1


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

[PATCH v4 1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport

[Jim Quinlan]

In normal use of smc/hvc transport in SCMI the message completion is
indicated by the return of the SMC call.  This commit provides for an
optional interrupt named "a2p" which is used instead to
indicate the completion of a message.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 Documentation/devicetree/bindings/arm/arm,scmi.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt
index b5ce5b39bb9c..667d58e0a659 100644
--- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
@@ -31,6 +31,14 @@ Optional properties:
 
 - mbox-names: shall be "tx" or "rx" depending on mboxes entries.
 
+- interrupts : when using smc or hvc transports, this optional
+	 property indicates that msg completion by the platform is indicated
+	 by an interrupt rather than by the return of the smc call. This
+	 should not be used except when the platform requires such behavior.
+
+- interrupt-names : if "interrupts" is present, interrupt-names must also
+	 be present and have the value "a2p".
+
 See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details
 about the generic mailbox controller and client driver bindings.
 
-- 
2.17.1

[PATCH v4 1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport

[Jim Quinlan]

In normal use of smc/hvc transport in SCMI the message completion is
indicated by the return of the SMC call.  This commit provides for an
optional interrupt named "a2p" which is used instead to
indicate the completion of a message.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 Documentation/devicetree/bindings/arm/arm,scmi.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt
index b5ce5b39bb9c..667d58e0a659 100644
--- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
@@ -31,6 +31,14 @@ Optional properties:
 
 - mbox-names: shall be "tx" or "rx" depending on mboxes entries.
 
+- interrupts : when using smc or hvc transports, this optional
+	 property indicates that msg completion by the platform is indicated
+	 by an interrupt rather than by the return of the smc call. This
+	 should not be used except when the platform requires such behavior.
+
+- interrupt-names : if "interrupts" is present, interrupt-names must also
+	 be present and have the value "a2p".
+
 See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details
 about the generic mailbox controller and client driver bindings.
 
-- 
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 v4 1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport

[Florian Fainelli]

On 12/22/2020 6:56 AM, Jim Quinlan wrote:
> In normal use of smc/hvc transport in SCMI the message completion is
> indicated by the return of the SMC call.  This commit provides for an
> optional interrupt named "a2p" which is used instead to
> indicate the completion of a message.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v4 1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport

[Florian Fainelli]

On 12/22/2020 6:56 AM, Jim Quinlan wrote:
> In normal use of smc/hvc transport in SCMI the message completion is
> indicated by the return of the SMC call.  This commit provides for an
> optional interrupt named "a2p" which is used instead to
> indicate the completion of a message.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

Re: [PATCH v4 1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport

[Rob Herring]

On Tue, 22 Dec 2020 09:56:02 -0500, Jim Quinlan wrote:
> In normal use of smc/hvc transport in SCMI the message completion is
> indicated by the return of the SMC call.  This commit provides for an
> optional interrupt named "a2p" which is used instead to
> indicate the completion of a message.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  Documentation/devicetree/bindings/arm/arm,scmi.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport

[Rob Herring]

On Tue, 22 Dec 2020 09:56:02 -0500, Jim Quinlan wrote:
> In normal use of smc/hvc transport in SCMI the message completion is
> indicated by the return of the SMC call.  This commit provides for an
> optional interrupt named "a2p" which is used instead to
> indicate the completion of a message.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  Documentation/devicetree/bindings/arm/arm,scmi.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
message to be indicated by an interrupt rather than the return of the smc
call.  This accommodates the existing behavior of the BrcmSTB SCMI
"platform" whose SW is already out in the field and cannot be changed.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/firmware/arm_scmi/smc.c | 38 ++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 82a82a5dc86a..fd41d436e34b 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -9,9 +9,11 @@
 #include <linux/arm-smccc.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/interrupt.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/slab.h>
 
 #include "common.h"
@@ -23,6 +25,8 @@
  * @shmem: Transmit/Receive shared memory area
  * @shmem_lock: Lock to protect access to Tx/Rx shared memory area
  * @func_id: smc/hvc call function id
+ * @irq: Optional; employed when platforms indicates msg completion by intr.
+ * @tx_complete: Optional, employed only when irq is valid.
  */
 
 struct scmi_smc {
@@ -30,8 +34,19 @@ struct scmi_smc {
 	struct scmi_shared_mem __iomem *shmem;
 	struct mutex shmem_lock;
 	u32 func_id;
+	int irq;
+	struct completion tx_complete;
 };
 
+static irqreturn_t smc_msg_done_isr(int irq, void *data)
+{
+	struct scmi_smc *scmi_info = data;
+
+	complete(&scmi_info->tx_complete);
+
+	return IRQ_HANDLED;
+}
+
 static bool smc_chan_available(struct device *dev, int idx)
 {
 	struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
@@ -51,7 +66,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	struct resource res;
 	struct device_node *np;
 	u32 func_id;
-	int ret;
+	int ret, irq = 0;
 
 	if (!tx)
 		return -ENODEV;
@@ -79,10 +94,29 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * If there is an interrupt named "a2p", then the
+	 * service and completion of a message is signaled by an interrupt
+	 * rather than by the return of the SMC call.
+	 */
+	ret = of_irq_get_byname(cdev->of_node, "a2p");
+	if (ret > 0) {
+		irq = ret;
+		ret = devm_request_irq(dev, irq, smc_msg_done_isr,
+				       IRQF_NO_SUSPEND,
+				       dev_name(dev),
+				       scmi_info);
+		if (ret) {
+			dev_err(dev, "failed to setup SCMI smc irq\n");
+			return ret;
+		}
+		init_completion(&scmi_info->tx_complete);
+	}
 	scmi_info->func_id = func_id;
 	scmi_info->cinfo = cinfo;
 	mutex_init(&scmi_info->shmem_lock);
 	cinfo->transport_info = scmi_info;
+	scmi_info->irq = irq;
 
 	return 0;
 }
@@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 	shmem_tx_prepare(scmi_info->shmem, xfer);
 
 	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+	if (scmi_info->irq)
+		wait_for_completion(&scmi_info->tx_complete);
 	scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
 
 	mutex_unlock(&scmi_info->shmem_lock);
-- 
2.17.1

[PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
message to be indicated by an interrupt rather than the return of the smc
call.  This accommodates the existing behavior of the BrcmSTB SCMI
"platform" whose SW is already out in the field and cannot be changed.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/firmware/arm_scmi/smc.c | 38 ++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 82a82a5dc86a..fd41d436e34b 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -9,9 +9,11 @@
 #include <linux/arm-smccc.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/interrupt.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/slab.h>
 
 #include "common.h"
@@ -23,6 +25,8 @@
  * @shmem: Transmit/Receive shared memory area
  * @shmem_lock: Lock to protect access to Tx/Rx shared memory area
  * @func_id: smc/hvc call function id
+ * @irq: Optional; employed when platforms indicates msg completion by intr.
+ * @tx_complete: Optional, employed only when irq is valid.
  */
 
 struct scmi_smc {
@@ -30,8 +34,19 @@ struct scmi_smc {
 	struct scmi_shared_mem __iomem *shmem;
 	struct mutex shmem_lock;
 	u32 func_id;
+	int irq;
+	struct completion tx_complete;
 };
 
+static irqreturn_t smc_msg_done_isr(int irq, void *data)
+{
+	struct scmi_smc *scmi_info = data;
+
+	complete(&scmi_info->tx_complete);
+
+	return IRQ_HANDLED;
+}
+
 static bool smc_chan_available(struct device *dev, int idx)
 {
 	struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
@@ -51,7 +66,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	struct resource res;
 	struct device_node *np;
 	u32 func_id;
-	int ret;
+	int ret, irq = 0;
 
 	if (!tx)
 		return -ENODEV;
@@ -79,10 +94,29 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * If there is an interrupt named "a2p", then the
+	 * service and completion of a message is signaled by an interrupt
+	 * rather than by the return of the SMC call.
+	 */
+	ret = of_irq_get_byname(cdev->of_node, "a2p");
+	if (ret > 0) {
+		irq = ret;
+		ret = devm_request_irq(dev, irq, smc_msg_done_isr,
+				       IRQF_NO_SUSPEND,
+				       dev_name(dev),
+				       scmi_info);
+		if (ret) {
+			dev_err(dev, "failed to setup SCMI smc irq\n");
+			return ret;
+		}
+		init_completion(&scmi_info->tx_complete);
+	}
 	scmi_info->func_id = func_id;
 	scmi_info->cinfo = cinfo;
 	mutex_init(&scmi_info->shmem_lock);
 	cinfo->transport_info = scmi_info;
+	scmi_info->irq = irq;
 
 	return 0;
 }
@@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 	shmem_tx_prepare(scmi_info->shmem, xfer);
 
 	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+	if (scmi_info->irq)
+		wait_for_completion(&scmi_info->tx_complete);
 	scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
 
 	mutex_unlock(&scmi_info->shmem_lock);
-- 
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 v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Florian Fainelli]

On 12/22/2020 6:56 AM, Jim Quinlan wrote:
> The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
> message to be indicated by an interrupt rather than the return of the smc
> call.  This accommodates the existing behavior of the BrcmSTB SCMI
> "platform" whose SW is already out in the field and cannot be changed.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>

This looks good to me, just one question below:

[snip]

> @@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>  	shmem_tx_prepare(scmi_info->shmem, xfer);
>  
>  	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> +	if (scmi_info->irq)
> +		wait_for_completion(&scmi_info->tx_complete);

Do we need this to have a preceding call to reinit_completion()? It does
not look like this is going to make any practical difference but there
are drivers doing that for correctness.
-- 
Florian

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Florian Fainelli]

On 12/22/2020 6:56 AM, Jim Quinlan wrote:
> The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
> message to be indicated by an interrupt rather than the return of the smc
> call.  This accommodates the existing behavior of the BrcmSTB SCMI
> "platform" whose SW is already out in the field and cannot be changed.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>

This looks good to me, just one question below:

[snip]

> @@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>  	shmem_tx_prepare(scmi_info->shmem, xfer);
>  
>  	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> +	if (scmi_info->irq)
> +		wait_for_completion(&scmi_info->tx_complete);

Do we need this to have a preceding call to reinit_completion()? It does
not look like this is going to make any practical difference but there
are drivers doing that for correctness.
-- 
Florian

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

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Sudeep Holla]

On Tue, Dec 22, 2020 at 07:37:22PM -0800, Florian Fainelli wrote:
> 
> 
> On 12/22/2020 6:56 AM, Jim Quinlan wrote:
> > The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
> > message to be indicated by an interrupt rather than the return of the smc
> > call.  This accommodates the existing behavior of the BrcmSTB SCMI
> > "platform" whose SW is already out in the field and cannot be changed.
> > 
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> 
> This looks good to me, just one question below:
> 
> [snip]
> 
> > @@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> >  	shmem_tx_prepare(scmi_info->shmem, xfer);
> >  
> >  	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> > +	if (scmi_info->irq)
> > +		wait_for_completion(&scmi_info->tx_complete);
> 
> Do we need this to have a preceding call to reinit_completion()? It does
> not look like this is going to make any practical difference but there
> are drivers doing that for correctness.

Why do you think that might not cause any issue ? After first message
is completed and ISR is executed, the completion flag remains done for
ever. So practically 2nd message onwards won't block in wait_for_completion
which means return from smc/hvc is actually completion too which is clearly
wrong or am I missing something ?

Jim, please confirm either way. If you agree I can add the below snippet,
no need to repost.

Regards,
Sudeep

--
diff --git i/drivers/firmware/arm_scmi/smc.c w/drivers/firmware/arm_scmi/smc.c
index fd41d436e34b..86eac0831d3c 100644
--- i/drivers/firmware/arm_scmi/smc.c
+++ w/drivers/firmware/arm_scmi/smc.c
@@ -144,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,

        shmem_tx_prepare(scmi_info->shmem, xfer);

+       if (scmi_info->irq)
+               reinit_completion(&scmi_info->tx_complete);
        arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
        if (scmi_info->irq)
                wait_for_completion(&scmi_info->tx_complete);

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Sudeep Holla]

On Tue, Dec 22, 2020 at 07:37:22PM -0800, Florian Fainelli wrote:
> 
> 
> On 12/22/2020 6:56 AM, Jim Quinlan wrote:
> > The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
> > message to be indicated by an interrupt rather than the return of the smc
> > call.  This accommodates the existing behavior of the BrcmSTB SCMI
> > "platform" whose SW is already out in the field and cannot be changed.
> > 
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> 
> This looks good to me, just one question below:
> 
> [snip]
> 
> > @@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> >  	shmem_tx_prepare(scmi_info->shmem, xfer);
> >  
> >  	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> > +	if (scmi_info->irq)
> > +		wait_for_completion(&scmi_info->tx_complete);
> 
> Do we need this to have a preceding call to reinit_completion()? It does
> not look like this is going to make any practical difference but there
> are drivers doing that for correctness.

Why do you think that might not cause any issue ? After first message
is completed and ISR is executed, the completion flag remains done for
ever. So practically 2nd message onwards won't block in wait_for_completion
which means return from smc/hvc is actually completion too which is clearly
wrong or am I missing something ?

Jim, please confirm either way. If you agree I can add the below snippet,
no need to repost.

Regards,
Sudeep

--
diff --git i/drivers/firmware/arm_scmi/smc.c w/drivers/firmware/arm_scmi/smc.c
index fd41d436e34b..86eac0831d3c 100644
--- i/drivers/firmware/arm_scmi/smc.c
+++ w/drivers/firmware/arm_scmi/smc.c
@@ -144,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,

        shmem_tx_prepare(scmi_info->shmem, xfer);

+       if (scmi_info->irq)
+               reinit_completion(&scmi_info->tx_complete);
        arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
        if (scmi_info->irq)
                wait_for_completion(&scmi_info->tx_complete);


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

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

> From: Sudeep Holla <sudeep.holla@arm.com>
> Date: Tue, Jan 5, 2021 at 12:35 PM
> Subject: Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
> To: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Jim Quinlan <jim2101024@gmail.com>, Sudeep Holla <sudeep.holla@arm.com>, <bcm-kernel-feedback-list@broadcom.com>, <james.quinlan@broadcom.com>, open list:SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE Mes... <linux-arm-kernel@lists.infradead.org>, open list <linux-kernel@vger.kernel.org>
>
>
> On Tue, Dec 22, 2020 at 07:37:22PM -0800, Florian Fainelli wrote:
> >
> >
> > On 12/22/2020 6:56 AM, Jim Quinlan wrote:
> > > The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
> > > message to be indicated by an interrupt rather than the return of the smc
> > > call.  This accommodates the existing behavior of the BrcmSTB SCMI
> > > "platform" whose SW is already out in the field and cannot be changed.
> > >
> > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> >
> > This looks good to me, just one question below:
> >
> > [snip]
> >
> > > @@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> > >     shmem_tx_prepare(scmi_info->shmem, xfer);
> > >
> > >     arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> > > +   if (scmi_info->irq)
> > > +           wait_for_completion(&scmi_info->tx_complete);
> >
> > Do we need this to have a preceding call to reinit_completion()? It does
> > not look like this is going to make any practical difference but there
> > are drivers doing that for correctness.
>
> Why do you think that might not cause any issue ? After first message
> is completed and ISR is executed, the completion flag remains done for
> ever.
Hi Sudeep,

I don't think that is the case;  the bottom routine,
do_wait_for_common(), decrements the x->done after a completion (which
does an increment).  Regardless, I think it is prudent to add the
reinit patch you've provided below.

BTW, one thing I could have done was incorporate the timeout value but
I thought that since this is the "fast" call we shouldn't be timing
out at all.

Thanks much,
Jim Quinlan
Broadcom STB



So practically 2nd message onwards won't block in wait_for_completion
> which means return from smc/hvc is actually completion too which is clearly
> wrong or am I missing something ?
>
> Jim, please confirm either way. If you agree I can add the below snippet,
> no need to repost.
>
> Regards,
> Sudeep
>
> --
> diff --git i/drivers/firmware/arm_scmi/smc.c w/drivers/firmware/arm_scmi/smc.c
> index fd41d436e34b..86eac0831d3c 100644
> --- i/drivers/firmware/arm_scmi/smc.c
> +++ w/drivers/firmware/arm_scmi/smc.c
> @@ -144,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>
>         shmem_tx_prepare(scmi_info->shmem, xfer);
>
> +       if (scmi_info->irq)
> +               reinit_completion(&scmi_info->tx_complete);
>         arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
>         if (scmi_info->irq)
>                 wait_for_completion(&scmi_info->tx_complete);
>
>
> This electronic communication and the information and any files transmitted with it, or attached to it, are confidential and are intended solely for the use of the individual or entity to whom it is addressed and may contain information that is confidential, legally privileged, protected by privacy laws, or otherwise restricted from disclosure to anyone else. If you are not the intended recipient or the person responsible for delivering the e-mail to the intended recipient, you are hereby notified that any use, copying, distributing, dissemination, forwarding, printing, or copying of this e-mail is strictly prohibited. If you received this e-mail in error, please return the e-mail to the sender, delete it from your computer, and destroy any printed copy of it.

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

> From: Sudeep Holla <sudeep.holla@arm.com>
> Date: Tue, Jan 5, 2021 at 12:35 PM
> Subject: Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
> To: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Jim Quinlan <jim2101024@gmail.com>, Sudeep Holla <sudeep.holla@arm.com>, <bcm-kernel-feedback-list@broadcom.com>, <james.quinlan@broadcom.com>, open list:SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE Mes... <linux-arm-kernel@lists.infradead.org>, open list <linux-kernel@vger.kernel.org>
>
>
> On Tue, Dec 22, 2020 at 07:37:22PM -0800, Florian Fainelli wrote:
> >
> >
> > On 12/22/2020 6:56 AM, Jim Quinlan wrote:
> > > The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
> > > message to be indicated by an interrupt rather than the return of the smc
> > > call.  This accommodates the existing behavior of the BrcmSTB SCMI
> > > "platform" whose SW is already out in the field and cannot be changed.
> > >
> > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> >
> > This looks good to me, just one question below:
> >
> > [snip]
> >
> > > @@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> > >     shmem_tx_prepare(scmi_info->shmem, xfer);
> > >
> > >     arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> > > +   if (scmi_info->irq)
> > > +           wait_for_completion(&scmi_info->tx_complete);
> >
> > Do we need this to have a preceding call to reinit_completion()? It does
> > not look like this is going to make any practical difference but there
> > are drivers doing that for correctness.
>
> Why do you think that might not cause any issue ? After first message
> is completed and ISR is executed, the completion flag remains done for
> ever.
Hi Sudeep,

I don't think that is the case;  the bottom routine,
do_wait_for_common(), decrements the x->done after a completion (which
does an increment).  Regardless, I think it is prudent to add the
reinit patch you've provided below.

BTW, one thing I could have done was incorporate the timeout value but
I thought that since this is the "fast" call we shouldn't be timing
out at all.

Thanks much,
Jim Quinlan
Broadcom STB



So practically 2nd message onwards won't block in wait_for_completion
> which means return from smc/hvc is actually completion too which is clearly
> wrong or am I missing something ?
>
> Jim, please confirm either way. If you agree I can add the below snippet,
> no need to repost.
>
> Regards,
> Sudeep
>
> --
> diff --git i/drivers/firmware/arm_scmi/smc.c w/drivers/firmware/arm_scmi/smc.c
> index fd41d436e34b..86eac0831d3c 100644
> --- i/drivers/firmware/arm_scmi/smc.c
> +++ w/drivers/firmware/arm_scmi/smc.c
> @@ -144,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>
>         shmem_tx_prepare(scmi_info->shmem, xfer);
>
> +       if (scmi_info->irq)
> +               reinit_completion(&scmi_info->tx_complete);
>         arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
>         if (scmi_info->irq)
>                 wait_for_completion(&scmi_info->tx_complete);
>
>
> This electronic communication and the information and any files transmitted with it, or attached to it, are confidential and are intended solely for the use of the individual or entity to whom it is addressed and may contain information that is confidential, legally privileged, protected by privacy laws, or otherwise restricted from disclosure to anyone else. If you are not the intended recipient or the person responsible for delivering the e-mail to the intended recipient, you are hereby notified that any use, copying, distributing, dissemination, forwarding, printing, or copying of this e-mail is strictly prohibited. If you received this e-mail in error, please return the e-mail to the sender, delete it from your computer, and destroy any printed copy of it.

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

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Sudeep Holla]

On Tue, Jan 05, 2021 at 01:32:49PM -0500, Jim Quinlan wrote:

[...]

>
> I don't think that is the case;  the bottom routine,
> do_wait_for_common(), decrements the x->done after a completion (which
> does an increment).  Regardless, I think it is prudent to add the
> reinit patch you've provided below.
>

Ah right, but I will add that.

> BTW, one thing I could have done was incorporate the timeout value but
> I thought that since this is the "fast" call we shouldn't be timing
> out at all.
>

Agreed, we can add it later. However it is not related to fast call, it is
more for protection against failure of delivery of interrupt from the platform
or any firmware responsible IMO.

> > This electronic communication and the information and any files
> > transmitted with it, or attached to it, are confidential and are intended
> > solely for the use of the individual or entity to whom it is addressed and
> > may contain information that is confidential, legally privileged,
> > protected by privacy laws, or otherwise restricted from disclosure to
> > anyone else. If you are not the intended recipient or the person
> > responsible for delivering the e-mail to the intended recipient, you are
> > hereby notified that any use, copying, distributing, dissemination,
> > forwarding, printing, or copying of this e-mail is strictly prohibited. If
> > you received this e-mail in error, please return the e-mail to the sender,
> > delete it from your computer, and destroy any printed copy of it.

I am assuming this was unintentional and ignoring. If not disregard my
response. Otherwise you may need to fix your mail server.

--
Regards,
Sudeep

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Sudeep Holla]

On Tue, Jan 05, 2021 at 01:32:49PM -0500, Jim Quinlan wrote:

[...]

>
> I don't think that is the case;  the bottom routine,
> do_wait_for_common(), decrements the x->done after a completion (which
> does an increment).  Regardless, I think it is prudent to add the
> reinit patch you've provided below.
>

Ah right, but I will add that.

> BTW, one thing I could have done was incorporate the timeout value but
> I thought that since this is the "fast" call we shouldn't be timing
> out at all.
>

Agreed, we can add it later. However it is not related to fast call, it is
more for protection against failure of delivery of interrupt from the platform
or any firmware responsible IMO.

> > This electronic communication and the information and any files
> > transmitted with it, or attached to it, are confidential and are intended
> > solely for the use of the individual or entity to whom it is addressed and
> > may contain information that is confidential, legally privileged,
> > protected by privacy laws, or otherwise restricted from disclosure to
> > anyone else. If you are not the intended recipient or the person
> > responsible for delivering the e-mail to the intended recipient, you are
> > hereby notified that any use, copying, distributing, dissemination,
> > forwarding, printing, or copying of this e-mail is strictly prohibited. If
> > you received this e-mail in error, please return the e-mail to the sender,
> > delete it from your computer, and destroy any printed copy of it.

I am assuming this was unintentional and ignoring. If not disregard my
response. Otherwise you may need to fix your mail server.

--
Regards,
Sudeep

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

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

On Wed, Jan 6, 2021 at 4:30 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Jan 05, 2021 at 01:32:49PM -0500, Jim Quinlan wrote:
>
> [...]
>
> >
> > I don't think that is the case;  the bottom routine,
> > do_wait_for_common(), decrements the x->done after a completion (which
> > does an increment).  Regardless, I think it is prudent to add the
> > reinit patch you've provided below.
> >
>
> Ah right, but I will add that.
>
> > BTW, one thing I could have done was incorporate the timeout value but
> > I thought that since this is the "fast" call we shouldn't be timing
> > out at all.
> >
>
> Agreed, we can add it later. However it is not related to fast call, it is
> more for protection against failure of delivery of interrupt from the platform
> or any firmware responsible IMO.
>
> > > This electronic communication and the information and any files
> > > transmitted with it, or attached to it, are confidential and are intended
> > > solely for the use of the individual or entity to whom it is addressed and
> > > may contain information that is confidential, legally privileged,
> > > protected by privacy laws, or otherwise restricted from disclosure to
> > > anyone else. If you are not the intended recipient or the person
> > > responsible for delivering the e-mail to the intended recipient, you are
> > > hereby notified that any use, copying, distributing, dissemination,
> > > forwarding, printing, or copying of this e-mail is strictly prohibited. If
> > > you received this e-mail in error, please return the e-mail to the sender,
> > > delete it from your computer, and destroy any printed copy of it.
>
> I am assuming this was unintentional and ignoring. If not disregard my
> response. Otherwise you may need to fix your mail server.
Hi Sudeep,
Yes please ignore the above legalize in my email -- our company is
attaching this text to all outgoing emails.  We are working on a
general solution.
Regards,
Jim Quinlan
Broadcom STB
>
> --
> Regards,
> Sudeep

Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

On Wed, Jan 6, 2021 at 4:30 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Jan 05, 2021 at 01:32:49PM -0500, Jim Quinlan wrote:
>
> [...]
>
> >
> > I don't think that is the case;  the bottom routine,
> > do_wait_for_common(), decrements the x->done after a completion (which
> > does an increment).  Regardless, I think it is prudent to add the
> > reinit patch you've provided below.
> >
>
> Ah right, but I will add that.
>
> > BTW, one thing I could have done was incorporate the timeout value but
> > I thought that since this is the "fast" call we shouldn't be timing
> > out at all.
> >
>
> Agreed, we can add it later. However it is not related to fast call, it is
> more for protection against failure of delivery of interrupt from the platform
> or any firmware responsible IMO.
>
> > > This electronic communication and the information and any files
> > > transmitted with it, or attached to it, are confidential and are intended
> > > solely for the use of the individual or entity to whom it is addressed and
> > > may contain information that is confidential, legally privileged,
> > > protected by privacy laws, or otherwise restricted from disclosure to
> > > anyone else. If you are not the intended recipient or the person
> > > responsible for delivering the e-mail to the intended recipient, you are
> > > hereby notified that any use, copying, distributing, dissemination,
> > > forwarding, printing, or copying of this e-mail is strictly prohibited. If
> > > you received this e-mail in error, please return the e-mail to the sender,
> > > delete it from your computer, and destroy any printed copy of it.
>
> I am assuming this was unintentional and ignoring. If not disregard my
> response. Otherwise you may need to fix your mail server.
Hi Sudeep,
Yes please ignore the above legalize in my email -- our company is
attaching this text to all outgoing emails.  We are working on a
general solution.
Regards,
Jim Quinlan
Broadcom STB
>
> --
> Regards,
> Sudeep

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

Re: [PATCH v4 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

Hi Sudeep,

Since RobH has reviewed  patch 1/.2 and Florian has acked it, can you
please accept patches 1 & 2?

Thanks,
Jim Quinlan
Broadcom STB

On Tue, Dec 22, 2020 at 9:56 AM Jim Quinlan <jim2101024@gmail.com> wrote:
>
> v4 -- s/message-serviced/a2p/ in the bindings commit message.
>    -- Changed author/s-o-b/committer email address as my company is now
>       appending boilerplate text to all outgoing emails.
>
> v3 -- Changed interrupt name from "message-serviced" to "a2p" (RobH)
>
> v2 -- Correct commit message, s/msg/message/, and remove extra WS on
>       "dt-bindings" commit (Sudeep)
>    -- Change interrupt name to "message-serviced", move irq assignent to end
>       of function. (Sudeep)
>
> v1 -- original.
>
> Jim Quinlan (2):
>   dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport
>   firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
>
>  .../devicetree/bindings/arm/arm,scmi.txt      |  8 ++++
>  drivers/firmware/arm_scmi/smc.c               | 38 ++++++++++++++++++-
>  2 files changed, 45 insertions(+), 1 deletion(-)
>
> --
> 2.17.1
>

Re: [PATCH v4 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan]

Hi Sudeep,

Since RobH has reviewed  patch 1/.2 and Florian has acked it, can you
please accept patches 1 & 2?

Thanks,
Jim Quinlan
Broadcom STB

On Tue, Dec 22, 2020 at 9:56 AM Jim Quinlan <jim2101024@gmail.com> wrote:
>
> v4 -- s/message-serviced/a2p/ in the bindings commit message.
>    -- Changed author/s-o-b/committer email address as my company is now
>       appending boilerplate text to all outgoing emails.
>
> v3 -- Changed interrupt name from "message-serviced" to "a2p" (RobH)
>
> v2 -- Correct commit message, s/msg/message/, and remove extra WS on
>       "dt-bindings" commit (Sudeep)
>    -- Change interrupt name to "message-serviced", move irq assignent to end
>       of function. (Sudeep)
>
> v1 -- original.
>
> Jim Quinlan (2):
>   dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport
>   firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
>
>  .../devicetree/bindings/arm/arm,scmi.txt      |  8 ++++
>  drivers/firmware/arm_scmi/smc.c               | 38 ++++++++++++++++++-
>  2 files changed, 45 insertions(+), 1 deletion(-)
>
> --
> 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 v4 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Sudeep Holla]

On Mon, Jan 04, 2021 at 09:57:31AM -0500, Jim Quinlan wrote:
> Hi Sudeep,
> 
> Since RobH has reviewed  patch 1/.2 and Florian has acked it, can you
> please accept patches 1 & 2?
> 

Sure, will start queuing patches later this week, will let you know when
I apply. Thanks.

-- 
Regards,
Sudeep

Re: [PATCH v4 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Sudeep Holla]

On Mon, Jan 04, 2021 at 09:57:31AM -0500, Jim Quinlan wrote:
> Hi Sudeep,
> 
> Since RobH has reviewed  patch 1/.2 and Florian has acked it, can you
> please accept patches 1 & 2?
> 

Sure, will start queuing patches later this week, will let you know when
I apply. Thanks.

-- 
Regards,
Sudeep

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

Re: [PATCH v4 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Sudeep Holla]

On Tue, 22 Dec 2020 09:56:01 -0500, Jim Quinlan wrote:
> v4 -- s/message-serviced/a2p/ in the bindings commit message.
>    -- Changed author/s-o-b/committer email address as my company is now
>       appending boilerplate text to all outgoing emails.
> 
> v3 -- Changed interrupt name from "message-serviced" to "a2p" (RobH)
> 
> v2 -- Correct commit message, s/msg/message/, and remove extra WS on
>       "dt-bindings" commit (Sudeep)
>    -- Change interrupt name to "message-serviced", move irq assignent to end
>       of function. (Sudeep)
> 
> [...]


Applied to sudeep.holla/linux (for-next/scmi), thanks!

[1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport
      https://git.kernel.org/sudeep.holla/c/99a064fb3a
[2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
      https://git.kernel.org/sudeep.holla/c/dd820ee21d

--
Regards,
Sudeep

Re: [PATCH v4 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Sudeep Holla]

On Tue, 22 Dec 2020 09:56:01 -0500, Jim Quinlan wrote:
> v4 -- s/message-serviced/a2p/ in the bindings commit message.
>    -- Changed author/s-o-b/committer email address as my company is now
>       appending boilerplate text to all outgoing emails.
> 
> v3 -- Changed interrupt name from "message-serviced" to "a2p" (RobH)
> 
> v2 -- Correct commit message, s/msg/message/, and remove extra WS on
>       "dt-bindings" commit (Sudeep)
>    -- Change interrupt name to "message-serviced", move irq assignent to end
>       of function. (Sudeep)
> 
> [...]


Applied to sudeep.holla/linux (for-next/scmi), thanks!

[1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport
      https://git.kernel.org/sudeep.holla/c/99a064fb3a
[2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
      https://git.kernel.org/sudeep.holla/c/dd820ee21d

--
Regards,
Sudeep


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