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

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

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


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]

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 "message-serviced" which is used instead to
indicate the completion of a message.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.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 55deb68230eb..7cdad11f40b1 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 "message-serviced".
+
 See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details
 about the generic mailbox controller and client driver bindings.
 
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]

On Thu, Nov 12, 2020 at 12:56 PM Jim Quinlan <james.quinlan@broadcom.com> 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 "message-serviced" which is used instead to
> indicate the completion of a message.
>
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.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 55deb68230eb..7cdad11f40b1 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 "message-serviced".
> +
>  See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details
>  about the generic mailbox controller and client driver bindings.
Hi Rob,

Are you okay with this commit?

Regards,
Jim Quinlan
Broadcom STB
>
> --
> 2.17.1
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

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

[Rob Herring]

On Thu, Nov 12, 2020 at 12:56:26PM -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 "message-serviced" which is used instead to
> indicate the completion of a message.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.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 55deb68230eb..7cdad11f40b1 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 "message-serviced".

Don't really need names when only one, but wouldn't 'a2p' be more 
concise and based on SCMI spec (just guessing...).

> +
>  See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details
>  about the generic mailbox controller and client driver bindings.
>  
> -- 
> 2.17.1
> 

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 2840 bytes --]

On Mon, Dec 7, 2020 at 2:01 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Nov 12, 2020 at 12:56:26PM -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 "message-serviced" which is used instead to
> > indicate the completion of a message.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.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 55deb68230eb..7cdad11f40b1 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 "message-serviced".
>
> Don't really need names when only one, but wouldn't 'a2p' be more
> concise and based on SCMI spec (just guessing...).
I gave it a name because (a) its presence is atypical/not common and
(b) future changes may add more interrupts and I don't want this
interrupt at index 0.

As far as "message-serviced", this was Sudeep's suggestion, IIRC,
although I had a similarly long name.  Sudeep, are you okay with
"a2p".

Thanks,
Jim


>
> > +
> >  See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details
> >  about the generic mailbox controller and client driver bindings.
> >
> > --
> > 2.17.1
> >
>
>

-- 
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.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 3232 bytes --]

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 <james.quinlan@broadcom.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..68ee6eb5fc27 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 "message-serviced", 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, "message-serviced");
+	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


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

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

[Sudeep Holla]

On Thu, Nov 12, 2020 at 12:56:27PM -0500, 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.
>

Sorry for missing to check with you earlier. Are these not fast smc calls ?
Can we check the SMC Function IDs for the same and expect IRQ to be present
if they are not fast calls ?

-- 
Regards,
Sudeep

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

Hi, these are fast calls.  Regards, Jim


On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Nov 12, 2020 at 12:56:27PM -0500, 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.
> >
>
> Sorry for missing to check with you earlier. Are these not fast smc calls ?
> Can we check the SMC Function IDs for the same and expect IRQ to be present
> if they are not fast calls ?
Hi, if I understand you correctly you want to do something like this:

 if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
        /* look for irq and request it */
}

But we  do use fast calls.
Regards,
Jim

>
> --
> Regards,
> Sudeep

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

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

[Sudeep Holla]

On Fri, Nov 13, 2020 at 09:26:43AM -0500, Jim Quinlan wrote:
> Hi, these are fast calls.  Regards, Jim
> 
> 
> On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Nov 12, 2020 at 12:56:27PM -0500, 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.
> > >
> >
> > Sorry for missing to check with you earlier. Are these not fast smc calls ?
> > Can we check the SMC Function IDs for the same and expect IRQ to be present
> > if they are not fast calls ?
> Hi, if I understand you correctly you want to do something like this:
>
>  if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
>         /* look for irq and request it */
> }
>

Yes.

> But we  do use fast calls.

What was the rationale for retaining fast SMC calls but use IRQ for Tx
completion ?

Is it because you offload it to some other microprocessor and don't
continue execution on secure side in whcih case you can afford fast call ?

--
Regards,
Sudeep

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]

