All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Add SCMI support for mailbox unidirectional channels
@ 2023-03-27 14:03 ` Cristian Marussi
  0 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-03-27 14:03 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, devicetree
  Cc: sudeep.holla, vincent.guittot, souvik.chakravarty,
	nicola.mazzucato, cristian.marussi, robh+dt,
	krzysztof.kozlowski+dt

Hi,

this series aims to extend SCMI mailbox transport layer to support mailbox
controllers that expose unidirectional channels.

SCMI communications between an agent like Linux and the platform fw server
happens through 2 main communication channels: an 'a2p' bidirectional
channel (called TX in SCMI parlance) used to send synchronous commands
and receive related replies and an optional 'p2a' unidirectional channel
(called RX) used to convey notfications or delayed responses to asynchronous
commands possibly emitted by the platform toward the agent.

The current SCMI mailbox transport support was modelled around mailboxes
that were bidirectional by nature, and, as such, fit well in the above SCMI
communication scheme, allowing us to currently describe the mailbox
transport channels as in the following examples:

 1.  system with a single TX 'a2p' defined over a mailbox bidirectional
     channel:

	mboxes = <&mb 0 0>;
	shmem = <&a2p_mem>;

 2. system with a TX 'a2p' defined over a bidirectional mailbox channel
    AND an optional RX 'p2a' defined over a unidirectional channel:

	mboxes = <&mb 0 0>, <&mb 0 1>,
	shmem = <&a2p_shmem>, <&p2a_shmem>;

This binding, as it is now, does NOT support the usage of mailbox
controllers exposing channels that are unidirectional by nature, like
it is the case with ARM MHUv2 mailboxes as an example, since 2 distinct
unidirectional mailbox channels would be needed to represent just the
SCMI TX bidirectional communication path.

Note that the mboxes property referred in the SCMI nodes to configure the
transport is compliant with (and parsed by) the mailbox common subsystem,
which is the entity that exposes and finally handles the mailbox controller:
as a consequence playing creatively (or dirty :P) with the syntax of the
mboxes property to fit our needs is not an option.

This series extends the SCMI mailbox-related bindings, which defines how
mboxes and shmem properties are interpreted, and the logic inside the
SCMI mailbox transport subsystem to support the usage of these type of
unidirectional mailbox channels, while aiming to maintain backward
compatibility with the original scheme based on bidirectional channels.

With these proposed DT extensions, in addition to the above definitions,
the following descriptions can be crafted for a system using a mailbox
controller exposing unidirectional channels:

 2. system with a single TX 'a2p' defined over a pair of unidirectional
    mailbox channels (similar to 1):

	mboxes = <&mb_tx 0 0>, <&mb_rx 0 0>;
	shmem = <&a2p_mem>;

 3. system with a TX 'a2p' defined over a pair of unidirectional channels
    AND an RX 'p2a' defined over a unidirectional channel (similar to 2):

	mboxes = <&mb_tx 0 0>, <&mb_rx 0 0>, <&mb_rx 0 1>;
	shmem = <&a2p_shmem>, <&p2a_shmem>;

The SCMI mailbox transport logic has been modified to select and make a
proper use of the needed channels depending on the combination of found
mboxes/shmem descriptors:

  a) 1 mbox / 1 shmem => SCMI TX over 1 mailbox bidirectional channel
  b) 2 mbox / 2 shmem => SCMI TX and RX over 2 mailbox bidirectional chans

  c) 2 mbox / 1 shmem => SCMI TX over 2 mailbox unidirectional chans
  d) 3 mbox / 2 shmem => SCMI TX and RX over 3 mailbox unidirectional chans

with any other combination considered invalid.

Note that, up until the changes in this series, the only valid configs
accepted by the SCMI mailbox transport are a) and b): this ensures backward
compatibility even in the case in which a DT sporting the new format (c,d)
is, wrongly, deployed with an old kernel still not supporting this new logic:
in such a case c) and d) configs will be simply rejected. (wrongly deployed
because installing a c) or d) styled-DT would be required only if the
underlying mailbox HW had effectively changed and used unidir chans)

I have tested this on a JUNO board (MHUv1 bidirectional) and TotalCompute
TC2 reference design (MHUv2 unidirectional). [1]

The series is based on v6.3-rc4.

Having said that, I am not completely sure if all of the above constraints
should (and/or could) be expressed in a more formal way also in the YAML
binding itself.

Any feedback or suggestion in these regards is highly appreciated.

Thanks,
Cristian

[1]: https://gitlab.arm.com/arm-reference-solutions/arm-reference-solutions-docs/-/blob/master/docs/totalcompute/tc2/tc2_sw_stack.rst

----


Cristian Marussi (2):
  dt-bindings: firmware: arm,scmi: Support mailboxes unidirectional
    channels
  firmware: arm_scmi: Add support for unidirectional mailbox channels

 .../bindings/firmware/arm,scmi.yaml           | 42 +++++++-
 drivers/firmware/arm_scmi/mailbox.c           | 95 ++++++++++++++++---
 2 files changed, 122 insertions(+), 15 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 0/2] Add SCMI support for mailbox unidirectional channels
@ 2023-03-27 14:03 ` Cristian Marussi
  0 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-03-27 14:03 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, devicetree
  Cc: sudeep.holla, vincent.guittot, souvik.chakravarty,
	nicola.mazzucato, cristian.marussi, robh+dt,
	krzysztof.kozlowski+dt

Hi,

this series aims to extend SCMI mailbox transport layer to support mailbox
controllers that expose unidirectional channels.

SCMI communications between an agent like Linux and the platform fw server
happens through 2 main communication channels: an 'a2p' bidirectional
channel (called TX in SCMI parlance) used to send synchronous commands
and receive related replies and an optional 'p2a' unidirectional channel
(called RX) used to convey notfications or delayed responses to asynchronous
commands possibly emitted by the platform toward the agent.

The current SCMI mailbox transport support was modelled around mailboxes
that were bidirectional by nature, and, as such, fit well in the above SCMI
communication scheme, allowing us to currently describe the mailbox
transport channels as in the following examples:

 1.  system with a single TX 'a2p' defined over a mailbox bidirectional
     channel:

	mboxes = <&mb 0 0>;
	shmem = <&a2p_mem>;

 2. system with a TX 'a2p' defined over a bidirectional mailbox channel
    AND an optional RX 'p2a' defined over a unidirectional channel:

	mboxes = <&mb 0 0>, <&mb 0 1>,
	shmem = <&a2p_shmem>, <&p2a_shmem>;

This binding, as it is now, does NOT support the usage of mailbox
controllers exposing channels that are unidirectional by nature, like
it is the case with ARM MHUv2 mailboxes as an example, since 2 distinct
unidirectional mailbox channels would be needed to represent just the
SCMI TX bidirectional communication path.

Note that the mboxes property referred in the SCMI nodes to configure the
transport is compliant with (and parsed by) the mailbox common subsystem,
which is the entity that exposes and finally handles the mailbox controller:
as a consequence playing creatively (or dirty :P) with the syntax of the
mboxes property to fit our needs is not an option.

This series extends the SCMI mailbox-related bindings, which defines how
mboxes and shmem properties are interpreted, and the logic inside the
SCMI mailbox transport subsystem to support the usage of these type of
unidirectional mailbox channels, while aiming to maintain backward
compatibility with the original scheme based on bidirectional channels.

