diff mbox series

[v5,05/10] soc: qcom: Add AOSS QMP communication driver

Message ID 20190131003933.11436-6-bjorn.andersson@linaro.org (mailing list archive)
State New, archived
Headers show
Series Qualcomm AOSS QMP driver and modem dts | expand

Commit Message

Bjorn Andersson Jan. 31, 2019, 12:39 a.m. UTC
The AOSS QMP driver is used to communicate with the AOSS for certain
side-channel requests, that are not enabled through the RPMh interface.

The communication is a very simple synchronous mechanism of messages
being written in message RAM and a doorbell in the AOSS is rung. As the
AOSS has processed the message length is cleared and an interrupt is
fired by the AOSS as acknowledgment.

Reviewed-by: Arun Kumar Neelakantam <aneela@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v4:
- None

Changes since v3:
- None

 drivers/soc/qcom/Kconfig          |   9 +
 drivers/soc/qcom/Makefile         |   1 +
 drivers/soc/qcom/aoss-qmp.c       | 317 ++++++++++++++++++++++++++++++
 include/linux/soc/qcom/aoss-qmp.h |  14 ++
 4 files changed, 341 insertions(+)
 create mode 100644 drivers/soc/qcom/aoss-qmp.c
 create mode 100644 include/linux/soc/qcom/aoss-qmp.h

Comments

Doug Anderson Feb. 1, 2019, 11:36 p.m. UTC | #1
Hi,

On Wed, Jan 30, 2019 at 4:40 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> +++ b/drivers/soc/qcom/aoss-qmp.c
> @@ -0,0 +1,317 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, Linaro Ltd
> + */
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/soc/qcom/aoss-qmp.h>
> +
> +#define QMP_DESC_MAGIC                 0x0
> +#define QMP_DESC_VERSION               0x4
> +#define QMP_DESC_FEATURES              0x8
> +
> +#define QMP_DESC_UCORE_LINK_STATE      0xc
> +#define QMP_DESC_UCORE_LINK_STATE_ACK  0x10
> +#define QMP_DESC_UCORE_CH_STATE                0x14
> +#define QMP_DESC_UCORE_CH_STATE_ACK    0x18
> +#define QMP_DESC_UCORE_MBOX_SIZE       0x1c
> +#define QMP_DESC_UCORE_MBOX_OFFSET     0x20
> +
> +#define QMP_DESC_MCORE_LINK_STATE      0x24
> +#define QMP_DESC_MCORE_LINK_STATE_ACK  0x28
> +#define QMP_DESC_MCORE_CH_STATE                0x2c
> +#define QMP_DESC_MCORE_CH_STATE_ACK    0x30
> +#define QMP_DESC_MCORE_MBOX_SIZE       0x34
> +#define QMP_DESC_MCORE_MBOX_OFFSET     0x38

I sure wish something in this file told me what a mcore and a ucore
were.  The only thing I can think of is that an "m" core is two "u"
cores flipped upside down and placed really close to each other.
...if we had 6 upside down "u" cores we'd have an "mmm" core.  Mmm,
core.


