[PATCH v4 0/2] mailbox: Add Broadcom STB mailbox driver for SCMI

[PATCH v4 0/2] mailbox: Add Broadcom STB mailbox driver for SCMI

[Jim Quinlan]

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

Patchset Summary:
  Adds a simple mailbox driver for the use of the ARM SCMI drivers.

v4:
  Commit "mailbox: Add Broadcom STB mailbox driver"
  -- Fixed indentation on Kconfig file (again, RandyD).
  -- Removed superfluous #ifdefs for ARM/ARM64 (RandyD).
  -- Fixed Copyright year on source file (RandyD).

v3:
  Commit "mailbox: Add Broadcom STB mailbox driver"
  -- Fixed indentation on Kconfig file (RandyD).

v2:
  Commit "mailbox: Add Broadcom STB mailbox driver"
  -- Remove the Kconfig dependency on SMP (Florian)
  Commit "mailbox: Add Broadcom STB mailbox driver"
  -- Drop label,unit address; changed title,description (RobH)

v1:
  -- Original submission.

Jim Quinlan (2):
  dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver
  mailbox: Add Broadcom STB mailbox driver

 .../bindings/mailbox/brcm,brcmstb-mbox.yaml   |  39 ++++
 drivers/mailbox/Kconfig                       |  12 ++
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/brcmstb-mailbox.c             | 167 ++++++++++++++++++
 4 files changed, 220 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
 create mode 100644 drivers/mailbox/brcmstb-mailbox.c

-- 
2.17.1


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

[PATCH v4 1/2] dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver

[Jim Quinlan]

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

Bindings are added.  Only one interrupt is needed because
we do not yet employ the SCMI p2a channel.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 .../bindings/mailbox/brcm,brcmstb-mbox.yaml   | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml

diff --git a/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
new file mode 100644
index 000000000000..797c0cc609a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+$id: http://devicetree.org/schemas/mailbox/brcm,brcmstb-mbox.yaml#
+
+title: Broadcom STB mailbox driver bindings
+
+maintainers:
+  - Jim Quinlan <james.quinlan@broadcom.com>
+
+properties:
+  compatible:
+    enum:
+      - brcm,brcmstb-mbox
+
+  interrupts:
+    items:
+      - description: a2p return interrupt, indicates SCMI msg completion.
+
+  "#mbox-cells":
+    const: 1
+
+required:
+  - compatible
+  - interrupts
+  - "#mbox-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    mailbox {
+      compatible = "brcm,brcmstb-mailbox";
+      #mbox-cells = <1>;
+      interrupts = <GIC_SPI 0xc6 IRQ_TYPE_LEVEL_HIGH>;
+    };
+...
-- 
2.17.1


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

[PATCH v4 2/2] mailbox: Add Broadcom STB mailbox driver

[Jim Quinlan]

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

This is a simple mailbox driver to be used by the SCMI protocol stack.  It
only implements the agent-to-platform channel; we may implement the
platform-to-agent channel in the future.  An unusual aspect of this driver
is how the completion of an SCMI message is indicated.  An SCMI message is
initiated with an ARM SMC call, but the return of this call does not
indicate the execution or completion of the message.  Rather, the message's
completion is signaled by an interrupt.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/mailbox/Kconfig           |  12 +++
 drivers/mailbox/Makefile          |   2 +
 drivers/mailbox/brcmstb-mailbox.c | 167 ++++++++++++++++++++++++++++++
 3 files changed, 181 insertions(+)
 create mode 100644 drivers/mailbox/brcmstb-mailbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 05b1009e2820..b1e697e8d5f9 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -254,4 +254,16 @@ config QCOM_IPCC
 	  acts as an interrupt controller for receiving interrupts from clients.
 	  Say Y here if you want to build this driver.
 
+config BRCMSTB_MBOX
+	tristate "Broadcom STB Mailbox"
+	depends on ARM64 || ARM
+	depends on ARM_SCMI_PROTOCOL && ARCH_BRCMSTB
+	default ARM_SCMI_PROTOCOL && ARCH_BRCMSTB
+	help
+	  Mailbox implementation of the Broadcom STB for the sole purposes
+	  of SCMI communication.  This is used by the SCMI drivers to
+	  communicate with FW that runs in EL3.  This mailbox only implements
+	  the agent-to-platform channgel of SCMI but may be augmented in
+	  the future to add the platform-to-agent channel.
+
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 2e06e02b2e03..95fbcd121f2e 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -54,3 +54,5 @@ obj-$(CONFIG_SUN6I_MSGBOX)	+= sun6i-msgbox.o
 obj-$(CONFIG_SPRD_MBOX)		+= sprd-mailbox.o
 
 obj-$(CONFIG_QCOM_IPCC)		+= qcom-ipcc.o