With these proposed DT extensions, in addition to the above definitions,
the following descriptions can be crafted for a system using a mailbox
controller exposing unidirectional channels:

 2. system with a single TX 'a2p' defined over a pair of unidirectional
    mailbox channels (similar to 1):

	mboxes = <&mb_tx 0 0>, <&mb_rx 0 0>;
	shmem = <&a2p_mem>;

 3. system with a TX 'a2p' defined over a pair of unidirectional channels
    AND an RX 'p2a' defined over a unidirectional channel (similar to 2):

	mboxes = <&mb_tx 0 0>, <&mb_rx 0 0>, <&mb_rx 0 1>;
	shmem = <&a2p_shmem>, <&p2a_shmem>;

The SCMI mailbox transport logic has been modified to select and make a
proper use of the needed channels depending on the combination of found
mboxes/shmem descriptors:

  a) 1 mbox / 1 shmem => SCMI TX over 1 mailbox bidirectional channel
  b) 2 mbox / 2 shmem => SCMI TX and RX over 2 mailbox bidirectional chans

  c) 2 mbox / 1 shmem => SCMI TX over 2 mailbox unidirectional chans
  d) 3 mbox / 2 shmem => SCMI TX and RX over 3 mailbox unidirectional chans

with any other combination considered invalid.

Note that, up until the changes in this series, the only valid configs
accepted by the SCMI mailbox transport are a) and b): this ensures backward
compatibility even in the case in which a DT sporting the new format (c,d)
is, wrongly, deployed with an old kernel still not supporting this new logic:
in such a case c) and d) configs will be simply rejected. (wrongly deployed
because installing a c) or d) styled-DT would be required only if the
underlying mailbox HW had effectively changed and used unidir chans)

I have tested this on a JUNO board (MHUv1 bidirectional) and TotalCompute
TC2 reference design (MHUv2 unidirectional). [1]

The series is based on v6.3-rc4.

Having said that, I am not completely sure if all of the above constraints
should (and/or could) be expressed in a more formal way also in the YAML
binding itself.

Any feedback or suggestion in these regards is highly appreciated.

Thanks,
Cristian

[1]: https://gitlab.arm.com/arm-reference-solutions/arm-reference-solutions-docs/-/blob/master/docs/totalcompute/tc2/tc2_sw_stack.rst

----


Cristian Marussi (2):
  dt-bindings: firmware: arm,scmi: Support mailboxes unidirectional
    channels
  firmware: arm_scmi: Add support for unidirectional mailbox channels

 .../bindings/firmware/arm,scmi.yaml           | 42 +++++++-
 drivers/firmware/arm_scmi/mailbox.c           | 95 ++++++++++++++++---
 2 files changed, 122 insertions(+), 15 deletions(-)

-- 
2.34.1


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2] dt-bindings: firmware: arm,scmi: Support mailboxes unidirectional channels
  2023-03-27 14:03 ` Cristian Marussi
@ 2023-03-27 14:03   ` Cristian Marussi
  -1 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-03-27 14:03 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, devicetree
  Cc: sudeep.holla, vincent.guittot, souvik.chakravarty,
	nicola.mazzucato, cristian.marussi, robh+dt,
	krzysztof.kozlowski+dt

SCMI defines two kinds of communication channels between the agent and the
platform: one bidirectional 'a2p' channel used by the agent to send SCMI
commands and synchronously receive the related replies, and an optional
'p2a' unidirectional channel used to asynchronously receive delayed
responses and notifications emitted from the platform.

When configuring an SCMI transport based on mailboxes, the current binding
supports only mailboxes providing bidirectional channels: in such a case
one mailbox channel can be easily assigned to each SCMI channel as above
described.

In case, instead, to have to deal with mailboxes providing only distinct
unidirectional channels, it becomes necessary to extend the binding in
order to be able to bind 2 distinct unidirectional mailbox channels to the
same SCMI 'a2p' channel.

Bidirectional and unidirectional channels support for the SCMI mailbox
transport can coexist by carefully considering the effective combination
of defined 'mboxes' and 'shmem' descriptors.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 .../bindings/firmware/arm,scmi.yaml           | 42 +++++++++++++++++--
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 2f7c51c75e85..9a7dc30e386f 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -63,10 +63,24 @@ properties:
   mboxes:
     description:
       List of phandle and mailbox channel specifiers. It should contain
-      exactly one or two mailboxes, one for transmitting messages("tx")
-      and another optional for receiving the notifications("rx") if supported.
+      exactly one, two or three mailboxes; the first one or two for transmitting
+      messages ("tx") and another optional ("rx") for receiving notifications
+      and delayed responses, if supported by the platform.
+      The number of mailboxes needed for transmitting messages depends on the
+      type of channels exposed by the specific underlying mailbox controller;
+      one single channel descriptor is enough if such channel is bidirectional,
+      while two channel descriptors are needed to represent the SCMI ("tx")
+      channel if the underlying mailbox channels are of unidirectional type.
+      The effective combination in numbers of mboxes and shmem descriptors let
+      the SCMI subsystem determine unambiguosly which type of SCMI channels are
+      made available by the underlying mailbox controller and how to use them.
+       1 mbox / 1 shmem => SCMI TX over 1 mailbox bidirectional channel
+       2 mbox / 2 shmem => SCMI TX and RX over 2 mailbox bidirectional channels
+       2 mbox / 1 shmem => SCMI TX over 2 mailbox unidirectional channels
+       3 mbox / 2 shmem => SCMI TX and RX over 3 mailbox unidirectional channels
+      Any other combination of mboxes and shmem is invalid.
     minItems: 1
-    maxItems: 2
+    maxItems: 3
 
   shmem:
     description:
@@ -234,7 +248,7 @@ $defs:
 
       mboxes:
         minItems: 1
-        maxItems: 2
+        maxItems: 3
 
       shmem:
         minItems: 1
@@ -393,6 +407,26 @@ examples:
         };
     };
 
