[PATCH RFC 0/2] Support mailbox interface in qcom,smsm driver

[PATCH RFC 0/2] Support mailbox interface in qcom,smsm driver

[Luca Weiss]

Take a shot at converting the last driver that requires direct
"qcom,ipc*" syscon references in devicetree by allowing the smsm driver
to use the mailbox interface to achieve the same effect.

I'm not very happy about how the devicetree change looks in the end.
Perhaps it's better to use mbox-names to not have <0> elements in dt,
and reference the items by name from the driver?

e.g. this change for msm8226 could be represented differently.

-		qcom,ipc-1 = <&apcs 8 13>;
-		qcom,ipc-2 = <&apcs 8 9>;
-		qcom,ipc-3 = <&apcs 8 19>;
+		mboxes = <0>, <&apcs 13>, <&apcs 9>, <&apcs 19>;

vs. for example:

-		qcom,ipc-1 = <&apcs 8 13>;
-		qcom,ipc-2 = <&apcs 8 9>;
-		qcom,ipc-3 = <&apcs 8 19>;
+		mboxes = <&apcs 13>, <&apcs 9>, <&apcs 19>;
+		mbox-names = "ipc-1", "ipc-2", "ipc-3";

But also here the name with 'ipc-N' is probably not particularly
fitting?

Please let me know your thoughts and any suggestions.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
Luca Weiss (2):
      dt-bindings: soc: qcom,smsm: Allow specifying mboxes instead of qcom,ipc
      soc: qcom: smsm: Support using mailbox interface

 .../devicetree/bindings/soc/qcom/qcom,smsm.yaml    | 48 ++++++++++++++++----
 drivers/soc/qcom/smsm.c                            | 51 +++++++++++++++++++++-
 2 files changed, 90 insertions(+), 9 deletions(-)
---
base-commit: ed30a4a51bb196781c8058073ea720133a65596f
change-id: 20240424-smsm-mbox-0666f35eae44

Best regards,
-- 
Luca Weiss <luca@z3ntu.xyz>

[PATCH RFC 1/2] dt-bindings: soc: qcom,smsm: Allow specifying mboxes instead of qcom,ipc

[Luca Weiss]

The qcom,ipc-N properties are essentially providing a reference to a
mailbox, so allow using the mboxes property to do the same in a more
structured way.

Since multiple SMSM hosts are supported, we need to be able to provide
the correct mailbox for each host. The old qcom,ipc-N properties map to
the mboxes property by index, starting at 0 since that's a valid SMSM
host also.

The new example shows how an smsm node with just qcom,ipc-3 should be
specified with the mboxes property.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 .../devicetree/bindings/soc/qcom/qcom,smsm.yaml    | 48 ++++++++++++++++++----
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smsm.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smsm.yaml
index db67cf043256..b12589171169 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,smsm.yaml
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smsm.yaml
@@ -33,6 +33,13 @@ properties:
       specifier of the column in the subscription matrix representing the local
       processor.
 
+  mboxes:
+    minItems: 1
+    maxItems: 5
+    description:
+      Reference to the mailbox representing the outgoing doorbell in APCS for
+      this client.
+
   '#size-cells':
     const: 0
 
@@ -98,15 +105,18 @@ required:
   - '#address-cells'
   - '#size-cells'
 
-anyOf:
+oneOf:
   - required:
-      - qcom,ipc-1
-  - required:
-      - qcom,ipc-2
-  - required:
-      - qcom,ipc-3
-  - required:
-      - qcom,ipc-4
+      - mboxes
+  - anyOf:
+      - required:
+          - qcom,ipc-1
+      - required:
+          - qcom,ipc-2
+      - required:
+          - qcom,ipc-3
+      - required:
+          - qcom,ipc-4
 
 additionalProperties: false
 
@@ -136,3 +146,25 @@ examples:
             #interrupt-cells = <2>;
         };
     };
