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

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

[Jim Quinlan]

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

This change allows the SCMI "platform" to use the smc/hvc transport and to
optionally indicate the completion of an SCMI message with an interrupt.
This is in contrast to the nominal case where the return of the SMC call
indicates message completion.

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..d3b0c9f387fe 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 : the name must be "msg-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 --]

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

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 2802 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 | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 82a82a5dc86a..3bf935dbd00e 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);
@@ -79,6 +94,20 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	/* Optional feature -- signal message completion using an interrupt */
+	ret = of_irq_get_byname(cdev->of_node, "msg-serviced");
+	if (ret > 0) {
+		scmi_info->irq = ret;
+		ret = devm_request_irq(dev, ret, 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);
@@ -111,6 +140,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 v1 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Sudeep Holla]

On Tue, Nov 10, 2020 at 01:38:19PM -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.
>
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/firmware/arm_scmi/smc.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> index 82a82a5dc86a..3bf935dbd00e 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);
> @@ -79,6 +94,20 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
>  	if (ret < 0)
>  		return ret;
>
> +	/* Optional feature -- signal message completion using an interrupt */
> +	ret = of_irq_get_byname(cdev->of_node, "msg-serviced");

So, looks like it is mandatory if "interrupts" is used. Please update the
binding or if that is not the practice followed elsewhere, drop search by
name. Also I prefer full name "message-serviced" if possible, not strong
opinion.

> +	if (ret > 0) {
> +		scmi_info->irq = ret;

May be set this only if we succeed setting up handler ? I mean move this
down.

Other than these, the changes look fine.

--
Regards,
Sudeep

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

[Jim Quinlan]

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

On Wed, Nov 11, 2020 at 5:42 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Nov 10, 2020 at 01:38:19PM -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.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/firmware/arm_scmi/smc.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> > index 82a82a5dc86a..3bf935dbd00e 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);
> > @@ -79,6 +94,20 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> >       if (ret < 0)
> >               return ret;
> >
> > +     /* Optional feature -- signal message completion using an interrupt */
> > +     ret = of_irq_get_byname(cdev->of_node, "msg-serviced");
>
> So, looks like it is mandatory if "interrupts" is used. Please update the
> binding or if that is not the practice followed elsewhere, drop search by
> name.

Well, I can certainly change the comment. I do not want it to be
"mandatory" if just interrupts are used.
 The reason I prefer using "interrupt-names" is that this allows
unforeseen use of future additional interrupts w/o caring about order
in the interrupts DT node.  If you are absolutely positive that there
will never be other interrupts used  in the future for the SCMI node
then I will drop it.

>
> Also I prefer full name "message-serviced" if possible, not strong
> opinion.

Will do.

>
>
> > +     if (ret > 0) {
> > +             scmi_info->irq = ret;
>
> May be set this only if we succeed setting up handler ? I mean move this
> down.

Will do.

Regards,
Jim Quinlan
Broadcom STB


>
>
> Other than these, the changes look fine.
>
> --
> Regards,
> Sudeep

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

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

[Cristian Marussi]

On Wed, Nov 11, 2020 at 11:45:24AM -0500, Jim Quinlan wrote:
> On Wed, Nov 11, 2020 at 5:42 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Tue, Nov 10, 2020 at 01:38:19PM -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.
> > >
> > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > ---
> > >  drivers/firmware/arm_scmi/smc.c | 31 +++++++++++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> > > index 82a82a5dc86a..3bf935dbd00e 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);
> > > @@ -79,6 +94,20 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> > >       if (ret < 0)
> > >               return ret;
> > >
> > > +     /* Optional feature -- signal message completion using an interrupt */
> > > +     ret = of_irq_get_byname(cdev->of_node, "msg-serviced");
> >
> > So, looks like it is mandatory if "interrupts" is used. Please update the
> > binding or if that is not the practice followed elsewhere, drop search by
> > name.
> 
> Well, I can certainly change the comment. I do not want it to be
> "mandatory" if just interrupts are used.
>  The reason I prefer using "interrupt-names" is that this allows
> unforeseen use of future additional interrupts w/o caring about order
> in the interrupts DT node.  If you are absolutely positive that there
> will never be other interrupts used  in the future for the SCMI node
> then I will drop it.
> 

What about the future possibility of adding p2a notifications handling
to SMC transport, won't that need some other IRQ (and shmem) ?

Regards

Cristian

> >
> > Also I prefer full name "message-serviced" if possible, not strong
> > opinion.
> 
> Will do.
> 
> >
> >
> > > +     if (ret > 0) {
> > > +             scmi_info->irq = ret;
> >
> > May be set this only if we succeed setting up handler ? I mean move this
> > down.
> 
> Will do.
> 
> Regards,
> Jim Quinlan
> Broadcom STB
> 
> 
> >
> >
> > Other than these, the changes look fine.
> >
> > --
> > Regards,
> > Sudeep

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

[Sudeep Holla]

On Wed, Nov 11, 2020 at 05:45:46PM +0000, Cristian Marussi wrote:
> On Wed, Nov 11, 2020 at 11:45:24AM -0500, Jim Quinlan wrote:
> > On Wed, Nov 11, 2020 at 5:42 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Tue, Nov 10, 2020 at 01:38:19PM -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.
> > > >
> > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > > ---
> > > >  drivers/firmware/arm_scmi/smc.c | 31 +++++++++++++++++++++++++++++++
> > > >  1 file changed, 31 insertions(+)
> > > >
> > > > diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> > > > index 82a82a5dc86a..3bf935dbd00e 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);
> > > > @@ -79,6 +94,20 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> > > >       if (ret < 0)
> > > >               return ret;
> > > >
> > > > +     /* Optional feature -- signal message completion using an interrupt */
> > > > +     ret = of_irq_get_byname(cdev->of_node, "msg-serviced");
> > >
> > > So, looks like it is mandatory if "interrupts" is used. Please update the
> > > binding or if that is not the practice followed elsewhere, drop search by
> > > name.
> >
> > Well, I can certainly change the comment. I do not want it to be
> > "mandatory" if just interrupts are used.
> >  The reason I prefer using "interrupt-names" is that this allows
> > unforeseen use of future additional interrupts w/o caring about order
> > in the interrupts DT node. If you are absolutely positive that there
> > will never be other interrupts used  in the future for the SCMI node
> > then I will drop it.
> >

Good point, please make it required property then if "interrupts" property
is present.

>
> What about the future possibility of adding p2a notifications handling
> to SMC transport, won't that need some other IRQ (and shmem) ?
>

Indeed it needs. Since this Tx completion interrupt is optional and may not
be present, better to fix the name so that when Rx/notification interrupt
is added in future, we can identify them easily.

--
Regards,
Sudeep

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

[Sudeep Holla]

On Tue, Nov 10, 2020 at 01:38:18PM -0500, Jim Quinlan wrote:
> This change allows the SCMI "platform" to use the smc/hvc transport and to

Incorrect statement above, it is OSPM using SMC/HVC as transport and not
the "platform" which refers to entity/firmware implementing SCMI
services to OSPM. Also this change is not adding SMC/HVC, just the
interrupt support.

> optionally indicate the completion of an SCMI message with an interrupt.
> This is in contrast to the nominal case where the return of the SMC call
> indicates message completion.
>
> 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..d3b0c9f387fe 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

s/msg/message/

> +	 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 : the name must be "msg-serviced".

Is this mandatory if "interrupts" is present ?

> +
> +

Extra blank line ?

--
Regards,
Sudeep