> +static int qmp_open(struct qmp *qmp)
> +{
> +       int ret;
> +       u32 val;
> +
> +       ret = wait_event_timeout(qmp->event, qmp_magic_valid(qmp), HZ);

I'm a totally noob here, but I'm curious: what kicks this event?  Do
we just assume that an IRQ is pending already when the probe()
function is called?  Maybe you could add a comment?

...or maybe you never actually get an IRQ here and just rely on the
magic value being right at boot in which case we should just check
qmp_magic_valid()

...or maybe you never actually get an IRQ here and this is equivalent
to msleep(1000) followed by a check of qmp_magic_valid()?


> +       if (!ret) {
> +               dev_err(qmp->dev, "QMP magic doesn't match\n");
> +               return -ETIMEDOUT;
> +       }
> +
> +       val = readl(qmp->msgram + QMP_DESC_VERSION);
> +       if (val != QMP_VERSION) {
> +               dev_err(qmp->dev, "unsupported QMP version %d\n", val);
> +               return -EINVAL;
> +       }
> +
> +       qmp->offset = readl(qmp->msgram + QMP_DESC_MCORE_MBOX_OFFSET);
> +       qmp->size = readl(qmp->msgram + QMP_DESC_MCORE_MBOX_SIZE);
> +       if (!qmp->size) {
> +               dev_err(qmp->dev, "invalid mailbox size 0x%zx\n", qmp->size);

nitty nit: Can you do "%#zx" to avoid the need for the 0x?


> +               return -EINVAL;
> +       }
> +
> +       /* Ack remote core's link state */
> +       val = readl(qmp->msgram + QMP_DESC_UCORE_LINK_STATE);
> +       writel(val, qmp->msgram + QMP_DESC_UCORE_LINK_STATE_ACK);
> +
> +       /* Set local core's link state to up */
> +       writel(QMP_STATE_UP, qmp->msgram + QMP_DESC_MCORE_LINK_STATE);
> +
> +       qmp_kick(qmp);
> +
> +       ret = wait_event_timeout(qmp->event, qmp_link_acked(qmp), HZ);
> +       if (!ret) {
> +               dev_err(qmp->dev, "ucore didn't ack link\n");
> +               goto timeout_close_link;
> +       }
> +
> +       writel(QMP_STATE_UP, qmp->msgram + QMP_DESC_MCORE_CH_STATE);
> +
> +       ret = wait_event_timeout(qmp->event, qmp_ucore_channel_up(qmp), HZ);

Again maybe a noob question, but what kicks the interrupt here?  Is
the other side looping waiting to see us write "QMP_STATE_UP" into
"QMP_DESC_MCORE_CH_STATE" and then it sends us another interrupt?
...or are we just getting lucky that the condition is right to begin
with?


> +       if (!ret) {
> +               dev_err(qmp->dev, "ucore didn't open channel\n");
> +               goto timeout_close_channel;
> +       }
> +
> +       /* Ack remote core's channel state */
> +       val = readl(qmp->msgram + QMP_DESC_UCORE_CH_STATE);
> +       writel(val, qmp->msgram + QMP_DESC_UCORE_CH_STATE_ACK);

nit: the readl() is silly here.  Just before this you called
qmp_ucore_channel_up() and that confirmed that the value you're
getting here is exactly equal to "QMP_STATE_UP".  Just write that.


> +static int qmp_probe(struct platform_device *pdev)
> +{
> +       struct resource *res;
> +       struct qmp *qmp;
> +       int irq;
> +       int ret;
> +
> +       qmp = devm_kzalloc(&pdev->dev, sizeof(*qmp), GFP_KERNEL);
> +       if (!qmp)
> +               return -ENOMEM;
> +
> +       qmp->dev = &pdev->dev;
> +       init_waitqueue_head(&qmp->event);
> +       mutex_init(&qmp->tx_lock);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       qmp->msgram = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(qmp->msgram))
> +               return PTR_ERR(qmp->msgram);
> +
> +       qmp->mbox_client.dev = &pdev->dev;
> +       qmp->mbox_client.knows_txdone = true;
> +       qmp->mbox_chan = mbox_request_channel(&qmp->mbox_client, 0);

nit: your code would be simplified a bit if you created
devm_mbox_request_channel() in a prior patch.


> +       if (IS_ERR(qmp->mbox_chan)) {
> +               dev_err(&pdev->dev, "failed to acquire ipc mailbox\n");
> +               return PTR_ERR(qmp->mbox_chan);
> +       }
> +
> +       irq = platform_get_irq(pdev, 0);
> +       ret = devm_request_irq(&pdev->dev, irq, qmp_intr, IRQF_ONESHOT,
> +                              "aoss-qmp", qmp);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "failed to request interrupt\n");
> +               mbox_free_channel(qmp->mbox_chan);
> +               return ret;
> +       }
> +
> +       ret = qmp_open(qmp);
> +       if (ret < 0) {
> +               mbox_free_channel(qmp->mbox_chan);
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, qmp);
> +
> +       if (of_property_read_bool(pdev->dev.of_node, "#power-domain-cells")) {
> +               qmp->pd_pdev = platform_device_register_data(&pdev->dev,
> +                                                            "aoss_qmp_pd",
> +                                                            PLATFORM_DEVID_NONE,
> +                                                            NULL, 0);
> +               if (IS_ERR(qmp->pd_pdev))
> +                       dev_err(&pdev->dev, "failed to register AOSS PD\n");

nit: I'd prefer dev_warn() for serious but non-fatal errors.  This
appears to be non-fatal since it doesn't cause you to return an error.

...ideally the error message should indicate that the error is being ignored.


I'm also not 100% sure why the "aoss_qmp_pd" needs to be broken up as
a separate driver.  I guess there is expectation that there will be
more sub-drivers that use qmp_send() and that stuffing them all in the
same driver would be too much?  It sure seems like your life would be
simplified if they were just one driver though unless you think
someone would want to enable "AOSS_QMP" without enabling
"AOSS_QMP_PD".


> +static int qmp_remove(struct platform_device *pdev)
> +{
> +       struct qmp *qmp = platform_get_drvdata(pdev);
> +
> +       platform_device_unregister(qmp->pd_pdev);

Presumably the above should be prefixed with:

if (!IS_ERR(qmp->pd_pdev))

...since it appears that the probe will return with no error if you
fail to register the pd_pdev and thus you need to handle it being an
error in remove.


> +       mbox_free_channel(qmp->mbox_chan);
> +       qmp_close(qmp);

nit: I always expect that remove should be in the opposite order of
probe.  That means qmp_close() should be before mbox_free_channel().
Bjorn Andersson Feb. 6, 2019, 1:33 a.m. UTC | #2
On Fri 01 Feb 15:36 PST 2019, Doug Anderson wrote:

> Hi,
> 
> On Wed, Jan 30, 2019 at 4:40 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > +++ b/drivers/soc/qcom/aoss-qmp.c
> > @@ -0,0 +1,317 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018, Linaro Ltd
> > + */
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/mailbox_client.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/soc/qcom/aoss-qmp.h>
> > +
> > +#define QMP_DESC_MAGIC                 0x0
> > +#define QMP_DESC_VERSION               0x4
> > +#define QMP_DESC_FEATURES              0x8
> > +
> > +#define QMP_DESC_UCORE_LINK_STATE      0xc
> > +#define QMP_DESC_UCORE_LINK_STATE_ACK  0x10
> > +#define QMP_DESC_UCORE_CH_STATE                0x14
> > +#define QMP_DESC_UCORE_CH_STATE_ACK    0x18
> > +#define QMP_DESC_UCORE_MBOX_SIZE       0x1c
> > +#define QMP_DESC_UCORE_MBOX_OFFSET     0x20
> > +
> > +#define QMP_DESC_MCORE_LINK_STATE      0x24
> > +#define QMP_DESC_MCORE_LINK_STATE_ACK  0x28
> > +#define QMP_DESC_MCORE_CH_STATE                0x2c
> > +#define QMP_DESC_MCORE_CH_STATE_ACK    0x30
> > +#define QMP_DESC_MCORE_MBOX_SIZE       0x34
> > +#define QMP_DESC_MCORE_MBOX_OFFSET     0x38
> 
> I sure wish something in this file told me what a mcore and a ucore
> were.  The only thing I can think of is that an "m" core is two "u"
> cores flipped upside down and placed really close to each other.
> ...if we had 6 upside down "u" cores we'd have an "mmm" core.  Mmm,
> core.
> 

I had to look at the code again to figure out which side was which, so
I'll add a comment here to indicate which is which.

> 
> > +static int qmp_open(struct qmp *qmp)
> > +{
> > +       int ret;
> > +       u32 val;
> > +
> > +       ret = wait_event_timeout(qmp->event, qmp_magic_valid(qmp), HZ);
> 
> I'm a totally noob here, but I'm curious: what kicks this event?  Do
> we just assume that an IRQ is pending already when the probe()
> function is called?  Maybe you could add a comment?
> 
> ...or maybe you never actually get an IRQ here and just rely on the
> magic value being right at boot in which case we should just check
> qmp_magic_valid()
> 
> ...or maybe you never actually get an IRQ here and this is equivalent
> to msleep(1000) followed by a check of qmp_magic_valid()?
> 

This must be me misinterpreting the downstream driver, the magic is
already in place when we enter here.

> 
> > +       if (!ret) {
> > +               dev_err(qmp->dev, "QMP magic doesn't match\n");
> > +               return -ETIMEDOUT;
> > +       }
> > +
> > +       val = readl(qmp->msgram + QMP_DESC_VERSION);
> > +       if (val != QMP_VERSION) {
> > +               dev_err(qmp->dev, "unsupported QMP version %d\n", val);
> > +               return -EINVAL;
> > +       }
> > +
> > +       qmp->offset = readl(qmp->msgram + QMP_DESC_MCORE_MBOX_OFFSET);
> > +       qmp->size = readl(qmp->msgram + QMP_DESC_MCORE_MBOX_SIZE);
> > +       if (!qmp->size) {
> > +               dev_err(qmp->dev, "invalid mailbox size 0x%zx\n", qmp->size);
> 
> nitty nit: Can you do "%#zx" to avoid the need for the 0x?
> 

Didn't know I could do that, but that said this is conditional on
!qmp->size, so I'm dropping the 0x0 from the error...
> 
> > +               return -EINVAL;
> > +       }
> > +
> > +       /* Ack remote core's link state */
> > +       val = readl(qmp->msgram + QMP_DESC_UCORE_LINK_STATE);
> > +       writel(val, qmp->msgram + QMP_DESC_UCORE_LINK_STATE_ACK);
> > +
> > +       /* Set local core's link state to up */
> > +       writel(QMP_STATE_UP, qmp->msgram + QMP_DESC_MCORE_LINK_STATE);
> > +
> > +       qmp_kick(qmp);
> > +
> > +       ret = wait_event_timeout(qmp->event, qmp_link_acked(qmp), HZ);
> > +       if (!ret) {
> > +               dev_err(qmp->dev, "ucore didn't ack link\n");
> > +               goto timeout_close_link;
> > +       }
> > +
> > +       writel(QMP_STATE_UP, qmp->msgram + QMP_DESC_MCORE_CH_STATE);
> > +
> > +       ret = wait_event_timeout(qmp->event, qmp_ucore_channel_up(qmp), HZ);
> 
> Again maybe a noob question, but what kicks the interrupt here?  Is
> the other side looping waiting to see us write "QMP_STATE_UP" into
> "QMP_DESC_MCORE_CH_STATE" and then it sends us another interrupt?
> ...or are we just getting lucky that the condition is right to begin
> with?
> 

I guess it does, but I think there is a kick inbetween here in the
downstream driver, I'll add one for good measure.

> 
> > +       if (!ret) {
> > +               dev_err(qmp->dev, "ucore didn't open channel\n");
> > +               goto timeout_close_channel;
> > +       }
> > +
> > +       /* Ack remote core's channel state */
> > +       val = readl(qmp->msgram + QMP_DESC_UCORE_CH_STATE);
> > +       writel(val, qmp->msgram + QMP_DESC_UCORE_CH_STATE_ACK);
> 
> nit: the readl() is silly here.  Just before this you called
> qmp_ucore_channel_up() and that confirmed that the value you're
> getting here is exactly equal to "QMP_STATE_UP".  Just write that.
> 

Right

> 
> > +static int qmp_probe(struct platform_device *pdev)
> > +{
> > +       struct resource *res;
> > +       struct qmp *qmp;
> > +       int irq;
> > +       int ret;
> > +
> > +       qmp = devm_kzalloc(&pdev->dev, sizeof(*qmp), GFP_KERNEL);
> > +       if (!qmp)
> > +               return -ENOMEM;
> > +
> > +       qmp->dev = &pdev->dev;
> > +       init_waitqueue_head(&qmp->event);
> > +       mutex_init(&qmp->tx_lock);
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       qmp->msgram = devm_ioremap_resource(&pdev->dev, res);
> > +       if (IS_ERR(qmp->msgram))
> > +               return PTR_ERR(qmp->msgram);
> > +
> > +       qmp->mbox_client.dev = &pdev->dev;
> > +       qmp->mbox_client.knows_txdone = true;
> > +       qmp->mbox_chan = mbox_request_channel(&qmp->mbox_client, 0);
> 
> nit: your code would be simplified a bit if you created
> devm_mbox_request_channel() in a prior patch.
> 
> 
> > +       if (IS_ERR(qmp->mbox_chan)) {
> > +               dev_err(&pdev->dev, "failed to acquire ipc mailbox\n");
> > +               return PTR_ERR(qmp->mbox_chan);
> > +       }
> > +
> > +       irq = platform_get_irq(pdev, 0);
> > +       ret = devm_request_irq(&pdev->dev, irq, qmp_intr, IRQF_ONESHOT,
> > +                              "aoss-qmp", qmp);
> > +       if (ret < 0) {
> > +               dev_err(&pdev->dev, "failed to request interrupt\n");
> > +               mbox_free_channel(qmp->mbox_chan);
> > +               return ret;
> > +       }
> > +
> > +       ret = qmp_open(qmp);
> > +       if (ret < 0) {
> > +               mbox_free_channel(qmp->mbox_chan);
> > +               return ret;
> > +       }
> > +
> > +       platform_set_drvdata(pdev, qmp);
> > +
> > +       if (of_property_read_bool(pdev->dev.of_node, "#power-domain-cells")) {
> > +               qmp->pd_pdev = platform_device_register_data(&pdev->dev,
> > +                                                            "aoss_qmp_pd",
> > +                                                            PLATFORM_DEVID_NONE,
> > +                                                            NULL, 0);
> > +               if (IS_ERR(qmp->pd_pdev))
> > +                       dev_err(&pdev->dev, "failed to register AOSS PD\n");
> 
> nit: I'd prefer dev_warn() for serious but non-fatal errors.  This
> appears to be non-fatal since it doesn't cause you to return an error.
> 
> ...ideally the error message should indicate that the error is being ignored.
> 

I think it makes more sense to keep this as dev_err() and actually
fail probe on this. Will update.

> 
> I'm also not 100% sure why the "aoss_qmp_pd" needs to be broken up as
> a separate driver.  I guess there is expectation that there will be
> more sub-drivers that use qmp_send() and that stuffing them all in the
> same driver would be too much?  It sure seems like your life would be
> simplified if they were just one driver though unless you think
> someone would want to enable "AOSS_QMP" without enabling
> "AOSS_QMP_PD".
> 

I think splitting them in different files makes a lot of sense, whether
they should be separate drivers or just linked to one chunk that's food
for thought.

> 
> > +static int qmp_remove(struct platform_device *pdev)
> > +{
> > +       struct qmp *qmp = platform_get_drvdata(pdev);
> > +
> > +       platform_device_unregister(qmp->pd_pdev);
> 
> Presumably the above should be prefixed with:
> 
> if (!IS_ERR(qmp->pd_pdev))
> 
> ...since it appears that the probe will return with no error if you
> fail to register the pd_pdev and thus you need to handle it being an
> error in remove.
> 

Let's just make sure this doesn't happen.

> 
> > +       mbox_free_channel(qmp->mbox_chan);
> > +       qmp_close(qmp);
> 
> nit: I always expect that remove should be in the opposite order of
> probe.  That means qmp_close() should be before mbox_free_channel().

You're right.

Thanks,
Bjorn
diff mbox series

Patch

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 1ee298f6bf17..28ab19bf8c98 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -3,6 +3,15 @@ 
 #
 menu "Qualcomm SoC drivers"
 
+config QCOM_AOSS_QMP
+	tristate "Qualcomm AOSS Messaging Driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on MAILBOX
+	help
+	  This driver provides the means for communicating with the
+	  micro-controller in the AOSS, using QMP, to control certain resource
+	  that are not exposed through RPMh.
+
 config QCOM_COMMAND_DB
 	bool "Qualcomm Command DB"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index ffe519b0cb66..2c04d27fbf9e 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -1,5 +1,6 @@ 
 # SPDX-License-Identifier: GPL-2.0
 CFLAGS_rpmh-rsc.o := -I$(src)
+obj-$(CONFIG_QCOM_AOSS_QMP) +=	aoss-qmp.o
 obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
 obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
 obj-$(CONFIG_QCOM_GLINK_SSR) +=	glink_ssr.o
diff --git a/drivers/soc/qcom/aoss-qmp.c b/drivers/soc/qcom/aoss-qmp.c
new file mode 100644
index 000000000000..86ee622cdadf
--- /dev/null
+++ b/drivers/soc/qcom/aoss-qmp.c
@@ -0,0 +1,317 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, Linaro Ltd
+ */
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/soc/qcom/aoss-qmp.h>
+
+#define QMP_DESC_MAGIC			0x0
+#define QMP_DESC_VERSION		0x4
+#define QMP_DESC_FEATURES		0x8
+
+#define QMP_DESC_UCORE_LINK_STATE	0xc
+#define QMP_DESC_UCORE_LINK_STATE_ACK	0x10
+#define QMP_DESC_UCORE_CH_STATE		0x14
+#define QMP_DESC_UCORE_CH_STATE_ACK	0x18
+#define QMP_DESC_UCORE_MBOX_SIZE	0x1c
+#define QMP_DESC_UCORE_MBOX_OFFSET	0x20
+
+#define QMP_DESC_MCORE_LINK_STATE	0x24
+#define QMP_DESC_MCORE_LINK_STATE_ACK	0x28
+#define QMP_DESC_MCORE_CH_STATE		0x2c
+#define QMP_DESC_MCORE_CH_STATE_ACK	0x30
+#define QMP_DESC_MCORE_MBOX_SIZE	0x34
+#define QMP_DESC_MCORE_MBOX_OFFSET	0x38
+
+#define QMP_STATE_UP	0x0000ffff
+#define QMP_STATE_DOWN	0xffff0000
+
+#define QMP_MAGIC	0x4d41494c
+#define QMP_VERSION	1
+
+/**
+ * struct qmp - driver state for QMP implementation
+ * @msgram: iomem referencing the message RAM used for communication
+ * @dev: reference to QMP device
+ * @mbox_client: mailbox client used to ring the doorbell on transmit
+ * @mbox_chan: mailbox channel used to ring the doorbell on transmit
+ * @offset: offset within @msgram where messages should be written
+ * @size: maximum size of the messages to be transmitted
+ * @event: wait_queue for synchronization with the IRQ
+ * @tx_lock: provides syncrhonization between multiple callers of qmp_send()
+ * @pd_pdev: platform device for the power-domain child device
+ */
+struct qmp {
+	void __iomem *msgram;
+	struct device *dev;
+
+	struct mbox_client mbox_client;
+	struct mbox_chan *mbox_chan;
+
+	size_t offset;
+	size_t size;
+
+	wait_queue_head_t event;
+
+	struct mutex tx_lock;
+
+	struct platform_device *pd_pdev;
+};
+
+static void qmp_kick(struct qmp *qmp)
+{
+	mbox_send_message(qmp->mbox_chan, NULL);
+	mbox_client_txdone(qmp->mbox_chan, 0);
+}
+
+static bool qmp_magic_valid(struct qmp *qmp)
+{
+	return readl(qmp->msgram + QMP_DESC_MAGIC) == QMP_MAGIC;
+}
+
+static bool qmp_link_acked(struct qmp *qmp)
+{
+	return readl(qmp->msgram + QMP_DESC_MCORE_LINK_STATE_ACK) == QMP_STATE_UP;
+}
+
+static bool qmp_mcore_channel_acked(struct qmp *qmp)
+{
+	return readl(qmp->msgram + QMP_DESC_MCORE_CH_STATE_ACK) == QMP_STATE_UP;
+}
+
+static bool qmp_ucore_channel_up(struct qmp *qmp)
+{
+	return readl(qmp->msgram + QMP_DESC_UCORE_CH_STATE) == QMP_STATE_UP;
+}
+
+static int qmp_open(struct qmp *qmp)
+{
+	int ret;
+	u32 val;
+
+	ret = wait_event_timeout(qmp->event, qmp_magic_valid(qmp), HZ);
+	if (!ret) {
+		dev_err(qmp->dev, "QMP magic doesn't match\n");
+		return -ETIMEDOUT;
+	}
+
+	val = readl(qmp->msgram + QMP_DESC_VERSION);
+	if (val != QMP_VERSION) {
+		dev_err(qmp->dev, "unsupported QMP version %d\n", val);
+		return -EINVAL;
+	}
+
+	qmp->offset = readl(qmp->msgram + QMP_DESC_MCORE_MBOX_OFFSET);
+	qmp->size = readl(qmp->msgram + QMP_DESC_MCORE_MBOX_SIZE);
+	if (!qmp->size) {
+		dev_err(qmp->dev, "invalid mailbox size 0x%zx\n", qmp->size);
+		return -EINVAL;
+	}
+
+	/* Ack remote core's link state */
+	val = readl(qmp->msgram + QMP_DESC_UCORE_LINK_STATE);
+	writel(val, qmp->msgram + QMP_DESC_UCORE_LINK_STATE_ACK);
+
+	/* Set local core's link state to up */
+	writel(QMP_STATE_UP, qmp->msgram + QMP_DESC_MCORE_LINK_STATE);
+
+	qmp_kick(qmp);
+
+	ret = wait_event_timeout(qmp->event, qmp_link_acked(qmp), HZ);
+	if (!ret) {
+		dev_err(qmp->dev, "ucore didn't ack link\n");
+		goto timeout_close_link;
+	}
+
+	writel(QMP_STATE_UP, qmp->msgram + QMP_DESC_MCORE_CH_STATE);
+
+	ret = wait_event_timeout(qmp->event, qmp_ucore_channel_up(qmp), HZ);
+	if (!ret) {
+		dev_err(qmp->dev, "ucore didn't open channel\n");
+		goto timeout_close_channel;
+	}
+
+	/* Ack remote core's channel state */
+	val = readl(qmp->msgram + QMP_DESC_UCORE_CH_STATE);
+	writel(val, qmp->msgram + QMP_DESC_UCORE_CH_STATE_ACK);
+
+	qmp_kick(qmp);
+
+	ret = wait_event_timeout(qmp->event, qmp_mcore_channel_acked(qmp), HZ);
+	if (!ret) {
+		dev_err(qmp->dev, "ucore didn't ack channel\n");
+		goto timeout_close_channel;
+	}
+
+	return 0;
+
+timeout_close_channel:
+	writel(QMP_STATE_DOWN, qmp->msgram + QMP_DESC_MCORE_CH_STATE);
+
+timeout_close_link:
+	writel(QMP_STATE_DOWN, qmp->msgram + QMP_DESC_MCORE_LINK_STATE);
+	qmp_kick(qmp);
+
+	return -ETIMEDOUT;
+}
+
+static void qmp_close(struct qmp *qmp)
+{
+	writel(QMP_STATE_DOWN, qmp->msgram + QMP_DESC_MCORE_CH_STATE);
+	writel(QMP_STATE_DOWN, qmp->msgram + QMP_DESC_MCORE_LINK_STATE);
+	qmp_kick(qmp);
+}
+
+static irqreturn_t qmp_intr(int irq, void *data)
+{
+	struct qmp *qmp = data;
+
+	wake_up_interruptible_all(&qmp->event);
+
+	return IRQ_HANDLED;
+}
+
+static bool qmp_message_empty(struct qmp *qmp)
+{
+	return readl(qmp->msgram + qmp->offset) == 0;
+}
+
+/**
+ * qmp_send() - send a message to the AOSS
+ * @qmp: qmp context
+ * @data: message to be sent
+ * @len: length of the message
+ *
+ * Transmit @data to AOSS and wait for the AOSS to acknowledge the message.
+ * @len must be a multiple of 4 and not longer than the mailbox size. Access is
+ * synchronized by this implementation.
+ *
+ * Return: 0 on success, negative errno on failure
+ */
+int qmp_send(struct qmp *qmp, const void *data, size_t len)
+{
+	int ret;
+
+	if (WARN_ON(len + sizeof(u32) > qmp->size))
+		return -EINVAL;
+
+	if (WARN_ON(len % sizeof(u32)))
+		return -EINVAL;
+
+	mutex_lock(&qmp->tx_lock);
+
+	/* The message RAM only implements 32-bit accesses */
+	__iowrite32_copy(qmp->msgram + qmp->offset + sizeof(u32),
+			 data, len / sizeof(u32));
+	writel(len, qmp->msgram + qmp->offset);
+	qmp_kick(qmp);
+
+	ret = wait_event_interruptible_timeout(qmp->event,
+					       qmp_message_empty(qmp), HZ);
+	if (!ret) {
+		dev_err(qmp->dev, "ucore did not ack channel\n");
+		ret = -ETIMEDOUT;
+
+		/* Clear message from buffer */
+		writel(0, qmp->msgram + qmp->offset);
+	} else {
+		ret = 0;
+	}
+
+	mutex_unlock(&qmp->tx_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(qmp_send);
+
+static int qmp_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct qmp *qmp;
+	int irq;
+	int ret;
+
+	qmp = devm_kzalloc(&pdev->dev, sizeof(*qmp), GFP_KERNEL);
+	if (!qmp)
+		return -ENOMEM;
+
+	qmp->dev = &pdev->dev;
+	init_waitqueue_head(&qmp->event);
+	mutex_init(&qmp->tx_lock);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	qmp->msgram = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(qmp->msgram))
+		return PTR_ERR(qmp->msgram);
+
+	qmp->mbox_client.dev = &pdev->dev;
+	qmp->mbox_client.knows_txdone = true;
+	qmp->mbox_chan = mbox_request_channel(&qmp->mbox_client, 0);
+	if (IS_ERR(qmp->mbox_chan)) {
+		dev_err(&pdev->dev, "failed to acquire ipc mailbox\n");
+		return PTR_ERR(qmp->mbox_chan);
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	ret = devm_request_irq(&pdev->dev, irq, qmp_intr, IRQF_ONESHOT,
+			       "aoss-qmp", qmp);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to request interrupt\n");
+		mbox_free_channel(qmp->mbox_chan);
+		return ret;
+	}
+
+	ret = qmp_open(qmp);
+	if (ret < 0) {
+		mbox_free_channel(qmp->mbox_chan);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, qmp);
+
+	if (of_property_read_bool(pdev->dev.of_node, "#power-domain-cells")) {
+		qmp->pd_pdev = platform_device_register_data(&pdev->dev,
+							     "aoss_qmp_pd",
+							     PLATFORM_DEVID_NONE,
+							     NULL, 0);
+		if (IS_ERR(qmp->pd_pdev))
+			dev_err(&pdev->dev, "failed to register AOSS PD\n");
+	}
+
+	return 0;
+}
+
+static int qmp_remove(struct platform_device *pdev)
+{
+	struct qmp *qmp = platform_get_drvdata(pdev);
+
+	platform_device_unregister(qmp->pd_pdev);
+
+	mbox_free_channel(qmp->mbox_chan);
+	qmp_close(qmp);
+
+	return 0;
+}
+
+static const struct of_device_id qmp_dt_match[] = {
+	{ .compatible = "qcom,sdm845-aoss-qmp", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, qmp_dt_match);
+
+static struct platform_driver qmp_driver = {
+	.driver = {
+		.name		= "aoss_qmp",
+		.of_match_table	= qmp_dt_match,
+	},
+	.probe = qmp_probe,
+	.remove	= qmp_remove,
+};
+module_platform_driver(qmp_driver);
+
+MODULE_DESCRIPTION("Qualcomm AOSS QMP driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/soc/qcom/aoss-qmp.h b/include/linux/soc/qcom/aoss-qmp.h
new file mode 100644
index 000000000000..a2ac891d7fd4
--- /dev/null
+++ b/include/linux/soc/qcom/aoss-qmp.h
@@ -0,0 +1,14 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2018, Linaro Ltd
+ */
+#ifndef __AOP_QMP_H__
+#define __AOP_QMP_H__
+
+#include <linux/types.h>
+
+struct qmp;
+
+int qmp_send(struct qmp *qmp, const void *data, size_t len);
+
+#endif