+  # Example using mboxes property
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    shared-memory {
+        compatible = "qcom,smsm";
+        #address-cells = <1>;
+        #size-cells = <0>;
+        mboxes = <0>, <0>, <0>, <&apcs 19>;
+
+        apps@0 {
+            reg = <0>;
+            #qcom,smem-state-cells = <1>;
+        };
+
+        wcnss@7 {
+            reg = <7>;
+            interrupts = <GIC_SPI 144 IRQ_TYPE_EDGE_RISING>;
+            interrupt-controller;
+            #interrupt-cells = <2>;
+        };
+    };

-- 
2.44.0

[PATCH RFC 2/2] soc: qcom: smsm: Support using mailbox interface

[Luca Weiss]

Add support for using the mbox interface instead of manually writing to
the syscon. With this change the driver will attempt to get the mailbox
first, and if that fails it will fall back to the existing way of using
qcom,ipc-* properties and converting to syscon.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 drivers/soc/qcom/smsm.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/smsm.c b/drivers/soc/qcom/smsm.c
index e7c7e9a640a6..ffe78ae34386 100644
--- a/drivers/soc/qcom/smsm.c
+++ b/drivers/soc/qcom/smsm.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/interrupt.h>
+#include <linux/mailbox_client.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of_irq.h>
@@ -71,6 +72,7 @@ struct smsm_host;
  * @lock:	spinlock for read-modify-write of the outgoing state
  * @entries:	context for each of the entries
  * @hosts:	context for each of the hosts
+ * @mbox_client: mailbox client handle
  */
 struct qcom_smsm {
 	struct device *dev;
@@ -88,6 +90,8 @@ struct qcom_smsm {
 
 	struct smsm_entry *entries;
 	struct smsm_host *hosts;
+
+	struct mbox_client mbox_client;
 };
 
 /**
@@ -120,11 +124,14 @@ struct smsm_entry {
  * @ipc_regmap:	regmap for outgoing interrupt
  * @ipc_offset:	offset in @ipc_regmap for outgoing interrupt
  * @ipc_bit:	bit in @ipc_regmap + @ipc_offset for outgoing interrupt
+ * @mbox_chan:	apcs ipc mailbox channel handle
  */
 struct smsm_host {
 	struct regmap *ipc_regmap;
 	int ipc_offset;
 	int ipc_bit;
+
+	struct mbox_chan *mbox_chan;
 };
 
 /**
@@ -172,7 +179,13 @@ static int smsm_update_bits(void *data, u32 mask, u32 value)
 		hostp = &smsm->hosts[host];
 
 		val = readl(smsm->subscription + host);
-		if (val & changes && hostp->ipc_regmap) {
+		if (!(val & changes))
+			continue;
+
+		if (hostp->mbox_chan) {
+			mbox_send_message(hostp->mbox_chan, NULL);
+			mbox_client_txdone(hostp->mbox_chan, 0);
+		} else if (hostp->ipc_regmap) {
 			regmap_write(hostp->ipc_regmap,
 				     hostp->ipc_offset,
 				     BIT(hostp->ipc_bit));
@@ -352,6 +365,28 @@ static const struct irq_domain_ops smsm_irq_ops = {
 	.xlate = irq_domain_xlate_twocell,
 };
 
+/**
+ * smsm_parse_mbox() - requests an mbox channel
+ * @smsm:	smsm driver context
+ * @host_id:	index of the remote host to be resolved
+ *
+ * Requests the desired channel using the mbox interface which is needed for
+ * sending the outgoing interrupts to a remove hosts - identified by @host_id.
+ */
+static int smsm_parse_mbox(struct qcom_smsm *smsm, unsigned int host_id)
+{
+	struct smsm_host *host = &smsm->hosts[host_id];
+	int ret = 0;
+
+	host->mbox_chan = mbox_request_channel(&smsm->mbox_client, host_id);
+	if (IS_ERR(host->mbox_chan)) {
+		ret = PTR_ERR(host->mbox_chan);
+		host->mbox_chan = NULL;
+	}
+
+	return ret;
+}
+
 /**
  * smsm_parse_ipc() - parses a qcom,ipc-%d device tree property
  * @smsm:	smsm driver context
@@ -521,8 +556,16 @@ static int qcom_smsm_probe(struct platform_device *pdev)
 			     "qcom,local-host",
 			     &smsm->local_host);
 
+	smsm->mbox_client.dev = &pdev->dev;
+	smsm->mbox_client.knows_txdone = true;
+
 	/* Parse the host properties */
 	for (id = 0; id < smsm->num_hosts; id++) {
+		/* Try using mbox interface first, otherwise fall back to syscon */
+		ret = smsm_parse_mbox(smsm, id);
+		if (!ret)
+			continue;
+
 		ret = smsm_parse_ipc(smsm, id);
 		if (ret < 0)
 			goto out_put;
@@ -609,6 +652,9 @@ static int qcom_smsm_probe(struct platform_device *pdev)
 
 	qcom_smem_state_unregister(smsm->state);
 out_put:
+	for (id = 0; id < smsm->num_hosts; id++)
+		mbox_free_channel(smsm->hosts[id].mbox_chan);
+
 	of_node_put(local_node);
 	return ret;
 }
@@ -622,6 +668,9 @@ static void qcom_smsm_remove(struct platform_device *pdev)
 		if (smsm->entries[id].domain)
 			irq_domain_remove(smsm->entries[id].domain);
 
+	for (id = 0; id < smsm->num_hosts; id++)
+		mbox_free_channel(smsm->hosts[id].mbox_chan);
+
 	qcom_smem_state_unregister(smsm->state);
 }
 