+
+obj-$(CONFIG_BRCMSTB_MBOX)	+= brcmstb-mailbox.o
diff --git a/drivers/mailbox/brcmstb-mailbox.c b/drivers/mailbox/brcmstb-mailbox.c
new file mode 100644
index 000000000000..d9e953995417
--- /dev/null
+++ b/drivers/mailbox/brcmstb-mailbox.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2017-2020, Broadcom */
+
+#include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/arm-smccc.h>
+
+#define BRCM_SCMI_SMC_OEM_FUNC	0x400
+#define BRCM_SCMI_MBOX_NUM	0
+#define BRCM_FID(ch) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+			IS_ENABLED(CONFIG_ARM64), \
+			ARM_SMCCC_OWNER_OEM, \
+			BRCM_SCMI_SMC_OEM_FUNC + (ch))
+enum {
+	A2P_CHAN = 0,
+	NUM_CHAN
+};
+
+struct chan_priv {
+	unsigned int mbox_num;
+	unsigned int ch;
+};
+
+struct brcm_mbox {
+	struct mbox_controller controller;
+	int irqs[NUM_CHAN];
+};
+
+static struct mbox_chan *brcm_mbox_of_xlate(struct mbox_controller *controller,
+					    const struct of_phandle_args *sp)
+{
+	unsigned int ch = sp->args[0];
+	struct brcm_mbox *mbox
+		= container_of(controller, struct brcm_mbox, controller);
+
+	if (!mbox || ch >= NUM_CHAN)
+		return ERR_PTR(-ENOENT);
+
+	return &mbox->controller.chans[ch];
+}
+
+static int announce_msg(unsigned int mbox_num, unsigned int ch)
+{
+	struct arm_smccc_res res;
+
+	if (ch >= NUM_CHAN)
+		return -EIO;
+	arm_smccc_smc(BRCM_FID(ch), mbox_num, 0, 0, 0, 0, 0, 0, &res);
+	if (res.a0)
+		return -EIO;
+	return 0;
+}
+
+static int brcm_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct chan_priv *priv = chan->con_priv;
+
+	return announce_msg(priv->mbox_num, priv->ch);
+}
+
+static int brcm_mbox_startup(struct mbox_chan *chan)
+{
+	return 0;
+}
+
+static const struct mbox_chan_ops brcm_mbox_ops = {
+	.send_data = brcm_mbox_send_data,
+	.startup = brcm_mbox_startup,
+};
+
+static irqreturn_t brcm_a2p_isr(int irq, void *data)
+{
+	struct mbox_chan *chan = data;
+
+	mbox_chan_received_data(chan, NULL);
+	return IRQ_HANDLED;
+}
+
+static int brcm_mbox_probe(struct platform_device *pdev)
+{
+	struct brcm_mbox *mbox;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct chan_priv *chan_priv;
+	int ret;
+
+	if (!np)
+		return -EINVAL;
+
+	mbox = devm_kzalloc(&pdev->dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	/* Allocate channels */
+	mbox->controller.chans = devm_kzalloc(
+		&pdev->dev, NUM_CHAN * sizeof(struct mbox_chan), GFP_KERNEL);
+	if (!mbox->controller.chans)
+		return -ENOMEM;
+	chan_priv = devm_kzalloc(
+		&pdev->dev, NUM_CHAN * sizeof(struct chan_priv), GFP_KERNEL);
+	if (!chan_priv)
+		return -ENOMEM;
+
+	mbox->irqs[A2P_CHAN] = platform_get_irq(pdev, 0);
+	ret = devm_request_irq(&pdev->dev, mbox->irqs[A2P_CHAN], brcm_a2p_isr,
+				IRQF_NO_SUSPEND, "brcm: SCMI a2p intr",
+				&mbox->controller.chans[A2P_CHAN]);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to setup SCMI a2p isr\n");
+		return ret;
+	}
+	chan_priv[A2P_CHAN].mbox_num = BRCM_SCMI_MBOX_NUM;
+	chan_priv[A2P_CHAN].ch = A2P_CHAN;
+	mbox->controller.chans[A2P_CHAN].con_priv = &chan_priv[A2P_CHAN];
+	mbox->controller.num_chans++;
+	mbox->controller.dev = &pdev->dev;
+	mbox->controller.ops = &brcm_mbox_ops;
+	mbox->controller.of_xlate = brcm_mbox_of_xlate;
+	ret = mbox_controller_register(&mbox->controller);
+	if (ret) {
+		dev_err(dev, "failed to register BrcmSTB mbox\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, mbox);
+	return 0;
+}
+
+static int brcm_mbox_remove(struct platform_device *pdev)
+{
+	struct brcm_mbox *mbox = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&mbox->controller);
+
+	return 0;
+}
+
+static const struct of_device_id brcm_mbox_of_match[] = {
+	{ .compatible = "brcm,brcmstb-mbox", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, brcm_mbox_of_match);
+
+static struct platform_driver brcm_mbox_driver = {
+	.probe = brcm_mbox_probe,
+	.remove = brcm_mbox_remove,
+	.driver = {
+		.name = "brcm_mbox",
+		.of_match_table = brcm_mbox_of_match,
+	},
+};
+
+module_platform_driver(brcm_mbox_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Broadcom STB SCMI Mailbox driver");
+MODULE_AUTHOR("Broadcom");
-- 
2.17.1


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

Re: [PATCH v4 1/2] dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver

[Rob Herring]

On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote:
> Bindings are added.  Only one interrupt is needed because
> we do not yet employ the SCMI p2a channel.

I still don't understand what this is. To repeat from v1: I thought SCMI 
was a mailbox consumer, not provider?

> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  .../bindings/mailbox/brcm,brcmstb-mbox.yaml   | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
> new file mode 100644
> index 000000000000..797c0cc609a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
> @@ -0,0 +1,39 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +$id: http://devicetree.org/schemas/mailbox/brcm,brcmstb-mbox.yaml#
> +
> +title: Broadcom STB mailbox driver bindings
> +
> +maintainers:
> +  - Jim Quinlan <james.quinlan@broadcom.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - brcm,brcmstb-mbox
> +
> +  interrupts:
> +    items:
> +      - description: a2p return interrupt, indicates SCMI msg completion.
> +
> +  "#mbox-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - interrupts
> +  - "#mbox-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    mailbox {
> +      compatible = "brcm,brcmstb-mailbox";
> +      #mbox-cells = <1>;
> +      interrupts = <GIC_SPI 0xc6 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +...
> -- 
> 2.17.1
> 

Re: [PATCH v4 1/2] dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver

[Jim Quinlan]

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

On Wed, Nov 4, 2020 at 4:50 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote:
> > Bindings are added.  Only one interrupt is needed because
> > we do not yet employ the SCMI p2a channel.
>
> I still don't understand what this is. To repeat from v1: I thought SCMI
> was a mailbox consumer, not provider?

Hi Rob,

I'm not sure where I am implying that SCMI is a mailbox provider?
Should I not mention "SCMI" in the subject line?

This is just a mailbox driver, "consumed" by SCMI.    Our SCMI DT node
looks like this:

brcm_scmi_mailbox: brcm_scmi_mailbox@0 {
        #mbox-cells = <1>;
        compatible = "brcm,brcmstb-mbox";
};

brcm_scmi@0 {
        compatible = "arm,scmi";
        mboxes = <&brcm_scmi_mailbox 0>;;
        mbox-names = "tx";
        shmem = <&NWMBOX>;
        /* ... */
};

Please advise,
Jim Quinlan
Broadcom STB

>
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  .../bindings/mailbox/brcm,brcmstb-mbox.yaml   | 39 +++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
> > new file mode 100644
> > index 000000000000..797c0cc609a3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml
> > @@ -0,0 +1,39 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +$id: http://devicetree.org/schemas/mailbox/brcm,brcmstb-mbox.yaml#
> > +
> > +title: Broadcom STB mailbox driver bindings
> > +
> > +maintainers:
> > +  - Jim Quinlan <james.quinlan@broadcom.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - brcm,brcmstb-mbox
> > +
> > +  interrupts:
> > +    items:
> > +      - description: a2p return interrupt, indicates SCMI msg completion.
> > +
> > +  "#mbox-cells":
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - interrupts
> > +  - "#mbox-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    mailbox {
> > +      compatible = "brcm,brcmstb-mailbox";
> > +      #mbox-cells = <1>;
> > +      interrupts = <GIC_SPI 0xc6 IRQ_TYPE_LEVEL_HIGH>;
> > +    };
> > +...
> > --
> > 2.17.1
> >
>
>

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

Re: [PATCH v4 1/2] dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver

[Rob Herring]

On Wed, Nov 4, 2020 at 4:04 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
>
> On Wed, Nov 4, 2020 at 4:50 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote:
> > > Bindings are added.  Only one interrupt is needed because
> > > we do not yet employ the SCMI p2a channel.
> >
> > I still don't understand what this is. To repeat from v1: I thought SCMI
> > was a mailbox consumer, not provider?
>
> Hi Rob,
>
> I'm not sure where I am implying that SCMI is a mailbox provider?
> Should I not mention "SCMI" in the subject line?
>
> This is just a mailbox driver, "consumed" by SCMI.    Our SCMI DT node
> looks like this:
>
> brcm_scmi_mailbox: brcm_scmi_mailbox@0 {
>         #mbox-cells = <1>;
>         compatible = "brcm,brcmstb-mbox";
> };
>
> brcm_scmi@0 {
>         compatible = "arm,scmi";
>         mboxes = <&brcm_scmi_mailbox 0>;;
>         mbox-names = "tx";
>         shmem = <&NWMBOX>;
>         /* ... */
> };

Okay, that makes more sense. Though it seems like this is just adding
a pointless level of indirection to turn an interrupt into a mailbox.
There's nothing more to 'the mailbox' is there? So why not either
allow SCMI to have an interrupt directly or have a generic irq mailbox
driver?

Rob

Re: [PATCH v4 1/2] dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver

[Jim Quinlan]

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

On Thu, Nov 5, 2020 at 10:13 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Nov 4, 2020 at 4:04 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> >
> > On Wed, Nov 4, 2020 at 4:50 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote:
> > > > Bindings are added.  Only one interrupt is needed because
> > > > we do not yet employ the SCMI p2a channel.
> > >
> > > I still don't understand what this is. To repeat from v1: I thought SCMI
> > > was a mailbox consumer, not provider?
> >
> > Hi Rob,
> >
> > I'm not sure where I am implying that SCMI is a mailbox provider?
> > Should I not mention "SCMI" in the subject line?
> >
> > This is just a mailbox driver, "consumed" by SCMI.    Our SCMI DT node
> > looks like this:
> >
> > brcm_scmi_mailbox: brcm_scmi_mailbox@0 {
> >         #mbox-cells = <1>;
> >         compatible = "brcm,brcmstb-mbox";
> > };
> >
> > brcm_scmi@0 {
> >         compatible = "arm,scmi";
> >         mboxes = <&brcm_scmi_mailbox 0>;;
> >         mbox-names = "tx";
> >         shmem = <&NWMBOX>;
> >         /* ... */
> > };
>
> Okay, that makes more sense. Though it seems like this is just adding
> a pointless level of indirection to turn an interrupt into a mailbox.
> There's nothing more to 'the mailbox' is there?
Correct.  Although you can see that it uses both interrupts and SMC
calls to get the job done.

> So why not either
> allow SCMI to have an interrupt directly
Not sure here -- perhaps the SCMI folks have an answer?

 > or have a generic irq mailbox
> driver?
The SCMI implementation doesn't offer a generic irq mailbox driver
AFAICT.  The SCMI folks recently provided  an "smc transport" driver
in "drivers/firmware/arm_scmi/smc.c" -- it is close to what we need
but is missing interrupts.

Regards,
Jim Quinlan
Broadcom STB

>
> Rob

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

Re: [PATCH v4 1/2] dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver

[Sudeep Holla]

On Thu, Nov 05, 2020 at 10:28:25AM -0500, Jim Quinlan wrote:
> On Thu, Nov 5, 2020 at 10:13 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Wed, Nov 4, 2020 at 4:04 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> > >
> > > On Wed, Nov 4, 2020 at 4:50 PM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote:
> > > > > Bindings are added.  Only one interrupt is needed because
> > > > > we do not yet employ the SCMI p2a channel.
> > > >
> > > > I still don't understand what this is. To repeat from v1: I thought SCMI
> > > > was a mailbox consumer, not provider?
> > >
> > > Hi Rob,
> > >
> > > I'm not sure where I am implying that SCMI is a mailbox provider?
> > > Should I not mention "SCMI" in the subject line?
> > >
> > > This is just a mailbox driver, "consumed" by SCMI.    Our SCMI DT node
> > > looks like this:
> > >
> > > brcm_scmi_mailbox: brcm_scmi_mailbox@0 {
> > >         #mbox-cells = <1>;
> > >         compatible = "brcm,brcmstb-mbox";
> > > };
> > >
> > > brcm_scmi@0 {
> > >         compatible = "arm,scmi";
> > >         mboxes = <&brcm_scmi_mailbox 0>;;
> > >         mbox-names = "tx";
> > >         shmem = <&NWMBOX>;
> > >         /* ... */
> > > };
> >
> > Okay, that makes more sense. Though it seems like this is just adding
> > a pointless level of indirection to turn an interrupt into a mailbox.
> > There's nothing more to 'the mailbox' is there?
>
> Correct.  Although you can see that it uses both interrupts and SMC
> calls to get the job done.
>

I was against having 2 separate solutions and would have raised my concern
again. As I mentioned earlier, either extend what we have or move the
existing SMC solution into this mailbox driver. Having 2 different solution
for this just because you have extra interrupt to deal with is definite
NACK from me as I had previously mentioned.

> > So why not either
> > allow SCMI to have an interrupt directly
> Not sure here -- perhaps the SCMI folks have an answer?
>

I did ask why can't you extend the existing SCMI/SMC binding to add this
as optional feature ?

> > or have a generic irq mailbox driver?

Fine with this too.

> The SCMI implementation doesn't offer a generic irq mailbox driver
> AFAICT.  The SCMI folks recently provided  an "smc transport" driver
> in "drivers/firmware/arm_scmi/smc.c" -- it is close to what we need
> but is missing interrupts.

IIRC, you were using SGIs and it can't be represented and use today as
is ? Am  I missing something or anything has changed ?

--
Regards,
Sudeep

Re: [PATCH v4 1/2] dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver

[Jim Quinlan]

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

On Thu, Nov 5, 2020 at 1:27 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Nov 05, 2020 at 10:28:25AM -0500, Jim Quinlan wrote:
> > On Thu, Nov 5, 2020 at 10:13 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Wed, Nov 4, 2020 at 4:04 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> > > >
> > > > On Wed, Nov 4, 2020 at 4:50 PM Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote:
> > > > > > Bindings are added.  Only one interrupt is needed because
> > > > > > we do not yet employ the SCMI p2a channel.
> > > > >
> > > > > I still don't understand what this is. To repeat from v1: I thought SCMI
> > > > > was a mailbox consumer, not provider?
> > > >
> > > > Hi Rob,
> > > >
> > > > I'm not sure where I am implying that SCMI is a mailbox provider?
> > > > Should I not mention "SCMI" in the subject line?
> > > >
> > > > This is just a mailbox driver, "consumed" by SCMI.    Our SCMI DT node
> > > > looks like this:
> > > >
> > > > brcm_scmi_mailbox: brcm_scmi_mailbox@0 {
> > > >         #mbox-cells = <1>;
> > > >         compatible = "brcm,brcmstb-mbox";
> > > > };
> > > >
> > > > brcm_scmi@0 {
> > > >         compatible = "arm,scmi";
> > > >         mboxes = <&brcm_scmi_mailbox 0>;;
> > > >         mbox-names = "tx";
> > > >         shmem = <&NWMBOX>;
> > > >         /* ... */
> > > > };
> > >
> > > Okay, that makes more sense. Though it seems like this is just adding
> > > a pointless level of indirection to turn an interrupt into a mailbox.
> > > There's nothing more to 'the mailbox' is there?
> >
> > Correct.  Although you can see that it uses both interrupts and SMC
> > calls to get the job done.
> >
>
> I was against having 2 separate solutions and would have raised my concern
> again. As I mentioned earlier, either extend what we have or move the
> existing SMC solution into this mailbox driver. Having 2 different solution
> for this just because you have extra interrupt to deal with is definite
> NACK from me as I had previously mentioned.
>
> > > So why not either
> > > allow SCMI to have an interrupt directly
> > Not sure here -- perhaps the SCMI folks have an answer?
> >
>
> I did ask why can't you extend the existing SCMI/SMC binding to add this
> as optional feature ?
Hi Sudeep,

Looking at the email you said, "In that case any reason why you can't
reuse the existing smc transport for SCMI." ,  and I replied with the
reason.  I did not interpret your statement  above as what you are
clearly saying now: "either extend what we have or move the existing
SMC solution into this mailbox driver. "

Fair enough, I  will look into this.

Regards,
Jim


>
> > > or have a generic irq mailbox driver?
>
> Fine with this too.
>
> > The SCMI implementation doesn't offer a generic irq mailbox driver
> > AFAICT.  The SCMI folks recently provided  an "smc transport" driver
> > in "drivers/firmware/arm_scmi/smc.c" -- it is close to what we need
> > but is missing interrupts.
>
> IIRC, you were using SGIs and it can't be represented and use today as
> is ? Am  I missing something or anything has changed ?
>
> --
> Regards,
> Sudeep

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

Re: [PATCH v4 1/2] dt-bindings: Add bindings for BrcmSTB SCMI mailbox driver

[Sudeep Holla]

On Thu, Nov 05, 2020 at 01:57:07PM -0500, Jim Quinlan wrote:
> On Thu, Nov 5, 2020 at 1:27 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Nov 05, 2020 at 10:28:25AM -0500, Jim Quinlan wrote:
> > > On Thu, Nov 5, 2020 at 10:13 AM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Wed, Nov 4, 2020 at 4:04 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> > > > >
> > > > > On Wed, Nov 4, 2020 at 4:50 PM Rob Herring <robh@kernel.org> wrote:
> > > > > >
> > > > > > On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote:
> > > > > > > Bindings are added.  Only one interrupt is needed because
> > > > > > > we do not yet employ the SCMI p2a channel.
> > > > > >
> > > > > > I still don't understand what this is. To repeat from v1: I thought SCMI
> > > > > > was a mailbox consumer, not provider?
> > > > >
> > > > > Hi Rob,
> > > > >
> > > > > I'm not sure where I am implying that SCMI is a mailbox provider?
> > > > > Should I not mention "SCMI" in the subject line?
> > > > >
> > > > > This is just a mailbox driver, "consumed" by SCMI.    Our SCMI DT node
> > > > > looks like this:
> > > > >
> > > > > brcm_scmi_mailbox: brcm_scmi_mailbox@0 {
> > > > >         #mbox-cells = <1>;
> > > > >         compatible = "brcm,brcmstb-mbox";
> > > > > };
> > > > >
> > > > > brcm_scmi@0 {
> > > > >         compatible = "arm,scmi";
> > > > >         mboxes = <&brcm_scmi_mailbox 0>;;
> > > > >         mbox-names = "tx";
> > > > >         shmem = <&NWMBOX>;
> > > > >         /* ... */
> > > > > };
> > > >
> > > > Okay, that makes more sense. Though it seems like this is just adding
> > > > a pointless level of indirection to turn an interrupt into a mailbox.
> > > > There's nothing more to 'the mailbox' is there?
> > >
> > > Correct.  Although you can see that it uses both interrupts and SMC
> > > calls to get the job done.
> > >
> >
> > I was against having 2 separate solutions and would have raised my concern
> > again. As I mentioned earlier, either extend what we have or move the
> > existing SMC solution into this mailbox driver. Having 2 different solution
> > for this just because you have extra interrupt to deal with is definite
> > NACK from me as I had previously mentioned.
> >
> > > > So why not either
> > > > allow SCMI to have an interrupt directly
> > > Not sure here -- perhaps the SCMI folks have an answer?
> > >
> >
> > I did ask why can't you extend the existing SCMI/SMC binding to add this
> > as optional feature ?
> Hi Sudeep,
> 
> Looking at the email you said, "In that case any reason why you can't
> reuse the existing smc transport for SCMI." ,  and I replied with the
> reason.  I did not interpret your statement  above as what you are
> clearly saying now: "either extend what we have or move the existing
> SMC solution into this mailbox driver. "
>

No, you are right. I didn't mention that explicitly. I wanted to, but
thought I will wait until this driver got traction to ask you to merge
them. Sorry for that. Anyways I am against having existing solution and
a mailbox for SMC, they need to be merged at any cost. Where the final
solution will be doesn't matter much to me, I am fine either way.

--
Regards,
Sudeep