On Fri, Nov 13, 2020 at 9:36 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Nov 13, 2020 at 09:26:43AM -0500, Jim Quinlan wrote:
> > Hi, these are fast calls.  Regards, Jim
> >
> >
> > On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Nov 12, 2020 at 12:56:27PM -0500, 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.
> > > >
> > >
> > > Sorry for missing to check with you earlier. Are these not fast smc calls ?
> > > Can we check the SMC Function IDs for the same and expect IRQ to be present
> > > if they are not fast calls ?
> > Hi, if I understand you correctly you want to do something like this:
> >
> >  if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
> >         /* look for irq and request it */
> > }
> >
>
> Yes.
>
> > But we  do use fast calls.
>
> What was the rationale for retaining fast SMC calls but use IRQ for Tx
> completion ?
>
> Is it because you offload it to some other microprocessor and don't
> continue execution on secure side in whcih case you can afford fast call ?

Hi Sudeep,
I have an answer for this but allow me time to contact the platform FW
engineer to make sure I have the full picture -- this may take a day
or two.  Regardless, our implementation has already "shipped" to
customers for some time so we may not be able to change it.
Regards, Jim
>
>
> --
> Regards,
> Sudeep

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 2354 bytes --]

On Fri, Nov 13, 2020 at 10:12 AM Jim Quinlan <james.quinlan@broadcom.com> wrote:
>
> On Fri, Nov 13, 2020 at 9:36 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Fri, Nov 13, 2020 at 09:26:43AM -0500, Jim Quinlan wrote:
> > > Hi, these are fast calls.  Regards, Jim
> > >
> > >
> > > On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Thu, Nov 12, 2020 at 12:56:27PM -0500, 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.
> > > > >
> > > >
> > > > Sorry for missing to check with you earlier. Are these not fast smc calls ?
> > > > Can we check the SMC Function IDs for the same and expect IRQ to be present
> > > > if they are not fast calls ?
> > > Hi, if I understand you correctly you want to do something like this:
> > >
> > >  if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
> > >         /* look for irq and request it */
> > > }
> > >
> >
> > Yes.
> >
> > > But we  do use fast calls.
> >
> > What was the rationale for retaining fast SMC calls but use IRQ for Tx
> > completion ?
> >
> > Is it because you offload it to some other microprocessor and don't
> > continue execution on secure side in whcih case you can afford fast call ?
Hi Sudeep,
Here is my understanding:  Some SMC calls may take a few longer to
complete than others.  The longer ones tie up the CPU core that is
handling the SMC call, and so nothing can be scheduled on that
specific core.  Unfortunately, we have a real-time OS that runs
sporadically on one specific core and if that happens to be the same
core that is handling the SMC, the RTOS will miss its deadline.  So we
need to have the SMC return immediately and use an SGI for task
completion.

Regards,
Jim Quinlan
Broadcom STB


>
> Hi Sudeep,
> I have an answer for this but allow me time to contact the platform FW
> engineer to make sure I have the full picture -- this may take a day
> or two.  Regardless, our implementation has already "shipped" to
> customers for some time so we may not be able to change it.
> Regards, Jim
> >
> >
> > --
> > Regards,
> > Sudeep

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

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

[Sudeep Holla]

On Thu, Nov 19, 2020 at 01:34:18PM -0500, Jim Quinlan wrote:
> On Fri, Nov 13, 2020 at 10:12 AM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> >
> > On Fri, Nov 13, 2020 at 9:36 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Fri, Nov 13, 2020 at 09:26:43AM -0500, Jim Quinlan wrote:
> > > > Hi, these are fast calls.  Regards, Jim
> > > >
> > > >
> > > > On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > > > > On Thu, Nov 12, 2020 at 12:56:27PM -0500, 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.
> > > > > >
> > > > >
> > > > > Sorry for missing to check with you earlier. Are these not fast smc calls ?
> > > > > Can we check the SMC Function IDs for the same and expect IRQ to be present
> > > > > if they are not fast calls ?
> > > > Hi, if I understand you correctly you want to do something like this:
> > > >
> > > >  if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
> > > >         /* look for irq and request it */
> > > > }
> > > >
> > >
> > > Yes.
> > >
> > > > But we  do use fast calls.
> > >
> > > What was the rationale for retaining fast SMC calls but use IRQ for Tx
> > > completion ?
> > >
> > > Is it because you offload it to some other microprocessor and don't
> > > continue execution on secure side in whcih case you can afford fast call ?
> Hi Sudeep,
>