-- 
2.44.0

Re: [PATCH RFC 2/2] soc: qcom: smsm: Support using mailbox interface

[Konrad Dybcio]

On 4/24/24 19:21, Luca Weiss wrote:
> Add support for using the mbox interface instead of manually writing to
> the syscon. With this change the driver will attempt to get the mailbox
> first, and if that fails it will fall back to the existing way of using
> qcom,ipc-* properties and converting to syscon.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---

Looks good!

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH RFC 1/2] dt-bindings: soc: qcom,smsm: Allow specifying mboxes instead of qcom,ipc

[Rob Herring]

On Wed, Apr 24, 2024 at 07:21:51PM +0200, Luca Weiss wrote:
> The qcom,ipc-N properties are essentially providing a reference to a
> mailbox, so allow using the mboxes property to do the same in a more
> structured way.

Can we mark qcom,ipc-N as deprecated then?
 
> Since multiple SMSM hosts are supported, we need to be able to provide
> the correct mailbox for each host. The old qcom,ipc-N properties map to
> the mboxes property by index, starting at 0 since that's a valid SMSM
> host also.
> 
> The new example shows how an smsm node with just qcom,ipc-3 should be
> specified with the mboxes property.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
>  .../devicetree/bindings/soc/qcom/qcom,smsm.yaml    | 48 ++++++++++++++++++----
>  1 file changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smsm.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smsm.yaml
> index db67cf043256..b12589171169 100644
> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smsm.yaml
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smsm.yaml
> @@ -33,6 +33,13 @@ properties:
>        specifier of the column in the subscription matrix representing the local
>        processor.
>  
> +  mboxes:
> +    minItems: 1
> +    maxItems: 5

Need to define what each entry is.

> +    description:
> +      Reference to the mailbox representing the outgoing doorbell in APCS for
> +      this client.
> +
>    '#size-cells':
>      const: 0
>  
> @@ -98,15 +105,18 @@ required:
>    - '#address-cells'
>    - '#size-cells'
>  
> -anyOf:
> +oneOf:
>    - required:
> -      - qcom,ipc-1
> -  - required:
> -      - qcom,ipc-2
> -  - required:
> -      - qcom,ipc-3
> -  - required:
> -      - qcom,ipc-4
> +      - mboxes
> +  - anyOf:
> +      - required:
> +          - qcom,ipc-1
> +      - required:
> +          - qcom,ipc-2
> +      - required:
> +          - qcom,ipc-3
> +      - required:
> +          - qcom,ipc-4
>  
>  additionalProperties: false
>  
> @@ -136,3 +146,25 @@ examples:
>              #interrupt-cells = <2>;
>          };
>      };
> +  # Example using mboxes property
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    shared-memory {
> +        compatible = "qcom,smsm";
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        mboxes = <0>, <0>, <0>, <&apcs 19>;
> +
> +        apps@0 {
> +            reg = <0>;
> +            #qcom,smem-state-cells = <1>;
> +        };
> +
> +        wcnss@7 {
> +            reg = <7>;
> +            interrupts = <GIC_SPI 144 IRQ_TYPE_EDGE_RISING>;
> +            interrupt-controller;
> +            #interrupt-cells = <2>;
> +        };
> +    };
> 
> -- 
> 2.44.0
> 

