[RFC PATCH v2 0/9] scpi: Add SCPI registry to handle legacy protocol

[RFC PATCH v2 0/9] scpi: Add SCPI registry to handle legacy protocol

[Neil Armstrong]

This patchset aims to support the legacy SCPI firmware implementation that was
delivered as early technology preview for the JUNO platform.

Finally a stable, maintained and public implementation for the SCPI protocol
has been upstreamed part of the JUNO support and it is the recommended way
of implementing SCP communication on ARMv8 platforms.

The Amlogic GXBB platform is using this legacy protocol, as the RK3368 & RK3399
platforms. Only the GXBB example is provided here, but it's unclear if other
Amlogic ARMv8 based SoCs uses this legacy procotol.

In order to support the legacy protocol :
 - Move the scpi_get_ops to a thin registry layer
 - Change the arm_scpi.c to use the registry layer
 - Add a separate config option to build the registry layer
 - Add the legacy SCPI driver based on the new implementation
 - For example, add the Amlogic GXBB MHU and SCPI DT cpufreq & sensors nodes

Initial RFC discution tread can be found at https://lkml.org/lkml/2016/5/26/111

Neil Armstrong (9):
  mailbox: Add Amlogic Meson Message-Handling-Unit
  dt-bindings: mailbox: Add Amlogic Meson MHU Bindings
  ARM64: dts: meson-gxbb: Add Meson MHU Node
  firmware: Add a SCPI registry to handle multiple implementations
  firmware: scpi: Switch arm_scpi to use new registry
  firmware: Add legacy SCPI protocol driver
  dt-bindings: arm: Update arm,scpi bindings with Meson GXBB SCPI
  ARM64: dts: meson-gxbb: Add SRAM node
  ARM64: dts: meson-gxbb: Add SCPI with cpufreq & sensors Nodes

 Documentation/devicetree/bindings/arm/arm,scpi.txt |   8 +-
 .../devicetree/bindings/mailbox/meson-mhu.txt      |  33 ++
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |  53 ++
 drivers/firmware/Kconfig                           |  24 +
 drivers/firmware/Makefile                          |   2 +
 drivers/firmware/arm_scpi.c                        |  14 +-
 drivers/firmware/legacy_scpi.c                     | 644 +++++++++++++++++++++
 drivers/firmware/scpi.c                            |  94 +++
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/meson_mhu.c                        | 199 +++++++
 include/linux/scpi_protocol.h                      |  15 +-
 11 files changed, 1075 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/meson-mhu.txt
 create mode 100644 drivers/firmware/legacy_scpi.c
 create mode 100644 drivers/firmware/scpi.c
 create mode 100644 drivers/mailbox/meson_mhu.c

-- 
2.7.0

[RFC PATCH v2 1/9] mailbox: Add Amlogic Meson Message-Handling-Unit

[Neil Armstrong]

Add Amlogic Meson SoCs Message-Handling-Unit as mailbox controller
with 2 independent channels/links to communicate with a remote processor.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/mailbox/Makefile    |   2 +
 drivers/mailbox/meson_mhu.c | 199 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 201 insertions(+)
 create mode 100644 drivers/mailbox/meson_mhu.c

diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 0be3e74..6aa9dbe 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -25,3 +25,5 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
 obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
 
 obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
+
+obj-$(CONFIG_ARCH_MESON)	+= meson_mhu.o
diff --git a/drivers/mailbox/meson_mhu.c b/drivers/mailbox/meson_mhu.c
new file mode 100644
index 0000000..0576b92
--- /dev/null
+++ b/drivers/mailbox/meson_mhu.c
@@ -0,0 +1,199 @@
+/*
+ * Copyright (C) 2016 BayLibre SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ * Heavily based on meson_mhu.c from :
+ * Copyright (C) 2013-2015 Fujitsu Semiconductor Ltd.
+ * Copyright (C) 2015 Linaro Ltd.
+ * Author: Jassi Brar <jaswinder.singh@linaro.org>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/spinlock.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/mailbox_controller.h>
+
+#define INTR_SET_OFS	0x0
+#define INTR_STAT_OFS	0x4
+#define INTR_CLR_OFS	0x8
+
+#define MHU_LP_OFFSET	0x10
+#define MHU_HP_OFFSET	0x1c
+
+#define TX_REG_OFFSET	0x24
+
+#define MHU_CHANS	2
+
+struct meson_mhu_link {
+	unsigned int irq;
+	void __iomem *tx_reg;
+	void __iomem *rx_reg;
+};
+
+struct meson_mhu {
+	void __iomem *base;
+	struct meson_mhu_link mlink[MHU_CHANS];
+	struct mbox_chan chan[MHU_CHANS];
+	struct mbox_controller mbox;
+};
+
+static irqreturn_t meson_mhu_rx_interrupt(int irq, void *p)
+{
+	struct mbox_chan *chan = p;
+	struct meson_mhu_link *mlink = chan->con_priv;
+	u32 val;
+
+	val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS);
+	if (!val)
+		return IRQ_NONE;
+
+	mbox_chan_received_data(chan, (void *)&val);
+
+	writel_relaxed(~0, mlink->rx_reg + INTR_CLR_OFS);
+
+	return IRQ_HANDLED;
+}
+
+static bool meson_mhu_last_tx_done(struct mbox_chan *chan)
+{
+	struct meson_mhu_link *mlink = chan->con_priv;
+	u32 val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
+
+	return (val == 0);
+}
+
+static int meson_mhu_send_data(struct mbox_chan *chan, void *data)
+{
+	struct meson_mhu_link *mlink = chan->con_priv;
+	u32 *arg = data;
+
+	writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS);
+
+	return 0;
+}
+
+static int meson_mhu_startup(struct mbox_chan *chan)
+{
+	struct meson_mhu_link *mlink = chan->con_priv;
+	int ret;
+
+	ret = request_irq(mlink->irq, meson_mhu_rx_interrupt,
+			  IRQF_ONESHOT, "meson_mhu_link", chan);
+	if (ret) {
+		dev_err(chan->mbox->dev,
+			"Unable to acquire IRQ %d\n", mlink->irq);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void meson_mhu_shutdown(struct mbox_chan *chan)
+{
+	struct meson_mhu_link *mlink = chan->con_priv;
+
+	free_irq(mlink->irq, chan);
+}
+
+static const struct mbox_chan_ops meson_mhu_ops = {
+	.send_data = meson_mhu_send_data,
+	.startup = meson_mhu_startup,
+	.shutdown = meson_mhu_shutdown,
+	.last_tx_done = meson_mhu_last_tx_done,
+};
+
+static int meson_mhu_probe(struct platform_device *pdev)
+{
+	int i, err;
+	struct meson_mhu *mhu;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	int meson_mhu_reg[MHU_CHANS] = {MHU_LP_OFFSET, MHU_HP_OFFSET};
+
+	/* Allocate memory for device */
+	mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
+	if (!mhu)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mhu->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(mhu->base)) {
+		dev_err(dev, "ioremap failed\n");
+		return PTR_ERR(mhu->base);
+	}
+
+	for (i = 0; i < MHU_CHANS; i++) {
+		mhu->chan[i].con_priv = &mhu->mlink[i];
+		mhu->mlink[i].irq = platform_get_irq(pdev, i);
+		if (mhu->mlink[i].irq < 0) {
+			dev_err(dev, "failed to get irq%d\n", i);
+			return mhu->mlink[i].irq;
+		}
+		mhu->mlink[i].rx_reg = mhu->base + meson_mhu_reg[i];
+		mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
+	}
+
+	mhu->mbox.dev = dev;
+	mhu->mbox.chans = &mhu->chan[0];
+	mhu->mbox.num_chans = MHU_CHANS;
+	mhu->mbox.ops = &meson_mhu_ops;
+	mhu->mbox.txdone_irq = false;
+	mhu->mbox.txdone_poll = true;
+	mhu->mbox.txpoll_period = 1;
+
+	platform_set_drvdata(pdev, mhu);
+
+	err = mbox_controller_register(&mhu->mbox);
+	if (err) {
+		dev_err(dev, "Failed to register mailboxes %d\n", err);
+		return err;
+	}
+
+	dev_info(dev, "Meson MHU Mailbox registered\n");
+	return 0;
+}
+
+static int meson_mhu_remove(struct platform_device *pdev)
+{
+	struct meson_mhu *mhu = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&mhu->mbox);
+
+	return 0;
+}
+
+static const struct of_device_id meson_mhu_dt_ids[] = {
+	{ .compatible = "amlogic,meson-gxbb-mhu", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, meson_mhu_ids);
+
+static struct platform_driver meson_wdt_driver = {
+	.probe	= meson_mhu_probe,
+	.remove	= meson_mhu_remove,
+	.driver = {
+		.name = "meson-mhu",
+		.of_match_table	= meson_mhu_dt_ids,
+	},
+};
+
+module_platform_driver(meson_wdt_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:meson-mhu");
+MODULE_DESCRIPTION("Amlogic Meson MHU Driver");
+MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
-- 
2.7.0

Re: [RFC PATCH v2 1/9] mailbox: Add Amlogic Meson Message-Handling-Unit

[Jassi Brar]

On Tue, Jun 21, 2016 at 3:32 PM, Neil Armstrong <narmstrong@baylibre.com> wrote:
> Add Amlogic Meson SoCs Message-Handling-Unit as mailbox controller
> with 2 independent channels/links to communicate with a remote processor.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

.....

> +++ b/drivers/mailbox/meson_mhu.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2016 BayLibre SAS.
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + * Heavily based on meson_mhu.c from :
> + * Copyright (C) 2013-2015 Fujitsu Semiconductor Ltd.
> + * Copyright (C) 2015 Linaro Ltd.
> + * Author: Jassi Brar <jaswinder.singh@linaro.org>

.........
> +
> +#define INTR_SET_OFS   0x0
> +#define INTR_STAT_OFS  0x4
> +#define INTR_CLR_OFS   0x8
> +
> +#define MHU_LP_OFFSET  0x10
> +#define MHU_HP_OFFSET  0x1c
> +
> +#define TX_REG_OFFSET  0x24
> +
It seems only some register offsets have changed from arm_mhu. So
maybe just adapt the arm_mhu driver to look for IP variant and assign
corresponding offsets to set,stat,clr registers.

Cheers.

Re: [RFC PATCH v2 1/9] mailbox: Add Amlogic Meson Message-Handling-Unit

[Neil Armstrong]

On 06/22/2016 06:31 AM, Jassi Brar wrote:
> On Tue, Jun 21, 2016 at 3:32 PM, Neil Armstrong <narmstrong@baylibre.com> wrote:
>> Add Amlogic Meson SoCs Message-Handling-Unit as mailbox controller
>> with 2 independent channels/links to communicate with a remote processor.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> .....
> 
>> +++ b/drivers/mailbox/meson_mhu.c
>> @@ -0,0 +1,199 @@
>> +/*
>> + * Copyright (C) 2016 BayLibre SAS.
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + * Heavily based on meson_mhu.c from :
>> + * Copyright (C) 2013-2015 Fujitsu Semiconductor Ltd.
>> + * Copyright (C) 2015 Linaro Ltd.
>> + * Author: Jassi Brar <jaswinder.singh@linaro.org>
> 
> .........
>> +
>> +#define INTR_SET_OFS   0x0
>> +#define INTR_STAT_OFS  0x4
>> +#define INTR_CLR_OFS   0x8
>> +
>> +#define MHU_LP_OFFSET  0x10
>> +#define MHU_HP_OFFSET  0x1c
>> +
>> +#define TX_REG_OFFSET  0x24
>> +
> It seems only some register offsets have changed from arm_mhu. So
> maybe just adapt the arm_mhu driver to look for IP variant and assign
> corresponding offsets to set,stat,clr registers.

Hi Jassi,

It's a good idea, but adding the platform_driver support along the amba_bus probe will add a lot a code.
The meson_mhu is a very simple and short driver, I think it'll be simpler to maintain beeing separated.
And I'm not certain on how it's close of ARM's real IP.

Neil

> Cheers.
> 

Re: [RFC PATCH v2 1/9] mailbox: Add Amlogic Meson Message-Handling-Unit

[Neil Armstrong]

On 06/22/2016 06:31 AM, Jassi Brar wrote:
> On Tue, Jun 21, 2016 at 3:32 PM, Neil Armstrong <narmstrong@baylibre.com> wrote:
>> Add Amlogic Meson SoCs Message-Handling-Unit as mailbox controller
>> with 2 independent channels/links to communicate with a remote processor.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> .....
> 
>> +++ b/drivers/mailbox/meson_mhu.c
>> @@ -0,0 +1,199 @@
>> +/*
>> + * Copyright (C) 2016 BayLibre SAS.
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + * Heavily based on meson_mhu.c from :
>> + * Copyright (C) 2013-2015 Fujitsu Semiconductor Ltd.
>> + * Copyright (C) 2015 Linaro Ltd.
>> + * Author: Jassi Brar <jaswinder.singh@linaro.org>
> 
> .........
>> +
>> +#define INTR_SET_OFS   0x0
>> +#define INTR_STAT_OFS  0x4
>> +#define INTR_CLR_OFS   0x8
>> +
>> +#define MHU_LP_OFFSET  0x10
>> +#define MHU_HP_OFFSET  0x1c
>> +
>> +#define TX_REG_OFFSET  0x24
>> +
> It seems only some register offsets have changed from arm_mhu. So
> maybe just adapt the arm_mhu driver to look for IP variant and assign
> corresponding offsets to set,stat,clr registers.
> 
> Cheers.
> 

Hi Jassi,

I did a quick & dirty merge with arm_mhu, and I don't it's a good idea...

Please have a look at attached patch.

Regards,
Neil
-------------------------------------------------------------------------------------------
From: Neil Armstrong <narmstrong@baylibre.com>
Subject: [PATCH] mailbox: arm_mhu: Add support for Amlogic Meson MHU

To achieve this integration, add support for generic probe from amba
or platform.
Move all register offsets to a data structure passed in either amba id or
platform dt id match table.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/mailbox/arm_mhu.c | 217 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 181 insertions(+), 36 deletions(-)

diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
index 99befa7..d7fb843 100644
--- a/drivers/mailbox/arm_mhu.c
+++ b/drivers/mailbox/arm_mhu.c
@@ -22,45 +22,68 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/amba/bus.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
 #include <linux/mailbox_controller.h>

-#define INTR_STAT_OFS	0x0
-#define INTR_SET_OFS	0x8
-#define INTR_CLR_OFS	0x10
+#define MHU_INTR_STAT_OFS	0x0
+#define MHU_INTR_SET_OFS	0x8
+#define MHU_INTR_CLR_OFS	0x10

 #define MHU_LP_OFFSET	0x0
 #define MHU_HP_OFFSET	0x20
 #define MHU_SEC_OFFSET	0x200
-#define TX_REG_OFFSET	0x100
+#define MHU_TX_REG_OFFSET	0x100

-#define MHU_CHANS	3
+#define MESON_INTR_SET_OFS	0x0
+#define MESON_INTR_STAT_OFS	0x4
+#define MESON_INTR_CLR_OFS	0x8
+
+#define MESON_MHU_LP_OFFSET	0x10
+#define MESON_MHU_HP_OFFSET	0x1c
+#define MESON_MHU_TX_OFFSET	0x24
+
+#define MAX_MHU_CHANS	3

 struct mhu_link {
 	unsigned irq;
-	void __iomem *tx_reg;
-	void __iomem *rx_reg;
+	void __iomem *tx_stat_reg;
+	void __iomem *tx_set_reg;
+	void __iomem *tx_clr_reg;
+	void __iomem *rx_stat_reg;
+	void __iomem *rx_set_reg;
+	void __iomem *rx_clr_reg;
 };

 struct arm_mhu {
 	void __iomem *base;
-	struct mhu_link mlink[MHU_CHANS];
-	struct mbox_chan chan[MHU_CHANS];
+	struct mhu_link mlink[MAX_MHU_CHANS];
+	struct mbox_chan chan[MAX_MHU_CHANS];
 	struct mbox_controller mbox;
 };

+struct arm_mhu_data {
+	unsigned int channels;
+	int rx_offsets[MAX_MHU_CHANS];
+	int tx_offsets[MAX_MHU_CHANS];
+	unsigned int intr_stat_offs;
+	unsigned int intr_set_offs;
+	unsigned int intr_clr_offs;
+};
+
 static irqreturn_t mhu_rx_interrupt(int irq, void *p)
 {
 	struct mbox_chan *chan = p;
 	struct mhu_link *mlink = chan->con_priv;
 	u32 val;

-	val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS);
+	val = readl_relaxed(mlink->rx_stat_reg);
 	if (!val)
 		return IRQ_NONE;

 	mbox_chan_received_data(chan, (void *)&val);

-	writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS);
+	writel_relaxed(val, mlink->rx_clr_reg);

 	return IRQ_HANDLED;
 }
@@ -68,7 +91,7 @@ static irqreturn_t mhu_rx_interrupt(int irq, void *p)
 static bool mhu_last_tx_done(struct mbox_chan *chan)
 {
 	struct mhu_link *mlink = chan->con_priv;
-	u32 val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
+	u32 val = readl_relaxed(mlink->tx_stat_reg);

 	return (val == 0);
 }
@@ -78,7 +101,7 @@ static int mhu_send_data(struct mbox_chan *chan, void *data)
 	struct mhu_link *mlink = chan->con_priv;
 	u32 *arg = data;

-	writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS);
+	writel_relaxed(*arg, mlink->tx_set_reg);

 	return 0;
 }
@@ -89,8 +112,8 @@ static int mhu_startup(struct mbox_chan *chan)
 	u32 val;
 	int ret;

-	val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
-	writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);
+	val = readl_relaxed(mlink->tx_stat_reg);
+	writel_relaxed(val, mlink->tx_clr_reg);

 	ret = request_irq(mlink->irq, mhu_rx_interrupt,
 			  IRQF_SHARED, "mhu_link", chan);
@@ -117,52 +140,155 @@ static const struct mbox_chan_ops mhu_ops = {
 	.last_tx_done = mhu_last_tx_done,
 };

-static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
+static struct arm_mhu_data arm_mhu_amba_data = {
+	.channels = 3,
+	.rx_offsets = {MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET},
+	.tx_offsets = {MHU_LP_OFFSET + MHU_TX_REG_OFFSET,
+		       MHU_HP_OFFSET + MHU_TX_REG_OFFSET,
+		       MHU_SEC_OFFSET + MHU_TX_REG_OFFSET},
+	.intr_stat_offs = MHU_INTR_STAT_OFS,
+	.intr_set_offs = MHU_INTR_SET_OFS,
+	.intr_clr_offs = MHU_INTR_CLR_OFS,
+};
+
+static const struct arm_mhu_data meson_mhu_data = {
+	.channels = 2,
+	.rx_offsets = {MESON_MHU_LP_OFFSET, MESON_MHU_HP_OFFSET},
+	.tx_offsets = {MESON_MHU_LP_OFFSET + MESON_MHU_TX_OFFSET,
+		       MESON_MHU_HP_OFFSET + MESON_MHU_TX_OFFSET},
+	.intr_stat_offs = MESON_INTR_STAT_OFS,
+	.intr_set_offs = MESON_INTR_SET_OFS,
+	.intr_clr_offs = MESON_INTR_CLR_OFS,
+};
+
+static struct arm_mhu *mhu_generic_probe(struct device *dev,
+					 struct resource *res,
+					 unsigned int irqs[],
+					 const struct arm_mhu_data *data)
 {
 	int i, err;
 	struct arm_mhu *mhu;
-	struct device *dev = &adev->dev;
-	int mhu_reg[MHU_CHANS] = {MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET};

 	/* Allocate memory for device */
 	mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
 	if (!mhu)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);

-	mhu->base = devm_ioremap_resource(dev, &adev->res);
+	mhu->base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(mhu->base)) {
 		dev_err(dev, "ioremap failed\n");
-		return PTR_ERR(mhu->base);
+		return ERR_CAST(mhu->base);
 	}