+  - |
+    firmware {
+        scmi {
+            compatible = "arm,scmi";
+            mboxes = <&mhu_U_tx 0 0>, <&mhu_U_rx 0 0>;
+            shmem = <&cpu_scp_lpri0>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            scmi_dvfs_2: protocol@13 {
+                reg = <0x13>;
+                #clock-cells = <1>;
+
+                mboxes = <&mhu_U_tx 1 0>, <&mhu_U_rx 1 0>, <&mhu_U_rx 1 1>;
+                shmem = <&cpu_scp_hpri0>, <&cpu_scp_hpri1>;
+            };
+        };
+    };
+
   - |
     firmware {
         scmi {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 1/2] dt-bindings: firmware: arm,scmi: Support mailboxes unidirectional channels
@ 2023-03-27 14:03   ` Cristian Marussi
  0 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-03-27 14:03 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, devicetree
  Cc: sudeep.holla, vincent.guittot, souvik.chakravarty,
	nicola.mazzucato, cristian.marussi, robh+dt,
	krzysztof.kozlowski+dt

SCMI defines two kinds of communication channels between the agent and the
platform: one bidirectional 'a2p' channel used by the agent to send SCMI
commands and synchronously receive the related replies, and an optional
'p2a' unidirectional channel used to asynchronously receive delayed
responses and notifications emitted from the platform.

When configuring an SCMI transport based on mailboxes, the current binding
supports only mailboxes providing bidirectional channels: in such a case
one mailbox channel can be easily assigned to each SCMI channel as above
described.

In case, instead, to have to deal with mailboxes providing only distinct
unidirectional channels, it becomes necessary to extend the binding in
order to be able to bind 2 distinct unidirectional mailbox channels to the
same SCMI 'a2p' channel.

Bidirectional and unidirectional channels support for the SCMI mailbox
transport can coexist by carefully considering the effective combination
of defined 'mboxes' and 'shmem' descriptors.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 .../bindings/firmware/arm,scmi.yaml           | 42 +++++++++++++++++--
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 2f7c51c75e85..9a7dc30e386f 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -63,10 +63,24 @@ properties:
   mboxes:
     description:
       List of phandle and mailbox channel specifiers. It should contain
-      exactly one or two mailboxes, one for transmitting messages("tx")
-      and another optional for receiving the notifications("rx") if supported.
+      exactly one, two or three mailboxes; the first one or two for transmitting
+      messages ("tx") and another optional ("rx") for receiving notifications
+      and delayed responses, if supported by the platform.
+      The number of mailboxes needed for transmitting messages depends on the
+      type of channels exposed by the specific underlying mailbox controller;
+      one single channel descriptor is enough if such channel is bidirectional,
+      while two channel descriptors are needed to represent the SCMI ("tx")
+      channel if the underlying mailbox channels are of unidirectional type.
+      The effective combination in numbers of mboxes and shmem descriptors let
+      the SCMI subsystem determine unambiguosly which type of SCMI channels are
+      made available by the underlying mailbox controller and how to use them.
+       1 mbox / 1 shmem => SCMI TX over 1 mailbox bidirectional channel
+       2 mbox / 2 shmem => SCMI TX and RX over 2 mailbox bidirectional channels
+       2 mbox / 1 shmem => SCMI TX over 2 mailbox unidirectional channels
+       3 mbox / 2 shmem => SCMI TX and RX over 3 mailbox unidirectional channels
+      Any other combination of mboxes and shmem is invalid.
     minItems: 1
-    maxItems: 2
+    maxItems: 3
 
   shmem:
     description:
@@ -234,7 +248,7 @@ $defs:
 
       mboxes:
         minItems: 1
-        maxItems: 2
+        maxItems: 3
 
       shmem:
         minItems: 1
@@ -393,6 +407,26 @@ examples:
         };
     };
 
+  - |
+    firmware {
+        scmi {
+            compatible = "arm,scmi";
+            mboxes = <&mhu_U_tx 0 0>, <&mhu_U_rx 0 0>;
+            shmem = <&cpu_scp_lpri0>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            scmi_dvfs_2: protocol@13 {
+                reg = <0x13>;
+                #clock-cells = <1>;
+
+                mboxes = <&mhu_U_tx 1 0>, <&mhu_U_rx 1 0>, <&mhu_U_rx 1 1>;
+                shmem = <&cpu_scp_hpri0>, <&cpu_scp_hpri1>;
+            };
+        };
+    };
+
   - |
     firmware {
         scmi {
-- 
2.34.1


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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] firmware: arm_scmi: Add support for unidirectional mailbox channels
  2023-03-27 14:03 ` Cristian Marussi
@ 2023-03-27 14:03   ` Cristian Marussi
  -1 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-03-27 14:03 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, devicetree
  Cc: sudeep.holla, vincent.guittot, souvik.chakravarty,
	nicola.mazzucato, cristian.marussi, robh+dt,
	krzysztof.kozlowski+dt

Extend the SCMI transport layer to support mailbox controllers that expose
communication channels that are unidirectional by nature.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/mailbox.c | 95 +++++++++++++++++++++++++----
 1 file changed, 84 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
index 112c285deb97..1efa5e9392c4 100644
--- a/drivers/firmware/arm_scmi/mailbox.c
+++ b/drivers/firmware/arm_scmi/mailbox.c
@@ -19,13 +19,15 @@
  * struct scmi_mailbox - Structure representing a SCMI mailbox transport
  *
  * @cl: Mailbox Client
- * @chan: Transmit/Receive mailbox channel
+ * @chan: Transmit/Receive mailbox uni/bi-directional channel
+ * @chan_receiver: Optional Receiver mailbox unidirectional channel
  * @cinfo: SCMI channel info
  * @shmem: Transmit/Receive shared memory area
  */
 struct scmi_mailbox {
 	struct mbox_client cl;
 	struct mbox_chan *chan;
+	struct mbox_chan *chan_receiver;
 	struct scmi_chan_info *cinfo;
 	struct scmi_shared_mem __iomem *shmem;
 };
@@ -48,30 +50,62 @@ static void rx_callback(struct mbox_client *cl, void *m)
 
 static bool mailbox_chan_available(struct device_node *of_node, int idx)
 {
+	int num_mb;
+
+	/*
+	 * Just check if bidirrectional channels are involved, and check the
+	 * index accordingly; proper full validation will be made later
+	 * in mailbox_chan_setup().
+	 */
+	num_mb = of_count_phandle_with_args(of_node, "mboxes", "#mbox-cells");
+	if (num_mb == 3 && idx == 1)
+		idx = 2;
+
 	return !of_parse_phandle_with_args(of_node, "mboxes",
 					   "#mbox-cells", idx, NULL);
 }
 
-static int mailbox_chan_validate(struct device *cdev)
+/**
+ * mailbox_chan_validate  - Validate transport configuration and map channels
+ *
+ * @cdev: Reference to the underlying transport device carrying the
+ *	  of_node descriptor to analyze.
+ * @a2p_rx_chan: A reference to an optional unidirectional channel to use
+ *		 for replies on the a2p channel. Set as zero if not present.
+ * @p2a_chan: A reference to the optional p2a channel.
+ *	      Set as zero if not present.
+ *
+ * At first, validate the transport configuration as described in terms of
+ * 'mboxes' and 'shmem', then determin which mailbox channel indexes are
+ * appropriate to be use in the current configuration.
+ *
+ * Return: 0 on Success or error
+ */
+static int mailbox_chan_validate(struct device *cdev,
+				 int *a2p_rx_chan, int *p2a_chan)
 {
 	int num_mb, num_sh, ret = 0;
 	struct device_node *np = cdev->of_node;
 
 	num_mb = of_count_phandle_with_args(np, "mboxes", "#mbox-cells");
 	num_sh = of_count_phandle_with_args(np, "shmem", NULL);
+	dev_dbg(cdev, "Found %d mboxes and %d shmems !\n", num_mb, num_sh);
+
 	/* Bail out if mboxes and shmem descriptors are inconsistent */
-	if (num_mb <= 0 || num_sh > 2 || num_mb != num_sh) {
-		dev_warn(cdev, "Invalid channel descriptor for '%s'\n",
-			 of_node_full_name(np));
+	if (num_mb <= 0 || num_sh <= 0 || num_sh > 2 || num_mb > 3 ||
+	    (num_mb == 1 && num_sh != 1) || (num_mb == 3 && num_sh != 2)) {
+		dev_warn(cdev,
+			 "Invalid channel descriptor for '%s' - mbs:%d  shm:%d\n",
+			 of_node_full_name(np), num_mb, num_sh);
 		return -EINVAL;
 	}
 
+	/* Bail out if provided shmem descriptors do not refer distinct areas  */
 	if (num_sh > 1) {
 		struct device_node *np_tx, *np_rx;
 
 		np_tx = of_parse_phandle(np, "shmem", 0);
 		np_rx = of_parse_phandle(np, "shmem", 1);
-		/* SCMI Tx and Rx shared mem areas have to be distinct */
 		if (!np_tx || !np_rx || np_tx == np_rx) {
 			dev_warn(cdev, "Invalid shmem descriptor for '%s'\n",
 				 of_node_full_name(np));
@@ -82,6 +116,29 @@ static int mailbox_chan_validate(struct device *cdev)
 		of_node_put(np_rx);
 	}
 
+	/* Calculate channels IDs to use depending on mboxes/shmem layout */
+	if (!ret) {
+		switch (num_mb) {
+		case 1:
+			*a2p_rx_chan = 0;
+			*p2a_chan = 0;
+			break;
+		case 2:
+			if (num_sh == 2) {
+				*a2p_rx_chan = 0;
+				*p2a_chan = 1;
+			} else {
+				*a2p_rx_chan = 1;
+				*p2a_chan = 0;
+			}
+			break;
+		case 3:
+			*a2p_rx_chan = 1;
+			*p2a_chan = 2;
+			break;
+		}
+	}
+
 	return ret;
 }
 
@@ -92,15 +149,18 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	struct device *cdev = cinfo->dev;
 	struct scmi_mailbox *smbox;
 	struct device_node *shmem;
-	int ret, idx = tx ? 0 : 1;
+	int ret, a2p_rx_chan, p2a_chan, idx = tx ? 0 : 1;
 	struct mbox_client *cl;
 	resource_size_t size;
 	struct resource res;
 
-	ret = mailbox_chan_validate(cdev);
+	ret = mailbox_chan_validate(cdev, &a2p_rx_chan, &p2a_chan);
 	if (ret)
 		return ret;
 
+	if (!tx && !p2a_chan)
+		return -ENODEV;
+
 	smbox = devm_kzalloc(dev, sizeof(*smbox), GFP_KERNEL);
 	if (!smbox)
 		return -ENOMEM;
@@ -130,15 +190,26 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	cl->tx_block = false;
 	cl->knows_txdone = tx;
 
-	smbox->chan = mbox_request_channel(cl, tx ? 0 : 1);
+	smbox->chan = mbox_request_channel(cl, tx ? 0 : p2a_chan);
 	if (IS_ERR(smbox->chan)) {
 		ret = PTR_ERR(smbox->chan);
 		if (ret != -EPROBE_DEFER)
-			dev_err(cdev, "failed to request SCMI %s mailbox\n",
-				tx ? "Tx" : "Rx");
+			dev_err(cdev,
+				"failed to request SCMI %s mailbox\n", desc);
 		return ret;
 	}
 
+	/* Additional unidirectional channel for TX if needed */
+	if (tx && a2p_rx_chan) {
+		smbox->chan_receiver = mbox_request_channel(cl, a2p_rx_chan);
+		if (IS_ERR(smbox->chan_receiver)) {
+			ret = PTR_ERR(smbox->chan_receiver);
+			if (ret != -EPROBE_DEFER)
+				dev_err(cdev, "failed to request SCMI Tx Receiver mailbox\n");
+			return ret;
+		}
+	}
+
 	cinfo->transport_info = smbox;
 	smbox->cinfo = cinfo;
 
@@ -152,8 +223,10 @@ static int mailbox_chan_free(int id, void *p, void *data)
 
 	if (smbox && !IS_ERR(smbox->chan)) {
 		mbox_free_channel(smbox->chan);
+		mbox_free_channel(smbox->chan_receiver);
 		cinfo->transport_info = NULL;
 		smbox->chan = NULL;
+		smbox->chan_receiver = NULL;
 		smbox->cinfo = NULL;
 	}
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] firmware: arm_scmi: Add support for unidirectional mailbox channels
@ 2023-03-27 14:03   ` Cristian Marussi
  0 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-03-27 14:03 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, devicetree
  Cc: sudeep.holla, vincent.guittot, souvik.chakravarty,
	nicola.mazzucato, cristian.marussi, robh+dt,
	krzysztof.kozlowski+dt

Extend the SCMI transport layer to support mailbox controllers that expose
communication channels that are unidirectional by nature.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/mailbox.c | 95 +++++++++++++++++++++++++----
 1 file changed, 84 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
index 112c285deb97..1efa5e9392c4 100644
--- a/drivers/firmware/arm_scmi/mailbox.c
+++ b/drivers/firmware/arm_scmi/mailbox.c
@@ -19,13 +19,15 @@
  * struct scmi_mailbox - Structure representing a SCMI mailbox transport
  *
  * @cl: Mailbox Client
- * @chan: Transmit/Receive mailbox channel
+ * @chan: Transmit/Receive mailbox uni/bi-directional channel
+ * @chan_receiver: Optional Receiver mailbox unidirectional channel
  * @cinfo: SCMI channel info
  * @shmem: Transmit/Receive shared memory area
  */
 struct scmi_mailbox {
 	struct mbox_client cl;
 	struct mbox_chan *chan;
+	struct mbox_chan *chan_receiver;
 	struct scmi_chan_info *cinfo;
 	struct scmi_shared_mem __iomem *shmem;
 };
@@ -48,30 +50,62 @@ static void rx_callback(struct mbox_client *cl, void *m)
 
 static bool mailbox_chan_available(struct device_node *of_node, int idx)
 {
+	int num_mb;
+
+	/*
+	 * Just check if bidirrectional channels are involved, and check the
+	 * index accordingly; proper full validation will be made later
+	 * in mailbox_chan_setup().
+	 */
+	num_mb = of_count_phandle_with_args(of_node, "mboxes", "#mbox-cells");
+	if (num_mb == 3 && idx == 1)
+		idx = 2;
+
 	return !of_parse_phandle_with_args(of_node, "mboxes",
 					   "#mbox-cells", idx, NULL);
 }
 
-static int mailbox_chan_validate(struct device *cdev)
+/**
+ * mailbox_chan_validate  - Validate transport configuration and map channels
+ *
+ * @cdev: Reference to the underlying transport device carrying the
+ *	  of_node descriptor to analyze.
+ * @a2p_rx_chan: A reference to an optional unidirectional channel to use
+ *		 for replies on the a2p channel. Set as zero if not present.
+ * @p2a_chan: A reference to the optional p2a channel.
+ *	      Set as zero if not present.
+ *
+ * At first, validate the transport configuration as described in terms of
+ * 'mboxes' and 'shmem', then determin which mailbox channel indexes are
+ * appropriate to be use in the current configuration.
+ *
+ * Return: 0 on Success or error
+ */
+static int mailbox_chan_validate(struct device *cdev,
+				 int *a2p_rx_chan, int *p2a_chan)
 {
 	int num_mb, num_sh, ret = 0;
 	struct device_node *np = cdev->of_node;
 
 	num_mb = of_count_phandle_with_args(np, "mboxes", "#mbox-cells");
 	num_sh = of_count_phandle_with_args(np, "shmem", NULL);
+	dev_dbg(cdev, "Found %d mboxes and %d shmems !\n", num_mb, num_sh);
+
 	/* Bail out if mboxes and shmem descriptors are inconsistent */
-	if (num_mb <= 0 || num_sh > 2 || num_mb != num_sh) {
-		dev_warn(cdev, "Invalid channel descriptor for '%s'\n",
-			 of_node_full_name(np));
+	if (num_mb <= 0 || num_sh <= 0 || num_sh > 2 || num_mb > 3 ||
+	    (num_mb == 1 && num_sh != 1) || (num_mb == 3 && num_sh != 2)) {
+		dev_warn(cdev,
+			 "Invalid channel descriptor for '%s' - mbs:%d  shm:%d\n",
+			 of_node_full_name(np), num_mb, num_sh);
 		return -EINVAL;
 	}
 
+	/* Bail out if provided shmem descriptors do not refer distinct areas  */
 	if (num_sh > 1) {
 		struct device_node *np_tx, *np_rx;
 
 		np_tx = of_parse_phandle(np, "shmem", 0);
 		np_rx = of_parse_phandle(np, "shmem", 1);
-		/* SCMI Tx and Rx shared mem areas have to be distinct */
 		if (!np_tx || !np_rx || np_tx == np_rx) {
 			dev_warn(cdev, "Invalid shmem descriptor for '%s'\n",
 				 of_node_full_name(np));
@@ -82,6 +116,29 @@ static int mailbox_chan_validate(struct device *cdev)
 		of_node_put(np_rx);
 	}
 
+	/* Calculate channels IDs to use depending on mboxes/shmem layout */
+	if (!ret) {
+		switch (num_mb) {
+		case 1:
+			*a2p_rx_chan = 0;
+			*p2a_chan = 0;
+			break;
+		case 2:
+			if (num_sh == 2) {
+				*a2p_rx_chan = 0;
+				*p2a_chan = 1;
+			} else {
+				*a2p_rx_chan = 1;
+				*p2a_chan = 0;
+			}
+			break;
+		case 3:
+			*a2p_rx_chan = 1;
+			*p2a_chan = 2;
+			break;
+		}
+	}
+
 	return ret;
 }
 
@@ -92,15 +149,18 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	struct device *cdev = cinfo->dev;
 	struct scmi_mailbox *smbox;
 	struct device_node *shmem;
-	int ret, idx = tx ? 0 : 1;
+	int ret, a2p_rx_chan, p2a_chan, idx = tx ? 0 : 1;
 	struct mbox_client *cl;
 	resource_size_t size;
 	struct resource res;
 
-	ret = mailbox_chan_validate(cdev);
+	ret = mailbox_chan_validate(cdev, &a2p_rx_chan, &p2a_chan);
 	if (ret)
 		return ret;
 
+	if (!tx && !p2a_chan)
+		return -ENODEV;
+
 	smbox = devm_kzalloc(dev, sizeof(*smbox), GFP_KERNEL);
 	if (!smbox)
 		return -ENOMEM;
@@ -130,15 +190,26 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 	cl->tx_block = false;
 	cl->knows_txdone = tx;
 
-	smbox->chan = mbox_request_channel(cl, tx ? 0 : 1);
+	smbox->chan = mbox_request_channel(cl, tx ? 0 : p2a_chan);
 	if (IS_ERR(smbox->chan)) {
 		ret = PTR_ERR(smbox->chan);
 		if (ret != -EPROBE_DEFER)
-			dev_err(cdev, "failed to request SCMI %s mailbox\n",
-				tx ? "Tx" : "Rx");
+			dev_err(cdev,
+				"failed to request SCMI %s mailbox\n", desc);
 		return ret;
 	}
 
+	/* Additional unidirectional channel for TX if needed */
+	if (tx && a2p_rx_chan) {
+		smbox->chan_receiver = mbox_request_channel(cl, a2p_rx_chan);
+		if (IS_ERR(smbox->chan_receiver)) {
+			ret = PTR_ERR(smbox->chan_receiver);
+			if (ret != -EPROBE_DEFER)
+				dev_err(cdev, "failed to request SCMI Tx Receiver mailbox\n");
+			return ret;
+		}
+	}
+
 	cinfo->transport_info = smbox;
 	smbox->cinfo = cinfo;
 
@@ -152,8 +223,10 @@ static int mailbox_chan_free(int id, void *p, void *data)
 
 	if (smbox && !IS_ERR(smbox->chan)) {
 		mbox_free_channel(smbox->chan);
+		mbox_free_channel(smbox->chan_receiver);
 		cinfo->transport_info = NULL;
 		smbox->chan = NULL;
+		smbox->chan_receiver = NULL;
 		smbox->cinfo = NULL;
 	}
 
-- 
2.34.1


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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: Support mailboxes unidirectional channels
  2023-03-27 14:03   ` Cristian Marussi
@ 2023-03-27 14:42     ` Krzysztof Kozlowski
  -1 siblings, 0 replies; 14+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-27 14:42 UTC (permalink / raw)
  To: Cristian Marussi, linux-kernel, linux-arm-kernel, devicetree
  Cc: sudeep.holla, vincent.guittot, souvik.chakravarty,
	nicola.mazzucato, robh+dt, krzysztof.kozlowski+dt

On 27/03/2023 16:03, Cristian Marussi wrote:
> SCMI defines two kinds of communication channels between the agent and the
> platform: one bidirectional 'a2p' channel used by the agent to send SCMI
> commands and synchronously receive the related replies, and an optional
> 'p2a' unidirectional channel used to asynchronously receive delayed
> responses and notifications emitted from the platform.
> 
> When configuring an SCMI transport based on mailboxes, the current binding
> supports only mailboxes providing bidirectional channels: in such a case
> one mailbox channel can be easily assigned to each SCMI channel as above
> described.
> 
> In case, instead, to have to deal with mailboxes providing only distinct
> unidirectional channels, it becomes necessary to extend the binding in
> order to be able to bind 2 distinct unidirectional mailbox channels to the
> same SCMI 'a2p' channel.
> 
> Bidirectional and unidirectional channels support for the SCMI mailbox
> transport can coexist by carefully considering the effective combination
> of defined 'mboxes' and 'shmem' descriptors.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: devicetree@vger.kernel.org

Please drop the autogenerated scripts/get_maintainer.pl CC-entries from
commit msg. There is no single need to store automated output of
get_maintainers.pl in the git log. It can be easily re-created at any
given time, thus its presence in the git history is redundant and
obfuscates the log.

If you need it for your own patch management purposes, keep it under ---.


> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  .../bindings/firmware/arm,scmi.yaml           | 42 +++++++++++++++++--
>  1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 2f7c51c75e85..9a7dc30e386f 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -63,10 +63,24 @@ properties:
>    mboxes:
>      description:
>        List of phandle and mailbox channel specifiers. It should contain
> -      exactly one or two mailboxes, one for transmitting messages("tx")
> -      and another optional for receiving the notifications("rx") if supported.
> +      exactly one, two or three mailboxes; the first one or two for transmitting
> +      messages ("tx") and another optional ("rx") for receiving notifications
> +      and delayed responses, if supported by the platform.
> +      The number of mailboxes needed for transmitting messages depends on the
> +      type of channels exposed by the specific underlying mailbox controller;
> +      one single channel descriptor is enough if such channel is bidirectional,
> +      while two channel descriptors are needed to represent the SCMI ("tx")
> +      channel if the underlying mailbox channels are of unidirectional type.
> +      The effective combination in numbers of mboxes and shmem descriptors let
> +      the SCMI subsystem determine unambiguosly which type of SCMI channels are
> +      made available by the underlying mailbox controller and how to use them.
> +       1 mbox / 1 shmem => SCMI TX over 1 mailbox bidirectional channel
> +       2 mbox / 2 shmem => SCMI TX and RX over 2 mailbox bidirectional channels
> +       2 mbox / 1 shmem => SCMI TX over 2 mailbox unidirectional channels
> +       3 mbox / 2 shmem => SCMI TX and RX over 3 mailbox unidirectional channels
> +      Any other combination of mboxes and shmem is invalid.
>      minItems: 1
> -    maxItems: 2
> +    maxItems: 3

Missing update to mbox-names.

>  
>    shmem:
>      description:
> @@ -234,7 +248,7 @@ $defs:
>  
>        mboxes:
>          minItems: 1
> -        maxItems: 2
> +        maxItems: 3

The same. How is it supposed to work? tx rx and that's it?

>  
>        shmem:
>          minItems: 1
> @@ -393,6 +407,26 @@ examples:
>          };
>      };
>  
> +  - |
> +    firmware {
> +        scmi {
> +            compatible = "arm,scmi";
> +            mboxes = <&mhu_U_tx 0 0>, <&mhu_U_rx 0 0>;
> +            shmem = <&cpu_scp_lpri0>;
> +
> +            #address-cells = <1>;
> +            #size-cells = <0>;

I don't think adding one more example with difference in only one piece
is needed here.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: Support mailboxes unidirectional channels
@ 2023-03-27 14:42     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 14+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-27 14:42 UTC (permalink / raw)
  To: Cristian Marussi, linux-kernel, linux-arm-kernel, devicetree
  Cc: sudeep.holla, vincent.guittot, souvik.chakravarty,
	nicola.mazzucato, robh+dt, krzysztof.kozlowski+dt

On 27/03/2023 16:03, Cristian Marussi wrote:
> SCMI defines two kinds of communication channels between the agent and the
> platform: one bidirectional 'a2p' channel used by the agent to send SCMI
> commands and synchronously receive the related replies, and an optional
> 'p2a' unidirectional channel used to asynchronously receive delayed
> responses and notifications emitted from the platform.
> 
> When configuring an SCMI transport based on mailboxes, the current binding
> supports only mailboxes providing bidirectional channels: in such a case
> one mailbox channel can be easily assigned to each SCMI channel as above
> described.
> 
> In case, instead, to have to deal with mailboxes providing only distinct
> unidirectional channels, it becomes necessary to extend the binding in
> order to be able to bind 2 distinct unidirectional mailbox channels to the
> same SCMI 'a2p' channel.
> 
> Bidirectional and unidirectional channels support for the SCMI mailbox
> transport can coexist by carefully considering the effective combination
> of defined 'mboxes' and 'shmem' descriptors.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: devicetree@vger.kernel.org

Please drop the autogenerated scripts/get_maintainer.pl CC-entries from
commit msg. There is no single need to store automated output of
get_maintainers.pl in the git log. It can be easily re-created at any
given time, thus its presence in the git history is redundant and
obfuscates the log.

If you need it for your own patch management purposes, keep it under ---.


> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  .../bindings/firmware/arm,scmi.yaml           | 42 +++++++++++++++++--
>  1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 2f7c51c75e85..9a7dc30e386f 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -63,10 +63,24 @@ properties:
>    mboxes:
>      description:
>        List of phandle and mailbox channel specifiers. It should contain
> -      exactly one or two mailboxes, one for transmitting messages("tx")
> -      and another optional for receiving the notifications("rx") if supported.
> +      exactly one, two or three mailboxes; the first one or two for transmitting
> +      messages ("tx") and another optional ("rx") for receiving notifications
> +      and delayed responses, if supported by the platform.
> +      The number of mailboxes needed for transmitting messages depends on the
> +      type of channels exposed by the specific underlying mailbox controller;
> +      one single channel descriptor is enough if such channel is bidirectional,
> +      while two channel descriptors are needed to represent the SCMI ("tx")
> +      channel if the underlying mailbox channels are of unidirectional type.
> +      The effective combination in numbers of mboxes and shmem descriptors let
> +      the SCMI subsystem determine unambiguosly which type of SCMI channels are
> +      made available by the underlying mailbox controller and how to use them.
> +       1 mbox / 1 shmem => SCMI TX over 1 mailbox bidirectional channel
> +       2 mbox / 2 shmem => SCMI TX and RX over 2 mailbox bidirectional channels
> +       2 mbox / 1 shmem => SCMI TX over 2 mailbox unidirectional channels
> +       3 mbox / 2 shmem => SCMI TX and RX over 3 mailbox unidirectional channels
> +      Any other combination of mboxes and shmem is invalid.
>      minItems: 1
> -    maxItems: 2
> +    maxItems: 3

Missing update to mbox-names.

>  
>    shmem:
>      description:
> @@ -234,7 +248,7 @@ $defs:
>  
>        mboxes:
>          minItems: 1
> -        maxItems: 2
> +        maxItems: 3

The same. How is it supposed to work? tx rx and that's it?

>  
>        shmem:
>          minItems: 1
> @@ -393,6 +407,26 @@ examples:
>          };
>      };
>  
> +  - |
> +    firmware {
> +        scmi {
> +            compatible = "arm,scmi";
> +            mboxes = <&mhu_U_tx 0 0>, <&mhu_U_rx 0 0>;
> +            shmem = <&cpu_scp_lpri0>;
> +
> +            #address-cells = <1>;
> +            #size-cells = <0>;

I don't think adding one more example with difference in only one piece
is needed here.

Best regards,
Krzysztof


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: Support mailboxes unidirectional channels
  2023-03-27 14:42     ` Krzysztof Kozlowski
@ 2023-03-27 15:27       ` Cristian Marussi
  -1 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-03-27 15:27 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-kernel, linux-arm-kernel, devicetree, sudeep.holla,
	vincent.guittot, souvik.chakravarty, nicola.mazzucato, robh+dt,
	krzysztof.kozlowski+dt

On Mon, Mar 27, 2023 at 04:42:12PM +0200, Krzysztof Kozlowski wrote:
> On 27/03/2023 16:03, Cristian Marussi wrote:
> > SCMI defines two kinds of communication channels between the agent and the
> > platform: one bidirectional 'a2p' channel used by the agent to send SCMI
> > commands and synchronously receive the related replies, and an optional
> > 'p2a' unidirectional channel used to asynchronously receive delayed
> > responses and notifications emitted from the platform.
> > 
> > When configuring an SCMI transport based on mailboxes, the current binding
> > supports only mailboxes providing bidirectional channels: in such a case
> > one mailbox channel can be easily assigned to each SCMI channel as above
> > described.
> > 
> > In case, instead, to have to deal with mailboxes providing only distinct
> > unidirectional channels, it becomes necessary to extend the binding in
> > order to be able to bind 2 distinct unidirectional mailbox channels to the
> > same SCMI 'a2p' channel.
> > 
> > Bidirectional and unidirectional channels support for the SCMI mailbox
> > transport can coexist by carefully considering the effective combination
> > of defined 'mboxes' and 'shmem' descriptors.
> > 
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > Cc: devicetree@vger.kernel.org
> 

Hi Krzysztof,

thanks for having a look.

> Please drop the autogenerated scripts/get_maintainer.pl CC-entries from
> commit msg. There is no single need to store automated output of
> get_maintainers.pl in the git log. It can be easily re-created at any
> given time, thus its presence in the git history is redundant and
> obfuscates the log.
> 
> If you need it for your own patch management purposes, keep it under ---.
> 

Ok. I use to add those Cc to trigger git-sendmail to add proper CCs but
in this case indeed I copied you on all the series anyway. I'll drop it.

> 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >  .../bindings/firmware/arm,scmi.yaml           | 42 +++++++++++++++++--
> >  1 file changed, 38 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > index 2f7c51c75e85..9a7dc30e386f 100644
> > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > @@ -63,10 +63,24 @@ properties:
> >    mboxes:
> >      description:
> >        List of phandle and mailbox channel specifiers. It should contain
> > -      exactly one or two mailboxes, one for transmitting messages("tx")
> > -      and another optional for receiving the notifications("rx") if supported.
> > +      exactly one, two or three mailboxes; the first one or two for transmitting
> > +      messages ("tx") and another optional ("rx") for receiving notifications
> > +      and delayed responses, if supported by the platform.
> > +      The number of mailboxes needed for transmitting messages depends on the
> > +      type of channels exposed by the specific underlying mailbox controller;
> > +      one single channel descriptor is enough if such channel is bidirectional,
> > +      while two channel descriptors are needed to represent the SCMI ("tx")
> > +      channel if the underlying mailbox channels are of unidirectional type.
> > +      The effective combination in numbers of mboxes and shmem descriptors let
> > +      the SCMI subsystem determine unambiguosly which type of SCMI channels are
> > +      made available by the underlying mailbox controller and how to use them.
> > +       1 mbox / 1 shmem => SCMI TX over 1 mailbox bidirectional channel
> > +       2 mbox / 2 shmem => SCMI TX and RX over 2 mailbox bidirectional channels
> > +       2 mbox / 1 shmem => SCMI TX over 2 mailbox unidirectional channels
> > +       3 mbox / 2 shmem => SCMI TX and RX over 3 mailbox unidirectional channels
> > +      Any other combination of mboxes and shmem is invalid.
> >      minItems: 1
> > -    maxItems: 2
> > +    maxItems: 3
> 
> Missing update to mbox-names.
> 
Ah .. missed that since mbox-names is not marked as a required property
in this binding. I'll add in V2.

> >  
> >    shmem:
> >      description:
> > @@ -234,7 +248,7 @@ $defs:
> >  
> >        mboxes:
> >          minItems: 1
> > -        maxItems: 2
> > +        maxItems: 3
> 
> The same. How is it supposed to work? tx rx and that's it?
> 

The logic is that SCMI transport can determine which type of channels
(bidir vs unidir) you are using by looking at how many mboxes and how
many shmem are defined as detailed in the description above.
(not using mbox-names refs because was never marked as required so it
 would break backward compatibility starting to use that)

I'll add a fix in V2 to fit mbox-names into this logic too.

> >  
> >        shmem:
> >          minItems: 1
> > @@ -393,6 +407,26 @@ examples:
> >          };
> >      };
> >  
> > +  - |
> > +    firmware {
> > +        scmi {
> > +            compatible = "arm,scmi";
> > +            mboxes = <&mhu_U_tx 0 0>, <&mhu_U_rx 0 0>;
> > +            shmem = <&cpu_scp_lpri0>;
> > +
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> 
> I don't think adding one more example with difference in only one piece
> is needed here.
> 

Mmm, I thought was sensible to add this example, given that a mailbox
transport configuration for a mailbox exposing unidrectional channels is
quite different from the usual bidirectional channel config already
present in the pre-existent example.

I'll add mbox-names into this example and see if I can change your
mind...or I can then finally drop it.

Thanks,
Cristian

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: Support mailboxes unidirectional channels
@ 2023-03-27 15:27       ` Cristian Marussi
  0 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-03-27 15:27 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-kernel, linux-arm-kernel, devicetree, sudeep.holla,
	vincent.guittot, souvik.chakravarty, nicola.mazzucato, robh+dt,
	krzysztof.kozlowski+dt

On Mon, Mar 27, 2023 at 04:42:12PM +0200, Krzysztof Kozlowski wrote:
> On 27/03/2023 16:03, Cristian Marussi wrote:
> > SCMI defines two kinds of communication channels between the agent and the
> > platform: one bidirectional 'a2p' channel used by the agent to send SCMI
> > commands and synchronously receive the related replies, and an optional
> > 'p2a' unidirectional channel used to asynchronously receive delayed
> > responses and notifications emitted from the platform.
> > 
> > When configuring an SCMI transport based on mailboxes, the current binding
> > supports only mailboxes providing bidirectional channels: in such a case
> > one mailbox channel can be easily assigned to each SCMI channel as above
> > described.
> > 
> > In case, instead, to have to deal with mailboxes providing only distinct
> > unidirectional channels, it becomes necessary to extend the binding in
> > order to be able to bind 2 distinct unidirectional mailbox channels to the
> > same SCMI 'a2p' channel.
> > 
> > Bidirectional and unidirectional channels support for the SCMI mailbox
> > transport can coexist by carefully considering the effective combination
> > of defined 'mboxes' and 'shmem' descriptors.
> > 
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > Cc: devicetree@vger.kernel.org
> 

Hi Krzysztof,

thanks for having a look.

> Please drop the autogenerated scripts/get_maintainer.pl CC-entries from
> commit msg. There is no single need to store automated output of
> get_maintainers.pl in the git log. It can be easily re-created at any
> given time, thus its presence in the git history is redundant and
> obfuscates the log.
> 
> If you need it for your own patch management purposes, keep it under ---.
> 

Ok. I use to add those Cc to trigger git-sendmail to add proper CCs but
in this case indeed I copied you on all the series anyway. I'll drop it.

> 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >  .../bindings/firmware/arm,scmi.yaml           | 42 +++++++++++++++++--
> >  1 file changed, 38 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > index 2f7c51c75e85..9a7dc30e386f 100644
> > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > @@ -63,10 +63,24 @@ properties:
> >    mboxes:
> >      description:
> >        List of phandle and mailbox channel specifiers. It should contain
> > -      exactly one or two mailboxes, one for transmitting messages("tx")
> > -      and another optional for receiving the notifications("rx") if supported.
> > +      exactly one, two or three mailboxes; the first one or two for transmitting
> > +      messages ("tx") and another optional ("rx") for receiving notifications
> > +      and delayed responses, if supported by the platform.
> > +      The number of mailboxes needed for transmitting messages depends on the
> > +      type of channels exposed by the specific underlying mailbox controller;
> > +      one single channel descriptor is enough if such channel is bidirectional,
> > +      while two channel descriptors are needed to represent the SCMI ("tx")
> > +      channel if the underlying mailbox channels are of unidirectional type.
> > +      The effective combination in numbers of mboxes and shmem descriptors let
> > +      the SCMI subsystem determine unambiguosly which type of SCMI channels are
> > +      made available by the underlying mailbox controller and how to use them.
> > +       1 mbox / 1 shmem => SCMI TX over 1 mailbox bidirectional channel
> > +       2 mbox / 2 shmem => SCMI TX and RX over 2 mailbox bidirectional channels
> > +       2 mbox / 1 shmem => SCMI TX over 2 mailbox unidirectional channels
> > +       3 mbox / 2 shmem => SCMI TX and RX over 3 mailbox unidirectional channels
> > +      Any other combination of mboxes and shmem is invalid.
> >      minItems: 1
> > -    maxItems: 2
> > +    maxItems: 3
> 
> Missing update to mbox-names.
> 
Ah .. missed that since mbox-names is not marked as a required property
in this binding. I'll add in V2.

> >  
> >    shmem:
> >      description:
> > @@ -234,7 +248,7 @@ $defs:
> >  
> >        mboxes:
> >          minItems: 1
> > -        maxItems: 2
> > +        maxItems: 3
> 
> The same. How is it supposed to work? tx rx and that's it?
> 

The logic is that SCMI transport can determine which type of channels
(bidir vs unidir) you are using by looking at how many mboxes and how
many shmem are defined as detailed in the description above.
(not using mbox-names refs because was never marked as required so it
 would break backward compatibility starting to use that)

I'll add a fix in V2 to fit mbox-names into this logic too.

> >  
> >        shmem:
> >          minItems: 1
> > @@ -393,6 +407,26 @@ examples:
> >          };
> >      };
> >  
> > +  - |
> > +    firmware {
> > +        scmi {
> > +            compatible = "arm,scmi";
> > +            mboxes = <&mhu_U_tx 0 0>, <&mhu_U_rx 0 0>;
> > +            shmem = <&cpu_scp_lpri0>;
> > +
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> 
> I don't think adding one more example with difference in only one piece
> is needed here.
> 

Mmm, I thought was sensible to add this example, given that a mailbox
transport configuration for a mailbox exposing unidrectional channels is
quite different from the usual bidirectional channel config already
present in the pre-existent example.

I'll add mbox-names into this example and see if I can change your
mind...or I can then finally drop it.

Thanks,
Cristian

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: Support mailboxes unidirectional channels
  2023-03-27 15:27       ` Cristian Marussi
@ 2023-03-28  6:36         ` Krzysztof Kozlowski
  -1 siblings, 0 replies; 14+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-28  6:36 UTC (permalink / raw)
  To: Cristian Marussi
  Cc: linux-kernel, linux-arm-kernel, devicetree, sudeep.holla,
	vincent.guittot, souvik.chakravarty, nicola.mazzucato, robh+dt,
	krzysztof.kozlowski+dt

On 27/03/2023 17:27, Cristian Marussi wrote:
>>> +  - |
>>> +    firmware {
>>> +        scmi {
>>> +            compatible = "arm,scmi";
>>> +            mboxes = <&mhu_U_tx 0 0>, <&mhu_U_rx 0 0>;
>>> +            shmem = <&cpu_scp_lpri0>;
>>> +
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>
>> I don't think adding one more example with difference in only one piece
>> is needed here.
>>
> 
> Mmm, I thought was sensible to add this example, given that a mailbox
> transport configuration for a mailbox exposing unidrectional channels is
> quite different from the usual bidirectional channel config already
> present in the pre-existent example.
> 
> I'll add mbox-names into this example and see if I can change your
> mind...or I can then finally drop it.

And what exactly this one more example changes? Does not validate
different parts of the binding if only one property differs...

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: Support mailboxes unidirectional channels
@ 2023-03-28  6:36         ` Krzysztof Kozlowski
  0 siblings, 0 replies; 14+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-28  6:36 UTC (permalink / raw)
  To: Cristian Marussi
  Cc: linux-kernel, linux-arm-kernel, devicetree, sudeep.holla,
	vincent.guittot, souvik.chakravarty, nicola.mazzucato, robh+dt,
	krzysztof.kozlowski+dt

On 27/03/2023 17:27, Cristian Marussi wrote:
>>> +  - |
>>> +    firmware {
>>> +        scmi {
>>> +            compatible = "arm,scmi";
>>> +            mboxes = <&mhu_U_tx 0 0>, <&mhu_U_rx 0 0>;
>>> +            shmem = <&cpu_scp_lpri0>;
>>> +
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>
>> I don't think adding one more example with difference in only one piece
>> is needed here.
>>
> 
> Mmm, I thought was sensible to add this example, given that a mailbox
> transport configuration for a mailbox exposing unidrectional channels is
> quite different from the usual bidirectional channel config already
> present in the pre-existent example.
> 
> I'll add mbox-names into this example and see if I can change your
> mind...or I can then finally drop it.

And what exactly this one more example changes? Does not validate
different parts of the binding if only one property differs...

Best regards,
Krzysztof


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: Support mailboxes unidirectional channels
  2023-03-28  6:36         ` Krzysztof Kozlowski
@ 2023-03-28  7:26           ` Cristian Marussi
  -1 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-03-28  7:26 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-kernel, linux-arm-kernel, devicetree, sudeep.holla,
	vincent.guittot, souvik.chakravarty, nicola.mazzucato, robh+dt,
	krzysztof.kozlowski+dt

On Tue, Mar 28, 2023 at 08:36:51AM +0200, Krzysztof Kozlowski wrote:
> On 27/03/2023 17:27, Cristian Marussi wrote:
> >>> +  - |
> >>> +    firmware {
> >>> +        scmi {
> >>> +            compatible = "arm,scmi";
> >>> +            mboxes = <&mhu_U_tx 0 0>, <&mhu_U_rx 0 0>;
> >>> +            shmem = <&cpu_scp_lpri0>;
> >>> +
> >>> +            #address-cells = <1>;
> >>> +            #size-cells = <0>;
> >>
> >> I don't think adding one more example with difference in only one piece
> >> is needed here.
> >>
> > 
> > Mmm, I thought was sensible to add this example, given that a mailbox
> > transport configuration for a mailbox exposing unidrectional channels is
> > quite different from the usual bidirectional channel config already
> > present in the pre-existent example.
> > 
> > I'll add mbox-names into this example and see if I can change your
> > mind...or I can then finally drop it.
> 
> And what exactly this one more example changes? Does not validate
> different parts of the binding if only one property differs...

Well it showcases how the extended new mboxes/shmem prop can be used in
to support such unidirectional channels (which is pretty much different
from the usual existing biridrectional synatx) ... anyway I never really
thought as the examples in terms of validation really (and I am not saying
that this is right eh) ... but more as an aid to help the unfortunate
human being that has finally to write some DT based on this.

Anyway since it does not seem appropriate, I'll just drop the whole
example in V3, after waiting for some more (if any) feedback on the
binding in general. Are the mbox-names fixes I added in V2 fine ?

Thanks,
Cristian

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: Support mailboxes unidirectional channels
@ 2023-03-28  7:26           ` Cristian Marussi
  0 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-03-28  7:26 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-kernel, linux-arm-kernel, devicetree, sudeep.holla,
	vincent.guittot, souvik.chakravarty, nicola.mazzucato, robh+dt,
	krzysztof.kozlowski+dt

On Tue, Mar 28, 2023 at 08:36:51AM +0200, Krzysztof Kozlowski wrote:
> On 27/03/2023 17:27, Cristian Marussi wrote:
> >>> +  - |
> >>> +    firmware {
> >>> +        scmi {
> >>> +            compatible = "arm,scmi";
> >>> +            mboxes = <&mhu_U_tx 0 0>, <&mhu_U_rx 0 0>;
> >>> +            shmem = <&cpu_scp_lpri0>;
> >>> +
> >>> +            #address-cells = <1>;
> >>> +            #size-cells = <0>;
> >>
> >> I don't think adding one more example with difference in only one piece
> >> is needed here.
> >>
> > 
> > Mmm, I thought was sensible to add this example, given that a mailbox
> > transport configuration for a mailbox exposing unidrectional channels is
> > quite different from the usual bidirectional channel config already
> > present in the pre-existent example.
> > 
> > I'll add mbox-names into this example and see if I can change your
> > mind...or I can then finally drop it.
> 
> And what exactly this one more example changes? Does not validate
> different parts of the binding if only one property differs...

Well it showcases how the extended new mboxes/shmem prop can be used in
to support such unidirectional channels (which is pretty much different
from the usual existing biridrectional synatx) ... anyway I never really
thought as the examples in terms of validation really (and I am not saying
that this is right eh) ... but more as an aid to help the unfortunate
human being that has finally to write some DT based on this.

Anyway since it does not seem appropriate, I'll just drop the whole
example in V3, after waiting for some more (if any) feedback on the
binding in general. Are the mbox-names fixes I added in V2 fine ?

Thanks,
Cristian

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2023-03-28  7:28 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-27 14:03 [PATCH 0/2] Add SCMI support for mailbox unidirectional channels Cristian Marussi
2023-03-27 14:03 ` Cristian Marussi
2023-03-27 14:03 ` [PATCH 1/2] dt-bindings: firmware: arm,scmi: Support mailboxes " Cristian Marussi
2023-03-27 14:03   ` Cristian Marussi
2023-03-27 14:42   ` Krzysztof Kozlowski
2023-03-27 14:42     ` Krzysztof Kozlowski
2023-03-27 15:27     ` Cristian Marussi
2023-03-27 15:27       ` Cristian Marussi
2023-03-28  6:36       ` Krzysztof Kozlowski
2023-03-28  6:36         ` Krzysztof Kozlowski
2023-03-28  7:26         ` Cristian Marussi
2023-03-28  7:26           ` Cristian Marussi
2023-03-27 14:03 ` [PATCH 2/2] firmware: arm_scmi: Add support for unidirectional mailbox channels Cristian Marussi
2023-03-27 14:03   ` Cristian Marussi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.