Thanks for the details. Unfortunately more questions:

> Here is my understanding:  Some SMC calls may take a few longer to
> complete than others. The longer ones tie up the CPU core that is
> handling the SMC call, and so nothing can be scheduled on that
> specific core.

So far good.

> Unfortunately, we have a real-time OS that runs
> sporadically on one specific core and if that happens to be the same
> core that is handling the SMC, the RTOS will miss its deadline.  So we
> need to have the SMC return immediately and use an SGI for task
> completion.
>

So it sounds more like it can't be fast call then.

Does that me, it will always return early and send SGI when the request
is complete ?

1. If yes, what happens if there are multiple requests in parallel and
   second one completes before the first. Can we handle that with this
   patch set. Of will the second request fails until the first one is
   complete ? It extends to number of cpus in the system worst case.

2. If no, will this not cause issues if we unconditional wait for interrupt
   every single time ?

--
Regards,
Sudeep

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 3636 bytes --]

On Fri, Nov 20, 2020 at 6:14 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Nov 19, 2020 at 01:34:18PM -0500, Jim Quinlan wrote:
> > On Fri, Nov 13, 2020 at 10:12 AM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> > >
> > > On Fri, Nov 13, 2020 at 9:36 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Fri, Nov 13, 2020 at 09:26:43AM -0500, Jim Quinlan wrote:
> > > > > Hi, these are fast calls.  Regards, Jim
> > > > >
> > > > >
> > > > > On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > > >
> > > > > > On Thu, Nov 12, 2020 at 12:56:27PM -0500, 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.
> > > > > > >
> > > > > >
> > > > > > Sorry for missing to check with you earlier. Are these not fast smc calls ?
> > > > > > Can we check the SMC Function IDs for the same and expect IRQ to be present
> > > > > > if they are not fast calls ?
> > > > > Hi, if I understand you correctly you want to do something like this:
> > > > >
> > > > >  if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
> > > > >         /* look for irq and request it */
> > > > > }
> > > > >
> > > >
> > > > Yes.
> > > >
> > > > > But we  do use fast calls.
> > > >
> > > > What was the rationale for retaining fast SMC calls but use IRQ for Tx
> > > > completion ?
> > > >
> > > > Is it because you offload it to some other microprocessor and don't
> > > > continue execution on secure side in whcih case you can afford fast call ?
> > Hi Sudeep,
> >
>
> Thanks for the details. Unfortunately more questions:
>
> > Here is my understanding:  Some SMC calls may take a few longer to
> > complete than others. The longer ones tie up the CPU core that is
> > handling the SMC call, and so nothing can be scheduled on that
> > specific core.
>
> So far good.
>
> > Unfortunately, we have a real-time OS that runs
> > sporadically on one specific core and if that happens to be the same
> > core that is handling the SMC, the RTOS will miss its deadline.  So we
> > need to have the SMC return immediately and use an SGI for task
> > completion.
> >
>
> So it sounds more like it can't be fast call then.
Hi Sudeep,

To be honest,  I'm not sure what the big difference between fast and
slow SMC calls are other than the latter is "yielding" and
interruptible.  We cannot tolerate them being interruptible.

>
> Does that me, it will always return early and send SGI when the request
> is complete ?
Most calls send the SGI and return immediately.  The ones that take
longer return from the SMC and send the SGI when the operation is
completed.
>
> 1. If yes, what happens if there are multiple requests in parallel and
>    second one completes before the first. Can we handle that with this
>    patch set. Of will the second request fails until the first one is
>    complete ? It extends to number of cpus in the system worst case.
With SCMI we only have one message pending  at  a time;  perhaps I do
not understand your question.  Having the SMC return is mostly a no-op
as far as the SCMI  driver is concerned.

