Introduce a driver implementing Qualcomm Messaging Protocol (QMP) to
communicate with the Always On Subsystem (AOSS) and expose the low-power
states for the remoteprocs as a set of power-domains and the QDSS clock
as a clock.
Changes since v6:
- First couple of patches merged for v5.2
- Squashed the qmp and qmp-pd driver into one and by that moved it all
to one file
- Expose QDSS clock as a clock instead of a power domain
Bjorn Andersson (3):
dt-bindings: soc: qcom: Add AOSS QMP binding
soc: qcom: Add AOSS QMP driver
arm64: dts: qcom: Add AOSS QMP node
Sibi Sankar (1):
arm64: dts: qcom: sdm845: Add Q6V5 MSS node
.../bindings/soc/qcom/qcom,aoss-qmp.txt | 81 +++
arch/arm64/boot/dts/qcom/sdm845.dtsi | 68 +++
drivers/soc/qcom/Kconfig | 11 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qcom_aoss.c | 473 ++++++++++++++++++
include/dt-bindings/power/qcom-aoss-qmp.h | 14 +
6 files changed, 648 insertions(+)
create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,aoss-qmp.txt
create mode 100644 drivers/soc/qcom/qcom_aoss.c
create mode 100644 include/dt-bindings/power/qcom-aoss-qmp.h
--
2.18.0
The AOSS QMP provides a number of power domains, used for QDSS and
PIL, add the node for this.
Tested-by: Sibi Sankar <[email protected]>
Reviewed-by: Sibi Sankar <[email protected]>
Signed-off-by: Bjorn Andersson <[email protected]>
---
Changes since v6:
- Added #clock-cells
arch/arm64/boot/dts/qcom/sdm845.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index fcb93300ca62..666bc88d3e81 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -14,6 +14,7 @@
#include <dt-bindings/interconnect/qcom,sdm845.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>
#include <dt-bindings/phy/phy-qcom-qusb2.h>
+#include <dt-bindings/power/qcom-aoss-qmp.h>
#include <dt-bindings/power/qcom-rpmpd.h>
#include <dt-bindings/reset/qcom,sdm845-aoss.h>
#include <dt-bindings/reset/qcom,sdm845-pdc.h>
@@ -2142,6 +2143,15 @@
#reset-cells = <1>;
};
+ aoss_qmp: qmp@c300000 {
+ compatible = "qcom,sdm845-aoss-qmp";
+ reg = <0 0x0c300000 0 0x100000>;
+ interrupts = <GIC_SPI 389 IRQ_TYPE_EDGE_RISING>;
+ mboxes = <&apss_shared 0>;
+
+ #clock-cells = <0>;
+ #power-domain-cells = <1>;
+ };
+
spmi_bus: spmi@c440000 {
compatible = "qcom,spmi-pmic-arb";
reg = <0 0x0c440000 0 0x1100>,
--
2.18.0
From: Sibi Sankar <[email protected]>
This patch adds Q6V5 MSS remoteproc node for SDM845 SoCs.
Reviewed-by: Douglas Anderson <[email protected]>
Signed-off-by: Sibi Sankar <[email protected]>
Signed-off-by: Bjorn Andersson <[email protected]>
---
Changes since v6:
- None
arch/arm64/boot/dts/qcom/sdm845.dtsi | 58 ++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 666bc88d3e81..2f3ab6acda3d 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1672,6 +1672,64 @@
};
};
+ mss_pil: remoteproc@4080000 {
+ compatible = "qcom,sdm845-mss-pil";
+ reg = <0 0x04080000 0 0x408>, <0 0x04180000 0 0x48>;
+ reg-names = "qdsp6", "rmb";
+
+ interrupts-extended =
+ <&intc GIC_SPI 266 IRQ_TYPE_EDGE_RISING>,
+ <&modem_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
+ <&modem_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
+ <&modem_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
+ <&modem_smp2p_in 3 IRQ_TYPE_EDGE_RISING>,
+ <&modem_smp2p_in 7 IRQ_TYPE_EDGE_RISING>;
+ interrupt-names = "wdog", "fatal", "ready",
+ "handover", "stop-ack",
+ "shutdown-ack";
+
+ clocks = <&gcc GCC_MSS_CFG_AHB_CLK>,
+ <&gcc GCC_MSS_Q6_MEMNOC_AXI_CLK>,
+ <&gcc GCC_BOOT_ROM_AHB_CLK>,
+ <&gcc GCC_MSS_GPLL0_DIV_CLK_SRC>,
+ <&gcc GCC_MSS_SNOC_AXI_CLK>,
+ <&gcc GCC_MSS_MFAB_AXIS_CLK>,
+ <&gcc GCC_PRNG_AHB_CLK>,
+ <&rpmhcc RPMH_CXO_CLK>;
+ clock-names = "iface", "bus", "mem", "gpll0_mss",
+ "snoc_axi", "mnoc_axi", "prng", "xo";
+
+ qcom,smem-states = <&modem_smp2p_out 0>;
+ qcom,smem-state-names = "stop";
+
+ resets = <&aoss_reset AOSS_CC_MSS_RESTART>,
+ <&pdc_reset PDC_MODEM_SYNC_RESET>;
+ reset-names = "mss_restart", "pdc_reset";
+
+ qcom,halt-regs = <&tcsr_mutex_regs 0x23000 0x25000 0x24000>;
+
+ power-domains = <&aoss_qmp AOSS_QMP_LS_MODEM>,
+ <&rpmhpd SDM845_CX>,
+ <&rpmhpd SDM845_MX>,
+ <&rpmhpd SDM845_MSS>;
+ power-domain-names = "load_state", "cx", "mx", "mss";
+
+ mba {
+ memory-region = <&mba_region>;
+ };
+
+ mpss {
+ memory-region = <&mpss_region>;
+ };
+
+ glink-edge {
+ interrupts = <GIC_SPI 449 IRQ_TYPE_EDGE_RISING>;
+ label = "modem";
+ qcom,remote-pid = <1>;
+ mboxes = <&apss_shared 12>;
+ };
+ };
+
gpucc: clock-controller@5090000 {
compatible = "qcom,sdm845-gpucc";
reg = <0 0x05090000 0 0x9000>;
--
2.18.0
Add binding for the QMP based side-channel communication mechanism to
the AOSS, which is used to control resources not exposed through the
RPMh interface.
Signed-off-by: Bjorn Andersson <[email protected]>
---
Changes since v6:
- Added #clock-cells
.../bindings/soc/qcom/qcom,aoss-qmp.txt | 81 +++++++++++++++++++
include/dt-bindings/power/qcom-aoss-qmp.h | 14 ++++
2 files changed, 95 insertions(+)
create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,aoss-qmp.txt
create mode 100644 include/dt-bindings/power/qcom-aoss-qmp.h
diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,aoss-qmp.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,aoss-qmp.txt
new file mode 100644
index 000000000000..14a45b3dc059
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,aoss-qmp.txt
@@ -0,0 +1,81 @@
+Qualcomm Always-On Subsystem side channel binding
+
+This binding describes the hardware component responsible for side channel
+requests to the always-on subsystem (AOSS), used for certain power management
+requests that is not handled by the standard RPMh interface. Each client in the
+SoC has it's own block of message RAM and IRQ for communication with the AOSS.
+The protocol used to communicate in the message RAM is known as Qualcomm
+Messagin Protocol (QMP)
+
+The AOSS side channel exposes control over a set of resources, used to control
+a set of debug related clocks and to affect the low power state of resources
+related to the secondary subsystems. These resources are exposed as a set of
+power-domains.
+
+- compatible:
+ Usage: required
+ Value type: <string>
+ Definition: must be "qcom,sdm845-aoss-qmp"
+
+- reg:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: the base address and size of the message RAM for this
+ client's communication with the AOSS
+
+- interrupts:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: should specify the AOSS message IRQ for this client
+
+- mboxes:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: reference to the mailbox representing the outgoing doorbell
+ in APCS for this client, as described in mailbox/mailbox.txt
+
+- #clock-cells:
+ Usage: optional
+ Value type: <u32>
+ Definition: must be 0
+ The single clock represents the QDSS clock.
+
+- #power-domain-cells:
+ Usage: optional
+ Value type: <u32>
+ Definition: must be 1
+ The provided power-domains are:
+ CDSP state (0), LPASS state (1), modem state (2), SLPI
+ state (3), SPSS state (4) and Venus state (5).
+
+= SUBNODES
+The AOSS side channel also provides the controls for three cooling devices,
+these are expressed as subnodes of the QMP node. The name of the node is used
+to identify the resource and must therefor be "cx", "mx" or "ebi".
+
+- #cooling-cells:
+ Usage: optional
+ Value type: <u32>
+ Definition: must be 2
+
+= EXAMPLE
+
+The following example represents the AOSS side-channel message RAM and the
+mechanism exposing the power-domains, as found in SDM845.
+
+ aoss_qmp: qmp@c300000 {
+ compatible = "qcom,sdm845-aoss-qmp";
+ reg = <0x0c300000 0x100000>;
+ interrupts = <GIC_SPI 389 IRQ_TYPE_EDGE_RISING>;
+ mboxes = <&apss_shared 0>;
+
+ #power-domain-cells = <1>;
+
+ cx_cdev: cx {
+ #cooling-cells = <2>;
+ };
+
+ mx_cdev: mx {
+ #cooling-cells = <2>;
+ };
+ };
diff --git a/include/dt-bindings/power/qcom-aoss-qmp.h b/include/dt-bindings/power/qcom-aoss-qmp.h
new file mode 100644
index 000000000000..ec336d31dee4
--- /dev/null
+++ b/include/dt-bindings/power/qcom-aoss-qmp.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018, Linaro Ltd. */
+
+#ifndef __DT_BINDINGS_POWER_QCOM_AOSS_QMP_H
+#define __DT_BINDINGS_POWER_QCOM_AOSS_QMP_H
+
+#define AOSS_QMP_LS_CDSP 0
+#define AOSS_QMP_LS_LPASS 1
+#define AOSS_QMP_LS_MODEM 2
+#define AOSS_QMP_LS_SLPI 3
+#define AOSS_QMP_LS_SPSS 4
+#define AOSS_QMP_LS_VENUS 5
+
+#endif
--
2.18.0
The Always On Subsystem (AOSS) Qualcomm Messaging Protocol (QMP) driver
is used to communicate with the AOSS for certain side-channel requests,
that are not available 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.
The driver exposes the QDSS clock as a clock and the low-power state
associated with the remoteprocs in the system as a set of power-domains.
Signed-off-by: Bjorn Andersson <[email protected]>
---
Changes since v6:
- Squash the pd into the same driver as the communication, to simplify
the interaction.
- Representing the QDSS clocks as a clock/power domain turns out to
cascade into a request to make all Coresight drivers have a secondary
compatible to replace the required bus clock with a required power
domain. So in v7 this is exposed as a clock instead.
- Some error checking updates, as reported by Doug.
drivers/soc/qcom/Kconfig | 11 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qcom_aoss.c | 473 +++++++++++++++++++++++++++++++++++
3 files changed, 485 insertions(+)
create mode 100644 drivers/soc/qcom/qcom_aoss.c
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 1ee298f6bf17..3e460b334b47 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -3,6 +3,17 @@
#
menu "Qualcomm SoC drivers"
+config QCOM_AOSS_QMP
+ tristate "Qualcomm AOSS Driver"
+ depends on ARCH_QCOM || COMPILE_TEST
+ depends on MAILBOX
+ select PM_GENERIC_DOMAINS
+ help
+ This driver provides the means of communicating with and controlling
+ the low-power state for resources related to the remoteproc
+ subsystems as well as controlling the debug clocks exposed by the Always On
+ Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
+
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..eeb088beb15f 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) += qcom_aoss.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/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
new file mode 100644
index 000000000000..f1fc26ab2e36
--- /dev/null
+++ b/drivers/soc/qcom/qcom_aoss.c
@@ -0,0 +1,473 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019, Linaro Ltd
+ */
+#include <linux/clk-provider.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/mailbox_client.h>
+#include <linux/pm_domain.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <dt-bindings/power/qcom-aoss-qmp.h>
+
+#define QMP_DESC_MAGIC 0x0
+#define QMP_DESC_VERSION 0x4
+#define QMP_DESC_FEATURES 0x8
+
+/* AOP-side offsets */
+#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
+
+/* Linux-side offsets */
+#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
+
+/* Requests are expected to be 96 bytes long */
+#define QMP_MSG_LEN 96
+
+/**
+ * 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()
+ * @qdss_clk: QDSS clock hw struct
+ * @pd_data: genpd data
+ */
+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 clk_hw qdss_clk;
+ struct genpd_onecell_data pd_data;
+};
+
+struct qmp_pd {
+ struct qmp *qmp;
+ struct generic_pm_domain pd;
+};
+
+#define to_qmp_pd_resource(res) container_of(res, struct qmp_pd, pd)
+
+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;
+
+ if (!qmp_magic_valid(qmp)) {
+ 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\n");
+ 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);
+
+ qmp_kick(qmp);
+
+ 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 */
+ writel(QMP_STATE_UP, 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;
+}
+
+static int qmp_qdss_clk_prepare(struct clk_hw *hw)
+{
+ struct qmp *qmp = container_of(hw, struct qmp, qdss_clk);
+ char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 1}";
+
+ return qmp_send(qmp, buf, sizeof(buf));
+}
+
+static void qmp_qdss_clk_unprepare(struct clk_hw *hw)
+{
+ struct qmp *qmp = container_of(hw, struct qmp, qdss_clk);
+ char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 0}";
+
+ qmp_send(qmp, buf, sizeof(buf));
+}
+
+static const struct clk_ops qmp_qdss_clk_ops = {
+ .prepare = qmp_qdss_clk_prepare,
+ .unprepare = qmp_qdss_clk_unprepare,
+};
+
+static int qmp_qdss_clk_add(struct qmp *qmp)
+{
+ struct clk_init_data qdss_init = {
+ .ops = &qmp_qdss_clk_ops,
+ .name = "qdss",
+ };
+ int ret;
+
+ qmp->qdss_clk.init = &qdss_init;
+ ret = clk_hw_register(qmp->dev, &qmp->qdss_clk);
+ if (ret < 0) {
+ dev_err(qmp->dev, "failed to register qdss clock\n");
+ return ret;
+ }
+
+ return of_clk_add_hw_provider(qmp->dev->of_node, of_clk_hw_simple_get,
+ &qmp->qdss_clk);
+}
+
+static void qmp_qdss_clk_remove(struct qmp *qmp)
+{
+ clk_hw_unregister(&qmp->qdss_clk);
+ of_clk_del_provider(qmp->dev->of_node);
+}
+
+static int qmp_pd_power_toggle(struct qmp_pd *res, bool enable)
+{
+ char buf[QMP_MSG_LEN] = {};
+
+ snprintf(buf, sizeof(buf),
+ "{class: image, res: load_state, name: %s, val: %s}",
+ res->pd.name, enable ? "on" : "off");
+ return qmp_send(res->qmp, buf, sizeof(buf));
+}
+
+static int qmp_pd_power_on(struct generic_pm_domain *domain)
+{
+ return qmp_pd_power_toggle(to_qmp_pd_resource(domain), true);
+}
+
+static int qmp_pd_power_off(struct generic_pm_domain *domain)
+{
+ return qmp_pd_power_toggle(to_qmp_pd_resource(domain), false);
+}
+
+static const char * const sdm845_resources[] = {
+ [AOSS_QMP_LS_CDSP] = "cdsp",
+ [AOSS_QMP_LS_LPASS] = "adsp",
+ [AOSS_QMP_LS_MODEM] = "modem",
+ [AOSS_QMP_LS_SLPI] = "slpi",
+ [AOSS_QMP_LS_SPSS] = "spss",
+ [AOSS_QMP_LS_VENUS] = "venus",
+};
+
+static int qmp_pd_add(struct qmp *qmp)
+{
+ struct genpd_onecell_data *data = &qmp->pd_data;
+ struct device *dev = qmp->dev;
+ struct qmp_pd *res;
+ size_t num = ARRAY_SIZE(sdm845_resources);
+ int ret;
+ int i;
+
+ res = devm_kcalloc(dev, num, sizeof(*res), GFP_KERNEL);
+ if (!res)
+ return -ENOMEM;
+
+ data->domains = devm_kcalloc(dev, num, sizeof(*data->domains),
+ GFP_KERNEL);
+ if (!data->domains)
+ return -ENOMEM;
+
+ for (i = 0; i < num; i++) {
+ res[i].qmp = qmp;
+ res[i].pd.name = sdm845_resources[i];
+ res[i].pd.power_on = qmp_pd_power_on;
+ res[i].pd.power_off = qmp_pd_power_off;
+
+ ret = pm_genpd_init(&res[i].pd, NULL, true);
+ if (ret < 0) {
+ dev_err(dev, "failed to init genpd\n");
+ goto unroll_genpds;
+ }
+
+ data->domains[i] = &res[i].pd;
+ }
+
+ data->num_domains = i;
+
+ ret = of_genpd_add_provider_onecell(dev->of_node, data);
+ if (ret < 0)
+ goto unroll_genpds;
+
+ return 0;
+
+unroll_genpds:
+ for (i--; i >= 0; i--)
+ pm_genpd_remove(data->domains[i]);
+
+ return ret;
+}
+
+static void qmp_pd_remove(struct qmp *qmp)
+{
+ struct genpd_onecell_data *data = &qmp->pd_data;
+ struct device *dev = qmp->dev;
+ int i;
+
+ of_genpd_del_provider(dev->of_node);
+
+ for (i = 0; i < data->num_domains; i++)
+ pm_genpd_remove(data->domains[i]);
+}
+
+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");
+ goto err_close_qmp;
+ }
+
+ ret = qmp_open(qmp);
+ if (ret < 0)
+ goto err_free_mbox;
+
+ ret = qmp_qdss_clk_add(qmp);
+ if (ret)
+ goto err_close_qmp;
+
+ ret = qmp_pd_add(qmp);
+ if (ret)
+ goto err_remove_qdss_clk;
+
+ platform_set_drvdata(pdev, qmp);
+
+ return 0;
+
+err_remove_qdss_clk:
+ qmp_qdss_clk_remove(qmp);
+err_close_qmp:
+ qmp_close(qmp);
+err_free_mbox:
+ mbox_free_channel(qmp->mbox_chan);
+
+ return ret;
+}
+
+static int qmp_remove(struct platform_device *pdev)
+{
+ struct qmp *qmp = platform_get_drvdata(pdev);
+
+ qmp_qdss_clk_remove(qmp);
+ qmp_pd_remove(qmp);
+
+ qmp_close(qmp);
+ mbox_free_channel(qmp->mbox_chan);
+
+ 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 = "qcom_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");
--
2.18.0
On Tue, 30 Apr 2019 21:37:31 -0700, Bjorn Andersson wrote:
> Add binding for the QMP based side-channel communication mechanism to
> the AOSS, which is used to control resources not exposed through the
> RPMh interface.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
> Changes since v6:
> - Added #clock-cells
>
> .../bindings/soc/qcom/qcom,aoss-qmp.txt | 81 +++++++++++++++++++
> include/dt-bindings/power/qcom-aoss-qmp.h | 14 ++++
> 2 files changed, 95 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,aoss-qmp.txt
> create mode 100644 include/dt-bindings/power/qcom-aoss-qmp.h
>
Reviewed-by: Rob Herring <[email protected]>
Hey Bjorn,
On 5/1/19 10:07 AM, Bjorn Andersson wrote:
> The Always On Subsystem (AOSS) Qualcomm Messaging Protocol (QMP) driver
> is used to communicate with the AOSS for certain side-channel requests,
> that are not available 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.
>
> The driver exposes the QDSS clock as a clock and the low-power state
> associated with the remoteprocs in the system as a set of power-domains.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
> Changes since v6:
> - Squash the pd into the same driver as the communication, to simplify
> the interaction.
> - Representing the QDSS clocks as a clock/power domain turns out to
> cascade into a request to make all Coresight drivers have a secondary
> compatible to replace the required bus clock with a required power
> domain. So in v7 this is exposed as a clock instead.
> - Some error checking updates, as reported by Doug.
>
> drivers/soc/qcom/Kconfig | 11 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom_aoss.c | 473 +++++++++++++++++++++++++++++++++++
> 3 files changed, 485 insertions(+)
> create mode 100644 drivers/soc/qcom/qcom_aoss.c
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 1ee298f6bf17..3e460b334b47 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -3,6 +3,17 @@
> #
> menu "Qualcomm SoC drivers"
>
> +config QCOM_AOSS_QMP
> + tristate "Qualcomm AOSS Driver"
> + depends on ARCH_QCOM || COMPILE_TEST
> + depends on MAILBOX
> + select PM_GENERIC_DOMAINS
> + help
> + This driver provides the means of communicating with and controlling
> + the low-power state for resources related to the remoteproc
> + subsystems as well as controlling the debug clocks exposed by the Always On
> + Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
> +
> 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..eeb088beb15f 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) += qcom_aoss.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/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> new file mode 100644
> index 000000000000..f1fc26ab2e36
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom_aoss.c
> @@ -0,0 +1,473 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, Linaro Ltd
> + */
> +#include <linux/clk-provider.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/pm_domain.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
alphabetical order..
> +#include <dt-bindings/power/qcom-aoss-qmp.h>
> +
> +#define QMP_DESC_MAGIC 0x0
> +#define QMP_DESC_VERSION 0x4
> +#define QMP_DESC_FEATURES 0x8
> +
> +/* AOP-side offsets */
> +#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
> +
> +/* Linux-side offsets */
> +#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
you may want to mention that the magic stands for MAIL
> +#define QMP_VERSION 1
> +
> +/* Requests are expected to be 96 bytes long */
> +#define QMP_MSG_LEN 96
> +
> +/**
> + * 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()
> + * @qdss_clk: QDSS clock hw struct
> + * @pd_data: genpd data
> + */
> +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 clk_hw qdss_clk;
> + struct genpd_onecell_data pd_data;
> +};
> +
> +struct qmp_pd {
> + struct qmp *qmp;
> + struct generic_pm_domain pd;
> +};
> +
> +#define to_qmp_pd_resource(res) container_of(res, struct qmp_pd, pd)
> +
> +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;
> +
> + if (!qmp_magic_valid(qmp)) {
> + 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\n");
> + 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);
> +
> + qmp_kick(qmp);
> +
> + 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 */
> + writel(QMP_STATE_UP, 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;
> +}
> +
> +static int qmp_qdss_clk_prepare(struct clk_hw *hw)
> +{
> + struct qmp *qmp = container_of(hw, struct qmp, qdss_clk);
> + char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 1}";
> +
> + return qmp_send(qmp, buf, sizeof(buf));
> +}
> +
> +static void qmp_qdss_clk_unprepare(struct clk_hw *hw)
> +{
> + struct qmp *qmp = container_of(hw, struct qmp, qdss_clk);
> + char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 0}";
> +
> + qmp_send(qmp, buf, sizeof(buf));
> +}
> +
> +static const struct clk_ops qmp_qdss_clk_ops = {
> + .prepare = qmp_qdss_clk_prepare,
> + .unprepare = qmp_qdss_clk_unprepare,
> +};
> +
> +static int qmp_qdss_clk_add(struct qmp *qmp)
> +{
> + struct clk_init_data qdss_init = {
> + .ops = &qmp_qdss_clk_ops,
> + .name = "qdss",
> + };
> + int ret;
> +
> + qmp->qdss_clk.init = &qdss_init;
> + ret = clk_hw_register(qmp->dev, &qmp->qdss_clk);
> + if (ret < 0) {
> + dev_err(qmp->dev, "failed to register qdss clock\n");
> + return ret;
> + }
> +
> + return of_clk_add_hw_provider(qmp->dev->of_node, of_clk_hw_simple_get,
> + &qmp->qdss_clk);
> +}
> +
> +static void qmp_qdss_clk_remove(struct qmp *qmp)
> +{
> + clk_hw_unregister(&qmp->qdss_clk);
> + of_clk_del_provider(qmp->dev->of_node);
> +}
> +
> +static int qmp_pd_power_toggle(struct qmp_pd *res, bool enable)
> +{
> + char buf[QMP_MSG_LEN] = {};
> +
> + snprintf(buf, sizeof(buf),
> + "{class: image, res: load_state, name: %s, val: %s}",
> + res->pd.name, enable ? "on" : "off");
> + return qmp_send(res->qmp, buf, sizeof(buf));
> +}
> +
> +static int qmp_pd_power_on(struct generic_pm_domain *domain)
> +{
> + return qmp_pd_power_toggle(to_qmp_pd_resource(domain), true);
> +}
> +
> +static int qmp_pd_power_off(struct generic_pm_domain *domain)
> +{
> + return qmp_pd_power_toggle(to_qmp_pd_resource(domain), false);
> +}
> +
> +static const char * const sdm845_resources[] = {
> + [AOSS_QMP_LS_CDSP] = "cdsp",
> + [AOSS_QMP_LS_LPASS] = "adsp",
> + [AOSS_QMP_LS_MODEM] = "modem",
> + [AOSS_QMP_LS_SLPI] = "slpi",
> + [AOSS_QMP_LS_SPSS] = "spss",
> + [AOSS_QMP_LS_VENUS] = "venus",
> +};
> +
> +static int qmp_pd_add(struct qmp *qmp)
> +{
> + struct genpd_onecell_data *data = &qmp->pd_data;
> + struct device *dev = qmp->dev;
> + struct qmp_pd *res;
> + size_t num = ARRAY_SIZE(sdm845_resources);
> + int ret;
> + int i;
> +
> + res = devm_kcalloc(dev, num, sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
> +
> + data->domains = devm_kcalloc(dev, num, sizeof(*data->domains),
> + GFP_KERNEL);
> + if (!data->domains)
> + return -ENOMEM;
> +
> + for (i = 0; i < num; i++) {
> + res[i].qmp = qmp;
> + res[i].pd.name = sdm845_resources[i];
> + res[i].pd.power_on = qmp_pd_power_on;
> + res[i].pd.power_off = qmp_pd_power_off;
> +
> + ret = pm_genpd_init(&res[i].pd, NULL, true);
> + if (ret < 0) {
> + dev_err(dev, "failed to init genpd\n");
> + goto unroll_genpds;
> + }
> +
> + data->domains[i] = &res[i].pd;
> + }
> +
> + data->num_domains = i;
> +
> + ret = of_genpd_add_provider_onecell(dev->of_node, data);
> + if (ret < 0)
> + goto unroll_genpds;
> +
> + return 0;
> +
> +unroll_genpds:
> + for (i--; i >= 0; i--)
> + pm_genpd_remove(data->domains[i]);
> +
> + return ret;
> +}
> +
> +static void qmp_pd_remove(struct qmp *qmp)
> +{
> + struct genpd_onecell_data *data = &qmp->pd_data;
> + struct device *dev = qmp->dev;
> + int i;
> +
> + of_genpd_del_provider(dev->of_node);
> +
> + for (i = 0; i < data->num_domains; i++)
> + pm_genpd_remove(data->domains[i]);
> +}
> +
> +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");
> + goto err_close_qmp;
shouldn't this be err_freebox_mbox instead? we can avoid this by doing a
request irq before mbox_request_channel...
> + }
> +
> + ret = qmp_open(qmp);
> + if (ret < 0)
> + goto err_free_mbox;
> +
> + ret = qmp_qdss_clk_add(qmp);
> + if (ret)
> + goto err_close_qmp;
> +
> + ret = qmp_pd_add(qmp);
> + if (ret)
> + goto err_remove_qdss_clk;
> +
> + platform_set_drvdata(pdev, qmp);
> +
> + return 0;
> +
> +err_remove_qdss_clk:
> + qmp_qdss_clk_remove(qmp);
> +err_close_qmp:
> + qmp_close(qmp);
> +err_free_mbox:
> + mbox_free_channel(qmp->mbox_chan);
> +
> + return ret;
> +}
> +
> +static int qmp_remove(struct platform_device *pdev)
> +{
> + struct qmp *qmp = platform_get_drvdata(pdev);
> +
> + qmp_qdss_clk_remove(qmp);
> + qmp_pd_remove(qmp);
> +
> + qmp_close(qmp);
> + mbox_free_channel(qmp->mbox_chan);
> +
> + 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 = "qcom_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");
>
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Hi Bjorn,
On 5/1/2019 10:07 AM, Bjorn Andersson wrote:
> The Always On Subsystem (AOSS) Qualcomm Messaging Protocol (QMP) driver
> is used to communicate with the AOSS for certain side-channel requests,
> that are not available 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.
>
> The driver exposes the QDSS clock as a clock and the low-power state
> associated with the remoteprocs in the system as a set of power-domains.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
> Changes since v6:
> - Squash the pd into the same driver as the communication, to simplify
> the interaction.
> - Representing the QDSS clocks as a clock/power domain turns out to
> cascade into a request to make all Coresight drivers have a secondary
> compatible to replace the required bus clock with a required power
> domain. So in v7 this is exposed as a clock instead.
> - Some error checking updates, as reported by Doug.
>
> drivers/soc/qcom/Kconfig | 11 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom_aoss.c | 473 +++++++++++++++++++++++++++++++++++
> 3 files changed, 485 insertions(+)
> create mode 100644 drivers/soc/qcom/qcom_aoss.c
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 1ee298f6bf17..3e460b334b47 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -3,6 +3,17 @@
> #
> menu "Qualcomm SoC drivers"
>
> +config QCOM_AOSS_QMP
> + tristate "Qualcomm AOSS Driver"
> + depends on ARCH_QCOM || COMPILE_TEST
> + depends on MAILBOX
> + select PM_GENERIC_DOMAINS
> + help
> + This driver provides the means of communicating with and controlling
> + the low-power state for resources related to the remoteproc
> + subsystems as well as controlling the debug clocks exposed by the Always On
> + Subsystem (AOSS) using Qualcomm Messaging Protocol (QMP).
> +
> 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..eeb088beb15f 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) += qcom_aoss.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/qcom_aoss.c b/drivers/soc/qcom/qcom_aoss.c
> new file mode 100644
> index 000000000000..f1fc26ab2e36
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom_aoss.c
> @@ -0,0 +1,473 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, Linaro Ltd
> + */
> +#include <linux/clk-provider.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/pm_domain.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/power/qcom-aoss-qmp.h>
> +
> +#define QMP_DESC_MAGIC 0x0
> +#define QMP_DESC_VERSION 0x4
> +#define QMP_DESC_FEATURES 0x8
> +
> +/* AOP-side offsets */
> +#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
> +
> +/* Linux-side offsets */
> +#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
> +
> +/* Requests are expected to be 96 bytes long */
> +#define QMP_MSG_LEN 96
> +
> +/**
> + * 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()
> + * @qdss_clk: QDSS clock hw struct
> + * @pd_data: genpd data
> + */
> +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 clk_hw qdss_clk;
> + struct genpd_onecell_data pd_data;
> +};
> +
> +struct qmp_pd {
> + struct qmp *qmp;
> + struct generic_pm_domain pd;
> +};
> +
> +#define to_qmp_pd_resource(res) container_of(res, struct qmp_pd, pd)
> +
> +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;
> +
> + if (!qmp_magic_valid(qmp)) {
> + dev_err(qmp->dev, "QMP magic doesn't match\n");
> + return -ETIMEDOUT;
-EINVAL might be correct error code here
> + }
> +
> + 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\n");
> + 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);
> +
> + qmp_kick(qmp);
> +
> + 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 */
> + writel(QMP_STATE_UP, 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;
> +}
> +
> +static int qmp_qdss_clk_prepare(struct clk_hw *hw)
> +{
> + struct qmp *qmp = container_of(hw, struct qmp, qdss_clk);
> + char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 1}";
> +
> + return qmp_send(qmp, buf, sizeof(buf));
> +}
> +
> +static void qmp_qdss_clk_unprepare(struct clk_hw *hw)
> +{
> + struct qmp *qmp = container_of(hw, struct qmp, qdss_clk);
> + char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 0}";
> +
> + qmp_send(qmp, buf, sizeof(buf));
> +}
> +
> +static const struct clk_ops qmp_qdss_clk_ops = {
> + .prepare = qmp_qdss_clk_prepare,
> + .unprepare = qmp_qdss_clk_unprepare,
> +};
> +
> +static int qmp_qdss_clk_add(struct qmp *qmp)
> +{
> + struct clk_init_data qdss_init = {
> + .ops = &qmp_qdss_clk_ops,
> + .name = "qdss",
> + };
> + int ret;
> +
> + qmp->qdss_clk.init = &qdss_init;
> + ret = clk_hw_register(qmp->dev, &qmp->qdss_clk);
> + if (ret < 0) {
> + dev_err(qmp->dev, "failed to register qdss clock\n");
> + return ret;
> + }
> +
> + return of_clk_add_hw_provider(qmp->dev->of_node, of_clk_hw_simple_get,
> + &qmp->qdss_clk);
> +}
> +
> +static void qmp_qdss_clk_remove(struct qmp *qmp)
> +{
> + clk_hw_unregister(&qmp->qdss_clk);
> + of_clk_del_provider(qmp->dev->of_node);
> +}
> +
> +static int qmp_pd_power_toggle(struct qmp_pd *res, bool enable)
> +{
> + char buf[QMP_MSG_LEN] = {};
> +
> + snprintf(buf, sizeof(buf),
> + "{class: image, res: load_state, name: %s, val: %s}",
> + res->pd.name, enable ? "on" : "off");
> + return qmp_send(res->qmp, buf, sizeof(buf));
> +}
> +
> +static int qmp_pd_power_on(struct generic_pm_domain *domain)
> +{
> + return qmp_pd_power_toggle(to_qmp_pd_resource(domain), true);
> +}
> +
> +static int qmp_pd_power_off(struct generic_pm_domain *domain)
> +{
> + return qmp_pd_power_toggle(to_qmp_pd_resource(domain), false);
> +}
> +
> +static const char * const sdm845_resources[] = {
> + [AOSS_QMP_LS_CDSP] = "cdsp",
> + [AOSS_QMP_LS_LPASS] = "adsp",
> + [AOSS_QMP_LS_MODEM] = "modem",
> + [AOSS_QMP_LS_SLPI] = "slpi",
> + [AOSS_QMP_LS_SPSS] = "spss",
> + [AOSS_QMP_LS_VENUS] = "venus",
> +};
> +
> +static int qmp_pd_add(struct qmp *qmp)
> +{
> + struct genpd_onecell_data *data = &qmp->pd_data;
> + struct device *dev = qmp->dev;
> + struct qmp_pd *res;
> + size_t num = ARRAY_SIZE(sdm845_resources);
> + int ret;
> + int i;
> +
> + res = devm_kcalloc(dev, num, sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
> +
> + data->domains = devm_kcalloc(dev, num, sizeof(*data->domains),
> + GFP_KERNEL);
> + if (!data->domains)
> + return -ENOMEM;
> +
> + for (i = 0; i < num; i++) {
> + res[i].qmp = qmp;
> + res[i].pd.name = sdm845_resources[i];
> + res[i].pd.power_on = qmp_pd_power_on;
> + res[i].pd.power_off = qmp_pd_power_off;
> +
> + ret = pm_genpd_init(&res[i].pd, NULL, true);
> + if (ret < 0) {
> + dev_err(dev, "failed to init genpd\n");
> + goto unroll_genpds;
> + }
> +
> + data->domains[i] = &res[i].pd;
> + }
> +
> + data->num_domains = i;
> +
> + ret = of_genpd_add_provider_onecell(dev->of_node, data);
> + if (ret < 0)
> + goto unroll_genpds;
> +
> + return 0;
> +
> +unroll_genpds:
> + for (i--; i >= 0; i--)
> + pm_genpd_remove(data->domains[i]);
> +
> + return ret;
> +}
> +
> +static void qmp_pd_remove(struct qmp *qmp)
> +{
> + struct genpd_onecell_data *data = &qmp->pd_data;
> + struct device *dev = qmp->dev;
> + int i;
> +
> + of_genpd_del_provider(dev->of_node);
> +
> + for (i = 0; i < data->num_domains; i++)
> + pm_genpd_remove(data->domains[i]);
> +}
> +
> +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");
> + goto err_close_qmp;
> + }
> +
> + ret = qmp_open(qmp);
> + if (ret < 0)
> + goto err_free_mbox;
> +
> + ret = qmp_qdss_clk_add(qmp);
> + if (ret)
> + goto err_close_qmp;
> +
> + ret = qmp_pd_add(qmp);
> + if (ret)
> + goto err_remove_qdss_clk;
> +
> + platform_set_drvdata(pdev, qmp);
> +
> + return 0;
> +
> +err_remove_qdss_clk:
> + qmp_qdss_clk_remove(qmp);
> +err_close_qmp:
> + qmp_close(qmp);
> +err_free_mbox:
> + mbox_free_channel(qmp->mbox_chan);
> +
> + return ret;
> +}
> +
> +static int qmp_remove(struct platform_device *pdev)
> +{
> + struct qmp *qmp = platform_get_drvdata(pdev);
> +
> + qmp_qdss_clk_remove(qmp);
> + qmp_pd_remove(qmp);
> +
> + qmp_close(qmp);
> + mbox_free_channel(qmp->mbox_chan);
> +
> + 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 = "qcom_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");
On 30-04-19, 21:37, Bjorn Andersson wrote:
> Add binding for the QMP based side-channel communication mechanism to
> the AOSS, which is used to control resources not exposed through the
> RPMh interface.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
> Changes since v6:
> - Added #clock-cells
>
> .../bindings/soc/qcom/qcom,aoss-qmp.txt | 81 +++++++++++++++++++
> include/dt-bindings/power/qcom-aoss-qmp.h | 14 ++++
> 2 files changed, 95 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,aoss-qmp.txt
> create mode 100644 include/dt-bindings/power/qcom-aoss-qmp.h
>
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,aoss-qmp.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,aoss-qmp.txt
> new file mode 100644
> index 000000000000..14a45b3dc059
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,aoss-qmp.txt
> @@ -0,0 +1,81 @@
> +Qualcomm Always-On Subsystem side channel binding
> +
> +This binding describes the hardware component responsible for side channel
> +requests to the always-on subsystem (AOSS), used for certain power management
> +requests that is not handled by the standard RPMh interface. Each client in the
> +SoC has it's own block of message RAM and IRQ for communication with the AOSS.
> +The protocol used to communicate in the message RAM is known as Qualcomm
> +Messagin Protocol (QMP)
s/Messagin/Messaging?
Rest lgtm:
Reviewed-by: Vinod Koul <[email protected]>
--
~Vinod
On 30-04-19, 21:37, Bjorn Andersson wrote:
> +#include <linux/clk-provider.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/pm_domain.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <dt-bindings/power/qcom-aoss-qmp.h>
I would have preferred this as first one, but this is fine too :)
> +/* Linux-side offsets */
> +#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
I prefer using GENMASK(15, 0)
> +#define QMP_STATE_DOWN 0xffff0000
GENMASK(31, 16)?
> +/**
> + * 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()
/s/syncrhonization/synchronization
> +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;
Isn't this redundant?
> + }
> +
> + mutex_unlock(&qmp->tx_lock);
> +
> + return ret;
> +}
--
~Vinod
On 30-04-19, 21:37, Bjorn Andersson wrote:
> The AOSS QMP provides a number of power domains, used for QDSS and
> PIL, add the node for this.
Reviewed-by: Vinod Koul <[email protected]>
--
~Vinod
On 30-04-19, 21:37, Bjorn Andersson wrote:
> From: Sibi Sankar <[email protected]>
>
> This patch adds Q6V5 MSS remoteproc node for SDM845 SoCs.
Reviewed-by: Vinod Koul <[email protected]>
--
~Vinod
Hi,
On Tue, Apr 30, 2019 at 9:37 PM Bjorn Andersson
<[email protected]> wrote:
>
> The AOSS QMP provides a number of power domains, used for QDSS and
> PIL, add the node for this.
>
> Tested-by: Sibi Sankar <[email protected]>
> Reviewed-by: Sibi Sankar <[email protected]>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
> Changes since v6:
> - Added #clock-cells
>
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index fcb93300ca62..666bc88d3e81 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -14,6 +14,7 @@
> #include <dt-bindings/interconnect/qcom,sdm845.h>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> #include <dt-bindings/phy/phy-qcom-qusb2.h>
> +#include <dt-bindings/power/qcom-aoss-qmp.h>
> #include <dt-bindings/power/qcom-rpmpd.h>
> #include <dt-bindings/reset/qcom,sdm845-aoss.h>
> #include <dt-bindings/reset/qcom,sdm845-pdc.h>
> @@ -2142,6 +2143,15 @@
Please avoid editing patches by hand. I needed to manually change the
"15" above to "16" to apply.
-Doug
Hi,
On Tue, Apr 30, 2019 at 9:38 PM Bjorn Andersson
<[email protected]> wrote:
>
> +static int qmp_qdss_clk_prepare(struct clk_hw *hw)
> +{
> + struct qmp *qmp = container_of(hw, struct qmp, qdss_clk);
> + char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 1}";
nit: "static const" the buf? No need to copy it to the stack each
time. In qmp_qdss_clk_unprepare() too.
...your string is also now fixed at 34 bytes big (including the '\0').
Do we still need to send exactly 96 bytes, or can we dumb this down to
36? We'll get a compile error if we overflow, right? If this truly
needs to be exactly 96 bytes maybe qmp_send()'s error checks should
check for things being exactly 96 bytes instead of checking for > and
% 4.
> +static int qmp_qdss_clk_add(struct qmp *qmp)
> +{
> + struct clk_init_data qdss_init = {
> + .ops = &qmp_qdss_clk_ops,
> + .name = "qdss",
> + };
Can't qdss_init be "static const"? That had the advantage of not
needing to construct it on the stack and also of it having a longer
lifetime. It looks like clk_register() stores the "hw" pointer in its
structure and the "hw" structure will have a pointer here. While I
can believe that it never looks at it again, it's nice if that pointer
doesn't point somewhere on an old stack.
I suppose we could go the other way and try to mark more stuff in this
module as __init and __initdata, but even then at least the pointer
won't be onto a stack. ;-)
> + int ret;
> +
> + qmp->qdss_clk.init = &qdss_init;
> + ret = clk_hw_register(qmp->dev, &qmp->qdss_clk);
> + if (ret < 0) {
> + dev_err(qmp->dev, "failed to register qdss clock\n");
> + return ret;
> + }
> +
> + return of_clk_add_hw_provider(qmp->dev->of_node, of_clk_hw_simple_get,
> + &qmp->qdss_clk);
devm_clk_hw_register() and devm_of_clk_add_hw_provider()? If you're
worried about ordering you could always throw in
devm_add_action_or_reset() to handle the qmp_pd_remove(), qmp_close()
and mbox_free_channel().
...with that you could fully get rid of qmp_remove() and also your
setting of drvdata.
> +static void qmp_pd_remove(struct qmp *qmp)
> +{
> + struct genpd_onecell_data *data = &qmp->pd_data;
> + struct device *dev = qmp->dev;
> + int i;
> +
> + of_genpd_del_provider(dev->of_node);
> +
> + for (i = 0; i < data->num_domains; i++)
> + pm_genpd_remove(data->domains[i]);
Still feels like the above loop would be better as:
for (i = data->num_domains - 1; i >= 0; i--)
(BTW: any way you could add me to the CC list for future patches so I
notice them earlier?)
-Doug
Quoting Doug Anderson (2019-05-23 09:38:13)
> Hi,
>
> On Tue, Apr 30, 2019 at 9:38 PM Bjorn Andersson
> <[email protected]> wrote:
>
> > +static int qmp_qdss_clk_add(struct qmp *qmp)
> > +{
> > + struct clk_init_data qdss_init = {
> > + .ops = &qmp_qdss_clk_ops,
> > + .name = "qdss",
> > + };
>
> Can't qdss_init be "static const"? That had the advantage of not
> needing to construct it on the stack and also of it having a longer
> lifetime. It looks like clk_register() stores the "hw" pointer in its
> structure and the "hw" structure will have a pointer here. While I
> can believe that it never looks at it again, it's nice if that pointer
> doesn't point somewhere on an old stack.
>
> I suppose we could go the other way and try to mark more stuff in this
> module as __init and __initdata, but even then at least the pointer
> won't be onto a stack. ;-)
>
Const would be nice, but otherwise making it static isn't a good idea.
The clk_init_data structure is all copied over, although we do leave a
dangling pointer to it stored inside the clk_hw structure we don't use
it after clk registration. Maybe we should overwrite the pointer with
NULL once we're done in clk_register() so that clk providers can't use
it. It might break somebody but would at least clarify this point.
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index aa51756fd4d6..56997a974408 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3438,9 +3438,9 @@ static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
return 0;
}
-static int clk_core_populate_parent_map(struct clk_core *core)
+static int clk_core_populate_parent_map(struct clk_core *core,
+ const struct clk_init_data *init)
{
- const struct clk_init_data *init = core->hw->init;
u8 num_parents = init->num_parents;
const char * const *parent_names = init->parent_names;
const struct clk_hw **parent_hws = init->parent_hws;
@@ -3520,6 +3520,14 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
{
int ret;
struct clk_core *core;
+ const struct clk_init_data *init = hw->init;
+
+ /*
+ * The init data is not supposed to be used outside of registration path.
+ * Set it to NULL so that provider drivers can't use it either and so that
+ * we catch use of hw->init early on in the core.
+ */
+ hw->init = NULL;
core = kzalloc(sizeof(*core), GFP_KERNEL);
if (!core) {
@@ -3527,17 +3535,17 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
goto fail_out;
}
- core->name = kstrdup_const(hw->init->name, GFP_KERNEL);
+ core->name = kstrdup_const(init->name, GFP_KERNEL);
if (!core->name) {
ret = -ENOMEM;
goto fail_name;
}
- if (WARN_ON(!hw->init->ops)) {
+ if (WARN_ON(!init->ops)) {
ret = -EINVAL;
goto fail_ops;
}
- core->ops = hw->init->ops;
+ core->ops = init->ops;
if (dev && pm_runtime_enabled(dev))
core->rpm_enabled = true;
@@ -3546,13 +3554,13 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
if (dev && dev->driver)
core->owner = dev->driver->owner;
core->hw = hw;
- core->flags = hw->init->flags;
- core->num_parents = hw->init->num_parents;
+ core->flags = init->flags;
+ core->num_parents = init->num_parents;
core->min_rate = 0;
core->max_rate = ULONG_MAX;
hw->core = core;
- ret = clk_core_populate_parent_map(core);
+ ret = clk_core_populate_parent_map(core, init);
if (ret)
goto fail_parents;
>
>
> > +static void qmp_pd_remove(struct qmp *qmp)
> > +{
> > + struct genpd_onecell_data *data = &qmp->pd_data;
> > + struct device *dev = qmp->dev;
> > + int i;
> > +
> > + of_genpd_del_provider(dev->of_node);
> > +
> > + for (i = 0; i < data->num_domains; i++)
> > + pm_genpd_remove(data->domains[i]);
>
> Still feels like the above loop would be better as:
> for (i = data->num_domains - 1; i >= 0; i--)
>
Reason being to remove in reverse order? Otherwise this looks like an
opinion.
Hi,
On Thu, May 23, 2019 at 11:05 AM Stephen Boyd <[email protected]> wrote:
>
> Quoting Doug Anderson (2019-05-23 09:38:13)
> > Hi,
> >
> > On Tue, Apr 30, 2019 at 9:38 PM Bjorn Andersson
> > <[email protected]> wrote:
> >
> > > +static int qmp_qdss_clk_add(struct qmp *qmp)
> > > +{
> > > + struct clk_init_data qdss_init = {
> > > + .ops = &qmp_qdss_clk_ops,
> > > + .name = "qdss",
> > > + };
> >
> > Can't qdss_init be "static const"? That had the advantage of not
> > needing to construct it on the stack and also of it having a longer
> > lifetime. It looks like clk_register() stores the "hw" pointer in its
> > structure and the "hw" structure will have a pointer here. While I
> > can believe that it never looks at it again, it's nice if that pointer
> > doesn't point somewhere on an old stack.
> >
> > I suppose we could go the other way and try to mark more stuff in this
> > module as __init and __initdata, but even then at least the pointer
> > won't be onto a stack. ;-)
> >
>
> Const would be nice, but otherwise making it static isn't a good idea.
Even aside from the whole "not having it store a pointer to the
stack", "static const" is likely to reduce overall memory consumption
/ number of instructions by a tiny bit because we don't need to copy
this structure onto the stack--we can just use it in place.
As written (or by just adding const but not static const): qmp_probe()
is 1840 bytes long.
...and has this snippet:
0xffffff80084a58d4 <+1152>: adrp x1, 0xffffff8008a5b000
<video_cc_sdm845_match_table+280>
0xffffff80084a58d8 <+1156>: add x1, x1, #0x600
0xffffff80084a58dc <+1160>: add x0, sp, #0x10
0xffffff80084a58e0 <+1164>: mov w2, #0x28 // #40
0xffffff80084a58e4 <+1168>: add x22, sp, #0x10
0xffffff80084a58e8 <+1172>: bl 0xffffff800896e800 <memcpy>
With this as static const: qmp_probe is 1820 bytes long.
...and has this snippet:
0xffffff80084a58dc <+1160>: adrp x8, 0xffffff8008a5b000
<video_cc_sdm845_match_table+280>
0xffffff80084a58e0 <+1164>: add x8, x8, #0x550
> The clk_init_data structure is all copied over, although we do leave a
> dangling pointer to it stored inside the clk_hw structure we don't use
> it after clk registration. Maybe we should overwrite the pointer with
> NULL once we're done in clk_register() so that clk providers can't use
> it. It might break somebody but would at least clarify this point.
Setting it to NULL seems like it would be a good idea. Now that I
think on it I believe I've actually tripped over this before trying to
read the '.name' from here... :-P
> > > +static void qmp_pd_remove(struct qmp *qmp)
> > > +{
> > > + struct genpd_onecell_data *data = &qmp->pd_data;
> > > + struct device *dev = qmp->dev;
> > > + int i;
> > > +
> > > + of_genpd_del_provider(dev->of_node);
> > > +
> > > + for (i = 0; i < data->num_domains; i++)
> > > + pm_genpd_remove(data->domains[i]);
> >
> > Still feels like the above loop would be better as:
> > for (i = data->num_domains - 1; i >= 0; i--)
> >
>
> Reason being to remove in reverse order? Otherwise this looks like an
> opinion.
1. Matches the order of the error handling case above (see unroll_genpds label)
2. In general you avoid more unexpected problems by un-initting in the
reverse order you initted.
-Doug
On Thu 23 May 09:38 PDT 2019, Doug Anderson wrote:
> Hi,
>
> On Tue, Apr 30, 2019 at 9:38 PM Bjorn Andersson
> <[email protected]> wrote:
> >
> > +static int qmp_qdss_clk_prepare(struct clk_hw *hw)
> > +{
> > + struct qmp *qmp = container_of(hw, struct qmp, qdss_clk);
> > + char buf[QMP_MSG_LEN] = "{class: clock, res: qdss, val: 1}";
>
> nit: "static const" the buf? No need to copy it to the stack each
> time. In qmp_qdss_clk_unprepare() too.
>
Thanks, that makes sense.
> ...your string is also now fixed at 34 bytes big (including the '\0').
> Do we still need to send exactly 96 bytes, or can we dumb this down to
> 36? We'll get a compile error if we overflow, right? If this truly
> needs to be exactly 96 bytes maybe qmp_send()'s error checks should
> check for things being exactly 96 bytes instead of checking for > and
> % 4.
>
I double checked with my contacts and the only requirement here is that
memory has to be word-accessed, so I'll figure out a sane way to write
this.
>
> > +static int qmp_qdss_clk_add(struct qmp *qmp)
> > +{
> > + struct clk_init_data qdss_init = {
> > + .ops = &qmp_qdss_clk_ops,
> > + .name = "qdss",
> > + };
>
> Can't qdss_init be "static const"? That had the advantage of not
> needing to construct it on the stack and also of it having a longer
> lifetime. It looks like clk_register() stores the "hw" pointer in its
> structure and the "hw" structure will have a pointer here. While I
> can believe that it never looks at it again, it's nice if that pointer
> doesn't point somewhere on an old stack.
>
The purpose here was for clk_hw_register() to consume it and never look
back, but I agree that it's a bit fragile. I'll review Stephen's
proposed patch.
> I suppose we could go the other way and try to mark more stuff in this
> module as __init and __initdata, but even then at least the pointer
> won't be onto a stack. ;-)
>
>
> > + int ret;
> > +
> > + qmp->qdss_clk.init = &qdss_init;
> > + ret = clk_hw_register(qmp->dev, &qmp->qdss_clk);
> > + if (ret < 0) {
> > + dev_err(qmp->dev, "failed to register qdss clock\n");
> > + return ret;
> > + }
> > +
> > + return of_clk_add_hw_provider(qmp->dev->of_node, of_clk_hw_simple_get,
> > + &qmp->qdss_clk);
>
> devm_clk_hw_register() and devm_of_clk_add_hw_provider()? If you're
> worried about ordering you could always throw in
> devm_add_action_or_reset() to handle the qmp_pd_remove(), qmp_close()
> and mbox_free_channel().
>
> ...with that you could fully get rid of qmp_remove() and also your
> setting of drvdata.
>
Yeah, I was worried about qmp_close() before unregistering the clock.
I'll take another look, will at least have to fix the error handling on
of_clk_add_hw_provider()
>
> > +static void qmp_pd_remove(struct qmp *qmp)
> > +{
> > + struct genpd_onecell_data *data = &qmp->pd_data;
> > + struct device *dev = qmp->dev;
> > + int i;
> > +
> > + of_genpd_del_provider(dev->of_node);
> > +
> > + for (i = 0; i < data->num_domains; i++)
> > + pm_genpd_remove(data->domains[i]);
>
> Still feels like the above loop would be better as:
> for (i = data->num_domains - 1; i >= 0; i--)
>
To me this carries a message that the removal order is significant,
which I'm unable to convince myself that it is.
>
> (BTW: any way you could add me to the CC list for future patches so I
> notice them earlier?)
>
Yes of course, thanks for your review.
Regards,
Bjorn
On Thu 23 May 11:05 PDT 2019, Stephen Boyd wrote:
> Quoting Doug Anderson (2019-05-23 09:38:13)
> > Hi,
> >
> > On Tue, Apr 30, 2019 at 9:38 PM Bjorn Andersson
> > <[email protected]> wrote:
> >
> > > +static int qmp_qdss_clk_add(struct qmp *qmp)
> > > +{
> > > + struct clk_init_data qdss_init = {
> > > + .ops = &qmp_qdss_clk_ops,
> > > + .name = "qdss",
> > > + };
> >
> > Can't qdss_init be "static const"? That had the advantage of not
> > needing to construct it on the stack and also of it having a longer
> > lifetime. It looks like clk_register() stores the "hw" pointer in its
> > structure and the "hw" structure will have a pointer here. While I
> > can believe that it never looks at it again, it's nice if that pointer
> > doesn't point somewhere on an old stack.
> >
> > I suppose we could go the other way and try to mark more stuff in this
> > module as __init and __initdata, but even then at least the pointer
> > won't be onto a stack. ;-)
> >
>
> Const would be nice, but otherwise making it static isn't a good idea.
> The clk_init_data structure is all copied over, although we do leave a
> dangling pointer to it stored inside the clk_hw structure we don't use
> it after clk registration. Maybe we should overwrite the pointer with
> NULL once we're done in clk_register() so that clk providers can't use
> it. It might break somebody but would at least clarify this point.
>
I had to read through the clock code to conclude that this was indeed
the design, so I'm happy with your patch of ensuring that this is
followed. Perhaps add a statement in the kerneldoc for struct clk_hw as
well to state that init won't be accessed past the return of
clk_register.
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index aa51756fd4d6..56997a974408 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3438,9 +3438,9 @@ static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
> return 0;
> }
>
> -static int clk_core_populate_parent_map(struct clk_core *core)
> +static int clk_core_populate_parent_map(struct clk_core *core,
> + const struct clk_init_data *init)
> {
> - const struct clk_init_data *init = core->hw->init;
> u8 num_parents = init->num_parents;
> const char * const *parent_names = init->parent_names;
> const struct clk_hw **parent_hws = init->parent_hws;
> @@ -3520,6 +3520,14 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
> {
> int ret;
> struct clk_core *core;
> + const struct clk_init_data *init = hw->init;
> +
> + /*
> + * The init data is not supposed to be used outside of registration path.
> + * Set it to NULL so that provider drivers can't use it either and so that
> + * we catch use of hw->init early on in the core.
> + */
> + hw->init = NULL;
>
> core = kzalloc(sizeof(*core), GFP_KERNEL);
> if (!core) {
> @@ -3527,17 +3535,17 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
> goto fail_out;
> }
>
> - core->name = kstrdup_const(hw->init->name, GFP_KERNEL);
> + core->name = kstrdup_const(init->name, GFP_KERNEL);
> if (!core->name) {
> ret = -ENOMEM;
> goto fail_name;
> }
>
> - if (WARN_ON(!hw->init->ops)) {
> + if (WARN_ON(!init->ops)) {
> ret = -EINVAL;
> goto fail_ops;
> }
> - core->ops = hw->init->ops;
> + core->ops = init->ops;
>
> if (dev && pm_runtime_enabled(dev))
> core->rpm_enabled = true;
> @@ -3546,13 +3554,13 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
> if (dev && dev->driver)
> core->owner = dev->driver->owner;
> core->hw = hw;
> - core->flags = hw->init->flags;
> - core->num_parents = hw->init->num_parents;
> + core->flags = init->flags;
> + core->num_parents = init->num_parents;
> core->min_rate = 0;
> core->max_rate = ULONG_MAX;
> hw->core = core;
>
> - ret = clk_core_populate_parent_map(core);
> + ret = clk_core_populate_parent_map(core, init);
> if (ret)
> goto fail_parents;
>
I've reviewed this and it looks good!
Regards,
Bjorn
>
> >
> >
> > > +static void qmp_pd_remove(struct qmp *qmp)
> > > +{
> > > + struct genpd_onecell_data *data = &qmp->pd_data;
> > > + struct device *dev = qmp->dev;
> > > + int i;
> > > +
> > > + of_genpd_del_provider(dev->of_node);
> > > +
> > > + for (i = 0; i < data->num_domains; i++)
> > > + pm_genpd_remove(data->domains[i]);
> >
> > Still feels like the above loop would be better as:
> > for (i = data->num_domains - 1; i >= 0; i--)
> >
>
> Reason being to remove in reverse order? Otherwise this looks like an
> opinion.
On 5/1/2019 10:07 AM, Bjorn Andersson wrote:
> The Always On Subsystem (AOSS) Qualcomm Messaging Protocol (QMP) driver
> is used to communicate with the AOSS for certain side-channel requests,
> that are not available 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.
>
> The driver exposes the QDSS clock as a clock and the low-power state
> associated with the remoteprocs in the system as a set of power-domains.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
> Changes since v6:
> - Squash the pd into the same driver as the communication, to simplify
> the interaction.
> - Representing the QDSS clocks as a clock/power domain turns out to
> cascade into a request to make all Coresight drivers have a secondary
> compatible to replace the required bus clock with a required power
> domain. So in v7 this is exposed as a clock instead.
> - Some error checking updates, as reported by Doug.
>
Thanks for the patch Bjorn.
Tested the QDSS functionality on SDM845 based Cheza board with this
change and it works just fine.
Tested-by: Sai Prakash Ranjan <[email protected]>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
On Sat 25 May 10:53 PDT 2019, Sai Prakash Ranjan wrote:
> On 5/1/2019 10:07 AM, Bjorn Andersson wrote:
> > The Always On Subsystem (AOSS) Qualcomm Messaging Protocol (QMP) driver
> > is used to communicate with the AOSS for certain side-channel requests,
> > that are not available 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.
> >
> > The driver exposes the QDSS clock as a clock and the low-power state
> > associated with the remoteprocs in the system as a set of power-domains.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> >
> > Changes since v6:
> > - Squash the pd into the same driver as the communication, to simplify
> > the interaction.
> > - Representing the QDSS clocks as a clock/power domain turns out to
> > cascade into a request to make all Coresight drivers have a secondary
> > compatible to replace the required bus clock with a required power
> > domain. So in v7 this is exposed as a clock instead.
> > - Some error checking updates, as reported by Doug.
> >
>
> Thanks for the patch Bjorn.
> Tested the QDSS functionality on SDM845 based Cheza board with this
> change and it works just fine.
>
> Tested-by: Sai Prakash Ranjan <[email protected]>
Thanks for the confirmation Sai!
Regards,
Bjorn
Quoting Bjorn Andersson (2019-05-23 12:09:25)
>
> I had to read through the clock code to conclude that this was indeed
> the design, so I'm happy with your patch of ensuring that this is
> followed. Perhaps add a statement in the kerneldoc for struct clk_hw as
> well to state that init won't be accessed past the return of
> clk_register.
>
I noticed that I had a branch for this that didn't go away this rebase.
I've thrown this on my todo list now.
I added the comment to the code, but I'm afraid I can't just merge this
patch to clk-next yet because various drivers access hw->init after
calling clk_register() and friends. I'll have to go through and find the
users by looking at git grep '->init' -- drivers/clk/ results. This is a
pain.