Re: [PATCH RFC 1/2] dt-bindings: soc: qcom,smsm: Allow specifying mboxes instead of qcom,ipc

[Luca Weiss]

On Donnerstag, 25. April 2024 18:17:15 MESZ Rob Herring wrote:
> On Wed, Apr 24, 2024 at 07:21:51PM +0200, Luca Weiss wrote:
> > The qcom,ipc-N properties are essentially providing a reference to a
> > mailbox, so allow using the mboxes property to do the same in a more
> > structured way.
> 
> Can we mark qcom,ipc-N as deprecated then?

Yes, that should be ok. Will also send a similar change to the other bindings
that support both qcom,ipc and mboxes.

>  
> > Since multiple SMSM hosts are supported, we need to be able to provide
> > the correct mailbox for each host. The old qcom,ipc-N properties map to
> > the mboxes property by index, starting at 0 since that's a valid SMSM
> > host also.
> > 
> > The new example shows how an smsm node with just qcom,ipc-3 should be
> > specified with the mboxes property.
> > 
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> >  .../devicetree/bindings/soc/qcom/qcom,smsm.yaml    | 48 ++++++++++++++++++----
> >  1 file changed, 40 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smsm.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smsm.yaml
> > index db67cf043256..b12589171169 100644
> > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smsm.yaml
> > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smsm.yaml
> > @@ -33,6 +33,13 @@ properties:
> >        specifier of the column in the subscription matrix representing the local
> >        processor.
> >  
> > +  mboxes:
> > +    minItems: 1
> > +    maxItems: 5
> 
> Need to define what each entry is.

The entry is (description from qcom,ipc-N)

  "the outgoing ipc bit used for signaling the N:th remote processor."

So you want me to add 5 times e.g.

- the IPC mailbox used for signaling the 0th remote processor
- the IPC mailbox used for signaling the 1st remote processor

etc? I don't really have any extra knowledge on smsm to be able to write
something better there..

Also what are your thoughts on this binding vs the alternative I wrote
in the cover letter? I'm not really happy about how the properties are
represented.

Regards
Luca


> 
> > +    description:
> > +      Reference to the mailbox representing the outgoing doorbell in APCS for
> > +      this client.
> > +
> >    '#size-cells':
> >      const: 0
> >  
> > @@ -98,15 +105,18 @@ required:
> >    - '#address-cells'
> >    - '#size-cells'
> >  
> > -anyOf:
> > +oneOf:
> >    - required:
> > -      - qcom,ipc-1
> > -  - required:
> > -      - qcom,ipc-2
> > -  - required:
> > -      - qcom,ipc-3
> > -  - required:
> > -      - qcom,ipc-4
> > +      - mboxes
> > +  - anyOf:
> > +      - required:
> > +          - qcom,ipc-1
> > +      - required:
> > +          - qcom,ipc-2
> > +      - required:
> > +          - qcom,ipc-3
> > +      - required:
> > +          - qcom,ipc-4
> >  
> >  additionalProperties: false
> >  
> > @@ -136,3 +146,25 @@ examples:
> >              #interrupt-cells = <2>;
> >          };
> >      };
> > +  # Example using mboxes property
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    shared-memory {
> > +        compatible = "qcom,smsm";
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        mboxes = <0>, <0>, <0>, <&apcs 19>;
> > +
> > +        apps@0 {
> > +            reg = <0>;
> > +            #qcom,smem-state-cells = <1>;
> > +        };
> > +
> > +        wcnss@7 {
> > +            reg = <7>;
> > +            interrupts = <GIC_SPI 144 IRQ_TYPE_EDGE_RISING>;
> > +            interrupt-controller;
> > +            #interrupt-cells = <2>;
> > +        };
> > +    };
> > 
>