Our SCMI messages cannot fail.  When we do have timeouts it indicates
that something is wrong and needs to be fixed.

Regards,
Jim Quinlan
Broadcom STB
>
> 2. If no, will this not cause issues if we unconditional wait for interrupt
>    every single time ?
>
> --
> Regards,
> Sudeep

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

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

[Sudeep Holla]

On Fri, Nov 20, 2020 at 08:27:38AM -0500, Jim Quinlan wrote:
> On Fri, Nov 20, 2020 at 6:14 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Nov 19, 2020 at 01:34:18PM -0500, Jim Quinlan wrote:
> > > On Fri, Nov 13, 2020 at 10:12 AM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> > > >
> > > > On Fri, Nov 13, 2020 at 9:36 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > > > > On Fri, Nov 13, 2020 at 09:26:43AM -0500, Jim Quinlan wrote:
> > > > > > Hi, these are fast calls.  Regards, Jim
> > > > > >
> > > > > >
> > > > > > On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > > > >
> > > > > > > On Thu, Nov 12, 2020 at 12:56:27PM -0500, 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.
> > > > > > > >
> > > > > > >
> > > > > > > Sorry for missing to check with you earlier. Are these not fast smc calls ?
> > > > > > > Can we check the SMC Function IDs for the same and expect IRQ to be present
> > > > > > > if they are not fast calls ?
> > > > > > Hi, if I understand you correctly you want to do something like this:
> > > > > >
> > > > > >  if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
> > > > > >         /* look for irq and request it */
> > > > > > }
> > > > > >
> > > > >
> > > > > Yes.
> > > > >
> > > > > > But we  do use fast calls.
> > > > >
> > > > > What was the rationale for retaining fast SMC calls but use IRQ for Tx
> > > > > completion ?
> > > > >
> > > > > Is it because you offload it to some other microprocessor and don't
> > > > > continue execution on secure side in whcih case you can afford fast call ?
> > > Hi Sudeep,
> > >
> >
> > Thanks for the details. Unfortunately more questions:
> >
> > > Here is my understanding:  Some SMC calls may take a few longer to
> > > complete than others. The longer ones tie up the CPU core that is
> > > handling the SMC call, and so nothing can be scheduled on that
> > > specific core.
> >
> > So far good.
> >
> > > Unfortunately, we have a real-time OS that runs
> > > sporadically on one specific core and if that happens to be the same
> > > core that is handling the SMC, the RTOS will miss its deadline.  So we
> > > need to have the SMC return immediately and use an SGI for task
> > > completion.
> > >
> >
> > So it sounds more like it can't be fast call then.
> Hi Sudeep,
>
> To be honest,  I'm not sure what the big difference between fast and
> slow SMC calls are other than the latter is "yielding" and
> interruptible.  We cannot tolerate them being interruptible.
>

OK

> >
> > Does that me, it will always return early and send SGI when the request
> > is complete ?
> Most calls send the SGI and return immediately.  The ones that take
> longer return from the SMC and send the SGI when the operation is
> completed.

That's relief.

> >
> > 1. If yes, what happens if there are multiple requests in parallel and
> >    second one completes before the first. Can we handle that with this
> >    patch set. Of will the second request fails until the first one is
> >    complete ? It extends to number of cpus in the system worst case.
>
> With SCMI we only have one message pending  at  a time;  perhaps I do
> not understand your question.  Having the SMC return is mostly a no-op
> as far as the SCMI  driver is concerned.
>

Yes we have lock, I forgot. There are requirements to make the smc atomic
by some vendors, was thinking about that and forgot about the lock and
how what I explained can never happen. Thanks for the patience.

If you ping and get Rob's ack on DT, I can take this patch along with
DT bindings for now as is. We can always enhance if required.

> Our SCMI messages cannot fail.  When we do have timeouts it indicates
> that something is wrong and needs to be fixed.
>

Good to know.

--
Regards,
Sudeep