-	for (i = 0; i < MHU_CHANS; i++) {
+	for (i = 0; i < data->channels; i++) {
+		void __iomem *reg_base;
 		mhu->chan[i].con_priv = &mhu->mlink[i];
-		mhu->mlink[i].irq = adev->irq[i];
-		mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
-		mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
+		mhu->mlink[i].irq = irqs[i];
+		reg_base = mhu->base + data->rx_offsets[i];
+		mhu->mlink[i].rx_stat_reg = reg_base + data->intr_stat_offs;
+		mhu->mlink[i].rx_set_reg = reg_base + data->intr_set_offs;
+		mhu->mlink[i].rx_clr_reg = reg_base + data->intr_clr_offs;
+		reg_base = mhu->base + data->tx_offsets[i];
+		mhu->mlink[i].rx_stat_reg = reg_base + data->intr_stat_offs;
+		mhu->mlink[i].rx_set_reg = reg_base + data->intr_set_offs;
+		mhu->mlink[i].rx_clr_reg = reg_base + data->intr_clr_offs;
 	}

 	mhu->mbox.dev = dev;
 	mhu->mbox.chans = &mhu->chan[0];
-	mhu->mbox.num_chans = MHU_CHANS;
+	mhu->mbox.num_chans = data->channels;
 	mhu->mbox.ops = &mhu_ops;
 	mhu->mbox.txdone_irq = false;
 	mhu->mbox.txdone_poll = true;
 	mhu->mbox.txpoll_period = 1;

-	amba_set_drvdata(adev, mhu);
-
 	err = mbox_controller_register(&mhu->mbox);
 	if (err) {
 		dev_err(dev, "Failed to register mailboxes %d\n", err);
-		return err;
+		return ERR_PTR(err);
 	}

 	dev_info(dev, "ARM MHU Mailbox registered\n");
 	return 0;
+};
+
+static const struct of_device_id mhu_dt_ids[] = {
+	{ .compatible = "amlogic,meson-gxbb-mhu", .data = &meson_mhu_data},
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mhu_dt_ids);
+
+static int mhu_platform_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	unsigned int irqs[MAX_MHU_CHANS];
+	const struct arm_mhu_data *data;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct arm_mhu *mhu;
+	int i;
+
+	match = of_match_device(mhu_dt_ids, &pdev->dev);
+	if (!match)
+		return -EINVAL;
+
+	data = match->data;
+	if (!data)
+		return -EINVAL;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (WARN_ON(!res))
+		return -EINVAL;
+
+	for (i = 0 ; i < data->channels ; ++i) {
+		irqs[i] = platform_get_irq(pdev, i);
+		if (WARN_ON(irqs[i] < 0))
+			return irqs[i];
+	}
+
+	mhu = mhu_generic_probe(dev, res, irqs, data);
+	if (IS_ERR(mhu))
+		return PTR_ERR(mhu);
+
+	platform_set_drvdata(pdev, mhu);
+
+	return 0;
+}
+
+static int mhu_platform_remove(struct platform_device *pdev)
+{
+	struct arm_mhu *mhu = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&mhu->mbox);
+
+	return 0;
+}
+
+static struct platform_driver mhu_platform_driver = {
+	.probe	= mhu_platform_probe,
+	.remove	= mhu_platform_remove,
+	.driver = {
+		.name = "meson-mhu",
+		.of_match_table	= mhu_dt_ids,
+	},
+};
+
+static int mhu_amba_probe(struct amba_device *adev, const struct amba_id *id)
+{
+	const struct arm_mhu_data *data = id->data;
+	struct device *dev = &adev->dev;
+	struct arm_mhu *mhu;
+
+	mhu = mhu_generic_probe(dev, &adev->res, adev->irq, data);
+	if (IS_ERR(mhu))
+		return PTR_ERR(mhu);
+
+	amba_set_drvdata(adev, mhu);
+
+	return 0;
 }

-static int mhu_remove(struct amba_device *adev)
+static int mhu_amba_remove(struct amba_device *adev)
 {
 	struct arm_mhu *mhu = amba_get_drvdata(adev);

@@ -171,24 +297,43 @@ static int mhu_remove(struct amba_device *adev)
 	return 0;
 }

-static struct amba_id mhu_ids[] = {
+static struct amba_id mhu_amba_ids[] = {
 	{
 		.id	= 0x1bb098,
 		.mask	= 0xffffff,
+		.data	= &arm_mhu_amba_data,
 	},
 	{ 0, 0 },
 };
-MODULE_DEVICE_TABLE(amba, mhu_ids);
+MODULE_DEVICE_TABLE(amba, mhu_amba_ids);

 static struct amba_driver arm_mhu_driver = {
 	.drv = {
 		.name	= "mhu",
 	},
-	.id_table	= mhu_ids,
-	.probe		= mhu_probe,
-	.remove		= mhu_remove,
+	.id_table	= mhu_amba_ids,
+	.probe		= mhu_amba_probe,
+	.remove		= mhu_amba_remove,
 };
-module_amba_driver(arm_mhu_driver);
+
+static int __init mhu_module_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&mhu_platform_driver);
+	if (ret)
+		return ret;
+
+	return amba_driver_register(&arm_mhu_driver);
+}
+module_init(mhu_module_init);
+
+static void __exit mhu_module_exit(void)
+{
+	platform_driver_unregister(&mhu_platform_driver);
+	amba_driver_unregister(&arm_mhu_driver);
+}
+module_exit(mhu_module_exit);

 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("ARM MHU Driver");
-- 
2.7.0

Re: [RFC PATCH v2 1/9] mailbox: Add Amlogic Meson Message-Handling-Unit

[Jassi Brar]

-#define INTR_STAT_OFS  0x0
-#define INTR_SET_OFS   0x8
-#define INTR_CLR_OFS   0x10
-
-#define MHU_LP_OFFSET  0x0
-#define MHU_HP_OFFSET  0x20
-#define MHU_SEC_OFFSET 0x200
-#define TX_REG_OFFSET  0x100
+#define INTR_SET_OFS   0x0
+#define INTR_STAT_OFS  0x4
+#define INTR_CLR_OFS   0x8

-#define MHU_CHANS      3
+#define MHU_LP_OFFSET  0x10
+#define MHU_HP_OFFSET  0x1c
+
+#define TX_REG_OFFSET  0x24
+
+#define MHU_CHANS      2

^^^^^^^^ this is precisely the difference if we ignore cosmetic
differences. So the IP is essentially the same.

[snip]

> -------------------------------------------------------------------------------------------
> From: Neil Armstrong <narmstrong@baylibre.com>
> Subject: [PATCH] mailbox: arm_mhu: Add support for Amlogic Meson MHU
>
Is there some version of MHU specified anywhere in manuals? It seems
weird Amlogic took the IP and only rearranged the registers. Maybe the
order is specific to non-Amba version of the IP?  Lets call it that.


> To achieve this integration, add support for generic probe from amba
> or platform.
> Move all register offsets to a data structure passed in either amba id or
> platform dt id match table.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/mailbox/arm_mhu.c | 217 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 181 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
> index 99befa7..d7fb843 100644
> --- a/drivers/mailbox/arm_mhu.c
> +++ b/drivers/mailbox/arm_mhu.c
> @@ -22,45 +22,68 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/amba/bus.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
>  #include <linux/mailbox_controller.h>
>
> -#define INTR_STAT_OFS  0x0
> -#define INTR_SET_OFS   0x8
> -#define INTR_CLR_OFS   0x10
> +#define MHU_INTR_STAT_OFS      0x0
> +#define MHU_INTR_SET_OFS       0x8
> +#define MHU_INTR_CLR_OFS       0x10
>
>  #define MHU_LP_OFFSET  0x0
>  #define MHU_HP_OFFSET  0x20
>  #define MHU_SEC_OFFSET 0x200
> -#define TX_REG_OFFSET  0x100
> +#define MHU_TX_REG_OFFSET      0x100
>
> -#define MHU_CHANS      3
> +#define MESON_INTR_SET_OFS     0x0
> +#define MESON_INTR_STAT_OFS    0x4
> +#define MESON_INTR_CLR_OFS     0x8
> +
> +#define MESON_MHU_LP_OFFSET    0x10
> +#define MESON_MHU_HP_OFFSET    0x1c
> +#define MESON_MHU_TX_OFFSET    0x24
> +
> +#define MAX_MHU_CHANS  3
>
MHU_CHANS always 3 doesn't hurt. Lets keep it unchanged.

>  struct mhu_link {
>         unsigned irq;
> -       void __iomem *tx_reg;
> -       void __iomem *rx_reg;
> +       void __iomem *tx_stat_reg;
> +       void __iomem *tx_set_reg;
> +       void __iomem *tx_clr_reg;
> +       void __iomem *rx_stat_reg;
> +       void __iomem *rx_set_reg;
> +       void __iomem *rx_clr_reg;
>  };

Yeah, this is OK.


>
>  struct arm_mhu {
>         void __iomem *base;
> -       struct mhu_link mlink[MHU_CHANS];
> -       struct mbox_chan chan[MHU_CHANS];
> +       struct mhu_link mlink[MAX_MHU_CHANS];
> +       struct mbox_chan chan[MAX_MHU_CHANS];
>         struct mbox_controller mbox;
>  };
just leave it MHU_CHANS

>
> +struct arm_mhu_data {
> +       unsigned int channels;
> +       int rx_offsets[MAX_MHU_CHANS];
> +       int tx_offsets[MAX_MHU_CHANS];
> +       unsigned int intr_stat_offs;
> +       unsigned int intr_set_offs;
> +       unsigned int intr_clr_offs;
> +};
This is unnecessary. Please remove it and code will be simpler -
assign rx/tx_regs directly in probe.


> +
>  static irqreturn_t mhu_rx_interrupt(int irq, void *p)
>  {
>         struct mbox_chan *chan = p;
>         struct mhu_link *mlink = chan->con_priv;
>         u32 val;
>
> -       val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS);
> +       val = readl_relaxed(mlink->rx_stat_reg);
>         if (!val)
>                 return IRQ_NONE;
>
>         mbox_chan_received_data(chan, (void *)&val);
>
> -       writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS);
> +       writel_relaxed(val, mlink->rx_clr_reg);
>
>         return IRQ_HANDLED;
>  }
> @@ -68,7 +91,7 @@ static irqreturn_t mhu_rx_interrupt(int irq, void *p)
>  static bool mhu_last_tx_done(struct mbox_chan *chan)
>  {
>         struct mhu_link *mlink = chan->con_priv;
> -       u32 val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
> +       u32 val = readl_relaxed(mlink->tx_stat_reg);
>
>         return (val == 0);
>  }
> @@ -78,7 +101,7 @@ static int mhu_send_data(struct mbox_chan *chan, void *data)
>         struct mhu_link *mlink = chan->con_priv;
>         u32 *arg = data;
>
> -       writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS);
> +       writel_relaxed(*arg, mlink->tx_set_reg);
>
>         return 0;
>  }
> @@ -89,8 +112,8 @@ static int mhu_startup(struct mbox_chan *chan)
>         u32 val;
>         int ret;
>
> -       val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
> -       writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);
> +       val = readl_relaxed(mlink->tx_stat_reg);
> +       writel_relaxed(val, mlink->tx_clr_reg);
>
>         ret = request_irq(mlink->irq, mhu_rx_interrupt,
>                           IRQF_SHARED, "mhu_link", chan);
> @@ -117,52 +140,155 @@ static const struct mbox_chan_ops mhu_ops = {
>         .last_tx_done = mhu_last_tx_done,
>  };
>
> -static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
> +static struct arm_mhu_data arm_mhu_amba_data = {
> +       .channels = 3,
> +       .rx_offsets = {MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET},
> +       .tx_offsets = {MHU_LP_OFFSET + MHU_TX_REG_OFFSET,
> +                      MHU_HP_OFFSET + MHU_TX_REG_OFFSET,
> +                      MHU_SEC_OFFSET + MHU_TX_REG_OFFSET},
> +       .intr_stat_offs = MHU_INTR_STAT_OFS,
> +       .intr_set_offs = MHU_INTR_SET_OFS,
> +       .intr_clr_offs = MHU_INTR_CLR_OFS,
> +};
> +
> +static const struct arm_mhu_data meson_mhu_data = {
> +       .channels = 2,
> +       .rx_offsets = {MESON_MHU_LP_OFFSET, MESON_MHU_HP_OFFSET},
> +       .tx_offsets = {MESON_MHU_LP_OFFSET + MESON_MHU_TX_OFFSET,
> +                      MESON_MHU_HP_OFFSET + MESON_MHU_TX_OFFSET},
> +       .intr_stat_offs = MESON_INTR_STAT_OFS,
> +       .intr_set_offs = MESON_INTR_SET_OFS,
> +       .intr_clr_offs = MESON_INTR_CLR_OFS,
> +};
> +
These could be directly set in amba vs platform probes ?

Thanks.

Re: [RFC PATCH v2 1/9] mailbox: Add Amlogic Meson Message-Handling-Unit

[Neil Armstrong]

On 06/25/2016 07:50 PM, Jassi Brar wrote:
> -#define INTR_STAT_OFS  0x0
> -#define INTR_SET_OFS   0x8
> -#define INTR_CLR_OFS   0x10
> -
> -#define MHU_LP_OFFSET  0x0
> -#define MHU_HP_OFFSET  0x20
> -#define MHU_SEC_OFFSET 0x200
> -#define TX_REG_OFFSET  0x100
> +#define INTR_SET_OFS   0x0
> +#define INTR_STAT_OFS  0x4
> +#define INTR_CLR_OFS   0x8
> 
> -#define MHU_CHANS      3
> +#define MHU_LP_OFFSET  0x10
> +#define MHU_HP_OFFSET  0x1c
> +
> +#define TX_REG_OFFSET  0x24
> +
> +#define MHU_CHANS      2
> 
> ^^^^^^^^ this is precisely the difference if we ignore cosmetic
> differences. So the IP is essentially the same.

Well, no. The overall design is similar. but clearly it's a different IP.

> [snip]
> 
>> -------------------------------------------------------------------------------------------
>> From: Neil Armstrong <narmstrong@baylibre.com>
>> Subject: [PATCH] mailbox: arm_mhu: Add support for Amlogic Meson MHU
>>
> Is there some version of MHU specified anywhere in manuals? It seems
> weird Amlogic took the IP and only rearranged the registers. Maybe the
> order is specific to non-Amba version of the IP?  Lets call it that.

I think Amlogic took an early Juno platform release and re-implemented the
MHU using the same concept but following their design rules.

> 
>> To achieve this integration, add support for generic probe from amba
>> or platform.
>> Move all register offsets to a data structure passed in either amba id or
>> platform dt id match table.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/mailbox/arm_mhu.c | 217 ++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 181 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
>> index 99befa7..d7fb843 100644
>> --- a/drivers/mailbox/arm_mhu.c
>> +++ b/drivers/mailbox/arm_mhu.c
>> @@ -22,45 +22,68 @@
>>  #include <linux/io.h>
>>  #include <linux/module.h>
>>  #include <linux/amba/bus.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>>  #include <linux/mailbox_controller.h>
>>
>> -#define INTR_STAT_OFS  0x0
>> -#define INTR_SET_OFS   0x8
>> -#define INTR_CLR_OFS   0x10
>> +#define MHU_INTR_STAT_OFS      0x0
>> +#define MHU_INTR_SET_OFS       0x8
>> +#define MHU_INTR_CLR_OFS       0x10
>>
>>  #define MHU_LP_OFFSET  0x0
>>  #define MHU_HP_OFFSET  0x20
>>  #define MHU_SEC_OFFSET 0x200
>> -#define TX_REG_OFFSET  0x100
>> +#define MHU_TX_REG_OFFSET      0x100
>>
>> -#define MHU_CHANS      3
>> +#define MESON_INTR_SET_OFS     0x0
>> +#define MESON_INTR_STAT_OFS    0x4
>> +#define MESON_INTR_CLR_OFS     0x8
>> +
>> +#define MESON_MHU_LP_OFFSET    0x10
>> +#define MESON_MHU_HP_OFFSET    0x1c
>> +#define MESON_MHU_TX_OFFSET    0x24
>> +
>> +#define MAX_MHU_CHANS  3
>>
> MHU_CHANS always 3 doesn't hurt. Lets keep it unchanged.
> 
>>  struct mhu_link {
>>         unsigned irq;
>> -       void __iomem *tx_reg;
>> -       void __iomem *rx_reg;
>> +       void __iomem *tx_stat_reg;
>> +       void __iomem *tx_set_reg;
>> +       void __iomem *tx_clr_reg;
>> +       void __iomem *rx_stat_reg;
>> +       void __iomem *rx_set_reg;
>> +       void __iomem *rx_clr_reg;
>>  };
> 
> Yeah, this is OK.
> 
> 
>>
>>  struct arm_mhu {
>>         void __iomem *base;
>> -       struct mhu_link mlink[MHU_CHANS];
>> -       struct mbox_chan chan[MHU_CHANS];
>> +       struct mhu_link mlink[MAX_MHU_CHANS];
>> +       struct mbox_chan chan[MAX_MHU_CHANS];
>>         struct mbox_controller mbox;
>>  };
> just leave it MHU_CHANS
> 
>>
>> +struct arm_mhu_data {
>> +       unsigned int channels;
>> +       int rx_offsets[MAX_MHU_CHANS];
>> +       int tx_offsets[MAX_MHU_CHANS];
>> +       unsigned int intr_stat_offs;
>> +       unsigned int intr_set_offs;
>> +       unsigned int intr_clr_offs;
>> +};
> This is unnecessary. Please remove it and code will be simpler -
> assign rx/tx_regs directly in probe.

I won't assume the platform driver is only for Amlogic, it does not
make sense.

> 
>> +
>>  static irqreturn_t mhu_rx_interrupt(int irq, void *p)
>>  {
>>         struct mbox_chan *chan = p;
>>         struct mhu_link *mlink = chan->con_priv;
>>         u32 val;
>>
>> -       val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS);
>> +       val = readl_relaxed(mlink->rx_stat_reg);
>>         if (!val)
>>                 return IRQ_NONE;
>>
>>         mbox_chan_received_data(chan, (void *)&val);
>>
>> -       writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS);
>> +       writel_relaxed(val, mlink->rx_clr_reg);
>>
>>         return IRQ_HANDLED;
>>  }
>> @@ -68,7 +91,7 @@ static irqreturn_t mhu_rx_interrupt(int irq, void *p)
>>  static bool mhu_last_tx_done(struct mbox_chan *chan)
>>  {
>>         struct mhu_link *mlink = chan->con_priv;
>> -       u32 val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
>> +       u32 val = readl_relaxed(mlink->tx_stat_reg);
>>
>>         return (val == 0);
>>  }
>> @@ -78,7 +101,7 @@ static int mhu_send_data(struct mbox_chan *chan, void *data)
>>         struct mhu_link *mlink = chan->con_priv;
>>         u32 *arg = data;
>>
>> -       writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS);
>> +       writel_relaxed(*arg, mlink->tx_set_reg);
>>
>>         return 0;
>>  }
>> @@ -89,8 +112,8 @@ static int mhu_startup(struct mbox_chan *chan)
>>         u32 val;
>>         int ret;
>>
>> -       val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
>> -       writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);
>> +       val = readl_relaxed(mlink->tx_stat_reg);
>> +       writel_relaxed(val, mlink->tx_clr_reg);
>>
>>         ret = request_irq(mlink->irq, mhu_rx_interrupt,
>>                           IRQF_SHARED, "mhu_link", chan);
>> @@ -117,52 +140,155 @@ static const struct mbox_chan_ops mhu_ops = {
>>         .last_tx_done = mhu_last_tx_done,
>>  };
>>
>> -static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
>> +static struct arm_mhu_data arm_mhu_amba_data = {
>> +       .channels = 3,
>> +       .rx_offsets = {MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET},
>> +       .tx_offsets = {MHU_LP_OFFSET + MHU_TX_REG_OFFSET,
>> +                      MHU_HP_OFFSET + MHU_TX_REG_OFFSET,
>> +                      MHU_SEC_OFFSET + MHU_TX_REG_OFFSET},
>> +       .intr_stat_offs = MHU_INTR_STAT_OFS,
>> +       .intr_set_offs = MHU_INTR_SET_OFS,
>> +       .intr_clr_offs = MHU_INTR_CLR_OFS,
>> +};
>> +
>> +static const struct arm_mhu_data meson_mhu_data = {
>> +       .channels = 2,
>> +       .rx_offsets = {MESON_MHU_LP_OFFSET, MESON_MHU_HP_OFFSET},
>> +       .tx_offsets = {MESON_MHU_LP_OFFSET + MESON_MHU_TX_OFFSET,
>> +                      MESON_MHU_HP_OFFSET + MESON_MHU_TX_OFFSET},
>> +       .intr_stat_offs = MESON_INTR_STAT_OFS,
>> +       .intr_set_offs = MESON_INTR_SET_OFS,
>> +       .intr_clr_offs = MESON_INTR_CLR_OFS,
>> +};
>> +
> These could be directly set in amba vs platform probes ?

It does not make sense to assume the platform driver is only for
amlogic gxbb, it could match other vendors aswell.

The amba could force a single struct, but it's smarter to use the
same mechanism and associate the struct to an ID.

> Thanks.
> 

My main question is : do you really want to transform this simple driver into
a dirty multi-bus generic mailbox driver ?
The meson_mhu is only 199 lines and this patch adds 181 lines to the arm_mhu driver.

I'll personally push to have two separate drivers here.

Thanks,
Neil

Re: [RFC PATCH v2 1/9] mailbox: Add Amlogic Meson Message-Handling-Unit

[Jassi Brar]

On Tue, Jun 28, 2016 at 7:30 PM, Neil Armstrong <narmstrong@baylibre.com> wrote:
> On 06/25/2016 07:50 PM, Jassi Brar wrote:
>> -#define INTR_STAT_OFS  0x0
>> -#define INTR_SET_OFS   0x8
>> -#define INTR_CLR_OFS   0x10
>> -
>> -#define MHU_LP_OFFSET  0x0
>> -#define MHU_HP_OFFSET  0x20
>> -#define MHU_SEC_OFFSET 0x200
>> -#define TX_REG_OFFSET  0x100
>> +#define INTR_SET_OFS   0x0
>> +#define INTR_STAT_OFS  0x4
>> +#define INTR_CLR_OFS   0x8
>>
>> -#define MHU_CHANS      3
>> +#define MHU_LP_OFFSET  0x10
>> +#define MHU_HP_OFFSET  0x1c
>> +
>> +#define TX_REG_OFFSET  0x24
>> +
>> +#define MHU_CHANS      2
>>
>> ^^^^^^^^ this is precisely the difference if we ignore cosmetic
>> differences. So the IP is essentially the same.
>
> Well, no. The overall design is similar. but clearly it's a different IP.
>
If your this patch works on your platform, then the diff from arm_mhu
tells the controller is the same but only with re-ordered registers.
And you already call it 'MHU' :)


>> [snip]
>>
>>> -------------------------------------------------------------------------------------------
>>> From: Neil Armstrong <narmstrong@baylibre.com>
>>> Subject: [PATCH] mailbox: arm_mhu: Add support for Amlogic Meson MHU
>>>
>> Is there some version of MHU specified anywhere in manuals? It seems
>> weird Amlogic took the IP and only rearranged the registers. Maybe the
>> order is specific to non-Amba version of the IP?  Lets call it that.
>
> I think Amlogic took an early Juno platform release and re-implemented the
> MHU using the same concept but following their design rules.
>
>>
>>> To achieve this integration, add support for generic probe from amba
>>> or platform.
>>> Move all register offsets to a data structure passed in either amba id or
>>> platform dt id match table.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/mailbox/arm_mhu.c | 217 ++++++++++++++++++++++++++++++++++++++--------
>>>  1 file changed, 181 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/mailbox/arm_mhu.c b/drivers/mailbox/arm_mhu.c
>>> index 99befa7..d7fb843 100644
>>> --- a/drivers/mailbox/arm_mhu.c
>>> +++ b/drivers/mailbox/arm_mhu.c
>>> @@ -22,45 +22,68 @@
>>>  #include <linux/io.h>
>>>  #include <linux/module.h>
>>>  #include <linux/amba/bus.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/platform_device.h>
>>>  #include <linux/mailbox_controller.h>
>>>
>>> -#define INTR_STAT_OFS  0x0
>>> -#define INTR_SET_OFS   0x8
>>> -#define INTR_CLR_OFS   0x10
>>> +#define MHU_INTR_STAT_OFS      0x0
>>> +#define MHU_INTR_SET_OFS       0x8
>>> +#define MHU_INTR_CLR_OFS       0x10
>>>
>>>  #define MHU_LP_OFFSET  0x0
>>>  #define MHU_HP_OFFSET  0x20
>>>  #define MHU_SEC_OFFSET 0x200
>>> -#define TX_REG_OFFSET  0x100
>>> +#define MHU_TX_REG_OFFSET      0x100
>>>
>>> -#define MHU_CHANS      3
>>> +#define MESON_INTR_SET_OFS     0x0
>>> +#define MESON_INTR_STAT_OFS    0x4
>>> +#define MESON_INTR_CLR_OFS     0x8
>>> +
>>> +#define MESON_MHU_LP_OFFSET    0x10
>>> +#define MESON_MHU_HP_OFFSET    0x1c
>>> +#define MESON_MHU_TX_OFFSET    0x24
>>> +
>>> +#define MAX_MHU_CHANS  3
>>>
>> MHU_CHANS always 3 doesn't hurt. Lets keep it unchanged.
>>
>>>  struct mhu_link {
>>>         unsigned irq;
>>> -       void __iomem *tx_reg;
>>> -       void __iomem *rx_reg;
>>> +       void __iomem *tx_stat_reg;
>>> +       void __iomem *tx_set_reg;
>>> +       void __iomem *tx_clr_reg;
>>> +       void __iomem *rx_stat_reg;
>>> +       void __iomem *rx_set_reg;
>>> +       void __iomem *rx_clr_reg;
>>>  };
>>
>> Yeah, this is OK.
>>
>>
>>>
>>>  struct arm_mhu {
>>>         void __iomem *base;
>>> -       struct mhu_link mlink[MHU_CHANS];
>>> -       struct mbox_chan chan[MHU_CHANS];
>>> +       struct mhu_link mlink[MAX_MHU_CHANS];
>>> +       struct mbox_chan chan[MAX_MHU_CHANS];
>>>         struct mbox_controller mbox;
>>>  };
>> just leave it MHU_CHANS
>>
>>>
>>> +struct arm_mhu_data {
>>> +       unsigned int channels;
>>> +       int rx_offsets[MAX_MHU_CHANS];
>>> +       int tx_offsets[MAX_MHU_CHANS];
>>> +       unsigned int intr_stat_offs;
>>> +       unsigned int intr_set_offs;
>>> +       unsigned int intr_clr_offs;
>>> +};
>> This is unnecessary. Please remove it and code will be simpler -
>> assign rx/tx_regs directly in probe.
>
> I won't assume the platform driver is only for Amlogic, it does not
> make sense.
>
It is unlikely other platforms will come with more random register
arrangements of MHU.


>>
>>> +
>>>  static irqreturn_t mhu_rx_interrupt(int irq, void *p)
>>>  {
>>>         struct mbox_chan *chan = p;
>>>         struct mhu_link *mlink = chan->con_priv;
>>>         u32 val;
>>>
>>> -       val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS);
>>> +       val = readl_relaxed(mlink->rx_stat_reg);
>>>         if (!val)
>>>                 return IRQ_NONE;
>>>
>>>         mbox_chan_received_data(chan, (void *)&val);
>>>
>>> -       writel_relaxed(val, mlink->rx_reg + INTR_CLR_OFS);
>>> +       writel_relaxed(val, mlink->rx_clr_reg);
>>>
>>>         return IRQ_HANDLED;
>>>  }
>>> @@ -68,7 +91,7 @@ static irqreturn_t mhu_rx_interrupt(int irq, void *p)
>>>  static bool mhu_last_tx_done(struct mbox_chan *chan)
>>>  {
>>>         struct mhu_link *mlink = chan->con_priv;
>>> -       u32 val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
>>> +       u32 val = readl_relaxed(mlink->tx_stat_reg);
>>>
>>>         return (val == 0);
>>>  }
>>> @@ -78,7 +101,7 @@ static int mhu_send_data(struct mbox_chan *chan, void *data)
>>>         struct mhu_link *mlink = chan->con_priv;
>>>         u32 *arg = data;
>>>
>>> -       writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS);
>>> +       writel_relaxed(*arg, mlink->tx_set_reg);
>>>
>>>         return 0;
>>>  }
>>> @@ -89,8 +112,8 @@ static int mhu_startup(struct mbox_chan *chan)
>>>         u32 val;
>>>         int ret;
>>>
>>> -       val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
>>> -       writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);
>>> +       val = readl_relaxed(mlink->tx_stat_reg);
>>> +       writel_relaxed(val, mlink->tx_clr_reg);
>>>
>>>         ret = request_irq(mlink->irq, mhu_rx_interrupt,
>>>                           IRQF_SHARED, "mhu_link", chan);
>>> @@ -117,52 +140,155 @@ static const struct mbox_chan_ops mhu_ops = {
>>>         .last_tx_done = mhu_last_tx_done,
>>>  };
>>>
>>> -static int mhu_probe(struct amba_device *adev, const struct amba_id *id)
>>> +static struct arm_mhu_data arm_mhu_amba_data = {
>>> +       .channels = 3,
>>> +       .rx_offsets = {MHU_LP_OFFSET, MHU_HP_OFFSET, MHU_SEC_OFFSET},
>>> +       .tx_offsets = {MHU_LP_OFFSET + MHU_TX_REG_OFFSET,
>>> +                      MHU_HP_OFFSET + MHU_TX_REG_OFFSET,
>>> +                      MHU_SEC_OFFSET + MHU_TX_REG_OFFSET},
>>> +       .intr_stat_offs = MHU_INTR_STAT_OFS,
>>> +       .intr_set_offs = MHU_INTR_SET_OFS,
>>> +       .intr_clr_offs = MHU_INTR_CLR_OFS,
>>> +};
>>> +
>>> +static const struct arm_mhu_data meson_mhu_data = {
>>> +       .channels = 2,
>>> +       .rx_offsets = {MESON_MHU_LP_OFFSET, MESON_MHU_HP_OFFSET},
>>> +       .tx_offsets = {MESON_MHU_LP_OFFSET + MESON_MHU_TX_OFFSET,
>>> +                      MESON_MHU_HP_OFFSET + MESON_MHU_TX_OFFSET},
>>> +       .intr_stat_offs = MESON_INTR_STAT_OFS,
>>> +       .intr_set_offs = MESON_INTR_SET_OFS,
>>> +       .intr_clr_offs = MESON_INTR_CLR_OFS,
>>> +};
>>> +
>> These could be directly set in amba vs platform probes ?
>
> It does not make sense to assume the platform driver is only for
> amlogic gxbb, it could match other vendors aswell.
>
Perhaps you didn't get my suggestion.

> The amba could force a single struct, but it's smarter to use the
> same mechanism and associate the struct to an ID.
>
>> Thanks.
>>
>
> My main question is : do you really want to transform this simple driver into
> a dirty multi-bus generic mailbox driver ?
> The meson_mhu is only 199 lines and this patch adds 181 lines to the arm_mhu driver.
>
> I'll personally push to have two separate drivers here.
>
It is a shame if we need to copy a driver only due to changed register
offsets. Let me give it a shot and see how worse off we would be.

Thanks.

Re: [RFC PATCH v2 1/9] mailbox: Add Amlogic Meson Message-Handling-Unit

[Jassi Brar]

On Tue, Jun 28, 2016 at 8:36 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Tue, Jun 28, 2016 at 7:30 PM, Neil Armstrong <narmstrong@baylibre.com> wrote:

>>
>> My main question is : do you really want to transform this simple driver into
>> a dirty multi-bus generic mailbox driver ?
>> The meson_mhu is only 199 lines and this patch adds 181 lines to the arm_mhu driver.
>>
>> I'll personally push to have two separate drivers here.
>>
> It is a shame if we need to copy a driver only due to changed register
> offsets. Let me give it a shot and see how worse off we would be.
>
OK, so I trimmed the differences further but it still doesn't look any
better. The best approach seems to be have a separate driver but with
consideration that its a 3rd party IP and hence likely to be used by
other platforms. That is, call it something meson-neutral. I will make
a few comments on the patch post.

Thanks

Re: [RFC PATCH v2 1/9] mailbox: Add Amlogic Meson Message-Handling-Unit

[Jassi Brar]

On Tue, Jun 21, 2016 at 3:32 PM, Neil Armstrong <narmstrong@baylibre.com> wrote:
> Add Amlogic Meson SoCs Message-Handling-Unit as mailbox controller
> with 2 independent channels/links to communicate with a remote processor.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/mailbox/Makefile    |   2 +
>  drivers/mailbox/meson_mhu.c | 199 ++++++++++++++++++++++++++++++++++++++++++++
>
Can we call it pdev_mhu.c or similar so that some other platform using
the MHU as a platform_device wouldn't have to embarrassingly call it
'Meson's MHU'?  And also the replace meson with that prefix in the
code.

>  2 files changed, 201 insertions(+)
>  create mode 100644 drivers/mailbox/meson_mhu.c
>
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 0be3e74..6aa9dbe 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -25,3 +25,5 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
>  obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
>
>  obj-$(CONFIG_HI6220_MBOX)      += hi6220-mailbox.o
> +
> +obj-$(CONFIG_ARCH_MESON)       += meson_mhu.o
> diff --git a/drivers/mailbox/meson_mhu.c b/drivers/mailbox/meson_mhu.c
> new file mode 100644
> index 0000000..0576b92
> --- /dev/null
> +++ b/drivers/mailbox/meson_mhu.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2016 BayLibre SAS.
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + * Heavily based on meson_mhu.c from :
> + * Copyright (C) 2013-2015 Fujitsu Semiconductor Ltd.
> + * Copyright (C) 2015 Linaro Ltd.
> + * Author: Jassi Brar <jaswinder.singh@linaro.org>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/spinlock.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mailbox_controller.h>
> +
> +#define INTR_SET_OFS   0x0
> +#define INTR_STAT_OFS  0x4
> +#define INTR_CLR_OFS   0x8
> +
> +#define MHU_LP_OFFSET  0x10
> +#define MHU_HP_OFFSET  0x1c
> +
> +#define TX_REG_OFFSET  0x24
> +
> +#define MHU_CHANS      2
> +
> +struct meson_mhu_link {
> +       unsigned int irq;
> +       void __iomem *tx_reg;
> +       void __iomem *rx_reg;
> +};
> +
> +struct meson_mhu {
> +       void __iomem *base;
> +       struct meson_mhu_link mlink[MHU_CHANS];
> +       struct mbox_chan chan[MHU_CHANS];
> +       struct mbox_controller mbox;
> +};
> +
> +static irqreturn_t meson_mhu_rx_interrupt(int irq, void *p)
> +{
> +       struct mbox_chan *chan = p;
> +       struct meson_mhu_link *mlink = chan->con_priv;
> +       u32 val;
> +
> +       val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS);
> +       if (!val)
> +               return IRQ_NONE;
> +
> +       mbox_chan_received_data(chan, (void *)&val);
> +
> +       writel_relaxed(~0, mlink->rx_reg + INTR_CLR_OFS);
> +
How about sync with arm_mhu.c by doing writel_relaxed(val,
mlink->rx_reg + INTR_CLR_OFS) ?


> +       return IRQ_HANDLED;
> +}
> +
> +static bool meson_mhu_last_tx_done(struct mbox_chan *chan)
> +{
> +       struct meson_mhu_link *mlink = chan->con_priv;
> +       u32 val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
> +
> +       return (val == 0);
> +}
> +
> +static int meson_mhu_send_data(struct mbox_chan *chan, void *data)
> +{
> +       struct meson_mhu_link *mlink = chan->con_priv;
> +       u32 *arg = data;
> +
> +       writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS);
> +
> +       return 0;
> +}
> +
> +static int meson_mhu_startup(struct mbox_chan *chan)
> +{
> +       struct meson_mhu_link *mlink = chan->con_priv;
> +       int ret;
> +
arm_mhu.c clears any pending TX before taking over by doing ...
       val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
       writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);

> +       ret = request_irq(mlink->irq, meson_mhu_rx_interrupt,
> +                         IRQF_ONESHOT, "meson_mhu_link", chan);
> +       if (ret) {
> +               dev_err(chan->mbox->dev,
> +                       "Unable to acquire IRQ %d\n", mlink->irq);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static void meson_mhu_shutdown(struct mbox_chan *chan)
> +{
> +       struct meson_mhu_link *mlink = chan->con_priv;
> +
> +       free_irq(mlink->irq, chan);
> +}
> +
> +static const struct mbox_chan_ops meson_mhu_ops = {
> +       .send_data = meson_mhu_send_data,
> +       .startup = meson_mhu_startup,
> +       .shutdown = meson_mhu_shutdown,
> +       .last_tx_done = meson_mhu_last_tx_done,
> +};
>
My two comments above may not be necessary for your platform, but I
would suggest we keep the core (from the declaration of struct
meson_mhu_link to this point) in sync with arm_mhu.c

> +static int meson_mhu_probe(struct platform_device *pdev)
> +{
> +       int i, err;
> +       struct meson_mhu *mhu;
> +       struct device *dev = &pdev->dev;
> +       struct resource *res;
> +       int meson_mhu_reg[MHU_CHANS] = {MHU_LP_OFFSET, MHU_HP_OFFSET};
> +
> +       /* Allocate memory for device */
> +       mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
> +       if (!mhu)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       mhu->base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(mhu->base)) {
> +               dev_err(dev, "ioremap failed\n");
> +               return PTR_ERR(mhu->base);
> +       }
> +
> +       for (i = 0; i < MHU_CHANS; i++) {
> +               mhu->chan[i].con_priv = &mhu->mlink[i];
> +               mhu->mlink[i].irq = platform_get_irq(pdev, i);
> +               if (mhu->mlink[i].irq < 0) {
> +                       dev_err(dev, "failed to get irq%d\n", i);
> +                       return mhu->mlink[i].irq;
> +               }
> +               mhu->mlink[i].rx_reg = mhu->base + meson_mhu_reg[i];
> +               mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
> +       }
> +
> +       mhu->mbox.dev = dev;
> +       mhu->mbox.chans = &mhu->chan[0];
> +       mhu->mbox.num_chans = MHU_CHANS;
> +       mhu->mbox.ops = &meson_mhu_ops;
> +       mhu->mbox.txdone_irq = false;
> +       mhu->mbox.txdone_poll = true;
> +       mhu->mbox.txpoll_period = 1;
> +
> +       platform_set_drvdata(pdev, mhu);
> +
> +       err = mbox_controller_register(&mhu->mbox);
> +       if (err) {
> +               dev_err(dev, "Failed to register mailboxes %d\n", err);
> +               return err;
> +       }
> +
> +       dev_info(dev, "Meson MHU Mailbox registered\n");
> +       return 0;
> +}
> +
> +static int meson_mhu_remove(struct platform_device *pdev)
> +{
> +       struct meson_mhu *mhu = platform_get_drvdata(pdev);
> +
> +       mbox_controller_unregister(&mhu->mbox);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id meson_mhu_dt_ids[] = {
> +       { .compatible = "amlogic,meson-gxbb-mhu", },
> +       { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, meson_mhu_ids);
> +
> +static struct platform_driver meson_wdt_driver = {
> +       .probe  = meson_mhu_probe,
> +       .remove = meson_mhu_remove,
> +       .driver = {
> +               .name = "meson-mhu",
> +               .of_match_table = meson_mhu_dt_ids,
> +       },
> +};
> +
> +module_platform_driver(meson_wdt_driver);
> +
wdt as in watchdog-timer from drivers/watchdog/meson_wdt.c ? :)

Thanks.

Re: [RFC PATCH v2 1/9] mailbox: Add Amlogic Meson Message-Handling-Unit

[Neil Armstrong]

2016-07-04 17:38 GMT+02:00 Jassi Brar <jassisinghbrar@gmail.com>:
> On Tue, Jun 21, 2016 at 3:32 PM, Neil Armstrong <narmstrong@baylibre.com> wrote:
>> Add Amlogic Meson SoCs Message-Handling-Unit as mailbox controller
>> with 2 independent channels/links to communicate with a remote processor.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/mailbox/Makefile    |   2 +
>>  drivers/mailbox/meson_mhu.c | 199 ++++++++++++++++++++++++++++++++++++++++++++
>>
> Can we call it pdev_mhu.c or similar so that some other platform using
> the MHU as a platform_device wouldn't have to embarrassingly call it
> 'Meson's MHU'?  And also the replace meson with that prefix in the
> code.

Yes, it may deserve a more generic naming, but pdev_mhu is not good
looking at all !
What about platform_mhu ?

>
>>  2 files changed, 201 insertions(+)
>>  create mode 100644 drivers/mailbox/meson_mhu.c
>>
>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>> index 0be3e74..6aa9dbe 100644
>> --- a/drivers/mailbox/Makefile
>> +++ b/drivers/mailbox/Makefile
>> @@ -25,3 +25,5 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
>>  obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
>>
>>  obj-$(CONFIG_HI6220_MBOX)      += hi6220-mailbox.o
>> +
>> +obj-$(CONFIG_ARCH_MESON)       += meson_mhu.o
>> diff --git a/drivers/mailbox/meson_mhu.c b/drivers/mailbox/meson_mhu.c
>> new file mode 100644
>> index 0000000..0576b92
>> --- /dev/null
>> +++ b/drivers/mailbox/meson_mhu.c
>> @@ -0,0 +1,199 @@
>> +/*
>> + * Copyright (C) 2016 BayLibre SAS.
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + * Heavily based on meson_mhu.c from :
>> + * Copyright (C) 2013-2015 Fujitsu Semiconductor Ltd.
>> + * Copyright (C) 2015 Linaro Ltd.
>> + * Author: Jassi Brar <jaswinder.singh@linaro.org>
>> + *
>> + * This program is free software: you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/mutex.h>
>> +#include <linux/delay.h>
>> +#include <linux/slab.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/mailbox_controller.h>
>> +
>> +#define INTR_SET_OFS   0x0
>> +#define INTR_STAT_OFS  0x4
>> +#define INTR_CLR_OFS   0x8
>> +
>> +#define MHU_LP_OFFSET  0x10
>> +#define MHU_HP_OFFSET  0x1c
>> +
>> +#define TX_REG_OFFSET  0x24
>> +
>> +#define MHU_CHANS      2
>> +
>> +struct meson_mhu_link {
>> +       unsigned int irq;
>> +       void __iomem *tx_reg;
>> +       void __iomem *rx_reg;
>> +};
>> +
>> +struct meson_mhu {
>> +       void __iomem *base;
>> +       struct meson_mhu_link mlink[MHU_CHANS];
>> +       struct mbox_chan chan[MHU_CHANS];
>> +       struct mbox_controller mbox;
>> +};
>> +
>> +static irqreturn_t meson_mhu_rx_interrupt(int irq, void *p)
>> +{
>> +       struct mbox_chan *chan = p;
>> +       struct meson_mhu_link *mlink = chan->con_priv;
>> +       u32 val;
>> +
>> +       val = readl_relaxed(mlink->rx_reg + INTR_STAT_OFS);
>> +       if (!val)
>> +               return IRQ_NONE;
>> +
>> +       mbox_chan_received_data(chan, (void *)&val);
>> +
>> +       writel_relaxed(~0, mlink->rx_reg + INTR_CLR_OFS);
>> +
> How about sync with arm_mhu.c by doing writel_relaxed(val,
> mlink->rx_reg + INTR_CLR_OFS) ?

The ~0 was taken from the vendor driver, but writing val should work,
I must test this change.

>
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static bool meson_mhu_last_tx_done(struct mbox_chan *chan)
>> +{
>> +       struct meson_mhu_link *mlink = chan->con_priv;
>> +       u32 val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
>> +
>> +       return (val == 0);
>> +}
>> +
>> +static int meson_mhu_send_data(struct mbox_chan *chan, void *data)
>> +{
>> +       struct meson_mhu_link *mlink = chan->con_priv;
>> +       u32 *arg = data;
>> +
>> +       writel_relaxed(*arg, mlink->tx_reg + INTR_SET_OFS);
>> +
>> +       return 0;
>> +}
>> +
>> +static int meson_mhu_startup(struct mbox_chan *chan)
>> +{
>> +       struct meson_mhu_link *mlink = chan->con_priv;
>> +       int ret;
>> +
> arm_mhu.c clears any pending TX before taking over by doing ...
>        val = readl_relaxed(mlink->tx_reg + INTR_STAT_OFS);
>        writel_relaxed(val, mlink->tx_reg + INTR_CLR_OFS);

I dropped it not knowing it it was necessary because not pressent in
the vendor code, but yes it would clearly be good to get it back.

>> +       ret = request_irq(mlink->irq, meson_mhu_rx_interrupt,
>> +                         IRQF_ONESHOT, "meson_mhu_link", chan);
>> +       if (ret) {
>> +               dev_err(chan->mbox->dev,
>> +                       "Unable to acquire IRQ %d\n", mlink->irq);
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static void meson_mhu_shutdown(struct mbox_chan *chan)
>> +{
>> +       struct meson_mhu_link *mlink = chan->con_priv;
>> +
>> +       free_irq(mlink->irq, chan);
>> +}
>> +
>> +static const struct mbox_chan_ops meson_mhu_ops = {
>> +       .send_data = meson_mhu_send_data,
>> +       .startup = meson_mhu_startup,
>> +       .shutdown = meson_mhu_shutdown,
>> +       .last_tx_done = meson_mhu_last_tx_done,
>> +};
>>
> My two comments above may not be necessary for your platform, but I
> would suggest we keep the core (from the declaration of struct
> meson_mhu_link to this point) in sync with arm_mhu.c

Yes, I'll keep them as synced as possible.

>> +static int meson_mhu_probe(struct platform_device *pdev)
>> +{
>> +       int i, err;
>> +       struct meson_mhu *mhu;
>> +       struct device *dev = &pdev->dev;
>> +       struct resource *res;
>> +       int meson_mhu_reg[MHU_CHANS] = {MHU_LP_OFFSET, MHU_HP_OFFSET};
>> +
>> +       /* Allocate memory for device */
>> +       mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
>> +       if (!mhu)
>> +               return -ENOMEM;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       mhu->base = devm_ioremap_resource(dev, res);
>> +       if (IS_ERR(mhu->base)) {
>> +               dev_err(dev, "ioremap failed\n");
>> +               return PTR_ERR(mhu->base);
>> +       }
>> +
>> +       for (i = 0; i < MHU_CHANS; i++) {
>> +               mhu->chan[i].con_priv = &mhu->mlink[i];
>> +               mhu->mlink[i].irq = platform_get_irq(pdev, i);
>> +               if (mhu->mlink[i].irq < 0) {
>> +                       dev_err(dev, "failed to get irq%d\n", i);
>> +                       return mhu->mlink[i].irq;
>> +               }
>> +               mhu->mlink[i].rx_reg = mhu->base + meson_mhu_reg[i];
>> +               mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
>> +       }
>> +
>> +       mhu->mbox.dev = dev;
>> +       mhu->mbox.chans = &mhu->chan[0];
>> +       mhu->mbox.num_chans = MHU_CHANS;
>> +       mhu->mbox.ops = &meson_mhu_ops;
>> +       mhu->mbox.txdone_irq = false;
>> +       mhu->mbox.txdone_poll = true;
>> +       mhu->mbox.txpoll_period = 1;
>> +
>> +       platform_set_drvdata(pdev, mhu);
>> +
>> +       err = mbox_controller_register(&mhu->mbox);
>> +       if (err) {
>> +               dev_err(dev, "Failed to register mailboxes %d\n", err);
>> +               return err;
>> +       }
>> +
>> +       dev_info(dev, "Meson MHU Mailbox registered\n");
>> +       return 0;
>> +}
>> +
>> +static int meson_mhu_remove(struct platform_device *pdev)
>> +{
>> +       struct meson_mhu *mhu = platform_get_drvdata(pdev);
>> +
>> +       mbox_controller_unregister(&mhu->mbox);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct of_device_id meson_mhu_dt_ids[] = {
>> +       { .compatible = "amlogic,meson-gxbb-mhu", },
>> +       { /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, meson_mhu_ids);
>> +
>> +static struct platform_driver meson_wdt_driver = {
>> +       .probe  = meson_mhu_probe,
>> +       .remove = meson_mhu_remove,
>> +       .driver = {
>> +               .name = "meson-mhu",
>> +               .of_match_table = meson_mhu_dt_ids,
>> +       },
>> +};
>> +
>> +module_platform_driver(meson_wdt_driver);
>> +
> wdt as in watchdog-timer from drivers/watchdog/meson_wdt.c ? :)

Sorry, it was a leftover from another pdev driver, but the freshly
pushed meson_gxbb_wdt !

> Thanks.

Thanks for the feedback,
Neil

Re: [RFC PATCH v2 1/9] mailbox: Add Amlogic Meson Message-Handling-Unit

[Jassi Brar]

On Wed, Jul 6, 2016 at 6:47 PM, Neil Armstrong <narmstrong@baylibre.com> wrote:
> 2016-07-04 17:38 GMT+02:00 Jassi Brar <jassisinghbrar@gmail.com>:
>> On Tue, Jun 21, 2016 at 3:32 PM, Neil Armstrong <narmstrong@baylibre.com> wrote:
>>> Add Amlogic Meson SoCs Message-Handling-Unit as mailbox controller
>>> with 2 independent channels/links to communicate with a remote processor.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/mailbox/Makefile    |   2 +
>>>  drivers/mailbox/meson_mhu.c | 199 ++++++++++++++++++++++++++++++++++++++++++++
>>>
>> Can we call it pdev_mhu.c or similar so that some other platform using
>> the MHU as a platform_device wouldn't have to embarrassingly call it
>> 'Meson's MHU'?  And also the replace meson with that prefix in the
>> code.
>
> Yes, it may deserve a more generic naming, but pdev_mhu is not good
> looking at all !
> What about platform_mhu ?
>
OK

[RFC PATCH v2 2/9] dt-bindings: mailbox: Add Amlogic Meson MHU Bindings

[Neil Armstrong]

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 .../devicetree/bindings/mailbox/meson-mhu.txt      | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/meson-mhu.txt

diff --git a/Documentation/devicetree/bindings/mailbox/meson-mhu.txt b/Documentation/devicetree/bindings/mailbox/meson-mhu.txt
new file mode 100644
index 0000000..4a80b44
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/meson-mhu.txt
@@ -0,0 +1,33 @@
+Amlogic Meson MHU Mailbox Driver
+================================
+
+The Amlogic's Meson SoCs Message-Handling-Unit (MHU) is a mailbox controller
+that has 2 independent channels/links to communicate with remote processor(s).
+MHU links are hardwired on a platform. A link raises interrupt for any
+received data. However, there is no specified way of knowing if the sent
+data has been read by the remote. This driver assumes the sender polls
+STAT register and the remote clears it after having read the data.
+
+Mailbox Device Node:
+====================
+
+Required properties:
+--------------------
+- compatible:		Shall be "amlogic,meson-gxbb-mhu"
+- reg:			Contains the mailbox register address range (base
+			address and length)
+- #mbox-cells		Shall be 1 - the index of the channel needed.
+- interrupts:		Contains the interrupt information corresponding to
+			each of the 2 links of MHU.
+
+Example:
+--------
+
+	mailbox: mailbox@c883c400 {
+		#mbox-cells = <1>;
+		compatible = "amlogic,meson-gxbb-mhu";
+		reg = <0 0xc883c400 0 0x4c>;
+		interrupts = <0 209 IRQ_TYPE_EDGE_RISING>,
+			   <0 210 IRQ_TYPE_EDGE_RISING>;
+		#mbox-cells = <1>;
+	};
-- 
2.7.0

[RFC PATCH v2 3/9] ARM64: dts: meson-gxbb: Add Meson MHU Node

[Neil Armstrong]

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index 806b903..77381d0 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -315,6 +315,14 @@
 			#address-cells = <2>;
 			#size-cells = <2>;
 			ranges = <0x0 0x0 0x0 0xc883c000 0x0 0x2000>;
+
+			mailbox: mailbox@400 {
+				compatible = "amlogic,meson-gxbb-mhu";
+				reg = <0 0x400 0 0x4c>;
+				interrupts = <0 209 IRQ_TYPE_EDGE_RISING>,
+					     <0 210 IRQ_TYPE_EDGE_RISING>;
+				#mbox-cells = <1>;
+			 };
 		};
 
 		apb: apb@d0000000 {
-- 
2.7.0

[RFC PATCH v2 4/9] firmware: Add a SCPI registry to handle multiple implementations

[Neil Armstrong]

Add a thin registry layer to store the current scpi_ops pointer out of the
arm_scpi.c location.
Add a register/unregister and devres managed unregister calls, and their
static inline disabled stubs.
Add the SCPI_FW config to enable the registry layer build.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/Kconfig      |  4 ++
 drivers/firmware/Makefile     |  1 +
 drivers/firmware/scpi.c       | 94 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/scpi_protocol.h | 15 ++++++-
 4 files changed, 113 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/scpi.c

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 6664f11..95b01f4 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -8,9 +8,13 @@ menu "Firmware Drivers"
 config ARM_PSCI_FW
 	bool
 
+config SCPI_FW
+	bool
+
 config ARM_SCPI_PROTOCOL
 	tristate "ARM System Control and Power Interface (SCPI) Message Protocol"
 	depends on ARM_MHU
+	select SCPI_FW
 	help
 	  System Control and Power Interface (SCPI) Message Protocol is
 	  defined for the purpose of communication between the Application
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 474bada..b697462 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -2,6 +2,7 @@
 # Makefile for the linux kernel.
 #
 obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
+obj-$(CONFIG_SCPI_FW)		+= scpi.o
 obj-$(CONFIG_ARM_SCPI_PROTOCOL)	+= arm_scpi.o
 obj-$(CONFIG_DMI)		+= dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)		+= dmi-sysfs.o
diff --git a/drivers/firmware/scpi.c b/drivers/firmware/scpi.c
new file mode 100644
index 0000000..87a559a
--- /dev/null
+++ b/drivers/firmware/scpi.c
@@ -0,0 +1,94 @@
+/*
+ * System Control and Power Interface (SCPI) Message Protocol registry
+ *
+ * SCPI Message Protocol is used between the System Control Processor(SCP)
+ * and the Application Processors(AP). The Message Handling Unit(MHU)
+ * provides a mechanism for inter-processor communication between SCP's
+ * Cortex M3 and AP.
+ *
+ * SCP offers control and management of the core/cluster power states,
+ * various power domain DVFS including the core/cluster, certain system
+ * clocks configuration, thermal sensors and many others.
+ *
+ * Copyright (C) 2015 ARM Ltd.
+ * Copyright (C) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/of.h>
+#include <linux/scpi_protocol.h>
+#include <linux/spinlock.h>
+
+static struct scpi_ops *g_ops;
+
+struct scpi_ops *get_scpi_ops(void)
+{
+	return g_ops;
+}
+EXPORT_SYMBOL_GPL(get_scpi_ops);
+
+int scpi_ops_register(struct scpi_ops *ops)
+{
+	if (!ops)
+		return -EINVAL;
+
+	if (g_ops)
+		return -EEXIST;
+
+	g_ops = ops;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(scpi_ops_register);
+
+void scpi_ops_unregister(struct scpi_ops *ops)
+{
+	if (g_ops == ops)
+		g_ops = NULL;
+}
+EXPORT_SYMBOL_GPL(scpi_ops_unregister);
+
+static void devm_scpi_ops_unregister(struct device *dev, void *res)
+{
+	scpi_ops_unregister(*(struct scpi_ops **)res);
+}
+
+int devm_scpi_ops_register(struct device *dev,
+				struct scpi_ops *ops)
+{
+	struct scpi_ops **rcops;
+	int ret;
+
+	rcops = devres_alloc(devm_scpi_ops_unregister, sizeof(*ops),
+			     GFP_KERNEL);
+	if (!rcops)
+		return -ENOMEM;
+
+	ret = scpi_ops_register(ops);
+	if (!ret) {
+		*rcops = ops;
+		devres_add(dev, rcops);
+	} else
+		devres_free(rcops);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_scpi_ops_register);
diff --git a/include/linux/scpi_protocol.h b/include/linux/scpi_protocol.h
index 35de50a..38c15d1 100644
--- a/include/linux/scpi_protocol.h
+++ b/include/linux/scpi_protocol.h
@@ -72,8 +72,21 @@ struct scpi_ops {
 	int (*sensor_get_value)(u16, u64 *);
 };
 
-#if IS_REACHABLE(CONFIG_ARM_SCPI_PROTOCOL)
+#if IS_REACHABLE(CONFIG_SCPI_FW)
 struct scpi_ops *get_scpi_ops(void);
+int scpi_ops_register(struct scpi_ops *drv);
+void scpi_ops_unregister(struct scpi_ops *drv);
+int devm_scpi_ops_register(struct device *dev, struct scpi_ops *drv);
 #else
 static inline struct scpi_ops *get_scpi_ops(void) { return NULL; }
+
+static inline int scpi_ops_register(struct scpi_ops *drv) { return -ENOTSUPP; }
+
+static inline void scpi_ops_unregister(struct scpi_ops *drv) { }
+
+static inline int devm_scpi_ops_register(struct device *dev,
+					 struct scpi_ops *drv)
+{
+	return -ENOTSUPP;
+}
 #endif
-- 
2.7.0

[RFC PATCH v2 5/9] firmware: scpi: Switch arm_scpi to use new registry

[Neil Armstrong]

Change the arm_scpi.c to use the registry layer instead of returning it's
context ->ops pointer.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/arm_scpi.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 7e3e595..ba6bc53 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -162,7 +162,6 @@ struct scpi_drvinfo {
 	u32 firmware_version;
 	int num_chans;
 	atomic_t next_chan;
-	struct scpi_ops *scpi_ops;
 	struct scpi_chan *channels;
 	struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
 };
@@ -526,7 +525,7 @@ static int scpi_sensor_get_info(u16 sensor_id, struct scpi_sensor_info *info)
 	return ret;
 }
 
-int scpi_sensor_get_value(u16 sensor, u64 *val)
+static int scpi_sensor_get_value(u16 sensor, u64 *val)
 {
 	__le16 id = cpu_to_le16(sensor);
 	struct sensor_value buf;
@@ -554,12 +553,6 @@ static struct scpi_ops scpi_ops = {
 	.sensor_get_value = scpi_sensor_get_value,
 };
 
-struct scpi_ops *get_scpi_ops(void)
-{
-	return scpi_info ? scpi_info->scpi_ops : NULL;
-}
-EXPORT_SYMBOL_GPL(get_scpi_ops);
-
 static int scpi_init_versions(struct scpi_drvinfo *info)
 {
 	int ret;
@@ -743,7 +736,10 @@ err:
 		  FW_REV_MAJOR(scpi_info->firmware_version),
 		  FW_REV_MINOR(scpi_info->firmware_version),
 		  FW_REV_PATCH(scpi_info->firmware_version));
-	scpi_info->scpi_ops = &scpi_ops;
+
+	ret = devm_scpi_ops_register(dev, &scpi_ops);
+	if (ret)
+		return ret;
 
 	ret = sysfs_create_groups(&dev->kobj, versions_groups);
 	if (ret)
-- 
2.7.0

[RFC PATCH v2 6/9] firmware: Add legacy SCPI protocol driver

[Neil Armstrong]

Add legacy SCPI driver based on the latest SCPI driver but modified to behave
like an earlier technology preview SCPI implementation that at least the
Amlogic GXBB ARMv8 based platform uses in it's SCP firmware implementation.

The main differences between the mainline, public and recommended SCPI
implementation are :
 - virtual channels is not implemented
 - command word is passed by the MHU instead of the virtual channel ID
 - uses "sender id" in the command word for each commands groups
 - payload size shift in command word is different
 - command word is not in SRAM, so command queuing is not possible
 - command indexes are different
 - command data structures differs
 - commands are redirected to low or high priority channels by their indexes, 
   so round-robin redirection is not possible

A clear disclaimer is added to make it clear this implementation should not
be used for new products and is only here to support already released SoCs.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/Kconfig       |  20 ++
 drivers/firmware/Makefile      |   1 +
 drivers/firmware/legacy_scpi.c | 644 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 665 insertions(+)
 create mode 100644 drivers/firmware/legacy_scpi.c

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 95b01f4..b9c2a33 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -31,6 +31,26 @@ config ARM_SCPI_PROTOCOL
 	  This protocol library provides interface for all the client drivers
 	  making use of the features offered by the SCP.
 
+config LEGACY_SCPI_PROTOCOL
+	bool "Legacy System Control and Power Interface (SCPI) Message Protocol"
+	default y if ARCH_MESON
+	select ARM_SCPI_FW
+	help
+	  System Control and Power Interface (SCPI) Message Protocol is
+	  defined for the purpose of communication between the Application
+	  Cores(AP) and the System Control Processor(SCP). The MHU peripheral
+	  provides a mechanism for inter-processor communication between SCP
+	  and AP.
+
+	  SCP controls most of the power managament on the Application
+	  Processors. It offers control and management of: the core/cluster
+	  power states, various power domain DVFS including the core/cluster,
+	  certain system clocks configuration, thermal sensors and many
+	  others.
+
+	  This protocol library provides interface for all the client drivers
+	  making use of the features offered by the legacy SCP protocol.
+
 config EDD
 	tristate "BIOS Enhanced Disk Drive calls determine boot disk"
 	depends on X86
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index b697462..c2cac9c 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -4,6 +4,7 @@
 obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
 obj-$(CONFIG_SCPI_FW)		+= scpi.o
 obj-$(CONFIG_ARM_SCPI_PROTOCOL)	+= arm_scpi.o
+obj-$(CONFIG_LEGACY_SCPI_PROTOCOL) += legacy_scpi.o
 obj-$(CONFIG_DMI)		+= dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)		+= dmi-sysfs.o
 obj-$(CONFIG_EDD)		+= edd.o
diff --git a/drivers/firmware/legacy_scpi.c b/drivers/firmware/legacy_scpi.c
new file mode 100644
index 0000000..4bd3ff7
--- /dev/null
+++ b/drivers/firmware/legacy_scpi.c
@@ -0,0 +1,644 @@
+/*
+ * Legacy System Control and Power Interface (SCPI) Message Protocol driver
+ *
+ * SCPI Message Protocol is used between the System Control Processor(SCP)
+ * and the Application Processors(AP). The Message Handling Unit(MHU)
+ * provides a mechanism for inter-processor communication between SCP's
+ * Cortex M3 and AP.
+ *
+ * SCP offers control and management of the core/cluster power states,
+ * various power domain DVFS including the core/cluster, certain system
+ * clocks configuration, thermal sensors and many others.
+ *
+ * Copyright (C) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * Heavily based on arm_scpi.c from :
+ * Copyright (C) 2015 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * DISCLAIMER
+ *
+ * This SCPI implementation is based on a technology preview release
+ * and new ARMv8 SoCs implementations should use the standard SCPI
+ * implementation as defined in the ARM DUI 0922G and implemented
+ * in the arm_scpi.c driver.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/bitmap.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/printk.h>
+#include <linux/scpi_protocol.h>
+#include <linux/slab.h>
+#include <linux/sort.h>
+#include <linux/spinlock.h>
+
+#define CMD_ID_SHIFT		0
+#define CMD_ID_MASK		0x7f
+#define CMD_SENDER_ID_SHIFT	8
+#define CMD_SENDER_ID_MASK	0xff
+#define CMD_DATA_SIZE_SHIFT	20
+#define CMD_DATA_SIZE_MASK	0x1ff
+#define PACK_SCPI_CMD(cmd_id, sender, tx_sz)				\
+	((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) |			\
+	(((sender) & CMD_SENDER_ID_MASK) << CMD_SENDER_ID_SHIFT) |	\
+	(((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
+
+#define CMD_SIZE(cmd)	(((cmd) >> CMD_DATA_SIZE_SHIFT) & CMD_DATA_SIZE_MASK)
+#define CMD_UNIQ_MASK	(CMD_TOKEN_ID_MASK << CMD_TOKEN_ID_SHIFT | CMD_ID_MASK)
+#define CMD_XTRACT_UNIQ(cmd)	((cmd) & CMD_UNIQ_MASK)
+
+#define MAX_DVFS_DOMAINS	3
+#define MAX_DVFS_OPPS		16
+#define DVFS_LATENCY(hdr)	(le32_to_cpu(hdr) >> 16)
+#define DVFS_OPP_COUNT(hdr)	((le32_to_cpu(hdr) >> 8) & 0xff)
+
+#define MAX_RX_TIMEOUT          (msecs_to_jiffies(30))
+
+enum legacy_scpi_error_codes {
+	SCPI_SUCCESS = 0, /* Success */
+	SCPI_ERR_PARAM = 1, /* Invalid parameter(s) */
+	SCPI_ERR_ALIGN = 2, /* Invalid alignment */
+	SCPI_ERR_SIZE = 3, /* Invalid size */
+	SCPI_ERR_HANDLER = 4, /* Invalid handler/callback */
+	SCPI_ERR_ACCESS = 5, /* Invalid access/permission denied */
+	SCPI_ERR_RANGE = 6, /* Value out of range */
+	SCPI_ERR_TIMEOUT = 7, /* Timeout has occurred */
+	SCPI_ERR_NOMEM = 8, /* Invalid memory area or pointer */
+	SCPI_ERR_PWRSTATE = 9, /* Invalid power state */
+	SCPI_ERR_SUPPORT = 10, /* Not supported or disabled */
+	SCPI_ERR_DEVICE = 11, /* Device error */
+	SCPI_ERR_BUSY = 12, /* Device busy */
+	SCPI_ERR_MAX
+};
+
+enum legacy_scpi_client_id {
+	SCPI_CL_NONE,
+	SCPI_CL_CLOCKS,
+	SCPI_CL_DVFS,
+	SCPI_CL_POWER,
+	SCPI_CL_THERMAL,
+	SCPI_CL_REMOTE,
+	SCPI_CL_LED_TIMER,
+	SCPI_MAX,
+};
+
+enum legacy_scpi_std_cmd {
+	SCPI_CMD_INVALID		= 0x00,
+	SCPI_CMD_SCPI_READY		= 0x01,
+	SCPI_CMD_SCPI_CAPABILITIES	= 0x02,
+	SCPI_CMD_EVENT			= 0x03,
+	SCPI_CMD_SET_CSS_PWR_STATE	= 0x04,
+	SCPI_CMD_GET_CSS_PWR_STATE	= 0x05,
+	SCPI_CMD_CFG_PWR_STATE_STAT	= 0x06,
+	SCPI_CMD_GET_PWR_STATE_STAT	= 0x07,
+	SCPI_CMD_SYS_PWR_STATE		= 0x08,
+	SCPI_CMD_L2_READY		= 0x09,
+	SCPI_CMD_SET_AP_TIMER		= 0x0a,
+	SCPI_CMD_CANCEL_AP_TIME		= 0x0b,
+	SCPI_CMD_DVFS_CAPABILITIES	= 0x0c,
+	SCPI_CMD_GET_DVFS_INFO		= 0x0d,
+	SCPI_CMD_SET_DVFS		= 0x0e,
+	SCPI_CMD_GET_DVFS		= 0x0f,
+	SCPI_CMD_GET_DVFS_STAT		= 0x10,
+	SCPI_CMD_SET_RTC		= 0x11,
+	SCPI_CMD_GET_RTC		= 0x12,
+	SCPI_CMD_CLOCK_CAPABILITIES	= 0x13,
+	SCPI_CMD_SET_CLOCK_INDEX	= 0x14,
+	SCPI_CMD_SET_CLOCK_VALUE	= 0x15,
+	SCPI_CMD_GET_CLOCK_VALUE	= 0x16,
+	SCPI_CMD_PSU_CAPABILITIES	= 0x17,
+	SCPI_CMD_SET_PSU		= 0x18,
+	SCPI_CMD_GET_PSU		= 0x19,
+	SCPI_CMD_SENSOR_CAPABILITIES	= 0x1a,
+	SCPI_CMD_SENSOR_INFO		= 0x1b,
+	SCPI_CMD_SENSOR_VALUE		= 0x1c,
+	SCPI_CMD_SENSOR_CFG_PERIODIC	= 0x1d,
+	SCPI_CMD_SENSOR_CFG_BOUNDS	= 0x1e,
+	SCPI_CMD_SENSOR_ASYNC_VALUE	= 0x1f,
+	SCPI_CMD_COUNT
+};
+
+struct legacy_scpi_xfer {
+	u32 cmd;
+	u32 status;
+	const void *tx_buf;
+	void *rx_buf;
+	unsigned int tx_len;
+	unsigned int rx_len;
+	struct completion done;
+};
+
+struct legacy_scpi_chan {
+	struct mbox_client cl;
+	struct mbox_chan *chan;
+	void __iomem *tx_payload;
+	void __iomem *rx_payload;
+	spinlock_t rx_lock; /* locking for the rx pending list */
+	struct mutex xfers_lock;
+	struct legacy_scpi_xfer t;
+};
+
+struct legacy_scpi_drvinfo {
+	int num_chans;
+	struct legacy_scpi_chan *channels;
+	struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
+};
+
+/*
+ * The SCP firmware only executes in little-endian mode, so any buffers
+ * shared through SCPI should have their contents converted to little-endian
+ */
+struct legacy_scpi_shared_mem {
+	__le32 status;
+	u8 payload[0];
+} __packed;
+
+struct scp_capabilities {
+	__le32 protocol_version;
+	__le32 event_version;
+	__le32 platform_version;
+	__le32 commands[4];
+} __packed;
+
+struct clk_get_info {
+	__le16 id;
+	__le16 flags;
+	__le32 min_rate;
+	__le32 max_rate;
+	u8 name[20];
+} __packed;
+
+struct clk_get_value {
+	__le32 rate;
+} __packed;
+
+struct clk_set_value {
+	__le32 rate;
+	__le16 id;
+	__le16 reserved;
+} __packed;
+
+struct dvfs_info {
+	__le32 header;
+	struct {
+		__le32 freq;
+		__le32 m_volt;
+	} opps[MAX_DVFS_OPPS];
+} __packed;
+
+struct dvfs_get {
+	u8 index;
+} __packed;
+
+struct dvfs_set {
+	u8 domain;
+	u8 index;
+} __packed;
+
+struct sensor_capabilities {
+	__le16 sensors;
+} __packed;
+
+struct sensor_info {
+	__le16 sensor_id;
+	u8 class;
+	u8 trigger_type;
+	char name[20];
+};
+
+struct sensor_value {
+	__le32 val;
+} __packed;
+
+static struct legacy_scpi_drvinfo *legacy_scpi_info;
+
+static int legacy_scpi_linux_errmap[SCPI_ERR_MAX] = {
+	/* better than switch case as long as return value is continuous */
+	0, /* SCPI_SUCCESS */
+	-EINVAL, /* SCPI_ERR_PARAM */
+	-ENOEXEC, /* SCPI_ERR_ALIGN */
+	-EMSGSIZE, /* SCPI_ERR_SIZE */
+	-EINVAL, /* SCPI_ERR_HANDLER */
+	-EACCES, /* SCPI_ERR_ACCESS */
+	-ERANGE, /* SCPI_ERR_RANGE */
+	-ETIMEDOUT, /* SCPI_ERR_TIMEOUT */
+	-ENOMEM, /* SCPI_ERR_NOMEM */
+	-EINVAL, /* SCPI_ERR_PWRSTATE */
+	-EOPNOTSUPP, /* SCPI_ERR_SUPPORT */
+	-EIO, /* SCPI_ERR_DEVICE */
+	-EBUSY, /* SCPI_ERR_BUSY */
+};
+
+static inline int legacy_scpi_to_linux_errno(int errno)
+{
+	if (errno >= SCPI_SUCCESS && errno < SCPI_ERR_MAX)
+		return legacy_scpi_linux_errmap[errno];
+	return -EIO;
+}
+
+static void legacy_scpi_handle_remote_msg(struct mbox_client *c, void *msg)
+{
+	struct legacy_scpi_chan *ch = container_of(c, struct legacy_scpi_chan, cl);
+	struct legacy_scpi_shared_mem *mem = ch->rx_payload;
+	unsigned long flags;
+	unsigned int len;
+
+	spin_lock_irqsave(&ch->rx_lock, flags);
+
+	len = ch->t.rx_len;
+
+	ch->t.status = le32_to_cpu(mem->status);
+	if (len)
+		memcpy_fromio(ch->t.rx_buf, mem->payload, len);
+
+	complete(&ch->t.done);
+
+	spin_unlock_irqrestore(&ch->rx_lock, flags);
+}
+
+static void legacy_scpi_tx_prepare(struct mbox_client *c, void *msg)
+{
+	struct legacy_scpi_chan *ch = container_of(c, struct legacy_scpi_chan, cl);
+
+	if (ch->t.tx_buf && ch->t.tx_len)
+		memcpy_toio(ch->tx_payload, ch->t.tx_buf, ch->t.tx_len);
+}
+
+static int high_priority_cmds[] = {
+	SCPI_CMD_GET_CSS_PWR_STATE,
+	SCPI_CMD_CFG_PWR_STATE_STAT,
+	SCPI_CMD_GET_PWR_STATE_STAT,
+	SCPI_CMD_SET_DVFS,
+	SCPI_CMD_GET_DVFS,
+	SCPI_CMD_SET_RTC,
+	SCPI_CMD_GET_RTC,
+	SCPI_CMD_SET_CLOCK_INDEX,
+	SCPI_CMD_SET_CLOCK_VALUE,
+	SCPI_CMD_GET_CLOCK_VALUE,
+	SCPI_CMD_SET_PSU,
+	SCPI_CMD_GET_PSU,
+	SCPI_CMD_SENSOR_CFG_PERIODIC,
+	SCPI_CMD_SENSOR_CFG_BOUNDS,
+};
+
+static int legacy_scpi_get_chan(u8 cmd)
+{
+	int idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(high_priority_cmds); idx++)
+		if (cmd == high_priority_cmds[idx])
+			return 1;
+
+	return 0;
+}
+
+static int legacy_scpi_send_message(u8 cmd, unsigned int sender,
+				void *tx_buf, unsigned int tx_len,
+				void *rx_buf, unsigned int rx_len)
+{
+	int ret;
+	u8 chan;
+	struct legacy_scpi_xfer *msg;
+	struct legacy_scpi_chan *legacy_scpi_chan;
+
+	chan = legacy_scpi_get_chan(cmd);
+	legacy_scpi_chan = legacy_scpi_info->channels + chan;
+
+	msg = &legacy_scpi_chan->t;
+
+	msg->cmd = PACK_SCPI_CMD(cmd, sender, tx_len);
+	msg->tx_buf = tx_buf;
+	msg->tx_len = tx_len;
+	msg->rx_buf = rx_buf;
+	msg->rx_len = rx_len;
+	init_completion(&msg->done);
+
+	ret = mbox_send_message(legacy_scpi_chan->chan, &msg->cmd);
+	if (ret < 0)
+		goto out;
+
+	if (!wait_for_completion_timeout(&msg->done, MAX_RX_TIMEOUT))
+		ret = -ETIMEDOUT;
+	else
+		/* first status word */
+		ret = msg->status;
+out:
+	/* SCPI error codes > 0, translate them to Linux scale*/
+	return ret > 0 ? legacy_scpi_to_linux_errno(ret) : ret;
+}
+
+static u32 legacy_scpi_get_version(void)
+{
+	/* TOFIX */
+	return 0;
+}
+
+static unsigned long legacy_scpi_clk_get_val(u16 clk_id)
+{
+	int ret;
+	struct clk_get_value clk;
+	__le16 le_clk_id = cpu_to_le16(clk_id);
+
+	ret = legacy_scpi_send_message(SCPI_CMD_GET_CLOCK_VALUE, SCPI_CL_CLOCKS,
+				&le_clk_id, sizeof(le_clk_id),
+				&clk, sizeof(clk));
+
+	return ret ? ret : le32_to_cpu(clk.rate);
+}
+
+static int legacy_scpi_clk_set_val(u16 clk_id, unsigned long rate)
+{
+	int stat;
+	struct clk_set_value clk = {
+		.id = cpu_to_le16(clk_id),
+		.rate = cpu_to_le32(rate)
+	};
+
+	return legacy_scpi_send_message(SCPI_CMD_SET_CLOCK_VALUE, SCPI_CL_CLOCKS,
+				&clk, sizeof(clk),
+				&stat, sizeof(stat));
+}
+
+static int legacy_scpi_dvfs_get_idx(u8 domain)
+{
+	int ret;
+	struct dvfs_get dvfs;
+
+	ret = legacy_scpi_send_message(SCPI_CMD_GET_DVFS, SCPI_CL_DVFS,
+				&domain, sizeof(domain),
+				&dvfs, sizeof(dvfs));
+
+	return ret ? ret : dvfs.index;
+}
+
+static int legacy_scpi_dvfs_set_idx(u8 domain, u8 index)
+{
+	int stat;
+	struct dvfs_set dvfs = {domain, index};
+
+	return legacy_scpi_send_message(SCPI_CMD_SET_DVFS, SCPI_CL_DVFS,
+				&dvfs, sizeof(dvfs),
+				&stat, sizeof(stat));
+}
+
+static struct scpi_dvfs_info *legacy_scpi_dvfs_get_info(u8 domain)
+{
+	struct scpi_dvfs_info *info;
+	struct scpi_opp *opp;
+	struct dvfs_info buf;
+	int ret, i;
+
+	if (domain >= MAX_DVFS_DOMAINS)
+		return ERR_PTR(-EINVAL);
+
+	if (legacy_scpi_info->dvfs[domain])	/* data already populated */
+		return legacy_scpi_info->dvfs[domain];
+
+	ret = legacy_scpi_send_message(SCPI_CMD_GET_DVFS_INFO, SCPI_CL_DVFS,
+				&domain, sizeof(domain),
+				&buf, sizeof(buf));
+
+	if (ret)
+		return ERR_PTR(ret);
+
+	info = kmalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
+
+	info->count = DVFS_OPP_COUNT(buf.header);
+	info->latency = DVFS_LATENCY(buf.header) * 1000; /* uS to nS */
+
+	info->opps = kcalloc(info->count, sizeof(*opp), GFP_KERNEL);
+	if (!info->opps) {
+		kfree(info);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	for (i = 0, opp = info->opps; i < info->count; i++, opp++) {
+		opp->freq = le32_to_cpu(buf.opps[i].freq);
+		opp->m_volt = le32_to_cpu(buf.opps[i].m_volt);
+	}
+
+	legacy_scpi_info->dvfs[domain] = info;
+	return info;
+}
+
+static int legacy_scpi_sensor_get_capability(u16 *sensors)
+{
+	struct sensor_capabilities cap_buf;
+	int ret;
+
+	ret = legacy_scpi_send_message(SCPI_CMD_SENSOR_CAPABILITIES,
+				SCPI_CL_THERMAL, NULL, 0, &cap_buf,
+				sizeof(cap_buf));
+	if (!ret)
+		*sensors = le16_to_cpu(cap_buf.sensors);
+
+	return ret;
+}
+
+static int legacy_scpi_sensor_get_info(u16 sensor_id,
+				     struct scpi_sensor_info *info)
+{
+	__le16 id = cpu_to_le16(sensor_id);
+	struct sensor_info _info;
+	int ret;
+
+	ret = legacy_scpi_send_message(SCPI_CMD_SENSOR_INFO, SCPI_CL_THERMAL,
+				&id, sizeof(id),
+				&_info, sizeof(_info));
+	if (!ret) {
+		memcpy(info, &_info, sizeof(*info));
+		info->sensor_id = le16_to_cpu(_info.sensor_id);
+	}
+
+	return ret;
+}
+
+static int legacy_scpi_sensor_get_value(u16 sensor, u64 *val)
+{
+	__le16 id = cpu_to_le16(sensor);
+	struct sensor_value buf;
+	int ret;
+
+	ret = legacy_scpi_send_message(SCPI_CMD_SENSOR_VALUE, SCPI_CL_THERMAL,
+				&id, sizeof(id),
+				&buf, sizeof(buf));
+	if (!ret)
+		*val = (u64)le32_to_cpu(buf.val);
+
+	return ret;
+}
+
+static struct scpi_ops legacy_scpi_ops = {
+	.get_version = legacy_scpi_get_version,
+	.clk_get_val = legacy_scpi_clk_get_val,
+	.clk_set_val = legacy_scpi_clk_set_val,
+	.dvfs_get_idx = legacy_scpi_dvfs_get_idx,
+	.dvfs_set_idx = legacy_scpi_dvfs_set_idx,
+	.dvfs_get_info = legacy_scpi_dvfs_get_info,
+	.sensor_get_capability = legacy_scpi_sensor_get_capability,
+	.sensor_get_info = legacy_scpi_sensor_get_info,
+	.sensor_get_value = legacy_scpi_sensor_get_value,
+};
+
+static void legacy_scpi_free_channels(struct device *dev,
+				struct legacy_scpi_chan *pchan, int count)
+{
+	int i;
+
+	for (i = 0; i < count && pchan->chan; i++, pchan++)
+		mbox_free_channel(pchan->chan);
+}
+
+static int legacy_scpi_remove(struct platform_device *pdev)
+{
+	int i;
+	struct device *dev = &pdev->dev;
+	struct legacy_scpi_drvinfo *info = platform_get_drvdata(pdev);
+
+	/* stop exporting SCPI ops */
+	legacy_scpi_info = NULL;
+
+	of_platform_depopulate(dev);
+	legacy_scpi_free_channels(dev, info->channels, info->num_chans);
+	platform_set_drvdata(pdev, NULL);
+
+	for (i = 0; i < MAX_DVFS_DOMAINS && info->dvfs[i]; i++) {
+		kfree(info->dvfs[i]->opps);
+		kfree(info->dvfs[i]);
+	}
+
+	return 0;
+}
+
+static int legacy_scpi_probe(struct platform_device *pdev)
+{
+	int count, idx, ret;
+	struct resource res;
+	struct legacy_scpi_chan *legacy_scpi_chan;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	legacy_scpi_info = devm_kzalloc(dev, sizeof(*legacy_scpi_info), GFP_KERNEL);
+	if (!legacy_scpi_info)
+		return -ENOMEM;
+
+	count = of_count_phandle_with_args(np, "mboxes", "#mbox-cells");
+	if (count < 0) {
+		dev_err(dev, "no mboxes property in '%s'\n", np->full_name);
+		return -ENODEV;
+	}
+
+	legacy_scpi_chan = devm_kcalloc(dev, count,
+				sizeof(*legacy_scpi_chan), GFP_KERNEL);
+	if (!legacy_scpi_chan)
+		return -ENOMEM;
+
+	for (idx = 0; idx < count; idx++) {
+		resource_size_t size;
+		struct legacy_scpi_chan *pchan = legacy_scpi_chan + idx;
+		struct mbox_client *cl = &pchan->cl;
+		struct device_node *shmem = of_parse_phandle(np, "shmem", idx);
+
+		if (of_address_to_resource(shmem, 0, &res)) {
+			dev_err(dev, "failed to get SCPI payload mem resource\n");
+			ret = -EINVAL;
+			goto err;
+		}
+
+		size = resource_size(&res);
+
+		dev_dbg(dev, "chan%d: sram start=%llx size=%lld\n",
+				idx, res.start, size);
+
+		pchan->rx_payload = devm_ioremap(dev, res.start, size);
+		if (!pchan->rx_payload) {
+			dev_err(dev, "failed to ioremap SCPI payload\n");
+			ret = -EADDRNOTAVAIL;
+			goto err;
+		}
+		pchan->tx_payload = pchan->rx_payload + (size >> 1);
+
+		dev_dbg(dev, "chan%d: payload rx=%p tx=%p\n",
+			idx, pchan->rx_payload, pchan->tx_payload);
+
+		cl->dev = dev;
+		cl->rx_callback = legacy_scpi_handle_remote_msg;
+		cl->tx_prepare = legacy_scpi_tx_prepare;
+		cl->tx_block = true;
+		cl->tx_tout = 20;
+		cl->knows_txdone = false; /* controller can't ack */
+
+		spin_lock_init(&pchan->rx_lock);
+		mutex_init(&pchan->xfers_lock);
+
+		pchan->chan = mbox_request_channel(cl, idx);
+		if (!IS_ERR(pchan->chan))
+			continue;
+		else
+			ret = PTR_ERR(pchan->chan);
+err:
+		legacy_scpi_free_channels(dev, legacy_scpi_chan, idx);
+		legacy_scpi_info = NULL;
+		return ret;
+	}
+
+	legacy_scpi_info->channels = legacy_scpi_chan;
+	legacy_scpi_info->num_chans = count;
+	platform_set_drvdata(pdev, legacy_scpi_info);
+
+	ret = devm_scpi_ops_register(dev, &legacy_scpi_ops);
+	if (ret)
+		return ret;
+
+	return of_platform_populate(dev->of_node, NULL, NULL, dev);
+}
+
+static const struct of_device_id legacy_scpi_of_match[] = {
+	{.compatible = "arm,legacy-scpi"},
+	{.compatible = "amlogic,meson-gxbb-scpi"},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, legacy_scpi_of_match);
+
+static struct platform_driver legacy_scpi_driver = {
+	.driver = {
+		.name = "legacy-scpi",
+		.of_match_table = legacy_scpi_of_match,
+	},
+	.probe = legacy_scpi_probe,
+	.remove = legacy_scpi_remove,
+};
+module_platform_driver(legacy_scpi_driver);
+
+MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
+MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
+MODULE_DESCRIPTION("ARM Legacy SCPI mailbox protocol driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.0

Re: [RFC PATCH v2 6/9] firmware: Add legacy SCPI protocol driver

[Sudeep Holla]

On 21/06/16 11:02, Neil Armstrong wrote:
> Add legacy SCPI driver based on the latest SCPI driver but modified to behave
> like an earlier technology preview SCPI implementation that at least the
> Amlogic GXBB ARMv8 based platform uses in it's SCP firmware implementation.
>
> The main differences between the mainline, public and recommended SCPI
> implementation are :
>   - virtual channels is not implemented
>   - command word is passed by the MHU instead of the virtual channel ID
>   - uses "sender id" in the command word for each commands groups
>   - payload size shift in command word is different
>   - command word is not in SRAM, so command queuing is not possible
>   - command indexes are different
>   - command data structures differs
>   - commands are redirected to low or high priority channels by their indexes,
>     so round-robin redirection is not possible

I doubt if that's the case. At-least the original arm scp f/w didn't
check that. Can you please trying sending any commands on any channel ?

>
> A clear disclaimer is added to make it clear this implementation should not
> be used for new products and is only here to support already released SoCs.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>   drivers/firmware/Kconfig       |  20 ++
>   drivers/firmware/Makefile      |   1 +
>   drivers/firmware/legacy_scpi.c | 644 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 665 insertions(+)
>   create mode 100644 drivers/firmware/legacy_scpi.c
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 95b01f4..b9c2a33 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -31,6 +31,26 @@ config ARM_SCPI_PROTOCOL
>   	  This protocol library provides interface for all the client drivers
>   	  making use of the features offered by the SCP.
>
> +config LEGACY_SCPI_PROTOCOL
> +	bool "Legacy System Control and Power Interface (SCPI) Message Protocol"

Do we really need to add another config ? I thought we could just manage
with compatibles.

> +	default y if ARCH_MESON
> +	select ARM_SCPI_FW
> +	help
> +	  System Control and Power Interface (SCPI) Message Protocol is
> +	  defined for the purpose of communication between the Application
> +	  Cores(AP) and the System Control Processor(SCP). The MHU peripheral
> +	  provides a mechanism for inter-processor communication between SCP
> +	  and AP.

[...]

> diff --git a/drivers/firmware/legacy_scpi.c b/drivers/firmware/legacy_scpi.c
> new file mode 100644
> index 0000000..4bd3ff7
> --- /dev/null
> +++ b/drivers/firmware/legacy_scpi.c
> @@ -0,0 +1,644 @@

[...]

> +
> +#define CMD_ID_SHIFT		0
> +#define CMD_ID_MASK		0x7f
> +#define CMD_SENDER_ID_SHIFT	8
> +#define CMD_SENDER_ID_MASK	0xff

Again this is something I introduced in the earlier driver. But from SCP
f/w perspective, it just sends that as is to the sender. I think we
retain token concept as is from the latest driver. Could you check
dropping them and check if f/w makes any assumption about these. It
should not IMO.

> +#define CMD_DATA_SIZE_SHIFT	20
> +#define CMD_DATA_SIZE_MASK	0x1ff
> +#define PACK_SCPI_CMD(cmd_id, sender, tx_sz)				\
> +	((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) |			\
> +	(((sender) & CMD_SENDER_ID_MASK) << CMD_SENDER_ID_SHIFT) |	\
> +	(((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
> +
> +#define CMD_SIZE(cmd)	(((cmd) >> CMD_DATA_SIZE_SHIFT) & CMD_DATA_SIZE_MASK)
> +#define CMD_UNIQ_MASK	(CMD_TOKEN_ID_MASK << CMD_TOKEN_ID_SHIFT | CMD_ID_MASK)
> +#define CMD_XTRACT_UNIQ(cmd)	((cmd) & CMD_UNIQ_MASK)
> +
> +#define MAX_DVFS_DOMAINS	3
> +#define MAX_DVFS_OPPS		16
> +#define DVFS_LATENCY(hdr)	(le32_to_cpu(hdr) >> 16)
> +#define DVFS_OPP_COUNT(hdr)	((le32_to_cpu(hdr) >> 8) & 0xff)
> +
> +#define MAX_RX_TIMEOUT          (msecs_to_jiffies(30))
> +
> +enum legacy_scpi_error_codes {

This along with many other defines are exactly same, not need to
duplicate them.

> +	SCPI_SUCCESS = 0, /* Success */
> +	SCPI_ERR_PARAM = 1, /* Invalid parameter(s) */
> +	SCPI_ERR_ALIGN = 2, /* Invalid alignment */
> +	SCPI_ERR_SIZE = 3, /* Invalid size */
> +	SCPI_ERR_HANDLER = 4, /* Invalid handler/callback */
> +	SCPI_ERR_ACCESS = 5, /* Invalid access/permission denied */
> +	SCPI_ERR_RANGE = 6, /* Value out of range */
> +	SCPI_ERR_TIMEOUT = 7, /* Timeout has occurred */
> +	SCPI_ERR_NOMEM = 8, /* Invalid memory area or pointer */
> +	SCPI_ERR_PWRSTATE = 9, /* Invalid power state */
> +	SCPI_ERR_SUPPORT = 10, /* Not supported or disabled */
> +	SCPI_ERR_DEVICE = 11, /* Device error */
> +	SCPI_ERR_BUSY = 12, /* Device busy */
> +	SCPI_ERR_MAX
> +};
> +
> +enum legacy_scpi_client_id {

Could be removed as mentioned above ?

> +	SCPI_CL_NONE,
> +	SCPI_CL_CLOCKS,
> +	SCPI_CL_DVFS,
> +	SCPI_CL_POWER,
> +	SCPI_CL_THERMAL,
> +	SCPI_CL_REMOTE,
> +	SCPI_CL_LED_TIMER,
> +	SCPI_MAX,
> +};
> +
> +enum legacy_scpi_std_cmd {
> +	SCPI_CMD_INVALID		= 0x00,
> +	SCPI_CMD_SCPI_READY		= 0x01,
> +	SCPI_CMD_SCPI_CAPABILITIES	= 0x02,
> +	SCPI_CMD_EVENT			= 0x03,
> +	SCPI_CMD_SET_CSS_PWR_STATE	= 0x04,
> +	SCPI_CMD_GET_CSS_PWR_STATE	= 0x05,
> +	SCPI_CMD_CFG_PWR_STATE_STAT	= 0x06,
> +	SCPI_CMD_GET_PWR_STATE_STAT	= 0x07,
> +	SCPI_CMD_SYS_PWR_STATE		= 0x08,
> +	SCPI_CMD_L2_READY		= 0x09,
> +	SCPI_CMD_SET_AP_TIMER		= 0x0a,
> +	SCPI_CMD_CANCEL_AP_TIME		= 0x0b,
> +	SCPI_CMD_DVFS_CAPABILITIES	= 0x0c,
> +	SCPI_CMD_GET_DVFS_INFO		= 0x0d,
> +	SCPI_CMD_SET_DVFS		= 0x0e,
> +	SCPI_CMD_GET_DVFS		= 0x0f,
> +	SCPI_CMD_GET_DVFS_STAT		= 0x10,
> +	SCPI_CMD_SET_RTC		= 0x11,
> +	SCPI_CMD_GET_RTC		= 0x12,
> +	SCPI_CMD_CLOCK_CAPABILITIES	= 0x13,
> +	SCPI_CMD_SET_CLOCK_INDEX	= 0x14,
> +	SCPI_CMD_SET_CLOCK_VALUE	= 0x15,
> +	SCPI_CMD_GET_CLOCK_VALUE	= 0x16,
> +	SCPI_CMD_PSU_CAPABILITIES	= 0x17,
> +	SCPI_CMD_SET_PSU		= 0x18,
> +	SCPI_CMD_GET_PSU		= 0x19,
> +	SCPI_CMD_SENSOR_CAPABILITIES	= 0x1a,
> +	SCPI_CMD_SENSOR_INFO		= 0x1b,
> +	SCPI_CMD_SENSOR_VALUE		= 0x1c,
> +	SCPI_CMD_SENSOR_CFG_PERIODIC	= 0x1d,
> +	SCPI_CMD_SENSOR_CFG_BOUNDS	= 0x1e,
> +	SCPI_CMD_SENSOR_ASYNC_VALUE	= 0x1f,
> +	SCPI_CMD_COUNT
> +};
> +
> +struct legacy_scpi_xfer {
> +	u32 cmd;
> +	u32 status;
> +	const void *tx_buf;
> +	void *rx_buf;
> +	unsigned int tx_len;
> +	unsigned int rx_len;
> +	struct completion done;
> +};
> +
> +struct legacy_scpi_chan {
> +	struct mbox_client cl;
> +	struct mbox_chan *chan;
> +	void __iomem *tx_payload;
> +	void __iomem *rx_payload;
> +	spinlock_t rx_lock; /* locking for the rx pending list */
> +	struct mutex xfers_lock;
> +	struct legacy_scpi_xfer t;
> +};
> +
> +struct legacy_scpi_drvinfo {
> +	int num_chans;
> +	struct legacy_scpi_chan *channels;
> +	struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
> +};
> +

Even these data structures could remain, and mended wherever needed or
an alternate can be added at worse. Complete copy paste seems
unnecessary to me.

> +	legacy_scpi_info->channels = legacy_scpi_chan;
> +	legacy_scpi_info->num_chans = count;
> +	platform_set_drvdata(pdev, legacy_scpi_info);
> +
> +	ret = devm_scpi_ops_register(dev, &legacy_scpi_ops);

Though the concept of registering scpi ops is really nice, I think we
may not require that for support this legacy scpi protocol.

In general, I see lot of code duplication, can you try not add another
file or config and introduce the legacy support into arm_scpi.c itself ?

-- 
Regards,
Sudeep

Re: [RFC PATCH v2 6/9] firmware: Add legacy SCPI protocol driver

[Neil Armstrong]

On 06/30/2016 12:53 PM, Sudeep Holla wrote:
> 
> 
> On 21/06/16 11:02, Neil Armstrong wrote:
>> Add legacy SCPI driver based on the latest SCPI driver but modified to behave
>> like an earlier technology preview SCPI implementation that at least the
>> Amlogic GXBB ARMv8 based platform uses in it's SCP firmware implementation.
>>
>> The main differences between the mainline, public and recommended SCPI
>> implementation are :
>>   - virtual channels is not implemented
>>   - command word is passed by the MHU instead of the virtual channel ID
>>   - uses "sender id" in the command word for each commands groups
>>   - payload size shift in command word is different
>>   - command word is not in SRAM, so command queuing is not possible
>>   - command indexes are different
>>   - command data structures differs
>>   - commands are redirected to low or high priority channels by their indexes,
>>     so round-robin redirection is not possible
> 
> I doubt if that's the case. At-least the original arm scp f/w didn't
> check that. Can you please trying sending any commands on any channel ?

I did, and it fails.
I'm waiting for advanced documentation for the fw, but it really seems the SCP
fw filter the command by the channel.

>>
>> A clear disclaimer is added to make it clear this implementation should not
>> be used for new products and is only here to support already released SoCs.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>   drivers/firmware/Kconfig       |  20 ++
>>   drivers/firmware/Makefile      |   1 +
>>   drivers/firmware/legacy_scpi.c | 644 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 665 insertions(+)
>>   create mode 100644 drivers/firmware/legacy_scpi.c
>>
>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index 95b01f4..b9c2a33 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -31,6 +31,26 @@ config ARM_SCPI_PROTOCOL
>>         This protocol library provides interface for all the client drivers
>>         making use of the features offered by the SCP.
>>
>> +config LEGACY_SCPI_PROTOCOL
>> +    bool "Legacy System Control and Power Interface (SCPI) Message Protocol"
> 
> Do we really need to add another config ? I thought we could just manage
> with compatibles.
> 
>> +    default y if ARCH_MESON
>> +    select ARM_SCPI_FW
>> +    help
>> +      System Control and Power Interface (SCPI) Message Protocol is
>> +      defined for the purpose of communication between the Application
>> +      Cores(AP) and the System Control Processor(SCP). The MHU peripheral
>> +      provides a mechanism for inter-processor communication between SCP
>> +      and AP.
> 
> [...]
> 
>> diff --git a/drivers/firmware/legacy_scpi.c b/drivers/firmware/legacy_scpi.c
>> new file mode 100644
>> index 0000000..4bd3ff7
>> --- /dev/null
>> +++ b/drivers/firmware/legacy_scpi.c
>> @@ -0,0 +1,644 @@
> 
> [...]
> 
>> +
>> +#define CMD_ID_SHIFT        0
>> +#define CMD_ID_MASK        0x7f
>> +#define CMD_SENDER_ID_SHIFT    8
>> +#define CMD_SENDER_ID_MASK    0xff
> 
> Again this is something I introduced in the earlier driver. But from SCP
> f/w perspective, it just sends that as is to the sender. I think we
> retain token concept as is from the latest driver. Could you check
> dropping them and check if f/w makes any assumption about these. It
> should not IMO.

I can check. I'm afraid it may check for these.

>> +#define CMD_DATA_SIZE_SHIFT    20
>> +#define CMD_DATA_SIZE_MASK    0x1ff
>> +#define PACK_SCPI_CMD(cmd_id, sender, tx_sz)                \
>> +    ((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) |            \
>> +    (((sender) & CMD_SENDER_ID_MASK) << CMD_SENDER_ID_SHIFT) |    \
>> +    (((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
>> +
>> +#define CMD_SIZE(cmd)    (((cmd) >> CMD_DATA_SIZE_SHIFT) & CMD_DATA_SIZE_MASK)
>> +#define CMD_UNIQ_MASK    (CMD_TOKEN_ID_MASK << CMD_TOKEN_ID_SHIFT | CMD_ID_MASK)
>> +#define CMD_XTRACT_UNIQ(cmd)    ((cmd) & CMD_UNIQ_MASK)
>> +
>> +#define MAX_DVFS_DOMAINS    3
>> +#define MAX_DVFS_OPPS        16
>> +#define DVFS_LATENCY(hdr)    (le32_to_cpu(hdr) >> 16)
>> +#define DVFS_OPP_COUNT(hdr)    ((le32_to_cpu(hdr) >> 8) & 0xff)
>> +
>> +#define MAX_RX_TIMEOUT          (msecs_to_jiffies(30))
>> +
>> +enum legacy_scpi_error_codes {
> 
> This along with many other defines are exactly same, not need to
> duplicate them.
> 
>> +    SCPI_SUCCESS = 0, /* Success */
>> +    SCPI_ERR_PARAM = 1, /* Invalid parameter(s) */
>> +    SCPI_ERR_ALIGN = 2, /* Invalid alignment */
>> +    SCPI_ERR_SIZE = 3, /* Invalid size */
>> +    SCPI_ERR_HANDLER = 4, /* Invalid handler/callback */
>> +    SCPI_ERR_ACCESS = 5, /* Invalid access/permission denied */
>> +    SCPI_ERR_RANGE = 6, /* Value out of range */
>> +    SCPI_ERR_TIMEOUT = 7, /* Timeout has occurred */
>> +    SCPI_ERR_NOMEM = 8, /* Invalid memory area or pointer */
>> +    SCPI_ERR_PWRSTATE = 9, /* Invalid power state */
>> +    SCPI_ERR_SUPPORT = 10, /* Not supported or disabled */
>> +    SCPI_ERR_DEVICE = 11, /* Device error */
>> +    SCPI_ERR_BUSY = 12, /* Device busy */
>> +    SCPI_ERR_MAX
>> +};
>> +
>> +enum legacy_scpi_client_id {
> 
> Could be removed as mentioned above ?
> 
>> +    SCPI_CL_NONE,
>> +    SCPI_CL_CLOCKS,
>> +    SCPI_CL_DVFS,
>> +    SCPI_CL_POWER,
>> +    SCPI_CL_THERMAL,
>> +    SCPI_CL_REMOTE,
>> +    SCPI_CL_LED_TIMER,
>> +    SCPI_MAX,
>> +};
>> +
>> +enum legacy_scpi_std_cmd {
>> +    SCPI_CMD_INVALID        = 0x00,
>> +    SCPI_CMD_SCPI_READY        = 0x01,
>> +    SCPI_CMD_SCPI_CAPABILITIES    = 0x02,
>> +    SCPI_CMD_EVENT            = 0x03,
>> +    SCPI_CMD_SET_CSS_PWR_STATE    = 0x04,
>> +    SCPI_CMD_GET_CSS_PWR_STATE    = 0x05,
>> +    SCPI_CMD_CFG_PWR_STATE_STAT    = 0x06,
>> +    SCPI_CMD_GET_PWR_STATE_STAT    = 0x07,
>> +    SCPI_CMD_SYS_PWR_STATE        = 0x08,
>> +    SCPI_CMD_L2_READY        = 0x09,
>> +    SCPI_CMD_SET_AP_TIMER        = 0x0a,
>> +    SCPI_CMD_CANCEL_AP_TIME        = 0x0b,
>> +    SCPI_CMD_DVFS_CAPABILITIES    = 0x0c,
>> +    SCPI_CMD_GET_DVFS_INFO        = 0x0d,
>> +    SCPI_CMD_SET_DVFS        = 0x0e,
>> +    SCPI_CMD_GET_DVFS        = 0x0f,
>> +    SCPI_CMD_GET_DVFS_STAT        = 0x10,
>> +    SCPI_CMD_SET_RTC        = 0x11,
>> +    SCPI_CMD_GET_RTC        = 0x12,
>> +    SCPI_CMD_CLOCK_CAPABILITIES    = 0x13,
>> +    SCPI_CMD_SET_CLOCK_INDEX    = 0x14,
>> +    SCPI_CMD_SET_CLOCK_VALUE    = 0x15,
>> +    SCPI_CMD_GET_CLOCK_VALUE    = 0x16,
>> +    SCPI_CMD_PSU_CAPABILITIES    = 0x17,
>> +    SCPI_CMD_SET_PSU        = 0x18,
>> +    SCPI_CMD_GET_PSU        = 0x19,
>> +    SCPI_CMD_SENSOR_CAPABILITIES    = 0x1a,
>> +    SCPI_CMD_SENSOR_INFO        = 0x1b,
>> +    SCPI_CMD_SENSOR_VALUE        = 0x1c,
>> +    SCPI_CMD_SENSOR_CFG_PERIODIC    = 0x1d,
>> +    SCPI_CMD_SENSOR_CFG_BOUNDS    = 0x1e,
>> +    SCPI_CMD_SENSOR_ASYNC_VALUE    = 0x1f,
>> +    SCPI_CMD_COUNT
>> +};
>> +
>> +struct legacy_scpi_xfer {
>> +    u32 cmd;
>> +    u32 status;
>> +    const void *tx_buf;
>> +    void *rx_buf;
>> +    unsigned int tx_len;
>> +    unsigned int rx_len;
>> +    struct completion done;
>> +};
>> +
>> +struct legacy_scpi_chan {
>> +    struct mbox_client cl;
>> +    struct mbox_chan *chan;
>> +    void __iomem *tx_payload;
>> +    void __iomem *rx_payload;
>> +    spinlock_t rx_lock; /* locking for the rx pending list */
>> +    struct mutex xfers_lock;
>> +    struct legacy_scpi_xfer t;
>> +};
>> +
>> +struct legacy_scpi_drvinfo {
>> +    int num_chans;
>> +    struct legacy_scpi_chan *channels;
>> +    struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
>> +};
>> +
> 
> Even these data structures could remain, and mended wherever needed or
> an alternate can be added at worse. Complete copy paste seems
> unnecessary to me.
> 
>> +    legacy_scpi_info->channels = legacy_scpi_chan;
>> +    legacy_scpi_info->num_chans = count;
>> +    platform_set_drvdata(pdev, legacy_scpi_info);
>> +
>> +    ret = devm_scpi_ops_register(dev, &legacy_scpi_ops);
> 
> Though the concept of registering scpi ops is really nice, I think we
> may not require that for support this legacy scpi protocol.
> 
> In general, I see lot of code duplication, can you try not add another
> file or config and introduce the legacy support into arm_scpi.c itself ?
> 

I can try but it will create a spaguetti monster since the core functions will be duplicated.
I really think the official scpi driver should follow it's path and stay clean and reliable,
and create a side-monster to support the ugly legacy based firmware for Amlogic and Rockchip.

The point is to find a way to share the scpi resource drivers in common !

Neil

[RFC PATCH v2 7/9] dt-bindings: arm: Update arm,scpi bindings with Meson GXBB SCPI

[Neil Armstrong]

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 Documentation/devicetree/bindings/arm/arm,scpi.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
index 313dabd..fe58212 100644
--- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
@@ -7,7 +7,7 @@ by Linux to initiate various system control and power operations.
 
 Required properties:
 
-- compatible : should be "arm,scpi"
+- compatible : should be "arm,scpi" or "amlogic,meson-gxbb-scpi"
 - mboxes: List of phandle and mailbox channel specifiers
 	  All the channels reserved by remote SCP firmware for use by
 	  SCPI message protocol should be specified in any order
@@ -60,7 +60,8 @@ A small area of SRAM is reserved for SCPI communication between application
 processors and SCP.
 
 Required properties:
-- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno
+- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno,
+		or "amlogic,meson-gxbb-sram" for Amlogic GXBB SoC.
 
 The rest of the properties should follow the generic mmio-sram description
 found in ../../sram/sram.txt
@@ -70,7 +71,8 @@ Each sub-node represents the reserved area for SCPI.
 Required sub-node properties:
 - reg : The base offset and size of the reserved area with the SRAM
 - compatible : should be "arm,juno-scp-shmem" for Non-secure SRAM based
-	       shared memory on Juno platforms
+	       shared memory on Juno platforms or
+	       "amlogic,meson-gxbb-scp-shmem" for Amlogic GXBB SoC.
 
 Sensor bindings for the sensors based on SCPI Message Protocol
 --------------------------------------------------------------
-- 
2.7.0

[RFC PATCH v2 8/9] ARM64: dts: meson-gxbb: Add SRAM node

[Neil Armstrong]

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index 77381d0..913ba86 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -124,6 +124,15 @@
 		#size-cells = <2>;
 		ranges;
 
+		sram: sram@c8000000 {
+			compatible = "amlogic,meson-gxbb-sram", "mmio-sram";
+			reg = <0x0 0xc8000000 0x0 0x14000>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0x0 0xc8000000 0x14000>;
+		};
+
 		cbus: cbus@c1100000 {
 			compatible = "simple-bus";
 			reg = <0x0 0xc1100000 0x0 0x100000>;
-- 
2.7.0

[RFC PATCH v2 9/9] ARM64: dts: meson-gxbb: Add SCPI with cpufreq & sensors Nodes

[Neil Armstrong]

Add the vcpu DVFS clock, the SCPU shared SRAM and the SCPI sensors nodes.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 36 +++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index 913ba86..0f2596e 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -61,6 +61,7 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x0>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 
 		cpu1: cpu@1 {
@@ -68,6 +69,7 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x1>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 
 		cpu2: cpu@2 {
@@ -75,6 +77,7 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x2>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 
 		cpu3: cpu@3 {
@@ -82,6 +85,7 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x3>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 	};
 
@@ -99,6 +103,28 @@
 		method = "smc";
 	};
 
+	scpi {
+		compatible = "amlogic,meson-gxbb-scpi";
+		mboxes = <&mailbox 0 &mailbox 1>;
+		shmem = <&cpu_scp_lpri &cpu_scp_hpri>;
+
+		clocks {
+			compatible = "arm,scpi-clocks";
+
+			scpi_dvfs: scpi_clocks@0 {
+				compatible = "arm,scpi-dvfs-clocks";
+				#clock-cells = <1>;
+				clock-indices = <0>;
+				clock-output-names = "vcpu";
+	   		};
+		};
+
+		scpi_sensors: sensors {
+			compatible = "arm,scpi-sensors";
+			#thermal-sensor-cells = <1>;
+		};
+	};
+
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupts = <GIC_PPI 13
@@ -131,6 +157,16 @@
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges = <0 0x0 0xc8000000 0x14000>;
+
+			cpu_scp_lpri: scp-shmem@0 {
+				compatible = "amlogic,meson-gxbb-scp-shmem";
+				reg = <0x13000 0x400>;
+			};
+
+			cpu_scp_hpri: scp-shmem@200 {
+				compatible = "amlogic,meson-gxbb-scp-shmem";
+				reg = <0x13400 0x400>;
+			};
 		};
 
 		cbus: cbus@c1100000 {
-- 
2.7.0

Re: [RFC PATCH v2 0/9] scpi: Add SCPI registry to handle legacy protocol

[Frank Wang]

Hi Neil,

On 2016/6/21 18:02, Neil Armstrong wrote:
> This patchset aims to support the legacy SCPI firmware implementation that was
> delivered as early technology preview for the JUNO platform.
>
> Finally a stable, maintained and public implementation for the SCPI protocol
> has been upstreamed part of the JUNO support and it is the recommended way
> of implementing SCP communication on ARMv8 platforms.
>
> The Amlogic GXBB platform is using this legacy protocol, as the RK3368 & RK3399
> platforms. Only the GXBB example is provided here, but it's unclear if other
> Amlogic ARMv8 based SoCs uses this legacy procotol.
>
> In order to support the legacy protocol :
>   - Move the scpi_get_ops to a thin registry layer
>   - Change the arm_scpi.c to use the registry layer
>   - Add a separate config option to build the registry layer
>   - Add the legacy SCPI driver based on the new implementation
>   - For example, add the Amlogic GXBB MHU and SCPI DT cpufreq & sensors nodes

Two comments may be not very associated with this series.

First, do you have any plan to implement the APIs for extended set ID of 
SCPI? If these APIs do not care commands, just focus on a message 
transmission access, something like a library role, and extended command 
can define in consumers driver module, I think it can more help for 
other consumers like Rockchip to send/receive nonstandard command data 
conveniently. Can it be?

Second, As far as I know, some legacy mailbox hardware which like 
Altera, Rockchip...  need write command and data register sequentially, 
then it can create a interrupt, however, arm-scpi first use inner 
scpi_xfer structure to package the message, then data is passed to 
msg_submit (At mailbox.c), further into the bottom mailbox driver, but 
the data type is converted void*, so the mailbox driver could not 
extract the contents of message, if it do cast type, it may become 
non-general from driver view. Hence, is it possible to add a message 
header into scpi_xfer which for the bottom mailbox driver or dope out 
any other methods to solve it?

BR.
Frank

> Initial RFC discution tread can be found at https://lkml.org/lkml/2016/5/26/111
>
> Neil Armstrong (9):
>    mailbox: Add Amlogic Meson Message-Handling-Unit
>    dt-bindings: mailbox: Add Amlogic Meson MHU Bindings
>    ARM64: dts: meson-gxbb: Add Meson MHU Node
>    firmware: Add a SCPI registry to handle multiple implementations
>    firmware: scpi: Switch arm_scpi to use new registry
>    firmware: Add legacy SCPI protocol driver
>    dt-bindings: arm: Update arm,scpi bindings with Meson GXBB SCPI
>    ARM64: dts: meson-gxbb: Add SRAM node
>    ARM64: dts: meson-gxbb: Add SCPI with cpufreq & sensors Nodes
>
>   Documentation/devicetree/bindings/arm/arm,scpi.txt |   8 +-
>   .../devicetree/bindings/mailbox/meson-mhu.txt      |  33 ++
>   arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |  53 ++
>   drivers/firmware/Kconfig                           |  24 +
>   drivers/firmware/Makefile                          |   2 +
>   drivers/firmware/arm_scpi.c                        |  14 +-
>   drivers/firmware/legacy_scpi.c                     | 644 +++++++++++++++++++++
>   drivers/firmware/scpi.c                            |  94 +++
>   drivers/mailbox/Makefile                           |   2 +
>   drivers/mailbox/meson_mhu.c                        | 199 +++++++
>   include/linux/scpi_protocol.h                      |  15 +-
>   11 files changed, 1075 insertions(+), 13 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/mailbox/meson-mhu.txt
>   create mode 100644 drivers/firmware/legacy_scpi.c
>   create mode 100644 drivers/firmware/scpi.c
>   create mode 100644 drivers/mailbox/meson_mhu.c
>

Re: [RFC PATCH v2 0/9] scpi: Add SCPI registry to handle legacy protocol

[Neil Armstrong]

On 06/22/2016 04:54 AM, Frank Wang wrote:
> Hi Neil,
> 
> On 2016/6/21 18:02, Neil Armstrong wrote:
>> This patchset aims to support the legacy SCPI firmware implementation that was
>> delivered as early technology preview for the JUNO platform.
>>
>> Finally a stable, maintained and public implementation for the SCPI protocol
>> has been upstreamed part of the JUNO support and it is the recommended way
>> of implementing SCP communication on ARMv8 platforms.
>>
>> The Amlogic GXBB platform is using this legacy protocol, as the RK3368 & RK3399
>> platforms. Only the GXBB example is provided here, but it's unclear if other
>> Amlogic ARMv8 based SoCs uses this legacy procotol.
>>
>> In order to support the legacy protocol :
>>   - Move the scpi_get_ops to a thin registry layer
>>   - Change the arm_scpi.c to use the registry layer
>>   - Add a separate config option to build the registry layer
>>   - Add the legacy SCPI driver based on the new implementation
>>   - For example, add the Amlogic GXBB MHU and SCPI DT cpufreq & sensors nodes
> 
> Two comments may be not very associated with this series.
> 
> First, do you have any plan to implement the APIs for extended set ID of SCPI? If these APIs do not care commands, just focus on a message transmission access, something like a library role, and extended command can define in consumers driver module, I think it can more help for other consumers like Rockchip to send/receive nonstandard command data conveniently. Can it be?

Yes, Amlogic only have a single extended command, but supporting Rockchips extended API is necessary.
I'll need to think about a convenient way to extend vendor API in a separate driver with a set or read/write functions, but
keeping the compatibility with the official SCPI extension API.

> Second, As far as I know, some legacy mailbox hardware which like Altera, Rockchip...  need write command and data register sequentially, then it can create a interrupt, however, arm-scpi first use inner scpi_xfer structure to package the message, then data is passed to msg_submit (At mailbox.c), further into the bottom mailbox driver, but the data type is converted void*, so the mailbox driver could not extract the contents of message, if it do cast type, it may become non-general from driver view. Hence, is it possible to add a message header into scpi_xfer which for the bottom mailbox driver or dope out any other methods to solve it?

Well, the message type should be considered "known" between the mailbox client and controller, but here Amlogic did copy ARM's MHU so we can simply cast the data into a uint32_t and push the
word in the status register, but the interface was not designed to handle a "message", only a status.
In your case, you seem to have used the message capability of the mailbox to transmit a proper message containing the command word and return length.
Therefore, I must think about implementing a per-vendor transmit layer, because a generic way won't work among the different mailbox controllers.

Would it be possible to actually test the next revision of the legacy SCPI implementation on your platform ? I'll try to implement it for rockchip with what I understood, but I'll need your help for the DT declaration side.

Regards,
Neil
> BR.
> Frank
> 
>> Initial RFC discution tread can be found at https://lkml.org/lkml/2016/5/26/111
>>
>> Neil Armstrong (9):
>>    mailbox: Add Amlogic Meson Message-Handling-Unit
>>    dt-bindings: mailbox: Add Amlogic Meson MHU Bindings
>>    ARM64: dts: meson-gxbb: Add Meson MHU Node
>>    firmware: Add a SCPI registry to handle multiple implementations
>>    firmware: scpi: Switch arm_scpi to use new registry
>>    firmware: Add legacy SCPI protocol driver
>>    dt-bindings: arm: Update arm,scpi bindings with Meson GXBB SCPI
>>    ARM64: dts: meson-gxbb: Add SRAM node
>>    ARM64: dts: meson-gxbb: Add SCPI with cpufreq & sensors Nodes
>>
>>   Documentation/devicetree/bindings/arm/arm,scpi.txt |   8 +-
>>   .../devicetree/bindings/mailbox/meson-mhu.txt      |  33 ++
>>   arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |  53 ++
>>   drivers/firmware/Kconfig                           |  24 +
>>   drivers/firmware/Makefile                          |   2 +
>>   drivers/firmware/arm_scpi.c                        |  14 +-
>>   drivers/firmware/legacy_scpi.c                     | 644 +++++++++++++++++++++
>>   drivers/firmware/scpi.c                            |  94 +++
>>   drivers/mailbox/Makefile                           |   2 +
>>   drivers/mailbox/meson_mhu.c                        | 199 +++++++
>>   include/linux/scpi_protocol.h                      |  15 +-
>>   11 files changed, 1075 insertions(+), 13 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/mailbox/meson-mhu.txt
>>   create mode 100644 drivers/firmware/legacy_scpi.c
>>   create mode 100644 drivers/firmware/scpi.c
>>   create mode 100644 drivers/mailbox/meson_mhu.c
>>
> 
> 

Re: [RFC PATCH v2 0/9] scpi: Add SCPI registry to handle legacy protocol

[Frank Wang]

Hi Neil & Caesar,

On 2016/6/23 20:45, Neil Armstrong wrote:
> On 06/22/2016 04:54 AM, Frank Wang wrote:
>> Hi Neil,
>>
>> On 2016/6/21 18:02, Neil Armstrong wrote:
>>> This patchset aims to support the legacy SCPI firmware implementation that was
>>> delivered as early technology preview for the JUNO platform.
>>>
>>> Finally a stable, maintained and public implementation for the SCPI protocol
>>> has been upstreamed part of the JUNO support and it is the recommended way
>>> of implementing SCP communication on ARMv8 platforms.
>>>
>>> The Amlogic GXBB platform is using this legacy protocol, as the RK3368 & RK3399
>>> platforms. Only the GXBB example is provided here, but it's unclear if other
>>> Amlogic ARMv8 based SoCs uses this legacy procotol.
>>>
>>> In order to support the legacy protocol :
>>>    - Move the scpi_get_ops to a thin registry layer
>>>    - Change the arm_scpi.c to use the registry layer
>>>    - Add a separate config option to build the registry layer
>>>    - Add the legacy SCPI driver based on the new implementation
>>>    - For example, add the Amlogic GXBB MHU and SCPI DT cpufreq & sensors nodes
>> Two comments may be not very associated with this series.
>>
>> First, do you have any plan to implement the APIs for extended set ID of SCPI? If these APIs do not care commands, just focus on a message transmission access, something like a library role, and extended command can define in consumers driver module, I think it can more help for other consumers like Rockchip to send/receive nonstandard command data conveniently. Can it be?
> Yes, Amlogic only have a single extended command, but supporting Rockchips extended API is necessary.
> I'll need to think about a convenient way to extend vendor API in a separate driver with a set or read/write functions, but
> keeping the compatibility with the official SCPI extension API.

Well, sounds great, how soon can you deliver it?

>> Second, As far as I know, some legacy mailbox hardware which like Altera, Rockchip...  need write command and data register sequentially, then it can create a interrupt, however, arm-scpi first use inner scpi_xfer structure to package the message, then data is passed to msg_submit (At mailbox.c), further into the bottom mailbox driver, but the data type is converted void*, so the mailbox driver could not extract the contents of message, if it do cast type, it may become non-general from driver view. Hence, is it possible to add a message header into scpi_xfer which for the bottom mailbox driver or dope out any other methods to solve it?
> Well, the message type should be considered "known" between the mailbox client and controller, but here Amlogic did copy ARM's MHU so we can simply cast the data into a uint32_t and push the
> word in the status register, but the interface was not designed to handle a "message", only a status.
> In your case, you seem to have used the message capability of the mailbox to transmit a proper message containing the command word and return length.

Yeah, you are right, Rockchip mailbox need command word and RX size to 
ensure data integrity in current, however RX size may be replaced from 
other item of xfer data. Plus, as last mail I mentioned that a interrupt 
can only be created via writing command register and data register 
(filled RX size at present) sequentially . So from controller view, 
knowing a proper value of data which it should know is required.

> Therefore, I must think about implementing a per-vendor transmit layer, because a generic way won't work among the different mailbox controllers.
>
> Would it be possible to actually test the next revision of the legacy SCPI implementation on your platform ? I'll try to implement it for rockchip with what I understood, but I'll need your help for the DT declaration side.

Of course, I will be glad to.

@Caesar, would you like to share any other good comments?


BR.
Frank

> Regards,
> Neil
>>> Initial RFC discution tread can be found at https://lkml.org/lkml/2016/5/26/111
>>>
>>> Neil Armstrong (9):
>>>     mailbox: Add Amlogic Meson Message-Handling-Unit
>>>     dt-bindings: mailbox: Add Amlogic Meson MHU Bindings
>>>     ARM64: dts: meson-gxbb: Add Meson MHU Node
>>>     firmware: Add a SCPI registry to handle multiple implementations
>>>     firmware: scpi: Switch arm_scpi to use new registry
>>>     firmware: Add legacy SCPI protocol driver
>>>     dt-bindings: arm: Update arm,scpi bindings with Meson GXBB SCPI
>>>     ARM64: dts: meson-gxbb: Add SRAM node
>>>     ARM64: dts: meson-gxbb: Add SCPI with cpufreq & sensors Nodes
>>>
>>>    Documentation/devicetree/bindings/arm/arm,scpi.txt |   8 +-
>>>    .../devicetree/bindings/mailbox/meson-mhu.txt      |  33 ++
>>>    arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |  53 ++
>>>    drivers/firmware/Kconfig                           |  24 +
>>>    drivers/firmware/Makefile                          |   2 +
>>>    drivers/firmware/arm_scpi.c                        |  14 +-
>>>    drivers/firmware/legacy_scpi.c                     | 644 +++++++++++++++++++++
>>>    drivers/firmware/scpi.c                            |  94 +++
>>>    drivers/mailbox/Makefile                           |   2 +
>>>    drivers/mailbox/meson_mhu.c                        | 199 +++++++
>>>    include/linux/scpi_protocol.h                      |  15 +-
>>>    11 files changed, 1075 insertions(+), 13 deletions(-)
>>>    create mode 100644 Documentation/devicetree/bindings/mailbox/meson-mhu.txt
>>>    create mode 100644 drivers/firmware/legacy_scpi.c
>>>    create mode 100644 drivers/firmware/scpi.c
>>>    create mode 100644 drivers/mailbox/meson_mhu.c
>>>
>>